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

Groups > linux.kernel > #1443310 > unrolled thread

[RFC 0/3] watchdog: introduce open deadline

Back to article view | Back to linux.kernel

Contents

  [RFC 0/3] watchdog: introduce open deadline Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2016-07-14 11:30 +0200
    [RFC 2/3] watchdog: introduce watchdog_worker_should_ping helper Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2016-07-14 11:30 +0200
    [RFC 1/3] watchdog: change watchdog_need_worker logic Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2016-07-14 11:30 +0200
      Re: [RFC 1/3] watchdog: change watchdog_need_worker logic Guenter Roeck <linux@roeck-us.net> - 2016-07-14 22:50 +0200
      Re: [RFC 1/3] watchdog: change watchdog_need_worker logic Wim Van Sebroeck <wim@iguana.be> - 2016-07-17 21:50 +0200
        Re: [RFC 1/3] watchdog: change watchdog_need_worker logic Guenter Roeck <linux@roeck-us.net> - 2016-07-17 22:00 +0200
          Re: [RFC 1/3] watchdog: change watchdog_need_worker logic Wim Van Sebroeck <wim@iguana.be> - 2016-07-17 22:40 +0200
    [RFC 3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2016-07-14 11:30 +0200
      Re: [RFC 3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE Guenter Roeck <linux@roeck-us.net> - 2016-07-14 16:50 +0200
        Re: [RFC 3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2016-07-15 09:50 +0200
          Re: [RFC 3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE Guenter Roeck <linux@roeck-us.net> - 2016-07-15 16:30 +0200
            Re: [RFC 3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2016-07-21 00:20 +0200
              Re: [RFC 3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE Guenter Roeck <linux@roeck-us.net> - 2016-07-21 02:40 +0200

#1443310 — [RFC 0/3] watchdog: introduce open deadline

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 - patches will be
submitted seperately) and a Wandboard.

Rasmus Villemoes (3):
  watchdog: change watchdog_need_worker logic
  watchdog: introduce watchdog_worker_should_ping helper
  watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE

 drivers/watchdog/Kconfig        | 21 +++++++++++++++
 drivers/watchdog/watchdog_dev.c | 60 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 74 insertions(+), 7 deletions(-)

-- 
2.5.0

[toc] | [next] | [standalone]

#1443312 — [RFC 2/3] 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 | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 14f8a92..da549e5 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -188,18 +188,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.5.0

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

#1443314 — [RFC 1/3] watchdog: change watchdog_need_worker logic

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

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

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

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

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

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

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

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

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 3595cff..14f8a92 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -92,9 +92,13 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 	 *   thus is aware that the framework supports generating heartbeat
 	 *   requests.
 	 * - Userspace requests a longer timeout than the hardware can handle.
+	 *
+	 * Alternatively, if userspace has not opened the watchdog
+	 * device, we take care of feeding the watchdog if it is
+	 * running.
 	 */
-	return hm && ((watchdog_active(wdd) && t > hm) ||
-		      (t && !watchdog_active(wdd) && watchdog_hw_running(wdd)));
+	return (hm && watchdog_active(wdd) && t > hm) ||
+		(t && !watchdog_active(wdd) && watchdog_hw_running(wdd));
 }
 
 static long watchdog_next_keepalive(struct watchdog_device *wdd)
@@ -107,7 +111,7 @@ static long watchdog_next_keepalive(struct watchdog_device *wdd)
 	unsigned int hw_heartbeat_ms;
 
 	virt_timeout = wd_data->last_keepalive + msecs_to_jiffies(timeout_ms);
-	hw_heartbeat_ms = min(timeout_ms, wdd->max_hw_heartbeat_ms);
+	hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms);
 	keepalive_interval = msecs_to_jiffies(hw_heartbeat_ms / 2);
 
 	if (!watchdog_active(wdd))
-- 
2.5.0

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

#1443772 — Re: [RFC 1/3] watchdog: change watchdog_need_worker logic

On Thu, Jul 14, 2016 at 11:16:26AM +0200, Rasmus Villemoes wrote:
> If the driver indicates that the watchdog is running, the framework
> should feed it until userspace opens the device, regardless of whether
> the driver has set max_hw_heartbeat_ms.
> 
> This patch only affects the case where wdd->max_hw_heartbeat_ms is
> zero, wdd->timeout is non-zero, the watchdog is not active and the
> hardware device is running (*):
> 
> - If wdd->timeout is zero, watchdog_need_worker() returns false both
> before and after this patch, and watchdog_next_keepalive() is not
> called.
> 
> - If watchdog_active(wdd), the return value from watchdog_need_worker
> is also the same as before (namely, hm && t > hm). Hence in that case,
> watchdog_next_keepalive() is only called if hm == max_hw_heartbeat_ms
> is non-zero, so the change to min_not_zero there is a no-op.
> 
> - If the watchdog is not active and the device is not running, we
> return false from watchdog_need_worker just as before.
> 
> That leaves the watchdog_hw_running(wdd) && !watchdog_active(wdd) &&
> wdd->timeout case. Again, it's easy to see that if
> wdd->max_hw_heartbeat_ms is non-zero, we return true from
> watchdog_need_worker with and without this patch, and the logic in
> watchdog_next_keepalive is unchanged. Finally, if
> wdd->max_hw_heartbeat_ms is 0, we used to end up in the
> cancel_delayed_work branch, whereas with this patch we end up
> scheduling a ping timeout_ms/2 from now.
> 
> (*) This should imply that no current kernel drivers are affected,
> since the only drivers which explicitly set WDOG_HW_RUNNING are
> imx2_wdt.c and dw_wdt.c, both of which also provide a non-zero value
> for max_hw_heartbeat_ms. The watchdog core also sets WDOG_HW_RUNNING,
> but only when the driver doesn't provide ->stop, in which case it
> must, according to Documentation/watchdog/watchdog-kernel-api.txt, set
> max_hw_heartbeat_ms.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

LGTM.

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

> ---
>  drivers/watchdog/watchdog_dev.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 3595cff..14f8a92 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -92,9 +92,13 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>  	 *   thus is aware that the framework supports generating heartbeat
>  	 *   requests.
>  	 * - Userspace requests a longer timeout than the hardware can handle.
> +	 *
> +	 * Alternatively, if userspace has not opened the watchdog
> +	 * device, we take care of feeding the watchdog if it is
> +	 * running.
>  	 */
> -	return hm && ((watchdog_active(wdd) && t > hm) ||
> -		      (t && !watchdog_active(wdd) && watchdog_hw_running(wdd)));
> +	return (hm && watchdog_active(wdd) && t > hm) ||
> +		(t && !watchdog_active(wdd) && watchdog_hw_running(wdd));
>  }
>  
>  static long watchdog_next_keepalive(struct watchdog_device *wdd)
> @@ -107,7 +111,7 @@ static long watchdog_next_keepalive(struct watchdog_device *wdd)
>  	unsigned int hw_heartbeat_ms;
>  
>  	virt_timeout = wd_data->last_keepalive + msecs_to_jiffies(timeout_ms);
> -	hw_heartbeat_ms = min(timeout_ms, wdd->max_hw_heartbeat_ms);
> +	hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms);
>  	keepalive_interval = msecs_to_jiffies(hw_heartbeat_ms / 2);
>  
>  	if (!watchdog_active(wdd))
> -- 
> 2.5.0
> 
> --
> 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]

#1445093 — Re: [RFC 1/3] watchdog: change watchdog_need_worker logic

Hi Rasmus,

> If the driver indicates that the watchdog is running, the framework
> should feed it until userspace opens the device, regardless of whether
> the driver has set max_hw_heartbeat_ms.
> 
> This patch only affects the case where wdd->max_hw_heartbeat_ms is
> zero, wdd->timeout is non-zero, the watchdog is not active and the
> hardware device is running (*):
> 
> - If wdd->timeout is zero, watchdog_need_worker() returns false both
> before and after this patch, and watchdog_next_keepalive() is not
> called.
> 
> - If watchdog_active(wdd), the return value from watchdog_need_worker
> is also the same as before (namely, hm && t > hm). Hence in that case,
> watchdog_next_keepalive() is only called if hm == max_hw_heartbeat_ms
> is non-zero, so the change to min_not_zero there is a no-op.
> 
> - If the watchdog is not active and the device is not running, we
> return false from watchdog_need_worker just as before.
> 
> That leaves the watchdog_hw_running(wdd) && !watchdog_active(wdd) &&
> wdd->timeout case. Again, it's easy to see that if
> wdd->max_hw_heartbeat_ms is non-zero, we return true from
> watchdog_need_worker with and without this patch, and the logic in
> watchdog_next_keepalive is unchanged. Finally, if
> wdd->max_hw_heartbeat_ms is 0, we used to end up in the
> cancel_delayed_work branch, whereas with this patch we end up
> scheduling a ping timeout_ms/2 from now.
> 
> (*) This should imply that no current kernel drivers are affected,
> since the only drivers which explicitly set WDOG_HW_RUNNING are
> imx2_wdt.c and dw_wdt.c, both of which also provide a non-zero value
> for max_hw_heartbeat_ms. The watchdog core also sets WDOG_HW_RUNNING,
> but only when the driver doesn't provide ->stop, in which case it
> must, according to Documentation/watchdog/watchdog-kernel-api.txt, set
> max_hw_heartbeat_ms.

This isn't completely true. We will have the following in the linux-watchdog tree:
drivers/watchdog/aspeed_wdt.c:		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
drivers/watchdog/dw_wdt.c:	set_bit(WDOG_HW_RUNNING, &wdd->status);
drivers/watchdog/dw_wdt.c:		set_bit(WDOG_HW_RUNNING, &wdd->status);
drivers/watchdog/imx2_wdt.c:	set_bit(WDOG_HW_RUNNING, &wdog->status);
drivers/watchdog/imx2_wdt.c:		set_bit(WDOG_HW_RUNNING, &wdog->status);
drivers/watchdog/max77620_wdt.c:		set_bit(WDOG_HW_RUNNING, &wdt_dev->status);
drivers/watchdog/sbsa_gwdt.c:		set_bit(WDOG_HW_RUNNING, &wdd->status);
drivers/watchdog/tangox_wdt.c:		set_bit(WDOG_HW_RUNNING, &dev->wdt.status);

I checked the ones that aren't mentioned and aspeed_wdt, max77620_wdt and sbsa_gwdt.c
also have a non-zero value for max_hw_heartbeat_ms. But tangox_wdt.c doesn't set it.
This one will need to be looked at closer.

> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/watchdog_dev.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 3595cff..14f8a92 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -92,9 +92,13 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>  	 *   thus is aware that the framework supports generating heartbeat
>  	 *   requests.
>  	 * - Userspace requests a longer timeout than the hardware can handle.
> +	 *
> +	 * Alternatively, if userspace has not opened the watchdog
> +	 * device, we take care of feeding the watchdog if it is
> +	 * running.
>  	 */
> -	return hm && ((watchdog_active(wdd) && t > hm) ||
> -		      (t && !watchdog_active(wdd) && watchdog_hw_running(wdd)));
> +	return (hm && watchdog_active(wdd) && t > hm) ||
> +		(t && !watchdog_active(wdd) && watchdog_hw_running(wdd));
>  }
>  
>  static long watchdog_next_keepalive(struct watchdog_device *wdd)
> @@ -107,7 +111,7 @@ static long watchdog_next_keepalive(struct watchdog_device *wdd)
>  	unsigned int hw_heartbeat_ms;
>  
>  	virt_timeout = wd_data->last_keepalive + msecs_to_jiffies(timeout_ms);
> -	hw_heartbeat_ms = min(timeout_ms, wdd->max_hw_heartbeat_ms);
> +	hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms);
>  	keepalive_interval = msecs_to_jiffies(hw_heartbeat_ms / 2);
>  
>  	if (!watchdog_active(wdd))
> -- 
> 2.5.0
> 

Kind regards,
Wim.

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

#1445094 — Re: [RFC 1/3] watchdog: change watchdog_need_worker logic

On 07/17/2016 12:24 PM, Wim Van Sebroeck wrote:
> Hi Rasmus,
>
>> If the driver indicates that the watchdog is running, the framework
>> should feed it until userspace opens the device, regardless of whether
>> the driver has set max_hw_heartbeat_ms.
>>
>> This patch only affects the case where wdd->max_hw_heartbeat_ms is
>> zero, wdd->timeout is non-zero, the watchdog is not active and the
>> hardware device is running (*):
>>
>> - If wdd->timeout is zero, watchdog_need_worker() returns false both
>> before and after this patch, and watchdog_next_keepalive() is not
>> called.
>>
>> - If watchdog_active(wdd), the return value from watchdog_need_worker
>> is also the same as before (namely, hm && t > hm). Hence in that case,
>> watchdog_next_keepalive() is only called if hm == max_hw_heartbeat_ms
>> is non-zero, so the change to min_not_zero there is a no-op.
>>
>> - If the watchdog is not active and the device is not running, we
>> return false from watchdog_need_worker just as before.
>>
>> That leaves the watchdog_hw_running(wdd) && !watchdog_active(wdd) &&
>> wdd->timeout case. Again, it's easy to see that if
>> wdd->max_hw_heartbeat_ms is non-zero, we return true from
>> watchdog_need_worker with and without this patch, and the logic in
>> watchdog_next_keepalive is unchanged. Finally, if
>> wdd->max_hw_heartbeat_ms is 0, we used to end up in the
>> cancel_delayed_work branch, whereas with this patch we end up
>> scheduling a ping timeout_ms/2 from now.
>>
>> (*) This should imply that no current kernel drivers are affected,
>> since the only drivers which explicitly set WDOG_HW_RUNNING are
>> imx2_wdt.c and dw_wdt.c, both of which also provide a non-zero value
>> for max_hw_heartbeat_ms. The watchdog core also sets WDOG_HW_RUNNING,
>> but only when the driver doesn't provide ->stop, in which case it
>> must, according to Documentation/watchdog/watchdog-kernel-api.txt, set
>> max_hw_heartbeat_ms.
>
> This isn't completely true. We will have the following in the linux-watchdog tree:
> drivers/watchdog/aspeed_wdt.c:		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> drivers/watchdog/dw_wdt.c:	set_bit(WDOG_HW_RUNNING, &wdd->status);
> drivers/watchdog/dw_wdt.c:		set_bit(WDOG_HW_RUNNING, &wdd->status);
> drivers/watchdog/imx2_wdt.c:	set_bit(WDOG_HW_RUNNING, &wdog->status);
> drivers/watchdog/imx2_wdt.c:		set_bit(WDOG_HW_RUNNING, &wdog->status);
> drivers/watchdog/max77620_wdt.c:		set_bit(WDOG_HW_RUNNING, &wdt_dev->status);
> drivers/watchdog/sbsa_gwdt.c:		set_bit(WDOG_HW_RUNNING, &wdd->status);
> drivers/watchdog/tangox_wdt.c:		set_bit(WDOG_HW_RUNNING, &dev->wdt.status);
>
> I checked the ones that aren't mentioned and aspeed_wdt, max77620_wdt and sbsa_gwdt.c
> also have a non-zero value for max_hw_heartbeat_ms. But tangox_wdt.c doesn't set it.
> This one will need to be looked at closer.
>

