Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1540205 > unrolled thread

[PATCH v2 0/2] watchdog: allow setting deadline for opening /dev/watchdogN

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 0/2] watchdog: allow setting deadline for opening /dev/watchdogN Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2016-12-12 10:30 +0100
    [PATCH v2 2/2] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2016-12-12 10:30 +0100
      Re: [PATCH v2 2/2] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE Guenter Roeck <linux@roeck-us.net> - 2016-12-12 18:00 +0100
    [PATCH v2 1/2] watchdog: introduce watchdog_worker_should_ping helper Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2016-12-12 10:30 +0100
    [PATCH v3 0/2] watchdog: allow setting deadline for opening /dev/watchdogN Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2016-12-14 15:00 +0100
      [PATCH v3 2/2] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2016-12-14 15:00 +0100
        Re: [PATCH v3 2/2] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT Guenter Roeck <linux@roeck-us.net> - 2017-01-02 16:30 +0100
          Re: [PATCH v3 2/2] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2017-01-03 17:10 +0100
      [PATCH v3 1/2] watchdog: introduce watchdog_worker_should_ping helper Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2016-12-14 15:00 +0100
      Re: [PATCH v3 0/2] watchdog: allow setting deadline for opening  /dev/watchdogN Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2017-01-02 09:20 +0100

#1540205 — [PATCH v2 0/2] watchdog: allow setting deadline for opening /dev/watchdogN

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.

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 (with a suitably
modified driver for setting WDOG_HW_RUNNING) and a Wandboard.

Changes since the initial RFC (https://lkml.org/lkml/2016/7/14/254):
take the timeout value from the device tree node rather than a
watchdog module parameter.

Rasmus Villemoes (2):
  watchdog: introduce watchdog_worker_should_ping helper
  watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE

 drivers/watchdog/Kconfig         | 19 +++++++++++++++++++
 drivers/watchdog/watchdog_core.c | 17 +++++++++++++++++
 drivers/watchdog/watchdog_dev.c  | 38 +++++++++++++++++++++++++++++++++++++-
 include/linux/watchdog.h         |  9 +++++++++
 4 files changed, 82 insertions(+), 1 deletion(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1540206 — [PATCH v2 2/2] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE

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 if userspace fails to come up.

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, ...).

The open timeout is also used as a maximum time for an application to
re-open /dev/watchdogN after closing it.

The open timeout is taken from the device tree
property "open-timeout", and if that is not present, defaults to
CONFIG_WATCHDOG_DEFAULT_OPEN_TIMEOUT (whose default value itself
is five minutes).

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/Kconfig         | 19 +++++++++++++++++++
 drivers/watchdog/watchdog_core.c | 17 +++++++++++++++++
 drivers/watchdog/watchdog_dev.c  | 33 ++++++++++++++++++++++++++++++++-
 include/linux/watchdog.h         |  9 +++++++++
 4 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 3eb58cb..908bb3f 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -52,6 +52,25 @@ config WATCHDOG_SYSFS
 	  Say Y here if you want to enable watchdog device status read through
 	  sysfs attributes.
 
+config WATCHDOG_OPEN_DEADLINE
+	bool "Allow deadline for opening watchdog device"
+	help
+	  If a watchdog driver indicates that to the framework that
+	  the hardware watchdog is running, the framework takes care
+	  of pinging the watchdog until userspace opens
+	  /dev/watchdogN. By selecting this option, the open-timeout
+	  device tree property is used as an upper bound for which the
+	  kernel does this - thus, if userspace hasn't opened the
+	  device within this time, the board reboots.
+
+config WATCHDOG_DEFAULT_OPEN_TIMEOUT
+	int "Default timeout value for opening watchdog device"
+	depends on WATCHDOG_OPEN_DEADLINE
+	default 300
+	help
+	  The default value used when the watchdog's device tree node
+	  does not have the "open-timeout" property.
+
 #
 # General Watchdog drivers
 #
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 74265b2..31294b2 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -191,6 +191,21 @@ void watchdog_set_restart_priority(struct watchdog_device *wdd, int priority)
 }
 EXPORT_SYMBOL_GPL(watchdog_set_restart_priority);
 
