[PATCH 00/11] RFC: Common machine reset handling

[PATCH 00/11] RFC: Common machine reset handling

[Domenico Andreoli]

Hi,

  I've been looking for a solution to my bcm4760 watchdog based restart
hook when I noticed that the kernel reboot/shutdown mechanism is having
a few unaddressed issues.

Those I identified are:

 1) context pointer often needed by the reset hook
    (currently local static data is used for this pourpose)
 2) unclear ownership/policy in case of multiple reset hooks
    (currently almost nobody seems to care much)
 
I put together this patchset to explore some possible interface
and improve the status, essentially moving away from the
function-pointer-assignament kind of reset hook registration.

It is meant to be platform agnostic and sits in kernel/machine_reset.c. It
opts in only when needed and when supported.

A common usage, as reset hook provider (drivers, boardfiles):

	void restart_hook(void *dev, enum reboot_mode mode, const char *cmd)
{
	/* reboot the board */
}

void release_hook(void *dev)
{
	/* release the hook */
}

void init_restart_hook(void *dev)
{
	struct reset_hook hook;
	reset_hook_init(&hook);
	hook.restart = restart_hook;
	hook.release = release_hook; /* optional */
	set_machine_reset(RESET_RESTART, &hook, dev);
}


A common usage, as reset hook consumer (arch specific hooks, kernel):

	void machine_restart(char *command)
{
	if (_machine_restart)
		_machine_restart(command);
	else
		default_restart(reboot_mode, command);
}

void machine_halt(void)
{
	if (_machine_halt)
		_machine_halt();
	else
		default_halt();
}

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


I don't see these impressive diffstats but at least it's possible to free
some static data along the way. Maybe there are some other desiderata that
could be satisfied?

Comments are welcome.

Thanks,
Domenico

[PATCH 01/11] machine-reset: platform generic handling

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: machine-reset-handling.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131031/2fbc9da9/attachment.ksh>

[PATCH 02/11] ARM: use the common machine reset handling

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: arm-machine-reset.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131031/11f22cfc/attachment.ksh>

[PATCH 03/11] ARM64: use the common machine reset handling

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: arm64-machine-reset.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131031/0011622b/attachment.ksh>

[PATCH 04/11] MIPS: use the common machine reset handling

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: mips-machine-reset.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131031/c88e68b5/attachment.ksh>

[PATCH 05/11] ARM: vexpress: consolidate machine reset func

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: arm-machine-reset-vexpress-enum.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131031/786483ca/attachment.ksh>

[PATCH 06/11] ARM: vexpress: use the common machine reset handling

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: arm-machine-reset-vexpress-hooks.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131031/d00492e7/attachment.ksh>

[PATCH 07/11] ARM: bcm2835: use the common machine reset handling

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: arm-machine-reset-bcm2835.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131031/0b56201b/attachment.ksh>

[PATCH 08/11] ARM: u300: use the common machine reset handling

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: arm-machine-reset-u300.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131031/48d951b1/attachment.ksh>

[PATCH 09/11] ARM: tps65910: use the common machine reset handling

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: tps65910-machine-reset.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131031/4c73965d/attachment.ksh>

[PATCH 10/11] max8907: use the common machine reset handling

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: max8907-machine-reset.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131031/381822e0/attachment.ksh>

[PATCH 11/11] ARM: sp805: use the common machine reset handling

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: sp805-machine-reset.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131031/b02aec4d/attachment.ksh>

[PATCH 00/11] RFC: Common machine reset handling

[Russell King - ARM Linux]

On Thu, Oct 31, 2013 at 07:27:08AM +0100, Domenico Andreoli wrote:
> Hi,
> 
>   I've been looking for a solution to my bcm4760 watchdog based restart
> hook when I noticed that the kernel reboot/shutdown mechanism is having
> a few unaddressed issues.
> 
> Those I identified are:
> 
>  1) context pointer often needed by the reset hook
>     (currently local static data is used for this pourpose)

I don't think that's a problem at all.

