Regression 2.6.28-git: suspend/resume failure. git bisected to usb suspend/resume patch

Regression 2.6.28-git: suspend/resume failure. git bisected to usb suspend/resume patch

[Christian Borntraeger]

Hello Alan,

git bisect found this patch 

a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 is first bad commit
commit a0d4922da2e4ccb0973095d8d29f36f6b1b5f703
Author: Alan Stern <stern@rowland.harvard.edu>
Date:   Wed Dec 17 15:06:03 2008 -0500

    USB: fix up suspend and resume for PCI host controllers

    This patch (as1192) rearranges the USB PCI host controller suspend and
    resume and resume routines:


To cause my suspend to ram problem with my Thinkpad T61p.
I can confirm that 
a81a81a25d3ecdab777abca87c5ddf484056103d works
and a0d4922da2e4ccb0973095d8d29f36f6b1b5f703  does not work.

Thing is, 29-rc1 with a0d4922da2e4ccb0973095d8d29f36f6b1b5f703  reverted also 
fails with suspend/resume. Seems that there was another problem introduced 
after that patch.

Anyway, back to usb: I also tested plain 2.6.28 + this patch:
plain 2.6.28: works fine
2.6.28 + this patch: does not work.

Christian

Re: Regression 2.6.28-git: suspend/resume failure. git bisected to usb suspend/resume patch

[Rafael J. Wysocki]

On Sunday 11 January 2009, Christian Borntraeger wrote:
> Hello Alan,
> 
> git bisect found this patch 
> 
> a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 is first bad commit
> commit a0d4922da2e4ccb0973095d8d29f36f6b1b5f703
> Author: Alan Stern <stern@rowland.harvard.edu>
> Date:   Wed Dec 17 15:06:03 2008 -0500
> 
>     USB: fix up suspend and resume for PCI host controllers
> 
>     This patch (as1192) rearranges the USB PCI host controller suspend and
>     resume and resume routines:
> 
> 
> To cause my suspend to ram problem with my Thinkpad T61p.
> I can confirm that 
> a81a81a25d3ecdab777abca87c5ddf484056103d works
> and a0d4922da2e4ccb0973095d8d29f36f6b1b5f703  does not work.
> 
> Thing is, 29-rc1 with a0d4922da2e4ccb0973095d8d29f36f6b1b5f703  reverted also 
> fails with suspend/resume. Seems that there was another problem introduced 
> after that patch.

Please try if suspend-resume works for you if you make commit
fa58d305d9925b01830e535896a7227a868a9e15
(PCI PM: Add suspend counterpart of pci_reenable_device) the head of your
tree ('git checkout -b test-branch fa58d305d9925b01830e535896a7227a868a9e15').

If it does, please check the same for commit
571ff7584bb9e05fca0eb79752ae55a46faf3a98
(PCI PM: Power-manage devices without drivers during suspend-resume).

Thanks,
Rafael

Re: Regression 2.6.28-git: suspend/resume failure. git bisected to usb suspend/resume patch

[Rafael J. Wysocki]

On Monday 12 January 2009, Rafael J. Wysocki wrote:
> On Sunday 11 January 2009, Christian Borntraeger wrote:
> > Hello Alan,
> > 
> > git bisect found this patch 
> > 
> > a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 is first bad commit
> > commit a0d4922da2e4ccb0973095d8d29f36f6b1b5f703
> > Author: Alan Stern <stern@rowland.harvard.edu>
> > Date:   Wed Dec 17 15:06:03 2008 -0500
> > 
> >     USB: fix up suspend and resume for PCI host controllers
> > 
> >     This patch (as1192) rearranges the USB PCI host controller suspend and
> >     resume and resume routines:
> > 
> > 
> > To cause my suspend to ram problem with my Thinkpad T61p.
> > I can confirm that 
> > a81a81a25d3ecdab777abca87c5ddf484056103d works
> > and a0d4922da2e4ccb0973095d8d29f36f6b1b5f703  does not work.
> > 
> > Thing is, 29-rc1 with a0d4922da2e4ccb0973095d8d29f36f6b1b5f703  reverted also 
> > fails with suspend/resume. Seems that there was another problem introduced 
> > after that patch.
> 
> Please try if suspend-resume works for you if you make commit
> fa58d305d9925b01830e535896a7227a868a9e15
> (PCI PM: Add suspend counterpart of pci_reenable_device) the head of your
> tree ('git checkout -b test-branch fa58d305d9925b01830e535896a7227a868a9e15').
> 
> If it does, please check the same for commit
> 571ff7584bb9e05fca0eb79752ae55a46faf3a98
> (PCI PM: Power-manage devices without drivers during suspend-resume).

