watchdog: Extend kernel API and add early_timeout_sec feature

There is a great deal of diversity in the watchdog hardware found on different devices. Differen hardware have different contstraints on them, many of the constraints that are excessively difficult for the user space to satisfy.

One such constraint is the length of the timeout value, which in many cases can be just a few seconds. Drivers are creating ad hoc solutions with timers and workqueues to extend the timeout in order to give user space more time between updates. Looking at the drivers it is clear that this has resulted to a lot of duplicate code.

Add an extension to the watchdog kernel API that allows the driver to describe tis HW constraints to the watchdog code. A kernel worker in the core is then used to extend the watchdog timeout on behalf of the user space. This allows the drivers to be simplified as core takes over the timer extending.

Tested-by Wenyou Yang <[email protected]>

Signed-off-by Timo Kokkonen <[email protected]>

---
 drivers/watchdog/watchdog_core.c | 101 +++++++++++++++++++++++++++++++++++++--
 drivers/watchdog/watchdog_dev.c  |  75 ++++++++++++++++++++++++++---
 include/linux/watchdog.h         |  23 +++++++++
 3 files changed, 189 insertions(+), 10 deletions(-)

drivers/watchdog/watchdog_core.c +5 -0

index cec9b55..fd12489 100644
@@ -99,6 +99,89 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 EXPORT_SYMBOL_GPL(watchdog_init_timeout);
 
 /**
+ * watchdog_init_parms() - initialize generic watchdog parameters
+ * @wdd: Watchdog device to operate
+ * @dev: Device that stores the device tree properties
+ *
+ * Initialize the generic timeout parameters. The driver needs to set

Quoted 17 lines · Timo Kokkonen

There is a great deal of diversity in the watchdog hardware found on different devices. Differen hardware have different contstraints on them, many of the constraints that are excessively difficult for the user space to satisfy.

One such constraint is the length of the timeout value, which in many cases can be just a few seconds. Drivers are creating ad hoc solutions with timers and workqueues to extend the timeout in order to give user space more time between updates. Looking at the drivers it is clear that this has resulted to a lot of duplicate code.

Add an extension to the watchdog kernel API that allows the driver to describe tis HW constraints to the watchdog code. A kernel worker in the core is then used to extend the watchdog timeout on behalf of the user space. This allows the drivers to be simplified as core takes over the timer extending.

Quoted 2 lines

Tested-by Wenyou Yang <[email protected]>

Signed-off-by Timo Kokkonen <[email protected]>

I started twice to review this series, and each time there is a new version before I can finish the review. Guess I'll wait until it settles down a bit before trying again :-(. Just a quick comment below.

Quoted 26 lines

---
  drivers/watchdog/watchdog_core.c | 101 +++++++++++++++++++++++++++++++++++++--
  drivers/watchdog/watchdog_dev.c  |  75 ++++++++++++++++++++++++++---
  include/linux/watchdog.h         |  23 +++++++++
  3 files changed, 189 insertions(+), 10 deletions(-)

drivers/watchdog/watchdog_core.c +12 -0

index cec9b55..fd12489 100644
@@ -99,6 +99,89 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
  EXPORT_SYMBOL_GPL(watchdog_init_timeout);
 
  /**
+ * watchdog_init_parms() - initialize generic watchdog parameters
+ * @wdd: Watchdog device to operate
+ * @dev: Device that stores the device tree properties
+ *
+ * Initialize the generic timeout parameters. The driver needs to set
+ * hw_features bitmask from @wdd prior calling this function in order
+ * to ensure the core knows how to handle the HW.
+ *
+ * A zero is returned on success and -EINVAL for failure.
+ */
+int watchdog_init_params(struct watchdog_device *wdd, struct device *dev)
+{

drivers/watchdog/watchdog_core.c int watchdog_init_timeout(struct watchdog_device *wdd, · line 116

Quoted 3 lines

drivers/watchdog/watchdog_core.c +3 -0

+	int ret = 0;
+
+	ret = watchdog_init_timeout(wdd, wdd->timeout, dev);

You are changing the semantics of watchdog_init_timeout here; for all practical purposes it no longer accepts the timeout passed as parameter, but expects the timeout to be configured in wdd->timeout instead. Please don't do that.

There is a great deal of diversity in the watchdog hardware found on different devices. Differen hardware have different contstraints on

them, many of the constraints that are excessively difficult for the user space to satisfy.

One such constraint is the length of the timeout value, which in many cases can be just a few seconds. Drivers are creating ad hoc solutions with timers and workqueues to extend the timeout in order to give user space more time between updates. Looking at the drivers it is clear that this has resulted to a lot of duplicate code.

Add an extension to the watchdog kernel API that allows the driver to describe tis HW constraints to the watchdog code. A kernel worker in

the core is then used to extend the watchdog timeout on behalf of the user space. This allows the drivers to be simplified as core takes over the timer extending.

Tested-by Wenyou Yang <[email protected]>

Signed-off-by Timo Kokkonen <[email protected]>

---
 drivers/watchdog/watchdog_core.c | 101 +++++++++++++++++++++++++++++++++++++--
 drivers/watchdog/watchdog_dev.c  |  75 ++++++++++++++++++++++++++---
 include/linux/watchdog.h         |  23 +++++++++
 3 files changed, 189 insertions(+), 10 deletions(-)

drivers/watchdog/watchdog_core.c +7 -0

index cec9b55..fd12489 100644
@@ -99,6 +99,89 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 EXPORT_SYMBOL_GPL(watchdog_init_timeout);
 
 /**
+ * watchdog_init_parms() - initialize generic watchdog parameters
+ * @wdd: Watchdog device to operate
+ * @dev: Device that stores the device tree properties
+ *
+ * Initialize the generic timeout parameters. The driver needs to set
+ * hw_features bitmask from @wdd prior calling this function in order
+ * to ensure the core knows how to handle the HW.

drivers/watchdog/watchdog_core.c +6 -0

+
+	/*
+	 * Max HW timeout needs to be set so that core knows when to
+	 * use a kernel worker to support longer watchdog timeouts
+	 */
+	if (!wdd->hw_max_timeout)

drivers/watchdog/watchdog_core.c +10 -0

+	}
+
+	mutex_unlock(&wdd->lock);
+}
+
+static int prepare_watchdog(struct watchdog_device *wdd)
+{
+	int err = 0;
+
+	/* Stop the watchdog now before user space opens the device */

drivers/watchdog/watchdog_core.c +6 -2

+		schedule_delayed_work(&wdd->work, wdd->hw_heartbeat);
+	}
+	return err;
+}
+
+/**
  * watchdog_register_device() - register a watchdog device
  * @wdd: watchdog device
  *

@@ -156,13 +239,24 @@ int watchdog_register_device(struct watchdog_device *wdd)
 	wdd->dev = device_create(watchdog_class, wdd->parent, devno,
 					NULL, "watchdog%d", wdd->id);
 	if (IS_ERR(wdd->dev)) {
-		watchdog_dev_unregister(wdd);
-		ida_simple_remove(&watchdog_ida, id);
 		ret = PTR_ERR(wdd->dev);

drivers/watchdog/watchdog_core.c +6 -0

 	}
 
 	return 0;
+
+dev_create_fail:
+	watchdog_dev_unregister(wdd);
+	ida_simple_remove(&watchdog_ida, id);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(watchdog_register_device);
 

@@ -181,6 +275,7 @@ void watchdog_unregister_device(struct watchdog_device *wdd)
 	if (wdd == NULL)
 		return;
 
+	cancel_delayed_work_sync(&wdd->work);
 	devno = wdd->cdev.dev;
 	ret = watchdog_dev_unregister(wdd);
 	if (ret)

drivers/watchdog/watchdog_dev.c +27 -6

index 6aaefba..04ac68c 100644
@@ -49,6 +49,14 @@ static dev_t watchdog_devt;
 /* the watchdog device behind /dev/watchdog */
 static struct watchdog_device *old_wdd;
 
+static int _watchdog_ping(struct watchdog_device *wddev)
+{
+	if (wddev->ops->ping)
+		return wddev->ops->ping(wddev);  /* ping the watchdog */
+	else
+		return wddev->ops->start(wddev); /* restart watchdog */
+}
+
 /*
  *	watchdog_ping: ping the watchdog.
  *	@wddev: the watchdog device to ping

@@ -73,10 +81,13 @@ static int watchdog_ping(struct watchdog_device *wddev)
 	if (!watchdog_active(wddev))
 		goto out_ping;
 
-	if (wddev->ops->ping)
-		err = wddev->ops->ping(wddev);  /* ping the watchdog */
-	else
-		err = wddev->ops->start(wddev); /* restart watchdog */
+	err = _watchdog_ping(wddev);
+
+	if (wddev->hw_max_timeout &&
+		wddev->timeout * HZ > wddev->hw_max_timeout) {
+		wddev->expires = jiffies + wddev->timeout * HZ;
+		schedule_delayed_work(&wddev->work, wddev->hw_heartbeat);
+	}
 
 out_ping:
 	mutex_unlock(&wddev->lock);

@@ -110,6 +121,13 @@ static int watchdog_start(struct watchdog_device *wddev)
 	if (err == 0)
 		set_bit(WDOG_ACTIVE, &wddev->status);
 
+	if (wddev->hw_max_timeout &&
+		wddev->timeout * HZ > wddev->hw_max_timeout) {
+		wddev->expires = jiffies + wddev->timeout * HZ;
+		schedule_delayed_work(&wddev->work, wddev->hw_heartbeat);
+	} else
+		cancel_delayed_work(&wddev->work);
+
 out_start:
 	mutex_unlock(&wddev->lock);
 	return err;

