watchdog: Add support for keepalives triggered by infrastructure

PATCH Superseded Guenter Roeck 3 participants 24 messages Nov 23, 2015 – Nov 24, 2015

Subsystems (4)

Documentation Arm i.MX and Layerscape Arm/Microchip (AT91) SoC support Watchdog device drivers

lore

Share & tools

lore

Guenter Roeck

author maintainer

2015-11-23 03:20 UTC #0 lore raw

v5 cover watchdog: Add support for keepalives triggered by infrastructure

The watchdog infrastructure is currently purely passive, meaning it only passes information from user space to drivers and vice versa.

Since watchdog hardware tends to have its own quirks, this can result in quite complex watchdog drivers. A number of scanarios are especially common.

- A watchdog is always active and can not be disabled, or can not be disabled once enabled. To support such hardware, watchdog drivers have to implement their own timers and use those timers to trigger watchdog keepalives while the watchdog device is not or not yet opened.
- A variant of this is the desire to enable a watchdog as soon as its driver has been instantiated, to protect the system while it is still booting up, but the watchdog daemon is not yet running.
- Some watchdogs have a very short maximum timeout, in the range of just a few seconds. Such low timeouts are difficult if not impossible to support from user space. Drivers supporting such watchdog hardware need to implement a timer function to augment heartbeats from user space.

This patch set solves the above problems while keeping changes to the watchdog core minimal.

- A new status flag, WDOG_RUNNING, informs the watchdog subsystem that a watchdog is running, and that the watchdog subsystem needs to generate heartbeat requests while the associated watchdog device is closed.
- A new parameter in the watchdog data structure, max_hw_timeout_ms, informs the watchdog subsystem about a maximum hardware timeout. The watchdog subsystem uses this information together with the configured timeout and the maximum permitted timeout to determine if it needs to generate additional heartbeat requests.

As part of this patchset, the semantics of the 'timeout' variable and of the WDOG_ACTIVE flag are changed slightly.

Per the current watchdog kernel API, the 'timeout' variable is supposed to reflect the actual hardware watcdog timeout. WDOG_ACTIVE is supposed to reflect if the hardware watchdog is running or not.

Unfortunately, this does not always reflect reality. In drivers which solve the above mentioned problems internally, 'timeout' is the watchdog timeout as seen from user space, and WDOG_ACTIVE reflects that user space is expected to send keepalive requests to the watchdog driver.

After this patch set is applied, this so far inofficial interpretation is the 'official' semantics for the timeout variable and the WDOG_ACTIVE flag. In other words, both values no longer reflect the hardware watchdog status, but its status as seen from user space.

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 unconditionally ensure that the minimum timeout 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 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.

Patch #6 to #8 are example conversions of some watchdog drivers. Those patches will require testing and are marked as RFT.

The patch set is also available in branch watchdog-timer of
git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git.
In this branch, the series is rebased on top of the watchdog-next branch in the same repository. This was done since other pending changes in the watchdog subsystem cause merge conflicts, and we want to be able to test all pending changes together.

The patch set was inspired by an earlier patch set from Timo Kokonnen.

v5:
- Patches #1 and #2 of the original patch series are now in mainline and have been dropped.
- Rebased to v4.4-rc1.
- Added patch to simplify watchdog_update_worker().
v4:
- Rebased to v4.3-rc3
- Rearranged patch sequence
- Dropped gpio driver patch. The driver was changed since v4.2, and merging the changes turned out to be too difficult.
- Various other cleanups as listed in individual patches
v3:
- Rebased to v4.2-rc8
- 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.
- Added patch 9/9.
v2:
- Rebased to v4.2-rc5
- 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.

Guenter Roeck

author maintainer

2015-11-23 03:20 UTC #1 lore raw

PATCH v5 1/8 watchdog: Introduce hardware maximum timeout in watchdog core

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.

Cc Timo Kokkonen <[email protected]>

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

---
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 |  28 ++++-
 drivers/watchdog/watchdog_dev.c                | 135 ++++++++++++++++++++++---
 include/linux/watchdog.h                       |  40 ++++++--
 3 files changed, 173 insertions(+), 30 deletions(-)

Documentation/watchdog/watchdog-kernel-api.txt +24 -4

index d8b0d3367706..f66859117d1f 100644
@@ -53,9 +53,12 @@ struct watchdog_device {
 	unsigned int timeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
+	unsigned int max_hw_timeout_ms;
 	void *driver_data;
-	struct mutex lock;
 	unsigned long status;
+	struct mutex lock;
+	unsigned long last_keepalive;
+	struct delayed_work work;
 	struct list_head deferred;
 };

@@ -73,18 +76,31 @@ 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, unless WDOG_ACTIVE
+  is set and userspace failed to send a heartbeat for at least 'timeout'
+  seconds.
 * bootstatus: status of the device after booting (reported with watchdog
   WDIOF_* status bits).
 * driver_data: a pointer to the drivers private data of a watchdog device.
   This data should only be accessed via the watchdog_set_drvdata and
   watchdog_get_drvdata routines.
-* lock: Mutex for WatchDog Timer Driver Core internal use only.
 * status: this field contains a number of status bits that give extra
   information about the status of the device (Like: is the watchdog timer
   running/active, is the nowayout bit set, is the device opened via
   the /dev/watchdog interface or not, ...).
+* lock: Mutex for WatchDog Timer Driver Core internal use only.
+* last_keepalive: Time of most recent keepalive triggered from user space,
+  in jiffies.
+* work: Worker data structure for WatchDog Timer Driver Core internal use only.
 * deferred: entry in wtd_deferred_reg_list which is used to
   register early initialized watchdogs.

@@ -160,7 +176,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 hw_max_timeout_ms set the hardware watchdog timeout
+  to the minimum of timeout and hw_max_timeout_ms. Those drivers set the
+  timeout value of the watchdog_device either to the requested timeout value
+  (if it is larger than hw_max_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 +119 ~~-16~~

211 lines not on this page · raw message

include/linux/watchdog.h +30 ~~-10~~

index 027b1f43f12d..b535b02b1d7f 100644
@@ -10,8 +10,10 @@
 
 
 #include <linux/bitops.h>
-#include <linux/device.h>
 #include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/workqueue.h>
 #include <uapi/linux/watchdog.h>
 
 struct watchdog_ops;

@@ -61,12 +63,21 @@ 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.
  * @driver-data:Pointer to the drivers private data.
- * @lock:	Lock for watchdog core internal use only.
  * @status:	Field that contains the devices internal status bits.
- * @deferred: entry in wtd_deferred_reg_list which is used to
- *			   register early initialized watchdogs.
+ * @lock:	Lock for watchdog core internal use only.
+ * @last_keepalive:
+ *		Time of most recent keepalive triggered from user space,
+ *		in jiffies (watchdog core internal).
+ * @work:	Data structure for worker function (watchdog core internal).
+ * @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.

@@ -88,8 +99,8 @@ struct watchdog_device {
 	unsigned int timeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
+	unsigned int max_hw_timeout_ms;
 	void *driver_data;
-	struct mutex lock;
 	unsigned long status;
 /* Bit numbers for status flags */
 #define WDOG_ACTIVE		0	/* Is the watchdog running/active */

@@ -97,6 +108,10 @@ 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 */
+	/* the following variables are for internal use only */
+	struct mutex lock;
+	unsigned long last_keepalive;
+	struct delayed_work work;
 	struct list_head deferred;
 };

@@ -121,13 +136,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

Uwe Kleine-König

2015-11-23 07:53 UTC #2 lore raw

Re: [PATCH v5 1/8] watchdog: Introduce hardware maximum timeout in watchdog core

Hello Guenter,

first of all thanks for picking this series up again.

I think all of this feedback doesn't need to stop your patches getting in. It should all be possible to improve afterwards.

Documentation/watchdog/watchdog-kernel-api.txt they are supported. These optional routines/operations are: · line 181

Quoted 8 lines · Guenter Roeck

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

@@ -160,7 +176,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 hw_max_timeout_ms set the hardware watchdog timeout
+  to the minimum of timeout and hw_max_timeout_ms. Those drivers set the

Actually this is something that the wdg core could abstract for drivers. Oh well, apart from hw_max_timeout_ms having ms accuracy.

Quoted 9 lines

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

+  timeout value of the watchdog_device either to the requested timeout value
+  (if it is larger than hw_max_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 +0 -0

index 56a649e66eb2..1dba3f57dba3 100644

drivers/watchdog/watchdog_dev.c static dev_t watchdog_devt; · line 82

Quoted 10 lines

drivers/watchdog/watchdog_dev.c +9 -0

[...]
+static long watchdog_next_keepalive(struct watchdog_device *wdd)
+{
+	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 = wdd->last_keepalive + msecs_to_jiffies(timeout_ms);

I think it's sensible to store

last_keepalive + timeout

(i.e. the expected expiration time) in struct watchdog_device instead of last_keepalive. This moves the (maybe expensive) calculation to a context that has userspace interaction anyhow. On the other hand this complicates the set_timeout call. Hmm.

Quoted 38 lines

drivers/watchdog/watchdog_dev.c +20 -8

+	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);
+}
[...]

@@ -61,26 +137,25 @@ static struct watchdog_device *old_wdd;
 
 static int watchdog_ping(struct watchdog_device *wdd)
 {
-	int err = 0;
+	int err;
 
 	mutex_lock(&wdd->lock);
+	wdd->last_keepalive = jiffies;
+	err = _watchdog_ping(wdd);
+	mutex_unlock(&wdd->lock);
 
-	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
-		err = -ENODEV;
-		goto out_ping;
-	}
+	return err;
+}
 
