swsusp issues

swsusp issues

[Benjamin Herrenschmidt]

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

Hi Pavel !

So one problem I was having with my new code is that swsusp "hooks" of
enter_state() early and doesn't go through the "normal" callback
mecanism, thus wasn't calling any of my new callbacks.

In addition, it has its own save/restore_processor_state that do not go
through pm_ops, but instead are arch inlines (and besides, I never found
any use of those for saving/restoring processor state, though I do use
them for other means, the semantics are quite crappy here).

This is all fairly inconsistent.

What about (note: I'm proposing here to do the job, not asking you to do
it) merging at least disk.c into main.c, that is, having swsusp just use
the "normal" infrastructure ?

suspend_prepare() and suspend_finish() are pretty much equivalent to
what disk.c does in (un)prepare_processes(), prepare_devices() and
finish() with the exception of free_some_memory() which is not used in
suspend to ram, and swsusp does this totally bogus SMP tricks (they
really can't work properly, you'll deadlock trying to use them,
really !)

It would be very easy to consolidate all of that in suspend_prepare() an
suspend_finish() (adding the free_some_memory if state is
PM_SUSPEND_DISK), and those have nice error handling.

Then, the "main" implementation does ops->enter(). On swsusp, this enter
sort-of depdends of the "type" of suspend to disk required. Either
"enter" does all the job (PM_DISK_FIRMWARE) which is then totally
equivalent to the core code needs, or it's replaced by a blurb that does
swsusp_suspend, swsusp_write, etc...

Here, we could do two things... Either add some conditionals to the main
code to reproduce that logic (or rather that illogic), or we could
simply unconditionally call ops->enter() and provide a helper function
that the arch can call that does the swsusp_suspend, swsusp_write,
etc... if that is the arch choice to do.

Finally, the whole pm_disk_mode is just disgustingly ugly. Especially
the fact that the user can go muck around and change it. I do not
support any mode but "PM_DISK_PLATFORM" on ppc and I really think that
should be the only implementation. That is, it's up to the platform
provided pm_ops to decide what to  do. Either doing all the work, or
using swsusp_suspend/swsusp_disk and then restart or shutdown or
whatever. This is all platform specific. Here, basically, the genric
code is exposing some x86 crappola.

Ben.



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: swsusp issues

[Pavel Machek]

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

Hi!

> So one problem I was having with my new code is that swsusp "hooks" of
> enter_state() early and doesn't go through the "normal" callback
> mecanism, thus wasn't calling any of my new callbacks.
> 
> In addition, it has its own save/restore_processor_state that do not go
> through pm_ops, but instead are arch inlines (and besides, I never found
> any use of those for saving/restoring processor state, though I do use
> them for other means, the semantics are quite crappy here).

Well, i386 definitely uses them for saving some 'less important' cpu
registers.

> This is all fairly inconsistent.
> 
> What about (note: I'm proposing here to do the job, not asking you to do
> it) merging at least disk.c into main.c, that is, having swsusp just use
> the "normal" infrastructure ?

Merging disk.c and main.c is certainly good idea, I never know which
one to look in, anyway.

> suspend_prepare() and suspend_finish() are pretty much equivalent to
> what disk.c does in (un)prepare_processes(), prepare_devices() and
> finish() with the exception of free_some_memory() which is not used in
> suspend to ram, and swsusp does this totally bogus SMP tricks (they
> really can't work properly, you'll deadlock trying to use them,
> really !)

Those smp tricks... see my development tree at
www.kernel.org/...git/pavel/work.git. We actually use cpu hotplug
infrastructure and it should work ok.

> Then, the "main" implementation does ops->enter(). On swsusp, this enter
> sort-of depdends of the "type" of suspend to disk required. Either
> "enter" does all the job (PM_DISK_FIRMWARE) which is then totally
> equivalent to the core code needs, or it's replaced by a blurb that does
> swsusp_suspend, swsusp_write, etc...

PM_DISK_FIRMWARE can be safely removed. See feature-removal*,
DISK_FIRMWARE is S4BIOS.

> Finally, the whole pm_disk_mode is just disgustingly ugly. Especially
> the fact that the user can go muck around and change it. I do not
> support any mode but "PM_DISK_PLATFORM" on ppc and I really think that
> should be the only implementation. That is, it's up to the platform
> provided pm_ops to decide what to  do. Either doing all the work, or
> using swsusp_suspend/swsusp_disk and then restart or shutdown or
> whatever. This is all platform specific. Here, basically, the genric
> code is exposing some x86 crappola.