@@ -145,9 +163,21 @@ static int watchdog_stop(struct watchdog_device *wddev)
 		goto out_stop;
 	}
 
-	err = wddev->ops->stop(wddev);
-	if (err == 0)
+	if (!wddev->hw_max_timeout || watchdog_is_stoppable(wddev)) {
+		cancel_delayed_work(&wddev->work);
+		err = wddev->ops->stop(wddev);
+		if (err == 0)
+			clear_bit(WDOG_ACTIVE, &wddev->status);

drivers/watchdog/watchdog_dev.c +0 -0

 out_stop:
 	mutex_unlock(&wddev->lock);

@@ -194,7 +224,38 @@ out_status:
 static int watchdog_set_timeout(struct watchdog_device *wddev,

drivers/watchdog/watchdog_dev.c +3 -0

+
+		goto out_timeout;
+	}
 
 	if ((wddev->ops->set_timeout == NULL) ||
 	    !(wddev->info->options & WDIOF_SETTIMEOUT))

include/linux/watchdog.h +5 -0

index 395b70e..027c99d 100644
@@ -12,6 +12,7 @@
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/cdev.h>
+#include <linux/workqueue.h>
 #include <uapi/linux/watchdog.h>
 
 struct watchdog_ops;

@@ -62,9 +63,13 @@ struct watchdog_ops {
  * @timeout:	The watchdog devices timeout value.
  * @min_timeout:The watchdog devices minimum timeout value.
  * @max_timeout:The watchdog devices maximum timeout value.
+ * @hw_max_timeout:The watchdog hardware maximum timeout value.
+ * @hw_heartbeat:Time interval in HW between timer pings.
  * @driver-data:Pointer to the drivers private data.
  * @lock:	Lock for watchdog core internal use only.
+ * @work:	Worker used to provide longer timeouts than hardware supports.
  * @status:	Field that contains the devices internal status bits.
+ * @hw_features:Feature bits describing how the watchdog HW works.
  *
  * The watchdog_device structure contains all information about a
  * watchdog timer device.

include/linux/watchdog.h +13 -0

 	unsigned long status;
 /* Bit numbers for status flags */
 #define WDOG_ACTIVE		0	/* Is the watchdog running/active */

@@ -95,6 +104,14 @@ 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 */
+
+/* Bits describing features supported by the HW */
+	unsigned long hw_features;
+
+/* Can the watchdog be stopped and started */
+#define WDOG_HW_IS_STOPPABLE		BIT(0)
+/* Is the watchdog already running when the driver starts up */
+#define WDOG_HW_RUNNING_AT_BOOT		BIT(1)
 };
 
 #define WATCHDOG_NOWAYOUT		IS_BUILTIN(CONFIG_WATCHDOG_NOWAYOUT)

@@ -120,6 +137,11 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
 		(t < wdd->min_timeout || t > wdd->max_timeout));
 }
 
