Bug 4320 - S4 reboots machine when AC is connected
Summary: S4 reboots machine when AC is connected
Status: CLOSED CODE_FIX
Alias: None
Product: ACPI
Classification: Unclassified
Component: Power-Sleep-Wake (show other bugs)
Hardware: i386 Linux
: P2 normal
Assignee: Alexey Starikovskiy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-03-09 10:23 UTC by Pierre Ossman
Modified: 2006-02-02 14:30 UTC (History)
2 users (show)

See Also:
Kernel Version: 2.6.11
Tree: Mainline
Regression: ---


Attachments
DSDT (164.94 KB, text/plain)
2005-03-28 22:21 UTC, Pierre Ossman
Details
Patch to set system_state at S4 power-off (345 bytes, patch)
2005-09-01 06:53 UTC, Alexey Starikovskiy
Details | Diff
Changes to PM subsystem to call ACPI (more) consistently (3.97 KB, patch)
2005-11-24 09:21 UTC, Alexey Starikovskiy
Details | Diff
Updated patch (4.73 KB, patch)
2005-11-30 03:46 UTC, Alexey Starikovskiy
Details | Diff
Updated patch with touching gpe in S4 (5.05 KB, patch)
2005-11-30 05:48 UTC, Alexey Starikovskiy
Details | Diff
Add proper handling of GPEs (6.36 KB, patch)
2005-11-30 08:00 UTC, Alexey Starikovskiy
Details | Diff
Small & pretty patch (5.07 KB, patch)
2005-12-01 03:20 UTC, Alexey Starikovskiy
Details | Diff
Last patch with Pavels' suggestions (5.17 KB, patch)
2005-12-01 04:29 UTC, Alexey Starikovskiy
Details | Diff

Description Pierre Ossman 2005-03-09 10:23:08 UTC
Distribution: Fedora Core 3
Hardware Environment: HP/Compaq nx7010
Problem Description:

When S4 is entered and ac is connected then the machine reboots instead of
powering down.
Behaviour is identical to that of bug 3909 and bug 3669. So an obvious first
patch is the same fix for S4 (if that's possible).

I haven't been playing around with S4 previously so I do not know if this is a
new bug or something that's always been there.
Comment 1 Shaohua 2005-03-09 17:33:24 UTC
How about the 'platform' method of S4? Did you test latest kernel?
Comment 2 Pierre Ossman 2005-03-10 01:53:00 UTC
'platform' has the same effect. (Doesn't that evaluate to S4 when ACPI handles PM?)

Kernel is 2.6.11.
Comment 3 Andrew Morton 2005-03-21 15:42:20 UTC
Pierre, was this actually a regression?  Did earlier kernels work OK?

If so, which ones?

Thanks.
Comment 4 Shaohua 2005-03-21 17:04:18 UTC
Is shutdown (I mean S5) ok in this laptop? If shutdown ok, S4 platform method 
should be ok to me.
Comment 5 Pierre Ossman 2005-03-28 04:08:28 UTC
I don't know if this is a regression since I haven't been using S4 previously. I
can do a quick test with an older kernel though. Which version(s) is relevant?

Shutdown (S5) is ok now, but it behaved in precisely the same way as this bug
before bug 3669 was fixed. I did an attempt at mimicing that fix for S4 but it
didn't have any effect. The structure isn't identical though so I might have put
it in the wrong place.
Comment 6 Shaohua 2005-03-28 16:51:16 UTC
Could you please attach your 'acpidmp' and 'cat /proc/acpi/wakeup' output? I'd 
like to check them.
Comment 7 Pierre Ossman 2005-03-28 22:21:40 UTC
Created attachment 4811 [details]
DSDT

/proc/acpi/wakeup:

Device	Sleep state	Status
C058	   5		disabled
C1AD	   3		disabled
C1A3	   3		disabled
C1A4	   3		disabled
C0AC	   3		disabled
C0B3	   3		disabled
C0B4	   3		disabled
C0B5	   3		disabled
C0E7	   3		disabled
C136	   3		*enabled
Comment 8 Shaohua 2005-03-29 18:43:01 UTC
S4 (shutdown method) does exactly a S5, so it's very strange why S5 survive 
but S4. Could you please try disable acpi for S4? S4 works even without ACPI.
Comment 9 Pierre Ossman 2005-04-02 13:27:39 UTC
I might have gotten this wrong but isn't S4 handled via
acpi_pm_prepare()/acpi_pm_enter() and S5 via acpi_power_off()?

How am I supposed to enter S4 with ACPI disabled? /proc/acpi does not exist and
/sys/power/disk does not accept 'platform' when ACPI isn't up and running.
Comment 10 Shaohua 2005-04-03 23:38:06 UTC
You can directly use 'shutdown' method of S4 without ACPI support. 'shutdown' 
S4 method will call acpi power off after the memory image is ready, so it just 
is a S5 actually.
Comment 11 Pierre Ossman 2005-04-04 06:36:02 UTC
As pointed out previously, S5 works fine. Only S4 (i.e. 'platform') causes the
reboot.

(also, with acpi disabled I get 'Please power me down manually' when using
'shutdown').
Comment 12 Pierre Ossman 2005-05-31 09:48:38 UTC
What is the difference between:

echo 4 > /proc/acpi/sleep

and

echo platform > /sys/power/disk
echo disk > /sys/power/state

?

The first works, the second doesn't.
Comment 13 Len Brown 2005-08-17 11:24:09 UTC
still an issue in 2.6.13?
Comment 14 Pierre Ossman 2005-08-18 11:56:09 UTC
2.6.13-rc5 no longer exhibits the problem. Great work! =)

(I've only tested swsusp1 so far, but swsusp2 shouldn't differ in this aspect)
Comment 15 Pierre Ossman 2005-08-31 01:40:18 UTC
I'm afraid it got broken again by 2.6.13-final. :/

I'll try to figure out which patch caused the problem..
Comment 16 Pierre Ossman 2005-08-31 07:43:22 UTC
This little gem is what's causing the problem:

http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=8dbddf17824861f2298de093549e6493d9844835
Comment 17 Alexey Starikovskiy 2005-09-01 06:53:30 UTC
Created attachment 5842 [details]
Patch to set system_state at S4 power-off

Does this patch help you?
Comment 18 Pierre Ossman 2005-09-01 08:08:18 UTC
Patch works nicely. Thanks.
Comment 19 Alexey Starikovskiy 2005-09-01 08:45:34 UTC
Thanks for testing :)
Comment 20 Pierre Ossman 2005-09-08 01:18:40 UTC
I've been looking over the code and I'm not sure this was properly fixed. By
calling device_shutdown() we get to the magic in acpi_shutdown(). But that does
a acpi_sleep_prepare(ACPI_STATE_S5). This is both incorrect in two ways:

* acpi_sleep_prepare() has already been called as part of the suspend cycle.
* the sleep state being entered is S4, not S5. So it's preparing for the wrong
state.

So the way I see it it boils down to if device_shutdown() is meant to be used
during suspend. If it is then ACPI is broken, if not then swsusp is broken.

I'm adding Pavel as cc since swsusp is his baby.

(Either way things sucks to be me since my hardware seems to need both S4 and S5
prepare stages for entering S4.)
Comment 21 Alexey Starikovskiy 2005-11-17 06:12:43 UTC
is the bug still present in 2.6.14 kernel?
Comment 22 Pierre Ossman 2005-11-17 06:46:15 UTC
No, since the patch has been added to mainline. I reopened the bug because the
patch seems wrong to me (even though it solves the problem). Execution
preparation code for S5 and then entering S4 seems like asking for problems down
the line.
Comment 23 Alexey Starikovskiy 2005-11-17 07:04:05 UTC
No, this patch did not go into mainline. Instead, there was a patch from Eric
Biederman who cleaned up all poweroff/restart paths. 
Comment 24 Pierre Ossman 2005-11-17 07:17:39 UTC
Correct, but his patch does the same thing. So the question remains. Is the
current behaviour valid according to the ACPI spec?
Comment 25 Alexey Starikovskiy 2005-11-24 09:21:34 UTC
Created attachment 6676 [details]
Changes to PM subsystem to call ACPI (more) consistently

