Re: [PATCH v3] rtc: omap: add support for pmic_power_en

[Johan Hovold]

On Tue, Oct 28, 2014 at 02:18:05PM -0700, Andrew Morton wrote:
> On Tue, 28 Oct 2014 09:36:33 +0100 Johan Hovold <johan@xxxxxxxxxx> wrote:
> 
> > > But it doesn't explain *why* we want the alarm to trigger before
> > > returning.
> > 
> > Should we really require every power-off handler to document arch
> > behaviour (even if its inconsistent and currently undocumented); in
> > this case that some arches return to user-space where we may oops if
> > called from process 0 (e.g. systemd, but not if using sysvinit)?
> 
> The kernel really doesn't have a problem related to excessive amounts
> of useful code comments :(
> 
> The bottom line is: did I provide a reader with the ability to
> understand why the code is this way?  If "no" then improvements beckon.
> 
> This does look like one code site where an elaborate explanation is
> warranted.  There's no way in which a reader can get to your above
> paragraph from the current rtc-omap.c.

I agree with you that I should add a comment on why that mdelay is
there to make it perfectly clear and obvious that it's purpose is to let
the alarm trigger, which in turn causes the pmic to power off the
system.

It is a power-off handler, and any power-off handler should not return
until it has at least attempted to power off the system. In this case,
that mandates a two-second delay.

So far, so good. I don't agree with you that every power-off handler
should document what happens if it fails to power-off the system. That
is, to describe what arches will happily return to user space, and under
what conditions (e.g. what init system is used) that this will cause a
panic.

If anything that belongs in arch code or kernel/reboot.c, and is
also something that is likely to change over time (consider the
power-off-handler call chains).

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/