Skip to contentSkip to conversation

Backport of watchdog core triggered keepalive infrastructure

PATCH New Maxim Yu. Osipov 3 participants 15 messages Oct 4, 2017 – Oct 25, 2017

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

cover Backport of watchdog core triggered keepalive infrastructure

Hello Ben,

This series contains

* backport of patches of watchdog core infrastructure supporting handling watchdog keepalive.

* imx21_wdt converted to use infrastructure triggered keepalives.

* backported support of WATCHDOG_HANDLE_BOOT_ENABLED option

On some systems it's desirable to have watchdog reboot the system when it does not come up fast enough. This adds a kernel parameter to disable the auto-update of watchdog before userspace takes over and a kernel option to set the default.

One of the important use cases is safe system update.
In that case, the bootloader enables the watchdog, and Linux should only take it over at the point the whole Linux system is sure to be able to have a fully working system again, including userspace services that could take the next update. If we start the trigger the watchdog too early, like it was so far, we risk to enter a system state where the kernel works but not all other services we need.

i.MX6 based board was used as a test platform.

We suppose that watchdog is enabled in bootloader and set to 120 seconds (maximum timeout supported by i.MX watchdog timer)

After applying these patches, built-in i.MX watchdog (imx21_wdt) into kernel and perform the following test cases:

1) option WATCHDOG_HANDLE_BOOT_ENABLED is turned off in kernel configuration w/o userspace application triggering the watchdog.

Make sure that no userspace application is triggering watchdog.

After around 120 seconds (timeout set in bootloader) board will reboot.

2) option WATCHDOG_HANDLE_BOOT_ENABLED is turned off in kernel configuration but userspace application is triggering watchdog more frequently that watchdog's timeout (by default set to 60 seconds for imx21_wdt).

Watchdog will not reboot the board until the application re-arms the watchdog.

3) option WATCHDOG_HANDLE_BOOT_ENABLED is turned on in kernel configuration w/o userspace application triggering the watchdog.

Make sure that no userspace application is triggering watchdog. Board will not reboot (watchdog keepalive is supported by watchdog core infrastructure, not by driver itself)

Kind regards,
Maxim.

Guenter Roeck (6):
watchdog: Introduce hardware maximum heartbeat in watchdog core
watchdog: Introduce WDOG_HW_RUNNING flag
watchdog: Make stop function optional
watchdog: imx2: Convert to use infrastructure triggered keepalives
watchdog: core: Fix circular locking dependency
watchdog: core: Clear WDOG_HW_RUNNING before calling the stop function

Pratyush Anand (1):
watchdog: skip min and max timeout validity check when
max_hw_heartbeat_ms is defined

Rasmus Villemoes (1):
watchdog: change watchdog_need_worker logic

Sebastian Reichel (1):
watchdog: core: add option to avoid early handling of watchdog

Wei Yongjun (1):
watchdog: core: Fix error handling of watchdog_dev_init()

 Documentation/watchdog/watchdog-kernel-api.txt |  51 +++++--
 drivers/watchdog/Kconfig                       |  11 ++
 drivers/watchdog/imx2_wdt.c                    |  74 ++-------
 drivers/watchdog/watchdog_core.c               |   4 +-
 drivers/watchdog/watchdog_dev.c                | 198 +++++++++++++++++++++++--
 include/linux/watchdog.h                       |  40 ++++-
 6 files changed, 279 insertions(+), 99 deletions(-)
Signature · 1 line
2.11.0
#1 lore raw

PATCH 1/10 watchdog: Introduce hardware maximum heartbeat in watchdog core

From: Guenter Roeck <[email protected]>

Backport from kernel.org, upstream commit 664a39236e71

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

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

[mosipov at ilbers.de backported to 4.4.y]
---
 Documentation/watchdog/watchdog-kernel-api.txt |  19 +++-
 drivers/watchdog/watchdog_dev.c                | 122 +++++++++++++++++++++++--
 include/linux/watchdog.h                       |  30 ++++--
 3 files changed, 154 insertions(+), 17 deletions(-)
