> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Thursday, December 23, 2010 2:32 AM > To: Xu, Andiry; Alan Stern > Cc: linux-usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxx; mjg@xxxxxxxxxx > Subject: Re: [PATCH 1/7] xHCI: synchronize irq in xhci_suspend() > > Alan, > > Please take a look at patch 1 if you have time. > > > Hi Andiry, > > It looks like these patches may have missed Greg's potentially-last pull > request for 2.6.37. I'll send the first (and possibly the 6th patch) to > Greg just in case Linus decides to do another rc. > OK. I've no concern whether they meet 2.6.37 or not. It's all up to you. > This patchset is a mixture of urgent bug fixes, new features, and > cleanup. In the future, can you break those up into separate patchsets? > OK. Sorry for the confusion, I do not mean to group them as a patchset. I assigned numbers to the patches just to make sure you do not miss any of them and some patches are based on others. > Here's my thoughts on getting these pushed: > > 1. synchronize irq - fixes some very scary warnings/errors on module > unload for Matthew and Rafael, so they should pushed sooner for 2.6.37. > > On the other hand, I'm a bit concerned about this bug fix because > Alan hasn't confirmed if all PCI host controllers set hcd->irq == > pci_dev->irq. I'm afraid a late fix will break some obscure PCI > host controller in a subtle way that will be hard to detect. > I'll modify the patch based on Alan's suggestions. > 2. AMD hudson quirk - too much code for this late in the release cycle. > It will have to wait until 2.6.38. I will try to get it reviewed > before the merge window opens, because I want to make sure the PCI > space map/unmap is correct. But it would have been better to see > this patch sooner, as it's going to be a stretch if it does get into > 2.6.38. > > 3. fix queue_trb in isoc transfers - does this fix a bug? I think isoc > transfers were working pretty soundly on 2.6.37. If it's a bug-fix by > inspection, then it could probably wait until 2.6.38. > It's a bug-fix by inspection. I modify it to apply with the queue_trb() usage. > 4. remove redundant parameter - definitely should wait for 2.6.38 > > 5. Fix cycle bit. I would be inclined to put this bug off until > 2.6.37, except that I'm pretty sure it will fix some issues I've > been seeing when the NEC host controller stops responding to the > UAS device queries. But it's not an urgent crashing-machines bug > fix at this point, so I think I'll put off pushing it until 2.6.38. > > 6/7 printing optimizations, should be left off until 2.6.38. > > I'll queue up patch 1 (along with the other resume fix I was holding > onto) and send them off to Greg, unless Alan has some last minute > objection to it. > > Sarah Sharp > > On Mon, Dec 20, 2010 at 06:29:23PM +0800, Andiry Xu wrote: > > >From 8668139f7857aad09c5308d2e5e47b3369c83b38 Mon Sep 17 00:00:00 2001 > > From: Andiry Xu <andiry.xu@xxxxxxx> > > Date: Mon, 20 Dec 2010 15:17:14 +0800 > > Subject: [PATCH 1/7] xHCI: synchronize irq in xhci_suspend() > > > > Synchronize the interrupts instead of free them in xhci_suspend(). This > will > > prevent a double free when the host is suspended and then the card > removed. > > > > Set hcd->irq to pdev->irq when using MSI and INTx, and add the equal > check in > > suspend_common(), so MSI-X synchronization will be handled by > xhci_suspend(), > > and MSI/INTx will be synchronized in suspend_common(). > > > > Reported-by: Matthew Garrett <mjg@xxxxxxxxxx> > > Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx> > > --- > > drivers/usb/core/hcd-pci.c | 6 +++++- > > drivers/usb/host/xhci.c | 40 +++++++++++-------------------------- > --- > > 2 files changed, 16 insertions(+), 30 deletions(-) > > > > diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c > > index 3799573..c793443 100644 > > --- a/drivers/usb/core/hcd-pci.c > > +++ b/drivers/usb/core/hcd-pci.c > > @@ -406,7 +406,11 @@ static int suspend_common(struct device *dev, bool > do_wakeup) > > return retval; > > } > > > > - synchronize_irq(pci_dev->irq); > > + /* xHCI uses MSI-X by default. Suppose other HCDs will have hcd->irq > > + * equal to pci_dev->irq. > > + */ > > + if (hcd->irq == pci_dev->irq) > > + synchronize_irq(pci_dev->irq); > > > > /* Downstream ports from this root hub should already be quiesced, > so > > * there will be no DMA activity. Now we can shut down the upstream > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > index 45e4a31..2947d94 100644 > > --- a/drivers/usb/host/xhci.c > > +++ b/drivers/usb/host/xhci.c > > @@ -202,6 +202,7 @@ static void xhci_free_irq(struct xhci_hcd *xhci) > > static int xhci_setup_msi(struct xhci_hcd *xhci) > > { > > int ret; > > + struct usb_hcd *hcd = xhci_to_hcd(xhci); > > struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)- > >self.controller); > > > > ret = pci_enable_msi(pdev); > > @@ -217,6 +218,7 @@ static int xhci_setup_msi(struct xhci_hcd *xhci) > > pci_disable_msi(pdev); > > } > > > > + hcd->irq = pdev->irq; > > return ret; > > } > > > > @@ -647,6 +649,7 @@ int xhci_suspend(struct xhci_hcd *xhci) > > int rc = 0; > > struct usb_hcd *hcd = xhci_to_hcd(xhci); > > u32 command; > > + int i; > > > > spin_lock_irq(&xhci->lock); > > clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > > @@ -677,10 +680,15 @@ int xhci_suspend(struct xhci_hcd *xhci) > > spin_unlock_irq(&xhci->lock); > > return -ETIMEDOUT; > > } > > - /* step 5: remove core well power */ > > - xhci_cleanup_msix(xhci); > > spin_unlock_irq(&xhci->lock); > > > > + /* step 5: remove core well power */ > > + /* synchronize irq when using MSI-X */ > > + if (xhci->msix_entries) { > > + for (i = 0; i < xhci->msix_count; i++) > > + synchronize_irq(xhci->msix_entries[i].vector); > > + } > > + > > return rc; > > } > > > > @@ -694,7 +702,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,9 +736,8 @@ 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); > > spin_unlock_irq(&xhci->lock); > > + xhci_cleanup_msix(xhci); > > > > #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING > > /* Tell the event ring poll function not to reschedule */ > > @@ -765,30 +771,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.1 > > > > > > > > -- > > 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
