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

[Damien Riegel <damien.riegel@savoirfairelinux.com>]

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.

> >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
>