[RFC] Add some hooks to generic suspend code

[RFC] Add some hooks to generic suspend code

[Benjamin Herrenschmidt]

Hi !

While consolidating the powermac suspend to ram and suspend to disk
implementations to properly use the new framework in kernel/power, among
others, I ended up with the need of adding various callbacks to
kernel/power/main.c. Here is a patch adding & documenting those.

The reasons I need them are:

	/* Call before process freezing. If returns 0, then no freeze
	 * should be done, if 1, freeze, negative -> error
	 */
	int (*pre_freeze)(suspend_state_t state);

I'm using that one for calling my "old style" notifiers (they are beeing phased
out but I still have a couple of drivers using them). The reason I do that here
is because that's how my APM emulation hooks, and that code interacts with userland
(to properly signal things like X of the suspend process), so I need to do that
before we freeze processes.

This call also allow me to decide not to freeze process (in my suspend-to-ram
implementation) by returning 0 (the absence of the callback defaults to freeze
of course, to not break existing code). So far, I have been using the freezer
though, as it seem to be fast enough, but I still dislike relying on it for
suspend to ram, it just hides bugs imho, and I've had a couple of problems with
it where it failed to stop a process occasionally, or lost track of one on
wakeup (and left it in D state in the refrigerator forever) for reasons I haven't
yet explained.


	/* called just before irqs are off and device second pass
	 * and sysdevs are suspended. This function can on some archs
	 * shut irqs off, in which case, they'll still be off when
	 * finish_irqs() is called.
	 */
	int (*prepare_irqs)(suspend_state_t state);

I use that one to do some housekeeping that I want to do before the interrupts are
switched off, that is after device_suspend() and before device_power_off(). That
includes some mucking around with the interrupt controller (so that it doesn't try
to deliver interrupts to a CPU that won't take them, that avoids some "issues" on
wakeup), plus a couple of other things that don't quite fit in sysdev's and that
I want to do before sysdev's get suspended.

Additionally, I shut interrupts down (with local_irq_disable) myself in that callback,
which means that the "common" code which does local_irq_save/restore will end up not
re-enabling them before it calls the "mirror" callback of that one (see below the
explanation of finish_irqs), which I do need.


	/* called after sysdevs and "irq off" devices have been
	 * worken up, irqs have just been restored to whatever state
	 * prepare_irqs() left them in.
	 */
	void (*finish_irqs)(suspend_state_t state);

This is the pending of the above callback. It gets called after sysdev's
have been woken up but before normal devices have. It's called after the core has
restored local interrupts to what they were upon exit of prepare_irqs(), so if
you do nothing special in prepare_irqs(), you'll get entered with irqs re-enabled
here, while if you exit prepare_irqs() with irqs off, you'll get here with irqs
off as well (and thus become responsible for re-enabling them).

I want this callback to have finer control of re-enabling interrutps. The interrupt
controller has been partially reconfigured earlier in arch code, but the CPU priority
is only lowered here, so that it starts hitting the CPU again only now. There is
also some code to properly wake up the CPU decrementer so it ticks right away, and
to force taking a pseudo-interrupt (to sort-of "kick" the interrupt controller into
life, seems to work around an issue that I think is related to a HW bug in the
interrupt controller we use). 

I also do some arch magic at this point, mirror of what was done in prepare_irqs(),
that doesn't quite fit in a sysdev.


	/* called after unfreezing userland */
	void (*post_freeze)(suspend_state_t state);

That one is the mirror of pre-freeze, gets called after userland has been re-enabled,
it also calls my old-style notifiers, which includes APM emulation, which is important
for sending the APM wakeup events to things like X.


Please comment, I would like these patches in early 2.6.13 stages,

Thanks,
Ben.


