LinuxLists.cc - 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

On Thursday 20 September 2007, you wrote:
> > When compared with 2.6.22-4, dmesg no longer lists S4 and S5 as
> > supported for my Toshiba Satellite A40 laptop (Mobile Intel Pentium 4,
> > 2.8GHz).
> >
> > -Linux version 2.6.22-2-686 (Debian 2.6.22-4) ([email protected]) ...
> > +Linux version 2.6.23-rc6 (root@faramir) ...
> > [...]
> > -ACPI: (supports S0 S3 S4 S5)
> > +ACPI: (supports S0 S3)
>
> Unexpected, and potentially pretty serious. Something went wrong with
> ACPI. Can you try to narrow down when it started happening?

rc1 still had all 4 levels. I'll run a bisect between rc1 and rc6.

2007-09-20 16:03:20

by Maciej Rutecki

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

Frans Pop pisze:
> On Thursday 20 September 2007, you wrote:
>>> When compared with 2.6.22-4, dmesg no longer lists S4 and S5 as
>>> supported for my Toshiba Satellite A40 laptop (Mobile Intel Pentium 4,
>>> 2.8GHz).
>>>
>>> -Linux version 2.6.22-2-686 (Debian 2.6.22-4) ([email protected]) ...
>>> +Linux version 2.6.23-rc6 (root@faramir) ...
>>> [...]
>>> -ACPI: (supports S0 S3 S4 S5)
>>> +ACPI: (supports S0 S3)
>> Unexpected, and potentially pretty serious. Something went wrong with
>> ACPI. Can you try to narrow down when it started happening?
>
> rc1 still had all 4 levels. I'll run a bisect between rc1 and rc6.

I have the same on HP/Compaq nx6310:
ACPI: (supports S0 S3)

(kernel 2.6.23-rc6)

But suspend to ram/disk works.

2007-09-20 16:33:52

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

On Thursday 20 September 2007, Frans Pop wrote:
> On Thursday 20 September 2007, you wrote:
> > > When compared with 2.6.22-4, dmesg no longer lists S4 and S5 as
> > > supported for my Toshiba Satellite A40 laptop (Mobile Intel Pentium
> > > 4, 2.8GHz).
> > >
> > > -Linux version 2.6.22-2-686 (Debian 2.6.22-4) ([email protected]) ...
> > > +Linux version 2.6.23-rc6 (root@faramir) ...
> > > [...]
> > > -ACPI: (supports S0 S3 S4 S5)
> > > +ACPI: (supports S0 S3)
> >
> > Unexpected, and potentially pretty serious. Something went wrong with
> > ACPI. Can you try to narrow down when it started happening?
>
> rc1 still had all 4 levels. I'll run a bisect between rc1 and rc6.

I've bisected it down to this trio of changes:
- b0cb1a19d05b8ea8 Replace CONFIG_SOFTWARE_SUSPEND with CONFIG_HIBERNATION
- 296699de6bdc7171 Introduce CONFIG_SUSPEND for suspend-to-Ram and standby
- 673d5b43daa00b42 ACPI: restore CONFIG_ACPI_SLEEP

The actual change is probably in the second of those.

I've seen the comment that the change may just be cosmetic, but I'll leave
it to others to confirm that (and if so, judge if it is desirable or not).

Cheers,
Frans Pop

2007-09-20 16:34:08

by Maciej Rutecki

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

Frans Pop pisze:
>>> [...]
>>> -ACPI: (supports S0 S3 S4 S5)
>>> +ACPI: (supports S0 S3)
>> Unexpected, and potentially pretty serious. Something went wrong with
>> ACPI. Can you try to narrow down when it started happening?
>
> rc1 still had all 4 levels. I'll run a bisect between rc1 and rc6.

2.6.23-rc1 OK (supports S0 S3 S4 S5)
2.6.23-rc2 wrong: (supports S0 S3)

2007-09-20 16:34:52

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