-	if (!watchdog_active(wdd))
-		goto out_ping;
+static void watchdog_ping_work(struct work_struct *work)
+{
+	struct watchdog_device *wdd;
 
-	if (wdd->ops->ping)

drivers/watchdog/watchdog_dev.c static struct watchdog_device *old_wdd; · line 83

Quoted 10 lines

drivers/watchdog/watchdog_dev.c +3 -5

-		err = wdd->ops->ping(wdd);	/* ping the watchdog */
-	else
-		err = wdd->ops->start(wdd);	/* restart watchdog */
+	wdd = container_of(to_delayed_work(work), struct watchdog_device, work);
 
-out_ping:
+	mutex_lock(&wdd->lock);
+	_watchdog_ping(wdd);
 	mutex_unlock(&wdd->lock);
-	return err;

Calling this function might come after last_keepalive + timeout in which case the watchdog shouldn't be pinged.

drivers/watchdog/watchdog_dev.c static int watchdog_start(struct watchdog_device *wdd) · line 189

Quoted 13 lines

drivers/watchdog/watchdog_dev.c +4 -1

 }
 
 /*

@@ -107,8 +182,11 @@ static int watchdog_start(struct watchdog_device *wdd)
 		goto out_start;
 
 	err = wdd->ops->start(wdd);
-	if (err == 0)
+	if (err == 0) {
 		set_bit(WDOG_ACTIVE, &wdd->status);
+		wdd->last_keepalive = jiffies;
+		watchdog_update_worker(wdd, true);
+	}

I think it's more correct to sample jiffies before calling .start. Something like:

	unsigned long started_at = jiffies;

	err = wdd->ops->start(wdd);
	if (err == 0)
		wdd->last_keepalive = jiffies;

drivers/watchdog/watchdog_dev.c static int watchdog_start(struct watchdog_device *wdd) · line 192

Quoted 3 lines

drivers/watchdog/watchdog_dev.c +0 -0
 out_start:
 	mutex_unlock(&wdd->lock);

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Guenter Roeck

author maintainer

2015-11-23 16:15 UTC #3 lore raw

Re: [PATCH v5 1/8] watchdog: Introduce hardware maximum timeout in watchdog core

Hi Uwe,

Quoted 8 lines · Uwe Kleine-König

Hello Guenter,
first of all thanks for picking this series up again.
I think all of this feedback doesn't need to stop your patches getting in. It should all be possible to improve afterwards.
On Sun, Nov 22, 2015 at 07:20:58PM -0800, Guenter Roeck wrote:

Quoted 12 lines

Documentation/watchdog/watchdog-kernel-api.txt they are supported. These optional routines/operations are: · line 181
Quoted 8 lines
Documentation/watchdog/watchdog-kernel-api.txt +3 -1
@@ -160,7 +176,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 hw_max_timeout_ms set the hardware watchdog timeout
+  to the minimum of timeout and hw_max_timeout_ms. Those drivers set the
Actually this is something that the wdg core could abstract for drivers. Oh well, apart from hw_max_timeout_ms having ms accuracy.

Not that sure. about the abstraction. The actual timeout to set depends on the hardware, and may have an unknown granularity. The watchdog core can not predict what that granularity would be. We can play with it, and try if it can be done, but I really would like this to be a separate patch.

hw_max_timeout is in ms because some watchdogs have a very low maximum HW timeout.

Quoted 22 lines

Quoted 9 lines

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

+  timeout value of the watchdog_device either to the requested timeout value
+  (if it is larger than hw_max_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 +0 -0

index 56a649e66eb2..1dba3f57dba3 100644

drivers/watchdog/watchdog_dev.c static dev_t watchdog_devt; · line 82

Quoted 10 lines

drivers/watchdog/watchdog_dev.c +9 -0

[...]
+static long watchdog_next_keepalive(struct watchdog_device *wdd)
+{
+	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 = wdd->last_keepalive + msecs_to_jiffies(timeout_ms);

I think it's sensible to store

Quoted 7 lines

last_keepalive + timeout
(i.e. the expected expiration time) in struct watchdog_device instead of last_keepalive. This moves the (maybe expensive) calculation to a context that has userspace interaction anyhow. On the other hand this complicates the set_timeout call. Hmm.

I would rather keep the code simple. It is not as if this is called all the time. Also, I need last_keepalive later in the series to determine if the minimum timeout between hardware pings has elapsed, so we would need both.

Quoted 43 lines

drivers/watchdog/watchdog_dev.c +21 ~~-11~~

+	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);
+}
[...]

@@ -61,26 +137,25 @@ static struct watchdog_device *old_wdd;
 
  static int watchdog_ping(struct watchdog_device *wdd)
  {
-	int err = 0;
+	int err;
 
  	mutex_lock(&wdd->lock);
+	wdd->last_keepalive = jiffies;
+	err = _watchdog_ping(wdd);
+	mutex_unlock(&wdd->lock);
 
-	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
-		err = -ENODEV;
-		goto out_ping;
-	}
+	return err;
+}
 
-	if (!watchdog_active(wdd))
-		goto out_ping;
+static void watchdog_ping_work(struct work_struct *work)
+{
+	struct watchdog_device *wdd;
 
-	if (wdd->ops->ping)
-		err = wdd->ops->ping(wdd);	/* ping the watchdog */
-	else
-		err = wdd->ops->start(wdd);	/* restart watchdog */
+	wdd = container_of(to_delayed_work(work), struct watchdog_device, work);

Quoted 9 lines

drivers/watchdog/watchdog_dev.c static struct watchdog_device *old_wdd; · line 83
Quoted 5 lines
drivers/watchdog/watchdog_dev.c +2 -2
-out_ping:
+	mutex_lock(&wdd->lock);
+	_watchdog_ping(wdd);
  	mutex_unlock(&wdd->lock);
-	return err;
Calling this function might come after last_keepalive + timeout in which case the watchdog shouldn't be pinged.

Unless the code is wrong, the last time this function is called should be at (timeout - max_hw_timeout), ie well before the timeout elapsed. Given that, I don't think this is something to be concerned about.

Quoted 17 lines