+static inline bool watchdog_is_stoppable(struct watchdog_device *wdd)
+{
+	return wdd->hw_features & WDOG_HW_IS_STOPPABLE;
+}
+
 /* Use the following functions to manipulate watchdog driver specific data */
 static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data)
 {

Hello Timo,

Quoted 2 lines · Timo Kokkonen

There is a great deal of diversity in the watchdog hardware found on different devices. Differen hardware have different contstraints on

s/Differen/Different/; s/contstraints/constraints/

Quoted 9 lines

them, many of the constraints that are excessively difficult for the user space to satisfy.

One such constraint is the length of the timeout value, which in many cases can be just a few seconds. Drivers are creating ad hoc solutions with timers and workqueues to extend the timeout in order to give user space more time between updates. Looking at the drivers it is clear that this has resulted to a lot of duplicate code.

Quoted 2 lines

Add an extension to the watchdog kernel API that allows the driver to describe tis HW constraints to the watchdog code. A kernel worker in

s/tis/its/

Quoted 3 lines

the core is then used to extend the watchdog timeout on behalf of the user space. This allows the drivers to be simplified as core takes over the timer extending.

In general a nice idea.

Quoted 26 lines

Tested-by Wenyou Yang <[email protected]>

Signed-off-by Timo Kokkonen <[email protected]>

---
  drivers/watchdog/watchdog_core.c | 101 +++++++++++++++++++++++++++++++++++++--
  drivers/watchdog/watchdog_dev.c  |  75 ++++++++++++++++++++++++++---
  include/linux/watchdog.h         |  23 +++++++++
  3 files changed, 189 insertions(+), 10 deletions(-)

drivers/watchdog/watchdog_core.c +10 -0

index cec9b55..fd12489 100644
@@ -99,6 +99,89 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
  EXPORT_SYMBOL_GPL(watchdog_init_timeout);
 
  /**
+ * watchdog_init_parms() - initialize generic watchdog parameters
+ * @wdd: Watchdog device to operate
+ * @dev: Device that stores the device tree properties
+ *
+ * Initialize the generic timeout parameters. The driver needs to set
+ * hw_features bitmask from @wdd prior calling this function in order
+ * to ensure the core knows how to handle the HW.
+ *
+ * A zero is returned on success and -EINVAL for failure.
+ */

drivers/watchdog/watchdog_core.c int watchdog_init_timeout(struct watchdog_device *wdd, · line 118

Quoted 7 lines

drivers/watchdog/watchdog_core.c +7 -0

+int watchdog_init_params(struct watchdog_device *wdd, struct device *dev)
+{
+	int ret = 0;
+
+	ret = watchdog_init_timeout(wdd, wdd->timeout, dev);
+	if (ret < 0)
+		return ret;

As Guenter pointed out, using wdd->timeout is unfortunate here.

Quoted 9 lines

drivers/watchdog/watchdog_core.c +9 -0

+
+	/*
+	 * Max HW timeout needs to be set so that core knows when to
+	 * use a kernel worker to support longer watchdog timeouts
+	 */
+	if (!wdd->hw_max_timeout)
+		return -EINVAL;
+
+	return ret;

drivers/watchdog/watchdog_core.c int watchdog_init_timeout(struct watchdog_device *wdd, · line 134

Quoted 7 lines

drivers/watchdog/watchdog_core.c +7 -0

+}
+EXPORT_SYMBOL_GPL(watchdog_init_params);
+
+static void watchdog_worker(struct work_struct *work)
+{
+	struct watchdog_device *wdd = container_of(to_delayed_work(work),
+						struct watchdog_device, work);

please align the follow-up line to the opening (.

drivers/watchdog/watchdog_core.c int watchdog_init_timeout(struct watchdog_device *wdd, · line 140

Quoted 6 lines

drivers/watchdog/watchdog_core.c +6 -0

+	bool boot_keepalive;
+	bool active_keepalive;
+
+	mutex_lock(&wdd->lock);
+
+	boot_keepalive = !watchdog_active(wdd) &&

I'm not sure about the semantic of watchdog_active here. Would be nice to clearify this. Does it mean /dev/watchdog is open, or the watchdog is running? Maybe introduce watchdog_running(wdd) and watchdog_opened(wdd)?

Is this related to the topic of this patch? You didn't mention anything in the commit log about the time until userspace comes up.

Quoted 4 lines

drivers/watchdog/watchdog_core.c +4 -0

+		!watchdog_is_stoppable(wdd);
+
+	active_keepalive = watchdog_active(wdd) &&
+		wdd->hw_max_timeout < wdd->timeout * HZ;

drivers/watchdog/watchdog_core.c int watchdog_init_timeout(struct watchdog_device *wdd, · line 150

Quoted 6 lines

drivers/watchdog/watchdog_core.c +6 -0

+
+	if (time_after(jiffies, wdd->expires) && watchdog_active(wdd)) {
+		pr_crit("I will reset your machine !\n");
+		mutex_unlock(&wdd->lock);
+		return;
+	}

I'd replace the pr_crit by a code comment. Also "I will reset your machine!" is wrong, the core just stops petting the watchdog.

If userspace comes in in time there is no harm, right?

drivers/watchdog/watchdog_core.c int watchdog_init_timeout(struct watchdog_device *wdd, · line 156

Quoted 6 lines

drivers/watchdog/watchdog_core.c +5 -0

+	if (boot_keepalive || active_keepalive) {
+		if (wdd->ops->ping)
+			wdd->ops->ping(wdd);
+		else
+			wdd->ops->start(wdd);

if (boot_keepalive || active_keepalive)

	_watchdog_ping(wdd);

drivers/watchdog/watchdog_core.c int watchdog_init_timeout(struct watchdog_device *wdd, · line 157

Quoted 2 lines

drivers/watchdog/watchdog_core.c +2 -0

+
+		schedule_delayed_work(&wdd->work,  wdd->hw_heartbeat);

s/ / /; Would it be sensible to assume hw_heartbeat to be timeout / 2 always?

Quoted 14 lines

drivers/watchdog/watchdog_core.c +14 -0

+	}
+
+	mutex_unlock(&wdd->lock);
+}
+
+static int prepare_watchdog(struct watchdog_device *wdd)
+{
+	int err = 0;
+
+	/* Stop the watchdog now before user space opens the device */
+	if (watchdog_is_stoppable(wdd) &&
+		wdd->hw_features & WDOG_HW_RUNNING_AT_BOOT) {
+		err = wdd->ops->stop(wdd);
+

drivers/watchdog/watchdog_core.c int watchdog_init_timeout(struct watchdog_device *wdd, · line 178

Quoted 6 lines

drivers/watchdog/watchdog_core.c +6 -0

+	} else if (!watchdog_is_stoppable(wdd) &&
+		wdd->hw_features & WDOG_HW_RUNNING_AT_BOOT) {
+		/*
+		 * Can't stop it, use a delayed worker to tick it
+		 * until it's open by user space
+		 */

I'd add a ping here because if the driver comes up a big part of the initial timeout setup by the bootloader might already be over.

Note that at least for the omap watchdog driver the assignment to timeout doesn't relate in any way to the timeout actually setup in hardware.

Quoted 20 lines

drivers/watchdog/watchdog_core.c +9 -3

+		schedule_delayed_work(&wdd->work, wdd->hw_heartbeat);
+	}
+	return err;
+}
+
+/**
   * watchdog_register_device() - register a watchdog device
   * @wdd: watchdog device
   *

@@ -156,13 +239,24 @@ int watchdog_register_device(struct watchdog_device *wdd)
  	wdd->dev = device_create(watchdog_class, wdd->parent, devno,
  					NULL, "watchdog%d", wdd->id);
  	if (IS_ERR(wdd->dev)) {
-		watchdog_dev_unregister(wdd);
-		ida_simple_remove(&watchdog_ida, id);
  		ret = PTR_ERR(wdd->dev);
-		return ret;
+		goto dev_create_fail;
+	}
+

drivers/watchdog/watchdog_core.c int watchdog_register_device(struct watchdog_device *wdd) · line 251

Quoted 6 lines

drivers/watchdog/watchdog_core.c +6 -0

+	INIT_DELAYED_WORK(&wdd->work, watchdog_worker);
+
+	if (wdd->hw_max_timeout) {
+		ret = prepare_watchdog(wdd);
+		if (ret)
+			goto dev_create_fail;

	} else {
		dev_warn("Incomplete watchdog driver implementation, please report or fix\n")

Quoted 81 lines

drivers/watchdog/watchdog_core.c +6 -0

  	}
 
  	return 0;
+
+dev_create_fail:
+	watchdog_dev_unregister(wdd);
+	ida_simple_remove(&watchdog_ida, id);
+	return ret;
  }
  EXPORT_SYMBOL_GPL(watchdog_register_device);
 

@@ -181,6 +275,7 @@ void watchdog_unregister_device(struct watchdog_device *wdd)
  	if (wdd == NULL)
  		return;
 
+	cancel_delayed_work_sync(&wdd->work);
  	devno = wdd->cdev.dev;
  	ret = watchdog_dev_unregister(wdd);
  	if (ret)

drivers/watchdog/watchdog_dev.c +27 -6

index 6aaefba..04ac68c 100644
@@ -49,6 +49,14 @@ static dev_t watchdog_devt;
  /* the watchdog device behind /dev/watchdog */
  static struct watchdog_device *old_wdd;
 
+static int _watchdog_ping(struct watchdog_device *wddev)
+{
+	if (wddev->ops->ping)
+		return wddev->ops->ping(wddev);  /* ping the watchdog */
+	else
+		return wddev->ops->start(wddev); /* restart watchdog */
+}
+
  /*
   *	watchdog_ping: ping the watchdog.
   *	@wddev: the watchdog device to ping

@@ -73,10 +81,13 @@ static int watchdog_ping(struct watchdog_device *wddev)
  	if (!watchdog_active(wddev))
  		goto out_ping;
 
-	if (wddev->ops->ping)
-		err = wddev->ops->ping(wddev);  /* ping the watchdog */
-	else
-		err = wddev->ops->start(wddev); /* restart watchdog */
+	err = _watchdog_ping(wddev);
+
+	if (wddev->hw_max_timeout &&
+		wddev->timeout * HZ > wddev->hw_max_timeout) {
+		wddev->expires = jiffies + wddev->timeout * HZ;
+		schedule_delayed_work(&wddev->work, wddev->hw_heartbeat);
+	}
 
  out_ping:
  	mutex_unlock(&wddev->lock);

@@ -110,6 +121,13 @@ static int watchdog_start(struct watchdog_device *wddev)
  	if (err == 0)
  		set_bit(WDOG_ACTIVE, &wddev->status);
 
+	if (wddev->hw_max_timeout &&
+		wddev->timeout * HZ > wddev->hw_max_timeout) {
+		wddev->expires = jiffies + wddev->timeout * HZ;
+		schedule_delayed_work(&wddev->work, wddev->hw_heartbeat);
+	} else
+		cancel_delayed_work(&wddev->work);
+
  out_start:
  	mutex_unlock(&wddev->lock);
  	return err;

@@ -145,9 +163,21 @@ static int watchdog_stop(struct watchdog_device *wddev)
  		goto out_stop;
  	}
 
-	err = wddev->ops->stop(wddev);
-	if (err == 0)
+	if (!wddev->hw_max_timeout || watchdog_is_stoppable(wddev)) {
+		cancel_delayed_work(&wddev->work);
+		err = wddev->ops->stop(wddev);
+		if (err == 0)
+			clear_bit(WDOG_ACTIVE, &wddev->status);

drivers/watchdog/watchdog_dev.c static int watchdog_stop(struct watchdog_device *wddev) · line 180

Quoted 10 lines

drivers/watchdog/watchdog_dev.c +9 -0

+	} else {
+		/* Unstoppable watchdogs need the worker to keep them alive */
  		clear_bit(WDOG_ACTIVE, &wddev->status);
+		/*
+		 * Ping it once as we don't know how much time there
+		 * is left in the watchdog timer.
+		 */
+		err = _watchdog_ping(wddev);
+		schedule_delayed_work(&wddev->work, wddev->hw_heartbeat);
+	}

Why not do:

	err = wddev->ops->stop(wddev);
	clear_bit(WDOG_ACTIVE, &wddev->status);
	if (err != 0) {
		err = _watchdog_ping(wddev);
		if (err) {
			dev_crit(...);
		schedule_delayed_work(...);
	}

This way you (maybe?) don't need to expand all drivers to tell you if the watchdog timer is stoppable, just assert that the stop callback fails which IMHO is easier and more natural.

Quoted 5 lines

drivers/watchdog/watchdog_dev.c +0 -0

  out_stop:
  	mutex_unlock(&wddev->lock);

@@ -194,7 +224,38 @@ out_status:
  static int watchdog_set_timeout(struct watchdog_device *wddev,

drivers/watchdog/watchdog_dev.c out_status: · line 233

Quoted 10 lines

drivers/watchdog/watchdog_dev.c +7 -1

  							unsigned int timeout)
  {
-	int err;
+	int err = 0;
+
+	if (wddev->hw_max_timeout) {
+		int hw_timeout;
+		/*
+		 * We can't support too short timeout values. There is
+		 * really no maximu however, anything longer than HW

s/maximu/maximum/

drivers/watchdog/watchdog_dev.c out_status: · line 238

Quoted 5 lines

drivers/watchdog/watchdog_dev.c +5 -0

+		 * maximum will be supported by the watchdog core on
+		 * behalf of the actual HW.
+		 */
+		if (timeout < wddev->min_timeout)
+			return -EINVAL;

Is it save to read min_timout without holding the lock?

Quoted 7 lines

drivers/watchdog/watchdog_dev.c +7 -0

+
+		mutex_lock(&wddev->lock);
+		if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+			err = -ENODEV;
+			goto out_timeout;
+		}
+

drivers/watchdog/watchdog_dev.c out_status: · line 250

Quoted 5 lines

drivers/watchdog/watchdog_dev.c +5 -0

+		if (timeout * HZ > wddev->hw_max_timeout)
+			schedule_delayed_work(&wddev->work,
+					wddev->hw_heartbeat);
+
+		hw_timeout = min(timeout, wddev->hw_max_timeout / HZ);

If you'd keep hw_max_timeout in seconds (i.e. what is used for timeout) you wouldn't need to divide here. If my watchdog has hw_max_timeout = 0.5s then you do wddev->ops->set_timeout(0) here.

drivers/watchdog/watchdog_dev.c out_status: · line 252

Quoted 2 lines

drivers/watchdog/watchdog_dev.c +2 -0

+		if (wddev->info->options & WDIOF_SETTIMEOUT)
+			err = wddev->ops->set_timeout(wddev, hw_timeout);

The case of not having WDIOF_SETTIMEOUT isn't handled here, right? Other places check for WDIOF_SETTIMEOUT and wddev->ops->set_timeout?!

drivers/watchdog/watchdog_dev.c out_status: · line 255

Quoted 3 lines

drivers/watchdog/watchdog_dev.c +3 -0

+
+		if (hw_timeout < timeout)
+			wddev->timeout = timeout;

What is the semantic of wddev->timeout? Does it hold the value set by userspace or the value the hardware is set to? You'd need to clarify that by adding more comments. Is this if correct?

Quoted 35 lines

drivers/watchdog/watchdog_dev.c +3 -0

+
+		goto out_timeout;
+	}
 
  	if ((wddev->ops->set_timeout == NULL) ||
  	    !(wddev->info->options & WDIOF_SETTIMEOUT))

include/linux/watchdog.h +5 -0

index 395b70e..027c99d 100644
@@ -12,6 +12,7 @@
  #include <linux/bitops.h>
  #include <linux/device.h>
  #include <linux/cdev.h>
+#include <linux/workqueue.h>
  #include <uapi/linux/watchdog.h>
 
  struct watchdog_ops;

@@ -62,9 +63,13 @@ struct watchdog_ops {
   * @timeout:	The watchdog devices timeout value.
   * @min_timeout:The watchdog devices minimum timeout value.
   * @max_timeout:The watchdog devices maximum timeout value.
+ * @hw_max_timeout:The watchdog hardware maximum timeout value.
+ * @hw_heartbeat:Time interval in HW between timer pings.
   * @driver-data:Pointer to the drivers private data.
   * @lock:	Lock for watchdog core internal use only.
+ * @work:	Worker used to provide longer timeouts than hardware supports.
   * @status:	Field that contains the devices internal status bits.
+ * @hw_features:Feature bits describing how the watchdog HW works.
   *
   * The watchdog_device structure contains all information about a
   * watchdog timer device.

@@ -86,8 +91,12 @@ struct watchdog_device {
  	unsigned int timeout;
  	unsigned int min_timeout;

include/linux/watchdog.h struct watchdog_device { · line 99

Quoted 7 lines

include/linux/watchdog.h +4 -0

  	unsigned int max_timeout;
+	unsigned int hw_max_timeout; /* in jiffies */
+	unsigned int hw_heartbeat; /* in jiffies */
+	unsigned long int expires; /* for keepalive worker */
  	void *driver_data;
  	struct mutex lock;
+	struct delayed_work work;

I'd group things together here that belong together.

Quoted 22 lines

include/linux/watchdog.h +8 -0

  	unsigned long status;
  /* Bit numbers for status flags */
  #define WDOG_ACTIVE		0	/* Is the watchdog running/active */

@@ -95,6 +104,14 @@ 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 */
+
+/* Bits describing features supported by the HW */
+	unsigned long hw_features;
+
+/* Can the watchdog be stopped and started */
+#define WDOG_HW_IS_STOPPABLE		BIT(0)
+/* Is the watchdog already running when the driver starts up */
+#define WDOG_HW_RUNNING_AT_BOOT		BIT(1)
  };
 
  #define WATCHDOG_NOWAYOUT		IS_BUILTIN(CONFIG_WATCHDOG_NOWAYOUT)

@@ -120,6 +137,11 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
  		(t < wdd->min_timeout || t > wdd->max_timeout));
  }
 

include/linux/watchdog.h static inline void *watchdog_get_drvdata(struct watchdog_device *wdd) · line 159

Quoted 13 lines

include/linux/watchdog.h +6 -0

+static inline bool watchdog_is_stoppable(struct watchdog_device *wdd)
+{
+	return wdd->hw_features & WDOG_HW_IS_STOPPABLE;
+}
+
  /* Use the following functions to manipulate watchdog driver specific data */
  static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data)
  {

@@ -134,6 +156,7 @@ static inline void *watchdog_get_drvdata(struct watchdog_device *wdd)
  /* drivers/watchdog/watchdog_core.c */
  extern int watchdog_init_timeout(struct watchdog_device *wdd,
  				  unsigned int timeout_parm, struct device *dev);
+int watchdog_init_params(struct watchdog_device *wdd, struct device *dev);

This needs commenting and probably embrace watchdog_init_timeout properly. i.e. remove watchdog_init_timeout.

Best regards
Uwe

There is a great deal of diversity in the watchdog hardware found on different devices. Differen hardware have different contstraints on them, many of the constraints that are excessively difficult for the user space to satisfy.

One such constraint is the length of the timeout value, which in many cases can be just a few seconds. Drivers are creating ad hoc solutions with timers and workqueues to extend the timeout in order to give user space more time between updates. Looking at the drivers it is clear that this has resulted to a lot of duplicate code.

Add an extension to the watchdog kernel API that allows the driver to describe tis HW constraints to the watchdog code. A kernel worker in the core is then used to extend the watchdog timeout on behalf of the user space. This allows the drivers to be simplified as core takes over the timer extending.

Tested-by Wenyou Yang <[email protected]>

Signed-off-by Timo Kokkonen <[email protected]>

---
 drivers/watchdog/watchdog_core.c | 101 +++++++++++++++++++++++++++++++++++++--
 drivers/watchdog/watchdog_dev.c  |  75 ++++++++++++++++++++++++++---
 include/linux/watchdog.h         |  23 +++++++++
 3 files changed, 189 insertions(+), 10 deletions(-)

drivers/watchdog/watchdog_core.c +98 -3

index cec9b55..fd12489 100644
@@ -99,6 +99,89 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 EXPORT_SYMBOL_GPL(watchdog_init_timeout);
 
 /**
+ * watchdog_init_parms() - initialize generic watchdog parameters
+ * @wdd: Watchdog device to operate
+ * @dev: Device that stores the device tree properties
+ *
+ * Initialize the generic timeout parameters. The driver needs to set
+ * hw_features bitmask from @wdd prior calling this function in order
+ * to ensure the core knows how to handle the HW.
+ *
+ * A zero is returned on success and -EINVAL for failure.
+ */
+int watchdog_init_params(struct watchdog_device *wdd, struct device *dev)
+{
+	int ret = 0;
+
+	ret = watchdog_init_timeout(wdd, wdd->timeout, dev);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Max HW timeout needs to be set so that core knows when to
+	 * use a kernel worker to support longer watchdog timeouts
+	 */
+	if (!wdd->hw_max_timeout)
+		return -EINVAL;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(watchdog_init_params);
+
+static void watchdog_worker(struct work_struct *work)
+{
+	struct watchdog_device *wdd = container_of(to_delayed_work(work),
+						struct watchdog_device, work);
+	bool boot_keepalive;
+	bool active_keepalive;
+
+	mutex_lock(&wdd->lock);
+
+	boot_keepalive = !watchdog_active(wdd) &&
+		!watchdog_is_stoppable(wdd);
+
+	active_keepalive = watchdog_active(wdd) &&
+		wdd->hw_max_timeout < wdd->timeout * HZ;
+
+	if (time_after(jiffies, wdd->expires) && watchdog_active(wdd)) {
+		pr_crit("I will reset your machine !\n");
+		mutex_unlock(&wdd->lock);
+		return;
+	}
+
+	if (boot_keepalive || active_keepalive) {
+		if (wdd->ops->ping)
+			wdd->ops->ping(wdd);
+		else
+			wdd->ops->start(wdd);
+
+		schedule_delayed_work(&wdd->work,  wdd->hw_heartbeat);
+	}
+
+	mutex_unlock(&wdd->lock);
+}
+
+static int prepare_watchdog(struct watchdog_device *wdd)
+{
+	int err = 0;
+
+	/* Stop the watchdog now before user space opens the device */
+	if (watchdog_is_stoppable(wdd) &&
+		wdd->hw_features & WDOG_HW_RUNNING_AT_BOOT) {
+		err = wdd->ops->stop(wdd);
+
+	} else if (!watchdog_is_stoppable(wdd) &&
+		wdd->hw_features & WDOG_HW_RUNNING_AT_BOOT) {
+		/*
+		 * Can't stop it, use a delayed worker to tick it
+		 * until it's open by user space
+		 */
+		schedule_delayed_work(&wdd->work, wdd->hw_heartbeat);
+	}
+	return err;
+}
+
+/**
  * watchdog_register_device() - register a watchdog device
  * @wdd: watchdog device
  *

@@ -156,13 +239,24 @@ int watchdog_register_device(struct watchdog_device *wdd)
 	wdd->dev = device_create(watchdog_class, wdd->parent, devno,
 					NULL, "watchdog%d", wdd->id);
 	if (IS_ERR(wdd->dev)) {
-		watchdog_dev_unregister(wdd);
-		ida_simple_remove(&watchdog_ida, id);
 		ret = PTR_ERR(wdd->dev);
-		return ret;
+		goto dev_create_fail;
+	}
+
+	INIT_DELAYED_WORK(&wdd->work, watchdog_worker);
+
+	if (wdd->hw_max_timeout) {
+		ret = prepare_watchdog(wdd);
+		if (ret)
+			goto dev_create_fail;
 	}
 
 	return 0;
+
+dev_create_fail:
+	watchdog_dev_unregister(wdd);
+	ida_simple_remove(&watchdog_ida, id);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(watchdog_register_device);
 

@@ -181,6 +275,7 @@ void watchdog_unregister_device(struct watchdog_device *wdd)
 	if (wdd == NULL)
 		return;
 
+	cancel_delayed_work_sync(&wdd->work);
 	devno = wdd->cdev.dev;
 	ret = watchdog_dev_unregister(wdd);
 	if (ret)

drivers/watchdog/watchdog_dev.c +68 -7

index 6aaefba..04ac68c 100644
@@ -49,6 +49,14 @@ static dev_t watchdog_devt;
 /* the watchdog device behind /dev/watchdog */
 static struct watchdog_device *old_wdd;
 
+static int _watchdog_ping(struct watchdog_device *wddev)
+{
+	if (wddev->ops->ping)
+		return wddev->ops->ping(wddev);  /* ping the watchdog */
+	else
+		return wddev->ops->start(wddev); /* restart watchdog */
+}
+
 /*
  *	watchdog_ping: ping the watchdog.
  *	@wddev: the watchdog device to ping

@@ -73,10 +81,13 @@ static int watchdog_ping(struct watchdog_device *wddev)
 	if (!watchdog_active(wddev))
 		goto out_ping;
 
-	if (wddev->ops->ping)
-		err = wddev->ops->ping(wddev);  /* ping the watchdog */
-	else
-		err = wddev->ops->start(wddev); /* restart watchdog */
+	err = _watchdog_ping(wddev);
+
+	if (wddev->hw_max_timeout &&
+		wddev->timeout * HZ > wddev->hw_max_timeout) {
+		wddev->expires = jiffies + wddev->timeout * HZ;
+		schedule_delayed_work(&wddev->work, wddev->hw_heartbeat);
+	}
 
 out_ping:
 	mutex_unlock(&wddev->lock);

@@ -110,6 +121,13 @@ static int watchdog_start(struct watchdog_device *wddev)
 	if (err == 0)
 		set_bit(WDOG_ACTIVE, &wddev->status);
 
+	if (wddev->hw_max_timeout &&
+		wddev->timeout * HZ > wddev->hw_max_timeout) {
+		wddev->expires = jiffies + wddev->timeout * HZ;
+		schedule_delayed_work(&wddev->work, wddev->hw_heartbeat);
+	} else
+		cancel_delayed_work(&wddev->work);
+
 out_start:
 	mutex_unlock(&wddev->lock);
 	return err;

@@ -145,9 +163,21 @@ static int watchdog_stop(struct watchdog_device *wddev)
 		goto out_stop;
 	}
 
-	err = wddev->ops->stop(wddev);
-	if (err == 0)
+	if (!wddev->hw_max_timeout || watchdog_is_stoppable(wddev)) {
+		cancel_delayed_work(&wddev->work);
+		err = wddev->ops->stop(wddev);
+		if (err == 0)
+			clear_bit(WDOG_ACTIVE, &wddev->status);
+	} else {
+		/* Unstoppable watchdogs need the worker to keep them alive */
 		clear_bit(WDOG_ACTIVE, &wddev->status);
+		/*
+		 * Ping it once as we don't know how much time there
+		 * is left in the watchdog timer.
+		 */
+		err = _watchdog_ping(wddev);
+		schedule_delayed_work(&wddev->work, wddev->hw_heartbeat);
+	}
 
 out_stop:
 	mutex_unlock(&wddev->lock);

@@ -194,7 +224,38 @@ out_status:
 static int watchdog_set_timeout(struct watchdog_device *wddev,
 							unsigned int timeout)
 {
-	int err;
+	int err = 0;
+
+	if (wddev->hw_max_timeout) {
+		int hw_timeout;
+		/*
+		 * We can't support too short timeout values. There is
+		 * really no maximu however, anything longer than HW
+		 * maximum will be supported by the watchdog core on
+		 * behalf of the actual HW.
+		 */
+		if (timeout < wddev->min_timeout)
+			return -EINVAL;
+
+		mutex_lock(&wddev->lock);
+		if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+			err = -ENODEV;
+			goto out_timeout;
+		}
+
+		if (timeout * HZ > wddev->hw_max_timeout)
+			schedule_delayed_work(&wddev->work,
+					wddev->hw_heartbeat);
+
+		hw_timeout = min(timeout, wddev->hw_max_timeout / HZ);
+		if (wddev->info->options & WDIOF_SETTIMEOUT)
+			err = wddev->ops->set_timeout(wddev, hw_timeout);
+
+		if (hw_timeout < timeout)
+			wddev->timeout = timeout;
+
+		goto out_timeout;
+	}
 
 	if ((wddev->ops->set_timeout == NULL) ||
 	    !(wddev->info->options & WDIOF_SETTIMEOUT))

include/linux/watchdog.h +23 -0

index 395b70e..027c99d 100644
@@ -12,6 +12,7 @@
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/cdev.h>
+#include <linux/workqueue.h>
 #include <uapi/linux/watchdog.h>
 
 struct watchdog_ops;

@@ -62,9 +63,13 @@ struct watchdog_ops {
  * @timeout:	The watchdog devices timeout value.
  * @min_timeout:The watchdog devices minimum timeout value.
  * @max_timeout:The watchdog devices maximum timeout value.
+ * @hw_max_timeout:The watchdog hardware maximum timeout value.
+ * @hw_heartbeat:Time interval in HW between timer pings.
  * @driver-data:Pointer to the drivers private data.
  * @lock:	Lock for watchdog core internal use only.
+ * @work:	Worker used to provide longer timeouts than hardware supports.
  * @status:	Field that contains the devices internal status bits.
+ * @hw_features:Feature bits describing how the watchdog HW works.
  *
  * The watchdog_device structure contains all information about a
  * watchdog timer device.

@@ -86,8 +91,12 @@ struct watchdog_device {
 	unsigned int timeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
+	unsigned int hw_max_timeout; /* in jiffies */
+	unsigned int hw_heartbeat; /* in jiffies */
+	unsigned long int expires; /* for keepalive worker */
 	void *driver_data;
 	struct mutex lock;
+	struct delayed_work work;
 	unsigned long status;
 /* Bit numbers for status flags */
 #define WDOG_ACTIVE		0	/* Is the watchdog running/active */

@@ -95,6 +104,14 @@ 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 */
+
+/* Bits describing features supported by the HW */
+	unsigned long hw_features;
+
+/* Can the watchdog be stopped and started */
+#define WDOG_HW_IS_STOPPABLE		BIT(0)
+/* Is the watchdog already running when the driver starts up */
+#define WDOG_HW_RUNNING_AT_BOOT		BIT(1)
 };
 
 #define WATCHDOG_NOWAYOUT		IS_BUILTIN(CONFIG_WATCHDOG_NOWAYOUT)

@@ -120,6 +137,11 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
 		(t < wdd->min_timeout || t > wdd->max_timeout));
 }
 
+static inline bool watchdog_is_stoppable(struct watchdog_device *wdd)
+{
+	return wdd->hw_features & WDOG_HW_IS_STOPPABLE;
+}
+
 /* Use the following functions to manipulate watchdog driver specific data */
 static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data)
 {

@@ -134,6 +156,7 @@ static inline void *watchdog_get_drvdata(struct watchdog_device *wdd)
 /* drivers/watchdog/watchdog_core.c */
 extern int watchdog_init_timeout(struct watchdog_device *wdd,
 				  unsigned int timeout_parm, struct device *dev);
+int watchdog_init_params(struct watchdog_device *wdd, struct device *dev);
 extern int watchdog_register_device(struct watchdog_device *);
 extern void watchdog_unregister_device(struct watchdog_device *);
 

Signature · 6 lines

2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 11 lines · Timo Kokkonen

There is a great deal of diversity in the watchdog hardware found on different devices. Differen hardware have different contstraints on them, many of the constraints that are excessively difficult for the user space to satisfy.

One such constraint is the length of the timeout value, which in many cases can be just a few seconds. Drivers are creating ad hoc solutions with timers and workqueues to extend the timeout in order to give user space more time between updates. Looking at the drivers it is clear that this has resulted to a lot of duplicate code.

Quoted 8 lines

Add an extension to the watchdog kernel API that allows the driver to describe tis HW constraints to the watchdog code. A kernel worker in the core is then used to extend the watchdog timeout on behalf of the user space. This allows the drivers to be simplified as core takes over the timer extending.

Tested-by Wenyou Yang <[email protected]>

Signed-off-by Timo Kokkonen <[email protected]>

Hi Timo,

Time to add my thoughts before you create the next version. It isn't complete, so I won't try a line by line review.

The init_params function isn't the best choice for a name. Ultimately we have other concepts to add - such as generic pretimeout support - which makes this more and more difficult to handle. A separate function to initialize the early timeout separately might make more sense. Either case, any new API must not provide less functionality than the old one.

The current semantics of timeout and max_timeout is that those _are_ the hardware limits. We can not just change that. Also, the new functionality should, as much as possible, be automatic and should not require changes in existing drivers to be useful for them.

I understand we need something line hw_timeout and hw_max_timeout, but that doesn't mean that those _have_ to be specified to be used. For a generic use case, it is sufficient to assume that timeout == hw_timeout and max_timeout == hw_max_timeout, and the hw_ values can be set automatically if not specified. The code should be more permissive to not bail out if , for example, hw_max_timeout is not specified (then hw_max_timeout == max_timeout).

Similar, it should not be necessary to have to specify hw_timeout and hw_max_timeout in the first place for those to be useful. We can instead do something like

	#define MIN_TIMEOUT	(60 * 10)	/* in seconds */

	if (!hw_max_timeout && max_timeout < MIN_TIMEOUT) {
		hw_max_timeout = max_timeout;
		max_timeout = MIN_TIMEOUT;
	}

with similar adjustments for hw_timeout and timeout. I am not sure if we should keep the current meaning of timeout and max_timeout (ie it means hw limits) and introduce sw_max_timeout and sw_timeout, or extend the meaning of max_timeout and timeout to mean both and introduce the hw_variables. The latter would let us specify more granularity for the hw_ values, so maybe the latter approach is better.

Limits should not be in jiffies unless only used internally by the watchdog subsystem. If more granularity is needed, use milliseconds and convert to jiffies where needed.

Consider adding helper functions such as _watchdog_ping as inline in watchdog_core.h.

hw_heartbeat should be set from the watchdog core; I don't think it needs to be provided by the driver. It can be calculated from hw_max_timeout and timeout as needed (to something like min(hw_max_timeout, timeout) / 2).

You don't need boot_kepalive and active_keepalive as separate flags in watchdog_worker.

hw_features is unfortunate, and I am not sure that it is needed. That the watchdog is running at boot time is not really a hardware feature, but a state. Either add a bit to 'status' in watchdog_device, or add a callback function. WDOG_HW_IS_STOPPABLE can be added to 'status' as well. It should probably be reversed, though, to something like WDOG_NOT_STOPPABLE, so that it does not have to be set to existing drivers.

There is a great deal of diversity in the watchdog hardware found on different devices. Differen hardware have different contstraints on them, many of the constraints that are excessively difficult for the user space to satisfy.

One such constraint is the length of the timeout value, which in many cases can be just a few seconds. Drivers are creating ad hoc solutions with timers and workqueues to extend the timeout in order to give user space more time between updates. Looking at the drivers it is clear that this has resulted to a lot of duplicate code.

Add an extension to the watchdog kernel API that allows the driver to describe tis HW constraints to the watchdog code. A kernel worker in the core is then used to extend the watchdog timeout on behalf of the user space. This allows the drivers to be simplified as core takes over the timer extending.

Tested-by Wenyou Yang <[email protected]>

Signed-off-by Timo Kokkonen <[email protected]>

---
 drivers/watchdog/watchdog_core.c | 101 +++++++++++++++++++++++++++++++++++++--
 drivers/watchdog/watchdog_dev.c  |  75 ++++++++++++++++++++++++++---
 include/linux/watchdog.h         |  23 +++++++++
 3 files changed, 189 insertions(+), 10 deletions(-)

static int watchdog_set_timeout(struct watchdog_device *wddev,

drivers/watchdog/watchdog_dev.c +4 -1

 							unsigned int timeout)
 {
-	int err;
+	int err = 0;
+
+	if (wddev->hw_max_timeout) {
+		int hw_timeout;

drivers/watchdog/watchdog_dev.c +11 -0

+		if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+			err = -ENODEV;
+			goto out_timeout;
+		}
+
+		if (timeout * HZ > wddev->hw_max_timeout)
+			schedule_delayed_work(&wddev->work,
+					wddev->hw_heartbeat);
+
+		hw_timeout = min(timeout, wddev->hw_max_timeout / HZ);
+		if (wddev->info->options & WDIOF_SETTIMEOUT)

Fill in the HW capabilities in watchdog_device structure and call watchdgog_init_params() to let watchdog core to init itself properly. The watchdog core can then ping stopped watchdog and the timer code in the driver can be removed.

Signed-off-by Timo Kokkonen <[email protected]>

Quoted 6 lines · Timo Kokkonen

Fill in the HW capabilities in watchdog_device structure and call watchdgog_init_params() to let watchdog core to init itself properly. The watchdog core can then ping stopped watchdog and the timer code in the driver can be removed.

Signed-off-by Timo Kokkonen <[email protected]>

This patch changes the default behaviour of the imx watchdog. Without the patch: If the system boot with watchdog enabled (by the bootloader), the driver sets up a threads to pet the watchdog. With the patch, imx2_wdt_ping() is called once during probe but the thread is not started. There are no timeout or early_timeout paremters in the DT or in the kernel command line.

I don't like the old behaviour, but I think there are some setups that rely on this feature.

Quoted 15 lines · Marc Kleine-Budde

Quoted 6 lines · Timo Kokkonen

Fill in the HW capabilities in watchdog_device structure and call watchdgog_init_params() to let watchdog core to init itself properly. The watchdog core can then ping stopped watchdog and the timer code in the driver can be removed.

Signed-off-by Timo Kokkonen <[email protected]>

This patch changes the default behaviour of the imx watchdog. Without the patch: If the system boot with watchdog enabled (by the bootloader), the driver sets up a threads to pet the watchdog. With the patch, imx2_wdt_ping() is called once during probe but the thread is not started. There are no timeout or early_timeout paremters in the DT or in the kernel command line.

Quoted 2 lines

I don't like the old behaviour, but I think there are some setups that rely on this feature.

BTW: the start-thread-if-wd-is-active feature was added in:

faad5de0b104 watchdog: imx2_wdt: convert to watchdog core api

Structures allocated on the beginning of the probe function must be freed in case of failure.

Quoted 2 lines · Timo Kokkonen

Structures allocated on the beginning of the probe function must be freed in case of failure.

Really ? The functions are allocated with devm_kzalloc. It might be useful to introduce an error on purpose, have the code jump to your new label, and observe what happens.

Use the new watchdog core extensions to let watchdog core take over boot time watchdog behavior. The difference is that early-timeout-sec device tree property becomes available for this driver and a running watchdog is not stopped unless the core decides to stop it.

Omap watchdog is running by default in the boot up but bootloader might have stopped it. Therefore we fill the WDOG_HW_RUNNING_AT_BOOT bit depending on the actual watchdog state so that the watchdog core can act properly.

Signed-off-by Timo Kokkonen <[email protected]>

---
 drivers/watchdog/omap_wdt.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

drivers/watchdog/omap_wdt.c +0 -0

index bbaf39a..7164f2e 100644

Hello,

Quoted 20 lines · Timo Kokkonen

Use the new watchdog core extensions to let watchdog core take over boot time watchdog behavior. The difference is that early-timeout-sec device tree property becomes available for this driver and a running watchdog is not stopped unless the core decides to stop it.

Omap watchdog is running by default in the boot up but bootloader might have stopped it. Therefore we fill the WDOG_HW_RUNNING_AT_BOOT bit depending on the actual watchdog state so that the watchdog core can act properly.

Signed-off-by Timo Kokkonen <[email protected]>

---
  drivers/watchdog/omap_wdt.c | 28 +++++++++++++++++++++++++++-
  1 file changed, 27 insertions(+), 1 deletion(-)

drivers/watchdog/omap_wdt.c +0 -0

index bbaf39a..7164f2e 100644

drivers/watchdog/omap_wdt.c static void omap_wdt_reload(struct omap_wdt_dev *wdev) · line 86

Quoted 9 lines

drivers/watchdog/omap_wdt.c +6 -0

@@ -78,6 +78,13 @@ static void omap_wdt_reload(struct omap_wdt_dev *wdev)
  	/* reloaded WCRR from WLDR */
  }
 
+static int omap_wdt_is_running(struct omap_wdt_dev *wdev)
+{
+	void __iomem *base = wdev->base;
+
+	return readl_relaxed(base + OMAP_WATCHDOG_SPR) == 0x4444;
+}

This isn't reliable. The sequence needed to enable the watchdog is

	writel(0xbbbb, base + OMAP_WATCHDOG_SPR);
	writel(0x4444, base + OMAP_WATCHDOG_SPR);

The sequence to stop is:

	writel(0xaaaa, base + OMAP_WATCHDOG_SPR);
	writel(0x5555, base + OMAP_WATCHDOG_SPR);

But:

barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00005555 UU..
barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x4444 barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00004444 DD..

So the register contains 0x4444 but the timer doesn't run. So at best testing for 0x4444 is a good heuristic.

Quoted 17 lines · Uwe Kleine-K?nig

drivers/watchdog/omap_wdt.c static void omap_wdt_reload(struct omap_wdt_dev *wdev) · line 86

Quoted 6 lines · Timo Kokkonen

drivers/watchdog/omap_wdt.c +6 -0

+static int omap_wdt_is_running(struct omap_wdt_dev *wdev)
+{
+	void __iomem *base = wdev->base;
+
+	return readl_relaxed(base + OMAP_WATCHDOG_SPR) == 0x4444;
+}

This isn't reliable. The sequence needed to enable the watchdog is

	writel(0xbbbb, base + OMAP_WATCHDOG_SPR);
	writel(0x4444, base + OMAP_WATCHDOG_SPR);

The sequence to stop is:

	writel(0xaaaa, base + OMAP_WATCHDOG_SPR);
	writel(0x5555, base + OMAP_WATCHDOG_SPR);

But:

Quoted 8 lines

barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00005555 UU..
barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x4444 barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00004444 DD..

So the register contains 0x4444 but the timer doesn't run. So at best testing for 0x4444 is a good heuristic.

Yeah.. I don't think we can get any better than that. Unless we start checking the counter register and see whether it really counts or not, and I think that's a bit overkill.. So I'd say we should be safe when assuming bootloader is doing things correctly. Although, we could add a comment to the code that the test may not be 100% reliable in case the start sequence have not been issued properly.

Thanks for pointing this out!

Hello Timo,

Quoted 27 lines · Timo Kokkonen

Quoted 17 lines · Uwe Kleine-K?nig

drivers/watchdog/omap_wdt.c static void omap_wdt_reload(struct omap_wdt_dev *wdev) · line 86

Quoted 6 lines · Timo Kokkonen

drivers/watchdog/omap_wdt.c +6 -0

+static int omap_wdt_is_running(struct omap_wdt_dev *wdev)
+{
+	void __iomem *base = wdev->base;
+
+	return readl_relaxed(base + OMAP_WATCHDOG_SPR) == 0x4444;
+}

This isn't reliable. The sequence needed to enable the watchdog is

	writel(0xbbbb, base + OMAP_WATCHDOG_SPR);
	writel(0x4444, base + OMAP_WATCHDOG_SPR);

The sequence to stop is:

	writel(0xaaaa, base + OMAP_WATCHDOG_SPR);
	writel(0x5555, base + OMAP_WATCHDOG_SPR);

But:

Quoted 8 lines

barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00005555 UU..
barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x4444 barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00004444 DD..

So the register contains 0x4444 but the timer doesn't run. So at best testing for 0x4444 is a good heuristic.

Quoted 8 lines

Yeah.. I don't think we can get any better than that. Unless we start checking the counter register and see whether it really counts or not, and I think that's a bit overkill.. So I'd say we should be safe when assuming bootloader is doing things correctly. Although, we could add a comment to the code that the test may not be 100% reliable in case the start sequence have not been issued properly.

Thanks for pointing this out!

It doesn't seem to much overhead to do:

	/*
	 * There is no register that tells us if the timer is running,
	 * so we have to resort to sample twice. The minimal frequency
	 * is 256 Hz (32768 Hz prescaled with 2**7).
	 */
	counter1 = readl(base + OMAP_WATCHDOG_CCR);
	mdelay(4);
	counter2 = readl(base + OMAP_WATCHDOG_CCR);
	return counter1 != counter2;

I'd say it's even worth to do:

	cntrl = readl(base + OMAP_WATCHDOG_CNTRL);
	if (cntrl & (1 << 5))
		shift = (cntrl >> 2) & 0x7;
	else
		shift = 0;
	counter1 = readl(base + OMAP_WATCHDOG_CCR);
	udelay(31 << shift);
	counter2 = readl(base + OMAP_WATCHDOG_CCR);
	return counter1 != counter2;

For some bonus points add some defines for the magic constants.

This is save as the OMAP_WATCHDOG_CNTRL doesn't seem to accept reads while the counter is running. Maybe even this could be used to detect a running timer?:

- enable timer:

	barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0xbbbb
	barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x4444

- read out WCLR

	barebox at TI AM335x BeagleBone black:/ md 0x44e35024+4
	44e35024: 00000020                                            ...

- write to WCLR

	barebox at TI AM335x BeagleBone black:/ mw 0x44e35024 0x0

- check result; didn't work

	barebox at TI AM335x BeagleBone black:/ md 0x44e35024+4
	44e35024: 00000020                                            ...

- stop timer

	barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0xaaaa
	barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x5555

- recheck WCLR

	barebox at TI AM335x BeagleBone black:/ md 0x44e35024+4
	44e35024: 00000020                                            ...

- write to WCLR

	barebox at TI AM335x BeagleBone black:/ mw 0x44e35024 0x0

- check result; write succeeded

	barebox at TI AM335x BeagleBone black:/ md 0x44e35024+4
	44e35024: 00000000                                           ....

(This is was tested on an AM335x.)

Best regards
Uwe

Hello Timo,

Quoted 27 lines · Timo Kokkonen

Quoted 17 lines · Uwe Kleine-K?nig

drivers/watchdog/omap_wdt.c static void omap_wdt_reload(struct omap_wdt_dev *wdev) · line 86

Quoted 6 lines · Timo Kokkonen

drivers/watchdog/omap_wdt.c +6 -0

+static int omap_wdt_is_running(struct omap_wdt_dev *wdev)
+{
+	void __iomem *base = wdev->base;
+
+	return readl_relaxed(base + OMAP_WATCHDOG_SPR) == 0x4444;
+}

This isn't reliable. The sequence needed to enable the watchdog is

	writel(0xbbbb, base + OMAP_WATCHDOG_SPR);
	writel(0x4444, base + OMAP_WATCHDOG_SPR);

The sequence to stop is:

	writel(0xaaaa, base + OMAP_WATCHDOG_SPR);
	writel(0x5555, base + OMAP_WATCHDOG_SPR);

But:

Quoted 8 lines

barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00005555 UU..
barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x4444 barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00004444 DD..

So the register contains 0x4444 but the timer doesn't run. So at best testing for 0x4444 is a good heuristic.

Quoted 8 lines

Yeah.. I don't think we can get any better than that. Unless we start checking the counter register and see whether it really counts or not, and I think that's a bit overkill.. So I'd say we should be safe when assuming bootloader is doing things correctly. Although, we could add a comment to the code that the test may not be 100% reliable in case the start sequence have not been issued properly.

Thanks for pointing this out!

It doesn't seem to much overhead to do:

	/*
	 * There is no register that tells us if the timer is running,
	 * so we have to resort to sample twice. The minimal frequency
	 * is 256 Hz (32768 Hz prescaled with 2**7).
	 */
	counter1 = readl(base + OMAP_WATCHDOG_CCR);
	mdelay(4);
	counter2 = readl(base + OMAP_WATCHDOG_CCR);
	return counter1 != counter2;

I'd say it's even worth to do:

	cntrl = readl(base + OMAP_WATCHDOG_CNTRL);
	if (cntrl & (1 << 5))
		shift = (cntrl >> 2) & 0x7;
	else
		shift = 0;
	counter1 = readl(base + OMAP_WATCHDOG_CCR);
	udelay(31 << shift);
	counter2 = readl(base + OMAP_WATCHDOG_CCR);
	return counter1 != counter2;

For some bonus points add some defines for the magic constants.

This is save as the OMAP_WATCHDOG_CNTRL doesn't seem to accept reads while the counter is running. Maybe even this could be used to detect a running timer?:

- enable timer:

	barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0xbbbb
	barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x4444

- read out WCLR

	barebox at TI AM335x BeagleBone black:/ md 0x44e35024+4
	44e35024: 00000020                                            ...

- write to WCLR

	barebox at TI AM335x BeagleBone black:/ mw 0x44e35024 0x0

- check result; didn't work

	barebox at TI AM335x BeagleBone black:/ md 0x44e35024+4
	44e35024: 00000020                                            ...

- stop timer

	barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0xaaaa
	barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x5555

- recheck WCLR

	barebox at TI AM335x BeagleBone black:/ md 0x44e35024+4
	44e35024: 00000020                                            ...

- write to WCLR

	barebox at TI AM335x BeagleBone black:/ mw 0x44e35024 0x0

- check result; write succeeded

	barebox at TI AM335x BeagleBone black:/ md 0x44e35024+4
	44e35024: 00000000                                           ....

(This is was tested on an AM335x.)

Best regards
Uwe

Quoted 60 lines · Uwe Kleine-K?nig

Quoted 27 lines · Timo Kokkonen

Quoted 17 lines · Uwe Kleine-K?nig

drivers/watchdog/omap_wdt.c static void omap_wdt_reload(struct omap_wdt_dev *wdev) · line 86

Quoted 6 lines · Timo Kokkonen

drivers/watchdog/omap_wdt.c +6 -0

+static int omap_wdt_is_running(struct omap_wdt_dev *wdev)
+{
+	void __iomem *base = wdev->base;
+
+	return readl_relaxed(base + OMAP_WATCHDOG_SPR) == 0x4444;
+}

This isn't reliable. The sequence needed to enable the watchdog is

	writel(0xbbbb, base + OMAP_WATCHDOG_SPR);
	writel(0x4444, base + OMAP_WATCHDOG_SPR);

The sequence to stop is:

	writel(0xaaaa, base + OMAP_WATCHDOG_SPR);
	writel(0x5555, base + OMAP_WATCHDOG_SPR);

But:

Quoted 8 lines

barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00005555 UU..
barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x4444 barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00004444 DD..

So the register contains 0x4444 but the timer doesn't run. So at best testing for 0x4444 is a good heuristic.

Quoted 8 lines

Yeah.. I don't think we can get any better than that. Unless we start checking the counter register and see whether it really counts or not, and I think that's a bit overkill.. So I'd say we should be safe when assuming bootloader is doing things correctly. Although, we could add a comment to the code that the test may not be 100% reliable in case the start sequence have not been issued properly.

Thanks for pointing this out!

It doesn't seem to much overhead to do:

	/*
	 * There is no register that tells us if the timer is running,
	 * so we have to resort to sample twice. The minimal frequency
	 * is 256 Hz (32768 Hz prescaled with 2**7).
	 */
	counter1 = readl(base + OMAP_WATCHDOG_CCR);
	mdelay(4);
	counter2 = readl(base + OMAP_WATCHDOG_CCR);
	return counter1 != counter2;

I'd say it's even worth to do:

	cntrl = readl(base + OMAP_WATCHDOG_CNTRL);
	if (cntrl & (1 << 5))
		shift = (cntrl >> 2) & 0x7;
	else
		shift = 0;
	counter1 = readl(base + OMAP_WATCHDOG_CCR);
	udelay(31 << shift);
	counter2 = readl(base + OMAP_WATCHDOG_CCR);
	return counter1 != counter2;

Quoted 3 lines

For some bonus points add some defines for the magic constants.

This is save as the OMAP_WATCHDOG_CNTRL doesn't seem to accept reads

Quoted 2 lines

while the counter is running. Maybe even this could be used to detect a running timer?:

Maybe, but you will get a data abort when the HW doesn't accept a read. How do you recover from that?

Also, it seems that for some reason the watchdog HW is very picky about which registers can be read in what condition. If I add the code to read watchdog counter register, I will get a data abort later at the end of the probe when the watchdog revision is read. I don't understand why that happens, this does not seem logica. Maybe it has got something to do with the fact that the watchdog is stopped? If I can't read the counter register, I can't really implement the above heuristic.. Any ideas?

Hello Timo,

Quoted 61 lines · Timo Kokkonen

Quoted 60 lines · Uwe Kleine-K?nig

Quoted 27 lines · Timo Kokkonen

Quoted 17 lines · Uwe Kleine-K?nig

Review

Quoted 6 lines · Timo Kokkonen

(quoted diff) +6 -0

+static int omap_wdt_is_running(struct omap_wdt_dev *wdev)
+{
+	void __iomem *base = wdev->base;
+
+	return readl_relaxed(base + OMAP_WATCHDOG_SPR) == 0x4444;
+}

This isn't reliable. The sequence needed to enable the watchdog is

	writel(0xbbbb, base + OMAP_WATCHDOG_SPR);
	writel(0x4444, base + OMAP_WATCHDOG_SPR);

The sequence to stop is:

	writel(0xaaaa, base + OMAP_WATCHDOG_SPR);
	writel(0x5555, base + OMAP_WATCHDOG_SPR);

But:

Quoted 8 lines

barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00005555 UU..
barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x4444 barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00004444 DD..

So the register contains 0x4444 but the timer doesn't run. So at best testing for 0x4444 is a good heuristic.

Quoted 8 lines

Yeah.. I don't think we can get any better than that. Unless we start checking the counter register and see whether it really counts or not, and I think that's a bit overkill.. So I'd say we should be safe when assuming bootloader is doing things correctly. Although, we could add a comment to the code that the test may not be 100% reliable in case the start sequence have not been issued properly.

Thanks for pointing this out!

It doesn't seem to much overhead to do:

	/*
	 * There is no register that tells us if the timer is running,
	 * so we have to resort to sample twice. The minimal frequency
	 * is 256 Hz (32768 Hz prescaled with 2**7).
	 */
	counter1 = readl(base + OMAP_WATCHDOG_CCR);
	mdelay(4);
	counter2 = readl(base + OMAP_WATCHDOG_CCR);
	return counter1 != counter2;

I'd say it's even worth to do:

	cntrl = readl(base + OMAP_WATCHDOG_CNTRL);
	if (cntrl & (1 << 5))
		shift = (cntrl >> 2) & 0x7;
	else
		shift = 0;
	counter1 = readl(base + OMAP_WATCHDOG_CCR);
	udelay(31 << shift);
	counter2 = readl(base + OMAP_WATCHDOG_CCR);
	return counter1 != counter2;

Quoted 3 lines

Quoted 3 lines

For some bonus points add some defines for the magic constants.

This is save as the OMAP_WATCHDOG_CNTRL doesn't seem to accept reads

s/reads/writes/ in the above line.

Quoted 6 lines

Quoted 2 lines

while the counter is running. Maybe even this could be used to detect a running timer?:

Maybe, but you will get a data abort when the HW doesn't accept a read. How do you recover from that?

Quoted 8 lines

Also, it seems that for some reason the watchdog HW is very picky about which registers can be read in what condition. If I add the code to read watchdog counter register, I will get a data abort later at the end of the probe when the watchdog revision is read. I don't understand why that happens, this does not seem logica. Maybe it has got something to do with the fact that the watchdog is stopped? If I can't read the counter register, I can't really implement the above heuristic.. Any ideas?

Which SoC do you use for your tests? Sounds strange. If you provide me your wip patch I can take a look.

Quoted 48 lines · Uwe Kleine-K?nig

Hello,

Quoted 20 lines · Timo Kokkonen

Use the new watchdog core extensions to let watchdog core take over boot time watchdog behavior. The difference is that early-timeout-sec device tree property becomes available for this driver and a running watchdog is not stopped unless the core decides to stop it.

Omap watchdog is running by default in the boot up but bootloader might have stopped it. Therefore we fill the WDOG_HW_RUNNING_AT_BOOT bit depending on the actual watchdog state so that the watchdog core can act properly.

Signed-off-by Timo Kokkonen <[email protected]>

---
 drivers/watchdog/omap_wdt.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

drivers/watchdog/omap_wdt.c +0 -0

index bbaf39a..7164f2e 100644

drivers/watchdog/omap_wdt.c static void omap_wdt_reload(struct omap_wdt_dev *wdev) · line 86

Quoted 9 lines

drivers/watchdog/omap_wdt.c +6 -0

@@ -78,6 +78,13 @@ static void omap_wdt_reload(struct omap_wdt_dev *wdev)
 	/* reloaded WCRR from WLDR */
 }
 
+static int omap_wdt_is_running(struct omap_wdt_dev *wdev)
+{
+	void __iomem *base = wdev->base;
+
+	return readl_relaxed(base + OMAP_WATCHDOG_SPR) == 0x4444;
+}

This isn't reliable. The sequence needed to enable the watchdog is

	writel(0xbbbb, base + OMAP_WATCHDOG_SPR);
	writel(0x4444, base + OMAP_WATCHDOG_SPR);

The sequence to stop is:

	writel(0xaaaa, base + OMAP_WATCHDOG_SPR);
	writel(0x5555, base + OMAP_WATCHDOG_SPR);

But:

barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00005555 UU..
barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x4444 barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
44e35048: 00004444 DD..

Quoted 2 lines

So the register contains 0x4444 but the timer doesn't run. So at best testing for 0x4444 is a good heuristic.

Yeah.. I don't think we can get any better than that. Unless we start checking the counter register and see whether it really counts or not, and I think that's a bit overkill.. So I'd say we should be safe when assuming

bootloader is doing things correctly. Although, we could add a comment to

Hello,

I talked to Marc Kleine-Budde about your approach, thought a bit more about it and want to share a few thoughts with you.

Actually your series addresses three different problems

a) some watchdog hardware isn't stoppable;
b) some watchdog hardware has short maximal timeout;
c) what to do with a watchdog that is already running at probe time?

The common solution is to add a mid-layer between userspace and the driver that bridges the possible hardware limitations when userspace wants to stop the watchdog or set a big timeout value. c) is a bit different but could make use of the infrastructure that is introduced while fixing a+b). The main difference between a+b) and c) is that for
c) you have to introduce some policy. If the series were mine I'd first do three commits that address a), b) and c) each. Then convert drivers to it.

