[PATCH] pm_ops: add irq enable/disable hooks

[PATCH] pm_ops: add irq enable/disable hooks

[Johannes Berg]

For powermac, we need to do some things between suspending devices and
device_power_off, for example setting the decrementer. This patch
introduces pm_ops.irq_off and pm_ops.irq_on which will be called instead
of disabling/enabling irqs so platforms can do a bit more work there if
necessary.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
My previous patches moving to the pm_ops interface for powermac were
buggy due to exactly this issue, but the bug never surfaced on my
machine, only on machines with an Apple Desktop Bus (or an emulation
thereof)

Does this look ok to you? Would you want different names? Should I stick
in a BUG_ON(interrupts_enabled()) or such?

 include/linux/pm.h  |   10 ++++++++++
 kernel/power/main.c |   10 ++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

--- wireless-dev.orig/include/linux/pm.h	2007-04-05 18:14:07.948549941 +0200
+++ wireless-dev/include/linux/pm.h	2007-04-05 23:15:29.934829459 +0200
@@ -131,9 +131,17 @@ typedef int __bitwise suspend_disk_metho
  * @prepare: Prepare the platform for the given suspend state. Can return a
  *	negative error code if necessary.
  *
+ * @irq_off: If assigned, the generic suspend code does not turn off IRQs
+ *	but relies on this callback instead. It is currently not called for
+ *	%PM_SUSPEND_DISK.
+ *
  * @enter: Enter the given suspend state, must be assigned. Can return a
  *	negative error code if necessary.
  *
+ * @irq_on: If assigned, the generic suspend code does not turn on IRQs
+ *	but relies on this callback instead. It is currently not called for
+ *	%PM_SUSPEND_DISK.
+ *
  * @finish: Called when the system has left the given state and all devices
  *	are resumed. The return value is ignored.
  *
@@ -152,7 +160,9 @@ typedef int __bitwise suspend_disk_metho
 struct pm_ops {
 	int (*valid)(suspend_state_t state);
 	int (*prepare)(suspend_state_t state);
+	void (*irq_off)(suspend_state_t state);
 	int (*enter)(suspend_state_t state);
+	void (*irq_on)(suspend_state_t state);
 	int (*finish)(suspend_state_t state);
 	suspend_disk_method_t pm_disk_mode;
 };
--- wireless-dev.orig/kernel/power/main.c	2007-04-05 18:14:07.988549941 +0200
+++ wireless-dev/kernel/power/main.c	2007-04-05 18:25:21.108549941 +0200
@@ -117,7 +117,10 @@ int suspend_enter(suspend_state_t state)
 	int error = 0;
 	unsigned long flags;
 
-	local_irq_save(flags);
+	if (pm_ops->irq_off)
+		pm_ops->irq_off(state);
+	else
+		local_irq_save(flags);
 
 	if ((error = device_power_down(PMSG_SUSPEND))) {
 		printk(KERN_ERR "Some devices failed to power down\n");
@@ -126,7 +129,10 @@ int suspend_enter(suspend_state_t state)
 	error = pm_ops->enter(state);
 	device_power_up();
  Done:
-	local_irq_restore(flags);
+	if (pm_ops->irq_on)
+		pm_ops->irq_on(state);
+	else
+		local_irq_restore(flags);
 	return error;
 }
 

Re: [PATCH] pm_ops: add irq enable/disable hooks

[Rafael J. Wysocki]

On Thursday, 5 April 2007 23:54, Johannes Berg wrote:
> For powermac, we need to do some things between suspending devices and
> device_power_off, for example setting the decrementer. This patch
> introduces pm_ops.irq_off and pm_ops.irq_on which will be called instead
> of disabling/enabling irqs so platforms can do a bit more work there if
> necessary.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> 
> ---
> My previous patches moving to the pm_ops interface for powermac were
> buggy due to exactly this issue, but the bug never surfaced on my
> machine, only on machines with an Apple Desktop Bus (or an emulation
> thereof)
> 
> Does this look ok to you? Would you want different names? Should I stick
> in a BUG_ON(interrupts_enabled()) or such?

Well is this possible to do something like

if (pm_ops->do_something_before_disabling_irqs)
	pm_ops->do_something_before_disabling_irqs()
local_irq_save(flags);
if (pm_ops->do_something_after_disabling_irqs)
	pm_ops->do_something_after_disabling_irqs()

and analogously for enabling the IRQs?

Rafael

Re: [PATCH] pm_ops: add irq enable/disable hooks

[Johannes Berg]

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

On Fri, 2007-04-06 at 01:30 +0200, Rafael J. Wysocki wrote:

> Well is this possible to do something like
> 
> if (pm_ops->do_something_before_disabling_irqs)
> 	pm_ops->do_something_before_disabling_irqs()
> local_irq_save(flags);
> if (pm_ops->do_something_after_disabling_irqs)
> 	pm_ops->do_something_after_disabling_irqs()
> 
> and analogously for enabling the IRQs?

Ultimately yes, but is it worth the added complexity? Somebody mucking
with pm_ops has to know what he's doing anyway.

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: [PATCH] pm_ops: add irq enable/disable hooks

[Rafael J. Wysocki]

On Friday, 6 April 2007 01:28, Johannes Berg wrote:
> On Fri, 2007-04-06 at 01:30 +0200, Rafael J. Wysocki wrote:
> 
> > Well is this possible to do something like
> > 
> > if (pm_ops->do_something_before_disabling_irqs)
> > 	pm_ops->do_something_before_disabling_irqs()
> > local_irq_save(flags);
> > if (pm_ops->do_something_after_disabling_irqs)
> > 	pm_ops->do_something_after_disabling_irqs()
> > 
> > and analogously for enabling the IRQs?
> 
> Ultimately yes, but is it worth the added complexity? Somebody mucking
> with pm_ops has to know what he's doing anyway.