PM_DISK_REBOOT is very usefull for testing (and I believe generic; you
could do that on ppc too). PM_DISK_SHUTDOWN is usefull for using
normal powerdown paths in case something is wrong with disk-specific
powerdown. We should move away from it, but please try not to kill it.

								Pavel

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: swsusp issues

[Benjamin Herrenschmidt]

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


> PM_DISK_REBOOT is very usefull for testing (and I believe generic; you
> could do that on ppc too). PM_DISK_SHUTDOWN is usefull for using
> normal powerdown paths in case something is wrong with disk-specific
> powerdown. We should move away from it, but please try not to kill it.

Well, I have a problem with these because they don't call the arch
prepare/finish callbacks.

Anyway, I discussed with Patrick yesterday and we came to the conclusion
that the best is in fact to just kill the current mecanism which was
done upside down. Kill pm_ops completely and instead of having a slim
"core" that uses callbacks all over, have the arch contain the main
state enter function and have the stuff in kernel/power/* be "library"
functions to be called by the arch code.

The idea here is then to have the arch provide main.c with an array of
"name",function pairs (name beeing the name in /sys/power/state and
function is what to call when the user echo's that name
to /sys/power/state). The rest would be entirely under arch control, and
that would remove most of the current cruft.

To simplify the job for swsusp and avoid the ugly in_suspend thing, we
should do a slight change of the low level code so that swsusp_suspend()
returns: 0 -> resume, 1 -> snapshotted, <0 -> error

We could provide an "example" default implementation that does only
swsusp that an arch can "drop in" if you want, but archs have to
implement the various "inline" callbacks anyway (save_processor_state &
friends).

Ben.



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: swsusp issues

[Pavel Machek]

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

Hi!

[Hmm, you seem to be awake at *exactly* wrong moments. I just decided
I really need some sleep 10 minutes ago.]

> > PM_DISK_REBOOT is very usefull for testing (and I believe generic; you
> > could do that on ppc too). PM_DISK_SHUTDOWN is usefull for using
> > normal powerdown paths in case something is wrong with disk-specific
> > powerdown. We should move away from it, but please try not to kill it.
> 
> Well, I have a problem with these because they don't call the arch
> prepare/finish callbacks.
> 
> Anyway, I discussed with Patrick yesterday and we came to the conclusion
> that the best is in fact to just kill the current mecanism which was
> done upside down. Kill pm_ops completely and instead of having a slim
> "core" that uses callbacks all over, have the arch contain the main
> state enter function and have the stuff in kernel/power/* be "library"
> functions to be called by the arch code.
> 
> The idea here is then to have the arch provide main.c with an array of
> "name",function pairs (name beeing the name in /sys/power/state and
> function is what to call when the user echo's that name
> to /sys/power/state). The rest would be entirely under arch control, and
> that would remove most of the current cruft.

swsusp/powerdown and swsusp/reboot cases should really only require
save_processor_state and friends filled by architecture... And at
least i386 and x86-64 (and possibly ia64) will have basically same
code for all the stuff except save_processor_state and friends...

I'm not sure what you want to do with "take the control of main.c". If
you do device calls in different order, or diverge in something
visible to drivers, you'll create maintanance nightmare. Is it really
impossible to handle ppc without adding more callbacks?

[Modulo apm emulation, I see why you need callbacks there, but that
should not be there or it should be in generic code, anyways].

> We could provide an "example" default implementation that does only
> swsusp that an arch can "drop in" if you want, but archs have to
> implement the various "inline" callbacks anyway (save_processor_state &
> friends).

...aha, so you know that much code can be shared :-). Yes, "example"
implementation should work okay. Use "example" implementation, add
custom save_processor_state, and you should have working
swsusp/powerdown...

							Pavel

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: swsusp issues

[Nigel Cunningham]

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

Hi.

On Sat, 2005-06-04 at 08:50, Benjamin Herrenschmidt wrote:
> > PM_DISK_REBOOT is very usefull for testing (and I believe generic; you
> > could do that on ppc too). PM_DISK_SHUTDOWN is usefull for using
> > normal powerdown paths in case something is wrong with disk-specific
> > powerdown. We should move away from it, but please try not to kill it.
> 
> Well, I have a problem with these because they don't call the arch
> prepare/finish callbacks.
> 
> Anyway, I discussed with Patrick yesterday and we came to the conclusion
> that the best is in fact to just kill the current mecanism which was
> done upside down. Kill pm_ops completely and instead of having a slim
> "core" that uses callbacks all over, have the arch contain the main
> state enter function and have the stuff in kernel/power/* be "library"
> functions to be called by the arch code.
> 
> The idea here is then to have the arch provide main.c with an array of
> "name",function pairs (name beeing the name in /sys/power/state and
> function is what to call when the user echo's that name
> to /sys/power/state). The rest would be entirely under arch control, and
> that would remove most of the current cruft.
> 
> To simplify the job for swsusp and avoid the ugly in_suspend thing, we
> should do a slight change of the low level code so that swsusp_suspend()
> returns: 0 -> resume, 1 -> snapshotted, <0 -> error
> 
> We could provide an "example" default implementation that does only
> swsusp that an arch can "drop in" if you want, but archs have to
> implement the various "inline" callbacks anyway (save_processor_state &
> friends).

How could I work with that in an implementation where the vast majority
of the code is arch-independent? I guess I'll have to see what you come
up with.

Regards,

Nigel


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: swsusp issues

[Benjamin Herrenschmidt]

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


> swsusp/powerdown and swsusp/reboot cases should really only require
> save_processor_state and friends filled by architecture... And at
> least i386 and x86-64 (and possibly ia64) will have basically same
> code for all the stuff except save_processor_state and friends...

Eugh.... No. Maybe you can get away without the arch callbackcs on x86,
but there are a bunch of things that need to be properly saved/restored
even for basic swsusp that don't fit in the driver model. (Besides, you
don't even call device_power_down, so the stuffs that are sysdev's
aren't dealt with properly neither).

> I'm not sure what you want to do with "take the control of main.c". If
> you do device calls in different order, or diverge in something
> visible to drivers, you'll create maintanance nightmare. Is it really
> impossible to handle ppc without adding more callbacks?

I'm pretty sure it's impossible to handle anything correctly. Your x86
implementation may "happen" to work but I'm really not confident about
it.

> [Modulo apm emulation, I see why you need callbacks there, but that
> should not be there or it should be in generic code, anyways].

There are other things that need proper massaging.

> > We could provide an "example" default implementation that does only
> > swsusp that an arch can "drop in" if you want, but archs have to
> > implement the various "inline" callbacks anyway (save_processor_state &
> > friends).
> 
> ...aha, so you know that much code can be shared :-). Yes, "example"
> implementation should work okay. Use "example" implementation, add
> custom save_processor_state, and you should have working
> swsusp/powerdown...

I don't care a bit about sharing code in that area. "How much" amounts
to 3 function calls, so honestly, that is not an issue. I agree with
Patrick here, the toplevel enter_state() function should probably just
be arch code.

BEn.



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: swsusp issues

[Benjamin Herrenschmidt]

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


> > We could provide an "example" default implementation that does only
> > swsusp that an arch can "drop in" if you want, but archs have to
> > implement the various "inline" callbacks anyway (save_processor_state &
> > friends).
> 
> How could I work with that in an implementation where the vast majority
> of the code is arch-independent? I guess I'll have to see what you come
> up with.

The arch independent code is called by the arch code, nothing new here.

I don't suggest arch re-implements swsusp_suspend() or
freeze_processes(). It's either that or having all the new callbacks
plus a major blowup of the crap in disk.c to fit in main.c 

Ben.




[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: swsusp issues

[Pavel Machek]

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

Hi!

> > swsusp/powerdown and swsusp/reboot cases should really only require
> > save_processor_state and friends filled by architecture... And at
> > least i386 and x86-64 (and possibly ia64) will have basically same
> > code for all the stuff except save_processor_state and friends...
> 
> Eugh.... No. Maybe you can get away without the arch callbackcs on x86,
> but there are a bunch of things that need to be properly saved/restored
> even for basic swsusp that don't fit in the driver model. (Besides, you
> don't even call device_power_down, so the stuffs that are sysdev's
> aren't dealt with properly neither).

Well, we might want to fix not calling device_power_down... 

> > > We could provide an "example" default implementation that does only
> > > swsusp that an arch can "drop in" if you want, but archs have to
> > > implement the various "inline" callbacks anyway (save_processor_state &
> > > friends).
> > 
> > ...aha, so you know that much code can be shared :-). Yes, "example"
> > implementation should work okay. Use "example" implementation, add
> > custom save_processor_state, and you should have working
> > swsusp/powerdown...
> 
> I don't care a bit about sharing code in that area. "How much" amounts
> to 3 function calls, so honestly, that is not an issue. I agree with
> Patrick here, the toplevel enter_state() function should probably just
> be arch code.

Well, if we are talking about single function (enter_state) being
moved to arch code... that should be okay. It is really simple
function.

							Pavel	 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: swsusp issues

[Benjamin Herrenschmidt]

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


> Well, if we are talking about single function (enter_state) being
> moved to arch code... that should be okay. It is really simple
> function.

That and breaking it down a little bit tho :)

Anyway, we should meet on irc one of these days with Patrick to discuss
that.

Ben.



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]