Guenter and I already said something similar, but I will eventually repeat it here more explicitly: When introducing a midlayer that abstracts between hardware and it's users the IMHO most important thing to get right is to be explicit about which side of a midlayer you're currently working at. That is, be explicit about watchdog_is_running:
Does it mean the hardware is running, or does userspace believe the watchdog to be active? Same for timeout, stoppable etc.pp.

When you consider changing the unit the watchdog core is using, why not change to nano seconds and 64 bit variables? You might be able to copy some algorithms and ideas from the timer core that uses these.

For c), I'd want to have a compile time setting that specifies the default value for the policy:
- disable at probe
- don't touch a running timer

- start at probe
This is friendly to distributions that might want to set "disable at probe"-default to be on the safe side. Still make this overridable by a kernel parameter. I'm not sure that "start at probe" is a sensible feature. (Either you want to start the watchdog early, in this case even before Linux starts; or you're not that strict then it doesn't matter much if it takes yet another little while until your application that pets the watchdog is up.)

Quoted 11 lines · Uwe Kleine-K?nig

Hello,

I talked to Marc Kleine-Budde about your approach, thought a bit more about it and want to share a few thoughts with you.

Actually your series addresses three different problems

a) some watchdog hardware isn't stoppable;
b) some watchdog hardware has short maximal timeout;
c) what to do with a watchdog that is already running at probe time?

