watchdog: Add support for keepalives triggered by infrastructure

From: Guenter Roeck <[email protected]>

Introduce an optional hardware maximum timeout in the watchdog core. The hardware maximum timeout can be lower than the maximum timeout.

Drivers can set the maximum hardware timeout value in the watchdog data structure. If the configured timeout exceeds the maximum hardware timeout, the watchdog core enables a timer function to assist sending keepalive requests to the watchdog driver.

Signed-off-by Guenter Roeck <[email protected]>

---
v7:
- Rebased to v4.5-rc1
- Moved new variables to local data structure
- Dropped Uwe's Acked-by: due to substantial changes
v6:
- Set last_keepalive more accurately when starting the watchdog
- Rebased to v4.4-rc2
- Added Uwe's Acked-by:
v5:
- Rebased to v4.4-rc1
v4:
- Improved and fixed documentation
- Split hw_timeout_ms variable to timeout_ms, hw_timeout_ms for clarity
- Dropped redundant comments
- Added comments explaining failure conditions in watchdog_timeout_invalid().
- Moved the call to watchdog_update_worker() into _watchdog_ping(). v3:
- Reworked and cleaned up some of the functions.
- No longer call the worker update function if all that is needed is to stop the worker.
- max_timeout will now be ignored if max_hw_timeout_ms is provided. v2:
- Improved and hopefully clarified documentation.
- Rearranged variables in struct watchdog_device such that internal variables come last.
- The code now ensures that the watchdog times out <timeout> seconds after the most recent keepalive sent from user space.
- The internal keepalive now stops silently and no longer generates a warning message. Reason is that it will now stop early, while there may still be a substantial amount of time for keepalives from user space to arrive. If such keepalives arrive late (for example if user space is configured to send keepalives just a few seconds before the watchdog times out), the message would just be noise and not provide any value.

---
 Documentation/watchdog/watchdog-kernel-api.txt |  19 +++-
 drivers/watchdog/watchdog_dev.c                | 136 +++++++++++++++++++++++--
 include/linux/watchdog.h                       |  28 +++--
 3 files changed, 164 insertions(+), 19 deletions(-)

Documentation/watchdog/watchdog-kernel-api.txt +17 -2