+static void
+watchdog_set_open_timeout(struct watchdog_device *wdd)
+{
+#ifdef CONFIG_WATCHDOG_OPEN_DEADLINE
+	u32 t;
+	struct device *dev;
+
+	dev = wdd->parent;
+	if (dev && !of_property_read_u32(dev->of_node, "open-timeout", &t))
+		wdd->open_timeout = t;
+	else
+		wdd->open_timeout = CONFIG_WATCHDOG_DEFAULT_OPEN_TIMEOUT;
+#endif
+}
+
 static int __watchdog_register_device(struct watchdog_device *wdd)
 {
 	int ret, id = -1;
@@ -225,6 +240,8 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		return id;
 	wdd->id = id;
 
+	watchdog_set_open_timeout(wdd);
+
 	ret = watchdog_dev_register(wdd);
 	if (ret) {
 		ida_simple_remove(&watchdog_ida, id);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index ca0a000..f725e0b 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -80,6 +80,29 @@ static struct watchdog_core_data *old_wd_data;
 
 static struct workqueue_struct *watchdog_wq;
 
+#ifdef CONFIG_WATCHDOG_OPEN_DEADLINE
+static bool watchdog_past_open_deadline(struct watchdog_device *wdd)
+{
+	if (!wdd->open_timeout)
+		return false;
+	return time_is_before_jiffies(wdd->open_deadline);
+}
+
+static void watchdog_set_open_deadline(struct watchdog_device *wdd)
+{
+	wdd->open_deadline = jiffies + msecs_to_jiffies(1000 * wdd->open_timeout);
+}
+#else
+static bool watchdog_past_open_deadline(struct watchdog_device *wdd)
+{
+	return false;
+}
+
+static void watchdog_set_open_deadline(struct watchdog_device *wdd)
+{
+}
+#endif
+
 static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 {
 	/* All variables in milli-seconds */
@@ -194,7 +217,13 @@ static int watchdog_ping(struct watchdog_device *wdd)
 
 static bool watchdog_worker_should_ping(struct watchdog_device *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(wdd);
 }
 
 static void watchdog_ping_work(struct work_struct *work)
@@ -857,6 +886,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
 		watchdog_ping(wdd);
 	}
 
+	watchdog_set_open_deadline(wdd);
 	watchdog_update_worker(wdd);
 
 	/* make sure that /dev/watchdog can be re-opened */
@@ -955,6 +985,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 = jiffies - 1;
+	watchdog_set_open_deadline(wdd);
 
 	/*
 	 * If the watchdog is running, prevent its driver from being unloaded,
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 35a4d81..c4e7ff8 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -76,6 +76,11 @@ struct watchdog_ops {
  * @max_hw_heartbeat_ms:
  *		Hardware limit for maximum timeout, in milli-seconds.
  *		Replaces max_timeout if specified.
+ * @open_timeout:
+ *              The maximum time for which the kernel will ping the
+ *              device after registration.
+ * @open_deadline:
+ *              Set to jiffies + @open_timeout at registration.
  * @reboot_nb:	The notifier block to stop watchdog on reboot.
  * @restart_nb:	The notifier block to register a restart function.
  * @driver_data:Pointer to the drivers private data.
@@ -107,6 +112,10 @@ struct watchdog_device {
 	unsigned int max_timeout;
 	unsigned int min_hw_heartbeat_ms;
 	unsigned int max_hw_heartbeat_ms;
+#ifdef CONFIG_WATCHDOG_OPEN_DEADLINE
+	unsigned long open_timeout;
+	unsigned long open_deadline;
+#endif
 	struct notifier_block reboot_nb;
 	struct notifier_block restart_nb;
 	void *driver_data;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1540457 — Re: [PATCH v2 2/2] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE

Hi,

On Mon, Dec 12, 2016 at 10:17:53AM +0100, 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 if userspace fails to come up.
> 
> 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, ...).
> 
> The open timeout is also used as a maximum time for an application to
> re-open /dev/watchdogN after closing it.
> 
> The open timeout is taken from the device tree
> property "open-timeout", and if that is not present, defaults to
> CONFIG_WATCHDOG_DEFAULT_OPEN_TIMEOUT (whose default value itself
> is five minutes).
> 

You'll needd to submit the devicetree change as separate patch.

For this patch itself, I am in general not a friend of adding configuration
options, especially if there are other means to achieve the same goal and
the added code is minimal. This is true even more so in this case, where a
devicetree property could be present. The configuration option here really
means "ignore this devicetree property", and I don't think this would be
a good idea.

The timeout should be set through through a device property (ie use
device_property_read_u32() instead of of_property_read_u32()), to support
non-devicetree systems (we should probably make a similar change for the
timeout itself).

I don't think that a default should be set if the property is not present.
It should mean "no timeout", as with the current code.

For the configuration option, you'll have to convince Wim to accept it.

Thanks,
Guenter

> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/Kconfig         | 19 +++++++++++++++++++
>  drivers/watchdog/watchdog_core.c | 17 +++++++++++++++++
>  drivers/watchdog/watchdog_dev.c  | 33 ++++++++++++++++++++++++++++++++-
>  include/linux/watchdog.h         |  9 +++++++++
>  4 files changed, 77 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 3eb58cb..908bb3f 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -52,6 +52,25 @@ config WATCHDOG_SYSFS
>  	  Say Y here if you want to enable watchdog device status read through
>  	  sysfs attributes.
>  
> +config WATCHDOG_OPEN_DEADLINE
> +	bool "Allow deadline for opening watchdog device"
> +	help
> +	  If a watchdog driver indicates that to the framework that
> +	  the hardware watchdog is running, the framework takes care
> +	  of pinging the watchdog until userspace opens
> +	  /dev/watchdogN. By selecting this option, the open-timeout
> +	  device tree property is used as an upper bound for which the
> +	  kernel does this - thus, if userspace hasn't opened the
> +	  device within this time, the board reboots.
> +
> +config WATCHDOG_DEFAULT_OPEN_TIMEOUT
> +	int "Default timeout value for opening watchdog device"
> +	depends on WATCHDOG_OPEN_DEADLINE
> +	default 300
> +	help
> +	  The default value used when the watchdog's device tree node
> +	  does not have the "open-timeout" property.
> +
>  #
>  # General Watchdog drivers
>  #
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 74265b2..31294b2 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -191,6 +191,21 @@ void watchdog_set_restart_priority(struct watchdog_device *wdd, int priority)
>  }
>  EXPORT_SYMBOL_GPL(watchdog_set_restart_priority);
>  
> +static void
> +watchdog_set_open_timeout(struct watchdog_device *wdd)
> +{
> +#ifdef CONFIG_WATCHDOG_OPEN_DEADLINE
> +	u32 t;
> +	struct device *dev;
> +
> +	dev = wdd->parent;
> +	if (dev && !of_property_read_u32(dev->of_node, "open-timeout", &t))
> +		wdd->open_timeout = t;
> +	else
> +		wdd->open_timeout = CONFIG_WATCHDOG_DEFAULT_OPEN_TIMEOUT;
> +#endif
> +}
> +
>  static int __watchdog_register_device(struct watchdog_device *wdd)
>  {
>  	int ret, id = -1;
> @@ -225,6 +240,8 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  		return id;
>  	wdd->id = id;
>  
> +	watchdog_set_open_timeout(wdd);
> +
>  	ret = watchdog_dev_register(wdd);
>  	if (ret) {
>  		ida_simple_remove(&watchdog_ida, id);
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index ca0a000..f725e0b 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -80,6 +80,29 @@ static struct watchdog_core_data *old_wd_data;
>  
>  static struct workqueue_struct *watchdog_wq;
>  
> +#ifdef CONFIG_WATCHDOG_OPEN_DEADLINE
> +static bool watchdog_past_open_deadline(struct watchdog_device *wdd)
> +{
> +	if (!wdd->open_timeout)
> +		return false;
> +	return time_is_before_jiffies(wdd->open_deadline);
> +}
> +
> +static void watchdog_set_open_deadline(struct watchdog_device *wdd)
> +{
> +	wdd->open_deadline = jiffies + msecs_to_jiffies(1000 * wdd->open_timeout);
> +}
> +#else
> +static bool watchdog_past_open_deadline(struct watchdog_device *wdd)
> +{
> +	return false;
> +}
> +
> +static void watchdog_set_open_deadline(struct watchdog_device *wdd)
> +{
> +}
> +#endif
> +
>  static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>  {
>  	/* All variables in milli-seconds */
> @@ -194,7 +217,13 @@ static int watchdog_ping(struct watchdog_device *wdd)
>  
>  static bool watchdog_worker_should_ping(struct watchdog_device *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(wdd);
>  }
>  
>  static void watchdog_ping_work(struct work_struct *work)
> @@ -857,6 +886,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
>  		watchdog_ping(wdd);
>  	}
>  
> +	watchdog_set_open_deadline(wdd);
>  	watchdog_update_worker(wdd);
>  
>  	/* make sure that /dev/watchdog can be re-opened */
> @@ -955,6 +985,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 = jiffies - 1;
> +	watchdog_set_open_deadline(wdd);
>  
>  	/*
>  	 * If the watchdog is running, prevent its driver from being unloaded,
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 35a4d81..c4e7ff8 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -76,6 +76,11 @@ struct watchdog_ops {
>   * @max_hw_heartbeat_ms:
>   *		Hardware limit for maximum timeout, in milli-seconds.
>   *		Replaces max_timeout if specified.
> + * @open_timeout:
> + *              The maximum time for which the kernel will ping the
> + *              device after registration.
> + * @open_deadline:
> + *              Set to jiffies + @open_timeout at registration.
>   * @reboot_nb:	The notifier block to stop watchdog on reboot.
>   * @restart_nb:	The notifier block to register a restart function.
>   * @driver_data:Pointer to the drivers private data.
> @@ -107,6 +112,10 @@ struct watchdog_device {
>  	unsigned int max_timeout;
>  	unsigned int min_hw_heartbeat_ms;
>  	unsigned int max_hw_heartbeat_ms;
> +#ifdef CONFIG_WATCHDOG_OPEN_DEADLINE
> +	unsigned long open_timeout;
> +	unsigned long open_deadline;
> +#endif
>  	struct notifier_block reboot_nb;
>  	struct notifier_block restart_nb;
>  	void *driver_data;
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1540207 — [PATCH v2 1/2] watchdog: introduce watchdog_worker_should_ping helper

This will be useful when the condition becomes slightly more
complicated in the next patch.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/watchdog_dev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 32930a0..ca0a000 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -192,6 +192,11 @@ static int watchdog_ping(struct watchdog_device *wdd)
 	return __watchdog_ping(wdd);
 }
 
+static bool watchdog_worker_should_ping(struct watchdog_device *wdd)
+{
+	return wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd));
+}
+
 static void watchdog_ping_work(struct work_struct *work)
 {
 	struct watchdog_core_data *wd_data;
@@ -202,7 +207,7 @@ static void watchdog_ping_work(struct work_struct *work)
 
 	mutex_lock(&wd_data->lock);
 	wdd = wd_data->wdd;
-	if (wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd)))
+	if (watchdog_worker_should_ping(wdd))
 		__watchdog_ping(wdd);
 	mutex_unlock(&wd_data->lock);
 }
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1541918 — [PATCH v3 0/2] watchdog: allow setting deadline for opening /dev/watchdogN

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.

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 (with a suitably
modified driver for setting WDOG_HW_RUNNING) and a Wandboard.

Changes since v2:

 - Use device_property_read_u32 rather than of_property_read_u32

  - Remove config option to ifdef this completely out, instead set the
    default timeout to "infinite" to preserve existing behaviour.

Changes since v1:

 - Take the timeout value from the device tree node rather than a
   watchdog module parameter.


Rasmus Villemoes (2):
  watchdog: introduce watchdog_worker_should_ping helper
  watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

 drivers/watchdog/Kconfig         | 14 ++++++++++++++
 drivers/watchdog/watchdog_core.c | 16 ++++++++++++++++
 drivers/watchdog/watchdog_dev.c  | 27 ++++++++++++++++++++++++++-
 include/linux/watchdog.h         |  7 +++++++
 4 files changed, 63 insertions(+), 1 deletion(-)

-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1541920 — [PATCH v3 2/2] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

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 if userspace fails to come up.

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, ...).

The open timeout is also used as a maximum time for an application to
re-open /dev/watchdogN after closing it.

The open timeout is taken from the device property "open-timeout", and
if that is not present, defaults to CONFIG_WATCHDOG_OPEN_TIMEOUT, which
in turn defaults to 0, which means there is no upper limit (as must be
the default, since there may be existing systems out there relying on
the kernel taking care of the watchdog forever).

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/Kconfig         | 14 ++++++++++++++
 drivers/watchdog/watchdog_core.c | 16 ++++++++++++++++
 drivers/watchdog/watchdog_dev.c  | 22 +++++++++++++++++++++-
 include/linux/watchdog.h         |  7 +++++++
 4 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 3eb58cb..890418c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -52,6 +52,20 @@ config WATCHDOG_SYSFS
 	  Say Y here if you want to enable watchdog device status read through
 	  sysfs attributes.
 
+config WATCHDOG_OPEN_TIMEOUT
+	int "Default timeout value for opening watchdog device (seconds)"
+	default 0
+	help
+	  If a watchdog driver indicates to the framework that the
+	  hardware watchdog is running, the framework takes care of
+	  pinging the watchdog until userspace opens
+	  /dev/watchdogN. This value (overridden by the device's
+	  "open-timeout" property if present) sets an upper bound for
+	  how long the kernel does this - thus, if userspace hasn't
+	  opened the device within the timeout, the board reboots. A
+	  value of 0 means there is no timeout.
+
+
 #
 # General Watchdog drivers
 #
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 74265b2..d968e0f 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -38,6 +38,7 @@
 #include <linux/idr.h>		/* For ida_* macros */
 #include <linux/err.h>		/* For IS_ERR macros */
 #include <linux/of.h>		/* For of_get_timeout_sec */
+#include <linux/property.h>	/* For device_property_read_u32 */
 
 #include "watchdog_core.h"	/* For watchdog_dev_register/... */
 
@@ -191,6 +192,19 @@ void watchdog_set_restart_priority(struct watchdog_device *wdd, int priority)
 }
 EXPORT_SYMBOL_GPL(watchdog_set_restart_priority);
 
