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:30 PM, Måns Rullgård wrote:
> Lars-Peter Clausen <lars@metafoo.de> writes:
>
>> 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.
>
> ARM does it its arch machine_restart() function.  MIPS should probably
> follow suit.
>

That's just cargo culting...

Either it is required, then it should probably be moved into the core 
kernel, or it is not required in which case it can be removed from the ARM 
machine_restart() implementation.