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

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

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

[Andrew Morton]

On Thu, 19 Apr 2007 13:00:11 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:

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

what on earth is a decrementer?

> 
> ---
>  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);

hm, ugly.  We could use attribute(weak) here, so we don't force
architectures to implement this with a macro.  But whatever.


It might be nice to identify which arch header is supposed to define
arch_s2ram_disable_irqs, perhaps via an explicit inclusion of that header.

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

[Johannes Berg]

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

On Thu, 2007-04-19 at 14:55 -0700, Andrew Morton wrote:

> what on earth is a decrementer?

BenH explained it here:
http://thread.gmane.org/gmane.linux.power-management.general/5088/focus=5230

> It might be nice to identify which arch header is supposed to define
> arch_s2ram_disable_irqs, perhaps via an explicit inclusion of that header.

It's supposed to go into asm/suspend.h which is included from
linux/suspend.h. I thought about doing it explicitly but opted against
it in the end. But if I change it to be a weak symbol (somebody tell me
how to please) then that will be moot.

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] s2ram: add arch irq disable/enable hooks

[Greg KH]

On Thu, Apr 19, 2007 at 01:00:11PM +0200, 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
> 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>
> 
> ---
>  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

Can't you just make this a "weak" symbol and override it in your ppc
code if you really need it, and make a "real" function here in this file
for all other arches?

thanks,

greg k-h

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

[Johannes Berg]

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

On Fri, 2007-04-20 at 06:57 +0000, Greg KH wrote:

> Can't you just make this a "weak" symbol and override it in your ppc
> code if you really need it, and make a "real" function here in this file
> for all other arches?

I'm not opposed to that. How do I do 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] s2ram: add arch irq disable/enable hooks

[Greg KH]

On Fri, Apr 20, 2007 at 05:31:33PM +0200, Johannes Berg wrote:
> On Fri, 2007-04-20 at 06:57 +0000, Greg KH wrote:
> 
> > Can't you just make this a "weak" symbol and override it in your ppc
> > code if you really need it, and make a "real" function here in this file
> > for all other arches?
> 
> I'm not opposed to that. How do I do that?

Look at the other examples in the kernel, pcibios_disable_device is one
such example.

thanks,

greg k-h

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

[Johannes Berg]

Thanks. Does this look good?

Haven't tested it yet but I don't see a reason it shouldn't work for me.

Sparse will probably complain about this, should I define them
somewhere?

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

--- wireless-dev.orig/kernel/power/main.c	2007-04-20 21:34:01.531107385 +0200
+++ wireless-dev/kernel/power/main.c	2007-04-20 21:34:02.141107385 +0200
@@ -128,13 +128,35 @@ static int suspend_prepare(suspend_state
 	return error;
 }
 
+/**
+ * arch_s2ram_disable_irqs - disable IRQs for suspend
+ *
+ * Disables IRQs (in the default case) and allows architectures
+ * to override it if more needs to be done.
+ */
+void __attribute__ ((weak)) arch_s2ram_disable_irqs(unsigned long *flags)
+{
+	local_irq_save(*flags);
+}
+
+/**
+ * arch_s2ram_enable_irqs - enable IRQs after suspend
+ *
+ * Enables IRQs (in the default case) and allows architectures
+ * to override it if more needs to be done.
+ */
+void __attribute__ ((weak)) arch_s2ram_enable_irqs(unsigned long *flags)
+{
+	local_irq_restore(*flags);
+}
 
 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 +165,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

[Andrew Morton]

On Fri, 20 Apr 2007 21:37:18 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:

>  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 +165,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;
>  }

The second BUG_ON() looks a bit fishy to me.  If someone calls
suspend_enter() with local interrupts disabled, it will trigger.

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

[Johannes Berg]

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

On Fri, 2007-04-20 at 12:44 -0700, Andrew Morton wrote:

> > -	local_irq_restore(flags);
> > +	arch_s2ram_enable_irqs(&flags);
> > +	BUG_ON(irqs_disabled());
> >  	return error;
> >  }
> 
> The second BUG_ON() looks a bit fishy to me.  If someone calls
> suspend_enter() with local interrupts disabled, it will trigger.