+static void
+watchdog_set_open_timeout(struct watchdog_device *wdd)
+{
+	u32 t;
+	struct device *dev;
+
+	dev = wdd->parent;
+	if (dev && !device_property_read_u32(dev, "open-timeout", &t))
+		wdd->open_timeout = t;
+	else
+		wdd->open_timeout = CONFIG_WATCHDOG_OPEN_TIMEOUT;
+}
+
 static int __watchdog_register_device(struct watchdog_device *wdd)
 {
 	int ret, id = -1;
@@ -225,6 +239,8 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		return id;
 	wdd->id = id;
 
+	watchdog_set_open_timeout(wdd);
+
 	ret = watchdog_dev_register(wdd);
 	if (ret) {
 		ida_simple_remove(&watchdog_ida, id);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index ca0a000..f153091 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -80,6 +80,18 @@ static struct watchdog_core_data *old_wd_data;
 
 static struct workqueue_struct *watchdog_wq;
 
+static bool watchdog_past_open_deadline(struct watchdog_device *wdd)
+{
+	if (!wdd->open_timeout)
+		return false;
+	return time_is_before_jiffies(wdd->open_deadline);
+}
+
+static void watchdog_set_open_deadline(struct watchdog_device *wdd)
+{
+	wdd->open_deadline = jiffies + msecs_to_jiffies(1000 * wdd->open_timeout);
+}
+
 static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 {
 	/* All variables in milli-seconds */
@@ -194,7 +206,13 @@ static int watchdog_ping(struct watchdog_device *wdd)
 
 static bool watchdog_worker_should_ping(struct watchdog_device *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(wdd);
 }
 
 static void watchdog_ping_work(struct work_struct *work)
@@ -857,6 +875,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
 		watchdog_ping(wdd);
 	}
 
+	watchdog_set_open_deadline(wdd);
 	watchdog_update_worker(wdd);
 
 	/* make sure that /dev/watchdog can be re-opened */
@@ -955,6 +974,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 = jiffies - 1;
+	watchdog_set_open_deadline(wdd);
 
 	/*
 	 * If the watchdog is running, prevent its driver from being unloaded,
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 35a4d81..a89a293 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -76,6 +76,11 @@ struct watchdog_ops {
  * @max_hw_heartbeat_ms:
  *		Hardware limit for maximum timeout, in milli-seconds.
  *		Replaces max_timeout if specified.
+ * @open_timeout:
+ *              The maximum time for which the kernel will ping the
+ *              device after registration.
+ * @open_deadline:
+ *              Set to jiffies + @open_timeout at registration.
  * @reboot_nb:	The notifier block to stop watchdog on reboot.
  * @restart_nb:	The notifier block to register a restart function.
  * @driver_data:Pointer to the drivers private data.
@@ -107,6 +112,8 @@ struct watchdog_device {
 	unsigned int max_timeout;
 	unsigned int min_hw_heartbeat_ms;
 	unsigned int max_hw_heartbeat_ms;
+	unsigned int open_timeout;
+	unsigned long open_deadline;
 	struct notifier_block reboot_nb;
 	struct notifier_block restart_nb;
 	void *driver_data;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1549249 — Re: [PATCH v3 2/2] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

On 12/14/2016 05:37 AM, 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 if userspace fails to come up.
>
> 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, ...).
>
> The open timeout is also used as a maximum time for an application to
> re-open /dev/watchdogN after closing it.
>
> The open timeout is taken from the device property "open-timeout", and
> if that is not present, defaults to CONFIG_WATCHDOG_OPEN_TIMEOUT, which
> in turn defaults to 0, which means there is no upper limit (as must be
> the default, since there may be existing systems out there relying on
> the kernel taking care of the watchdog forever).
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/Kconfig         | 14 ++++++++++++++
>  drivers/watchdog/watchdog_core.c | 16 ++++++++++++++++
>  drivers/watchdog/watchdog_dev.c  | 22 +++++++++++++++++++++-
>  include/linux/watchdog.h         |  7 +++++++
>  4 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 3eb58cb..890418c 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -52,6 +52,20 @@ config WATCHDOG_SYSFS
>  	  Say Y here if you want to enable watchdog device status read through
>  	  sysfs attributes.
>
> +config WATCHDOG_OPEN_TIMEOUT
> +	int "Default timeout value for opening watchdog device (seconds)"
> +	default 0
> +	help
> +	  If a watchdog driver indicates to the framework that the
> +	  hardware watchdog is running, the framework takes care of
> +	  pinging the watchdog until userspace opens
> +	  /dev/watchdogN. This value (overridden by the device's
> +	  "open-timeout" property if present) sets an upper bound for
> +	  how long the kernel does this - thus, if userspace hasn't
> +	  opened the device within the timeout, the board reboots. A
> +	  value of 0 means there is no timeout.
> +
> +

Dual empty line. Also, as mentioned before, I am not a friend of such
configuration variables. It forces distribution vendors to make the decision
for everyone. Please consider defining a command line parameter such as
watchdog_open_timeout.

>  #
>  # General Watchdog drivers
>  #
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 74265b2..d968e0f 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -38,6 +38,7 @@
>  #include <linux/idr.h>		/* For ida_* macros */
>  #include <linux/err.h>		/* For IS_ERR macros */
>  #include <linux/of.h>		/* For of_get_timeout_sec */
> +#include <linux/property.h>	/* For device_property_read_u32 */
>
>  #include "watchdog_core.h"	/* For watchdog_dev_register/... */
>
> @@ -191,6 +192,19 @@ void watchdog_set_restart_priority(struct watchdog_device *wdd, int priority)
>  }
>  EXPORT_SYMBOL_GPL(watchdog_set_restart_priority);
>
> +static void
> +watchdog_set_open_timeout(struct watchdog_device *wdd)
> +{
> +	u32 t;
> +	struct device *dev;
> +
> +	dev = wdd->parent;
> +	if (dev && !device_property_read_u32(dev, "open-timeout", &t))
> +		wdd->open_timeout = t;

The check if dev == NULL is unnecessary.

New devicetree properties need to be documented and approved by devicetree maintainers.

> +	else
> +		wdd->open_timeout = CONFIG_WATCHDOG_OPEN_TIMEOUT;
> +}
> +
>  static int __watchdog_register_device(struct watchdog_device *wdd)
>  {
>  	int ret, id = -1;
> @@ -225,6 +239,8 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  		return id;
>  	wdd->id = id;
>
> +	watchdog_set_open_timeout(wdd);
> +
Consider moving this call to watchdog_dev_register().

>  	ret = watchdog_dev_register(wdd);
>  	if (ret) {
>  		ida_simple_remove(&watchdog_ida, id);
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index ca0a000..f153091 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -80,6 +80,18 @@ static struct watchdog_core_data *old_wd_data;
>
>  static struct workqueue_struct *watchdog_wq;
>
> +static bool watchdog_past_open_deadline(struct watchdog_device *wdd)
> +{
> +	if (!wdd->open_timeout)
> +		return false;
> +	return time_is_before_jiffies(wdd->open_deadline);
> +}
> +
> +static void watchdog_set_open_deadline(struct watchdog_device *wdd)
> +{
> +	wdd->open_deadline = jiffies + msecs_to_jiffies(1000 * wdd->open_timeout);
> +}
> +
>  static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>  {
>  	/* All variables in milli-seconds */
> @@ -194,7 +206,13 @@ static int watchdog_ping(struct watchdog_device *wdd)
>
>  static bool watchdog_worker_should_ping(struct watchdog_device *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(wdd);
>  }
>
>  static void watchdog_ping_work(struct work_struct *work)
> @@ -857,6 +875,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
>  		watchdog_ping(wdd);
>  	}
>
> +	watchdog_set_open_deadline(wdd);
>  	watchdog_update_worker(wdd);
>
>  	/* make sure that /dev/watchdog can be re-opened */
> @@ -955,6 +974,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 = jiffies - 1;
> +	watchdog_set_open_deadline(wdd);
>
>  	/*
>  	 * If the watchdog is running, prevent its driver from being unloaded,
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 35a4d81..a89a293 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -76,6 +76,11 @@ struct watchdog_ops {
>   * @max_hw_heartbeat_ms:
>   *		Hardware limit for maximum timeout, in milli-seconds.
>   *		Replaces max_timeout if specified.
> + * @open_timeout:
> + *              The maximum time for which the kernel will ping the
> + *              device after registration.
> + * @open_deadline:
> + *              Set to jiffies + @open_timeout at registration.
>   * @reboot_nb:	The notifier block to stop watchdog on reboot.
>   * @restart_nb:	The notifier block to register a restart function.
>   * @driver_data:Pointer to the drivers private data.
> @@ -107,6 +112,8 @@ struct watchdog_device {
>  	unsigned int max_timeout;
>  	unsigned int min_hw_heartbeat_ms;
>  	unsigned int max_hw_heartbeat_ms;
> +	unsigned int open_timeout;
> +	unsigned long open_deadline;

None of those need to be visible in drivers. I don't see why the variables should be
defined here and not in struct watchdog_core_data.

>  	struct notifier_block reboot_nb;
>  	struct notifier_block restart_nb;
>  	void *driver_data;
>

[toc] | [prev] | [next] | [standalone]

#1549910 — Re: [PATCH v3 2/2] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

On 2017-01-02 16:22, Guenter Roeck wrote:
> On 12/14/2016 05:37 AM, Rasmus Villemoes wrote:
>> +config WATCHDOG_OPEN_TIMEOUT
>> +    int "Default timeout value for opening watchdog device (seconds)"
>> +    default 0
>> +    help
>> +      If a watchdog driver indicates to the framework that the
>> +      hardware watchdog is running, the framework takes care of
>> +      pinging the watchdog until userspace opens
>> +      /dev/watchdogN. This value (overridden by the device's
>> +      "open-timeout" property if present) sets an upper bound for
>> +      how long the kernel does this - thus, if userspace hasn't
>> +      opened the device within the timeout, the board reboots. A
>> +      value of 0 means there is no timeout.
>> +
>> +
>
> Dual empty line. Also, as mentioned before, I am not a friend of such
> configuration variables. It forces distribution vendors to make the
> decision
> for everyone.

Well, unless the distribution can guarantee that something opens the 
watchdog device in a timely manner, the only sensible decision is to 
keep the default infinite timeout, so I don't see how this is in any way 
a burden for general distributions.

> Please consider defining a command line parameter such as
> watchdog_open_timeout.

I'd be happy to, and that was exactly what I did in the original 
proposal, where it could also be tweaked after boot via sysfs. But I'd 
still like the default value of that command line parameter to be 
settable via Kconfig. That's a lot easier to maintain than having to do 
parts of the kernel configuration in u-boot or whatever bootloader one uses.

>> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
>> index 35a4d81..a89a293 100644
>> --- a/include/linux/watchdog.h
>> +++ b/include/linux/watchdog.h
>> @@ -76,6 +76,11 @@ struct watchdog_ops {
>>   * @max_hw_heartbeat_ms:
>>   *        Hardware limit for maximum timeout, in milli-seconds.
>>   *        Replaces max_timeout if specified.
>> + * @open_timeout:
>> + *              The maximum time for which the kernel will ping the
>> + *              device after registration.
>> + * @open_deadline:
>> + *              Set to jiffies + @open_timeout at registration.
>>   * @reboot_nb:    The notifier block to stop watchdog on reboot.
>>   * @restart_nb:    The notifier block to register a restart function.
>>   * @driver_data:Pointer to the drivers private data.
>> @@ -107,6 +112,8 @@ struct watchdog_device {
>>      unsigned int max_timeout;
>>      unsigned int min_hw_heartbeat_ms;
>>      unsigned int max_hw_heartbeat_ms;
>> +    unsigned int open_timeout;
>> +    unsigned long open_deadline;
>
> None of those need to be visible in drivers. I don't see why the
> variables should be
> defined here and not in struct watchdog_core_data.

I completely agree, these values have nothing to do with individual 
drivers, but as I tried to convince you, they also, IMO, have nothing to 
do with individual devices.

So, since

 > New devicetree properties need to be documented and approved by
 > devicetree maintainers.

I'd like to start with implementing this _just_ as a command line 
parameter (if I can't convince you to accept the Kconfig part we can 
always maintain that trivial addition internally).  If anyone down the 
road feels strongly about the possibility of setting the deadline via a 
device property, it can be added later if they do that work.

How about that?

-- 
Rasmus Villemoes
Software Developer
Prevas A/S
Hedeager 1
DK-8200 Aarhus N
+45 51210274
rasmus.villemoes@prevas.dk
www.prevas.dk

[toc] | [prev] | [next] | [standalone]

#1541921 — [PATCH v3 1/2] watchdog: introduce watchdog_worker_should_ping helper

This will be useful when the condition becomes slightly more
complicated in the next patch.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/watchdog_dev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 32930a0..ca0a000 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -192,6 +192,11 @@ static int watchdog_ping(struct watchdog_device *wdd)
 	return __watchdog_ping(wdd);
 }
 
+static bool watchdog_worker_should_ping(struct watchdog_device *wdd)
+{
+	return wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd));
+}
+
 static void watchdog_ping_work(struct work_struct *work)
 {
 	struct watchdog_core_data *wd_data;
@@ -202,7 +207,7 @@ static void watchdog_ping_work(struct work_struct *work)
 
 	mutex_lock(&wd_data->lock);
 	wdd = wd_data->wdd;
-	if (wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd)))
+	if (watchdog_worker_should_ping(wdd))
 		__watchdog_ping(wdd);
 	mutex_unlock(&wd_data->lock);
 }
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1549014 — Re: [PATCH v3 0/2] watchdog: allow setting deadline for opening /dev/watchdogN

On 2016-12-14 14:37, Rasmus Villemoes wrote:

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

Ping.

Rasmus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web