drivers/watchdog/watchdog_dev.c static int watchdog_start(struct watchdog_device *wdd) · line 189
Quoted 13 lines
drivers/watchdog/watchdog_dev.c +4 -1
  }
 
  /*
@@ -107,8 +182,11 @@ static int watchdog_start(struct watchdog_device *wdd)
  		goto out_start;
 
  	err = wdd->ops->start(wdd);
-	if (err == 0)
+	if (err == 0) {
  		set_bit(WDOG_ACTIVE, &wdd->status);
+		wdd->last_keepalive = jiffies;
+		watchdog_update_worker(wdd, true);
+	}
I think it's more correct to sample jiffies before calling .start. Something like:

Quoted 6 lines

	unsigned long started_at = jiffies;

	err = wdd->ops->start(wdd);
	if (err == 0)
		wdd->last_keepalive = jiffies;

I assume you mean

		wdd->last_keepalive = started_at;

Agreed, that makes more sense. I'll change the code accordingly.

Thanks,
Guenter

Uwe Kleine-König

2015-11-23 18:26 UTC #4 lore raw

Re: [PATCH v5 1/8] watchdog: Introduce hardware maximum timeout in watchdog core

Hello Guenter,

Quoted 14 lines · Guenter Roeck

Quoted 13 lines · Uwe Kleine-König
Documentation/watchdog/watchdog-kernel-api.txt they are supported. These optional routines/operations are: · line 181
Quoted 8 lines · Guenter Roeck
Documentation/watchdog/watchdog-kernel-api.txt +3 -1
@@ -160,7 +176,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 hw_max_timeout_ms set the hardware watchdog timeout
+  to the minimum of timeout and hw_max_timeout_ms. Those drivers set the
Actually this is something that the wdg core could abstract for drivers. Oh well, apart from hw_max_timeout_ms having ms accuracy.

Quoted 2 lines

Not that sure. about the abstraction. The actual timeout to set depends on the hardware, and may have an unknown granularity. The watchdog core can

What I wanted to say is that the driver core can do (ignoring scaling due to different units):

	if (timeout > wdd->hw_max_timeout_ms)
		wdd->set_timeout(wdd->hw_max_timeout_ms);
	else
		wdd->set_timeout(timeout)

So that the device specific driver can assume that timeout passed to its set_timeout callback is always less than or equal to hw_max_timeout_ms. And so it doesn't need to compare against hw_max_timeout_ms again.

Quoted 36 lines

not predict what that granularity would be. We can play with it, and try if it can be done, but I really would like this to be a separate patch.
hw_max_timeout is in ms because some watchdogs have a very low maximum HW timeout.
Quoted 22 lines
Quoted 9 lines
Documentation/watchdog/watchdog-kernel-api.txt +2 -0
+  timeout value of the watchdog_device either to the requested timeout value
+  (if it is larger than hw_max_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 +0 -0
index 56a649e66eb2..1dba3f57dba3 100644
drivers/watchdog/watchdog_dev.c static dev_t watchdog_devt; · line 82
Quoted 10 lines
drivers/watchdog/watchdog_dev.c +9 -0
[...]
+static long watchdog_next_keepalive(struct watchdog_device *wdd)
+{
+	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 = wdd->last_keepalive + msecs_to_jiffies(timeout_ms);
I think it's sensible to store
Quoted 7 lines
last_keepalive + timeout
(i.e. the expected expiration time) in struct watchdog_device instead of last_keepalive. This moves the (maybe expensive) calculation to a context that has userspace interaction anyhow. On the other hand this complicates the set_timeout call. Hmm.

Quoted 4 lines

I would rather keep the code simple. It is not as if this is called all the time. Also, I need last_keepalive later in the series to determine if the minimum timeout between hardware pings has elapsed, so we would need both.

I'm not sure my suggestion improves the situation, but I will keep my idea in mind and check once the dust settled.

Quoted 52 lines

Quoted 43 lines

drivers/watchdog/watchdog_dev.c +21 ~~-11~~

+	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);
+}
[...]

@@ -61,26 +137,25 @@ static struct watchdog_device *old_wdd;
 
 static int watchdog_ping(struct watchdog_device *wdd)
 {
-	int err = 0;
+	int err;
 
 	mutex_lock(&wdd->lock);
+	wdd->last_keepalive = jiffies;
+	err = _watchdog_ping(wdd);
+	mutex_unlock(&wdd->lock);
 
-	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
-		err = -ENODEV;
-		goto out_ping;
-	}
+	return err;
+}
 
-	if (!watchdog_active(wdd))
-		goto out_ping;
+static void watchdog_ping_work(struct work_struct *work)
+{
+	struct watchdog_device *wdd;
 
-	if (wdd->ops->ping)
-		err = wdd->ops->ping(wdd);	/* ping the watchdog */
-	else
-		err = wdd->ops->start(wdd);	/* restart watchdog */
+	wdd = container_of(to_delayed_work(work), struct watchdog_device, work);

Quoted 9 lines

drivers/watchdog/watchdog_dev.c static struct watchdog_device *old_wdd; · line 83
Quoted 5 lines
drivers/watchdog/watchdog_dev.c +2 -2
-out_ping:
+	mutex_lock(&wdd->lock);
+	_watchdog_ping(wdd);
 	mutex_unlock(&wdd->lock);
-	return err;
Calling this function might come after last_keepalive + timeout in which case the watchdog shouldn't be pinged.

Quoted 3 lines

Unless the code is wrong, the last time this function is called should be at (timeout - max_hw_timeout), ie well before the timeout elapsed. Given that, I don't think this is something to be concerned about.

Depends on max_hw_timeout and the load of the machine if that can happen. And thinking again, if the ping didn't come in until last_keepalive + timeout, the machine is reset anyhow ...

Quoted 17 lines

Quoted 17 lines
drivers/watchdog/watchdog_dev.c static int watchdog_start(struct watchdog_device *wdd) · line 189
Quoted 13 lines
drivers/watchdog/watchdog_dev.c +4 -1
 }
 
 /*
@@ -107,8 +182,11 @@ static int watchdog_start(struct watchdog_device *wdd)
 		goto out_start;
 
 	err = wdd->ops->start(wdd);
-	if (err == 0)
+	if (err == 0) {
 		set_bit(WDOG_ACTIVE, &wdd->status);
+		wdd->last_keepalive = jiffies;
+		watchdog_update_worker(wdd, true);
+	}
I think it's more correct to sample jiffies before calling .start. Something like:

Quoted 8 lines

Quoted 6 lines

	unsigned long started_at = jiffies;

	err = wdd->ops->start(wdd);
	if (err == 0)
		wdd->last_keepalive = jiffies;

I assume you mean

		wdd->last_keepalive = started_at;

right.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Guenter Roeck

author maintainer

2015-11-23 21:55 UTC #5 lore raw

Re: [PATCH v5 1/8] watchdog: Introduce hardware maximum timeout in watchdog core

Hi Uwe,

Quoted 23 lines · Uwe Kleine-König

Hello Guenter,
Quoted 14 lines · Guenter Roeck
Quoted 13 lines · Uwe Kleine-König
Documentation/watchdog/watchdog-kernel-api.txt they are supported. These optional routines/operations are: · line 181
Quoted 8 lines · Guenter Roeck
Documentation/watchdog/watchdog-kernel-api.txt +3 -1
@@ -160,7 +176,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 hw_max_timeout_ms set the hardware watchdog timeout
+  to the minimum of timeout and hw_max_timeout_ms. Those drivers set the
Actually this is something that the wdg core could abstract for drivers. Oh well, apart from hw_max_timeout_ms having ms accuracy.
Quoted 2 lines
Not that sure. about the abstraction. The actual timeout to set depends on the hardware, and may have an unknown granularity. The watchdog core can
What I wanted to say is that the driver core can do (ignoring scaling due to different units):

Quoted 9 lines

	if (timeout > wdd->hw_max_timeout_ms)
		wdd->set_timeout(wdd->hw_max_timeout_ms);
	else
		wdd->set_timeout(timeout)
So that the device specific driver can assume that timeout passed to its set_timeout callback is always less than or equal to hw_max_timeout_ms. And so it doesn't need to compare against hw_max_timeout_ms again.

I catually tried that at some point, but gave up since it ended up adding a lot of complexity.

Problem is that the set_timeout function in the driver code sets wdd->timeout based on the parameter it receives, and it can be different from both 'timeout' and wdd->hw_max_timeout_ms. On top of that, we have to deal with the changed units, which would either require a different callback function or we would have to change all drivers to milli-seconds.

I would prefer to get the patchset accepted, and then someone can spend time trying to clean this up further.

Quoted 96 lines

Quoted 36 lines
not predict what that granularity would be. We can play with it, and try if it can be done, but I really would like this to be a separate patch.
hw_max_timeout is in ms because some watchdogs have a very low maximum HW timeout.
Quoted 22 lines
Quoted 9 lines
Documentation/watchdog/watchdog-kernel-api.txt +2 -0
+  timeout value of the watchdog_device either to the requested timeout value
+  (if it is larger than hw_max_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 +0 -0
index 56a649e66eb2..1dba3f57dba3 100644
drivers/watchdog/watchdog_dev.c static dev_t watchdog_devt; · line 82
Quoted 10 lines
drivers/watchdog/watchdog_dev.c +9 -0
[...]
+static long watchdog_next_keepalive(struct watchdog_device *wdd)
+{
+	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 = wdd->last_keepalive + msecs_to_jiffies(timeout_ms);
I think it's sensible to store
Quoted 7 lines
last_keepalive + timeout
(i.e. the expected expiration time) in struct watchdog_device instead of last_keepalive. This moves the (maybe expensive) calculation to a context that has userspace interaction anyhow. On the other hand this complicates the set_timeout call. Hmm.
Quoted 4 lines
I would rather keep the code simple. It is not as if this is called all the time. Also, I need last_keepalive later in the series to determine if the minimum timeout between hardware pings has elapsed, so we would need both.
I'm not sure my suggestion improves the situation, but I will keep my idea in mind and check once the dust settled.
Quoted 52 lines
Quoted 52 lines
Quoted 38 lines
drivers/watchdog/watchdog_dev.c +20 -8
+	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);
+}
[...]
@@ -61,26 +137,25 @@ static struct watchdog_device *old_wdd;
 
 static int watchdog_ping(struct watchdog_device *wdd)
 {
-	int err = 0;
+	int err;
 
 	mutex_lock(&wdd->lock);
+	wdd->last_keepalive = jiffies;
+	err = _watchdog_ping(wdd);
+	mutex_unlock(&wdd->lock);
 
-	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
-		err = -ENODEV;
-		goto out_ping;
-	}
+	return err;
+}
 
-	if (!watchdog_active(wdd))
-		goto out_ping;
+static void watchdog_ping_work(struct work_struct *work)
+{
+	struct watchdog_device *wdd;
 
-	if (wdd->ops->ping)
drivers/watchdog/watchdog_dev.c static struct watchdog_device *old_wdd; · line 83
Quoted 10 lines
drivers/watchdog/watchdog_dev.c +3 -5
-		err = wdd->ops->ping(wdd);	/* ping the watchdog */
-	else
-		err = wdd->ops->start(wdd);	/* restart watchdog */
+	wdd = container_of(to_delayed_work(work), struct watchdog_device, work);
 
-out_ping:
+	mutex_lock(&wdd->lock);
+	_watchdog_ping(wdd);
 	mutex_unlock(&wdd->lock);
-	return err;
Calling this function might come after last_keepalive + timeout in which case the watchdog shouldn't be pinged.

Quoted 8 lines

Quoted 3 lines
Unless the code is wrong, the last time this function is called should be at (timeout - max_hw_timeout), ie well before the timeout elapsed. Given that, I don't think this is something to be concerned about.
Depends on max_hw_timeout and the load of the machine if that can happen. And thinking again, if the ping didn't come in until last_keepalive + timeout, the machine is reset anyhow ...

If the worker (which is, after all, running at high priority) doesn't get time to run by the time the watchdog expires, the system is really in bad trouble. And, yes, you are right, if the watchdog is working we should never hit that situation to start with.

Thanks,
Guenter

Uwe Kleine-König

Acked-by on v5 1/8

2015-11-24 07:16 UTC #6 lore raw

Re: [PATCH v5 1/8] watchdog: Introduce hardware maximum timeout in watchdog core

In reply to Guenter Roeck

Hello Guenter,

Quoted 10 lines · Guenter Roeck

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.
Cc Timo Kokkonen <[email protected]>
Signed-off-by Guenter Roeck <[email protected]>

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

Thanks
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Guenter Roeck

author maintainer

2015-11-24 15:03 UTC #7 lore raw

Re: [PATCH v5 1/8] watchdog: Introduce hardware maximum timeout in watchdog core

Quoted 6 lines · Uwe Kleine-König

Hello Guenter,
Quoted 3 lines · Guenter Roeck
Introduce an optional hardware maximum timeout in the watchdog core. The hardware maximum timeout can be lower than the maximum timeout.

Quoted 9 lines

Quoted 7 lines
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.
Cc Timo Kokkonen <[email protected]>
Signed-off-by Guenter Roeck <[email protected]>
Acked-by Uwe Kleine-König <[email protected]>

Hi Uwe,

thanks a lot for the Ack. I have v6 in the works, which changes recording of last_keepalive when the watchdog is started. Does your Ack include that change, or do you prefer to have another look ?

Thanks,
Guenter

Uwe Kleine-König

2015-11-24 16:11 UTC #8 lore raw

Re: [PATCH v5 1/8] watchdog: Introduce hardware maximum timeout in watchdog core

Hello Guenter,

Quoted 3 lines · Guenter Roeck

thanks a lot for the Ack. I have v6 in the works, which changes recording of last_keepalive when the watchdog is started. Does your Ack include that change, or do you prefer to have another look ?

this change is to sample jiffies before .start is called as I suggested, right? For that change keeping my ack is ok of course.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Guenter Roeck

author maintainer

2015-11-24 16:45 UTC #9 lore raw

Re: [PATCH v5 1/8] watchdog: Introduce hardware maximum timeout in watchdog core

Hi Uwe,

Quoted 10 lines · Uwe Kleine-König

Hello Guenter,
Quoted 3 lines · Guenter Roeck
thanks a lot for the Ack. I have v6 in the works, which changes recording of last_keepalive when the watchdog is started. Does your Ack include that change, or do you prefer to have another look ?
this change is to sample jiffies before .start is called as I suggested, right? For that change keeping my ack is ok of course.

Correct. This is how the code now looks like.

        started_at = jiffies;
        if (watchdog_hw_running(wdd) && wdd->ops->ping)
                err = wdd->ops->ping(wdd);
        else
                err = wdd->ops->start(wdd);
        if (err == 0) {
                set_bit(WDOG_ACTIVE, &wdd->status);
                wdd->last_keepalive = started_at;
                watchdog_update_worker(wdd);
        }

Thanks,
Guenter

Uwe Kleine-König

2015-11-24 21:14 UTC #10 lore raw

Re: [PATCH v5 1/8] watchdog: Introduce hardware maximum timeout in watchdog core

Hello Guenter,

Quoted 12 lines · Guenter Roeck

Quoted 8 lines · Uwe Kleine-König
Quoted 3 lines · Guenter Roeck
thanks a lot for the Ack. I have v6 in the works, which changes recording of last_keepalive when the watchdog is started. Does your Ack include that change, or do you prefer to have another look ?
this change is to sample jiffies before .start is called as I suggested, right? For that change keeping my ack is ok of course.
Correct. This is how the code now looks like.

Quoted 10 lines

        started_at = jiffies;
        if (watchdog_hw_running(wdd) && wdd->ops->ping)
                err = wdd->ops->ping(wdd);
        else
                err = wdd->ops->start(wdd);
        if (err == 0) {
                set_bit(WDOG_ACTIVE, &wdd->status);
                wdd->last_keepalive = started_at;
                watchdog_update_worker(wdd);
        }

\o/

Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Guenter Roeck

author maintainer

2015-11-23 03:20 UTC #11 lore raw

PATCH v5 2/8 watchdog: Introduce WDOG_RUNNING flag

In reply to Guenter Roeck

The WDOG_RUNNING flag is expected to be set by watchdog drivers if the hardware watchdog is running. If the flag is set, the watchdog subsystem will ping the watchdog even if the watchdog device is closed.

The watchdog driver stop function is now optional and may be omitted if the watchdog can not be stopped. If stopping the watchdog is not possible but the driver implements a stop function, it is responsible to set the WDOG_RUNNING flag in its stop function.

Cc Timo Kokkonen <[email protected]>

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

---
v5: Rebased to v4.4-rc1
v4: No change
v3: Clarified meaning of WDOG_ACTIVE.
Do not call cancel_delayed_work_sync() from watchdog_update_worker(). Call it directly where needed instead, to keep the common code simple. Do not (re-)start an already running watchdog when opening the watchdog device. Send a heartbeat request instead.
v2: Improved documentation.

---
 Documentation/watchdog/watchdog-kernel-api.txt | 28 +++++++++------
 drivers/watchdog/watchdog_core.c               |  2 +-
 drivers/watchdog/watchdog_dev.c                | 49 +++++++++++++++++++-------
 include/linux/watchdog.h                       |  7 ++++
 4 files changed, 63 insertions(+), 23 deletions(-)

Documentation/watchdog/watchdog-kernel-api.txt +18 ~~-10~~

index f66859117d1f..3f0963b2c33e 100644
@@ -146,17 +146,18 @@ are:
   device.
   The routine needs a pointer to the watchdog timer device structure as a
   parameter. It returns zero on success or a negative errno code for failure.
-* stop: with this routine the watchdog timer device is being stopped.
-  The routine needs a pointer to the watchdog timer device structure as a
-  parameter. It returns zero on success or a negative errno code for failure.
-  Some watchdog timer hardware can only be started and not be stopped. The
-  driver supporting this hardware needs to make sure that a start and stop
-  routine is being provided. This can be done by using a timer in the driver
-  that regularly sends a keepalive ping to the watchdog timer hardware.
 
 Not all watchdog timer hardware supports the same functionality. That's why
 all other routines/operations are optional. They only need to be provided if
 they are supported. These optional routines/operations are:
