> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Friday, December 03, 2010 2:11 AM > To: Alan Stern > Cc: linux-usb@xxxxxxxxxxxxxxx; Matthew Garrett; Xu, Andiry > Subject: Re: USB core suspend issue > > On Thu, Dec 02, 2010 at 10:37:54AM -0500, Alan Stern wrote: > > On Wed, 1 Dec 2010, Sarah Sharp wrote: > > > > > Hi Alan, > > > > > > Matthew Garret points out that the current xHCI code doesn't work so > > > well when the host is suspended and then the card is removed. The > > > xhci_suspend() method frees the MSI-X interrupts because the USB core > > > calls synchronize_irq() with the legacy interrupt in suspend_common() > in > > > hcd-pci.c. The hotplug layer will attempt to deallocate the > interrupts > > > the card had before suspend, which results in a double free. > > > > > > Is there a reason the USB PCI code calls synchronize_irq()? > > > > See commit 8de98402652c01839ae321be (USB: Fix USB suspend/resume > > crasher (#2)) by Ben Herrenschmidt -- 5 years ago! He's the best > > person to ask. My best guess is that this was done to avoid a race in > > which an IRQ is received before the suspend starts but is not delivered > > until after the controller is put into D3, at which point the ISR would > > get a bus exception when trying to read the controller's status > > register. > > > > Let me turn your question around: Is there a reason xhci_suspend() > > frees the MSI-X interrupts instead of just synchronizing them? > > I don't know, that was Andiry's code. I think he was just trying to > work around the USB core synchronizing the legacy PCI IRQ. Perhaps the > xHCI driver could synchronize the MSI-X interrupts and the USB core > could skip the legacy PCI IRQ synchronization if hcd->irq == -1? > > Andiry, was there a reason you were freeing the MSI-X vectors? Is there > some bug you were working around? > > Sarah Sharp Oh, thanks for reminding me of this...I think it's a legacy issue of my code. When I was testing my original pm patches several months ago, the host controller will always restore fail during system resume(it's a BIOS issue of my platform). And in my original design, when the host restore fails, it will call xhci_halt() and xhci_reset() to reset the host controller. However, with MSI-X enabled, this is not enough, the MSI-X vectors should be freed and then re-setup, otherwise the devices will fail recognition after resume. So I added the MSI-X free and setup code. It appears the xhci_cleanup_msix() is added to the wrong place. It should appear in xhci_resume() just before xhci_setup_msix(), not in xhci_suspend(). And in current driver, when host restore fails, the whole host controller will be freed and re-initialized, so MSI-X will be take cared too. I've done some quick tests, remove the xhci_cleanup_msix() in xhci_suspend() and corresponding msix setup code in xhci_resume(). S3 works fine in both cases, whether the host controller restore succeed or not. After I've done more stress tests, I'll submit a patch to fix this. Thanks, Andiry -- 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