Huh, hm, I don't think it's valid to do that. I'm not sure why the code
does irq_save/restore instead of just disable/enable. The only users of
this function are in the power management core code (main.c and user.c)
and they certainly always call it with interrupts enabled, the entry
point for other users is pm_suspend() and that surely cannot be called
with interrupts disabled. Rafael?

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] s2ram: add arch irq disable/enable hooks

[Rafael J. Wysocki]

On Friday, 20 April 2007 21:51, Johannes Berg wrote:
> On Fri, 2007-04-20 at 12:44 -0700, Andrew Morton wrote:
> 
> > > -	local_irq_restore(flags);
> > > +	arch_s2ram_enable_irqs(&flags);
> > > +	BUG_ON(irqs_disabled());
> > >  	return error;
> > >  }
> > 
> > The second BUG_ON() looks a bit fishy to me.  If someone calls
> > suspend_enter() with local interrupts disabled, it will trigger.
> 
> Huh, hm, I don't think it's valid to do that. I'm not sure why the code
> does irq_save/restore instead of just disable/enable.

Well, I can't say why exactly suspend_enter() usues irq_save/restore, but it
looks like that's not needed.  For the suspend to disk we call
local_irq_disable/enable() in the corresponding places and, for example,
acpi_pm_enter apparently uses irq_save/restore() itself too.

> The only users of this function are in the power management core code (main.c
> and user.c) and they certainly always call it with interrupts enabled, the
> entry point for other users is pm_suspend() and that surely cannot be called 
> with interrupts disabled. Rafael?

That's correct, plus pm_suspend() also uses enter_state().

In fact suspend_enter() has been made extern so that we can call it from
kernel/power/user.c and no one else is supposed to use it.

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

[Johannes Berg]

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

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

> Well, I can't say why exactly suspend_enter() usues irq_save/restore, but it
> looks like that's not needed.

powerpc works fine just with local_irq_disable/enable, never did
anything else and my new patches just ignore the *flags argument.

> For the suspend to disk we call
> local_irq_disable/enable() in the corresponding places and, for example,
> acpi_pm_enter apparently uses irq_save/restore() itself too.

Why would acpi_pm_enter do that? It can always assume irqs are disabled.
Or does it need to enable them for something (hey, that would mean Ben
is right in that it probably wants to use these new hooks...)

> > The only users of this function are in the power management core code (main.c
> > and user.c) and they certainly always call it with interrupts enabled, the
> > entry point for other users is pm_suspend() and that surely cannot be called 
> > with interrupts disabled. Rafael?
> 
> That's correct, plus pm_suspend() also uses enter_state().

Sure, but it can't be called with interrupts disabled.

> In fact suspend_enter() has been made extern so that we can call it from
> kernel/power/user.c and no one else is supposed to use it.

Right.

So, any argument against re-spinning this patch, removing the unsigned
long *flags stuff and using local_irq_disable/enable?

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] s2ram: add arch irq disable/enable hooks

[Rafael J. Wysocki]

On Friday, 20 April 2007 22:51, Johannes Berg wrote:
> On Fri, 2007-04-20 at 22:36 +0200, Rafael J. Wysocki wrote:
> 
> > Well, I can't say why exactly suspend_enter() usues irq_save/restore, but it
> > looks like that's not needed.
> 
> powerpc works fine just with local_irq_disable/enable, never did
> anything else and my new patches just ignore the *flags argument.
> 
> > For the suspend to disk we call
> > local_irq_disable/enable() in the corresponding places and, for example,
> > acpi_pm_enter apparently uses irq_save/restore() itself too.
> 
> Why would acpi_pm_enter do that? It can always assume irqs are disabled.
> Or does it need to enable them for something (hey, that would mean Ben
> is right in that it probably wants to use these new hooks...)
> 
> > > The only users of this function are in the power management core code (main.c
> > > and user.c) and they certainly always call it with interrupts enabled, the
> > > entry point for other users is pm_suspend() and that surely cannot be called 
> > > with interrupts disabled. Rafael?
> > 
> > That's correct, plus pm_suspend() also uses enter_state().
> 
> Sure, but it can't be called with interrupts disabled.
> 
> > In fact suspend_enter() has been made extern so that we can call it from
> > kernel/power/user.c and no one else is supposed to use it.
> 
> Right.
> 
> So, any argument against re-spinning this patch, removing the unsigned
> long *flags stuff and using local_irq_disable/enable?