+* stop: with this routine the watchdog timer device is being stopped.
+  The routine needs a pointer to the watchdog timer device structure as a
+  parameter. It returns zero on success or a negative errno code for failure.
+  Some watchdog timer hardware can only be started and not be stopped. A
+  driver supporting such hardware does not have to implement the stop routine.
+  If a driver has no stop function, the watchdog core will set WDOG_RUNNING and
+  start calling the driver's keepalive pings function after the watchdog device
+  is closed.
 * ping: this is the routine that sends a keepalive ping to the watchdog timer
   hardware.
   The routine needs a pointer to the watchdog timer device structure as a

@@ -196,9 +197,8 @@ they are supported. These optional routines/operations are:
 The status bits should (preferably) be set with the set_bit and clear_bit alike
 bit-operations. The status bits that are defined are:
 * WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device
-  is active or not. When the watchdog is active after booting, then you should
-  set this status bit (Note: when you register the watchdog timer device with
-  this bit set, then opening /dev/watchdog will skip the start operation)
+  is active or not from user perspective. User space is expected to send
+  heartbeat requests to the driver while this flag is set.
 * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
   was opened via /dev/watchdog.
   (This bit should only be used by the WatchDog Timer Driver Core).

@@ -212,6 +212,14 @@ bit-operations. The status bits that are defined are:
   any watchdog_ops, so that you can be sure that no operations (other then
   unref) will get called after unregister, even if userspace still holds a
   reference to /dev/watchdog
+* WDOG_RUNNING: Set by the watchdog driver if the hardware watchdog is running.
+  The bit must be set if the watchdog timer hardware can not be stopped.
+  The bit may also be set if the watchdog timer is running aftyer booting,
+  before the watchdog device is opened. If set, the watchdog infrastructure
+  will send keepalives to the watchdog hardware while WDOG_ACTIVE is not set.
+  Note: when you register the watchdog timer device with this bit set,
+  then opening /dev/watchdog will skip the start operation but send a keepalive
+  request instead.
 
   To set the WDOG_NO_WAY_OUT status bit (before registering your watchdog
   timer device) you can either:

drivers/watchdog/watchdog_core.c +1 -1

index 873f13972cf4..c2202ab62490 100644
@@ -145,7 +145,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		return -EINVAL;
 
 	/* Mandatory operations need to be supported */
-	if (wdd->ops->start == NULL || wdd->ops->stop == NULL)
+	if (!wdd->ops->start)
 		return -EINVAL;
 
 	watchdog_check_min_max_timeout(wdd);

drivers/watchdog/watchdog_dev.c +37 ~~-12~~

101 lines not on this page · raw message

include/linux/watchdog.h +7 -0

index b535b02b1d7f..f0292d56caf0 100644
@@ -108,6 +108,7 @@ struct watchdog_device {
 #define WDOG_ALLOW_RELEASE	2	/* Did we receive the magic char ? */
 #define WDOG_NO_WAY_OUT		3	/* Is 'nowayout' feature set ? */
 #define WDOG_UNREGISTERED	4	/* Has the device been unregistered */
+#define WDOG_RUNNING		5	/* True if HW watchdog running */
 	/* the following variables are for internal use only */
 	struct mutex lock;
 	unsigned long last_keepalive;

@@ -124,6 +125,12 @@ static inline bool watchdog_active(struct watchdog_device *wdd)
 	return test_bit(WDOG_ACTIVE, &wdd->status);
 }
 
+/* Use the following function to check whether or not the watchdog is running */
+static inline bool watchdog_running(struct watchdog_device *wdd)
+{
+	return test_bit(WDOG_RUNNING, &wdd->status);
+}
+
 /* Use the following function to set the nowayout feature */
 static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool nowayout)
 {

Signature · 1 line

2.1.4

Alexander Stein

2015-11-23 16:30 UTC #12 lore raw

Re: [PATCH v5 2/8] watchdog: Introduce WDOG_RUNNING flag

Documentation/watchdog/watchdog-kernel-api.txt bit-operations. The status bits that are defined are: · line 217

Quoted 11 lines · Guenter Roeck

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

index f66859117d1f..3f0963b2c33e 100644
@@ -212,6 +212,14 @@ bit-operations. The status bits that are defined are:
   any watchdog_ops, so that you can be sure that no operations (other then
   unref) will get called after unregister, even if userspace still holds a
   reference to /dev/watchdog
+* WDOG_RUNNING: Set by the watchdog driver if the hardware watchdog is running.
+  The bit must be set if the watchdog timer hardware can not be stopped.
+  The bit may also be set if the watchdog timer is running aftyer booting,

                                                                 ^
                                                                 typo

Best regards,
Alexander

Signature · 17 lines

Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
[email protected]

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:    +49 (0) 3765 38600-4100
 
Managing Directors:
	Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
	Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
	Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

Guenter Roeck

author maintainer

2015-11-23 16:42 UTC #13 lore raw

Re: [PATCH v5 2/8] watchdog: Introduce WDOG_RUNNING flag

Quoted 14 lines · Alexander Stein

Documentation/watchdog/watchdog-kernel-api.txt bit-operations. The status bits that are defined are: · line 217

Quoted 11 lines · Guenter Roeck

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

index f66859117d1f..3f0963b2c33e 100644
@@ -212,6 +212,14 @@ bit-operations. The status bits that are defined are:
    any watchdog_ops, so that you can be sure that no operations (other then
    unref) will get called after unregister, even if userspace still holds a
    reference to /dev/watchdog
+* WDOG_RUNNING: Set by the watchdog driver if the hardware watchdog is running.
+  The bit must be set if the watchdog timer hardware can not be stopped.
+  The bit may also be set if the watchdog timer is running aftyer booting,

                                                                  ^
                                                                  typo

Thanks, fixed.

Guenter

Uwe Kleine-König

2015-11-23 19:26 UTC #14 lore raw

Re: [PATCH v5 2/8] watchdog: Introduce WDOG_RUNNING flag

In reply to Guenter Roeck

Hello Guenter,

Quoted 13 lines · Guenter Roeck

include/linux/watchdog.h +1 -0

index b535b02b1d7f..f0292d56caf0 100644
@@ -108,6 +108,7 @@ struct watchdog_device {
 #define WDOG_ALLOW_RELEASE	2	/* Did we receive the magic char ? */
 #define WDOG_NO_WAY_OUT		3	/* Is 'nowayout' feature set ? */
 #define WDOG_UNREGISTERED	4	/* Has the device been unregistered */
+#define WDOG_RUNNING		5	/* True if HW watchdog running */
 	/* the following variables are for internal use only */
 	struct mutex lock;
 	unsigned long last_keepalive;

include/linux/watchdog.h static inline bool watchdog_active(struct watchdog_device *wdd) · line 128

Quoted 4 lines

include/linux/watchdog.h +1 -0

@@ -124,6 +125,12 @@ static inline bool watchdog_active(struct watchdog_device *wdd)
 	return test_bit(WDOG_ACTIVE, &wdd->status);
 }
 
+/* Use the following function to check whether or not the watchdog is running */

I'd like to have this comment more clear to distinguish between device state and userspace view. Maybe also call the flag WDOG_HW_RUNNING to make this more clear?

Quoted 5 lines

include/linux/watchdog.h +5 -0

+static inline bool watchdog_running(struct watchdog_device *wdd)
+{
+	return test_bit(WDOG_RUNNING, &wdd->status);
+}
+

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Guenter Roeck

author maintainer

2015-11-23 21:48 UTC #15 lore raw

Re: [PATCH v5 2/8] watchdog: Introduce WDOG_RUNNING flag

Quoted 19 lines · Uwe Kleine-König

Hello Guenter,

Quoted 16 lines · Guenter Roeck

include/linux/watchdog.h +1 -0

index b535b02b1d7f..f0292d56caf0 100644
@@ -108,6 +108,7 @@ struct watchdog_device {
 #define WDOG_ALLOW_RELEASE	2	/* Did we receive the magic char ? */
 #define WDOG_NO_WAY_OUT		3	/* Is 'nowayout' feature set ? */
 #define WDOG_UNREGISTERED	4	/* Has the device been unregistered */
+#define WDOG_RUNNING		5	/* True if HW watchdog running */
 	/* the following variables are for internal use only */
 	struct mutex lock;
 	unsigned long last_keepalive;

@@ -124,6 +125,12 @@ static inline bool watchdog_active(struct watchdog_device *wdd)
 	return test_bit(WDOG_ACTIVE, &wdd->status);
 }

Quoted 6 lines

include/linux/watchdog.h static inline bool watchdog_active(struct watchdog_device *wdd) · line 128
Quoted 1 line
include/linux/watchdog.h +1 -0
+/* Use the following function to check whether or not the watchdog is running */
I'd like to have this comment more clear to distinguish between device state and userspace view. Maybe also call the flag WDOG_HW_RUNNING to make this more clear?

Sure, I can make it WDOG_HW_RUNNING and watchdog_hw_running() and clarify the comment.

Thanks,
Guenter

Guenter Roeck

author maintainer

2015-11-23 03:21 UTC #16 lore raw

PATCH v5 3/8 watchdog: Make set_timeout function optional

In reply to Guenter Roeck

For some watchdogs, the hardware timeout is fixed, and the watchdog driver depends on the watchdog core to handle the actual timeout. In this situation, the watchdog driver might only set the 'timeout' variable but do nothing else. This can as well be handled by the infrastructure, so make the set_timeout callback optional. If WDIOF_SETTIMEOUT is configured but the .set_timeout callback is not available, update the timeout variable in the infrastructure code.

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

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

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

---
 Documentation/watchdog/watchdog-kernel-api.txt | 5 +++++
 drivers/watchdog/watchdog_dev.c                | 9 ++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

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

index 3f0963b2c33e..f480a9355b43 100644
@@ -184,6 +184,11 @@ they are supported. These optional routines/operations are:
   (if it is larger than hw_max_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).
+  If the watchdog driver does not have to perform any action but setting the
+  watchdog_device.timeout, this callback can be omitted.
+  If set_timeout is not provided but, WDIOF_SETTIMEOUT is set, the watchdog
+  infrastructure updates the timeout value of the watchdog_device internally
+  to the requested value.
 * get_timeleft: this routines returns the time that's left before a reset.
 * ref: the operation that calls kref_get on the kref of a dynamically
   allocated watchdog_device struct.

drivers/watchdog/watchdog_dev.c +6 -3

index bb3bd16ffaf6..e73407b42df5 100644
@@ -282,9 +282,9 @@ out_status:
 static int watchdog_set_timeout(struct watchdog_device *wdd,
 							unsigned int timeout)
 {
-	int err;
+	int err = 0;
 
-	if (!wdd->ops->set_timeout || !(wdd->info->options & WDIOF_SETTIMEOUT))
+	if (!(wdd->info->options & WDIOF_SETTIMEOUT))
 		return -EOPNOTSUPP;
 
 	if (watchdog_timeout_invalid(wdd, timeout))

@@ -297,7 +297,10 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
 		goto out_timeout;
 	}
 
-	err = wdd->ops->set_timeout(wdd, timeout);
+	if (wdd->ops->set_timeout)
+		err = wdd->ops->set_timeout(wdd, timeout);
+	else
+		wdd->timeout = timeout;
 
 	watchdog_update_worker(wdd, true);

Signature · 1 line

2.1.4

Guenter Roeck

author maintainer

2015-11-23 03:21 UTC #17 lore raw

PATCH v5 4/8 watchdog: Add support for minimum time between heartbeats

In reply to Guenter Roeck

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

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

---
v5: Rebased to v4.4-rc1
Fixed typo in documentation.
v4: Added patch

---
 Documentation/watchdog/watchdog-kernel-api.txt |  4 ++++
 drivers/watchdog/watchdog_dev.c                | 12 ++++++++++++
 include/linux/watchdog.h                       |  6 ++++++
 3 files changed, 22 insertions(+)

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

index f480a9355b43..f9f6eccbbc0b 100644
@@ -53,11 +53,13 @@ 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;
 	void *driver_data;
 	unsigned long status;
 	struct mutex lock;
 	unsigned long last_keepalive;
+	unsigned long last_hw_keepalive;
 	struct delayed_work work;
 	struct list_head deferred;
 };

