driver power operations (was Re: suspend2 merge)

driver power operations (was Re: suspend2 merge)

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 2996 bytes --]

On Wed, 2007-04-25 at 22:31 +0200, Pavel Machek wrote:

> > So, the "suspend" and "resume" for the functions being called for that are
> > wrong, but then we call them with PMSG_FREEZE. ;-)  Still, we could add
> > .freeze() and .thaw() callbacks for hibernation just fine.  This wouldn't even
> > be that difficult ...
> 
> It would be ugly big patch I'm afraid.

It'd be a lot of code churn, but well worth it. And most of the changes
would be trivial too. You need to start looking beyond "this is ugly in
the short term" and realise that it's much more maintainable in the long
term if driver writers know what they're supposed to do as opposed to
just hacking at it until it mostly works or just doing a full device
down/up cycle including resetting full driver state.

Look at it now:

 * FREEZE       Quiesce operations so that a consistent image can be saved;
 *              but do NOT otherwise enter a low power device state, and do
 *              NOT emit system wakeup events.
 *
 * PRETHAW      Quiesce as if for FREEZE; additionally, prepare for restoring
 *              the system from a snapshot taken after an earlier FREEZE.
 *              Some drivers will need to reset their hardware state instead
 *              of preserving it, to ensure that it's never mistaken for the
 *              state which that earlier snapshot had set up.

Why is prethaw even necessary? As far as I can tell it's only necessary
because resume() can't tell you whether you just want to thaw or need to
reset since it doesn't tell you at what point it's invoked.

Having ->freeze(), ->thaw() and ->restart() (can somebody come up with a
better name?) that are called at the appropriate places (with
freeze/thaw around preparing the image and freeze/restart around
restoring would go a long way of clearing up the confusion in all the
drivers. Of course, it'd have to be documented that freeze/thaw isn't
the only valid combination but that freeze/restart is used too, but
that's not hard to do nor hard to understand.

And, incidentally, it could possibly make both suspend and hibernate
work much faster too. The comments there talk about "minimally power
management aware" drivers which always do the wrong thing for suspend,
in that they always reset everything... Of course, some drivers will
actually need to do that, but if freeze/suspend and thaw/restart/resume
have the same prototypes (probably just int <function>(void)) then
drivers can trivially assign the same there.
And hibernate would benefit since a lot of drivers could do a lot less
work for freeze/thaw.

Or, if we don't want to have five calls and use 40 bytes (on 64-bit)
just for these callback pointers for each device we could just as well
have a single callback ->pm(what) and make "what" indicate which one of
these five things... But then drivers can't make that code depend on the
swsusp configuration which would be doable with five callbacks.

johannes

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

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

Re: driver power operations (was Re: suspend2 merge)

[Rafael J. Wysocki]

On Friday, 27 April 2007 12:21, Johannes Berg wrote:
> On Wed, 2007-04-25 at 22:31 +0200, Pavel Machek wrote:
> 
> > > So, the "suspend" and "resume" for the functions being called for that are
> > > wrong, but then we call them with PMSG_FREEZE. ;-)  Still, we could add
> > > .freeze() and .thaw() callbacks for hibernation just fine.  This wouldn't even
> > > be that difficult ...
> > 
> > It would be ugly big patch I'm afraid.
> 
> It'd be a lot of code churn, but well worth it. And most of the changes
> would be trivial too. You need to start looking beyond "this is ugly in
> the short term" and realise that it's much more maintainable in the long
> term if driver writers know what they're supposed to do as opposed to
> just hacking at it until it mostly works or just doing a full device
> down/up cycle including resetting full driver state.
> 
> Look at it now:
> 
>  * FREEZE       Quiesce operations so that a consistent image can be saved;
>  *              but do NOT otherwise enter a low power device state, and do
>  *              NOT emit system wakeup events.
>  *
>  * PRETHAW      Quiesce as if for FREEZE; additionally, prepare for restoring
>  *              the system from a snapshot taken after an earlier FREEZE.
>  *              Some drivers will need to reset their hardware state instead
>  *              of preserving it, to ensure that it's never mistaken for the
>  *              state which that earlier snapshot had set up.
> 
> Why is prethaw even necessary? As far as I can tell it's only necessary
> because resume() can't tell you whether you just want to thaw or need to
> reset since it doesn't tell you at what point it's invoked.
> 
> Having ->freeze(), ->thaw() and ->restart() (can somebody come up with a
> better name?) that are called at the appropriate places (with
> freeze/thaw around preparing the image and freeze/restart around
> restoring would go a long way of clearing up the confusion in all the
> drivers. Of course, it'd have to be documented that freeze/thaw isn't
> the only valid combination but that freeze/restart is used too, but
> that's not hard to do nor hard to understand.
> 
> And, incidentally, it could possibly make both suspend and hibernate
> work much faster too. The comments there talk about "minimally power
> management aware" drivers which always do the wrong thing for suspend,
> in that they always reset everything... Of course, some drivers will
> actually need to do that, but if freeze/suspend and thaw/restart/resume
> have the same prototypes (probably just int <function>(void)) then
> drivers can trivially assign the same there.
> And hibernate would benefit since a lot of drivers could do a lot less
> work for freeze/thaw.

I violently agree with all of the above.

Moreover, for the hibernation we have two special cases that are of no interest
for the suspend:
1) drivers compiled as modules and not loaded before we restore the image
2) drivers that need to allocate much memory in .freeze()

