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

Re: [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

Re: [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.

Re: [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.

Re: [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@vger.kernel.org
> > Cc: linux-mips@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