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

[PATCH v5 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.

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.

v5 is identical to v4 posted in January, just rebased to current
master (v4.12-rc2).

v4 is mostly identical to v1. The differences are that the ability to
compile out this feature is removed, and the ability to set the
default value for the watchdog.open_timeout command line parameter via
Kconfig is split into a separate patch.

Compared to v2/v3, this drops the ability to set the open_timeout via
a device property; I'll leave implementing that to those who actually
need it.

Rasmus Villemoes (3):
  watchdog: introduce watchdog_worker_should_ping helper
  watchdog: introduce watchdog.open_timeout commandline parameter
  watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

 Documentation/watchdog/watchdog-parameters.txt | 10 +++++++
 drivers/watchdog/Kconfig                       |  9 +++++++
 drivers/watchdog/watchdog_dev.c                | 37 +++++++++++++++++++++++---
 3 files changed, 52 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v5 1/3] watchdog: introduce watchdog_worker_should_ping helper

[Rasmus Villemoes]

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 | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index d5d2bbd..caa4b90 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -192,18 +192,23 @@ static int watchdog_ping(struct watchdog_device *wdd)
 	return __watchdog_ping(wdd);
 }
 
+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));
+}
+
 static void watchdog_ping_work(struct work_struct *work)
 {
 	struct watchdog_core_data *wd_data;
-	struct watchdog_device *wdd;
 
 	wd_data = container_of(to_delayed_work(work), struct watchdog_core_data,
 			       work);
 
 	mutex_lock(&wd_data->lock);
-	wdd = wd_data->wdd;
-	if (wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd)))
-		__watchdog_ping(wdd);
+	if (watchdog_worker_should_ping(wd_data))
+		__watchdog_ping(wd_data->wdd);
 	mutex_unlock(&wd_data->lock);
 }
 
-- 
2.7.4

