[PATCH v2 0/6] watchdog: factorize reboot notifier registration

[PATCH v2 0/6] 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, do not stop the watchdog on
SYS_DOWN. So for this watchdog, it will now stop on reboot if
"always-running" is not set, whereas it did not prior to 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.


Changes in v2:
 - call ops->stop only if watchdog is active
 - squashed gpio_wdt-related commits to keep a consistent behaviour.

Damien Riegel (6):
  watchdog: core: add reboot notifier support
  watchdog: bcm47xx_wdt: use core reboot notifier
  watchdog: cadence_wdt: use core reboot notifier
  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               | 37 ++++++++++++++++++++++++++
 include/linux/bcm47xx_wdt.h                    |  2 --
 include/linux/watchdog.h                       |  9 +++++++
 9 files changed, 63 insertions(+), 150 deletions(-)

-- 
2.5.0

[PATCH v2 1/6] 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               | 37 ++++++++++++++++++++++++++
 include/linux/watchdog.h                       |  9 +++++++
 3 files changed, 54 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..551af04 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -138,6 +138,25 @@ 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) {
+		if (watchdog_active(wdd)) {
+			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 +257,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 +320,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 v2 2/6] 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: Guenter Roeck <linux@roeck-us.net>
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 v2 3/6] 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: Guenter Roeck <linux@roeck-us.net>
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 v2 4/6] 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.

Note that this watchdog used to stop unconditionnaly on SYS_HALT and
SYS_POWER_OFF. The core function now calls ops->stop on SYS_HALT and
SYS_DOWN. To prevent the watchdog from being stopped on reboot, the
"always-running" property must be set, otherwise it will now be stopped.

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 90d59d3..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_POWER_OFF:
-		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 v2 5/6] 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: Guenter Roeck <linux@roeck-us.net>
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 v2 6/6] 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 v2 1/6] watchdog: core: add reboot notifier support

[Guenter Roeck]

On 11/20/2015 01:54 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.
>
> 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>

Series looks good to me.

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

I'll apply the series to my watchdog-next branch and run it through a test cycle.

Guenter

Re: [PATCH v2 6/6] watchdog: w83627hf_wdt: use core reboot notifier

[Guenter Roeck]

On 11/20/2015 01:54 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 v2 4/6] watchdog: gpio_wdt: use core reboot notifier

[Guenter Roeck]

On 11/20/2015 01:54 PM, Damien Riegel wrote:
> Get rid of the custom reboot notifier block registration and use the one
> provided by the watchdog core.
>
> Note that this watchdog used to stop unconditionnaly on SYS_HALT and
> SYS_POWER_OFF. The core function now calls ops->stop on SYS_HALT and
> SYS_DOWN. To prevent the watchdog from being stopped on reboot, the
> "always-running" property must be set, otherwise it will now be stopped.
>
> 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: In-reply-to: <1448056496-2335-1-git-send-email-damien.riegel@savoirfairelinux.com>

[Harald Geyer]

Hi,

sorry for breaking the threading. Only stumbled upon this message in the
webarchive of the Mailinglist and couldn't reconstruct full headers ...

I think there is an issue with your patch:

+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) {

I think you want this to be (code == SYS_POWER_OFF || code == SYS_HALT)
AFAIK SYS_DOWN is the code for a reboot, where the system should come
back up immediatly, so probably we shouldn't disable the watchdog in
this case, for the system might crash during going down.

More importantly however we should stop the watchdog on SYS_POWER_OFF
I think.

Otherwise your series looks good to me and I look forward to rebase
my own patches on it.

HTH,
Harald

+		if (watchdog_active(wdd)) {
+			int ret;
+
+			ret = wdd->ops->stop(wdd);
+			if (ret)
+				return NOTIFY_BAD;
+		}
+	}
+
+	return NOTIFY_DONE;
+}
-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via bitcoin to 1FUtd8T9jRN1rFz63vZz7s2fDtB6d6A7aS

Re: In-reply-to: <1448056496-2335-1-git-send-email-damien.riegel@savoirfairelinux.com>

[Damien Riegel]

[Ccing Guenter as he reviewed the patches]