Ah, please do the above with the USB commit
a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 reverted.

Thanks,
Rafael

Re: Regression 2.6.28-git: suspend/resume failure. git bisected to usb suspend/resume patch

[Rafael J. Wysocki]

On Monday 12 January 2009, Rafael J. Wysocki wrote:
> On Monday 12 January 2009, Rafael J. Wysocki wrote:
> > On Sunday 11 January 2009, Christian Borntraeger wrote:
> > > Hello Alan,
> > > 
> > > git bisect found this patch 
> > > 
> > > a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 is first bad commit
> > > commit a0d4922da2e4ccb0973095d8d29f36f6b1b5f703
> > > Author: Alan Stern <stern@rowland.harvard.edu>
> > > Date:   Wed Dec 17 15:06:03 2008 -0500
> > > 
> > >     USB: fix up suspend and resume for PCI host controllers
> > > 
> > >     This patch (as1192) rearranges the USB PCI host controller suspend and
> > >     resume and resume routines:
> > > 
> > > 
> > > To cause my suspend to ram problem with my Thinkpad T61p.
> > > I can confirm that 
> > > a81a81a25d3ecdab777abca87c5ddf484056103d works
> > > and a0d4922da2e4ccb0973095d8d29f36f6b1b5f703  does not work.
> > > 
> > > Thing is, 29-rc1 with a0d4922da2e4ccb0973095d8d29f36f6b1b5f703  reverted also 
> > > fails with suspend/resume. Seems that there was another problem introduced 
> > > after that patch.

Please see if reverting commit 7503bfbae89eba07b46441a5d1594647f6b8ab7d
(cpumask: use work_on_cpu in acpi-cpufreq.c for drv_read and drv_write) in
addition to a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 helps.

Also, please check if the appended patch fixes the issue with
a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 .

Thanks,
Rafael


---
 drivers/usb/core/hcd-pci.c  |   96 +++++++-------------------------------------
 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, 16 insertions(+), 84 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
@@ -201,6 +201,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
@@ -247,26 +248,6 @@ int usb_hcd_pci_suspend(struct pci_dev *
 	 * 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);
 
@@ -315,7 +296,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
@@ -325,65 +306,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);
 
@@ -398,6 +322,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,
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: Regression 2.6.28-git: suspend/resume failure. git bisected to usb suspend/resume patch

[Jeff Chua]

On Tue, Jan 13, 2009 at 1:25 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Please see if reverting commit 7503bfbae89eba07b46441a5d1594647f6b8ab7d
> (cpumask: use work_on_cpu in acpi-cpufreq.c for drv_read and drv_write) in
> addition to a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 helps.

I can confirm that with these two commits reverted, my X61 can resume from STD.

> Also, please check if the appended patch fixes the issue with
> a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 .

This patch does not apply cleanly to the latest git pull.

Thanks,
Jeff.

Re: Regression 2.6.28-git: suspend/resume failure. git bisected to usb suspend/resume patch

[Rafael J. Wysocki]

On Tuesday 13 January 2009, Jeff Chua wrote:
> On Tue, Jan 13, 2009 at 1:25 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Please see if reverting commit 7503bfbae89eba07b46441a5d1594647f6b8ab7d
> > (cpumask: use work_on_cpu in acpi-cpufreq.c for drv_read and drv_write) in
> > addition to a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 helps.
> 
> I can confirm that with these two commits reverted, my X61 can resume from STD.
> 
> > Also, please check if the appended patch fixes the issue with
> > a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 .
> 
> This patch does not apply cleanly to the latest git pull.

I've just sent you a version that should apply to it in another thread.

Thanks,
Rafael

Re: Regression 2.6.28-git: suspend/resume failure. git bisected to usb suspend/resume patch

[Christian Borntraeger]

Am Montag 12 Januar 2009 schrieb Rafael J. Wysocki:
> > Please try if suspend-resume works for you if you make commit
> > fa58d305d9925b01830e535896a7227a868a9e15
> > (PCI PM: Add suspend counterpart of pci_reenable_device) the head of your
> > tree ('git checkout -b test-branch 
fa58d305d9925b01830e535896a7227a868a9e15').
> > 
> > If it does, please check the same for commit
> > 571ff7584bb9e05fca0eb79752ae55a46faf3a98
> > (PCI PM: Power-manage devices without drivers during suspend-resume).
> 
> Ah, please do the above with the USB commit
> a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 reverted.

Will do.

In the meantime I have bisected another problem: 
commit 7503bfbae89eba07b46441a5d1594647f6b8ab7d
Author: Mike Travis <travis@sgi.com>
Date:   Sun Jan 4 05:18:09 2009 -0800

    cpumask: use work_on_cpu in acpi-cpufreq.c for drv_read and drv_write

but I just see that this is already known.
O dear. How many patches do we know that broke S2R in this merge window? 4? 5?


Christian

Re: Regression 2.6.28-git: suspend/resume failure. git bisected to usb suspend/resume patch

[Rafael J. Wysocki]

On Tuesday 13 January 2009, Christian Borntraeger wrote:
> Am Montag 12 Januar 2009 schrieb Rafael J. Wysocki:
> > > Please try if suspend-resume works for you if you make commit
> > > fa58d305d9925b01830e535896a7227a868a9e15
> > > (PCI PM: Add suspend counterpart of pci_reenable_device) the head of your
> > > tree ('git checkout -b test-branch 
> fa58d305d9925b01830e535896a7227a868a9e15').
> > > 
> > > If it does, please check the same for commit
> > > 571ff7584bb9e05fca0eb79752ae55a46faf3a98
> > > (PCI PM: Power-manage devices without drivers during suspend-resume).
> > 
> > Ah, please do the above with the USB commit
> > a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 reverted.
> 
> Will do.
> 
> In the meantime I have bisected another problem: 
> commit 7503bfbae89eba07b46441a5d1594647f6b8ab7d
> Author: Mike Travis <travis@sgi.com>
> Date:   Sun Jan 4 05:18:09 2009 -0800
> 
>     cpumask: use work_on_cpu in acpi-cpufreq.c for drv_read and drv_write
> 
> but I just see that this is already known.
> O dear. How many patches do we know that broke S2R in this merge window? 4? 5?

We _know_ of 2.  The others were just good testing points.

Thanks,
Rafael

Re: Regression 2.6.28-git: suspend/resume failure. git bisected to usb suspend/resume patch

[Ingo Molnar]

* Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Am Montag 12 Januar 2009 schrieb Rafael J. Wysocki:
> > > Please try if suspend-resume works for you if you make commit
> > > fa58d305d9925b01830e535896a7227a868a9e15
> > > (PCI PM: Add suspend counterpart of pci_reenable_device) the head of your
> > > tree ('git checkout -b test-branch 
> fa58d305d9925b01830e535896a7227a868a9e15').
> > > 
> > > If it does, please check the same for commit
> > > 571ff7584bb9e05fca0eb79752ae55a46faf3a98
> > > (PCI PM: Power-manage devices without drivers during suspend-resume).
> > 
> > Ah, please do the above with the USB commit
> > a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 reverted.
> 
> Will do.
> 
> In the meantime I have bisected another problem: 
> commit 7503bfbae89eba07b46441a5d1594647f6b8ab7d
> Author: Mike Travis <travis@sgi.com>
> Date:   Sun Jan 4 05:18:09 2009 -0800
> 
>     cpumask: use work_on_cpu in acpi-cpufreq.c for drv_read and drv_write

that's reverted in upstream -git already.

	Ingo