Re: [PATCH 5/7] watchdog: gpio_wdt: use core reboot notifier

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

On 11/20/2015 07:54 AM, Damien Riegel wrote:
> On Fri, Nov 20, 2015 at 07:35:47AM -0800, Guenter Roeck wrote:
>> On 11/20/2015 07:21 AM, Damien Riegel wrote:
>>> On Fri, Nov 20, 2015 at 08:15:33AM +0100, Mike Looijmans wrote:
>>>> On 20-11-15 04:27, Guenter Roeck wrote:
>>>>> On 11/18/2015 01:02 PM, Damien Riegel wrote:
>>>>>> Get rid of the custom reboot notifier block registration and use
>>>>>> the one provided by the watchdog core.
>>>>>>
>>>>>> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
>>>>>> Reviewed-by: Vivien Didelot <vivien.didelot@savoirlinux.com> ---
>>>>>> drivers/watchdog/gpio_wdt.c | 35
>>>>>> ++--------------------------------- 1 file changed, 2
>>>>>> insertions(+), 33 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/watchdog/gpio_wdt.c
>>>>>> b/drivers/watchdog/gpio_wdt.c index 1a3c6e8..035c238 100644 ---
>>>>>> a/drivers/watchdog/gpio_wdt.c +++ b/drivers/watchdog/gpio_wdt.c @@
>>>>>> -12,10 +12,8 @@ #include <linux/err.h> #include <linux/delay.h>
>>>>>> #include <linux/module.h> -#include <linux/notifier.h> #include
>>>>>> <linux/of_gpio.h> #include <linux/platform_device.h> -#include
>>>>>> <linux/reboot.h> #include <linux/watchdog.h>
>>>>>>
>>>>>>   #define SOFT_TIMEOUT_MIN    1
>>>>>> @@ -36,7 +34,6 @@ struct gpio_wdt_priv { unsigned int hw_algo;
>>>>>> unsigned int        hw_margin; unsigned long last_jiffies; -
>>>>>> struct notifier_block    notifier; struct timer_list    timer;
>>>>>> struct watchdog_device    wdd; }; @@ -126,26 +123,6 @@ static int
>>>>>> gpio_wdt_set_timeout(struct watchdog_device *wdd, unsigned int t)
>>>>>> return gpio_wdt_ping(wdd); }
>>>>>>
>>>>>> -static int gpio_wdt_notify_sys(struct notifier_block *nb,
>>>>>> unsigned long code, -                   void *unused) -{ - struct
>>>>>> gpio_wdt_priv *priv = container_of(nb, struct gpio_wdt_priv, -
>>>>>> notifier); - - mod_timer(&priv->timer, 0); - -    switch (code) {
>>>>>> -    case SYS_HALT: -    case SYS_DOWN: - gpio_wdt_disable(priv);
>>>>>> - break; -    default: -        break; - } -
>>>>> Slight difference in semantics here: The stop function only stops
>>>>> the watchdog if the 'always_running' flag is not set. The notifier
>>>>> here always stops it, or at least tries to stop it. Not really sure
>>>>> what that means, since the always_running flag is supposed to mean
>>>>> "the watchdog can not be stopped".
>>>>>
>>>>> Copying Mike Looijmans, who added the always-running flag, for
>>>>> input.
>>>>
>>>> Okay, I don't know quite what the "core" will do.
>>>>
>>>> If the system wants to reboot, and the watchdog is in
>>>> "always_running" mode, it must NOT stop the watchdog. You have to
>>>> keep kicking the dog until the system reboots and the bootloader can
>>>> take over. Otherwise, the watchdog may turn off the mains power (I
>>>> developed this for a medical device, which had to completely shut
>>>> down in case of trouble) or do something else that wasn't supposed
>>>> to happen yet.
>>>>
>>>> When shutting down, the assumption is that either the power-down has
>>>> already occured before the watchdog might kick in, or that the
>>>> watchdog will shut down the system because the kernel stopped
>>>> kicking it. So no special case here, it doesn't really matter
>>>> whether you kick it once more or not.
>>>>
>>>> If the external watchdog reboots the system rather than shuts down
>>>> power, the above scenario won't hurt.
>>>>
>>>> Hope this answers your question, if not, feel free to ask for more
>>>> information.
>>>>
>>>
>>> The core calls ops->stop on SYS_HALT and SYS_DOWN if the watchdog
>>> called watchdog_stop_on_reboot during initialization.
>>>
>>> In the previous patch of this serie, I change the condition on which
>>> gpio_wdt_disable is called in gpio_wdt_notify_sys, replacing
>>> SYS_POWER_OFF by SYS_DOWN. Regarding what you just said, not stopping
>>> on SYS_DOWN was a deliberate choice, so we should not modify this
>>> watchdog's behaviour.
>>>
>> Yes, or not stop it if it is configured for "always enabled", which
>> this patch does.
>>
> Right. Should I squash together the two commits which modify the gpio
> watchdog ? That way, we would keep the same behaviour all along the
> patchset.
>
Yes, think that would be a good idea - if for nothing else to avoid that
one gets applied and the other doesn't. Please add a note to the description
about the changes and the reason for it.

Thanks,
Guenter

>>> But actually, I don't understand why the notifier is registered in the
>>> first place in that case.
>>>
>>
>> Good question. Either case I think we are good, because the new behavior will
>> be to not stop the watchdog on shutdown if it is configured to "always enabled".
>>
>> Thanks,
>> Guenter
>>
>