Well, this seems to be more natural ("if you want to do something before/after
disabling the IRQs, define it" instead of "you can do something instead of
calling local_irq_save(), but please remember to disable the IRQs yourself
in that case").

BTW, it need not be in pm_ops (actually, why should it be there?).
Alternatively, you can define something like arch_prepare_for_disabling_irqs()
and/or arch_prepare_device_power_down() that will be empty on x86, for
example.

BTW2, I think BUG_ON(irqs_enabled) is needed after the arch does something
instead of or after local_irq_save().

Rafael

Re: [PATCH] pm_ops: add irq enable/disable hooks

[Johannes Berg]

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

On Fri, 2007-04-06 at 02:02 +0200, Rafael J. Wysocki wrote:

> Well, this seems to be more natural ("if you want to do something before/after
> disabling the IRQs, define it" instead of "you can do something instead of
> calling local_irq_save(), but please remember to disable the IRQs yourself
> in that case").

Heh. Yeah, I guess. It just didn't seem worth it. I personally don't
care, I just need to be able to get at those spots.

> BTW, it need not be in pm_ops (actually, why should it be there?).
> Alternatively, you can define something like arch_prepare_for_disabling_irqs()
> and/or arch_prepare_device_power_down() that will be empty on x86, for
> example.

Not sure, it might be different for different suspend methods. We
actually need to do some platform-function stuff inbetween, and if we
ever want some S4-like state then we might need to do it differently.

> BTW2, I think BUG_ON(irqs_enabled) is needed after the arch does something
> instead of or after local_irq_save().

Sure, I can add that. Probably also in the resume path.

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: [PATCH] pm_ops: add irq enable/disable hooks

[Rafael J. Wysocki]

On Friday, 6 April 2007 02:09, Johannes Berg wrote:
> On Fri, 2007-04-06 at 02:02 +0200, Rafael J. Wysocki wrote:
> 
> > Well, this seems to be more natural ("if you want to do something before/after
> > disabling the IRQs, define it" instead of "you can do something instead of
> > calling local_irq_save(), but please remember to disable the IRQs yourself
> > in that case").
> 
> Heh. Yeah, I guess. It just didn't seem worth it. I personally don't
> care, I just need to be able to get at those spots.
> 
> > BTW, it need not be in pm_ops (actually, why should it be there?).
> > Alternatively, you can define something like arch_prepare_for_disabling_irqs()
> > and/or arch_prepare_device_power_down() that will be empty on x86, for
> > example.
> 
> Not sure, it might be different for different suspend methods. We
> actually need to do some platform-function stuff inbetween, and if we
> ever want some S4-like state then we might need to do it differently.

Ah, OK

Rafael

Re: [PATCH] pm_ops: add irq enable/disable hooks

[Johannes Berg]

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

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

> > Not sure, it might be different for different suspend methods. We
> > actually need to do some platform-function stuff inbetween, and if we
> > ever want some S4-like state then we might need to do it differently.
> 
> Ah, OK

Keep in mind that I don't know that yet, and am not totally sure I ever
will implement something S4-like (it would probably require kexec or
similar tricks). Also, these handlers are not even called fro the
suspend to disk case right now (and documented that way.)

I will repost with some BUG_ON() assertions, but should I change it to
have 4 handlers before_irq_off/after_irq_off/before_irq_on/after_irq_on
instead of the two I have now?

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: [PATCH] pm_ops: add irq enable/disable hooks

[Rafael J. Wysocki]

On Friday, 6 April 2007 10:48, Johannes Berg wrote:
> On Fri, 2007-04-06 at 02:17 +0200, Rafael J. Wysocki wrote:
> 
> > > Not sure, it might be different for different suspend methods. We
> > > actually need to do some platform-function stuff inbetween, and if we
> > > ever want some S4-like state then we might need to do it differently.
> > 
> > Ah, OK
> 
> Keep in mind that I don't know that yet, and am not totally sure I ever
> will implement something S4-like (it would probably require kexec or
> similar tricks). Also, these handlers are not even called fro the
> suspend to disk case right now (and documented that way.)
> 
> I will repost with some BUG_ON() assertions, but should I change it to
> have 4 handlers before_irq_off/after_irq_off/before_irq_on/after_irq_on
> instead of the two I have now?

Frankly, I'm not sure.

For practical purposes the BUG_ON() assertions will suffice, so I think you
can keep the two handlers.  I'd change the names, though, to something
like quiesce() and activate(), for example.

[Hm, it feels more appropriate to define them for all platforms and make them
call local_irq_save() on the platforms that don't need to do anything more.]

BTW, please remember to update the SNAPSHOT_S2RAM ioctl accordingly (well,
I think we should move the common code to a separate function).

Greetings,
Rafael

Re: [PATCH] pm_ops: add irq enable/disable hooks

[Johannes Berg]

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

On Fri, 2007-04-06 at 11:41 +0200, Rafael J. Wysocki wrote:

> Frankly, I'm not sure.
> 
> For practical purposes the BUG_ON() assertions will suffice, so I think you
> can keep the two handlers.  I'd change the names, though, to something
> like quiesce() and activate(), for example.

Sure.

> [Hm, it feels more appropriate to define them for all platforms and make them
> call local_irq_save() on the platforms that don't need to do anything more.]

Is there much point in that? It seems to make implementing new pm_ops a
bit more complex seeing that nobody but us seems to require such a thing
yet.

> BTW, please remember to update the SNAPSHOT_S2RAM ioctl accordingly (well,
> I think we should move the common code to a separate function).

Good point. I'll take a look.

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: [PATCH] pm_ops: add irq enable/disable hooks

[Rafael J. Wysocki]

On Friday, 6 April 2007 11:44, Johannes Berg wrote:
> On Fri, 2007-04-06 at 11:41 +0200, Rafael J. Wysocki wrote:
> 
> > Frankly, I'm not sure.
> > 
> > For practical purposes the BUG_ON() assertions will suffice, so I think you
> > can keep the two handlers.  I'd change the names, though, to something
> > like quiesce() and activate(), for example.
> 
> Sure.
> 
> > [Hm, it feels more appropriate to define them for all platforms and make them
> > call local_irq_save() on the platforms that don't need to do anything more.]
> 
> Is there much point in that? It seems to make implementing new pm_ops a
> bit more complex seeing that nobody but us seems to require such a thing
> yet.

That's why I said "it feels". :-)

Still, we can, for example, define default_quiesce() and default_activate()
that will only call local_irq_save/restore() to be used by the architectures
that don't need anything more etc.

Greetings,
Rafael

Re: [PATCH] pm_ops: add irq enable/disable hooks

[Johannes Berg]

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

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

> That's why I said "it feels". :-)

:)

> Still, we can, for example, define default_quiesce() and default_activate()
> that will only call local_irq_save/restore() to be used by the architectures
> that don't need anything more etc.

Like I did with pm_ops_only_mem or whatever it was...

I don't feel strongly either way. But the last big discussion when I
changed all pm_ops users sort of discouraged me ;)

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: [PATCH] pm_ops: add irq enable/disable hooks

[Pavel Machek]

Hi!

> > Frankly, I'm not sure.
> > 
> > For practical purposes the BUG_ON() assertions will suffice, so I think you
> > can keep the two handlers.  I'd change the names, though, to something
> > like quiesce() and activate(), for example.
> 
> Sure.
> 
> > [Hm, it feels more appropriate to define them for all platforms and make them
> > call local_irq_save() on the platforms that don't need to do anything more.]
> 
> Is there much point in that? It seems to make implementing new pm_ops a
> bit more complex seeing that nobody but us seems to require such a thing
> yet.

And why do _you_ need it? Unfortunately I do not know what the
decrementer is...?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] pm_ops: add irq enable/disable hooks

[Johannes Berg]

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

On Fri, 2007-04-06 at 21:19 +0200, Pavel Machek wrote:

> And why do _you_ need it? Unfortunately I do not know what the
> decrementer is...?

The decrementer is a sort of timer that counts down and once it
overflows raises an exception in the CPU. Like a timer interrupt, but
not coming from anything external. We need to be able to make sure we
don't get a decrementer exception at some spots.

Also, we need to call some platform function things at that point to
prepare the system for suspend, and it needs to be done with interrupts
enabled but after all devices are suspended.

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: [PATCH] pm_ops: add irq enable/disable hooks

[Pavel Machek]

Hi!

> > And why do _you_ need it? Unfortunately I do not know what the
> > decrementer is...?
> 
> The decrementer is a sort of timer that counts down and once it
> overflows raises an exception in the CPU. Like a timer interrupt, but
> not coming from anything external. We need to be able to make sure we
> don't get a decrementer exception at some spots.
> 
> Also, we need to call some platform function things at that point to
> prepare the system for suspend, and it needs to be done with interrupts
> enabled but after all devices are suspended.

Hmm, and can't you simply create sysdev for decrementer and special
platform handling? sysdevs should be suspended last...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] pm_ops: add irq enable/disable hooks

[Johannes Berg]

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

On Tue, 2007-04-10 at 13:36 +0200, Pavel Machek wrote:

> Hmm, and can't you simply create sysdev for decrementer and special
> platform handling? sysdevs should be suspended last...

In theory, yes.

In practise, however, it seems to be impossible to get a sysdev into the
queue that is suspended before any other sysdevs are suspended (i.e.
right after interrupts are disabled)

And then there are the platform functions. In theory, they could be done
with a regular struct device, but in practice they need to be the very
last thing before interrupts are disabled, and that again is impossible
to achieve.

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: [PATCH] pm_ops: add irq enable/disable hooks

[Pavel Machek]

Hi!

> > Hmm, and can't you simply create sysdev for decrementer and special
> > platform handling? sysdevs should be suspended last...
> 
> In theory, yes.
> 
> In practise, however, it seems to be impossible to get a sysdev into the
> queue that is suspended before any other sysdevs are suspended (i.e.
> right after interrupts are disabled)
> 
> And then there are the platform functions. In theory, they could be done
> with a regular struct device, but in practice they need to be the very
> last thing before interrupts are disabled, and that again is impossible
> to achieve.

Is it feasible to improve sysdev handling to allow this?

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

Re: [PATCH] pm_ops: add irq enable/disable hooks

[Johannes Berg]

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

Hi,

> > > Hmm, and can't you simply create sysdev for decrementer and special
> > > platform handling? sysdevs should be suspended last...
> > 
> > In theory, yes.
> > 
> > In practise, however, it seems to be impossible to get a sysdev into the
> > queue that is suspended before any other sysdevs are suspended (i.e.
> > right after interrupts are disabled)
> > 
> > And then there are the platform functions. In theory, they could be done
> > with a regular struct device, but in practice they need to be the very
> > last thing before interrupts are disabled, and that again is impossible
> > to achieve.
> 
> Is it feasible to improve sysdev handling to allow this?

Actually, I think I spoke too soon when I wrote above.
device_power_down() calls sysdev's last so even being first in the
sysdev queue probably wouldn't help here. Then again, I'm not 100%
certain that we do need the decrementer right after disabling IRQs, it
seems to me on casual inspections that it isn't necessary but I'd have
to take another look.

Pushing aside the decrementer, the second issue with the platform
functions remain. I'm not opposed to working on something to allow me to
order the device tree suspend, but I have absolutely no idea how that
could be done.

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: [PATCH] pm_ops: add irq enable/disable hooks

[Benjamin Herrenschmidt]

On Tue, 2007-04-10 at 14:00 +0200, Pavel Machek wrote:
> Hi!
> 
> > > Hmm, and can't you simply create sysdev for decrementer and special
> > > platform handling? sysdevs should be suspended last...
> > 
> > In theory, yes.
> > 
> > In practise, however, it seems to be impossible to get a sysdev into the
> > queue that is suspended before any other sysdevs are suspended (i.e.
> > right after interrupts are disabled)
> > 
> > And then there are the platform functions. In theory, they could be done
> > with a regular struct device, but in practice they need to be the very
> > last thing before interrupts are disabled, and that again is impossible
> > to achieve.
> 
> Is it feasible to improve sysdev handling to allow this?

It would be an absolutely ugly hack imho...

The trick to get the decrementer right is to really do it at the same
time as we turn irqs off... hence the idea to make the whole "turning
irqs off" platform specific to get the platform a chance to perform
whatever trickery is necessary after normal driver suspend and around
IRQ disabling.

(In the decrementer case, it looks approx like:
  1- set decrementer to max value
  2- switch irqs off
  3- set decrementer to max value (again)
 )

Along with that, we have all sort of platform specific hackery we need
to perform just before disabling IRQs on suspend and just after
re-enabling them on resume. That's really the simplest/easiest way to do
it.

In fact, I'm pretty sure ACPI would love to use such a hook into as
well, in order to run all that motherboard stuff that needs to be able
to sleep, take semaphores, etc... before IRQs are off but after all
devices have suspended.

sysdev's are just a pain in the neck if you ask me... Most of the
problems I've seen with cpufreq are due to the fact that it's a sysdev.
They were a bad idea in the first place and keep being abused :-)

Ben.

Re: [PATCH] pm_ops: add irq enable/disable hooks

[Alan Stern]

On Wed, 11 Apr 2007, Benjamin Herrenschmidt wrote:

> > Is it feasible to improve sysdev handling to allow this?
> 
> It would be an absolutely ugly hack imho...
> 
> The trick to get the decrementer right is to really do it at the same
> time as we turn irqs off... hence the idea to make the whole "turning
> irqs off" platform specific to get the platform a chance to perform
> whatever trickery is necessary after normal driver suspend and around
> IRQ disabling.
> 
> (In the decrementer case, it looks approx like:
>   1- set decrementer to max value
>   2- switch irqs off
>   3- set decrementer to max value (again)
>  )
> 
> Along with that, we have all sort of platform specific hackery we need
> to perform just before disabling IRQs on suspend and just after
> re-enabling them on resume. That's really the simplest/easiest way to do
> it.
> 
> In fact, I'm pretty sure ACPI would love to use such a hook into as
> well, in order to run all that motherboard stuff that needs to be able
> to sleep, take semaphores, etc... before IRQs are off but after all
> devices have suspended.
> 
> sysdev's are just a pain in the neck if you ask me... Most of the
> problems I've seen with cpufreq are due to the fact that it's a sysdev.
> They were a bad idea in the first place and keep being abused :-)

Have you guys considered using a notifier chain for this?  The platform 
code could register itself on the chain at boot time.  When suspending you 
send out notifications just before and just after disabling interrupts, 
and inversely on resume.

Alan Stern

Re: [PATCH] pm_ops: add irq enable/disable hooks

[Johannes Berg]

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

On Wed, 2007-04-11 at 10:07 -0400, Alan Stern wrote:

> Have you guys considered using a notifier chain for this?  The platform 
> code could register itself on the chain at boot time.  When suspending you 
> send out notifications just before and just after disabling interrupts, 
> and inversely on resume.

Not really, but I don't see the point. There shouldn't be anything but
the platform using it so why have a chain? And we'd have to register two
things since it's not exactly in the inverse order here.

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: [PATCH] pm_ops: add irq enable/disable hooks

[Benjamin Herrenschmidt]

> Have you guys considered using a notifier chain for this?  The platform 
> code could register itself on the chain at boot time.  When suspending you 
> send out notifications just before and just after disabling interrupts, 
> and inversely on resume.

Except that in the case of the decrementer, I really need to be just
around the actual switching of irqs off ...

Ben.

Re: [PATCH] pm_ops: add irq enable/disable hooks

[Johannes Berg]

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

On Tue, 2007-04-10 at 13:36 +0200, Pavel Machek wrote:

> Hmm, and can't you simply create sysdev for decrementer and special
> platform handling? sysdevs should be suspended last...

So I looked again and noticed that my earlier statement was wrong.

The special platform handling can well be done with platform devices, I
can simply insert them into the power management list early by using a
quite early initcall when registering them, that way they'll be
suspended last. So contrary to what I said, those aren't a problem.

The decrementer, however, I looked at again and noticed that I had a
wrong assumption. Let me give a short description of how the decrementer
works first:

The decrementer is a 32-bit register that counts down based on an
implementation-dependent clock, on my powerbook it's somewhere around
18MHz, on my powermac 32MHz (IIRC). When the most significant bit
transitions from 0 to 1 the decrementer exception is raised, when
exceptions are enabled it's taken. However, when exceptions are
disabled, it stays pending until it is taken when exceptions are enabled
again.

Because the exception stays pending, we need to set the decrementer to
the highest possible value before interrupts are disabled so we can be
sure that the exception is not pending. This is to ensure that the
processor can later go to sleep properly to be turned off.

Since we only have a limited amount of time after setting the
decrementer to such a high value (granted, it's at least a minute in all
cases I've seen so far) we like to do it right before disabling
interrupts.

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: [PATCH] pm_ops: add irq enable/disable hooks

[Pavel Machek]

Hi!

> For powermac, we need to do some things between suspending devices and
> device_power_off, for example setting the decrementer. This patch
> introduces pm_ops.irq_off and pm_ops.irq_on which will be called instead
> of disabling/enabling irqs so platforms can do a bit more work there if
> necessary.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> 
> ---
> My previous patches moving to the pm_ops interface for powermac were
> buggy due to exactly this issue, but the bug never surfaced on my
> machine, only on machines with an Apple Desktop Bus (or an emulation
> thereof)
> 
> Does this look ok to you? Would you want different names? Should I stick
> in a BUG_ON(interrupts_enabled()) or such?
> 
>  include/linux/pm.h  |   10 ++++++++++
>  kernel/power/main.c |   10 ++++++++--
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> --- wireless-dev.orig/include/linux/pm.h	2007-04-05 18:14:07.948549941 +0200
> +++ wireless-dev/include/linux/pm.h	2007-04-05 23:15:29.934829459 +0200
> @@ -131,9 +131,17 @@ typedef int __bitwise suspend_disk_metho
>   * @prepare: Prepare the platform for the given suspend state. Can return a
>   *	negative error code if necessary.
>   *
> + * @irq_off: If assigned, the generic suspend code does not turn off IRQs
> + *	but relies on this callback instead. It is currently not called for
> + *	%PM_SUSPEND_DISK.

Well, I guess you should also describe what irq_off's responsibilities
are...? Obviously it needs to disable irqs, but if you need something
special on ppc, does that mean it needs to do more?


>   * @enter: Enter the given suspend state, must be assigned. Can return a
>   *	negative error code if necessary.
>   *
> + * @irq_on: If assigned, the generic suspend code does not turn on IRQs
> + *	but relies on this callback instead. It is currently not called for
> + *	%PM_SUSPEND_DISK.
> + *
>   * @finish: Called when the system has left the given state and all devices
>   *	are resumed. The return value is ignored.
>   *
> @@ -152,7 +160,9 @@ typedef int __bitwise suspend_disk_metho
>  struct pm_ops {
>  	int (*valid)(suspend_state_t state);
>  	int (*prepare)(suspend_state_t state);
> +	void (*irq_off)(suspend_state_t state);
>  	int (*enter)(suspend_state_t state);
> +	void (*irq_on)(suspend_state_t state);
>  	int (*finish)(suspend_state_t state);
>  	suspend_disk_method_t pm_disk_mode;
>  };
> --- wireless-dev.orig/kernel/power/main.c	2007-04-05 18:14:07.988549941 +0200
> +++ wireless-dev/kernel/power/main.c	2007-04-05 18:25:21.108549941 +0200
> @@ -117,7 +117,10 @@ int suspend_enter(suspend_state_t state)
>  	int error = 0;
>  	unsigned long flags;
>  
> -	local_irq_save(flags);
> +	if (pm_ops->irq_off)
> +		pm_ops->irq_off(state);
> +	else
> +		local_irq_save(flags);
>  

I'd just unconditionally call irq_off, and set up irq_off to do
local_irq_save on all the other archs... This is just ugly.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH v2] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

For powermac, we need to do some things between suspending devices and
device_power_off, for example setting the decrementer. This patch
introduces pm_ops.quiesce and pm_ops.activate which will be called instead
of disabling/enabling irqs so platforms get control over this step.

Since those two new calls are required, this patch also introduces default
methods that can be assigned and updates all pm_ops users to use these
defaults.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
Rafael: I don't need to change SNAPSHOT_S2RAM since it calls suspend_enter
which is where the change is.

I'm not sure if it makes sense to have this for PM_SUSPEND_DISK as well so
unless somebody comes up with a reason for now don't.

 arch/arm/common/sharpsl_pm.c            |    2 ++
 arch/arm/mach-at91/pm.c                 |    2 ++
 arch/arm/mach-omap1/pm.c                |    2 ++
 arch/arm/mach-omap2/pm.c                |    2 ++
 arch/arm/mach-pnx4008/pm.c              |    2 ++
 arch/arm/mach-pxa/pm.c                  |    2 ++
 arch/arm/mach-sa1100/pm.c               |    2 ++
 arch/arm/plat-s3c24xx/pm.c              |    2 ++
 arch/powerpc/platforms/powermac/setup.c |    2 ++
 arch/sh/boards/hp6xx/pm.c               |    2 ++
 drivers/acpi/sleep/main.c               |    7 +++++++
 include/linux/pm.h                      |   12 ++++++++++++
 kernel/power/main.c                     |   29 ++++++++++++++++++++++++++---
 13 files changed, 65 insertions(+), 3 deletions(-)

--- wireless-dev.orig/include/linux/pm.h	2007-04-11 17:02:22.969481358 +0200
+++ wireless-dev/include/linux/pm.h	2007-04-11 17:02:42.609481358 +0200
@@ -135,9 +135,17 @@ typedef int __bitwise suspend_disk_metho
  * @prepare: Prepare the platform for the given suspend state. Can return a
  *	negative error code if necessary.
  *
+ * @quiesce: This callback must be assigned and at least turn off IRQs. If
+ *	your platform requires nothing else, assign %pm_default_quiesce.
+ *	It is currently not called for %PM_SUSPEND_DISK.
+ *
  * @enter: Enter the given suspend state, must be assigned. Can return a
  *	negative error code if necessary.
  *
+ * @activate: This callback must be assigned and at least turn on IRQS. If
+ *	your platform requires nothing else, assign %pm_default_activate.
+ *	It is currently not called for %PM_SUSPEND_DISK.
+ *
  * @finish: Called when the system has left the given state and all devices
  *	are resumed. The return value is ignored.
  *
@@ -155,7 +163,9 @@ typedef int __bitwise suspend_disk_metho
 struct pm_ops {
 	int (*valid)(suspend_state_t state);
 	int (*prepare)(suspend_state_t state);
+	void (*quiesce)(suspend_state_t state);
 	int (*enter)(suspend_state_t state);
+	void (*activate)(suspend_state_t state);
 	int (*finish)(suspend_state_t state);
 	suspend_disk_method_t pm_disk_mode;
 };
@@ -169,6 +179,8 @@ extern struct pm_ops *pm_ops;
 extern int pm_suspend(suspend_state_t state);
 
 extern int pm_valid_only_mem(suspend_state_t state);
+extern void pm_default_quiesce(suspend_state_t state);
+extern void pm_default_activate(suspend_state_t state);
 
 /*
  * Device power management
--- wireless-dev.orig/kernel/power/main.c	2007-04-11 17:02:23.149481358 +0200
+++ wireless-dev/kernel/power/main.c	2007-04-11 17:18:14.879481358 +0200
@@ -60,6 +60,28 @@ int pm_valid_only_mem(suspend_state_t st
 	return state == PM_SUSPEND_MEM;
 }
 
+/**
+ * pm_default_quiesce - default quiesce callback
+ *
+ * pm_ops drivers that need to do nothing but disable
+ * IRQs use this instead of their own code.
+ */
+void pm_default_quiesce(suspend_state_t state)
+{
+	local_irq_disable();
+}
+
+/**
+ * pm_default_activate - default activate callback
+ *
+ * pm_ops drivers that need to do nothing but enable
+ * IRQs use this instead of their own code.
+ */
+void pm_default_activate(suspend_state_t state)
+{
+	local_irq_enable();
+}
+
 
 static inline void pm_finish(suspend_state_t state)
 {
@@ -132,9 +154,9 @@ static int suspend_prepare(suspend_state
 int suspend_enter(suspend_state_t state)
 {
 	int error = 0;
-	unsigned long flags;
 
-	local_irq_save(flags);
+	pm_ops->quiesce(state);
+	BUG_ON(!irqs_disabled());
 
 	if ((error = device_power_down(PMSG_SUSPEND))) {
 		printk(KERN_ERR "Some devices failed to power down\n");
@@ -143,7 +165,8 @@ int suspend_enter(suspend_state_t state)
 	error = pm_ops->enter(state);
 	device_power_up();
  Done:
-	local_irq_restore(flags);
+	pm_ops->activate(state);
+	BUG_ON(irqs_disabled());
 	return error;
 }
 
--- wireless-dev.orig/arch/arm/common/sharpsl_pm.c	2007-04-11 17:07:50.109481358 +0200
+++ wireless-dev/arch/arm/common/sharpsl_pm.c	2007-04-11 17:08:28.439481358 +0200
@@ -767,7 +767,9 @@ static void sharpsl_apm_get_power_status
 
 static struct pm_ops sharpsl_pm_ops = {
 	.prepare	= pxa_pm_prepare,
+	.quiesce	= pm_default_quiesce,
 	.enter		= corgi_pxa_pm_enter,
+	.activate	= pm_default_activate,
 	.finish		= pxa_pm_finish,
 	.valid		= pm_valid_only_mem,
 };
--- wireless-dev.orig/arch/arm/mach-at91/pm.c	2007-04-11 17:07:50.889481358 +0200
+++ wireless-dev/arch/arm/mach-at91/pm.c	2007-04-11 17:10:17.399481358 +0200
@@ -203,7 +203,9 @@ error:
 static struct pm_ops at91_pm_ops ={
 	.valid		= at91_pm_valid_state,
 	.prepare	= at91_pm_prepare,
+	.quiesce	= pm_default_quiesce,
 	.enter		= at91_pm_enter,
+	.activate	= pm_default_activate,
 };
 
 static int __init at91_pm_init(void)
--- wireless-dev.orig/arch/arm/mach-omap1/pm.c	2007-04-11 17:07:50.219481358 +0200
+++ wireless-dev/arch/arm/mach-omap1/pm.c	2007-04-11 17:08:48.149481358 +0200
@@ -699,7 +699,9 @@ static struct irqaction omap_wakeup_irq 
 
 static struct pm_ops omap_pm_ops ={
 	.prepare	= omap_pm_prepare,
+	.quiesce	= pm_default_quiesce,
 	.enter		= omap_pm_enter,
+	.activate	= pm_default_activate,
 	.finish		= omap_pm_finish,
 	.valid		= pm_valid_only_mem,
 };
--- wireless-dev.orig/arch/arm/mach-omap2/pm.c	2007-04-11 17:07:50.349481358 +0200
+++ wireless-dev/arch/arm/mach-omap2/pm.c	2007-04-11 17:09:08.369481358 +0200
@@ -371,7 +371,9 @@ static int omap2_pm_finish(suspend_state
 
 static struct pm_ops omap_pm_ops = {
 	.prepare	= omap2_pm_prepare,
+	.quiesce	= pm_default_quiesce,
 	.enter		= omap2_pm_enter,
+	.activate	= pm_default_activate,
 	.finish		= omap2_pm_finish,
 	.valid		= pm_valid_only_mem,
 };
--- wireless-dev.orig/arch/arm/mach-pnx4008/pm.c	2007-04-11 17:07:50.539481358 +0200
+++ wireless-dev/arch/arm/mach-pnx4008/pm.c	2007-04-11 17:09:23.759481358 +0200
@@ -116,7 +116,9 @@ static int pnx4008_pm_enter(suspend_stat
 }
 
 static struct pm_ops pnx4008_pm_ops = {
+	.quiesce = pm_default_quiesce,
 	.enter = pnx4008_pm_enter,
+	.activate = pm_default_activate,
 	.valid = pm_valid_only_mem,
 };
 
--- wireless-dev.orig/arch/arm/mach-pxa/pm.c	2007-04-11 17:07:50.639481358 +0200
+++ wireless-dev/arch/arm/mach-pxa/pm.c	2007-04-11 17:09:45.959481358 +0200
@@ -225,7 +225,9 @@ EXPORT_SYMBOL_GPL(pxa_pm_finish);
 
 static struct pm_ops pxa_pm_ops = {
 	.prepare	= pxa_pm_prepare,
+	.quiesce	= pm_default_quiesce,
 	.enter		= pxa_pm_enter,
+	.activate	= pm_default_activate,
 	.finish		= pxa_pm_finish,
 	.valid		= pm_valid_only_mem,
 };
--- wireless-dev.orig/arch/arm/mach-sa1100/pm.c	2007-04-11 17:07:50.729481358 +0200
+++ wireless-dev/arch/arm/mach-sa1100/pm.c	2007-04-11 17:09:59.759481358 +0200
@@ -132,7 +132,9 @@ unsigned long sleep_phys_sp(void *sp)
 }
 
 static struct pm_ops sa11x0_pm_ops = {
+	.quiesce	= pm_default_quiesce,
 	.enter		= sa11x0_pm_enter,
+	.activate	= pm_default_activate,
 	.valid		= pm_valid_only_mem,
 };
 
--- wireless-dev.orig/arch/arm/plat-s3c24xx/pm.c	2007-04-11 17:07:50.959481358 +0200
+++ wireless-dev/arch/arm/plat-s3c24xx/pm.c	2007-04-11 17:10:36.039481358 +0200
@@ -613,7 +613,9 @@ static int s3c2410_pm_enter(suspend_stat
 }
 
 static struct pm_ops s3c2410_pm_ops = {
+	.quiesce	= pm_default_quiesce,
 	.enter		= s3c2410_pm_enter,
+	.activate	= pm_default_activate,
 	.valid		= pm_valid_only_mem,
 };
 
--- wireless-dev.orig/arch/powerpc/platforms/powermac/setup.c	2007-04-11 17:07:51.029481358 +0200
+++ wireless-dev/arch/powerpc/platforms/powermac/setup.c	2007-04-11 17:10:53.659481358 +0200
@@ -475,7 +475,9 @@ static int pmac_pm_valid(suspend_state_t
 static struct pm_ops pmac_pm_ops = {
 	.pm_disk_mode	= PM_DISK_SHUTDOWN,
 	.prepare	= pmac_pm_prepare,
+	.quiesce	= pm_default_quiesce,
 	.enter		= pmac_pm_enter,
+	.activate	= pm_default_activate,
 	.finish		= pmac_pm_finish,
 	.valid		= pmac_pm_valid,
 };
--- wireless-dev.orig/arch/sh/boards/hp6xx/pm.c	2007-04-11 17:07:51.099481358 +0200
+++ wireless-dev/arch/sh/boards/hp6xx/pm.c	2007-04-11 17:11:06.269481358 +0200
@@ -68,7 +68,9 @@ static int hp6x0_pm_enter(suspend_state_
 }
 
 static struct pm_ops hp6x0_pm_ops = {
+	.quiesce	= pm_default_quiesce,
 	.enter		= hp6x0_pm_enter,
+	.activate	= pm_default_activate,
 	.valid		= pm_valid_only_mem,
 };
 
--- wireless-dev.orig/drivers/acpi/sleep/main.c	2007-04-11 17:07:51.339481358 +0200
+++ wireless-dev/drivers/acpi/sleep/main.c	2007-04-11 17:12:49.679481358 +0200
@@ -182,10 +182,17 @@ static int acpi_pm_state_valid(suspend_s
 	}
 }
 
+/*
+ * If .quiesce or .activate are changed, keep in mind
+ * that they aren't currently called for S4. That can
+ * be changed if needed, of course.
+ */
 static struct pm_ops acpi_pm_ops = {
 	.valid = acpi_pm_state_valid,
 	.prepare = acpi_pm_prepare,
+	.quiesce = pm_default_quiesce,
 	.enter = acpi_pm_enter,
+	.activate = pm_default_activate,
 	.finish = acpi_pm_finish,
 };
 

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Dmitry Krivoschekov]

Hi Johannes,

Johannes Berg wrote:
> For powermac, we need to do some things between suspending devices and
> device_power_off, for example setting the decrementer. 
I doubt your reasoning is cogent enough. If you add such a global thing,
you should prove it, or clearly explain what is the thing for.
I don't believe that it's not possible to solve that decrementer case
without this change. Can't you add appropriate platform_device which
would register early enough, to meet that timing requirement when it
gets suspended?


>
>  
> --- wireless-dev.orig/arch/powerpc/platforms/powermac/setup.c	2007-04-11 17:07:51.029481358 +0200
> +++ wireless-dev/arch/powerpc/platforms/powermac/setup.c	2007-04-11 17:10:53.659481358 +0200
> @@ -475,7 +475,9 @@ static int pmac_pm_valid(suspend_state_t
>  static struct pm_ops pmac_pm_ops = {
>  	.pm_disk_mode	= PM_DISK_SHUTDOWN,
>  	.prepare	= pmac_pm_prepare,
> +	.quiesce	= pm_default_quiesce,
>  	.enter		= pmac_pm_enter,
> +	.activate	= pm_default_activate,
>  	.finish		= pmac_pm_finish,
>  	.valid		= pmac_pm_valid,
>  };

So, where is that callbacks that are so desirable for powermac?


Thanks,
Dmitry

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Thu, 2007-04-12 at 00:47 +0400, Dmitry Krivoschekov wrote:

> I doubt your reasoning is cogent enough. If you add such a global thing,
> you should prove it, or clearly explain what is the thing for.
> I don't believe that it's not possible to solve that decrementer case
> without this change. Can't you add appropriate platform_device which
> would register early enough, to meet that timing requirement when it
> gets suspended?

I think Ben explained that.

> >  static struct pm_ops pmac_pm_ops = {
> >  	.pm_disk_mode	= PM_DISK_SHUTDOWN,
> >  	.prepare	= pmac_pm_prepare,
> > +	.quiesce	= pm_default_quiesce,
> >  	.enter		= pmac_pm_enter,
> > +	.activate	= pm_default_activate,
> >  	.finish		= pmac_pm_finish,
> >  	.valid		= pmac_pm_valid,
> >  };
> 
> So, where is that callbacks that are so desirable for powermac?

If you'd looked at the current pmac_pm_valid, you'd see that it allows
nothing. Currently, pm_ops are totally broken on powermac. The callback
that is so desirable for powermac will be added when I actually make
powermac use the pm_ops infrastructure which currently isn't possible
due to this problem.

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: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

> I doubt your reasoning is cogent enough. If you add such a global thing,
> you should prove it, or clearly explain what is the thing for.
> I don't believe that it's not possible to solve that decrementer case
> without this change. Can't you add appropriate platform_device which
> would register early enough, to meet that timing requirement when it
> gets suspended?

That's would be absolutely disgusting... (relying on magic ordering of
platform devices).

The fact is, the architecture needs to do various things around the
IRQ disabling and enabling, and thus we should have a hook for it.
I fail to understand why there is so much resistance here...

Ben.

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

Hi!

> > I doubt your reasoning is cogent enough. If you add such a global thing,
> > you should prove it, or clearly explain what is the thing for.
> > I don't believe that it's not possible to solve that decrementer case
> > without this change. Can't you add appropriate platform_device which
> > would register early enough, to meet that timing requirement when it
> > gets suspended?
> 
> That's would be absolutely disgusting... (relying on magic ordering of
> platform devices).

Adding platform hook to disable interrupts just because we need one
platform device last is not nice, either. (And does decrementer even
need to come last?)

Anyway, platform devices may need specific ordering on more than power
pc, so perhaps we should just make ordering more stable so that
relying on it is no longer a hack?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Rafael J. Wysocki]

On Thursday, 12 April 2007 12:16, Pavel Machek wrote:
> Hi!
> 
> > > I doubt your reasoning is cogent enough. If you add such a global thing,
> > > you should prove it, or clearly explain what is the thing for.
> > > I don't believe that it's not possible to solve that decrementer case
> > > without this change. Can't you add appropriate platform_device which
> > > would register early enough, to meet that timing requirement when it
> > > gets suspended?
> > 
> > That's would be absolutely disgusting... (relying on magic ordering of
> > platform devices).
> 
> Adding platform hook to disable interrupts just because we need one
> platform device last is not nice, either. (And does decrementer even
> need to come last?)
> 
> Anyway, platform devices may need specific ordering on more than power
> pc, so perhaps we should just make ordering more stable so that
> relying on it is no longer a hack?

I second that, although I think it also would be a global change.

Greetings,
Rafael

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Thu, 2007-04-12 at 12:16 +0200, Pavel Machek wrote:

> Adding platform hook to disable interrupts just because we need one
> platform device last is not nice, either. (And does decrementer even
> need to come last?)

But the hack is classifying the decrementer as a platform device in the
first place. And then we'd still need to have one platform device and
one sysdev since we want to wake up the decrementer before enabling IRQs
but set it to a high value before disabling IRQs. Or we'd need some sort
of special foo device that has hooks both before and after and comes
last/first, just for one user?

Besides, we can always rewrite it later when somebody introduces a foo
device that fits above model. Since this patch essentially changes
nothing, I'd been hoping to get it into .22 so we can finally start
having a sane userspace interface for suspend on powermac. I can dream,
right?

Besides, disabling IRQs really seems like a platform issue and as such
pm_ops should be able to control it. Why are you so much opposed to that
idea? Would a patch that says

  Introduce pm_ops.quiesce and pm_ops.activate to make platforms
  control IRQ disabling and enabling (respectively).

and doesn't mention powermac be more acceptable?

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: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

Hi!

> > Adding platform hook to disable interrupts just because we need one
> > platform device last is not nice, either. (And does decrementer even
> > need to come last?)
> 
> But the hack is classifying the decrementer as a platform device in the
> first place. And then we'd still need to have one platform device and
> one sysdev since we want to wake up the decrementer before enabling
> IRQs