On Mon, Nov 23, 2015 at 04:02:52PM +0100, Harald Geyer wrote:
> Hi,
> 
> sorry for breaking the threading. Only stumbled upon this message in the
> webarchive of the Mailinglist and couldn't reconstruct full headers ...
> 
> I think there is an issue with your patch:
> 
> +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) {
> 
> I think you want this to be (code == SYS_POWER_OFF || code == SYS_HALT)
> AFAIK SYS_DOWN is the code for a reboot, where the system should come
> back up immediatly, so probably we shouldn't disable the watchdog in
> this case, for the system might crash during going down.

Well, most of the drivers (all of them but gpio) that I changed stopped
on SYS_HALT and SYS_DOWN, so they explicitely wanted to stop the
watchdog on reboot. I just factorized that in watchdog core.

Maybe they should not stop on reboot in the first place, but this serie
does not introduce a new behaviour. Also, note that the watchdog will
be stopped only if it registers for that by calling
watchdog_stop_on_reboot, so there is no side effect for watchdogs that
should not be stopped.

> 
> More importantly however we should stop the watchdog on SYS_POWER_OFF
> I think.
> 
My understanding here is that if the system is powered off, the watchdog
will be powered off too, so there is no need to stop it.


Damien

> Otherwise your series looks good to me and I look forward to rebase
> my own patches on it.
> 
> HTH,
> Harald
> 
> +		if (watchdog_active(wdd)) {
> +			int ret;
> +
> +			ret = wdd->ops->stop(wdd);
> +			if (ret)
> +				return NOTIFY_BAD;
> +		}
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> -- 
> If you want to support my work:
> see http://friends.ccbib.org/harald/supporting/
> or donate via bitcoin to 1FUtd8T9jRN1rFz63vZz7s2fDtB6d6A7aS

Re: In-reply-to: <1448056496-2335-1-git-send-email-damien.riegel@savoirfairelinux.com>

[Guenter Roeck]

On 11/23/2015 07:02 AM, Harald Geyer wrote:
> Hi,
>
> sorry for breaking the threading. Only stumbled upon this message in the
> webarchive of the Mailinglist and couldn't reconstruct full headers ...
>
> I think there is an issue with your patch:
>
> +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) {
>
> I think you want this to be (code == SYS_POWER_OFF || code == SYS_HALT)
> AFAIK SYS_DOWN is the code for a reboot, where the system should come
> back up immediatly, so probably we shouldn't disable the watchdog in
> this case, for the system might crash during going down.
>
> More importantly however we should stop the watchdog on SYS_POWER_OFF
> I think.
>

Harald,

please have a look into existing watchdog drivers. Unless I am counting wrong,
there are 31 watchdog drivers using the above code, and only 3 watchdog drivers
(gpio, it8712, scx200) using SYS_POWER_OFF || SYS_HALT. I think it makes sense
to make a majority decision when moving code from drivers into the infrastructure.

Thanks,
Guenter

Re: In-reply-to: <1448056496-2335-1-git-send-email-damien.riegel@savoirfairelinux.com>

[Harald Geyer]

Damien Riegel writes:
> > +	if (code == SYS_DOWN || code == SYS_HALT) {
> > 
> > I think you want this to be (code == SYS_POWER_OFF || code == SYS_HALT)
> > AFAIK SYS_DOWN is the code for a reboot, where the system should come
> > back up immediatly, so probably we shouldn't disable the watchdog in
> > this case, for the system might crash during going down.
> 
> Well, most of the drivers (all of them but gpio) that I changed stopped
> on SYS_HALT and SYS_DOWN, so they explicitely wanted to stop the
> watchdog on reboot. I just factorized that in watchdog core.

If they wanted to do so, or the code just got copied around, is
unclear. SYS_DOWN isn't the most helpful name after all, but ...
 
> Maybe they should not stop on reboot in the first place, but this serie
> does not introduce a new behaviour.

okay. I can always send a follow up patch, if I care enough.

> > More importantly however we should stop the watchdog on SYS_POWER_OFF
> > I think.
> > 
> My understanding here is that if the system is powered off, the watchdog
> will be powered off too, so there is no need to stop it.

This is true for many platforms, but I'm pretty sure that not all
platforms have a real power off. So unless you can think of a reason not
to stop the watchdog on power off, I think the core should do it.
Actually if this can't be the default, we probably need to extend
your code so that drivers can select the behaviour they want.
(Of course we would hate to do that, as power management and watchdogs
are pretty orthogonal subsystems and having one depend on the behaviour
of the other is very unfortunate.)

HTE,
Harald

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via bitcoin to 1FUtd8T9jRN1rFz63vZz7s2fDtB6d6A7aS

Re: In-reply-to: <1448056496-2335-1-git-send-email-damien.riegel@savoirfairelinux.com>

[Damien Riegel]

On Mon, Nov 23, 2015 at 07:20:57PM +0100, Harald Geyer wrote:
> Damien Riegel writes:
> > > +	if (code == SYS_DOWN || code == SYS_HALT) {
> > > 
> > > I think you want this to be (code == SYS_POWER_OFF || code == SYS_HALT)
> > > AFAIK SYS_DOWN is the code for a reboot, where the system should come
> > > back up immediatly, so probably we shouldn't disable the watchdog in
> > > this case, for the system might crash during going down.
> > 
> > Well, most of the drivers (all of them but gpio) that I changed stopped
> > on SYS_HALT and SYS_DOWN, so they explicitely wanted to stop the
> > watchdog on reboot. I just factorized that in watchdog core.
> 
> If they wanted to do so, or the code just got copied around, is
> unclear. SYS_DOWN isn't the most helpful name after all, but ...
>  
> > Maybe they should not stop on reboot in the first place, but this serie
> > does not introduce a new behaviour.
> 
> okay. I can always send a follow up patch, if I care enough.
> 

I don't understand why you assume this is not the desired behaviour and
that the code was just copied around.

> > > More importantly however we should stop the watchdog on SYS_POWER_OFF
> > > I think.
> > > 
> > My understanding here is that if the system is powered off, the watchdog
> > will be powered off too, so there is no need to stop it.
> 
> This is true for many platforms, but I'm pretty sure that not all
> platforms have a real power off. So unless you can think of a reason not
> to stop the watchdog on power off, I think the core should do it.
> Actually if this can't be the default, we probably need to extend
> your code so that drivers can select the behaviour they want.
> (Of course we would hate to do that, as power management and watchdogs
> are pretty orthogonal subsystems and having one depend on the behaviour
> of the other is very unfortunate.)

We have to assume that power-off means that the system will be powered
off (...). If a platform has no real power-off, then it should be
halted, and in that case the core would stop the watchdog to prevent a
spurious reboot.

But maybe you're right and we should not make such distinction between
power-off and halt, but I don't really want to make changes in drivers I
don't know the context in which they are used.


Damien.

Re: In-reply-to: <1448056496-2335-1-git-send-email-damien.riegel@savoirfairelinux.com>

[Harald Geyer]

Damien Riegel writes:
> On Mon, Nov 23, 2015 at 07:20:57PM +0100, Harald Geyer wrote:
> > Damien Riegel writes:
> > > > +	if (code == SYS_DOWN || code == SYS_HALT) {
> > > > 
> > > > I think you want this to be (code == SYS_POWER_OFF || code == SYS_HALT)
> > > > AFAIK SYS_DOWN is the code for a reboot, where the system should come
> > > > back up immediatly, so probably we shouldn't disable the watchdog in
> > > > this case, for the system might crash during going down.
> > > 
> > > Well, most of the drivers (all of them but gpio) that I changed stopped
> > > on SYS_HALT and SYS_DOWN, so they explicitely wanted to stop the
> > > watchdog on reboot. I just factorized that in watchdog core.
> > 
> > If they wanted to do so, or the code just got copied around, is
> > unclear. SYS_DOWN isn't the most helpful name after all, but ...
> >  
> > > Maybe they should not stop on reboot in the first place, but this serie
> > > does not introduce a new behaviour.
> > 
> > okay. I can always send a follow up patch, if I care enough.
> > 
> 
> I don't understand why you assume this is not the desired behaviour and
> that the code was just copied around.

Of course this is only weak evidence: I can't think of a reason why
this would be the desired behaviour other then working around some
obscure hardware issue. Yet this code is present in nearly all the
drivers ... so looks like copy&paste. Also so far nobody has presented
a reason for this code, so looks like it is not a widely known issue.

 
> > > > More importantly however we should stop the watchdog on SYS_POWER_OFF
> > > > I think.
> > > > 
> > > My understanding here is that if the system is powered off, the watchdog
> > > will be powered off too, so there is no need to stop it.
> > 
> > This is true for many platforms, but I'm pretty sure that not all
> > platforms have a real power off. So unless you can think of a reason not
> > to stop the watchdog on power off, I think the core should do it.
> > Actually if this can't be the default, we probably need to extend
> > your code so that drivers can select the behaviour they want.
> > (Of course we would hate to do that, as power management and watchdogs
> > are pretty orthogonal subsystems and having one depend on the behaviour
> > of the other is very unfortunate.)
> 
> We have to assume that power-off means that the system will be powered
> off (...). If a platform has no real power-off, then it should be
> halted, and in that case the core would stop the watchdog to prevent a
> spurious reboot.

If on a platform without real power-off somebody at the shell
(or maybe some script) issues 'poweroff' - what code will be
sent to the notifier? Honestly I don't know, but I wouldn't rely
that it is SYS_HALT ...
 
> But maybe you're right and we should not make such distinction between
> power-off and halt, but I don't really want to make changes in drivers I
> don't know the context in which they are used.

Actually you did make changes in at least one driver: gpio_wdt.c
And this can't be argued with majority, because we can be quite confident,
that stopping the watchdog before power-off won't cause problems for any
driver, but OTOH there is no way to forsee the effect of not
stopping gpio_wdt.c ...

Actually since the gpio watchdog talks to some external hardware, we
have no way to tell what the effect of a real power-off will be on the
external hardware as it might have its own power supply that the kernel
can't control.

I can offer to prepare a kernel with some printk's and test it on
some platform where I suspect that there could be issues. But I don't
see much point in it: Even if it turns out not to be an issue on the
platform I test, that wouldn't provide much confidence for other platforms.

Harald

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via bitcoin to 1FUtd8T9jRN1rFz63vZz7s2fDtB6d6A7aS

Re: In-reply-to: <1448056496-2335-1-git-send-email-damien.riegel@savoirfairelinux.com>

[Guenter Roeck]

Harald,

On 11/25/2015 09:04 AM, Harald Geyer wrote:
> Damien Riegel writes:
>> On Mon, Nov 23, 2015 at 07:20:57PM +0100, Harald Geyer wrote:
>>> Damien Riegel writes:
>>>>> +	if (code == SYS_DOWN || code == SYS_HALT) {
>>>>>
>>>>> I think you want this to be (code == SYS_POWER_OFF || code == SYS_HALT)
>>>>> AFAIK SYS_DOWN is the code for a reboot, where the system should come
>>>>> back up immediatly, so probably we shouldn't disable the watchdog in
>>>>> this case, for the system might crash during going down.
>>>>
>>>> Well, most of the drivers (all of them but gpio) that I changed stopped
>>>> on SYS_HALT and SYS_DOWN, so they explicitely wanted to stop the
>>>> watchdog on reboot. I just factorized that in watchdog core.
>>>
>>> If they wanted to do so, or the code just got copied around, is
>>> unclear. SYS_DOWN isn't the most helpful name after all, but ...
>>>
>>>> Maybe they should not stop on reboot in the first place, but this serie
>>>> does not introduce a new behaviour.
>>>
>>> okay. I can always send a follow up patch, if I care enough.
>>>
>>
>> I don't understand why you assume this is not the desired behaviour and
>> that the code was just copied around.
>
> Of course this is only weak evidence: I can't think of a reason why
> this would be the desired behaviour other then working around some
> obscure hardware issue. Yet this code is present in nearly all the
> drivers ... so looks like copy&paste. Also so far nobody has presented

That "looks like" is just an assumption. that you can not think of a reason
doesn't mean there isn't one.

> a reason for this code, so looks like it is not a widely known issue.
>

Should or can we assume that the vast majority of watchdog driver writers
did not know what they were doing ? I do not think so. We have to assume
that they knew what they were doing, and we have to support that behavior.

We can support different functionality, but the starting point is to support
what is there today.

>
>>>>> More importantly however we should stop the watchdog on SYS_POWER_OFF
>>>>> I think.
>>>>>
>>>> My understanding here is that if the system is powered off, the watchdog
>>>> will be powered off too, so there is no need to stop it.
>>>
>>> This is true for many platforms, but I'm pretty sure that not all
>>> platforms have a real power off. So unless you can think of a reason not
>>> to stop the watchdog on power off, I think the core should do it.
>>> Actually if this can't be the default, we probably need to extend
>>> your code so that drivers can select the behaviour they want.
>>> (Of course we would hate to do that, as power management and watchdogs
>>> are pretty orthogonal subsystems and having one depend on the behaviour
>>> of the other is very unfortunate.)
>>
>> We have to assume that power-off means that the system will be powered
>> off (...). If a platform has no real power-off, then it should be
>> halted, and in that case the core would stop the watchdog to prevent a
>> spurious reboot.
>
> If on a platform without real power-off somebody at the shell
> (or maybe some script) issues 'poweroff' - what code will be
> sent to the notifier? Honestly I don't know, but I wouldn't rely
> that it is SYS_HALT ...
>
>> But maybe you're right and we should not make such distinction between
>> power-off and halt, but I don't really want to make changes in drivers I
>> don't know the context in which they are used.
>
> Actually you did make changes in at least one driver: gpio_wdt.c
> And this can't be argued with majority, because we can be quite confident,
> that stopping the watchdog before power-off won't cause problems for any
> driver, but OTOH there is no way to forsee the effect of not
> stopping gpio_wdt.c ...
>
> Actually since the gpio watchdog talks to some external hardware, we
> have no way to tell what the effect of a real power-off will be on the
> external hardware as it might have its own power supply that the kernel
> can't control.
>
> I can offer to prepare a kernel with some printk's and test it on
> some platform where I suspect that there could be issues. But I don't
> see much point in it: Even if it turns out not to be an issue on the
> platform I test, that wouldn't provide much confidence for other platforms.
>

My take is that the infrastructure should support the majority of the
existing use cases. If there are other use cases, there are two options:
Enhance the infrastructure to support more if not all use cases, or
don't use the infrastructure for the non-matching use cases.

For poweroff, one could also argue that the watchdog should _not_ stop.
Reason is that if the poweroff fails, for whatever reason, we don't want
the system to be stuck in some odd state. Keeping the watchdog running
ensures that the system comes back alive. If the poweroff request results
in the system entering the rom monitor, I would expect the rom monitor to
disable or ping the watchdog.

Similar arguments can really be made for all cases. Should the watchdog
be stopped on restart because the restart is known to take too long for
the maximum watchdog interval ? Should it be stopped on device shutdown
(which is also registered by some watchdog drivers, and called when the
system restarts) ?

In other words, one can always find arguments one way or another. Some
may be platform / architecture specific, some may be use case specific.
Maybe the behavior should or could be made configurable.

We can either start somewhere and move common code into the watchdog core,
or we can do nothing. I prefer to start somewhere and improve it as we
go along.

Thanks,
Guenter

Re: In-reply-to: <1448056496-2335-1-git-send-email-damien.riegel@savoirfairelinux.com>

[Harald Geyer]

Hi Guenter!

Guenter Roeck writes:
> On 11/25/2015 09:04 AM, Harald Geyer wrote:
> > Damien Riegel writes:
> >> On Mon, Nov 23, 2015 at 07:20:57PM +0100, Harald Geyer wrote:
> >>> Damien Riegel writes:
> >>>>> +	if (code == SYS_DOWN || code == SYS_HALT) {
> >>>>>
> >>>>> I think you want this to be (code == SYS_POWER_OFF || code == SYS_HALT)
> >>>>> AFAIK SYS_DOWN is the code for a reboot, where the system should come
> >>>>> back up immediatly, so probably we shouldn't disable the watchdog in
> >>>>> this case, for the system might crash during going down.
> >>>>
> >>>> Well, most of the drivers (all of them but gpio) that I changed stopped
> >>>> on SYS_HALT and SYS_DOWN, so they explicitely wanted to stop the
> >>>> watchdog on reboot. I just factorized that in watchdog core.
> >>>
> >>> If they wanted to do so, or the code just got copied around, is
> >>> unclear. SYS_DOWN isn't the most helpful name after all, but ...
> >>>
> >>>> Maybe they should not stop on reboot in the first place, but this serie
> >>>> does not introduce a new behaviour.
> >>>
> >>> okay. I can always send a follow up patch, if I care enough.
> >>>
> >>
> >> I don't understand why you assume this is not the desired behaviour and
> >> that the code was just copied around.
> >
> > Of course this is only weak evidence: I can't think of a reason why
> > this would be the desired behaviour other then working around some
> > obscure hardware issue. Yet this code is present in nearly all the
> > drivers ... so looks like copy&paste. Also so far nobody has presented
> 
> That "looks like" is just an assumption. that you can not think of a reason
> doesn't mean there isn't one.
> 
> > a reason for this code, so looks like it is not a widely known issue.
> >
> 
> Should or can we assume that the vast majority of watchdog driver writers
> did not know what they were doing ? I do not think so. We have to assume
> that they knew what they were doing, and we have to support that behavior.

So far nobody has come forth and said: I knew what I was doing, my
reasoning was this and that. But maybe we have just not CCed enough
people... 
but the SYS_DOWN case really is a minor issue.
 
> We can support different functionality, but the starting point is to support
> what is there today.

Yes, I have already agreed to propose a follow up patch, if I care
enough, so let's drop the SYS_DOWN down case from the discussion for now.

> >>>>> More importantly however we should stop the watchdog on SYS_POWER_OFF
> >>>>> I think.
> >>>>>
> >>>> My understanding here is that if the system is powered off, the watchdog
> >>>> will be powered off too, so there is no need to stop it.
> >>>
> >>> This is true for many platforms, but I'm pretty sure that not all
> >>> platforms have a real power off. So unless you can think of a reason not
> >>> to stop the watchdog on power off, I think the core should do it.
> >>> Actually if this can't be the default, we probably need to extend
> >>> your code so that drivers can select the behaviour they want.
> >>> (Of course we would hate to do that, as power management and watchdogs
> >>> are pretty orthogonal subsystems and having one depend on the behaviour
> >>> of the other is very unfortunate.)
> >>
> >> We have to assume that power-off means that the system will be powered
> >> off (...). If a platform has no real power-off, then it should be
> >> halted, and in that case the core would stop the watchdog to prevent a
> >> spurious reboot.
> >
> > If on a platform without real power-off somebody at the shell
> > (or maybe some script) issues 'poweroff' - what code will be
> > sent to the notifier? Honestly I don't know, but I wouldn't rely
> > that it is SYS_HALT ...
> >
> >> But maybe you're right and we should not make such distinction between
> >> power-off and halt, but I don't really want to make changes in drivers I
> >> don't know the context in which they are used.
> >
> > Actually you did make changes in at least one driver: gpio_wdt.c
> > And this can't be argued with majority, because we can be quite confident,
> > that stopping the watchdog before power-off won't cause problems for any
> > driver, but OTOH there is no way to forsee the effect of not
> > stopping gpio_wdt.c ...
> >
> > Actually since the gpio watchdog talks to some external hardware, we
> > have no way to tell what the effect of a real power-off will be on the
> > external hardware as it might have its own power supply that the kernel
> > can't control.
> >
> > I can offer to prepare a kernel with some printk's and test it on
> > some platform where I suspect that there could be issues. But I don't
> > see much point in it: Even if it turns out not to be an issue on the
> > platform I test, that wouldn't provide much confidence for other platforms.
> >
> 
> My take is that the infrastructure should support the majority of the
> existing use cases. If there are other use cases, there are two options:
> Enhance the infrastructure to support more if not all use cases, or
> don't use the infrastructure for the non-matching use cases.

I see where you are coming from, but then the gpio watchdog probably
shouldn't be moved to the core infrastructure yet. Not without an
Ack from the original author...

> For poweroff, one could also argue that the watchdog should _not_ stop.
> Reason is that if the poweroff fails, for whatever reason, we don't want
> the system to be stuck in some odd state. Keeping the watchdog running
> ensures that the system comes back alive. If the poweroff request results
> in the system entering the rom monitor, I would expect the rom monitor to
> disable or ping the watchdog.
> 
> Similar arguments can really be made for all cases. Should the watchdog
> be stopped on restart because the restart is known to take too long for
> the maximum watchdog interval ? Should it be stopped on device shutdown
> (which is also registered by some watchdog drivers, and called when the
> system restarts) ?
> 
> In other words, one can always find arguments one way or another. Some
> may be platform / architecture specific, some may be use case specific.
> Maybe the behavior should or could be made configurable.

Yes, looks like that's the outcome of this discussion.

> We can either start somewhere and move common code into the watchdog core,
> or we can do nothing. I prefer to start somewhere and improve it as we
> go along.

Sure, though we should try not to introduce any regressions during this
process.

Since my patch for the stmp3xxx won't be able to use the core infrastructure
for now, can it get picked up as is?
http://www.spinics.net/lists/linux-watchdog/msg07482.html

* it fixes a real bug
* AFAIK there is no unaddressed feedback from the Mailinglist
* we can always improve it (core infrastructure) as we go along

Thanks,
Harald

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via bitcoin to 1FUtd8T9jRN1rFz63vZz7s2fDtB6d6A7aS

Re: In-reply-to: <1448056496-2335-1-git-send-email-damien.riegel@savoirfairelinux.com>

[Guenter Roeck]

On 11/25/2015 01:56 PM, Harald Geyer wrote:
> Hi Guenter!
>
> Guenter Roeck writes:
>> On 11/25/2015 09:04 AM, Harald Geyer wrote:
>>> Damien Riegel writes:
>>>> On Mon, Nov 23, 2015 at 07:20:57PM +0100, Harald Geyer wrote:
>>>>> Damien Riegel writes:
>>>>>>> +	if (code == SYS_DOWN || code == SYS_HALT) {
>>>>>>>
>>>>>>> I think you want this to be (code == SYS_POWER_OFF || code == SYS_HALT)
>>>>>>> AFAIK SYS_DOWN is the code for a reboot, where the system should come
>>>>>>> back up immediatly, so probably we shouldn't disable the watchdog in
>>>>>>> this case, for the system might crash during going down.
>>>>>>
>>>>>> Well, most of the drivers (all of them but gpio) that I changed stopped
>>>>>> on SYS_HALT and SYS_DOWN, so they explicitely wanted to stop the
>>>>>> watchdog on reboot. I just factorized that in watchdog core.
>>>>>
>>>>> If they wanted to do so, or the code just got copied around, is
>>>>> unclear. SYS_DOWN isn't the most helpful name after all, but ...
>>>>>
>>>>>> Maybe they should not stop on reboot in the first place, but this serie
>>>>>> does not introduce a new behaviour.
>>>>>
>>>>> okay. I can always send a follow up patch, if I care enough.
>>>>>
>>>>
>>>> I don't understand why you assume this is not the desired behaviour and
>>>> that the code was just copied around.
>>>
>>> Of course this is only weak evidence: I can't think of a reason why
>>> this would be the desired behaviour other then working around some
>>> obscure hardware issue. Yet this code is present in nearly all the
>>> drivers ... so looks like copy&paste. Also so far nobody has presented
>>
>> That "looks like" is just an assumption. that you can not think of a reason
>> doesn't mean there isn't one.
>>
>>> a reason for this code, so looks like it is not a widely known issue.
>>>
>>
>> Should or can we assume that the vast majority of watchdog driver writers
>> did not know what they were doing ? I do not think so. We have to assume
>> that they knew what they were doing, and we have to support that behavior.
>
> So far nobody has come forth and said: I knew what I was doing, my
> reasoning was this and that. But maybe we have just not CCed enough
> people...
> but the SYS_DOWN case really is a minor issue.
>
>> We can support different functionality, but the starting point is to support
>> what is there today.
>
> Yes, I have already agreed to propose a follow up patch, if I care
> enough, so let's drop the SYS_DOWN down case from the discussion for now.
>
>>>>>>> More importantly however we should stop the watchdog on SYS_POWER_OFF
>>>>>>> I think.
>>>>>>>
>>>>>> My understanding here is that if the system is powered off, the watchdog
>>>>>> will be powered off too, so there is no need to stop it.
>>>>>
>>>>> This is true for many platforms, but I'm pretty sure that not all
>>>>> platforms have a real power off. So unless you can think of a reason not
>>>>> to stop the watchdog on power off, I think the core should do it.
>>>>> Actually if this can't be the default, we probably need to extend
>>>>> your code so that drivers can select the behaviour they want.
>>>>> (Of course we would hate to do that, as power management and watchdogs
>>>>> are pretty orthogonal subsystems and having one depend on the behaviour
>>>>> of the other is very unfortunate.)
>>>>
>>>> We have to assume that power-off means that the system will be powered
>>>> off (...). If a platform has no real power-off, then it should be
>>>> halted, and in that case the core would stop the watchdog to prevent a
>>>> spurious reboot.
>>>
>>> If on a platform without real power-off somebody at the shell
>>> (or maybe some script) issues 'poweroff' - what code will be
>>> sent to the notifier? Honestly I don't know, but I wouldn't rely
>>> that it is SYS_HALT ...
>>>
>>>> But maybe you're right and we should not make such distinction between
>>>> power-off and halt, but I don't really want to make changes in drivers I
>>>> don't know the context in which they are used.
>>>
>>> Actually you did make changes in at least one driver: gpio_wdt.c
>>> And this can't be argued with majority, because we can be quite confident,
>>> that stopping the watchdog before power-off won't cause problems for any
>>> driver, but OTOH there is no way to forsee the effect of not
>>> stopping gpio_wdt.c ...
>>>
>>> Actually since the gpio watchdog talks to some external hardware, we
>>> have no way to tell what the effect of a real power-off will be on the
>>> external hardware as it might have its own power supply that the kernel
>>> can't control.
>>>
>>> I can offer to prepare a kernel with some printk's and test it on
>>> some platform where I suspect that there could be issues. But I don't
>>> see much point in it: Even if it turns out not to be an issue on the
>>> platform I test, that wouldn't provide much confidence for other platforms.
>>>
>>
>> My take is that the infrastructure should support the majority of the
>> existing use cases. If there are other use cases, there are two options:
>> Enhance the infrastructure to support more if not all use cases, or
>> don't use the infrastructure for the non-matching use cases.
>
> I see where you are coming from, but then the gpio watchdog probably
> shouldn't be moved to the core infrastructure yet. Not without an
> Ack from the original author...
>
>> For poweroff, one could also argue that the watchdog should _not_ stop.
>> Reason is that if the poweroff fails, for whatever reason, we don't want
>> the system to be stuck in some odd state. Keeping the watchdog running
>> ensures that the system comes back alive. If the poweroff request results
>> in the system entering the rom monitor, I would expect the rom monitor to
>> disable or ping the watchdog.
>>
>> Similar arguments can really be made for all cases. Should the watchdog
>> be stopped on restart because the restart is known to take too long for
>> the maximum watchdog interval ? Should it be stopped on device shutdown
>> (which is also registered by some watchdog drivers, and called when the
>> system restarts) ?
>>
>> In other words, one can always find arguments one way or another. Some
>> may be platform / architecture specific, some may be use case specific.
>> Maybe the behavior should or could be made configurable.
>
> Yes, looks like that's the outcome of this discussion.
>
>> We can either start somewhere and move common code into the watchdog core,
>> or we can do nothing. I prefer to start somewhere and improve it as we
>> go along.
>
> Sure, though we should try not to introduce any regressions during this
> process.
>
> Since my patch for the stmp3xxx won't be able to use the core infrastructure
> for now, can it get picked up as is?
> http://www.spinics.net/lists/linux-watchdog/msg07482.html
>
> * it fixes a real bug

Matter of definition, I guess.

> * AFAIK there is no unaddressed feedback from the Mailinglist
> * we can always improve it (core infrastructure) as we go along
>

That got lost somehow. I see no problems with it.
I'll dig it up and send a Reviewed-by: response.

Guenter

Re: [PATCH v2 0/6] watchdog: factorize reboot notifier registration

[Wim Van Sebroeck]

Hi Damien,

> 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, do not stop the watchdog on
> SYS_DOWN. So for this watchdog, it will now stop on reboot if
> "always-running" is not set, whereas it did not prior to 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.
> 
> 
> Changes in v2:
>  - call ops->stop only if watchdog is active
>  - squashed gpio_wdt-related commits to keep a consistent behaviour.
> 
> Damien Riegel (6):
>   watchdog: core: add reboot notifier support
>   watchdog: bcm47xx_wdt: use core reboot notifier
>   watchdog: cadence_wdt: use core reboot notifier
>   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               | 37 ++++++++++++++++++++++++++
>  include/linux/bcm47xx_wdt.h                    |  2 --
>  include/linux/watchdog.h                       |  9 +++++++
>  9 files changed, 63 insertions(+), 150 deletions(-)

This patchset has been added to linux-watchdog-next.

Kind regards,
Wim.