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

[Mike Looijmans <mike.looijmans@topic.nl>]

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.


Mike.

>
> Thanks,
> Guenter
>
>> -    return NOTIFY_DONE;
>> -}
>> -
>>   static const struct watchdog_info gpio_wdt_ident = {
>>       .options    = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
>>                 WDIOF_SETTIMEOUT,
>> @@ -224,23 +201,16 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>>
>>       setup_timer(&priv->timer, gpio_wdt_hwping, (unsigned long)&priv->wdd);
>>
>> +    watchdog_stop_on_reboot(&priv->wdd);
>> +
>>       ret = watchdog_register_device(&priv->wdd);
>>       if (ret)
>>           return ret;
>>
>> -    priv->notifier.notifier_call = gpio_wdt_notify_sys;
>> -    ret = register_reboot_notifier(&priv->notifier);
>> -    if (ret)
>> -        goto error_unregister;
>> -
>>       if (priv->always_running)
>>           gpio_wdt_start_impl(priv);
>>
>>       return 0;
>> -
>> -error_unregister:
>> -    watchdog_unregister_device(&priv->wdd);
>> -    return ret;
>>   }
>>
>>   static int gpio_wdt_remove(struct platform_device *pdev)
>> @@ -248,7 +218,6 @@ static int gpio_wdt_remove(struct platform_device *pdev)
>>       struct gpio_wdt_priv *priv = platform_get_drvdata(pdev);
>>
>>       del_timer_sync(&priv->timer);
>> -    unregister_reboot_notifier(&priv->notifier);
>>       watchdog_unregister_device(&priv->wdd);
>>
>>       return 0;
>>
>



Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

Visit us at : Aerospace Electrical Systems Expo Europe which will be held from 17.11.2015 till 19.11.2015, Findorffstrasse 101 Bremen, Germany, Hall 5, stand number C65
http://www.aesexpo.eu