I had a brief look; the tangox_wdt problem is my fault. I overlooked that with
my commit 'watchdog: tangox: Mark running watchdog correctly'.

We have a number of options: Set max_hw_heartbeat_ms in tangox_wdt.c,
accept this patch, or both. I think we should accept this patch.

Thanks,
Guenter

>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>   drivers/watchdog/watchdog_dev.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
>> index 3595cff..14f8a92 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -92,9 +92,13 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>>   	 *   thus is aware that the framework supports generating heartbeat
>>   	 *   requests.
>>   	 * - Userspace requests a longer timeout than the hardware can handle.
>> +	 *
>> +	 * Alternatively, if userspace has not opened the watchdog
>> +	 * device, we take care of feeding the watchdog if it is
>> +	 * running.
>>   	 */
>> -	return hm && ((watchdog_active(wdd) && t > hm) ||
>> -		      (t && !watchdog_active(wdd) && watchdog_hw_running(wdd)));
>> +	return (hm && watchdog_active(wdd) && t > hm) ||
>> +		(t && !watchdog_active(wdd) && watchdog_hw_running(wdd));
>>   }
>>
>>   static long watchdog_next_keepalive(struct watchdog_device *wdd)
>> @@ -107,7 +111,7 @@ static long watchdog_next_keepalive(struct watchdog_device *wdd)
>>   	unsigned int hw_heartbeat_ms;
>>
>>   	virt_timeout = wd_data->last_keepalive + msecs_to_jiffies(timeout_ms);
>> -	hw_heartbeat_ms = min(timeout_ms, wdd->max_hw_heartbeat_ms);
>> +	hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms);
>>   	keepalive_interval = msecs_to_jiffies(hw_heartbeat_ms / 2);
>>
>>   	if (!watchdog_active(wdd))
>> --
>> 2.5.0
>>
>
> Kind regards,
> Wim.
>
>

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

#1445118 — Re: [RFC 1/3] watchdog: change watchdog_need_worker logic

Hi Guenter,

> On 07/17/2016 12:24 PM, Wim Van Sebroeck wrote:
> >Hi Rasmus,
> >
> >>If the driver indicates that the watchdog is running, the framework
> >>should feed it until userspace opens the device, regardless of whether
> >>the driver has set max_hw_heartbeat_ms.
> >>
> >>This patch only affects the case where wdd->max_hw_heartbeat_ms is
> >>zero, wdd->timeout is non-zero, the watchdog is not active and the
> >>hardware device is running (*):
> >>
> >>- If wdd->timeout is zero, watchdog_need_worker() returns false both
> >>before and after this patch, and watchdog_next_keepalive() is not
> >>called.
> >>
> >>- If watchdog_active(wdd), the return value from watchdog_need_worker
> >>is also the same as before (namely, hm && t > hm). Hence in that case,
> >>watchdog_next_keepalive() is only called if hm == max_hw_heartbeat_ms
> >>is non-zero, so the change to min_not_zero there is a no-op.
> >>
> >>- If the watchdog is not active and the device is not running, we
> >>return false from watchdog_need_worker just as before.
> >>
> >>That leaves the watchdog_hw_running(wdd) && !watchdog_active(wdd) &&
> >>wdd->timeout case. Again, it's easy to see that if
> >>wdd->max_hw_heartbeat_ms is non-zero, we return true from
> >>watchdog_need_worker with and without this patch, and the logic in
> >>watchdog_next_keepalive is unchanged. Finally, if
> >>wdd->max_hw_heartbeat_ms is 0, we used to end up in the
> >>cancel_delayed_work branch, whereas with this patch we end up
> >>scheduling a ping timeout_ms/2 from now.
> >>
> >>(*) This should imply that no current kernel drivers are affected,
> >>since the only drivers which explicitly set WDOG_HW_RUNNING are
> >>imx2_wdt.c and dw_wdt.c, both of which also provide a non-zero value
> >>for max_hw_heartbeat_ms. The watchdog core also sets WDOG_HW_RUNNING,
> >>but only when the driver doesn't provide ->stop, in which case it
> >>must, according to Documentation/watchdog/watchdog-kernel-api.txt, set
> >>max_hw_heartbeat_ms.
> >
> >This isn't completely true. We will have the following in the 
> >linux-watchdog tree:
> >drivers/watchdog/aspeed_wdt.c:		set_bit(WDOG_HW_RUNNING, 
> >&wdt->wdd.status);
> >drivers/watchdog/dw_wdt.c:	set_bit(WDOG_HW_RUNNING, &wdd->status);
> >drivers/watchdog/dw_wdt.c:		set_bit(WDOG_HW_RUNNING, 
> >&wdd->status);
> >drivers/watchdog/imx2_wdt.c:	set_bit(WDOG_HW_RUNNING, &wdog->status);
> >drivers/watchdog/imx2_wdt.c:		set_bit(WDOG_HW_RUNNING, 
> >&wdog->status);
> >drivers/watchdog/max77620_wdt.c:		set_bit(WDOG_HW_RUNNING, 
> >&wdt_dev->status);
> >drivers/watchdog/sbsa_gwdt.c:		set_bit(WDOG_HW_RUNNING, 
> >&wdd->status);
> >drivers/watchdog/tangox_wdt.c:		set_bit(WDOG_HW_RUNNING, 
> >&dev->wdt.status);
> >
> >I checked the ones that aren't mentioned and aspeed_wdt, max77620_wdt and 
> >sbsa_gwdt.c
> >also have a non-zero value for max_hw_heartbeat_ms. But tangox_wdt.c 
> >doesn't set it.
> >This one will need to be looked at closer.
> >
> 
> I had a brief look; the tangox_wdt problem is my fault. I overlooked that 
> with
> my commit 'watchdog: tangox: Mark running watchdog correctly'.
> 
> We have a number of options: Set max_hw_heartbeat_ms in tangox_wdt.c,
> accept this patch, or both. I think we should accept this patch.