Can decrementer be waked a little later? 

> but set it to a high value before disabling IRQs. Or we'd need some sort
> of special foo device that has hooks both before and after and comes
> last/first, just for one user?

Have two devices, then.

> Besides, we can always rewrite it later when somebody introduces a foo
> device that fits above model. Since this patch essentially changes
> nothing, I'd been hoping to get it into .22 so we can finally start

No. Hacks like this are impossible to get rid of.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Fri, 2007-04-13 at 23:00 +0200, Pavel Machek wrote:

> Have two devices, then.

I'm definitely *not* going to do hacks like that.

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: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

Hi!

> > Have two devices, then.
> 
> I'm definitely *not* going to do hacks like that.

Come on, that's still better than introducing new pm_op with unclear
semantic, then calling it from half the places that need it.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Paul Mackerras]

Pavel Machek writes:

> > Besides, we can always rewrite it later when somebody introduces a foo
> > device that fits above model. Since this patch essentially changes
> > nothing, I'd been hoping to get it into .22 so we can finally start
> 
> No. Hacks like this are impossible to get rid of.

OK, we'll just stick with our separate code path that does what we
need, then.

Paul.

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

On Fri, 2007-04-13 at 23:00 +0200, Pavel Machek wrote:

> > Besides, we can always rewrite it later when somebody introduces a foo
> > device that fits above model. Since this patch essentially changes
> > nothing, I'd been hoping to get it into .22 so we can finally start
> 
> No. Hacks like this are impossible to get rid of.

it's the cleanest solution, it's you who is a hack

Ben.

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

On Thu, 2007-04-12 at 12:16 +0200, Pavel Machek wrote:

> Adding platform hook to disable interrupts just because we need one
> platform device last is not nice, either. (And does decrementer even
> need to come last?)
> 
> Anyway, platform devices may need specific ordering on more than power
> pc, so perhaps we should just make ordering more stable so that
> relying on it is no longer a hack?

It would be pretty hard to do cleanly without raping the core driver
suspend/resume core.

I have experience implementing suspend/resume in all sorts of
environments, and I do strongly beleive that an arch hook right at this
point will be useful to more than that anyway.

There are actions the arch code might need to take after drivers and
before going to "atomic" mode (irqs off mean you can no longer sleep or
take semaphores etc...), and in a similar vein, actions to take on
resume just after interrupts are back and before all the drivers get
woken up.

The decrementer is one example, there might be other processor specific
bits or arch specific bits that are better done at that stage.

I think the way to go for 2.6.22 is to have that hook unless you can
propose me a way to cleanly provide a platform device that is guaranteed
to suspend last and resume first in all cases (and even then, I wouldn't
be that happy since the proper decrementer stop procedure involves
really wrapping the "irqs off" operation).

Why not give this added flexibility ? Archs who don't care don't need to
bother and it will make us happy... it's not like we are about to -add-
burden to other architectures.

Ben.

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Rafael J. Wysocki]

On Thursday, 12 April 2007 13:23, Benjamin Herrenschmidt wrote:
> On Thu, 2007-04-12 at 12:16 +0200, Pavel Machek wrote:
> 
> > Adding platform hook to disable interrupts just because we need one
> > platform device last is not nice, either. (And does decrementer even
> > need to come last?)
> > 
> > Anyway, platform devices may need specific ordering on more than power
> > pc, so perhaps we should just make ordering more stable so that
> > relying on it is no longer a hack?
> 
> It would be pretty hard to do cleanly without raping the core driver
> suspend/resume core.

Agreed.

> I have experience implementing suspend/resume in all sorts of
> environments, and I do strongly beleive that an arch hook right at this
> point will be useful to more than that anyway.
> 
> There are actions the arch code might need to take after drivers and
> before going to "atomic" mode (irqs off mean you can no longer sleep or
> take semaphores etc...), and in a similar vein, actions to take on
> resume just after interrupts are back and before all the drivers get
> woken up.
> 
> The decrementer is one example, there might be other processor specific
> bits or arch specific bits that are better done at that stage.
> 
> I think the way to go for 2.6.22 is to have that hook unless you can
> propose me a way to cleanly provide a platform device that is guaranteed
> to suspend last and resume first in all cases (and even then, I wouldn't
> be that happy since the proper decrementer stop procedure involves
> really wrapping the "irqs off" operation).
> 
> Why not give this added flexibility ? Archs who don't care don't need to
> bother and it will make us happy... it's not like we are about to -add-
> burden to other architectures.

Well, I think it would be reasonable to add the quiesce()/activate() hooks for
all of the above reasons.  However, once we've done that, it'll be quite
difficult to remove them, so we should better be sure they are really really
needed and there's no other way to implement what you need (or all of the
alternative ways are far worse).

Greetings,
Rafael

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[David Brownell]

On Thursday 12 April 2007 8:03 am, Rafael J. Wysocki wrote:
> On Thursday, 12 April 2007 13:23, Benjamin Herrenschmidt wrote:

> > Why not give this added flexibility ? Archs who don't care don't need to
> > bother and it will make us happy... it's not like we are about to -add-
> > burden to other architectures.
> 
> Well, I think it would be reasonable to add the quiesce()/activate() hooks for
> all of the above reasons. 

Makes sense to me.  Though I'd rather see pm_set_ops() patch the default
hooks than expect all platforms to change ... that would shrink the size
of the patch adding these, as well as the potential cost of removing them.


> However, once we've done that, it'll be quite 
> difficult to remove them, so we should better be sure they are really really
> needed and there's no other way to implement what you need (or all of the
> alternative ways are far worse).

In general I think too much of the way PM "works" now is a bit more
due to convenient side effects than by clear intent.  So while I agree
that adding hooks should be done with care, this one certain seems to
be a step in the right direction.

- Dave

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Thu, 2007-04-12 at 09:32 -0700, David Brownell wrote:

> Makes sense to me.  Though I'd rather see pm_set_ops() patch the default
> hooks than expect all platforms to change ... that would shrink the size
> of the patch adding these, as well as the potential cost of removing them.

I was thinking about that, but wasn't sure if we could do that. I like
interfaces that don't change global variables they are passed. But if I
document it properly I can change the patch to do this easily.

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 --]

[PATCH v3] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

For powermac, we need to do some things between suspending devices and
device_power_off, for example setting the decrementer. This patch
introduces pm_ops.quiesce and pm_ops.activate which will be called instead
of disabling/enabling irqs so platforms get control over this step.

If not assigned, these two calls will be assigned with defaults during
set_pm_ops.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
This doesn't make sense for suspend to disk since there the arch gets
much more control over the process anyway, and using pm_ops for suspend
to disk is a hack in the first place (only makes sense for ACPI).
Therefore, this patch doesn't affect suspend to disk.

You can now chose either version 2 or 3 of this patch :)

 include/linux/pm.h  |   10 ++++++++++
 kernel/power/main.c |   35 ++++++++++++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 3 deletions(-)

--- wireless-dev.orig/include/linux/pm.h	2007-04-13 09:39:42.465356880 +0200
+++ wireless-dev/include/linux/pm.h	2007-04-13 09:43:42.985356880 +0200
@@ -135,9 +135,17 @@ typedef int __bitwise suspend_disk_metho
  * @prepare: Prepare the platform for the given suspend state. Can return a
  *	negative error code if necessary.
  *
+ * @quiesce: If assigned, this callback must at least turn off IRQs. If
+ *	left unassigned, a default callback that does nothing but turn
+ *	off IRQs is assigned during pm_set_ops().
+ *
  * @enter: Enter the given suspend state, must be assigned. Can return a
  *	negative error code if necessary.
  *
+ * @activate: If assigned, this callback must at least turn on IRQs. If
+ *	left unassigned, a default callback that does nothing but turn
+ *	on IRQs is assigned during pm_set_ops().
+ *
  * @finish: Called when the system has left the given state and all devices
  *	are resumed. The return value is ignored.
  *
@@ -155,7 +163,9 @@ typedef int __bitwise suspend_disk_metho
 struct pm_ops {
 	int (*valid)(suspend_state_t state);
 	int (*prepare)(suspend_state_t state);
+	void (*quiesce)(suspend_state_t state);
 	int (*enter)(suspend_state_t state);
+	void (*activate)(suspend_state_t state);
 	int (*finish)(suspend_state_t state);
 	suspend_disk_method_t pm_disk_mode;
 };
--- wireless-dev.orig/kernel/power/main.c	2007-04-13 09:39:42.465356880 +0200
+++ wireless-dev/kernel/power/main.c	2007-04-13 09:47:38.645356880 +0200
@@ -33,6 +33,28 @@ struct pm_ops *pm_ops;
 suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
 
 /**
+ * pm_default_quiesce - default quiesce callback
+ *
+ * pm_ops drivers that need to do nothing but disable IRQs
+ * leave .quiesce unassigned and pm_set_ops() assigns this.
+ */
+static void pm_default_quiesce(suspend_state_t state)
+{
+	local_irq_disable();
+}
+
+/**
+ * pm_default_activate - default activate callback
+ *
+ * pm_ops drivers that need to do nothing but enable IRQs
+ * leave .activate unassigned and pm_set_ops assigns this.
+ */
+static void pm_default_activate(suspend_state_t state)
+{
+	local_irq_enable();
+}
+
+/**
  *	pm_set_ops - Set the global power method table. 
  *	@ops:	Pointer to ops structure.
  */
@@ -45,6 +67,12 @@ void pm_set_ops(struct pm_ops * ops)
 		pm_disk_mode = ops->pm_disk_mode;
 	} else
 		pm_disk_mode = PM_DISK_SHUTDOWN;
+
+	if (!ops->quiesce)
+		ops->quiesce = pm_default_quiesce;
+	if (!ops->activate)
+		ops->activate = pm_default_activate;
+
 	mutex_unlock(&pm_mutex);
 }
 
