Re: Usage of restart_handler in pwrseq_emmc

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hello,

On 2015-06-03 17:03, Guenter Roeck wrote:
> On 06/03/2015 03:01 AM, Marek Szyprowski wrote:
>> Hello,
>>
>> On 2015-06-02 17:29, Heiko Stübner wrote:
>>> I'm confused by the pwrseq-emmc registering a restart_handler for 
>>> resetting an
>>> emmc in a panic-reboot case at priority 129 to "schedules it just 
>>> before
>>> system reboot".
>>>
>>> >From what I remember from the restart-handler discussion the 
>>> actuall usage is
>>> traversing the ordered list until one registered handler sucessfully 
>>> restarts
>>> the system and not to have arbitary actions in there not related to 
>>> the actual
>>> restart process?
>>>
>>> The actual documentation in kernel/reboot.c supports this assumption,
>>> describing register_restart_handler as "Register function to be 
>>> called to
>>> reset the system".
>>>
>>>
>>> Additionally, 128 isn't even _the_ priority to reboot the system as 
>>> described
>>> above and some drivers use higher priorities per default, see in
>>> drivers/power/reset arm-versatile-reboot.c; at91-reset.c; 
>>> rmobile-reset.c and
>>> some more.
>>>
>>>
>>> So I guess this should use some other mechanism (reboot notifier) 
>>> instead of
>>> restart_handlers?
>>
>> The first problem with reboot notifiers is that they are called too 
>> early - before
>> device_shutdown(), what interferes with the code in mmc_bus_shutdown 
>> and causes
>> lockup. The second problem is
>> that reboot notifiers are not called from emergency_restart() path. I 
>> agree that
>> 129 value for priority might not be the best, maybe according to 
>> documentation,
>> 255 value should be used to ensure that the handler will be called 
>> first before
>> any real restart handler.
>>
>
> There is no non-real restart handler, and the documentation does not 
> say anything
> about "called first before any real restart handler". Even with a 
> priority of 255
> you would have no guarantee that your handler is called. Restart 
> handlers are
> supposed to restart the system, nothing else. Actually, you have no 
> guarantee
> that the restart handler is called in the first place - not all 
> architectures
> support it (currently only arm, arm64, and mips do). Presumably mmc 
> support is
> not limited to those architectures.
>
>> If you have any idea how to avoid restart handler and ensure proper 
>> eMMC card
>> reboot sequence on any system reboot, I'm open for suggestions.
>>
>
> Why not execute the device-specific restart in the shutdown function ?
> You could register a reboot notifier to mark that a reboot is happening,
> and then execute the restart at the end of mmc_bus_shutdown.

Okay, this will solve one issue with reboot notifier, but there is still 
a problem
with emergency_restart(). Do you think that it will be okay to add a call to
restart_notifiers (for example with some higher priority) also for 
emergency case?
If so, I can rework my emmc pwr seq driver to use it and propose a patch for
emergency restart code.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland