Re: Thinkpad suspend-to-disk regression

Re: Thinkpad suspend-to-disk regression

[Guilherme Salgado]

On 4/24/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Tuesday, 24 April 2007 20:29, Guilherme Salgado wrote:
> > (I've already sent this to linux-acpi but got no response so far, so I
> > thought it could be a good idea to send directly to you)
> >
> > Hi there,
> >
> > I recently noticed that my thinkpad T60 would lose the sound after
> > being suspended to disk (https://launchpad.net/bugs/80893), requiring
> > a cold restart to fix it. After some git-bisect work I found this
> > regression was a consequence of
> > http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commitdiff;h=9185cfa92507d07ac787bc73d06c42222eec7239
> > (patch inlined here)
> >
> > Is there any chance of fixing the regression this introduced without
> > reverting it? Please let me know if you want any other info or have a
> > patch for me to try.
>
> You don't need a patch, I think.  Please try
>
> # echo shutdown > /sys/power/disk
>
> before the suspend (assuming you use the built-in swsusp).  If that works,
> just make your configuration scripts echo 'shutdown' to /sys/power/disk at
> system startup.
>

I use Ubuntu's default /etc/acpi/hibernate.sh, which already sets
/sys/power/disk to shutdown before echoing "disk" to /sys/power/state.

Anything else I could try?

Cheers,
Guilherme

Re: Thinkpad suspend-to-disk regression

[Rafael J. Wysocki]

On Wednesday, 25 April 2007 04:25, Guilherme Salgado wrote:
> On 4/24/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Tuesday, 24 April 2007 20:29, Guilherme Salgado wrote:
> > > (I've already sent this to linux-acpi but got no response so far, so I
> > > thought it could be a good idea to send directly to you)
> > >
> > > Hi there,
> > >
> > > I recently noticed that my thinkpad T60 would lose the sound after
> > > being suspended to disk (https://launchpad.net/bugs/80893), requiring
> > > a cold restart to fix it. After some git-bisect work I found this
> > > regression was a consequence of
> > > http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commitdiff;h=9185cfa92507d07ac787bc73d06c42222eec7239
> > > (patch inlined here)
> > >
> > > Is there any chance of fixing the regression this introduced without
> > > reverting it? Please let me know if you want any other info or have a
> > > patch for me to try.
> >
> > You don't need a patch, I think.  Please try
> >
> > # echo shutdown > /sys/power/disk
> >
> > before the suspend (assuming you use the built-in swsusp).  If that works,
> > just make your configuration scripts echo 'shutdown' to /sys/power/disk at
> > system startup.
> >
> 
> I use Ubuntu's default /etc/acpi/hibernate.sh, which already sets
> /sys/power/disk to shutdown before echoing "disk" to /sys/power/state.

In that case it's impossible that the commit you have identified causes the
problem to happen.

It doesn't even touch the 'shutdown' code path.

Greetings,
Rafael

Re: Thinkpad suspend-to-disk regression

[Guilherme Salgado]

On 4/25/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, 25 April 2007 04:25, Guilherme Salgado wrote:
> > On 4/24/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Tuesday, 24 April 2007 20:29, Guilherme Salgado wrote:
> > > > (I've already sent this to linux-acpi but got no response so far, so I
> > > > thought it could be a good idea to send directly to you)
> > > >
> > > > Hi there,
> > > >
> > > > I recently noticed that my thinkpad T60 would lose the sound after
> > > > being suspended to disk (https://launchpad.net/bugs/80893), requiring
> > > > a cold restart to fix it. After some git-bisect work I found this
> > > > regression was a consequence of
> > > > http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commitdiff;h=9185cfa92507d07ac787bc73d06c42222eec7239
> > > > (patch inlined here)
> > > >
> > > > Is there any chance of fixing the regression this introduced without
> > > > reverting it? Please let me know if you want any other info or have a
> > > > patch for me to try.
> > >
> > > You don't need a patch, I think.  Please try
> > >
> > > # echo shutdown > /sys/power/disk
> > >
> > > before the suspend (assuming you use the built-in swsusp).  If that works,
> > > just make your configuration scripts echo 'shutdown' to /sys/power/disk at
> > > system startup.
> > >
> >
> > I use Ubuntu's default /etc/acpi/hibernate.sh, which already sets
> > /sys/power/disk to shutdown before echoing "disk" to /sys/power/state.
>
> In that case it's impossible that the commit you have identified causes the
> problem to happen.
>
> It doesn't even touch the 'shutdown' code path.

Is it be possible that the change to /sys/power/disk is not doing what
it should? I think that's likely to be the case because I got a bunch
of people to try a kernel with the patch reverted and it fixed the
problem they were having.

The most recent comments at https://bugs.launchpad.net/+bugs/80893 are
all confirming that the kernel at
http://rookery.ubuntu.com/~kyle/kernels/salgado/2007-04-25/ fixes
their problem.

I'd be glad to do anything to further debug this if you can give me
some pointers.

Cheers,
Guilherme

Re: Thinkpad suspend-to-disk regression

[Rafael J. Wysocki]

On Wednesday, 2 May 2007 19:28, Guilherme Salgado wrote:
> On 4/25/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday, 25 April 2007 04:25, Guilherme Salgado wrote:
> > > On 4/24/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > On Tuesday, 24 April 2007 20:29, Guilherme Salgado wrote:
> > > > > (I've already sent this to linux-acpi but got no response so far, so I
> > > > > thought it could be a good idea to send directly to you)
> > > > >
> > > > > Hi there,
> > > > >
> > > > > I recently noticed that my thinkpad T60 would lose the sound after
> > > > > being suspended to disk (https://launchpad.net/bugs/80893), requiring
> > > > > a cold restart to fix it. After some git-bisect work I found this
> > > > > regression was a consequence of
> > > > > http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commitdiff;h=9185cfa92507d07ac787bc73d06c42222eec7239
> > > > > (patch inlined here)
> > > > >
> > > > > Is there any chance of fixing the regression this introduced without
> > > > > reverting it? Please let me know if you want any other info or have a
> > > > > patch for me to try.
> > > >
> > > > You don't need a patch, I think.  Please try
> > > >
> > > > # echo shutdown > /sys/power/disk
> > > >
> > > > before the suspend (assuming you use the built-in swsusp).  If that works,
> > > > just make your configuration scripts echo 'shutdown' to /sys/power/disk at
> > > > system startup.
> > > >
> > >
> > > I use Ubuntu's default /etc/acpi/hibernate.sh, which already sets
> > > /sys/power/disk to shutdown before echoing "disk" to /sys/power/state.
> >
> > In that case it's impossible that the commit you have identified causes the
> > problem to happen.
> >
> > It doesn't even touch the 'shutdown' code path.
> 
> Is it be possible that the change to /sys/power/disk is not doing what
> it should? I think that's likely to be the case because I got a bunch
> of people to try a kernel with the patch reverted and it fixed the
> problem they were having.
> 
> The most recent comments at https://bugs.launchpad.net/+bugs/80893 are
> all confirming that the kernel at
> http://rookery.ubuntu.com/~kyle/kernels/salgado/2007-04-25/ fixes
> their problem.
> 
> I'd be glad to do anything to further debug this if you can give me
> some pointers.

Well, the problem is that we have switched to the platform mode by default
and "echo shutdown > /sys/power/disk" before the suspend should get the old
behavior back

Please try to apply the appended patch instead of reverting the entire commit
and see if the problem is still present.

Greetings,
Rafael

---
 kernel/power/main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.21-rc7/kernel/power/main.c
===================================================================
--- linux-2.6.21-rc7.orig/kernel/power/main.c
+++ linux-2.6.21-rc7/kernel/power/main.c
@@ -30,7 +30,7 @@
 DEFINE_MUTEX(pm_mutex);
 
 struct pm_ops *pm_ops;
-suspend_disk_method_t pm_disk_mode = PM_DISK_PLATFORM;
+suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
 
 /**
  *	pm_set_ops - Set the global power method table. 

Re: Thinkpad suspend-to-disk regression

[Guilherme Salgado]

On 5/2/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, 2 May 2007 19:28, Guilherme Salgado wrote:
> > On 4/25/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Wednesday, 25 April 2007 04:25, Guilherme Salgado wrote:
> > > > On 4/24/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > On Tuesday, 24 April 2007 20:29, Guilherme Salgado wrote:
> > > > > > (I've already sent this to linux-acpi but got no response so far, so I
> > > > > > thought it could be a good idea to send directly to you)
> > > > > >
> > > > > > Hi there,
> > > > > >
> > > > > > I recently noticed that my thinkpad T60 would lose the sound after
> > > > > > being suspended to disk (https://launchpad.net/bugs/80893), requiring
> > > > > > a cold restart to fix it. After some git-bisect work I found this
> > > > > > regression was a consequence of
> > > > > > http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commitdiff;h=9185cfa92507d07ac787bc73d06c42222eec7239
> > > > > > (patch inlined here)
> > > > > >
> > > > > > Is there any chance of fixing the regression this introduced without
> > > > > > reverting it? Please let me know if you want any other info or have a
> > > > > > patch for me to try.
> > > > >
> > > > > You don't need a patch, I think.  Please try
> > > > >
> > > > > # echo shutdown > /sys/power/disk
> > > > >
> > > > > before the suspend (assuming you use the built-in swsusp).  If that works,
> > > > > just make your configuration scripts echo 'shutdown' to /sys/power/disk at
> > > > > system startup.
> > > > >
> > > >
> > > > I use Ubuntu's default /etc/acpi/hibernate.sh, which already sets
> > > > /sys/power/disk to shutdown before echoing "disk" to /sys/power/state.
> > >
> > > In that case it's impossible that the commit you have identified causes the
> > > problem to happen.
> > >
> > > It doesn't even touch the 'shutdown' code path.
> >
> > Is it be possible that the change to /sys/power/disk is not doing what
> > it should? I think that's likely to be the case because I got a bunch
> > of people to try a kernel with the patch reverted and it fixed the
> > problem they were having.
> >
> > The most recent comments at https://bugs.launchpad.net/+bugs/80893 are
> > all confirming that the kernel at
> > http://rookery.ubuntu.com/~kyle/kernels/salgado/2007-04-25/ fixes
> > their problem.
> >
> > I'd be glad to do anything to further debug this if you can give me
> > some pointers.
>
> Well, the problem is that we have switched to the platform mode by default
> and "echo shutdown > /sys/power/disk" before the suspend should get the old
> behavior back
>

Right, that's why I asked if it'd be possible that echoing "shutdown"
to /sys/power/disk could somehow be a no-op in this case.

> Please try to apply the appended patch instead of reverting the entire commit
> and see if the problem is still present.
>

That works just fine, yes. Are you willing to change the default back
to PM_DISK_SHUTDOWN or should we find out why "echo shutdown >
/sys/power/disk" is not having the same effect as changing the
default, as it should?

Cheers,
Guilherme

Re: Thinkpad suspend-to-disk regression

[Rafael J. Wysocki]

On Thursday, 3 May 2007 15:45, Guilherme Salgado wrote:
> On 5/2/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday, 2 May 2007 19:28, Guilherme Salgado wrote:
> > > On 4/25/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > On Wednesday, 25 April 2007 04:25, Guilherme Salgado wrote:
> > > > > On 4/24/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > > On Tuesday, 24 April 2007 20:29, Guilherme Salgado wrote:
> > > > > > > (I've already sent this to linux-acpi but got no response so far, so I
> > > > > > > thought it could be a good idea to send directly to you)
> > > > > > >
> > > > > > > Hi there,
> > > > > > >
> > > > > > > I recently noticed that my thinkpad T60 would lose the sound after
> > > > > > > being suspended to disk (https://launchpad.net/bugs/80893), requiring
> > > > > > > a cold restart to fix it. After some git-bisect work I found this
> > > > > > > regression was a consequence of
> > > > > > > http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commitdiff;h=9185cfa92507d07ac787bc73d06c42222eec7239
> > > > > > > (patch inlined here)
> > > > > > >
> > > > > > > Is there any chance of fixing the regression this introduced without
> > > > > > > reverting it? Please let me know if you want any other info or have a
> > > > > > > patch for me to try.
> > > > > >
> > > > > > You don't need a patch, I think.  Please try
> > > > > >
> > > > > > # echo shutdown > /sys/power/disk
> > > > > >
> > > > > > before the suspend (assuming you use the built-in swsusp).  If that works,
> > > > > > just make your configuration scripts echo 'shutdown' to /sys/power/disk at
> > > > > > system startup.
> > > > > >
> > > > >
> > > > > I use Ubuntu's default /etc/acpi/hibernate.sh, which already sets
> > > > > /sys/power/disk to shutdown before echoing "disk" to /sys/power/state.
> > > >
> > > > In that case it's impossible that the commit you have identified causes the
> > > > problem to happen.
> > > >
> > > > It doesn't even touch the 'shutdown' code path.
> > >
> > > Is it be possible that the change to /sys/power/disk is not doing what
> > > it should? I think that's likely to be the case because I got a bunch
> > > of people to try a kernel with the patch reverted and it fixed the
> > > problem they were having.
> > >
> > > The most recent comments at https://bugs.launchpad.net/+bugs/80893 are
> > > all confirming that the kernel at
> > > http://rookery.ubuntu.com/~kyle/kernels/salgado/2007-04-25/ fixes
> > > their problem.
> > >
> > > I'd be glad to do anything to further debug this if you can give me
> > > some pointers.
> >
> > Well, the problem is that we have switched to the platform mode by default
> > and "echo shutdown > /sys/power/disk" before the suspend should get the old
> > behavior back
> >
> 
> Right, that's why I asked if it'd be possible that echoing "shutdown"
> to /sys/power/disk could somehow be a no-op in this case.
> 
> > Please try to apply the appended patch instead of reverting the entire commit
> > and see if the problem is still present.
> >
> 
> That works just fine, yes. Are you willing to change the default back
> to PM_DISK_SHUTDOWN or should we find out why "echo shutdown >
> /sys/power/disk" is not having the same effect as changing the
> default, as it should?

In fact, I'd rather would like to find out what's the reason for the
"echo shutdown > /sys/power/disk" not working.

Besides, could you please check if the problems with the 'platform' mode
suspend persist with the appended patch applied?

Greetings,
Rafael

---

 kernel/power/disk.c |    1 -
 kernel/power/user.c |   15 +++------------
 2 files changed, 3 insertions(+), 13 deletions(-)

Index: linux-2.6.21/kernel/power/disk.c
===================================================================
--- linux-2.6.21.orig/kernel/power/disk.c	2007-05-03 12:24:05.000000000 +0200
+++ linux-2.6.21/kernel/power/disk.c	2007-05-03 14:42:26.000000000 +0200
@@ -195,7 +195,6 @@ int hibernate(void)
 
 	if (in_suspend) {
 		enable_nonboot_cpus();
-		platform_finish();
 		device_resume();
 		resume_console();
 		pr_debug("PM: writing image.\n");
Index: linux-2.6.21/kernel/power/user.c
===================================================================
--- linux-2.6.21.orig/kernel/power/user.c	2007-05-03 12:22:57.000000000 +0200
+++ linux-2.6.21/kernel/power/user.c	2007-05-03 14:40:49.000000000 +0200
@@ -169,7 +169,7 @@ static inline int snapshot_suspend(int p
 	}
 	enable_nonboot_cpus();
  Resume_devices:
-	if (platform_suspend)
+	if (platform_suspend && (!in_suspend || error))
 		platform_finish();
 
 	device_resume();
@@ -179,17 +179,12 @@ static inline int snapshot_suspend(int p
 	return error;
 }
 
-static inline int snapshot_restore(int platform_suspend)
+static inline int snapshot_restore(void)
 {
 	int error;
 
 	mutex_lock(&pm_mutex);
 	pm_prepare_console();
-	if (platform_suspend) {
-		error = platform_prepare();
-		if (error)
-			goto Finish;
-	}
 	suspend_console();
 	error = device_suspend(PMSG_PRETHAW);
 	if (error)
@@ -201,12 +196,8 @@ static inline int snapshot_restore(int p
 
 	enable_nonboot_cpus();
  Resume_devices:
-	if (platform_suspend)
-		platform_finish();
-
 	device_resume();
 	resume_console();
- Finish:
 	pm_restore_console();
 	mutex_unlock(&pm_mutex);
 	return error;
@@ -272,7 +263,7 @@ static int snapshot_ioctl(struct inode *
 			error = -EPERM;
 			break;
 		}
-		error = snapshot_restore(data->platform_suspend);
+		error = snapshot_restore();
 		break;
 
 	case SNAPSHOT_FREE:

Re: Thinkpad suspend-to-disk regression

[Guilherme Salgado]

On 5/3/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, 3 May 2007 15:45, Guilherme Salgado wrote:
> > On 5/2/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Wednesday, 2 May 2007 19:28, Guilherme Salgado wrote:
> > > > On 4/25/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > On Wednesday, 25 April 2007 04:25, Guilherme Salgado wrote:
> > > > > > On 4/24/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > > > On Tuesday, 24 April 2007 20:29, Guilherme Salgado wrote:
> > > > > > > > (I've already sent this to linux-acpi but got no response so far, so I
> > > > > > > > thought it could be a good idea to send directly to you)
> > > > > > > >
> > > > > > > > Hi there,
> > > > > > > >
> > > > > > > > I recently noticed that my thinkpad T60 would lose the sound after
> > > > > > > > being suspended to disk (https://launchpad.net/bugs/80893), requiring
> > > > > > > > a cold restart to fix it. After some git-bisect work I found this
> > > > > > > > regression was a consequence of
> > > > > > > > http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commitdiff;h=9185cfa92507d07ac787bc73d06c42222eec7239
> > > > > > > > (patch inlined here)
> > > > > > > >
> > > > > > > > Is there any chance of fixing the regression this introduced without
> > > > > > > > reverting it? Please let me know if you want any other info or have a
> > > > > > > > patch for me to try.
> > > > > > >
> > > > > > > You don't need a patch, I think.  Please try
> > > > > > >
> > > > > > > # echo shutdown > /sys/power/disk
> > > > > > >
> > > > > > > before the suspend (assuming you use the built-in swsusp).  If that works,
> > > > > > > just make your configuration scripts echo 'shutdown' to /sys/power/disk at
> > > > > > > system startup.
> > > > > > >
> > > > > >
> > > > > > I use Ubuntu's default /etc/acpi/hibernate.sh, which already sets
> > > > > > /sys/power/disk to shutdown before echoing "disk" to /sys/power/state.
> > > > >
> > > > > In that case it's impossible that the commit you have identified causes the
> > > > > problem to happen.
> > > > >
> > > > > It doesn't even touch the 'shutdown' code path.
> > > >
> > > > Is it be possible that the change to /sys/power/disk is not doing what
> > > > it should? I think that's likely to be the case because I got a bunch
> > > > of people to try a kernel with the patch reverted and it fixed the
> > > > problem they were having.
> > > >
> > > > The most recent comments at https://bugs.launchpad.net/+bugs/80893 are
> > > > all confirming that the kernel at
> > > > http://rookery.ubuntu.com/~kyle/kernels/salgado/2007-04-25/ fixes
> > > > their problem.
> > > >
> > > > I'd be glad to do anything to further debug this if you can give me
> > > > some pointers.
> > >
> > > Well, the problem is that we have switched to the platform mode by default
> > > and "echo shutdown > /sys/power/disk" before the suspend should get the old
> > > behavior back
> > >
> >
> > Right, that's why I asked if it'd be possible that echoing "shutdown"
> > to /sys/power/disk could somehow be a no-op in this case.
> >
> > > Please try to apply the appended patch instead of reverting the entire commit
> > > and see if the problem is still present.
> > >
> >
> > That works just fine, yes. Are you willing to change the default back
> > to PM_DISK_SHUTDOWN or should we find out why "echo shutdown >
> > /sys/power/disk" is not having the same effect as changing the
> > default, as it should?
>
> In fact, I'd rather would like to find out what's the reason for the
> "echo shutdown > /sys/power/disk" not working.
>

So do I, but I'm sort of short on ideas for how to debug this (IANAKD
:-); that's why I asked if you'd have any pointers.

> Besides, could you please check if the problems with the 'platform' mode
> suspend persist with the appended patch applied?
>

Yep, the problem persists, unfortunately.

Cheers,
Guilherme

Re: Thinkpad suspend-to-disk regression

[Rafael J. Wysocki]

On Friday, 4 May 2007 01:34, Guilherme Salgado wrote:
> On 5/3/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, 3 May 2007 15:45, Guilherme Salgado wrote:
> > > On 5/2/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > On Wednesday, 2 May 2007 19:28, Guilherme Salgado wrote:
> > > > > On 4/25/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > > On Wednesday, 25 April 2007 04:25, Guilherme Salgado wrote:
> > > > > > > On 4/24/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > > > > On Tuesday, 24 April 2007 20:29, Guilherme Salgado wrote:
> > > > > > > > > (I've already sent this to linux-acpi but got no response so far, so I
> > > > > > > > > thought it could be a good idea to send directly to you)
> > > > > > > > >
> > > > > > > > > Hi there,
> > > > > > > > >
> > > > > > > > > I recently noticed that my thinkpad T60 would lose the sound after
> > > > > > > > > being suspended to disk (https://launchpad.net/bugs/80893), requiring
> > > > > > > > > a cold restart to fix it. After some git-bisect work I found this
> > > > > > > > > regression was a consequence of
> > > > > > > > > http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commitdiff;h=9185cfa92507d07ac787bc73d06c42222eec7239
> > > > > > > > > (patch inlined here)
> > > > > > > > >
> > > > > > > > > Is there any chance of fixing the regression this introduced without
> > > > > > > > > reverting it? Please let me know if you want any other info or have a
> > > > > > > > > patch for me to try.
> > > > > > > >
> > > > > > > > You don't need a patch, I think.  Please try
> > > > > > > >
> > > > > > > > # echo shutdown > /sys/power/disk
> > > > > > > >
> > > > > > > > before the suspend (assuming you use the built-in swsusp).  If that works,
> > > > > > > > just make your configuration scripts echo 'shutdown' to /sys/power/disk at
> > > > > > > > system startup.
> > > > > > > >
> > > > > > >
> > > > > > > I use Ubuntu's default /etc/acpi/hibernate.sh, which already sets
> > > > > > > /sys/power/disk to shutdown before echoing "disk" to /sys/power/state.
> > > > > >
> > > > > > In that case it's impossible that the commit you have identified causes the
> > > > > > problem to happen.
> > > > > >
> > > > > > It doesn't even touch the 'shutdown' code path.
> > > > >
> > > > > Is it be possible that the change to /sys/power/disk is not doing what
> > > > > it should? I think that's likely to be the case because I got a bunch
> > > > > of people to try a kernel with the patch reverted and it fixed the
> > > > > problem they were having.
> > > > >
> > > > > The most recent comments at https://bugs.launchpad.net/+bugs/80893 are
> > > > > all confirming that the kernel at
> > > > > http://rookery.ubuntu.com/~kyle/kernels/salgado/2007-04-25/ fixes
> > > > > their problem.
> > > > >
> > > > > I'd be glad to do anything to further debug this if you can give me
> > > > > some pointers.
> > > >
> > > > Well, the problem is that we have switched to the platform mode by default
> > > > and "echo shutdown > /sys/power/disk" before the suspend should get the old
> > > > behavior back
> > > >
> > >
> > > Right, that's why I asked if it'd be possible that echoing "shutdown"
> > > to /sys/power/disk could somehow be a no-op in this case.
> > >
> > > > Please try to apply the appended patch instead of reverting the entire commit
> > > > and see if the problem is still present.
> > > >
> > >
> > > That works just fine, yes. Are you willing to change the default back
> > > to PM_DISK_SHUTDOWN or should we find out why "echo shutdown >
> > > /sys/power/disk" is not having the same effect as changing the
> > > default, as it should?
> >
> > In fact, I'd rather would like to find out what's the reason for the
> > "echo shutdown > /sys/power/disk" not working.
> >
> 
> So do I, but I'm sort of short on ideas for how to debug this (IANAKD
> :-); that's why I asked if you'd have any pointers.

Hmm, can you try to start the hibernation manually?  I mean, do

# echo 5 > /proc/sys/kernel/printk
# echo shutdown > /sys/power/disk

and check what it shows:

# cat /sys/power/disk

If this is 'shutdown', then do

# echo disk > /sys/power/state

and see if that works.

If you've done it already, please let me know and I'll prepare a debug patch
for you.

> 
> > Besides, could you please check if the problems with the 'platform' mode
> > suspend persist with the appended patch applied?
> >
> 
> Yep, the problem persists, unfortunately.

OK, so much for that.  Thanks for testing.

Greetings,
Rafael

Re: Thinkpad suspend-to-disk regression

[Guilherme Salgado]

On 5/4/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, 4 May 2007 01:34, Guilherme Salgado wrote:
> > On 5/3/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Thursday, 3 May 2007 15:45, Guilherme Salgado wrote:
> > > > On 5/2/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > On Wednesday, 2 May 2007 19:28, Guilherme Salgado wrote:
> > > > > > On 4/25/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > > > On Wednesday, 25 April 2007 04:25, Guilherme Salgado wrote:
> > > > > > > > On 4/24/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > > > > > On Tuesday, 24 April 2007 20:29, Guilherme Salgado wrote:
> > > > > > > > > > (I've already sent this to linux-acpi but got no response so far, so I
> > > > > > > > > > thought it could be a good idea to send directly to you)
> > > > > > > > > >
> > > > > > > > > > Hi there,
> > > > > > > > > >
> > > > > > > > > > I recently noticed that my thinkpad T60 would lose the sound after
> > > > > > > > > > being suspended to disk (https://launchpad.net/bugs/80893), requiring
> > > > > > > > > > a cold restart to fix it. After some git-bisect work I found this
> > > > > > > > > > regression was a consequence of
> > > > > > > > > > http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commitdiff;h=9185cfa92507d07ac787bc73d06c42222eec7239
> > > > > > > > > > (patch inlined here)
> > > > > > > > > >
> > > > > > > > > > Is there any chance of fixing the regression this introduced without
> > > > > > > > > > reverting it? Please let me know if you want any other info or have a
> > > > > > > > > > patch for me to try.
> > > > > > > > >
> > > > > > > > > You don't need a patch, I think.  Please try
> > > > > > > > >
> > > > > > > > > # echo shutdown > /sys/power/disk
> > > > > > > > >
> > > > > > > > > before the suspend (assuming you use the built-in swsusp).  If that works,
> > > > > > > > > just make your configuration scripts echo 'shutdown' to /sys/power/disk at
> > > > > > > > > system startup.
> > > > > > > > >
> > > > > > > >
> > > > > > > > I use Ubuntu's default /etc/acpi/hibernate.sh, which already sets
> > > > > > > > /sys/power/disk to shutdown before echoing "disk" to /sys/power/state.
> > > > > > >
> > > > > > > In that case it's impossible that the commit you have identified causes the
> > > > > > > problem to happen.
> > > > > > >
> > > > > > > It doesn't even touch the 'shutdown' code path.
> > > > > >
> > > > > > Is it be possible that the change to /sys/power/disk is not doing what
> > > > > > it should? I think that's likely to be the case because I got a bunch
> > > > > > of people to try a kernel with the patch reverted and it fixed the
> > > > > > problem they were having.
> > > > > >
> > > > > > The most recent comments at https://bugs.launchpad.net/+bugs/80893 are
> > > > > > all confirming that the kernel at
> > > > > > http://rookery.ubuntu.com/~kyle/kernels/salgado/2007-04-25/ fixes
> > > > > > their problem.
> > > > > >
> > > > > > I'd be glad to do anything to further debug this if you can give me
> > > > > > some pointers.
> > > > >
> > > > > Well, the problem is that we have switched to the platform mode by default
> > > > > and "echo shutdown > /sys/power/disk" before the suspend should get the old
> > > > > behavior back
> > > > >
> > > >
> > > > Right, that's why I asked if it'd be possible that echoing "shutdown"
> > > > to /sys/power/disk could somehow be a no-op in this case.
> > > >
> > > > > Please try to apply the appended patch instead of reverting the entire commit
> > > > > and see if the problem is still present.
> > > > >
> > > >
> > > > That works just fine, yes. Are you willing to change the default back
> > > > to PM_DISK_SHUTDOWN or should we find out why "echo shutdown >
> > > > /sys/power/disk" is not having the same effect as changing the
> > > > default, as it should?
> > >
> > > In fact, I'd rather would like to find out what's the reason for the
> > > "echo shutdown > /sys/power/disk" not working.
> > >
> >
> > So do I, but I'm sort of short on ideas for how to debug this (IANAKD
> > :-); that's why I asked if you'd have any pointers.
>
> Hmm, can you try to start the hibernation manually?  I mean, do
>
> # echo 5 > /proc/sys/kernel/printk
> # echo shutdown > /sys/power/disk
>
> and check what it shows:
>
> # cat /sys/power/disk
>
> If this is 'shutdown', then do
>
> # echo disk > /sys/power/state
>
> and see if that works.
>
> If you've done it already, please let me know and I'll prepare a debug patch
> for you.
>

This is what my hibernation script does, but I did it manually hoping
that waiting a few seconds between changing /sys/power/disk and
/sys/power/state could solve the problem. Unfortunately, it didn't; I
still get no sound after resuming.

> >
> > > Besides, could you please check if the problems with the 'platform' mode
> > > suspend persist with the appended patch applied?
> > >
> >
> > Yep, the problem persists, unfortunately.
>
> OK, so much for that.  Thanks for testing.
>

Thank you!

Re: Thinkpad suspend-to-disk regression

[Joerg Platte]

Am Freitag, 4. Mai 2007 schrieb Guilherme Salgado:

> This is what my hibernation script does, but I did it manually hoping
> that waiting a few seconds between changing /sys/power/disk and
> /sys/power/state could solve the problem. Unfortunately, it didn't; I
> still get no sound after resuming.

I had a similar problem with a recent kernel (but not with 2.6.21-rc7) on my 
T40p. Sound was lost after suspend-to-disk but could be re-activated by 
suspending to RAM. I did not further debug this, because I'm using suspend to 
disk regularly. You can try to suspend to RAM to see if it helps.

regards,
Jörg

-- 
PGP Key: send mail with subject 'SEND PGP-KEY' PGP Key-ID: FD 4E 21 1D
PGP Fingerprint: 388A872AFC5649D3 BCEC65778BE0C605
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Thinkpad suspend-to-disk regression

[Guilherme Salgado]

On 5/4/07, Joerg Platte <lists@naasa.net> wrote:
> Am Freitag, 4. Mai 2007 schrieb Guilherme Salgado:
>
> > This is what my hibernation script does, but I did it manually hoping
> > that waiting a few seconds between changing /sys/power/disk and
> > /sys/power/state could solve the problem. Unfortunately, it didn't; I
> > still get no sound after resuming.
>
> I had a similar problem with a recent kernel (but not with 2.6.21-rc7) on my
> T40p. Sound was lost after suspend-to-disk but could be re-activated by
> suspending to RAM. I did not further debug this, because I'm using suspend to
> disk regularly. You can try to suspend to RAM to see if it helps.
>

Yeah, I've heard reports that suspending to RAM brings the sound back,
but that's not really a solution to the problem. The last kernel I
tried was 2.6.21-rc4 and it has the problem; I'll pull the most recent
one and give it a try.

Thanks,

Re: Thinkpad suspend-to-disk regression

[Guilherme Salgado]

On 5/4/07, Guilherme Salgado <gsalgado@gmail.com> wrote:
> On 5/4/07, Joerg Platte <lists@naasa.net> wrote:
> > Am Freitag, 4. Mai 2007 schrieb Guilherme Salgado:
> >
> > > This is what my hibernation script does, but I did it manually hoping
> > > that waiting a few seconds between changing /sys/power/disk and
> > > /sys/power/state could solve the problem. Unfortunately, it didn't; I
> > > still get no sound after resuming.
> >
> > I had a similar problem with a recent kernel (but not with 2.6.21-rc7) on my
> > T40p. Sound was lost after suspend-to-disk but could be re-activated by
> > suspending to RAM. I did not further debug this, because I'm using suspend to
> > disk regularly. You can try to suspend to RAM to see if it helps.
> >
>
> Yeah, I've heard reports that suspending to RAM brings the sound back,
> but that's not really a solution to the problem. The last kernel I
> tried was 2.6.21-rc4 and it has the problem; I'll pull the most recent
> one and give it a try.
>

Just tested and sound works fine after resuming a 2.6.21 kernel.

I just manually suspended my thinkpad twice. The first time with
/sys/power/disk set to 'shutdown' and the second with it set to
'platform'. In both cases I got sound after resuming. IOW, the problem
seems to be fixed.

Since I'd like to get this fix into Ubuntu's kernel, I guess I'll have
to play with git bisect some more. I'd appreciate any hints as to what
revision could have fixed this. I know for sure that it wasn't fixed
on 2.6.21-rc4 but it's fixed in 2.6.21.

Cheers,
Guilherme

Re: Thinkpad suspend-to-disk regression

[Rafael J. Wysocki]

On Friday, 4 May 2007 18:08, Guilherme Salgado wrote:
> On 5/4/07, Guilherme Salgado <gsalgado@gmail.com> wrote:
> > On 5/4/07, Joerg Platte <lists@naasa.net> wrote:
> > > Am Freitag, 4. Mai 2007 schrieb Guilherme Salgado:
> > >
> > > > This is what my hibernation script does, but I did it manually hoping
> > > > that waiting a few seconds between changing /sys/power/disk and
> > > > /sys/power/state could solve the problem. Unfortunately, it didn't; I
> > > > still get no sound after resuming.
> > >
> > > I had a similar problem with a recent kernel (but not with 2.6.21-rc7) on my
> > > T40p. Sound was lost after suspend-to-disk but could be re-activated by
> > > suspending to RAM. I did not further debug this, because I'm using suspend to
> > > disk regularly. You can try to suspend to RAM to see if it helps.
> > >
> >
> > Yeah, I've heard reports that suspending to RAM brings the sound back,
> > but that's not really a solution to the problem. The last kernel I
> > tried was 2.6.21-rc4 and it has the problem; I'll pull the most recent
> > one and give it a try.
> >
> 
> Just tested and sound works fine after resuming a 2.6.21 kernel.
> 
> I just manually suspended my thinkpad twice. The first time with
> /sys/power/disk set to 'shutdown' and the second with it set to
> 'platform'. In both cases I got sound after resuming. IOW, the problem
> seems to be fixed.
> 
> Since I'd like to get this fix into Ubuntu's kernel, I guess I'll have
> to play with git bisect some more. I'd appreciate any hints as to what
> revision could have fixed this. I know for sure that it wasn't fixed
> on 2.6.21-rc4 but it's fixed in 2.6.21.

I'm afraid I can't help you much with that.

I'd probably concentrate on the ACPI-related changes and the changes related
to timers (hrtimers, NO_HZ etc.).

Greetings,
Rafael

Thinkpad suspend-to-disk regression

[Guilherme Salgado]

Hi there,

I recently noticed that my thinkpad T60 would lose the sound after
being suspended to disk (https://launchpad.net/bugs/80893), requiring
a cold restart to fix it. After some git-bisect work I found this
regression was a consequence of
http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commitdiff;h=9185cfa92507d07ac787bc73d06c42222eec7239
(patch inlined here)

Is there any chance of fixing the regression this introduced without
reverting it? Please let me know if you want any other info or have a
patch for me to try.

Cheers,
Guilherme

diff --git a/kernel/power/disk.c b/kernel/power/disk.c
index 0b00f56..88fc5d7 100644
--- a/kernel/power/disk.c
+++ b/kernel/power/disk.c
@@ -60,9 +60,11 @@ static void power_down(suspend_disk_method_t mode)
 {
 	switch(mode) {
 	case PM_DISK_PLATFORM:
-		kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
-		pm_ops->enter(PM_SUSPEND_DISK);
-		break;
+		if (pm_ops && pm_ops->enter) {
+			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
+			pm_ops->enter(PM_SUSPEND_DISK);
+			break;
+		}
 	case PM_DISK_SHUTDOWN:
 		kernel_power_off();
 		break;
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 500eb87..ff3a618 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -29,7 +29,7 @@
 DEFINE_MUTEX(pm_mutex);

 struct pm_ops *pm_ops;
-suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
+suspend_disk_method_t pm_disk_mode = PM_DISK_PLATFORM;

 /**
  *	pm_set_ops - Set the global power method table.