@@ -132,9 +160,9 @@ static int suspend_prepare(suspend_state
 int suspend_enter(suspend_state_t state)
 {
 	int error = 0;
-	unsigned long flags;
 
-	local_irq_save(flags);
+	pm_ops->quiesce(state);
+	BUG_ON(!irqs_disabled());
 
 	if ((error = device_power_down(PMSG_SUSPEND))) {
 		printk(KERN_ERR "Some devices failed to power down\n");
@@ -143,7 +171,8 @@ int suspend_enter(suspend_state_t state)
 	error = pm_ops->enter(state);
 	device_power_up();
  Done:
-	local_irq_restore(flags);
+	pm_ops->activate(state);
+	BUG_ON(irqs_disabled());
 	return error;
 }
 

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Dmitry Krivoschekov]

Benjamin Herrenschmidt wrote:
> On Thu, 2007-04-12 at 12:16 +0200, Pavel Machek wrote:
>
>> Adding platform hook to disable interrupts just because we need one
>> platform device last is not nice, either. (And does decrementer even
>> need to come last?)
>>
>> Anyway, platform devices may need specific ordering on more than power
>> pc, so perhaps we should just make ordering more stable so that
>> relying on it is no longer a hack?
>
> It would be pretty hard to do cleanly without raping the core driver
> suspend/resume core.
>
> I have experience implementing suspend/resume in all sorts of
> environments, and I do strongly beleive that an arch hook right at this
> point will be useful to more than that anyway.
>
> There are actions the arch code might need to take after drivers and
> before going to "atomic" mode (irqs off mean you can no longer sleep or
> take semaphores etc...), and in a similar vein, actions to take on
> resume just after interrupts are back and before all the drivers get
> woken up.
>
> The decrementer is one example, there might be other processor specific
> bits or arch specific bits that are better done at that stage.

It'd be nice adding some examples to patch header, and accurate
explanation what is possible to do at this new level, this would
sign to other people that they don't have to invent theirs own hacks
anymore.

>
> I think the way to go for 2.6.22 is to have that hook unless you can
> propose me a way to cleanly provide a platform device that is guaranteed
> to suspend last and resume first in all cases (and even then, I wouldn't
> be that happy since the proper decrementer stop procedure involves
> really wrapping the "irqs off" operation).
>
> Why not give this added flexibility ? Archs who don't care don't need to
> bother and it will make us happy... it's not like we are about to -add-
> burden to other architectures.
>
>

Actually, I personally two hands up for  adding the flexibility,
but you should define what is supposed to do on this level and
what is don't, or not desirable.

For example, I'd like to enter back to suspend mode
right from "activate" stage, because I've woken up just
to update some data and I do not want to resume all devices
for that, is it ok for "activate"?


Thanks,
Dmitry

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

> Actually, I personally two hands up for  adding the flexibility,
> but you should define what is supposed to do on this level and
> what is don't, or not desirable.
> 
> For example, I'd like to enter back to suspend mode
> right from "activate" stage, because I've woken up just
> to update some data and I do not want to resume all devices
> for that, is it ok for "activate"?

That's an interesting approach... might be something we can define via
activate result code...

Ben.

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Thu, 2007-04-12 at 21:36 +0400, Dmitry Krivoschekov wrote:

> It'd be nice adding some examples to patch header, and accurate
> explanation what is possible to do at this new level, this would
> sign to other people that they don't have to invent theirs own hacks
> anymore.

That's pretty hard to do generically since it'll most likely differ for
all platforms. I can paste some powermac specific code but that's
unlikely to help anybody.

> For example, I'd like to enter back to suspend mode
> right from "activate" stage, because I've woken up just
> to update some data and I do not want to resume all devices
> for that, is it ok for "activate"?

I'd think that better be done within ->enter itself, no? Like putting a
loop into ->enter:

do {
  go to sleep;
while (should sleep);

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: [PATCH v2] pm_ops: add system quiesce/activate hooks

[David Brownell]

On Thursday 12 April 2007 11:54 pm, Johannes Berg wrote:
> On Thu, 2007-04-12 at 21:36 +0400, Dmitry Krivoschekov wrote:
> 
> > For example, I'd like to enter back to suspend mode
> > right from "activate" stage, because I've woken up just
> > to update some data and I do not want to resume all devices
> > for that, is it ok for "activate"?

That came up on the list a while back in some email from MontaVista, ISTR.
And ISTR some scientific or industrial system needed it too.


> I'd think that better be done within ->enter itself, no? Like putting a
> loop into ->enter:
> 
> do {
>   go to sleep;
> while (should sleep);

Better answer, yes.  Plus remember that the "activate" stage is at a
weird spot in the resume sequence:  some of the devices have been
resumed, but not all of them.

Surely if this "partial wakeup" scenario is needed, it's going to
be highly platform-specific which devices need to be resumed so
the relvant work can be done?


In fact, I wonder if a better way to look at this might not be
that it's not a real "system sleep" state so much as just a deep
runtime PM state ... the sort of thing that the idle loop ought
to be able to enter, in a system that's been quiesced far enough
because of NO_HZ, power-aware drivers, and general inactivity.
For some systems I'm relatively sure that'd be a good perspective: 
ones where the runtime PM is good enough.


Another solution recognized that the only work that was needed
was updating certain counters and GPIOs.  So the code that went
to sleep knew about various levels of wakeup; if it was just the
counting update, it left most clocks (and the DRAM!) off, and
went right back to sleep.  The real intelligence was in the
sleep logic called by enter().  The generalization was in that
down'n'dirty platform code; it had callouts to other code which 
also lived in the SRAM, and not every wake event caused return
from enter().

- Dave

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Thu, 2007-04-12 at 21:36 +0400, Dmitry Krivoschekov wrote:

> For example, I'd like to enter back to suspend mode
> right from "activate" stage, because I've woken up just
> to update some data and I do not want to resume all devices
> for that, is it ok for "activate"?

Come to think of it, this sounds more like a confusion of terms instead
of actually thinking "activate" would be good.

quiesce here is mostly to turn off IRQs and activate mostly to turn them
back on. The enter callback is what should actually enter suspend. I can
see how you can confused "enter" suspend and "activate" suspend though.

The docs in the patch I posted should make it clear enough, but if
somebody can come up with a better name for "activate" I'm all ears.

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: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

> The docs in the patch I posted should make it clear enough, but if
> somebody can come up with a better name for "activate" I'm all ears.

I personally quite like your original irq_off/irq_on :-)

But I won't put up a fight for it if others disagree..

Ben.

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Rafael J. Wysocki]

On Friday, 13 April 2007 11:07, Benjamin Herrenschmidt wrote:
> 
> > The docs in the patch I posted should make it clear enough, but if
> > somebody can come up with a better name for "activate" I'm all ears.
> 
> I personally quite like your original irq_off/irq_on :-)
> 
> But I won't put up a fight for it if others disagree..

Well, I didn't like them, because they should refer to the local CPU only (the
other ones are supposed to be disabled at that point).  Also, since we are
going to do some things apart from disabling the (local) IRQs in there, I
thought some other names would be better.  Still, if that's confusing, I'm not
going to fight either. :-)

Greetings,
Rafael

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Fri, 2007-04-13 at 13:47 +0200, Rafael J. Wysocki wrote:

> Well, I didn't like them, because they should refer to the local CPU only (the
> other ones are supposed to be disabled at that point).  Also, since we are
> going to do some things apart from disabling the (local) IRQs in there, I
> thought some other names would be better.

True.

>   Still, if that's confusing, I'm not
> going to fight either. :-)

Well it seemed to me that Dmitry's question might have been due to a
slight confusion here. Then again, the docs are there and clearly
describe what to do... I've clarified the description again and will
resend in a minute.

Do we want the patch that munges pm_ops when registering or the one that
modifies all current users?

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 --]

[PATCH v4] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

For powermac, we need to do some things between suspending devices and
device_power_off, for example setting the decrementer. This patch
introduces pm_ops.quiesce and pm_ops.activate which will be called instead
of disabling/enabling irqs so platforms get control over this step.

If not assigned, these two calls will be assigned with defaults during
set_pm_ops.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
 include/linux/pm.h  |   12 ++++++++++++
 kernel/power/main.c |   35 ++++++++++++++++++++++++++++++++---
 2 files changed, 44 insertions(+), 3 deletions(-)

--- wireless-dev.orig/include/linux/pm.h	2007-04-13 10:09:17.835356880 +0200
+++ wireless-dev/include/linux/pm.h	2007-04-13 14:57:01.525356880 +0200
@@ -135,9 +135,19 @@ typedef int __bitwise suspend_disk_metho
  * @prepare: Prepare the platform for the given suspend state. Can return a
  *	negative error code if necessary.
  *
+ * @quiesce: This callback is called after devices are suspended but before
+ *	they are powered down. If assigned, this callback must at least turn
+ *	off local IRQs. If left unassigned, a default callback that does
+ *	nothing but turn off local IRQs is assigned during pm_set_ops().
+ *
  * @enter: Enter the given suspend state, must be assigned. Can return a
  *	negative error code if necessary.
  *
+ * @activate: This callback is called after devices are powered up but
+ *	before they resume. If assigned, this callback must at least turn
+ *	on local IRQs. If left unassigned, a default callback that does
+ *	nothing but turn on local IRQs is assigned during pm_set_ops().
+ *
  * @finish: Called when the system has left the given state and all devices
  *	are resumed. The return value is ignored.
  *
@@ -155,7 +165,9 @@ typedef int __bitwise suspend_disk_metho
 struct pm_ops {
 	int (*valid)(suspend_state_t state);
 	int (*prepare)(suspend_state_t state);
+	void (*quiesce)(suspend_state_t state);
 	int (*enter)(suspend_state_t state);
+	void (*activate)(suspend_state_t state);
 	int (*finish)(suspend_state_t state);
 	suspend_disk_method_t pm_disk_mode;
 };
--- wireless-dev.orig/kernel/power/main.c	2007-04-13 10:09:17.835356880 +0200
+++ wireless-dev/kernel/power/main.c	2007-04-13 14:53:44.975356880 +0200
@@ -33,6 +33,28 @@ struct pm_ops *pm_ops;
 suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
 
 /**
+ * pm_default_quiesce - default quiesce callback
+ *
+ * pm_ops drivers that need to do nothing but disable IRQs
+ * leave .quiesce unassigned and pm_set_ops() assigns this.
+ */
+static void pm_default_quiesce(suspend_state_t state)
+{
+	local_irq_disable();
+}
+
+/**
+ * pm_default_activate - default activate callback
+ *
+ * pm_ops drivers that need to do nothing but enable IRQs
+ * leave .activate unassigned and pm_set_ops assigns this.
+ */
+static void pm_default_activate(suspend_state_t state)
+{
+	local_irq_enable();
+}
+
+/**
  *	pm_set_ops - Set the global power method table. 
  *	@ops:	Pointer to ops structure.
  */
@@ -45,6 +67,12 @@ void pm_set_ops(struct pm_ops * ops)
 		pm_disk_mode = ops->pm_disk_mode;
 	} else
 		pm_disk_mode = PM_DISK_SHUTDOWN;
+
+	if (!ops->quiesce)
+		ops->quiesce = pm_default_quiesce;
+	if (!ops->activate)
+		ops->activate = pm_default_activate;
+
 	mutex_unlock(&pm_mutex);
 }
 