Not from me.

Rafael

[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 have arch_s2ram_{en,dis}able_irqs functions to control
this step.

This patch also replaces local_irq_save/restore with
local_irq_disable/enable since the code must be called with interrupts
enabled.

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

---
This version uses weak symbols instead of macros.

 kernel/power/main.c |   28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

--- wireless-dev.orig/kernel/power/main.c	2007-04-20 23:35:20.911107385 +0200
+++ wireless-dev/kernel/power/main.c	2007-04-20 23:35:21.511107385 +0200
@@ -128,13 +128,34 @@ static int suspend_prepare(suspend_state
 	return error;
 }
 
+/**
+ * arch_s2ram_disable_irqs - disable IRQs for suspend
+ *
+ * Disables IRQs (in the default case) and allows architectures
+ * to override it if more needs to be done.
+ */
+void __attribute__ ((weak)) arch_s2ram_disable_irqs(void)
+{
+	local_irq_disable();
+}
+
+/**
+ * arch_s2ram_enable_irqs - enable IRQs after suspend
+ *
+ * Enables IRQs (in the default case) and allows architectures
+ * to override it if more needs to be done.
+ */
+void __attribute__ ((weak)) arch_s2ram_enable_irqs(void)
+{
+	local_irq_enable();
+}
 
 int suspend_enter(suspend_state_t state)
 {
 	int error = 0;
-	unsigned long flags;
 
-	local_irq_save(flags);
+	arch_s2ram_disable_irqs();
+	BUG_ON(!irqs_disabled());
 
 	if ((error = device_power_down(PMSG_SUSPEND))) {
 		printk(KERN_ERR "Some devices failed to power down\n");
@@ -143,7 +164,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();
+	BUG_ON(irqs_disabled());
 	return error;
 }
 

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

[Pavel Machek]

On Sat 2007-04-21 15:57:33, 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 allows
> architectures to have arch_s2ram_{en,dis}able_irqs functions to control
> this step.
> 
> This patch also replaces local_irq_save/restore with
> local_irq_disable/enable since the code must be called with interrupts
> enabled.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

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

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

[David Brownell]

On Saturday 21 April 2007, Johannes Berg wrote:

>  int suspend_enter(suspend_state_t state)
>  {
>  	int error = 0;
> -	unsigned long flags;
>  
> -	local_irq_save(flags);
> +	arch_s2ram_disable_irqs();

Sorry for not highlighting this before, but these are badly misnamed.
They apply for every system suspend state except PM_SUSPEND_DISK ...
NOT just suspend-to-ram, as specified by the function name.


> +	BUG_ON(!irqs_disabled());
>  
>  	if ((error = device_power_down(PMSG_SUSPEND))) {
>  		printk(KERN_ERR "Some devices failed to power down\n");
> @@ -143,7 +164,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();

Same here.

> +	BUG_ON(irqs_disabled());
>  	return error;
>  }
>  
> 

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

[Johannes Berg]

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

On Sat, 2007-04-21 at 08:41 -0700, David Brownell wrote:

> Sorry for not highlighting this before, but these are badly misnamed.
> They apply for every system suspend state except PM_SUSPEND_DISK ...
> NOT just suspend-to-ram, as specified by the function name.

Good point. arch_pm_disable_irqs?

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] s2ram: add arch irq disable/enable hooks

[David Brownell]

On Saturday 21 April 2007, Johannes Berg wrote:
> On Sat, 2007-04-21 at 08:41 -0700, David Brownell wrote:
> 
> > Sorry for not highlighting this before, but these are badly misnamed.
> > They apply for every system suspend state except PM_SUSPEND_DISK ...
> > NOT just suspend-to-ram, as specified by the function name.
> 
> Good point. arch_pm_disable_irqs?

Or maybe arch_suspend_disable_irqs() ... there are PM
operations that don't involve system suspend states.

- Dave

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

[Johannes Berg]

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

On Sat, 2007-04-21 at 09:55 -0700, David Brownell wrote:

> Or maybe arch_suspend_disable_irqs() ... there are PM
> operations that don't involve system suspend states.

Yeah, probably better and I'm fine with that, but the crowd who screamed
when I noted that CONFIG_SOFTWARE_SUSPEND had nothing to do with suspend
states might be confused... It should be obvious that this isn't called
for suspend to disk.

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] s2ram: add arch irq disable/enable hooks

[David Brownell]

On Saturday 21 April 2007, Johannes Berg wrote:
> On Sat, 2007-04-21 at 09:55 -0700, David Brownell wrote:
> 
> > Or maybe arch_suspend_disable_irqs() ... there are PM
> > operations that don't involve system suspend states.
> 
> Yeah, probably better and I'm fine with that, but the crowd who screamed
> when I noted that CONFIG_SOFTWARE_SUSPEND had nothing to do with suspend
> states might be confused... It should be obvious that this isn't called
> for suspend to disk.

Yeah, but SOFTWARE_SUSPEND is also a misnomer.  It's more of a
system checkpoint/restore framework.  And even userspace makes
that distinction clear, calling it "Hibernate" not "Suspend".

I feel a Kconfig patch coming on ... ;)

