Usage of restart_handler in pwrseq_emmc

Usage of restart_handler in pwrseq_emmc

[Heiko Stübner]

Hi,

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?


Heiko

Re: Usage of restart_handler in pwrseq_emmc

[Guenter Roeck]

On 06/02/2015 08:29 AM, Heiko Stübner wrote:
> Hi,
>
> 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?
>

Looks like it. Assuming that a restart handler with priority 129 will always
be executed is a bad idea, and having it do anything but restart the system
is an even worse idea and is really asking for trouble.

Guenter

Re: Usage of restart_handler in pwrseq_emmc

[Marek Szyprowski]

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.

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.

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

Re: Usage of restart_handler in pwrseq_emmc

[Guenter Roeck]

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.

Guenter

Re: Usage of restart_handler in pwrseq_emmc

[Marek Szyprowski]

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

Re: Usage of restart_handler in pwrseq_emmc

[Guenter Roeck]

On 06/08/2015 02:36 AM, Marek Szyprowski wrote:
> 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 functi
>> 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.
>
I don't think so. The restart handlers are called from machine_restart(),
and it is up to the architecture maintainers to decide if they want to use it
or not. Besides, we'd need to extend the restart handler API to permit this use
case.

The comment with emergency_restart() specifically states "without shutting down
any hardware", so it may be difficult to add anything there.

I would suggest to ask the power maintainers for advice; we can discuss lots of
things, but ultimately they will be the ones who need to agree.

Thanks,
Guenter