Quoted 8 lines

The common solution is to add a mid-layer between userspace and the driver that bridges the possible hardware limitations when userspace wants to stop the watchdog or set a big timeout value. c) is a bit different but could make use of the infrastructure that is introduced while fixing a+b). The main difference between a+b) and c) is that for
c) you have to introduce some policy. If the series were mine I'd first do three commits that address a), b) and c) each. Then convert drivers to it.

The infrastructure needed for both a and b is the same, the actual code to implement either a or b is also touching the same functions and quite slim, so I think it may not be feasible to separate those in their own patches. But I'll think about it and see if logical separation makes sense.

Quoted 7 lines

Guenter and I already said something similar, but I will eventually repeat it here more explicitly: When introducing a midlayer that abstracts between hardware and it's users the IMHO most important thing to get right is to be explicit about which side of a midlayer you're currently working at. That is, be explicit about watchdog_is_running:
Does it mean the hardware is running, or does userspace believe the watchdog to be active? Same for timeout, stoppable etc.pp.

Yes, I agree. Thanks for clarifying this. I will keep this in mind and try to be explicit when naming functions and variables. The best way would be to also rename the existing variables too, but that would also touch all the existing drivers. Keeping the current naming scheme in place no doubt is less explicit about which side of the new midlayer they are working on. But luckily the logic of the existing driver doesn't change, they are all exposing things directly to userland and they can be documented as is. The new variables and functions add interfaces only between the new midlayer and HW, so they are explicit as well. So I think it is a fair compromise to leave all existing interface towards the user space as is.