> I don't see these impressive diffstats but at least it's possible to free
> some static data along the way. Maybe there are some other desiderata that
> could be satisfied?

If that's the only benefit, then I don't think this is worth the churn,
sorry.

[PATCH 08/11] ARM: u300: use the common machine reset handling

[Linus Walleij]

On Wed, Oct 30, 2013 at 11:27 PM, Domenico Andreoli
<domenico.andreoli@linux.com> wrote:

> From: Domenico Andreoli <domenico.andreoli@linux.com>
>
> Proof of concept: u300 as provider of reset hooks.

Do you have a u300 to test this on or are you just guessing?

Anyway, concept looks correct, so Acked-by given that the
overall idea is accepted.

Yours,
Linus Walleij

[PATCH 08/11] ARM: u300: use the common machine reset handling

[Domenico Andreoli]

On Thu, Oct 31, 2013 at 07:40:25AM -0700, Linus Walleij wrote:
> On Wed, Oct 30, 2013 at 11:27 PM, Domenico Andreoli
> <domenico.andreoli@linux.com> wrote:
> 
> > From: Domenico Andreoli <domenico.andreoli@linux.com>
> >
> > Proof of concept: u300 as provider of reset hooks.
> 
> Do you have a u300 to test this on or are you just guessing?

I don't have any u300, I picked it randomly. It's just to show some usage
and possibly discuss it. There are quite a lot of these hooks all around.

> Anyway, concept looks correct, so Acked-by given that the
> overall idea is accepted.

uhm.. don't see any general acceptance right now. maybe later..

thanks,
Domenico

[PATCH 00/11] RFC: Common machine reset handling

[Stephen Warren]

On 10/31/2013 12:27 AM, Domenico Andreoli wrote:
> Hi,
> 
>   I've been looking for a solution to my bcm4760 watchdog based restart
> hook when I noticed that the kernel reboot/shutdown mechanism is having
> a few unaddressed issues.
> 
> Those I identified are:
> 
>  1) context pointer often needed by the reset hook
>     (currently local static data is used for this pourpose)
>  2) unclear ownership/policy in case of multiple reset hooks
>     (currently almost nobody seems to care much)

I'm not sure how this patchset solves (2); even with the new API, it's
still the case that whichever code calls set_machine_reset() last wins,
just like before where whichever code wrote to pm_power_off won. I'm not
sure what this series attempts to solve.

[PATCH 04/11] MIPS: use the common machine reset handling

[Vineet Gupta]

On 10/31/2013 11:57 AM, Domenico Andreoli wrote:
> From: Domenico Andreoli <domenico.andreoli@linux.com>
> 
> Proof of concept: MIPS as a consumer of the machine reset hooks.
> 
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: linux-arch at vger.kernel.org
> Cc: linux-mips at vger.kernel.org
> Signed-off-by: Domenico Andreoli <domenico.andreoli@linux.com>
> ---
>  arch/mips/kernel/reset.c |    7 +++++++
>  kernel/power/Kconfig     |    2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> Index: b/arch/mips/kernel/reset.c
> ===================================================================
> --- a/arch/mips/kernel/reset.c
> +++ b/arch/mips/kernel/reset.c
> @@ -11,6 +11,7 @@
>  #include <linux/pm.h>
>  #include <linux/types.h>
>  #include <linux/reboot.h>
> +#include <linux/machine_reset.h>
>  
>  #include <asm/reboot.h>
>  
> @@ -29,16 +30,22 @@ void machine_restart(char *command)
>  {
>  	if (_machine_restart)
>  		_machine_restart(command);
> +	else
> +		default_restart(reboot_mode, command);
>  }
>  
>  void machine_halt(void)
>  {
>  	if (_machine_halt)
>  		_machine_halt();
> +	else
> +		default_halt();
>  }
>  
>  void machine_power_off(void)
>  {
>  	if (pm_power_off)
>  		pm_power_off();
> +	else
> +		default_power_off();
>  }
> Index: b/kernel/power/Kconfig
> ===================================================================
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -297,4 +297,4 @@ config CPU_PM
>  config MACHINE_RESET
>  	bool
>  	default n
> -	depends on ARM || ARM64
> +	depends on ARM || ARM64 || MIPS