@@ -132,9 +160,9 @@ static int suspend_prepare(suspend_state
 int suspend_enter(suspend_state_t state)
 {
 	int error = 0;
-	unsigned long flags;
 
-	local_irq_save(flags);
+	pm_ops->quiesce(state);
+	BUG_ON(!irqs_disabled());
 
 	if ((error = device_power_down(PMSG_SUSPEND))) {
 		printk(KERN_ERR "Some devices failed to power down\n");
@@ -143,7 +171,8 @@ int suspend_enter(suspend_state_t state)
 	error = pm_ops->enter(state);
 	device_power_up();
  Done:
-	local_irq_restore(flags);
+	pm_ops->activate(state);
+	BUG_ON(irqs_disabled());
 	return error;
 }
 

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Rafael J. Wysocki]

On Friday, 13 April 2007 15:26, Johannes Berg wrote:
> For powermac, we need to do some things between suspending devices and
> device_power_off, for example setting the decrementer. This patch
> introduces pm_ops.quiesce and pm_ops.activate which will be called instead
> of disabling/enabling irqs so platforms get control over this step.
> 
> If not assigned, these two calls will be assigned with defaults during
> set_pm_ops.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

I have no objections.  Pavel, what do you think?

Rafael


> ---
>  include/linux/pm.h  |   12 ++++++++++++
>  kernel/power/main.c |   35 ++++++++++++++++++++++++++++++++---
>  2 files changed, 44 insertions(+), 3 deletions(-)
> 
> --- wireless-dev.orig/include/linux/pm.h	2007-04-13 10:09:17.835356880 +0200
> +++ wireless-dev/include/linux/pm.h	2007-04-13 14:57:01.525356880 +0200
> @@ -135,9 +135,19 @@ typedef int __bitwise suspend_disk_metho
>   * @prepare: Prepare the platform for the given suspend state. Can return a
>   *	negative error code if necessary.
>   *
> + * @quiesce: This callback is called after devices are suspended but before
> + *	they are powered down. If assigned, this callback must at least turn
> + *	off local IRQs. If left unassigned, a default callback that does
> + *	nothing but turn off local IRQs is assigned during pm_set_ops().
> + *
>   * @enter: Enter the given suspend state, must be assigned. Can return a
>   *	negative error code if necessary.
>   *
> + * @activate: This callback is called after devices are powered up but
> + *	before they resume. If assigned, this callback must at least turn
> + *	on local IRQs. If left unassigned, a default callback that does
> + *	nothing but turn on local IRQs is assigned during pm_set_ops().
> + *
>   * @finish: Called when the system has left the given state and all devices
>   *	are resumed. The return value is ignored.
>   *
> @@ -155,7 +165,9 @@ typedef int __bitwise suspend_disk_metho
>  struct pm_ops {
>  	int (*valid)(suspend_state_t state);
>  	int (*prepare)(suspend_state_t state);
> +	void (*quiesce)(suspend_state_t state);
>  	int (*enter)(suspend_state_t state);
> +	void (*activate)(suspend_state_t state);
>  	int (*finish)(suspend_state_t state);
>  	suspend_disk_method_t pm_disk_mode;
>  };
> --- wireless-dev.orig/kernel/power/main.c	2007-04-13 10:09:17.835356880 +0200
> +++ wireless-dev/kernel/power/main.c	2007-04-13 14:53:44.975356880 +0200
> @@ -33,6 +33,28 @@ struct pm_ops *pm_ops;
>  suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
>  
>  /**
> + * pm_default_quiesce - default quiesce callback
> + *
> + * pm_ops drivers that need to do nothing but disable IRQs
> + * leave .quiesce unassigned and pm_set_ops() assigns this.
> + */
> +static void pm_default_quiesce(suspend_state_t state)
> +{
> +	local_irq_disable();
> +}
> +
> +/**
> + * pm_default_activate - default activate callback
> + *
> + * pm_ops drivers that need to do nothing but enable IRQs
> + * leave .activate unassigned and pm_set_ops assigns this.
> + */
> +static void pm_default_activate(suspend_state_t state)
> +{
> +	local_irq_enable();
> +}
> +
> +/**
>   *	pm_set_ops - Set the global power method table. 
>   *	@ops:	Pointer to ops structure.
>   */
> @@ -45,6 +67,12 @@ void pm_set_ops(struct pm_ops * ops)
>  		pm_disk_mode = ops->pm_disk_mode;
>  	} else
>  		pm_disk_mode = PM_DISK_SHUTDOWN;
> +
> +	if (!ops->quiesce)
> +		ops->quiesce = pm_default_quiesce;
> +	if (!ops->activate)
> +		ops->activate = pm_default_activate;
> +
>  	mutex_unlock(&pm_mutex);
>  }
>  
> @@ -132,9 +160,9 @@ static int suspend_prepare(suspend_state
>  int suspend_enter(suspend_state_t state)
>  {
>  	int error = 0;
> -	unsigned long flags;
>  
> -	local_irq_save(flags);
> +	pm_ops->quiesce(state);
> +	BUG_ON(!irqs_disabled());
>  
>  	if ((error = device_power_down(PMSG_SUSPEND))) {
>  		printk(KERN_ERR "Some devices failed to power down\n");
> @@ -143,7 +171,8 @@ int suspend_enter(suspend_state_t state)
>  	error = pm_ops->enter(state);
>  	device_power_up();
>   Done:
> -	local_irq_restore(flags);
> +	pm_ops->activate(state);
> +	BUG_ON(irqs_disabled());
>  	return error;
>  }

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

Hi!

> > For powermac, we need to do some things between suspending devices and
> > device_power_off, for example setting the decrementer. This patch
> > introduces pm_ops.quiesce and pm_ops.activate which will be called instead
> > of disabling/enabling irqs so platforms get control over this step.
> > 
> > If not assigned, these two calls will be assigned with defaults during
> > set_pm_ops.
> > 
> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> 
> I have no objections.  Pavel, what do you think?

That it is same old hack it used to be. There's nothing magic about
decrementer, and I do not think it is _neccessary_ to touch it just
before cli. Just disable it from sysdev handler or something.

> > --- wireless-dev.orig/include/linux/pm.h	2007-04-13 10:09:17.835356880 +0200
> > +++ wireless-dev/include/linux/pm.h	2007-04-13 14:57:01.525356880 +0200
> > @@ -135,9 +135,19 @@ typedef int __bitwise suspend_disk_metho
> >   * @prepare: Prepare the platform for the given suspend state. Can return a
> >   *	negative error code if necessary.
> >   *
> > + * @quiesce: This callback is called after devices are suspended but before
> > + *	they are powered down. If assigned, this callback must at least turn
> > + *	off local IRQs. If left unassigned, a default callback that does
> > + *	nothing but turn off local IRQs is assigned during pm_set_ops().

Is it called for s2ram? swsusp? uswsusp?

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

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Fri, 2007-04-13 at 22:58 +0200, Pavel Machek wrote:

> That it is same old hack it used to be. There's nothing magic about
> decrementer, and I do not think it is _neccessary_ to touch it just
> before cli. Just disable it from sysdev handler or something.

No, you cannot disable it from sysdev handler because you have to be
able to take interrupts. Can we *please* move on to some more productive
discussion?

> > > --- wireless-dev.orig/include/linux/pm.h	2007-04-13 10:09:17.835356880 +0200
> > > +++ wireless-dev/include/linux/pm.h	2007-04-13 14:57:01.525356880 +0200
> > > @@ -135,9 +135,19 @@ typedef int __bitwise suspend_disk_metho
> > >   * @prepare: Prepare the platform for the given suspend state. Can return a
> > >   *	negative error code if necessary.
> > >   *
> > > + * @quiesce: This callback is called after devices are suspended but before
> > > + *	they are powered down. If assigned, this callback must at least turn
> > > + *	off local IRQs. If left unassigned, a default callback that does
> > > + *	nothing but turn off local IRQs is assigned during pm_set_ops().
> 
> Is it called for s2ram? swsusp? uswsusp?

Whoops, I had that in an earlier version, it's not supposed to be called
for (u)swsusp because pm_ops involvement there is a hack anyway. I'll
fix the description.

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: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

Hi!

> > That it is same old hack it used to be. There's nothing magic about
> > decrementer, and I do not think it is _neccessary_ to touch it just
> > before cli. Just disable it from sysdev handler or something.
> 
> No, you cannot disable it from sysdev handler because you have to be
> able to take interrupts. Can we *please* move on to some more productive
> discussion?

So what happens if you disable it as a platform device? There has to
be better solution than this. What is so special about decrementer?

We were able to handle MTRR registers on PC. Timer setup fit into the
model too...

> > > > @@ -135,9 +135,19 @@ typedef int __bitwise suspend_disk_metho
> > > >   * @prepare: Prepare the platform for the given suspend state. Can return a
> > > >   *	negative error code if necessary.
> > > >   *
> > > > + * @quiesce: This callback is called after devices are suspended but before
> > > > + *	they are powered down. If assigned, this callback must at least turn
> > > > + *	off local IRQs. If left unassigned, a default callback that does
> > > > + *	nothing but turn off local IRQs is assigned during pm_set_ops().
> > 
> > Is it called for s2ram? swsusp? uswsusp?
> 
> Whoops, I had that in an earlier version, it's not supposed to be called
> for (u)swsusp because pm_ops involvement there is a hack anyway. I'll
> fix the description.

But (u)swsusp needs to shut down the devices, and it does both suspend
and powerdown, too...?
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Fri, 2007-04-13 at 23:12 +0200, Pavel Machek wrote:


> > Whoops, I had that in an earlier version, it's not supposed to be called
> > for (u)swsusp because pm_ops involvement there is a hack anyway. I'll
> > fix the description.
> 
> But (u)swsusp needs to shut down the devices, and it does both suspend
> and powerdown, too...?

Yes, but the special S4 state is just hacked onto pm_ops, nothing but
ACPI S4 will ever sanely use pm_ops. If I'd invented pm_ops I'd made
pm_ops solely for suspend to ram/standby/... and added a *separate* hook
for pm_disk_mode and S4 because logically that really doesn't belong to
pm_ops which does almost exclusively things for suspend to ram/standby.

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: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

On Fri 2007-04-13 23:18:05, Johannes Berg wrote:
> On Fri, 2007-04-13 at 23:12 +0200, Pavel Machek wrote:
> 
> 
> > > Whoops, I had that in an earlier version, it's not supposed to be called
> > > for (u)swsusp because pm_ops involvement there is a hack anyway. I'll
> > > fix the description.
> > 
> > But (u)swsusp needs to shut down the devices, and it does both suspend
> > and powerdown, too...?
> 
> Yes, but the special S4 state is just hacked onto pm_ops, nothing but
> ACPI S4 will ever sanely use pm_ops. If I'd invented pm_ops I'd made
> pm_ops solely for suspend to ram/standby/... and added a *separate* hook
> for pm_disk_mode and S4 because logically that really doesn't belong to
> pm_ops which does almost exclusively things for suspend to ram/standby.

Still, swsusp shuts down the devices:

        if ((error = arch_prepare_suspend()))
                return error;

        local_irq_disable();
        /* At this point, device_suspend() has been called, but *not*
         * device_power_down(). We *must* device_power_down() now.
         * Otherwise, drivers for some devices (e.g. interrupt
controllers)
         * become desynchronized with the actual state of the hardware
         * at resume time, and evil weirdness ensues.
         */
        if ((error = device_power_down(PMSG_FREEZE))) {
                printk(KERN_ERR "Some devices failed to power down, aborting suspend\n");
                goto Enable_irqs;
        }

This local_irq_disable() is very similar to the one you are doing in
suspend-to-RAM. According to your description, it needs to play with
the decrementor, too. But I'd really prefer not to have pm_ops call
here -- exactly because swsusp has nothing to do with pm_ops in
shutdown mode.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Fri, 2007-04-13 at 23:33 +0200, Pavel Machek wrote:

> Still, swsusp shuts down the devices:
> 
>         if ((error = arch_prepare_suspend()))
>                 return error;
> 
>         local_irq_disable();
>         /* At this point, device_suspend() has been called, but *not*
>          * device_power_down(). We *must* device_power_down() now.
>          * Otherwise, drivers for some devices (e.g. interrupt
> controllers)
>          * become desynchronized with the actual state of the hardware
>          * at resume time, and evil weirdness ensues.
>          */
>         if ((error = device_power_down(PMSG_FREEZE))) {
>                 printk(KERN_ERR "Some devices failed to power down, aborting suspend\n");
>                 goto Enable_irqs;
>         }
> 
> This local_irq_disable() is very similar to the one you are doing in
> suspend-to-RAM. According to your description, it needs to play with
> the decrementor, too. But I'd really prefer not to have pm_ops call
> here -- exactly because swsusp has nothing to do with pm_ops in
> shutdown mode.

Right. But we don't actually care because we just power down, we don't
have to put the CPU into a proper state.

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: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

On Fri 2007-04-13 23:45:48, Johannes Berg wrote:
> On Fri, 2007-04-13 at 23:33 +0200, Pavel Machek wrote:
> 
> > Still, swsusp shuts down the devices:
> > 
> >         if ((error = arch_prepare_suspend()))
> >                 return error;
> > 
> >         local_irq_disable();
> >         /* At this point, device_suspend() has been called, but *not*
> >          * device_power_down(). We *must* device_power_down() now.
> >          * Otherwise, drivers for some devices (e.g. interrupt
> > controllers)
> >          * become desynchronized with the actual state of the hardware
> >          * at resume time, and evil weirdness ensues.
> >          */
> >         if ((error = device_power_down(PMSG_FREEZE))) {
> >                 printk(KERN_ERR "Some devices failed to power down, aborting suspend\n");
> >                 goto Enable_irqs;
> >         }
> > 
> > This local_irq_disable() is very similar to the one you are doing in
> > suspend-to-RAM. According to your description, it needs to play with
> > the decrementor, too. But I'd really prefer not to have pm_ops call
> > here -- exactly because swsusp has nothing to do with pm_ops in
> > shutdown mode.
> 
> Right. But we don't actually care because we just power down, we don't
> have to put the CPU into a proper state.

Look again, this sequence is before taking system snapshot, and then
we have to recover and save the snapshot, too.

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

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Fri, 2007-04-13 at 23:52 +0200, Pavel Machek wrote:

> Look again, this sequence is before taking system snapshot, and then
> we have to recover and save the snapshot, too.

I know. But that doesn't make us care.

As I explained previously, the decrementer exception is pending until it
is taken. It doesn't matter one bit whether it is pending during the
snapshot, but it does matter if we need the CPU to go to sleep later
because it won't when the exception is pending.

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: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Rafael J. Wysocki]

On Friday, 13 April 2007 23:59, Johannes Berg wrote:
> On Fri, 2007-04-13 at 23:52 +0200, Pavel Machek wrote:
> 
> > Look again, this sequence is before taking system snapshot, and then
> > we have to recover and save the snapshot, too.
> 
> I know. But that doesn't make us care.
> 
> As I explained previously, the decrementer exception is pending until it
> is taken. It doesn't matter one bit whether it is pending during the
> snapshot, but it does matter if we need the CPU to go to sleep later
> because it won't when the exception is pending.

Technically, they may be not needed by (u)swsusp, but since (u)swsusp already
uses pm_ops in the platform mode, it formally should use these new hooks in
the platform mode too.  Otherwise it won't make sense to use pm_ops in
(u)swsusp at all, IMHO.

Greetings,
Rafael

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Sat, 2007-04-14 at 00:18 +0200, Rafael J. Wysocki wrote:

> Technically, they may be not needed by (u)swsusp, but since (u)swsusp already
> uses pm_ops in the platform mode, it formally should use these new hooks in
> the platform mode too.  

Sure. I'll do that.

> Otherwise it won't make sense to use pm_ops in
> (u)swsusp at all, IMHO.

IMHO it never has and never will be, it's just to make it all fit the
ACPI model ;)

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: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Rafael J. Wysocki]

On Saturday, 14 April 2007 00:20, Johannes Berg wrote:
> On Sat, 2007-04-14 at 00:18 +0200, Rafael J. Wysocki wrote:
> 
> > Technically, they may be not needed by (u)swsusp, but since (u)swsusp already
> > uses pm_ops in the platform mode, it formally should use these new hooks in
> > the platform mode too.  
> 
> Sure. I'll do that.
> 
> > Otherwise it won't make sense to use pm_ops in
> > (u)swsusp at all, IMHO.
> 
> IMHO it never has and never will be, it's just to make it all fit the
> ACPI model ;)

I'm not against changing that, as you might have noticed. :-)

Greetings,
Rafael

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Sat, 2007-04-14 at 00:49 +0200, Rafael J. Wysocki wrote:

> > IMHO it never has and never will be, it's just to make it all fit the
> > ACPI model ;)
> 
> I'm not against changing that, as you might have noticed. :-)

Yeah, but I can happily ignore it ;) Maybe if I really have a lot of
time at some point...

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: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Rafael J. Wysocki]

On Friday, 13 April 2007 23:33, Pavel Machek wrote:
> On Fri 2007-04-13 23:18:05, Johannes Berg wrote:
> > On Fri, 2007-04-13 at 23:12 +0200, Pavel Machek wrote:
> > 
> > 
> > > > Whoops, I had that in an earlier version, it's not supposed to be called
> > > > for (u)swsusp because pm_ops involvement there is a hack anyway. I'll
> > > > fix the description.
> > > 
> > > But (u)swsusp needs to shut down the devices, and it does both suspend
> > > and powerdown, too...?
> > 
> > Yes, but the special S4 state is just hacked onto pm_ops, nothing but
> > ACPI S4 will ever sanely use pm_ops. If I'd invented pm_ops I'd made
> > pm_ops solely for suspend to ram/standby/... and added a *separate* hook
> > for pm_disk_mode and S4 because logically that really doesn't belong to
> > pm_ops which does almost exclusively things for suspend to ram/standby.
> 
> Still, swsusp shuts down the devices:
> 
>         if ((error = arch_prepare_suspend()))
>                 return error;
> 
>         local_irq_disable();
>         /* At this point, device_suspend() has been called, but *not*
>          * device_power_down(). We *must* device_power_down() now.
>          * Otherwise, drivers for some devices (e.g. interrupt
> controllers)
>          * become desynchronized with the actual state of the hardware
>          * at resume time, and evil weirdness ensues.
>          */
>         if ((error = device_power_down(PMSG_FREEZE))) {
>                 printk(KERN_ERR "Some devices failed to power down, aborting suspend\n");
>                 goto Enable_irqs;
>         }
> 
> This local_irq_disable() is very similar to the one you are doing in
> suspend-to-RAM. According to your description, it needs to play with
> the decrementor, too. But I'd really prefer not to have pm_ops call
> here -- exactly because swsusp has nothing to do with pm_ops in
> shutdown mode.

