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
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 <[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.
--
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 <[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.
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
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 <[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.
>
> 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
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.
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
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
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
On Thu, 01 Nov 2007 07:20:16 -0400
Jeff Garzik <[email protected]> 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
On Thu, 1 Nov 2007 09:41:46 +0100
Jens Axboe <[email protected]> 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 <[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.
>
> --
> 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(-)
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;
}
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
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..
On Fri, Nov 02 2007, Kristen Carlson Accardi wrote:
> On Thu, 1 Nov 2007 09:41:46 +0100
> Jens Axboe <[email protected]> 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 <[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.
> >
> > --
> > 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
Jens Axboe wrote:
> On Fri, Nov 02 2007, Kristen Carlson Accardi wrote:
>> On Thu, 1 Nov 2007 09:41:46 +0100
>> Jens Axboe <[email protected]> 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 <[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.
>>>
>>> --
>>> 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
On Mon, 05 Nov 2007 07:53:58 -0500
Jeff Garzik <[email protected]> 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.