[PATCH v10 0/3] watchdog: allow setting deadline for opening /dev/watchdogN

[PATCH v10 0/3] watchdog: allow setting deadline for opening /dev/watchdogN

[Rasmus Villemoes]

If a watchdog driver tells the framework that the device is running,
the framework takes care of feeding the watchdog until userspace opens
the device. If the userspace application which is supposed to do that
never comes up properly, the watchdog is fed indefinitely by the
kernel. This can be especially problematic for embedded devices.

The existing handle_boot_enabled cmdline parameter/config option
partially solves that, but that is only usable for the subset of
hardware watchdogs that have (or can be configured by the bootloader
to have) a timeout that is sufficient to make it realistic for
userspace to come up. Many devices have timeouts of only a few
seconds, or even less, making handle_boot_enabled insufficient.

These patches allow one to set a maximum time for which the kernel
will feed the watchdog, thus ensuring that either userspace has come
up, or the board gets reset. This allows fallback logic in the
bootloader to attempt some recovery (for example, if an automatic
update is in progress, it could roll back to the previous version).

The patches have been tested on a Raspberry Pi 2 and a Wandboard.

Changes in v10: The open_timeout now only applies to the first open
from userspace. If userspace needs to close and re-open the watchdog
device (e.g. to re-exec itself), and wants the board to reset in case
it doesn't come back quickly enough, the open_timeout can easily be
emulated by combining nowayout with an appropriate WDIOC_SETTIMEOUT.

Changes in v9: Make the unit seconds instead of milliseconds.

Rasmus Villemoes (3):
  watchdog: introduce watchdog.open_timeout commandline parameter
  watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT
  watchdog: make the device time out at open_deadline when open_timeout
    is used

 .../watchdog/watchdog-parameters.txt          |  8 ++++
 drivers/watchdog/Kconfig                      |  9 ++++
 drivers/watchdog/watchdog_dev.c               | 48 ++++++++++++++++---
 3 files changed, 59 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH v10 1/3] watchdog: introduce watchdog.open_timeout commandline parameter

[Rasmus Villemoes]

The watchdog framework takes care of feeding a hardware watchdog until
userspace opens /dev/watchdogN. If that never happens for some reason
(buggy init script, corrupt root filesystem or whatnot) but the kernel
itself is fine, the machine stays up indefinitely. This patch allows
setting an upper limit for how long the kernel will take care of the
watchdog, thus ensuring that the watchdog will eventually reset the
machine.

A value of 0 (the default) means infinite timeout, preserving the
current behaviour.

This is particularly useful for embedded devices where some fallback
logic is implemented in the bootloader (e.g., use a different root
partition, boot from network, ...).

There is already handle_boot_enabled serving a similar purpose. However,
such a binary choice is unsuitable if the hardware watchdog cannot be
programmed by the bootloader to provide a timeout long enough for
userspace to get up and running. Many of the embedded devices we see use
external (gpio-triggered) watchdogs with a fixed timeout of the order of
1-2 seconds.

The open timeout only applies for the first open from
userspace. Should userspace need to close the watchdog device, with
the intention of re-opening it shortly, the application can emulate
the open timeout feature by combining the nowayout feature with an
appropriate WDIOC_SETTIMEOUT immediately prior to closing the device.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 .../watchdog/watchdog-parameters.txt          |  8 +++++
 drivers/watchdog/watchdog_dev.c               | 36 ++++++++++++++++++-
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
index 0b88e333f9e1..32d3606caa65 100644
--- a/Documentation/watchdog/watchdog-parameters.txt
+++ b/Documentation/watchdog/watchdog-parameters.txt
@@ -8,6 +8,14 @@ See Documentation/admin-guide/kernel-parameters.rst for information on
 providing kernel parameters for builtin drivers versus loadable
 modules.
 
+The watchdog core parameter watchdog.open_timeout is the maximum time,
+in seconds, for which the watchdog framework will take care of pinging
+a running hardware watchdog until userspace opens the corresponding
+/dev/watchdogN device. A value of 0 (the default) means an infinite
+timeout. Setting this to a non-zero value can be useful to ensure that
+either userspace comes up properly, or the board gets reset and allows
+fallback logic in the bootloader to try something else.
+
 
 -------------------------------------------------
 acquirewdt:
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 252a7c7b6592..e4b51db48f0e 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -69,6 +69,7 @@ struct watchdog_core_data {
 	struct mutex lock;
 	ktime_t last_keepalive;
 	ktime_t last_hw_keepalive;
+	ktime_t open_deadline;
 	struct hrtimer timer;
 	struct kthread_work work;
 	unsigned long status;		/* Internal status bits */
@@ -87,6 +88,19 @@ static struct kthread_worker *watchdog_kworker;
 static bool handle_boot_enabled =
 	IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
 
+static unsigned open_timeout;
+
+static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
+{
+	return ktime_after(ktime_get(), data->open_deadline);
+}
+
+static void watchdog_set_open_deadline(struct watchdog_core_data *data)
+{
+	data->open_deadline = open_timeout ?
+		ktime_get() + ktime_set(open_timeout, 0) : KTIME_MAX;
+}
+
 static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 {
 	/* All variables in milli-seconds */
@@ -211,7 +225,13 @@ static bool watchdog_worker_should_ping(struct watchdog_core_data *wd_data)
 {
 	struct watchdog_device *wdd = wd_data->wdd;
 
-	return wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd));
+	if (!wdd)
+		return false;
+
+	if (watchdog_active(wdd))
+		return true;
+
+	return watchdog_hw_running(wdd) && !watchdog_past_open_deadline(wd_data);
 }
 
 static void watchdog_ping_work(struct kthread_work *work)
@@ -824,6 +844,15 @@ static int watchdog_open(struct inode *inode, struct file *file)
 	if (!hw_running)
 		kref_get(&wd_data->kref);
 
+	/*
+	 * open_timeout only applies for the first open from
+	 * userspace. Set open_deadline to infinity so that the kernel
+	 * will take care of an always-running hardware watchdog in
+	 * case the device gets magic-closed or WDIOS_DISABLECARD is
+	 * applied.
+	 */
+	wd_data->open_deadline = KTIME_MAX;
+
 	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
 	return stream_open(inode, file);
 
@@ -983,6 +1012,7 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
 
 	/* Record time of most recent heartbeat as 'just before now'. */
 	wd_data->last_hw_keepalive = ktime_sub(ktime_get(), 1);