Maciek Rutecki wrote:
> Frans Pop pisze:
>> On Thursday 20 September 2007, you wrote:
>>>> When compared with 2.6.22-4, dmesg no longer lists S4 and S5 as
>>>> supported for my Toshiba Satellite A40 laptop (Mobile Intel Pentium 4,
>>>> 2.8GHz).
>>>>
>>>> -Linux version 2.6.22-2-686 (Debian 2.6.22-4) ([email protected]) ...
>>>> +Linux version 2.6.23-rc6 (root@faramir) ...
>>>> [...]
>>>> -ACPI: (supports S0 S3 S4 S5)
>>>> +ACPI: (supports S0 S3)
>>> Unexpected, and potentially pretty serious. Something went wrong with
>>> ACPI. Can you try to narrow down when it started happening?
>> rc1 still had all 4 levels. I'll run a bisect between rc1 and rc6.
>
> I have the same on HP/Compaq nx6310:
> ACPI: (supports S0 S3)
This is due to Rafael' split of suspend from hibernation.
namely 296699de6bdc717189a331ab6bbe90e05c94db06.
Detection of S4 lacks printk(), so even if S4 is supported, it will not be reported.
Adding Rafael to the discussion :)

Regards,
Alex.

2007-09-20 16:59:57

by Romano Giannetti

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on ToshibaSatellite A40

On Thu, 2007-09-20 at 18:01 +0200, Maciek Rutecki wrote:
> Frans Pop pisze:

> >> Unexpected, and potentially pretty serious. Something went wrong with
> >> ACPI. Can you try to narrow down when it started happening?
> >
> > rc1 still had all 4 levels. I'll run a bisect between rc1 and rc6.
>
> I have the same on HP/Compaq nx6310:
> ACPI: (supports S0 S3)
>
> (kernel 2.6.23-rc6)

Linux version 2.6.20-16-generic (ubuntu):
ACPI: (supports S0 S3 S4 S5)

Linux version 2.6.23-rc2-rg (with a little snd-hda-intel patch)
ACPI: (supports S0 S3)

So it seems happened between rc1 and rc2

> But suspend to ram/disk works.

For me too (with s2ram, not with vanilla echo ram > ...). And the system
is otherwise ok.

Romano

> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
La presente comunicación tiene carácter confidencial y es para el exclusivo uso del destinatario indicado en la misma. Si Ud. no es el destinatario indicado, le informamos que cualquier forma de distribución, reproducción o uso de esta comunicación y/o de la información contenida en la misma están estrictamente prohibidos por la ley. Si Ud. ha recibido esta comunicación por error, por favor, notifíquelo inmediatamente al remitente contestando a este mensaje y proceda a continuación a destruirlo. Gracias por su colaboración.

This communication contains confidential information. It is for the exclusive use of the intended addressee. If you are not the intended addressee, please note that any form of distribution, copying or use of this communication or the information in it is strictly prohibited by law. If you have received this communication in error, please immediately notify the sender by reply e-mail and destroy this message. Thank you for your cooperation.

2007-09-20 17:30:16

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

Please try this patch.

Regards,
Alex.

Attachments:

consolidate_Sx_handling.patch (5.38 kB)

2007-09-20 18:18:16

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

On Thursday 20 September 2007, you wrote:
> Please try this patch.

Works. All states are now listed again.
I've not tested suspend to disk, but suspend to ram and power off work fine.

> +printk(KERN_INFO PREFIX "(supports");
> #ifdef CONFIG_SUSPEND
> - printk(KERN_INFO PREFIX "(supports");
> for (i = ACPI_STATE_S0; i < ACPI_STATE_S4; i++) {

Isn't there a risk now that we now end up printing
ACPI: (supports)
if CONFIG_SUSPEND is not enabled and >S4 is not supported?

Or, more probably, it would print
ACPI: (supports S5)
as it is unlikely that "off" is not supported :-)

Maybe S0 should be taken outside the #ifdef and the loop as that state is
also basically always there?

Thanks,
FJP

2007-09-20 18:34:15

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

Frans Pop wrote:
> On Thursday 20 September 2007, you wrote:
>> Please try this patch.
>
> Works. All states are now listed again.
> I've not tested suspend to disk, but suspend to ram and power off work fine.
>
>> +printk(KERN_INFO PREFIX "(supports");
>> #ifdef CONFIG_SUSPEND
>> - printk(KERN_INFO PREFIX "(supports");
>> for (i = ACPI_STATE_S0; i < ACPI_STATE_S4; i++) {
>
> Isn't there a risk now that we now end up printing
> ACPI: (supports)
> if CONFIG_SUSPEND is not enabled and >S4 is not supported?
>
> Or, more probably, it would print
> ACPI: (supports S5)
Don't know what does it mean to support S0 exactly... :)
> as it is unlikely that "off" is not supported :-)
>
> Maybe S0 should be taken outside the #ifdef and the loop as that state is
> also basically always there?
Don't think it is worth the trouble. We already have this loop almost completely unrolled,
let's not make it complete mess...

