Suspend to ram regression (2.6.24-rc1-git)

DISCUSSION Jens Axboe 5 participants 15 messages Oct 31, 2007 – Nov 5, 2007 libata

lore

Share & tools

lore

Jens Axboe

author

2007-10-31 20:13 UTC #0 lore raw

Hi,

My x60 stopped suspending about two days ago. It just freezes after printing

Suspending console(s)

where it would normally turn everything off and the 'moon' light would go on. Posting this message in case somebody else knows what is up, if not I'll do a bisect on it tomorrow.

Signature · 1 line

Jens Axboe

Jens Axboe

author

2007-11-01 08:44 UTC #1 lore raw

Quoted 10 lines · Jens Axboe

Hi,
My x60 stopped suspending about two days ago. It just freezes after printing
Suspending console(s)
where it would normally turn everything off and the 'moon' light would go on. Posting this message in case somebody else knows what is up, if not I'll do a bisect on it tomorrow.

Did the bisect, it points to this commit:

1556594f913fa81d008cecfe46d7211c919a853 is first bad commit commit 31556594f913fa81d008cecfe46d7211c919a853

Author: Kristen Carlson Accardi <[email protected]>
Date:   Thu Oct 25 01:33:26 2007 -0400

[libata] AHCI: add hw link power management support

Booting any kernel after this commit fails suspending to ram, it just sits there forever.

Signature · 1 line

Jens Axboe

Jens Axboe

author

2007-11-01 09:24 UTC #2 lore raw

Quoted 16 lines · Jens Axboe

Quoted 10 lines · Jens Axboe
Hi,
My x60 stopped suspending about two days ago. It just freezes after printing
Suspending console(s)
where it would normally turn everything off and the 'moon' light would go on. Posting this message in case somebody else knows what is up, if not I'll do a bisect on it tomorrow.
Did the bisect, it points to this commit:
1556594f913fa81d008cecfe46d7211c919a853 is first bad commit commit 31556594f913fa81d008cecfe46d7211c919a853

Quoted 7 lines

Author: Kristen Carlson Accardi <[email protected]>
Date:   Thu Oct 25 01:33:26 2007 -0400
[libata] AHCI: add hw link power management support
Booting any kernel after this commit fails suspending to ram, it just sits there forever.

Reverting just the default AHCI flags makes it work again. IOW, with the below patch I can suspend properly with current -git.

drivers/ata/ahci.c +1 -2

index ed9b407..77f7631 100644
@@ -190,8 +190,7 @@ enum {
 
 	AHCI_FLAG_COMMON		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 					  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN |
-					  ATA_FLAG_IPM,
+					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN,
 	AHCI_LFLAG_COMMON		= ATA_LFLAG_SKIP_D2H_BSY,
 };

Signature · 1 line

Jens Axboe

Jens Axboe

author

2007-11-01 09:27 UTC #3 lore raw

Quoted 33 lines · Jens Axboe

Quoted 16 lines · Jens Axboe
Quoted 10 lines · Jens Axboe
Hi,
My x60 stopped suspending about two days ago. It just freezes after printing
Suspending console(s)
where it would normally turn everything off and the 'moon' light would go on. Posting this message in case somebody else knows what is up, if not I'll do a bisect on it tomorrow.
Did the bisect, it points to this commit:
1556594f913fa81d008cecfe46d7211c919a853 is first bad commit commit 31556594f913fa81d008cecfe46d7211c919a853
Quoted 7 lines
Author: Kristen Carlson Accardi <[email protected]>
Date:   Thu Oct 25 01:33:26 2007 -0400
[libata] AHCI: add hw link power management support
Booting any kernel after this commit fails suspending to ram, it just sits there forever.
Reverting just the default AHCI flags makes it work again. IOW, with the below patch I can suspend properly with current -git.
drivers/ata/ahci.c +0 -0
index ed9b407..77f7631 100644