@@ -83,6 +85,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, unless WDOG_ACTIVE

drivers/watchdog/watchdog_dev.c +12 -0

index e73407b42df5..832bd5ac15b5 100644
@@ -111,6 +111,8 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd,
 
 static int _watchdog_ping(struct watchdog_device *wdd)
 {
+	unsigned long earliest_keepalive = wdd->last_hw_keepalive +
+				msecs_to_jiffies(wdd->min_hw_heartbeat_ms);
 	int err;
 
 	if (test_bit(WDOG_UNREGISTERED, &wdd->status))

@@ -119,6 +121,13 @@ static int _watchdog_ping(struct watchdog_device *wdd)
 	if (!watchdog_active(wdd) && !watchdog_running(wdd))
 		return 0;
 
+	if (time_is_after_jiffies(earliest_keepalive)) {
+		mod_delayed_work(watchdog_wq, &wdd->work,
+				 earliest_keepalive - jiffies);
+		return 0;
+	}
+
+	wdd->last_hw_keepalive = jiffies;
 	if (wdd->ops->ping)
 		err = wdd->ops->ping(wdd);  /* ping the watchdog */
 	else

@@ -665,6 +674,9 @@ int watchdog_dev_register(struct watchdog_device *wdd)
 		return err;
 	}
 
+	/* Record time of most recent heartbeat as 'just before now'. */
+	wdd->last_hw_keepalive = jiffies - 1;
+
 	/*
 	 * If the watchdog is running, prevent its driver from being unloaded,
 	 * and schedule an immediate ping.

include/linux/watchdog.h +6 -0

index f0292d56caf0..84bd76b674a3 100644
@@ -66,6 +66,8 @@ struct watchdog_ops {
  * @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.
+ * @min_hw_heartbeat_ms:
+ *		Minimum time between heartbeats, in milli-seconds.
  * @max_hw_timeout_ms:
  *		Hardware limit for maximum timeout, in milli-seconds.
  *		Replaces max_timeout if specified.

@@ -75,6 +77,8 @@ struct watchdog_ops {
  * @last_keepalive:
  *		Time of most recent keepalive triggered from user space,
  *		in jiffies (watchdog core internal).
+ * @last_hw_keepalive:
+ *		Time of most recent keepalive sent to the driver, in jiffies.
  * @work:	Data structure for worker function (watchdog core internal).
  * @deferred:	entry in wtd_deferred_reg_list which is used to
  *		register early initialized watchdogs.

@@ -99,6 +103,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;
 	void *driver_data;
 	unsigned long status;

@@ -112,6 +117,7 @@ struct watchdog_device {
 	/* the following variables are for internal use only */
 	struct mutex lock;
 	unsigned long last_keepalive;
+	unsigned long last_hw_keepalive;
 	struct delayed_work work;
 	struct list_head deferred;
 };

Signature · 1 line

2.1.4

Guenter Roeck

author maintainer

2015-11-23 03:21 UTC #18 lore raw

PATCH v5 5/8 watchdog: Simplify update_worker

In reply to Guenter Roeck

Drop 'cancel' parameter; simply cancel worker unconditionally if not needed.

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

---
v5: Added patch.

 drivers/watchdog/watchdog_dev.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

drivers/watchdog/watchdog_dev.c +7 -8

index 832bd5ac15b5..722ea86fcc7c 100644
@@ -96,15 +96,14 @@ static long watchdog_next_keepalive(struct watchdog_device *wdd)
 	return min_t(long, last_heartbeat - jiffies, keepalive_interval);
 }
 
-static inline void watchdog_update_worker(struct watchdog_device *wdd,
-					  bool cancel)
+static inline void watchdog_update_worker(struct watchdog_device *wdd)
 {
 	if (watchdog_need_worker(wdd)) {
 		long t = watchdog_next_keepalive(wdd);
 
 		if (t > 0)
 			mod_delayed_work(watchdog_wq, &wdd->work, t);
-	} else if (cancel) {
+	} else {
 		cancel_delayed_work(&wdd->work);
 	}
 }

@@ -133,7 +132,7 @@ static int _watchdog_ping(struct watchdog_device *wdd)
 	else
 		err = wdd->ops->start(wdd); /* restart watchdog */
 
-	watchdog_update_worker(wdd, false);
+	watchdog_update_worker(wdd);
 
 	return err;
 }