+	watchdog_set_open_deadline(wd_data);
 
 	/*
 	 * If the watchdog is running, prevent its driver from being unloaded,
@@ -1181,3 +1211,7 @@ 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)) ")");
+
+module_param(open_timeout, uint, 0644);
+MODULE_PARM_DESC(open_timeout,
+	"Maximum time (in seconds, 0 means infinity) for userspace to take over a running watchdog (default=0)");
-- 
2.20.1

Re: [PATCH v10 1/3] watchdog: introduce watchdog.open_timeout commandline parameter

[Guenter Roeck]

On Wed, Jun 05, 2019 at 02:06:41PM +0000, Rasmus Villemoes wrote:
> The watchdog framework takes care of feeding a hardware watchdog until
> userspace opens /dev/watchdogN. If that never happens for some reason
> (buggy init script, corrupt root filesystem or whatnot) but the kernel
> itself is fine, the machine stays up indefinitely. This patch allows
> setting an upper limit for how long the kernel will take care of the
> watchdog, thus ensuring that the watchdog will eventually reset the
> machine.
> 
> A value of 0 (the default) means infinite timeout, preserving the
> current behaviour.
> 
> This is particularly useful for embedded devices where some fallback
> logic is implemented in the bootloader (e.g., use a different root
> partition, boot from network, ...).
> 
> There is already handle_boot_enabled serving a similar purpose. However,
> such a binary choice is unsuitable if the hardware watchdog cannot be
> programmed by the bootloader to provide a timeout long enough for
> userspace to get up and running. Many of the embedded devices we see use
> external (gpio-triggered) watchdogs with a fixed timeout of the order of
> 1-2 seconds.
> 
> The open timeout only applies for the first open from
> userspace. Should userspace need to close the watchdog device, with
> the intention of re-opening it shortly, the application can emulate
> the open timeout feature by combining the nowayout feature with an
> appropriate WDIOC_SETTIMEOUT immediately prior to closing the device.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

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

> ---
>  .../watchdog/watchdog-parameters.txt          |  8 +++++
>  drivers/watchdog/watchdog_dev.c               | 36 ++++++++++++++++++-
>  2 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
> index 0b88e333f9e1..32d3606caa65 100644
> --- a/Documentation/watchdog/watchdog-parameters.txt
> +++ b/Documentation/watchdog/watchdog-parameters.txt
> @@ -8,6 +8,14 @@ See Documentation/admin-guide/kernel-parameters.rst for information on
>  providing kernel parameters for builtin drivers versus loadable
>  modules.
>  
> +The watchdog core parameter watchdog.open_timeout is the maximum time,
> +in seconds, for which the watchdog framework will take care of pinging
> +a running hardware watchdog until userspace opens the corresponding
> +/dev/watchdogN device. A value of 0 (the default) means an infinite
> +timeout. Setting this to a non-zero value can be useful to ensure that
> +either userspace comes up properly, or the board gets reset and allows
> +fallback logic in the bootloader to try something else.
> +
>  
>  -------------------------------------------------
>  acquirewdt:
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 252a7c7b6592..e4b51db48f0e 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -69,6 +69,7 @@ struct watchdog_core_data {
>  	struct mutex lock;
>  	ktime_t last_keepalive;
>  	ktime_t last_hw_keepalive;
> +	ktime_t open_deadline;
>  	struct hrtimer timer;
>  	struct kthread_work work;
>  	unsigned long status;		/* Internal status bits */
> @@ -87,6 +88,19 @@ static struct kthread_worker *watchdog_kworker;
>  static bool handle_boot_enabled =
>  	IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
>  
> +static unsigned open_timeout;
> +
> +static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
> +{
> +	return ktime_after(ktime_get(), data->open_deadline);
> +}
> +
> +static void watchdog_set_open_deadline(struct watchdog_core_data *data)
> +{
> +	data->open_deadline = open_timeout ?
> +		ktime_get() + ktime_set(open_timeout, 0) : KTIME_MAX;
> +}
> +
>  static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>  {
>  	/* All variables in milli-seconds */
> @@ -211,7 +225,13 @@ static bool watchdog_worker_should_ping(struct watchdog_core_data *wd_data)
>  {
>  	struct watchdog_device *wdd = wd_data->wdd;
>  
> -	return wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd));
> +	if (!wdd)
> +		return false;
> +
> +	if (watchdog_active(wdd))
> +		return true;
> +
> +	return watchdog_hw_running(wdd) && !watchdog_past_open_deadline(wd_data);
>  }
>  
>  static void watchdog_ping_work(struct kthread_work *work)
> @@ -824,6 +844,15 @@ static int watchdog_open(struct inode *inode, struct file *file)
>  	if (!hw_running)
>  		kref_get(&wd_data->kref);
>  
> +	/*
> +	 * open_timeout only applies for the first open from
> +	 * userspace. Set open_deadline to infinity so that the kernel
> +	 * will take care of an always-running hardware watchdog in
> +	 * case the device gets magic-closed or WDIOS_DISABLECARD is
> +	 * applied.
> +	 */
> +	wd_data->open_deadline = KTIME_MAX;
> +
>  	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
>  	return stream_open(inode, file);
>  
> @@ -983,6 +1012,7 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
>  
>  	/* Record time of most recent heartbeat as 'just before now'. */
>  	wd_data->last_hw_keepalive = ktime_sub(ktime_get(), 1);
> +	watchdog_set_open_deadline(wd_data);
>  
>  	/*
>  	 * If the watchdog is running, prevent its driver from being unloaded,
> @@ -1181,3 +1211,7 @@ 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)) ")");
> +
> +module_param(open_timeout, uint, 0644);
> +MODULE_PARM_DESC(open_timeout,
> +	"Maximum time (in seconds, 0 means infinity) for userspace to take over a running watchdog (default=0)");

[PATCH v10 2/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

[Rasmus Villemoes]

This allows setting a default value for the watchdog.open_timeout
commandline parameter via Kconfig.

Some BSPs allow remote updating of the kernel image and root file
system, but updating the bootloader requires physical access. Hence, if
one has a firmware update that requires relaxing the
watchdog.open_timeout a little, the value used must be baked into the
kernel image itself and cannot come from the u-boot environment via the
kernel command line.

Being able to set the initial value in .config doesn't change the fact
that the value on the command line, if present, takes precedence, and is
of course immensely useful for development purposes while one has
console acccess, as well as usable in the cases where one can make a
permanent update of the kernel command line.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 Documentation/watchdog/watchdog-parameters.txt | 8 ++++----
 drivers/watchdog/Kconfig                       | 9 +++++++++
 drivers/watchdog/watchdog_dev.c                | 5 +++--
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
index 32d3606caa65..ec919dc895ff 100644
--- a/Documentation/watchdog/watchdog-parameters.txt
+++ b/Documentation/watchdog/watchdog-parameters.txt
@@ -11,10 +11,10 @@ modules.
 The watchdog core parameter watchdog.open_timeout is the maximum time,
 in seconds, for which the watchdog framework will take care of pinging
 a running hardware watchdog until userspace opens the corresponding
-/dev/watchdogN device. A value of 0 (the default) means an infinite
-timeout. Setting this to a non-zero value can be useful to ensure that
-either userspace comes up properly, or the board gets reset and allows
-fallback logic in the bootloader to try something else.
+/dev/watchdogN device. A value of 0 means an infinite timeout. Setting
+this to a non-zero value can be useful to ensure that either userspace
+comes up properly, or the board gets reset and allows fallback logic
+in the bootloader to try something else.
 
 
 -------------------------------------------------
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index ffe754539f5a..a8bd621e12f8 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -58,6 +58,15 @@ config WATCHDOG_HANDLE_BOOT_ENABLED
 	  the watchdog on its own. Thus if your userspace does not start fast
 	  enough your device will reboot.
 
+config WATCHDOG_OPEN_TIMEOUT
+	int "Timeout value for opening watchdog device"
+	default 0
+	help
+	  The maximum time, in seconds, for which the watchdog framework takes
+	  care of pinging a hardware watchdog.  A value of 0 means infinite. The
+	  value set here can be overridden by the commandline parameter
+	  "watchdog.open_timeout".
+
 config WATCHDOG_SYSFS
 	bool "Read different watchdog information through sysfs"
 	help
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index e4b51db48f0e..334b810db2cf 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -88,7 +88,7 @@ static struct kthread_worker *watchdog_kworker;
 static bool handle_boot_enabled =
 	IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
 
-static unsigned open_timeout;
+static unsigned open_timeout = CONFIG_WATCHDOG_OPEN_TIMEOUT;
 
 static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
 {
@@ -1214,4 +1214,5 @@ MODULE_PARM_DESC(handle_boot_enabled,
 
 module_param(open_timeout, uint, 0644);
 MODULE_PARM_DESC(open_timeout,
-	"Maximum time (in seconds, 0 means infinity) for userspace to take over a running watchdog (default=0)");
+	"Maximum time (in seconds, 0 means infinity) for userspace to take over a running watchdog (default="
+	__MODULE_STRING(CONFIG_WATCHDOG_OPEN_TIMEOUT) ")");
-- 
2.20.1

Re: [PATCH v10 2/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

[Guenter Roeck]

On Wed, Jun 05, 2019 at 02:06:43PM +0000, Rasmus Villemoes wrote:
> This allows setting a default value for the watchdog.open_timeout
> commandline parameter via Kconfig.
> 
> Some BSPs allow remote updating of the kernel image and root file
> system, but updating the bootloader requires physical access. Hence, if
> one has a firmware update that requires relaxing the
> watchdog.open_timeout a little, the value used must be baked into the
> kernel image itself and cannot come from the u-boot environment via the
> kernel command line.
> 
> Being able to set the initial value in .config doesn't change the fact
> that the value on the command line, if present, takes precedence, and is
> of course immensely useful for development purposes while one has
> console acccess, as well as usable in the cases where one can make a
> permanent update of the kernel command line.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

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

> ---
>  Documentation/watchdog/watchdog-parameters.txt | 8 ++++----
>  drivers/watchdog/Kconfig                       | 9 +++++++++
>  drivers/watchdog/watchdog_dev.c                | 5 +++--
>  3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
> index 32d3606caa65..ec919dc895ff 100644
> --- a/Documentation/watchdog/watchdog-parameters.txt
> +++ b/Documentation/watchdog/watchdog-parameters.txt
> @@ -11,10 +11,10 @@ modules.
>  The watchdog core parameter watchdog.open_timeout is the maximum time,
>  in seconds, for which the watchdog framework will take care of pinging
>  a running hardware watchdog until userspace opens the corresponding
> -/dev/watchdogN device. A value of 0 (the default) means an infinite
> -timeout. Setting this to a non-zero value can be useful to ensure that
> -either userspace comes up properly, or the board gets reset and allows
> -fallback logic in the bootloader to try something else.
> +/dev/watchdogN device. A value of 0 means an infinite timeout. Setting
> +this to a non-zero value can be useful to ensure that either userspace
> +comes up properly, or the board gets reset and allows fallback logic
> +in the bootloader to try something else.
>  
>  
>  -------------------------------------------------
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index ffe754539f5a..a8bd621e12f8 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -58,6 +58,15 @@ config WATCHDOG_HANDLE_BOOT_ENABLED
>  	  the watchdog on its own. Thus if your userspace does not start fast
>  	  enough your device will reboot.
>  
> +config WATCHDOG_OPEN_TIMEOUT
> +	int "Timeout value for opening watchdog device"
> +	default 0
> +	help
> +	  The maximum time, in seconds, for which the watchdog framework takes
> +	  care of pinging a hardware watchdog.  A value of 0 means infinite. The
> +	  value set here can be overridden by the commandline parameter
> +	  "watchdog.open_timeout".
> +
>  config WATCHDOG_SYSFS
>  	bool "Read different watchdog information through sysfs"
>  	help
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index e4b51db48f0e..334b810db2cf 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -88,7 +88,7 @@ static struct kthread_worker *watchdog_kworker;
>  static bool handle_boot_enabled =
>  	IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
>  
> -static unsigned open_timeout;
> +static unsigned open_timeout = CONFIG_WATCHDOG_OPEN_TIMEOUT;
>  
>  static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
>  {
> @@ -1214,4 +1214,5 @@ MODULE_PARM_DESC(handle_boot_enabled,
>  
>  module_param(open_timeout, uint, 0644);
>  MODULE_PARM_DESC(open_timeout,
> -	"Maximum time (in seconds, 0 means infinity) for userspace to take over a running watchdog (default=0)");
> +	"Maximum time (in seconds, 0 means infinity) for userspace to take over a running watchdog (default="
> +	__MODULE_STRING(CONFIG_WATCHDOG_OPEN_TIMEOUT) ")");

[PATCH v10 3/3] watchdog: make the device time out at open_deadline when open_timeout is used

[Rasmus Villemoes]

When the watchdog device is not open by userspace, the kernel takes
care of pinging it. When the open_timeout feature is in use, we should
ensure that the hardware fires close to open_timeout seconds after the
kernel has assumed responsibility for the device.

To do this, simply reuse the logic that is already in place for
ensuring the same thing when userspace is responsible for regularly
pinging the device:

- When watchdog_active(wdd), this patch doesn't change anything.

- When !watchdoc_active(wdd), the "virtual timeout" should be taken to
be ->open_deadline". When the open_timeout feature is not used or the
device has been opened at least once, ->open_deadline is KTIME_MAX,
and the arithmetic ends up returning keepalive_interval as we used to.

This has been tested on a Wandboard with various combinations of
open_timeout and timeout-sec properties for the on-board watchdog by
booting with 'init=/bin/sh', timestamping the lines on the serial
console, and comparing the timestamp of the 'imx2-wdt 20bc000.wdog:
timeout nnn sec' line with the timestamp of the 'U-Boot SPL ...'
line (which appears just after reset).

Suggested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/watchdog_dev.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 334b810db2cf..edfb884044e0 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -133,14 +133,15 @@ static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
 	ktime_t virt_timeout;
 	unsigned int hw_heartbeat_ms;
 
-	virt_timeout = ktime_add(wd_data->last_keepalive,
-				 ms_to_ktime(timeout_ms));
+	if (watchdog_active(wdd))
+		virt_timeout = ktime_add(wd_data->last_keepalive,
+					 ms_to_ktime(timeout_ms));
+	else
+		virt_timeout = wd_data->open_deadline;
+
 	hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms);
 	keepalive_interval = ms_to_ktime(hw_heartbeat_ms / 2);
 
-	if (!watchdog_active(wdd))
-		return keepalive_interval;
-
 	/*
 	 * To ensure that the watchdog times out wdd->timeout seconds
 	 * after the most recent ping from userspace, the last
-- 
2.20.1

Re: [PATCH v10 3/3] watchdog: make the device time out at open_deadline when open_timeout is used

[Guenter Roeck]

On Wed, Jun 05, 2019 at 02:06:44PM +0000, Rasmus Villemoes wrote:
> When the watchdog device is not open by userspace, the kernel takes
> care of pinging it. When the open_timeout feature is in use, we should
> ensure that the hardware fires close to open_timeout seconds after the
> kernel has assumed responsibility for the device.
> 
> To do this, simply reuse the logic that is already in place for
> ensuring the same thing when userspace is responsible for regularly
> pinging the device:
> 
> - When watchdog_active(wdd), this patch doesn't change anything.
> 
> - When !watchdoc_active(wdd), the "virtual timeout" should be taken to

s/watchdoc_active/watchdog_active/

otherwise

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

> be ->open_deadline". When the open_timeout feature is not used or the
> device has been opened at least once, ->open_deadline is KTIME_MAX,
> and the arithmetic ends up returning keepalive_interval as we used to.
> 
> This has been tested on a Wandboard with various combinations of
> open_timeout and timeout-sec properties for the on-board watchdog by
> booting with 'init=/bin/sh', timestamping the lines on the serial
> console, and comparing the timestamp of the 'imx2-wdt 20bc000.wdog:
> timeout nnn sec' line with the timestamp of the 'U-Boot SPL ...'
> line (which appears just after reset).
> 
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/watchdog_dev.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 334b810db2cf..edfb884044e0 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -133,14 +133,15 @@ static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
>  	ktime_t virt_timeout;
>  	unsigned int hw_heartbeat_ms;
>  
> -	virt_timeout = ktime_add(wd_data->last_keepalive,
> -				 ms_to_ktime(timeout_ms));
> +	if (watchdog_active(wdd))
> +		virt_timeout = ktime_add(wd_data->last_keepalive,
> +					 ms_to_ktime(timeout_ms));
> +	else
> +		virt_timeout = wd_data->open_deadline;
> +
>  	hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms);
>  	keepalive_interval = ms_to_ktime(hw_heartbeat_ms / 2);
>  
> -	if (!watchdog_active(wdd))
> -		return keepalive_interval;
> -
>  	/*
>  	 * To ensure that the watchdog times out wdd->timeout seconds
>  	 * after the most recent ping from userspace, the last

Re: [PATCH v10 3/3] watchdog: make the device time out at open_deadline when open_timeout is used

[Rasmus Villemoes]

On 07/06/2019 20.38, Guenter Roeck wrote:
> On Wed, Jun 05, 2019 at 02:06:44PM +0000, Rasmus Villemoes wrote:
>> When the watchdog device is not open by userspace, the kernel takes
>> care of pinging it. When the open_timeout feature is in use, we should
>> ensure that the hardware fires close to open_timeout seconds after the
>> kernel has assumed responsibility for the device.
>>
>> To do this, simply reuse the logic that is already in place for
>> ensuring the same thing when userspace is responsible for regularly
>> pinging the device:
>>
>> - When watchdog_active(wdd), this patch doesn't change anything.
>>
>> - When !watchdoc_active(wdd), the "virtual timeout" should be taken to
> 
> s/watchdoc_active/watchdog_active/
> 
> otherwise
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Thanks! Wim, can you fix up if/when applying, or do you prefer I resend?

Rasmus

Re: [PATCH v10 3/3] watchdog: make the device time out at open_deadline when open_timeout is used

[Guenter Roeck]

On 6/14/19 1:41 AM, Rasmus Villemoes wrote:
> On 07/06/2019 20.38, Guenter Roeck wrote:
>> On Wed, Jun 05, 2019 at 02:06:44PM +0000, Rasmus Villemoes wrote:
>>> When the watchdog device is not open by userspace, the kernel takes
>>> care of pinging it. When the open_timeout feature is in use, we should
>>> ensure that the hardware fires close to open_timeout seconds after the
>>> kernel has assumed responsibility for the device.
>>>
>>> To do this, simply reuse the logic that is already in place for
>>> ensuring the same thing when userspace is responsible for regularly
>>> pinging the device:
>>>
>>> - When watchdog_active(wdd), this patch doesn't change anything.
>>>
>>> - When !watchdoc_active(wdd), the "virtual timeout" should be taken to
>>
>> s/watchdoc_active/watchdog_active/
>>
>> otherwise
>>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> Thanks! Wim, can you fix up if/when applying, or do you prefer I resend?
> 

I made the change when applying the patch to my watchdog-next branch,
and Wim usually picks up patches from there, so we should be good.

Thanks,
Guenter

Re: [PATCH v10 3/3] watchdog: make the device time out at open_deadline when open_timeout is used

[Wim Van Sebroeck]

Hi Rasmus,

> > On Wed, Jun 05, 2019 at 02:06:44PM +0000, Rasmus Villemoes wrote:
> >> When the watchdog device is not open by userspace, the kernel takes
> >> care of pinging it. When the open_timeout feature is in use, we should
> >> ensure that the hardware fires close to open_timeout seconds after the
> >> kernel has assumed responsibility for the device.
> >>
> >> To do this, simply reuse the logic that is already in place for
> >> ensuring the same thing when userspace is responsible for regularly
> >> pinging the device:
> >>
> >> - When watchdog_active(wdd), this patch doesn't change anything.
> >>
> >> - When !watchdoc_active(wdd), the "virtual timeout" should be taken to
> > 
> > s/watchdoc_active/watchdog_active/
> > 
> > otherwise
> > 
> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> Thanks! Wim, can you fix up if/when applying, or do you prefer I resend?

I'll fix up when applying. No need to resend a new patch for that.

Kind regards,
Wim.