* [cip-dev] [PATCH 01/10] watchdog: Introduce hardware maximum heartbeat in watchdog core
2017-10-04 14:40 [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Maxim Yu. Osipov
@ 2017-10-04 14:40 ` Maxim Yu. Osipov
2017-10-04 14:40 ` [cip-dev] [PATCH 02/10] watchdog: Introduce WDOG_HW_RUNNING flag Maxim Yu. Osipov
` (10 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Maxim Yu. Osipov @ 2017-10-04 14:40 UTC (permalink / raw)
To: cip-dev
From: Guenter Roeck <linux@roeck-us.net>
Backport from kernel.org, upstream commit 664a39236e71
Introduce an optional hardware maximum heartbeat in the watchdog core.
The hardware maximum heartbeat can be lower than the maximum timeout.
Drivers can set the maximum hardware heartbeat value in the watchdog data
structure. If the configured timeout exceeds the maximum hardware heartbeat,
the watchdog core enables a timer function to assist sending keepalive
requests to the watchdog driver.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
[mosipov at ilbers.de backported to 4.4.y]
Signed-off-by: Maxim Yu. Osipov <mosipov@ilbers.de>
---
Documentation/watchdog/watchdog-kernel-api.txt | 19 +++-
drivers/watchdog/watchdog_dev.c | 122 +++++++++++++++++++++++--
include/linux/watchdog.h | 30 ++++--
3 files changed, 154 insertions(+), 17 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index d8b0d3367706..9887fa6d8f68 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -53,6 +53,7 @@ struct watchdog_device {
unsigned int timeout;
unsigned int min_timeout;
unsigned int max_timeout;
+ unsigned int max_hw_heartbeat_ms;
void *driver_data;
struct mutex lock;
unsigned long status;
@@ -73,8 +74,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 WDOG_ACTIVE is set.
* min_timeout: the watchdog timer's minimum timeout value (in seconds).
-* max_timeout: the watchdog timer's maximum timeout value (in seconds).
+ If set, the minimum configurable value for 'timeout'.
+* max_timeout: the watchdog timer's maximum timeout value (in seconds),
+ as seen from userspace. If set, the maximum configurable value for
+ 'timeout'. Not used if max_hw_heartbeat_ms is non-zero.
+* max_hw_heartbeat_ms: Maximum hardware heartbeat, in milli-seconds.
+ If set, the infrastructure will send heartbeats to the watchdog driver
+ if 'timeout' is larger than max_hw_heartbeat_ms, unless WDOG_ACTIVE
+ is set and userspace failed to send a heartbeat for at least 'timeout'
+ seconds.
* 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.
@@ -160,7 +171,11 @@ they are supported. These optional routines/operations are:
and -EIO for "could not write value to the watchdog". On success this
routine should set the timeout value of the watchdog_device to the
achieved timeout value (which may be different from the requested one
- because the watchdog does not necessarily has a 1 second resolution).
+ because the watchdog does not necessarily have a 1 second resolution).
+ Drivers implementing max_hw_heartbeat_ms set the hardware watchdog heartbeat
+ to the minimum of timeout and max_hw_heartbeat_ms. Those drivers set the
+ timeout value of the watchdog_device either to the requested timeout value
+ (if it is larger than max_hw_heartbeat_ms), or to the achieved timeout value.
(Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
watchdog's info structure).
* get_timeleft: this routines returns the time that's left before a reset.
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 56a649e66eb2..df43c586e53f 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -35,9 +35,11 @@
#include <linux/module.h> /* For module stuff/... */
#include <linux/types.h> /* For standard types (like size_t) */
#include <linux/errno.h> /* For the -ENODEV/... values */
+#include <linux/jiffies.h> /* For timeout functions */
#include <linux/kernel.h> /* For printk/panic/... */
#include <linux/fs.h> /* For file operations */
#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,73 @@ 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)
+{
+ /* All variables in milli-seconds */
+ unsigned int hm = wdd->max_hw_heartbeat_ms;
+ unsigned int t = wdd->timeout * 1000;
+
+ /*
+ * A worker to generate heartbeat requests is needed if all of the
+ * following conditions are true.
+ * - Userspace activated the watchdog.
+ * - The driver provided a value for the maximum hardware timeout, and
+ * thus is aware that the framework supports generating heartbeat
+ * requests.
+ * - Userspace requests a longer timeout than the hardware can handle.
+ */
+ return watchdog_active(wdd) && hm && t > hm;
+}
+
+static long watchdog_next_keepalive(struct watchdog_device *wdd)
+{
+ unsigned int timeout_ms = wdd->timeout * 1000;
+ unsigned long keepalive_interval;
+ unsigned long last_heartbeat;
+ unsigned long virt_timeout;
+ unsigned int hw_heartbeat_ms;
+
+ virt_timeout = wdd->last_keepalive + msecs_to_jiffies(timeout_ms);
+ hw_heartbeat_ms = min(timeout_ms, wdd->max_hw_heartbeat_ms);
+ keepalive_interval = msecs_to_jiffies(hw_heartbeat_ms / 2);
+
+ /*
+ * To ensure that the watchdog times out wdd->timeout seconds
+ * after the most recent ping from userspace, the last
+ * worker ping has to come in hw_heartbeat_ms before this timeout.
+ */
+ last_heartbeat = virt_timeout - msecs_to_jiffies(hw_heartbeat_ms);
+ return min_t(long, last_heartbeat - jiffies, keepalive_interval);
+}
+
+static inline void watchdog_update_worker(struct watchdog_device *wdd)
+{
+ if (watchdog_need_worker(wdd)) {
+ long t = watchdog_next_keepalive(wdd);
+
+ if (t > 0)
+ mod_delayed_work(watchdog_wq, &wdd->work, t);
+ } else {
+ cancel_delayed_work(&wdd->work);
+ }
+}
+
+static int __watchdog_ping(struct watchdog_device *wdd)
+{
+ int err;
+
+ if (wdd->ops->ping)
+ err = wdd->ops->ping(wdd); /* ping the watchdog */
+ else
+ err = wdd->ops->start(wdd); /* restart watchdog */
+
+ watchdog_update_worker(wdd);
+
+ return err;
+}
+
/*
* watchdog_ping: ping the watchdog.
* @wdd: the watchdog device to ping
@@ -73,16 +142,27 @@ static int watchdog_ping(struct watchdog_device *wdd)
if (!watchdog_active(wdd))
goto out_ping;
- if (wdd->ops->ping)
- err = wdd->ops->ping(wdd); /* ping the watchdog */
- else
- err = wdd->ops->start(wdd); /* restart watchdog */
+ wdd->last_keepalive = jiffies;
+ err = __watchdog_ping(wdd);
out_ping:
mutex_unlock(&wdd->lock);
return err;
}
+static void watchdog_ping_work(struct work_struct *work)
+{
+ struct watchdog_device *wdd;
+
+ wdd = container_of(to_delayed_work(work), struct watchdog_device,
+ work);
+
+ mutex_lock(&wdd->lock);
+ if (wdd && watchdog_active(wdd))
+ __watchdog_ping(wdd);
+ mutex_unlock(&wdd->lock);
+}
+
/*
* watchdog_start: wrapper to start the watchdog.
* @wdd: the watchdog device to start
@@ -95,6 +175,7 @@ out_ping:
static int watchdog_start(struct watchdog_device *wdd)
{
int err = 0;
+ unsigned long started_at;
mutex_lock(&wdd->lock);
@@ -106,9 +187,13 @@ static int watchdog_start(struct watchdog_device *wdd)
if (watchdog_active(wdd))
goto out_start;
+ started_at = jiffies;
err = wdd->ops->start(wdd);
- if (err == 0)
+ if (err == 0) {
set_bit(WDOG_ACTIVE, &wdd->status);
+ wdd->last_keepalive = started_at;
+ watchdog_update_worker(wdd);
+ }
out_start:
mutex_unlock(&wdd->lock);
@@ -146,8 +231,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);
+ cancel_delayed_work(&wdd->work);
+ }
out_stop:
mutex_unlock(&wdd->lock);
@@ -211,6 +298,8 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
err = wdd->ops->set_timeout(wdd, timeout);
+ watchdog_update_worker(wdd);
+
out_timeout:
mutex_unlock(&wdd->lock);
return err;
@@ -490,6 +579,8 @@ static int watchdog_release(struct inode *inode, struct file *file)
/* Allow the owner module to be unloaded again */
module_put(wdd->ops->owner);
+ cancel_delayed_work_sync(&wdd->work);
+
/* make sure that /dev/watchdog can be re-opened */
clear_bit(WDOG_DEV_OPEN, &wdd->status);
@@ -527,6 +618,11 @@ 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->id == 0) {
old_wdd = wdd;
watchdog_miscdev.parent = wdd->parent;
@@ -573,6 +669,8 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
set_bit(WDOG_UNREGISTERED, &wdd->status);
mutex_unlock(&wdd->lock);
+ cancel_delayed_work_sync(&wdd->work);
+
cdev_del(&wdd->cdev);
if (wdd->id == 0) {
misc_deregister(&watchdog_miscdev);
@@ -589,7 +687,16 @@ 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");
+ return -ENOMEM;
+ }
+
+ err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
if (err < 0)
pr_err("watchdog: unable to allocate char dev region\n");
return err;
@@ -604,4 +711,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 027b1f43f12d..26aba9b17ac3 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -10,8 +10,9 @@
#include <linux/bitops.h>
-#include <linux/device.h>
#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
#include <uapi/linux/watchdog.h>
struct watchdog_ops;
@@ -61,12 +62,17 @@ struct watchdog_ops {
* @bootstatus: Status of the watchdog device at boot.
* @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_timeout:The watchdog devices maximum timeout value (in seconds)
+ * as configurable from user space. Only relevant if
+ * max_hw_heartbeat_ms is not provided.
+ * @max_hw_heartbeat_ms:
+ * Hardware limit for maximum timeout, in milli-seconds.
+ * Replaces max_timeout if specified.
* @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.
+ * @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,8 +94,11 @@ struct watchdog_device {
unsigned int timeout;
unsigned int min_timeout;
unsigned int max_timeout;
+ unsigned int max_hw_heartbeat_ms;
void *driver_data;
struct mutex lock;
+ unsigned long last_keepalive;
+ struct delayed_work work;
unsigned long status;
/* Bit numbers for status flags */
#define WDOG_ACTIVE 0 /* Is the watchdog running/active */
@@ -121,13 +130,18 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
{
/*
* The timeout is invalid if
+ * - the requested value is larger than UINT_MAX / 1000
+ * (since internal calculations are done in milli-seconds),
+ * or
* - the requested value is smaller than the configured minimum timeout,
* or
- * - a maximum timeout is configured, and the requested value is larger
- * than the maximum timeout.
+ * - a maximum hardware timeout is not configured, a maximum timeout
+ * is configured, and the requested value is larger than the
+ * configured maximum timeout.
*/
- return t < wdd->min_timeout ||
- (wdd->max_timeout && t > wdd->max_timeout);
+ return t > UINT_MAX / 1000 || t < wdd->min_timeout ||
+ (!wdd->max_hw_heartbeat_ms && wdd->max_timeout &&
+ t > wdd->max_timeout);
}
/* Use the following functions to manipulate watchdog driver specific data */
--
2.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [cip-dev] [PATCH 02/10] watchdog: Introduce WDOG_HW_RUNNING flag
2017-10-04 14:40 [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Maxim Yu. Osipov
2017-10-04 14:40 ` [cip-dev] [PATCH 01/10] watchdog: Introduce hardware maximum heartbeat in watchdog core Maxim Yu. Osipov
@ 2017-10-04 14:40 ` Maxim Yu. Osipov
2017-10-25 9:43 ` Ben Hutchings
2017-10-04 14:40 ` [cip-dev] [PATCH 03/10] watchdog: Make stop function optional Maxim Yu. Osipov
` (9 subsequent siblings)
11 siblings, 1 reply; 16+ messages in thread
From: Maxim Yu. Osipov @ 2017-10-04 14:40 UTC (permalink / raw)
To: cip-dev
From: Guenter Roeck <linux@roeck-us.net>
Backport from kernel.org, upstream commit ee142889e32f
The WDOG_HW_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_HW_RUNNING flag in its stop function.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
[mosipov at ilbers.de backported to 4.4.y]
Signed-off-by: Maxim Yu. Osipov <mosipov@ilbers.de>
---
Documentation/watchdog/watchdog-kernel-api.txt | 22 ++++++++----
drivers/watchdog/watchdog_dev.c | 49 ++++++++++++++++++++------
include/linux/watchdog.h | 10 ++++++
3 files changed, 64 insertions(+), 17 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 9887fa6d8f68..b12789745f86 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -144,10 +144,10 @@ 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. 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.
+ Some watchdog timer hardware can only be started and not be stopped.
+ If a watchdog can not be stopped, the watchdog driver must set the
+ WDOG_HW_RUNNING flag in its stop function to inform the watchdog core that
+ the watchdog is still running.
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
@@ -191,9 +191,8 @@ they are supported. These optional routines/operations are:
The status bits should (preferably) be set with the set_bit and clear_bit alike
bit-operations. The status bits that are defined are:
* WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device
- is active or not. When the watchdog is active after booting, then you should
- set this status bit (Note: when you register the watchdog timer device with
- this bit set, then opening /dev/watchdog will skip the start operation)
+ is active or not from user perspective. User space is expected to send
+ heartbeat requests to the driver while this flag is set.
* WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
was opened via /dev/watchdog.
(This bit should only be used by the WatchDog Timer Driver Core).
@@ -202,6 +201,15 @@ bit-operations. The status bits that are defined are:
(This bit should only be used by the WatchDog Timer Driver Core).
* WDOG_NO_WAY_OUT: this bit stores the nowayout setting for the watchdog.
If this bit is set then the watchdog timer will not be able to stop.
+* WDOG_HW_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. The bit may also be set if the watchdog timer is running after
+ booting, before the watchdog device is opened. If set, the watchdog
+ infrastructure will send keepalives to the watchdog hardware while
+ WDOG_ACTIVE is not set.
+ Note: when you register the watchdog timer device with this bit set,
+ then opening /dev/watchdog will skip the start operation but send a keepalive
+ request instead.
* WDOG_UNREGISTERED: this bit gets set by the WatchDog Timer Driver Core
after calling watchdog_unregister_device, and then checked before calling
any watchdog_ops, so that you can be sure that no operations (other then
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index df43c586e53f..55dc73f9b837 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -68,7 +68,8 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
* requests.
* - Userspace requests a longer timeout than the hardware can handle.
*/
- return watchdog_active(wdd) && hm && t > hm;
+ return hm && ((watchdog_active(wdd) && t > hm) ||
+ (t && !watchdog_active(wdd) && watchdog_hw_running(wdd)));
}
static long watchdog_next_keepalive(struct watchdog_device *wdd)
@@ -83,6 +84,9 @@ static long watchdog_next_keepalive(struct watchdog_device *wdd)
hw_heartbeat_ms = min(timeout_ms, wdd->max_hw_heartbeat_ms);
keepalive_interval = msecs_to_jiffies(hw_heartbeat_ms / 2);
+ if (!watchdog_active(wdd))
+ return keepalive_interval;
+
/*
* To ensure that the watchdog times out wdd->timeout seconds
* after the most recent ping from userspace, the last
@@ -139,7 +143,7 @@ static int watchdog_ping(struct watchdog_device *wdd)
goto out_ping;
}
- if (!watchdog_active(wdd))
+ if (!watchdog_active(wdd) && !watchdog_hw_running(wdd))
goto out_ping;
wdd->last_keepalive = jiffies;
@@ -158,7 +162,7 @@ static void watchdog_ping_work(struct work_struct *work)
work);
mutex_lock(&wdd->lock);
- if (wdd && watchdog_active(wdd))
+ if (wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd)))
__watchdog_ping(wdd);
mutex_unlock(&wdd->lock);
}
@@ -188,7 +192,10 @@ static int watchdog_start(struct watchdog_device *wdd)
goto out_start;
started_at = jiffies;
- err = wdd->ops->start(wdd);
+ if (watchdog_hw_running(wdd) && wdd->ops->ping)
+ err = wdd->ops->ping(wdd);
+ else
+ err = wdd->ops->start(wdd);
if (err == 0) {
set_bit(WDOG_ACTIVE, &wdd->status);
wdd->last_keepalive = started_at;
@@ -233,7 +240,7 @@ static int watchdog_stop(struct watchdog_device *wdd)
err = wdd->ops->stop(wdd);
if (err == 0) {
clear_bit(WDOG_ACTIVE, &wdd->status);
- cancel_delayed_work(&wdd->work);
+ watchdog_update_worker(wdd);
}
out_stop:
@@ -519,7 +526,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_hw_running(wdd) && !try_module_get(wdd->ops->owner))
goto out;
err = watchdog_start(wdd);
@@ -528,7 +535,7 @@ static int watchdog_open(struct inode *inode, struct file *file)
file->private_data = wdd;
- if (wdd->ops->ref)
+ if (!watchdog_hw_running(wdd) && wdd->ops->ref)
wdd->ops->ref(wdd);
/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
@@ -577,15 +584,22 @@ static int watchdog_release(struct inode *inode, struct file *file)
}
/* 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_hw_running(wdd))
+ module_put(wdd->ops->owner);
cancel_delayed_work_sync(&wdd->work);
+ watchdog_update_worker(wdd);
/* make sure that /dev/watchdog can be re-opened */
clear_bit(WDOG_DEV_OPEN, &wdd->status);
/* Note wdd may be gone after this, do not use after this! */
- if (wdd->ops->unref)
+ if (!watchdog_hw_running(wdd) && wdd->ops->unref)
wdd->ops->unref(wdd);
return 0;
@@ -651,9 +665,24 @@ int watchdog_dev_register(struct watchdog_device *wdd)
if (wdd->id == 0) {
misc_deregister(&watchdog_miscdev);
old_wdd = NULL;
+ if (wdd->ops->unref)
+ wdd->ops->unref(wdd);
}
+ return err;
}
- return err;
+
+ /*
+ * If the watchdog is running, prevent its driver from being unloaded,
+ * and schedule an immediate ping.
+ */
+ if (watchdog_hw_running(wdd)) {
+ __module_get(wdd->ops->owner);
+ if (wdd->ops->ref)
+ wdd->ops->ref(wdd);
+ queue_delayed_work(watchdog_wq, &wdd->work, 0);
+ }
+
+ return 0;
}
/*
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 26aba9b17ac3..cbe31d5b352c 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -106,6 +106,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_HW_RUNNING 5 /* True if HW watchdog running */
struct list_head deferred;
};
@@ -118,6 +119,15 @@ 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 hardware watchdog
+ * is running
+ */
+static inline bool watchdog_hw_running(struct watchdog_device *wdd)
+{
+ return test_bit(WDOG_HW_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.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [cip-dev] [PATCH 02/10] watchdog: Introduce WDOG_HW_RUNNING flag
2017-10-04 14:40 ` [cip-dev] [PATCH 02/10] watchdog: Introduce WDOG_HW_RUNNING flag Maxim Yu. Osipov
@ 2017-10-25 9:43 ` Ben Hutchings
0 siblings, 0 replies; 16+ messages in thread
From: Ben Hutchings @ 2017-10-25 9:43 UTC (permalink / raw)
To: cip-dev
On Wed, 2017-10-04 at 16:40 +0200, Maxim Yu. Osipov wrote:
> From: Guenter Roeck <linux@roeck-us.net>
>
> Backport from kernel.org, upstream commit ee142889e32f
>
> The WDOG_HW_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_HW_RUNNING flag in its stop function.
[...]
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
[...]
> @@ -651,9 +665,24 @@ int watchdog_dev_register(struct watchdog_device
> *wdd)
> ? if (wdd->id == 0) {
> ? misc_deregister(&watchdog_miscdev);
> ? old_wdd = NULL;
> + if (wdd->ops->unref)
> + wdd->ops->unref(wdd);
[...]
This doesn't look right. This is decrementing a reference counter
because the old_wdd reference is going away, but we never increment a
reference counter when old_wdd is set.
The upstream commit doesn't make any change here. There is a
kref_put() here in the upstream version but that was added as part of
the watchdog_device/watchdog_core_data split.
Ben.
--
Ben Hutchings
Software Developer, Codethink Ltd.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [cip-dev] [PATCH 03/10] watchdog: Make stop function optional
2017-10-04 14:40 [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Maxim Yu. Osipov
2017-10-04 14:40 ` [cip-dev] [PATCH 01/10] watchdog: Introduce hardware maximum heartbeat in watchdog core Maxim Yu. Osipov
2017-10-04 14:40 ` [cip-dev] [PATCH 02/10] watchdog: Introduce WDOG_HW_RUNNING flag Maxim Yu. Osipov
@ 2017-10-04 14:40 ` Maxim Yu. Osipov
2017-10-04 14:40 ` [cip-dev] [PATCH 04/10] watchdog: imx2: Convert to use infrastructure triggered keepalives Maxim Yu. Osipov
` (8 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Maxim Yu. Osipov @ 2017-10-04 14:40 UTC (permalink / raw)
To: cip-dev
From: Guenter Roeck <linux@roeck-us.net>
Backport from kernel.org, upstream commit d0684c8a9354
Not all hardware watchdogs can be stopped. The driver for
such watchdogs would typically only set the WATCHDOG_HW_RUNNING
flag in its stop function. Make the stop function optional and set
WATCHDOG_HW_RUNNING in the watchdog core if it is not provided.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
[mosipov at ilbers.de backported to 4.4.y]
Signed-off-by: Maxim Yu. Osipov <mosipov@ilbers.de>
---
Documentation/watchdog/watchdog-kernel-api.txt | 20 ++++++++++++--------
drivers/watchdog/watchdog_core.c | 2 +-
drivers/watchdog/watchdog_dev.c | 6 +++++-
3 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index b12789745f86..a376f281f5a7 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -85,7 +85,8 @@ It contains following fields:
If set, the infrastructure will send heartbeats to the watchdog driver
if 'timeout' is larger than max_hw_heartbeat_ms, unless WDOG_ACTIVE
is set and userspace failed to send a heartbeat for at least 'timeout'
- seconds.
+ seconds. max_hw_heartbeat_ms must be set if a driver does not implement
+ the stop function.
* 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.
@@ -141,17 +142,20 @@ 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.
- If a watchdog can not be stopped, the watchdog driver must set the
- WDOG_HW_RUNNING flag in its stop function to inform the watchdog core that
- the watchdog is still running.
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_HW_RUNNING
+ and start calling the driver's keepalive pings function after the watchdog
+ device is closed.
+ If a watchdog driver does not implement the stop function, it must set
+ max_hw_heartbeat_ms.
* 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
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 873f13972cf4..41f1854d026b 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 || (!wdd->ops->stop && !wdd->max_hw_heartbeat_ms))
return -EINVAL;
watchdog_check_min_max_timeout(wdd);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 55dc73f9b837..ca64724bf59a 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -237,7 +237,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_HW_RUNNING, &wdd->status);
+
if (err == 0) {
clear_bit(WDOG_ACTIVE, &wdd->status);
watchdog_update_worker(wdd);
--
2.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [cip-dev] [PATCH 04/10] watchdog: imx2: Convert to use infrastructure triggered keepalives
2017-10-04 14:40 [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Maxim Yu. Osipov
` (2 preceding siblings ...)
2017-10-04 14:40 ` [cip-dev] [PATCH 03/10] watchdog: Make stop function optional Maxim Yu. Osipov
@ 2017-10-04 14:40 ` Maxim Yu. Osipov
2017-10-04 14:40 ` [cip-dev] [PATCH 05/10] watchdog: core: Fix circular locking dependency Maxim Yu. Osipov
` (7 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Maxim Yu. Osipov @ 2017-10-04 14:40 UTC (permalink / raw)
To: cip-dev
From: Guenter Roeck <linux@roeck-us.net>
Backport from kernel.org, upstream commit 11d7aba9ceb7
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 <linux@roeck-us.net>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
[mosipov at ilbers.de backported to 4.4.y]
Signed-off-by: Maxim Yu. Osipov <mosipov@ilbers.de>
---
drivers/watchdog/imx2_wdt.c | 74 ++++++++-------------------------------------
1 file changed, 13 insertions(+), 61 deletions(-)
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 29ef719a6a3c..378e7c5dfc50 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_HW_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,
};
@@ -257,7 +223,7 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
wdog->info = &imx2_wdt_info;
wdog->ops = &imx2_wdt_ops;
wdog->min_timeout = 1;
- wdog->max_timeout = IMX2_WDT_MAX_TIME;
+ wdog->max_hw_heartbeat_ms = IMX2_WDT_MAX_TIME * 1000;
wdog->parent = &pdev->dev;
ret = clk_prepare_enable(wdev->clk);
@@ -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_HW_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.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [cip-dev] [PATCH 05/10] watchdog: core: Fix circular locking dependency
2017-10-04 14:40 [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Maxim Yu. Osipov
` (3 preceding siblings ...)
2017-10-04 14:40 ` [cip-dev] [PATCH 04/10] watchdog: imx2: Convert to use infrastructure triggered keepalives Maxim Yu. Osipov
@ 2017-10-04 14:40 ` Maxim Yu. Osipov
2017-10-04 14:41 ` [cip-dev] [PATCH 06/10] watchdog: skip min and max timeout validity check when max_hw_heartbeat_ms is defined Maxim Yu. Osipov
` (6 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Maxim Yu. Osipov @ 2017-10-04 14:40 UTC (permalink / raw)
To: cip-dev
From: Guenter Roeck <linux@roeck-us.net>
Backport from kernel.org, upstream commit e1f30282a1d3
lockdep reports the following circular locking dependency.
======================================================
INFO: possible circular locking dependency detected ]
4.6.0-rc3-00191-gfabf418 #162 Not tainted
-------------------------------------------------------
systemd/1 is trying to acquire lock:
((&(&wd_data->work)->work)){+.+...}, at: [<80141650>] flush_work+0x0/0x280
but task is already holding lock:
(&wd_data->lock){+.+...}, at: [<804acfa8>] watchdog_release+0x18/0x190
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&wd_data->lock){+.+...}:
>-------[<80662310>] mutex_lock_nested+0x64/0x4a8
>-------[<804aca4c>] watchdog_ping_work+0x18/0x4c
>-------[<80143128>] process_one_work+0x1ac/0x500
>-------[<801434b4>] worker_thread+0x38/0x554
>-------[<80149510>] kthread+0xf4/0x108
>-------[<80107c10>] ret_from_fork+0x14/0x24
-> #0 ((&(&wd_data->work)->work)){+.+...}:
>-------[<8017c4e8>] lock_acquire+0x70/0x90
>-------[<8014169c>] flush_work+0x4c/0x280
>-------[<801440f8>] __cancel_work_timer+0x9c/0x1e0
>-------[<804acfcc>] watchdog_release+0x3c/0x190
>-------[<8022c5e8>] __fput+0x80/0x1c8
>-------[<80147b28>] task_work_run+0x94/0xc8
>-------[<8010b998>] do_work_pending+0x8c/0xb4
>-------[<80107ba8>] slow_work_pending+0xc/0x20
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&wd_data->lock);
lock((&(&wd_data->work)->work));
lock(&wd_data->lock);
lock((&(&wd_data->work)->work));
*** DEADLOCK ***
1 lock held by systemd/1:
stack backtrace:
CPU: 2 PID: 1 Comm: systemd Not tainted 4.6.0-rc3-00191-gfabf418 #162
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[<8010f5e4>] (unwind_backtrace) from [<8010c038>] (show_stack+0x10/0x14)
[<8010c038>] (show_stack) from [<8039d7fc>] (dump_stack+0xa8/0xd4)
[<8039d7fc>] (dump_stack) from [<80177ee0>] (print_circular_bug+0x214/0x334)
[<80177ee0>] (print_circular_bug) from [<80179230>] (check_prevs_add+0x4dc/0x8e8)
[<80179230>] (check_prevs_add) from [<8017b3d8>] (__lock_acquire+0xc6c/0x14ec)
[<8017b3d8>] (__lock_acquire) from [<8017c4e8>] (lock_acquire+0x70/0x90)
[<8017c4e8>] (lock_acquire) from [<8014169c>] (flush_work+0x4c/0x280)
[<8014169c>] (flush_work) from [<801440f8>] (__cancel_work_timer+0x9c/0x1e0)
[<801440f8>] (__cancel_work_timer) from [<804acfcc>] (watchdog_release+0x3c/0x190)
[<804acfcc>] (watchdog_release) from [<8022c5e8>] (__fput+0x80/0x1c8)
[<8022c5e8>] (__fput) from [<80147b28>] (task_work_run+0x94/0xc8)
[<80147b28>] (task_work_run) from [<8010b998>] (do_work_pending+0x8c/0xb4)
[<8010b998>] (do_work_pending) from [<80107ba8>] (slow_work_pending+0xc/0x20)
Turns out the call to cancel_delayed_work_sync() in watchdog_release()
is not necessary and can be dropped. If the worker is no longer necessary,
the subsequent call to watchdog_update_worker() will cancel it. If it is
already running, it won't do anything, since the worker function checks
if it needs to ping the watchdog or not.
Reported-by: Clemens Gruber <clemens.gruber@pqgruber.com>
Tested-by: Clemens Gruber <clemens.gruber@pqgruber.com>
Fixes: 11d7aba9ceb7 ("watchdog: imx2: Convert to use infrastructure triggered keepalives")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Cc: stable <stable@vger.kernel.org>
[mosipov at ilbers.de backported to 4.4.y]
Signed-off-by: Maxim Yu. Osipov <mosipov@ilbers.de>
---
drivers/watchdog/watchdog_dev.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index ca64724bf59a..2eca04d869d9 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -596,7 +596,6 @@ static int watchdog_release(struct inode *inode, struct file *file)
if (!watchdog_hw_running(wdd))
module_put(wdd->ops->owner);
- cancel_delayed_work_sync(&wdd->work);
watchdog_update_worker(wdd);
/* make sure that /dev/watchdog can be re-opened */
--
2.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [cip-dev] [PATCH 06/10] watchdog: skip min and max timeout validity check when max_hw_heartbeat_ms is defined
2017-10-04 14:40 [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Maxim Yu. Osipov
` (4 preceding siblings ...)
2017-10-04 14:40 ` [cip-dev] [PATCH 05/10] watchdog: core: Fix circular locking dependency Maxim Yu. Osipov
@ 2017-10-04 14:41 ` Maxim Yu. Osipov
2017-10-04 14:41 ` [cip-dev] [PATCH 07/10] watchdog: change watchdog_need_worker logic Maxim Yu. Osipov
` (5 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Maxim Yu. Osipov @ 2017-10-04 14:41 UTC (permalink / raw)
To: cip-dev
From: Pratyush Anand <panand@redhat.com>
Backport from kernel.org, upstream commit 1894cad9bf2c
When max_hw_heartbeat_ms has a none zero value, max_timeout is not used.
So it's value can be 0. In such case if a driver uses min_timeout
functionality, then check will always fail.
This patch fixes above issue.
Signed-off-by: Pratyush Anand <panand@redhat.com>
Signed-off-by: Fu Wei <fu.wei@linaro.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
[mosipov at ilbers.de backported to 4.4.y]
Signed-off-by: Maxim Yu. Osipov <mosipov@ilbers.de>
---
drivers/watchdog/watchdog_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 41f1854d026b..5de14c51bf95 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -88,7 +88,7 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
* Check that we have valid min and max timeout values, if
* not reset them both to 0 (=not used or unknown)
*/
- if (wdd->min_timeout > wdd->max_timeout) {
+ if (!wdd->max_hw_heartbeat_ms && wdd->min_timeout > wdd->max_timeout) {
pr_info("Invalid min and max timeout values, resetting to 0!\n");
wdd->min_timeout = 0;
wdd->max_timeout = 0;
--
2.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [cip-dev] [PATCH 07/10] watchdog: change watchdog_need_worker logic
2017-10-04 14:40 [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Maxim Yu. Osipov
` (5 preceding siblings ...)
2017-10-04 14:41 ` [cip-dev] [PATCH 06/10] watchdog: skip min and max timeout validity check when max_hw_heartbeat_ms is defined Maxim Yu. Osipov
@ 2017-10-04 14:41 ` Maxim Yu. Osipov
2017-10-04 14:41 ` [cip-dev] [PATCH 08/10] watchdog: core: Fix error handling of watchdog_dev_init() Maxim Yu. Osipov
` (4 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Maxim Yu. Osipov @ 2017-10-04 14:41 UTC (permalink / raw)
To: cip-dev
From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Backport from kernel.org, upstream commit 3fbfe9264756
If the driver indicates that the watchdog is running, the framework
should feed it until userspace opens the device, regardless of whether
the driver has set max_hw_heartbeat_ms.
This patch only affects the case where wdd->max_hw_heartbeat_ms is
zero, wdd->timeout is non-zero, the watchdog is not active and the
hardware device is running (*):
- If wdd->timeout is zero, watchdog_need_worker() returns false both
before and after this patch, and watchdog_next_keepalive() is not
called.
- If watchdog_active(wdd), the return value from watchdog_need_worker
is also the same as before (namely, hm && t > hm). Hence in that case,
watchdog_next_keepalive() is only called if hm == max_hw_heartbeat_ms
is non-zero, so the change to min_not_zero there is a no-op.
- If the watchdog is not active and the device is not running, we
return false from watchdog_need_worker just as before.
That leaves the watchdog_hw_running(wdd) && !watchdog_active(wdd) &&
wdd->timeout case. Again, it's easy to see that if
wdd->max_hw_heartbeat_ms is non-zero, we return true from
watchdog_need_worker with and without this patch, and the logic in
watchdog_next_keepalive is unchanged. Finally, if
wdd->max_hw_heartbeat_ms is 0, we used to end up in the
cancel_delayed_work branch, whereas with this patch we end up
scheduling a ping timeout_ms/2 from now.
(*) This should imply that no current kernel drivers are affected,
since the only drivers which explicitly set WDOG_HW_RUNNING are
imx2_wdt.c and dw_wdt.c, both of which also provide a non-zero value
for max_hw_heartbeat_ms. The watchdog core also sets WDOG_HW_RUNNING,
but only when the driver doesn't provide ->stop, in which case it
must, according to Documentation/watchdog/watchdog-kernel-api.txt, set
max_hw_heartbeat_ms.
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
[mosipov at ilbers.de backported to 4.4.y]
Signed-off-by: Maxim Yu. Osipov <mosipov@ilbers.de>
---
drivers/watchdog/watchdog_dev.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 2eca04d869d9..e3eacf27b1e7 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -67,9 +67,13 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
* thus is aware that the framework supports generating heartbeat
* requests.
* - Userspace requests a longer timeout than the hardware can handle.
+ *
+ * Alternatively, if userspace has not opened the watchdog
+ * device, we take care of feeding the watchdog if it is
+ * running.
*/
- return hm && ((watchdog_active(wdd) && t > hm) ||
- (t && !watchdog_active(wdd) && watchdog_hw_running(wdd)));
+ return (hm && watchdog_active(wdd) && t > hm) ||
+ (t && !watchdog_active(wdd) && watchdog_hw_running(wdd));
}
static long watchdog_next_keepalive(struct watchdog_device *wdd)
@@ -81,7 +85,7 @@ static long watchdog_next_keepalive(struct watchdog_device *wdd)
unsigned int hw_heartbeat_ms;
virt_timeout = wdd->last_keepalive + msecs_to_jiffies(timeout_ms);
- hw_heartbeat_ms = min(timeout_ms, wdd->max_hw_heartbeat_ms);
+ hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms);
keepalive_interval = msecs_to_jiffies(hw_heartbeat_ms / 2);
if (!watchdog_active(wdd))
--
2.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [cip-dev] [PATCH 08/10] watchdog: core: Fix error handling of watchdog_dev_init()
2017-10-04 14:40 [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Maxim Yu. Osipov
` (6 preceding siblings ...)
2017-10-04 14:41 ` [cip-dev] [PATCH 07/10] watchdog: change watchdog_need_worker logic Maxim Yu. Osipov
@ 2017-10-04 14:41 ` Maxim Yu. Osipov
2017-10-04 14:41 ` [cip-dev] [PATCH 09/10] watchdog: core: Clear WDOG_HW_RUNNING before calling the stop function Maxim Yu. Osipov
` (3 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Maxim Yu. Osipov @ 2017-10-04 14:41 UTC (permalink / raw)
To: cip-dev
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Backport from kernel.org, upstream commit 138913cb632b
Fix the error handling paths of watchdog_dev_init().
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
[mosipov at ilbers.de backported to 4.4.y]
Signed-off-by: Maxim Yu. Osipov <mosipov@ilbers.de>
---
drivers/watchdog/watchdog_dev.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index e3eacf27b1e7..6cc5ad34c5bc 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -733,8 +733,15 @@ int __init watchdog_dev_init(void)
}
err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
- if (err < 0)
+ if (err < 0) {
pr_err("watchdog: unable to allocate char dev region\n");
+ goto err_alloc;
+ }
+
+ return 0;
+
+err_alloc:
+ destroy_workqueue(watchdog_wq);
return err;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [cip-dev] [PATCH 09/10] watchdog: core: Clear WDOG_HW_RUNNING before calling the stop function
2017-10-04 14:40 [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Maxim Yu. Osipov
` (7 preceding siblings ...)
2017-10-04 14:41 ` [cip-dev] [PATCH 08/10] watchdog: core: Fix error handling of watchdog_dev_init() Maxim Yu. Osipov
@ 2017-10-04 14:41 ` Maxim Yu. Osipov
2017-10-04 14:41 ` [cip-dev] [PATCH 10/10] watchdog: core: add option to avoid early handling of watchdog Maxim Yu. Osipov
` (2 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Maxim Yu. Osipov @ 2017-10-04 14:41 UTC (permalink / raw)
To: cip-dev
From: Guenter Roeck <linux@roeck-us.net>
Backport from kernel.org, upstream commit 3c10bbde10fe
WDOG_HW_RUNNING indicates that the hardware watchdog is running while the
watchdog device is closed. The flag may be set by the driver when it is
instantiated to indicate that the watchdog is running, and that the
watchdog core needs to send heartbeat requests to the driver until the
watchdog device is opened.
When the watchdog device is closed, the flag can be used by the driver's
stop function to indicate to the watchdog core that it was unable to stop
the watchdog, and that the watchdog core needs to send heartbeat requests.
This only works if the flag is actually cleared when the watchdog is
stopped. To avoid having to clear the flag in each driver's stop function,
clear it in the watchdog core before calling the stop function.
Reported-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Fixes: ee142889e32f ("watchdog: Introduce WDOG_HW_RUNNING flag")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
[mosipov at ilbers.de backported to 4.4.y]
Signed-off-by: Maxim Yu. Osipov <mosipov@ilbers.de>
---
drivers/watchdog/watchdog_dev.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 6cc5ad34c5bc..e65a14f213ff 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -241,10 +241,12 @@ static int watchdog_stop(struct watchdog_device *wdd)
goto out_stop;
}
- if (wdd->ops->stop)
+ if (wdd->ops->stop) {
+ clear_bit(WDOG_HW_RUNNING, &wdd->status);
err = wdd->ops->stop(wdd);
- else
+ } else {
set_bit(WDOG_HW_RUNNING, &wdd->status);
+ }
if (err == 0) {
clear_bit(WDOG_ACTIVE, &wdd->status);
--
2.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [cip-dev] [PATCH 10/10] watchdog: core: add option to avoid early handling of watchdog
2017-10-04 14:40 [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Maxim Yu. Osipov
` (8 preceding siblings ...)
2017-10-04 14:41 ` [cip-dev] [PATCH 09/10] watchdog: core: Clear WDOG_HW_RUNNING before calling the stop function Maxim Yu. Osipov
@ 2017-10-04 14:41 ` Maxim Yu. Osipov
2017-10-09 15:00 ` [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Ben Hutchings
2017-10-25 9:46 ` Ben Hutchings
11 siblings, 0 replies; 16+ messages in thread
From: Maxim Yu. Osipov @ 2017-10-04 14:41 UTC (permalink / raw)
To: cip-dev
From: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Backport from kernel.org, upstream commit 2501b015313f
On some systems its desirable to have watchdog reboot the system
when it does not come up fast enough. This adds a kernel parameter
to disable the auto-update of watchdog before userspace takes over
and a kernel option to set the default. The info messages were
added to shorten error searching on misconfigured systems.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
[mosipov at ilbers.de backported to 4.4.y]
Signed-off-by: Maxim Yu. Osipov <mosipov@ilbers.de>
---
drivers/watchdog/Kconfig | 11 +++++++++++
drivers/watchdog/watchdog_dev.c | 21 +++++++++++++++++----
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 1c427beffadd..88743cd0a7e6 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -46,6 +46,17 @@ config WATCHDOG_NOWAYOUT
get killed. If you say Y here, the watchdog cannot be stopped once
it has been started.
+config WATCHDOG_HANDLE_BOOT_ENABLED
+ bool "Update boot-enabled watchdog until userspace takes over"
+ default y
+ help
+ The default watchdog behaviour (which you get if you say Y here) is
+ to ping watchdog devices that were enabled before the driver has
+ been loaded until control is taken over from userspace using the
+ /dev/watchdog file. If you say N here, the kernel will not update
+ the watchdog on its own. Thus if your userspace does not start fast
+ enough your device will reboot.
+
#
# General Watchdog drivers
#
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index e65a14f213ff..28eaed17c4fe 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -53,6 +53,9 @@ static struct watchdog_device *old_wdd;
static struct workqueue_struct *watchdog_wq;
+static bool handle_boot_enabled =
+ IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
+
static inline bool watchdog_need_worker(struct watchdog_device *wdd)
{
/* All variables in milli-seconds */
@@ -685,10 +688,15 @@ int watchdog_dev_register(struct watchdog_device *wdd)
* and schedule an immediate ping.
*/
if (watchdog_hw_running(wdd)) {
- __module_get(wdd->ops->owner);
- if (wdd->ops->ref)
- wdd->ops->ref(wdd);
- queue_delayed_work(watchdog_wq, &wdd->work, 0);
+ if (handle_boot_enabled) {
+ __module_get(wdd->ops->owner);
+ if (wdd->ops->ref)
+ wdd->ops->ref(wdd);
+ queue_delayed_work(watchdog_wq, &wdd->work, 0);
+ } else {
+ pr_info("watchdog%d running and kernel based pre-userspace handler disabled\n",
+ wdd->id);
+ }
}
return 0;
@@ -758,3 +766,8 @@ void __exit watchdog_dev_exit(void)
unregister_chrdev_region(watchdog_devt, MAX_DOGS);
destroy_workqueue(watchdog_wq);
}
+
+module_param(handle_boot_enabled, bool, 0444);
+MODULE_PARM_DESC(handle_boot_enabled,
+ "Watchdog core auto-updates boot enabled watchdogs before userspace takes over (default="
+ __MODULE_STRING(IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED)) ")");
--
2.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure
2017-10-04 14:40 [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Maxim Yu. Osipov
` (9 preceding siblings ...)
2017-10-04 14:41 ` [cip-dev] [PATCH 10/10] watchdog: core: add option to avoid early handling of watchdog Maxim Yu. Osipov
@ 2017-10-09 15:00 ` Ben Hutchings
2017-10-09 15:26 ` Jan Kiszka
2017-10-25 9:46 ` Ben Hutchings
11 siblings, 1 reply; 16+ messages in thread
From: Ben Hutchings @ 2017-10-09 15:00 UTC (permalink / raw)
To: cip-dev
On Wed, 2017-10-04 at 16:40 +0200, Maxim Yu. Osipov wrote:
> Hello Ben,
>
> This series contains
>
> * backport of patches of watchdog core infrastructure
> supporting handling watchdog keepalive.
>
> * imx21_wdt converted to use infrastructure triggered keepalives.
>
> * backported support of WATCHDOG_HANDLE_BOOT_ENABLED option
[...]
Are you working for Siemens on this? Can you provide the config you are
using?
The reason I ask is that CIP kernel branches will have hardware support
limited to the needs of members. Currently, none of the kernel
configurations I collected enable CONFIG_IMX2_WDT, or even
CONFIG_ARCH_MXC, which implies that imx2_wdt won't be supported.
Ben.
--
Ben Hutchings
Software Developer, Codethink Ltd.
^ permalink raw reply [flat|nested] 16+ messages in thread* [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure
2017-10-09 15:00 ` [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Ben Hutchings
@ 2017-10-09 15:26 ` Jan Kiszka
0 siblings, 0 replies; 16+ messages in thread
From: Jan Kiszka @ 2017-10-09 15:26 UTC (permalink / raw)
To: cip-dev
On 2017-10-09 17:00, Ben Hutchings wrote:
> On Wed, 2017-10-04 at 16:40 +0200, Maxim Yu. Osipov wrote:
>> Hello Ben,
>>
>> This series contains
>>
>> * backport of patches of watchdog core infrastructure
>> supporting handling watchdog keepalive.
>>
>> * imx21_wdt converted to use infrastructure triggered keepalives.
>>
>> * backported support of WATCHDOG_HANDLE_BOOT_ENABLED option
> [...]
>
> Are you working for Siemens on this? Can you provide the config you are
> using?
Yes, Maxim is working for a project of us, and I asked him to propose
these backports for CIP. They will likely become relevant for other
boards as well because the bootloader-started watchdog is a cornerstone
of safe system updates.
>
> The reason I ask is that CIP kernel branches will have hardware support
> limited to the needs of members. Currently, none of the kernel
> configurations I collected enable CONFIG_IMX2_WDT, or even
> CONFIG_ARCH_MXC, which implies that imx2_wdt won't be supported.
Yeah, this config wasn't included in the set I sent back then. Maxim,
please provide it to Ben. We are in the process of migrating to the CIP
kernel right now.
Thanks,
Jan
--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 16+ messages in thread
* [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure
2017-10-04 14:40 [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Maxim Yu. Osipov
` (10 preceding siblings ...)
2017-10-09 15:00 ` [cip-dev] [PATCH 00/10] Backport of watchdog core triggered keepalive infrastructure Ben Hutchings
@ 2017-10-25 9:46 ` Ben Hutchings
11 siblings, 0 replies; 16+ messages in thread
From: Ben Hutchings @ 2017-10-25 9:46 UTC (permalink / raw)
To: cip-dev
Please use one of these formats for upstream references in the commit
messages:
commit 0123456789012345678901234567890123456789 upstream.
[ Upstream commit 0123456789012345678901234567890123456789 ]
Ben.
--
Ben Hutchings
Software Developer, Codethink Ltd.
^ permalink raw reply [flat|nested] 16+ messages in thread