@@ -198,7 +197,7 @@ static int watchdog_start(struct watchdog_device *wdd)
 	if (err == 0) {
 		set_bit(WDOG_ACTIVE, &wdd->status);
 		wdd->last_keepalive = jiffies;
-		watchdog_update_worker(wdd, true);
+		watchdog_update_worker(wdd);
 	}
 
 out_start:

@@ -243,7 +242,7 @@ static int watchdog_stop(struct watchdog_device *wdd)
 
 	if (err == 0) {
 		clear_bit(WDOG_ACTIVE, &wdd->status);
-		watchdog_update_worker(wdd, true);
+		watchdog_update_worker(wdd);
 	}
 
 out_stop:

@@ -311,7 +310,7 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
 	else
 		wdd->timeout = timeout;
 
-	watchdog_update_worker(wdd, true);
+	watchdog_update_worker(wdd);
 
 out_timeout:
 	mutex_unlock(&wdd->lock);

@@ -590,7 +589,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
 	}
 
 	cancel_delayed_work_sync(&wdd->work);
-	watchdog_update_worker(wdd, false);
+	watchdog_update_worker(wdd);
 
 	/*
 	 * Allow the owner module to be unloaded again unless the watchdog

Signature · 1 line

2.1.4

Uwe Kleine-König

2015-11-24 07:13 UTC #19 lore raw

Re: [PATCH v5 5/8] watchdog: Simplify update_worker

Hello,

Quoted 4 lines · Guenter Roeck

Drop 'cancel' parameter; simply cancel worker unconditionally if not needed.
Signed-off-by Guenter Roeck <[email protected]>

This is a good change, but given that watchdog_update_worker is introduced in this series it should be squashed accordingly.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Guenter Roeck

author maintainer

2015-11-24 07:25 UTC #20 lore raw

Re: [PATCH v5 5/8] watchdog: Simplify update_worker

Hi Uwe,

Quoted 11 lines · Uwe Kleine-König

Hello,
Quoted 4 lines · Guenter Roeck
Drop 'cancel' parameter; simply cancel worker unconditionally if not needed.
Signed-off-by Guenter Roeck <[email protected]>
This is a good change, but given that watchdog_update_worker is introduced in this series it should be squashed accordingly.

As I tried to explain, I didn't squash it because, even though I could not find any trouble with it, I am still not sure if it may cause trouble or not. As such, I found it better to keep it separate to make it easier to revert it _if_, as unlikely as it may be, it should be needed.

Ultimately I'd like to get some input from Wim on this.

Thanks,
Guenter

Guenter Roeck

author maintainer

2015-11-23 03:21 UTC #21 lore raw

PATCH v5 6/8 watchdog: imx2: Convert to use infrastructure triggered keepalives

In reply to Guenter Roeck

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

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

---
 drivers/watchdog/imx2_wdt.c | 72 ++++++++-------------------------------------
 1 file changed, 12 insertions(+), 60 deletions(-)

drivers/watchdog/imx2_wdt.c +12 ~~-60~~

index 29ef719a6a3c..96a9eae69fb9 100644
@@ -25,7 +25,6 @@
 #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>

@@ -34,7 +33,6 @@
 #include <linux/platform_device.h>
 #include <linux/reboot.h>
 #include <linux/regmap.h>
-#include <linux/timer.h>
 #include <linux/watchdog.h>
 
 #define DRIVER_NAME "imx2-wdt"

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

@@ -151,16 +148,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)
 {

@@ -177,40 +164,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_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,
 };

@@ -277,9 +243,10 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
 	watchdog_set_nowayout(wdog, nowayout);
 	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_RUNNING, &wdog->status);
+	}
 
 	/*
 	 * Disable the watchdog power down counter at boot. Otherwise the power

@@ -320,7 +287,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");
 	}

@@ -334,10 +300,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");

@@ -355,10 +320,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);

@@ -384,19 +345,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;

Signature · 1 line

2.1.4

Guenter Roeck

author maintainer

2015-11-23 03:21 UTC #22 lore raw

PATCH v5 7/8 watchdog: retu: Convert to use infrastructure triggered keepalives

In reply to Guenter Roeck

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

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

---
 drivers/watchdog/retu_wdt.c | 78 ++++-----------------------------------------
 1 file changed, 7 insertions(+), 71 deletions(-)

drivers/watchdog/retu_wdt.c +7 ~~-71~~

index 39cd51df2ffc..9a85c687824c 100644
@@ -28,69 +28,22 @@
 /* Watchdog timer values in seconds */
 #define RETU_WDT_MAX_TIMER	63
 
