ACPI code in platform mode hibernation code paths (was: Re: [PATCH] swsusp: do not use pm_ops)

ACPI code in platform mode hibernation code paths (was: Re: [PATCH] swsusp: do not use pm_ops)

[Rafael J. Wysocki]

Hi,

[Added linux-acpi to the CC list, should be there from the start]

On Wednesday, 2 May 2007 16:11, Alexey Starikovskiy wrote:
> Rafael,
> 
> > Concluding, it seems to me that the "restore" code path is correct, but the
> > "hibernate" code path is not and should be reworked.  Also, it seems that
> > acpi_pm_prepare() and acpi_pm_enter() should be fixed for the s2ram case
> > either (_PTS should be executed after device_suspend() and _GTS should
> > be executed in acpi_pm_enter(), right before the transition is completed).
> 
> Current implementation is not fully up-to spec, so we may try to get
> it closer to, I agree.

Okay.  Since we're trying to separate the hibernation code from the
suspend code anyway, we can use the opportunity to introduce some new
callbacks for the hibernation and/or redefine the existing ones.

The spec suggests that we need the following callbacks:

(1) prepare() - called after device_suspend(), execute _PTS and disable GPEs
(2) cancel() - called at any time after prepare() if there's an error, execute
    _WAK and enable run-time GPEs
(3) enter() - called after the image has been saved, execute _GTS and do what's
    currently done in pm_enter()
(4) finish() - called after the image has been restored, do what's currently
    done in pm_finish()

[At least, the execution of _GTS in pm_prepare() seems to be dangerous at first
sight.]

We also might need a callback that will be run before device_suspend() to
invoke some ACPI-related magic needed at that point, but I have no idea what
it would have to do. :-)

> > When we restore the system state from a hibernation image, the "boot kernel" is
> > first started.  It loads the image into memory, calls
> > device_suspend(PMSG_PRETHAW), suspends sysdevs etc., and replaces itself with
> > the "resumed kernel".  It doesn't call acpi_pm_prepare(), which I think is
> > right, because it doesn't want to start any power transition, not even a
> > fake one.  Now, the "resumed kernel" takes control, resumes sysdevs and calls

> Currently call to prepare() is needed to stop ACPI devices to send
> GPEs to ACPI drivers.

Does it mean that we need to call pm_prepare() (or an equivalent function)
before device_suspend()?  If that's the case, then which part of pm_prepare()
is essential here?

> If you remove it, Acer laptops will resume without ACPI interrupt at
> all (with all problems from it).

A recent discussion on the LKML lead to the conclusion that for the
hibernation we shouldn't use .suspend() callbacks before snapshotting the
system memory.  Instead, we should use some other callbacks to quiesce devices,
create the snapshot, reactivate devices, save the image and carry out the
actual power transition after that.  Would something like this be viable from
the ACPI point of view?

> > acpi_pm_finish(), which seems to be about OK, except that I'm not sure if _BFS
> > should be executed in that case (the ACPI spec seems to assume that the
> > hibernation image will be loaded into memory by a boot loader).
> 
> > Next, we suspend sysdevs etc., and create the memory snapshot.  We want
> > to be able to save it, so w call acpi_pm_finish(), which causes _BFS and _WAK
> > to be executed *after* _GTS, which is clearly against the spec (might this be the
> > reason why (7) is sometimes necessary?).  Moreover, calling _BFS at this stage
> > makes no sense, IMHO, since there hasn't been any transition (the system has
> > not slept).  What I think we should do at this point is to execute _WAK only,
> > which means "power transition aborted" to the firmware, and continue with
> > device_resume().
> 
> But I don't get your idea about executing _finish() between _prepare()
> and _enter()...
> _finish is executed only if _prepare() fails, so we are rolling back,

Well, no.  Please have a look at the code in kernel/power/disk.c.

Should we remove it from the nonerror code paths?

> or it is executed after we loaded the image and transfered execution
> to it, so again -- we are going from _prepare() state to running
> state...

Currently that's not the case.

Greetings,
Rafael

Re: ACPI code in platform mode hibernation code paths (was: Re: [PATCH] swsusp: do not use pm_ops)

[Pavel Machek]

Hi!

Crazy idea... could we kill hibernate_ops-like struct, and just create
a device for ACPI, using its suspend()/resume()/whatever callbacks to
do the ACPI magic?

> Okay.  Since we're trying to separate the hibernation code from the
> suspend code anyway, we can use the opportunity to introduce some new
> callbacks for the hibernation and/or redefine the existing ones.
> 
> The spec suggests that we need the following callbacks:
> 
> (1) prepare() - called after device_suspend(), execute _PTS and
> disable GPEs

sysdev .suspend() method would do the trick.

> (2) cancel() - called at any time after prepare() if there's an error, execute
>     _WAK and enable run-time GPEs

sysdev .resume() should do the trick. 

> (3) enter() - called after the image has been saved, execute _GTS and do what's
>     currently done in pm_enter()

This one is tricky. It is essentially
powerdown_but_enter_S4_instead. I guess we can live with if()... as we
need to special-case reboot in the same place.

> (4) finish() - called after the image has been restored, do what's currently
>     done in pm_finish()

platform (?) device .resume() method should work.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: ACPI code in platform mode hibernation code paths (was: Re: [PATCH] swsusp: do not use pm_ops)

[Rafael J. Wysocki]

Hi,

On Friday, 4 May 2007 00:48, Pavel Machek wrote:
> Hi!
> 
> Crazy idea... could we kill hibernate_ops-like struct, and just create
> a device for ACPI, using its suspend()/resume()/whatever callbacks to
> do the ACPI magic?

Hmm, I didn't think about that.  It seems to be viable at first sight.

Still, I think we can first separate hibernation_ops from pm_ops, figure out
what they should be and then try to replace them with a cleaner solution.

> > Okay.  Since we're trying to separate the hibernation code from the
> > suspend code anyway, we can use the opportunity to introduce some new
> > callbacks for the hibernation and/or redefine the existing ones.
> > 
> > The spec suggests that we need the following callbacks:

In fact, I should have added

(0) start() - called before device_suspend(), execute _TTS(S4)

and I'm not sure if the GPEs should be disabled here or in prepare()

In principle this could be done as a device's .resume() call, but that would
have to be the very first device registered (can we do that?).

> > (1) prepare() - called after device_suspend(), execute _PTS and
> > disable GPEs
> 
> sysdev .suspend() method would do the trick.

Yes.

> > (2) cancel() - called at any time after prepare() if there's an error, execute
> >     _WAK and enable run-time GPEs
> 
> sysdev .resume() should do the trick.

But .resume() would be called unconditionally, so there should be a way of
figuring out what to do - looks complicated.
 
> > (3) enter() - called after the image has been saved, execute _GTS and do what's
> >     currently done in pm_enter()
> 
> This one is tricky. It is essentially
> powerdown_but_enter_S4_instead. I guess we can live with if()... as we
> need to special-case reboot in the same place.

Yes.

> > (4) finish() - called after the image has been restored, do what's currently
> >     done in pm_finish()
>
> platform (?) device .resume() method should work.

Hmm, perhaps.

And we need one more (in fact this one should be called finish() and the
previous one wake() or something like that):

(5) finish() - called after device_resume(), but only after the image has been
restored or in case of a hibernation error, execute _TTS(S0).  It looks like
this also should enable the GPEs (or the previous one; that's the information
I'm looking for).

Greetings,
Rafael

Re: ACPI code in platform mode hibernation code paths (was: Re: [PATCH] swsusp: do not use pm_ops)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

On Fri, 2007-05-04 at 00:48 +0200, Pavel Machek wrote:

> Crazy idea... could we kill hibernate_ops-like struct, and just create
> a device for ACPI, using its suspend()/resume()/whatever callbacks to
> do the ACPI magic?

Doesn't that have the ordering problem again? You must ensure that this
sysdev is suspended as the last one, and that's currently impossible if
ACPI is modular.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: ACPI code in platform mode hibernation code paths (was: Re: [PATCH] swsusp: do not use pm_ops)

[Pavel Machek]

Hi!

> > Crazy idea... could we kill hibernate_ops-like struct, and just create
> > a device for ACPI, using its suspend()/resume()/whatever callbacks to
> > do the ACPI magic?
> 
> Doesn't that have the ordering problem again? You must ensure that this
> sysdev is suspended as the last one, and that's currently impossible if
> ACPI is modular.

I do not think acpi has these kinds of ordering requirements... (And I
do not see what it has to do with module or not).



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: ACPI code in platform mode hibernation code paths (was: Re: [PATCH] swsusp: do not use pm_ops)

[Rafael J. Wysocki]

Hi,

On Friday, 4 May 2007 14:08, Pavel Machek wrote:
> Hi!
> 
> > > Crazy idea... could we kill hibernate_ops-like struct, and just create
> > > a device for ACPI, using its suspend()/resume()/whatever callbacks to
> > > do the ACPI magic?
> > 
> > Doesn't that have the ordering problem again? You must ensure that this
> > sysdev is suspended as the last one, and that's currently impossible if
> > ACPI is modular.
> 
> I do not think acpi has these kinds of ordering requirements... (And I
> do not see what it has to do with module or not).

Theoretically, ACPI has some ordering requirements.  For example, according to
the spec, the _PTS system-control method should be executed *after* devices are
placed in the appropriate Dx states, which (theoretically) requires us to
execute it after device_suspend() (we don't do this in practice, but I think we
should).

There are some more ordering assumptions like this in the spec and I think
we should at least try to follow them or, if that breaks things, document why
we don't.

That's why I think we should try to do what's needed using hibernation_ops 
(perhaps we'll need to add a couple of callbacks to hibernation_ops) and
then try to replace hibernation_ops with another mechanism allowing us to do
the same.  We first need to determine which operations have to be carried out
at what points so that things don't break.

Greetings,
Rafael