2.6.21 suspend-to-disk regression

2.6.21 suspend-to-disk regression

[Marcus Better]

Hi,

suspend-to-disk isn't working well anymore on my Thinkpad R60 with Intel
Core 2 Duo and kernel 2.6.21 (x86_64). It's a Debian system with uswsusp
0.6~cvs20070202-1, and I'm using s2disk. I believe it was quite stable
around 2.6.20. 

It does succeed sometimes, but often it fails at suspend. Several times it
hung at "snapshotting system" with the sleep LED blinking. Once it just
rebooted at that point. Sometimes it suspends correctly but the image is
not recognized at boot.

I don't know if this is similar to any of the previously listed regressions,
it's a bit daunting to wade through all the suspend-related threads...

Marcus

Re: 2.6.21 suspend-to-disk regression

[Rafael J. Wysocki]

Hi,

On Sunday, 29 April 2007 16:22, Marcus Better wrote:
> Hi,
> 
> suspend-to-disk isn't working well anymore on my Thinkpad R60 with Intel
> Core 2 Duo and kernel 2.6.21 (x86_64). It's a Debian system with uswsusp
> 0.6~cvs20070202-1, and I'm using s2disk. I believe it was quite stable
> around 2.6.20. 
> 
> It does succeed sometimes, but often it fails at suspend. Several times it
> hung at "snapshotting system" with the sleep LED blinking. Once it just
> rebooted at that point. Sometimes it suspends correctly but the image is
> not recognized at boot.
> 
> I don't know if this is similar to any of the previously listed regressions,
> it's a bit daunting to wade through all the suspend-related threads...

We have some suspend regressions on Thinkpads reported, but I think they
are different to what you describe.

Can you please apply the appended patch and see if that helps?

Rafael

---
 kernel/power/disk.c |    4 ++--
 kernel/power/user.c |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