> Or, if we don't want to have five calls and use 40 bytes (on 64-bit)
> just for these callback pointers for each device we could just as well
> have a single callback ->pm(what) and make "what" indicate which one of
> these five things... But then drivers can't make that code depend on the
> swsusp configuration which would be doable with five callbacks.

Five callbacks are fine by me, especially if we can define reasonable defaults
for the hibernation (and can we?).

Rafael

Re: driver power operations (was Re: suspend2 merge)

[Pavel Machek]

Hi!

> > And, incidentally, it could possibly make both suspend and hibernate
> > work much faster too. The comments there talk about "minimally power
> > management aware" drivers which always do the wrong thing for suspend,
> > in that they always reset everything... Of course, some drivers will
> > actually need to do that, but if freeze/suspend and thaw/restart/resume
> > have the same prototypes (probably just int <function>(void)) then
> > drivers can trivially assign the same there.
> > And hibernate would benefit since a lot of drivers could do a lot less
> > work for freeze/thaw.
> 
> I violently agree with all of the above.
> 
> Moreover, for the hibernation we have two special cases that are of no interest
> for the suspend:
> 1) drivers compiled as modules and not loaded before we restore the image
> 2) drivers that need to allocate much memory in .freeze()
> 
> > Or, if we don't want to have five calls and use 40 bytes (on 64-bit)
> > just for these callback pointers for each device we could just as well
> > have a single callback ->pm(what) and make "what" indicate which one of
> > these five things... But then drivers can't make that code depend on the
> > swsusp configuration which would be doable with five callbacks.
> 
> Five callbacks are fine by me, especially if we can define reasonable defaults
> for the hibernation (and can we?).

Well, we still can default to suspend(PMSG_FREEZE) for freeze(), and
resume() for thaw(). Anything else is just not sane way forward.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: driver power operations (was Re: suspend2 merge)

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 754 bytes --]

On Fri, 2007-04-27 at 14:40 +0200, Pavel Machek wrote:

> > Five callbacks are fine by me, especially if we can define reasonable defaults
> > for the hibernation (and can we?).
> 
> Well, we still can default to suspend(PMSG_FREEZE) for freeze(), and
> resume() for thaw(). Anything else is just not sane way forward.

I think we should remove the argument to suspend() in the same patch
series. Yes, that would mean porting all drivers that currently use it,
but that's not actually all that many since most drivers are dumbed-down
wrt. power management.

And realistically, resume for thaw makes no sense, nor does suspend for
freeze, so we probably want to change those over to suspend/restart and
use them. or something.

johannes

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

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

Re: driver power operations (was Re: suspend2 merge)

[Pavel Machek]

Hi!

> > > Five callbacks are fine by me, especially if we can define reasonable defaults
> > > for the hibernation (and can we?).
> > 
> > Well, we still can default to suspend(PMSG_FREEZE) for freeze(), and
> > resume() for thaw(). Anything else is just not sane way forward.
> 
> I think we should remove the argument to suspend() in the same patch
> series. Yes, that would mean porting all drivers that currently use
> it,