- Dave

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

[Johannes Berg]

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

On Sat, 2007-04-21 at 10:06 -0700, David Brownell wrote:

> And even userspace makes
> that distinction clear, calling it "Hibernate" not "Suspend".

But that was quite a fight, at least in the gnome camp ;)

> I feel a Kconfig patch coming on ... ;)

Not from me. But you'll get an updated one for this :)

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] s2ram: add arch irq disable/enable hooks

[Pavel Machek]

On Sat 2007-04-21 19:49:23, Johannes Berg wrote:
> On Sat, 2007-04-21 at 10:06 -0700, David Brownell wrote:
> 
> > And even userspace makes
> > that distinction clear, calling it "Hibernate" not "Suspend".
> 
> But that was quite a fight, at least in the gnome camp ;)

Yep. Maybe we'll want to change "suspend to ram -> suspend", "swsusp
-> hibernate" one day, but it will be big and ugly patch confusing
everyone, and I do not think that day is close.

> > I feel a Kconfig patch coming on ... ;)
> 
> Not from me. But you'll get an updated one for this :)

Please don't. Original patch is fine.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Johannes Berg]

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

On Sat, 2007-04-21 at 23:47 +0200, Pavel Machek wrote:

> Please don't. Original patch is fine.

I don't care any more, I just want the hooks. Go discuss with the others
and then send me an email which names you want.

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] s2ram: add arch irq disable/enable hooks

[Pavel Machek]

Hi!

> > > Or maybe arch_suspend_disable_irqs() ... there are PM
> > > operations that don't involve system suspend states.
> > 
> > Yeah, probably better and I'm fine with that, but the crowd who screamed
> > when I noted that CONFIG_SOFTWARE_SUSPEND had nothing to do with suspend
> > states might be confused... It should be obvious that this isn't called
> > for suspend to disk.
> 
> Yeah, but SOFTWARE_SUSPEND is also a misnomer.  It's more of a

Well, that's the way it is. "suspend" == s2ram, standby, swsusp,
swsusp+S4 in kernel context.

> system checkpoint/restore framework.  And even userspace makes
> that distinction clear, calling it "Hibernate" not "Suspend".

Gnome deciding to confuse their users is _their_ problem.

> I feel a Kconfig patch coming on ... ;)

I do not think confusion from a change is worth the benefit.

									Pavel

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

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

[David Brownell]

