Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler

[Lars-Peter Clausen <lars@metafoo.de>]

On 01/11/2015 01:15 PM, Måns Rullgård wrote:
> Guenter Roeck <linux@roeck-us.net> writes:
>
>> On 01/10/2015 12:08 PM, Måns Rullgård wrote:
>>> Lars-Peter Clausen <lars@metafoo.de> writes:
>>>
>>>> Use the recently introduced do_kernel_restart() function as the default restart
>>>> handler if the platform did not explicitly provide a restart handler. This
>>>> allows use restart handler that have been registered by device drivers to
>>>> restart the machine.
>>>>
>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>> ---
>>>>    arch/mips/kernel/reset.c |    2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/mips/kernel/reset.c b/arch/mips/kernel/reset.c
>>>> index 07fc524..36cd80c 100644
>>>> --- a/arch/mips/kernel/reset.c
>>>> +++ b/arch/mips/kernel/reset.c
>>>> @@ -19,7 +19,7 @@
>>>>     * So handle all using function pointers to machine specific
>>>>     * functions.
>>>>     */
>>>> -void (*_machine_restart)(char *command);
>>>> +void (*_machine_restart)(char *command) = do_kernel_restart;
>>>>    void (*_machine_halt)(void);
>>>>    void (*pm_power_off)(void);
>>>
>>> There is already a similar patch posted by Kevin Cernekee:
>>> http://www.linux-mips.org/archives/linux-mips/2014-12/msg00410.html
>>>
>> Personally I prefer the earlier patch, though I guess that is personal
>> preference.
>
> They both achieve the same thing, though Kevin's is more in line with
> what ARM does.  Missing from both is a fallback while(1) loop in case no
> restart handlers are registered.  With the restart moved to the watchdog
> driver, there's a possibility that this might happen.
>

In my opinion if such a fallback is needed it should be put into the kernel 
core reboot implementation and not into individual restart handler 
implementations.

My first version of this patch was do_kernel_restart() followed by a 
machine_halt() (so it goes to sleep instead of busy looping) as a fallback. 
But I couldn't find a good reason why that should be done at the individual 
restart handler level, so I dropped it.

- Lars