drivers/ata/ahci.c enum { · line 195

Quoted 8 lines

drivers/ata/ahci.c +1 -2

@@ -190,8 +190,7 @@ enum {
 
 	AHCI_FLAG_COMMON		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 					  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN |
-					  ATA_FLAG_IPM,
+					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN,
 	AHCI_LFLAG_COMMON		= ATA_LFLAG_SKIP_D2H_BSY,
 };

I should also mention that just stubbing out ahci_enable_alpm() and ahci_disable_alpm() in ahci.c is NOT enough to make it work.

Signature · 1 line

Jens Axboe

Jeff Garzik

2007-11-01 11:20 UTC #4 lore raw

In reply to Jens Axboe

Quoted 8 lines · Jens Axboe

Reverting just the default AHCI flags makes it work again. IOW, with the below patch I can suspend properly with current -git.
drivers/ata/ahci.c +0 -0
index ed9b407..77f7631 100644

drivers/ata/ahci.c enum { · line 194

Quoted 7 lines

drivers/ata/ahci.c +1 -2

@@ -190,8 +190,7 @@ enum {
 
 	AHCI_FLAG_COMMON		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 					  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN |
-					  ATA_FLAG_IPM,
+					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN,
 	AHCI_LFLAG_COMMON		= ATA_LFLAG_SKIP_D2H_BSY,

sounds like the easy thing to do, in light of this breakage, might be to default it to off, add a module parameter turning it on by setting that flag.

Jens Axboe

author

2007-11-01 11:27 UTC #5 lore raw

Quoted 11 lines · Jeff Garzik

Quoted 10 lines · Jens Axboe
Reverting just the default AHCI flags makes it work again. IOW, with the below patch I can suspend properly with current -git.
drivers/ata/ahci.c +0 -1
index ed9b407..77f7631 100644
@@ -190,8 +190,7 @@ enum {
  	AHCI_FLAG_COMMON		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 					  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN |

Quoted 8 lines

drivers/ata/ahci.c enum { · line 194
Quoted 3 lines
drivers/ata/ahci.c +1 -1
-					  ATA_FLAG_IPM,
+					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN,
 	AHCI_LFLAG_COMMON		= ATA_LFLAG_SKIP_D2H_BSY,
sounds like the easy thing to do, in light of this breakage, might be to default it to off, add a module parameter turning it on by setting that flag.

Wouldn't it be better to just get this bug fixed? IOW, is there a reason for disabling ALPM if it's Bug Free?

I'd suggest committing the patch disabling IPM, then Kristen can debug the thing in piece in quiet. Once confident it works with ahci again, we can revert that commit.

Signature · 1 line

Jens Axboe

Jeff Garzik

2007-11-01 11:55 UTC #6 lore raw

Quoted 18 lines · Jens Axboe

Quoted 16 lines · Jeff Garzik
drivers/ata/ahci.c enum { · line 193
Quoted 13 lines · Jens Axboe
Reverting just the default AHCI flags makes it work again. IOW, with the below patch I can suspend properly with current -git.
drivers/ata/ahci.c +1 -2
index ed9b407..77f7631 100644
@@ -190,8 +190,7 @@ enum {
  	AHCI_FLAG_COMMON		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 					  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN |
-					  ATA_FLAG_IPM,
+					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN,
 	AHCI_LFLAG_COMMON		= ATA_LFLAG_SKIP_D2H_BSY,
sounds like the easy thing to do, in light of this breakage, might be to default it to off, add a module parameter turning it on by setting that flag.

Quoted 6 lines

Wouldn't it be better to just get this bug fixed? IOW, is there a reason for disabling ALPM if it's Bug Free?
I'd suggest committing the patch disabling IPM, then Kristen can debug the thing in piece in quiet. Once confident it works with ahci again, we can revert that commit.

Right -- if you are going to commit a patch "disabling" it, it is best to do so via a simple module option, which allows users to easily try the feature in parallel with Intel's debugging.

Jeff

Jens Axboe

author

2007-11-01 12:04 UTC #7 lore raw

Quoted 26 lines · Jeff Garzik

Quoted 19 lines · Jens Axboe
Quoted 10 lines · Jeff Garzik
Quoted 9 lines · Jens Axboe
Reverting just the default AHCI flags makes it work again. IOW, with the below patch I can suspend properly with current -git.
drivers/ata/ahci.c +0 -0
index ed9b407..77f7631 100644
@@ -190,8 +190,7 @@ enum {
  	AHCI_FLAG_COMMON		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 					  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
Quoted 8 lines
drivers/ata/ahci.c enum { · line 193
Quoted 4 lines
drivers/ata/ahci.c +1 -2
-					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN |
-					  ATA_FLAG_IPM,
+					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN,
 	AHCI_LFLAG_COMMON		= ATA_LFLAG_SKIP_D2H_BSY,
sounds like the easy thing to do, in light of this breakage, might be to default it to off, add a module parameter turning it on by setting that flag.
Quoted 5 lines
Wouldn't it be better to just get this bug fixed? IOW, is there a reason for disabling ALPM if it's Bug Free?
I'd suggest committing the patch disabling IPM, then Kristen can debug the thing in piece in quiet. Once confident it works with ahci again, we can revert that commit.

Quoted 3 lines

Right -- if you are going to commit a patch "disabling" it, it is best to do so via a simple module option, which allows users to easily try the feature in parallel with Intel's debugging.

OK, so you just want the option to be temporary? In that case I think a config option is better, since you don't risk breaking peoples setups later when removing the option. That can be quite annoying. Ala the below.

drivers/ata/Kconfig +8 -0

index ba63619..e276ab6 100644
@@ -48,6 +48,14 @@ config SATA_AHCI
 
 	  If unsure, say N.
 
+config SATA_AHCI_IPM
+	bool "AHCI power management"
+	depends on EXPERIMENTAL && SATA_AHCI
+	help
+	  This option adds support for AHCI power management. It current
+	  breaks suspend on some laptops. This option is temporary and will
+	  go away once those issues are fully resolved.
+
 config SATA_SVW
 	tristate "ServerWorks Frodo / Apple K2 SATA support"
 	depends on PCI

drivers/ata/ahci.c +5 -2

index ed9b407..37266ce 100644
@@ -190,8 +190,11 @@ enum {
 
 	AHCI_FLAG_COMMON		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 					  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN |
-					  ATA_FLAG_IPM,
+					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN
+#ifdef CONFIG_SATA_AHCI_IPM
+					  | ATA_FLAG_IPM
+#endif
+					,
 	AHCI_LFLAG_COMMON		= ATA_LFLAG_SKIP_D2H_BSY,
 };

Signature · 1 line

Jens Axboe

Kristen Carlson Accardi

2007-11-01 16:59 UTC #8 lore raw

In reply to Jeff Garzik

Quoted 10 lines · On Thu, 01 Nov 2007 07:20:16 -0400 Jeff Garzik <[email protected]> wrote:

Quoted 9 lines · Jens Axboe
Reverting just the default AHCI flags makes it work again. IOW, with the below patch I can suspend properly with current -git.
drivers/ata/ahci.c +0 -0
index ed9b407..77f7631 100644
@@ -190,8 +190,7 @@ enum {
 

Quoted 12 lines

drivers/ata/ahci.c enum { · line 194
Quoted 6 lines
drivers/ata/ahci.c +1 -2
 	AHCI_FLAG_COMMON		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 					  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN |
-					  ATA_FLAG_IPM,
+					  ATA_FLAG_ACPI_SATA | ATA_FLAG_AN,
 	AHCI_LFLAG_COMMON		= ATA_LFLAG_SKIP_D2H_BSY,
sounds like the easy thing to do, in light of this breakage, might be to default it to off, add a module parameter turning it on by setting that flag.

Can you give me a day to fix it first? I had a similar problem earlier on in the development, and the fix was not that bad. The issue was related to not bringing the link back up to active before doing suspend/resume, and the fix was really straight forward. I'll take a look at this today, and if I can't come up with a pretty fast fix I'll let you know.

Kristen

Kristen Carlson Accardi

2007-11-02 23:38 UTC #9 lore raw

In reply to Jens Axboe

Quoted 16 lines · On Thu, 1 Nov 2007 09:41:46 +0100 Jens Axboe <[email protected]> wrote:

Quoted 10 lines · Jens Axboe
Hi,
My x60 stopped suspending about two days ago. It just freezes after printing
Suspending console(s)
where it would normally turn everything off and the 'moon' light would go on. Posting this message in case somebody else knows what is up, if not I'll do a bisect on it tomorrow.
Did the bisect, it points to this commit:
1556594f913fa81d008cecfe46d7211c919a853 is first bad commit commit 31556594f913fa81d008cecfe46d7211c919a853

Quoted 11 lines

Author: Kristen Carlson Accardi <[email protected]>
Date:   Thu Oct 25 01:33:26 2007 -0400
[libata] AHCI: add hw link power management support
Booting any kernel after this commit fails suspending to ram, it just sits there forever.
Signature · 1 line
Jens Axboe

Does this patch fix your problem? It seems to get hung up while disabling DIPM, and after thinking about this a bit, I don't think we really need to do this anyway.

Don't disable dipm with SET FEATURES

Signed-off-by Kristen Carlson Accardi <[email protected]>

---
 drivers/ata/libata-core.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

drivers/ata/libata-core.c +10 -8

Index: 2.6-git/drivers/ata/libata-core.c
===================================================================
@@ -676,10 +676,11 @@ static int ata_dev_set_dipm(struct ata_d
 		if (rc)
 			return rc;
 
-		/* disable DIPM */
-		if (ata_dev_enabled(dev) && (dev->flags & ATA_DFLAG_DIPM))
-			err_mask = ata_dev_set_feature(dev,
-					SETFEATURES_SATA_DISABLE, SATA_DIPM);
+		/*
+		 * we don't have to disable DIPM since IPM flags
+		 * disallow transitions to SLUMBER, which effectively
+		 * disable DIPM if it does not support PARTIAL
+		 */
 		break;
 	case NOT_AVAILABLE:
 	case MAX_PERFORMANCE:

@@ -689,10 +690,11 @@ static int ata_dev_set_dipm(struct ata_d
 		if (rc)
 			return rc;
 
-		/* disable DIPM */
-		if (ata_dev_enabled(dev) && (dev->flags & ATA_DFLAG_DIPM))
-			err_mask = ata_dev_set_feature(dev,
-					SETFEATURES_SATA_DISABLE, SATA_DIPM);
+		/*
+		 * we don't have to disable DIPM since IPM flags
+		 * disallow all transitions which effectively
+		 * disable DIPM anyway.
+		 */
 		break;
 	}

Theodore Tso

2007-11-03 06:24 UTC #10 lore raw

Quoted 4 lines · Kristen Carlson Accardi

Does this patch fix your problem? It seems to get hung up while disabling DIPM, and after thinking about this a bit, I don't think we really need to do this anyway.

Yep, this fixes suspend/resume on my X61 thinkpad. Interestingly I'm not seeing any power savings with ALPM set to min_power, at least not compared to what I get after suspend and resuming (which mysteriously cause the power utilization of my laptop to drop by a watt --- maybe that is engaging ALPM as part of the suspend process? I dunno).

- Ted

Arjan van de Ven

2007-11-03 17:02 UTC #11 lore raw

Quoted 5 lines · On Sat, 3 Nov 2007 02:23:04 -0400 Theodore Tso <[email protected]> wrote:

Yep, this fixes suspend/resume on my X61 thinkpad. Interestingly I'm not seeing any power savings with ALPM set to min_power, at least not compared to what I get after suspend and resuming (which mysteriously cause the power utilization of my laptop to drop by a watt --- maybe that is engaging ALPM as part of the suspend process? I dunno).

ALPM seems to help only if your disk is idle for 2 seconds or more; unfortunately several standard linux desktops have the disk spin up all the time via all kinds of things.... there's some "standard guilty" ones if you're interested in chasing this down..

Jens Axboe

author

2007-11-05 12:50 UTC #12 lore raw

In reply to Kristen Carlson Accardi

Quoted 27 lines · Kristen Carlson Accardi

Quoted 24 lines · On Thu, 1 Nov 2007 09:41:46 +0100 Jens Axboe <[email protected]> wrote:
Quoted 10 lines · Jens Axboe
Hi,
My x60 stopped suspending about two days ago. It just freezes after printing
Suspending console(s)
where it would normally turn everything off and the 'moon' light would go on. Posting this message in case somebody else knows what is up, if not I'll do a bisect on it tomorrow.
Did the bisect, it points to this commit:
1556594f913fa81d008cecfe46d7211c919a853 is first bad commit commit 31556594f913fa81d008cecfe46d7211c919a853
Author: Kristen Carlson Accardi <[email protected]>
Date:   Thu Oct 25 01:33:26 2007 -0400
[libata] AHCI: add hw link power management support
Booting any kernel after this commit fails suspending to ram, it just sits there forever.

Quoted 7 lines

Quoted 3 lines
Signature · 1 line
Jens Axboe
Does this patch fix your problem? It seems to get hung up while disabling DIPM, and after thinking about this a bit, I don't think we really need to do this anyway.

Yep, works for me! Can we get this expedited upstream?

Signature · 1 line

Jens Axboe

Jeff Garzik

2007-11-05 12:54 UTC #13 lore raw

Quoted 27 lines · Jens Axboe

Quoted 26 lines · Kristen Carlson Accardi
Quoted 23 lines · On Thu, 1 Nov 2007 09:41:46 +0100 Jens Axboe <[email protected]> wrote:
Quoted 10 lines · Jens Axboe
Hi,
My x60 stopped suspending about two days ago. It just freezes after printing
Suspending console(s)
where it would normally turn everything off and the 'moon' light would go on. Posting this message in case somebody else knows what is up, if not I'll do a bisect on it tomorrow.
Did the bisect, it points to this commit:
1556594f913fa81d008cecfe46d7211c919a853 is first bad commit commit 31556594f913fa81d008cecfe46d7211c919a853
Author: Kristen Carlson Accardi <[email protected]>
Date:   Thu Oct 25 01:33:26 2007 -0400
[libata] AHCI: add hw link power management support
Booting any kernel after this commit fails suspending to ram, it just sits there forever.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Signature · 1 line
Jens Axboe
Does this patch fix your problem? It seems to get hung up while disabling DIPM, and after thinking about this a bit, I don't think we really need to do this anyway.
Yep, works for me! Can we get this expedited upstream?

If everybody's happy with it, I've got it ready for upstream in libata-dev.git#dlpm-fix

Jeff

Kristen Carlson Accardi

2007-11-05 17:22 UTC #14 lore raw

Quoted 6 lines · On Mon, 05 Nov 2007 07:53:58 -0500 Jeff Garzik <[email protected]> wrote:

Quoted 1 line
Yep, works for me! Can we get this expedited upstream?
If everybody's happy with it, I've got it ready for upstream in libata-dev.git#dlpm-fix
Jeff

Thanks! It'd be great if you could get it in ASAP.