Regards,
Alex.
>
> Thanks,
> FJP
>

2007-09-20 19:48:28

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

On Thursday, 20 September 2007 18:34, Alexey Starikovskiy wrote:
> Maciek Rutecki wrote:
> > Frans Pop pisze:
> >> On Thursday 20 September 2007, you wrote:
> >>>> When compared with 2.6.22-4, dmesg no longer lists S4 and S5 as
> >>>> supported for my Toshiba Satellite A40 laptop (Mobile Intel Pentium 4,
> >>>> 2.8GHz).
> >>>>
> >>>> -Linux version 2.6.22-2-686 (Debian 2.6.22-4) ([email protected]) ...
> >>>> +Linux version 2.6.23-rc6 (root@faramir) ...
> >>>> [...]
> >>>> -ACPI: (supports S0 S3 S4 S5)
> >>>> +ACPI: (supports S0 S3)
> >>> Unexpected, and potentially pretty serious. Something went wrong with
> >>> ACPI. Can you try to narrow down when it started happening?
> >> rc1 still had all 4 levels. I'll run a bisect between rc1 and rc6.
> >
> > I have the same on HP/Compaq nx6310:
> > ACPI: (supports S0 S3)
> This is due to Rafael' split of suspend from hibernation.
> namely 296699de6bdc717189a331ab6bbe90e05c94db06.
> Detection of S4 lacks printk(), so even if S4 is supported, it will not be reported.
> Adding Rafael to the discussion :)

Thanks, I've already spotted the fix patch.

Greetings,
Rafael

2007-09-20 20:02:22

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

On Thursday, 20 September 2007 20:33, Alexey Starikovskiy wrote:
> Frans Pop wrote:
> > On Thursday 20 September 2007, you wrote:
> >> Please try this patch.
> >
> > Works. All states are now listed again.
> > I've not tested suspend to disk, but suspend to ram and power off work fine.
> >
> >> +printk(KERN_INFO PREFIX "(supports");
> >> #ifdef CONFIG_SUSPEND
> >> - printk(KERN_INFO PREFIX "(supports");
> >> for (i = ACPI_STATE_S0; i < ACPI_STATE_S4; i++) {
> >
> > Isn't there a risk now that we now end up printing
> > ACPI: (supports)
> > if CONFIG_SUSPEND is not enabled and >S4 is not supported?
> >
> > Or, more probably, it would print
> > ACPI: (supports S5)
> Don't know what does it mean to support S0 exactly... :)
> > as it is unlikely that "off" is not supported :-)
> >
> > Maybe S0 should be taken outside the #ifdef and the loop as that state is
> > also basically always there?
> Don't think it is worth the trouble. We already have this loop almost completely unrolled,
> let's not make it complete mess...

Well, you could use "(supports S0" instead of just "(supports". ;-)

Greetings,
Rafael

2007-09-20 20:32:46

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

On Thursday 20 September 2007, Rafael J. Wysocki wrote:
> On Thursday, 20 September 2007 20:33, Alexey Starikovskiy wrote:
> > Frans Pop wrote:
> > > On Thursday 20 September 2007, you wrote:
> > >> Please try this patch.
> > >
> > > Works. All states are now listed again.
> > > I've not tested suspend to disk, but suspend to ram and power off
> > > work fine.
> > >
> > >> +printk(KERN_INFO PREFIX "(supports");

Note that this printk should be indented.

> > >> #ifdef CONFIG_SUSPEND
> > >> - printk(KERN_INFO PREFIX "(supports");
> > >> for (i = ACPI_STATE_S0; i < ACPI_STATE_S4; i++) {
> > >
> > > Isn't there a risk now that we now end up printing
> > > ACPI: (supports)
> > > if CONFIG_SUSPEND is not enabled and >S4 is not supported?
> > >
> > > Or, more probably, it would print
> > > ACPI: (supports S5)
> >
> > Don't know what does it mean to support S0 exactly... :)

Agreed, though arguably the same goes for S5. I guess you could say they are
all states that can be switched to.

> > > as it is unlikely that "off" is not supported :-)
> > >
> > > Maybe S0 should be taken outside the #ifdef and the loop as that
> > > state is also basically always there?
> >
> > Don't think it is worth the trouble. We already have this loop almost
> > completely unrolled, let's not make it complete mess...
>
> Well, you could use "(supports S0" instead of just "(supports". ;-)

