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

[Alex,Shi]

> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/