On 05/21/2015 01:32 AM, fu.wei@xxxxxxxxxx wrote:
From: Fu Wei <fu.wei@xxxxxxxxxx>
Also update Documentation/watchdog/watchdog-kernel-api.txt to
introduce:
(1)the new elements in the watchdog_device and watchdog_ops struct;
(2)the new API "watchdog_init_timeouts".
Reasons:
(1)kernel already has two watchdog drivers are using "pretimeout":
drivers/char/ipmi/ipmi_watchdog.c
drivers/watchdog/kempld_wdt.c(but the definition is different)
(2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
drivers
Signed-off-by: Fu Wei <fu.wei@xxxxxxxxxx>
---
Documentation/watchdog/watchdog-kernel-api.txt | 62 ++++++++++++---
drivers/watchdog/watchdog_core.c | 103 +++++++++++++++++++------
drivers/watchdog/watchdog_dev.c | 48 ++++++++++++
include/linux/watchdog.h | 30 ++++++-
4 files changed, 208 insertions(+), 35 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index a0438f3..43900df 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -49,6 +49,9 @@ struct watchdog_device {
unsigned int timeout;
unsigned int min_timeout;
unsigned int max_timeout;
+ unsigned int pretimeout;
+ unsigned int min_pretimeout;
+ unsigned int max_pretimeout;
void *driver_data;
struct mutex lock;
unsigned long status;
@@ -70,6 +73,9 @@ It contains following fields:
* timeout: the watchdog timer's timeout value (in seconds).
* min_timeout: the watchdog timer's minimum timeout value (in seconds).
* max_timeout: the watchdog timer's maximum timeout value (in seconds).
+* pretimeout: the watchdog timer's pretimeout value (in seconds).
+* min_pretimeout: the watchdog timer's minimum pretimeout value (in seconds).
+* max_pretimeout: the watchdog timer's maximum pretimeout value (in seconds).
* bootstatus: status of the device after booting (reported with watchdog
WDIOF_* status bits).
* driver_data: a pointer to the drivers private data of a watchdog device.
@@ -92,6 +98,7 @@ struct watchdog_ops {
int (*ping)(struct watchdog_device *);
unsigned int (*status)(struct watchdog_device *);
int (*set_timeout)(struct watchdog_device *, unsigned int);
+ int (*set_pretimeout)(struct watchdog_device *, unsigned int);
unsigned int (*get_timeleft)(struct watchdog_device *);
void (*ref)(struct watchdog_device *);
void (*unref)(struct watchdog_device *);
@@ -153,9 +160,19 @@ 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 has a 1 second resolution;
+ If the driver supports pretimeout, then the timeout value must be greater
+ than that).
(Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
watchdog's info structure).
+* set_pretimeout: this routine checks and changes the pretimeout of the
+ watchdog timer device. It returns 0 on success, -EINVAL for "parameter out of
+ range" and -EIO for "could not write value to the watchdog". On success this
+ routine should set the pretimeout value of the watchdog_device to the
+ achieved pretimeout value (which may be different from the requested one
+ because the watchdog does not necessarily has a 1 second resolution).
+ (Note: the WDIOF_PRETIMEOUT 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.
* ref: the operation that calls kref_get on the kref of a dynamically
allocated watchdog_device struct.
@@ -213,14 +230,41 @@ The watchdog_get_drvdata function allows you to retrieve driver specific data.
The argument of this function is the watchdog device where you want to retrieve
data from. The function returns the pointer to the driver specific data.
-To initialize the timeout field, the following function can be used:
+To initialize the timeout and pretimeout fields, the following function can be
+used:
-extern int watchdog_init_timeout(struct watchdog_device *wdd,
- unsigned int timeout_parm, struct device *dev);
I think this API should still be listed here. Drivers not supporting pretimeout
can and should still use it.
+extern int watchdog_init_timeouts(struct watchdog_device *wdd,
+ unsigned int pretimeout_parm,
+ unsigned int timeout_parm,
+ void (*update_limits)(struct watchdog_device *),
+ struct device *dev);
-The watchdog_init_timeout function allows you to initialize the timeout field
-using the module timeout parameter or by retrieving the timeout-sec property from
-the device tree (if the module timeout parameter is invalid). Best practice is
-to set the default timeout value as timeout value in the watchdog_device and
-then use this function to set the user "preferred" timeout value.
+The watchdog_init_timeouts function allows you to initialize the pretimeout and
+timeout fields using the module pretimeout and timeout parameter or by
+retrieving the elements in the timeout-sec property(the first element is for
+timeout, the second one is for pretimeout) from the device tree(if the module
+pretimeout and timeout parameter are invalid).
+Normally, the pretimeout value will affect the limitation of timeout, and it
+is also hardware related. So you can write a function in your driver to update
+the limitation of timeout, according to the pretimeout value. Then pass the
+function pointer by the update_limits parameter. If you driver doesn't
+need this adjustment, just pass NULL to the update_limits parameter.
Is there a reason to believe that the update_limits function is necessary and
can not be handled by the set_pretimeout and set_timeout driver functions,
or possibly by the driver itself after calling watchdog_init_timeouts() ?
+Most of watchdog timers have not pretimeout as the warning signal. They just
+reset the system, once the timeout watch period expires. In this case, we can
+pass 0 to the pretimeout_parm, and pass NULL to the update_limits. Or use the
+old API: watchdog_init_timeout(see below)
+Best practice is to set the default pretimeout and timeout value as pretimeout
+and timeout value in the watchdog_device and then use this function to set the
+user "preferred" pretimeout value.
This routine returns zero on success and a negative errno code for failure.
+
+For backward compatibility, we also support the timeout initialization API:
+
Why only for backward compatibility ? Why (try to) force new drivers which don't
support pretimeout to use the new API function ?
+static inline int watchdog_init_timeout(struct watchdog_device *wdd,
+ unsigned int timeout_parm,
+ struct device *dev);
+
+Because of the introduction of pretimeout and watchdog_init_timeouts, this
+function has become a small wrapper function of watchdog_init_timeouts.
The last sentence is irrelevant; how watchdog_init_timeout() is implemented
is an implementation detail, not part of the API.
+
+
Empty lines at end of file will cause whitespace errors on merge. Doesn't checkpatch
complain about this ?
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index cec9b55..460796e 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -43,60 +43,115 @@
static DEFINE_IDA(watchdog_ida);
static struct class *watchdog_class;
-static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
+static void watchdog_check_min_max_timeouts(struct watchdog_device *wdd)
{
/*
- * Check that we have valid min and max timeout values, if
- * not reset them both to 0 (=not used or unknown)
+ * Check that we have valid min and max pretimeout and timeout values,
+ * if not reset them both to 0 (=not used or unknown)
*/
+ if (wdd->min_pretimeout > wdd->max_pretimeout) {
+ pr_info("Invalid min and max pretimeout, resetting to 0!\n");
Please drop the "!" at the end. While used in the old code, it is unnecessary
and often frowned upon nowadays.
+ wdd->min_pretimeout = 0;
+ wdd->max_pretimeout = 0;
+ }
if (wdd->min_timeout > wdd->max_timeout) {
pr_info("Invalid min and max timeout values, resetting to 0!\n");
wdd->min_timeout = 0;
wdd->max_timeout = 0;
}
+ /*
+ * Check that we have valid min and max timeout values,
+ * if not reset them both to pretimeout limits
+ */
+ if (wdd->min_pretimeout && wdd->min_timeout < wdd->min_pretimeout) {
+ pr_info("Invalid min timeout, resetting to min pretimeout!\n");
+ wdd->min_timeout = wdd->min_pretimeout;
+ }
+ if (wdd->max_pretimeout && wdd->max_timeout < wdd->max_pretimeout) {
+ pr_info("Invalid max timeout, resetting to max pretimeout!\n");
+ wdd->max_timeout = wdd->max_pretimeout;
+ }
}
/**
- * watchdog_init_timeout() - initialize the timeout field
+ * watchdog_init_timeouts() - initialize the pretimeout and timeout field
+ * @pretimeout_parm: pretimeout module parameter
* @timeout_parm: timeout module parameter
* @dev: Device that stores the timeout-sec property
*
- * Initialize the timeout field of the watchdog_device struct with either the
- * timeout module parameter (if it is valid value) or the timeout-sec property
- * (only if it is a valid value and the timeout_parm is out of bounds).
- * If none of them are valid then we keep the old value (which should normally
- * be the default timeout value.
+ * Initialize the pretimeout and timeout field of the watchdog_device struct
+ * with either the pretimeout and timeout module parameter (if it is valid
+ * value) or the timeout-sec property (only if it is a valid value and the
+ * pretimeout_parm and timeout_parm is out of bounds). If none of them are
+ * valid then we keep the old value (which should normally be the default
+ * timeout value.
valid, then we keep ... default timeout value).
*
* A zero is returned on success and -EINVAL for failure.
*/
-int watchdog_init_timeout(struct watchdog_device *wdd,
- unsigned int timeout_parm, struct device *dev)
+int watchdog_init_timeouts(struct watchdog_device *wdd,
+ unsigned int pretimeout_parm,
+ unsigned int timeout_parm,
+ void (*update_limits)(struct watchdog_device *),
+ struct device *dev)
{
- unsigned int t = 0;
+ unsigned int timeout = 0, pretimeout = 0;
+ const __be32 *ppretimeout;
int ret = 0;
+ struct property *timeout_sec;
+ int length;
- watchdog_check_min_max_timeout(wdd);
+ watchdog_check_min_max_timeouts(wdd);
- /* try to get the timeout module parameter first */
- if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm) {
- wdd->timeout = timeout_parm;
- return ret;
+ /* try to get the timeout and pretimeout module parameter first */
+ if (pretimeout_parm) {
+ if (!watchdog_pretimeout_invalid(wdd, pretimeout_parm)) {
+ wdd->pretimeout = pretimeout_parm;
+ if (update_limits)
+ update_limits(wdd);
+ } else {
+ pr_warn("Invalid pretimeout parameter!\n");
We didn't have all this noise earlier. Can we keep it that way ?
+ ret = -EINVAL;
+ }
}
- if (timeout_parm)
+
+ if (timeout_parm) {
+ if (!watchdog_timeout_invalid(wdd, timeout_parm)) {
+ wdd->timeout = timeout_parm;
+ return ret;
+ }
+ pr_warn("Invalid timeout parameter!\n");
ret = -EINVAL;
+ }
/* try to get the timeout_sec property */
if (dev == NULL || dev->of_node == NULL)
return ret;
- of_property_read_u32(dev->of_node, "timeout-sec", &t);
- if (!watchdog_timeout_invalid(wdd, t) && t)
- wdd->timeout = t;
- else
+
+ timeout_sec = of_find_property(dev->of_node, "timeout-sec", &length);
+ if (timeout_sec) {
+ ppretimeout = of_prop_next_u32(timeout_sec, NULL, &timeout);
Could you just use of_property_read_u32_array() and pass length as parameter ?
+ if (length == 2) {
+ of_prop_next_u32(timeout_sec, ppretimeout, &pretimeout);
+ if (!watchdog_pretimeout_invalid(wdd, pretimeout) &&
+ pretimeout) {
+ wdd->pretimeout = pretimeout;
+ if (update_limits)
+ update_limits(wdd);
+ } else {
+ ret = -EINVAL;
+ }
+ }
+ if (!watchdog_timeout_invalid(wdd, timeout) && timeout)
+ wdd->timeout = timeout;
+ else
+ ret = -EINVAL;
+ } else {
ret = -EINVAL;
+ }
return ret;
}
-EXPORT_SYMBOL_GPL(watchdog_init_timeout);
+EXPORT_SYMBOL_GPL(watchdog_init_timeouts);
/**
* watchdog_register_device() - register a watchdog device
@@ -119,7 +174,7 @@ int watchdog_register_device(struct watchdog_device *wdd)
if (wdd->ops->start == NULL || wdd->ops->stop == NULL)
return -EINVAL;
- watchdog_check_min_max_timeout(wdd);
+ watchdog_check_min_max_timeouts(wdd);
/*
* Note: now that all watchdog_device data has been verified, we
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 6aaefba..b519257 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -218,6 +218,38 @@ out_timeout:
}
/*
+ * watchdog_set_pretimeout: set the watchdog timer pretimeout
+ * @wddev: the watchdog device to set the timeout for
+ * @pretimeout: pretimeout to set in seconds
+ */
+
+static int watchdog_set_pretimeout(struct watchdog_device *wddev,
+ unsigned int pretimeout)
+{
+ int err;
+
+ if (!wddev->ops->set_pretimeout ||
+ !(wddev->info->options & WDIOF_PRETIMEOUT))
+ return -EOPNOTSUPP;
+
+ if (watchdog_pretimeout_invalid(wddev, pretimeout))
+ return -EINVAL;
+
+ mutex_lock(&wddev->lock);
+
+ if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+ err = -ENODEV;
+ goto out_pretimeout;
+ }
+
+ err = wddev->ops->set_pretimeout(wddev, pretimeout);
+
+out_pretimeout:
+ mutex_unlock(&wddev->lock);
+ return err;
+}
+
+/*
* watchdog_get_timeleft: wrapper to get the time left before a reboot
* @wddev: the watchdog device to get the remaining time from
* @timeleft: the time that's left
@@ -388,6 +420,22 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
if (wdd->timeout == 0)
return -EOPNOTSUPP;
return put_user(wdd->timeout, p);
+ case WDIOC_SETPRETIMEOUT:
+ if (get_user(val, p))
+ return -EFAULT;
+ err = watchdog_set_pretimeout(wdd, val);
+ if (err < 0)
+ return err;
+ /* If the watchdog is active then we send a keepalive ping
+ * to make sure that the watchdog keep's running (and if
+ * possible that it takes the new timeout) */
Please use the standard multi-line comment format.
+ watchdog_ping(wdd);
+ /* Fall */
+ case WDIOC_GETPRETIMEOUT:
+ /* timeout == 0 means that we don't use the pretimeout */
pretimeout == 0 means ...
"use" or "know" ?
+ if (wdd->pretimeout == 0)
+ return -EOPNOTSUPP;
+ return put_user(wdd->pretimeout, p);
case WDIOC_GETTIMELEFT:
err = watchdog_get_timeleft(wdd, &val);
if (err)
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index a746bf5..df430a3 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -25,6 +25,7 @@ struct watchdog_device;
* @ping: The routine that sends a keepalive ping to the watchdog device.
* @status: The routine that shows the status of the watchdog device.
* @set_timeout:The routine for setting the watchdog devices timeout value.
+ * @set_pretimeout:The routine for setting the watchdog devices pretimeout value
* @get_timeleft:The routine that get's the time that's left before a reset.
* @ref: The ref operation for dyn. allocated watchdog_device structs
* @unref: The unref operation for dyn. allocated watchdog_device structs
@@ -44,6 +45,7 @@ struct watchdog_ops {
int (*ping)(struct watchdog_device *);
unsigned int (*status)(struct watchdog_device *);
int (*set_timeout)(struct watchdog_device *, unsigned int);
+ int (*set_pretimeout)(struct watchdog_device *, unsigned int);
unsigned int (*get_timeleft)(struct watchdog_device *);
void (*ref)(struct watchdog_device *);
void (*unref)(struct watchdog_device *);
@@ -62,6 +64,9 @@ struct watchdog_ops {
* @timeout: The watchdog devices timeout value.
* @min_timeout:The watchdog devices minimum timeout value.
* @max_timeout:The watchdog devices maximum timeout value.
+ * @pretimeout: The watchdog devices pretimeout value.
+ * @min_pretimeout:The watchdog devices minimum pretimeout value.
+ * @max_pretimeout:The watchdog devices maximum pretimeout value.
* @driver-data:Pointer to the drivers private data.
* @lock: Lock for watchdog core internal use only.
* @status: Field that contains the devices internal status bits.
@@ -86,6 +91,9 @@ struct watchdog_device {
unsigned int timeout;
unsigned int min_timeout;
unsigned int max_timeout;
+ unsigned int pretimeout;
+ unsigned int min_pretimeout;
+ unsigned int max_pretimeout;
void *driver_data;
struct mutex lock;
unsigned long status;
@@ -120,6 +128,14 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
(t < wdd->min_timeout || t > wdd->max_timeout));
}
+/* Use the following function to check if a pretimeout value is invalid */
+static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd,
+ unsigned int t)
+{
+ return ((wdd->max_pretimeout != 0) &&
Please drop the unnecessary ( ).
+ (t < wdd->min_pretimeout || t > wdd->max_pretimeout));
+}
+
/* Use the following functions to manipulate watchdog driver specific data */
static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data)
{
@@ -132,11 +148,21 @@ static inline void *watchdog_get_drvdata(struct watchdog_device *wdd)
}
/* drivers/watchdog/watchdog_core.c */
-extern int watchdog_init_timeout(struct watchdog_device *wdd,
- unsigned int timeout_parm, struct device *dev);
+extern int watchdog_init_timeouts(struct watchdog_device *wdd,
+ unsigned int pretimeout_parm,
+ unsigned int timeout_parm,
+ void (*update_limits)(struct watchdog_device *),
+ struct device *dev);
extern int watchdog_register_device(struct watchdog_device *);
extern void watchdog_unregister_device(struct watchdog_device *);
+static inline int watchdog_init_timeout(struct watchdog_device *wdd,
+ unsigned int timeout_parm,
+ struct device *dev)
+{
+ return watchdog_init_timeouts(wdd, 0, timeout_parm, NULL, dev);
+}
+
#ifdef CONFIG_HARDLOCKUP_DETECTOR
void watchdog_nmi_disable_all(void);
void watchdog_nmi_enable_all(void);
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