Well, if you can do it in one patch series, go ahead. But I think such
massive change all over kernel will take slightly longer, so I'd
prefer to keep the dummy argument for a while (so it still compiles)
and fix it slowly.

Of course, it is up to the person doing the series. And yes, such
series would be welcome.

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

Re: driver power operations (was Re: suspend2 merge)

[Alan Stern]

On Fri, 27 Apr 2007, Johannes Berg wrote:

> Look at it now:
> 
>  * FREEZE       Quiesce operations so that a consistent image can be saved;
>  *              but do NOT otherwise enter a low power device state, and do
>  *              NOT emit system wakeup events.
>  *
>  * PRETHAW      Quiesce as if for FREEZE; additionally, prepare for restoring
>  *              the system from a snapshot taken after an earlier FREEZE.
>  *              Some drivers will need to reset their hardware state instead
>  *              of preserving it, to ensure that it's never mistaken for the
>  *              state which that earlier snapshot had set up.
> 
> Why is prethaw even necessary? As far as I can tell it's only necessary
> because resume() can't tell you whether you just want to thaw or need to
> reset since it doesn't tell you at what point it's invoked.

I think you're wrong here.  It's a little hard to say because the 
terminology is confusing and not yet standardized.

For the sake of argument, let's call the stages of STD and STR by these 
names (also noted are the current PSMG values):

	Suspend to disk:
	"prepare to create snapshot" (= FREEZE)
	"continue after snapshot" (= RESUME)

	Resume from disk:
	"prepare to restore snapshot" (= PRETHAW)
	"continue after restore" (= RESUME)

	Suspend to RAM:
	"suspend" (= SUSPEND)
	"resume" (= RESUME)

The real reason for adding PRETHAW was that drivers couldn't distinguish
between "continue after restore" and "resume", other than by examining the
device's state -- since the PM core doesn't pass any information to the
resume() method.

I suppose we could have modified the "prepare to create snapshot" code 
instead, but doing so would mean that "continue after snapshot" and 
"continue after restore" would always do the same thing, which is not 
necessarily a good idea.

Anyway, based on this analysis it seems reasonable to have Six (6) method 
pointers.  Suggested names (in the same order as above):

	pre_snaphot()
	post_snapshot()
	pre_restore()
	post_restore()
	suspend()
	resume()

People apparently assume that pre_snapshot() and pre_restore() would 
always do the same thing and hence be redundant.  I'm not so sure; time 
will tell.  Doing it this way certainly is more clear.

Then there's the question of having early_ and late_ versions of some of 
these things (i.e., one called with interrupts enabled, the other with 
interrupts disabled).  I don't know to what extent that would be 
necessary; perhaps the each method call should occur in two phases with 
the interrupt-enable status changed in between.  Then the interrupt-enable 
setting could be passed as an argument.

Alan Stern

Re: driver power operations (was Re: suspend2 merge)

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 2168 bytes --]

On Fri, 2007-04-27 at 10:34 -0400, Alan Stern wrote:

> For the sake of argument, let's call the stages of STD and STR by these 
> names (also noted are the current PSMG values):
> 
> 	Suspend to disk:
> 	"prepare to create snapshot" (= FREEZE)
> 	"continue after snapshot" (= RESUME)
> 
> 	Resume from disk:
> 	"prepare to restore snapshot" (= PRETHAW)
> 	"continue after restore" (= RESUME)
> 
> 	Suspend to RAM:
> 	"suspend" (= SUSPEND)
> 	"resume" (= RESUME)
> 
> The real reason for adding PRETHAW was that drivers couldn't distinguish
> between "continue after restore" and "resume", other than by examining the
> device's state -- since the PM core doesn't pass any information to the
> resume() method.

That's pretty much what I said about prethaw though, no? Anyway,

> Anyway, based on this analysis it seems reasonable to have Six (6) method 
> pointers.  Suggested names (in the same order as above):
> 
> 	pre_snaphot()
> 	post_snapshot()
> 	pre_restore()
> 	post_restore()
> 	suspend()
> 	resume()
> 
> People apparently assume that pre_snapshot() and pre_restore() would 
> always do the same thing and hence be redundant.  I'm not so sure; time 
> will tell.  Doing it this way certainly is more clear.