This particular idiom is frowned upon for new code. As new arches get added this
list keeps getting bigger and then those who don't need the feature need to add
the anti dependency. Also in this particular case the dependency is trivial so you
can just "select" it in arch/*/Kconfig

[PATCH 00/11] RFC: Common machine reset handling

[Domenico Andreoli]

On Thu, Oct 31, 2013 at 03:49:18PM -0600, Stephen Warren wrote:
> On 10/31/2013 12:27 AM, Domenico Andreoli wrote:
> > Hi,
> > 
> >   I've been looking for a solution to my bcm4760 watchdog based restart
> > hook when I noticed that the kernel reboot/shutdown mechanism is having
> > a few unaddressed issues.
> > 
> > Those I identified are:
> > 
> >  1) context pointer often needed by the reset hook
> >     (currently local static data is used for this pourpose)
> >  2) unclear ownership/policy in case of multiple reset hooks
> >     (currently almost nobody seems to care much)
> 
> I'm not sure how this patchset solves (2); even with the new API, it's
> still the case that whichever code calls set_machine_reset() last wins,
> just like before where whichever code wrote to pm_power_off won. I'm not
> sure what this series attempts to solve.

That's right, the last wins. But the previous has a chance to know.

I only supposed there is somebody in charge of selecting the best handler
for the machine. Don't know how fancy this decision is but at least for
the vexpress there is also a sysfs way to configure different reset methods
from user-space.

So cleaning up things after the handler is replaced seemed a sensible
thing to do.

Another "problem" this patch would solve is the registration of the
reset handler in a architecture independent way. Now an otherwise platform
generic gpio HW reset driver would need to do different things on different
architectures.

thanks,
Domenico

[PATCH 04/11] MIPS: use the common machine reset handling

[Domenico Andreoli]

On Fri, Nov 01, 2013 at 10:41:54AM +0530, Vineet Gupta wrote:
> On 10/31/2013 11:57 AM, Domenico Andreoli wrote:
> > From: Domenico Andreoli <domenico.andreoli@linux.com>
> > 
> > Proof of concept: MIPS as a consumer of the machine reset hooks.
> > 
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: linux-arch at vger.kernel.org
> > Cc: linux-mips at vger.kernel.org
> > Signed-off-by: Domenico Andreoli <domenico.andreoli@linux.com>
> > ---
> >  arch/mips/kernel/reset.c |    7 +++++++
> >  kernel/power/Kconfig     |    2 +-
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> > 
> > Index: b/arch/mips/kernel/reset.c
> > ===================================================================
> > --- a/arch/mips/kernel/reset.c
> > +++ b/arch/mips/kernel/reset.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/pm.h>
> >  #include <linux/types.h>
> >  #include <linux/reboot.h>
> > +#include <linux/machine_reset.h>
> >  
> >  #include <asm/reboot.h>
> >  
> > @@ -29,16 +30,22 @@ void machine_restart(char *command)
> >  {
> >  	if (_machine_restart)
> >  		_machine_restart(command);
> > +	else
> > +		default_restart(reboot_mode, command);
> >  }
> >  
> >  void machine_halt(void)
> >  {
> >  	if (_machine_halt)
> >  		_machine_halt();
> > +	else
> > +		default_halt();
> >  }
> >  
> >  void machine_power_off(void)
> >  {
> >  	if (pm_power_off)
> >  		pm_power_off();
> > +	else
> > +		default_power_off();
> >  }
> > Index: b/kernel/power/Kconfig
> > ===================================================================
> > --- a/kernel/power/Kconfig
> > +++ b/kernel/power/Kconfig
> > @@ -297,4 +297,4 @@ config CPU_PM
> >  config MACHINE_RESET
> >  	bool
> >  	default n
> > -	depends on ARM || ARM64
> > +	depends on ARM || ARM64 || MIPS
> 
> This particular idiom is frowned upon for new code. As new arches get added this
> list keeps getting bigger and then those who don't need the feature need to add
> the anti dependency. Also in this particular case the dependency is trivial so you
> can just "select" it in arch/*/Kconfig