Hmm, I missed that. :-(

I think we need to make things clear: Either we add the additional hooks to
pm_ops in which case they should be taken into account in the (u)swsusp code
too, or we don't add them at all.

Well, there also is one more solution.  Namely, we can add the hooks to pm_ops
and make (u)swsusp use something else instead of pm_ops, but that would require
some more consideration.

Greetings,
Rafael

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Sat, 2007-04-14 at 00:09 +0200, Rafael J. Wysocki wrote:

> Hmm, I missed that. :-(
> 
> I think we need to make things clear: Either we add the additional hooks to
> pm_ops in which case they should be taken into account in the (u)swsusp code
> too, or we don't add them at all.

I can do that. I don't care one bit since I will never use pm_ops for
suspend to disk anyway.

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: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

On Sat 2007-04-14 00:13:26, Johannes Berg wrote:
> On Sat, 2007-04-14 at 00:09 +0200, Rafael J. Wysocki wrote:
> 
> > Hmm, I missed that. :-(
> > 
> > I think we need to make things clear: Either we add the additional hooks to
> > pm_ops in which case they should be taken into account in the (u)swsusp code
> > too, or we don't add them at all.
> 
> I can do that. I don't care one bit since I will never use pm_ops for
> suspend to disk anyway.

Please don't do that just yet.

I see ppc as being really special here.

Normally, preparation for s2ram and preparation for snapshot is really
similar, but you have exception to that rule. I do think clean
solution is redesigning plaform/sysdevs to have stable order.

If you added one platform and one sysdev, would that _work_? AFAICT
you say it would and Ben says it would not. (Lets leave uglyness
debate for later).
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Paul Mackerras]

Pavel Machek writes:

> I see ppc as being really special here.

It is, I guess.  The salient point is that once the decrementer has an
interrupt pending, the only way to clear it is to take the interrupt.
There is no explicit access to the interrupt pending bit and no
separate enable bit for its interrupt, only the global interrupt
enable for the whole CPU.  We have to turn on the global interrupt
enable when finally putting the system into the suspend state (for
suspend to RAM), otherwise on some machines we'll never wake up again.

Effectively, when dealing with the decrementer during preparation for
suspend-to-RAM, we may need to have interrupts enabled briefly in
order to clear a pending decrementer interrupt.

Paul.

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

> Hmm, I missed that. :-(
> 
> I think we need to make things clear: Either we add the additional hooks to
> pm_ops in which case they should be taken into account in the (u)swsusp code
> too, or we don't add them at all.
> 
> Well, there also is one more solution.  Namely, we can add the hooks to pm_ops
> and make (u)swsusp use something else instead of pm_ops, but that would require
> some more consideration.

Well, what would be nice would be if swsusp wasn't just such a gross
contraption completely bypassing the rest of the suspend/resume
framework...

Ben.

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

On Sat 2007-04-14 08:25:22, Benjamin Herrenschmidt wrote:
> 
> > Hmm, I missed that. :-(
> > 
> > I think we need to make things clear: Either we add the additional hooks to
> > pm_ops in which case they should be taken into account in the (u)swsusp code
> > too, or we don't add them at all.
> > 
> > Well, there also is one more solution.  Namely, we can add the hooks to pm_ops
> > and make (u)swsusp use something else instead of pm_ops, but that would require
> > some more consideration.
> 
> Well, what would be nice would be if swsusp wasn't just such a gross
> contraption completely bypassing the rest of the suspend/resume
> framework...

Great, so:

* you claim that current code is hacked up

* I claim, that if I apply the patch, the result will be ugly hack.

Good; I think you agree that Johaness' patch certainly does not fix
the uglyness of current code.

=> we both agree that after applying the patch, we'll have hacked-up
code. So... can we do some cleanups before hacking on it more?

If sysdevs need ordering, lets add ordering. If platform devices need
ordering, lets do it. If acpi S4 should not be using pm_ops, we should
probably fix that. If swsusp should not be called from pm_ops, we can
probably change that (but we need to keep userland interface).

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

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

Ok, PowerPC Decrementer 101

The processor contains a special register, the decrementer, which
keeps ... decrementing. It can be set to any arbitrary value at any time
and will decrement in sync with the processor timebase.

There are some subtle differences between implementations regarding what
happens when reaching 0, but the basic idea is that you get an interrupt
(depending on the processor, that interrupt is somewhat a level
interrupt asserted when the decrementer is negative or it can be a kind
of edge interrupt queued up when the dec transitions from 0 to -1).

This decrementer is used as the main timer. Thus it needs to be
operating normally at all time until interrupts are off or the scheduler
will stop working properly, kernel timers will not fire, etc...

(and saying that platforms devices should use mdelay instead is just
gross, I won't even go there. Interrupts are still on -> the core kernel
should operate normally and that includes the main timer source).

Now what happens when we put the processors (well, most desktop
processors, at least the one that concern us in that discussion) to
sleep is that they get out of sleep when an interrupt occur, for
example ... a decrementer interrupt. This is not good for STR for
various reasons related to the way STR works in hardware (the
northbridge snoops that the CPU is going to sleep and starts putting
things down, ultimately shutting the CPU off, it can't really cope if
the CPU wakes up right away and start doing things). Unfortunately, for
other reasons, the procedure of putting the CPU to sleep involves
turning interrupts on. For all external interrupts, that isn't a problem
as we have previously shut them all down on the main PIC, but it is with
the DEC.

The "trick" is that once interrupts are off, we want the DEC to be set
to such a high value that it won't tick anytime soon (that is actually
several seconds, enough in practice). But if we do that after IRQs have
been turned off (from a sysdev), we have the risk that it might have
ticked between turning IRQs off and our sysdev, and thus a DEC
interrupts is already "queued up" (especially on CPUs where it acts as
an edge interrupt) and will screw up our attempt to put the CPU to sleep
later on.

The procedure we use is to set it to 0x7fffffff with IRQs on, then turn
IRQs off, then set it back to 0x7fffffff in case it kicked in just
before and the timer interrupt set it back to a short value. As you can
imagine, thoseh have to be done close together as part of the main irq
disabling procedure, after platform devices have run (that is we can
consider the scheduler as "off") and before sysdev's etc...

Now, in addition to that, we have some weird motherboard stuff we need
to turn off/on, which has to be done after drivers (because it renders
various busses inaccessible in some cases, and might cause DMA snooping
to stop working, I'm not 100% sure, but I know for sure it has to be
done late) but can't be done as a sysdev because we need some
infrastructure like the i2c stuff (and others) that requires semaphores
and timers. It's based on something remotely akin to AML in that we have
to execute "scripts" provided by the firmware and the code to do so need
to run in an environment where scheduler & timers are operating.

That later thing could be dealt with using a platform device if we could
guarantee that platform device is put to sleep last of all devices in
the tree and woken up first. Right now, we have no such guarantee and no
mecanism for it, and I don't see a solution showing up for 2.6.22

In the long run, we might be able to break up that phase to have each
individual device that has such functions associated have ways to call
into them after the device has been put to sleep, but that involves more
complication, probably hook in the generic PCI code etc... and more
ordering issues vs. some motherboard foo so it's definitely not on the
short term radar.

For all those reasons, I do think that the proper, clean and incremental
approach to get our stuff working is to have that pair of hooks allowing
us to "replace" the local_irq_disable/enable calls...

Now it does not need to be pm_ops. I'm fine with arch_pm_irq_quiesce()
kind of thing (or find a better name if you can, maybe
arch_pm_after_devices_suspend() arch_pm_before_device_wakeup() ?) and
have the default implementation of these just do
local_irq_disable/enable.

It's basically about quiescing the scheduler/timers, which on powerpc
(bcs of the way the DEC operates) requires a little bit more than just a
call to local_irq_disable. And once the hook is there, use it for some
other arch specific bits that we can't quite fit anywhere else at the
moment.

Ben.

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Rafael J. Wysocki]

On Saturday, 14 April 2007 01:19, Benjamin Herrenschmidt wrote:
> Ok, PowerPC Decrementer 101
> 
> The processor contains a special register, the decrementer, which
> keeps ... decrementing. It can be set to any arbitrary value at any time
> and will decrement in sync with the processor timebase.
> 
> There are some subtle differences between implementations regarding what
> happens when reaching 0, but the basic idea is that you get an interrupt
> (depending on the processor, that interrupt is somewhat a level
> interrupt asserted when the decrementer is negative or it can be a kind
> of edge interrupt queued up when the dec transitions from 0 to -1).
> 
> This decrementer is used as the main timer. Thus it needs to be
> operating normally at all time until interrupts are off or the scheduler
> will stop working properly, kernel timers will not fire, etc...
> 
> (and saying that platforms devices should use mdelay instead is just
> gross, I won't even go there. Interrupts are still on -> the core kernel
> should operate normally and that includes the main timer source).
> 
> Now what happens when we put the processors (well, most desktop
> processors, at least the one that concern us in that discussion) to
> sleep is that they get out of sleep when an interrupt occur, for
> example ... a decrementer interrupt. This is not good for STR for
> various reasons related to the way STR works in hardware (the
> northbridge snoops that the CPU is going to sleep and starts putting
> things down, ultimately shutting the CPU off, it can't really cope if
> the CPU wakes up right away and start doing things). Unfortunately, for
> other reasons, the procedure of putting the CPU to sleep involves
> turning interrupts on. For all external interrupts, that isn't a problem
> as we have previously shut them all down on the main PIC, but it is with
> the DEC.
> 
> The "trick" is that once interrupts are off, we want the DEC to be set
> to such a high value that it won't tick anytime soon (that is actually
> several seconds, enough in practice). But if we do that after IRQs have
> been turned off (from a sysdev), we have the risk that it might have
> ticked between turning IRQs off and our sysdev, and thus a DEC
> interrupts is already "queued up" (especially on CPUs where it acts as
> an edge interrupt) and will screw up our attempt to put the CPU to sleep
> later on.
> 
> The procedure we use is to set it to 0x7fffffff with IRQs on, then turn
> IRQs off, then set it back to 0x7fffffff in case it kicked in just
> before and the timer interrupt set it back to a short value. As you can
> imagine, thoseh have to be done close together as part of the main irq
> disabling procedure, after platform devices have run (that is we can
> consider the scheduler as "off") and before sysdev's etc...
> 
> Now, in addition to that, we have some weird motherboard stuff we need
> to turn off/on, which has to be done after drivers (because it renders
> various busses inaccessible in some cases, and might cause DMA snooping
> to stop working, I'm not 100% sure, but I know for sure it has to be
> done late) but can't be done as a sysdev because we need some
> infrastructure like the i2c stuff (and others) that requires semaphores
> and timers. It's based on something remotely akin to AML in that we have
> to execute "scripts" provided by the firmware and the code to do so need
> to run in an environment where scheduler & timers are operating.
> 
> That later thing could be dealt with using a platform device if we could
> guarantee that platform device is put to sleep last of all devices in
> the tree and woken up first. Right now, we have no such guarantee and no
> mecanism for it, and I don't see a solution showing up for 2.6.22
> 
> In the long run, we might be able to break up that phase to have each
> individual device that has such functions associated have ways to call
> into them after the device has been put to sleep, but that involves more
> complication, probably hook in the generic PCI code etc... and more
> ordering issues vs. some motherboard foo so it's definitely not on the
> short term radar.
> 
> For all those reasons, I do think that the proper, clean and incremental
> approach to get our stuff working is to have that pair of hooks allowing
> us to "replace" the local_irq_disable/enable calls...
> 
> Now it does not need to be pm_ops. I'm fine with arch_pm_irq_quiesce()
> kind of thing (or find a better name if you can, maybe
> arch_pm_after_devices_suspend() arch_pm_before_device_wakeup() ?) and
> have the default implementation of these just do
> local_irq_disable/enable.

I like this idea.

Greetings,
Rafael

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Sat, 2007-04-14 at 11:14 +0200, Rafael J. Wysocki wrote:

> > Now it does not need to be pm_ops. I'm fine with arch_pm_irq_quiesce()
> > kind of thing (or find a better name if you can, maybe
> > arch_pm_after_devices_suspend() arch_pm_before_device_wakeup() ?) and
> > have the default implementation of these just do
> > local_irq_disable/enable.
> 
> I like this idea.

I don't really. There can possibly be multiple pm_ops for one arch, who
knows that they all need to do the same thing here? (It would probably
be true for us right now though.)

Also, all other things suspend to ram does go through pm_ops so IMHO
adding an arch hook here now would just unnecessarily complicate the
whole thing since suddenly you may need to do more than just pm_ops for
suspend to ram.

On the other hand, if doing an arch hook here and something like
CONFIG_ARCH_HAS_SUSPEND_IRQ_HOOKS would actually allow us to end this
pointless discussion, then why not.

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: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

> On the other hand, if doing an arch hook here and something like
> CONFIG_ARCH_HAS_SUSPEND_IRQ_HOOKS would actually allow us to end this
> pointless discussion, then why not.

The preferred approach for that sort of thing nowadays is to have
the generic code contain

#ifndef arch_foo_bar()
#define arch_foo_bar()	do { } while(0)
#endif

And have the arch code do

void arch_foo_bar(void)
{
	xxx
}
#define arch_foo_bar arch_foo_bar

Ben.

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

Hi!

> Ok, PowerPC Decrementer 101

(Thanks for explanation).

> Now, in addition to that, we have some weird motherboard stuff we need
> to turn off/on, which has to be done after drivers (because it renders
> various busses inaccessible in some cases, and might cause DMA snooping
> to stop working, I'm not 100% sure, but I know for sure it has to be
> done late) but can't be done as a sysdev because we need some
> infrastructure like the i2c stuff (and others) that requires semaphores
> and timers. It's based on something remotely akin to AML in that we have
> to execute "scripts" provided by the firmware and the code to do so need
> to run in an environment where scheduler & timers are operating.

Does the "weird motherboard" stuff need to be suspended/resumed for
swsusp memory snapshot?

> For all those reasons, I do think that the proper, clean and incremental
> approach to get our stuff working is to have that pair of hooks allowing
> us to "replace" the local_irq_disable/enable calls...
> 
> Now it does not need to be pm_ops. I'm fine with arch_pm_irq_quiesce()
> kind of thing (or find a better name if you can, maybe

Well, I guess arch_pm_irq_quiesce_for_s2ram() would be
acceptable... but that would be only called for s2ram... which should
be enough for decrementer AFAICT.

> It's basically about quiescing the scheduler/timers, which on powerpc
> (bcs of the way the DEC operates) requires a little bit more than just a
> call to local_irq_disable. And once the hook is there, use it for some
> other arch specific bits that we can't quite fit anywhere else at the
> moment.

As decrementer is special for s2ram, we can add the hook. (It is
single hook). If we need to do something for snapshots, too... well,
we can still add arch_pm_irq_quiesce_for_snapshot() and
arch_pm_irq_quiesce_for_powerdown() etc, but it would get ugly fast.

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

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Mon, 2007-04-16 at 09:32 +0200, Pavel Machek wrote:

> As decrementer is special for s2ram, we can add the hook. (It is
> single hook).

No, it's two, for enabling as well.

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: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

Hi!

> > As decrementer is special for s2ram, we can add the hook. (It is
> > single hook).
> 
> No, it's two, for enabling as well.

Two is ok, six would not be.
									Pavel

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

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Paul Mackerras]

Johannes Berg writes:

> No, it's two, for enabling as well.

I'm not sure we actually need a special enable hook.  I assume we have
a sysdev for the timekeeping stuff, whose resume function gets called
before interrupts are enabled.  That can set the decrementer to a
suitable value.  If we get an extra decrementer interrupt when
interrupts are enabled with local_irq_enable, it doesn't matter.

Paul.

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

On Mon, 2007-04-16 at 09:32 +0200, Pavel Machek wrote:

> Does the "weird motherboard" stuff need to be suspended/resumed for
> swsusp memory snapshot?

I don't think so... I need to get into the details to verify, it might
make sense to run some of the "resume" bits of it when resuming a STD
kernel but I doubt it. It won't harm tho.

> Well, I guess arch_pm_irq_quiesce_for_s2ram() would be
> acceptable... but that would be only called for s2ram... which should
> be enough for decrementer AFAICT.

That's a pretty ugly name :-0 I'd be fine just having
arch_pm_irq_quiesce() and only call it from the right s2ram call sites,
or pass it an argument indicating the type of suspend and let the arch
decide what to do. I fail to see the point in being so overly
restrictive on something which will in the long run have little to no
impact.

> As decrementer is special for s2ram, we can add the hook. (It is
> single hook). If we need to do something for snapshots, too... well,
> we can still add arch_pm_irq_quiesce_for_snapshot() and
> arch_pm_irq_quiesce_for_powerdown() etc, but it would get ugly fast.

Which is why I think we should stick with the pair

arch_pm_irq_quiesce() and arch_pm_irq_activate() or whatever name you
come up with, and either have them only called for s2ram or pass the
suspend type to them, I don't like the over long weird names you are
coming up with but that's a detail :-)

Ben.

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Rafael J. Wysocki]