We accept this patch and add a fix for tangox_wdt.c .

> 
> Thanks,
> Guenter
> 
> >>
> >>Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >>---
> >>  drivers/watchdog/watchdog_dev.c | 10 +++++++---
> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/watchdog/watchdog_dev.c 
> >>b/drivers/watchdog/watchdog_dev.c
> >>index 3595cff..14f8a92 100644
> >>--- a/drivers/watchdog/watchdog_dev.c
> >>+++ b/drivers/watchdog/watchdog_dev.c
> >>@@ -92,9 +92,13 @@ static inline bool watchdog_need_worker(struct 
> >>watchdog_device *wdd)
> >>  	 *   thus is aware that the framework supports generating heartbeat
> >>  	 *   requests.
> >>  	 * - Userspace requests a longer timeout than the hardware can 
> >>  	 handle.
> >>+	 *
> >>+	 * Alternatively, if userspace has not opened the watchdog
> >>+	 * device, we take care of feeding the watchdog if it is
> >>+	 * running.
> >>  	 */
> >>-	return hm && ((watchdog_active(wdd) && t > hm) ||
> >>-		      (t && !watchdog_active(wdd) && 
> >>watchdog_hw_running(wdd)));
> >>+	return (hm && watchdog_active(wdd) && t > hm) ||
> >>+		(t && !watchdog_active(wdd) && watchdog_hw_running(wdd));
> >>  }
> >>
> >>  static long watchdog_next_keepalive(struct watchdog_device *wdd)
> >>@@ -107,7 +111,7 @@ static long watchdog_next_keepalive(struct 
> >>watchdog_device *wdd)
> >>  	unsigned int hw_heartbeat_ms;
> >>
> >>  	virt_timeout = wd_data->last_keepalive + 
> >>  	msecs_to_jiffies(timeout_ms);
> >>-	hw_heartbeat_ms = min(timeout_ms, wdd->max_hw_heartbeat_ms);
> >>+	hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms);
> >>  	keepalive_interval = msecs_to_jiffies(hw_heartbeat_ms / 2);
> >>
> >>  	if (!watchdog_active(wdd))
> >>--
> >>2.5.0
> >>
> >
> >Kind regards,
> >Wim.
> >
> >
> 

Kind regards,
Wim.

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

#1443315 — [RFC 3/3] 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.

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.

Setting a timeout of 0 (either on the kernel command line or via
sysfs) makes the system behave as if WATCHDOG_OPEN_DEADLINE=n.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/Kconfig        | 21 +++++++++++++++++++++
 drivers/watchdog/watchdog_dev.c | 39 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index b4b3e25..2674ddf 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -53,6 +53,27 @@ 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, you can set a
