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

[Tero Kristo <t-kristo@ti.com>]

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