Re: [RFC] [PATCH 5/10] Generic Watchdog Timer Driver

From: Mike Waychison
Date: Wed Feb 23 2011 - 16:59:24 EST

On 02/23/11 12:43, Wim Van Sebroeck wrote:
commit 1959ea403c69af855f5cd13e5c9b33123d2137b2
Author: Wim Van Sebroeck<wim@xxxxxxxxx>
Date: Fri Jun 18 09:45:49 2010 +0000

watchdog: WatchDog Timer Driver Core - Part 5

This part add's the WDIOC_SETTIMEOUT and WDIOC_GETTIMEOUT ioctl
functionality to the WatchDog Timer Driver Core framework.

Signed-off-by: Alan Cox<alan@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Wim Van Sebroeck<wim@xxxxxxxxx>

diff --git a/Documentation/watchdog/src/watchdog-with-timer-example.c b/Documentation/watchdog/src/watchdog-with-timer-example.c
index 6a4af47..f1d4f217 100644
--- a/Documentation/watchdog/src/watchdog-with-timer-example.c
+++ b/Documentation/watchdog/src/watchdog-with-timer-example.c
@@ -110,18 +110,27 @@ static int wdt_stop(struct watchdog_device *wdd)
return 0;
}

+static int wdt_set_timeout(struct watchdog_device *wdd, int new_timeout)
+{
+ if (new_timeout< 1)
+ return -EINVAL;
+ return 0;
+}
+
/*
* The watchdog kernel structures
*/
static const struct watchdog_info wdt_info = {
.identity = DRV_NAME,
- .options = WDIOF_KEEPALIVEPING,
+ .options = WDIOF_SETTIMEOUT |
+ WDIOF_KEEPALIVEPING,
};

static const struct watchdog_ops wdt_ops = {
.start = wdt_start,
.stop = wdt_stop,
.ping = wdt_ping,
+ .set_timeout = wdt_set_timeout,
};

static struct watchdog_device wdt_dev = {
@@ -139,6 +148,9 @@ static int __devinit wdt_probe(struct platform_device *pdev)

/* Register other stuff */

+ /* Set watchdog_device parameters */
+ wdt_dev.timeout = timeout;
+
/* Register the watchdog timer device */
res = register_watchdogdevice(&wdt_dev);
if (res) {
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 3de69e7..f15e8d4 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -42,6 +42,7 @@ struct watchdog_device {
char *name;
const struct watchdog_info *info;
const struct watchdog_ops *ops;
+ int timeout;
int bootstatus;
long status;
};
@@ -51,6 +52,7 @@ It contains following fields:
* info: a pointer to a watchdog_info structure. This structure gives some
additional information about the watchdog timer itself.
* ops: a pointer to the list of watchdog operations that the watchdog supports.
+* timeout: the watchdog timer's timeout value (in seconds).
* bootstatus: status of the device after booting (reported with watchdog
WDIOF_* status bits).
* status: this field contains a number of status bits that give extra
@@ -67,6 +69,7 @@ struct watchdog_ops {
/* optional operations */
int (*ping)(struct watchdog_device *);
int (*status)(struct watchdog_device *);
+ int (*set_timeout)(struct watchdog_device *, int);
};

Some operations are mandatory and some are optional. The mandatory operations
@@ -102,6 +105,12 @@ they are supported. These optional routines/operations are:
info structure).
* status: this routine checks the status of the watchdog timer device. The
status of the device is reported with watchdog WDIOF_* status flags/bits.
+* set_timeout: this routine checks and changes the timeout of the watchdog
+ timer device. It returns 0 on success and an errno code on failure. On success
+ the timeout value of the watchdog_device will be changed to the value that
+ was just used to re-program the watchdog timer device.
+ (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
+ watchdog's info structure).

The status bits should (preferably) be set with the set_bit and clear_bit alike
bit-operations. The status bit's that are defined are:
diff --git a/drivers/watchdog/core/watchdog_dev.c b/drivers/watchdog/core/watchdog_dev.c
index 2bf4f67..b80c6e6 100644
--- a/drivers/watchdog/core/watchdog_dev.c
+++ b/drivers/watchdog/core/watchdog_dev.c
@@ -221,6 +221,26 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
return -EOPNOTSUPP;
watchdog_ping(wdd);
return 0;
+ case WDIOC_SETTIMEOUT:
+ if ((wdd->ops->set_timeout == NULL) ||
+ !(wdd->info->options& WDIOF_SETTIMEOUT))
+ return -EOPNOTSUPP;
+ if (get_user(val, p))
+ return -EFAULT;

Should we sanity check that val > 0 here? I realize that the driver would need to do it's own sanity checking, but a value of 0 or < 0 makes no sense to the core itself.

Maybe timeout is best expressed as u32 if this is the case.

+ err = wdd->ops->set_timeout(wdd, val);
+ if (err< 0)
+ return err;
+ wdd->timeout = val;
+ /* If the watchdog is active then we sent a keepalive ping
+ * to make sure that the watchdog keep's running (and if
+ * possible that it takes the new timeout) */
+ watchdog_ping(wdd);
+ /* Fall */
+ case WDIOC_GETTIMEOUT:
+ /* timeout == 0 means that we don't know the timeout */
+ if (wdd->timeout)
+ return put_user(wdd->timeout, p);
+ return -EOPNOTSUPP;
default:
return -ENOTTY;
}
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 736814f..e35f51f 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -70,6 +70,7 @@ struct watchdog_ops {
/* optional operations */
int (*ping)(struct watchdog_device *);
int (*status)(struct watchdog_device *);
+ int (*set_timeout)(struct watchdog_device *, int);
};

/* The structure that defines a watchdog device */
@@ -78,6 +79,7 @@ struct watchdog_device {
const struct watchdog_info *info;
const struct watchdog_ops *ops;
int bootstatus;
+ int timeout;
long status;
#define WDOG_ACTIVE 0 /* is the watchdog running/active */
#define WDOG_DEV_OPEN 1 /* is the watchdog opened via

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/