Documentation/watchdog/watchdog-kernel-api.txt +17 -2
index d8b0d3367706..9887fa6d8f68 100644
@@ -53,6 +53,7 @@ struct watchdog_device {
 	unsigned int timeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
+	unsigned int max_hw_heartbeat_ms;
 	void *driver_data;
 	struct mutex lock;
 	unsigned long status;
@@ -73,8 +74,18 @@ It contains following fields:
   additional information about the watchdog timer itself. (Like it's unique name)
 * ops: a pointer to the list of watchdog operations that the watchdog supports.
 * timeout: the watchdog timer's timeout value (in seconds).
+  This is the time after which the system will reboot if user space does
+  not send a heartbeat request if WDOG_ACTIVE is set.
 * min_timeout: the watchdog timer's minimum timeout value (in seconds).
-* max_timeout: the watchdog timer's maximum timeout value (in seconds).
+  If set, the minimum configurable value for 'timeout'.
+* max_timeout: the watchdog timer's maximum timeout value (in seconds),
+  as seen from userspace. If set, the maximum configurable value for
+  'timeout'. Not used if max_hw_heartbeat_ms is non-zero.
+* max_hw_heartbeat_ms: Maximum hardware heartbeat, in milli-seconds.
+  If set, the infrastructure will send heartbeats to the watchdog driver
+  if 'timeout' is larger than max_hw_heartbeat_ms, 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.
@@ -160,7 +171,11 @@ they are supported. These optional routines/operations are:
   and -EIO for "could not write value to the watchdog". On success this
   routine should set the timeout value of the watchdog_device to the
   achieved timeout value (which may be different from the requested one
-  because the watchdog does not necessarily has a 1 second resolution).
+  because the watchdog does not necessarily have a 1 second resolution).
+  Drivers implementing max_hw_heartbeat_ms set the hardware watchdog heartbeat
+  to the minimum of timeout and max_hw_heartbeat_ms. Those drivers set the
+  timeout value of the watchdog_device either to the requested timeout value
+  (if it is larger than max_hw_heartbeat_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 +115 -7

204 lines not on this page · raw message

include/linux/watchdog.h +22 -8
index 027b1f43f12d..26aba9b17ac3 100644
@@ -10,8 +10,9 @@
 
 
 #include <linux/bitops.h>
-#include <linux/device.h>
 #include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
 #include <uapi/linux/watchdog.h>
 
 struct watchdog_ops;
@@ -61,12 +62,17 @@ 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_heartbeat_ms is not provided.
+ * @max_hw_heartbeat_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.
+ * @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 +94,11 @@ struct watchdog_device {
 	unsigned int timeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
+	unsigned int max_hw_heartbeat_ms;
 	void *driver_data;
 	struct mutex lock;
+	unsigned long last_keepalive;
+	struct delayed_work work;
 	unsigned long status;
 /* Bit numbers for status flags */
 #define WDOG_ACTIVE		0	/* Is the watchdog running/active */
@@ -121,13 +130,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_heartbeat_ms && wdd->max_timeout &&
+		 t > wdd->max_timeout);
 }
 
 /* Use the following functions to manipulate watchdog driver specific data */
Signature · 1 line
2.11.0
#2 lore raw

PATCH 2/10 watchdog: Introduce WDOG_HW_RUNNING flag

In reply to Maxim Yu. Osipov

From: Guenter Roeck <[email protected]>

Backport from kernel.org, upstream commit ee142889e32f

The WDOG_HW_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_HW_RUNNING flag in its stop function.

[mosipov at ilbers.de backported to 4.4.y]
---
 Documentation/watchdog/watchdog-kernel-api.txt | 22 ++++++++----
 drivers/watchdog/watchdog_dev.c                | 49 ++++++++++++++++++++------
 include/linux/watchdog.h                       | 10 ++++++
 3 files changed, 64 insertions(+), 17 deletions(-)
Documentation/watchdog/watchdog-kernel-api.txt +15 -7
index 9887fa6d8f68..b12789745f86 100644
@@ -144,10 +144,10 @@ 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. 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.
+  Some watchdog timer hardware can only be started and not be stopped.
+  If a watchdog can not be stopped, the watchdog driver must set the
+  WDOG_HW_RUNNING flag in its stop function to inform the watchdog core that
+  the watchdog is still running.
 
 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
@@ -191,9 +191,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).
@@ -202,6 +201,15 @@ bit-operations. The status bits that are defined are:
   (This bit should only be used by the WatchDog Timer Driver Core).
 * WDOG_NO_WAY_OUT: this bit stores the nowayout setting for the watchdog.
   If this bit is set then the watchdog timer will not be able to stop.
+* WDOG_HW_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 after
+  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.
 * WDOG_UNREGISTERED: this bit gets set by the WatchDog Timer Driver Core
   after calling watchdog_unregister_device, and then checked before calling
   any watchdog_ops, so that you can be sure that no operations (other then
drivers/watchdog/watchdog_dev.c +39 -10

118 lines not on this page · raw message

include/linux/watchdog.h +10 -0
index 26aba9b17ac3..cbe31d5b352c 100644
@@ -106,6 +106,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_HW_RUNNING		5	/* True if HW watchdog running */
 	struct list_head deferred;
 };
 
@@ -118,6 +119,15 @@ 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 hardware watchdog
+ * is running
+ */
+static inline bool watchdog_hw_running(struct watchdog_device *wdd)
+{
+	return test_bit(WDOG_HW_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.11.0
#3 lore raw

PATCH 2/10 watchdog: Introduce WDOG_HW_RUNNING flag

Quoted 13 lines · Maxim Yu. Osipov

From: Guenter Roeck <[email protected]>

Backport from kernel.org, upstream commit ee142889e32f

The WDOG_HW_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_HW_RUNNING flag in its stop function.

[...]

Quoted 2 lines
drivers/watchdog/watchdog_dev.c +0 -0

[...]

drivers/watchdog/watchdog_dev.c int watchdog_dev_register(struct watchdog_device · line 670

Quoted 7 lines
drivers/watchdog/watchdog_dev.c +2 -0
@@ -651,9 +665,24 @@ int watchdog_dev_register(struct watchdog_device
 *wdd)
 ?		if (wdd->id == 0) {
 ?			misc_deregister(&watchdog_miscdev);
 ?			old_wdd = NULL;
+			if (wdd->ops->unref)
+				wdd->ops->unref(wdd);

[...]

This doesn't look right. This is decrementing a reference counter because the old_wdd reference is going away, but we never increment a reference counter when old_wdd is set.

The upstream commit doesn't make any change here. There is a kref_put() here in the upstream version but that was added as part of the watchdog_device/watchdog_core_data split.

Ben.

Signature · 2 lines
Ben Hutchings
Software Developer, Codethink Ltd.
#4 lore raw

PATCH 3/10 watchdog: Make stop function optional

In reply to Maxim Yu. Osipov

From: Guenter Roeck <[email protected]>

Backport from kernel.org, upstream commit d0684c8a9354

Not all hardware watchdogs can be stopped. The driver for such watchdogs would typically only set the WATCHDOG_HW_RUNNING flag in its stop function. Make the stop function optional and set WATCHDOG_HW_RUNNING in the watchdog core if it is not provided.

[mosipov at ilbers.de backported to 4.4.y]
---
 Documentation/watchdog/watchdog-kernel-api.txt | 20 ++++++++++++--------
 drivers/watchdog/watchdog_core.c               |  2 +-
 drivers/watchdog/watchdog_dev.c                |  6 +++++-
 3 files changed, 18 insertions(+), 10 deletions(-)
Documentation/watchdog/watchdog-kernel-api.txt +12 -8
index b12789745f86..a376f281f5a7 100644
@@ -85,7 +85,8 @@ It contains following fields:
   If set, the infrastructure will send heartbeats to the watchdog driver
   if 'timeout' is larger than max_hw_heartbeat_ms, unless WDOG_ACTIVE
   is set and userspace failed to send a heartbeat for at least 'timeout'
-  seconds.
+  seconds. max_hw_heartbeat_ms must be set if a driver does not implement
+  the stop function.
 * 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.
@@ -141,17 +142,20 @@ 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.
-  If a watchdog can not be stopped, the watchdog driver must set the
-  WDOG_HW_RUNNING flag in its stop function to inform the watchdog core that
-  the watchdog is still running.
 
 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_HW_RUNNING
+  and start calling the driver's keepalive pings function after the watchdog
+  device is closed.
+  If a watchdog driver does not implement the stop function, it must set
+  max_hw_heartbeat_ms.
 * 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
drivers/watchdog/watchdog_core.c +1 -1
index 873f13972cf4..41f1854d026b 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 || (!wdd->ops->stop && !wdd->max_hw_heartbeat_ms))
 		return -EINVAL;
 
 	watchdog_check_min_max_timeout(wdd);
drivers/watchdog/watchdog_dev.c +5 -1
index 55dc73f9b837..ca64724bf59a 100644
@@ -237,7 +237,11 @@ static int watchdog_stop(struct watchdog_device *wdd)
 		goto out_stop;
 	}
 
-	err = wdd->ops->stop(wdd);
+	if (wdd->ops->stop)
+		err = wdd->ops->stop(wdd);
+	else
+		set_bit(WDOG_HW_RUNNING, &wdd->status);
+
 	if (err == 0) {
 		clear_bit(WDOG_ACTIVE, &wdd->status);
 		watchdog_update_worker(wdd);
Signature · 1 line
2.11.0
#5 lore raw

PATCH 4/10 watchdog: imx2: Convert to use infrastructure triggered keepalives

In reply to Maxim Yu. Osipov

From: Guenter Roeck <[email protected]>

Backport from kernel.org, upstream commit 11d7aba9ceb7

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.

[mosipov at ilbers.de backported to 4.4.y]
---
 drivers/watchdog/imx2_wdt.c | 74 ++++++++-------------------------------------
 1 file changed, 13 insertions(+), 61 deletions(-)
drivers/watchdog/imx2_wdt.c +13 -61
index 29ef719a6a3c..378e7c5dfc50 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_HW_RUNNING, &wdog->status);
 
-	if (imx2_wdt_is_running(wdev)) {
-		imx2_wdt_set_timeout(wdog, wdog->timeout);
-		imx2_wdt_timer_ping((unsigned long)wdog);
-	}
+	return imx2_wdt_ping(wdog);
 }
 
 static const struct watchdog_ops imx2_wdt_ops = {
 	.owner = THIS_MODULE,
 	.start = imx2_wdt_start,
-	.stop = imx2_wdt_stop,
 	.ping = imx2_wdt_ping,
 	.set_timeout = imx2_wdt_set_timeout,
 };
@@ -257,7 +223,7 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
 	wdog->info		= &imx2_wdt_info;
 	wdog->ops		= &imx2_wdt_ops;
 	wdog->min_timeout	= 1;
-	wdog->max_timeout	= IMX2_WDT_MAX_TIME;
+	wdog->max_hw_heartbeat_ms = IMX2_WDT_MAX_TIME * 1000;
 	wdog->parent		= &pdev->dev;
 
 	ret = clk_prepare_enable(wdev->clk);
@@ -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_HW_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.11.0
#6 lore raw

PATCH 5/10 watchdog: core: Fix circular locking dependency

In reply to Maxim Yu. Osipov

From: Guenter Roeck <[email protected]>

Backport from kernel.org, upstream commit e1f30282a1d3

lockdep reports the following circular locking dependency.

======================================================

INFO: possible circular locking dependency detected ]

