Date:	Wed, 16 Apr 2014 21:41:19 +0100
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Sebastian Capella <sebastian.capella@...aro.org>
Cc:	Pavel Machek <pavel@....cz>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"linaro-kernel@...ts.linaro.org" <linaro-kernel@...ts.linaro.org>,
	Len Brown <len.brown@...el.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>
Subject: Re: [PATCH RFC] PM / Hibernate: no kernel_power_off when
	pm_power_off NULL

On Wed, Apr 16, 2014 at 09:28:28AM -0700, Sebastian Capella wrote:
> On 15 April 2014 14:18, Pavel Machek <pavel@....cz> wrote:
> > On Tue 2014-04-15 21:54:53, Russell King - ARM Linux wrote:
> >> What I'm basically saying is that I see no reason for ARM to do something
> >> different to what x86 does.
> >>
> >> What is pretty clear to me is that ARM is compatible with x86, which is
> >> compatible with kernel/reboot.c, and it's the hibernate code which is
> >> the odd one out.
> >
> > I'm pretty sure the original code did not return. Anyway, the best
> > solution, given how many platforms are out there, would be to
> >
> > a) document that it should not return
> >
> > b) fix hibernation to handle the returning case, anyway.
> 
> Thanks Russell and Pavel!
> 
> This sounds fine to me.  Any objections?

Here is the i386 code from 2.2.20:

void machine_halt(void)
{
}

void machine_power_off(void)
{
        if (acpi_power_off)
                acpi_power_off();
}

Both can return.  On the other hand, machine_restart() can never return as
the final attempt to perform that action in machine_real_restart is a jump
to 16-bit code.

2.4 kernels then modified it to this:

void machine_halt(void)
{
}

void machine_power_off(void)
{
        if (pm_power_off)
                pm_power_off();
}

with machine_restart() doing similar to v2.2.

2.6.0 also did the same as 2.4 kernels.  2.6.16 then changed to this:

void machine_restart(char * __unused)
{
        machine_shutdown();
        machine_emergency_restart();
}

void machine_halt(void)
{
}

void machine_power_off(void)
{
        if (pm_power_off) {
                machine_shutdown();
                pm_power_off();
        }
}

which starts adding some extra stuff into the power off hook - but
again, it's a no-op unless the pm_power_off function pointer is
initialised.

Today, it's:

void machine_power_off(void)
{
        machine_ops.power_off();
}

which calls native_power_off():

static void native_machine_power_off(void)
{
        if (pm_power_off) {
                if (!reboot_force)
                        machine_shutdown();
                pm_power_off();
        }
        /* A fallback in case there is no PM info available */
        tboot_shutdown(TB_SHUTDOWN_HALT);
}

and tboot_shutdown():

void tboot_shutdown(u32 shutdown_type)
{
        void (*shutdown)(void);

        if (!tboot_enabled())
                return;

so it's entirely possible today for machine_power_off() on x86 to return.

So... the i386 code has had this "machine_power_off() can return"
behaviour for a significant number of years and persists to this day.

I'd say scrap (a) _unless_ we're going to add while (1) loops to all
the architectures.  Alternatively, we could just accept that
machine_power_off() may return and deal with that case (iow, not
crash) in generic code.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists