linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Curtis Klein <curtis.klein@hpe.com>
Cc: wim@linux-watchdog.org, linux-watchdog@vger.kernel.org
Subject: Re: [PATCH v3] watchdog: Add hrtimer-based pretimeout feature
Date: Wed, 3 Feb 2021 17:35:06 -0800	[thread overview]
Message-ID: <20210204013506.GA87215@roeck-us.net> (raw)
In-Reply-To: <1612383090-27110-1-git-send-email-curtis.klein@hpe.com>

On Wed, Feb 03, 2021 at 12:11:30PM -0800, Curtis Klein wrote:
> This adds the option to use a hrtimer to generate a watchdog pretimeout
> event for hardware watchdogs that do not natively support watchdog
> pretimeouts.
> 
> With this enabled, all watchdogs will appear to have pretimeout support
> in userspace. If no pretimeout value is set, there will be no change in
> the watchdog's behavior. If a pretimeout value is set for a specific
> watchdog that does not have built-in pretimeout support, a timer will be
> started that should fire at the specified time before the watchdog
> timeout would occur. When the watchdog is successfully pinged, the timer
> will be restarted. If the timer is allowed to fire it will generate a
> pretimeout event. However because a software timer is used, it may not
> be able to fire in every circumstance.
> 
> If the watchdog does support a pretimeout natively, that functionality
> will be used instead of the hrtimer.
> 
> The general design of this feaure was inspired by the software watchdog,
> specifically its own pretimeout implementation. However the software
> watchdog and this feature are completely independent. They can be used
> together; with or without CONFIG_SOFT_WATCHDOG_PRETIMEOUT enabled.
> 
> The main advantage of using the hrtimer pretimeout with a hardware
> watchdog, compared to running the software watchdog with a hardware
> watchdog, is that if the hardware watchdog driver is unable to ping the
> watchdog (e.g. due to a bus or communication error), then the hrtimer
> pretimeout would still fire whereas the software watchdog would not.
> 
> Signed-off-by: Curtis Klein <curtis.klein@hpe.com>

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

