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

From: Guenter Roeck <linux@roeck-us.net>
To: Tero Kristo <t-kristo@ti.com>,
	wim@linux-watchdog.org, linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, jan.kiszka@siemens.com
Subject: Re: [PATCHv2 2/5] watchdog: add support for adjusting last known HW keepalive time
Date: Sun, 5 Jul 2020 07:58:02 -0700	[thread overview]
Message-ID: <faeebfbb-cb8c-e31e-50ef-af42e1b11e0e@roeck-us.net> (raw)
In-Reply-To: <20200703120406.7092-3-t-kristo@ti.com>

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 *);
>  
> 