Index: linux-work/include/linux/pm.h
===================================================================
--- linux-work.orig/include/linux/pm.h	2005-05-31 16:29:22.000000000 +1000
+++ linux-work/include/linux/pm.h	2005-05-31 16:57:29.000000000 +1000
@@ -169,9 +169,38 @@
 
 struct pm_ops {
 	suspend_disk_method_t pm_disk_mode;
+	
+	/* Call before process freezing. If returns 0, then no freeze
+	 * should be done, if 1, freeze, negative -> error
+	 */
+	int (*pre_freeze)(suspend_state_t state);
+
+	/* called before devices are suspended */
 	int (*prepare)(suspend_state_t state);
+
+	/* called just before irqs are off and device second pass
+	 * and sysdevs are suspended. This function can on some archs
+	 * shut irqs off, in which case, they'll still be off when
+	 * finish_irqs() is called.
+	 */
+	int (*prepare_irqs)(suspend_state_t state);
+
+	/* called for entering the actual suspend state. Exits with
+	 * machine worken up and interrupts off
+	 */
 	int (*enter)(suspend_state_t state);
-	int (*finish)(suspend_state_t state);
+
+	/* called after sysdevs and "irq off" devices have been
+	 * worken up, irqs have just been restored to whatever state
+	 * prepare_irqs() left them in.
+	 */
+	void (*finish_irqs)(suspend_state_t state);
+
+	/* called after all devices are woken up, processes still frozen */
+	void (*finish)(suspend_state_t state);
+
+	/* called after unfreezing userland */
+	void (*post_freeze)(suspend_state_t state);
 };
 
 extern void pm_set_ops(struct pm_ops *);
Index: linux-work/kernel/power/main.c
===================================================================
--- linux-work.orig/kernel/power/main.c	2005-05-31 16:29:22.000000000 +1000
+++ linux-work/kernel/power/main.c	2005-05-31 16:57:29.000000000 +1000
@@ -23,6 +23,7 @@
 
 struct pm_ops * pm_ops = NULL;
 suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
+static int pm_process_frozen;
 
 /**
  *	pm_set_ops - Set the global power method table. 
@@ -49,32 +50,53 @@
 static int suspend_prepare(suspend_state_t state)
 {
 	int error = 0;
+	int freeze = 1;
 
 	if (!pm_ops || !pm_ops->enter)
 		return -EPERM;
 
 	pm_prepare_console();
 
-	if (freeze_processes()) {
+	if (pm_ops->pre_freeze)
+		freeze = pm_ops->pre_freeze(state);
+	if (freeze < 0)
+		goto Console;
+
+	if (freeze && freeze_processes()) {
 		error = -EAGAIN;
 		goto Thaw;
 	}
+	pm_process_frozen = freeze;
 
 	if (pm_ops->prepare) {
+		pr_debug("preparing arch...\n");
 		if ((error = pm_ops->prepare(state)))
 			goto Thaw;
 	}
 
+	pr_debug("suspending devices...\n");
 	if ((error = device_suspend(PMSG_SUSPEND))) {
 		printk(KERN_ERR "Some devices failed to suspend\n");
 		goto Finish;
 	}
+
+	if (pm_ops->prepare_irqs) {
+		pr_debug("preparing arch irqs...\n");
+		if ((error = pm_ops->prepare_irqs(state)))
+			goto Finish;
+	}
+
 	return 0;
  Finish:
 	if (pm_ops->finish)
 		pm_ops->finish(state);
  Thaw:
-	thaw_processes();
+	if (freeze)
+		thaw_processes();
+
+	if (pm_ops->post_freeze)
+		pm_ops->post_freeze(state);
+ Console:
 	pm_restore_console();
 	return error;
 }
@@ -109,10 +131,18 @@
 
 static void suspend_finish(suspend_state_t state)
 {
+	if (!pm_ops)
+		return;
+
+	if (pm_ops->finish_irqs)
+		pm_ops->finish_irqs(state);
 	device_resume();
-	if (pm_ops && pm_ops->finish)
+	if (pm_ops->finish)
 		pm_ops->finish(state);
-	thaw_processes();
+	if (pm_process_frozen)
+		thaw_processes();
+	if (pm_ops->post_freeze)
+		pm_ops->post_freeze(state);
 	pm_restore_console();
 }
 

Re: [linux-pm] [RFC] Add some hooks to generic suspend code

[Pavel Machek]

Hi!

> While consolidating the powermac suspend to ram and suspend to disk
> implementations to properly use the new framework in kernel/power, among
> others, I ended up with the need of adding various callbacks to
> kernel/power/main.c. Here is a patch adding & documenting those.
> 
> The reasons I need them are:
> 
> 	/* Call before process freezing. If returns 0, then no freeze
> 	 * should be done, if 1, freeze, negative -> error
> 	 */
> 	int (*pre_freeze)(suspend_state_t state);
> 
> I'm using that one for calling my "old style" notifiers (they are beeing phased
> out but I still have a couple of drivers using them). The reason I do that here
> is because that's how my APM emulation hooks, and that code interacts with userland
> (to properly signal things like X of the suspend process), so I need to do that
> before we freeze processes.

This should not be needed in future, right? Could it be marked
deprecated or something?

> 	/* called after sysdevs and "irq off" devices have been
> 	 * worken up, irqs have just been restored to whatever state
> 	 * prepare_irqs() left them in.
> 	 */
> 	void (*finish_irqs)(suspend_state_t state);
> 
> This is the pending of the above callback. It gets called after sysdev's
> have been woken up but before normal devices have. It's called after the core has
> restored local interrupts to what they were upon exit of prepare_irqs(), so if
> you do nothing special in prepare_irqs(), you'll get entered with irqs re-enabled
> here, while if you exit prepare_irqs() with irqs off, you'll get here with irqs
> off as well (and thus become responsible for re-enabling them).
> 
> I want this callback to have finer control of re-enabling interrutps. The interrupt
> controller has been partially reconfigured earlier in arch code, but the CPU priority
> is only lowered here, so that it starts hitting the CPU again only now. There is
> also some code to properly wake up the CPU decrementer so it ticks right away, and
> to force taking a pseudo-interrupt (to sort-of "kick" the interrupt controller into
> life, seems to work around an issue that I think is related to a HW bug in the
> interrupt controller we use). 

Could you simply reconfigure interrupt controller fully in earlier arch code?

> 	/* called after unfreezing userland */
> 	void (*post_freeze)(suspend_state_t state);
> 
> That one is the mirror of pre-freeze, gets called after userland has been re-enabled,
> it also calls my old-style notifiers, which includes APM emulation, which is important
> for sending the APM wakeup events to things like X.

Could this be marked deprecated, too?

Alternatively, proper way of notifying X (etc) should be created, and
done from generic code....

									Pavel

Re: [linux-pm] [RFC] Add some hooks to generic suspend code

[Benjamin Herrenschmidt]

On Tue, 2005-05-31 at 12:13 +0200, Pavel Machek wrote:
> Hi!
> 
> > While consolidating the powermac suspend to ram and suspend to disk
> > implementations to properly use the new framework in kernel/power, among
> > others, I ended up with the need of adding various callbacks to
> > kernel/power/main.c. Here is a patch adding & documenting those.
> > 
> > The reasons I need them are:
> > 
> > 	/* Call before process freezing. If returns 0, then no freeze
> > 	 * should be done, if 1, freeze, negative -> error
> > 	 */
> > 	int (*pre_freeze)(suspend_state_t state);
> > 
> > I'm using that one for calling my "old style" notifiers (they are beeing phased
> > out but I still have a couple of drivers using them). The reason I do that here
> > is because that's how my APM emulation hooks, and that code interacts with userland
> > (to properly signal things like X of the suspend process), so I need to do that
> > before we freeze processes.
> 
> This should not be needed in future, right? Could it be marked
> deprecated or something?

Not really ... I need to notify userland before we freeze processes.

