* [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure
@ 2016-01-26 2:53 Guenter Roeck
2016-01-26 2:53 ` [PATCH v7 1/9] watchdog: Introduce hardware maximum timeout in watchdog core Guenter Roeck
` (9 more replies)
0 siblings, 10 replies; 19+ messages in thread
From: Guenter Roeck @ 2016-01-26 2:53 UTC (permalink / raw)
To: linux-watchdog
Cc: Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Doug Anderson, Jonathan Corbet,
Guenter Roeck
From: Guenter Roeck <linux@roeck-us.net>
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_HW_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.
As part of this patchset, the semantics of the 'timeout' variable and of
the WDOG_ACTIVE flag are changed slightly.
Per the current watchdog kernel API, the 'timeout' variable is supposed
to reflect the actual hardware watcdog timeout. WDOG_ACTIVE is supposed
to reflect if the hardware watchdog is running or not.
Unfortunately, this does not always reflect reality. In drivers which solve
the above mentioned problems internally, 'timeout' is the watchdog timeout
as seen from user space, and WDOG_ACTIVE reflects that user space is expected
to send keepalive requests to the watchdog driver.
After this patch set is applied, this so far inofficial interpretation
is the 'official' semantics for the timeout variable and the WDOG_ACTIVE
flag. In other words, both values no longer reflect the hardware watchdog
status, but its status as seen from user space.
Patch #1 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 #2 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 #3 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 #4 adds code to ensure that the minimum time between heartbeats meets
constraints provided by the watchdog driver.
Patch #5 simplifies the watchdog_update_worker() function introduced with
patch #1 to only take a single argument, and to always cancel any pending
work if a worker is not or no longer needed. This patch is kept as separate
patch on purpose, to enable dropping or reverting it easily if it causes
any problems. It should not cause any problems; this is just out of an
abundance of caution.
Patch #6 to #9 are example conversions of some watchdog drivers.
Those patches will require testing and are marked as RFT.
The patch series is also available in branch watchdog-timer of
git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git.
The series is curently based on top of v4.5-rc1.
The patch series was inspired by an earlier patch set from Timo Kokonnen.
v7:
- Rebased to v4.5-rc1.
- Moved new variables to local data structure
- Fixed typos in documentation (hw_max_timeout_ms -> max_hw_timeout_ms).
- Enforce that max_hw_timeout_ms must be set if the stop function is
not implemented by a driver.
- Set max_hw_timeout_ms in converted drivers.
v6:
- Added patch #9, converting the dw_wdt driver.
- Set last_keepalive more accurately when starting the watchdog.
- Rebased to v4.4-rc2.
- Renamed WDOG_RUNNING to WDOG_HW_RUNNING and watchdog_running() to
watchdog_hw_running().
v5:
- Patches #1 and #2 of the original patch series are now in mainline
and have been dropped.
- Rebased to v4.4-rc1.
- Added patch to simplify watchdog_update_worker().
v4:
- Rebased to v4.3-rc3
- Rearranged patch sequence
- Dropped gpio driver patch. The driver was changed since v4.2,
and merging the changes turned out to be too difficult.
- Various other cleanups as listed in individual patches
v3:
- Rebased to v4.2-rc8
- Reworked and cleaned up some of the functions.
- No longer call the worker update function if all that is needed is to stop
the worker.
- max_timeout will now be ignored if max_hw_timeout_ms is provided.
- Added patch 9/9.
v2:
- Rebased to v4.2-rc5
- Improved and hopefully clarified documentation.
- Rearranged variables in struct watchdog_device such that internal variables
come last.
- The code now ensures that the watchdog times out <timeout> seconds after
the most recent keepalive sent from user space.
- The internal keepalive now stops silently and no longer generates a
warning message. Reason is that it will now stop early, while there
may still be a substantial amount of time for keepalives from user space
to arrive. If such keepalives arrive late (for example if user space
is configured to send keepalives just a few seconds before the watchdog
times out), the message would just be noise and not provide any value.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v7 1/9] watchdog: Introduce hardware maximum timeout in watchdog core
2016-01-26 2:53 [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Guenter Roeck
@ 2016-01-26 2:53 ` Guenter Roeck
2016-02-28 13:18 ` Wim Van Sebroeck
2016-01-26 2:53 ` [PATCH v7 2/9] watchdog: Introduce WDOG_HW_RUNNING flag Guenter Roeck
` (8 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Guenter Roeck @ 2016-01-26 2:53 UTC (permalink / raw)
To: linux-watchdog
Cc: Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Doug Anderson, Jonathan Corbet,
Guenter Roeck
From: Guenter Roeck <linux@roeck-us.net>
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 hardware timeout value in the watchdog data
structure. If the configured timeout exceeds the maximum hardware timeout,
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>
---
v7:
- Rebased to v4.5-rc1
- Moved new variables to local data structure
- Dropped Uwe's Acked-by: due to substantial changes
v6:
- Set last_keepalive more accurately when starting the watchdog
- Rebased to v4.4-rc2
- Added Uwe's Acked-by:
v5:
- Rebased to v4.4-rc1
v4:
- Improved and fixed documentation
- Split hw_timeout_ms variable to timeout_ms, hw_timeout_ms for clarity
- Dropped redundant comments
- Added comments explaining failure conditions in watchdog_timeout_invalid().
- Moved the call to watchdog_update_worker() into _watchdog_ping().
v3:
- Reworked and cleaned up some of the functions.
- No longer call the worker update function if all that is needed is to stop
the worker.
- max_timeout will now be ignored if max_hw_timeout_ms is provided.
v2:
- Improved and hopefully clarified documentation.
- Rearranged variables in struct watchdog_device such that internal variables
come last.
- The code now ensures that the watchdog times out <timeout> seconds after
the most recent keepalive sent from user space.
- The internal keepalive now stops silently and no longer generates a
warning message. Reason is that it will now stop early, while there
may still be a substantial amount of time for keepalives from user space
to arrive. If such keepalives arrive late (for example if user space
is configured to send keepalives just a few seconds before the watchdog
times out), the message would just be noise and not provide any value.
---
Documentation/watchdog/watchdog-kernel-api.txt | 19 +++-
drivers/watchdog/watchdog_dev.c | 136 +++++++++++++++++++++++--
include/linux/watchdog.h | 28 +++--
3 files changed, 164 insertions(+), 19 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 55120a055a14..46979568b9e5 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -52,6 +52,7 @@ struct watchdog_device {
unsigned int timeout;
unsigned int min_timeout;
unsigned int max_timeout;
+ unsigned int max_hw_timeout_ms;
struct notifier_block reboot_nb;
struct notifier_block restart_nb;
void *driver_data;
@@ -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_timeout_ms is non-zero.
+* max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds.
+ If set, the infrastructure will send heartbeats to the watchdog driver
+ if 'timeout' is larger than max_hw_timeout_ms, unless WDOG_ACTIVE
+ is set and userspace failed to send a heartbeat for at least 'timeout'
+ seconds.
* reboot_nb: notifier block that is registered for reboot notifications, for
internal use only. If the driver calls watchdog_stop_on_reboot, watchdog core
will stop the watchdog on such notifications.
@@ -153,7 +164,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_timeout_ms set the hardware watchdog timeout
+ to the minimum of timeout and max_hw_timeout_ms. Those drivers set the
+ timeout value of the watchdog_device either to the requested timeout value
+ (if it is larger than max_hw_timeout_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 ba2ecce4aae6..20e4ce0ebf6c 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -36,6 +36,7 @@
#include <linux/errno.h> /* For the -ENODEV/... values */
#include <linux/fs.h> /* For file operations */
#include <linux/init.h> /* For __init/__exit/... */
+#include <linux/jiffies.h> /* For timeout functions */
#include <linux/kernel.h> /* For printk/panic/... */
#include <linux/kref.h> /* For data references */
#include <linux/miscdevice.h> /* For handling misc devices */
@@ -44,6 +45,7 @@
#include <linux/slab.h> /* For memory functions */
#include <linux/types.h> /* For standard types (like size_t) */
#include <linux/watchdog.h> /* For watchdog specific items */
+#include <linux/workqueue.h> /* For workqueue */
#include <linux/uaccess.h> /* For copy_to_user/put_user/... */
#include "watchdog_core.h"
@@ -61,6 +63,8 @@ struct watchdog_core_data {
struct cdev cdev;
struct watchdog_device *wdd;
struct mutex lock;
+ unsigned long last_keepalive;
+ struct delayed_work work;
unsigned long status; /* Internal status bits */
#define _WDOG_DEV_OPEN 0 /* Opened ? */
#define _WDOG_ALLOW_RELEASE 1 /* Did we receive the magic char ? */
@@ -71,6 +75,77 @@ static dev_t watchdog_devt;
/* Reference to watchdog device behind /dev/watchdog */
static struct watchdog_core_data *old_wd_data;
+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_timeout_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)
+{
+ struct watchdog_core_data *wd_data = wdd->wd_data;
+ unsigned int timeout_ms = wdd->timeout * 1000;
+ unsigned long keepalive_interval;
+ unsigned long last_heartbeat;
+ unsigned long virt_timeout;
+ unsigned int hw_timeout_ms;
+
+ virt_timeout = wd_data->last_keepalive + msecs_to_jiffies(timeout_ms);
+ hw_timeout_ms = min(timeout_ms, wdd->max_hw_timeout_ms);
+ keepalive_interval = msecs_to_jiffies(hw_timeout_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_timeout_ms before this timeout.
+ */
+ last_heartbeat = virt_timeout - msecs_to_jiffies(hw_timeout_ms);
+ return min_t(long, last_heartbeat - jiffies, keepalive_interval);
+}
+
+static inline void watchdog_update_worker(struct watchdog_device *wdd,
+ bool cancel)
+{
+ struct watchdog_core_data *wd_data = wdd->wd_data;
+
+ if (watchdog_need_worker(wdd)) {
+ long t = watchdog_next_keepalive(wdd);
+
+ if (t > 0)
+ mod_delayed_work(watchdog_wq, &wd_data->work, t);
+ } else if (cancel) {
+ cancel_delayed_work(&wd_data->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, false);
+
+ return err;
+}
+
/*
* watchdog_ping: ping the watchdog.
* @wdd: the watchdog device to ping
@@ -85,17 +160,28 @@ static struct watchdog_core_data *old_wd_data;
static int watchdog_ping(struct watchdog_device *wdd)
{
- int err;
+ struct watchdog_core_data *wd_data = wdd->wd_data;
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 */
+ wd_data->last_keepalive = jiffies;
+ return __watchdog_ping(wdd);
+}
- return err;
+static void watchdog_ping_work(struct work_struct *work)
+{
+ struct watchdog_core_data *wd_data;
+ struct watchdog_device *wdd;
+
+ wd_data = container_of(to_delayed_work(work), struct watchdog_core_data,
+ work);
+
+ mutex_lock(&wd_data->lock);
+ wdd = wd_data->wdd;
+ if (wdd && watchdog_active(wdd))
+ __watchdog_ping(wdd);
+ mutex_unlock(&wd_data->lock);
}
/*
@@ -111,14 +197,20 @@ static int watchdog_ping(struct watchdog_device *wdd)
static int watchdog_start(struct watchdog_device *wdd)
{
+ struct watchdog_core_data *wd_data = wdd->wd_data;
+ unsigned long started_at;
int err;
if (watchdog_active(wdd))
return 0;
+ started_at = jiffies;
err = wdd->ops->start(wdd);
- if (err == 0)
+ if (err == 0) {
set_bit(WDOG_ACTIVE, &wdd->status);
+ wd_data->last_keepalive = started_at;
+ watchdog_update_worker(wdd, true);
+ }
return err;
}
@@ -137,6 +229,7 @@ static int watchdog_start(struct watchdog_device *wdd)
static int watchdog_stop(struct watchdog_device *wdd)
{
+ struct watchdog_core_data *wd_data = wdd->wd_data;
int err;
if (!watchdog_active(wdd))
@@ -149,8 +242,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(&wd_data->work);
+ }
return err;
}
@@ -183,13 +278,19 @@ static unsigned int watchdog_get_status(struct watchdog_device *wdd)
static int watchdog_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)
{
+ int err;
+
if (!wdd->ops->set_timeout || !(wdd->info->options & WDIOF_SETTIMEOUT))
return -EOPNOTSUPP;
if (watchdog_timeout_invalid(wdd, timeout))
return -EINVAL;
- return wdd->ops->set_timeout(wdd, timeout);
+ err = wdd->ops->set_timeout(wdd, timeout);
+
+ watchdog_update_worker(wdd, true);
+
+ return err;
}
/*
@@ -609,6 +710,8 @@ static int watchdog_release(struct inode *inode, struct file *file)
watchdog_ping(wdd);
}
+ cancel_delayed_work_sync(&wd_data->work);
+
/* make sure that /dev/watchdog can be re-opened */
clear_bit(_WDOG_DEV_OPEN, &wd_data->status);
@@ -658,6 +761,11 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
wd_data->wdd = wdd;
wdd->wd_data = wd_data;
+ if (!watchdog_wq)
+ return -ENODEV;
+
+ INIT_DELAYED_WORK(&wd_data->work, watchdog_ping_work);
+
if (wdd->id == 0) {
old_wd_data = wd_data;
watchdog_miscdev.parent = wdd->parent;
@@ -715,6 +823,8 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
wdd->wd_data = NULL;
mutex_unlock(&wd_data->lock);
+ cancel_delayed_work_sync(&wd_data->work);
+
kref_put(&wd_data->kref, watchdog_core_data_release);
}
@@ -780,6 +890,13 @@ int __init watchdog_dev_init(void)
{
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 = class_register(&watchdog_class);
if (err < 0) {
pr_err("couldn't register class\n");
@@ -806,4 +923,5 @@ void __exit watchdog_dev_exit(void)
{
unregister_chrdev_region(watchdog_devt, MAX_DOGS);
class_unregister(&watchdog_class);
+ destroy_workqueue(watchdog_wq);
}
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index b585fa2507ee..cd5e6f84bf2f 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 <linux/notifier.h>
#include <uapi/linux/watchdog.h>
@@ -61,14 +62,19 @@ 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_timeout_ms is not provided.
+ * @max_hw_timeout_ms:
+ * Hardware limit for maximum timeout, in milli-seconds.
+ * Replaces max_timeout if specified.
* @reboot_nb: The notifier block to stop watchdog on reboot.
* @restart_nb: The notifier block to register a restart function.
* @driver_data:Pointer to the drivers private data.
* @wd_data: Pointer to watchdog core internal data.
* @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.
@@ -89,6 +95,7 @@ struct watchdog_device {
unsigned int timeout;
unsigned int min_timeout;
unsigned int max_timeout;
+ unsigned int max_hw_timeout_ms;
struct notifier_block reboot_nb;
struct notifier_block restart_nb;
void *driver_data;
@@ -128,13 +135,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_timeout_ms && wdd->max_timeout &&
+ t > wdd->max_timeout);
}
/* Use the following functions to manipulate watchdog driver specific data */
--
2.1.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v7 2/9] watchdog: Introduce WDOG_HW_RUNNING flag
2016-01-26 2:53 [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Guenter Roeck
2016-01-26 2:53 ` [PATCH v7 1/9] watchdog: Introduce hardware maximum timeout in watchdog core Guenter Roeck
@ 2016-01-26 2:53 ` Guenter Roeck
2016-01-26 2:53 ` [PATCH v7 3/9] watchdog: Make set_timeout function optional Guenter Roeck
` (7 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2016-01-26 2:53 UTC (permalink / raw)
To: linux-watchdog
Cc: Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Doug Anderson, Jonathan Corbet,
Guenter Roeck
From: Guenter Roeck <linux@roeck-us.net>
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>
---
v7: Fix typos in documentation (hw_max_timeout_ms -> max_hw_timeout_ms).
Enforce that max_hw_timeout_ms must be set if the stop function is
not implemented by a driver.
Rebased to v4.5-rc1
v6: Fixed typo in documentation.
Renamed WDOG_RUNNING to WDOG_HW_RUNNING and watchdog_running() to
watchdog_hw_running().
Rebased to v4.4-rc2.
v5: Rebased to v4.4-rc1.
v4: No change.
v3: Clarified meaning of WDOG_ACTIVE.
Do not call cancel_delayed_work_sync() from watchdog_update_worker().
Call it directly where needed instead, to keep the common code simple.
Do not (re-)start an already running watchdog when opening the watchdog
device. Send a heartbeat request instead.
v2: Improved documentation.
---
Documentation/watchdog/watchdog-kernel-api.txt | 34 ++++++++++-----
drivers/watchdog/watchdog_core.c | 2 +-
drivers/watchdog/watchdog_dev.c | 58 +++++++++++++++++++-------
include/linux/watchdog.h | 10 +++++
4 files changed, 78 insertions(+), 26 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 46979568b9e5..738bd6809328 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_timeout_ms, unless WDOG_ACTIVE
is set and userspace failed to send a heartbeat for at least 'timeout'
- seconds.
+ seconds. max_hw_timeout_ms must be set if a driver does not implement
+ the stop function.
* reboot_nb: notifier block that is registered for reboot notifications, for
internal use only. If the driver calls watchdog_stop_on_reboot, watchdog core
will stop the watchdog on such notifications.
@@ -134,17 +135,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. 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_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_timeout_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
@@ -184,11 +188,19 @@ The 'ref' and 'unref' operations are no longer used and deprecated.
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_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.
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 e600fd93b7de..07c9931e2123 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -199,7 +199,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_timeout_ms))
return -EINVAL;
watchdog_check_min_max_timeout(wdd);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 20e4ce0ebf6c..469b13b8363b 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -92,7 +92,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)
@@ -108,6 +109,9 @@ static long watchdog_next_keepalive(struct watchdog_device *wdd)
hw_timeout_ms = min(timeout_ms, wdd->max_hw_timeout_ms);
keepalive_interval = msecs_to_jiffies(hw_timeout_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
@@ -162,7 +166,7 @@ static int watchdog_ping(struct watchdog_device *wdd)
{
struct watchdog_core_data *wd_data = wdd->wd_data;
- if (!watchdog_active(wdd))
+ if (!watchdog_active(wdd) && !watchdog_hw_running(wdd))
return 0;
wd_data->last_keepalive = jiffies;
@@ -179,7 +183,7 @@ static void watchdog_ping_work(struct work_struct *work)
mutex_lock(&wd_data->lock);
wdd = wd_data->wdd;
- if (wdd && watchdog_active(wdd))
+ if (wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd)))
__watchdog_ping(wdd);
mutex_unlock(&wd_data->lock);
}
@@ -205,7 +209,10 @@ static int watchdog_start(struct watchdog_device *wdd)
return 0;
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);
wd_data->last_keepalive = started_at;
@@ -229,8 +236,7 @@ static int watchdog_start(struct watchdog_device *wdd)
static int watchdog_stop(struct watchdog_device *wdd)
{
- struct watchdog_core_data *wd_data = wdd->wd_data;
- int err;
+ int err = 0;
if (!watchdog_active(wdd))
return 0;
@@ -241,10 +247,14 @@ static int watchdog_stop(struct watchdog_device *wdd)
return -EBUSY;
}
- 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);
- cancel_delayed_work(&wd_data->work);
+ watchdog_update_worker(wdd, true);
}
return err;
@@ -639,7 +649,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)) {
err = -EBUSY;
goto out_clear;
}
@@ -650,7 +660,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
file->private_data = wd_data;
- kref_get(&wd_data->kref);
+ if (!watchdog_hw_running(wdd))
+ kref_get(&wd_data->kref);
/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
return nonseekable_open(inode, file);
@@ -711,15 +722,22 @@ static int watchdog_release(struct inode *inode, struct file *file)
}
cancel_delayed_work_sync(&wd_data->work);
+ watchdog_update_worker(wdd, false);
/* make sure that /dev/watchdog can be re-opened */
clear_bit(_WDOG_DEV_OPEN, &wd_data->status);
done:
mutex_unlock(&wd_data->lock);
- /* Allow the owner module to be unloaded again */
- module_put(wd_data->cdev.owner);
- kref_put(&wd_data->kref, watchdog_core_data_release);
+ /*
+ * 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);
+ kref_put(&wd_data->kref, watchdog_core_data_release);
+ }
return 0;
}
@@ -796,8 +814,20 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
old_wd_data = NULL;
kref_put(&wd_data->kref, watchdog_core_data_release);
}
+ 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);
+ kref_get(&wd_data->kref);
+ queue_delayed_work(watchdog_wq, &wd_data->work, 0);
+ }
+
+ return 0;
}
/*
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index cd5e6f84bf2f..19c675eed80f 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -105,6 +105,7 @@ struct watchdog_device {
#define WDOG_ACTIVE 0 /* Is the watchdog running/active */
#define WDOG_NO_WAY_OUT 1 /* Is 'nowayout' feature set ? */
#define WDOG_STOP_ON_REBOOT 2 /* Should be stopped on reboot */
+#define WDOG_HW_RUNNING 3 /* True if HW watchdog running */
struct list_head deferred;
};
@@ -117,6 +118,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.1.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v7 3/9] watchdog: Make set_timeout function optional
2016-01-26 2:53 [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Guenter Roeck
2016-01-26 2:53 ` [PATCH v7 1/9] watchdog: Introduce hardware maximum timeout in watchdog core Guenter Roeck
2016-01-26 2:53 ` [PATCH v7 2/9] watchdog: Introduce WDOG_HW_RUNNING flag Guenter Roeck
@ 2016-01-26 2:53 ` Guenter Roeck
2016-01-26 2:53 ` [PATCH v7 4/9] watchdog: Add support for minimum time between heartbeats Guenter Roeck
` (6 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2016-01-26 2:53 UTC (permalink / raw)
To: linux-watchdog
Cc: Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Doug Anderson, Jonathan Corbet,
Guenter Roeck
From: Guenter Roeck <linux@roeck-us.net>
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.
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v7: Rebased to v4.5-rc1
v6: Rebased to v4.4-rc2
v5: Rebased to v4.4-rc1
v4: No changes
v3: No changes
v2: No changes
---
Documentation/watchdog/watchdog-kernel-api.txt | 5 +++++
drivers/watchdog/watchdog_dev.c | 9 ++++++---
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 738bd6809328..62d49d6a7ff5 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -175,6 +175,11 @@ they are supported. These optional routines/operations are:
(if it is larger than max_hw_timeout_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).
+ If the watchdog driver does not have to perform any action but setting the
+ watchdog_device.timeout, 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
+ to the requested value.
* get_timeleft: this routines returns the time that's left before a reset.
* restart: this routine restarts the machine. It returns 0 on success or a
negative errno code for failure.
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 469b13b8363b..1a8f3861fe92 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -288,15 +288,18 @@ static unsigned int watchdog_get_status(struct watchdog_device *wdd)
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))
return -EINVAL;
- 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);
--
2.1.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v7 4/9] watchdog: Add support for minimum time between heartbeats
2016-01-26 2:53 [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Guenter Roeck
` (2 preceding siblings ...)
2016-01-26 2:53 ` [PATCH v7 3/9] watchdog: Make set_timeout function optional Guenter Roeck
@ 2016-01-26 2:53 ` Guenter Roeck
2016-01-26 8:07 ` Uwe Kleine-König
2016-01-26 2:53 ` [PATCH v7 5/9] watchdog: Simplify update_worker Guenter Roeck
` (5 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Guenter Roeck @ 2016-01-26 2:53 UTC (permalink / raw)
To: linux-watchdog
Cc: Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Doug Anderson, Jonathan Corbet,
Guenter Roeck
From: Guenter Roeck <linux@roeck-us.net>
Some watchdogs require a minimum time between heartbeats.
Examples are the watchdogs in DA9062 and AT91SAM9x.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v7: Rebased to v4.5-rc1
v6: Rebased to v4.4-rc2
v5: Rebased to v4.4-rc1
Fixed typo in documentation.
v4: Added patch
---
Documentation/watchdog/watchdog-kernel-api.txt | 3 +++
drivers/watchdog/watchdog_dev.c | 15 +++++++++++++++
include/linux/watchdog.h | 3 +++
3 files changed, 21 insertions(+)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 62d49d6a7ff5..2221fb4f2739 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -52,6 +52,7 @@ struct watchdog_device {
unsigned int timeout;
unsigned int min_timeout;
unsigned int max_timeout;
+ unsigned int min_hw_heartbeat_ms;
unsigned int max_hw_timeout_ms;
struct notifier_block reboot_nb;
struct notifier_block restart_nb;
@@ -81,6 +82,8 @@ It contains following fields:
* 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_timeout_ms is non-zero.
+* min_hw_heartbeat_ms: Minimum time between heartbeats sent to the chip,
+ in milli-seconds.
* max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds.
If set, the infrastructure will send heartbeats to the watchdog driver
if 'timeout' is larger than max_hw_timeout_ms, unless WDOG_ACTIVE
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 1a8f3861fe92..ff03cbc8e081 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -64,6 +64,7 @@ struct watchdog_core_data {
struct watchdog_device *wdd;
struct mutex lock;
unsigned long last_keepalive;
+ unsigned long last_hw_keepalive;
struct delayed_work work;
unsigned long status; /* Internal status bits */
#define _WDOG_DEV_OPEN 0 /* Opened ? */
@@ -138,8 +139,19 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd,
static int __watchdog_ping(struct watchdog_device *wdd)
{
+ struct watchdog_core_data *wd_data = wdd->wd_data;
+ unsigned long earliest_keepalive = wd_data->last_hw_keepalive +
+ msecs_to_jiffies(wdd->min_hw_heartbeat_ms);
int err;
+ if (time_is_after_jiffies(earliest_keepalive)) {
+ mod_delayed_work(watchdog_wq, &wd_data->work,
+ earliest_keepalive - jiffies);
+ return 0;
+ }
+
+ wd_data->last_hw_keepalive = jiffies;
+
if (wdd->ops->ping)
err = wdd->ops->ping(wdd); /* ping the watchdog */
else
@@ -820,6 +832,9 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
return err;
}
+ /* Record time of most recent heartbeat as 'just before now'. */
+ wd_data->last_hw_keepalive = jiffies - 1;
+
/*
* If the watchdog is running, prevent its driver from being unloaded,
* and schedule an immediate ping.
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 19c675eed80f..537e84e2592a 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -65,6 +65,8 @@ struct watchdog_ops {
* @max_timeout:The watchdog devices maximum timeout value (in seconds)
* as configurable from user space. Only relevant if
* max_hw_timeout_ms is not provided.
+ * @min_hw_heartbeat_ms:
+ * Minimum time between heartbeats, in milli-seconds.
* @max_hw_timeout_ms:
* Hardware limit for maximum timeout, in milli-seconds.
* Replaces max_timeout if specified.
@@ -95,6 +97,7 @@ struct watchdog_device {
unsigned int timeout;
unsigned int min_timeout;
unsigned int max_timeout;
+ unsigned int min_hw_heartbeat_ms;
unsigned int max_hw_timeout_ms;
struct notifier_block reboot_nb;
struct notifier_block restart_nb;
--
2.1.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v7 5/9] watchdog: Simplify update_worker
2016-01-26 2:53 [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Guenter Roeck
` (3 preceding siblings ...)
2016-01-26 2:53 ` [PATCH v7 4/9] watchdog: Add support for minimum time between heartbeats Guenter Roeck
@ 2016-01-26 2:53 ` Guenter Roeck
2016-01-26 2:53 ` [RFT PATCH v7 6/9] watchdog: imx2: Convert to use infrastructure triggered keepalives Guenter Roeck
` (4 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2016-01-26 2:53 UTC (permalink / raw)
To: linux-watchdog
Cc: Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Doug Anderson, Jonathan Corbet,
Guenter Roeck
From: Guenter Roeck <linux@roeck-us.net>
Drop 'cancel' parameter; simply cancel worker unconditionally
if not needed.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v7: Rebased to v4.5-rc1
v6: Rebased to v4.4-rc2
v5: Introduced patch
---
drivers/watchdog/watchdog_dev.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index ff03cbc8e081..47e854be862c 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -122,8 +122,7 @@ static long watchdog_next_keepalive(struct watchdog_device *wdd)
return min_t(long, last_heartbeat - jiffies, keepalive_interval);
}
-static inline void watchdog_update_worker(struct watchdog_device *wdd,
- bool cancel)
+static inline void watchdog_update_worker(struct watchdog_device *wdd)
{
struct watchdog_core_data *wd_data = wdd->wd_data;
@@ -132,7 +131,7 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd,
if (t > 0)
mod_delayed_work(watchdog_wq, &wd_data->work, t);
- } else if (cancel) {
+ } else {
cancel_delayed_work(&wd_data->work);
}
}
@@ -157,7 +156,7 @@ static int __watchdog_ping(struct watchdog_device *wdd)
else
err = wdd->ops->start(wdd); /* restart watchdog */
- watchdog_update_worker(wdd, false);
+ watchdog_update_worker(wdd);
return err;
}
@@ -228,7 +227,7 @@ static int watchdog_start(struct watchdog_device *wdd)
if (err == 0) {
set_bit(WDOG_ACTIVE, &wdd->status);
wd_data->last_keepalive = started_at;
- watchdog_update_worker(wdd, true);
+ watchdog_update_worker(wdd);
}
return err;
@@ -266,7 +265,7 @@ static int watchdog_stop(struct watchdog_device *wdd)
if (err == 0) {
clear_bit(WDOG_ACTIVE, &wdd->status);
- watchdog_update_worker(wdd, true);
+ watchdog_update_worker(wdd);
}
return err;
@@ -313,7 +312,7 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
else
wdd->timeout = timeout;
- watchdog_update_worker(wdd, true);
+ watchdog_update_worker(wdd);
return err;
}
@@ -737,7 +736,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
}
cancel_delayed_work_sync(&wd_data->work);
- watchdog_update_worker(wdd, false);
+ watchdog_update_worker(wdd);
/* make sure that /dev/watchdog can be re-opened */
clear_bit(_WDOG_DEV_OPEN, &wd_data->status);
--
2.1.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFT PATCH v7 6/9] watchdog: imx2: Convert to use infrastructure triggered keepalives
2016-01-26 2:53 [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Guenter Roeck
` (4 preceding siblings ...)
2016-01-26 2:53 ` [PATCH v7 5/9] watchdog: Simplify update_worker Guenter Roeck
@ 2016-01-26 2:53 ` Guenter Roeck
2016-01-29 19:40 ` Guenter Roeck
2016-01-26 2:53 ` [RFT PATCH v7 7/9] watchdog: retu: " Guenter Roeck
` (3 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Guenter Roeck @ 2016-01-26 2:53 UTC (permalink / raw)
To: linux-watchdog
Cc: Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Doug Anderson, Jonathan Corbet,
Guenter Roeck
From: Guenter Roeck <linux@roeck-us.net>
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>
---
v7: Set max_hw_timeout_ms
Rebased to v4.5-rc1
v6: Rename WDOG_RUNNING to WDOG_HW_RUNNING
Rebased to v4.4-rc2
v5: Rebased to v4.4-rc1
v4: No changes
v3: No changes
v2: No changes
---
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 e47966aa2db0..1a48cb28718f 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -25,14 +25,12 @@
#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>
#include <linux/of_address.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
-#include <linux/timer.h>
#include <linux/watchdog.h>
#define DRIVER_NAME "imx2-wdt"
@@ -60,7 +58,6 @@
struct imx2_wdt_device {
struct clk *clk;
struct regmap *regmap;
- struct timer_list timer; /* Pings the watchdog when closed */
struct watchdog_device wdog;
};
@@ -146,16 +143,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)
{
@@ -172,40 +159,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,
.restart = imx2_wdt_restart,
@@ -253,7 +219,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_timeout_ms = IMX2_WDT_MAX_TIME * 1000;
wdog->parent = &pdev->dev;
ret = clk_prepare_enable(wdev->clk);
@@ -274,9 +240,10 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
watchdog_set_restart_priority(wdog, 128);
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
@@ -309,7 +276,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");
}
@@ -323,10 +289,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");
@@ -344,10 +309,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);
@@ -373,19 +334,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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFT PATCH v7 7/9] watchdog: retu: Convert to use infrastructure triggered keepalives
2016-01-26 2:53 [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Guenter Roeck
` (5 preceding siblings ...)
2016-01-26 2:53 ` [RFT PATCH v7 6/9] watchdog: imx2: Convert to use infrastructure triggered keepalives Guenter Roeck
@ 2016-01-26 2:53 ` Guenter Roeck
2016-01-26 2:53 ` [RFT PATCH v7 8/9] watchdog: at91sam9: " Guenter Roeck
` (2 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2016-01-26 2:53 UTC (permalink / raw)
To: linux-watchdog
Cc: Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Doug Anderson, Jonathan Corbet,
Guenter Roeck
From: Guenter Roeck <linux@roeck-us.net>
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>
---
v7: Set max_hw_timeout_ms
Rebased to v4.5-rc1
v6: Rename WDOG_RUNNING to WDOG_HW_RUNNING
Rebased to v4.4-rc2
v5: Rebased to v4.4-rc1
v4: No changes
v3: No changes
v2: No changes
---
drivers/watchdog/retu_wdt.c | 80 +++++----------------------------------------
1 file changed, 8 insertions(+), 72 deletions(-)
diff --git a/drivers/watchdog/retu_wdt.c b/drivers/watchdog/retu_wdt.c
index 39cd51df2ffc..fd4a64432146 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_HW_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,40 +62,27 @@ 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;
+ retu_wdt->max_hw_timeout_ms = RETU_WDT_MAX_TIMER * 1000;
retu_wdt->parent = &pdev->dev;
- 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);
@@ -154,10 +92,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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFT PATCH v7 8/9] watchdog: at91sam9: Convert to use infrastructure triggered keepalives
2016-01-26 2:53 [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Guenter Roeck
` (6 preceding siblings ...)
2016-01-26 2:53 ` [RFT PATCH v7 7/9] watchdog: retu: " Guenter Roeck
@ 2016-01-26 2:53 ` Guenter Roeck
2016-01-26 2:53 ` [RFT PATCH v7 9/9] watchdog: dw_wdt: Convert to use watchdog infrastructure Guenter Roeck
2016-01-26 8:09 ` [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Uwe Kleine-König
9 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2016-01-26 2:53 UTC (permalink / raw)
To: linux-watchdog
Cc: Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Doug Anderson, Jonathan Corbet,
Guenter Roeck
From: Guenter Roeck <linux@roeck-us.net>
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 <linux@roeck-us.net>
---
v7: Rebased to v4.5-rc1
v6: Rebased to v4.4-rc2
v5: Rebased to v4.4-rc1
v4: No changes
v3: No changes
v2: No changes
---
drivers/watchdog/at91sam9_wdt.c | 103 +++++-----------------------------------
1 file changed, 12 insertions(+), 91 deletions(-)
diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 7e6acaf3ece4..a1c1c79b700e 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -30,7 +30,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>
@@ -49,8 +48,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)
@@ -65,9 +64,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;
@@ -84,11 +80,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;
struct clk *sclk;
@@ -109,47 +102,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)
@@ -159,8 +118,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);
@@ -182,31 +141,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,
@@ -222,32 +165,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);
}
/* ......................................................................... */
@@ -261,8 +184,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)
@@ -393,7 +314,7 @@ 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);
+
clk_disable_unprepare(wdt->sclk);
return 0;
--
2.1.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFT PATCH v7 9/9] watchdog: dw_wdt: Convert to use watchdog infrastructure
2016-01-26 2:53 [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Guenter Roeck
` (7 preceding siblings ...)
2016-01-26 2:53 ` [RFT PATCH v7 8/9] watchdog: at91sam9: " Guenter Roeck
@ 2016-01-26 2:53 ` Guenter Roeck
2016-01-29 17:52 ` Doug Anderson
2016-01-26 8:09 ` [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Uwe Kleine-König
9 siblings, 1 reply; 19+ messages in thread
From: Guenter Roeck @ 2016-01-26 2:53 UTC (permalink / raw)
To: linux-watchdog
Cc: Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Doug Anderson, Jonathan Corbet,
Guenter Roeck
From: Guenter Roeck <linux@roeck-us.net>
Convert driver to use watchdog infrastructure. This includes
infrastructure support to handle watchdog keepalive if the watchdog
is running while the watchdog device is closed.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v7: Set max_hw_timeout_ms
Rebased to v4.5-rc1
v6: Added patch
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/dw_wdt.c | 323 +++++++++++++++++-----------------------------
2 files changed, 117 insertions(+), 207 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 4f0e7be0da34..36b3835bc524 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -329,6 +329,7 @@ config SA1100_WATCHDOG
config DW_WATCHDOG
tristate "Synopsys DesignWare watchdog"
depends on HAS_IOMEM
+ select WATCHDOG_CORE
help
Say Y here if to include support for the Synopsys DesignWare
watchdog timer found in many chips.
diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 8fefa4ad46d4..a088451e28cb 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -12,9 +12,8 @@
* and these are a function of the input clock frequency.
*
* The DesignWare watchdog cannot be stopped once it has been started so we
- * use a software timer to implement a ping that will keep the watchdog alive.
- * If we receive an expected close for the watchdog then we keep the timer
- * running, otherwise the timer is stopped and the watchdog will expire.
+ * do not implement a stop function. The watchdog core will continue to send
+ * heartbeat requests after the watchdog device has been closed.
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -22,12 +21,9 @@
#include <linux/bitops.h>
#include <linux/clk.h>
#include <linux/delay.h>
-#include <linux/device.h>
#include <linux/err.h>
-#include <linux/fs.h>
#include <linux/io.h>
#include <linux/kernel.h>
-#include <linux/miscdevice.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/notifier.h>
@@ -35,8 +31,6 @@
#include <linux/pm.h>
#include <linux/platform_device.h>
#include <linux/reboot.h>
-#include <linux/timer.h>
-#include <linux/uaccess.h>
#include <linux/watchdog.h>
#define WDOG_CONTROL_REG_OFFSET 0x00
@@ -57,53 +51,50 @@ module_param(nowayout, bool, 0);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
-#define WDT_TIMEOUT (HZ / 2)
-
-static struct {
+struct dw_wdt {
void __iomem *regs;
struct clk *clk;
- unsigned long in_use;
- unsigned long next_heartbeat;
- struct timer_list timer;
- int expect_close;
struct notifier_block restart_handler;
-} dw_wdt;
+ struct watchdog_device wdd;
+};
+
+#define to_dw_wdt(wdd) container_of(wdd, struct dw_wdt, wdd)
-static inline int dw_wdt_is_enabled(void)
+static inline int dw_wdt_is_enabled(struct dw_wdt *dw_wdt)
{
- return readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET) &
+ return readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET) &
WDOG_CONTROL_REG_WDT_EN_MASK;
}
-static inline int dw_wdt_top_in_seconds(unsigned top)
+static inline int dw_wdt_top_in_seconds(struct dw_wdt *dw_wdt, unsigned top)
{
/*
* There are 16 possible timeout values in 0..15 where the number of
* cycles is 2 ^ (16 + i) and the watchdog counts down.
*/
- return (1U << (16 + top)) / clk_get_rate(dw_wdt.clk);
+ return (1U << (16 + top)) / clk_get_rate(dw_wdt->clk);
}
-static int dw_wdt_get_top(void)
+static int dw_wdt_get_top(struct dw_wdt *dw_wdt)
{
- int top = readl(dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
+ int top = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
- return dw_wdt_top_in_seconds(top);
+ return dw_wdt_top_in_seconds(dw_wdt, top);
}
-static inline void dw_wdt_set_next_heartbeat(void)
+static int dw_wdt_ping(struct watchdog_device *wdd)
{
- dw_wdt.next_heartbeat = jiffies + dw_wdt_get_top() * HZ;
-}
+ struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
-static void dw_wdt_keepalive(void)
-{
- writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
+ writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt->regs +
WDOG_COUNTER_RESTART_REG_OFFSET);
+
+ return 0;
}
-static int dw_wdt_set_top(unsigned top_s)
+static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
{
+ struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
int i, top_val = DW_WDT_MAX_TOP;
/*
@@ -111,7 +102,7 @@ static int dw_wdt_set_top(unsigned top_s)
* always look for >=.
*/
for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
- if (dw_wdt_top_in_seconds(i) >= top_s) {
+ if (dw_wdt_top_in_seconds(dw_wdt, i) >= top_s) {
top_val = i;
break;
}
@@ -123,33 +114,43 @@ static int dw_wdt_set_top(unsigned top_s)
* effectively get a pat of the watchdog right here.
*/
writel(top_val | top_val << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
- dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
+ dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
- /*
- * Add an explicit pat to handle versions of the watchdog that
- * don't have TOPINIT. This won't hurt on versions that have
- * it.
- */
- dw_wdt_keepalive();
+ wdd->timeout = dw_wdt_top_in_seconds(dw_wdt, top_val);
- dw_wdt_set_next_heartbeat();
+ return 0;
+}
+
+static int dw_wdt_start(struct watchdog_device *wdd)
+{
+ struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
+
+ dw_wdt_set_timeout(wdd, wdd->timeout);
- return dw_wdt_top_in_seconds(top_val);
+ set_bit(WDOG_HW_RUNNING, &wdd->status);
+
+ writel(WDOG_CONTROL_REG_WDT_EN_MASK,
+ dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
+
+ return 0;
}
static int dw_wdt_restart_handle(struct notifier_block *this,
- unsigned long mode, void *cmd)
+ unsigned long mode, void *cmd)
{
+ struct dw_wdt *dw_wdt;
u32 val;
- writel(0, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
- val = readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+ dw_wdt = container_of(this, struct dw_wdt, restart_handler);
+
+ writel(0, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
+ val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
if (val & WDOG_CONTROL_REG_WDT_EN_MASK)
- writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
- WDOG_COUNTER_RESTART_REG_OFFSET);
+ writel(WDOG_COUNTER_RESTART_KICK_VALUE,
+ dw_wdt->regs + WDOG_COUNTER_RESTART_REG_OFFSET);
else
writel(WDOG_CONTROL_REG_WDT_EN_MASK,
- dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+ dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
/* wait for reset to assert... */
mdelay(500);
@@ -157,74 +158,12 @@ static int dw_wdt_restart_handle(struct notifier_block *this,
return NOTIFY_DONE;
}
-static void dw_wdt_ping(unsigned long data)
+static unsigned int dw_wdt_get_timeleft(struct watchdog_device *wdd)
{
- if (time_before(jiffies, dw_wdt.next_heartbeat) ||
- (!nowayout && !dw_wdt.in_use)) {
- dw_wdt_keepalive();
- mod_timer(&dw_wdt.timer, jiffies + WDT_TIMEOUT);
- } else
- pr_crit("keepalive missed, machine will reset\n");
-}
-
-static int dw_wdt_open(struct inode *inode, struct file *filp)
-{
- if (test_and_set_bit(0, &dw_wdt.in_use))
- return -EBUSY;
-
- /* Make sure we don't get unloaded. */
- __module_get(THIS_MODULE);
-
- if (!dw_wdt_is_enabled()) {
- /*
- * The watchdog is not currently enabled. Set the timeout to
- * something reasonable and then start it.
- */
- dw_wdt_set_top(DW_WDT_DEFAULT_SECONDS);
- writel(WDOG_CONTROL_REG_WDT_EN_MASK,
- dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
- }
-
- dw_wdt_set_next_heartbeat();
-
- return nonseekable_open(inode, filp);
-}
-
-static ssize_t dw_wdt_write(struct file *filp, const char __user *buf,
- size_t len, loff_t *offset)
-{
- if (!len)
- return 0;
-
- if (!nowayout) {
- size_t i;
-
- dw_wdt.expect_close = 0;
-
- for (i = 0; i < len; ++i) {
- char c;
-
- if (get_user(c, buf + i))
- return -EFAULT;
-
- if (c == 'V') {
- dw_wdt.expect_close = 1;
- break;
- }
- }
- }
+ struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
- dw_wdt_set_next_heartbeat();
- dw_wdt_keepalive();
- mod_timer(&dw_wdt.timer, jiffies + WDT_TIMEOUT);
-
- return len;
-}
-
-static u32 dw_wdt_time_left(void)
-{
- return readl(dw_wdt.regs + WDOG_CURRENT_COUNT_REG_OFFSET) /
- clk_get_rate(dw_wdt.clk);
+ return readl(dw_wdt->regs + WDOG_CURRENT_COUNT_REG_OFFSET) /
+ clk_get_rate(dw_wdt->clk);
}
static const struct watchdog_info dw_wdt_ident = {
@@ -233,78 +172,33 @@ static const struct watchdog_info dw_wdt_ident = {
.identity = "Synopsys DesignWare Watchdog",
};
-static long dw_wdt_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
-{
- unsigned long val;
- int timeout;
-
- switch (cmd) {
- case WDIOC_GETSUPPORT:
- return copy_to_user((void __user *)arg, &dw_wdt_ident,
- sizeof(dw_wdt_ident)) ? -EFAULT : 0;
-
- case WDIOC_GETSTATUS:
- case WDIOC_GETBOOTSTATUS:
- return put_user(0, (int __user *)arg);
-
- case WDIOC_KEEPALIVE:
- dw_wdt_set_next_heartbeat();
- return 0;
-
- case WDIOC_SETTIMEOUT:
- if (get_user(val, (int __user *)arg))
- return -EFAULT;
- timeout = dw_wdt_set_top(val);
- return put_user(timeout , (int __user *)arg);
-
- case WDIOC_GETTIMEOUT:
- return put_user(dw_wdt_get_top(), (int __user *)arg);
-
- case WDIOC_GETTIMELEFT:
- /* Get the time left until expiry. */
- if (get_user(val, (int __user *)arg))
- return -EFAULT;
- return put_user(dw_wdt_time_left(), (int __user *)arg);
-
- default:
- return -ENOTTY;
- }
-}
-
-static int dw_wdt_release(struct inode *inode, struct file *filp)
-{
- clear_bit(0, &dw_wdt.in_use);
-
- if (!dw_wdt.expect_close) {
- del_timer(&dw_wdt.timer);
-
- if (!nowayout)
- pr_crit("unexpected close, system will reboot soon\n");
- else
- pr_crit("watchdog cannot be disabled, system will reboot soon\n");
- }
-
- dw_wdt.expect_close = 0;
-
- return 0;
-}
+static const struct watchdog_ops dw_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = dw_wdt_start,
+ .ping = dw_wdt_ping,
+ .set_timeout = dw_wdt_set_timeout,
+ .get_timeleft = dw_wdt_get_timeleft,
+};
#ifdef CONFIG_PM_SLEEP
static int dw_wdt_suspend(struct device *dev)
{
- clk_disable_unprepare(dw_wdt.clk);
+ struct dw_wdt *dw_wdt = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(dw_wdt->clk);
return 0;
}
static int dw_wdt_resume(struct device *dev)
{
- int err = clk_prepare_enable(dw_wdt.clk);
+ struct dw_wdt *dw_wdt = dev_get_drvdata(dev);
+ int err = clk_prepare_enable(dw_wdt->clk);
if (err)
return err;
- dw_wdt_keepalive();
+ dw_wdt_ping(&dw_wdt->wdd);
return 0;
}
@@ -312,67 +206,82 @@ static int dw_wdt_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(dw_wdt_pm_ops, dw_wdt_suspend, dw_wdt_resume);
-static const struct file_operations wdt_fops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .open = dw_wdt_open,
- .write = dw_wdt_write,
- .unlocked_ioctl = dw_wdt_ioctl,
- .release = dw_wdt_release
-};
-
-static struct miscdevice dw_wdt_miscdev = {
- .fops = &wdt_fops,
- .name = "watchdog",
- .minor = WATCHDOG_MINOR,
-};
-
static int dw_wdt_drv_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
+ struct watchdog_device *wdd;
+ struct dw_wdt *dw_wdt;
+ struct resource *mem;
int ret;
- struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- dw_wdt.regs = devm_ioremap_resource(&pdev->dev, mem);
- if (IS_ERR(dw_wdt.regs))
- return PTR_ERR(dw_wdt.regs);
+ dw_wdt = devm_kzalloc(dev, sizeof(*dw_wdt), GFP_KERNEL);
+ if (!dw_wdt)
+ return -ENOMEM;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ dw_wdt->regs = devm_ioremap_resource(dev, mem);
+ if (IS_ERR(dw_wdt->regs))
+ return PTR_ERR(dw_wdt->regs);
- dw_wdt.clk = devm_clk_get(&pdev->dev, NULL);
- if (IS_ERR(dw_wdt.clk))
- return PTR_ERR(dw_wdt.clk);
+ dw_wdt->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(dw_wdt->clk))
+ return PTR_ERR(dw_wdt->clk);
- ret = clk_prepare_enable(dw_wdt.clk);
+ ret = clk_prepare_enable(dw_wdt->clk);
if (ret)
return ret;
- ret = misc_register(&dw_wdt_miscdev);
+ wdd = &dw_wdt->wdd;
+ wdd->info = &dw_wdt_ident;
+ wdd->ops = &dw_wdt_ops;
+ wdd->min_timeout = 1;
+ wdd->max_hw_timeout_ms =
+ dw_wdt_top_in_seconds(dw_wdt, DW_WDT_MAX_TOP) * 1000;
+ wdd->parent = dev;
+
+ watchdog_set_drvdata(wdd, dw_wdt);
+ watchdog_set_nowayout(wdd, nowayout);
+ watchdog_init_timeout(wdd, 0, dev);
+
+ /*
+ * If the watchdog is already running, use its already configured
+ * timeout. Otherwise use the default or the value provided through
+ * devicetree.
+ */
+ if (dw_wdt_is_enabled(dw_wdt)) {
+ wdd->timeout = dw_wdt_get_top(dw_wdt);
+ set_bit(WDOG_HW_RUNNING, &wdd->status);
+ } else {
+ wdd->timeout = DW_WDT_DEFAULT_SECONDS;
+ watchdog_init_timeout(wdd, 0, dev);
+ }
+
+ platform_set_drvdata(pdev, dw_wdt);
+
+ ret = watchdog_register_device(wdd);
if (ret)
goto out_disable_clk;
- dw_wdt.restart_handler.notifier_call = dw_wdt_restart_handle;
- dw_wdt.restart_handler.priority = 128;
- ret = register_restart_handler(&dw_wdt.restart_handler);
+ dw_wdt->restart_handler.notifier_call = dw_wdt_restart_handle;
+ dw_wdt->restart_handler.priority = 128;
+ ret = register_restart_handler(&dw_wdt->restart_handler);
if (ret)
pr_warn("cannot register restart handler\n");
- dw_wdt_set_next_heartbeat();
- setup_timer(&dw_wdt.timer, dw_wdt_ping, 0);
- mod_timer(&dw_wdt.timer, jiffies + WDT_TIMEOUT);
-
return 0;
out_disable_clk:
- clk_disable_unprepare(dw_wdt.clk);
-
+ clk_disable_unprepare(dw_wdt->clk);
return ret;
}
static int dw_wdt_drv_remove(struct platform_device *pdev)
{
- unregister_restart_handler(&dw_wdt.restart_handler);
-
- misc_deregister(&dw_wdt_miscdev);
+ struct dw_wdt *dw_wdt = platform_get_drvdata(pdev);
- clk_disable_unprepare(dw_wdt.clk);
+ unregister_restart_handler(&dw_wdt->restart_handler);
+ watchdog_unregister_device(&dw_wdt->wdd);
+ clk_disable_unprepare(dw_wdt->clk);
return 0;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v7 4/9] watchdog: Add support for minimum time between heartbeats
2016-01-26 2:53 ` [PATCH v7 4/9] watchdog: Add support for minimum time between heartbeats Guenter Roeck
@ 2016-01-26 8:07 ` Uwe Kleine-König
2016-01-26 14:42 ` Guenter Roeck
0 siblings, 1 reply; 19+ messages in thread
From: Uwe Kleine-König @ 2016-01-26 8:07 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-watchdog, Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
linux-doc, Doug Anderson, Jonathan Corbet, Guenter Roeck
Hello Guenter,
On Mon, Jan 25, 2016 at 06:53:11PM -0800, Guenter Roeck wrote:
> From: Guenter Roeck <linux@roeck-us.net>
>
> Some watchdogs require a minimum time between heartbeats.
> Examples are the watchdogs in DA9062 and AT91SAM9x.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v7: Rebased to v4.5-rc1
> v6: Rebased to v4.4-rc2
> v5: Rebased to v4.4-rc1
> Fixed typo in documentation.
> v4: Added patch
> ---
> Documentation/watchdog/watchdog-kernel-api.txt | 3 +++
> drivers/watchdog/watchdog_dev.c | 15 +++++++++++++++
> include/linux/watchdog.h | 3 +++
> 3 files changed, 21 insertions(+)
>
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index 62d49d6a7ff5..2221fb4f2739 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -52,6 +52,7 @@ struct watchdog_device {
> unsigned int timeout;
> unsigned int min_timeout;
> unsigned int max_timeout;
> + unsigned int min_hw_heartbeat_ms;
> unsigned int max_hw_timeout_ms;
> struct notifier_block reboot_nb;
> struct notifier_block restart_nb;
> @@ -81,6 +82,8 @@ It contains following fields:
> * 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_timeout_ms is non-zero.
> +* min_hw_heartbeat_ms: Minimum time between heartbeats sent to the chip,
> + in milli-seconds.
> * max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds.
> If set, the infrastructure will send heartbeats to the watchdog driver
> if 'timeout' is larger than max_hw_timeout_ms, unless WDOG_ACTIVE
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 1a8f3861fe92..ff03cbc8e081 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -64,6 +64,7 @@ struct watchdog_core_data {
> struct watchdog_device *wdd;
> struct mutex lock;
> unsigned long last_keepalive;
> + unsigned long last_hw_keepalive;
> struct delayed_work work;
> unsigned long status; /* Internal status bits */
> #define _WDOG_DEV_OPEN 0 /* Opened ? */
> @@ -138,8 +139,19 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd,
>
> static int __watchdog_ping(struct watchdog_device *wdd)
> {
> + struct watchdog_core_data *wd_data = wdd->wd_data;
> + unsigned long earliest_keepalive = wd_data->last_hw_keepalive +
> + msecs_to_jiffies(wdd->min_hw_heartbeat_ms);
> int err;
>
> + if (time_is_after_jiffies(earliest_keepalive)) {
> + mod_delayed_work(watchdog_wq, &wd_data->work,
> + earliest_keepalive - jiffies);
> + return 0;
> + }
> +
> + wd_data->last_hw_keepalive = jiffies;
> +
Do you need to undo this assignment when ping fails?
> if (wdd->ops->ping)
> err = wdd->ops->ping(wdd); /* ping the watchdog */
> else
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure
2016-01-26 2:53 [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Guenter Roeck
` (8 preceding siblings ...)
2016-01-26 2:53 ` [RFT PATCH v7 9/9] watchdog: dw_wdt: Convert to use watchdog infrastructure Guenter Roeck
@ 2016-01-26 8:09 ` Uwe Kleine-König
2016-01-26 14:43 ` Guenter Roeck
9 siblings, 1 reply; 19+ messages in thread
From: Uwe Kleine-König @ 2016-01-26 8:09 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-watchdog, Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
linux-doc, Doug Anderson, Jonathan Corbet, Guenter Roeck
Hello Guenter,
On Mon, Jan 25, 2016 at 06:53:07PM -0800, Guenter Roeck wrote:
> Patch #1 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 #2 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 #3 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 #4 adds code to ensure that the minimum time between heartbeats meets
> constraints provided by the watchdog driver.
>
> Patch #5 simplifies the watchdog_update_worker() function introduced with
> patch #1 to only take a single argument, and to always cancel any pending
> work if a worker is not or no longer needed. This patch is kept as separate
> patch on purpose, to enable dropping or reverting it easily if it causes
> any problems. It should not cause any problems; this is just out of an
> abundance of caution.
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> for patches
1-5.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v7 4/9] watchdog: Add support for minimum time between heartbeats
2016-01-26 8:07 ` Uwe Kleine-König
@ 2016-01-26 14:42 ` Guenter Roeck
0 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2016-01-26 14:42 UTC (permalink / raw)
To: Uwe Kleine-König, Guenter Roeck
Cc: linux-watchdog, Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
linux-doc, Doug Anderson, Jonathan Corbet
Hi Uwe,
On 01/26/2016 12:07 AM, Uwe Kleine-König wrote:
> Hello Guenter,
>
> On Mon, Jan 25, 2016 at 06:53:11PM -0800, Guenter Roeck wrote:
>> From: Guenter Roeck <linux@roeck-us.net>
>>
>> Some watchdogs require a minimum time between heartbeats.
>> Examples are the watchdogs in DA9062 and AT91SAM9x.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v7: Rebased to v4.5-rc1
>> v6: Rebased to v4.4-rc2
>> v5: Rebased to v4.4-rc1
>> Fixed typo in documentation.
>> v4: Added patch
>> ---
>> Documentation/watchdog/watchdog-kernel-api.txt | 3 +++
>> drivers/watchdog/watchdog_dev.c | 15 +++++++++++++++
>> include/linux/watchdog.h | 3 +++
>> 3 files changed, 21 insertions(+)
>>
>> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
>> index 62d49d6a7ff5..2221fb4f2739 100644
>> --- a/Documentation/watchdog/watchdog-kernel-api.txt
>> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
>> @@ -52,6 +52,7 @@ struct watchdog_device {
>> unsigned int timeout;
>> unsigned int min_timeout;
>> unsigned int max_timeout;
>> + unsigned int min_hw_heartbeat_ms;
>> unsigned int max_hw_timeout_ms;
>> struct notifier_block reboot_nb;
>> struct notifier_block restart_nb;
>> @@ -81,6 +82,8 @@ It contains following fields:
>> * 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_timeout_ms is non-zero.
>> +* min_hw_heartbeat_ms: Minimum time between heartbeats sent to the chip,
>> + in milli-seconds.
>> * max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds.
>> If set, the infrastructure will send heartbeats to the watchdog driver
>> if 'timeout' is larger than max_hw_timeout_ms, unless WDOG_ACTIVE
>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
>> index 1a8f3861fe92..ff03cbc8e081 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -64,6 +64,7 @@ struct watchdog_core_data {
>> struct watchdog_device *wdd;
>> struct mutex lock;
>> unsigned long last_keepalive;
>> + unsigned long last_hw_keepalive;
>> struct delayed_work work;
>> unsigned long status; /* Internal status bits */
>> #define _WDOG_DEV_OPEN 0 /* Opened ? */
>> @@ -138,8 +139,19 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd,
>>
>> static int __watchdog_ping(struct watchdog_device *wdd)
>> {
>> + struct watchdog_core_data *wd_data = wdd->wd_data;
>> + unsigned long earliest_keepalive = wd_data->last_hw_keepalive +
>> + msecs_to_jiffies(wdd->min_hw_heartbeat_ms);
>> int err;
>>
>> + if (time_is_after_jiffies(earliest_keepalive)) {
>> + mod_delayed_work(watchdog_wq, &wd_data->work,
>> + earliest_keepalive - jiffies);
>> + return 0;
>> + }
>> +
>> + wd_data->last_hw_keepalive = jiffies;
>> +
>
> Do you need to undo this assignment when ping fails?
>
I thought about it, but decided against it. I concluded that it is better
in this situation to only try again after the minimum delay between
heartbeats.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure
2016-01-26 8:09 ` [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Uwe Kleine-König
@ 2016-01-26 14:43 ` Guenter Roeck
0 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2016-01-26 14:43 UTC (permalink / raw)
To: Uwe Kleine-König, Guenter Roeck
Cc: linux-watchdog, Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
linux-doc, Doug Anderson, Jonathan Corbet
Hi Uwe,
On 01/26/2016 12:09 AM, Uwe Kleine-König wrote:
> Hello Guenter,
>
> On Mon, Jan 25, 2016 at 06:53:07PM -0800, Guenter Roeck wrote:
>> Patch #1 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 #2 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 #3 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 #4 adds code to ensure that the minimum time between heartbeats meets
>> constraints provided by the watchdog driver.
>>
>> Patch #5 simplifies the watchdog_update_worker() function introduced with
>> patch #1 to only take a single argument, and to always cancel any pending
>> work if a worker is not or no longer needed. This patch is kept as separate
>> patch on purpose, to enable dropping or reverting it easily if it causes
>> any problems. It should not cause any problems; this is just out of an
>> abundance of caution.
>
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> for patches
> 1-5.
>
Thanks a lot for your time!
Guenter
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFT PATCH v7 9/9] watchdog: dw_wdt: Convert to use watchdog infrastructure
2016-01-26 2:53 ` [RFT PATCH v7 9/9] watchdog: dw_wdt: Convert to use watchdog infrastructure Guenter Roeck
@ 2016-01-29 17:52 ` Doug Anderson
2016-01-29 18:24 ` Guenter Roeck
0 siblings, 1 reply; 19+ messages in thread
From: Doug Anderson @ 2016-01-29 17:52 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-watchdog, Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Jonathan Corbet, Guenter Roeck
Guenter,
On Mon, Jan 25, 2016 at 6:53 PM, Guenter Roeck
<patchwork@patchwork.roeck-us.net> wrote:
> From: Guenter Roeck <linux@roeck-us.net>
>
> Convert driver to use watchdog infrastructure. This includes
> infrastructure support to handle watchdog keepalive if the watchdog
> is running while the watchdog device is closed.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>
> ---
> v7: Set max_hw_timeout_ms
> Rebased to v4.5-rc1
> v6: Added patch
> ---
> drivers/watchdog/Kconfig | 1 +
> drivers/watchdog/dw_wdt.c | 323 +++++++++++++++++-----------------------------
> 2 files changed, 117 insertions(+), 207 deletions(-)
I used the following test environment:
* Rockchip rk3288-based Chromebook
* v4.4-based kernel (specifically commit b1ba57898bfc
("veyron_defconfig: update for 4.5-rc") from Heiko Stuebner "somewhat
stable" development tree
* Pulled in some USB-related fixes (just for my own testing)
* Pulled in Wim's most recent pull request for watchdog subsystem
(ac36856fe432 watchdog: asm9260: remove __init and __exit annotations)
* Applied all 9 of your patches
* Test with daisydog
<https://chromium.googlesource.com/chromiumos/third_party/daisydog/>
I did these tests:
###
# Confirm normal functionality
###
stop daisydog
daisydog -c
# HW watchdog interval is 43 seconds
# /dev/watchdog reported boot status: normal-boot
cat /dev/watchdog
# cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop!
# /dev/watchdog: Invalid argument
for i in $(seq 60); do
echo $i
sleep 1
done
# 1 - 43 printed, then reboot
###
# Try setting interval, confirm all good (no reboots)
# ...then confirm that reboot happens in 10 sec
###
stop daisydog
daisydog -c
# HW watchdog interval is 43 seconds
# /dev/watchdog reported boot status: normal-boot
daisydog -i 10
# HW watchdog interval is 10 seconds
# /dev/watchdog reported boot status: normal-boot
<Ctrl-C>
daisydog -c
# HW watchdog interval is 10 seconds
# /dev/watchdog reported boot status: normal-boot
sleep 60
# No reboots
cat /dev/watchdog
# cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop!
# /dev/watchdog: Invalid argument
for i in $(seq 60); do
echo $i
sleep 1
done
# 1 - 11 printed, then reboot
###
# Try setting interval super long
# (outside of hardware range)
###
stop daisydog
daisydog -i 100 -c
# HW watchdog interval is 86 seconds
# /dev/watchdog reported boot status: normal-boot
cat /dev/watchdog
# cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop!
# /dev/watchdog: Invalid argument
for i in $(seq 200); do
echo $i
sleep 1
done
# 1 - 87 printed, then reboot; seems right to me.
###
# Try with a interval that's in hardware range, but above default
###
stop daisydog
daisydog -i 70 -c
# HW watchdog interval is 86 seconds
# /dev/watchdog reported boot status: normal-boot
cat /dev/watchdog
# cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop!
# /dev/watchdog: Invalid argument
for i in $(seq 200); do
echo $i
sleep 1
done
# 1 - 87 printed, then reboot; seems right to me
###
# Make sure my previous fix not regressed:
# 04b1a62e6bb9 ("watchdog: dw_wdt: keepalive the watchdog at write time")
###
stop daisydog
while true; do daisydog -c > /dev/null; done
# Let the above run for a 2 minutes and see no reboot.
###
# See what happens when you change things midway
###
stop daisydog
daisydog -c
# HW watchdog interval is 43 seconds
# /dev/watchdog reported boot status: normal-boot
cat /dev/watchdog
# cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop!
# /dev/watchdog: Invalid argument
for i in $(seq 30); do
echo $i
sleep 1
done
# 1 - 30 printed
daisydog -c -i 10
# HW watchdog interval is 10 seconds
# /dev/watchdog reported boot status: normal-boot
sleep 60
# No reboot; right since the "-c -i 10" patted + closed properly
===
I also tried the new functionality enabled by your new patch and added
"timeout-sec = <10>" to my device tree. That seemed to work. Upon
bootup the watchdog defaulted to 10 seconds. I also tried setting it
to 86 in the device tree with similar success.
===
The above seem to cover all the cases I can think of.
Tested-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFT PATCH v7 9/9] watchdog: dw_wdt: Convert to use watchdog infrastructure
2016-01-29 17:52 ` Doug Anderson
@ 2016-01-29 18:24 ` Guenter Roeck
0 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2016-01-29 18:24 UTC (permalink / raw)
To: Doug Anderson, Guenter Roeck
Cc: linux-watchdog, Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Jonathan Corbet
Hi Doug,
On 01/29/2016 09:52 AM, Doug Anderson wrote:
> Guenter,
>
> On Mon, Jan 25, 2016 at 6:53 PM, Guenter Roeck
> <patchwork@patchwork.roeck-us.net> wrote:
>> From: Guenter Roeck <linux@roeck-us.net>
>>
>> Convert driver to use watchdog infrastructure. This includes
>> infrastructure support to handle watchdog keepalive if the watchdog
>> is running while the watchdog device is closed.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>
>> ---
>> v7: Set max_hw_timeout_ms
>> Rebased to v4.5-rc1
>> v6: Added patch
>> ---
>> drivers/watchdog/Kconfig | 1 +
>> drivers/watchdog/dw_wdt.c | 323 +++++++++++++++++-----------------------------
>> 2 files changed, 117 insertions(+), 207 deletions(-)
>
> I used the following test environment:
> * Rockchip rk3288-based Chromebook
> * v4.4-based kernel (specifically commit b1ba57898bfc
> ("veyron_defconfig: update for 4.5-rc") from Heiko Stuebner "somewhat
> stable" development tree
> * Pulled in some USB-related fixes (just for my own testing)
> * Pulled in Wim's most recent pull request for watchdog subsystem
> (ac36856fe432 watchdog: asm9260: remove __init and __exit annotations)
> * Applied all 9 of your patches
> * Test with daisydog
> <https://chromium.googlesource.com/chromiumos/third_party/daisydog/>
>
>
> I did these tests:
>
> ###
> # Confirm normal functionality
> ###
>
> stop daisydog
> daisydog -c
> # HW watchdog interval is 43 seconds
> # /dev/watchdog reported boot status: normal-boot
>
> cat /dev/watchdog
> # cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop!
> # /dev/watchdog: Invalid argument
>
> for i in $(seq 60); do
> echo $i
> sleep 1
> done
> # 1 - 43 printed, then reboot
>
> ###
> # Try setting interval, confirm all good (no reboots)
> # ...then confirm that reboot happens in 10 sec
> ###
>
> stop daisydog
> daisydog -c
> # HW watchdog interval is 43 seconds
> # /dev/watchdog reported boot status: normal-boot
>
> daisydog -i 10
> # HW watchdog interval is 10 seconds
> # /dev/watchdog reported boot status: normal-boot
>
> <Ctrl-C>
>
> daisydog -c
> # HW watchdog interval is 10 seconds
> # /dev/watchdog reported boot status: normal-boot
>
> sleep 60
> # No reboots
>
> cat /dev/watchdog
> # cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop!
> # /dev/watchdog: Invalid argument
>
> for i in $(seq 60); do
> echo $i
> sleep 1
> done
> # 1 - 11 printed, then reboot
>
> ###
> # Try setting interval super long
> # (outside of hardware range)
> ###
>
> stop daisydog
> daisydog -i 100 -c
> # HW watchdog interval is 86 seconds
> # /dev/watchdog reported boot status: normal-boot
>
> cat /dev/watchdog
> # cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop!
> # /dev/watchdog: Invalid argument
>
> for i in $(seq 200); do
> echo $i
> sleep 1
> done
> # 1 - 87 printed, then reboot; seems right to me.
>
> ###
> # Try with a interval that's in hardware range, but above default
> ###
>
> stop daisydog
> daisydog -i 70 -c
> # HW watchdog interval is 86 seconds
> # /dev/watchdog reported boot status: normal-boot
>
> cat /dev/watchdog
> # cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop!
> # /dev/watchdog: Invalid argument
>
> for i in $(seq 200); do
> echo $i
> sleep 1
> done
> # 1 - 87 printed, then reboot; seems right to me
>
> ###
> # Make sure my previous fix not regressed:
> # 04b1a62e6bb9 ("watchdog: dw_wdt: keepalive the watchdog at write time")
> ###
>
> stop daisydog
> while true; do daisydog -c > /dev/null; done
>
> # Let the above run for a 2 minutes and see no reboot.
>
> ###
> # See what happens when you change things midway
> ###
>
> stop daisydog
> daisydog -c
> # HW watchdog interval is 43 seconds
> # /dev/watchdog reported boot status: normal-boot
>
> cat /dev/watchdog
> # cat: [ 5401.044582] watchdog watchdog0: watchdog did not stop!
> # /dev/watchdog: Invalid argument
>
> for i in $(seq 30); do
> echo $i
> sleep 1
> done
> # 1 - 30 printed
>
> daisydog -c -i 10
> # HW watchdog interval is 10 seconds
> # /dev/watchdog reported boot status: normal-boot
>
> sleep 60
> # No reboot; right since the "-c -i 10" patted + closed properly
>
> ===
>
> I also tried the new functionality enabled by your new patch and added
> "timeout-sec = <10>" to my device tree. That seemed to work. Upon
> bootup the watchdog defaulted to 10 seconds. I also tried setting it
> to 86 in the device tree with similar success.
>
> ===
>
> The above seem to cover all the cases I can think of.
>
> Tested-by: Douglas Anderson <dianders@chromium.org>
>
Excellent - thanks a lot for the testing!
Guenter
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFT PATCH v7 6/9] watchdog: imx2: Convert to use infrastructure triggered keepalives
2016-01-26 2:53 ` [RFT PATCH v7 6/9] watchdog: imx2: Convert to use infrastructure triggered keepalives Guenter Roeck
@ 2016-01-29 19:40 ` Guenter Roeck
0 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2016-01-29 19:40 UTC (permalink / raw)
To: Guenter Roeck, linux-watchdog
Cc: Wim Van Sebroeck, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Doug Anderson, Jonathan Corbet
On 01/25/2016 06:53 PM, Guenter Roeck wrote:
> From: Guenter Roeck <linux@roeck-us.net>
>
> 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>
I managed to test this patch with qemu (after implementing the imx2 watchdog
in qemu). It works as expected, so please consider it upgraded from RFT
to a "real" patch.
Guenter
> ---
> v7: Set max_hw_timeout_ms
> Rebased to v4.5-rc1
> v6: Rename WDOG_RUNNING to WDOG_HW_RUNNING
> Rebased to v4.4-rc2
> v5: Rebased to v4.4-rc1
> v4: No changes
> v3: No changes
> v2: No changes
> ---
> 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 e47966aa2db0..1a48cb28718f 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -25,14 +25,12 @@
> #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>
> #include <linux/of_address.h>
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
> -#include <linux/timer.h>
> #include <linux/watchdog.h>
>
> #define DRIVER_NAME "imx2-wdt"
> @@ -60,7 +58,6 @@
> struct imx2_wdt_device {
> struct clk *clk;
> struct regmap *regmap;
> - struct timer_list timer; /* Pings the watchdog when closed */
> struct watchdog_device wdog;
> };
>
> @@ -146,16 +143,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)
> {
> @@ -172,40 +159,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,
> .restart = imx2_wdt_restart,
> @@ -253,7 +219,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_timeout_ms = IMX2_WDT_MAX_TIME * 1000;
> wdog->parent = &pdev->dev;
>
> ret = clk_prepare_enable(wdev->clk);
> @@ -274,9 +240,10 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
> watchdog_set_restart_priority(wdog, 128);
> 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
> @@ -309,7 +276,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");
> }
> @@ -323,10 +289,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");
> @@ -344,10 +309,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);
> @@ -373,19 +334,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;
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v7 1/9] watchdog: Introduce hardware maximum timeout in watchdog core
2016-01-26 2:53 ` [PATCH v7 1/9] watchdog: Introduce hardware maximum timeout in watchdog core Guenter Roeck
@ 2016-02-28 13:18 ` Wim Van Sebroeck
2016-02-28 18:43 ` Guenter Roeck
0 siblings, 1 reply; 19+ messages in thread
From: Wim Van Sebroeck @ 2016-02-28 13:18 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-watchdog, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Doug Anderson, Jonathan Corbet,
Guenter Roeck
Hi Guenter,
Code is OK, but I still have one last remark:
When I coded the generic watchdog framework, I used the following terminology:
* timeout for userspace timeout's
* heartbeat for the internal hardware timeout.
I would like us to stick to this, so that the story keeps being clear and consistent.
So the hardware maximum timeout where you talk about is actually
a maximum heartbeat value. Can you change this in v8?
And then directly fold the drop of the 'cancel' parameter in the same patch?
I also like to have "Make set_timeout function optional" as the first patch of the series.
This patch can be used even without the other patches.
I also like the WDOG_HW_RUNNING flag. But I would split the patch that adds this into 2 patches:
* a patch that adds the WDOG_HW_RUNNING flag
* and a patch that makes the stop function optional.
Thanks in advance,
Wim.
> From: Guenter Roeck <linux@roeck-us.net>
>
> 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 hardware timeout value in the watchdog data
> structure. If the configured timeout exceeds the maximum hardware timeout,
> 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>
> ---
> v7:
> - Rebased to v4.5-rc1
> - Moved new variables to local data structure
> - Dropped Uwe's Acked-by: due to substantial changes
> v6:
> - Set last_keepalive more accurately when starting the watchdog
> - Rebased to v4.4-rc2
> - Added Uwe's Acked-by:
> v5:
> - Rebased to v4.4-rc1
> v4:
> - Improved and fixed documentation
> - Split hw_timeout_ms variable to timeout_ms, hw_timeout_ms for clarity
> - Dropped redundant comments
> - Added comments explaining failure conditions in watchdog_timeout_invalid().
> - Moved the call to watchdog_update_worker() into _watchdog_ping().
> v3:
> - Reworked and cleaned up some of the functions.
> - No longer call the worker update function if all that is needed is to stop
> the worker.
> - max_timeout will now be ignored if max_hw_timeout_ms is provided.
> v2:
> - Improved and hopefully clarified documentation.
> - Rearranged variables in struct watchdog_device such that internal variables
> come last.
> - The code now ensures that the watchdog times out <timeout> seconds after
> the most recent keepalive sent from user space.
> - The internal keepalive now stops silently and no longer generates a
> warning message. Reason is that it will now stop early, while there
> may still be a substantial amount of time for keepalives from user space
> to arrive. If such keepalives arrive late (for example if user space
> is configured to send keepalives just a few seconds before the watchdog
> times out), the message would just be noise and not provide any value.
> ---
> Documentation/watchdog/watchdog-kernel-api.txt | 19 +++-
> drivers/watchdog/watchdog_dev.c | 136 +++++++++++++++++++++++--
> include/linux/watchdog.h | 28 +++--
> 3 files changed, 164 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index 55120a055a14..46979568b9e5 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -52,6 +52,7 @@ struct watchdog_device {
> unsigned int timeout;
> unsigned int min_timeout;
> unsigned int max_timeout;
> + unsigned int max_hw_timeout_ms;
> struct notifier_block reboot_nb;
> struct notifier_block restart_nb;
> void *driver_data;
> @@ -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_timeout_ms is non-zero.
> +* max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds.
> + If set, the infrastructure will send heartbeats to the watchdog driver
> + if 'timeout' is larger than max_hw_timeout_ms, unless WDOG_ACTIVE
> + is set and userspace failed to send a heartbeat for at least 'timeout'
> + seconds.
> * reboot_nb: notifier block that is registered for reboot notifications, for
> internal use only. If the driver calls watchdog_stop_on_reboot, watchdog core
> will stop the watchdog on such notifications.
> @@ -153,7 +164,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_timeout_ms set the hardware watchdog timeout
> + to the minimum of timeout and max_hw_timeout_ms. Those drivers set the
> + timeout value of the watchdog_device either to the requested timeout value
> + (if it is larger than max_hw_timeout_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 ba2ecce4aae6..20e4ce0ebf6c 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -36,6 +36,7 @@
> #include <linux/errno.h> /* For the -ENODEV/... values */
> #include <linux/fs.h> /* For file operations */
> #include <linux/init.h> /* For __init/__exit/... */
> +#include <linux/jiffies.h> /* For timeout functions */
> #include <linux/kernel.h> /* For printk/panic/... */
> #include <linux/kref.h> /* For data references */
> #include <linux/miscdevice.h> /* For handling misc devices */
> @@ -44,6 +45,7 @@
> #include <linux/slab.h> /* For memory functions */
> #include <linux/types.h> /* For standard types (like size_t) */
> #include <linux/watchdog.h> /* For watchdog specific items */
> +#include <linux/workqueue.h> /* For workqueue */
> #include <linux/uaccess.h> /* For copy_to_user/put_user/... */
>
> #include "watchdog_core.h"
> @@ -61,6 +63,8 @@ struct watchdog_core_data {
> struct cdev cdev;
> struct watchdog_device *wdd;
> struct mutex lock;
> + unsigned long last_keepalive;
> + struct delayed_work work;
> unsigned long status; /* Internal status bits */
> #define _WDOG_DEV_OPEN 0 /* Opened ? */
> #define _WDOG_ALLOW_RELEASE 1 /* Did we receive the magic char ? */
> @@ -71,6 +75,77 @@ static dev_t watchdog_devt;
> /* Reference to watchdog device behind /dev/watchdog */
> static struct watchdog_core_data *old_wd_data;
>
> +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_timeout_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)
> +{
> + struct watchdog_core_data *wd_data = wdd->wd_data;
> + unsigned int timeout_ms = wdd->timeout * 1000;
> + unsigned long keepalive_interval;
> + unsigned long last_heartbeat;
> + unsigned long virt_timeout;
> + unsigned int hw_timeout_ms;
> +
> + virt_timeout = wd_data->last_keepalive + msecs_to_jiffies(timeout_ms);
> + hw_timeout_ms = min(timeout_ms, wdd->max_hw_timeout_ms);
> + keepalive_interval = msecs_to_jiffies(hw_timeout_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_timeout_ms before this timeout.
> + */
> + last_heartbeat = virt_timeout - msecs_to_jiffies(hw_timeout_ms);
> + return min_t(long, last_heartbeat - jiffies, keepalive_interval);
> +}
> +
> +static inline void watchdog_update_worker(struct watchdog_device *wdd,
> + bool cancel)
> +{
> + struct watchdog_core_data *wd_data = wdd->wd_data;
> +
> + if (watchdog_need_worker(wdd)) {
> + long t = watchdog_next_keepalive(wdd);
> +
> + if (t > 0)
> + mod_delayed_work(watchdog_wq, &wd_data->work, t);
> + } else if (cancel) {
> + cancel_delayed_work(&wd_data->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, false);
> +
> + return err;
> +}
> +
> /*
> * watchdog_ping: ping the watchdog.
> * @wdd: the watchdog device to ping
> @@ -85,17 +160,28 @@ static struct watchdog_core_data *old_wd_data;
>
> static int watchdog_ping(struct watchdog_device *wdd)
> {
> - int err;
> + struct watchdog_core_data *wd_data = wdd->wd_data;
>
> 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 */
> + wd_data->last_keepalive = jiffies;
> + return __watchdog_ping(wdd);
> +}
>
> - return err;
> +static void watchdog_ping_work(struct work_struct *work)
> +{
> + struct watchdog_core_data *wd_data;
> + struct watchdog_device *wdd;
> +
> + wd_data = container_of(to_delayed_work(work), struct watchdog_core_data,
> + work);
> +
> + mutex_lock(&wd_data->lock);
> + wdd = wd_data->wdd;
> + if (wdd && watchdog_active(wdd))
> + __watchdog_ping(wdd);
> + mutex_unlock(&wd_data->lock);
> }
>
> /*
> @@ -111,14 +197,20 @@ static int watchdog_ping(struct watchdog_device *wdd)
>
> static int watchdog_start(struct watchdog_device *wdd)
> {
> + struct watchdog_core_data *wd_data = wdd->wd_data;
> + unsigned long started_at;
> int err;
>
> if (watchdog_active(wdd))
> return 0;
>
> + started_at = jiffies;
> err = wdd->ops->start(wdd);
> - if (err == 0)
> + if (err == 0) {
> set_bit(WDOG_ACTIVE, &wdd->status);
> + wd_data->last_keepalive = started_at;
> + watchdog_update_worker(wdd, true);
> + }
>
> return err;
> }
> @@ -137,6 +229,7 @@ static int watchdog_start(struct watchdog_device *wdd)
>
> static int watchdog_stop(struct watchdog_device *wdd)
> {
> + struct watchdog_core_data *wd_data = wdd->wd_data;
> int err;
>
> if (!watchdog_active(wdd))
> @@ -149,8 +242,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(&wd_data->work);
> + }
>
> return err;
> }
> @@ -183,13 +278,19 @@ static unsigned int watchdog_get_status(struct watchdog_device *wdd)
> static int watchdog_set_timeout(struct watchdog_device *wdd,
> unsigned int timeout)
> {
> + int err;
> +
> if (!wdd->ops->set_timeout || !(wdd->info->options & WDIOF_SETTIMEOUT))
> return -EOPNOTSUPP;
>
> if (watchdog_timeout_invalid(wdd, timeout))
> return -EINVAL;
>
> - return wdd->ops->set_timeout(wdd, timeout);
> + err = wdd->ops->set_timeout(wdd, timeout);
> +
> + watchdog_update_worker(wdd, true);
> +
> + return err;
> }
>
> /*
> @@ -609,6 +710,8 @@ static int watchdog_release(struct inode *inode, struct file *file)
> watchdog_ping(wdd);
> }
>
> + cancel_delayed_work_sync(&wd_data->work);
> +
> /* make sure that /dev/watchdog can be re-opened */
> clear_bit(_WDOG_DEV_OPEN, &wd_data->status);
>
> @@ -658,6 +761,11 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
> wd_data->wdd = wdd;
> wdd->wd_data = wd_data;
>
> + if (!watchdog_wq)
> + return -ENODEV;
> +
> + INIT_DELAYED_WORK(&wd_data->work, watchdog_ping_work);
> +
> if (wdd->id == 0) {
> old_wd_data = wd_data;
> watchdog_miscdev.parent = wdd->parent;
> @@ -715,6 +823,8 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
> wdd->wd_data = NULL;
> mutex_unlock(&wd_data->lock);
>
> + cancel_delayed_work_sync(&wd_data->work);
> +
> kref_put(&wd_data->kref, watchdog_core_data_release);
> }
>
> @@ -780,6 +890,13 @@ int __init watchdog_dev_init(void)
> {
> 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 = class_register(&watchdog_class);
> if (err < 0) {
> pr_err("couldn't register class\n");
> @@ -806,4 +923,5 @@ void __exit watchdog_dev_exit(void)
> {
> unregister_chrdev_region(watchdog_devt, MAX_DOGS);
> class_unregister(&watchdog_class);
> + destroy_workqueue(watchdog_wq);
> }
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index b585fa2507ee..cd5e6f84bf2f 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 <linux/notifier.h>
> #include <uapi/linux/watchdog.h>
>
> @@ -61,14 +62,19 @@ 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_timeout_ms is not provided.
> + * @max_hw_timeout_ms:
> + * Hardware limit for maximum timeout, in milli-seconds.
> + * Replaces max_timeout if specified.
> * @reboot_nb: The notifier block to stop watchdog on reboot.
> * @restart_nb: The notifier block to register a restart function.
> * @driver_data:Pointer to the drivers private data.
> * @wd_data: Pointer to watchdog core internal data.
> * @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.
> @@ -89,6 +95,7 @@ struct watchdog_device {
> unsigned int timeout;
> unsigned int min_timeout;
> unsigned int max_timeout;
> + unsigned int max_hw_timeout_ms;
> struct notifier_block reboot_nb;
> struct notifier_block restart_nb;
> void *driver_data;
> @@ -128,13 +135,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_timeout_ms && wdd->max_timeout &&
> + t > wdd->max_timeout);
> }
>
> /* Use the following functions to manipulate watchdog driver specific data */
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v7 1/9] watchdog: Introduce hardware maximum timeout in watchdog core
2016-02-28 13:18 ` Wim Van Sebroeck
@ 2016-02-28 18:43 ` Guenter Roeck
0 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2016-02-28 18:43 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: linux-watchdog, linux-kernel, Timo Kokkonen,
Uwe Kleine-König, linux-doc, Doug Anderson, Jonathan Corbet
Hi Wim,
On 02/28/2016 05:18 AM, Wim Van Sebroeck wrote:
> Hi Guenter,
>
> Code is OK, but I still have one last remark:
> When I coded the generic watchdog framework, I used the following terminology:
> * timeout for userspace timeout's
> * heartbeat for the internal hardware timeout.
> I would like us to stick to this, so that the story keeps being clear and consistent.
>
Ok,
> So the hardware maximum timeout where you talk about is actually
> a maximum heartbeat value. Can you change this in v8?
Ok.
> And then directly fold the drop of the 'cancel' parameter in the same patch?
>
Ok.
> I also like to have "Make set_timeout function optional" as the first patch of the series.
> This patch can be used even without the other patches.
>
Ok.
> I also like the WDOG_HW_RUNNING flag. But I would split the patch that adds this into 2 patches:
> * a patch that adds the WDOG_HW_RUNNING flag
> * and a patch that makes the stop function optional.
>
Ok.
Done all and pushed into the watchdog-timer branch of my repository on kernel.org.
I'll do some more testing, wait for results from 0day, and then resubmit the series.
Thanks,
Guenter
> Thanks in advance,
> Wim.
>
>> From: Guenter Roeck <linux@roeck-us.net>
>>
>> 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 hardware timeout value in the watchdog data
>> structure. If the configured timeout exceeds the maximum hardware timeout,
>> 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>
>> ---
>> v7:
>> - Rebased to v4.5-rc1
>> - Moved new variables to local data structure
>> - Dropped Uwe's Acked-by: due to substantial changes
>> v6:
>> - Set last_keepalive more accurately when starting the watchdog
>> - Rebased to v4.4-rc2
>> - Added Uwe's Acked-by:
>> v5:
>> - Rebased to v4.4-rc1
>> v4:
>> - Improved and fixed documentation
>> - Split hw_timeout_ms variable to timeout_ms, hw_timeout_ms for clarity
>> - Dropped redundant comments
>> - Added comments explaining failure conditions in watchdog_timeout_invalid().
>> - Moved the call to watchdog_update_worker() into _watchdog_ping().
>> v3:
>> - Reworked and cleaned up some of the functions.
>> - No longer call the worker update function if all that is needed is to stop
>> the worker.
>> - max_timeout will now be ignored if max_hw_timeout_ms is provided.
>> v2:
>> - Improved and hopefully clarified documentation.
>> - Rearranged variables in struct watchdog_device such that internal variables
>> come last.
>> - The code now ensures that the watchdog times out <timeout> seconds after
>> the most recent keepalive sent from user space.
>> - The internal keepalive now stops silently and no longer generates a
>> warning message. Reason is that it will now stop early, while there
>> may still be a substantial amount of time for keepalives from user space
>> to arrive. If such keepalives arrive late (for example if user space
>> is configured to send keepalives just a few seconds before the watchdog
>> times out), the message would just be noise and not provide any value.
>> ---
>> Documentation/watchdog/watchdog-kernel-api.txt | 19 +++-
>> drivers/watchdog/watchdog_dev.c | 136 +++++++++++++++++++++++--
>> include/linux/watchdog.h | 28 +++--
>> 3 files changed, 164 insertions(+), 19 deletions(-)
>>
>> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
>> index 55120a055a14..46979568b9e5 100644
>> --- a/Documentation/watchdog/watchdog-kernel-api.txt
>> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
>> @@ -52,6 +52,7 @@ struct watchdog_device {
>> unsigned int timeout;
>> unsigned int min_timeout;
>> unsigned int max_timeout;
>> + unsigned int max_hw_timeout_ms;
>> struct notifier_block reboot_nb;
>> struct notifier_block restart_nb;
>> void *driver_data;
>> @@ -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_timeout_ms is non-zero.
>> +* max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds.
>> + If set, the infrastructure will send heartbeats to the watchdog driver
>> + if 'timeout' is larger than max_hw_timeout_ms, unless WDOG_ACTIVE
>> + is set and userspace failed to send a heartbeat for at least 'timeout'
>> + seconds.
>> * reboot_nb: notifier block that is registered for reboot notifications, for
>> internal use only. If the driver calls watchdog_stop_on_reboot, watchdog core
>> will stop the watchdog on such notifications.
>> @@ -153,7 +164,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_timeout_ms set the hardware watchdog timeout
>> + to the minimum of timeout and max_hw_timeout_ms. Those drivers set the
>> + timeout value of the watchdog_device either to the requested timeout value
>> + (if it is larger than max_hw_timeout_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 ba2ecce4aae6..20e4ce0ebf6c 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -36,6 +36,7 @@
>> #include <linux/errno.h> /* For the -ENODEV/... values */
>> #include <linux/fs.h> /* For file operations */
>> #include <linux/init.h> /* For __init/__exit/... */
>> +#include <linux/jiffies.h> /* For timeout functions */
>> #include <linux/kernel.h> /* For printk/panic/... */
>> #include <linux/kref.h> /* For data references */
>> #include <linux/miscdevice.h> /* For handling misc devices */
>> @@ -44,6 +45,7 @@
>> #include <linux/slab.h> /* For memory functions */
>> #include <linux/types.h> /* For standard types (like size_t) */
>> #include <linux/watchdog.h> /* For watchdog specific items */
>> +#include <linux/workqueue.h> /* For workqueue */
>> #include <linux/uaccess.h> /* For copy_to_user/put_user/... */
>>
>> #include "watchdog_core.h"
>> @@ -61,6 +63,8 @@ struct watchdog_core_data {
>> struct cdev cdev;
>> struct watchdog_device *wdd;
>> struct mutex lock;
>> + unsigned long last_keepalive;
>> + struct delayed_work work;
>> unsigned long status; /* Internal status bits */
>> #define _WDOG_DEV_OPEN 0 /* Opened ? */
>> #define _WDOG_ALLOW_RELEASE 1 /* Did we receive the magic char ? */
>> @@ -71,6 +75,77 @@ static dev_t watchdog_devt;
>> /* Reference to watchdog device behind /dev/watchdog */
>> static struct watchdog_core_data *old_wd_data;
>>
>> +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_timeout_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)
>> +{
>> + struct watchdog_core_data *wd_data = wdd->wd_data;
>> + unsigned int timeout_ms = wdd->timeout * 1000;
>> + unsigned long keepalive_interval;
>> + unsigned long last_heartbeat;
>> + unsigned long virt_timeout;
>> + unsigned int hw_timeout_ms;
>> +
>> + virt_timeout = wd_data->last_keepalive + msecs_to_jiffies(timeout_ms);
>> + hw_timeout_ms = min(timeout_ms, wdd->max_hw_timeout_ms);
>> + keepalive_interval = msecs_to_jiffies(hw_timeout_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_timeout_ms before this timeout.
>> + */
>> + last_heartbeat = virt_timeout - msecs_to_jiffies(hw_timeout_ms);
>> + return min_t(long, last_heartbeat - jiffies, keepalive_interval);
>> +}
>> +
>> +static inline void watchdog_update_worker(struct watchdog_device *wdd,
>> + bool cancel)
>> +{
>> + struct watchdog_core_data *wd_data = wdd->wd_data;
>> +
>> + if (watchdog_need_worker(wdd)) {
>> + long t = watchdog_next_keepalive(wdd);
>> +
>> + if (t > 0)
>> + mod_delayed_work(watchdog_wq, &wd_data->work, t);
>> + } else if (cancel) {
>> + cancel_delayed_work(&wd_data->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, false);
>> +
>> + return err;
>> +}
>> +
>> /*
>> * watchdog_ping: ping the watchdog.
>> * @wdd: the watchdog device to ping
>> @@ -85,17 +160,28 @@ static struct watchdog_core_data *old_wd_data;
>>
>> static int watchdog_ping(struct watchdog_device *wdd)
>> {
>> - int err;
>> + struct watchdog_core_data *wd_data = wdd->wd_data;
>>
>> 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 */
>> + wd_data->last_keepalive = jiffies;
>> + return __watchdog_ping(wdd);
>> +}
>>
>> - return err;
>> +static void watchdog_ping_work(struct work_struct *work)
>> +{
>> + struct watchdog_core_data *wd_data;
>> + struct watchdog_device *wdd;
>> +
>> + wd_data = container_of(to_delayed_work(work), struct watchdog_core_data,
>> + work);
>> +
>> + mutex_lock(&wd_data->lock);
>> + wdd = wd_data->wdd;
>> + if (wdd && watchdog_active(wdd))
>> + __watchdog_ping(wdd);
>> + mutex_unlock(&wd_data->lock);
>> }
>>
>> /*
>> @@ -111,14 +197,20 @@ static int watchdog_ping(struct watchdog_device *wdd)
>>
>> static int watchdog_start(struct watchdog_device *wdd)
>> {
>> + struct watchdog_core_data *wd_data = wdd->wd_data;
>> + unsigned long started_at;
>> int err;
>>
>> if (watchdog_active(wdd))
>> return 0;
>>
>> + started_at = jiffies;
>> err = wdd->ops->start(wdd);
>> - if (err == 0)
>> + if (err == 0) {
>> set_bit(WDOG_ACTIVE, &wdd->status);
>> + wd_data->last_keepalive = started_at;
>> + watchdog_update_worker(wdd, true);
>> + }
>>
>> return err;
>> }
>> @@ -137,6 +229,7 @@ static int watchdog_start(struct watchdog_device *wdd)
>>
>> static int watchdog_stop(struct watchdog_device *wdd)
>> {
>> + struct watchdog_core_data *wd_data = wdd->wd_data;
>> int err;
>>
>> if (!watchdog_active(wdd))
>> @@ -149,8 +242,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(&wd_data->work);
>> + }
>>
>> return err;
>> }
>> @@ -183,13 +278,19 @@ static unsigned int watchdog_get_status(struct watchdog_device *wdd)
>> static int watchdog_set_timeout(struct watchdog_device *wdd,
>> unsigned int timeout)
>> {
>> + int err;
>> +
>> if (!wdd->ops->set_timeout || !(wdd->info->options & WDIOF_SETTIMEOUT))
>> return -EOPNOTSUPP;
>>
>> if (watchdog_timeout_invalid(wdd, timeout))
>> return -EINVAL;
>>
>> - return wdd->ops->set_timeout(wdd, timeout);
>> + err = wdd->ops->set_timeout(wdd, timeout);
>> +
>> + watchdog_update_worker(wdd, true);
>> +
>> + return err;
>> }
>>
>> /*
>> @@ -609,6 +710,8 @@ static int watchdog_release(struct inode *inode, struct file *file)
>> watchdog_ping(wdd);
>> }
>>
>> + cancel_delayed_work_sync(&wd_data->work);
>> +
>> /* make sure that /dev/watchdog can be re-opened */
>> clear_bit(_WDOG_DEV_OPEN, &wd_data->status);
>>
>> @@ -658,6 +761,11 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
>> wd_data->wdd = wdd;
>> wdd->wd_data = wd_data;
>>
>> + if (!watchdog_wq)
>> + return -ENODEV;
>> +
>> + INIT_DELAYED_WORK(&wd_data->work, watchdog_ping_work);
>> +
>> if (wdd->id == 0) {
>> old_wd_data = wd_data;
>> watchdog_miscdev.parent = wdd->parent;
>> @@ -715,6 +823,8 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
>> wdd->wd_data = NULL;
>> mutex_unlock(&wd_data->lock);
>>
>> + cancel_delayed_work_sync(&wd_data->work);
>> +
>> kref_put(&wd_data->kref, watchdog_core_data_release);
>> }
>>
>> @@ -780,6 +890,13 @@ int __init watchdog_dev_init(void)
>> {
>> 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 = class_register(&watchdog_class);
>> if (err < 0) {
>> pr_err("couldn't register class\n");
>> @@ -806,4 +923,5 @@ void __exit watchdog_dev_exit(void)
>> {
>> unregister_chrdev_region(watchdog_devt, MAX_DOGS);
>> class_unregister(&watchdog_class);
>> + destroy_workqueue(watchdog_wq);
>> }
>> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
>> index b585fa2507ee..cd5e6f84bf2f 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 <linux/notifier.h>
>> #include <uapi/linux/watchdog.h>
>>
>> @@ -61,14 +62,19 @@ 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_timeout_ms is not provided.
>> + * @max_hw_timeout_ms:
>> + * Hardware limit for maximum timeout, in milli-seconds.
>> + * Replaces max_timeout if specified.
>> * @reboot_nb: The notifier block to stop watchdog on reboot.
>> * @restart_nb: The notifier block to register a restart function.
>> * @driver_data:Pointer to the drivers private data.
>> * @wd_data: Pointer to watchdog core internal data.
>> * @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.
>> @@ -89,6 +95,7 @@ struct watchdog_device {
>> unsigned int timeout;
>> unsigned int min_timeout;
>> unsigned int max_timeout;
>> + unsigned int max_hw_timeout_ms;
>> struct notifier_block reboot_nb;
>> struct notifier_block restart_nb;
>> void *driver_data;
>> @@ -128,13 +135,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_timeout_ms && wdd->max_timeout &&
>> + t > wdd->max_timeout);
>> }
>>
>> /* Use the following functions to manipulate watchdog driver specific data */
>> --
>> 2.1.4
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2016-02-28 18:43 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-26 2:53 [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Guenter Roeck
2016-01-26 2:53 ` [PATCH v7 1/9] watchdog: Introduce hardware maximum timeout in watchdog core Guenter Roeck
2016-02-28 13:18 ` Wim Van Sebroeck
2016-02-28 18:43 ` Guenter Roeck
2016-01-26 2:53 ` [PATCH v7 2/9] watchdog: Introduce WDOG_HW_RUNNING flag Guenter Roeck
2016-01-26 2:53 ` [PATCH v7 3/9] watchdog: Make set_timeout function optional Guenter Roeck
2016-01-26 2:53 ` [PATCH v7 4/9] watchdog: Add support for minimum time between heartbeats Guenter Roeck
2016-01-26 8:07 ` Uwe Kleine-König
2016-01-26 14:42 ` Guenter Roeck
2016-01-26 2:53 ` [PATCH v7 5/9] watchdog: Simplify update_worker Guenter Roeck
2016-01-26 2:53 ` [RFT PATCH v7 6/9] watchdog: imx2: Convert to use infrastructure triggered keepalives Guenter Roeck
2016-01-29 19:40 ` Guenter Roeck
2016-01-26 2:53 ` [RFT PATCH v7 7/9] watchdog: retu: " Guenter Roeck
2016-01-26 2:53 ` [RFT PATCH v7 8/9] watchdog: at91sam9: " Guenter Roeck
2016-01-26 2:53 ` [RFT PATCH v7 9/9] watchdog: dw_wdt: Convert to use watchdog infrastructure Guenter Roeck
2016-01-29 17:52 ` Doug Anderson
2016-01-29 18:24 ` Guenter Roeck
2016-01-26 8:09 ` [PATCH v7 0/9] watchdog: Add support for keepalives triggered by infrastructure Uwe Kleine-König
2016-01-26 14:43 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.