linux-kernel - Re: [PATCH 2/3] usb: enable pci MSI/MSIX in usb core

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1329986384.21053.165.camel@debian>
Date:	Thu, 23 Feb 2012 16:39:44 +0800
From:	"Alex,Shi" <alex.shi@...el.com>
To:	Sarah Sharp <sarah.a.sharp@...ux.intel.com>
Cc:	Felipe Balbi <balbi@...com>, gregkh@...uxfoundation.org,
	stern@...land.harvard.edu, linux-usb@...r.kernel.org,
	andiry.xu@....com, clemens@...isch.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] usb: enable pci MSI/MSIX in usb core


> This PCI device and vendor ID is now used in two drivers (xhci and USB
> core).  Please create a separate patch to add this ID to the pci_ids.h
> file, and remove the reference here and in the xHCI driver.

Yes, should be. 
> Why don't you rename hcd_no_msi() to hcd_supports_msi() and remove the
> negation of the return value?

OK. 
> 
> > 
> > +		/* register hc_driver's msix_irq handler */
> > +		for (i = 0; i < hcd->msix_count; i++) {
> > +			retval = request_irq(hcd->msix_entries[i].vector,
> > +					(irq_handler_t)hcd->driver->msix_irq,
> > +					0, hcd->driver->description, hcd);
> 
> I really think you need to allow the host controller driver to set
> different pointers for the msix data pointer.  It's something that we
> need to figure out, so that we can have the infrastructure in place for
> multiple event rings.
> 
> I'm not sure whether the new get MSIX count needs to allow the xHCI
> driver to return an array of pointers, or if the driver can modify the
> irq pointer later?  I don't think you can modify the irq data pointer
> after it's been requested (that would lead to all kinds of race
> conditions, I think).
> 
> It's probably better to allow the xHCI driver to pass this function the
> pointers it needs for each MSI-X vector.  You'll always call
> usb_hcd_request_msi_msix_irqs() after you call xhci_init(), correct?  At
> that point, we should have allocated the multiple event rings, so it
> should be easy to pass the pointers to this function.

What do you mean: there is a relation between event rings
msix_entries.vectors.  and we need to presents this relationships in the
msix interrupt handler? 

So does the following mode you like?

request_irq(hcd->msix_entries[i].vector, msix_irq_handler, 0, "",
hcd->ring_handler[i]);

Or another way to do it if we know which ring will handle the irq, like:

irqreturn_t xhci_msi_irq(int irq, struct usb_hcd *hcd)

	switch (irq2ring(irq))
	case ring0: driver_handle_ring(ring0);
	case ring1: driver_handle_ring(ring1);

In fact, since there is no actual usage of multiple rings now, I have no
much idea of the relationships. 

BTW, if it is possible do this change to another patch? 
> 
> > @@ -888,7 +696,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
> >  		if (retval)
> >  			return retval;
> >  		xhci_dbg(xhci, "Start the primary HCD\n");
> > -		retval = xhci_run(hcd->primary_hcd);
> > +		if (dev_is_pci(hcd->self.controller))
> > +			retval = usb_hcd_register_msi_msix_irqs(
> > +						hcd->primary_hcd);
> 
> Why not change this function to take a count of msix vectors and
> pointers for data?  Then you don't need the new usb_hcd driver method
> for getting the msix count.
> 

Uh, the key is msix vector numbers maybe changed after be freed in
suspend and re-get here. Are there examples to keep the vector number in
suspending?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/