On Saturday 21 April 2007, Pavel Machek wrote:

> > system checkpoint/restore framework.  And even userspace makes
> > that distinction clear, calling it "Hibernate" not "Suspend".
> 
> Gnome deciding to confuse their users is _their_ problem.

Hot news flash for Pavel:  "hibernate" is much more widely used
and understood than "software suspend".  Everyone using Windows
has already been trained in what it means.  But even today lots
of folk scratch their heads at "software suspend" ... "gee, isn't
all suspending done by software?"


> > I feel a Kconfig patch coming on ... ;)
> 
> I do not think confusion from a change is worth the benefit.

Glad you waited to see the patch before you objected!

Oh wait -- you didn't wait.  Sorry, I seem to have
misunderstood the process here.  Could you explain
this new revised Linux patch process to me?

- Dave

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

[Pavel Machek]

Hi!

> > > system checkpoint/restore framework.  And even userspace makes
> > > that distinction clear, calling it "Hibernate" not "Suspend".
> > 
> > Gnome deciding to confuse their users is _their_ problem.
> 
> Hot news flash for Pavel:  "hibernate" is much more widely used
> and understood than "software suspend".  Everyone using Windows
> has already been trained in what it means.  But even today lots
> of folk scratch their heads at "software suspend" ... "gee, isn't
> all suspending done by software?"

Well, in apm case it is firmware, not software. That's how the name
was born.

> > > I feel a Kconfig patch coming on ... ;)
> > 
> > I do not think confusion from a change is worth the benefit.
> 
> Glad you waited to see the patch before you objected!

Uf I thought you were going to
s/CONFIG_SOFTWARE_SUSPEND/CONFIG/HIBERNATION/ over the kernel, and
wanted to spare you some time.

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

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

[David Woodhouse]

On Sun, 2007-04-22 at 13:09 +0000, Pavel Machek wrote:
> Uf I thought you were going to
> s/CONFIG_SOFTWARE_SUSPEND/CONFIG/HIBERNATION/ over the kernel, 

I think that's a really good idea. The small amount of churn in the
short term will be far outweighed by the benefit in the long term.

Just do it.

-- 
dwmw2

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

[Pavel Machek]

Hi!

