Re: [RFC PATCH 0/6] kernel: Add support for restart notifier call chain

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

On 07/01/2014 04:24 AM, Catalin Marinas wrote:
> On Mon, Jun 30, 2014 at 09:28:51PM +0100, Guenter Roeck wrote:
>> On Mon, Jun 30, 2014 at 08:59:47PM +0100, Russell King - ARM Linux wrote:
>>> On Mon, Jun 30, 2014 at 12:11:33PM -0700, Guenter Roeck wrote:
>>>> Various drivers implement architecture and/or device specific means
>>>> to restart (reset) the system. Various mechanisms have been implemented
>>>> to support those schemes. The best known mechanism is arm_pm_restart,
>>>> which is a function pointer to be set either from platform specific code
>>>> or from drivers. Another mechanism is to use hardware watchdogs to issue
>>>> a reset; this mechanism is used if there is no other method available
>>>> to reset a board or system. Two examples are alim7101_wdt, which currently
>>>> uses the reboot notifier to trigger a reset, and moxart_wdt, which registers
>>>> the arm_pm_restart function.
>>>>
>>>> The existing mechanisms have a number of drawbacks. Typically only one scheme
>>>> to restart the system is supported (at least if arm_pm_restart is used).
>>>> At least in theory there can be mutliple means to restart the system, some of
>>>> which may be less desirable (for example one mechanism may only reset the CPU,
>>>> while another may reset the entire system). Using arm_pm_restart can also be
>>>> racy if the function pointer is set from a driver, as the driver may be in
>>>> the process of being unloaded when arm_pm_restart is called.
>>>> Using the reboot notifier is always racy, as it is unknown if and when
>>>> other functions using the reboot notifier have completed execution
>>>> by the time the watchdog fires.
>>>>
>>>> To solve the problem, introduce a system restart notifier. This notifier
>>>> is expected to be called from the architecture specific machine_restart()
>>>> function. Drivers providing system restart functionality (such as the watchdog
>>>> drivers mentioned above) are expected to register with this notifier.
>>>>
>>>> Patch 1 of this series implements the notifier function. Patches 2 and 3
>>>> implement calling the notifier chain from arm and arm64 restart code.
>>>> Patch 4 and 5 convert existing restart handlers in the watchdog subsystem
>>>> to use the restart notifier. Patch 6 unexports arm_pm_restart to ensure
>>>> that no one gets the idea to implement a restart handler as module.
>>>
>>> I think you need to restructure stuff somewhat, because I think
>>> you've missed drivers/power/reset/ entirely, or at least you've
>>> missed drivers/power/reset/restart-poweroff.c which calls
>>> arm_pm_restart directly.  I'm not quite sure how we ended up with
>>> that...
>>
>> Yes, guess I missed (and did not really expect) that arm_pm_restart
>> is called from multiple places.
>
> Most of the ARM-specific code in drivers/power/reset/ consists of SoC
> power-off/restart back-ends (e.g. vexpress-poweroff.c). Since there is
> no generic pm_restart, we continued to use arm_pm_restart (also for
> arm64 since we share some of the drivers). Maybe some driver model here
> would help.
>
>> What is restart-poweroff supposed to do in the first place, and why
>> doesn't it call machine_restart() ? If it is what I think it is, ie
>> a fallback for pm_power_off, it could be made generic and does not
>> really have to depend on ARM.
>
> I think this one pretends to do a power-off via restart. It could call
> machine_restart() but this only passes the default reboot_mode to
> arm_pm_restart().
>

Unless one sets reboot_mode first. See my proposed patch in
https://lkml.org/lkml/2014/6/30/792.

Guenter