Index: linux-2.6.21-rc7/kernel/power/disk.c
===================================================================
--- linux-2.6.21-rc7.orig/kernel/power/disk.c
+++ linux-2.6.21-rc7/kernel/power/disk.c
@@ -170,9 +170,9 @@ int pm_suspend_disk(void)
 
 	if (in_suspend) {
 		enable_nonboot_cpus();
-		platform_finish();
 		device_resume();
 		resume_console();
+		platform_finish();
 		pr_debug("PM: writing image.\n");
 		error = swsusp_write();
 		if (!error)
@@ -189,9 +189,9 @@ int pm_suspend_disk(void)
  Enable_cpus:
 	enable_nonboot_cpus();
  Resume_devices:
-	platform_finish();
 	device_resume();
 	resume_console();
+	platform_finish();
  Thaw:
 	unprepare_processes();
  Finish:
Index: linux-2.6.21-rc7/kernel/power/user.c
===================================================================
--- linux-2.6.21-rc7.orig/kernel/power/user.c
+++ linux-2.6.21-rc7/kernel/power/user.c
@@ -170,11 +170,11 @@ static inline int snapshot_suspend(int p
 	}
 	enable_nonboot_cpus();
  Resume_devices:
+	device_resume();
+	resume_console();
 	if (platform_suspend)
 		platform_finish();
 
-	device_resume();
-	resume_console();
  Finish:
 	mutex_unlock(&pm_mutex);
 	return error;
@@ -202,11 +202,11 @@ static inline int snapshot_restore(int p
 
 	enable_nonboot_cpus();
  Resume_devices:
+	device_resume();
+	resume_console();
 	if (platform_suspend)
 		platform_finish();
 
-	device_resume();
-	resume_console();
  Finish:
 	pm_restore_console();
 	mutex_unlock(&pm_mutex);

Re: 2.6.21 suspend-to-disk regression

[Marcus Better]

[-- Attachment #1.1: Type: text/plain, Size: 443 bytes --]

Rafael J. Wysocki wrote:
> Can you please apply the appended patch and see if that helps?

Only tried once so far, but it powered off while writing out the image, 
towards the end. When booting it of course didn't resume, and said the 
partitions were not cleanly shut down.

I think I had similar behaviour once before, so it may not be related to the 
patch. I should probably try more to see if the patch had any effect.

Marcus

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: 2.6.21 suspend-to-disk regression

[Marcus Better]

[-- Attachment #1.1: Type: text/plain, Size: 159 bytes --]

Rafael J. Wysocki wrote:
> Can you please apply the appended patch and see if that helps?

I just had a successful suspend and resume with this patch.

Marcus

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: 2.6.21 suspend-to-disk regression

[Marcus Better]

[-- Attachment #1.1: Type: text/plain, Size: 173 bytes --]

Rafael J. Wysocki wrote:
> Can you please apply the appended patch and see if that helps?

A second attempt also succeeded, so I think it's an improvement at least.

Marcus

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: 2.6.21 suspend-to-disk regression

[Rafael J. Wysocki]

On Monday, 30 April 2007 14:34, Marcus Better wrote:
> Rafael J. Wysocki wrote:
> > Can you please apply the appended patch and see if that helps?
> 
> A second attempt also succeeded, so I think it's an improvement at least.

OK, so please run with it applied for a while to make sure it helps.

Ugh, we seem to have a problem with the code ordering in which
platform_finish() goes before device_resume(), at least in the hibernation
(aka s2disk) case.

This is the second report confirming that the change of this ordering has fixed
the a suspend problem, but the change of this ordering is intrusive, so I was
afraid to do it before 2.6.21.  Can we try to change it now?

Rafael

Re: 2.6.21 suspend-to-disk regression

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 597 bytes --]

On Mon, 2007-04-30 at 16:21 +0200, Rafael J. Wysocki wrote:

> Ugh, we seem to have a problem with the code ordering in which
> platform_finish() goes before device_resume(), at least in the hibernation
> (aka s2disk) case.
> 
> This is the second report confirming that the change of this ordering has fixed
> the a suspend problem, but the change of this ordering is intrusive, so I was
> afraid to do it before 2.6.21.  Can we try to change it now?

I don't care about that ordering at all; ACPI is the only user of
hibernation_ops (which are still part of pm_ops now.)

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: 2.6.21 suspend-to-disk regression

[Pavel Machek]

Hi!

> > Rafael J. Wysocki wrote:
> > > Can you please apply the appended patch and see if that helps?
> > 
> > A second attempt also succeeded, so I think it's an improvement at least.
> 
> OK, so please run with it applied for a while to make sure it helps.
> 
> Ugh, we seem to have a problem with the code ordering in which
> platform_finish() goes before device_resume(), at least in the hibernation
> (aka s2disk) case.

Was not platform_finish before device_resume requirement for MSI
interrupts working?

> This is the second report confirming that the change of this ordering has fixed
> the a suspend problem, but the change of this ordering is intrusive, so I was
> afraid to do it before 2.6.21.  Can we try to change it now?

I guess so.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.21 suspend-to-disk regression

[Marcus Better]

[-- Attachment #1.1: Type: text/plain, Size: 288 bytes --]

Rafael J. Wysocki wrote:
> OK, so please run with it applied for a while to make sure it helps.

I have tested some more, and it seems to work. There was only the one reboot 
during writing that happened on my first attempt, but it hasn't happened 
since then.

Thanks,

Marcus

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: 2.6.21 suspend-to-disk regression

[Rafael J. Wysocki]

On Wednesday, 2 May 2007 13:41, Marcus Better wrote:
> Rafael J. Wysocki wrote:
> > OK, so please run with it applied for a while to make sure it helps.
> 
> I have tested some more, and it seems to work. There was only the one reboot 
> during writing that happened on my first attempt, but it hasn't happened 
> since then.

Could you please test the appended patch instead of the previous one?

Rafael

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

Index: linux-2.6.21/kernel/power/disk.c
===================================================================
--- linux-2.6.21.orig/kernel/power/disk.c	2007-05-02 22:09:55.000000000 +0200
+++ linux-2.6.21/kernel/power/disk.c	2007-05-02 22:18:06.000000000 +0200
@@ -215,7 +215,9 @@ int hibernate(void)
  Enable_cpus:
 	enable_nonboot_cpus();
  Resume_devices:
-	platform_finish();
+	if (error)
+		platform_finish();
+
 	device_resume();
 	resume_console();
  Thaw:
Index: linux-2.6.21/kernel/power/user.c
===================================================================
--- linux-2.6.21.orig/kernel/power/user.c	2007-05-02 22:09:55.000000000 +0200
+++ linux-2.6.21/kernel/power/user.c	2007-05-02 22:18:06.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: 2.6.21 suspend-to-disk regression

[Marcus Better]

[-- Attachment #1.1: Type: text/plain, Size: 289 bytes --]

Rafael J. Wysocki wrote:
> Could you please test the appended patch instead of the previous one?

It doesn't work. The suspend LED flashed for a while but the screen was blank 
and didn't show any messages. It powered down after a while, without writing 
anything to disk.

Marcus

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: 2.6.21 suspend-to-disk regression

[Rafael J. Wysocki]

On Thursday, 3 May 2007 13:41, Marcus Better wrote:
> Rafael J. Wysocki wrote:
> > Could you please test the appended patch instead of the previous one?
> 
> It doesn't work. The suspend LED flashed for a while but the screen was blank 
> and didn't show any messages. It powered down after a while, without writing 
> anything to disk.

Hmm, perhaps we should do something more, like the following:

 drivers/acpi/hardware/hwsleep.c |   22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Index: linux-2.6.21/drivers/acpi/hardware/hwsleep.c
===================================================================
--- linux-2.6.21.orig/drivers/acpi/hardware/hwsleep.c	2007-04-26 05:08:32.000000000 +0200
+++ linux-2.6.21/drivers/acpi/hardware/hwsleep.c	2007-05-03 12:57:22.000000000 +0200
@@ -193,18 +193,13 @@ acpi_status acpi_enter_sleep_state_prep(
 	arg.type = ACPI_TYPE_INTEGER;
 	arg.integer.value = sleep_state;
 
-	/* Run the _PTS and _GTS methods */
+	/* Run the _PTS method */
 
 	status = acpi_evaluate_object(NULL, METHOD_NAME__PTS, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		return_ACPI_STATUS(status);
 	}
 
-	status = acpi_evaluate_object(NULL, METHOD_NAME__GTS, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		return_ACPI_STATUS(status);
-	}
-
 	/* Setup the argument to _SST */
 
 	switch (sleep_state) {
@@ -266,6 +261,8 @@ acpi_status asmlinkage acpi_enter_sleep_
 	u32 PM1Bcontrol;
 	struct acpi_bit_register_info *sleep_type_reg_info;
 	struct acpi_bit_register_info *sleep_enable_reg_info;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
 	u32 in_value;
 	acpi_status status;
 
@@ -312,6 +309,19 @@ acpi_status asmlinkage acpi_enter_sleep_
 		return_ACPI_STATUS(status);
 	}
 
+	/* Execute the _GTS method */
+
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = sleep_state;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+
+	status = acpi_evaluate_object(NULL, METHOD_NAME__GTS, &arg_list, NULL);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+		return_ACPI_STATUS(status);
+	}
+
 	/* Get current value of PM1A control */
 
 	status = acpi_hw_register_read(ACPI_MTX_DO_NOT_LOCK,

Re: 2.6.21 suspend-to-disk regression

[Marcus Better]

[-- Attachment #1.1: Type: text/plain, Size: 382 bytes --]

Rafael J. Wysocki wrote:
> Hmm, perhaps we should do something more, like the following:

No, almost same thing. Screen goes blank, but then there is disk activity 
before poweroff. (That makes me a bit unsure as to whether there was disk 
activity with the previous patch, I might have missed it...) At the next boot 
it doesn't resume, just reboots from scratch.

Marcus

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: 2.6.21 suspend-to-disk regression

[Rafael J. Wysocki]

On Thursday, 3 May 2007 20:08, Marcus Better wrote:
> Rafael J. Wysocki wrote:
> > Hmm, perhaps we should do something more, like the following:
> 
> No, almost same thing. Screen goes blank, but then there is disk activity 
> before poweroff. (That makes me a bit unsure as to whether there was disk 
> activity with the previous patch, I might have missed it...) At the next boot 
> it doesn't resume, just reboots from scratch.

Well, I wish I had your machine here!

If I may ask you to try some more patches, I'd like to check what exactly
makes it work when the code ordering is changed.

Could you please apply the appended patch and see what happens?

Rafael

---
 drivers/acpi/hardware/hwsleep.c |   60 ----------------------------------------
 kernel/power/disk.c             |    4 +-
 kernel/power/user.c             |    8 ++---
 3 files changed, 6 insertions(+), 66 deletions(-)

Index: linux-2.6.21-rc7/kernel/power/disk.c
===================================================================
--- linux-2.6.21-rc7.orig/kernel/power/disk.c
+++ linux-2.6.21-rc7/kernel/power/disk.c
@@ -170,9 +170,9 @@ int pm_suspend_disk(void)
 
 	if (in_suspend) {
 		enable_nonboot_cpus();
-		platform_finish();
 		device_resume();
 		resume_console();
+		platform_finish();
 		pr_debug("PM: writing image.\n");
 		error = swsusp_write();
 		if (!error)
@@ -189,9 +189,9 @@ int pm_suspend_disk(void)
  Enable_cpus:
 	enable_nonboot_cpus();
  Resume_devices:
-	platform_finish();
 	device_resume();
 	resume_console();
+	platform_finish();
  Thaw:
 	unprepare_processes();
  Finish:
Index: linux-2.6.21-rc7/kernel/power/user.c
===================================================================
--- linux-2.6.21-rc7.orig/kernel/power/user.c
+++ linux-2.6.21-rc7/kernel/power/user.c
@@ -170,11 +170,11 @@ static inline int snapshot_suspend(int p
 	}
 	enable_nonboot_cpus();
  Resume_devices:
+	device_resume();
+	resume_console();
 	if (platform_suspend)
 		platform_finish();
 
-	device_resume();
-	resume_console();
  Finish:
 	mutex_unlock(&pm_mutex);
 	return error;
@@ -202,11 +202,11 @@ static inline int snapshot_restore(int p
 
 	enable_nonboot_cpus();
  Resume_devices:
+	device_resume();
+	resume_console();
 	if (platform_suspend)
 		platform_finish();
 
-	device_resume();
-	resume_console();
  Finish:
 	pm_restore_console();
 	mutex_unlock(&pm_mutex);
Index: linux-2.6.21-rc7/drivers/acpi/hardware/hwsleep.c
===================================================================
--- linux-2.6.21-rc7.orig/drivers/acpi/hardware/hwsleep.c
+++ linux-2.6.21-rc7/drivers/acpi/hardware/hwsleep.c
@@ -505,54 +505,6 @@ acpi_status acpi_leave_sleep_state(u8 sl
 
 	ACPI_FUNCTION_TRACE(acpi_leave_sleep_state);
 
-	/*
-	 * Set SLP_TYPE and SLP_EN to state S0.
-	 * This is unclear from the ACPI Spec, but it is required
-	 * by some machines.
-	 */
-	status = acpi_get_sleep_type_data(ACPI_STATE_S0,
-					  &acpi_gbl_sleep_type_a,
-					  &acpi_gbl_sleep_type_b);
-	if (ACPI_SUCCESS(status)) {
-		sleep_type_reg_info =
-		    acpi_hw_get_bit_register_info(ACPI_BITREG_SLEEP_TYPE_A);
-		sleep_enable_reg_info =
-		    acpi_hw_get_bit_register_info(ACPI_BITREG_SLEEP_ENABLE);
-
-		/* Get current value of PM1A control */
-
-		status = acpi_hw_register_read(ACPI_MTX_DO_NOT_LOCK,
-					       ACPI_REGISTER_PM1_CONTROL,
-					       &PM1Acontrol);
-		if (ACPI_SUCCESS(status)) {
-
-			/* Clear SLP_EN and SLP_TYP fields */
-
-			PM1Acontrol &= ~(sleep_type_reg_info->access_bit_mask |
-					 sleep_enable_reg_info->
-					 access_bit_mask);
-			PM1Bcontrol = PM1Acontrol;
-
-			/* Insert SLP_TYP bits */
-
-			PM1Acontrol |=
-			    (acpi_gbl_sleep_type_a << sleep_type_reg_info->
-			     bit_position);
-			PM1Bcontrol |=
-			    (acpi_gbl_sleep_type_b << sleep_type_reg_info->
-			     bit_position);
-
-			/* Just ignore any errors */
-
-			(void)acpi_hw_register_write(ACPI_MTX_DO_NOT_LOCK,
-						     ACPI_REGISTER_PM1A_CONTROL,
-						     PM1Acontrol);
-			(void)acpi_hw_register_write(ACPI_MTX_DO_NOT_LOCK,
-						     ACPI_REGISTER_PM1B_CONTROL,
-						     PM1Bcontrol);
-		}
-	}
-
 	/* Ensure enter_sleep_state_prep -> enter_sleep_state ordering */
 
 	acpi_gbl_sleep_type_a = ACPI_SLEEP_TYPE_INVALID;
@@ -565,18 +517,6 @@ acpi_status acpi_leave_sleep_state(u8 sl
 
 	/* Ignore any errors from these methods */
 
-	arg.integer.value = ACPI_SST_WAKING;
-	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		ACPI_EXCEPTION((AE_INFO, status, "During Method _SST"));
-	}
-
-	arg.integer.value = sleep_state;
-	status = acpi_evaluate_object(NULL, METHOD_NAME__BFS, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		ACPI_EXCEPTION((AE_INFO, status, "During Method _BFS"));
-	}
-
 	status = acpi_evaluate_object(NULL, METHOD_NAME__WAK, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		ACPI_EXCEPTION((AE_INFO, status, "During Method _WAK"));