4.6.0-rc3-00191-gfabf418 #162 Not tainted
-------------------------------------------------------
systemd/1 is trying to acquire lock:
((&(&wd_data->work)->work)){+.+...}, at: [<80141650>] flush_work+0x0/0x280

but task is already holding lock:

(&wd_data->lock){+.+...}, at: [<804acfa8>] watchdog_release+0x18/0x190

which lock already depends on the new lock. the existing dependency chain (in reverse order) is:

-> #1 (&wd_data->lock){+.+...}:

Quoted 6 lines

-------[<80662310>] mutex_lock_nested+0x64/0x4a8
-------[<804aca4c>] watchdog_ping_work+0x18/0x4c
-------[<80143128>] process_one_work+0x1ac/0x500
-------[<801434b4>] worker_thread+0x38/0x554
-------[<80149510>] kthread+0xf4/0x108
-------[<80107c10>] ret_from_fork+0x14/0x24

-> #0 ((&(&wd_data->work)->work)){+.+...}:

Quoted 8 lines

-------[<8017c4e8>] lock_acquire+0x70/0x90 -------[<8014169c>] flush_work+0x4c/0x280 -------[<801440f8>] __cancel_work_timer+0x9c/0x1e0 -------[<804acfcc>] watchdog_release+0x3c/0x190 -------[<8022c5e8>] __fput+0x80/0x1c8 -------[<80147b28>] task_work_run+0x94/0xc8 -------[<8010b998>] do_work_pending+0x8c/0xb4 -------[<80107ba8>] slow_work_pending+0xc/0x20

