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

["Rafael J. Wysocki" <rjw@sisk.pl>]

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.