[PATCH] USB: Fix suspend-resume of PCI USB controllers

[PATCH] USB: Fix suspend-resume of PCI USB controllers

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

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@sisk.pl>
Tested-by: Andrey Borzenkov <arvidjaar@mail.ru>
Tested-by: "Jeff Chua" <jeff.chua.linux@gmail.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: "Zdenek Kabelac" <zdenek.kabelac@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
---
 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 */

[PATCH] USB: Fix suspend-resume of PCI USB controllers

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

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@sisk.pl>
Tested-by: Andrey Borzenkov <arvidjaar@mail.ru>
Tested-by: "Jeff Chua" <jeff.chua.linux@gmail.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: "Zdenek Kabelac" <zdenek.kabelac@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
---
 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 */

Re: [PATCH] USB: Fix suspend-resume of PCI USB controllers

[Greg KH]

On Tue, Jan 20, 2009 at 01:26:56AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> 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@sisk.pl>
> Tested-by: Andrey Borzenkov <arvidjaar@mail.ru>
> Tested-by: "Jeff Chua" <jeff.chua.linux@gmail.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: "Zdenek Kabelac" <zdenek.kabelac@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>


Alan, does this look ok for you?

thanks,

greg k-h

Re: [PATCH] USB: Fix suspend-resume of PCI USB controllers

[Greg KH]

On Tue, Jan 20, 2009 at 01:26:56AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> 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@sisk.pl>
> Tested-by: Andrey Borzenkov <arvidjaar@mail.ru>
> Tested-by: "Jeff Chua" <jeff.chua.linux@gmail.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: "Zdenek Kabelac" <zdenek.kabelac@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>


Alan, does this look ok for you?

thanks,

greg k-h

Re: [PATCH] USB: Fix suspend-resume of PCI USB controllers

[Alan Stern]

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@sisk.pl>
> > 
> > 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@sisk.pl>
> > Tested-by: Andrey Borzenkov <arvidjaar@mail.ru>
> > Tested-by: "Jeff Chua" <jeff.chua.linux@gmail.com>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > Cc: "Zdenek Kabelac" <zdenek.kabelac@gmail.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> 
> 
> 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

Re: [PATCH] USB: Fix suspend-resume of PCI USB controllers

[Alan Stern]

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@sisk.pl>
> > 
> > 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@sisk.pl>
> > Tested-by: Andrey Borzenkov <arvidjaar@mail.ru>
> > Tested-by: "Jeff Chua" <jeff.chua.linux@gmail.com>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > Cc: "Zdenek Kabelac" <zdenek.kabelac@gmail.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> 
> 
> 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

Re: [PATCH] USB: Fix suspend-resume of PCI USB controllers

[Len Brown]

> > 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 <len.brown@intel.com>

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

Re: [PATCH] USB: Fix suspend-resume of PCI USB controllers

[Len Brown]

> > 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 <len.brown@intel.com>

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

Re: [PATCH] USB: Fix suspend-resume of PCI USB controllers

[Rafael J. Wysocki]

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 <len.brown@intel.com>
> 
> 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

Re: [PATCH] USB: Fix suspend-resume of PCI USB controllers

[Rafael J. Wysocki]

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 <len.brown@intel.com>
> 
> 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