Right. I did assume that pre_snapshot and pre_restore would be
effectively the same since they both have to quiesce the device and
assume not much more. I'm not averse to making it explicit, many drivers
that don't care can just assign the same function.

> Then there's the question of having early_ and late_ versions of some of 
> these things (i.e., one called with interrupts enabled, the other with 
> interrupts disabled).  I don't know to what extent that would be 
> necessary; perhaps the each method call should occur in two phases with 
> the interrupt-enable status changed in between.  Then the interrupt-enable 
> setting could be passed as an argument.

Good point. Though if we go for passing the interrupt-enable setting as
an argument then many drivers will have the same
"if (irqs_disabled()) return" code. Hm. I guess passing it isn't even
strictly necessary.

johannes

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

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

Re: driver power operations (was Re: suspend2 merge)

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 439 bytes --]

On Fri, 2007-04-27 at 16:39 +0200, Johannes Berg wrote:

> Good point. Though if we go for passing the interrupt-enable setting as
> an argument then many drivers will have the same
> "if (irqs_disabled()) return" code. Hm. I guess passing it isn't even
> strictly necessary.

Eh, the point I actually wanted to make is that many drivers don't care
for the irqs disabled case and would have to add code to exclude it.

johannes

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

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

Re: driver power operations (was Re: suspend2 merge)

[Rafael J. Wysocki]

On Friday, 27 April 2007 16:34, Alan Stern wrote:
> On Fri, 27 Apr 2007, Johannes Berg wrote:
> 
> > Look at it now:
> > 
> >  * FREEZE       Quiesce operations so that a consistent image can be saved;
> >  *              but do NOT otherwise enter a low power device state, and do
> >  *              NOT emit system wakeup events.
> >  *
> >  * PRETHAW      Quiesce as if for FREEZE; additionally, prepare for restoring
> >  *              the system from a snapshot taken after an earlier FREEZE.
> >  *              Some drivers will need to reset their hardware state instead
> >  *              of preserving it, to ensure that it's never mistaken for the
> >  *              state which that earlier snapshot had set up.
> > 
> > Why is prethaw even necessary? As far as I can tell it's only necessary
> > because resume() can't tell you whether you just want to thaw or need to
> > reset since it doesn't tell you at what point it's invoked.
> 
> I think you're wrong here.  It's a little hard to say because the 
> terminology is confusing and not yet standardized.
> 
> For the sake of argument, let's call the stages of STD and STR by these 
> names (also noted are the current PSMG values):
> 
> 	Suspend to disk:
> 	"prepare to create snapshot" (= FREEZE)
> 	"continue after snapshot" (= RESUME)
> 
> 	Resume from disk:
> 	"prepare to restore snapshot" (= PRETHAW)
> 	"continue after restore" (= RESUME)
> 
> 	Suspend to RAM:
> 	"suspend" (= SUSPEND)
> 	"resume" (= RESUME)
> 
> The real reason for adding PRETHAW was that drivers couldn't distinguish
> between "continue after restore" and "resume", other than by examining the
> device's state -- since the PM core doesn't pass any information to the
> resume() method.
> 
> I suppose we could have modified the "prepare to create snapshot" code 
> instead, but doing so would mean that "continue after snapshot" and 
> "continue after restore" would always do the same thing, which is not 
> necessarily a good idea.
> 
> Anyway, based on this analysis it seems reasonable to have Six (6) method 
> pointers.  Suggested names (in the same order as above):
> 
> 	pre_snaphot()
> 	post_snapshot()
> 	pre_restore()
> 	post_restore()
> 	suspend()
> 	resume()
> 
> People apparently assume that pre_snapshot() and pre_restore() would 
> always do the same thing and hence be redundant.  I'm not so sure; time 
> will tell.  Doing it this way certainly is more clear.

How do we differentiate between post_snapshot() and post_restore()?
I mean, after the restore we're entering the same code path as after the
snapshot, so do we use a global var for this purpose?

Rafael

Re: driver power operations (was Re: suspend2 merge)

[Rafael J. Wysocki]

On Friday, 27 April 2007 16:49, Johannes Berg wrote:
> On Fri, 2007-04-27 at 16:39 +0200, Johannes Berg wrote:
> 
> > Good point. Though if we go for passing the interrupt-enable setting as
> > an argument then many drivers will have the same
> > "if (irqs_disabled()) return" code. Hm. I guess passing it isn't even
> > strictly necessary.
> 
> Eh, the point I actually wanted to make is that many drivers don't care
> for the irqs disabled case and would have to add code to exclude it.

