On 2/1/21 4:39 PM, 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@xxxxxxx>
> ---
> 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.c | 2 +
> drivers/watchdog/watchdog_core.h | 27 ++++++++++++++
> drivers/watchdog/watchdog_dev.c | 51 +++++++++++---------------
> drivers/watchdog/watchdog_hrtimer_pretimeout.c | 45 +++++++++++++++++++++++
> drivers/watchdog/watchdog_hrtimer_pretimeout.h | 19 ++++++++++
> drivers/watchdog/watchdog_pretimeout.c | 6 ++-
> 8 files changed, 128 insertions(+), 31 deletions(-)
> create mode 100644 drivers/watchdog/watchdog_hrtimer_pretimeout.c
> create mode 100644 drivers/watchdog/watchdog_hrtimer_pretimeout.h
>
> 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.c b/drivers/watchdog/watchdog_core.c
> index 0e9a995..1449bf124 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -27,7 +27,9 @@
> #include <linux/module.h> /* For EXPORT_SYMBOL/module stuff/... */
> #include <linux/types.h> /* For standard types */
> #include <linux/errno.h> /* For the -ENODEV/... values */
> +#include <linux/hrtimer.h> /* For hrtimers */
> #include <linux/kernel.h> /* For printk/panic/... */
> +#include <linux/kthread.h> /* For kthread_work */
The additional include files should be included where needed, in watchdog_core.h
and/or in watchdog_hrtimer_pretimeout.c. I don't think they are needed here.
> #include <linux/reboot.h> /* For restart handler */
> #include <linux/watchdog.h> /* For watchdog specific items */
> #include <linux/init.h> /* For __init/__exit/... */
> diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
> index a5062e8..323600a 100644
> --- a/drivers/watchdog/watchdog_core.h
> +++ b/drivers/watchdog/watchdog_core.h
> @@ -25,6 +25,33 @@
> #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 *);
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 2946f3a..2a2e8d8 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@xxxxxxxxx>.
> *
> + * (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.
> @@ -45,30 +46,7 @@
>
> #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 ? */
> -};
> +#include "watchdog_hrtimer_pretimeout.h"
>
> /* the dev_t structure to store the dynamically allocated watchdog devices */
> static dev_t watchdog_devt;
> @@ -185,6 +163,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 +256,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 +267,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 +309,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 +346,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 +396,8 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
> {
> int err = 0;
>
> - if (!(wdd->info->options & WDIOF_PRETIMEOUT))
> + if (!(wdd->info->options & WDIOF_PRETIMEOUT) &&
> + !IS_ENABLED(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT))
Please add something like
static inline bool watchdog_have_pretimeout(struct watchdog_device *wdd)
{
return wdd->info->options & WDIOF_PRETIMEOUT || IS_ENABLED(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT);
}
to watchdog_core.h and use it where appropriate.
> return -EOPNOTSUPP;
>
> if (watchdog_pretimeout_invalid(wdd, timeout))
> @@ -595,12 +584,14 @@ 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))
> + !(wdd->info->options & WDIOF_PRETIMEOUT) &&
> + !IS_ENABLED(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT))
> 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)))
> + ((!(wdd->info->options & WDIOF_PRETIMEOUT) &&
> + !IS_ENABLED(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT)) ||
> + !IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV)))
> mode = 0;
>
> return mode;
> @@ -1009,6 +1000,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 +1088,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..a26a18c
> --- /dev/null
> +++ b/drivers/watchdog/watchdog_hrtimer_pretimeout.c
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * (c) Copyright 2021 Hewlett Packard Enterprise Development LP.
> + */
> +
> +#include <linux/hrtimer.h>
> +#include <linux/kthread.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_hrtimer_pretimeout.h b/drivers/watchdog/watchdog_hrtimer_pretimeout.h
> new file mode 100644
> index 00000000..ad136b2
> --- /dev/null
> +++ b/drivers/watchdog/watchdog_hrtimer_pretimeout.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * (c) Copyright 2021 Hewlett Packard Enterprise Development LP.
> + */
> +
> +#ifndef __WATCHDOG_HRTIMER_PRETIMEOUT_H
> +#define __WATCHDOG_HRTIMER_PRETIMEOUT_H
> +
> +#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 void watchdog_hrtimer_pretimeout_init(struct watchdog_device *wdd) {}
> +static void watchdog_hrtimer_pretimeout_start(struct watchdog_device *wdd) {}
> +static void watchdog_hrtimer_pretimeout_stop(struct watchdog_device *wdd) {}
> +#endif
> +
I don't think a separate include file is warranted here. Just add the above
function declarations to watchdog_core.h. CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT
is already used there anyway.
Thanks,
Guenter
> +#endif
> diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
> index 01ca84b..5629198 100644
> --- a/drivers/watchdog/watchdog_pretimeout.c
> +++ b/drivers/watchdog/watchdog_pretimeout.c
> @@ -177,7 +177,8 @@ int watchdog_register_pretimeout(struct watchdog_device *wdd)
> {
> struct watchdog_pretimeout *p;
>
> - if (!(wdd->info->options & WDIOF_PRETIMEOUT))
> + if (!(wdd->info->options & WDIOF_PRETIMEOUT) &&
> + !IS_ENABLED(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT))
> return 0;
>
> p = kzalloc(sizeof(*p), GFP_KERNEL);
> @@ -197,7 +198,8 @@ void watchdog_unregister_pretimeout(struct watchdog_device *wdd)
> {
> struct watchdog_pretimeout *p, *t;
>
> - if (!(wdd->info->options & WDIOF_PRETIMEOUT))
> + if (!(wdd->info->options & WDIOF_PRETIMEOUT) &&
> + !IS_ENABLED(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT))
> return;
>
> spin_lock_irq(&pretimeout_lock);
>
