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

[Guenter Roeck <linux@roeck-us.net>]

On 01/11/2015 04:25 AM, Lars-Peter Clausen wrote:
> 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.
>
Agreed.

> 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.
>
That should probably be added to do_kernel_restart().

Guenter