> > 	/* called after sysdevs and "irq off" devices have been
> > 	 * worken up, irqs have just been restored to whatever state
> > 	 * prepare_irqs() left them in.
> > 	 */
> > 	void (*finish_irqs)(suspend_state_t state);
> > 
> > This is the pending of the above callback. It gets called after sysdev's
> > have been woken up but before normal devices have. It's called after the core has
> > restored local interrupts to what they were upon exit of prepare_irqs(), so if
> > you do nothing special in prepare_irqs(), you'll get entered with irqs re-enabled
> > here, while if you exit prepare_irqs() with irqs off, you'll get here with irqs
> > off as well (and thus become responsible for re-enabling them).
> > 
> > I want this callback to have finer control of re-enabling interrutps. The interrupt
> > controller has been partially reconfigured earlier in arch code, but the CPU priority
> > is only lowered here, so that it starts hitting the CPU again only now. There is
> > also some code to properly wake up the CPU decrementer so it ticks right away, and
> > to force taking a pseudo-interrupt (to sort-of "kick" the interrupt controller into
> > life, seems to work around an issue that I think is related to a HW bug in the
> > interrupt controller we use). 
> 
> Could you simply reconfigure interrupt controller fully in earlier arch code?

Nope, it has to happen between device_suspend, ant device_power_off.
Basically, I need control around the time we switch irqs off.

> > 	/* called after unfreezing userland */
> > 	void (*post_freeze)(suspend_state_t state);
> > 
> > That one is the mirror of pre-freeze, gets called after userland has been re-enabled,
> > it also calls my old-style notifiers, which includes APM emulation, which is important
> > for sending the APM wakeup events to things like X.
> 
> Could this be marked deprecated, too?
> 
> Alternatively, proper way of notifying X (etc) should be created, and
> done from generic code....

Sure, ideally. However, existing X knows how to deal with APM events,
and thus APM emulation is an important thing to get something that
works. Pne thing I should do is consolidate PPC APM emu with ARM one as
I think Russell improve my stuff significantly.

Ben.

Re: [linux-pm] [RFC] Add some hooks to generic suspend code

[Pavel Machek]

Hi!

> > > While consolidating the powermac suspend to ram and suspend to disk
> > > implementations to properly use the new framework in kernel/power, among
> > > others, I ended up with the need of adding various callbacks to
> > > kernel/power/main.c. Here is a patch adding & documenting those.
> > > 
> > > The reasons I need them are:
> > > 
> > > 	/* Call before process freezing. If returns 0, then no freeze
> > > 	 * should be done, if 1, freeze, negative -> error
> > > 	 */
> > > 	int (*pre_freeze)(suspend_state_t state);
> > > 
> > > I'm using that one for calling my "old style" notifiers (they are beeing phased
> > > out but I still have a couple of drivers using them). The reason I do that here
> > > is because that's how my APM emulation hooks, and that code interacts with userland
> > > (to properly signal things like X of the suspend process), so I need to do that
> > > before we freeze processes.
> > 
> > This should not be needed in future, right? Could it be marked
> > deprecated or something?
> 
> Not really ... I need to notify userland before we freeze processes.

Why do you need it? Do you initiate suspend without userland asking
you to?

Anyway, it should not be arch-dependend. We need one good mechanism of
notifying userland, not one per architecture.

> > > 	/* called after unfreezing userland */
> > > 	void (*post_freeze)(suspend_state_t state);
> > > 
> > > That one is the mirror of pre-freeze, gets called after userland has been re-enabled,
> > > it also calls my old-style notifiers, which includes APM emulation, which is important
> > > for sending the APM wakeup events to things like X.
> > 
> > Could this be marked deprecated, too?
> > 
> > Alternatively, proper way of notifying X (etc) should be created, and
> > done from generic code....
> 
> Sure, ideally. However, existing X knows how to deal with APM events,
> and thus APM emulation is an important thing to get something that
> works. Pne thing I should do is consolidate PPC APM emu with ARM one as
> I think Russell improve my stuff significantly.

Perhaps we need apm emulation on i386, too?
									Pavel

Re: [linux-pm] [RFC] Add some hooks to generic suspend code

[Benjamin Herrenschmidt]

> Why do you need it? Do you initiate suspend without userland asking
> you to?

Because there is an existing API, via /dev/apm_bios, and that's all X
understands ! And because I've always done that ;)

> Anyway, it should not be arch-dependend. We need one good mechanism of
> notifying userland, not one per architecture.

We need to define a new mecanism, I think. In the meantime, my APM
emulation works though and I won't drop it. 

> > > > 	/* called after unfreezing userland */
> > > > 	void (*post_freeze)(suspend_state_t state);
> > > > 
> > > > That one is the mirror of pre-freeze, gets called after userland has been re-enabled,
> > > > it also calls my old-style notifiers, which includes APM emulation, which is important
> > > > for sending the APM wakeup events to things like X.
> > > 
> > > Could this be marked deprecated, too?
> > > 
> > > Alternatively, proper way of notifying X (etc) should be created, and
> > > done from generic code....
> > 
> > Sure, ideally. However, existing X knows how to deal with APM events,
> > and thus APM emulation is an important thing to get something that
> > works. Pne thing I should do is consolidate PPC APM emu with ARM one as
> > I think Russell improve my stuff significantly.
> 
> Perhaps we need apm emulation on i386, too?

Maybe. It may help in some cases.

Ben.

Re: [linux-pm] [RFC] Add some hooks to generic suspend code

[Pavel Machek]

Hi!

> > Why do you need it? Do you initiate suspend without userland asking
> > you to?
> 
> Because there is an existing API, via /dev/apm_bios, and that's all X
> understands ! And because I've always done that ;)

Try stopping doing that ;-).

[On i386, we do not emulate apm, and it still works. Reason is that we
switch to other console before suspend, so X has to give up
framebuffer control, anyway.]

								Pavel

Re: [linux-pm] [RFC] Add some hooks to generic suspend code

[Benjamin Herrenschmidt]

On Wed, 2005-06-01 at 10:13 +0200, Pavel Machek wrote:
> Hi!
> 
> > > Why do you need it? Do you initiate suspend without userland asking
> > > you to?
> > 
> > Because there is an existing API, via /dev/apm_bios, and that's all X
> > understands ! And because I've always done that ;)
> 
> Try stopping doing that ;-).

Certainly not short-term. Again, it would be nice to have something
better, but heh, you need to go step by step. I have this big rework
where I re-implement most of the pmac suspend code on top of the generic
code (cleans up a lot of stuff) but I don't want to touch the userland
ABI for now, that would be too much of a chance. And /dev/apm_bios X
notofication stuff seems to actually fix problems for some users.

> [On i386, we do not emulate apm, and it still works. Reason is that we
> switch to other console before suspend, so X has to give up
> framebuffer control, anyway.]

Well, I sort-of work :) I have reported cases of X locking the machine
up under some circumstances. Note that historically, I was not switching
consoles in the pmac PM code, though I'm doing it nowadays.

There are other uses of those "events" in /dev/apm_bios. Some people run
scripts on resume triggered by these for example, etc...

I'd rather not break an existing and relied upon userland interface now,
at least not until we have a well accepted replacement that has been
around for some time.

I do agree however that it may be nice to make the APM emulation code
more generic & shared between architectures. That's something I intend
to look into next. But I would like my current stuff to get in after
2.6.12 is released.

Ben.

Re: [linux-pm] [RFC] Add some hooks to generic suspend code

[Pavel Machek]

Hi!

> > > > Why do you need it? Do you initiate suspend without userland asking
> > > > you to?
> > > 
> > > Because there is an existing API, via /dev/apm_bios, and that's all X
> > > understands ! And because I've always done that ;)
> > 
> > Try stopping doing that ;-).
> 
> Certainly not short-term. Again, it would be nice to have something
> better, but heh, you need to go step by step. I have this big rework
> where I re-implement most of the pmac suspend code on top of the generic
> code (cleans up a lot of stuff) but I don't want to touch the userland
> ABI for now, that would be too much of a chance. And /dev/apm_bios X
> notofication stuff seems to actually fix problems for some users.