[PATCH v5 2/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.

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.

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

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

diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
index 914518a..4801ec6 100644
--- a/Documentation/watchdog/watchdog-parameters.txt
+++ b/Documentation/watchdog/watchdog-parameters.txt
@@ -8,6 +8,15 @@ See Documentation/admin-guide/kernel-parameters.rst for information on
 providing kernel parameters for builtin drivers versus loadable
 modules.
 
+The watchdog core currently understands one parameter,
+watchdog.open_timeout. This is the maximum time, in milliseconds, for
+which the watchdog framework will take care of pinging a 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 caa4b90..c807067 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -66,6 +66,7 @@ struct watchdog_core_data {
 	struct mutex lock;
 	unsigned long last_keepalive;
 	unsigned long last_hw_keepalive;
+	unsigned long open_deadline;
 	struct delayed_work work;
 	unsigned long status;		/* Internal status bits */
 #define _WDOG_DEV_OPEN		0	/* Opened ? */
@@ -80,6 +81,21 @@ static struct watchdog_core_data *old_wd_data;
 
 static struct workqueue_struct *watchdog_wq;
 
+static unsigned open_timeout;
+module_param(open_timeout, uint, 0644);
+
+static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
+{
+	if (!open_timeout)
+		return false;
+	return time_is_before_jiffies(data->open_deadline);
+}
+
+static void watchdog_set_open_deadline(struct watchdog_core_data *data)
+{
+	data->open_deadline = jiffies + msecs_to_jiffies(open_timeout);
+}
+
 static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 {
 	/* All variables in milli-seconds */
@@ -196,7 +212,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 work_struct *work)
@@ -857,6 +879,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
 		watchdog_ping(wdd);
 	}
 
+	watchdog_set_open_deadline(wd_data);
 	watchdog_update_worker(wdd);
 
 	/* make sure that /dev/watchdog can be re-opened */
@@ -955,6 +978,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(wd_data);
 
 	/*
 	 * If the watchdog is running, prevent its driver from being unloaded,
-- 
2.7.4

[PATCH v5 3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

[Rasmus Villemoes]

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

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

diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
index 4801ec6..6a55a3d 100644
--- a/Documentation/watchdog/watchdog-parameters.txt
+++ b/Documentation/watchdog/watchdog-parameters.txt
@@ -12,10 +12,11 @@ The watchdog core currently understands one parameter,
 watchdog.open_timeout. This is the maximum time, in milliseconds, for
 which the watchdog framework will take care of pinging a 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.
+device. The defalt value is CONFIG_WATCHDOG_OPEN_TIMEOUT. 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 8b9049d..11946fb 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -52,6 +52,15 @@ config WATCHDOG_SYSFS
 	  Say Y here if you want to enable watchdog device status read through
 	  sysfs attributes.
 
+config WATCHDOG_OPEN_TIMEOUT
+	int "Timeout value for opening watchdog device"
+	default 0
+	help
+	  The maximum time, in milliseconds, 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".
+
 #
 # General Watchdog drivers
 #
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index c807067..098b9cb 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -81,7 +81,7 @@ static struct watchdog_core_data *old_wd_data;
 
 static struct workqueue_struct *watchdog_wq;
 
-static unsigned open_timeout;
+static unsigned open_timeout = CONFIG_WATCHDOG_OPEN_TIMEOUT;
 module_param(open_timeout, uint, 0644);
 
 static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
-- 
2.7.4

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

[Alan Cox]

On Mon, 22 May 2017 16:06:36 +0200
Rasmus Villemoes <rasmus.villemoes@prevas.dk> 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. 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).


This makes sense except for being a CONFIG_ option not a boot parameter.
If it's a boot parameter then the same kernel works for multiple systems
and is general. If it's compile time then you have to build a custom
kernel.

For some embedded stuff that might not matter (although I bet they'd
prefer it command line/device tree too) but for something like an x86
platform where you are deploying a standard vendor supplied kernel it's
bad to do it that way IMHO.

In other words I think you should drop patch 3 but the rest is good.

Alan

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

[Guenter Roeck]

On Mon, May 22, 2017 at 07:07:51PM +0100, Alan Cox wrote:
> On Mon, 22 May 2017 16:06:36 +0200
> Rasmus Villemoes <rasmus.villemoes@prevas.dk> 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. 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).
> 
> 
> This makes sense except for being a CONFIG_ option not a boot parameter.
> If it's a boot parameter then the same kernel works for multiple systems
> and is general. If it's compile time then you have to build a custom
> kernel.
> 
> For some embedded stuff that might not matter (although I bet they'd
> prefer it command line/device tree too) but for something like an x86
> platform where you are deploying a standard vendor supplied kernel it's
> bad to do it that way IMHO.
> 
> In other words I think you should drop patch 3 but the rest is good.
> 

Same here. Can we assume a formal Reviewed-by: from you for the first two
patches ?

Thanks,
Guenter

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

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

[Rasmus Villemoes]

On 2017-05-22 20:07, Alan Cox wrote:
> On Mon, 22 May 2017 16:06:36 +0200
> Rasmus Villemoes <rasmus.villemoes@prevas.dk> 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. 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).
> 
> 
> This makes sense except for being a CONFIG_ option not a boot parameter.
> If it's a boot parameter then the same kernel works for multiple systems
> and is general. If it's compile time then you have to build a custom
> kernel.

I don't understand. Patch 3 simply allows the default to be set in the
config, but it is very much still possible to set it via the boot
command line, overriding the default (whether that was a CONFIG or a
hardcoded 0).

> For some embedded stuff that might not matter (although I bet they'd
> prefer it command line/device tree too)

When building a full BSP, yes, one can cook it into the boot command
line in u-boot, but that means that to update it one has to edit one's
u-boot recipe. It also often makes testing-turnaround times longer,
since updating the kernel can often be done by just scp'ing a new image
to the board, while updating the bootloader is sometimes a much more
involved procedure (not to mention that that's sometimes not even
possible to do remotely). There's also the issue of the command line
getting rather crowded if every aspect of the kernel has to be
configured at boot time.

As an embedded developer, I'm all for being able to override this
timeout as well lots of other kernel behavior via the command line, but
at the same time, I also prefer being able to set the default values in
the kernel's .config.

 but for something like an x86
> platform where you are deploying a standard vendor supplied kernel it's
> bad to do it that way IMHO.

A "standard vendor supplied kernel" will never set that CONFIG to
anything but its default - how could it possibly decide how long
userspace should have to open /dev/watchdog0, or even if userspace will
ever do that? Again, patch 3 doesn't in any way remove the possiblity of
setting this on the command line, and the end user is no worse off just
because the vendor had to keep a default setting.

> In other words I think you should drop patch 3 but the rest is good.

For the life of me, I cannot see the downside of patch 3, or how it's
any different from CONSOLE_LOGLEVEL_DEFAULT, BLK_DEV_RAM_SIZE, or any
number of other parameters-with-defaults-from-Kconfig.

Rasmus

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

[Esben Haabendal]

On 22 May 2017 at 16:06, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
>
> Rasmus Villemoes (3):
>   watchdog: introduce watchdog_worker_should_ping helper
>   watchdog: introduce watchdog.open_timeout commandline parameter
>   watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT
>
>  Documentation/watchdog/watchdog-parameters.txt | 10 +++++++
>  drivers/watchdog/Kconfig                       |  9 +++++++
>  drivers/watchdog/watchdog_dev.c                | 37 +++++++++++++++++++++++---
>  3 files changed, 52 insertions(+), 4 deletions(-)


Thanks. This is definitely useful for embedded applications.

If this get merged, it should be able to replace several out-of-tree
hacks to accomplish the same thing.

For all 3 patches:
Reviewed-by: Esben Haabendal <esben@haabendal.dk>

/Esben

Re: [PATCH v5 1/3] watchdog: introduce watchdog_worker_should_ping helper

[Guenter Roeck]

On 05/22/2017 07:06 AM, Rasmus Villemoes wrote:
> This will be useful when the condition becomes slightly more
> complicated in the next patch.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

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

> ---
>   drivers/watchdog/watchdog_dev.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index d5d2bbd..caa4b90 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -192,18 +192,23 @@ static int watchdog_ping(struct watchdog_device *wdd)
>   	return __watchdog_ping(wdd);
>   }
>   
> +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));
> +}
> +
>   static void watchdog_ping_work(struct work_struct *work)
>   {
>   	struct watchdog_core_data *wd_data;
> -	struct watchdog_device *wdd;
>   
>   	wd_data = container_of(to_delayed_work(work), struct watchdog_core_data,
>   			       work);
>   
>   	mutex_lock(&wd_data->lock);
> -	wdd = wd_data->wdd;
> -	if (wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd)))
> -		__watchdog_ping(wdd);
> +	if (watchdog_worker_should_ping(wd_data))
> +		__watchdog_ping(wd_data->wdd);
>   	mutex_unlock(&wd_data->lock);
>   }
>   
> 

Re: [PATCH v5 2/3] watchdog: introduce watchdog.open_timeout commandline parameter

[Guenter Roeck]

On 05/22/2017 07:06 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.
> 
> 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.
> 
> A value of 0 (the default) means infinite timeout, preserving the
> current behaviour.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   Documentation/watchdog/watchdog-parameters.txt |  9 +++++++++
>   drivers/watchdog/watchdog_dev.c                | 26 +++++++++++++++++++++++++-
>   2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
> index 914518a..4801ec6 100644
> --- a/Documentation/watchdog/watchdog-parameters.txt
> +++ b/Documentation/watchdog/watchdog-parameters.txt
> @@ -8,6 +8,15 @@ See Documentation/admin-guide/kernel-parameters.rst for information on
>   providing kernel parameters for builtin drivers versus loadable
>   modules.
>   
> +The watchdog core currently understands one parameter,

We have a second parameter queued, handle_boot_enabled. Please rephrase
so we don't have to change the text with each new parameter.

Thanks,
Guenter

> +watchdog.open_timeout. This is the maximum time, in milliseconds, for
> +which the watchdog framework will take care of pinging a 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 caa4b90..c807067 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -66,6 +66,7 @@ struct watchdog_core_data {
>   	struct mutex lock;
>   	unsigned long last_keepalive;
>   	unsigned long last_hw_keepalive;
> +	unsigned long open_deadline;
>   	struct delayed_work work;
>   	unsigned long status;		/* Internal status bits */
>   #define _WDOG_DEV_OPEN		0	/* Opened ? */
> @@ -80,6 +81,21 @@ static struct watchdog_core_data *old_wd_data;
>   
>   static struct workqueue_struct *watchdog_wq;
>   
> +static unsigned open_timeout;
> +module_param(open_timeout, uint, 0644);
> +
> +static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
> +{
> +	if (!open_timeout)
> +		return false;
> +	return time_is_before_jiffies(data->open_deadline);
> +}
> +
> +static void watchdog_set_open_deadline(struct watchdog_core_data *data)
> +{
> +	data->open_deadline = jiffies + msecs_to_jiffies(open_timeout);
> +}
> +
>   static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>   {
>   	/* All variables in milli-seconds */
> @@ -196,7 +212,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 work_struct *work)
> @@ -857,6 +879,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
>   		watchdog_ping(wdd);
>   	}
>   
> +	watchdog_set_open_deadline(wd_data);
>   	watchdog_update_worker(wdd);
>   
>   	/* make sure that /dev/watchdog can be re-opened */
> @@ -955,6 +978,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(wd_data);
>   
>   	/*
>   	 * If the watchdog is running, prevent its driver from being unloaded,
> 

Re: [PATCH v5 2/3] watchdog: introduce watchdog.open_timeout commandline parameter

[Rasmus Villemoes]

On 2017-05-25 02:56, Guenter Roeck wrote:
> On 05/22/2017 07:06 AM, Rasmus Villemoes wrote:
>> diff --git a/Documentation/watchdog/watchdog-parameters.txt
>> b/Documentation/watchdog/watchdog-parameters.txt
>> index 914518a..4801ec6 100644
>> --- a/Documentation/watchdog/watchdog-parameters.txt
>> +++ b/Documentation/watchdog/watchdog-parameters.txt
>> @@ -8,6 +8,15 @@ See Documentation/admin-guide/kernel-parameters.rst
>> for information on
>>   providing kernel parameters for builtin drivers versus loadable
>>   modules.
>>   +The watchdog core currently understands one parameter,
> 
> We have a second parameter queued, handle_boot_enabled. Please rephrase
> so we don't have to change the text with each new parameter.

I agree that it makes sense to rephrase to avoid having to edit when
other parameters are added, and I'll send v6 in a moment. But regarding
the handle_boot_enabled, I think my patch set implements a superset of
that functionality (just set watchdog.open_timeout to 1ms), which is why
I asked Sebastian specifically to look at the patches, and he said that
they'd work for his use case as well. So while I personally don't really
care if both go in, it does seem a little silly and somewhat confusing
to have two parameters to do more-or-less the same thing.

Btw, where can I find the watchdog-next tree (or whatever it's called)
to see what is queued up?

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

[PATCH v6 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.

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.

v6 tweaks the wording in watchdog-parameters.txt to avoid having to
update it if and when the watchdog core grows new parameters. It also
adds a little more rationale to the commit messages for 2/3 and 3/3,
and adds Reviewed-bys to 1/3 which is unchanged from v5.

v5 is identical to v4 posted in January, just rebased to current
master (v4.12-rc2).

v4 is mostly identical to v1. The differences are that the ability to
compile out this feature is removed, and the ability to set the
default value for the watchdog.open_timeout command line parameter via
Kconfig is split into a separate patch.

Compared to v2/v3, this drops the ability to set the open_timeout via
a device property; I'll leave implementing that to those who actually
need it.

Rasmus Villemoes (3):
  watchdog: introduce watchdog_worker_should_ping helper
  watchdog: introduce watchdog.open_timeout commandline parameter
  watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

 Documentation/watchdog/watchdog-parameters.txt |  9 +++++++
 drivers/watchdog/Kconfig                       |  9 +++++++
 drivers/watchdog/watchdog_dev.c                | 37 +++++++++++++++++++++++---
 3 files changed, 51 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v6 1/3] watchdog: introduce watchdog_worker_should_ping helper

[Rasmus Villemoes]

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

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Esben Haabendal <esben@haabendal.dk>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/watchdog_dev.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index d5d2bbd..caa4b90 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -192,18 +192,23 @@ static int watchdog_ping(struct watchdog_device *wdd)
 	return __watchdog_ping(wdd);
 }
 
+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));
+}
+
 static void watchdog_ping_work(struct work_struct *work)
 {
 	struct watchdog_core_data *wd_data;
-	struct watchdog_device *wdd;
 
 	wd_data = container_of(to_delayed_work(work), struct watchdog_core_data,
 			       work);
 
 	mutex_lock(&wd_data->lock);
-	wdd = wd_data->wdd;
-	if (wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd)))
-		__watchdog_ping(wdd);
+	if (watchdog_worker_should_ping(wd_data))
+		__watchdog_ping(wd_data->wdd);
 	mutex_unlock(&wd_data->lock);
 }
 
-- 
2.7.4

[PATCH v6 2/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.

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.

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

The unit is milliseconds rather than seconds because that covers more
use cases. For example, one can effectively disable the kernel handling
by setting the open_timeout to 1 ms. There are also customers with very
strict requirements that may want to set the open_timeout to something
like 4500 ms, which combined with a hardware watchdog that must be
pinged every 250 ms ensures userspace is up no more than 5 seconds after
the bootloader hands control to the kernel (250 ms until the driver gets
registered and kernel handling starts, 4500 ms of kernel handling, and
then up to 250 ms from the last ping until userspace takes over).

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

diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
index 914518a..8577c27 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 milliseconds, for which the watchdog framework will take care of
+pinging a 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 caa4b90..c807067 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -66,6 +66,7 @@ struct watchdog_core_data {
 	struct mutex lock;
 	unsigned long last_keepalive;
 	unsigned long last_hw_keepalive;
+	unsigned long open_deadline;
 	struct delayed_work work;
 	unsigned long status;		/* Internal status bits */
 #define _WDOG_DEV_OPEN		0	/* Opened ? */
@@ -80,6 +81,21 @@ static struct watchdog_core_data *old_wd_data;
 
 static struct workqueue_struct *watchdog_wq;
 
+static unsigned open_timeout;
+module_param(open_timeout, uint, 0644);
+
+static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
+{
+	if (!open_timeout)
+		return false;
+	return time_is_before_jiffies(data->open_deadline);
+}
+
+static void watchdog_set_open_deadline(struct watchdog_core_data *data)
+{
+	data->open_deadline = jiffies + msecs_to_jiffies(open_timeout);
+}
+
 static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 {
 	/* All variables in milli-seconds */
@@ -196,7 +212,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 work_struct *work)
@@ -857,6 +879,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
 		watchdog_ping(wdd);
 	}
 
+	watchdog_set_open_deadline(wd_data);
 	watchdog_update_worker(wdd);
 
 	/* make sure that /dev/watchdog can be re-opened */
@@ -955,6 +978,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(wd_data);
 
 	/*
 	 * If the watchdog is running, prevent its driver from being unloaded,
-- 
2.7.4

[PATCH v6 3/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 | 3 ++-
 drivers/watchdog/Kconfig                       | 9 +++++++++
 drivers/watchdog/watchdog_dev.c                | 2 +-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
index 8577c27..fa34625 100644
--- a/Documentation/watchdog/watchdog-parameters.txt
+++ b/Documentation/watchdog/watchdog-parameters.txt
@@ -11,7 +11,8 @@ modules.
 The watchdog core parameter watchdog.open_timeout is the maximum time,
 in milliseconds, for which the watchdog framework will take care of
 pinging a hardware watchdog until userspace opens the corresponding
-/dev/watchdogN device. A value of 0 (the default) means an infinite
+/dev/watchdogN device. The defalt value is
+CONFIG_WATCHDOG_OPEN_TIMEOUT. 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 8b9049d..11946fb 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -52,6 +52,15 @@ config WATCHDOG_SYSFS
 	  Say Y here if you want to enable watchdog device status read through
 	  sysfs attributes.
 
+config WATCHDOG_OPEN_TIMEOUT
+	int "Timeout value for opening watchdog device"
+	default 0
+	help
+	  The maximum time, in milliseconds, 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".
+
 #
 # General Watchdog drivers
 #
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index c807067..098b9cb 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -81,7 +81,7 @@ static struct watchdog_core_data *old_wd_data;
 
 static struct workqueue_struct *watchdog_wq;
 
-static unsigned open_timeout;
+static unsigned open_timeout = CONFIG_WATCHDOG_OPEN_TIMEOUT;
 module_param(open_timeout, uint, 0644);
 
 static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
-- 
2.7.4

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

[Rasmus Villemoes]

On 2017-05-30 10:56, Rasmus Villemoes wrote:
> 
> v6 tweaks the wording in watchdog-parameters.txt to avoid having to
> update it if and when the watchdog core grows new parameters. It also
> adds a little more rationale to the commit messages for 2/3 and 3/3,
> and adds Reviewed-bys to 1/3 which is unchanged from v5.

Ping. Guenther, is there anything I can do on my end to get this in
before the 4.13 merge window opens? I realize you're busy, but I'd
really appreciate just a brief response to this and the other emails
I've sent the past few weeks.


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

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

[Guenter Roeck]

On 06/06/2017 01:08 AM, Rasmus Villemoes wrote:
> On 2017-05-30 10:56, Rasmus Villemoes wrote:
>>
>> v6 tweaks the wording in watchdog-parameters.txt to avoid having to
>> update it if and when the watchdog core grows new parameters. It also
>> adds a little more rationale to the commit messages for 2/3 and 3/3,
>> and adds Reviewed-bys to 1/3 which is unchanged from v5.
> 
> Ping. Guenther, is there anything I can do on my end to get this in
> before the 4.13 merge window opens? I realize you're busy, but I'd
> really appreciate just a brief response to this and the other emails
> I've sent the past few weeks.
> 
> 

You made a suggestion that this overlaps with another option, which forces me
to go back and check the entire exchange from the start, for which I did not
have time yet, sorry.

Guenter

Re: [v6, 2/3] watchdog: introduce watchdog.open_timeout commandline parameter

[Guenter Roeck]

On Tue, May 30, 2017 at 10:56:46AM +0200, 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.
> 
> 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.
> 
> A value of 0 (the default) means infinite timeout, preserving the
> current behaviour.
> 
> The unit is milliseconds rather than seconds because that covers more
> use cases. For example, one can effectively disable the kernel handling
> by setting the open_timeout to 1 ms. There are also customers with very
> strict requirements that may want to set the open_timeout to something
> like 4500 ms, which combined with a hardware watchdog that must be
> pinged every 250 ms ensures userspace is up no more than 5 seconds after
> the bootloader hands control to the kernel (250 ms until the driver gets
> registered and kernel handling starts, 4500 ms of kernel handling, and
> then up to 250 ms from the last ping until userspace takes over).
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

I finally found the time to look into this. It is sufficiently different
to handle_boot_enabled to keep it separate. I am mostly ok with the patch.
One comment below.

> ---
>  Documentation/watchdog/watchdog-parameters.txt |  8 ++++++++
>  drivers/watchdog/watchdog_dev.c                | 26 +++++++++++++++++++++++++-
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
> index 914518a..8577c27 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 milliseconds, for which the watchdog framework will take care of
> +pinging a 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 caa4b90..c807067 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -66,6 +66,7 @@ struct watchdog_core_data {
>  	struct mutex lock;
>  	unsigned long last_keepalive;
>  	unsigned long last_hw_keepalive;
> +	unsigned long open_deadline;
>  	struct delayed_work work;
>  	unsigned long status;		/* Internal status bits */
>  #define _WDOG_DEV_OPEN		0	/* Opened ? */
> @@ -80,6 +81,21 @@ static struct watchdog_core_data *old_wd_data;
>  
>  static struct workqueue_struct *watchdog_wq;
>  
> +static unsigned open_timeout;
> +module_param(open_timeout, uint, 0644);