Please try it with both "shutdown" and "platform" options to /sys/power/disk...
Comment 26 Pierre Ossman 2005-11-29 04:36:06 UTC
Sorry for taking so long. It's been a busy weekend.

I can't see where you patch changes anything though?

Current call chain:

kernel/power/disk.c:prepare_processes()
\- drivers/acpi/sleep/main.c:acpi_pm_prepare()
   \- acpi_sleep_prepare(ACPI_STATE_S4)
kernel/power/disk.c:power_down()
\- kernel/sys.c:kernel_poweroff_prepare()
|  ... device_shutdown()
|  \- drivers/acpi/sleep/poweroff.c:acpi_shutdown()
|     \- acpi_sleep_prepare(ACPI_STATE_S5)
\- drivers/acpi/sleep/main.c:acpi_pm_enter()
   \- acpi_sleep_enter(ACPI_STATE_S4)

*suspended*

Call chaing after your patch:

kernel/power/disk.c:power_down()
\- drivers/acpi/sleep/main.c:acpi_pm_prepare()
|  \- acpi_sleep_prepare(ACPI_STATE_S4)
\- kernel/sys.c:kernel_poweroff_prepare()
|  \- kernel/reboot.c:machine_power_off_prepare()
|     \- drivers/acpi/sleep/poweroff.c:acpi_poweroff_prepare()
|        \- acpi_sleep_prepare(ACPI_STATE_S5)
\- drivers/acpi/sleep/main.c:acpi_pm_enter()
   \- acpi_sleep_enter(ACPI_STATE_S4)

*suspended*

I.e. both prepare for first S4, then S5 and finally enters S4.
Comment 27 Alexey Starikovskiy 2005-11-30 03:46:37 UTC
Created attachment 6720 [details]
Updated patch

Sorry, cleaned it too much. Intension was to remove call to
machine_power_off_prepare from 'platform' path. Please take a look again...
Comment 28 Pierre Ossman 2005-11-30 05:08:05 UTC
We're now back at where this bug started, but with cleaner code. :)

So the computer scientist in me is satisified, by the user isn't quite content yet.

I've learnt a lot more about suspend since this bug was opened so I'll have
another go at replicating the modifications in bug 3909 and bug 3669 again.
Hopefully there is a way to get this machine to handle S4 properly.
Comment 29 Alexey Starikovskiy 2005-11-30 05:48:45 UTC
Created attachment 6721 [details]
Updated patch with touching gpe in S4

Speeking of GPEs, may be this will help you?
Comment 30 Pierre Ossman 2005-11-30 05:55:08 UTC
I just tried the very same thing and it works like a charm. :)

I am a bit concerned about acpi_wakeup_gpe_poweroff_prepare() checking for
ACPI_STATE_S5 though. I'll see if it works as well when checking for S4.
Comment 31 Pierre Ossman 2005-11-30 06:00:04 UTC
Hmm... had a look at my list of devices and realised they're only 3 or 5 (no
4:s). So I can't really test if it should leave 4:s enabled. Perhaps they don't
even exist?

Anyway, the latest patch seems great to me (with the note that
acpi_wakeup_gpe_poweroff_prepare() might need changes).
Comment 32 Pavel Machek 2005-11-30 07:05:22 UTC
Well, the patch may fix one problem, but it makes already ugly part of kernel
even more complex. In particular, why are you removing acpi sysclass? Using its
shutdown method seems nicer than adding hooks by hand.
Comment 33 Alexey Starikovskiy 2005-11-30 07:57:44 UTC
Because it was added to solve this particular problem and it seems to not solve
it quite well. But we can return to this matter later, then the patch will go to
you for submission... Would you prefer one more system_state =
SYSTEM_SUSPEND_DISK or something like that?
Comment 34 Alexey Starikovskiy 2005-11-30 08:00:34 UTC
Created attachment 6722 [details]
Add proper handling of GPEs

sleep state of the GPEs is the lowest state from which it can wake system, so 5
means it can wake system from any state, including S4.
Comment 35 Pierre Ossman 2005-11-30 13:10:50 UTC
The latest incarnation of the patch works fine aswell.
Comment 36 Alexey Starikovskiy 2005-12-01 03:20:35 UTC
Created attachment 6732 [details]
Small & pretty patch 