other info that might help us debug this:

Possible unsafe locking scenario:

CPU0 CPU1
---- ----

lock(&wd_data->lock);
                        lock((&(&wd_data->work)->work));
                        lock(&wd_data->lock);

lock((&(&wd_data->work)->work));

*** DEADLOCK ***

1 lock held by systemd/1:

stack backtrace:
CPU: 2 PID: 1 Comm: systemd Not tainted 4.6.0-rc3-00191-gfabf418 #162
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[<8010f5e4>] (unwind_backtrace) from [<8010c038>] (show_stack+0x10/0x14)
[<8010c038>] (show_stack) from [<8039d7fc>] (dump_stack+0xa8/0xd4)
[<8039d7fc>] (dump_stack) from [<80177ee0>] (print_circular_bug+0x214/0x334)
[<80177ee0>] (print_circular_bug) from [<80179230>] (check_prevs_add+0x4dc/0x8e8)
[<80179230>] (check_prevs_add) from [<8017b3d8>] (__lock_acquire+0xc6c/0x14ec)
[<8017b3d8>] (__lock_acquire) from [<8017c4e8>] (lock_acquire+0x70/0x90)
[<8017c4e8>] (lock_acquire) from [<8014169c>] (flush_work+0x4c/0x280)
[<8014169c>] (flush_work) from [<801440f8>] (__cancel_work_timer+0x9c/0x1e0)
[<801440f8>] (__cancel_work_timer) from [<804acfcc>] (watchdog_release+0x3c/0x190)
[<804acfcc>] (watchdog_release) from [<8022c5e8>] (__fput+0x80/0x1c8)
[<8022c5e8>] (__fput) from [<80147b28>] (task_work_run+0x94/0xc8)
[<80147b28>] (task_work_run) from [<8010b998>] (do_work_pending+0x8c/0xb4)
[<8010b998>] (do_work_pending) from [<80107ba8>] (slow_work_pending+0xc/0x20)

Turns out the call to cancel_delayed_work_sync() in watchdog_release() is not necessary and can be dropped. If the worker is no longer necessary, the subsequent call to watchdog_update_worker() will cancel it. If it is already running, it won't do anything, since the worker function checks if it needs to ping the watchdog or not.

Fixes 11d7aba9ceb7 ("watchdog: imx2: Convert to use infrastructure triggered keepalives")
Cc stable <[email protected]>
[mosipov at ilbers.de backported to 4.4.y]
---
 drivers/watchdog/watchdog_dev.c | 1 -
 1 file changed, 1 deletion(-)
drivers/watchdog/watchdog_dev.c +0 -1
index ca64724bf59a..2eca04d869d9 100644
@@ -596,7 +596,6 @@ static int watchdog_release(struct inode *inode, struct file *file)
 	if (!watchdog_hw_running(wdd))
 		module_put(wdd->ops->owner);
 
-	cancel_delayed_work_sync(&wdd->work);
 	watchdog_update_worker(wdd);
 
 	/* make sure that /dev/watchdog can be re-opened */
Signature · 1 line
2.11.0
#7 lore raw

PATCH 6/10 watchdog: skip min and max timeout validity check when max_hw_heartbeat_ms is defined

In reply to Maxim Yu. Osipov

From: Pratyush Anand <[email protected]>

Backport from kernel.org, upstream commit 1894cad9bf2c

When max_hw_heartbeat_ms has a none zero value, max_timeout is not used. So it's value can be 0. In such case if a driver uses min_timeout functionality, then check will always fail.

This patch fixes above issue.

