[lkml]   [2009]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] USB: Fix suspend-resume of PCI USB controllers
    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 <>
    > >
    > > 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 <>
    > > Tested-by: Andrey Borzenkov <>
    > > Tested-by: "Jeff Chua" <>
    > > Cc: Alan Stern <>
    > > Cc: Andrew Morton <>
    > > Cc: Christian Borntraeger <>
    > > Cc: "Zdenek Kabelac" <>
    > > Cc: Ingo Molnar <>
    > 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

     \ /
      Last update: 2009-01-20 16:59    [W:0.024 / U:0.400 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site