Quoted 3 lines

When you consider changing the unit the watchdog core is using, why not change to nano seconds and 64 bit variables? You might be able to copy some algorithms and ideas from the timer core that uses these.

Yes, sounds like a good idea. I will look into it.

Hello,

Quoted 21 lines · Timo Kokkonen

Quoted 11 lines · Uwe Kleine-K?nig

Hello,

I talked to Marc Kleine-Budde about your approach, thought a bit more about it and want to share a few thoughts with you.

Actually your series addresses three different problems

a) some watchdog hardware isn't stoppable;
b) some watchdog hardware has short maximal timeout;
c) what to do with a watchdog that is already running at probe time?

Quoted 8 lines

The common solution is to add a mid-layer between userspace and the driver that bridges the possible hardware limitations when userspace wants to stop the watchdog or set a big timeout value. c) is a bit different but could make use of the infrastructure that is introduced while fixing a+b). The main difference between a+b) and c) is that for
c) you have to introduce some policy. If the series were mine I'd first do three commits that address a), b) and c) each. Then convert drivers to it.

Quoted 5 lines

The infrastructure needed for both a and b is the same, the actual code to implement either a or b is also touching the same functions and quite slim, so I think it may not be feasible to separate those in their own patches. But I'll think about it and see if logical separation makes sense.

