Bug 4320

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.