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

[Pavel Machek <pavel@ucw.cz>]

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