index 55120a055a14..46979568b9e5 100644
@@ -52,6 +52,7 @@ struct watchdog_device {
 	unsigned int timeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
+	unsigned int max_hw_timeout_ms;
 	struct notifier_block reboot_nb;
 	struct notifier_block restart_nb;
 	void *driver_data;

@@ -73,8 +74,18 @@ It contains following fields:
   additional information about the watchdog timer itself. (Like it's unique name)
 * ops: a pointer to the list of watchdog operations that the watchdog supports.
 * timeout: the watchdog timer's timeout value (in seconds).
+  This is the time after which the system will reboot if user space does
+  not send a heartbeat request if WDOG_ACTIVE is set.
 * min_timeout: the watchdog timer's minimum timeout value (in seconds).
-* max_timeout: the watchdog timer's maximum timeout value (in seconds).
+  If set, the minimum configurable value for 'timeout'.
+* max_timeout: the watchdog timer's maximum timeout value (in seconds),
+  as seen from userspace. If set, the maximum configurable value for
+  'timeout'. Not used if max_hw_timeout_ms is non-zero.
+* max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds.
+  If set, the infrastructure will send heartbeats to the watchdog driver
+  if 'timeout' is larger than max_hw_timeout_ms, unless WDOG_ACTIVE
+  is set and userspace failed to send a heartbeat for at least 'timeout'
+  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.

@@ -153,7 +164,11 @@ they are supported. These optional routines/operations are:
   and -EIO for "could not write value to the watchdog". On success this
   routine should set the timeout value of the watchdog_device to the
   achieved timeout value (which may be different from the requested one
-  because the watchdog does not necessarily has a 1 second resolution).
+  because the watchdog does not necessarily have a 1 second resolution).
+  Drivers implementing max_hw_timeout_ms set the hardware watchdog timeout
+  to the minimum of timeout and max_hw_timeout_ms. Those drivers set the
+  timeout value of the watchdog_device either to the requested timeout value
+  (if it is larger than max_hw_timeout_ms), or to the achieved timeout value.
   (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
   watchdog's info structure).
 * get_timeleft: this routines returns the time that's left before a reset.

drivers/watchdog/watchdog_dev.c +127 -9

index ba2ecce4aae6..20e4ce0ebf6c 100644
@@ -36,6 +36,7 @@
 #include <linux/errno.h>	/* For the -ENODEV/... values */
 #include <linux/fs.h>		/* For file operations */
 #include <linux/init.h>		/* For __init/__exit/... */
+#include <linux/jiffies.h>	/* For timeout functions */
 #include <linux/kernel.h>	/* For printk/panic/... */
 #include <linux/kref.h>		/* For data references */
 #include <linux/miscdevice.h>	/* For handling misc devices */

@@ -44,6 +45,7 @@
 #include <linux/slab.h>		/* For memory functions */
 #include <linux/types.h>	/* For standard types (like size_t) */
 #include <linux/watchdog.h>	/* For watchdog specific items */
+#include <linux/workqueue.h>	/* For workqueue */
 #include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
 
 #include "watchdog_core.h"

@@ -61,6 +63,8 @@ struct watchdog_core_data {
 	struct cdev cdev;
 	struct watchdog_device *wdd;
 	struct mutex lock;
+	unsigned long last_keepalive;
+	struct delayed_work work;
 	unsigned long status;		/* Internal status bits */
 #define _WDOG_DEV_OPEN		0	/* Opened ? */
 #define _WDOG_ALLOW_RELEASE	1	/* Did we receive the magic char ? */

@@ -71,6 +75,77 @@ static dev_t watchdog_devt;
 /* Reference to watchdog device behind /dev/watchdog */
 static struct watchdog_core_data *old_wd_data;
 
+static struct workqueue_struct *watchdog_wq;
+
+static inline bool watchdog_need_worker(struct watchdog_device *wdd)
+{
+	/* All variables in milli-seconds */
+	unsigned int hm = wdd->max_hw_timeout_ms;
+	unsigned int t = wdd->timeout * 1000;
+
+	/*
+	 * A worker to generate heartbeat requests is needed if all of the
+	 * following conditions are true.
+	 * - Userspace activated the watchdog.
+	 * - The driver provided a value for the maximum hardware timeout, and
+	 *   thus is aware that the framework supports generating heartbeat
+	 *   requests.
+	 * - Userspace requests a longer timeout than the hardware can handle.
+	 */
+	return watchdog_active(wdd) && hm && t > hm;
+}
+
+static long watchdog_next_keepalive(struct watchdog_device *wdd)
+{
+	struct watchdog_core_data *wd_data = wdd->wd_data;
+	unsigned int timeout_ms = wdd->timeout * 1000;
+	unsigned long keepalive_interval;
+	unsigned long last_heartbeat;
+	unsigned long virt_timeout;
+	unsigned int hw_timeout_ms;
+
+	virt_timeout = wd_data->last_keepalive + msecs_to_jiffies(timeout_ms);
+	hw_timeout_ms = min(timeout_ms, wdd->max_hw_timeout_ms);
+	keepalive_interval = msecs_to_jiffies(hw_timeout_ms / 2);
+
+	/*
+	 * To ensure that the watchdog times out wdd->timeout seconds
+	 * after the most recent ping from userspace, the last
+	 * worker ping has to come in hw_timeout_ms before this timeout.
+	 */
+	last_heartbeat = virt_timeout - msecs_to_jiffies(hw_timeout_ms);
+	return min_t(long, last_heartbeat - jiffies, keepalive_interval);
+}
+
+static inline void watchdog_update_worker(struct watchdog_device *wdd,
+					  bool cancel)
+{
+	struct watchdog_core_data *wd_data = wdd->wd_data;
+
+	if (watchdog_need_worker(wdd)) {
+		long t = watchdog_next_keepalive(wdd);
+
+		if (t > 0)
+			mod_delayed_work(watchdog_wq, &wd_data->work, t);
+	} else if (cancel) {
+		cancel_delayed_work(&wd_data->work);
+	}
+}
+
+static int __watchdog_ping(struct watchdog_device *wdd)
+{
+	int err;
+
+	if (wdd->ops->ping)
+		err = wdd->ops->ping(wdd);  /* ping the watchdog */
+	else
+		err = wdd->ops->start(wdd); /* restart watchdog */
+
+	watchdog_update_worker(wdd, false);
+
+	return err;
+}
+
 /*
  *	watchdog_ping: ping the watchdog.
  *	@wdd: the watchdog device to ping

@@ -85,17 +160,28 @@ static struct watchdog_core_data *old_wd_data;
 
 static int watchdog_ping(struct watchdog_device *wdd)
 {
-	int err;
+	struct watchdog_core_data *wd_data = wdd->wd_data;
 
 	if (!watchdog_active(wdd))
 		return 0;
 
-	if (wdd->ops->ping)
-		err = wdd->ops->ping(wdd);	/* ping the watchdog */
-	else
-		err = wdd->ops->start(wdd);	/* restart watchdog */
+	wd_data->last_keepalive = jiffies;
+	return __watchdog_ping(wdd);
+}
 
-	return err;
+static void watchdog_ping_work(struct work_struct *work)
+{
+	struct watchdog_core_data *wd_data;
+	struct watchdog_device *wdd;
+
+	wd_data = container_of(to_delayed_work(work), struct watchdog_core_data,
+			       work);
+
+	mutex_lock(&wd_data->lock);
+	wdd = wd_data->wdd;
+	if (wdd && watchdog_active(wdd))
+		__watchdog_ping(wdd);
+	mutex_unlock(&wd_data->lock);
 }
 
 /*

@@ -111,14 +197,20 @@ static int watchdog_ping(struct watchdog_device *wdd)
 
 static int watchdog_start(struct watchdog_device *wdd)
 {
+	struct watchdog_core_data *wd_data = wdd->wd_data;
+	unsigned long started_at;
 	int err;
 
 	if (watchdog_active(wdd))
 		return 0;
 
+	started_at = jiffies;
 	err = wdd->ops->start(wdd);
-	if (err == 0)
+	if (err == 0) {
 		set_bit(WDOG_ACTIVE, &wdd->status);
+		wd_data->last_keepalive = started_at;
+		watchdog_update_worker(wdd, true);
+	}
 
 	return err;
 }

@@ -137,6 +229,7 @@ static int watchdog_start(struct watchdog_device *wdd)
 
 static int watchdog_stop(struct watchdog_device *wdd)
 {
+	struct watchdog_core_data *wd_data = wdd->wd_data;
 	int err;
 
 	if (!watchdog_active(wdd))

@@ -149,8 +242,10 @@ static int watchdog_stop(struct watchdog_device *wdd)
 	}
 
 	err = wdd->ops->stop(wdd);
-	if (err == 0)
+	if (err == 0) {
 		clear_bit(WDOG_ACTIVE, &wdd->status);
+		cancel_delayed_work(&wd_data->work);
+	}
 
 	return err;
 }

@@ -183,13 +278,19 @@ static unsigned int watchdog_get_status(struct watchdog_device *wdd)
 static int watchdog_set_timeout(struct watchdog_device *wdd,
 							unsigned int timeout)
 {
+	int err;
+
 	if (!wdd->ops->set_timeout || !(wdd->info->options & WDIOF_SETTIMEOUT))
 		return -EOPNOTSUPP;
 
 	if (watchdog_timeout_invalid(wdd, timeout))
 		return -EINVAL;
 
-	return wdd->ops->set_timeout(wdd, timeout);
+	err = wdd->ops->set_timeout(wdd, timeout);
+
+	watchdog_update_worker(wdd, true);
+
+	return err;
 }
 
 /*

@@ -609,6 +710,8 @@ static int watchdog_release(struct inode *inode, struct file *file)
 		watchdog_ping(wdd);
 	}
 
+	cancel_delayed_work_sync(&wd_data->work);
+
 	/* make sure that /dev/watchdog can be re-opened */
 	clear_bit(_WDOG_DEV_OPEN, &wd_data->status);
 

@@ -658,6 +761,11 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
 	wd_data->wdd = wdd;
 	wdd->wd_data = wd_data;
 
+	if (!watchdog_wq)
+		return -ENODEV;
+
+	INIT_DELAYED_WORK(&wd_data->work, watchdog_ping_work);
+
 	if (wdd->id == 0) {
 		old_wd_data = wd_data;
 		watchdog_miscdev.parent = wdd->parent;

@@ -715,6 +823,8 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
 	wdd->wd_data = NULL;
 	mutex_unlock(&wd_data->lock);
 
+	cancel_delayed_work_sync(&wd_data->work);
+
 	kref_put(&wd_data->kref, watchdog_core_data_release);
 }
 

@@ -780,6 +890,13 @@ int __init watchdog_dev_init(void)
 {
 	int err;
 
+	watchdog_wq = alloc_workqueue("watchdogd",
+				      WQ_HIGHPRI | WQ_MEM_RECLAIM, 0);
+	if (!watchdog_wq) {
+		pr_err("Failed to create watchdog workqueue\n");
+		return -ENOMEM;
+	}
+
 	err = class_register(&watchdog_class);
 	if (err < 0) {
 		pr_err("couldn't register class\n");

@@ -806,4 +923,5 @@ void __exit watchdog_dev_exit(void)
 {
 	unregister_chrdev_region(watchdog_devt, MAX_DOGS);
 	class_unregister(&watchdog_class);
+	destroy_workqueue(watchdog_wq);
 }

include/linux/watchdog.h +20 -8

index b585fa2507ee..cd5e6f84bf2f 100644
@@ -10,8 +10,9 @@
 
 
 #include <linux/bitops.h>
-#include <linux/device.h>
 #include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
 #include <linux/notifier.h>
 #include <uapi/linux/watchdog.h>
 

@@ -61,14 +62,19 @@ struct watchdog_ops {
  * @bootstatus:	Status of the watchdog device at boot.
  * @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).
+ * @max_timeout:The watchdog devices maximum timeout value (in seconds)
+ *		as configurable from user space. Only relevant if
+ *		max_hw_timeout_ms is not provided.
+ * @max_hw_timeout_ms:
+ *		Hardware limit for maximum timeout, in milli-seconds.
+ *		Replaces max_timeout if specified.
  * @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.
  * @wd_data:	Pointer to watchdog core internal data.
  * @status:	Field that contains the devices internal status bits.
- * @deferred: entry in wtd_deferred_reg_list which is used to
- *			   register early initialized watchdogs.
+ * @deferred:	Entry in wtd_deferred_reg_list which is used to
+ *		register early initialized watchdogs.
  *
  * The watchdog_device structure contains all information about a
  * watchdog timer device.

@@ -89,6 +95,7 @@ struct watchdog_device {
 	unsigned int timeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
+	unsigned int max_hw_timeout_ms;
 	struct notifier_block reboot_nb;
 	struct notifier_block restart_nb;
 	void *driver_data;

@@ -128,13 +135,18 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
 {
 	/*
 	 * The timeout is invalid if
+	 * - the requested value is larger than UINT_MAX / 1000
+	 *   (since internal calculations are done in milli-seconds),
+	 * or
 	 * - the requested value is smaller than the configured minimum timeout,
 	 * or
-	 * - a maximum timeout is configured, and the requested value is larger
-	 *   than the maximum timeout.
+	 * - a maximum hardware timeout is not configured, a maximum timeout
+	 *   is configured, and the requested value is larger than the
+	 *   configured maximum timeout.
 	 */
-	return t < wdd->min_timeout ||
-		(wdd->max_timeout && t > wdd->max_timeout);
+	return t > UINT_MAX / 1000 || t < wdd->min_timeout ||
+		(!wdd->max_hw_timeout_ms && wdd->max_timeout &&
+		 t > wdd->max_timeout);
 }
 
 /* Use the following functions to manipulate watchdog driver specific data */

Signature · 1 line

2.1.4

Hi Guenter,

Code is OK, but I still have one last remark:
When I coded the generic watchdog framework, I used the following terminology:
* timeout for userspace timeout's
* heartbeat for the internal hardware timeout.
I would like us to stick to this, so that the story keeps being clear and consistent.

So the hardware maximum timeout where you talk about is actually a maximum heartbeat value. Can you change this in v8?

And then directly fold the drop of the 'cancel' parameter in the same patch?

I also like to have "Make set_timeout function optional" as the first patch of the series. This patch can be used even without the other patches.

I also like the WDOG_HW_RUNNING flag. But I would split the patch that adds this into 2 patches:
* a patch that adds the WDOG_HW_RUNNING flag
* and a patch that makes the stop function optional.

Thanks in advance,
Wim.

Quoted 423 lines

From: Guenter Roeck <[email protected]>

Introduce an optional hardware maximum timeout in the watchdog core. The hardware maximum timeout can be lower than the maximum timeout.

Drivers can set the maximum hardware timeout value in the watchdog data structure. If the configured timeout exceeds the maximum hardware timeout, the watchdog core enables a timer function to assist sending keepalive requests to the watchdog driver.

Signed-off-by Guenter Roeck <[email protected]>

---
v7:
- Rebased to v4.5-rc1
- Moved new variables to local data structure
- Dropped Uwe's Acked-by: due to substantial changes
v6:
- Set last_keepalive more accurately when starting the watchdog
- Rebased to v4.4-rc2
- Added Uwe's Acked-by:
v5:
- Rebased to v4.4-rc1
v4:
- Improved and fixed documentation
- Split hw_timeout_ms variable to timeout_ms, hw_timeout_ms for clarity
- Dropped redundant comments
- Added comments explaining failure conditions in watchdog_timeout_invalid().
- Moved the call to watchdog_update_worker() into _watchdog_ping(). v3:
- Reworked and cleaned up some of the functions.
- No longer call the worker update function if all that is needed is to stop the worker.
- max_timeout will now be ignored if max_hw_timeout_ms is provided. v2:
- Improved and hopefully clarified documentation.
- Rearranged variables in struct watchdog_device such that internal variables come last.
- The code now ensures that the watchdog times out <timeout> seconds after the most recent keepalive sent from user space.
- The internal keepalive now stops silently and no longer generates a warning message. Reason is that it will now stop early, while there may still be a substantial amount of time for keepalives from user space to arrive. If such keepalives arrive late (for example if user space is configured to send keepalives just a few seconds before the watchdog times out), the message would just be noise and not provide any value.

---
  Documentation/watchdog/watchdog-kernel-api.txt |  19 +++-
  drivers/watchdog/watchdog_dev.c                | 136 +++++++++++++++++++++++--
  include/linux/watchdog.h                       |  28 +++--
  3 files changed, 164 insertions(+), 19 deletions(-)

Documentation/watchdog/watchdog-kernel-api.txt +17 -2

index 55120a055a14..46979568b9e5 100644
@@ -52,6 +52,7 @@ struct watchdog_device {
  	unsigned int timeout;
  	unsigned int min_timeout;
  	unsigned int max_timeout;
+	unsigned int max_hw_timeout_ms;
  	struct notifier_block reboot_nb;
  	struct notifier_block restart_nb;
  	void *driver_data;

@@ -73,8 +74,18 @@ It contains following fields:
    additional information about the watchdog timer itself. (Like it's unique name)
  * ops: a pointer to the list of watchdog operations that the watchdog supports.
  * timeout: the watchdog timer's timeout value (in seconds).
+  This is the time after which the system will reboot if user space does
+  not send a heartbeat request if WDOG_ACTIVE is set.
  * min_timeout: the watchdog timer's minimum timeout value (in seconds).
-* max_timeout: the watchdog timer's maximum timeout value (in seconds).
+  If set, the minimum configurable value for 'timeout'.
+* max_timeout: the watchdog timer's maximum timeout value (in seconds),
+  as seen from userspace. If set, the maximum configurable value for
+  'timeout'. Not used if max_hw_timeout_ms is non-zero.
+* max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds.
+  If set, the infrastructure will send heartbeats to the watchdog driver
+  if 'timeout' is larger than max_hw_timeout_ms, unless WDOG_ACTIVE
+  is set and userspace failed to send a heartbeat for at least 'timeout'
+  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.

@@ -153,7 +164,11 @@ they are supported. These optional routines/operations are:
    and -EIO for "could not write value to the watchdog". On success this
    routine should set the timeout value of the watchdog_device to the
    achieved timeout value (which may be different from the requested one
-  because the watchdog does not necessarily has a 1 second resolution).
+  because the watchdog does not necessarily have a 1 second resolution).
+  Drivers implementing max_hw_timeout_ms set the hardware watchdog timeout
+  to the minimum of timeout and max_hw_timeout_ms. Those drivers set the
+  timeout value of the watchdog_device either to the requested timeout value
+  (if it is larger than max_hw_timeout_ms), or to the achieved timeout value.
    (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
    watchdog's info structure).
  * get_timeleft: this routines returns the time that's left before a reset.

drivers/watchdog/watchdog_dev.c +127 -9

index ba2ecce4aae6..20e4ce0ebf6c 100644
@@ -36,6 +36,7 @@
  #include <linux/errno.h>	/* For the -ENODEV/... values */
  #include <linux/fs.h>		/* For file operations */
  #include <linux/init.h>		/* For __init/__exit/... */
+#include <linux/jiffies.h>	/* For timeout functions */
  #include <linux/kernel.h>	/* For printk/panic/... */
  #include <linux/kref.h>		/* For data references */
  #include <linux/miscdevice.h>	/* For handling misc devices */

@@ -44,6 +45,7 @@
  #include <linux/slab.h>		/* For memory functions */
  #include <linux/types.h>	/* For standard types (like size_t) */
  #include <linux/watchdog.h>	/* For watchdog specific items */
+#include <linux/workqueue.h>	/* For workqueue */
  #include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
 
  #include "watchdog_core.h"

@@ -61,6 +63,8 @@ struct watchdog_core_data {
  	struct cdev cdev;
  	struct watchdog_device *wdd;
  	struct mutex lock;
+	unsigned long last_keepalive;
+	struct delayed_work work;
  	unsigned long status;		/* Internal status bits */
  #define _WDOG_DEV_OPEN		0	/* Opened ? */
  #define _WDOG_ALLOW_RELEASE	1	/* Did we receive the magic char ? */

@@ -71,6 +75,77 @@ static dev_t watchdog_devt;
  /* Reference to watchdog device behind /dev/watchdog */
  static struct watchdog_core_data *old_wd_data;
 
+static struct workqueue_struct *watchdog_wq;
+
+static inline bool watchdog_need_worker(struct watchdog_device *wdd)
+{
+	/* All variables in milli-seconds */
+	unsigned int hm = wdd->max_hw_timeout_ms;
+	unsigned int t = wdd->timeout * 1000;
+
+	/*
+	 * A worker to generate heartbeat requests is needed if all of the
+	 * following conditions are true.
+	 * - Userspace activated the watchdog.
+	 * - The driver provided a value for the maximum hardware timeout, and
+	 *   thus is aware that the framework supports generating heartbeat
+	 *   requests.
+	 * - Userspace requests a longer timeout than the hardware can handle.
+	 */
+	return watchdog_active(wdd) && hm && t > hm;
+}
+
+static long watchdog_next_keepalive(struct watchdog_device *wdd)
+{
+	struct watchdog_core_data *wd_data = wdd->wd_data;
+	unsigned int timeout_ms = wdd->timeout * 1000;
+	unsigned long keepalive_interval;
+	unsigned long last_heartbeat;
+	unsigned long virt_timeout;
+	unsigned int hw_timeout_ms;
+
+	virt_timeout = wd_data->last_keepalive + msecs_to_jiffies(timeout_ms);
+	hw_timeout_ms = min(timeout_ms, wdd->max_hw_timeout_ms);
+	keepalive_interval = msecs_to_jiffies(hw_timeout_ms / 2);
+
+	/*
+	 * To ensure that the watchdog times out wdd->timeout seconds
+	 * after the most recent ping from userspace, the last
+	 * worker ping has to come in hw_timeout_ms before this timeout.
+	 */
+	last_heartbeat = virt_timeout - msecs_to_jiffies(hw_timeout_ms);
+	return min_t(long, last_heartbeat - jiffies, keepalive_interval);
+}
+
+static inline void watchdog_update_worker(struct watchdog_device *wdd,
+					  bool cancel)
+{
+	struct watchdog_core_data *wd_data = wdd->wd_data;
+
+	if (watchdog_need_worker(wdd)) {
+		long t = watchdog_next_keepalive(wdd);
+
+		if (t > 0)
+			mod_delayed_work(watchdog_wq, &wd_data->work, t);
+	} else if (cancel) {
+		cancel_delayed_work(&wd_data->work);
+	}
+}
+
+static int __watchdog_ping(struct watchdog_device *wdd)
+{
+	int err;
+
+	if (wdd->ops->ping)
+		err = wdd->ops->ping(wdd);  /* ping the watchdog */
+	else
+		err = wdd->ops->start(wdd); /* restart watchdog */
+
+	watchdog_update_worker(wdd, false);
+
+	return err;
+}
+
  /*
   *	watchdog_ping: ping the watchdog.
   *	@wdd: the watchdog device to ping

@@ -85,17 +160,28 @@ static struct watchdog_core_data *old_wd_data;
 
  static int watchdog_ping(struct watchdog_device *wdd)
  {
-	int err;
+	struct watchdog_core_data *wd_data = wdd->wd_data;
 
  	if (!watchdog_active(wdd))
  		return 0;
 
-	if (wdd->ops->ping)
-		err = wdd->ops->ping(wdd);	/* ping the watchdog */
-	else
-		err = wdd->ops->start(wdd);	/* restart watchdog */
+	wd_data->last_keepalive = jiffies;
+	return __watchdog_ping(wdd);
+}
 
-	return err;
+static void watchdog_ping_work(struct work_struct *work)
+{
+	struct watchdog_core_data *wd_data;
+	struct watchdog_device *wdd;
+
+	wd_data = container_of(to_delayed_work(work), struct watchdog_core_data,
+			       work);
+
+	mutex_lock(&wd_data->lock);
+	wdd = wd_data->wdd;
+	if (wdd && watchdog_active(wdd))
+		__watchdog_ping(wdd);
+	mutex_unlock(&wd_data->lock);
  }
 
  /*

@@ -111,14 +197,20 @@ static int watchdog_ping(struct watchdog_device *wdd)
 
  static int watchdog_start(struct watchdog_device *wdd)
  {
+	struct watchdog_core_data *wd_data = wdd->wd_data;
+	unsigned long started_at;
  	int err;
 
  	if (watchdog_active(wdd))
  		return 0;
 
+	started_at = jiffies;
  	err = wdd->ops->start(wdd);
-	if (err == 0)
+	if (err == 0) {
  		set_bit(WDOG_ACTIVE, &wdd->status);
+		wd_data->last_keepalive = started_at;
+		watchdog_update_worker(wdd, true);
+	}
 
  	return err;
  }

@@ -137,6 +229,7 @@ static int watchdog_start(struct watchdog_device *wdd)
 
  static int watchdog_stop(struct watchdog_device *wdd)
  {
+	struct watchdog_core_data *wd_data = wdd->wd_data;
  	int err;
 
  	if (!watchdog_active(wdd))

@@ -149,8 +242,10 @@ static int watchdog_stop(struct watchdog_device *wdd)
  	}
 
  	err = wdd->ops->stop(wdd);
-	if (err == 0)
+	if (err == 0) {
  		clear_bit(WDOG_ACTIVE, &wdd->status);
+		cancel_delayed_work(&wd_data->work);
+	}
 
  	return err;
  }

@@ -183,13 +278,19 @@ static unsigned int watchdog_get_status(struct watchdog_device *wdd)
  static int watchdog_set_timeout(struct watchdog_device *wdd,
  							unsigned int timeout)
  {
+	int err;
+
  	if (!wdd->ops->set_timeout || !(wdd->info->options & WDIOF_SETTIMEOUT))
  		return -EOPNOTSUPP;
 
  	if (watchdog_timeout_invalid(wdd, timeout))
  		return -EINVAL;
 
-	return wdd->ops->set_timeout(wdd, timeout);
+	err = wdd->ops->set_timeout(wdd, timeout);
+
+	watchdog_update_worker(wdd, true);
+
+	return err;
  }
 
  /*

@@ -609,6 +710,8 @@ static int watchdog_release(struct inode *inode, struct file *file)
  		watchdog_ping(wdd);
  	}
 
+	cancel_delayed_work_sync(&wd_data->work);
+
  	/* make sure that /dev/watchdog can be re-opened */
  	clear_bit(_WDOG_DEV_OPEN, &wd_data->status);
 

@@ -658,6 +761,11 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
  	wd_data->wdd = wdd;
  	wdd->wd_data = wd_data;
 
+	if (!watchdog_wq)
+		return -ENODEV;
+
+	INIT_DELAYED_WORK(&wd_data->work, watchdog_ping_work);
+
  	if (wdd->id == 0) {
  		old_wd_data = wd_data;
  		watchdog_miscdev.parent = wdd->parent;

@@ -715,6 +823,8 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
  	wdd->wd_data = NULL;
  	mutex_unlock(&wd_data->lock);
 
+	cancel_delayed_work_sync(&wd_data->work);
+
  	kref_put(&wd_data->kref, watchdog_core_data_release);
  }
 

@@ -780,6 +890,13 @@ int __init watchdog_dev_init(void)
  {
  	int err;
 
+	watchdog_wq = alloc_workqueue("watchdogd",
+				      WQ_HIGHPRI | WQ_MEM_RECLAIM, 0);
+	if (!watchdog_wq) {
+		pr_err("Failed to create watchdog workqueue\n");
+		return -ENOMEM;
+	}
+
  	err = class_register(&watchdog_class);
  	if (err < 0) {
  		pr_err("couldn't register class\n");

@@ -806,4 +923,5 @@ void __exit watchdog_dev_exit(void)
  {
  	unregister_chrdev_region(watchdog_devt, MAX_DOGS);
  	class_unregister(&watchdog_class);
+	destroy_workqueue(watchdog_wq);
  }

include/linux/watchdog.h +20 -9

index b585fa2507ee..cd5e6f84bf2f 100644
@@ -10,8 +10,9 @@
 
 
  #include <linux/bitops.h>
-#include <linux/device.h>
  #include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
  #include <linux/notifier.h>
  #include <uapi/linux/watchdog.h>
 

@@ -61,14 +62,19 @@ struct watchdog_ops {
   * @bootstatus:	Status of the watchdog device at boot.
   * @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).
+ * @max_timeout:The watchdog devices maximum timeout value (in seconds)
+ *		as configurable from user space. Only relevant if
+ *		max_hw_timeout_ms is not provided.
+ * @max_hw_timeout_ms:
+ *		Hardware limit for maximum timeout, in milli-seconds.
+ *		Replaces max_timeout if specified.
   * @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.
   * @wd_data:	Pointer to watchdog core internal data.
   * @status:	Field that contains the devices internal status bits.
- * @deferred: entry in wtd_deferred_reg_list which is used to
- *			   register early initialized watchdogs.
+ * @deferred:	Entry in wtd_deferred_reg_list which is used to
+ *		register early initialized watchdogs.
   *
   * The watchdog_device structure contains all information about a
   * watchdog timer device.

@@ -89,6 +95,7 @@ struct watchdog_device {
  	unsigned int timeout;
  	unsigned int min_timeout;
  	unsigned int max_timeout;
+	unsigned int max_hw_timeout_ms;
  	struct notifier_block reboot_nb;
  	struct notifier_block restart_nb;
  	void *driver_data;

@@ -128,13 +135,18 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
  {
  	/*
  	 * The timeout is invalid if
+	 * - the requested value is larger than UINT_MAX / 1000
+	 *   (since internal calculations are done in milli-seconds),
+	 * or
  	 * - the requested value is smaller than the configured minimum timeout,
  	 * or
-	 * - a maximum timeout is configured, and the requested value is larger
-	 *   than the maximum timeout.
+	 * - a maximum hardware timeout is not configured, a maximum timeout
+	 *   is configured, and the requested value is larger than the
+	 *   configured maximum timeout.
  	 */
-	return t < wdd->min_timeout ||
-		(wdd->max_timeout && t > wdd->max_timeout);
+	return t > UINT_MAX / 1000 || t < wdd->min_timeout ||
+		(!wdd->max_hw_timeout_ms && wdd->max_timeout &&
+		 t > wdd->max_timeout);
  }
 
  /* Use the following functions to manipulate watchdog driver specific data */
--

2.1.4

From: Guenter Roeck <[email protected]>

Some watchdogs require a minimum time between heartbeats. Examples are the watchdogs in DA9062 and AT91SAM9x.

Signed-off-by Guenter Roeck <[email protected]>

---
v7: Rebased to v4.5-rc1
v6: Rebased to v4.4-rc2
v5: Rebased to v4.4-rc1
Fixed typo in documentation.
v4: Added patch

---
 Documentation/watchdog/watchdog-kernel-api.txt |  3 +++
 drivers/watchdog/watchdog_dev.c                | 15 +++++++++++++++
 include/linux/watchdog.h                       |  3 +++
 3 files changed, 21 insertions(+)

Documentation/watchdog/watchdog-kernel-api.txt +3 -0

index 62d49d6a7ff5..2221fb4f2739 100644
@@ -52,6 +52,7 @@ struct watchdog_device {
 	unsigned int timeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
+	unsigned int min_hw_heartbeat_ms;
 	unsigned int max_hw_timeout_ms;
 	struct notifier_block reboot_nb;
 	struct notifier_block restart_nb;

@@ -81,6 +82,8 @@ It contains following fields:
 * max_timeout: the watchdog timer's maximum timeout value (in seconds),
   as seen from userspace. If set, the maximum configurable value for
   'timeout'. Not used if max_hw_timeout_ms is non-zero.
+* min_hw_heartbeat_ms: Minimum time between heartbeats sent to the chip,
+  in milli-seconds.
 * max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds.
   If set, the infrastructure will send heartbeats to the watchdog driver
   if 'timeout' is larger than max_hw_timeout_ms, unless WDOG_ACTIVE

drivers/watchdog/watchdog_dev.c +4 -0

index 1a8f3861fe92..ff03cbc8e081 100644
@@ -64,6 +64,7 @@ struct watchdog_core_data {
 	struct watchdog_device *wdd;
 	struct mutex lock;
 	unsigned long last_keepalive;
+	unsigned long last_hw_keepalive;
 	struct delayed_work work;
 	unsigned long status;		/* Internal status bits */
 #define _WDOG_DEV_OPEN		0	/* Opened ? */

@@ -138,8 +139,19 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd,
 
 static int __watchdog_ping(struct watchdog_device *wdd)
 {
+	struct watchdog_core_data *wd_data = wdd->wd_data;
+	unsigned long earliest_keepalive = wd_data->last_hw_keepalive +
+				msecs_to_jiffies(wdd->min_hw_heartbeat_ms);

Hello Guenter,

Quoted 60 lines · Guenter Roeck

From: Guenter Roeck <[email protected]>

Some watchdogs require a minimum time between heartbeats. Examples are the watchdogs in DA9062 and AT91SAM9x.

Signed-off-by Guenter Roeck <[email protected]>

---
v7: Rebased to v4.5-rc1
v6: Rebased to v4.4-rc2
v5: Rebased to v4.4-rc1
Fixed typo in documentation.
v4: Added patch

---
  Documentation/watchdog/watchdog-kernel-api.txt |  3 +++
  drivers/watchdog/watchdog_dev.c                | 15 +++++++++++++++
  include/linux/watchdog.h                       |  3 +++
  3 files changed, 21 insertions(+)

Documentation/watchdog/watchdog-kernel-api.txt +3 -0

index 62d49d6a7ff5..2221fb4f2739 100644
@@ -52,6 +52,7 @@ struct watchdog_device {
  	unsigned int timeout;
  	unsigned int min_timeout;
  	unsigned int max_timeout;
+	unsigned int min_hw_heartbeat_ms;
  	unsigned int max_hw_timeout_ms;
  	struct notifier_block reboot_nb;
  	struct notifier_block restart_nb;

@@ -81,6 +82,8 @@ It contains following fields:
  * max_timeout: the watchdog timer's maximum timeout value (in seconds),
    as seen from userspace. If set, the maximum configurable value for
    'timeout'. Not used if max_hw_timeout_ms is non-zero.
+* min_hw_heartbeat_ms: Minimum time between heartbeats sent to the chip,
+  in milli-seconds.
  * max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds.
    If set, the infrastructure will send heartbeats to the watchdog driver
    if 'timeout' is larger than max_hw_timeout_ms, unless WDOG_ACTIVE

drivers/watchdog/watchdog_dev.c +4 -0

index 1a8f3861fe92..ff03cbc8e081 100644
@@ -64,6 +64,7 @@ struct watchdog_core_data {
  	struct watchdog_device *wdd;
  	struct mutex lock;
  	unsigned long last_keepalive;
+	unsigned long last_hw_keepalive;
  	struct delayed_work work;
  	unsigned long status;		/* Internal status bits */
  #define _WDOG_DEV_OPEN		0	/* Opened ? */

@@ -138,8 +139,19 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd,
 
  static int __watchdog_ping(struct watchdog_device *wdd)
  {
+	struct watchdog_core_data *wd_data = wdd->wd_data;
+	unsigned long earliest_keepalive = wd_data->last_hw_keepalive +
+				msecs_to_jiffies(wdd->min_hw_heartbeat_ms);
  	int err;
 

drivers/watchdog/watchdog_dev.c static inline void watchdog_update_worker(struct watchdog_device *wdd, · line 154

Quoted 8 lines

drivers/watchdog/watchdog_dev.c +8 -0

+	if (time_is_after_jiffies(earliest_keepalive)) {
+		mod_delayed_work(watchdog_wq, &wd_data->work,
+				 earliest_keepalive - jiffies);
+		return 0;
+	}
+
+	wd_data->last_hw_keepalive = jiffies;
+

Do you need to undo this assignment when ping fails?

From: Guenter Roeck <[email protected]>

The watchdog infrastructure now supports handling watchdog keepalive if the watchdog is running while the watchdog device is closed. Convert the driver to use this infrastructure.

Signed-off-by Guenter Roeck <[email protected]>

---
v7: Set max_hw_timeout_ms

     Rebased to v4.5-rc1
v6: Rename WDOG_RUNNING to WDOG_HW_RUNNING
     Rebased to v4.4-rc2

v5: Rebased to v4.4-rc1
v4: No changes
v3: No changes
v2: No changes

---
  drivers/watchdog/imx2_wdt.c | 74 ++++++++-------------------------------------
  1 file changed, 13 insertions(+), 61 deletions(-)

drivers/watchdog/imx2_wdt.c +13 -61

index e47966aa2db0..1a48cb28718f 100644
@@ -25,14 +25,12 @@
  #include <linux/delay.h>
  #include <linux/init.h>
  #include <linux/io.h>
-#include <linux/jiffies.h>
  #include <linux/kernel.h>
  #include <linux/module.h>
  #include <linux/moduleparam.h>
  #include <linux/of_address.h>
  #include <linux/platform_device.h>
  #include <linux/regmap.h>
-#include <linux/timer.h>
  #include <linux/watchdog.h>
 
  #define DRIVER_NAME "imx2-wdt"

@@ -60,7 +58,6 @@
  struct imx2_wdt_device {
  	struct clk *clk;
  	struct regmap *regmap;
-	struct timer_list timer;	/* Pings the watchdog when closed */
  	struct watchdog_device wdog;
  };
 

@@ -146,16 +143,6 @@ static int imx2_wdt_ping(struct watchdog_device *wdog)
  	return 0;
  }
 
-static void imx2_wdt_timer_ping(unsigned long arg)
-{
-	struct watchdog_device *wdog = (struct watchdog_device *)arg;
-	struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
-
-	/* ping it every wdog->timeout / 2 seconds to prevent reboot */
-	imx2_wdt_ping(wdog);
-	mod_timer(&wdev->timer, jiffies + wdog->timeout * HZ / 2);
-}
-
  static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
  				unsigned int new_timeout)
  {

@@ -172,40 +159,19 @@ static int imx2_wdt_start(struct watchdog_device *wdog)
  {
  	struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
 
-	if (imx2_wdt_is_running(wdev)) {
-		/* delete the timer that pings the watchdog after close */
-		del_timer_sync(&wdev->timer);
+	if (imx2_wdt_is_running(wdev))
  		imx2_wdt_set_timeout(wdog, wdog->timeout);
-	} else
+	else
  		imx2_wdt_setup(wdog);
 
-	return imx2_wdt_ping(wdog);
-}
-
-static int imx2_wdt_stop(struct watchdog_device *wdog)
-{
-	/*
-	 * We don't need a clk_disable, it cannot be disabled once started.
-	 * We use a timer to ping the watchdog while /dev/watchdog is closed
-	 */
-	imx2_wdt_timer_ping((unsigned long)wdog);
-	return 0;
-}
-
-static inline void imx2_wdt_ping_if_active(struct watchdog_device *wdog)
-{
-	struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
+	set_bit(WDOG_HW_RUNNING, &wdog->status);
 
-	if (imx2_wdt_is_running(wdev)) {
-		imx2_wdt_set_timeout(wdog, wdog->timeout);
-		imx2_wdt_timer_ping((unsigned long)wdog);
-	}
+	return imx2_wdt_ping(wdog);
  }
 
  static const struct watchdog_ops imx2_wdt_ops = {
  	.owner = THIS_MODULE,
  	.start = imx2_wdt_start,
-	.stop = imx2_wdt_stop,
  	.ping = imx2_wdt_ping,
  	.set_timeout = imx2_wdt_set_timeout,
  	.restart = imx2_wdt_restart,

@@ -253,7 +219,7 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
  	wdog->info		= &imx2_wdt_info;
  	wdog->ops		= &imx2_wdt_ops;
  	wdog->min_timeout	= 1;
-	wdog->max_timeout	= IMX2_WDT_MAX_TIME;
+	wdog->max_hw_timeout_ms	= IMX2_WDT_MAX_TIME * 1000;
  	wdog->parent		= &pdev->dev;
 
  	ret = clk_prepare_enable(wdev->clk);

@@ -274,9 +240,10 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
  	watchdog_set_restart_priority(wdog, 128);
  	watchdog_init_timeout(wdog, timeout, &pdev->dev);
 
-	setup_timer(&wdev->timer, imx2_wdt_timer_ping, (unsigned long)wdog);
-
-	imx2_wdt_ping_if_active(wdog);
+	if (imx2_wdt_is_running(wdev)) {
+		imx2_wdt_set_timeout(wdog, wdog->timeout);
+		set_bit(WDOG_HW_RUNNING, &wdog->status);
+	}
 
  	/*
  	 * Disable the watchdog power down counter at boot. Otherwise the power

@@ -309,7 +276,6 @@ static int __exit imx2_wdt_remove(struct platform_device *pdev)
  	watchdog_unregister_device(wdog);
 
  	if (imx2_wdt_is_running(wdev)) {
-		del_timer_sync(&wdev->timer);
  		imx2_wdt_ping(wdog);
  		dev_crit(&pdev->dev, "Device removed: Expect reboot!\n");
  	}

@@ -323,10 +289,9 @@ static void imx2_wdt_shutdown(struct platform_device *pdev)
 
  	if (imx2_wdt_is_running(wdev)) {
  		/*
-		 * We are running, we need to delete the timer but will
-		 * give max timeout before reboot will take place
+		 * We are running, configure max timeout before reboot
+		 * will take place.
  		 */
-		del_timer_sync(&wdev->timer);
  		imx2_wdt_set_timeout(wdog, IMX2_WDT_MAX_TIME);
  		imx2_wdt_ping(wdog);
  		dev_crit(&pdev->dev, "Device shutdown: Expect reboot!\n");

@@ -344,10 +309,6 @@ static int imx2_wdt_suspend(struct device *dev)
  	if (imx2_wdt_is_running(wdev)) {
  		imx2_wdt_set_timeout(wdog, IMX2_WDT_MAX_TIME);
  		imx2_wdt_ping(wdog);
-
-		/* The watchdog is not active */
-		if (!watchdog_active(wdog))
-			del_timer_sync(&wdev->timer);
  	}
 
  	clk_disable_unprepare(wdev->clk);

@@ -373,19 +334,10 @@ static int imx2_wdt_resume(struct device *dev)
  		 * watchdog again.
  		 */
  		imx2_wdt_setup(wdog);
+	}
+	if (imx2_wdt_is_running(wdev)) {
  		imx2_wdt_set_timeout(wdog, wdog->timeout);
  		imx2_wdt_ping(wdog);
-	} else if (imx2_wdt_is_running(wdev)) {
-		/* Resuming from non-deep sleep state. */
-		imx2_wdt_set_timeout(wdog, wdog->timeout);
-		imx2_wdt_ping(wdog);
-		/*
-		 * But the watchdog is not active, then start
-		 * the timer again.
-		 */
-		if (!watchdog_active(wdog))
-			mod_timer(&wdev->timer,
-				  jiffies + wdog->timeout * HZ / 2);
  	}
 
  	return 0;

From: Guenter Roeck <[email protected]>

Convert driver to use watchdog infrastructure. This includes infrastructure support to handle watchdog keepalive if the watchdog is running while the watchdog device is closed.

Signed-off-by Guenter Roeck <[email protected]>

---
v7: Set max_hw_timeout_ms

    Rebased to v4.5-rc1
v6: Added patch
---
 drivers/watchdog/Kconfig  |   1 +
 drivers/watchdog/dw_wdt.c | 323 +++++++++++++++++-----------------------------
 2 files changed, 117 insertions(+), 207 deletions(-)

Guenter,

Quoted 8 lines · Guenter Roeck

From: Guenter Roeck <[email protected]>

Convert driver to use watchdog infrastructure. This includes infrastructure support to handle watchdog keepalive if the watchdog is running while the watchdog device is closed.

Signed-off-by Guenter Roeck <[email protected]>

Quoted 8 lines

---
v7: Set max_hw_timeout_ms

     Rebased to v4.5-rc1
v6: Added patch
---
  drivers/watchdog/Kconfig  |   1 +
  drivers/watchdog/dw_wdt.c | 323 +++++++++++++++++-----------------------------
  2 files changed, 117 insertions(+), 207 deletions(-)

I used the following test environment:
* Rockchip rk3288-based Chromebook
* v4.4-based kernel (specifically commit b1ba57898bfc ("veyron_defconfig: update for 4.5-rc") from Heiko Stuebner "somewhat stable" development tree
* Pulled in some USB-related fixes (just for my own testing)
* Pulled in Wim's most recent pull request for watchdog subsystem (ac36856fe432 watchdog: asm9260: remove __init and __exit annotations)
* Applied all 9 of your patches
* Test with daisydog
<https://chromium.googlesource.com/chromiumos/third_party/daisydog/>

I did these tests:

###
# Confirm normal functionality
###

stop daisydog
daisydog -c
# HW watchdog interval is 43 seconds

# /dev/watchdog reported boot status: normal-boot

cat /dev/watchdog
# cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop! # /dev/watchdog: Invalid argument

for i in $(seq 60); do
echo $i
sleep 1
done
# 1 - 43 printed, then reboot

###
# Try setting interval, confirm all good (no reboots)

# ...then confirm that reboot happens in 10 sec
###

stop daisydog
daisydog -c
# HW watchdog interval is 43 seconds

# /dev/watchdog reported boot status: normal-boot

daisydog -i 10
# HW watchdog interval is 10 seconds

# /dev/watchdog reported boot status: normal-boot

<Ctrl-C>

daisydog -c
# HW watchdog interval is 10 seconds

# /dev/watchdog reported boot status: normal-boot

sleep 60
# No reboots

cat /dev/watchdog
# cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop! # /dev/watchdog: Invalid argument

for i in $(seq 60); do
echo $i
sleep 1
done
# 1 - 11 printed, then reboot

###
# Try setting interval super long
# (outside of hardware range)
###

stop daisydog
daisydog -i 100 -c
# HW watchdog interval is 86 seconds

# /dev/watchdog reported boot status: normal-boot

cat /dev/watchdog
# cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop! # /dev/watchdog: Invalid argument

for i in $(seq 200); do
echo $i
sleep 1
done
# 1 - 87 printed, then reboot; seems right to me.

###
# Try with a interval that's in hardware range, but above default
###

stop daisydog
daisydog -i 70 -c
# HW watchdog interval is 86 seconds

# /dev/watchdog reported boot status: normal-boot

cat /dev/watchdog
# cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop! # /dev/watchdog: Invalid argument

for i in $(seq 200); do
echo $i
sleep 1
done
# 1 - 87 printed, then reboot; seems right to me

###
# Make sure my previous fix not regressed:
# 04b1a62e6bb9 ("watchdog: dw_wdt: keepalive the watchdog at write time")
###

stop daisydog
while true; do daisydog -c > /dev/null; done

# Let the above run for a 2 minutes and see no reboot.

###
# See what happens when you change things midway
###

stop daisydog
daisydog -c
# HW watchdog interval is 43 seconds

# /dev/watchdog reported boot status: normal-boot

cat /dev/watchdog
# cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop! # /dev/watchdog: Invalid argument

for i in $(seq 30); do
echo $i
sleep 1
done
# 1 - 30 printed

daisydog -c -i 10
# HW watchdog interval is 10 seconds

# /dev/watchdog reported boot status: normal-boot

sleep 60
# No reboot; right since the "-c -i 10" patted + closed properly

===

I also tried the new functionality enabled by your new patch and added "timeout-sec = <10>" to my device tree. That seemed to work. Upon bootup the watchdog defaulted to 10 seconds. I also tried setting it to 86 in the device tree with similar success.

===

The above seem to cover all the cases I can think of.

Tested-by Douglas Anderson <[email protected]>

Patch #1 adds timer functionality to the watchdog core. It solves the problem of short maximum hardware timeouts by augmenting heartbeats triggered from user space with internally triggered heartbeats.

Patch #2 adds functionality to generate heartbeats while the watchdog device is closed. It handles situation where where the watchdog is running after the driver has been instantiated, but the device is not yet opened, and post-close situations necessary if a watchdog can not be stopped.

Patch #3 makes the set_timeout function optional. This is now possible since timeout changes can now be completely handled in the watchdog core, for example if the hardware watchdog timeout is fixed.

Patch #4 adds code to ensure that the minimum time between heartbeats meets constraints provided by the watchdog driver.

Patch #5 simplifies the watchdog_update_worker() function introduced with patch #1 to only take a single argument, and to always cancel any pending work if a worker is not or no longer needed. This patch is kept as separate patch on purpose, to enable dropping or reverting it easily if it causes any problems. It should not cause any problems; this is just out of an abundance of caution.

Hello Guenter,

Quoted 16 lines · Guenter Roeck

Patch #1 adds timer functionality to the watchdog core. It solves the problem of short maximum hardware timeouts by augmenting heartbeats triggered from user space with internally triggered heartbeats.

Patch #2 adds functionality to generate heartbeats while the watchdog device is closed. It handles situation where where the watchdog is running after the driver has been instantiated, but the device is not yet opened, and post-close situations necessary if a watchdog can not be stopped.

Patch #3 makes the set_timeout function optional. This is now possible since timeout changes can now be completely handled in the watchdog core, for example if the hardware watchdog timeout is fixed.

Patch #4 adds code to ensure that the minimum time between heartbeats meets constraints provided by the watchdog driver.

Quoted 6 lines

Patch #5 simplifies the watchdog_update_worker() function introduced with patch #1 to only take a single argument, and to always cancel any pending work if a worker is not or no longer needed. This patch is kept as separate patch on purpose, to enable dropping or reverting it easily if it causes any problems. It should not cause any problems; this is just out of an abundance of caution.

Acked-by Uwe Kleine-König <[email protected]> for patches

1-5.