I think we can use 'stages' and pass them as arguments to the functions.

In that case we can have two callbacks for the hibernation (I'd prefer to say
'hibernation' instead of 'suspend to disk' from now on), one 'quiesce' callback
and one 'activate' callback that can be called many times in one
snapshot/restore cycle with different arguments, for example:

quiesce(PREPARE) -- that may be needed for drivers that allocate much memory
before quiescing devices (if any)
...
quiesce(PRE_SNAPSHOT)
...
quiesce(PRE_SNAPSHOT_IRQ_OFF)
...
activate(POST_SNAPSHOT_IRQ_OFF)
...
activate(POST_SNAPSHOT)
...
activate(FINISH)

etc.

Greetings,
Rafael

Re: driver power operations (was Re: suspend2 merge)

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 432 bytes --]

On Fri, 2007-04-27 at 17:12 +0200, Rafael J. Wysocki wrote:

> How do we differentiate between post_snapshot() and post_restore()?
> I mean, after the restore we're entering the same code path as after the
> snapshot, so do we use a global var for this purpose?

That's pretty easy to do though, we already know at which point we are
so we just put an if(...) invoke_post_snapshot() else
invoke_post_restore().

johannes

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

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

Re: driver power operations (was Re: suspend2 merge)

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 942 bytes --]

On Fri, 2007-04-27 at 17:20 +0200, Rafael J. Wysocki wrote:

> I think we can use 'stages' and pass them as arguments to the functions.
> 
> In that case we can have two callbacks for the hibernation (I'd prefer to say
> 'hibernation' instead of 'suspend to disk' from now on), one 'quiesce' callback
> and one 'activate' callback that can be called many times in one
> snapshot/restore cycle with different arguments, for example:

But you're not proposing to add suspend/resume to this interface too, I
hope :)

> quiesce(PREPARE) -- that may be needed for drivers that allocate much memory
> before quiescing devices (if any)
> ...
> quiesce(PRE_SNAPSHOT)
> ...
> quiesce(PRE_SNAPSHOT_IRQ_OFF)
> ...
> activate(POST_SNAPSHOT_IRQ_OFF)
> ...
> activate(POST_SNAPSHOT)
> ...
> activate(FINISH)

I'm still not sure I like having to switch on the argument for every
implementation. Is it really worth it?

johannes

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

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

Re: driver power operations (was Re: suspend2 merge)

[Linus Torvalds]

On Fri, 27 Apr 2007, Johannes Berg wrote:
> 
> Eh, the point I actually wanted to make is that many drivers don't care
> for the irqs disabled case and would have to add code to exclude it.

You really *really* want to do a two-phase thing, at least for the case I 
care about. Whether that snapshotting thing does or not, I could care 
less.

There's a damn good reason why the kernel uses 

	/* phase 1 */
	for_each_dev()
		dev->suspend(dev);

	cli();
	/* phase 2 */
	for_each_dev()
		dev->suspend_late(dev);

(and the reverse case on resume).

The reason is simply that there are two totally different cases: things 
like disks etc want to spin down and do slow and high-level operations, 
while things like USB controllers and console devices do *not* want to be 
suspended early, because if you do, you lose debuggability.

So some things really *really* want to be done when they know that there 
isn't anything else going on any more, and they want to delay the shutdown 
to the very end. While other things really *require* that they can send 
requests that can take time, and cannot run with interrupts disabled.

I actually think that "snapshot" is totally different, exactly because for 
snapshotting, the slow operations like spinning down disks etc probably 
don't really even exist, and would always be no-ops. But who knows..

Anyway, I do have a final comment: 

     DO NOT PASS "STATE FLAGS" TO DRIVERS 

