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.
How about the 'platform' method of S4? Did you test latest kernel?
'platform' has the same effect. (Doesn't that evaluate to S4 when ACPI handles PM?) Kernel is 2.6.11.
Pierre, was this actually a regression? Did earlier kernels work OK? If so, which ones? Thanks.
Is shutdown (I mean S5) ok in this laptop? If shutdown ok, S4 platform method should be ok to me.
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.
Could you please attach your 'acpidmp' and 'cat /proc/acpi/wakeup' output? I'd like to check them.
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
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.
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.
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.
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').
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.
still an issue in 2.6.13?
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)
I'm afraid it got broken again by 2.6.13-final. :/ I'll try to figure out which patch caused the problem..
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
Created attachment 5842 [details] Patch to set system_state at S4 power-off Does this patch help you?
Patch works nicely. Thanks.
Thanks for testing :)
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.)
is the bug still present in 2.6.14 kernel?
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.
No, this patch did not go into mainline. Instead, there was a patch from Eric Biederman who cleaned up all poweroff/restart paths.
Correct, but his patch does the same thing. So the question remains. Is the current behaviour valid according to the ACPI spec?
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...
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.
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...
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.
Created attachment 6721 [details] Updated patch with touching gpe in S4 Speeking of GPEs, may be this will help you?
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.
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).
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.
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?
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.
The latest incarnation of the patch works fine aswell.
Created attachment 6732 [details] Small & pretty patch Pavel, do you like this patch more?
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. :)
Looks good to me. :)
Thanks for testing and for suggestions, moving bug to resolved. Len, Pavel have suggested what this patch should go through -mm.
patch in comment #37 applied to acpi-test tree
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.
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.
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?
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?
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.
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'?
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.
Shipped in 2.6.16-rc1-git6 -- closing.