Ok.

> I'd rather not break an existing and relied upon userland interface now,
> at least not until we have a well accepted replacement that has been
> around for some time.
> 
> I do agree however that it may be nice to make the APM emulation code
> more generic & shared between architectures. That's something I intend
> to look into next. But I would like my current stuff to get in after
> 2.6.12 is released.

Well, but that means that we can get those "please don't use these
callbacks if you can avoid it" messages, right :-).

Seems like lots of stuff is going to happen in pm post-2.6.12: I'd
like to finally fix pm_message_t, too...
								Pavel

Re: [linux-pm] [RFC] Add some hooks to generic suspend code

[Nigel Cunningham]

Hi.

On Wed, 2005-06-01 at 19:06, Pavel Machek wrote:
> Seems like lots of stuff is going to happen in pm post-2.6.12: I'd
> like to finally fix pm_message_t, too...

Speaking of which, how do you want to move forward with the refrigerator
patches I sent you a while back?

Regards,

Nigel

Re: [linux-pm] [RFC] Add some hooks to generic suspend code

[Pavel Machek]

Hi!

> > Seems like lots of stuff is going to happen in pm post-2.6.12: I'd
> > like to finally fix pm_message_t, too...
> 
> Speaking of which, how do you want to move forward with the refrigerator
> patches I sent you a while back?

Can you send them again? I'll either comment on them or integrate them
into tree for linus (linux-good).
								Pavel

Re: [linux-pm] [RFC] Add some hooks to generic suspend code

[Benjamin Herrenschmidt]

> Well, but that means that we can get those "please don't use these
> callbacks if you can avoid it" messages, right :-).

Heh, well, I'll look if I can do something more specific to APM emu
there, maybe I'll find time to just switch over to a generic
implementation.

> Seems like lots of stuff is going to happen in pm post-2.6.12: I'd
> like to finally fix pm_message_t, too...

Yup. I have a couple of driver patches fixing things in this area too.

Ben.

Re: [RFC] Add some hooks to generic suspend code

[Stefan Seyfried]

Pavel Machek wrote:

> Anyway, it should not be arch-dependend. We need one good mechanism of
> notifying userland, not one per architecture.

Yes.

>> Sure, ideally. However, existing X knows how to deal with APM events,
>> and thus APM emulation is an important thing to get something that
>> works. Pne thing I should do is consolidate PPC APM emu with ARM one as
>> I think Russell improve my stuff significantly.
> 
> Perhaps we need apm emulation on i386, too?

No. This is too ugly for words IMO. If we have one good mechanism of
notifying userland, X can use this mechanism. Let's kill APM, not keep
it alive.
-- 
Stefan Seyfried
QA / R&D Team Mobile Devices        |              "Any ideas, John?"
SUSE LINUX Products GmbH, Nürnberg  | "Well, surrounding them's out."

Re: [RFC] Add some hooks to generic suspend code

[Benjamin Herrenschmidt]

On Thu, 2005-06-02 at 18:21 +0200, Stefan Seyfried wrote:

> >> Sure, ideally. However, existing X knows how to deal with APM events,
> >> and thus APM emulation is an important thing to get something that
> >> works. Pne thing I should do is consolidate PPC APM emu with ARM one as
> >> I think Russell improve my stuff significantly.
> > 
> > Perhaps we need apm emulation on i386, too?
> 
> No. This is too ugly for words IMO. If we have one good mechanism of
> notifying userland, X can use this mechanism. Let's kill APM, not keep
> it alive.

Euh... maybe but I still think we need to keep this userland interface
alive for a little while. At least ARM and PPC have existing stuff that
rely on it.

Ben.