Subject: Re: [PATCH 3/7] Celleb: Support for Power/Reset buttons

From: Arnd Bergmann <[email-masked]>
Date: Sun, 30 Sep 2007 23:22:51 +0200

On Sunday 30 September 2007, you wrote:
> 
> > kernel_restart() seems appropriate for that.
> 
> Now a comment to this one says:
> 
>  *      This is not safe to call in interrupt context.
> 
> and AFAIU, this is called from an interrupt. Am I missing anything or 
> would this be wrong to call kernel_restart() here?

It should be called through a workqueue, afaics, do avoid calling
a sleeping notifier from an interrupt.

> > > +static irqreturn_t beat_reset_event(int virq, void *arg)
> > > +{
> > > +   printk(KERN_DEBUG "Beat: reset button pressed\n");
> > > +   beat_pm_poweroff_flag = 0;
> > > +   if (kill_cad_pid(SIGINT, 1)) {
> > > +           /* Just in case killing init process failed */
> > > +           beat_restart(NULL);
> > > +   }
> > > +   return IRQ_HANDLED;
> > > +}
> > 
> > same here, except calling kernel_halt() in the end.
> 
> kernel_halt() doesn't have a similar comment, but it does call the 
> kernel_shutdown_prepare() too, so, seems like it should not be called from 
> an interrupt either?

yes

> Now, another question, is it generally good to hard-wire "reset" and 
> "power-off" buttons in the kernel? Isn't it better to just register them 
> as input event sources and let userspace (power-management daemon) decide 
> what to do with them? Like poweroff / reboot / suspend / hibernate / ...

Tough question. I can see arguments for either way, but I guess I'd tend
to handling it in the kernel, choosing simplicity over consistency.

	Arnd <><