Suspend to ram regression (2.6.24-rc1-git)

Suspend to ram regression (2.6.24-rc1-git)

[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.

-- 
Jens Axboe

Re: Suspend to ram regression (2.6.24-rc1-git)

[Jens Axboe]

On Wed, Oct 31 2007, Jens Axboe wrote:
> 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 <kristen.c.accardi@intel.com>
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.

-- 
Jens Axboe

Re: Suspend to ram regression (2.6.24-rc1-git)

[Jens Axboe]

On Thu, Nov 01 2007, Jens Axboe wrote:
> On Wed, Oct 31 2007, Jens Axboe wrote:
> > 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 <kristen.c.accardi@intel.com>
> 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.

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index ed9b407..77f7631 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -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,
 };
 

-- 
Jens Axboe

Re: Suspend to ram regression (2.6.24-rc1-git)

[Jens Axboe]

On Thu, Nov 01 2007, Jens Axboe wrote:
> On Thu, Nov 01 2007, Jens Axboe wrote:
> > On Wed, Oct 31 2007, Jens Axboe wrote:
> > > 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 <kristen.c.accardi@intel.com>
> > 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.
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index ed9b407..77f7631 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -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.

-- 
Jens Axboe

Re: Suspend to ram regression (2.6.24-rc1-git)

[Jeff Garzik]

Jens Axboe wrote:
> Reverting just the default AHCI flags makes it work again. IOW, with the
> below patch I can suspend properly with current -git.
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index ed9b407..77f7631 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -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.

Re: Suspend to ram regression (2.6.24-rc1-git)

[Jens Axboe]

On Thu, Nov 01 2007, Jeff Garzik wrote:
> Jens Axboe wrote:
>> Reverting just the default AHCI flags makes it work again. IOW, with the
>> below patch I can suspend properly with current -git.
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index ed9b407..77f7631 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -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.

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.

-- 
Jens Axboe

Re: Suspend to ram regression (2.6.24-rc1-git)

[Jeff Garzik]

Jens Axboe wrote:
> On Thu, Nov 01 2007, Jeff Garzik wrote:
>> Jens Axboe wrote:
>>> Reverting just the default AHCI flags makes it work again. IOW, with the
>>> below patch I can suspend properly with current -git.
>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>> index ed9b407..77f7631 100644
>>> --- a/drivers/ata/ahci.c
>>> +++ b/drivers/ata/ahci.c
>>> @@ -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.
> 
> 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

Re: Suspend to ram regression (2.6.24-rc1-git)

[Jens Axboe]

On Thu, Nov 01 2007, Jeff Garzik wrote:
> Jens Axboe wrote:
>> On Thu, Nov 01 2007, Jeff Garzik wrote:
>>> Jens Axboe wrote:
>>>> Reverting just the default AHCI flags makes it work again. IOW, with the
>>>> below patch I can suspend properly with current -git.
>>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>>> index ed9b407..77f7631 100644
>>>> --- a/drivers/ata/ahci.c
>>>> +++ b/drivers/ata/ahci.c
>>>> @@ -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.
>> 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.

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.

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index ba63619..e276ab6 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -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
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index ed9b407..37266ce 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -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,
 };
 

-- 
Jens Axboe

Re: Suspend to ram regression (2.6.24-rc1-git)

[Kristen Carlson Accardi]

On Thu, 01 Nov 2007 07:20:16 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Jens Axboe wrote:
> > Reverting just the default AHCI flags makes it work again. IOW, with the
> > below patch I can suspend properly with current -git.
> > 
> > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> > index ed9b407..77f7631 100644
> > --- a/drivers/ata/ahci.c
> > +++ b/drivers/ata/ahci.c
> > @@ -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.
> 

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

Re: Suspend to ram regression (2.6.24-rc1-git)

[Kristen Carlson Accardi]

On Thu, 1 Nov 2007 09:41:46 +0100
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Wed, Oct 31 2007, Jens Axboe wrote:
> > 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 <kristen.c.accardi@intel.com>
> 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.
> 
> -- 
> 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 <kristen.c.accardi@intel.com>

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

Index: 2.6-git/drivers/ata/libata-core.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-core.c	2007-11-01 09:59:17.000000000 -0700
+++ 2.6-git/drivers/ata/libata-core.c	2007-11-02 16:35:13.000000000 -0700
@@ -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;
 	}
 

Re: Suspend to ram regression (2.6.24-rc1-git)

[Theodore Tso]

On Fri, Nov 02, 2007 at 04:37:08PM -0700, Kristen Carlson Accardi wrote:
> 
> 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

Re: Suspend to ram regression (2.6.24-rc1-git)

[Arjan van de Ven]

On Sat, 3 Nov 2007 02:23:04 -0400
Theodore Tso <tytso@mit.edu> 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..

Re: Suspend to ram regression (2.6.24-rc1-git)

[Jens Axboe]

On Fri, Nov 02 2007, Kristen Carlson Accardi wrote:
> On Thu, 1 Nov 2007 09:41:46 +0100
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Wed, Oct 31 2007, Jens Axboe wrote:
> > > 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 <kristen.c.accardi@intel.com>
> > 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.
> > 
> > -- 
> > 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?

-- 
Jens Axboe

Re: Suspend to ram regression (2.6.24-rc1-git)

[Jeff Garzik]

Jens Axboe wrote:
> On Fri, Nov 02 2007, Kristen Carlson Accardi wrote:
>> On Thu, 1 Nov 2007 09:41:46 +0100
>> Jens Axboe <jens.axboe@oracle.com> wrote:
>>
>>> On Wed, Oct 31 2007, Jens Axboe wrote:
>>>> 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 <kristen.c.accardi@intel.com>
>>> 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.
>>>
>>> -- 
>>> 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

Re: Suspend to ram regression (2.6.24-rc1-git)

[Kristen Carlson Accardi]

On Mon, 05 Nov 2007 07:53:58 -0500
Jeff Garzik <jeff@garzik.org> wrote:

> > 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.