Pavel, do you like this patch more?
Comment 37 Alexey Starikovskiy 2005-12-01 04:29:14 UTC
Created attachment 6733 [details]
Last patch with Pavels' suggestions

This is supposed to be the last patch before closing this bug, hope everyone is
happy with it. :)
Comment 38 Pierre Ossman 2005-12-01 04:50:27 UTC
Looks good to me. :)
Comment 39 Alexey Starikovskiy 2005-12-01 04:59:28 UTC
Thanks for testing and for suggestions, moving bug to resolved.
Len, Pavel have suggested what this patch should go through -mm.
Comment 40 Len Brown 2005-12-15 10:17:59 UTC
patch in comment #37 applied to acpi-test tree
Comment 41 Shaohua 2005-12-27 17:33:32 UTC
Can anybody give me a hint why we need this patch?
From my understanding, you hope we disable all GPEs in S4, right? 
But 'prepare_processes' already calls pm_ops->prepare, which should disables 
the GPEs.
Comment 42 Pierre Ossman 2005-12-27 22:04:38 UTC
The point was that the first solution to this bug was to prepare for S5 when
doing S4. This is rather confusing when trying to understand the code and bound
to lead to problems down the line.

The latest patch does a proper fix of this bug by generalising the GPE disable
function so that it can do its work for any state. For the hardware in question
for this bug it happens to do the same thing as for S5, but that might not be
the case on other machines.

As for pm_ops->prepare, it only disables GPE:s on S5. The patch fixes that.
Comment 43 Shaohua 2005-12-27 22:16:54 UTC
Sorry, I mean pm_ops->enter will acpi_enable_wakeup_device, which should clear 
wakeup flag for GPEs which aren't enabled by userspace. And 
finally 'acpi_enter_sleep_state' should disable all such GPEs. This is true 
for S4. So why should we need this patch?
Comment 44 Shaohua 2005-12-27 22:26:59 UTC
I bet what we really need to do is just to bypass acpi_sleep_prepare in 
acpi_shutdown if we are doing S4. Am I missing anything?
Comment 45 Pierre Ossman 2005-12-27 23:40:00 UTC
Since it doesn't work without this patch there is some difference. I'm not too
familiar with the low level details of GPE:s, but it seems that the one you're
pointing to enables all wake GPE:s, not just those able to wake up from the
target state.

Bypassing the prepare stage doesn't sound like a good idea. Any S4 triggers in
the DSDT wouldn't be honored in that case, possibly causing all kinds of trouble.
Comment 46 Shaohua 2005-12-28 16:51:13 UTC
pm_ops->enter will call 'acpi_hw_enable_all_wakeup_gpes'. This routine will 
enable wakeup GPEs if the GPEs are enabled for wake, otherwise they are 
disabled. As I said, acpi_enable_wakeup_device will set disabled flags for 
wakeup GPEs. So pm_ops->enter will disable wakeup GPEs. This code path is 
similar with S3, if it doesn't work, then S3 will not work either.
Is it possible you can just try to bypass 'acpi_sleep_prepare' (just remove it 
for test)in 'acpi_shutdown'?
Comment 47 Pierre Ossman 2005-12-29 00:14:48 UTC
If I remove acpi_sleep_prepare() from acpi_shutdown() then we're back at the way
it was at comment 15 where it only got executed for S5. Or do you want me to
bypass it completely (i.e. not only in acpi_shutdown())?

Does enabling/disabling GPE:s call any DSDT "functions" (or whatever they're
called)? It wouldn't surprise me if the DSDT is coded to do the hardware magic
in the wrong places.

If it's possible, we could also make a routine to print out all GPE info. That
way I could compare them and see if we can find why one works and the other
doesn't. I'd need some pointers on what should be printed though.
Comment 48 Len Brown 2006-02-02 14:30:11 UTC
Shipped in 2.6.16-rc1-git6 -- closing.

Note You need to log in before you can comment on or make changes to this bug.