Messages in this thread | | | Subject | Re: [PATCH v3 2/2] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT | From | Guenter Roeck <> | Date | Mon, 2 Jan 2017 07:22:39 -0800 |
| |
On 12/14/2016 05:37 AM, Rasmus Villemoes wrote: > The watchdog framework takes care of feeding a hardware watchdog until > userspace opens /dev/watchdogN. If that never happens for some reason > (buggy init script, corrupt root filesystem or whatnot) but the kernel > itself is fine, the machine stays up indefinitely. This patch allows > setting an upper limit for how long the kernel will take care of the > watchdog, thus ensuring that the watchdog will eventually reset the > machine if userspace fails to come up. > > This is particularly useful for embedded devices where some fallback > logic is implemented in the bootloader (e.g., use a different root > partition, boot from network, ...). > > The open timeout is also used as a maximum time for an application to > re-open /dev/watchdogN after closing it. > > The open timeout is taken from the device property "open-timeout", and > if that is not present, defaults to CONFIG_WATCHDOG_OPEN_TIMEOUT, which > in turn defaults to 0, which means there is no upper limit (as must be > the default, since there may be existing systems out there relying on > the kernel taking care of the watchdog forever). > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > drivers/watchdog/Kconfig | 14 ++++++++++++++ > drivers/watchdog/watchdog_core.c | 16 ++++++++++++++++ > drivers/watchdog/watchdog_dev.c | 22 +++++++++++++++++++++- > include/linux/watchdog.h | 7 +++++++ > 4 files changed, 58 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 3eb58cb..890418c 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -52,6 +52,20 @@ config WATCHDOG_SYSFS > Say Y here if you want to enable watchdog device status read through > sysfs attributes. > > +config WATCHDOG_OPEN_TIMEOUT > + int "Default timeout value for opening watchdog device (seconds)" > + default 0 > + help > + If a watchdog driver indicates to the framework that the > + hardware watchdog is running, the framework takes care of > + pinging the watchdog until userspace opens > + /dev/watchdogN. This value (overridden by the device's > + "open-timeout" property if present) sets an upper bound for > + how long the kernel does this - thus, if userspace hasn't > + opened the device within the timeout, the board reboots. A > + value of 0 means there is no timeout. > + > +
Dual empty line. Also, as mentioned before, I am not a friend of such configuration variables. It forces distribution vendors to make the decision for everyone. Please consider defining a command line parameter such as watchdog_open_timeout.
> # > # General Watchdog drivers > # > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c > index 74265b2..d968e0f 100644 > --- a/drivers/watchdog/watchdog_core.c > +++ b/drivers/watchdog/watchdog_core.c > @@ -38,6 +38,7 @@ > #include <linux/idr.h> /* For ida_* macros */ > #include <linux/err.h> /* For IS_ERR macros */ > #include <linux/of.h> /* For of_get_timeout_sec */ > +#include <linux/property.h> /* For device_property_read_u32 */ > > #include "watchdog_core.h" /* For watchdog_dev_register/... */ > > @@ -191,6 +192,19 @@ void watchdog_set_restart_priority(struct watchdog_device *wdd, int priority) > } > EXPORT_SYMBOL_GPL(watchdog_set_restart_priority); > > +static void > +watchdog_set_open_timeout(struct watchdog_device *wdd) > +{ > + u32 t; > + struct device *dev; > + > + dev = wdd->parent; > + if (dev && !device_property_read_u32(dev, "open-timeout", &t)) > + wdd->open_timeout = t;
The check if dev == NULL is unnecessary.
New devicetree properties need to be documented and approved by devicetree maintainers.
> + else > + wdd->open_timeout = CONFIG_WATCHDOG_OPEN_TIMEOUT; > +} > + > static int __watchdog_register_device(struct watchdog_device *wdd) > { > int ret, id = -1; > @@ -225,6 +239,8 @@ static int __watchdog_register_device(struct watchdog_device *wdd) > return id; > wdd->id = id; > > + watchdog_set_open_timeout(wdd); > + Consider moving this call to watchdog_dev_register().
> ret = watchdog_dev_register(wdd); > if (ret) { > ida_simple_remove(&watchdog_ida, id); > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c > index ca0a000..f153091 100644 > --- a/drivers/watchdog/watchdog_dev.c > +++ b/drivers/watchdog/watchdog_dev.c > @@ -80,6 +80,18 @@ static struct watchdog_core_data *old_wd_data; > > static struct workqueue_struct *watchdog_wq; > > +static bool watchdog_past_open_deadline(struct watchdog_device *wdd) > +{ > + if (!wdd->open_timeout) > + return false; > + return time_is_before_jiffies(wdd->open_deadline); > +} > + > +static void watchdog_set_open_deadline(struct watchdog_device *wdd) > +{ > + wdd->open_deadline = jiffies + msecs_to_jiffies(1000 * wdd->open_timeout); > +} > + > static inline bool watchdog_need_worker(struct watchdog_device *wdd) > { > /* All variables in milli-seconds */ > @@ -194,7 +206,13 @@ static int watchdog_ping(struct watchdog_device *wdd) > > static bool watchdog_worker_should_ping(struct watchdog_device *wdd) > { > - return wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd)); > + if (!wdd) > + return false; > + > + if (watchdog_active(wdd)) > + return true; > + > + return watchdog_hw_running(wdd) && !watchdog_past_open_deadline(wdd); > } > > static void watchdog_ping_work(struct work_struct *work) > @@ -857,6 +875,7 @@ static int watchdog_release(struct inode *inode, struct file *file) > watchdog_ping(wdd); > } > > + watchdog_set_open_deadline(wdd); > watchdog_update_worker(wdd); > > /* make sure that /dev/watchdog can be re-opened */ > @@ -955,6 +974,7 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno) > > /* Record time of most recent heartbeat as 'just before now'. */ > wd_data->last_hw_keepalive = jiffies - 1; > + watchdog_set_open_deadline(wdd); > > /* > * If the watchdog is running, prevent its driver from being unloaded, > diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h > index 35a4d81..a89a293 100644 > --- a/include/linux/watchdog.h > +++ b/include/linux/watchdog.h > @@ -76,6 +76,11 @@ struct watchdog_ops { > * @max_hw_heartbeat_ms: > * Hardware limit for maximum timeout, in milli-seconds. > * Replaces max_timeout if specified. > + * @open_timeout: > + * The maximum time for which the kernel will ping the > + * device after registration. > + * @open_deadline: > + * Set to jiffies + @open_timeout at registration. > * @reboot_nb: The notifier block to stop watchdog on reboot. > * @restart_nb: The notifier block to register a restart function. > * @driver_data:Pointer to the drivers private data. > @@ -107,6 +112,8 @@ struct watchdog_device { > unsigned int max_timeout; > unsigned int min_hw_heartbeat_ms; > unsigned int max_hw_heartbeat_ms; > + unsigned int open_timeout; > + unsigned long open_deadline;
None of those need to be visible in drivers. I don't see why the variables should be defined here and not in struct watchdog_core_data.
> struct notifier_block reboot_nb; > struct notifier_block restart_nb; > void *driver_data; >
| |