-struct retu_wdt_dev {
-	struct retu_dev		*rdev;
-	struct device		*dev;
-	struct delayed_work	ping_work;
-};
-
-/*
- * Since Retu watchdog cannot be disabled in hardware, we must kick it
- * with a timer until userspace watchdog software takes over. If
- * CONFIG_WATCHDOG_NOWAYOUT is set, we never start the feeding.
- */
-static void retu_wdt_ping_enable(struct retu_wdt_dev *wdev)
-{
-	retu_write(wdev->rdev, RETU_REG_WATCHDOG, RETU_WDT_MAX_TIMER);
-	schedule_delayed_work(&wdev->ping_work,
-			round_jiffies_relative(RETU_WDT_MAX_TIMER * HZ / 2));
-}
-
-static void retu_wdt_ping_disable(struct retu_wdt_dev *wdev)
-{
-	retu_write(wdev->rdev, RETU_REG_WATCHDOG, RETU_WDT_MAX_TIMER);
-	cancel_delayed_work_sync(&wdev->ping_work);
-}
-
-static void retu_wdt_ping_work(struct work_struct *work)
-{
-	struct retu_wdt_dev *wdev = container_of(to_delayed_work(work),
-						struct retu_wdt_dev, ping_work);
-	retu_wdt_ping_enable(wdev);
-}
-
 static int retu_wdt_start(struct watchdog_device *wdog)
 {
-	struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct retu_dev *rdev = watchdog_get_drvdata(wdog);
 
-	retu_wdt_ping_disable(wdev);
+	set_bit(WDOG_RUNNING, &wdog->status);
 
-	return retu_write(wdev->rdev, RETU_REG_WATCHDOG, wdog->timeout);
-}
-
-static int retu_wdt_stop(struct watchdog_device *wdog)
-{
-	struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
-
-	retu_wdt_ping_enable(wdev);
-
-	return 0;
-}
-
-static int retu_wdt_ping(struct watchdog_device *wdog)
-{
-	struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
-
-	return retu_write(wdev->rdev, RETU_REG_WATCHDOG, wdog->timeout);
+	return retu_write(rdev, RETU_REG_WATCHDOG, wdog->timeout);
 }
 
 static int retu_wdt_set_timeout(struct watchdog_device *wdog,
 				unsigned int timeout)
 {
-	struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct retu_dev *rdev = watchdog_get_drvdata(wdog);
 
 	wdog->timeout = timeout;
-	return retu_write(wdev->rdev, RETU_REG_WATCHDOG, wdog->timeout);
+	return retu_write(rdev, RETU_REG_WATCHDOG, wdog->timeout);
 }
 
 static const struct watchdog_info retu_wdt_info = {

@@ -101,8 +54,6 @@ static const struct watchdog_info retu_wdt_info = {
 static const struct watchdog_ops retu_wdt_ops = {
 	.owner		= THIS_MODULE,
 	.start		= retu_wdt_start,
-	.stop		= retu_wdt_stop,
-	.ping		= retu_wdt_ping,
 	.set_timeout	= retu_wdt_set_timeout,
 };

@@ -111,17 +62,12 @@ static int retu_wdt_probe(struct platform_device *pdev)
 	struct retu_dev *rdev = dev_get_drvdata(pdev->dev.parent);
 	bool nowayout = WATCHDOG_NOWAYOUT;
 	struct watchdog_device *retu_wdt;
-	struct retu_wdt_dev *wdev;
 	int ret;
 
 	retu_wdt = devm_kzalloc(&pdev->dev, sizeof(*retu_wdt), GFP_KERNEL);
 	if (!retu_wdt)
 		return -ENOMEM;
 
-	wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
-	if (!wdev)
-		return -ENOMEM;
-
 	retu_wdt->info		= &retu_wdt_info;
 	retu_wdt->ops		= &retu_wdt_ops;
 	retu_wdt->timeout	= RETU_WDT_MAX_TIMER;

@@ -129,22 +75,14 @@ static int retu_wdt_probe(struct platform_device *pdev)
 	retu_wdt->max_timeout	= RETU_WDT_MAX_TIMER;
 	retu_wdt->parent	= &pdev->dev;
 
-	watchdog_set_drvdata(retu_wdt, wdev);
+	watchdog_set_drvdata(retu_wdt, rdev);
 	watchdog_set_nowayout(retu_wdt, nowayout);
 
-	wdev->rdev		= rdev;
-	wdev->dev		= &pdev->dev;
-
-	INIT_DELAYED_WORK(&wdev->ping_work, retu_wdt_ping_work);
-
 	ret = watchdog_register_device(retu_wdt);
 	if (ret < 0)
 		return ret;
 
-	if (nowayout)
-		retu_wdt_ping(retu_wdt);
-	else
-		retu_wdt_ping_enable(wdev);
+	retu_wdt_start(retu_wdt);
 
 	platform_set_drvdata(pdev, retu_wdt);

@@ -154,10 +92,8 @@ static int retu_wdt_probe(struct platform_device *pdev)
 static int retu_wdt_remove(struct platform_device *pdev)
 {
 	struct watchdog_device *wdog = platform_get_drvdata(pdev);
-	struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
 
 	watchdog_unregister_device(wdog);
-	cancel_delayed_work_sync(&wdev->ping_work);
 
 	return 0;
 }

Signature · 1 line

2.1.4

Guenter Roeck

author maintainer

2015-11-23 03:21 UTC #23 lore raw

PATCH v5 8/8 watchdog: at91sam9: Convert to use infrastructure triggered keepalives

In reply to Guenter Roeck

The watchdog infrastructure now supports handling watchdog keepalive if the watchdog is running while the watchdog device is closed. The infrastructure now also supports generating additional heartbeats if the maximum hardware timeout is smaller than or close to the configured timeout. Convert the driver to use this infrastructure.

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

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

---
 drivers/watchdog/at91sam9_wdt.c | 103 +++++-----------------------------------
 1 file changed, 12 insertions(+), 91 deletions(-)

drivers/watchdog/at91sam9_wdt.c +12 ~~-91~~

index 7e6acaf3ece4..a1c1c79b700e 100644
@@ -30,7 +30,6 @@
 #include <linux/types.h>
 #include <linux/watchdog.h>
 #include <linux/jiffies.h>
-#include <linux/timer.h>
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
 #include <linux/of.h>

@@ -49,8 +48,8 @@
  * use this to convert a watchdog
  * value from/to milliseconds.
  */
-#define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
-#define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
+#define ticks_to_ms_rounddown(t)	((((t) + 1) * 1000) >> 8)
+#define ticks_to_ms_roundup(t)		(((((t) + 1) * 1000) + 255) >> 8)
 #define ticks_to_secs(t)		(((t) + 1) >> 8)
 #define secs_to_ticks(s)		((s) ? (((s) << 8) - 1) : 0)

@@ -65,9 +64,6 @@
 /* Hardware timeout in seconds */
 #define WDT_HW_TIMEOUT 2
 
-/* Timer heartbeat (500ms) */
-#define WDT_TIMEOUT	(HZ/2)
-
 /* User land timeout */
 #define WDT_HEARTBEAT 15
 static int heartbeat;

@@ -84,11 +80,8 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 struct at91wdt {
 	struct watchdog_device wdd;
 	void __iomem *base;
-	unsigned long next_heartbeat;	/* the next_heartbeat for the timer */
-	struct timer_list timer;	/* The timer that pings the watchdog */
 	u32 mr;
 	u32 mr_mask;
-	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
 	bool nowayout;
 	unsigned int irq;
 	struct clk *sclk;

@@ -109,47 +102,13 @@ static irqreturn_t wdt_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-/*
- * Reload the watchdog timer.  (ie, pat the watchdog)
- */
-static inline void at91_wdt_reset(struct at91wdt *wdt)
-{
-	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
-}
-
-/*
- * Timer tick
- */
-static void at91_ping(unsigned long data)
-{
-	struct at91wdt *wdt = (struct at91wdt *)data;
-	if (time_before(jiffies, wdt->next_heartbeat) ||
-	    !watchdog_active(&wdt->wdd)) {
-		at91_wdt_reset(wdt);
-		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
-	} else {
-		pr_crit("I will reset your machine !\n");
-	}
-}
-
 static int at91_wdt_start(struct watchdog_device *wdd)
 {
 	struct at91wdt *wdt = to_wdt(wdd);
-	/* calculate when the next userspace timeout will be */
-	wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
-	return 0;
-}
 
-static int at91_wdt_stop(struct watchdog_device *wdd)
-{
-	/* The watchdog timer hardware can not be stopped... */
-	return 0;
-}
+	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
 
-static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_timeout)
-{
-	wdd->timeout = new_timeout;
-	return at91_wdt_start(wdd);
+	return 0;
 }
 
 static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)

@@ -159,8 +118,8 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 	u32 value;
 	int err;
 	u32 mask = wdt->mr_mask;
-	unsigned long min_heartbeat = 1;
-	unsigned long max_heartbeat;
+	unsigned int min_timeout = jiffies_to_msecs(1);
+	unsigned int hw_timeout;
 	struct device *dev = &pdev->dev;
 
 	tmp = wdt_read(wdt, AT91_WDT_MR);

@@ -182,31 +141,15 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 	delta = (tmp & AT91_WDT_WDD) >> 16;
 
 	if (delta < value)
-		min_heartbeat = ticks_to_hz_roundup(value - delta);
+		min_timeout = ticks_to_ms_roundup(value - delta);
 
-	max_heartbeat = ticks_to_hz_rounddown(value);
-	if (!max_heartbeat) {
+	hw_timeout = ticks_to_ms_rounddown(value);
+	if (hw_timeout < min_timeout * 2) {
 		dev_err(dev,
 			"heartbeat is too small for the system to handle it correctly\n");
 		return -EINVAL;
 	}
-
-	/*
-	 * Try to reset the watchdog counter 4 or 2 times more often than
-	 * actually requested, to avoid spurious watchdog reset.
-	 * If this is not possible because of the min_heartbeat value, reset
-	 * it at the min_heartbeat period.
-	 */
-	if ((max_heartbeat / 4) >= min_heartbeat)
-		wdt->heartbeat = max_heartbeat / 4;
-	else if ((max_heartbeat / 2) >= min_heartbeat)
-		wdt->heartbeat = max_heartbeat / 2;
-	else
-		wdt->heartbeat = min_heartbeat;
-
-	if (max_heartbeat < min_heartbeat + 4)
-		dev_warn(dev,
-			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
+	wdt->wdd.max_hw_timeout_ms = hw_timeout;
 
 	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
 		err = request_irq(wdt->irq, wdt_interrupt,

@@ -222,32 +165,12 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 			 "watchdog already configured differently (mr = %x expecting %x)\n",
 			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
 
-	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
-
-	/*
-	 * Use min_heartbeat the first time to avoid spurious watchdog reset:
-	 * we don't know for how long the watchdog counter is running, and
-	 *  - resetting it right now might trigger a watchdog fault reset
-	 *  - waiting for heartbeat time might lead to a watchdog timeout
-	 *    reset
-	 */
-	mod_timer(&wdt->timer, jiffies + min_heartbeat);
-
 	/* Try to set timeout from device tree first */
 	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
 		watchdog_init_timeout(&wdt->wdd, heartbeat, dev);
 	watchdog_set_nowayout(&wdt->wdd, wdt->nowayout);
-	err = watchdog_register_device(&wdt->wdd);
-	if (err)
-		goto out_stop_timer;
-
-	wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
 
-	return 0;
-
-out_stop_timer:
-	del_timer(&wdt->timer);
-	return err;
+	return watchdog_register_device(&wdt->wdd);
 }
 
 /* ......................................................................... */

@@ -261,8 +184,6 @@ static const struct watchdog_info at91_wdt_info = {
 static const struct watchdog_ops at91_wdt_ops = {
 	.owner =	THIS_MODULE,
 	.start =	at91_wdt_start,
-	.stop =		at91_wdt_stop,
-	.set_timeout =	at91_wdt_set_timeout,
 };
 
 #if defined(CONFIG_OF)

@@ -393,7 +314,7 @@ static int __exit at91wdt_remove(struct platform_device *pdev)
 	watchdog_unregister_device(&wdt->wdd);
 
 	pr_warn("I quit now, hardware will probably reboot!\n");
-	del_timer(&wdt->timer);
+
 	clk_disable_unprepare(wdt->sclk);
 
 	return 0;

Signature · 1 line

2.1.4