[PATCH 0/7] watchdog: factorize reboot notifier registration

[PATCH 0/7] watchdog: factorize reboot notifier registration

[Damien Riegel]

Many drivers implements the exact same piece of code to register a
reboot notifier. It can be nice to factorize this in the watchdog core.

The first patch adds a new helper function, watchdog_stop_on_reboot,
which registers a reboot notifier block for this device. If used, the
watchdog core will care about stopping the watchdog when receiving a
reboot notification.

The following patches bring this change to the current watchdog drivers
that use watchdog_core.

This action is done only when the reboot code is SYS_HALT or SYS_DOWN,
since that is what most of the drivers are doing.

Only 3 drivers, including gpio_wdt, stop the watchdog on SYS_POWER_OFF.
So gpio_wdt is the only driver whose behaviour is affected by this
patchset.

This change has been compile-tested with allyesconfig on arm.
It has been tested with (not mainlined yet) ts-4800's watchdog driver.

Damien Riegel (7):
  watchdog: core: add reboot notifier support
  watchdog: bcm47xx_wdt: use core reboot notifier
  watchdog: cadence_wdt: use core reboot notifier
  watchdog: gpio_wdt: stop on SYS_DOWN instead of SYS_POWER_OFF
  watchdog: gpio_wdt: use core reboot notifier
  watchdog: softdog: use core reboot notifier
  watchdog: w83627hf_wdt: use core reboot notifier

 Documentation/watchdog/watchdog-kernel-api.txt |  8 ++++++
 drivers/watchdog/bcm47xx_wdt.c                 | 24 ++---------------
 drivers/watchdog/cadence_wdt.c                 | 36 +-------------------------
 drivers/watchdog/gpio_wdt.c                    | 35 ++-----------------------
 drivers/watchdog/softdog.c                     | 30 ++-------------------
 drivers/watchdog/w83627hf_wdt.c                | 32 ++---------------------
 drivers/watchdog/watchdog_core.c               | 35 +++++++++++++++++++++++++
 include/linux/bcm47xx_wdt.h                    |  2 --
 include/linux/watchdog.h                       |  9 +++++++
 9 files changed, 61 insertions(+), 150 deletions(-)

-- 
2.5.0

[PATCH 1/7] watchdog: core: add reboot notifier support

[Damien Riegel]

Many watchdog drivers register a reboot notifier in order to stop the
watchdog on system reboot. Thus we can factorize this code in the
watchdog core.

For that purpose, a new notifier block is added in watchdog_device for
internal use only, as well as a new watchdog_stop_on_reboot helper
function.

If this helper is called, watchdog core registers the related notifier
block and will stop the watchdog when SYS_HALT or SYS_DOWN is received.

Since this operation can be critical on some platforms, abort the device
registration if the reboot notifier registration fails.

Suggested-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 Documentation/watchdog/watchdog-kernel-api.txt |  8 ++++++
 drivers/watchdog/watchdog_core.c               | 35 ++++++++++++++++++++++++++
 include/linux/watchdog.h                       |  9 +++++++
 3 files changed, 52 insertions(+)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index dbc6a65..0a37da7 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -53,6 +53,7 @@ struct watchdog_device {
 	unsigned int timeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
+	struct notifier_block reboot_nb;
 	struct notifier_block restart_nb;
 	void *driver_data;
 	struct mutex lock;
@@ -76,6 +77,9 @@ It contains following fields:
 * timeout: the watchdog timer's timeout value (in seconds).
 * min_timeout: the watchdog timer's minimum timeout value (in seconds).
 * max_timeout: the watchdog timer's maximum timeout value (in seconds).
+* reboot_nb: notifier block that is registered for reboot notifications, for
+  internal use only. If the driver calls watchdog_stop_on_reboot, watchdog core
+  will stop the watchdog on such notifications.
 * restart_nb: notifier block that is registered for machine restart, for
   internal use only. If a watchdog is capable of restarting the machine, it
   should define ops->restart. Priority can be changed through
@@ -240,6 +244,10 @@ to set the default timeout value as timeout value in the watchdog_device and
 then use this function to set the user "preferred" timeout value.
 This routine returns zero on success and a negative errno code for failure.
 
+To disable the watchdog on reboot, the user must call the following helper:
+
+static inline void watchdog_stop_on_reboot(struct watchdog_device *wdd);
+
 To change the priority of the restart handler the following helper should be
 used:
 
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 28dc901..2d49f8d 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -138,6 +138,23 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 }
 EXPORT_SYMBOL_GPL(watchdog_init_timeout);
 
