Re: [PATCH v7 4/9] watchdog: Add support for minimum time between heartbeats

[Uwe Kleine-König]

Hello Guenter,

On Mon, Jan 25, 2016 at 06:53:11PM -0800, Guenter Roeck wrote:
> From: Guenter Roeck <linux@xxxxxxxxxxxx>
> 
> Some watchdogs require a minimum time between heartbeats.
> Examples are the watchdogs in DA9062 and AT91SAM9x.
> 
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
> v7: Rebased to v4.5-rc1
> v6: Rebased to v4.4-rc2
> v5: Rebased to v4.4-rc1
>     Fixed typo in documentation.
> v4: Added patch
> ---
>  Documentation/watchdog/watchdog-kernel-api.txt |  3 +++
>  drivers/watchdog/watchdog_dev.c                | 15 +++++++++++++++
>  include/linux/watchdog.h                       |  3 +++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index 62d49d6a7ff5..2221fb4f2739 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -52,6 +52,7 @@ struct watchdog_device {
>  	unsigned int timeout;
>  	unsigned int min_timeout;
>  	unsigned int max_timeout;
> +	unsigned int min_hw_heartbeat_ms;
>  	unsigned int max_hw_timeout_ms;
>  	struct notifier_block reboot_nb;
>  	struct notifier_block restart_nb;
> @@ -81,6 +82,8 @@ It contains following fields:
>  * max_timeout: the watchdog timer's maximum timeout value (in seconds),
>    as seen from userspace. If set, the maximum configurable value for
>    'timeout'. Not used if max_hw_timeout_ms is non-zero.
> +* min_hw_heartbeat_ms: Minimum time between heartbeats sent to the chip,
> +  in milli-seconds.
>  * max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds.
>    If set, the infrastructure will send heartbeats to the watchdog driver
>    if 'timeout' is larger than max_hw_timeout_ms, unless WDOG_ACTIVE
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 1a8f3861fe92..ff03cbc8e081 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -64,6 +64,7 @@ struct watchdog_core_data {
>  	struct watchdog_device *wdd;
>  	struct mutex lock;
>  	unsigned long last_keepalive;
> +	unsigned long last_hw_keepalive;
>  	struct delayed_work work;
>  	unsigned long status;		/* Internal status bits */
>  #define _WDOG_DEV_OPEN		0	/* Opened ? */
> @@ -138,8 +139,19 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd,
>  
>  static int __watchdog_ping(struct watchdog_device *wdd)
>  {
> +	struct watchdog_core_data *wd_data = wdd->wd_data;
> +	unsigned long earliest_keepalive = wd_data->last_hw_keepalive +
> +				msecs_to_jiffies(wdd->min_hw_heartbeat_ms);
>  	int err;
>  
> +	if (time_is_after_jiffies(earliest_keepalive)) {
> +		mod_delayed_work(watchdog_wq, &wd_data->work,
> +				 earliest_keepalive - jiffies);
> +		return 0;
> +	}
> +
> +	wd_data->last_hw_keepalive = jiffies;
> +

Do you need to undo this assignment when ping fails?

>  	if (wdd->ops->ping)
>  		err = wdd->ops->ping(wdd);  /* ping the watchdog */
>  	else

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |