> 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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
