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@xxxxxxxxxxxxxxxxxxxx>
> >>Reviewed-by: Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxx>
> >>---
> >> 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.
But actually, I don't understand why the notifier is registered in the
first place in that case.
Damien
>
> 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@xxxxxxxxxxxxxxxxx
> 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