---
 drivers/watchdog/watchdog_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/watchdog/watchdog_core.c +1 -1
index 41f1854d026b..5de14c51bf95 100644
@@ -88,7 +88,7 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
 	 * Check that we have valid min and max timeout values, if
 	 * not reset them both to 0 (=not used or unknown)
 	 */
-	if (wdd->min_timeout > wdd->max_timeout) {
+	if (!wdd->max_hw_heartbeat_ms && wdd->min_timeout > wdd->max_timeout) {
 		pr_info("Invalid min and max timeout values, resetting to 0!\n");
 		wdd->min_timeout = 0;
 		wdd->max_timeout = 0;
Signature · 1 line
2.11.0
#8 lore raw

PATCH 7/10 watchdog: change watchdog_need_worker logic

In reply to Maxim Yu. Osipov

From: Rasmus Villemoes <[email protected]>

Backport from kernel.org, upstream commit 3fbfe9264756

If the driver indicates that the watchdog is running, the framework should feed it until userspace opens the device, regardless of whether the driver has set max_hw_heartbeat_ms.

This patch only affects the case where wdd->max_hw_heartbeat_ms is zero, wdd->timeout is non-zero, the watchdog is not active and the hardware device is running (*):

- If wdd->timeout is zero, watchdog_need_worker() returns false both before and after this patch, and watchdog_next_keepalive() is not called.

- If watchdog_active(wdd), the return value from watchdog_need_worker is also the same as before (namely, hm && t > hm). Hence in that case, watchdog_next_keepalive() is only called if hm == max_hw_heartbeat_ms is non-zero, so the change to min_not_zero there is a no-op.

- If the watchdog is not active and the device is not running, we return false from watchdog_need_worker just as before.

That leaves the watchdog_hw_running(wdd) && !watchdog_active(wdd) && wdd->timeout case. Again, it's easy to see that if wdd->max_hw_heartbeat_ms is non-zero, we return true from watchdog_need_worker with and without this patch, and the logic in watchdog_next_keepalive is unchanged. Finally, if wdd->max_hw_heartbeat_ms is 0, we used to end up in the cancel_delayed_work branch, whereas with this patch we end up scheduling a ping timeout_ms/2 from now.

(*) This should imply that no current kernel drivers are affected, since the only drivers which explicitly set WDOG_HW_RUNNING are imx2_wdt.c and dw_wdt.c, both of which also provide a non-zero value for max_hw_heartbeat_ms. The watchdog core also sets WDOG_HW_RUNNING, but only when the driver doesn't provide ->stop, in which case it must, according to Documentation/watchdog/watchdog-kernel-api.txt, set max_hw_heartbeat_ms.

---
 drivers/watchdog/watchdog_dev.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/watchdog/watchdog_dev.c +7 -3
index 2eca04d869d9..e3eacf27b1e7 100644
@@ -67,9 +67,13 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 	 *   thus is aware that the framework supports generating heartbeat
 	 *   requests.
 	 * - Userspace requests a longer timeout than the hardware can handle.
+	 *
+	 * Alternatively, if userspace has not opened the watchdog
+	 * device, we take care of feeding the watchdog if it is
+	 * running.
 	 */
-	return hm && ((watchdog_active(wdd) && t > hm) ||
-		      (t && !watchdog_active(wdd) && watchdog_hw_running(wdd)));
+	return (hm && watchdog_active(wdd) && t > hm) ||
+		(t && !watchdog_active(wdd) && watchdog_hw_running(wdd));
 }
 
 static long watchdog_next_keepalive(struct watchdog_device *wdd)
@@ -81,7 +85,7 @@ static long watchdog_next_keepalive(struct watchdog_device *wdd)
 	unsigned int hw_heartbeat_ms;
 
 	virt_timeout = wdd->last_keepalive + msecs_to_jiffies(timeout_ms);
-	hw_heartbeat_ms = min(timeout_ms, wdd->max_hw_heartbeat_ms);
+	hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms);
 	keepalive_interval = msecs_to_jiffies(hw_heartbeat_ms / 2);
 
 	if (!watchdog_active(wdd))
Signature · 1 line
2.11.0
#9 lore raw

