[PATCHv2,2/5] watchdog: add support for adjusting last known HW keepalive time

Message ID 20200703120406.7092-3-t-kristo@ti.com
State New
Headers show
Series
  • [PATCHv2,1/5] watchdog: use __watchdog_ping in startup
Related show

Commit Message

Tero Kristo July 3, 2020, 12:04 p.m.
Certain watchdogs require the watchdog only to be pinged within a
specific time window, pinging too early or too late cause the watchdog
to fire. In cases where this sort of watchdog has been started before
kernel comes up, we must adjust the watchdog keepalive window to match
the actually running timer, so add a new driver API for this purpose.

Signed-off-by: Tero Kristo <t-kristo@ti.com>

---
 drivers/watchdog/watchdog_dev.c | 23 +++++++++++++++++++++++
 include/linux/watchdog.h        |  2 ++
 2 files changed, 25 insertions(+)

-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Comments

Guenter Roeck July 5, 2020, 2:58 p.m. | #1
On 7/3/20 5:04 AM, Tero Kristo wrote:
> Certain watchdogs require the watchdog only to be pinged within a

> specific time window, pinging too early or too late cause the watchdog

> to fire. In cases where this sort of watchdog has been started before

> kernel comes up, we must adjust the watchdog keepalive window to match

> the actually running timer, so add a new driver API for this purpose.

> 

> Signed-off-by: Tero Kristo <t-kristo@ti.com>

> ---

>  drivers/watchdog/watchdog_dev.c | 23 +++++++++++++++++++++++

>  include/linux/watchdog.h        |  2 ++

>  2 files changed, 25 insertions(+)

> 

> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c

> index bc1cfa288553..5848551cf29d 100644

> --- a/drivers/watchdog/watchdog_dev.c

> +++ b/drivers/watchdog/watchdog_dev.c

> @@ -1138,6 +1138,29 @@ void watchdog_dev_unregister(struct watchdog_device *wdd)

>  	watchdog_cdev_unregister(wdd);

>  }

>  

> +/*

> + *	watchdog_set_last_hw_keepalive: set last HW keepalive time for watchdog

> + *

> + *	Adjusts the last known HW keepalive time for a watchdog timer.

> + *	This is needed in case where watchdog has been started before

> + *	kernel by someone like bootloader, and it can't be pinged


... needed if the watchdog is already running when the probe function
is called, and ...

> + *	immediately. This adjusts the watchdog ping period to match

> + *	the currently running timer.


It doesn't adjust the ping period.

> + */


last_ping_ms needs to be documented (the last heartbeat was last_ping_ms
milliseconds ago ?), both here and in Documentation/watchdog/watchdog-kernel-api.rst.
It needs to be documented that the function must be called immediately
after watchdog registration, and that min_hw_heartbeat_ms must
be set for it to be useful.

> +int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,

> +				   unsigned int last_ping_ms)

> +{

> +	struct watchdog_core_data *wd_data = wdd->wd_data;


This needs a NULL check, in case it is called before watchdog driver
registration.

> +	ktime_t now;

> +

> +	now = ktime_get();

> +

> +	wd_data->last_hw_keepalive = ktime_sub(now, ms_to_ktime(last_ping_ms));

> +

> +	return __watchdog_ping(wdd);

> +}

> +EXPORT_SYMBOL_GPL(watchdog_set_last_hw_keepalive);

> +

>  /*

>   *	watchdog_dev_init: init dev part of watchdog core

>   *

> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h

> index 1464ce6ffa31..9b19e6bb68b5 100644

> --- a/include/linux/watchdog.h

> +++ b/include/linux/watchdog.h

> @@ -210,6 +210,8 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd,

>  extern int watchdog_register_device(struct watchdog_device *);

>  extern void watchdog_unregister_device(struct watchdog_device *);

>  

> +int watchdog_set_last_hw_keepalive(struct watchdog_device *, unsigned int);

> +

>  /* devres register variant */

>  int devm_watchdog_register_device(struct device *dev, struct watchdog_device *);

>  

>
Tero Kristo July 13, 2020, 12:45 p.m. | #2
On 05/07/2020 17:58, Guenter Roeck wrote:
> On 7/3/20 5:04 AM, Tero Kristo wrote:

>> Certain watchdogs require the watchdog only to be pinged within a

>> specific time window, pinging too early or too late cause the watchdog

>> to fire. In cases where this sort of watchdog has been started before

>> kernel comes up, we must adjust the watchdog keepalive window to match

>> the actually running timer, so add a new driver API for this purpose.

>>

>> Signed-off-by: Tero Kristo <t-kristo@ti.com>

>> ---

>>   drivers/watchdog/watchdog_dev.c | 23 +++++++++++++++++++++++

>>   include/linux/watchdog.h        |  2 ++

>>   2 files changed, 25 insertions(+)

>>

>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c

>> index bc1cfa288553..5848551cf29d 100644

>> --- a/drivers/watchdog/watchdog_dev.c

>> +++ b/drivers/watchdog/watchdog_dev.c

>> @@ -1138,6 +1138,29 @@ void watchdog_dev_unregister(struct watchdog_device *wdd)

>>   	watchdog_cdev_unregister(wdd);

>>   }

>>   

>> +/*

>> + *	watchdog_set_last_hw_keepalive: set last HW keepalive time for watchdog

>> + *

>> + *	Adjusts the last known HW keepalive time for a watchdog timer.

>> + *	This is needed in case where watchdog has been started before

>> + *	kernel by someone like bootloader, and it can't be pinged

> 

> ... needed if the watchdog is already running when the probe function

> is called, and ...

> 

>> + *	immediately. This adjusts the watchdog ping period to match

>> + *	the currently running timer.

> 

> It doesn't adjust the ping period.

> 

>> + */

> 

> last_ping_ms needs to be documented (the last heartbeat was last_ping_ms

> milliseconds ago ?), both here and in Documentation/watchdog/watchdog-kernel-api.rst.

> It needs to be documented that the function must be called immediately

> after watchdog registration, and that min_hw_heartbeat_ms must

> be set for it to be useful.

> 

>> +int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,

>> +				   unsigned int last_ping_ms)

>> +{

>> +	struct watchdog_core_data *wd_data = wdd->wd_data;

> 

> This needs a NULL check, in case it is called before watchdog driver

> registration.


Ok will fix all the above in next revision.

-Tero

> 

>> +	ktime_t now;

>> +

>> +	now = ktime_get();

>> +

>> +	wd_data->last_hw_keepalive = ktime_sub(now, ms_to_ktime(last_ping_ms));

>> +

>> +	return __watchdog_ping(wdd);

>> +}

>> +EXPORT_SYMBOL_GPL(watchdog_set_last_hw_keepalive);

>> +

>>   /*

>>    *	watchdog_dev_init: init dev part of watchdog core

>>    *

>> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h

>> index 1464ce6ffa31..9b19e6bb68b5 100644

>> --- a/include/linux/watchdog.h

>> +++ b/include/linux/watchdog.h

>> @@ -210,6 +210,8 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd,

>>   extern int watchdog_register_device(struct watchdog_device *);

>>   extern void watchdog_unregister_device(struct watchdog_device *);

>>   

>> +int watchdog_set_last_hw_keepalive(struct watchdog_device *, unsigned int);

>> +

>>   /* devres register variant */

>>   int devm_watchdog_register_device(struct device *dev, struct watchdog_device *);

>>   

>>

> 


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Patch

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index bc1cfa288553..5848551cf29d 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -1138,6 +1138,29 @@  void watchdog_dev_unregister(struct watchdog_device *wdd)
 	watchdog_cdev_unregister(wdd);
 }
 
+/*
+ *	watchdog_set_last_hw_keepalive: set last HW keepalive time for watchdog
+ *
+ *	Adjusts the last known HW keepalive time for a watchdog timer.
+ *	This is needed in case where watchdog has been started before
+ *	kernel by someone like bootloader, and it can't be pinged
+ *	immediately. This adjusts the watchdog ping period to match
+ *	the currently running timer.
+ */
+int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,
+				   unsigned int last_ping_ms)
+{
+	struct watchdog_core_data *wd_data = wdd->wd_data;
+	ktime_t now;
+
+	now = ktime_get();
+
+	wd_data->last_hw_keepalive = ktime_sub(now, ms_to_ktime(last_ping_ms));
+
+	return __watchdog_ping(wdd);
+}
+EXPORT_SYMBOL_GPL(watchdog_set_last_hw_keepalive);
+
 /*
  *	watchdog_dev_init: init dev part of watchdog core
  *
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 1464ce6ffa31..9b19e6bb68b5 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -210,6 +210,8 @@  extern int watchdog_init_timeout(struct watchdog_device *wdd,
 extern int watchdog_register_device(struct watchdog_device *);
 extern void watchdog_unregister_device(struct watchdog_device *);
 
+int watchdog_set_last_hw_keepalive(struct watchdog_device *, unsigned int);
+
 /* devres register variant */
 int devm_watchdog_register_device(struct device *dev, struct watchdog_device *);