Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots

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

On 05/07/2014 04:52 AM, Lucas Stach wrote:
> Hi Guenter,
>
> Am Freitag, den 02.05.2014, 21:29 -0700 schrieb Guenter Roeck:
>> On Fri, May 02, 2014 at 06:22:43PM -0700, Maxime Ripard wrote:
>>> Hi Guenter,
>>>
>>> On Thu, May 01, 2014 at 08:41:29AM -0700, Guenter Roeck wrote:
>>>> Some hardware implements reboot through its watchdog hardware,
>>>> for example by triggering a watchdog timeout. Platform specific
>>>> code starts to spread into watchdog drivers, typically by setting
>>>> pointers to a callback functions which is then called from the
>>>> platform reset handler.
>>>>
>>>> To simplify code and provide a unified API to trigger reboots by
>>>> watchdog drivers, provide a single API to trigger such reboots
>>>> through the watchdog subsystem.
>>>>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>>   drivers/watchdog/watchdog_core.c |   17 +++++++++++++++++
>>>>   include/linux/watchdog.h         |   11 +++++++++++
>>>>   2 files changed, 28 insertions(+)
>>>>
>>>> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
>>>> index cec9b55..4ec6e2f 100644
>>>> --- a/drivers/watchdog/watchdog_core.c
>>>> +++ b/drivers/watchdog/watchdog_core.c
>>>> @@ -43,6 +43,17 @@
>>>>   static DEFINE_IDA(watchdog_ida);
>>>>   static struct class *watchdog_class;
>>>>
>>>> +static struct watchdog_device *wdd_reboot_dev;
>>>> +
>>>> +void watchdog_do_reboot(enum reboot_mode mode, const char *cmd)
>>>> +{
>>>> +	if (wdd_reboot_dev) {
>>>> +		if (wdd_reboot_dev->ops->reboot)
>>>> +			wdd_reboot_dev->ops->reboot(wdd_reboot_dev, mode, cmd);
>>>> +	}
>>>> +}
>>>> +EXPORT_SYMBOL(watchdog_do_reboot);
>>>> +
>>>>   static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
>>>>   {
>>>>   	/*
>>>> @@ -162,6 +173,9 @@ int watchdog_register_device(struct watchdog_device *wdd)
>>>>   		return ret;
>>>>   	}
>>>>
>>>> +	if (wdd->ops->reboot)
>>>> +		wdd_reboot_dev = wdd;
>>>> +
>>>
>>> Overall, it looks really great, but I guess we can make it a
>>> list. Otherwise, we might end up in a situation where we could not
>>> reboot anymore, like this one for example:
>>>    - a first watchdog is probed, registers a reboot function
>>>    - a second watchdog is probed, registers a reboot function that
>>>      overwrites the first one.
>>>    - then, the second watchdog disappears for some reason, and the
>>>      reboot is set to NULL
>>>
>> I thought about that, but how likely (or unlikely) is that to ever happen ?
>> So I figured it is not worth the effort, and would just add complexity without
>> real gain. We could always add the list later if we ever encounter a situation
>> where two watchdogs in the same system provide a reboot callback.
>>
>
> While this is not directly related to the issue you are fixing with this
> series, I would like to have it considered when talking about a watchdog
> system reboot API.
>
> On i.MX we have the same situation where we have to reboot through the
> SoC watchdog. This works, but may leave the external components of the
> system (those not integrated in the SoC) in an undefined state. So if we
> have a PMIC with integrated watchdog we would rather like to this one to
> reboot the system, as it the reset is then much more closer to a
> power-on-reset.
>
> This means we could have multiple watchdogs in the system, where we
> really want a specific one (maybe designated through a DT property) to
> do the reset. This isn't compatible with the "last watchdog that
> registers a handler wins the system reset" logic in your patch.
>

Wouldn't the order in which watchdogs are configured in dt define that ?
The last one wins.

Thanks,
Guenter