On Saturday, 14 April 2007 00:25, Benjamin Herrenschmidt wrote:
> 
> > Hmm, I missed that. :-(
> > 
> > I think we need to make things clear: Either we add the additional hooks to
> > pm_ops in which case they should be taken into account in the (u)swsusp code
> > too, or we don't add them at all.
> > 
> > Well, there also is one more solution.  Namely, we can add the hooks to pm_ops
> > and make (u)swsusp use something else instead of pm_ops, but that would require
> > some more consideration.
> 
> Well, what would be nice would be if swsusp wasn't just such a gross
> contraption completely bypassing the rest of the suspend/resume
> framework...

IMHO, swsusp is a really special case, especially when it doesn't use pm_ops.

On a PC it may look like a "typical" suspend/resume operation, but I think it
generally is not one.  Although we try to use as many pieces of the framework
in swsusp as we can, we're still running into problems with that.  There are
just too many _practical_ differences.

Greetings,
Rafael

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

On Fri, 2007-04-13 at 23:12 +0200, Pavel Machek wrote:
> Hi!
> 
> > > That it is same old hack it used to be. There's nothing magic about
> > > decrementer, and I do not think it is _neccessary_ to touch it just
> > > before cli. Just disable it from sysdev handler or something.
> > 
> > No, you cannot disable it from sysdev handler because you have to be
> > able to take interrupts. Can we *please* move on to some more productive
> > discussion?
> 
> So what happens if you disable it as a platform device? There has to
> be better solution than this. What is so special about decrementer?

That you are dead since once it's disabled, kernel timers etc... stop
working.

> We were able to handle MTRR registers on PC. Timer setup fit into the
> model too...

I don't care what you do on x86. If I tell you I need to take control at
that point, then I need. Beside I really can't understand why you are so
dead set against an arch hook there. It's not like this will do anything
to you... it can't propagate to drivers, it's purely arch stuff, so
what ? 

Ben.

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

Hi!

> > > > That it is same old hack it used to be. There's nothing magic about
> > > > decrementer, and I do not think it is _neccessary_ to touch it just
> > > > before cli. Just disable it from sysdev handler or something.
> > > 
> > > No, you cannot disable it from sysdev handler because you have to be
> > > able to take interrupts. Can we *please* move on to some more productive
> > > discussion?
> > 
> > So what happens if you disable it as a platform device? There has to
> > be better solution than this. What is so special about decrementer?
> 
> That you are dead since once it's disabled, kernel timers etc... stop
> working.
> 
> > We were able to handle MTRR registers on PC. Timer setup fit into the
> > model too...
> 
> I don't care what you do on x86. If I tell you I need to take control at
> that point, then I need. Beside I really can't understand why you are so
> dead set against an arch hook there. It's not like this will do anything
> to you... it can't propagate to drivers, it's purely arch stuff, so
> what ? 

There are about 7 places where we do stop_platform_devices()
local_irq_disable() stop_sysdevs(). You call your hook from one of
them and I suspect you need to call it from more than one. Plus, the
hook needs to have documented semantics, so people know if they should
do local_irq_disable() or if they should call your hook.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Johannes Berg]

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

On Fri, 2007-04-13 at 23:56 +0200, Pavel Machek wrote:

> There are about 7 places where we do stop_platform_devices()
> local_irq_disable() stop_sysdevs().

I can only find three of those.

> You call your hook from one of
> them and I suspect you need to call it from more than one. Plus, the
> hook needs to have documented semantics, so people know if they should
> do local_irq_disable() or if they should call your hook.

All but one are related to suspend to disk. I don't see anybody
inventing yet another entry point to the kernel that actually uses
pm_ops internally, but if they do surely they'll follow the code that is
there.

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: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

> There are about 7 places where we do stop_platform_devices()
> local_irq_disable() stop_sysdevs(). You call your hook from one of
> them and I suspect you need to call it from more than one. Plus, the
> hook needs to have documented semantics, so people know if they should
> do local_irq_disable() or if they should call your hook.

The simple fact that these sequence is copied in 7 different places is a
pretty good illustration of what's wrong with the core pm code ...

Ben.

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

On Fri, 2007-04-13 at 22:58 +0200, Pavel Machek wrote:
> Hi!
> 
> > > For powermac, we need to do some things between suspending devices and
> > > device_power_off, for example setting the decrementer. This patch
> > > introduces pm_ops.quiesce and pm_ops.activate which will be called instead
> > > of disabling/enabling irqs so platforms get control over this step.
> > > 
> > > If not assigned, these two calls will be assigned with defaults during
> > > set_pm_ops.
> > > 
> > > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> > 
> > I have no objections.  Pavel, what do you think?
> 
> That it is same old hack it used to be. There's nothing magic about
> decrementer, and I do not think it is _neccessary_ to touch it just
> before cli. Just disable it from sysdev handler or something.

You don't know what you're talking about... so please stop being an ass
and be constructive for once.

We need to deal with the DEC -there-, and with other arch issues while
at it. A sysdev will not do. A non-sydev device will not do due to
ordering issues among other things, at least not without some
significant rework of the driver core PM code.

Thus this is the solution to make it work. Now, if you have decided that
you will refuse making it work on powermacs, then I'll have no choice
but try to push that patch through anyway since you aren't proposing any
alternative solution that would work for us.

(BTW. I still wonder how ACPI can get away without such a hook either
since they motherboard suspend/resume code needs to run with irqs on and
needs to run after all devices are suspended... if they rely on magic
ordering of devices, that would explain why the stuff isn't reliable.
Ordering of devices at the toplevel is -not- reliable in the current
model).
 
Ben.

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

Hi!

> > > > For powermac, we need to do some things between suspending devices and
> > > > device_power_off, for example setting the decrementer. This patch
> > > > introduces pm_ops.quiesce and pm_ops.activate which will be called instead
> > > > of disabling/enabling irqs so platforms get control over this step.
> > > > 
> > > > If not assigned, these two calls will be assigned with defaults during
> > > > set_pm_ops.
> > > > 
> > > > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> > > 
> > > I have no objections.  Pavel, what do you think?
> > 
> > That it is same old hack it used to be. There's nothing magic about
> > decrementer, and I do not think it is _neccessary_ to touch it just
> > before cli. Just disable it from sysdev handler or something.
> 
> You don't know what you're talking about... so please stop being an ass
> and be constructive for once.

It is quite hard to be constructive when you call me body parts. I do
not mind being called a horse, but I'm definitely not for rent.

> We need to deal with the DEC -there-, and with other arch issues
> while

You still did not tell me what other arch issues.

> at it. A sysdev will not do. A non-sydev device will not do due to
> ordering issues among other things, at least not without some
> significant rework of the driver core PM code.

Lets do the rework of core PM code, then.

> Thus this is the solution to make it work. Now, if you have decided that
> you will refuse making it work on powermacs, then I'll have no choice
> but try to push that patch through anyway since you aren't proposing any
> alternative solution that would work for us.

There's more than one local_irq_disable() in suspend code. I need to
know what your magic does, to be able to decide when to call it. (See
the other mail).

You seem to be pushing "this absolutely needs to be done for 2.6.22,
and lets screw the design, we need it now". I disagree. I will not
accept "You don't know what you are talking about, so just merge our
code, we will not tell you". Sorry.

If you absolutely must, just do #ifdef CONFIG_PPC at the specific
part... that way it is at least clear who is responsible, and the
braindamage does not spread.

> (BTW. I still wonder how ACPI can get away without such a hook either
> since they motherboard suspend/resume code needs to run with irqs on and
> needs to run after all devices are suspended... if they rely on
> magic

What motherboard code? We do not have anything like that on PC.

We have "drivers" for few chips every PC has, like i8259 timer,
interrupt controller, APIC, etc. Sysdevs and platform devices are
designed for hardware like that.

If you want to have a rule "platform device is not supposed to rely on
timers still working",  I think you can get that. Such low-level
devices should really use udelay()/mdelay() that spins and does not
need timer hw.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

> We have "drivers" for few chips every PC has, like i8259 timer,
> interrupt controller, APIC, etc. Sysdevs and platform devices are
> designed for hardware like that.
> 
> If you want to have a rule "platform device is not supposed to rely on
> timers still working",  I think you can get that. Such low-level
> devices should really use udelay()/mdelay() that spins and does not
> need timer hw.

That's nonsense.

Ben.

Re: [PATCH v4] pm_ops: add system quiesce/activate hooks

[David Brownell]

On Friday 13 April 2007 3:23 pm, Benjamin Herrenschmidt wrote:
> 
> > We have "drivers" for few chips every PC has, like i8259 timer,
> > interrupt controller, APIC, etc. Sysdevs and platform devices are
> > designed for hardware like that.
> > 
> > If you want to have a rule "platform device is not supposed to rely on
> > timers still working",  I think you can get that. Such low-level
> > devices should really use udelay()/mdelay() that spins and does not
> > need timer hw.
> 
> That's nonsense.

Agreed.  Platform devices are the *primary* device types on
most embedded systems.  Their suspend() and resume() methods
are allowed to use timers and sleep() just like they are with
any other bus type.

We do know that timers aren't available during the suspend_late()
and resume_early() calls, for any bus type.

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Pavel Machek]

Hi!

> > Why not give this added flexibility ? Archs who don't care don't need to
> > bother and it will make us happy... it's not like we are about to -add-
> > burden to other architectures.
> 
> Actually, I personally two hands up for  adding the flexibility,
> but you should define what is supposed to do on this level and
> what is don't, or not desirable.
> 
> For example, I'd like to enter back to suspend mode
> right from "activate" stage, because I've woken up just
> to update some data and I do not want to resume all devices
> for that, is it ok for "activate"?

No, I do not think we can do that w/o major surgery.

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

Re: [PATCH v2] pm_ops: add system quiesce/activate hooks

[Benjamin Herrenschmidt]

> So, where is that callbacks that are so desirable for powermac?

One thing at a time... better to separate the patch adding the callback
from the powermac side implementation. The current powermac code does
not work with the generic pm_ops framework yet anyway (that's why
Johannes is trying to fix), it uses it's own ioctl and doesn't go
through the core PM stuff yet.

Ben

[PATCH] s2ram: add arch irq disable/enable hooks

[Johannes Berg]

For powermac, we need to do some things between suspending devices and
device_power_off, for example setting the decrementer. This patch
allows architectures to define arch_s2ram_{en,dis}able_irqs in their
asm/suspend.h to have control over this step.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
Even though I don't like it, here it is. I still don't see why this
shouldn't be part of pm_ops when pm_ops is responsible for everything
else related to s2ram. But it at least lets me do what I need to.

 kernel/power/main.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

--- wireless-dev.orig/kernel/power/main.c	2007-04-17 18:52:24.536830941 +0200
+++ wireless-dev/kernel/power/main.c	2007-04-17 19:09:40.966830941 +0200
@@ -128,13 +128,22 @@ static int suspend_prepare(suspend_state
 	return error;
 }
 
+#ifndef arch_s2ram_disable_irqs
+#define arch_s2ram_disable_irqs(flags) local_irq_save(*flags)
+#endif
+
+#ifndef arch_s2ram_enable_irqs
+#define arch_s2ram_enable_irqs(flags) local_irq_restore(*flags)
+#endif
+
 
 int suspend_enter(suspend_state_t state)
 {
 	int error = 0;
 	unsigned long flags;
 
-	local_irq_save(flags);
+	arch_s2ram_disable_irqs(&flags);
+	BUG_ON(!irqs_disabled());
 
 	if ((error = device_power_down(PMSG_SUSPEND))) {
 		printk(KERN_ERR "Some devices failed to power down\n");
@@ -143,7 +152,8 @@ int suspend_enter(suspend_state_t state)
 	error = pm_ops->enter(state);
 	device_power_up();
  Done:
-	local_irq_restore(flags);
+	arch_s2ram_enable_irqs(&flags);
+	BUG_ON(irqs_disabled());
 	return error;
 }
 

Re: [PATCH] s2ram: add arch irq disable/enable hooks

[Pavel Machek]

Hi!

> For powermac, we need to do some things between suspending devices and
> device_power_off, for example setting the decrementer. This patch
> allows architectures to define arch_s2ram_{en,dis}able_irqs in their
> asm/suspend.h to have control over this step.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

Looks ok to me.



								Pavel

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