> ---
> Changes from v2
>  - Added required includes to watchdog_core.h
>  - Added watchdog_have_pretimeout function and use throughout
>  - Moved function declarations to watchdog_core.h and made stubs static
> 
> Changes from v1 (watchdog: Add software pretimeout support)
>  - Changed subject for clarity
>  - Renamed KCONFIG to WATCHDOG_HRTIMER_PRETIMEOUT also for clarity
>  - Moved init/start/stop logic to watchdog_hrtimer_pretimeout.c
>  - Moved watchdog_core_data struct to watchdog_core.h so it can be
>     used in watchdog_hrtimer_pretimeout.c and watchdog_core.c
> 
>  drivers/watchdog/Kconfig                       |  8 +++++
>  drivers/watchdog/Makefile                      |  1 +
>  drivers/watchdog/watchdog_core.h               | 48 ++++++++++++++++++++++++++
>  drivers/watchdog/watchdog_dev.c                | 47 +++++++++----------------
>  drivers/watchdog/watchdog_hrtimer_pretimeout.c | 44 +++++++++++++++++++++++
>  drivers/watchdog/watchdog_pretimeout.c         |  5 +--
>  6 files changed, 121 insertions(+), 32 deletions(-)
>  create mode 100644 drivers/watchdog/watchdog_hrtimer_pretimeout.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7ff941e..a5f0ca8 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -73,6 +73,14 @@ config WATCHDOG_SYSFS
>  	  Say Y here if you want to enable watchdog device status read through
>  	  sysfs attributes.
>  
> +config WATCHDOG_HRTIMER_PRETIMEOUT
> +	bool "Enable watchdog hrtimer-based pretimeouts"
> +	help
> +	  Enable this if you want to use a hrtimer timer based pretimeout for
> +	  watchdogs that do not natively support pretimeout support. Be aware
> +	  that because this pretimeout functionality uses hrtimers, it may not
> +	  be able to fire before the actual watchdog fires in some situations.
> +
>  comment "Watchdog Pretimeout Governors"
>  
>  config WATCHDOG_PRETIMEOUT_GOV
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 5c74ee1..6fecaab 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_WATCHDOG_CORE)	+= watchdog.o
>  watchdog-objs	+= watchdog_core.o watchdog_dev.o
>  
>  watchdog-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV)	+= watchdog_pretimeout.o
> +watchdog-$(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT)	+= watchdog_hrtimer_pretimeout.o
>  
>  obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_NOOP)	+= pretimeout_noop.o
>  obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_PANIC)	+= pretimeout_panic.o
> diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
> index a5062e8..5b35a84 100644
> --- a/drivers/watchdog/watchdog_core.h
> +++ b/drivers/watchdog/watchdog_core.h
> @@ -7,6 +7,8 @@
>   *
>   *	(c) Copyright 2008-2011 Wim Van Sebroeck <wim@iguana.be>.
>   *
> + *	(c) Copyright 2021 Hewlett Packard Enterprise Development LP.
> + *
>   *	This source code is part of the generic code that can be used
>   *	by all the watchdog timer drivers.
>   *
> @@ -22,12 +24,58 @@
>   *	This material is provided "AS-IS" and at no charge.
>   */
>  
> +#include <linux/hrtimer.h>
> +#include <linux/kthread.h>
> +
>  #define MAX_DOGS	32	/* Maximum number of watchdog devices */
>  
>  /*
> + * struct watchdog_core_data - watchdog core internal data
> + * @dev:	The watchdog's internal device
> + * @cdev:	The watchdog's Character device.
> + * @wdd:	Pointer to watchdog device.
> + * @lock:	Lock for watchdog core.
> + * @status:	Watchdog core internal status bits.
> + */
> +struct watchdog_core_data {
> +	struct device dev;
> +	struct cdev cdev;
> +	struct watchdog_device *wdd;
> +	struct mutex lock;
> +	ktime_t last_keepalive;
> +	ktime_t last_hw_keepalive;
> +	ktime_t open_deadline;
> +	struct hrtimer timer;
> +	struct kthread_work work;
> +#if IS_ENABLED(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT)
> +	struct hrtimer pretimeout_timer;
> +#endif
> +	unsigned long status;		/* Internal status bits */
> +#define _WDOG_DEV_OPEN		0	/* Opened ? */
> +#define _WDOG_ALLOW_RELEASE	1	/* Did we receive the magic char ? */
> +#define _WDOG_KEEPALIVE		2	/* Did we receive a keepalive ? */
> +};
> +
> +/*
>   *	Functions/procedures to be called by the core
>   */
>  extern int watchdog_dev_register(struct watchdog_device *);
>  extern void watchdog_dev_unregister(struct watchdog_device *);
>  extern int __init watchdog_dev_init(void);
>  extern void __exit watchdog_dev_exit(void);
> +
> +static inline bool watchdog_have_pretimeout(struct watchdog_device *wdd)
> +{
> +	return wdd->info->options & WDIOF_PRETIMEOUT ||
> +	       IS_ENABLED(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT);
> +}
> +
> +#if IS_ENABLED(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT)
> +void watchdog_hrtimer_pretimeout_init(struct watchdog_device *wdd);
> +void watchdog_hrtimer_pretimeout_start(struct watchdog_device *wdd);
> +void watchdog_hrtimer_pretimeout_stop(struct watchdog_device *wdd);
> +#else
> +static inline void watchdog_hrtimer_pretimeout_init(struct watchdog_device *wdd) {}
> +static inline void watchdog_hrtimer_pretimeout_start(struct watchdog_device *wdd) {}
> +static inline void watchdog_hrtimer_pretimeout_stop(struct watchdog_device *wdd) {}
> +#endif
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 2946f3a..3eb3814 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -7,6 +7,7 @@
>   *
>   *	(c) Copyright 2008-2011 Wim Van Sebroeck <wim@iguana.be>.
>   *
> + *	(c) Copyright 2021 Hewlett Packard Enterprise Development LP.
>   *
>   *	This source code is part of the generic code that can be used
>   *	by all the watchdog timer drivers.
> @@ -46,30 +47,6 @@
>  #include "watchdog_core.h"
>  #include "watchdog_pretimeout.h"
>  
> -/*
> - * struct watchdog_core_data - watchdog core internal data
> - * @dev:	The watchdog's internal device
> - * @cdev:	The watchdog's Character device.
> - * @wdd:	Pointer to watchdog device.
> - * @lock:	Lock for watchdog core.
> - * @status:	Watchdog core internal status bits.
> - */
> -struct watchdog_core_data {
> -	struct device dev;
> -	struct cdev cdev;
> -	struct watchdog_device *wdd;
> -	struct mutex lock;
> -	ktime_t last_keepalive;
> -	ktime_t last_hw_keepalive;
> -	ktime_t open_deadline;
> -	struct hrtimer timer;
> -	struct kthread_work work;
> -	unsigned long status;		/* Internal status bits */
> -#define _WDOG_DEV_OPEN		0	/* Opened ? */
> -#define _WDOG_ALLOW_RELEASE	1	/* Did we receive the magic char ? */
> -#define _WDOG_KEEPALIVE		2	/* Did we receive a keepalive ? */
> -};
> -
>  /* the dev_t structure to store the dynamically allocated watchdog devices */
>  static dev_t watchdog_devt;
>  /* Reference to watchdog device behind /dev/watchdog */
> @@ -185,6 +162,9 @@ static int __watchdog_ping(struct watchdog_device *wdd)
>  	else
>  		err = wdd->ops->start(wdd); /* restart watchdog */
>  
> +	if (err == 0)
> +		watchdog_hrtimer_pretimeout_start(wdd);
> +
>  	watchdog_update_worker(wdd);
>  
>  	return err;
> @@ -275,8 +255,10 @@ static int watchdog_start(struct watchdog_device *wdd)
>  	started_at = ktime_get();
>  	if (watchdog_hw_running(wdd) && wdd->ops->ping) {
>  		err = __watchdog_ping(wdd);
> -		if (err == 0)
> +		if (err == 0) {
>  			set_bit(WDOG_ACTIVE, &wdd->status);
> +			watchdog_hrtimer_pretimeout_start(wdd);
> +		}
>  	} else {
>  		err = wdd->ops->start(wdd);
>  		if (err == 0) {
> @@ -284,6 +266,7 @@ static int watchdog_start(struct watchdog_device *wdd)
>  			wd_data->last_keepalive = started_at;
>  			wd_data->last_hw_keepalive = started_at;
>  			watchdog_update_worker(wdd);
> +			watchdog_hrtimer_pretimeout_start(wdd);
>  		}
>  	}
>  
> @@ -325,6 +308,7 @@ static int watchdog_stop(struct watchdog_device *wdd)
>  	if (err == 0) {
>  		clear_bit(WDOG_ACTIVE, &wdd->status);
>  		watchdog_update_worker(wdd);
> +		watchdog_hrtimer_pretimeout_stop(wdd);
>  	}
>  
>  	return err;
> @@ -361,6 +345,9 @@ static unsigned int watchdog_get_status(struct watchdog_device *wdd)
>  	if (test_and_clear_bit(_WDOG_KEEPALIVE, &wd_data->status))
>  		status |= WDIOF_KEEPALIVEPING;
>  
> +	if (IS_ENABLED(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT))
> +		status |= WDIOF_PRETIMEOUT;
> +
>  	return status;
>  }
>  
> @@ -408,7 +395,7 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
>  {
>  	int err = 0;
>  
> -	if (!(wdd->info->options & WDIOF_PRETIMEOUT))
> +	if (!watchdog_have_pretimeout(wdd))
>  		return -EOPNOTSUPP;
>  
>  	if (watchdog_pretimeout_invalid(wdd, timeout))
> @@ -594,13 +581,11 @@ static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
>  
>  	if (attr == &dev_attr_timeleft.attr && !wdd->ops->get_timeleft)
>  		mode = 0;
> -	else if (attr == &dev_attr_pretimeout.attr &&
> -		 !(wdd->info->options & WDIOF_PRETIMEOUT))
> +	else if (attr == &dev_attr_pretimeout.attr && !watchdog_have_pretimeout(wdd))
>  		mode = 0;
>  	else if ((attr == &dev_attr_pretimeout_governor.attr ||
>  		  attr == &dev_attr_pretimeout_available_governors.attr) &&
> -		 (!(wdd->info->options & WDIOF_PRETIMEOUT) ||
> -		  !IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV)))
> +		 (!watchdog_have_pretimeout(wdd) || !IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV)))
>  		mode = 0;
>  
>  	return mode;
> @@ -1009,6 +994,7 @@ static int watchdog_cdev_register(struct watchdog_device *wdd)
>  	kthread_init_work(&wd_data->work, watchdog_ping_work);
>  	hrtimer_init(&wd_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_HARD);
>  	wd_data->timer.function = watchdog_timer_expired;
> +	watchdog_hrtimer_pretimeout_init(wdd);
>  
>  	if (wdd->id == 0) {
>  		old_wd_data = wd_data;
> @@ -1096,6 +1082,7 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
>  
>  	hrtimer_cancel(&wd_data->timer);
>  	kthread_cancel_work_sync(&wd_data->work);
> +	watchdog_hrtimer_pretimeout_stop(wdd);
>  
>  	put_device(&wd_data->dev);
>  }
> diff --git a/drivers/watchdog/watchdog_hrtimer_pretimeout.c b/drivers/watchdog/watchdog_hrtimer_pretimeout.c
> new file mode 100644
> index 00000000..940b537
> --- /dev/null
> +++ b/drivers/watchdog/watchdog_hrtimer_pretimeout.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * (c) Copyright 2021 Hewlett Packard Enterprise Development LP.
> + */
> +
> +#include <linux/hrtimer.h>
> +#include <linux/watchdog.h>
> +
> +#include "watchdog_core.h"
> +#include "watchdog_pretimeout.h"
> +
> +static enum hrtimer_restart watchdog_hrtimer_pretimeout(struct hrtimer *timer)
> +{
> +	struct watchdog_core_data *wd_data;
> +
> +	wd_data = container_of(timer, struct watchdog_core_data, pretimeout_timer);
> +
> +	watchdog_notify_pretimeout(wd_data->wdd);
> +	return HRTIMER_NORESTART;
> +}
> +
> +void watchdog_hrtimer_pretimeout_init(struct watchdog_device *wdd)
> +{
> +	struct watchdog_core_data *wd_data = wdd->wd_data;
> +
> +	hrtimer_init(&wd_data->pretimeout_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	wd_data->pretimeout_timer.function = watchdog_hrtimer_pretimeout;
> +}
> +
> +void watchdog_hrtimer_pretimeout_start(struct watchdog_device *wdd)
> +{
> +	if (!(wdd->info->options & WDIOF_PRETIMEOUT) &&
> +	    !watchdog_pretimeout_invalid(wdd, wdd->pretimeout))
> +		hrtimer_start(&wdd->wd_data->pretimeout_timer,
> +			      ktime_set(wdd->timeout - wdd->pretimeout, 0),
> +			      HRTIMER_MODE_REL);
> +	else
> +		hrtimer_cancel(&wdd->wd_data->pretimeout_timer);
> +}
> +
> +void watchdog_hrtimer_pretimeout_stop(struct watchdog_device *wdd)
> +{
> +	hrtimer_cancel(&wdd->wd_data->pretimeout_timer);
> +}
> diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
> index 01ca84b..4d1c223 100644
> --- a/drivers/watchdog/watchdog_pretimeout.c
> +++ b/drivers/watchdog/watchdog_pretimeout.c
> @@ -9,6 +9,7 @@
>  #include <linux/string.h>
>  #include <linux/watchdog.h>
>  
> +#include "watchdog_core.h"
>  #include "watchdog_pretimeout.h"
>  
>  /* Default watchdog pretimeout governor */
> @@ -177,7 +178,7 @@ int watchdog_register_pretimeout(struct watchdog_device *wdd)
>  {
>  	struct watchdog_pretimeout *p;
>  
> -	if (!(wdd->info->options & WDIOF_PRETIMEOUT))
> +	if (!watchdog_have_pretimeout(wdd))
>  		return 0;
>  
>  	p = kzalloc(sizeof(*p), GFP_KERNEL);
> @@ -197,7 +198,7 @@ void watchdog_unregister_pretimeout(struct watchdog_device *wdd)
>  {
>  	struct watchdog_pretimeout *p, *t;
>  
> -	if (!(wdd->info->options & WDIOF_PRETIMEOUT))
> +	if (!watchdog_have_pretimeout(wdd))
>  		return;
>  
>  	spin_lock_irq(&pretimeout_lock);
> -- 
> 2.7.4
> 

  reply	other threads:[~2021-02-04  1:35 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-03 20:11 Curtis Klein
2021-02-04  1:35 ` Guenter Roeck [this message]
2021-09-02  6:55 ` Jiri Slaby
2021-09-02 14:05   ` Guenter Roeck
2021-09-04  8:16     ` was [Re: [PATCH v3] watchdog: Add hrtimer-based pretimeout feature] Jiri Slaby
     [not found]       ` <54d77fb1-2531-c6ed-738e-9f661443b097@kernel.org>
2021-09-05 22:22         ` Klein, Curtis
2021-09-05 22:56           ` Guenter Roeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210204013506.GA87215@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=curtis.klein@hpe.com \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=wim@linux-watchdog.org \
    --subject='Re: [PATCH v3] watchdog: Add hrtimer-based pretimeout feature' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).