From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
Tested-by: Andrey Borzenkov <[email protected]>
Tested-by: "Jeff Chua" <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Christian Borntraeger <[email protected]>
Cc: "Zdenek Kabelac" <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
drivers/usb/core/hcd-pci.c | 116 ++++++++++----------------------------------
drivers/usb/core/hcd.h | 1
drivers/usb/host/ehci-pci.c | 1
drivers/usb/host/ohci-pci.c | 1
drivers/usb/host/uhci-hcd.c | 1
5 files changed, 27 insertions(+), 93 deletions(-)
Index: linux-2.6/drivers/usb/core/hcd-pci.c
===================================================================
--- linux-2.6.orig/drivers/usb/core/hcd-pci.c
+++ linux-2.6/drivers/usb/core/hcd-pci.c
@@ -200,6 +200,7 @@ int usb_hcd_pci_suspend(struct pci_dev *
struct usb_hcd *hcd = pci_get_drvdata(dev);
int retval = 0;
int wake, w;
+ int has_pci_pm;
/* Root hub suspend should have stopped all downstream traffic,
* and all bus master traffic. And done so for both the interface
@@ -229,6 +230,15 @@ int usb_hcd_pci_suspend(struct pci_dev *
synchronize_irq(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
+ * link (except maybe for PME# resume signaling) and enter some PCI
+ * low power state, if the hardware allows.
+ */
+ pci_disable_device(dev);
+
+ pci_save_state(dev);
+
/* Don't fail on error to enable wakeup. We rely on pci code
* to reject requests the hardware can't implement, rather
* than coding the same thing.
@@ -240,35 +250,6 @@ int usb_hcd_pci_suspend(struct pci_dev *
wake = w;
dev_dbg(&dev->dev, "wakeup: %d\n", wake);
- /* Downstream ports from this root hub should already be quiesced, so
- * there will be no DMA activity. Now we can shut down the upstream
- * link (except maybe for PME# resume signaling) and enter some PCI
- * low power state, if the hardware allows.
- */
- pci_disable_device(dev);
- done:
- return retval;
-}
-EXPORT_SYMBOL_GPL(usb_hcd_pci_suspend);
-
-/**
- * usb_hcd_pci_suspend_late - suspend a PCI-based HCD after IRQs are disabled
- * @dev: USB Host Controller being suspended
- * @message: Power Management message describing this state transition
- *
- * Store this function in the HCD's struct pci_driver as .suspend_late.
- */
-int usb_hcd_pci_suspend_late(struct pci_dev *dev, pm_message_t message)
-{
- int retval = 0;
- int has_pci_pm;
-
- /* We might already be suspended (runtime PM -- not yet written) */
- if (dev->current_state != PCI_D0)
- goto done;
-
- pci_save_state(dev);
-
/* Don't change state if we don't need to */
if (message.event == PM_EVENT_FREEZE ||
message.event == PM_EVENT_PRETHAW) {
@@ -314,7 +295,7 @@ int usb_hcd_pci_suspend_late(struct pci_
done:
return retval;
}
-EXPORT_SYMBOL_GPL(usb_hcd_pci_suspend_late);
+EXPORT_SYMBOL_GPL(usb_hcd_pci_suspend);
/**
* usb_hcd_pci_resume_early - resume a PCI-based HCD before IRQs are enabled
@@ -324,65 +305,8 @@ EXPORT_SYMBOL_GPL(usb_hcd_pci_suspend_la
*/
int usb_hcd_pci_resume_early(struct pci_dev *dev)
{
- int retval = 0;
- pci_power_t state = dev->current_state;
-
-#ifdef CONFIG_PPC_PMAC
- /* Reenable ASIC clocks for USB */
- if (machine_is(powermac)) {
- struct device_node *of_node;
-
- of_node = pci_device_to_OF_node(dev);
- if (of_node)
- pmac_call_feature(PMAC_FTR_USB_ENABLE,
- of_node, 0, 1);
- }
-#endif
-
- /* NOTE: chip docs cover clean "real suspend" cases (what Linux
- * calls "standby", "suspend to RAM", and so on). There are also
- * dirty cases when swsusp fakes a suspend in "shutdown" mode.
- */
- if (state != PCI_D0) {
-#ifdef DEBUG
- int pci_pm;
- u16 pmcr;
-
- pci_pm = pci_find_capability(dev, PCI_CAP_ID_PM);
- pci_read_config_word(dev, pci_pm + PCI_PM_CTRL, &pmcr);
- pmcr &= PCI_PM_CTRL_STATE_MASK;
- if (pmcr) {
- /* Clean case: power to USB and to HC registers was
- * maintained; remote wakeup is easy.
- */
- dev_dbg(&dev->dev, "resume from PCI D%d\n", pmcr);
- } else {
- /* Clean: HC lost Vcc power, D0 uninitialized
- * + Vaux may have preserved port and transceiver
- * state ... for remote wakeup from D3cold
- * + or not; HCD must reinit + re-enumerate
- *
- * Dirty: D0 semi-initialized cases with swsusp
- * + after BIOS init
- * + after Linux init (HCD statically linked)
- */
- dev_dbg(&dev->dev, "resume from previous PCI D%d\n",
- state);
- }
-#endif
-
- retval = pci_set_power_state(dev, PCI_D0);
- } else {
- /* Same basic cases: clean (powered/not), dirty */
- dev_dbg(&dev->dev, "PCI legacy resume\n");
- }
-
- if (retval < 0)
- dev_err(&dev->dev, "can't resume: %d\n", retval);
- else
- pci_restore_state(dev);
-
- return retval;
+ pci_restore_state(dev);
+ return 0;
}
EXPORT_SYMBOL_GPL(usb_hcd_pci_resume_early);
@@ -397,6 +321,18 @@ int usb_hcd_pci_resume(struct pci_dev *d
struct usb_hcd *hcd;
int retval;
+#ifdef CONFIG_PPC_PMAC
+ /* Reenable ASIC clocks for USB */
+ if (machine_is(powermac)) {
+ struct device_node *of_node;
+
+ of_node = pci_device_to_OF_node(dev);
+ if (of_node)
+ pmac_call_feature(PMAC_FTR_USB_ENABLE,
+ of_node, 0, 1);
+ }
+#endif
+
hcd = pci_get_drvdata(dev);
if (hcd->state != HC_STATE_SUSPENDED) {
dev_dbg(hcd->self.controller,
@@ -404,6 +340,8 @@ int usb_hcd_pci_resume(struct pci_dev *d
return 0;
}
+ pci_enable_wake(dev, PCI_D0, false);
+
retval = pci_enable_device(dev);
if (retval < 0) {
dev_err(&dev->dev, "can't re-enable after resume, %d!\n",
Index: linux-2.6/drivers/usb/core/hcd.h
===================================================================
--- linux-2.6.orig/drivers/usb/core/hcd.h
+++ linux-2.6/drivers/usb/core/hcd.h
@@ -257,7 +257,6 @@ extern void usb_hcd_pci_remove(struct pc
#ifdef CONFIG_PM
extern int usb_hcd_pci_suspend(struct pci_dev *dev, pm_message_t msg);
-extern int usb_hcd_pci_suspend_late(struct pci_dev *dev, pm_message_t msg);
extern int usb_hcd_pci_resume_early(struct pci_dev *dev);
extern int usb_hcd_pci_resume(struct pci_dev *dev);
#endif /* CONFIG_PM */
Index: linux-2.6/drivers/usb/host/ehci-pci.c
===================================================================
--- linux-2.6.orig/drivers/usb/host/ehci-pci.c
+++ linux-2.6/drivers/usb/host/ehci-pci.c
@@ -432,7 +432,6 @@ static struct pci_driver ehci_pci_driver
#ifdef CONFIG_PM
.suspend = usb_hcd_pci_suspend,
- .suspend_late = usb_hcd_pci_suspend_late,
.resume_early = usb_hcd_pci_resume_early,
.resume = usb_hcd_pci_resume,
#endif
Index: linux-2.6/drivers/usb/host/ohci-pci.c
===================================================================
--- linux-2.6.orig/drivers/usb/host/ohci-pci.c
+++ linux-2.6/drivers/usb/host/ohci-pci.c
@@ -487,7 +487,6 @@ static struct pci_driver ohci_pci_driver
#ifdef CONFIG_PM
.suspend = usb_hcd_pci_suspend,
- .suspend_late = usb_hcd_pci_suspend_late,
.resume_early = usb_hcd_pci_resume_early,
.resume = usb_hcd_pci_resume,
#endif
Index: linux-2.6/drivers/usb/host/uhci-hcd.c
===================================================================
--- linux-2.6.orig/drivers/usb/host/uhci-hcd.c
+++ linux-2.6/drivers/usb/host/uhci-hcd.c
@@ -942,7 +942,6 @@ static struct pci_driver uhci_pci_driver
#ifdef CONFIG_PM
.suspend = usb_hcd_pci_suspend,
- .suspend_late = usb_hcd_pci_suspend_late,
.resume_early = usb_hcd_pci_resume_early,
.resume = usb_hcd_pci_resume,
#endif /* PM */
On Tue, Jan 20, 2009 at 01:26:56AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> Tested-by: Andrey Borzenkov <[email protected]>
> Tested-by: "Jeff Chua" <[email protected]>
> Cc: Alan Stern <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Christian Borntraeger <[email protected]>
> Cc: "Zdenek Kabelac" <[email protected]>
> Cc: Ingo Molnar <[email protected]>
Alan, does this look ok for you?
thanks,
greg k-h
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Tested-by: Andrey Borzenkov <[email protected]>
> > Tested-by: "Jeff Chua" <[email protected]>
> > Cc: Alan Stern <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Christian Borntraeger <[email protected]>
> > Cc: "Zdenek Kabelac" <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
>
>
> 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
> > 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.
Rafael's patch is required for upstream to resume on my T61.
Tested-by: Len Brown <[email protected]>
Perhaps Greg can apply Rafael's patch now to fix upstream ASAP,
while Alan prepares a 2nd patch to address his observations above?
thanks,
Len Brown, Intel Open Source Technology Center
On Tuesday 20 January 2009, Len Brown wrote:
>
> > > 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.
>
> Rafael's patch is required for upstream to resume on my T61.
>
> Tested-by: Len Brown <[email protected]>
>
> Perhaps Greg can apply Rafael's patch now to fix upstream ASAP,
> while Alan prepares a 2nd patch to address his observations above?
I'd very much appreciate doing that. My intention was only to fix the urgent
issue at hand.
Thanks,
Rafael