+	  maximum time for which the kernel will do this after the
+	  device has been registered.
+
+config WATCHDOG_OPEN_TIMEOUT
+	int "Timeout value for opening watchdog device"
+	depends on WATCHDOG_OPEN_DEADLINE
+	default 120000
+	help
+	  The maximum time, in milliseconds, for which the watchdog
+	  framework takes care of pinging a watchdog device. A value
+	  of 0 means infinite. The value set here can be overridden by
+	  the commandline parameter "watchdog.open_timeout" or through
+	  sysfs.
+
 #
 # General Watchdog drivers
 #
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index da549e5..584c8a5 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -65,6 +65,9 @@ struct watchdog_core_data {
 	struct mutex lock;
 	unsigned long last_keepalive;
 	unsigned long last_hw_keepalive;
+#ifdef CONFIG_WATCHDOG_OPEN_DEADLINE
+	unsigned long open_deadline;
+#endif
 	struct delayed_work work;
 	unsigned long status;		/* Internal status bits */
 #define _WDOG_DEV_OPEN		0	/* Opened ? */
@@ -78,6 +81,32 @@ static struct watchdog_core_data *old_wd_data;
 
 static struct workqueue_struct *watchdog_wq;
 
+#ifdef CONFIG_WATCHDOG_OPEN_DEADLINE
+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)
+{
+	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);
+}
+#else
+static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
+{
+	return false;
+}
+
+static void watchdog_set_open_deadline(struct watchdog_core_data *data)
+{
+}
+#endif
+
 static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 {
 	/* All variables in milli-seconds */
@@ -192,7 +221,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)
@@ -745,6 +780,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 */
@@ -843,6 +879,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.5.0

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

#1443490 — Re: [RFC 3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE

On 07/14/2016 02:16 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.
>
> Setting a timeout of 0 (either on the kernel command line or via
> sysfs) makes the system behave as if WATCHDOG_OPEN_DEADLINE=n.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   drivers/watchdog/Kconfig        | 21 +++++++++++++++++++++
>   drivers/watchdog/watchdog_dev.c | 39 ++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index b4b3e25..2674ddf 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -53,6 +53,27 @@ 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, you can set a
> +	  maximum time for which the kernel will do this after the
> +	  device has been registered.
> +
> +config WATCHDOG_OPEN_TIMEOUT
> +	int "Timeout value for opening watchdog device"
> +	depends on WATCHDOG_OPEN_DEADLINE
> +	default 120000
> +	help
> +	  The maximum time, in milliseconds, for which the watchdog
> +	  framework takes care of pinging a watchdog device. A value
> +	  of 0 means infinite. The value set here can be overridden by
> +	  the commandline parameter "watchdog.open_timeout" or through
> +	  sysfs.
> +

I like the basic idea, and we always thought about implementing it,
though as "initial timeout" (I personally preferred that term).
However, implementing it as configuration option diminishes its
value substantially, since it means that using it in multi-platform
images (such as multi_v7_defconfig) becomes impossible.

The initial timeout should be specified as module option or as
devicetree parameter, and there should be no additional configuration
option.

Where does the module parameter in watchdog_dev.c end up ?

Thanks,
Guenter

>   #
>   # General Watchdog drivers
>   #
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index da549e5..584c8a5 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -65,6 +65,9 @@ struct watchdog_core_data {
>   	struct mutex lock;
>   	unsigned long last_keepalive;
>   	unsigned long last_hw_keepalive;
> +#ifdef CONFIG_WATCHDOG_OPEN_DEADLINE
> +	unsigned long open_deadline;
> +#endif
>   	struct delayed_work work;
>   	unsigned long status;		/* Internal status bits */
>   #define _WDOG_DEV_OPEN		0	/* Opened ? */
> @@ -78,6 +81,32 @@ static struct watchdog_core_data *old_wd_data;
>
>   static struct workqueue_struct *watchdog_wq;
>
> +#ifdef CONFIG_WATCHDOG_OPEN_DEADLINE
> +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)
> +{
> +	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);
> +}
> +#else
> +static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
> +{
> +	return false;
> +}
> +
> +static void watchdog_set_open_deadline(struct watchdog_core_data *data)
> +{
> +}
> +#endif
> +
>   static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>   {
>   	/* All variables in milli-seconds */
> @@ -192,7 +221,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)
> @@ -745,6 +780,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 */
> @@ -843,6 +879,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,
>

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

#1444010 — Re: [RFC 3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE

On 2016-07-14 16:42, Guenter Roeck wrote:
> On 07/14/2016 02:16 AM, Rasmus Villemoes wrote:
>>
>> +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, you can set a
>> +      maximum time for which the kernel will do this after the
>> +      device has been registered.
>> +
>> +config WATCHDOG_OPEN_TIMEOUT
>> +    int "Timeout value for opening watchdog device"
>> +    depends on WATCHDOG_OPEN_DEADLINE
>> +    default 120000
>> +    help
>> +      The maximum time, in milliseconds, for which the watchdog
>> +      framework takes care of pinging a watchdog device. A value
>> +      of 0 means infinite. The value set here can be overridden by
>> +      the commandline parameter "watchdog.open_timeout" or through
>> +      sysfs.
>> +
>
> I like the basic idea, and we always thought about implementing it,
> though as "initial timeout" (I personally preferred that term).

I also used WATCHDOG_INIT_TIMEOUT in my first few drafts, and my helper 
watchdog_set_open_deadline was called watchdog_set_init_timeout. But 
then I stumbled on watchdog_init_timeout in watchdog_core.c, and thought 
that might end up being quite confusing. I think having 'open' part of 
the name is quite natural, but I don't really have strong feelings about 
the naming of this thing.