(or, even worse, assume that drivers would test "implicit" state by 
calling the same function under two different states, and then have the 
drivers test for "are interrupts disabled? Then I need to do something 
else").

If drivers are possibly going to do two different things, make it two 
different entry-points. There's absolutely no downsides. It's _clearer_ to 
the device writer when he gets called two different ways that it's not the 
same case, and in case a particular device can do the same thing for both 
cases, he can just set the function pointer to the same entry for both.

Never EVER pass dynamic flags that modify behaviour. It's simply bad 
programming. A function should do *one* thing, and do it well. 

		Linus

Re: driver power operations (was Re: suspend2 merge)

[Linus Torvalds]

On Fri, 27 Apr 2007, Rafael J. Wysocki wrote:
> 
> I think we can use 'stages' and pass them as arguments to the functions.

No, no NOOOO!

If you use stages, just describe them in the function name instead.	

> quiesce(PREPARE) -- that may be needed for drivers that allocate much memory
> before quiescing devices (if any)
> ...
> quiesce(PRE_SNAPSHOT)
> ...
> quiesce(PRE_SNAPSHOT_IRQ_OFF)

There is *no* advantage to this (and _lots_ of disadvantages) compared to 
saying

	dev->snapshot_prepare(dev);
	dev->snapshot_freeze(dev);
	dev->snapshot(dev)

The latter is
 - more readable
 - MUCH easier for programmers to write readable code for (if-statements 
   and case-statements are *by*definition* more complicated to parse both 
   for humans and for CPU's - static information is good)
 - allows for the different stages to have different arguments, and 
   somewhat related to that, to have better static C type checking.

Look here, which one is more readable:

	int some_mixed_function(int arg)
	{
		do_one_thing();
		if (arg == SLEEP)
			do_another_thing();
		else
			do_yet_another_thing();
	}

or

	int do_sleep(void)
	{
		do_one_thing();
		do_another_thing();
	}

	int prepare_to_sleep(void)
	{
		do_one_thing();
		do_yet_another_thing();
	}

and quite frankly, while the second case may take more lines of code, 
anybody who says that it's not clearer what it does (because it can 
"self-document" with function names etc) is either lying, or just a really 
bad programmer. The second case is also likely faster and probably not 
larger code-size-wise either, since it does static decisions _statically_ 
(since all callers are realistically going to use a constant argument 
anyway, and the argument really is static).

Finally, the second case is *much* easier to fix, exactly because it 
doesn't mix up the cases. You can change the arguments, you can have 
totally different locking, you don't need things like

	int gfp = (arg == SLEEP) ? GFP_ATOMIC : GFP_KERNEL;

etc, and it's just more logical.

So don't overload a function. That's the *bug* with the current 
"dev->suspend()" interface already. Don't re-create it. The current one 
overloads two *totally*different* operations onto one function. 

Just don't do it. Not in the suspend part, not *ever*.

		Linus

Re: driver power operations (was Re: suspend2 merge)

[David Brownell]

On Friday 27 April 2007, Johannes Berg wrote:
> 
>  * FREEZE       Quiesce operations so that a consistent image can be saved;
>  *              but do NOT otherwise enter a low power device state, and do
>  *              NOT emit system wakeup events.
>  *
>  * PRETHAW      Quiesce as if for FREEZE; additionally, prepare for restoring
>  *              the system from a snapshot taken after an earlier FREEZE.
>  *              Some drivers will need to reset their hardware state instead
>  *              of preserving it, to ensure that it's never mistaken for the
>  *              state which that earlier snapshot had set up.
> 
> Why is prethaw even necessary?

Read the patch comments for the patch adding that transition.  Briefly,
adding that transition to swsusp resume was a significant bugfix for
all drivers that rely on controller state to determine how to resume.

(That's mostly drivers that are intelligent about wakeup events... so
unless you're working with such drivers, the issue may be unclear.)


> As far as I can tell it's only necessary 
> because resume() can't tell you whether you just want to thaw or need to
> reset since it doesn't tell you at what point it's invoked.

More like:  because swsusp overloaded the suspend()/resume() code paths
to do double duty.

Instead of just putting devices into low power states (just *which* state
is another discussion), they evolved into support for swsusp transitions...
causing trouble (and sometimes breakage) for non-swsusp models.


> Having ->freeze(), ->thaw() and ->restart() (can somebody come up with a
> better name?) that are called at the appropriate places (with
> freeze/thaw around preparing the image and freeze/restart around
> restoring would go a long way of clearing up the confusion in all the
> drivers. Of course, it'd have to be documented that freeze/thaw isn't
> the only valid combination but that freeze/restart is used too, but
> that's not hard to do nor hard to understand.

I suspect that after snapshot resume restart() should always be used.
That shouldn't be hard to understand at all.  It'd be sub-optimal in
the same cases today's system resume is sub-optimal:  devices that
were in low power states before system suspend wouldn't be that way
after system resume.


> And, incidentally, it could possibly make both suspend and hibernate
> work much faster too. The comments there talk about "minimally power
> management aware" drivers which always do the wrong thing for suspend,
> in that they always reset everything...

That comment was purely about existing practice ... and was mostly
about resume() processing, not suspend() paths.

It's an unfortunate reality that most device drivers are stupid in
terms of power management, so we need to be clear about just how
stupid they're allowed to be without being terminally broken.

Additionally, it would be a Good Thing if changes to clean up the
swsusp-related code paths didn't make "real suspend" more painful.


> Of course, some drivers will 
> actually need to do that, but if freeze/suspend and thaw/restart/resume
> have the same prototypes (probably just int <function>(void)) then
> drivers can trivially assign the same there.
> And hibernate would benefit since a lot of drivers could do a lot less
> work for freeze/thaw.

That actually gets into discussions from a while back about wanting
to be able to quiesce() devices, as separate from actually putting
them into low power states.

- Dave

Re: driver power operations (was Re: suspend2 merge)

[Rafael J. Wysocki]

On Friday, 27 April 2007 17:56, David Brownell wrote:
> On Friday 27 April 2007, Johannes Berg wrote:
> > 
> >  * FREEZE       Quiesce operations so that a consistent image can be saved;
> >  *              but do NOT otherwise enter a low power device state, and do
> >  *              NOT emit system wakeup events.
> >  *
> >  * PRETHAW      Quiesce as if for FREEZE; additionally, prepare for restoring
> >  *              the system from a snapshot taken after an earlier FREEZE.
> >  *              Some drivers will need to reset their hardware state instead
> >  *              of preserving it, to ensure that it's never mistaken for the
> >  *              state which that earlier snapshot had set up.
> > 
> > Why is prethaw even necessary?
> 
> Read the patch comments for the patch adding that transition.  Briefly,
> adding that transition to swsusp resume was a significant bugfix for
> all drivers that rely on controller state to determine how to resume.
> 
> (That's mostly drivers that are intelligent about wakeup events... so
> unless you're working with such drivers, the issue may be unclear.)
> 
> 
> > As far as I can tell it's only necessary 
> > because resume() can't tell you whether you just want to thaw or need to
> > reset since it doesn't tell you at what point it's invoked.
> 
> More like:  because swsusp overloaded the suspend()/resume() code paths
> to do double duty.
> 
> Instead of just putting devices into low power states (just *which* state
> is another discussion), they evolved into support for swsusp transitions...
> causing trouble (and sometimes breakage) for non-swsusp models.
> 
> 
> > Having ->freeze(), ->thaw() and ->restart() (can somebody come up with a
> > better name?) that are called at the appropriate places (with
> > freeze/thaw around preparing the image and freeze/restart around
> > restoring would go a long way of clearing up the confusion in all the
> > drivers. Of course, it'd have to be documented that freeze/thaw isn't
> > the only valid combination but that freeze/restart is used too, but
> > that's not hard to do nor hard to understand.
> 
> I suspect that after snapshot resume restart() should always be used.
> That shouldn't be hard to understand at all.  It'd be sub-optimal in
> the same cases today's system resume is sub-optimal:  devices that
> were in low power states before system suspend wouldn't be that way
> after system resume.
> 
> 
> > And, incidentally, it could possibly make both suspend and hibernate
> > work much faster too. The comments there talk about "minimally power
> > management aware" drivers which always do the wrong thing for suspend,
> > in that they always reset everything...
> 
> That comment was purely about existing practice ... and was mostly
> about resume() processing, not suspend() paths.
> 
> It's an unfortunate reality that most device drivers are stupid in
> terms of power management, so we need to be clear about just how
> stupid they're allowed to be without being terminally broken.
> 
> Additionally, it would be a Good Thing if changes to clean up the
> swsusp-related code paths didn't make "real suspend" more painful.
> 
> 
> > Of course, some drivers will 
> > actually need to do that, but if freeze/suspend and thaw/restart/resume
> > have the same prototypes (probably just int <function>(void)) then
> > drivers can trivially assign the same there.
> > And hibernate would benefit since a lot of drivers could do a lot less
> > work for freeze/thaw.
> 
> That actually gets into discussions from a while back about wanting
> to be able to quiesce() devices, as separate from actually putting
> them into low power states.

Yes, exactly.

Moreover, I think we should separate the current suspend code from the
hibernation (aka STD) code paths we're discussing.  I mean, we need
hibernation-specific equivalents of drivers/base/power/suspend.c etc.

Greetings,
Rafael

Re: driver power operations (was Re: suspend2 merge)

[Rafael J. Wysocki]

On Friday, 27 April 2007 17:52, Linus Torvalds wrote:
> 
> On Fri, 27 Apr 2007, Rafael J. Wysocki wrote:
> > 
> > I think we can use 'stages' and pass them as arguments to the functions.
> 
> No, no NOOOO!
> 
> If you use stages, just describe them in the function name instead.	
> 
> > quiesce(PREPARE) -- that may be needed for drivers that allocate much memory
> > before quiescing devices (if any)
> > ...
> > quiesce(PRE_SNAPSHOT)
> > ...
> > quiesce(PRE_SNAPSHOT_IRQ_OFF)
> 
> There is *no* advantage to this (and _lots_ of disadvantages) compared to 
> saying
> 
> 	dev->snapshot_prepare(dev);
> 	dev->snapshot_freeze(dev);
> 	dev->snapshot(dev)
> 
> The latter is
>  - more readable
>  - MUCH easier for programmers to write readable code for (if-statements 
>    and case-statements are *by*definition* more complicated to parse both 
>    for humans and for CPU's - static information is good)
>  - allows for the different stages to have different arguments, and 
>    somewhat related to that, to have better static C type checking.
> 
> Look here, which one is more readable:
> 
> 	int some_mixed_function(int arg)
> 	{
> 		do_one_thing();
> 		if (arg == SLEEP)
> 			do_another_thing();
> 		else
> 			do_yet_another_thing();
> 	}
> 
> or
> 
> 	int do_sleep(void)
> 	{
> 		do_one_thing();
> 		do_another_thing();
> 	}
> 
> 	int prepare_to_sleep(void)
> 	{
> 		do_one_thing();
> 		do_yet_another_thing();
> 	}
> 
> and quite frankly, while the second case may take more lines of code, 
> anybody who says that it's not clearer what it does (because it can 
> "self-document" with function names etc) is either lying, or just a really 
> bad programmer. The second case is also likely faster and probably not 
> larger code-size-wise either, since it does static decisions _statically_ 
> (since all callers are realistically going to use a constant argument 
> anyway, and the argument really is static).
> 
> Finally, the second case is *much* easier to fix, exactly because it 
> doesn't mix up the cases. You can change the arguments, you can have 
> totally different locking, you don't need things like
> 
> 	int gfp = (arg == SLEEP) ? GFP_ATOMIC : GFP_KERNEL;
> 
> etc, and it's just more logical.
> 
> So don't overload a function. That's the *bug* with the current 
> "dev->suspend()" interface already. Don't re-create it. The current one 
> overloads two *totally*different* operations onto one function. 
> 
> Just don't do it. Not in the suspend part, not *ever*.

OK, I won't.

Greetings,
Rafael

Re: driver power operations (was Re: suspend2 merge)

[Pavel Machek]

Hi!

> > > So, the "suspend" and "resume" for the functions being called for that are
> > > wrong, but then we call them with PMSG_FREEZE. ;-)  Still, we could add
> > > .freeze() and .thaw() callbacks for hibernation just fine.  This wouldn't even
> > > be that difficult ...
> > 
> > It would be ugly big patch I'm afraid.
> 
> It'd be a lot of code churn, but well worth it. And most of the changes
> would be trivial too. You need to start looking beyond "this is ugly in
> the short term" and realise that it's much more maintainable in the long
> term if driver writers know what they're supposed to do as opposed to
> just hacking at it until it mostly works or just doing a full device
> down/up cycle including resetting full driver state.

I do not disagree with you. It will be ugly big patch, but it is
probably worth it, so the patch will be welcome.

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