Yeah, probably you have to actually try it to see if it's sensible.

Quoted 8 lines

Quoted 7 lines

Guenter and I already said something similar, but I will eventually repeat it here more explicitly: When introducing a midlayer that abstracts between hardware and it's users the IMHO most important thing to get right is to be explicit about which side of a midlayer you're currently working at. That is, be explicit about watchdog_is_running:
Does it mean the hardware is running, or does userspace believe the watchdog to be active? Same for timeout, stoppable etc.pp.

Quoted 12 lines

Yes, I agree. Thanks for clarifying this. I will keep this in mind and try to be explicit when naming functions and variables. The best way would be to also rename the existing variables too, but that would also touch all the existing drivers. Keeping the current naming scheme in place no doubt is less explicit about which side of the new midlayer they are working on. But luckily the logic of the existing driver doesn't change, they are all exposing things directly to userland and they can be documented as is. The new variables and functions add interfaces only between the new midlayer and HW, so they are explicit as well. So I think it is a fair compromise to leave all existing interface towards the user space as is.

Maybe a good first step before addressing a+b+c) is to cleanup the naming? Not sure how well this works though.

Quoted 5 lines

Quoted 3 lines

When you consider changing the unit the watchdog core is using, why not change to nano seconds and 64 bit variables? You might be able to copy some algorithms and ideas from the timer core that uses these.

Yes, sounds like a good idea. I will look into it.

I wouldn't be surprised if you had to build a parallel watchdog device model for that and assert that both work until all drivers are converted to the new model. Sounds like fun :-)