PATCH 8/10 watchdog: core: Fix error handling of watchdog_dev_init()

In reply to Maxim Yu. Osipov

From: Wei Yongjun <[email protected]>

Backport from kernel.org, upstream commit 138913cb632b

Fix the error handling paths of watchdog_dev_init().

---
 drivers/watchdog/watchdog_dev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/watchdog/watchdog_dev.c +8 -1
index e3eacf27b1e7..6cc5ad34c5bc 100644
@@ -733,8 +733,15 @@ int __init watchdog_dev_init(void)
 	}
 
 	err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
-	if (err < 0)
+	if (err < 0) {
 		pr_err("watchdog: unable to allocate char dev region\n");
+		goto err_alloc;
+	}
+
+	return 0;
+
+err_alloc:
+	destroy_workqueue(watchdog_wq);
 	return err;
 }
 
Signature · 1 line
2.11.0
#10 lore raw

PATCH 9/10 watchdog: core: Clear WDOG_HW_RUNNING before calling the stop function

In reply to Maxim Yu. Osipov

From: Guenter Roeck <[email protected]>

Backport from kernel.org, upstream commit 3c10bbde10fe

WDOG_HW_RUNNING indicates that the hardware watchdog is running while the watchdog device is closed. The flag may be set by the driver when it is instantiated to indicate that the watchdog is running, and that the watchdog core needs to send heartbeat requests to the driver until the watchdog device is opened.

When the watchdog device is closed, the flag can be used by the driver's stop function to indicate to the watchdog core that it was unable to stop the watchdog, and that the watchdog core needs to send heartbeat requests. This only works if the flag is actually cleared when the watchdog is stopped. To avoid having to clear the flag in each driver's stop function, clear it in the watchdog core before calling the stop function.

Fixes ee142889e32f ("watchdog: Introduce WDOG_HW_RUNNING flag")
[mosipov at ilbers.de backported to 4.4.y]
---
 drivers/watchdog/watchdog_dev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/watchdog/watchdog_dev.c +4 -2
index 6cc5ad34c5bc..e65a14f213ff 100644
@@ -241,10 +241,12 @@ static int watchdog_stop(struct watchdog_device *wdd)
 		goto out_stop;
 	}
 
-	if (wdd->ops->stop)
+	if (wdd->ops->stop) {
+		clear_bit(WDOG_HW_RUNNING, &wdd->status);
 		err = wdd->ops->stop(wdd);
-	else
+	} else {
 		set_bit(WDOG_HW_RUNNING, &wdd->status);
+	}
 
 	if (err == 0) {
 		clear_bit(WDOG_ACTIVE, &wdd->status);
Signature · 1 line
2.11.0
#11 lore raw

PATCH 10/10 watchdog: core: add option to avoid early handling of watchdog

In reply to Maxim Yu. Osipov

From: Sebastian Reichel <[email protected]>

Backport from kernel.org, upstream commit 2501b015313f

On some systems its desirable to have watchdog reboot the system when it does not come up fast enough. This adds a kernel parameter to disable the auto-update of watchdog before userspace takes over and a kernel option to set the default. The info messages were added to shorten error searching on misconfigured systems.

---
 drivers/watchdog/Kconfig        | 11 +++++++++++
 drivers/watchdog/watchdog_dev.c | 21 +++++++++++++++++----
 2 files changed, 28 insertions(+), 4 deletions(-)
drivers/watchdog/Kconfig +11 -0
index 1c427beffadd..88743cd0a7e6 100644
@@ -46,6 +46,17 @@ config WATCHDOG_NOWAYOUT
 	  get killed. If you say Y here, the watchdog cannot be stopped once
 	  it has been started.
 
+config WATCHDOG_HANDLE_BOOT_ENABLED
+	bool "Update boot-enabled watchdog until userspace takes over"
+	default y
+	help
+	  The default watchdog behaviour (which you get if you say Y here) is
+	  to ping watchdog devices that were enabled before the driver has
+	  been loaded until control is taken over from userspace using the
+	  /dev/watchdog file. If you say N here, the kernel will not update
+	  the watchdog on its own. Thus if your userspace does not start fast
+	  enough your device will reboot.
+
 #
 # General Watchdog drivers
 #
drivers/watchdog/watchdog_dev.c +17 -4
index e65a14f213ff..28eaed17c4fe 100644
@@ -53,6 +53,9 @@ static struct watchdog_device *old_wdd;
 
 static struct workqueue_struct *watchdog_wq;
 
+static bool handle_boot_enabled =
+	IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
+
 static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 {
 	/* All variables in milli-seconds */
@@ -685,10 +688,15 @@ int watchdog_dev_register(struct watchdog_device *wdd)
 	 * and schedule an immediate ping.
 	 */
 	if (watchdog_hw_running(wdd)) {
-		__module_get(wdd->ops->owner);
-		if (wdd->ops->ref)
-			wdd->ops->ref(wdd);
-		queue_delayed_work(watchdog_wq, &wdd->work, 0);
+		if (handle_boot_enabled) {
+			__module_get(wdd->ops->owner);
+			if (wdd->ops->ref)
+				wdd->ops->ref(wdd);
+			queue_delayed_work(watchdog_wq, &wdd->work, 0);
+		} else {
+			pr_info("watchdog%d running and kernel based pre-userspace handler disabled\n",
+					wdd->id);
+		}
 	}
 
 	return 0;
@@ -758,3 +766,8 @@ void __exit watchdog_dev_exit(void)
 	unregister_chrdev_region(watchdog_devt, MAX_DOGS);
 	destroy_workqueue(watchdog_wq);
 }
+
+module_param(handle_boot_enabled, bool, 0444);
+MODULE_PARM_DESC(handle_boot_enabled,
+	"Watchdog core auto-updates boot enabled watchdogs before userspace takes over (default="
+	__MODULE_STRING(IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED)) ")");
Signature · 1 line
2.11.0
#12 lore raw