After thinking about this a bit more, I think this does make sense for three
(admittedly minor) reasons:
- consistency between messages with and without CONFIG_SUSPEND
- consistency with /proc/acpi/sleep
- avoiding unnecessary change from previous versions.

Please consider the attached patch which applies on top of Alexey's. Feel
free to integrate it in his patch.

Signed-off-by: Frans Pop <[email protected]>

Attachments:

(No filename) (1.78 kB)
consolidate_Sx_handling_addendum.patch (772.00 B)
Download all attachments

2007-09-20 20:40:43

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

On Thursday, 20 September 2007 22:32, Frans Pop wrote:
> On Thursday 20 September 2007, Rafael J. Wysocki wrote:
> > On Thursday, 20 September 2007 20:33, Alexey Starikovskiy wrote:
> > > Frans Pop wrote:
> > > > On Thursday 20 September 2007, you wrote:
> > > >> Please try this patch.
> > > >
> > > > Works. All states are now listed again.
> > > > I've not tested suspend to disk, but suspend to ram and power off
> > > > work fine.
> > > >
> > > >> +printk(KERN_INFO PREFIX "(supports");
>
> Note that this printk should be indented.
>
> > > >> #ifdef CONFIG_SUSPEND
> > > >> - printk(KERN_INFO PREFIX "(supports");
> > > >> for (i = ACPI_STATE_S0; i < ACPI_STATE_S4; i++) {
> > > >
> > > > Isn't there a risk now that we now end up printing
> > > > ACPI: (supports)
> > > > if CONFIG_SUSPEND is not enabled and >S4 is not supported?
> > > >
> > > > Or, more probably, it would print
> > > > ACPI: (supports S5)
> > >
> > > Don't know what does it mean to support S0 exactly... :)
>
> Agreed, though arguably the same goes for S5. I guess you could say they are
> all states that can be switched to.
>
> > > > as it is unlikely that "off" is not supported :-)
> > > >
> > > > Maybe S0 should be taken outside the #ifdef and the loop as that
> > > > state is also basically always there?
> > >
> > > Don't think it is worth the trouble. We already have this loop almost
> > > completely unrolled, let's not make it complete mess...
> >
> > Well, you could use "(supports S0" instead of just "(supports". ;-)
>
> After thinking about this a bit more, I think this does make sense for three
> (admittedly minor) reasons:
> - consistency between messages with and without CONFIG_SUSPEND
> - consistency with /proc/acpi/sleep
> - avoiding unnecessary change from previous versions.
>
> Please consider the attached patch which applies on top of Alexey's. Feel
> free to integrate it in his patch.
>
> Signed-off-by: Frans Pop <[email protected]>

Alexey, do you agree?

(patch reproduced below for convenience).

diff --git a/drivers/acpi/sleep/main.c b/drivers/acpi/sleep/main.c
index 638172f..85633c5 100644
--- a/drivers/acpi/sleep/main.c
+++ b/drivers/acpi/sleep/main.c
@@ -401,9 +401,11 @@ int __init acpi_sleep_init(void)
if (acpi_disabled)
return 0;