this dependency guarantees only that the option is available only on the
supported arches.

anyway I've not a strong opinion, I only picked the least invasive solution
I was able to think.

thanks,
Domenico

[PATCH 00/11] RFC: Common machine reset handling

[Stephen Warren]

On 10/31/2013 11:16 PM, Domenico Andreoli wrote:
> On Thu, Oct 31, 2013 at 03:49:18PM -0600, Stephen Warren wrote:
>> On 10/31/2013 12:27 AM, Domenico Andreoli wrote:
>>> Hi,
>>>
>>>   I've been looking for a solution to my bcm4760 watchdog based restart
>>> hook when I noticed that the kernel reboot/shutdown mechanism is having
>>> a few unaddressed issues.
>>>
>>> Those I identified are:
>>>
>>>  1) context pointer often needed by the reset hook
>>>     (currently local static data is used for this pourpose)
>>>  2) unclear ownership/policy in case of multiple reset hooks
>>>     (currently almost nobody seems to care much)
>>
>> I'm not sure how this patchset solves (2); even with the new API, it's
>> still the case that whichever code calls set_machine_reset() last wins,
>> just like before where whichever code wrote to pm_power_off won. I'm not
>> sure what this series attempts to solve.
> 
> That's right, the last wins. But the previous has a chance to know.
> 
> I only supposed there is somebody in charge of selecting the best handler
> for the machine. Don't know how fancy this decision is but at least for
> the vexpress there is also a sysfs way to configure different reset methods
> from user-space.

For PMICs that provide power off, we've been adding a property to DT to
indicate whether the PMIC is *the* system power off controller or not.
If the property is present, the PMIC registers itself in the poweroff
hook. If not, it doesn't. So, there really isn't an algorithm for
selecting the power off mechanism, but rather we designate one mechanism
ahead of time, and that's the only one that's relevant. We could
probably do the same for reset mechanisms.

I guess the vexpress situation is actually the same; there's a single
concept of a custom vexpress reset, it's just that sysfs is used to
select exactly what that does?

> So cleaning up things after the handler is replaced seemed a sensible
> thing to do.

Can't we avoid replacing handlers, but only registering a single handler?

> Another "problem" this patch would solve is the registration of the
> reset handler in a architecture independent way. Now an otherwise platform
> generic gpio HW reset driver would need to do different things on different
> architectures.

OK, if there are architecture differences in how the hooks are
registered, that seems like a good thing to fix.

[PATCH 00/11] RFC: Common machine reset handling

[Russell King - ARM Linux]

On Fri, Nov 01, 2013 at 09:58:49AM -0600, Stephen Warren wrote:
> For PMICs that provide power off, we've been adding a property to DT to
> indicate whether the PMIC is *the* system power off controller or not.
> If the property is present, the PMIC registers itself in the poweroff
> hook. If not, it doesn't. So, there really isn't an algorithm for
> selecting the power off mechanism, but rather we designate one mechanism
> ahead of time, and that's the only one that's relevant. We could
> probably do the same for reset mechanisms.
> 
> I guess the vexpress situation is actually the same; there's a single
> concept of a custom vexpress reset, it's just that sysfs is used to
> select exactly what that does?

I'm not aware of that.  Vexpress has the following mechanisms:

- reset - this causes the system to be restarted without powering off.
- restart - this causes the system to be powered off and back on.
- poweroff - this causes the system to power off.

Obviously, poweroff is what needs to happen when someone issues the
poweroff command (or, when we get hibernate support, the power off
hook will also be called to power the system off after saving all
system state.)  So, a power off callback really better power the
system off and not reboot it.

reset vs restart is a choice, and one of those should happen as a result
of the reboot command, or other similar event which ends up requesting
a system restart.  That may be configurable.

Ultimately though, this should have no bearing on the hooking of poweroff
and restart callbacks; the only difference there is on Vexpress is the
function code passed to the system controller.