cover Backport of watchdog core triggered keepalive infrastructure

In reply to Maxim Yu. Osipov
Quoted 10 lines · Maxim Yu. Osipov

Hello Ben,

This series contains

* backport of patches of watchdog core infrastructure supporting handling watchdog keepalive.

* imx21_wdt converted to use infrastructure triggered keepalives.

* backported support of WATCHDOG_HANDLE_BOOT_ENABLED option

[...]

Are you working for Siemens on this? Can you provide the config you are using?

The reason I ask is that CIP kernel branches will have hardware support limited to the needs of members. Currently, none of the kernel configurations I collected enable CONFIG_IMX2_WDT, or even CONFIG_ARCH_MXC, which implies that imx2_wdt won't be supported.

Ben.

Signature · 2 lines
Ben Hutchings
Software Developer, Codethink Ltd.
#13 lore raw

cover Backport of watchdog core triggered keepalive infrastructure

Quoted 8 lines · Ben Hutchings
Quoted 7 lines · Maxim Yu. Osipov

Hello Ben,

This series contains

* backport of patches of watchdog core infrastructure supporting handling watchdog keepalive.

Quoted 7 lines
Quoted 3 lines

* imx21_wdt converted to use infrastructure triggered keepalives.

* backported support of WATCHDOG_HANDLE_BOOT_ENABLED option

[...]

Are you working for Siemens on this? Can you provide the config you are using?

Yes, Maxim is working for a project of us, and I asked him to propose these backports for CIP. They will likely become relevant for other boards as well because the bootloader-started watchdog is a cornerstone of safe system updates.

Quoted 5 lines

The reason I ask is that CIP kernel branches will have hardware support limited to the needs of members. Currently, none of the kernel configurations I collected enable CONFIG_IMX2_WDT, or even CONFIG_ARCH_MXC, which implies that imx2_wdt won't be supported.

Yeah, this config wasn't included in the set I sent back then. Maxim, please provide it to Ben. We are in the process of migrating to the CIP kernel right now.

Thanks,
Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
#14 lore raw

cover Backport of watchdog core triggered keepalive infrastructure

In reply to Maxim Yu. Osipov

Please use one of these formats for upstream references in the commit messages:

    commit 0123456789012345678901234567890123456789 upstream.

    [ Upstream commit 0123456789012345678901234567890123456789 ]

Ben.

Signature · 2 lines
Ben Hutchings
Software Developer, Codethink Ltd.