The watchdog infrastructure is currently purely passive, meaning
it only passes information from user space to drivers and vice versa.
Since watchdog hardware tends to have its own quirks, this can result
in quite complex watchdog drivers. A number of scanarios are especially common.
- A watchdog is always active and can not be disabled, or can not be disabled
once enabled. To support such hardware, watchdog drivers have to implement
their own timers and use those timers to trigger watchdog keepalives while
the watchdog device is not or not yet opened.
- A variant of this is the desire to enable a watchdog as soon as its driver
has been instantiated, to protect the system while it is still booting up,
but the watchdog daemon is not yet running.
- Some watchdogs have a very short maximum timeout, in the range of just a few
seconds. Such low timeouts are difficult if not impossible to support from
user space. Drivers supporting such watchdog hardware need to implement
a timer function to augment heartbeats from user space.
This patch set solves the above problems while keeping changes to the
watchdog core minimal.
- A new status flag, WDOG_RUNNING, informs the watchdog subsystem that a
watchdog is running, and that the watchdog subsystem needs to generate
heartbeat requests while the associated watchdog device is closed.
- A new parameter in the watchdog data structure, max_hw_timeout_ms, informs
the watchdog subsystem about a maximum hardware timeout. The watchdog
subsystem uses this information together with the configured timeout
and the maximum permitted timeout to determine if it needs to generate
additional heartbeat requests.
Patch #1 is a preparatory patch.
Patch #2 adds timer functionality to the watchdog core. It solves the problem
of short maximum hardware timeouts by augmenting heartbeats triggered from
user space with internally triggered heartbeats.
Patch #3 adds functionality to generate heartbeats while the watchdog device is
closed. It handles situation where where the watchdog is running after
the driver has been instantiated, but the device is not yet opened,
and post-close situations necessary if a watchdog can not be stopped.
Patch #4 makes the set_timeout function optional. This is now possible since
timeout changes can now be completely handled in the watchdog core, for
example if the hardware watchdog timeout is fixed.
Patch #5 to #8 are example conversions of some watchdog drivers.
Those patches will require testing.
This patch set does not solve all limitations of the watchdog subsystem.
Specifically, it does not add support for the following features.
- It is desirable to be able to specify a maximum early timeout,
from booting the system to opening the watchdog device.
- Some watchdogs may require a minimum period of time between
heartbeats. Examples are DA9062 and possibly AT91SAM9x.
This and other features will be adddessed with subsequent patches.
The patch set is inspired by an earlier patch set from Timo Kokonnen.
The current code uses 'wdd', wddev', and 'watchdog' as variable names
for struct watchdog_device. This is confusing and makes it difficult
to enhance the code. Replace it all with 'wdd'.
Cc: Timo Kokkonen <[email protected]>
Cc: Uwe Kleine-König <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/watchdog_dev.c | 151 ++++++++++++++++++++--------------------
1 file changed, 75 insertions(+), 76 deletions(-)
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 6aaefbad303e..06171c73daf5 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -51,7 +51,7 @@ static struct watchdog_device *old_wdd;
/*
* watchdog_ping: ping the watchdog.
- * @wddev: the watchdog device to ping
+ * @wdd: the watchdog device to ping
*
* If the watchdog has no own ping operation then it needs to be
* restarted via the start operation. This wrapper function does
@@ -59,65 +59,65 @@ static struct watchdog_device *old_wdd;
* We only ping when the watchdog device is running.
*/
-static int watchdog_ping(struct watchdog_device *wddev)
+static int watchdog_ping(struct watchdog_device *wdd)
{
int err = 0;
- mutex_lock(&wddev->lock);
+ mutex_lock(&wdd->lock);
- if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+ if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
err = -ENODEV;
goto out_ping;
}
- if (!watchdog_active(wddev))
+ if (!watchdog_active(wdd))
goto out_ping;
- if (wddev->ops->ping)
- err = wddev->ops->ping(wddev); /* ping the watchdog */
+ if (wdd->ops->ping)
+ err = wdd->ops->ping(wdd); /* ping the watchdog */
else
- err = wddev->ops->start(wddev); /* restart watchdog */
+ err = wdd->ops->start(wdd); /* restart watchdog */
out_ping:
- mutex_unlock(&wddev->lock);
+ mutex_unlock(&wdd->lock);
return err;
}
/*
* watchdog_start: wrapper to start the watchdog.
- * @wddev: the watchdog device to start
+ * @wdd: the watchdog device to start
*
* Start the watchdog if it is not active and mark it active.
* This function returns zero on success or a negative errno code for
* failure.
*/
-static int watchdog_start(struct watchdog_device *wddev)
+static int watchdog_start(struct watchdog_device *wdd)
{
int err = 0;
- mutex_lock(&wddev->lock);
+ mutex_lock(&wdd->lock);
- if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+ if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
err = -ENODEV;
goto out_start;
}
- if (watchdog_active(wddev))
+ if (watchdog_active(wdd))
goto out_start;
- err = wddev->ops->start(wddev);
+ err = wdd->ops->start(wdd);
if (err == 0)
- set_bit(WDOG_ACTIVE, &wddev->status);
+ set_bit(WDOG_ACTIVE, &wdd->status);
out_start:
- mutex_unlock(&wddev->lock);
+ mutex_unlock(&wdd->lock);
return err;
}
/*
* watchdog_stop: wrapper to stop the watchdog.
- * @wddev: the watchdog device to stop
+ * @wdd: the watchdog device to stop
*
* Stop the watchdog if it is still active and unmark it active.
* This function returns zero on success or a negative errno code for
@@ -125,155 +125,154 @@ out_start:
* If the 'nowayout' feature was set, the watchdog cannot be stopped.
*/
-static int watchdog_stop(struct watchdog_device *wddev)
+static int watchdog_stop(struct watchdog_device *wdd)
{
int err = 0;
- mutex_lock(&wddev->lock);
+ mutex_lock(&wdd->lock);
- if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+ if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
err = -ENODEV;
goto out_stop;
}
- if (!watchdog_active(wddev))
+ if (!watchdog_active(wdd))
goto out_stop;
- if (test_bit(WDOG_NO_WAY_OUT, &wddev->status)) {
- dev_info(wddev->dev, "nowayout prevents watchdog being stopped!\n");
+ if (test_bit(WDOG_NO_WAY_OUT, &wdd->status)) {
+ dev_info(wdd->dev, "nowayout prevents watchdog being stopped!\n");
err = -EBUSY;
goto out_stop;
}
- err = wddev->ops->stop(wddev);
+ err = wdd->ops->stop(wdd);
if (err == 0)
- clear_bit(WDOG_ACTIVE, &wddev->status);
+ clear_bit(WDOG_ACTIVE, &wdd->status);
out_stop:
- mutex_unlock(&wddev->lock);
+ mutex_unlock(&wdd->lock);
return err;
}
/*
* watchdog_get_status: wrapper to get the watchdog status
- * @wddev: the watchdog device to get the status from
+ * @wdd: the watchdog device to get the status from
* @status: the status of the watchdog device
*
* Get the watchdog's status flags.
*/
-static int watchdog_get_status(struct watchdog_device *wddev,
+static int watchdog_get_status(struct watchdog_device *wdd,
unsigned int *status)
{
int err = 0;
*status = 0;
- if (!wddev->ops->status)
+ if (!wdd->ops->status)
return -EOPNOTSUPP;
- mutex_lock(&wddev->lock);
+ mutex_lock(&wdd->lock);
- if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+ if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
err = -ENODEV;
goto out_status;
}
- *status = wddev->ops->status(wddev);
+ *status = wdd->ops->status(wdd);
out_status:
- mutex_unlock(&wddev->lock);
+ mutex_unlock(&wdd->lock);
return err;
}
/*
* watchdog_set_timeout: set the watchdog timer timeout
- * @wddev: the watchdog device to set the timeout for
+ * @wdd: the watchdog device to set the timeout for
* @timeout: timeout to set in seconds
*/
-static int watchdog_set_timeout(struct watchdog_device *wddev,
+static int watchdog_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)
{
int err;
- if ((wddev->ops->set_timeout == NULL) ||
- !(wddev->info->options & WDIOF_SETTIMEOUT))
+ if (!wdd->ops->set_timeout || !(wdd->info->options & WDIOF_SETTIMEOUT))
return -EOPNOTSUPP;
- if (watchdog_timeout_invalid(wddev, timeout))
+ if (watchdog_timeout_invalid(wdd, timeout))
return -EINVAL;
- mutex_lock(&wddev->lock);
+ mutex_lock(&wdd->lock);
- if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+ if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
err = -ENODEV;
goto out_timeout;
}
- err = wddev->ops->set_timeout(wddev, timeout);
+ err = wdd->ops->set_timeout(wdd, timeout);
out_timeout:
- mutex_unlock(&wddev->lock);
+ mutex_unlock(&wdd->lock);
return err;
}
/*
* watchdog_get_timeleft: wrapper to get the time left before a reboot
- * @wddev: the watchdog device to get the remaining time from
+ * @wdd: the watchdog device to get the remaining time from
* @timeleft: the time that's left
*
* Get the time before a watchdog will reboot (if not pinged).
*/
-static int watchdog_get_timeleft(struct watchdog_device *wddev,
+static int watchdog_get_timeleft(struct watchdog_device *wdd,
unsigned int *timeleft)
{
int err = 0;
*timeleft = 0;
- if (!wddev->ops->get_timeleft)
+ if (!wdd->ops->get_timeleft)
return -EOPNOTSUPP;
- mutex_lock(&wddev->lock);
+ mutex_lock(&wdd->lock);
- if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+ if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
err = -ENODEV;
goto out_timeleft;
}
- *timeleft = wddev->ops->get_timeleft(wddev);
+ *timeleft = wdd->ops->get_timeleft(wdd);
out_timeleft:
- mutex_unlock(&wddev->lock);
+ mutex_unlock(&wdd->lock);
return err;
}
/*
* watchdog_ioctl_op: call the watchdog drivers ioctl op if defined
- * @wddev: the watchdog device to do the ioctl on
+ * @wdd: the watchdog device to do the ioctl on
* @cmd: watchdog command
* @arg: argument pointer
*/
-static int watchdog_ioctl_op(struct watchdog_device *wddev, unsigned int cmd,
+static int watchdog_ioctl_op(struct watchdog_device *wdd, unsigned int cmd,
unsigned long arg)
{
int err;
- if (!wddev->ops->ioctl)
+ if (!wdd->ops->ioctl)
return -ENOIOCTLCMD;
- mutex_lock(&wddev->lock);
+ mutex_lock(&wdd->lock);
- if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+ if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
err = -ENODEV;
goto out_ioctl;
}
- err = wddev->ops->ioctl(wddev, cmd, arg);
+ err = wdd->ops->ioctl(wdd, cmd, arg);
out_ioctl:
- mutex_unlock(&wddev->lock);
+ mutex_unlock(&wdd->lock);
return err;
}
@@ -513,43 +512,43 @@ static struct miscdevice watchdog_miscdev = {
/*
* watchdog_dev_register: register a watchdog device
- * @watchdog: watchdog device
+ * @wdd: watchdog device
*
* Register a watchdog device including handling the legacy
* /dev/watchdog node. /dev/watchdog is actually a miscdevice and
* thus we set it up like that.
*/
-int watchdog_dev_register(struct watchdog_device *watchdog)
+int watchdog_dev_register(struct watchdog_device *wdd)
{
int err, devno;
- if (watchdog->id == 0) {
- old_wdd = watchdog;
- watchdog_miscdev.parent = watchdog->parent;
+ if (wdd->id == 0) {
+ old_wdd = wdd;
+ watchdog_miscdev.parent = wdd->parent;
err = misc_register(&watchdog_miscdev);
if (err != 0) {
pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
- watchdog->info->identity, WATCHDOG_MINOR, err);
+ wdd->info->identity, WATCHDOG_MINOR, err);
if (err == -EBUSY)
pr_err("%s: a legacy watchdog module is probably present.\n",
- watchdog->info->identity);
+ wdd->info->identity);
old_wdd = NULL;
return err;
}
}
/* Fill in the data structures */
- devno = MKDEV(MAJOR(watchdog_devt), watchdog->id);
- cdev_init(&watchdog->cdev, &watchdog_fops);
- watchdog->cdev.owner = watchdog->ops->owner;
+ devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
+ cdev_init(&wdd->cdev, &watchdog_fops);
+ wdd->cdev.owner = wdd->ops->owner;
/* Add the device */
- err = cdev_add(&watchdog->cdev, devno, 1);
+ err = cdev_add(&wdd->cdev, devno, 1);
if (err) {
pr_err("watchdog%d unable to add device %d:%d\n",
- watchdog->id, MAJOR(watchdog_devt), watchdog->id);
- if (watchdog->id == 0) {
+ wdd->id, MAJOR(watchdog_devt), wdd->id);
+ if (wdd->id == 0) {
misc_deregister(&watchdog_miscdev);
old_wdd = NULL;
}
@@ -564,14 +563,14 @@ int watchdog_dev_register(struct watchdog_device *watchdog)
* Unregister the watchdog and if needed the legacy /dev/watchdog device.
*/
-int watchdog_dev_unregister(struct watchdog_device *watchdog)
+int watchdog_dev_unregister(struct watchdog_device *wdd)
{
- mutex_lock(&watchdog->lock);
- set_bit(WDOG_UNREGISTERED, &watchdog->status);
- mutex_unlock(&watchdog->lock);
+ mutex_lock(&wdd->lock);
+ set_bit(WDOG_UNREGISTERED, &wdd->status);
+ mutex_unlock(&wdd->lock);
- cdev_del(&watchdog->cdev);
- if (watchdog->id == 0) {
+ cdev_del(&wdd->cdev);
+ if (wdd->id == 0) {
misc_deregister(&watchdog_miscdev);
old_wdd = NULL;
}
--
2.1.4
Introduce an optional hardware maximum timeout in the watchdog core.
The hardware maximum timeout can be lower than the maximum timeout.
Drivers can set the maximum hardare timeout value in the watchdog data
structure. If the configured timeout exceeds half the value of the
maximum hardware timeout, the watchdog core enables a timer function
to assist sending keepalive requests to the watchdog driver.
Cc: Timo Kokkonen <[email protected]>
Cc: Uwe Kleine-König <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
Documentation/watchdog/watchdog-kernel-api.txt | 14 +++
drivers/watchdog/watchdog_dev.c | 121 +++++++++++++++++++++----
include/linux/watchdog.h | 21 ++++-
3 files changed, 135 insertions(+), 21 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index d8b0d3367706..5fa085276874 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -53,9 +53,12 @@ struct watchdog_device {
unsigned int timeout;
unsigned int min_timeout;
unsigned int max_timeout;
+ unsigned int max_hw_timeout_ms;
+ unsigned long last_keepalive;
void *driver_data;
struct mutex lock;
unsigned long status;
+ struct delayed_work work;
struct list_head deferred;
};
@@ -73,8 +76,18 @@ It contains following fields:
additional information about the watchdog timer itself. (Like it's unique name)
* ops: a pointer to the list of watchdog operations that the watchdog supports.
* timeout: the watchdog timer's timeout value (in seconds).
+ This is the time after which the system will reboot if user space does
+ not send a heartbeat request if the watchdog device is opened.
+ This may or may not be the hardware watchdog timeout. See max_hw_timeout_ms
+ for more details.
* min_timeout: the watchdog timer's minimum timeout value (in seconds).
* max_timeout: the watchdog timer's maximum timeout value (in seconds).
+* max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds. May differ
+ from max_timeout. If set, the infrastructure will send a heartbeat to the
+ watchdog driver if 'timeout' is larger than 'max_hw_timeout / 2',
+ unless user space failed to ping the watchdog for 'timeout' seconds.
+* last_keepalive: Time of most recent keepalive triggered from user space,
+ in jiffies.
* bootstatus: status of the device after booting (reported with watchdog
WDIOF_* status bits).
* driver_data: a pointer to the drivers private data of a watchdog device.
@@ -85,6 +98,7 @@ It contains following fields:
information about the status of the device (Like: is the watchdog timer
running/active, is the nowayout bit set, is the device opened via
the /dev/watchdog interface or not, ...).
+* work: Worker data structure for WatchDog Timer Driver Core internal use only.
* deferred: entry in wtd_deferred_reg_list which is used to
register early initialized watchdogs.
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 06171c73daf5..25849c1d6dc1 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -37,7 +37,9 @@
#include <linux/errno.h> /* For the -ENODEV/... values */
#include <linux/kernel.h> /* For printk/panic/... */
#include <linux/fs.h> /* For file operations */
+#include <linux/jiffies.h> /* For timeout functions */
#include <linux/watchdog.h> /* For watchdog specific items */
+#include <linux/workqueue.h> /* For workqueue */
#include <linux/miscdevice.h> /* For handling misc devices */
#include <linux/init.h> /* For __init/__exit/... */
#include <linux/uaccess.h> /* For copy_to_user/put_user/... */
@@ -49,6 +51,53 @@ static dev_t watchdog_devt;
/* the watchdog device behind /dev/watchdog */
static struct watchdog_device *old_wdd;
+static struct workqueue_struct *watchdog_wq;
+
+static inline bool watchdog_need_worker(struct watchdog_device *wdd)
+{
+ unsigned int hm = wdd->max_hw_timeout_ms;
+ unsigned int m = wdd->max_timeout * 1000;
+
+ return watchdog_active(wdd) && hm && hm != m &&
+ wdd->timeout * 500 > hm;
+}
+
+static inline void watchdog_update_worker(struct watchdog_device *wdd,
+ bool cancel, bool sync)
+{
+ if (watchdog_need_worker(wdd)) {
+ unsigned int t = wdd->timeout * 1000;
+
+ if (wdd->max_hw_timeout_ms && t > wdd->max_hw_timeout_ms)
+ t = wdd->max_hw_timeout_ms;
+ queue_delayed_work(watchdog_wq, &wdd->work,
+ msecs_to_jiffies(t / 2));
+ } else if (cancel) {
+ if (sync)
+ cancel_delayed_work_sync(&wdd->work);
+ else
+ cancel_delayed_work(&wdd->work);
+ }
+}
+
+static int _watchdog_ping(struct watchdog_device *wdd)
+{
+ int err;
+
+ if (test_bit(WDOG_UNREGISTERED, &wdd->status))
+ return -ENODEV;
+
+ if (!watchdog_active(wdd))
+ return 0;
+
+ if (wdd->ops->ping)
+ err = wdd->ops->ping(wdd); /* ping the watchdog */
+ else
+ err = wdd->ops->start(wdd); /* restart watchdog */
+
+ return err;
+}
+
/*
* watchdog_ping: ping the watchdog.
* @wdd: the watchdog device to ping
@@ -61,26 +110,34 @@ static struct watchdog_device *old_wdd;
static int watchdog_ping(struct watchdog_device *wdd)
{
- int err = 0;
+ int err;
mutex_lock(&wdd->lock);
+ err = _watchdog_ping(wdd);
+ wdd->last_keepalive = jiffies;
+ mutex_unlock(&wdd->lock);
- if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
- err = -ENODEV;
- goto out_ping;
- }
+ return err;
+}
- if (!watchdog_active(wdd))
- goto out_ping;
+static void watchdog_ping_work(struct work_struct *work)
+{
+ struct watchdog_device *wdd;
- if (wdd->ops->ping)
- err = wdd->ops->ping(wdd); /* ping the watchdog */
- else
- err = wdd->ops->start(wdd); /* restart watchdog */
+ wdd = container_of(to_delayed_work(work), struct watchdog_device, work);
-out_ping:
+ mutex_lock(&wdd->lock);
+ if (watchdog_active(wdd) &&
+ time_after(jiffies, wdd->last_keepalive +
+ msecs_to_jiffies(wdd->timeout * 1000))) {
+ dev_crit(wdd->dev, "Timer expired. System will reboot soon!\n");
+ goto out;
+ }
+ _watchdog_ping(wdd);
+ watchdog_update_worker(wdd, false, false);
+
+out:
mutex_unlock(&wdd->lock);
- return err;
}
/*
@@ -107,8 +164,10 @@ static int watchdog_start(struct watchdog_device *wdd)
goto out_start;
err = wdd->ops->start(wdd);
- if (err == 0)
+ if (err == 0) {
set_bit(WDOG_ACTIVE, &wdd->status);
+ watchdog_update_worker(wdd, false, false);
+ }
out_start:
mutex_unlock(&wdd->lock);
@@ -146,8 +205,10 @@ static int watchdog_stop(struct watchdog_device *wdd)
}
err = wdd->ops->stop(wdd);
- if (err == 0)
+ if (err == 0) {
clear_bit(WDOG_ACTIVE, &wdd->status);
+ watchdog_update_worker(wdd, true, false);
+ }
out_stop:
mutex_unlock(&wdd->lock);
@@ -211,6 +272,8 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
err = wdd->ops->set_timeout(wdd, timeout);
+ watchdog_update_worker(wdd, true, false);
+
out_timeout:
mutex_unlock(&wdd->lock);
return err;
@@ -483,6 +546,8 @@ static int watchdog_release(struct inode *inode, struct file *file)
watchdog_ping(wdd);
}
+ cancel_delayed_work_sync(&wdd->work);
+
/* Allow the owner module to be unloaded again */
module_put(wdd->ops->owner);
@@ -523,6 +588,14 @@ int watchdog_dev_register(struct watchdog_device *wdd)
{
int err, devno;
+ if (!watchdog_wq)
+ return -ENODEV;
+
+ INIT_DELAYED_WORK(&wdd->work, watchdog_ping_work);
+
+ if (!wdd->max_hw_timeout_ms)
+ wdd->max_hw_timeout_ms = wdd->max_timeout * 1000;
+
if (wdd->id == 0) {
old_wdd = wdd;
watchdog_miscdev.parent = wdd->parent;
@@ -574,6 +647,9 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
misc_deregister(&watchdog_miscdev);
old_wdd = NULL;
}
+
+ cancel_delayed_work_sync(&wdd->work);
+
return 0;
}
@@ -585,9 +661,21 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
int __init watchdog_dev_init(void)
{
- int err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
+ int err;
+
+ watchdog_wq = alloc_workqueue("watchdogd",
+ WQ_HIGHPRI | WQ_MEM_RECLAIM, 0);
+ if (!watchdog_wq) {
+ pr_err("Failed to create watchdog workqueue\n");
+ err = -ENOMEM;
+ goto abort;
+ }
+
+ err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
if (err < 0)
pr_err("watchdog: unable to allocate char dev region\n");
+
+abort:
return err;
}
@@ -600,4 +688,5 @@ int __init watchdog_dev_init(void)
void __exit watchdog_dev_exit(void)
{
unregister_chrdev_region(watchdog_devt, MAX_DOGS);
+ destroy_workqueue(watchdog_wq);
}
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index f47feada5b42..2703b2511481 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -12,6 +12,7 @@
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/cdev.h>
+#include <linux/workqueue.h>
#include <uapi/linux/watchdog.h>
struct watchdog_ops;
@@ -59,14 +60,21 @@ struct watchdog_ops {
* @info: Pointer to a watchdog_info structure.
* @ops: Pointer to the list of watchdog operations.
* @bootstatus: Status of the watchdog device at boot.
- * @timeout: The watchdog devices timeout value.
- * @min_timeout:The watchdog devices minimum timeout value.
- * @max_timeout:The watchdog devices maximum timeout value.
+ * @timeout: The watchdog devices timeout value, in seconds.
+ * @min_timeout:The watchdog devices minimum timeout value, in seconds.
+ * @max_timeout:The watchdog devices maximum timeout value, in seconds.
+ * @max_hw_timeout_ms:
+ * Hardware limit for maximum timeout, in milli-seconds,
+ * if different from max_timeout.
+ * @last_keepalive:
+ * Time of most recent keepalive triggered from user space,
+ * in jiffies (watchdog core internal).
* @driver-data:Pointer to the drivers private data.
* @lock: Lock for watchdog core internal use only.
* @status: Field that contains the devices internal status bits.
- * @deferred: entry in wtd_deferred_reg_list which is used to
- * register early initialized watchdogs.
+ * @work: Data structure for worker function (watchdog core internal).
+ * @deferred: entry in wtd_deferred_reg_list which is used to
+ * register early initialized watchdogs.
*
* The watchdog_device structure contains all information about a
* watchdog timer device.
@@ -88,6 +96,8 @@ struct watchdog_device {
unsigned int timeout;
unsigned int min_timeout;
unsigned int max_timeout;
+ unsigned int max_hw_timeout_ms;
+ unsigned long last_keepalive;
void *driver_data;
struct mutex lock;
unsigned long status;
@@ -97,6 +107,7 @@ struct watchdog_device {
#define WDOG_ALLOW_RELEASE 2 /* Did we receive the magic char ? */
#define WDOG_NO_WAY_OUT 3 /* Is 'nowayout' feature set ? */
#define WDOG_UNREGISTERED 4 /* Has the device been unregistered */
+ struct delayed_work work;
struct list_head deferred;
};
--
2.1.4
The WDOG_RUNNING flag is expected to be set by watchdog drivers if
the hardware watchdog is running. If the flag is set, the watchdog
subsystem will ping the watchdog even if the watchdog device is closed.
The watchdog driver stop function is now optional and may be omitted
if the watchdog can not be stopped. If stopping the watchdog is not
possible but the driver implements a stop function, it is responsible
to set the WDOG_RUNNING flag in its stop function.
Cc: Timo Kokkonen <[email protected]>
Cc: Uwe Kleine-König <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
Documentation/watchdog/watchdog-kernel-api.txt | 19 ++++++++-----
drivers/watchdog/watchdog_core.c | 2 +-
drivers/watchdog/watchdog_dev.c | 39 ++++++++++++++++++++------
include/linux/watchdog.h | 7 +++++
4 files changed, 50 insertions(+), 17 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 5fa085276874..7fda3c86cf46 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -144,17 +144,18 @@ are:
device.
The routine needs a pointer to the watchdog timer device structure as a
parameter. It returns zero on success or a negative errno code for failure.
-* stop: with this routine the watchdog timer device is being stopped.
- The routine needs a pointer to the watchdog timer device structure as a
- parameter. It returns zero on success or a negative errno code for failure.
- Some watchdog timer hardware can only be started and not be stopped. The
- driver supporting this hardware needs to make sure that a start and stop
- routine is being provided. This can be done by using a timer in the driver
- that regularly sends a keepalive ping to the watchdog timer hardware.
Not all watchdog timer hardware supports the same functionality. That's why
all other routines/operations are optional. They only need to be provided if
they are supported. These optional routines/operations are:
+* stop: with this routine the watchdog timer device is being stopped.
+ The routine needs a pointer to the watchdog timer device structure as a
+ parameter. It returns zero on success or a negative errno code for failure.
+ Some watchdog timer hardware can only be started and not be stopped. A
+ driver supporting such hardware does not have to implement the stop routine.
+ If a driver has no stop function, the watchdog core will set WDOG_RUNNING and
+ start calling the driver's keepalive pings function after the watchdog device
+ is closed.
* ping: this is the routine that sends a keepalive ping to the watchdog timer
hardware.
The routine needs a pointer to the watchdog timer device structure as a
@@ -206,6 +207,10 @@ bit-operations. The status bits that are defined are:
any watchdog_ops, so that you can be sure that no operations (other then
unref) will get called after unregister, even if userspace still holds a
reference to /dev/watchdog
+* WDOG_RUNNING: Set by the watchdog driver if the hardware watchdog is running.
+ The bit must be set if the watchdog timer hardware can not be stopped;
+ otherwise it is optional. If set, the watchdog driver core will send
+ keepalive pings to the watchdog hardware while the watchdog device is closed.
To set the WDOG_NO_WAY_OUT status bit (before registering your watchdog
timer device) you can either:
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 1a8059455413..b38d1b7ae10e 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -145,7 +145,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
return -EINVAL;
/* Mandatory operations need to be supported */
- if (wdd->ops->start == NULL || wdd->ops->stop == NULL)
+ if (!wdd->ops->start)
return -EINVAL;
watchdog_check_min_max_timeout(wdd);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 25849c1d6dc1..e0fbc4ac9bb7 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -58,8 +58,9 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
unsigned int hm = wdd->max_hw_timeout_ms;
unsigned int m = wdd->max_timeout * 1000;
- return watchdog_active(wdd) && hm && hm != m &&
- wdd->timeout * 500 > hm;
+ return (watchdog_active(wdd) && hm && hm != m &&
+ wdd->timeout * 500 > hm) ||
+ (!watchdog_active(wdd) && watchdog_running(wdd));
}
static inline void watchdog_update_worker(struct watchdog_device *wdd,
@@ -87,7 +88,7 @@ static int _watchdog_ping(struct watchdog_device *wdd)
if (test_bit(WDOG_UNREGISTERED, &wdd->status))
return -ENODEV;
- if (!watchdog_active(wdd))
+ if (!watchdog_active(wdd) && !watchdog_running(wdd))
return 0;
if (wdd->ops->ping)
@@ -204,7 +205,11 @@ static int watchdog_stop(struct watchdog_device *wdd)
goto out_stop;
}
- err = wdd->ops->stop(wdd);
+ if (wdd->ops->stop)
+ err = wdd->ops->stop(wdd);
+ else
+ set_bit(WDOG_RUNNING, &wdd->status);
+
if (err == 0) {
clear_bit(WDOG_ACTIVE, &wdd->status);
watchdog_update_worker(wdd, true, false);
@@ -489,7 +494,7 @@ static int watchdog_open(struct inode *inode, struct file *file)
* If the /dev/watchdog device is open, we don't want the module
* to be unloaded.
*/
- if (!try_module_get(wdd->ops->owner))
+ if (!watchdog_running(wdd) && !try_module_get(wdd->ops->owner))
goto out;
err = watchdog_start(wdd);
@@ -546,10 +551,15 @@ static int watchdog_release(struct inode *inode, struct file *file)
watchdog_ping(wdd);
}
- cancel_delayed_work_sync(&wdd->work);
+ watchdog_update_worker(wdd, true, true);
- /* Allow the owner module to be unloaded again */
- module_put(wdd->ops->owner);
+ /*
+ * Allow the owner module to be unloaded again unless the watchdog
+ * is still running. If the watchdog is still running, it can not
+ * be stopped, and its driver must not be unloaded.
+ */
+ if (!watchdog_running(wdd))
+ module_put(wdd->ops->owner);
/* make sure that /dev/watchdog can be re-opened */
clear_bit(WDOG_DEV_OPEN, &wdd->status);
@@ -625,8 +635,19 @@ int watchdog_dev_register(struct watchdog_device *wdd)
misc_deregister(&watchdog_miscdev);
old_wdd = NULL;
}
+ return err;
}
- return err;
+
+ /*
+ * If the watchdog is running, prevent its driver from being unloaded,
+ * and schedule an immediate ping.
+ */
+ if (watchdog_running(wdd)) {
+ __module_get(wdd->ops->owner);
+ queue_delayed_work(watchdog_wq, &wdd->work, 0);
+ }
+
+ return 0;
}
/*
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 2703b2511481..b29f0181130b 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -107,6 +107,7 @@ struct watchdog_device {
#define WDOG_ALLOW_RELEASE 2 /* Did we receive the magic char ? */
#define WDOG_NO_WAY_OUT 3 /* Is 'nowayout' feature set ? */
#define WDOG_UNREGISTERED 4 /* Has the device been unregistered */
+#define WDOG_RUNNING 5 /* True if HW watchdog running */
struct delayed_work work;
struct list_head deferred;
};
@@ -120,6 +121,12 @@ static inline bool watchdog_active(struct watchdog_device *wdd)
return test_bit(WDOG_ACTIVE, &wdd->status);
}
+/* Use the following function to check whether or not the watchdog is running */
+static inline bool watchdog_running(struct watchdog_device *wdd)
+{
+ return test_bit(WDOG_RUNNING, &wdd->status);
+}
+
/* Use the following function to set the nowayout feature */
static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool nowayout)
{
--
2.1.4
For some watchdogs, the hardware timeout is fixed, and the
watchdog driver depends on the watchdog core to handle the
actual timeout. In this situation, the watchdog driver might
only set the 'timeout' variable but do nothing else.
This can as well be handled by the infrastructure, so make
the set_timeout callback optional. If WDIOF_SETTIMEOUT is
configured but the .set_timeout callback is not available,
update the timeout variable in the infrastructure code.
Signed-off-by: Guenter Roeck <[email protected]>
---
Documentation/watchdog/watchdog-kernel-api.txt | 4 ++++
drivers/watchdog/watchdog_dev.c | 9 ++++++---
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 7fda3c86cf46..2f1a4ad7e565 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -178,6 +178,10 @@ they are supported. These optional routines/operations are:
because the watchdog does not necessarily has a 1 second resolution).
(Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
watchdog's info structure).
+ If the watchdog driver does not have to perform any action but setting the
+ timeout value of the watchdog_device, this callback can be omitted.
+ If set_timeout is not provided but WDIOF_SETTIMEOUT is set, the watchdog
+ infrastructure updates the timeout value of the watchdog_device internally.
* get_timeleft: this routines returns the time that's left before a reset.
* ref: the operation that calls kref_get on the kref of a dynamically
allocated watchdog_device struct.
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index e0fbc4ac9bb7..73bae196a081 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -260,9 +260,9 @@ out_status:
static int watchdog_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)
{
- int err;
+ int err = 0;
- if (!wdd->ops->set_timeout || !(wdd->info->options & WDIOF_SETTIMEOUT))
+ if (!(wdd->info->options & WDIOF_SETTIMEOUT))
return -EOPNOTSUPP;
if (watchdog_timeout_invalid(wdd, timeout))
@@ -275,7 +275,10 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
goto out_timeout;
}
- err = wdd->ops->set_timeout(wdd, timeout);
+ if (wdd->ops->set_timeout)
+ err = wdd->ops->set_timeout(wdd, timeout);
+ else
+ wdd->timeout = timeout;
watchdog_update_worker(wdd, true, false);
--
2.1.4
The watchdog infrastructure now supports handling watchdog keepalive
if the watchdog is running while the watchdog device is closed.
Convert the driver to use this infrastructure.
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/imx2_wdt.c | 72 ++++++++-------------------------------------
1 file changed, 12 insertions(+), 60 deletions(-)
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 0bb1a1d1b170..66feef254661 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -25,7 +25,6 @@
#include <linux/delay.h>
#include <linux/init.h>
#include <linux/io.h>
-#include <linux/jiffies.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
@@ -34,7 +33,6 @@
#include <linux/platform_device.h>
#include <linux/reboot.h>
#include <linux/regmap.h>
-#include <linux/timer.h>
#include <linux/watchdog.h>
#define DRIVER_NAME "imx2-wdt"
@@ -62,7 +60,6 @@
struct imx2_wdt_device {
struct clk *clk;
struct regmap *regmap;
- struct timer_list timer; /* Pings the watchdog when closed */
struct watchdog_device wdog;
struct notifier_block restart_handler;
};
@@ -151,16 +148,6 @@ static int imx2_wdt_ping(struct watchdog_device *wdog)
return 0;
}
-static void imx2_wdt_timer_ping(unsigned long arg)
-{
- struct watchdog_device *wdog = (struct watchdog_device *)arg;
- struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
-
- /* ping it every wdog->timeout / 2 seconds to prevent reboot */
- imx2_wdt_ping(wdog);
- mod_timer(&wdev->timer, jiffies + wdog->timeout * HZ / 2);
-}
-
static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
unsigned int new_timeout)
{
@@ -177,40 +164,19 @@ static int imx2_wdt_start(struct watchdog_device *wdog)
{
struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
- if (imx2_wdt_is_running(wdev)) {
- /* delete the timer that pings the watchdog after close */
- del_timer_sync(&wdev->timer);
+ if (imx2_wdt_is_running(wdev))
imx2_wdt_set_timeout(wdog, wdog->timeout);
- } else
+ else
imx2_wdt_setup(wdog);
- return imx2_wdt_ping(wdog);
-}
-
-static int imx2_wdt_stop(struct watchdog_device *wdog)
-{
- /*
- * We don't need a clk_disable, it cannot be disabled once started.
- * We use a timer to ping the watchdog while /dev/watchdog is closed
- */
- imx2_wdt_timer_ping((unsigned long)wdog);
- return 0;
-}
-
-static inline void imx2_wdt_ping_if_active(struct watchdog_device *wdog)
-{
- struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
+ set_bit(WDOG_RUNNING, &wdog->status);
- if (imx2_wdt_is_running(wdev)) {
- imx2_wdt_set_timeout(wdog, wdog->timeout);
- imx2_wdt_timer_ping((unsigned long)wdog);
- }
+ return imx2_wdt_ping(wdog);
}
static const struct watchdog_ops imx2_wdt_ops = {
.owner = THIS_MODULE,
.start = imx2_wdt_start,
- .stop = imx2_wdt_stop,
.ping = imx2_wdt_ping,
.set_timeout = imx2_wdt_set_timeout,
};
@@ -277,9 +243,10 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
watchdog_set_nowayout(wdog, nowayout);
watchdog_init_timeout(wdog, timeout, &pdev->dev);
- setup_timer(&wdev->timer, imx2_wdt_timer_ping, (unsigned long)wdog);
-
- imx2_wdt_ping_if_active(wdog);
+ if (imx2_wdt_is_running(wdev)) {
+ imx2_wdt_set_timeout(wdog, wdog->timeout);
+ set_bit(WDOG_RUNNING, &wdog->status);
+ }
/*
* Disable the watchdog power down counter at boot. Otherwise the power
@@ -320,7 +287,6 @@ static int __exit imx2_wdt_remove(struct platform_device *pdev)
watchdog_unregister_device(wdog);
if (imx2_wdt_is_running(wdev)) {
- del_timer_sync(&wdev->timer);
imx2_wdt_ping(wdog);
dev_crit(&pdev->dev, "Device removed: Expect reboot!\n");
}
@@ -334,10 +300,9 @@ static void imx2_wdt_shutdown(struct platform_device *pdev)
if (imx2_wdt_is_running(wdev)) {
/*
- * We are running, we need to delete the timer but will
- * give max timeout before reboot will take place
+ * We are running, configure max timeout before reboot
+ * will take place.
*/
- del_timer_sync(&wdev->timer);
imx2_wdt_set_timeout(wdog, IMX2_WDT_MAX_TIME);
imx2_wdt_ping(wdog);
dev_crit(&pdev->dev, "Device shutdown: Expect reboot!\n");
@@ -355,10 +320,6 @@ static int imx2_wdt_suspend(struct device *dev)
if (imx2_wdt_is_running(wdev)) {
imx2_wdt_set_timeout(wdog, IMX2_WDT_MAX_TIME);
imx2_wdt_ping(wdog);
-
- /* The watchdog is not active */
- if (!watchdog_active(wdog))
- del_timer_sync(&wdev->timer);
}
clk_disable_unprepare(wdev->clk);
@@ -384,19 +345,10 @@ static int imx2_wdt_resume(struct device *dev)
* watchdog again.
*/
imx2_wdt_setup(wdog);
+ }
+ if (imx2_wdt_is_running(wdev)) {
imx2_wdt_set_timeout(wdog, wdog->timeout);
imx2_wdt_ping(wdog);
- } else if (imx2_wdt_is_running(wdev)) {
- /* Resuming from non-deep sleep state. */
- imx2_wdt_set_timeout(wdog, wdog->timeout);
- imx2_wdt_ping(wdog);
- /*
- * But the watchdog is not active, then start
- * the timer again.
- */
- if (!watchdog_active(wdog))
- mod_timer(&wdev->timer,
- jiffies + wdog->timeout * HZ / 2);
}
return 0;
--
2.1.4
The watchdog infrastructure now supports handling watchdog keepalive
if the watchdog is running while the watchdog device is closed.
Convert the driver to use this infrastructure.
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/retu_wdt.c | 78 ++++-----------------------------------------
1 file changed, 7 insertions(+), 71 deletions(-)
diff --git a/drivers/watchdog/retu_wdt.c b/drivers/watchdog/retu_wdt.c
index b7c68e275aeb..ce2982a7670c 100644
--- a/drivers/watchdog/retu_wdt.c
+++ b/drivers/watchdog/retu_wdt.c
@@ -28,69 +28,22 @@
/* Watchdog timer values in seconds */
#define RETU_WDT_MAX_TIMER 63
-struct retu_wdt_dev {
- struct retu_dev *rdev;
- struct device *dev;
- struct delayed_work ping_work;
-};
-
-/*
- * Since Retu watchdog cannot be disabled in hardware, we must kick it
- * with a timer until userspace watchdog software takes over. If
- * CONFIG_WATCHDOG_NOWAYOUT is set, we never start the feeding.
- */
-static void retu_wdt_ping_enable(struct retu_wdt_dev *wdev)
-{
- retu_write(wdev->rdev, RETU_REG_WATCHDOG, RETU_WDT_MAX_TIMER);
- schedule_delayed_work(&wdev->ping_work,
- round_jiffies_relative(RETU_WDT_MAX_TIMER * HZ / 2));
-}
-
-static void retu_wdt_ping_disable(struct retu_wdt_dev *wdev)
-{
- retu_write(wdev->rdev, RETU_REG_WATCHDOG, RETU_WDT_MAX_TIMER);
- cancel_delayed_work_sync(&wdev->ping_work);
-}
-
-static void retu_wdt_ping_work(struct work_struct *work)
-{
- struct retu_wdt_dev *wdev = container_of(to_delayed_work(work),
- struct retu_wdt_dev, ping_work);
- retu_wdt_ping_enable(wdev);
-}
-
static int retu_wdt_start(struct watchdog_device *wdog)
{
- struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+ struct retu_dev *rdev = watchdog_get_drvdata(wdog);
- retu_wdt_ping_disable(wdev);
+ set_bit(WDOG_RUNNING, &wdog->status);
- return retu_write(wdev->rdev, RETU_REG_WATCHDOG, wdog->timeout);
-}
-
-static int retu_wdt_stop(struct watchdog_device *wdog)
-{
- struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
-
- retu_wdt_ping_enable(wdev);
-
- return 0;
-}
-
-static int retu_wdt_ping(struct watchdog_device *wdog)
-{
- struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
-
- return retu_write(wdev->rdev, RETU_REG_WATCHDOG, wdog->timeout);
+ return retu_write(rdev, RETU_REG_WATCHDOG, wdog->timeout);
}
static int retu_wdt_set_timeout(struct watchdog_device *wdog,
unsigned int timeout)
{
- struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+ struct retu_dev *rdev = watchdog_get_drvdata(wdog);
wdog->timeout = timeout;
- return retu_write(wdev->rdev, RETU_REG_WATCHDOG, wdog->timeout);
+ return retu_write(rdev, RETU_REG_WATCHDOG, wdog->timeout);
}
static const struct watchdog_info retu_wdt_info = {
@@ -101,8 +54,6 @@ static const struct watchdog_info retu_wdt_info = {
static const struct watchdog_ops retu_wdt_ops = {
.owner = THIS_MODULE,
.start = retu_wdt_start,
- .stop = retu_wdt_stop,
- .ping = retu_wdt_ping,
.set_timeout = retu_wdt_set_timeout,
};
@@ -111,39 +62,26 @@ static int retu_wdt_probe(struct platform_device *pdev)
struct retu_dev *rdev = dev_get_drvdata(pdev->dev.parent);
bool nowayout = WATCHDOG_NOWAYOUT;
struct watchdog_device *retu_wdt;
- struct retu_wdt_dev *wdev;
int ret;
retu_wdt = devm_kzalloc(&pdev->dev, sizeof(*retu_wdt), GFP_KERNEL);
if (!retu_wdt)
return -ENOMEM;
- wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
- if (!wdev)
- return -ENOMEM;
-
retu_wdt->info = &retu_wdt_info;
retu_wdt->ops = &retu_wdt_ops;
retu_wdt->timeout = RETU_WDT_MAX_TIMER;
retu_wdt->min_timeout = 0;
retu_wdt->max_timeout = RETU_WDT_MAX_TIMER;
- watchdog_set_drvdata(retu_wdt, wdev);
+ watchdog_set_drvdata(retu_wdt, rdev);
watchdog_set_nowayout(retu_wdt, nowayout);
- wdev->rdev = rdev;
- wdev->dev = &pdev->dev;
-
- INIT_DELAYED_WORK(&wdev->ping_work, retu_wdt_ping_work);
-
ret = watchdog_register_device(retu_wdt);
if (ret < 0)
return ret;
- if (nowayout)
- retu_wdt_ping(retu_wdt);
- else
- retu_wdt_ping_enable(wdev);
+ retu_wdt_start(retu_wdt);
platform_set_drvdata(pdev, retu_wdt);
@@ -153,10 +91,8 @@ static int retu_wdt_probe(struct platform_device *pdev)
static int retu_wdt_remove(struct platform_device *pdev)
{
struct watchdog_device *wdog = platform_get_drvdata(pdev);
- struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
watchdog_unregister_device(wdog);
- cancel_delayed_work_sync(&wdev->ping_work);
return 0;
}
--
2.1.4
The watchdog infrastructure now supports handling watchdog keepalive
if the watchdog is running while the watchdog device is closed.
The infrastructure now also supports generating additional heartbeats
if the maximum hardware timeout is smaller than or close to the
configured timeout. Convert the driver to use this infrastructure.
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/gpio_wdt.c | 65 ++++++++-------------------------------------
1 file changed, 11 insertions(+), 54 deletions(-)
diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
index 1687cc2d7122..cbbdae440bfa 100644
--- a/drivers/watchdog/gpio_wdt.c
+++ b/drivers/watchdog/gpio_wdt.c
@@ -32,12 +32,8 @@ struct gpio_wdt_priv {
bool active_low;
bool state;
bool always_running;
- bool armed;
unsigned int hw_algo;
- unsigned int hw_margin;
- unsigned long last_jiffies;
struct notifier_block notifier;
- struct timer_list timer;
struct watchdog_device wdd;
};
@@ -50,20 +46,12 @@ static void gpio_wdt_disable(struct gpio_wdt_priv *priv)
gpio_direction_input(priv->gpio);
}
-static void gpio_wdt_start_impl(struct gpio_wdt_priv *priv)
-{
- priv->state = priv->active_low;
- gpio_direction_output(priv->gpio, priv->state);
- priv->last_jiffies = jiffies;
- mod_timer(&priv->timer, priv->last_jiffies + priv->hw_margin);
-}
-
static int gpio_wdt_start(struct watchdog_device *wdd)
{
struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
- gpio_wdt_start_impl(priv);
- priv->armed = true;
+ priv->state = priv->active_low;
+ gpio_direction_output(priv->gpio, priv->state);
return 0;
}
@@ -72,10 +60,9 @@ static int gpio_wdt_stop(struct watchdog_device *wdd)
{
struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
- priv->armed = false;
if (!priv->always_running) {
- mod_timer(&priv->timer, 0);
gpio_wdt_disable(priv);
+ clear_bit(WDOG_RUNNING, &priv->wdd.status);
}
return 0;
@@ -85,32 +72,6 @@ static int gpio_wdt_ping(struct watchdog_device *wdd)
{
struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
- priv->last_jiffies = jiffies;
-
- return 0;
-}
-
-static int gpio_wdt_set_timeout(struct watchdog_device *wdd, unsigned int t)
-{
- wdd->timeout = t;
-
- return gpio_wdt_ping(wdd);
-}
-
-static void gpio_wdt_hwping(unsigned long data)
-{
- struct watchdog_device *wdd = (struct watchdog_device *)data;
- struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
-
- if (priv->armed && time_after(jiffies, priv->last_jiffies +
- msecs_to_jiffies(wdd->timeout * 1000))) {
- dev_crit(wdd->dev, "Timer expired. System will reboot soon!\n");
- return;
- }
-
- /* Restart timer */
- mod_timer(&priv->timer, jiffies + priv->hw_margin);
-
switch (priv->hw_algo) {
case HW_ALGO_TOGGLE:
/* Toggle output pin */
@@ -124,6 +85,8 @@ static void gpio_wdt_hwping(unsigned long data)
gpio_set_value_cansleep(priv->gpio, priv->active_low);
break;
}
+
+ return 0;
}
static int gpio_wdt_notify_sys(struct notifier_block *nb, unsigned long code,
@@ -132,12 +95,10 @@ static int gpio_wdt_notify_sys(struct notifier_block *nb, unsigned long code,
struct gpio_wdt_priv *priv = container_of(nb, struct gpio_wdt_priv,
notifier);
- mod_timer(&priv->timer, 0);
-
switch (code) {
case SYS_HALT:
case SYS_POWER_OFF:
- gpio_wdt_disable(priv);
+ gpio_wdt_stop(&priv->wdd);
break;
default:
break;
@@ -157,7 +118,6 @@ static const struct watchdog_ops gpio_wdt_ops = {
.start = gpio_wdt_start,
.stop = gpio_wdt_stop,
.ping = gpio_wdt_ping,
- .set_timeout = gpio_wdt_set_timeout,
};
static int gpio_wdt_probe(struct platform_device *pdev)
@@ -205,9 +165,6 @@ static int gpio_wdt_probe(struct platform_device *pdev)
if (hw_margin < 2 || hw_margin > 65535)
return -EINVAL;
- /* Use safe value (1/2 of real timeout) */
- priv->hw_margin = msecs_to_jiffies(hw_margin / 2);
-
priv->always_running = of_property_read_bool(pdev->dev.of_node,
"always-running");
@@ -217,11 +174,15 @@ static int gpio_wdt_probe(struct platform_device *pdev)
priv->wdd.ops = &gpio_wdt_ops;
priv->wdd.min_timeout = SOFT_TIMEOUT_MIN;
priv->wdd.max_timeout = SOFT_TIMEOUT_MAX;
+ priv->wdd.max_hw_timeout_ms = hw_margin;
if (watchdog_init_timeout(&priv->wdd, 0, &pdev->dev) < 0)
priv->wdd.timeout = SOFT_TIMEOUT_DEF;
- setup_timer(&priv->timer, gpio_wdt_hwping, (unsigned long)&priv->wdd);
+ if (priv->always_running) {
+ set_bit(WDOG_RUNNING, &priv->wdd.status);
+ gpio_wdt_start(&priv->wdd);
+ }
ret = watchdog_register_device(&priv->wdd);
if (ret)
@@ -232,9 +193,6 @@ static int gpio_wdt_probe(struct platform_device *pdev)
if (ret)
goto error_unregister;
- if (priv->always_running)
- gpio_wdt_start_impl(priv);
-
return 0;
error_unregister:
@@ -246,7 +204,6 @@ static int gpio_wdt_remove(struct platform_device *pdev)
{
struct gpio_wdt_priv *priv = platform_get_drvdata(pdev);
- del_timer_sync(&priv->timer);
unregister_reboot_notifier(&priv->notifier);
watchdog_unregister_device(&priv->wdd);
--
2.1.4
The watchdog infrastructure now supports handling watchdog keepalive
if the watchdog is running while the watchdog device is closed.
The infrastructure now also supports generating additional heartbeats
if the maximum hardware timeout is smaller than or close to the
configured timeout. Convert the driver to use this
infrastructure.
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/at91sam9_wdt.c | 102 +++++-----------------------------------
1 file changed, 11 insertions(+), 91 deletions(-)
diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index e4698f7c5f93..0de39b52962c 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -29,7 +29,6 @@
#include <linux/types.h>
#include <linux/watchdog.h>
#include <linux/jiffies.h>
-#include <linux/timer.h>
#include <linux/bitops.h>
#include <linux/uaccess.h>
#include <linux/of.h>
@@ -48,8 +47,8 @@
* use this to convert a watchdog
* value from/to milliseconds.
*/
-#define ticks_to_hz_rounddown(t) ((((t) + 1) * HZ) >> 8)
-#define ticks_to_hz_roundup(t) (((((t) + 1) * HZ) + 255) >> 8)
+#define ticks_to_ms_rounddown(t) ((((t) + 1) * 1000) >> 8)
+#define ticks_to_ms_roundup(t) (((((t) + 1) * 1000) + 255) >> 8)
#define ticks_to_secs(t) (((t) + 1) >> 8)
#define secs_to_ticks(s) ((s) ? (((s) << 8) - 1) : 0)
@@ -64,9 +63,6 @@
/* Hardware timeout in seconds */
#define WDT_HW_TIMEOUT 2
-/* Timer heartbeat (500ms) */
-#define WDT_TIMEOUT (HZ/2)
-
/* User land timeout */
#define WDT_HEARTBEAT 15
static int heartbeat;
@@ -83,11 +79,8 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
struct at91wdt {
struct watchdog_device wdd;
void __iomem *base;
- unsigned long next_heartbeat; /* the next_heartbeat for the timer */
- struct timer_list timer; /* The timer that pings the watchdog */
u32 mr;
u32 mr_mask;
- unsigned long heartbeat; /* WDT heartbeat in jiffies */
bool nowayout;
unsigned int irq;
};
@@ -107,47 +100,13 @@ static irqreturn_t wdt_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}
-/*
- * Reload the watchdog timer. (ie, pat the watchdog)
- */
-static inline void at91_wdt_reset(struct at91wdt *wdt)
-{
- wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
-}
-
-/*
- * Timer tick
- */
-static void at91_ping(unsigned long data)
-{
- struct at91wdt *wdt = (struct at91wdt *)data;
- if (time_before(jiffies, wdt->next_heartbeat) ||
- !watchdog_active(&wdt->wdd)) {
- at91_wdt_reset(wdt);
- mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
- } else {
- pr_crit("I will reset your machine !\n");
- }
-}
-
static int at91_wdt_start(struct watchdog_device *wdd)
{
struct at91wdt *wdt = to_wdt(wdd);
- /* calculate when the next userspace timeout will be */
- wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
- return 0;
-}
-static int at91_wdt_stop(struct watchdog_device *wdd)
-{
- /* The watchdog timer hardware can not be stopped... */
- return 0;
-}
+ wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
-static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_timeout)
-{
- wdd->timeout = new_timeout;
- return at91_wdt_start(wdd);
+ return 0;
}
static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
@@ -157,8 +116,8 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
u32 value;
int err;
u32 mask = wdt->mr_mask;
- unsigned long min_heartbeat = 1;
- unsigned long max_heartbeat;
+ unsigned int min_timeout = jiffies_to_msecs(1);
+ unsigned int hw_timeout;
struct device *dev = &pdev->dev;
tmp = wdt_read(wdt, AT91_WDT_MR);
@@ -180,31 +139,15 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
delta = (tmp & AT91_WDT_WDD) >> 16;
if (delta < value)
- min_heartbeat = ticks_to_hz_roundup(value - delta);
+ min_timeout = ticks_to_ms_roundup(value - delta);
- max_heartbeat = ticks_to_hz_rounddown(value);
- if (!max_heartbeat) {
+ hw_timeout = ticks_to_ms_rounddown(value);
+ if (hw_timeout < min_timeout * 2) {
dev_err(dev,
"heartbeat is too small for the system to handle it correctly\n");
return -EINVAL;
}
-
- /*
- * Try to reset the watchdog counter 4 or 2 times more often than
- * actually requested, to avoid spurious watchdog reset.
- * If this is not possible because of the min_heartbeat value, reset
- * it at the min_heartbeat period.
- */
- if ((max_heartbeat / 4) >= min_heartbeat)
- wdt->heartbeat = max_heartbeat / 4;
- else if ((max_heartbeat / 2) >= min_heartbeat)
- wdt->heartbeat = max_heartbeat / 2;
- else
- wdt->heartbeat = min_heartbeat;
-
- if (max_heartbeat < min_heartbeat + 4)
- dev_warn(dev,
- "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
+ wdt->wdd.max_hw_timeout_ms = hw_timeout;
if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
err = request_irq(wdt->irq, wdt_interrupt,
@@ -220,32 +163,12 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
"watchdog already configured differently (mr = %x expecting %x)\n",
tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
- setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
-
- /*
- * Use min_heartbeat the first time to avoid spurious watchdog reset:
- * we don't know for how long the watchdog counter is running, and
- * - resetting it right now might trigger a watchdog fault reset
- * - waiting for heartbeat time might lead to a watchdog timeout
- * reset
- */
- mod_timer(&wdt->timer, jiffies + min_heartbeat);
-
/* Try to set timeout from device tree first */
if (watchdog_init_timeout(&wdt->wdd, 0, dev))
watchdog_init_timeout(&wdt->wdd, heartbeat, dev);
watchdog_set_nowayout(&wdt->wdd, wdt->nowayout);
- err = watchdog_register_device(&wdt->wdd);
- if (err)
- goto out_stop_timer;
-
- wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
-
- return 0;
-out_stop_timer:
- del_timer(&wdt->timer);
- return err;
+ return watchdog_register_device(&wdt->wdd);
}
/* ......................................................................... */
@@ -259,8 +182,6 @@ static const struct watchdog_info at91_wdt_info = {
static const struct watchdog_ops at91_wdt_ops = {
.owner = THIS_MODULE,
.start = at91_wdt_start,
- .stop = at91_wdt_stop,
- .set_timeout = at91_wdt_set_timeout,
};
#if defined(CONFIG_OF)
@@ -376,7 +297,6 @@ static int __exit at91wdt_remove(struct platform_device *pdev)
watchdog_unregister_device(&wdt->wdd);
pr_warn("I quit now, hardware will probably reboot!\n");
- del_timer(&wdt->timer);
return 0;
}
--
2.1.4
Hello Guenter,
On Mon, Aug 03, 2015 at 07:13:26PM -0700, Guenter Roeck wrote:
> This patch set does not solve all limitations of the watchdog subsystem.
> Specifically, it does not add support for the following features.
>
> - It is desirable to be able to specify a maximum early timeout,
> from booting the system to opening the watchdog device.
> - Some watchdogs may require a minimum period of time between
> heartbeats. Examples are DA9062 and possibly AT91SAM9x.
Other things that come to my mind:
- move handling of nowayout into the core
- some drivers use a reboot notifier. Not sure what they are intended
to do there. For i.MX2x it triggers a reboot; for gpio-wdt it tries
to stop the watchdog.
Of course this doesn't mean this should addressed here. I like the
series in general.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello Guenter,
On Mon, Aug 03, 2015 at 07:13:27PM -0700, Guenter Roeck wrote:
> The current code uses 'wdd', wddev', and 'watchdog' as variable names
> for struct watchdog_device. This is confusing and makes it difficult
> to enhance the code. Replace it all with 'wdd'.
Seems sensible.
Acked-by: Uwe Kleine-K?nig <[email protected]>
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Mon, Aug 03, 2015 at 07:13:28PM -0700, Guenter Roeck wrote:
> Introduce an optional hardware maximum timeout in the watchdog core.
> The hardware maximum timeout can be lower than the maximum timeout.
Is this only until all drivers are converted to make use of the central
worker? Otherwise this doesn't make sense, right?
> Drivers can set the maximum hardare timeout value in the watchdog data
s/hardare/hardware/
> structure. If the configured timeout exceeds half the value of the
> maximum hardware timeout, the watchdog core enables a timer function
> to assist sending keepalive requests to the watchdog driver.
I don't understand why you want to halve the maximum hw-timeout. If my
watchdog has hw-max-timeout = 5s and userspace sets it to 3s there
should be no need for assistance?! I think the implementation is the
other way round?
> ---
> Documentation/watchdog/watchdog-kernel-api.txt | 14 +++
> drivers/watchdog/watchdog_dev.c | 121 +++++++++++++++++++++----
> include/linux/watchdog.h | 21 ++++-
> 3 files changed, 135 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index d8b0d3367706..5fa085276874 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -53,9 +53,12 @@ struct watchdog_device {
> unsigned int timeout;
> unsigned int min_timeout;
> unsigned int max_timeout;
> + unsigned int max_hw_timeout_ms;
> + unsigned long last_keepalive;
> void *driver_data;
> struct mutex lock;
> unsigned long status;
> + struct delayed_work work;
> struct list_head deferred;
> };
>
> @@ -73,8 +76,18 @@ It contains following fields:
> additional information about the watchdog timer itself. (Like it's unique name)
> * ops: a pointer to the list of watchdog operations that the watchdog supports.
> * timeout: the watchdog timer's timeout value (in seconds).
> + This is the time after which the system will reboot if user space does
> + not send a heartbeat request if the watchdog device is opened.
> + This may or may not be the hardware watchdog timeout. See max_hw_timeout_ms
> + for more details.
Hmm, what is timeout then? Is this the value that the driver currently
handles? Or the framework with the automatic pings? Probably the
former?! This needs better wording.
> * min_timeout: the watchdog timer's minimum timeout value (in seconds).
> * max_timeout: the watchdog timer's maximum timeout value (in seconds).
> +* max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds. May differ
> + from max_timeout. If set, the infrastructure will send a heartbeat to the
> + watchdog driver if 'timeout' is larger than 'max_hw_timeout / 2',
> + unless user space failed to ping the watchdog for 'timeout' seconds.
In the long run max_timeout should be removed, right?
> +* last_keepalive: Time of most recent keepalive triggered from user space,
> + in jiffies.
> * bootstatus: status of the device after booting (reported with watchdog
> WDIOF_* status bits).
> * driver_data: a pointer to the drivers private data of a watchdog device.
> @@ -85,6 +98,7 @@ It contains following fields:
> information about the status of the device (Like: is the watchdog timer
> running/active, is the nowayout bit set, is the device opened via
> the /dev/watchdog interface or not, ...).
> +* work: Worker data structure for WatchDog Timer Driver Core internal use only.
> * deferred: entry in wtd_deferred_reg_list which is used to
> register early initialized watchdogs.
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 06171c73daf5..25849c1d6dc1 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -37,7 +37,9 @@
> #include <linux/errno.h> /* For the -ENODEV/... values */
> #include <linux/kernel.h> /* For printk/panic/... */
> #include <linux/fs.h> /* For file operations */
> +#include <linux/jiffies.h> /* For timeout functions */
> #include <linux/watchdog.h> /* For watchdog specific items */
> +#include <linux/workqueue.h> /* For workqueue */
> #include <linux/miscdevice.h> /* For handling misc devices */
> #include <linux/init.h> /* For __init/__exit/... */
> #include <linux/uaccess.h> /* For copy_to_user/put_user/... */
> @@ -49,6 +51,53 @@ static dev_t watchdog_devt;
> /* the watchdog device behind /dev/watchdog */
> static struct watchdog_device *old_wdd;
>
> +static struct workqueue_struct *watchdog_wq;
> +
> +static inline bool watchdog_need_worker(struct watchdog_device *wdd)
> +{
> + unsigned int hm = wdd->max_hw_timeout_ms;
> + unsigned int m = wdd->max_timeout * 1000;
> +
> + return watchdog_active(wdd) && hm && hm != m &&
> + wdd->timeout * 500 > hm;
I don't understand what max_timeout is now that there is max_hw_timeout.
So I don't understand why you need hm != m either.
Taking the example from above (hw-maxtimeout = 5000ms, current timeout =
3s) this doesn't trigger.
And the other way round:
- hw-max-timeout = 3s
- timeout = 5s
In this case userspace might send a ping only after 4 seconds, but
watchdog_need_worker will be false.
What is the meaning of WDOG_ACTIVE now? does it mean userspace has the
device open? Then this looks wrong, too.
/me wonders if he understood that function correctly?!
> @@ -88,6 +96,8 @@ struct watchdog_device {
> unsigned int timeout;
> unsigned int min_timeout;
> unsigned int max_timeout;
> + unsigned int max_hw_timeout_ms;
> + unsigned long last_keepalive;
> void *driver_data;
> struct mutex lock;
> unsigned long status;
It would be nice to group this a bit to make it more clear which members
are supposed to be set by driver and which are not.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Mon, Aug 03, 2015 at 07:13:29PM -0700, Guenter Roeck wrote:
> The WDOG_RUNNING flag is expected to be set by watchdog drivers if
> the hardware watchdog is running. If the flag is set, the watchdog
> subsystem will ping the watchdog even if the watchdog device is closed.
>
> The watchdog driver stop function is now optional and may be omitted
> if the watchdog can not be stopped. If stopping the watchdog is not
> possible but the driver implements a stop function, it is responsible
> to set the WDOG_RUNNING flag in its stop function.
>
> Cc: Timo Kokkonen <[email protected]>
> Cc: Uwe Kleine-K?nig <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> Documentation/watchdog/watchdog-kernel-api.txt | 19 ++++++++-----
> drivers/watchdog/watchdog_core.c | 2 +-
> drivers/watchdog/watchdog_dev.c | 39 ++++++++++++++++++++------
> include/linux/watchdog.h | 7 +++++
> 4 files changed, 50 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index 5fa085276874..7fda3c86cf46 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -144,17 +144,18 @@ are:
> device.
> The routine needs a pointer to the watchdog timer device structure as a
> parameter. It returns zero on success or a negative errno code for failure.
> -* stop: with this routine the watchdog timer device is being stopped.
> - The routine needs a pointer to the watchdog timer device structure as a
> - parameter. It returns zero on success or a negative errno code for failure.
> - Some watchdog timer hardware can only be started and not be stopped. The
> - driver supporting this hardware needs to make sure that a start and stop
> - routine is being provided. This can be done by using a timer in the driver
> - that regularly sends a keepalive ping to the watchdog timer hardware.
>
> Not all watchdog timer hardware supports the same functionality. That's why
> all other routines/operations are optional. They only need to be provided if
> they are supported. These optional routines/operations are:
> +* stop: with this routine the watchdog timer device is being stopped.
> + The routine needs a pointer to the watchdog timer device structure as a
> + parameter. It returns zero on success or a negative errno code for failure.
> + Some watchdog timer hardware can only be started and not be stopped. A
> + driver supporting such hardware does not have to implement the stop routine.
> + If a driver has no stop function, the watchdog core will set WDOG_RUNNING and
> + start calling the driver's keepalive pings function after the watchdog device
> + is closed.
closed here means stop-closed. I'd like to have that more explicit. When
there is a stop function (for whatever reasons) for a non-stoppable
device, the .stop callback should return 0, right? Make this explicit.
> * ping: this is the routine that sends a keepalive ping to the watchdog timer
> hardware.
> The routine needs a pointer to the watchdog timer device structure as a
> @@ -206,6 +207,10 @@ bit-operations. The status bits that are defined are:
> any watchdog_ops, so that you can be sure that no operations (other then
> unref) will get called after unregister, even if userspace still holds a
> reference to /dev/watchdog
> +* WDOG_RUNNING: Set by the watchdog driver if the hardware watchdog is running.
> + The bit must be set if the watchdog timer hardware can not be stopped;
This is ambigous. On i.MX2x the watchdog timer cannot be stopped, but
reset-default is off. So at probe (assuming the timer is off) the bit is
not supposed to be set.
> + otherwise it is optional. If set, the watchdog driver core will send
> + keepalive pings to the watchdog hardware while the watchdog device is closed.
s/closed/stopped by userspace/
> To set the WDOG_NO_WAY_OUT status bit (before registering your watchdog
> timer device) you can either:
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 2703b2511481..b29f0181130b 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -107,6 +107,7 @@ struct watchdog_device {
> #define WDOG_ALLOW_RELEASE 2 /* Did we receive the magic char ? */
> #define WDOG_NO_WAY_OUT 3 /* Is 'nowayout' feature set ? */
> #define WDOG_UNREGISTERED 4 /* Has the device been unregistered */
> +#define WDOG_RUNNING 5 /* True if HW watchdog running */
> struct delayed_work work;
> struct list_head deferred;
> };
> @@ -120,6 +121,12 @@ static inline bool watchdog_active(struct watchdog_device *wdd)
> return test_bit(WDOG_ACTIVE, &wdd->status);
> }
>
> +/* Use the following function to check whether or not the watchdog is running */
Maybe make it more explict here that this flag is about hw-state while
watchdog_active above is about userspace view.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Uwe,
On 08/04/2015 04:24 AM, Uwe Kleine-K?nig wrote:
> Hello Guenter,
>
> On Mon, Aug 03, 2015 at 07:13:26PM -0700, Guenter Roeck wrote:
>> This patch set does not solve all limitations of the watchdog subsystem.
>> Specifically, it does not add support for the following features.
>>
>> - It is desirable to be able to specify a maximum early timeout,
>> from booting the system to opening the watchdog device.
>> - Some watchdogs may require a minimum period of time between
>> heartbeats. Examples are DA9062 and possibly AT91SAM9x.
>
> Other things that come to my mind:
> - move handling of nowayout into the core
> - some drivers use a reboot notifier. Not sure what they are intended
> to do there. For i.MX2x it triggers a reboot; for gpio-wdt it tries
> to stop the watchdog.
>
Be careful - those are different functions. The imx2 driver installs
a _restart_ handler, which indeed is supposed to restart the system.
The gpio wdt driver installs a _reboot_ notifier, which is supposed to
stop the watchdog. Other drivers install a shutdown callback in the
platform driver structure to do the same; not really sure what is better.
> Of course this doesn't mean this should addressed here. I like the
> series in general.
>
The list wasn't supposed to be exhaustive. Otherwise adding "support devm"
should have been there as well. I even have a patch hanging around somewhere
to do that. Guess I should have said something like "does not solve all
timeout related limitations ...".
Thanks,
Guenter
Hi Uwe,
On 08/04/2015 05:18 AM, Uwe Kleine-K?nig wrote:
> On Mon, Aug 03, 2015 at 07:13:28PM -0700, Guenter Roeck wrote:
>> Introduce an optional hardware maximum timeout in the watchdog core.
>> The hardware maximum timeout can be lower than the maximum timeout.
> Is this only until all drivers are converted to make use of the central
> worker? Otherwise this doesn't make sense, right?
>
>> Drivers can set the maximum hardare timeout value in the watchdog data
> s/hardare/hardware/
>
Always those fat fingers ;-)
>> structure. If the configured timeout exceeds half the value of the
>> maximum hardware timeout, the watchdog core enables a timer function
>> to assist sending keepalive requests to the watchdog driver.
> I don't understand why you want to halve the maximum hw-timeout. If my
> watchdog has hw-max-timeout = 5s and userspace sets it to 3s there
> should be no need for assistance?! I think the implementation is the
> other way round?
>
It is supposed to reflect the _maximum_ timeout. That is different to
the time between heartbeats, which is supposed to be less; using half
the value of the maximum hardware timeout seemed to be a safe number.
It is supposed to be a constant after initialization and should not change
afterwards if the (soft) timeout is changed. Not sure how to explain
that better.
>> ---
>> Documentation/watchdog/watchdog-kernel-api.txt | 14 +++
>> drivers/watchdog/watchdog_dev.c | 121 +++++++++++++++++++++----
>> include/linux/watchdog.h | 21 ++++-
>> 3 files changed, 135 insertions(+), 21 deletions(-)
>>
>> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
>> index d8b0d3367706..5fa085276874 100644
>> --- a/Documentation/watchdog/watchdog-kernel-api.txt
>> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
>> @@ -53,9 +53,12 @@ struct watchdog_device {
>> unsigned int timeout;
>> unsigned int min_timeout;
>> unsigned int max_timeout;
>> + unsigned int max_hw_timeout_ms;
>> + unsigned long last_keepalive;
>> void *driver_data;
>> struct mutex lock;
>> unsigned long status;
>> + struct delayed_work work;
>> struct list_head deferred;
>> };
>>
>> @@ -73,8 +76,18 @@ It contains following fields:
>> additional information about the watchdog timer itself. (Like it's unique name)
>> * ops: a pointer to the list of watchdog operations that the watchdog supports.
>> * timeout: the watchdog timer's timeout value (in seconds).
>> + This is the time after which the system will reboot if user space does
>> + not send a heartbeat request if the watchdog device is opened.
>> + This may or may not be the hardware watchdog timeout. See max_hw_timeout_ms
>> + for more details.
> Hmm, what is timeout then? Is this the value that the driver currently
> handles? Or the framework with the automatic pings? Probably the
> former?! This needs better wording.
>
As I say above, "This is the time after which the system will reboot if user
space does not send a heartbeat request if the watchdog device is opened".
Not sure how to express that better. Any idea ?
>> * min_timeout: the watchdog timer's minimum timeout value (in seconds).
>> * max_timeout: the watchdog timer's maximum timeout value (in seconds).
>> +* max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds. May differ
>> + from max_timeout. If set, the infrastructure will send a heartbeat to the
>> + watchdog driver if 'timeout' is larger than 'max_hw_timeout / 2',
>> + unless user space failed to ping the watchdog for 'timeout' seconds.
> In the long run max_timeout should be removed, right?
>
It could be removed, yes, though that would require each of the drivers
to set max_hw_timeout_ms. That is a much larger task, though, and might be
difficult to accomplish since each driver handles it differently (in some
cases it is a hard limit, in others it is an arbitrary number).
>> +* last_keepalive: Time of most recent keepalive triggered from user space,
>> + in jiffies.
>> * bootstatus: status of the device after booting (reported with watchdog
>> WDIOF_* status bits).
>> * driver_data: a pointer to the drivers private data of a watchdog device.
>> @@ -85,6 +98,7 @@ It contains following fields:
>> information about the status of the device (Like: is the watchdog timer
>> running/active, is the nowayout bit set, is the device opened via
>> the /dev/watchdog interface or not, ...).
>> +* work: Worker data structure for WatchDog Timer Driver Core internal use only.
>> * deferred: entry in wtd_deferred_reg_list which is used to
>> register early initialized watchdogs.
>>
>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
>> index 06171c73daf5..25849c1d6dc1 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -37,7 +37,9 @@
>> #include <linux/errno.h> /* For the -ENODEV/... values */
>> #include <linux/kernel.h> /* For printk/panic/... */
>> #include <linux/fs.h> /* For file operations */
>> +#include <linux/jiffies.h> /* For timeout functions */
>> #include <linux/watchdog.h> /* For watchdog specific items */
>> +#include <linux/workqueue.h> /* For workqueue */
>> #include <linux/miscdevice.h> /* For handling misc devices */
>> #include <linux/init.h> /* For __init/__exit/... */
>> #include <linux/uaccess.h> /* For copy_to_user/put_user/... */
>> @@ -49,6 +51,53 @@ static dev_t watchdog_devt;
>> /* the watchdog device behind /dev/watchdog */
>> static struct watchdog_device *old_wdd;
>>
>> +static struct workqueue_struct *watchdog_wq;
>> +
>> +static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>> +{
>> + unsigned int hm = wdd->max_hw_timeout_ms;
>> + unsigned int m = wdd->max_timeout * 1000;
>> +
>> + return watchdog_active(wdd) && hm && hm != m &&
>> + wdd->timeout * 500 > hm;
>
> I don't understand what max_timeout is now that there is max_hw_timeout.
> So I don't understand why you need hm != m either.
>
Backward compatibility. A driver which does not set max_hw_timeout_ms,
or sets both to the same value, by definition expects to handle everything
internally, and thus no worker is configured.
> Taking the example from above (hw-maxtimeout = 5000ms, current timeout =
> 3s) this doesn't trigger.
>
This is intentional. The idea here is that the driver set max_timeout
(here to a low value), and thus doesn't expect additional internal
heartbeats generated from the kernel. In the above example, user space
would be expected to send heartbeats after less than 3s, which does
not require kernel assistance. Even if the current timeout is set to 5s,
user space would be expected to send heartbeats much more often than that,
say every 2 or 3 seconds. Again, this does not require kernel assistance.
> And the other way round:
> - hw-max-timeout = 3s
> - timeout = 5s
>
> In this case userspace might send a ping only after 4 seconds, but
> watchdog_need_worker will be false.
>
Yep, that is wrong. The condition should be
wdd->timeout * 1000 > hm
to trigger internal heartbeats every 1.5 seconds.
> What is the meaning of WDOG_ACTIVE now? does it mean userspace has the
> device open? Then this looks wrong, too.
>
Yes, it is, and always was. Why is that wrong ? It indicates if the
keepalive worker needs to run or not, and in this state it won't need
to run if the watchdog is not active (that state is added in the next patch).
> /me wonders if he understood that function correctly?!
>
>> @@ -88,6 +96,8 @@ struct watchdog_device {
>> unsigned int timeout;
>> unsigned int min_timeout;
>> unsigned int max_timeout;
>> + unsigned int max_hw_timeout_ms;
>> + unsigned long last_keepalive;
>> void *driver_data;
>> struct mutex lock;
>> unsigned long status;
> It would be nice to group this a bit to make it more clear which members
> are supposed to be set by driver and which are not.
>
Good idea. I'll do that.
Thanks,
Guenter
Hello,
On Mon, Aug 03, 2015 at 07:13:30PM -0700, Guenter Roeck wrote:
> For some watchdogs, the hardware timeout is fixed, and the
> watchdog driver depends on the watchdog core to handle the
> actual timeout. In this situation, the watchdog driver might
> only set the 'timeout' variable but do nothing else.
> This can as well be handled by the infrastructure, so make
> the set_timeout callback optional. If WDIOF_SETTIMEOUT is
> configured but the .set_timeout callback is not available,
> update the timeout variable in the infrastructure code.
>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> Documentation/watchdog/watchdog-kernel-api.txt | 4 ++++
> drivers/watchdog/watchdog_dev.c | 9 ++++++---
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index 7fda3c86cf46..2f1a4ad7e565 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -178,6 +178,10 @@ they are supported. These optional routines/operations are:
> because the watchdog does not necessarily has a 1 second resolution).
> (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
> watchdog's info structure).
> + If the watchdog driver does not have to perform any action but setting the
> + timeout value of the watchdog_device, this callback can be omitted.
> + If set_timeout is not provided but WDIOF_SETTIMEOUT is set, the watchdog
> + infrastructure updates the timeout value of the watchdog_device internally.
What is the semantic of struct watchdog_device.timeout? Is it the
corrently configured hw-timeout? Or what userspace sees? In the former
case timeout shouldn't be updated. And in the latter case it's wrong
that the worker thread uses this member to determine the needed rate for
it's auto pinging.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello,
On Mon, Aug 03, 2015 at 07:13:29PM -0700, Guenter Roeck wrote:
> The WDOG_RUNNING flag is expected to be set by watchdog drivers if
> the hardware watchdog is running. If the flag is set, the watchdog
> subsystem will ping the watchdog even if the watchdog device is closed.
>
> The watchdog driver stop function is now optional and may be omitted
> if the watchdog can not be stopped. If stopping the watchdog is not
> possible but the driver implements a stop function, it is responsible
> to set the WDOG_RUNNING flag in its stop function.
>
> Cc: Timo Kokkonen <[email protected]>
> Cc: Uwe Kleine-K?nig <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> Documentation/watchdog/watchdog-kernel-api.txt | 19 ++++++++-----
> drivers/watchdog/watchdog_core.c | 2 +-
> drivers/watchdog/watchdog_dev.c | 39 ++++++++++++++++++++------
> include/linux/watchdog.h | 7 +++++
> 4 files changed, 50 insertions(+), 17 deletions(-)
Another thing that I noticed just now after looking at a later patch in
this series: Conceptually that worker stuff better fits into
watchdog_core.c than watchdog_dev.c, doesn't it? But maybe this
separation doesn't make sense anyhow?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello,
On Mon, Aug 03, 2015 at 07:13:31PM -0700, Guenter Roeck wrote:
> The watchdog infrastructure now supports handling watchdog keepalive
> if the watchdog is running while the watchdog device is closed.
> Convert the driver to use this infrastructure.
>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> drivers/watchdog/imx2_wdt.c | 72 ++++++++-------------------------------------
I tested the series on an i.MX27 machine and it works nicely. When the
watchdog is already running at probe time it is constantly pinged until
userspace opens /dev/watchdog. With the patch I sent earlier today the
functionallity is also given, that is the machine resets iff I stop
pinging. And when I do a stopping-close the worker looks for this in
hardware non-stoppable device.
Very nice, and also \o/ for the diffstat,
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Tue, Aug 04, 2015 at 08:31:43AM -0700, Guenter Roeck wrote:
> Hi Uwe,
>
> On 08/04/2015 05:18 AM, Uwe Kleine-K?nig wrote:
> >On Mon, Aug 03, 2015 at 07:13:28PM -0700, Guenter Roeck wrote:
> >>Introduce an optional hardware maximum timeout in the watchdog core.
> >>The hardware maximum timeout can be lower than the maximum timeout.
> >Is this only until all drivers are converted to make use of the central
> >worker? Otherwise this doesn't make sense, right?
> >
> >>Drivers can set the maximum hardare timeout value in the watchdog data
> >s/hardare/hardware/
> >
> Always those fat fingers ;-)
>
> >>structure. If the configured timeout exceeds half the value of the
> >>maximum hardware timeout, the watchdog core enables a timer function
> >>to assist sending keepalive requests to the watchdog driver.
> >I don't understand why you want to halve the maximum hw-timeout. If my
> >watchdog has hw-max-timeout = 5s and userspace sets it to 3s there
> >should be no need for assistance?! I think the implementation is the
> >other way round?
> >
> It is supposed to reflect the _maximum_ timeout. That is different to
> the time between heartbeats, which is supposed to be less; using half
> the value of the maximum hardware timeout seemed to be a safe number.
Right, I got that. With hw-max-timeout = 5s the machine resets after 5s
not caring for the device. And so pinging repeatedly after 2.5s is fine.
But if userspace sets a timeout of 3s (probably with the intention to
ping with a frequency of 1/1.5s) there is no need for worker-assistance,
because the pings coming in each 1.5s provided by userspace are good
enough.
> >>+static inline bool watchdog_need_worker(struct watchdog_device *wdd)
> >>+{
> >>+ unsigned int hm = wdd->max_hw_timeout_ms;
> >>+ unsigned int m = wdd->max_timeout * 1000;
> >>+
> >>+ return watchdog_active(wdd) && hm && hm != m &&
> >>+ wdd->timeout * 500 > hm;
> >
> >I don't understand what max_timeout is now that there is max_hw_timeout.
> >So I don't understand why you need hm != m either.
> >
>
> Backward compatibility. A driver which does not set max_hw_timeout_ms,
> or sets both to the same value, by definition expects to handle everything
> internally, and thus no worker is configured.
And a driver that does
max_timeout = 5
max_hw_timeout = 5125
falls through the cracks.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Uwe,
On 08/04/2015 08:41 AM, Uwe Kleine-K?nig wrote:
> Hello,
>
> On Mon, Aug 03, 2015 at 07:13:29PM -0700, Guenter Roeck wrote:
>> The WDOG_RUNNING flag is expected to be set by watchdog drivers if
>> the hardware watchdog is running. If the flag is set, the watchdog
>> subsystem will ping the watchdog even if the watchdog device is closed.
>>
>> The watchdog driver stop function is now optional and may be omitted
>> if the watchdog can not be stopped. If stopping the watchdog is not
>> possible but the driver implements a stop function, it is responsible
>> to set the WDOG_RUNNING flag in its stop function.
>>
>> Cc: Timo Kokkonen <[email protected]>
>> Cc: Uwe Kleine-K?nig <[email protected]>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>> Documentation/watchdog/watchdog-kernel-api.txt | 19 ++++++++-----
>> drivers/watchdog/watchdog_core.c | 2 +-
>> drivers/watchdog/watchdog_dev.c | 39 ++++++++++++++++++++------
>> include/linux/watchdog.h | 7 +++++
>> 4 files changed, 50 insertions(+), 17 deletions(-)
> Another thing that I noticed just now after looking at a later patch in
> this series: Conceptually that worker stuff better fits into
> watchdog_core.c than watchdog_dev.c, doesn't it? But maybe this
> separation doesn't make sense anyhow?
>
I actually started with that approach.
Problem is that the functionality added by the patch set is to a large degree
associated with code in watchdog_dev.c. I would have to export static functions
from watchdog_dev.c (such as _watchdog_ping), and I would have to export some
of the worker functions (such as watchdog_update_worker) if the code would be
in watchdog_core.c.
On the other side, when adding the code to watchdog_dev.c, I did not have to
export any functions. So adding the code there seemed to make more sense to me.
Thanks,
Guenter
Hi Uwe,
On 08/04/2015 08:52 AM, Uwe Kleine-K?nig wrote:
> On Tue, Aug 04, 2015 at 08:31:43AM -0700, Guenter Roeck wrote:
>> Hi Uwe,
>>
>> On 08/04/2015 05:18 AM, Uwe Kleine-K?nig wrote:
>>> On Mon, Aug 03, 2015 at 07:13:28PM -0700, Guenter Roeck wrote:
>>>> Introduce an optional hardware maximum timeout in the watchdog core.
>>>> The hardware maximum timeout can be lower than the maximum timeout.
>>> Is this only until all drivers are converted to make use of the central
>>> worker? Otherwise this doesn't make sense, right?
>>>
>>>> Drivers can set the maximum hardare timeout value in the watchdog data
>>> s/hardare/hardware/
>>>
>> Always those fat fingers ;-)
>>
>>>> structure. If the configured timeout exceeds half the value of the
>>>> maximum hardware timeout, the watchdog core enables a timer function
>>>> to assist sending keepalive requests to the watchdog driver.
>>> I don't understand why you want to halve the maximum hw-timeout. If my
>>> watchdog has hw-max-timeout = 5s and userspace sets it to 3s there
>>> should be no need for assistance?! I think the implementation is the
>>> other way round?
>>>
>> It is supposed to reflect the _maximum_ timeout. That is different to
>> the time between heartbeats, which is supposed to be less; using half
>> the value of the maximum hardware timeout seemed to be a safe number.
> Right, I got that. With hw-max-timeout = 5s the machine resets after 5s
> not caring for the device. And so pinging repeatedly after 2.5s is fine.
> But if userspace sets a timeout of 3s (probably with the intention to
> ping with a frequency of 1/1.5s) there is no need for worker-assistance,
> because the pings coming in each 1.5s provided by userspace are good
> enough.
>
Yes, that is how it is supposed to work.
>>>> +static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>>>> +{
>>>> + unsigned int hm = wdd->max_hw_timeout_ms;
>>>> + unsigned int m = wdd->max_timeout * 1000;
>>>> +
>>>> + return watchdog_active(wdd) && hm && hm != m &&
>>>> + wdd->timeout * 500 > hm;
>>>
>>> I don't understand what max_timeout is now that there is max_hw_timeout.
>>> So I don't understand why you need hm != m either.
>>>
>>
>> Backward compatibility. A driver which does not set max_hw_timeout_ms,
>> or sets both to the same value, by definition expects to handle everything
>> internally, and thus no worker is configured.
> And a driver that does
>
> max_timeout = 5
> max_hw_timeout = 5125
>
> falls through the cracks.
>
Hmm - not that this configuration makes any sense, but you are right.
I'll make it "hm < m".
Thanks,
Guenter
Hi Uwe,
On 08/04/2015 08:38 AM, Uwe Kleine-K?nig wrote:
> Hello,
>
> On Mon, Aug 03, 2015 at 07:13:30PM -0700, Guenter Roeck wrote:
>> For some watchdogs, the hardware timeout is fixed, and the
>> watchdog driver depends on the watchdog core to handle the
>> actual timeout. In this situation, the watchdog driver might
>> only set the 'timeout' variable but do nothing else.
>> This can as well be handled by the infrastructure, so make
>> the set_timeout callback optional. If WDIOF_SETTIMEOUT is
>> configured but the .set_timeout callback is not available,
>> update the timeout variable in the infrastructure code.
>>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>> Documentation/watchdog/watchdog-kernel-api.txt | 4 ++++
>> drivers/watchdog/watchdog_dev.c | 9 ++++++---
>> 2 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
>> index 7fda3c86cf46..2f1a4ad7e565 100644
>> --- a/Documentation/watchdog/watchdog-kernel-api.txt
>> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
>> @@ -178,6 +178,10 @@ they are supported. These optional routines/operations are:
>> because the watchdog does not necessarily has a 1 second resolution).
>> (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
>> watchdog's info structure).
>> + If the watchdog driver does not have to perform any action but setting the
>> + timeout value of the watchdog_device, this callback can be omitted.
>> + If set_timeout is not provided but WDIOF_SETTIMEOUT is set, the watchdog
>> + infrastructure updates the timeout value of the watchdog_device internally.
> What is the semantic of struct watchdog_device.timeout? Is it the
> corrently configured hw-timeout? Or what userspace sees? In the former
> case timeout shouldn't be updated. And in the latter case it's wrong
> that the worker thread uses this member to determine the needed rate for
> it's auto pinging.
>
.timeout is what user-space sees, but it is also what may be configured
into the hardware, depending on the value of max_hw_timeout and on the driver.
For example, if timeout = 5s and max_hw_timeout = 6000ms, the driver would (or might)
configure the _current_ hardware timeout to 5s. The worker would not be activated.
If, on the other side, timeout = 10s and max_hw_timeout = 6000ms, the driver would
configure the hardware timeout to 6s, the worker would send a heartbeat every
3 seconds, and user space would be expected to send a heartbeat every 5 seconds
or so (assuming it uses timeout/2 to determine when to send heartbeats).
Thanks,
Guenter
On 04/08/15 03:13, Guenter Roeck wrote:
> The watchdog infrastructure is currently purely passive, meaning
> it only passes information from user space to drivers and vice versa.
>
> Since watchdog hardware tends to have its own quirks, this can result
> in quite complex watchdog drivers. A number of scanarios are especially common.
>
> - A watchdog is always active and can not be disabled, or can not be disabled
> once enabled. To support such hardware, watchdog drivers have to implement
> their own timers and use those timers to trigger watchdog keepalives while
> the watchdog device is not or not yet opened.
> - A variant of this is the desire to enable a watchdog as soon as its driver
> has been instantiated, to protect the system while it is still booting up,
> but the watchdog daemon is not yet running.
Just mentioning that patting the watchdog in the boot loader
(by patching grub etc.) can be a more general solution here as it
avoids hangs if the kernel crashes before it runs the watchdog driver,
which is especially true if PXE loaded across the net for example.
Also this tends to be better spaced between boot start and user space loading.
> - Some watchdogs have a very short maximum timeout, in the range of just a few
> seconds. Such low timeouts are difficult if not impossible to support from
> user space. Drivers supporting such watchdog hardware need to implement
> a timer function to augment heartbeats from user space.
Fair enough.
thanks,
Pádraig.
On 08/04/2015 04:43 PM, Pádraig Brady wrote:
> On 04/08/15 03:13, Guenter Roeck wrote:
>> The watchdog infrastructure is currently purely passive, meaning
>> it only passes information from user space to drivers and vice versa.
>>
>> Since watchdog hardware tends to have its own quirks, this can result
>> in quite complex watchdog drivers. A number of scanarios are especially common.
>>
>> - A watchdog is always active and can not be disabled, or can not be disabled
>> once enabled. To support such hardware, watchdog drivers have to implement
>> their own timers and use those timers to trigger watchdog keepalives while
>> the watchdog device is not or not yet opened.
>> - A variant of this is the desire to enable a watchdog as soon as its driver
>> has been instantiated, to protect the system while it is still booting up,
>> but the watchdog daemon is not yet running.
>
> Just mentioning that patting the watchdog in the boot loader
> (by patching grub etc.) can be a more general solution here as it
> avoids hangs if the kernel crashes before it runs the watchdog driver,
> which is especially true if PXE loaded across the net for example.
> Also this tends to be better spaced between boot start and user space loading.
>
I understand. However, that is not always sufficient, and it may not work
well since grub would need to know about the actual watchdog hardware.
Also, there are systems where the time between "watchdog driver instantiated"
and "watchdog daemon started" is just too large for the maximum watchdog
hardware timeout.
The point here is that there _are_ many drivers which implement this
functionality in the driver code. Not counting the drivers I converted as an
exercise, "git grep mod_timer | cut -f1 -d: | sort -u | wc" in drivers/watchdog
suggests that there are 20 more drivers implementing their own heartbeat
management. With that, it just makes sense to move the functionality to
the infrastructure.
Thanks,
Guenter
Hello P?draig,
On Wed, Aug 05, 2015 at 12:43:39AM +0100, P?draig Brady wrote:
> On 04/08/15 03:13, Guenter Roeck wrote:
> > The watchdog infrastructure is currently purely passive, meaning
> > it only passes information from user space to drivers and vice versa.
> >
> > Since watchdog hardware tends to have its own quirks, this can result
> > in quite complex watchdog drivers. A number of scanarios are especially common.
> >
> > - A watchdog is always active and can not be disabled, or can not be disabled
> > once enabled. To support such hardware, watchdog drivers have to implement
> > their own timers and use those timers to trigger watchdog keepalives while
> > the watchdog device is not or not yet opened.
> > - A variant of this is the desire to enable a watchdog as soon as its driver
> > has been instantiated, to protect the system while it is still booting up,
> > but the watchdog daemon is not yet running.
>
> Just mentioning that patting the watchdog in the boot loader
> (by patching grub etc.) can be a more general solution here as it
> avoids hangs if the kernel crashes before it runs the watchdog driver,
> which is especially true if PXE loaded across the net for example.
> Also this tends to be better spaced between boot start and user space loading.
the watchdog I'm currently working with on a powerpc platform has a
unchangable timeout of ~1 s. To make the machine boot I patched the
bootloader and need some automatic pinging in the kernel before
userspace takes over.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Uwe,
On 08/05/2015 12:36 AM, Uwe Kleine-K?nig wrote:
> Hello P?draig,
>
> On Wed, Aug 05, 2015 at 12:43:39AM +0100, P?draig Brady wrote:
>> On 04/08/15 03:13, Guenter Roeck wrote:
>>> The watchdog infrastructure is currently purely passive, meaning
>>> it only passes information from user space to drivers and vice versa.
>>>
>>> Since watchdog hardware tends to have its own quirks, this can result
>>> in quite complex watchdog drivers. A number of scanarios are especially common.
>>>
>>> - A watchdog is always active and can not be disabled, or can not be disabled
>>> once enabled. To support such hardware, watchdog drivers have to implement
>>> their own timers and use those timers to trigger watchdog keepalives while
>>> the watchdog device is not or not yet opened.
>>> - A variant of this is the desire to enable a watchdog as soon as its driver
>>> has been instantiated, to protect the system while it is still booting up,
>>> but the watchdog daemon is not yet running.
>>
>> Just mentioning that patting the watchdog in the boot loader
>> (by patching grub etc.) can be a more general solution here as it
>> avoids hangs if the kernel crashes before it runs the watchdog driver,
>> which is especially true if PXE loaded across the net for example.
>> Also this tends to be better spaced between boot start and user space loading.
>
> the watchdog I'm currently working with on a powerpc platform has a
> unchangable timeout of ~1 s. To make the machine boot I patched the
> bootloader and need some automatic pinging in the kernel before
> userspace takes over.
>
Does using arch_initcall in the watchdog driver help here, or is that
still too slow and you really need to hack the kernel ?
Thanks,
Guenter
Hello Guenter,
On Tue, Aug 04, 2015 at 09:03:27AM -0700, Guenter Roeck wrote:
> On 08/04/2015 08:52 AM, Uwe Kleine-K?nig wrote:
> >On Tue, Aug 04, 2015 at 08:31:43AM -0700, Guenter Roeck wrote:
> >>On 08/04/2015 05:18 AM, Uwe Kleine-K?nig wrote:
> >>>On Mon, Aug 03, 2015 at 07:13:28PM -0700, Guenter Roeck wrote:
> >>>>structure. If the configured timeout exceeds half the value of the
> >>>>maximum hardware timeout, the watchdog core enables a timer function
> >>>>to assist sending keepalive requests to the watchdog driver.
> >>>I don't understand why you want to halve the maximum hw-timeout. If my
> >>>watchdog has hw-max-timeout = 5s and userspace sets it to 3s there
> >>>should be no need for assistance?! I think the implementation is the
> >>>other way round?
> >>>
> >>It is supposed to reflect the _maximum_ timeout. That is different to
> >>the time between heartbeats, which is supposed to be less; using half
> >>the value of the maximum hardware timeout seemed to be a safe number.
> >Right, I got that. With hw-max-timeout = 5s the machine resets after 5s
> >not caring for the device. And so pinging repeatedly after 2.5s is fine.
> >But if userspace sets a timeout of 3s (probably with the intention to
> >ping with a frequency of 1/1.5s) there is no need for worker-assistance,
> >because the pings coming in each 1.5s provided by userspace are good
> >enough.
> >
> Yes, that is how it is supposed to work.
So for the changelog you want:
If the configured timeout exceeds the maximum hardware timeout
the watchdog core enables a timer function ...
right?
> >>>>+static inline bool watchdog_need_worker(struct watchdog_device *wdd)
> >>>>+{
> >>>>+ unsigned int hm = wdd->max_hw_timeout_ms;
> >>>>+ unsigned int m = wdd->max_timeout * 1000;
> >>>>+
> >>>>+ return watchdog_active(wdd) && hm && hm != m &&
> >>>>+ wdd->timeout * 500 > hm;
One problem with the worker I see is that the reset will probably be
delayed with your worker. Consider userspace sets timeout = 10 s because
if the main application doesn't work for 12 s something dangerous can
happen. (Consider a guillotine where the blade can only be hold up for
12 s when not locked. :-) Now if the hw-max-timeout is 9s you setup a
timer to ping at $last_keepalive + 4.5 s and $last_keepalive + 9 s (not
taking timer and system latency into account). That means the system
only resets 18 s after the last userspace ping. Oops.
So ideally you send the last auto-ping at $last_keepalive +
$configured_timeout - $hw-max-timeout (assuming the hardware is
configured for $hw-max-timeout).
> >>>I don't understand what max_timeout is now that there is max_hw_timeout.
> >>>So I don't understand why you need hm != m either.
> >>>
> >>
> >>Backward compatibility. A driver which does not set max_hw_timeout_ms,
> >>or sets both to the same value, by definition expects to handle everything
> >>internally, and thus no worker is configured.
> >And a driver that does
> >
> > max_timeout = 5
> > max_hw_timeout = 5125
> >
> >falls through the cracks.
> >
> Hmm - not that this configuration makes any sense, but you are right.
> I'll make it "hm < m".
It does not? What do you expect max_timeout to be set to if the maximal
hw-timeout is 5125 ms? 0 would work, but IMHO you need some more
documentation then.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello Guenter,
On Wed, Aug 05, 2015 at 12:50:25AM -0700, Guenter Roeck wrote:
> On 08/05/2015 12:36 AM, Uwe Kleine-K?nig wrote:
> >the watchdog I'm currently working with on a powerpc platform has a
> >unchangable timeout of ~1 s. To make the machine boot I patched the
> >bootloader and need some automatic pinging in the kernel before
> >userspace takes over.
> >
>
> Does using arch_initcall in the watchdog driver help here, or is that
> still too slow and you really need to hack the kernel ?
I didn't do any measurements yet, but having the bootloader extracting
the kernel, and then send a ping before jumping into it is good enough
even without CONFIG_GPIO_WATCHDOG_ARCH_INITCALL to get the machine up.
(Userspace doesn't do any watchdog handling yet.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On 08/05/2015 01:22 AM, Uwe Kleine-K?nig wrote:
> Hello Guenter,
>
> On Tue, Aug 04, 2015 at 09:03:27AM -0700, Guenter Roeck wrote:
>> On 08/04/2015 08:52 AM, Uwe Kleine-K?nig wrote:
>>> On Tue, Aug 04, 2015 at 08:31:43AM -0700, Guenter Roeck wrote:
>>>> On 08/04/2015 05:18 AM, Uwe Kleine-K?nig wrote:
>>>>> On Mon, Aug 03, 2015 at 07:13:28PM -0700, Guenter Roeck wrote:
>>>>>> structure. If the configured timeout exceeds half the value of the
>>>>>> maximum hardware timeout, the watchdog core enables a timer function
>>>>>> to assist sending keepalive requests to the watchdog driver.
>>>>> I don't understand why you want to halve the maximum hw-timeout. If my
>>>>> watchdog has hw-max-timeout = 5s and userspace sets it to 3s there
>>>>> should be no need for assistance?! I think the implementation is the
>>>>> other way round?
>>>>>
>>>> It is supposed to reflect the _maximum_ timeout. That is different to
>>>> the time between heartbeats, which is supposed to be less; using half
>>>> the value of the maximum hardware timeout seemed to be a safe number.
>>> Right, I got that. With hw-max-timeout = 5s the machine resets after 5s
>>> not caring for the device. And so pinging repeatedly after 2.5s is fine.
>>> But if userspace sets a timeout of 3s (probably with the intention to
>>> ping with a frequency of 1/1.5s) there is no need for worker-assistance,
>>> because the pings coming in each 1.5s provided by userspace are good
>>> enough.
>>>
>> Yes, that is how it is supposed to work.
> So for the changelog you want:
>
> If the configured timeout exceeds the maximum hardware timeout
> the watchdog core enables a timer function ...
>
> right?
>
Something like that. You are right, the changelog needs an update.
>>>>>> +static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>>>>>> +{
>>>>>> + unsigned int hm = wdd->max_hw_timeout_ms;
>>>>>> + unsigned int m = wdd->max_timeout * 1000;
>>>>>> +
>>>>>> + return watchdog_active(wdd) && hm && hm != m &&
>>>>>> + wdd->timeout * 500 > hm;
> One problem with the worker I see is that the reset will probably be
> delayed with your worker. Consider userspace sets timeout = 10 s because
> if the main application doesn't work for 12 s something dangerous can
> happen. (Consider a guillotine where the blade can only be hold up for
> 12 s when not locked. :-) Now if the hw-max-timeout is 9s you setup a
> timer to ping at $last_keepalive + 4.5 s and $last_keepalive + 9 s (not
> taking timer and system latency into account). That means the system
> only resets 18 s after the last userspace ping. Oops.
>
> So ideally you send the last auto-ping at $last_keepalive +
> $configured_timeout - $hw-max-timeout (assuming the hardware is
> configured for $hw-max-timeout).
>
Yes, you are right, and makes sense. In practice that means I would
schedule an auto-ping 1s after the most recent user ping in your
example above, not at fixed intervals of 4.5s. I'll look into it.
Thinking about it, it is better anyway to reschedule the auto-ping
after a user ping, to avoid unnecessary pings.
>>>>> I don't understand what max_timeout is now that there is max_hw_timeout.
>>>>> So I don't understand why you need hm != m either.
>>>>>
>>>>
>>>> Backward compatibility. A driver which does not set max_hw_timeout_ms,
>>>> or sets both to the same value, by definition expects to handle everything
>>>> internally, and thus no worker is configured.
>>> And a driver that does
>>>
>>> max_timeout = 5
>>> max_hw_timeout = 5125
>>>
>>> falls through the cracks.
>>>
>> Hmm - not that this configuration makes any sense, but you are right.
>> I'll make it "hm < m".
> It does not? What do you expect max_timeout to be set to if the maximal
> hw-timeout is 5125 ms? 0 would work, but IMHO you need some more
> documentation then.
>
0 would work, or anything larger than 5.125s.
If you want to set max_timeout to 0, there is no need to set max_hw_timeout.
On Mon, Aug 03, 2015 at 07:13:26PM -0700, Guenter Roeck wrote:
> - Some watchdogs have a very short maximum timeout, in the range of just a few
> seconds. Such low timeouts are difficult if not impossible to support from
> user space. Drivers supporting such watchdog hardware need to implement
> a timer function to augment heartbeats from user space.
> - A new status flag, WDOG_RUNNING, informs the watchdog subsystem that a
> watchdog is running, and that the watchdog subsystem needs to generate
> heartbeat requests while the associated watchdog device is closed.
> Patch #2 adds timer functionality to the watchdog core. It solves the problem
> of short maximum hardware timeouts by augmenting heartbeats triggered from
> user space with internally triggered heartbeats.
>
> Patch #3 adds functionality to generate heartbeats while the watchdog device is
> closed. It handles situation where where the watchdog is running after
> the driver has been instantiated, but the device is not yet opened,
> and post-close situations necessary if a watchdog can not be stopped.
These sound concerning because it seems that heartbeats could be generated
outside of the direct control of userspace. I have a program that depends
on having direct control over whether heartbeats are generated (or more
specifically, *not* generated.) If these new features introduce a new way
for heartbeats to be generated, is there a way I can detect or disable
that behavior from userspace? Unwanted heartbeats could break my program
and may lead to data corruption.
A related issue from some years ago is the unfortunate fact that closing
the watchdog device also generates a heartbeat. I'd like to disable that
also, and submitted a patch for it here:
http://www.spinics.net/lists/linux-watchdog/msg01477.html
(Without the patch, I have to work around it by closing the device
prematurely as a way to generate the potentially final heartbeat, and then
reopen it again if I want to continue the heartbeats.)
Dave
Hi David,
On 08/05/2015 10:13 AM, David Teigland wrote:
> On Mon, Aug 03, 2015 at 07:13:26PM -0700, Guenter Roeck wrote:
>> - Some watchdogs have a very short maximum timeout, in the range of just a few
>> seconds. Such low timeouts are difficult if not impossible to support from
>> user space. Drivers supporting such watchdog hardware need to implement
>> a timer function to augment heartbeats from user space.
>
>> - A new status flag, WDOG_RUNNING, informs the watchdog subsystem that a
>> watchdog is running, and that the watchdog subsystem needs to generate
>> heartbeat requests while the associated watchdog device is closed.
>
>> Patch #2 adds timer functionality to the watchdog core. It solves the problem
>> of short maximum hardware timeouts by augmenting heartbeats triggered from
>> user space with internally triggered heartbeats.
>>
>> Patch #3 adds functionality to generate heartbeats while the watchdog device is
>> closed. It handles situation where where the watchdog is running after
>> the driver has been instantiated, but the device is not yet opened,
>> and post-close situations necessary if a watchdog can not be stopped.
>
> These sound concerning because it seems that heartbeats could be generated
> outside of the direct control of userspace. I have a program that depends
> on having direct control over whether heartbeats are generated (or more
> specifically, *not* generated.) If these new features introduce a new way
> for heartbeats to be generated, is there a way I can detect or disable
> that behavior from userspace? Unwanted heartbeats could break my program
> and may lead to data corruption.
>
Not really. The heartbeats will be generated such that the watchdog expires
no later that <last heartbeat from userspace + configured timeout>. I discussed
this already with Uwe; he had the same concern. This isn't in the current
version of the patch set, but it will be in the next version. That means
that nothing will change from user space perspective.
> A related issue from some years ago is the unfortunate fact that closing
> the watchdog device also generates a heartbeat. I'd like to disable that
> also, and submitted a patch for it here:
> http://www.spinics.net/lists/linux-watchdog/msg01477.html
>
That is a different issue, though, and unrelated to this patch set.
Wim had a good point there: Presumably the problem you are trying to solve
applies to the entire system, not to a specific watchdog. What you are looking
for looks more like a system parameter, not like something to set with an ioctl
message. The reason here is that you'd still want to be able to use standard
applications such as systemd or watchdogd to trigger heartbeats, and not depend
on your own.
Thanks,
Guenter
On Wed, Aug 05, 2015 at 10:41:51AM -0700, Guenter Roeck wrote:
> Not really. The heartbeats will be generated such that the watchdog expires
> no later that <last heartbeat from userspace + configured timeout>. I discussed
> this already with Uwe; he had the same concern. This isn't in the current
> version of the patch set, but it will be in the next version. That means
> that nothing will change from user space perspective.
Sounds good, thanks.
> >A related issue from some years ago is the unfortunate fact that closing
> >the watchdog device also generates a heartbeat. I'd like to disable that
> >also, and submitted a patch for it here:
> >http://www.spinics.net/lists/linux-watchdog/msg01477.html
> >
>
> That is a different issue, though, and unrelated to this patch set.
> Wim had a good point there: Presumably the problem you are trying to solve
> applies to the entire system, not to a specific watchdog. What you are looking
> for looks more like a system parameter, not like something to set with an ioctl
> message. The reason here is that you'd still want to be able to use standard
> applications such as systemd or watchdogd to trigger heartbeats, and not depend
> on your own.
I'd need this behavior when the system is running my program (sanlock with
wdmd), which uses /dev/watchdog. No other programs (systemd or watchdogd)
could be using /dev/watchdog at the same time.
Dave
Hi David,
On 08/05/2015 10:51 AM, David Teigland wrote:
> On Wed, Aug 05, 2015 at 10:41:51AM -0700, Guenter Roeck wrote:
>> Not really. The heartbeats will be generated such that the watchdog expires
>> no later that <last heartbeat from userspace + configured timeout>. I discussed
>> this already with Uwe; he had the same concern. This isn't in the current
>> version of the patch set, but it will be in the next version. That means
>> that nothing will change from user space perspective.
>
> Sounds good, thanks.
>
>>> A related issue from some years ago is the unfortunate fact that closing
>>> the watchdog device also generates a heartbeat. I'd like to disable that
>>> also, and submitted a patch for it here:
>>> http://www.spinics.net/lists/linux-watchdog/msg01477.html
>>>
>>
>> That is a different issue, though, and unrelated to this patch set.
>> Wim had a good point there: Presumably the problem you are trying to solve
>> applies to the entire system, not to a specific watchdog. What you are looking
>> for looks more like a system parameter, not like something to set with an ioctl
>> message. The reason here is that you'd still want to be able to use standard
>> applications such as systemd or watchdogd to trigger heartbeats, and not depend
>> on your own.
>
> I'd need this behavior when the system is running my program (sanlock with
> wdmd), which uses /dev/watchdog. No other programs (systemd or watchdogd)
> could be using /dev/watchdog at the same time.
>
I think I can understand why Wim was reluctant to accept your patch;
I must admit I don't understand your use case either.
I wonder if you are actually mis-using the watchdog subsystem to generate
hard resets. After all, you could avoid the unexpected close situation with
an exit handler in your application. That handler could catch anything but
SIGKILL, but anyone using SIGKILL doesn't really deserve better. If the intent
is to reset the system after the application closes, executing "/sbin/restart -f"
might be a safer approach than just killing the watchdog.
In addition to that, I don't think it is a good idea to rely on the assumption
that the watchdog will expire exactly after the configured timeout.
Many watchdog drivers implement a soft timeout on top of the hardware timeout,
and thus already implement the internal heartbeat. Most of those drivers
will stop sending internal heartbeats if user space did not send a heartbeat
within the configured timeout period. The actual reset will then occur later,
after the actual hardware watchdog timed out. This can be as much as the
hardware timeout period, which may be substantial.
Thanks,
Guenter
On Wed, Aug 05, 2015 at 12:01:38PM -0700, Guenter Roeck wrote:
> I think I can understand why Wim was reluctant to accept your patch;
> I must admit I don't understand your use case either.
Very breifly, sanlock is a shared storage based lease manager, and the
expiration of a lease is tied to the expiration of the watchdog. I have
to ensure that the watchdog expires at or before the time that the lease
expires. This means that I cannot allow a watchdog heartbeat apart from a
corresponding lease renewal on the shared storage. Otherwise, the
calculation by other hosts of the time of the hard reset will be wrong,
and the data on shared storage could be corrupted.
> I wonder if you are actually mis-using the watchdog subsystem to generate
> hard resets.
I am indeed using it to generate hard resets.
> After all, you could avoid the unexpected close situation with
> an exit handler in your application. That handler could catch anything but
> SIGKILL, but anyone using SIGKILL doesn't really deserve better.
I avoid the unexpected close situation by prematurely closing the device
to generate the heartbeat from close, and then reopening if needed. That
covers the SIGKILL case. So, I have a work around, but the patch would
still be nice.
> If the intent is to reset the system after the application closes,
> executing "/sbin/restart -f" might be a safer approach than just killing
> the watchdog.
I need to reset the system if the application crashes, or if the
application is running but can't renew its lease. In the former case,
executing something doesn't work. In the later case, I have done similar
(with /proc/sysrq-trigger), but it doesn't always apply and I'd still want
the hardware reset as redundancy.
> In addition to that, I don't think it is a good idea to rely on the assumption
> that the watchdog will expire exactly after the configured timeout.
> Many watchdog drivers implement a soft timeout on top of the hardware timeout,
> and thus already implement the internal heartbeat. Most of those drivers
> will stop sending internal heartbeats if user space did not send a heartbeat
> within the configured timeout period. The actual reset will then occur later,
> after the actual hardware watchdog timed out. This can be as much as the
> hardware timeout period, which may be substantial.
OK, thanks, I'll look into this in more detail. Is there a way I can
identify which cases these are, or do you know an example I can look at?
In the worst case I'd have to extend the lease expiration time by a full
timeout period when the dubious drivers are used.
Dave
Hi David,
On 08/05/2015 12:51 PM, David Teigland wrote:
> On Wed, Aug 05, 2015 at 12:01:38PM -0700, Guenter Roeck wrote:
>> I think I can understand why Wim was reluctant to accept your patch;
>> I must admit I don't understand your use case either.
>
> Very breifly, sanlock is a shared storage based lease manager, and the
> expiration of a lease is tied to the expiration of the watchdog. I have
> to ensure that the watchdog expires at or before the time that the lease
> expires. This means that I cannot allow a watchdog heartbeat apart from a
> corresponding lease renewal on the shared storage. Otherwise, the
> calculation by other hosts of the time of the hard reset will be wrong,
> and the data on shared storage could be corrupted.
>
>> I wonder if you are actually mis-using the watchdog subsystem to generate
>> hard resets.
>
> I am indeed using it to generate hard resets.
>
So there is no concern that the hard reset may corrupt some data ? Interesting.
Hope you don't use any SSDs - some of those don't like that.
>> After all, you could avoid the unexpected close situation with
>> an exit handler in your application. That handler could catch anything but
>> SIGKILL, but anyone using SIGKILL doesn't really deserve better.
>
> I avoid the unexpected close situation by prematurely closing the device
> to generate the heartbeat from close, and then reopening if needed. That
> covers the SIGKILL case. So, I have a work around, but the patch would
> still be nice.
>
Sounds messy....
>> If the intent is to reset the system after the application closes,
>> executing "/sbin/restart -f" might be a safer approach than just killing
>> the watchdog.
>
> I need to reset the system if the application crashes, or if the
> application is running but can't renew its lease. In the former case,
> executing something doesn't work. In the later case, I have done similar
Maybe you could have the system monitor (systemd or whatever it is) the
application and run /sbin/restart if it crashes. Essentially monitor
the application from the outside.
> (with /proc/sysrq-trigger), but it doesn't always apply and I'd still want
> the hardware reset as redundancy.
>
>> In addition to that, I don't think it is a good idea to rely on the assumption
>> that the watchdog will expire exactly after the configured timeout.
>> Many watchdog drivers implement a soft timeout on top of the hardware timeout,
>> and thus already implement the internal heartbeat. Most of those drivers
>> will stop sending internal heartbeats if user space did not send a heartbeat
>> within the configured timeout period. The actual reset will then occur later,
>> after the actual hardware watchdog timed out. This can be as much as the
>> hardware timeout period, which may be substantial.
>
> OK, thanks, I'll look into this in more detail. Is there a way I can
> identify which cases these are, or do you know an example I can look at?
> In the worst case I'd have to extend the lease expiration time by a full
> timeout period when the dubious drivers are used.
>
git grep mod_timer drivers/watchdog | cut -f1 -d: | sort -u
gives you the following list:
alim7101_wdt.c
at91sam9_wdt.c
bcm47xx_wdt.c
bcm63xx_wdt.c
cpu5wdt.c
dw_wdt.c
ep93xx_wdt.c
gpio_wdt.c
imx2_wdt.c
machzwd.c
mixcomwd.c
mpc8xxx_wdt.c
mtx-1_wdt.c
nuc900_wdt.c
pcwd.c
pika_wdt.c
rdc321x_wdt.c
sbc60xxwdt.c
sc520_wdt.c
shwdt.c
softdog.c
via_wdt.c
w83877f_wdt.c
Those would be the immediate candidates to look out for. Note that this situation
will actually improve with my patch set, since it tries to tie the actual expiry
to the configured timeout. This will only work if the driver(s) are converted
to use the new infrastructure, of course.
Still, the ABI guarantees that "the hardware watchdog will reset the system
(causing a reboot) after the timeout occurs", but that doesn't mean that it
will reset the system immediately. I think the only safe guarantee is that
it won't reset the system as long as the timeout did _not_ occur. Extending
the lease expiration in your application by a timeout period will help,
but there is still no _guarantee_ that the reset will occur within
"expiration time + timeout".
I am not even sure if all watchdog drivers which don't implement a soft timer
will always timeout exactly after "timeout" seconds. It will be "at least
timeout" seconds, but I would not bet that it is always the exact time.
Guenter