> However, implementing it as configuration option diminishes its
> value substantially, since it means that using it in multi-platform
> images (such as multi_v7_defconfig) becomes impossible.

If one wants to allow this feature in an existing _defconfig, one can 
set OPEN_DEADLINE=y and OPEN_TIMEOUT=0; we could change the default for 
the latter to that. (I thought about just having that single config 
option with a default of 0, but it wasn't much more code to allow this 
thing to be compiled out completely.) Platforms without a running 
watchdog won't be affected, and for those with, having a non-zero 
deadline requires opt-in anyway.

> The initial timeout should be specified as module option or as
> devicetree parameter, and there should be no additional configuration
> option.

I was under the impression that device tree was exclusively for 
describing hardware, and this certainly is not that. I also wanted to 
avoid having to modify each driver, which would seem to be necessary if 
it was module parameter/DT - the only thing required of a driver now is 
that it correctly reports WDOG_HW_RUNNING.

Regardless of implementation, it's not something that any "distro" 
kernel is going to enable (with non-zero deadline), so to use it will 
require some customization - which could be a .config tweak, passing a 
command line parameter, a custom dtb, and probably other options. ISTM 
that the first two are the most generic and require the least repeated 
work across platforms.

> Where does the module parameter in watchdog_dev.c end up ?

# cat /sys/module/watchdog/parameters/open_timeout
120000

Rasmus

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

#1444343 — Re: [RFC 3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE

On 07/15/2016 12:32 AM, Rasmus Villemoes wrote:
> On 2016-07-14 16:42, Guenter Roeck wrote:
>> On 07/14/2016 02:16 AM, Rasmus Villemoes wrote:
>>>
>>> +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, you can set a
>>> +      maximum time for which the kernel will do this after the
>>> +      device has been registered.
>>> +
>>> +config WATCHDOG_OPEN_TIMEOUT
>>> +    int "Timeout value for opening watchdog device"
>>> +    depends on WATCHDOG_OPEN_DEADLINE
>>> +    default 120000
>>> +    help
>>> +      The maximum time, in milliseconds, for which the watchdog
>>> +      framework takes care of pinging a watchdog device. A value
>>> +      of 0 means infinite. The value set here can be overridden by
>>> +      the commandline parameter "watchdog.open_timeout" or through
>>> +      sysfs.
>>> +
>>
>> I like the basic idea, and we always thought about implementing it,
>> though as "initial timeout" (I personally preferred that term).
>
> I also used WATCHDOG_INIT_TIMEOUT in my first few drafts, and my helper watchdog_set_open_deadline was called watchdog_set_init_timeout. But then I stumbled on watchdog_init_timeout in watchdog_core.c, and thought that might end up being quite confusing. I think having 'open' part of the name is quite natural, but I don't really have strong feelings about the naming of this thing.
>
You could use "INITIAL".

