lkml.org 
[lkml]   [2012]   [Feb]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 2/3] usb: enable pci MSI/MSIX in usb core
    From
    Date

    > 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?



    \
     
     \ /
      Last update: 2012-02-23 09:43    [W:0.030 / U:30.120 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site