-printk(KERN_INFO PREFIX "(supports");
+ sleep_states[ACPI_STATE_S0] = 1;
+ printk(KERN_INFO PREFIX "(supports S0");
+
#ifdef CONFIG_SUSPEND
- for (i = ACPI_STATE_S0; i < ACPI_STATE_S4; i++) {
+ for (i = ACPI_STATE_S1; i < ACPI_STATE_S4; i++) {
status = acpi_get_sleep_type_data(i, &type_a, &type_b);
if (ACPI_SUCCESS(status)) {
sleep_states[i] = 1;

2007-09-20 21:18:56

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

On Thursday 20 September 2007, Frans Pop wrote:
> On Thursday 20 September 2007, Rafael J. Wysocki wrote:
> > On Thursday, 20 September 2007 20:33, Alexey Starikovskiy wrote:
> > > Frans Pop wrote:
> > > > Maybe S0 should be taken outside the #ifdef and the loop as that
> > > > state is also basically always there?
> > >
> > > Don't think it is worth the trouble. We already have this loop almost
> > > completely unrolled, let's not make it complete mess...
> >
> > Well, you could use "(supports S0" instead of just "(supports". ;-)
>
> After thinking about this a bit more, I think this does make sense for
> three (admittedly minor) reasons:
> - consistency between messages with and without CONFIG_SUSPEND
> - consistency with /proc/acpi/sleep
> - avoiding unnecessary change from previous versions.

One additional reason...

With current code, sleep_states(0) will not be set if compiled without
CONFIG_SUSPEND, which means that S0 will also disappear from
/proc/acpi/sleep. With pre-2.6.23 code it would be listed there.

I think that is even a more relevant change than the display issue and could
possibly even be considered a user-space interface regression.
My proposed patch fixes that too.

Note that the patch currently does not call acpi_get_sleep_type_data for S0,
but (partially from Rafael's comment) I was assuming that for S0 that does
not really matter. If it does, then the patch could easily be adjusted to
include that.

2007-09-21 14:05:16

[permalink] [raw]

Subject: Re: 2.6.23-rc6: S4 and S5 no longer listed as supported on Toshiba Satellite A40

Rafael J. Wysocki wrote:
> On Thursday, 20 September 2007 22:32, Frans Pop wrote:
>> On Thursday 20 September 2007, Rafael J. Wysocki wrote:
>>> On Thursday, 20 September 2007 20:33, Alexey Starikovskiy wrote:
>>>> Frans Pop wrote:
>>>>> On Thursday 20 September 2007, you wrote:
>>>>>> Please try this patch.
>>>>> Works. All states are now listed again.
>>>>> I've not tested suspend to disk, but suspend to ram and power off
>>>>> work fine.
>>>>>
>>>>>> +printk(KERN_INFO PREFIX "(supports");
>> Note that this printk should be indented.
>>
>>>>>> #ifdef CONFIG_SUSPEND
>>>>>> - printk(KERN_INFO PREFIX "(supports");
>>>>>> for (i = ACPI_STATE_S0; i < ACPI_STATE_S4; i++) {
>>>>> Isn't there a risk now that we now end up printing
>>>>> ACPI: (supports)
>>>>> if CONFIG_SUSPEND is not enabled and >S4 is not supported?
>>>>>
>>>>> Or, more probably, it would print
>>>>> ACPI: (supports S5)
>>>> Don't know what does it mean to support S0 exactly... :)
>> Agreed, though arguably the same goes for S5. I guess you could say they are
>> all states that can be switched to.
>>
>>>>> as it is unlikely that "off" is not supported :-)
>>>>>
>>>>> Maybe S0 should be taken outside the #ifdef and the loop as that
>>>>> state is also basically always there?
>>>> Don't think it is worth the trouble. We already have this loop almost
>>>> completely unrolled, let's not make it complete mess...
>>> Well, you could use "(supports S0" instead of just "(supports". ;-)
>> After thinking about this a bit more, I think this does make sense for three
>> (admittedly minor) reasons:
>> - consistency between messages with and without CONFIG_SUSPEND
>> - consistency with /proc/acpi/sleep
>> - avoiding unnecessary change from previous versions.
>>
>> Please consider the attached patch which applies on top of Alexey's. Feel
>> free to integrate it in his patch.
>>
>> Signed-off-by: Frans Pop <[email protected]>
>
> Alexey, do you agree?
Yes, was thinking to do it myself, but my ISP died this morning....

Regards,
Alex.
>
> (patch reproduced below for convenience).
>
> diff --git a/drivers/acpi/sleep/main.c b/drivers/acpi/sleep/main.c
> index 638172f..85633c5 100644
> --- a/drivers/acpi/sleep/main.c
> +++ b/drivers/acpi/sleep/main.c
> @@ -401,9 +401,11 @@ int __init acpi_sleep_init(void)
> if (acpi_disabled)
> return 0;
>
> -printk(KERN_INFO PREFIX "(supports");
> + sleep_states[ACPI_STATE_S0] = 1;
> + printk(KERN_INFO PREFIX "(supports S0");
> +
> #ifdef CONFIG_SUSPEND
> - for (i = ACPI_STATE_S0; i < ACPI_STATE_S4; i++) {
> + for (i = ACPI_STATE_S1; i < ACPI_STATE_S4; i++) {
> status = acpi_get_sleep_type_data(i, &type_a, &type_b);
> if (ACPI_SUCCESS(status)) {
> sleep_states[i] = 1;
>

2007-09-21 14:20:25