+static int watchdog_reboot_notifier(struct notifier_block *nb,
+				    unsigned long code, void *data)
+{
+	struct watchdog_device *wdd = container_of(nb, struct watchdog_device,
+						   reboot_nb);
+
+	if (code == SYS_DOWN || code == SYS_HALT) {
+		int ret;
+
+		ret = wdd->ops->stop(wdd);
+		if (ret)
+			return NOTIFY_BAD;
+	}
+
+	return NOTIFY_DONE;
+}
+
 static int watchdog_restart_notifier(struct notifier_block *nb,
 				     unsigned long action, void *data)
 {
@@ -238,6 +255,21 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		return ret;
 	}
 
+	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
+		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
+
+		ret = register_reboot_notifier(&wdd->reboot_nb);
+		if (ret) {
+			dev_err(wdd->dev, "Cannot register reboot notifier (%d)\n",
+				ret);
+			watchdog_dev_unregister(wdd);
+			device_destroy(watchdog_class, devno);
+			ida_simple_remove(&watchdog_ida, wdd->id);
+			wdd->dev = NULL;
+			return ret;
+		}
+	}
+
 	if (wdd->ops->restart) {
 		wdd->restart_nb.notifier_call = watchdog_restart_notifier;
 
@@ -286,6 +318,9 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
 	if (wdd->ops->restart)
 		unregister_restart_handler(&wdd->restart_nb);
 
+	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
+		unregister_reboot_notifier(&wdd->reboot_nb);
+
 	devno = wdd->cdev.dev;
 	ret = watchdog_dev_unregister(wdd);
 	if (ret)
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 5b52c83..a88f955 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -65,6 +65,7 @@ struct watchdog_ops {
  * @timeout:	The watchdog devices timeout value (in seconds).
  * @min_timeout:The watchdog devices minimum timeout value (in seconds).
  * @max_timeout:The watchdog devices maximum timeout value (in seconds).
+ * @reboot_nb:	The notifier block to stop watchdog on reboot.
  * @restart_nb:	The notifier block to register a restart function.
  * @driver-data:Pointer to the drivers private data.
  * @lock:	Lock for watchdog core internal use only.
@@ -92,6 +93,7 @@ struct watchdog_device {
 	unsigned int timeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
+	struct notifier_block reboot_nb;
 	struct notifier_block restart_nb;
 	void *driver_data;
 	struct mutex lock;
@@ -102,6 +104,7 @@ struct watchdog_device {
 #define WDOG_ALLOW_RELEASE	2	/* Did we receive the magic char ? */
 #define WDOG_NO_WAY_OUT		3	/* Is 'nowayout' feature set ? */
 #define WDOG_UNREGISTERED	4	/* Has the device been unregistered */
+#define WDOG_STOP_ON_REBOOT	5	/* Should be stopped on reboot */
 	struct list_head deferred;
 };
 
@@ -121,6 +124,12 @@ static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool noway
 		set_bit(WDOG_NO_WAY_OUT, &wdd->status);
 }
 
+/* Use the following function to stop the watchdog on reboot */
+static inline void watchdog_stop_on_reboot(struct watchdog_device *wdd)
+{
+	set_bit(WDOG_STOP_ON_REBOOT, &wdd->status);
+}
+
 /* Use the following function to check if a timeout value is invalid */
 static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
 {
-- 
2.5.0

[PATCH 2/7] watchdog: bcm47xx_wdt: use core reboot notifier

[Damien Riegel]

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/bcm47xx_wdt.c | 24 ++----------------------
 include/linux/bcm47xx_wdt.h    |  2 --
 2 files changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/watchdog/bcm47xx_wdt.c b/drivers/watchdog/bcm47xx_wdt.c
index 01bffe1..df1c2a4 100644
--- a/drivers/watchdog/bcm47xx_wdt.c
+++ b/drivers/watchdog/bcm47xx_wdt.c
@@ -20,7 +20,6 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/platform_device.h>
-#include <linux/reboot.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
 #include <linux/timer.h>
@@ -168,17 +167,6 @@ static const struct watchdog_info bcm47xx_wdt_info = {
 				WDIOF_MAGICCLOSE,
 };
 
-static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
-				  unsigned long code, void *unused)
-{
-	struct bcm47xx_wdt *wdt;
-
-	wdt = container_of(this, struct bcm47xx_wdt, notifier);
-	if (code == SYS_DOWN || code == SYS_HALT)
-		wdt->wdd.ops->stop(&wdt->wdd);
-	return NOTIFY_DONE;
-}
-
 static struct watchdog_ops bcm47xx_wdt_soft_ops = {
 	.owner		= THIS_MODULE,
 	.start		= bcm47xx_wdt_soft_start,
@@ -215,24 +203,17 @@ static int bcm47xx_wdt_probe(struct platform_device *pdev)
 		goto err_timer;
 	watchdog_set_nowayout(&wdt->wdd, nowayout);
 	watchdog_set_restart_priority(&wdt->wdd, 64);
-
-	wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
-
-	ret = register_reboot_notifier(&wdt->notifier);
-	if (ret)
-		goto err_timer;
+	watchdog_stop_on_reboot(&wdt->wdd);
 
 	ret = watchdog_register_device(&wdt->wdd);
 	if (ret)
-		goto err_notifier;
+		goto err_timer;
 
 	dev_info(&pdev->dev, "BCM47xx Watchdog Timer enabled (%d seconds%s%s)\n",
 		timeout, nowayout ? ", nowayout" : "",
 		soft ? ", Software Timer" : "");
 	return 0;
 
-err_notifier:
-	unregister_reboot_notifier(&wdt->notifier);
 err_timer:
 	if (soft)
 		del_timer_sync(&wdt->soft_timer);
@@ -248,7 +229,6 @@ static int bcm47xx_wdt_remove(struct platform_device *pdev)
 		return -ENXIO;
 
 	watchdog_unregister_device(&wdt->wdd);
-	unregister_reboot_notifier(&wdt->notifier);
 
 	return 0;
 }
diff --git a/include/linux/bcm47xx_wdt.h b/include/linux/bcm47xx_wdt.h
index b708786..8d9d07e 100644
--- a/include/linux/bcm47xx_wdt.h
+++ b/include/linux/bcm47xx_wdt.h
@@ -1,7 +1,6 @@
 #ifndef LINUX_BCM47XX_WDT_H_
 #define LINUX_BCM47XX_WDT_H_
 
-#include <linux/notifier.h>
 #include <linux/timer.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
@@ -15,7 +14,6 @@ struct bcm47xx_wdt {
 	void *driver_data;
 
 	struct watchdog_device wdd;
-	struct notifier_block notifier;
 
 	struct timer_list soft_timer;
 	atomic_t soft_ticks;
-- 
2.5.0

[PATCH 3/7] watchdog: cadence_wdt: use core reboot notifier

[Damien Riegel]

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/cadence_wdt.c | 36 +-----------------------------------
 1 file changed, 1 insertion(+), 35 deletions(-)

diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
index bcfd2a2..abf64eb 100644
--- a/drivers/watchdog/cadence_wdt.c
+++ b/drivers/watchdog/cadence_wdt.c
@@ -18,7 +18,6 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <linux/reboot.h>
 #include <linux/watchdog.h>
 
 #define CDNS_WDT_DEFAULT_TIMEOUT	10
@@ -72,7 +71,6 @@ MODULE_PARM_DESC(nowayout,
  * @ctrl_clksel: counter clock prescaler selection
  * @io_lock: spinlock for IO register access
  * @cdns_wdt_device: watchdog device structure
- * @cdns_wdt_notifier: notifier structure
  *
  * Structure containing parameters specific to cadence watchdog.
  */
@@ -84,7 +82,6 @@ struct cdns_wdt {
 	u32			ctrl_clksel;
 	spinlock_t		io_lock;
 	struct watchdog_device	cdns_wdt_device;
-	struct notifier_block	cdns_wdt_notifier;
 };
 
 /* Write access to Registers */
@@ -280,29 +277,6 @@ static struct watchdog_ops cdns_wdt_ops = {
 	.set_timeout = cdns_wdt_settimeout,
 };
 
-/**
- * cdns_wdt_notify_sys - Notifier for reboot or shutdown.
- *
- * @this: handle to notifier block
- * @code: turn off indicator
- * @unused: unused
- * Return: NOTIFY_DONE
- *
- * This notifier is invoked whenever the system reboot or shutdown occur
- * because we need to disable the WDT before system goes down as WDT might
- * reset on the next boot.
- */
-static int cdns_wdt_notify_sys(struct notifier_block *this, unsigned long code,
-			       void *unused)
-{
-	struct cdns_wdt *wdt = container_of(this, struct cdns_wdt,
-					    cdns_wdt_notifier);
-	if (code == SYS_DOWN || code == SYS_HALT)
-		cdns_wdt_stop(&wdt->cdns_wdt_device);
-
-	return NOTIFY_DONE;
-}
-
 /************************Platform Operations*****************************/
 /**
  * cdns_wdt_probe - Probe call for the device.
@@ -360,6 +334,7 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 	}
 
 	watchdog_set_nowayout(cdns_wdt_device, nowayout);
+	watchdog_stop_on_reboot(cdns_wdt_device);
 	watchdog_set_drvdata(cdns_wdt_device, wdt);
 
 	wdt->clk = devm_clk_get(&pdev->dev, NULL);
@@ -386,14 +361,6 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 
 	spin_lock_init(&wdt->io_lock);
 
-	wdt->cdns_wdt_notifier.notifier_call = &cdns_wdt_notify_sys;
-	ret = register_reboot_notifier(&wdt->cdns_wdt_notifier);
-	if (ret != 0) {
-		dev_err(&pdev->dev, "cannot register reboot notifier err=%d)\n",
-			ret);
-		goto err_clk_disable;
-	}
-
 	ret = watchdog_register_device(cdns_wdt_device);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to register wdt device\n");
@@ -427,7 +394,6 @@ static int cdns_wdt_remove(struct platform_device *pdev)
 
 	cdns_wdt_stop(&wdt->cdns_wdt_device);
 	watchdog_unregister_device(&wdt->cdns_wdt_device);
-	unregister_reboot_notifier(&wdt->cdns_wdt_notifier);
 	clk_disable_unprepare(wdt->clk);
 
 	return 0;
-- 
2.5.0

[PATCH 4/7] watchdog: gpio_wdt: stop on SYS_DOWN instead of SYS_POWER_OFF

[Damien Riegel]

gpio_wdt is one the very few drivers that do not stop the device on
SYS_HALT and SYS_DOWN, but on SYS_HALT and SYS_POWER_OFF.

As most of the drivers, it makes more sense to stop the watchdog on
SYS_DOWN instead of SYS_POWER_OFF.

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/watchdog/gpio_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
index 90d59d3..1a3c6e8 100644
--- a/drivers/watchdog/gpio_wdt.c
+++ b/drivers/watchdog/gpio_wdt.c
@@ -136,7 +136,7 @@ static int gpio_wdt_notify_sys(struct notifier_block *nb, unsigned long code,
 
 	switch (code) {
 	case SYS_HALT:
-	case SYS_POWER_OFF:
+	case SYS_DOWN:
 		gpio_wdt_disable(priv);
 		break;
 	default:
-- 
2.5.0

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

[Damien Riegel]

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;
-	}
-
-	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;
-- 
2.5.0

[PATCH 6/7] watchdog: softdog: use core reboot notifier

[Damien Riegel]

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/softdog.c | 30 ++----------------------------
 1 file changed, 2 insertions(+), 28 deletions(-)

diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
index 0dc5e323d..fe1e151 100644
--- a/drivers/watchdog/softdog.c
+++ b/drivers/watchdog/softdog.c
@@ -43,7 +43,6 @@
 #include <linux/types.h>
 #include <linux/timer.h>
 #include <linux/watchdog.h>
-#include <linux/notifier.h>
 #include <linux/reboot.h>
 #include <linux/init.h>
 #include <linux/jiffies.h>
@@ -122,26 +121,9 @@ static int softdog_set_timeout(struct watchdog_device *w, unsigned int t)
 }
 
 /*
- *	Notifier for system down
- */
-
-static int softdog_notify_sys(struct notifier_block *this, unsigned long code,
-	void *unused)
-{
-	if (code == SYS_DOWN || code == SYS_HALT)
-		/* Turn the WDT off */
-		softdog_stop(NULL);
-	return NOTIFY_DONE;
-}
-
-/*
  *	Kernel Interfaces
  */
 
-static struct notifier_block softdog_notifier = {
-	.notifier_call	= softdog_notify_sys,
-};
-
 static struct watchdog_info softdog_info = {
 	.identity = "Software Watchdog",
 	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
@@ -175,18 +157,11 @@ static int __init watchdog_init(void)
 	softdog_dev.timeout = soft_margin;
 
 	watchdog_set_nowayout(&softdog_dev, nowayout);
-
-	ret = register_reboot_notifier(&softdog_notifier);
-	if (ret) {
-		pr_err("cannot register reboot notifier (err=%d)\n", ret);
-		return ret;
-	}
+	watchdog_stop_on_reboot(&softdog_dev);
 
 	ret = watchdog_register_device(&softdog_dev);
-	if (ret) {
-		unregister_reboot_notifier(&softdog_notifier);
+	if (ret)
 		return ret;
-	}
 
 	pr_info("Software Watchdog Timer: 0.08 initialized. soft_noboot=%d soft_margin=%d sec soft_panic=%d (nowayout=%d)\n",
 		soft_noboot, soft_margin, soft_panic, nowayout);
@@ -197,7 +172,6 @@ static int __init watchdog_init(void)
 static void __exit watchdog_exit(void)
 {
 	watchdog_unregister_device(&softdog_dev);
-	unregister_reboot_notifier(&softdog_notifier);
 }
 
 module_init(watchdog_init);
-- 
2.5.0

[PATCH 7/7] watchdog: w83627hf_wdt: use core reboot notifier

[Damien Riegel]

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/w83627hf_wdt.c | 32 ++------------------------------
 1 file changed, 2 insertions(+), 30 deletions(-)

diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
index 5824e25..cab14bc 100644
--- a/drivers/watchdog/w83627hf_wdt.c
+++ b/drivers/watchdog/w83627hf_wdt.c
@@ -36,8 +36,6 @@
 #include <linux/types.h>
 #include <linux/watchdog.h>
 #include <linux/ioport.h>
-#include <linux/notifier.h>
-#include <linux/reboot.h>
 #include <linux/init.h>
 #include <linux/io.h>
 
@@ -288,18 +286,6 @@ static unsigned int wdt_get_time(struct watchdog_device *wdog)
 }
 
 /*
- *	Notifier for system down
- */
-static int wdt_notify_sys(struct notifier_block *this, unsigned long code,
-	void *unused)
-{
-	if (code == SYS_DOWN || code == SYS_HALT)
-		wdt_set_time(0);	/* Turn the WDT off */
-
-	return NOTIFY_DONE;
-}
-
-/*
  *	Kernel Interfaces
  */
 
@@ -329,10 +315,6 @@ static struct watchdog_device wdt_dev = {
  *	turn the timebomb registers off.
  */
 
-static struct notifier_block wdt_notifier = {
-	.notifier_call = wdt_notify_sys,
-};
-
 static int wdt_find(int addr)
 {
 	u8 val;
@@ -456,6 +438,7 @@ static int __init wdt_init(void)
 
 	watchdog_init_timeout(&wdt_dev, timeout, NULL);
 	watchdog_set_nowayout(&wdt_dev, nowayout);
+	watchdog_stop_on_reboot(&wdt_dev);
 
 	ret = w83627hf_init(&wdt_dev, chip);
 	if (ret) {
@@ -463,30 +446,19 @@ static int __init wdt_init(void)
 		return ret;
 	}
 
-	ret = register_reboot_notifier(&wdt_notifier);
-	if (ret != 0) {
-		pr_err("cannot register reboot notifier (err=%d)\n", ret);
-		return ret;
-	}
-
 	ret = watchdog_register_device(&wdt_dev);
 	if (ret)
-		goto unreg_reboot;
+		return ret;
 
 	pr_info("initialized. timeout=%d sec (nowayout=%d)\n",
 		wdt_dev.timeout, nowayout);
 
 	return ret;
-
-unreg_reboot:
-	unregister_reboot_notifier(&wdt_notifier);
-	return ret;
 }
 
 static void __exit wdt_exit(void)
 {
 	watchdog_unregister_device(&wdt_dev);
-	unregister_reboot_notifier(&wdt_notifier);
 }
 
 module_init(wdt_init);
-- 
2.5.0

Re: [PATCH 1/7] watchdog: core: add reboot notifier support

[Guenter Roeck]

On 11/18/2015 01:02 PM, Damien Riegel wrote:
> Many watchdog drivers register a reboot notifier in order to stop the
> watchdog on system reboot. Thus we can factorize this code in the
> watchdog core.
>
> For that purpose, a new notifier block is added in watchdog_device for
> internal use only, as well as a new watchdog_stop_on_reboot helper
> function.
>
> If this helper is called, watchdog core registers the related notifier
> block and will stop the watchdog when SYS_HALT or SYS_DOWN is received.
>
> Since this operation can be critical on some platforms, abort the device
> registration if the reboot notifier registration fails.
>

Hi Damien,

Good idea. Couple of comments below.

Thanks,
Guenter

> Suggested-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   Documentation/watchdog/watchdog-kernel-api.txt |  8 ++++++
>   drivers/watchdog/watchdog_core.c               | 35 ++++++++++++++++++++++++++
>   include/linux/watchdog.h                       |  9 +++++++
>   3 files changed, 52 insertions(+)
>
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index dbc6a65..0a37da7 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -53,6 +53,7 @@ struct watchdog_device {
>   	unsigned int timeout;
>   	unsigned int min_timeout;
>   	unsigned int max_timeout;
> +	struct notifier_block reboot_nb;
>   	struct notifier_block restart_nb;
>   	void *driver_data;
>   	struct mutex lock;
> @@ -76,6 +77,9 @@ It contains following fields:
>   * timeout: the watchdog timer's timeout value (in seconds).
>   * min_timeout: the watchdog timer's minimum timeout value (in seconds).
>   * max_timeout: the watchdog timer's maximum timeout value (in seconds).
> +* reboot_nb: notifier block that is registered for reboot notifications, for
> +  internal use only. If the driver calls watchdog_stop_on_reboot, watchdog core
> +  will stop the watchdog on such notifications.
>   * restart_nb: notifier block that is registered for machine restart, for
>     internal use only. If a watchdog is capable of restarting the machine, it
>     should define ops->restart. Priority can be changed through
> @@ -240,6 +244,10 @@ to set the default timeout value as timeout value in the watchdog_device and
>   then use this function to set the user "preferred" timeout value.
>   This routine returns zero on success and a negative errno code for failure.
>
> +To disable the watchdog on reboot, the user must call the following helper:
> +
> +static inline void watchdog_stop_on_reboot(struct watchdog_device *wdd);
> +
>   To change the priority of the restart handler the following helper should be
>   used:
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 28dc901..2d49f8d 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -138,6 +138,23 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>   }
>   EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>
> +static int watchdog_reboot_notifier(struct notifier_block *nb,
> +				    unsigned long code, void *data)
> +{
> +	struct watchdog_device *wdd = container_of(nb, struct watchdog_device,
> +						   reboot_nb);
> +
> +	if (code == SYS_DOWN || code == SYS_HALT) {
> +		int ret;
> +
> +		ret = wdd->ops->stop(wdd);

Nitpick, maybe, but only stop if running ?
Maybe that isn't a problem, but I am a bit concerned about side
effects of trying to stop a watchdog which isn't running.

> +		if (ret)
> +			return NOTIFY_BAD;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
>   static int watchdog_restart_notifier(struct notifier_block *nb,
>   				     unsigned long action, void *data)
>   {
> @@ -238,6 +255,21 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>   		return ret;
>   	}
>
> +	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
> +		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
> +
> +		ret = register_reboot_notifier(&wdd->reboot_nb);
> +		if (ret) {
> +			dev_err(wdd->dev, "Cannot register reboot notifier (%d)\n",
> +				ret);
> +			watchdog_dev_unregister(wdd);
> +			device_destroy(watchdog_class, devno);
> +			ida_simple_remove(&watchdog_ida, wdd->id);
> +			wdd->dev = NULL;
> +			return ret;

At some point this really asks for a proper error handler in this function.
Separate patch, though.

> +		}
> +	}
> +
>   	if (wdd->ops->restart) {
>   		wdd->restart_nb.notifier_call = watchdog_restart_notifier;
>
> @@ -286,6 +318,9 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
>   	if (wdd->ops->restart)
>   		unregister_restart_handler(&wdd->restart_nb);
>
> +	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
> +		unregister_reboot_notifier(&wdd->reboot_nb);
> +
>   	devno = wdd->cdev.dev;
>   	ret = watchdog_dev_unregister(wdd);
>   	if (ret)
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 5b52c83..a88f955 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -65,6 +65,7 @@ struct watchdog_ops {
>    * @timeout:	The watchdog devices timeout value (in seconds).
>    * @min_timeout:The watchdog devices minimum timeout value (in seconds).
>    * @max_timeout:The watchdog devices maximum timeout value (in seconds).
> + * @reboot_nb:	The notifier block to stop watchdog on reboot.
>    * @restart_nb:	The notifier block to register a restart function.
>    * @driver-data:Pointer to the drivers private data.
>    * @lock:	Lock for watchdog core internal use only.
> @@ -92,6 +93,7 @@ struct watchdog_device {
>   	unsigned int timeout;
>   	unsigned int min_timeout;
>   	unsigned int max_timeout;
> +	struct notifier_block reboot_nb;
>   	struct notifier_block restart_nb;
>   	void *driver_data;
>   	struct mutex lock;
> @@ -102,6 +104,7 @@ struct watchdog_device {
>   #define WDOG_ALLOW_RELEASE	2	/* Did we receive the magic char ? */
>   #define WDOG_NO_WAY_OUT		3	/* Is 'nowayout' feature set ? */
>   #define WDOG_UNREGISTERED	4	/* Has the device been unregistered */
> +#define WDOG_STOP_ON_REBOOT	5	/* Should be stopped on reboot */
>   	struct list_head deferred;
>   };
>
> @@ -121,6 +124,12 @@ static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool noway
>   		set_bit(WDOG_NO_WAY_OUT, &wdd->status);
>   }
>
> +/* Use the following function to stop the watchdog on reboot */
> +static inline void watchdog_stop_on_reboot(struct watchdog_device *wdd)
> +{
> +	set_bit(WDOG_STOP_ON_REBOOT, &wdd->status);
> +}
> +
>   /* Use the following function to check if a timeout value is invalid */
>   static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
>   {
>

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

[Guenter Roeck]

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.

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

Re: [PATCH 2/7] watchdog: bcm47xx_wdt: use core reboot notifier

[Guenter Roeck]

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>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/bcm47xx_wdt.c | 24 ++----------------------
>   include/linux/bcm47xx_wdt.h    |  2 --
>   2 files changed, 2 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/watchdog/bcm47xx_wdt.c b/drivers/watchdog/bcm47xx_wdt.c
> index 01bffe1..df1c2a4 100644
> --- a/drivers/watchdog/bcm47xx_wdt.c
> +++ b/drivers/watchdog/bcm47xx_wdt.c
> @@ -20,7 +20,6 @@
>   #include <linux/module.h>
>   #include <linux/moduleparam.h>
>   #include <linux/platform_device.h>
> -#include <linux/reboot.h>
>   #include <linux/types.h>
>   #include <linux/watchdog.h>
>   #include <linux/timer.h>
> @@ -168,17 +167,6 @@ static const struct watchdog_info bcm47xx_wdt_info = {
>   				WDIOF_MAGICCLOSE,
>   };
>
> -static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
> -				  unsigned long code, void *unused)
> -{
> -	struct bcm47xx_wdt *wdt;
> -
> -	wdt = container_of(this, struct bcm47xx_wdt, notifier);
> -	if (code == SYS_DOWN || code == SYS_HALT)
> -		wdt->wdd.ops->stop(&wdt->wdd);
> -	return NOTIFY_DONE;
> -}
> -
>   static struct watchdog_ops bcm47xx_wdt_soft_ops = {
>   	.owner		= THIS_MODULE,
>   	.start		= bcm47xx_wdt_soft_start,
> @@ -215,24 +203,17 @@ static int bcm47xx_wdt_probe(struct platform_device *pdev)
>   		goto err_timer;
>   	watchdog_set_nowayout(&wdt->wdd, nowayout);
>   	watchdog_set_restart_priority(&wdt->wdd, 64);
> -
> -	wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
> -
> -	ret = register_reboot_notifier(&wdt->notifier);
> -	if (ret)
> -		goto err_timer;
> +	watchdog_stop_on_reboot(&wdt->wdd);
>
>   	ret = watchdog_register_device(&wdt->wdd);
>   	if (ret)
> -		goto err_notifier;
> +		goto err_timer;
>
>   	dev_info(&pdev->dev, "BCM47xx Watchdog Timer enabled (%d seconds%s%s)\n",
>   		timeout, nowayout ? ", nowayout" : "",
>   		soft ? ", Software Timer" : "");
>   	return 0;
>
> -err_notifier:
> -	unregister_reboot_notifier(&wdt->notifier);
>   err_timer:
>   	if (soft)
>   		del_timer_sync(&wdt->soft_timer);
> @@ -248,7 +229,6 @@ static int bcm47xx_wdt_remove(struct platform_device *pdev)
>   		return -ENXIO;
>
>   	watchdog_unregister_device(&wdt->wdd);
> -	unregister_reboot_notifier(&wdt->notifier);
>
>   	return 0;
>   }
> diff --git a/include/linux/bcm47xx_wdt.h b/include/linux/bcm47xx_wdt.h
> index b708786..8d9d07e 100644
> --- a/include/linux/bcm47xx_wdt.h
> +++ b/include/linux/bcm47xx_wdt.h
> @@ -1,7 +1,6 @@
>   #ifndef LINUX_BCM47XX_WDT_H_
>   #define LINUX_BCM47XX_WDT_H_
>
> -#include <linux/notifier.h>
>   #include <linux/timer.h>
>   #include <linux/types.h>
>   #include <linux/watchdog.h>
> @@ -15,7 +14,6 @@ struct bcm47xx_wdt {
>   	void *driver_data;
>
>   	struct watchdog_device wdd;
> -	struct notifier_block notifier;
>
>   	struct timer_list soft_timer;
>   	atomic_t soft_ticks;
>

Re: [PATCH 3/7] watchdog: cadence_wdt: use core reboot notifier

[Guenter Roeck]

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>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH 6/7] watchdog: softdog: use core reboot notifier

[Guenter Roeck]

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>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH 4/7] watchdog: gpio_wdt: stop on SYS_DOWN instead of SYS_POWER_OFF

[Guenter Roeck]

On 11/18/2015 01:02 PM, Damien Riegel wrote:
> gpio_wdt is one the very few drivers that do not stop the device on
> SYS_HALT and SYS_DOWN, but on SYS_HALT and SYS_POWER_OFF.
>
> As most of the drivers, it makes more sense to stop the watchdog on
> SYS_DOWN instead of SYS_POWER_OFF.
>
> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Makes sense.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/gpio_wdt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> index 90d59d3..1a3c6e8 100644
> --- a/drivers/watchdog/gpio_wdt.c
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -136,7 +136,7 @@ static int gpio_wdt_notify_sys(struct notifier_block *nb, unsigned long code,
>
>   	switch (code) {
>   	case SYS_HALT:
> -	case SYS_POWER_OFF:
> +	case SYS_DOWN:
>   		gpio_wdt_disable(priv);
>   		break;
>   	default:
>

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

[Mike Looijmans]

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

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

[Damien Riegel]

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.

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

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

[Guenter Roeck]

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.

> 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

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

[Damien Riegel]

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
> 

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

[Guenter Roeck]

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