On Mon, 19 Jan 2009, Greg KH wrote: > On Tue, Jan 20, 2009 at 01:26:56AM +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rjw@xxxxxxx> > > > > Commit a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 > > (USB: fix up suspend and resume for PCI host controllers) attempted > > to fix the suspend-resume of PCI USB controllers, but unfortunately > > it did that incorrectly and interrupts are left enabled by the USB > > controllers' ->suspend_late() callback as a result. This leads to > > serious problems during suspend which are very difficult to debug. > > > > Fix the issue by removing the ->suspend_late() callback of PCI > > USB controllers and moving the code from there to the ->suspend() > > callback executed with interrupts enabled. Additionally, make > > the ->resume() callback of PCI USB controllers execute > > pci_enable_wake(dev, PCI_D0, false) to disable wake-up from the > > full power state (PCI_D0). > > > > Signed-off-by: Rafael J. Wysocki <rjw@xxxxxxx> > > Tested-by: Andrey Borzenkov <arvidjaar@xxxxxxx> > > Tested-by: "Jeff Chua" <jeff.chua.linux@xxxxxxxxx> > > Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> > > Cc: Christian Borntraeger <borntraeger@xxxxxxxxxx> > > Cc: "Zdenek Kabelac" <zdenek.kabelac@xxxxxxxxx> > > Cc: Ingo Molnar <mingo@xxxxxxx> > > > Alan, does this look ok for you? There are a few things I don't like about it: In usb_hcd_pci_suspend, the failure path for pci_set_power_state doesn't undo all the changes that have already been made. In fact, the easiest way to do the rest of the recovery probably is to call usb_hcd_pci_resume directly. In the !has_pci_pm path we don't call pci_set_power_state. This means controllers with legacy power management won't get suspended. Maybe pci_set_power_state should be called always, and has_pci_pm be used only for interpreting the return code and printing debug messages. In usb_hcd_pci_resume, the pci_wake_from_d3 call should be moved up right next to the pci_enable_wake call. It makes no sense for them to be separated by pci_enable_device and pci_set_master. After all, even if you can't re-enable the device you probably don't want it to continue being a wakeup source. It's a little annoying that several debug messages in usb_hcd_pci_resume have been removed. Can't we display the power state upon entry, before trying to change it? It stands out that the resume method contains no call to match pci_set_power_state() in the suspend method. There should at least be a comment about it. Some of these problems predate Rafael's patch. Given that it has already helped a number of people, we might want to merge the patch and then make additional changes to address these issues. Alan Stern -- 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