> >  int suspend_enter(suspend_state_t state)
> >  {
> >  	int error = 0;
> > -	unsigned long flags;
> >  
> > -	local_irq_save(flags);
> > +	arch_s2ram_disable_irqs();
> 
> Sorry for not highlighting this before, but these are badly misnamed.
> They apply for every system suspend state except PM_SUSPEND_DISK ...
> NOT just suspend-to-ram, as specified by the function name.

Every suspend state means "standby" and "s2ram" in this context. I
think this name is better than alternatives.

								Pavel

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

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

[David Brownell]

On Saturday 21 April 2007, Pavel Machek wrote:
> Hi!
> 
> > >  int suspend_enter(suspend_state_t state)
> > >  {
> > >  	int error = 0;
> > > -	unsigned long flags;
> > >  
> > > -	local_irq_save(flags);
> > > +	arch_s2ram_disable_irqs();
> > 
> > Sorry for not highlighting this before, but these are badly misnamed.
> > They apply for every system suspend state except PM_SUSPEND_DISK ...
> > NOT just suspend-to-ram, as specified by the function name.
> 
> Every suspend state means "standby" and "s2ram" in this context. I
> think this name is better than alternatives.

Every suspend state is **NOT** s2ram though.

This is a very straightforward point.  A function that's
called on non-S2RAM paths shouldn't be named as if it's
only for S2RAM.

Ergo, those two functions are misnamed.  End of story.

- Dave

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

[Pavel Machek]

On Sat 2007-04-21 20:05:55, David Brownell wrote:
> On Saturday 21 April 2007, Pavel Machek wrote:
> > Hi!
> > 
> > > >  int suspend_enter(suspend_state_t state)
> > > >  {
> > > >  	int error = 0;
> > > > -	unsigned long flags;
> > > >  
> > > > -	local_irq_save(flags);
> > > > +	arch_s2ram_disable_irqs();
> > > 
> > > Sorry for not highlighting this before, but these are badly misnamed.
> > > They apply for every system suspend state except PM_SUSPEND_DISK ...
> > > NOT just suspend-to-ram, as specified by the function name.
> > 
> > Every suspend state means "standby" and "s2ram" in this context. I
> > think this name is better than alternatives.
> 
> Every suspend state is **NOT** s2ram though.
> 
> This is a very straightforward point.  A function that's
> called on non-S2RAM paths shouldn't be named as if it's
> only for S2RAM.

And the function that's called for s2ram+standby should not be named
as if it's also for swsusp...

> Ergo, those two functions are misnamed.  End of story.

...which is not only misnamed, it is also actively confusing. (And
someone _will_ ask me to call that function from swsusp, too.

...maybe we could solve it with a big fat comment?

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

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

[Johannes Berg]

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

On Sun, 2007-04-22 at 13:13 +0000, Pavel Machek wrote:

> > This is a very straightforward point.  A function that's
> > called on non-S2RAM paths shouldn't be named as if it's
> > only for S2RAM.
> 
> And the function that's called for s2ram+standby should not be named
> as if it's also for swsusp...
> 
> > Ergo, those two functions are misnamed.  End of story.
> 
> ...which is not only misnamed, it is also actively confusing. (And
> someone _will_ ask me to call that function from swsusp, too.
> 
> ...maybe we could solve it with a big fat comment?

So how about we don't call them s2ram which is confusing since s2ram is
only one suspend state we support but call them suspend as David
originally proposed and add a comment that since suspend-to-disk isn't a
true suspend state, they are not called there?

Has anybody figured out if (and if yes, where) we should add them to
some header file?

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] s2ram: add arch irq disable/enable hooks

[Pavel Machek]

Hi!

> > > This is a very straightforward point.  A function that's
> > > called on non-S2RAM paths shouldn't be named as if it's
> > > only for S2RAM.
> > 
> > And the function that's called for s2ram+standby should not be named
> > as if it's also for swsusp...
> > 
> > > Ergo, those two functions are misnamed.  End of story.
> > 
> > ...which is not only misnamed, it is also actively confusing. (And
> > someone _will_ ask me to call that function from swsusp, too.
> > 
> > ...maybe we could solve it with a big fat comment?
> 
> So how about we don't call them s2ram which is confusing since s2ram is
> only one suspend state we support but call them suspend as David
> originally proposed and add a comment that since suspend-to-disk isn't a
> true suspend state, they are not called there?

calling them ..._suspend_... and explaining in comment is probably ok.

> Has anybody figured out if (and if yes, where) we should add them to
> some header file?

include/linux/pm.h is probably the right place.

Feel free to add my acked-by: and send it to akpm.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[David Brownell]

On Tuesday 24 April 2007, Pavel Machek wrote:
> Hi!
> 
> > > > This is a very straightforward point.  A function that's
> > > > called on non-S2RAM paths shouldn't be named as if it's
> > > > only for S2RAM.
> > > 
> > > And the function that's called for s2ram+standby should not be named
> > > as if it's also for swsusp...
> > > 
> > > > Ergo, those two functions are misnamed.  End of story.
> > > 
> > > ...which is not only misnamed, it is also actively confusing. (And
> > > someone _will_ ask me to call that function from swsusp, too.
> > > 
> > > ...maybe we could solve it with a big fat comment?
> > 
> > So how about we don't call them s2ram which is confusing since s2ram is
> > only one suspend state we support but call them suspend as David
> > originally proposed and add a comment that since suspend-to-disk isn't a
> > true suspend state, they are not called there?
> 
> calling them ..._suspend_... and explaining in comment is probably ok.

Yah, I'm fine with that too.



> > Has anybody figured out if (and if yes, where) we should add them to
> > some header file?
> 
> include/linux/pm.h is probably the right place.
> 
> Feel free to add my acked-by: and send it to akpm.
> 									Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 

[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;
 }
 

[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