>> However, implementing it as configuration option diminishes its
>> value substantially, since it means that using it in multi-platform
>> images (such as multi_v7_defconfig) becomes impossible.
>
> If one wants to allow this feature in an existing _defconfig, one can set OPEN_DEADLINE=y and OPEN_TIMEOUT=0; we could change the default for the latter to that. (I thought about just having that single config option with a default of 0, but it wasn't much more code to allow this thing to be compiled out completely.) Platforms without a running watchdog won't be affected, and for those with, having a non-zero deadline requires opt-in anyway.
>

Ok, off to Wim to decide then. I don't see the value of WATCHDOG_OPEN_DEADLINE,
and for sure don't like the WATCHDOG_OPEN_TIMEOUT parameter.

>> The initial timeout should be specified as module option or as
>> devicetree parameter, and there should be no additional configuration
>> option.
>
> I was under the impression that device tree was exclusively for describing hardware, and this certainly is not that. I also wanted to avoid having to modify each driver, which would seem to be necessary if it was module parameter/DT - the only thing required of a driver now is that it correctly reports WDOG_HW_RUNNING.

What is "hardware" ? It is supposed to describe the system, isn't it ? Part of that system is its clock rate,
and the means how the OS is loaded, and both have impact on the initial timeout (and the regular timeout).

You might as well argue that clock rates should not be in devicetree either. Clock rates are, after all,
just reflecting the _use_ of the hardware, not the hardware itself.

Devicetree could be handled in the core, with a function to set the initial timeout,
or possibly even with the watchdog registration itself.

>
> Regardless of implementation, it's not something that any "distro" kernel is going to enable (with non-zero deadline), so to use it will require some customization - which could be a .config tweak, passing a command line parameter, a custom dtb, and probably other options. ISTM that the first two are the most generic and require the least repeated work across platforms.
>

Each system running a specific kernel will require its own dtb, so I don't see your point there.

Guenter

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

#1447506 — Re: [RFC 3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE

On 2016-07-15 16:29, Guenter Roeck wrote:
> On 07/15/2016 12:32 AM, Rasmus Villemoes wrote:
>
>>> The initial timeout should be specified as module option or as
>>> devicetree parameter, and there should be no additional configuration
>>> option.
>>
>> I was under the impression that device tree was exclusively for
>> describing hardware, and this certainly is not that. I also wanted to
>> avoid having to modify each driver, which would seem to be necessary
>> if it was module parameter/DT - the only thing required of a driver
>> now is that it correctly reports WDOG_HW_RUNNING.
>
> What is "hardware" ? It is supposed to describe the system, isn't it ?
> Part of that system is its clock rate,
> and the means how the OS is loaded, and both have impact on the initial
> timeout (and the regular timeout).
>
> You might as well argue that clock rates should not be in devicetree
> either. Clock rates are, after all,
> just reflecting the _use_ of the hardware, not the hardware itself.

But they are used to configure hardware. The init timeout is not a 
property of any particular device - it configures how the kernel 
behaves, and as such I find it quite natural to have it in the kernel's 
.config (and overridable on command line and via sysfs).

> Devicetree could be handled in the core, with a function to set the
> initial timeout,
> or possibly even with the watchdog registration itself.

But where in the device tree would you put this value? I'd really prefer 
not having to modify the node representing each individual watchdog 
device I might use.

Rasmus

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

#1447550 — Re: [RFC 3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_DEADLINE

On Thu, Jul 21, 2016 at 12:08:52AM +0200, Rasmus Villemoes wrote:
> On 2016-07-15 16:29, Guenter Roeck wrote:
> >On 07/15/2016 12:32 AM, Rasmus Villemoes wrote:
> >
> >>>The initial timeout should be specified as module option or as
> >>>devicetree parameter, and there should be no additional configuration
> >>>option.
> >>
> >>I was under the impression that device tree was exclusively for
> >>describing hardware, and this certainly is not that. I also wanted to
> >>avoid having to modify each driver, which would seem to be necessary
> >>if it was module parameter/DT - the only thing required of a driver
> >>now is that it correctly reports WDOG_HW_RUNNING.
> >
> >What is "hardware" ? It is supposed to describe the system, isn't it ?
> >Part of that system is its clock rate,
> >and the means how the OS is loaded, and both have impact on the initial
> >timeout (and the regular timeout).
> >
> >You might as well argue that clock rates should not be in devicetree
> >either. Clock rates are, after all,
> >just reflecting the _use_ of the hardware, not the hardware itself.
> 
> But they are used to configure hardware. The init timeout is not a property
> of any particular device - it configures how the kernel behaves, and as such
> I find it quite natural to have it in the kernel's .config (and overridable
> on command line and via sysfs).
> 

I hear you. "configure hardware" is a slippery term, though. After all,
one would typically configure the initial timeout in hardware, just as
any "normal" timeout. In many cases, this will actually already be the
case (and should be), since the watchdog should be enabled by the
ROMMON or BIOS before control is passed to the kernel. As such, the
initial timeout should already be set when the driver is loaded.

Also, I would want to be able to use the same kernel, and the same
root file system, on different machines without having to bother about
system variants, even more so if I was responsible for potentially dozens
of different variants with subtle differences in hardware. One ends
up having to maintain configuration files which happen to closely look like
devicetree files, plus an entire infrastructure to detect and configure
hardware variants. Such configuration files may be part of the root file
system, or have to be maintained separately. Either creates additional
overhead.

Unfortunately, those configuration files can only be read after the kernel
passed control to user space, which typically happens after the driver
to be controlled was already loaded. Using sysfs is therefore pretty much
useless - one might as well start the watchdog daemon as early as possible
after passing control to user space.

This leaves module parameters, which have to be passed on the kernel command
line to be useful. This is not really desirable either in most situations,
since now the system variant specific configuration has to be implemented
somewhere in the ROMMON, BIOS, or bootloader configuration file. Another level
of complexity added, since the per-variant boot parameters have to be managed
somewhere. Plus, as mentioned above, if the initial timeout has to be passed
as parameter to the kernel, the passing entity might as well program it
into the hardware directly, and start the watchdog, thereby fixing the gap
between hand-off to kernel and loading the watchdog driver.

So what is left ? Situations where the hardware does not tell software what
its configured timeout is, and situations where the maximum hardware timeout
is smaller than the required initial timeout. Both would, in my opinion, warrant
the use of a devicetree property, but I understand that others may have a
different opinion.

Overall, my conclusion is that if a devicetree property is not acceptable
for some reason, we should drop the notion of supporting an initial or
"open" timeout entirely, and leave it up to the BIOS/ROMMON as well as the
respective watchdog driver to set an acceptable default or initial timeout.
This initial timeout can (and will) be overwritten with the desired runtime
timeout by the watchdog daemon after it opened the watchdog device.

> >Devicetree could be handled in the core, with a function to set the
> >initial timeout,
> >or possibly even with the watchdog registration itself.
> 
> But where in the device tree would you put this value? I'd really prefer not
> having to modify the node representing each individual watchdog device I
> might use.
> 
The existing "timeout-sec" property is in the watchdog node as well.

Thanks,
Guenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web