Alan, Can the USB core rely on hcd->irq being equal to pci_dev->irq in suspend_common() for PCI host controllers that use legacy interrupts? > > Yes, that's possible. Here are the modifications: > > > > 1. Set hcd->irq = pdev->irq in xhci driver when using MSI; > > 2. Synchronize irq in xhci_suspend() only when using MSI-X; > > 3. Synchronize irq only if pci_dev->irq is equal to hcd->irq in > > suspend_common(). Andiry, if Alan says we can't rely on that fact, you could change the suspend_common() code to skip the irq sync if pci_dev->irq is equal to -1. You would have to set pci_dev->irq to -1 when you enable MSI-X in the xHCI driver. Sarah Sharp On Wed, Dec 15, 2010 at 07:57:46AM -0800, Sarah Sharp wrote: > On Wed, Dec 15, 2010 at 05:11:50PM +0800, Xu, Andiry wrote: > > > -----Original Message----- > > > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > > > Sent: Wednesday, December 15, 2010 8:24 AM > > > To: Xu, Andiry > > > Cc: linux-usb@xxxxxxxxxxxxxxx; Matthew Garrett; Alan Stern > > > Subject: Re: [RFC] xHCI: synchronize interrupts in xhci_suspend() > > > > > > On Mon, Dec 06, 2010 at 06:22:19PM +0800, Andiry Xu wrote: > > > > Matthew, > > > > > > > > Can you help to test if this patch fix your xhci suspend and remove > > > issue? Thanks. > > > > > > Andiry, > > > > > > This code doesn't change the code in the USB core that synchronizes > > the > > > legacy PCI interrupts. So if you have to fall back to MSI, then > > > pci_dev->irq is equal to your MSI irq, and suspend_common() will > > > synchronize the irq a second time after xhci_suspend() is called. > > Maybe > > > suspend_common() should only call synchronize_irq() if pci_dev->irq is > > > the same as hcd->irq? > > > > > > > Well, I think synchronize_irq()is fine to be called twice. But perhaps > > you are right, it's pointless to call it for a second time. > > > > Does the hcd->irq equal check in suspend_common() affect other HCDs? I > > think it's OK but I'm not sure. Is there any reason other HCDs do not > > use MSI-X and MSI? > > I'm not sure whether it will effect other PCI host controller drivers. > Alan might know. The only reason that other HCDs don't implement MSI-X > and MSI is because no one saw a need for it. I think someone mentioned > that a lot of EHCI host controllers were buggy when it came to MSI. > Clemens Ladisch had some patches to enable MSI/MSI-X for select hosts. > He posted the patches back in June, but I haven't had the cycles to take > a look at them. > > > > Also, what happens if you free the legacy IRQ, set up MSI-X, and then > > > the USB core tries to synchronize pci_dev->irq? Is pci_dev->irq still > > > set to the legacy IRQ that the driver freed? If so, we're possibly > > > synchronizing someone else's IRQ. > > > > > > > Yes, that's possible. Here are the modifications: > > > > 1. Set hcd->irq = pdev->irq in xhci driver when using MSI; > > 2. Synchronize irq in xhci_suspend() only when using MSI-X; > > 3. Synchronize irq only if pci_dev->irq is equal to hcd->irq in > > suspend_common(). > > > > And then MSI-X synchronization will be handled by xhci_suspend(), and > > MSI and INTx will be synchronized in suspend_common(). Is this solution > > feasible? > > I think that looks correct, as long as other PCI host controllers aren't > changing hcd->irq or pdev->irq. > > Sarah Sharp > > > > > From: Andiry Xu <andiry.xu@xxxxxxx> > > > > Date: Mon, 6 Dec 2010 16:58:16 +0800 > > > > Subject: [PATCH] xHCI: synchronize interrupts in xhci_suspend() > > > > > > > > Matthew Garrett reports a interrupt double free in xHCI code when > > the > > > > host is suspended and then the card removed. > > > > > > > > Synchronize the interrupts instead of free them in xhci_suspned(). > > > > > > > > Reported-by: Matthew Garrett <mjg@xxxxxxxxxx> > > > > Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx> > > > > --- > > > > drivers/usb/host/xhci.c | 37 > > +++++++++---------------------------- > > > > 1 files changed, 9 insertions(+), 28 deletions(-) > > > > > > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > > > index 45e4a31..41a9225 100644 > > > > --- a/drivers/usb/host/xhci.c > > > > +++ b/drivers/usb/host/xhci.c > > > > @@ -646,7 +646,9 @@ int xhci_suspend(struct xhci_hcd *xhci) > > > > { > > > > int rc = 0; > > > > struct usb_hcd *hcd = xhci_to_hcd(xhci); > > > > + struct pci_dev *pdev = > > to_pci_dev(hcd->self.controller); > > > > u32 command; > > > > + int i; > > > > > > > > spin_lock_irq(&xhci->lock); > > > > clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > > > > @@ -678,9 +680,14 @@ int xhci_suspend(struct xhci_hcd *xhci) > > > > return -ETIMEDOUT; > > > > } > > > > /* step 5: remove core well power */ > > > > - xhci_cleanup_msix(xhci); > > > > spin_unlock_irq(&xhci->lock); > > > > > > > > + if (xhci->msix_entries) { > > > > + for (i = 0; i < xhci->msix_count; i++) > > > > + synchronize_irq(xhci->msix_entries[i].vector); > > > > + } else if (pdev->irq >= 0) > > > > + synchronize_irq(pdev->irq); > > > > + > > > > return rc; > > > > } > > > > > > > > @@ -694,7 +701,6 @@ int xhci_resume(struct xhci_hcd *xhci, bool > > > hibernated) > > > > { > > > > u32 command, temp = 0; > > > > struct usb_hcd *hcd = xhci_to_hcd(xhci); > > > > - struct pci_dev *pdev = > > to_pci_dev(hcd->self.controller); > > > > int old_state, retval; > > > > > > > > old_state = hcd->state; > > > > @@ -729,8 +735,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool > > > hibernated) > > > > xhci_dbg(xhci, "Stop HCD\n"); > > > > xhci_halt(xhci); > > > > xhci_reset(xhci); > > > > - if (hibernated) > > > > - xhci_cleanup_msix(xhci); > > > > + xhci_cleanup_msix(xhci); > > > > spin_unlock_irq(&xhci->lock); > > > > > > > > #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING > > > > @@ -765,30 +770,6 @@ int xhci_resume(struct xhci_hcd *xhci, bool > > > hibernated) > > > > return retval; > > > > } > > > > > > > > - spin_unlock_irq(&xhci->lock); > > > > - /* Re-setup MSI-X */ > > > > - if (hcd->irq) > > > > - free_irq(hcd->irq, hcd); > > > > - hcd->irq = -1; > > > > - > > > > - retval = xhci_setup_msix(xhci); > > > > - if (retval) > > > > - /* fall back to msi*/ > > > > - retval = xhci_setup_msi(xhci); > > > > - > > > > - if (retval) { > > > > - /* fall back to legacy interrupt*/ > > > > - retval = request_irq(pdev->irq, &usb_hcd_irq, > > IRQF_SHARED, > > > > - hcd->irq_descr, hcd); > > > > - if (retval) { > > > > - xhci_err(xhci, "request interrupt %d failed\n", > > > > - pdev->irq); > > > > - return retval; > > > > - } > > > > - hcd->irq = pdev->irq; > > > > - } > > > > - > > > > - spin_lock_irq(&xhci->lock); > > > > /* step 4: set Run/Stop bit */ > > > > command = xhci_readl(xhci, &xhci->op_regs->command); > > > > command |= CMD_RUN; > > > > -- > > > > 1.7.0.4 > > > > > > > > > > > > > > > > > -- > 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 -- 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