MODULE_PARM_DESC is missing. I would also prefer to keep module_param()
and MODULE_PARM_DESC() together with the existing module parameter, ie at
the end of the file.

Thanks,
Guenter

Re: [v6,3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

[Guenter Roeck]

On Tue, May 30, 2017 at 10:56:47AM +0200, 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>

Wim, any thoughts on making this configurable ? I used to be opposed to it,
but it does seem to make some sense to me now after thinking about it.

Thanks,
Guenter

> ---
>  Documentation/watchdog/watchdog-parameters.txt | 3 ++-
>  drivers/watchdog/Kconfig                       | 9 +++++++++
>  drivers/watchdog/watchdog_dev.c                | 2 +-
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
> index 8577c27..fa34625 100644
> --- a/Documentation/watchdog/watchdog-parameters.txt
> +++ b/Documentation/watchdog/watchdog-parameters.txt
> @@ -11,7 +11,8 @@ modules.
>  The watchdog core parameter watchdog.open_timeout is the maximum time,
>  in milliseconds, for which the watchdog framework will take care of
>  pinging a hardware watchdog until userspace opens the corresponding
> -/dev/watchdogN device. A value of 0 (the default) means an infinite
> +/dev/watchdogN device. The defalt value is
> +CONFIG_WATCHDOG_OPEN_TIMEOUT. 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 8b9049d..11946fb 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -52,6 +52,15 @@ config WATCHDOG_SYSFS
>  	  Say Y here if you want to enable watchdog device status read through
>  	  sysfs attributes.
>  
> +config WATCHDOG_OPEN_TIMEOUT
> +	int "Timeout value for opening watchdog device"
> +	default 0
> +	help
> +	  The maximum time, in milliseconds, 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".
> +
>  #
>  # General Watchdog drivers
>  #
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index c807067..098b9cb 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -81,7 +81,7 @@ static struct watchdog_core_data *old_wd_data;
>  
>  static struct workqueue_struct *watchdog_wq;
>  
> -static unsigned open_timeout;
> +static unsigned open_timeout = CONFIG_WATCHDOG_OPEN_TIMEOUT;
>  module_param(open_timeout, uint, 0644);
>  
>  static bool watchdog_past_open_deadline(struct watchdog_core_data *data)

Re: [v6,3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

[Wim Van Sebroeck]

Hi Guenter,

> > 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>
> 
> Wim, any thoughts on making this configurable ? I used to be opposed to it,
> but it does seem to make some sense to me now after thinking about it.

I will look at it later this week.

Kind regards,
Wim.