[RFC 0/2] Proposal for Watchdog Timer Resolution

[RFC 0/2] Proposal for Watchdog Timer Resolution

[Markus Mayer]

I would like to propose the following extension to the watchdog
framework to allow userland programs access to the resolution of the
watchdog timer should the hardware support such a feature. The first
driver to make use of this feature would be the BCM Kona watchdog
driver.

I would also like to propose introducing a sysfs interface to the
watchdog framework in a later phase of updating the watchdog framework.
The sysfs interface would exist in addition to the existing ioctl
interface. In the future, the ioctl interface could be deprecated in
favour of the sysfs interface or both interfaces could be kept
indefinitely.

Phase 1:

Add get resolution/set resolution ioctls.
Add necessary flags and call-back functions.
Extend bcm_kona_wdt.c to make use of the new functionality.

Phase 2:

Abstract the ioctl interface (into watchdog_ioctl.c) with a new set of
per-driver ops, similar to what other frameworks are already doing.

Phase 3:

Introduce a sysfs interface (watchdog_sysfs.c) hooking into the already
established per-driver ops. ioctl and sysfs calls would use the same
framework and driver functions, requiring no code duplication.

Please let me know if this sounds sensible or whether an alternate
approach would be preferred.

To give you an idea I am attaching an initial patch series for phase 1.
Please keep in mind that the series is not meant to be applied as is.
The code may need a bit more cleaning up. Also, I didn't add any
documentation for the new features. Lastly, the series is currently
only broken down into 2 patches (all framework changes and the actual
driver changes), without further regard as to how this series may be
applied.

Patch 2/2 is dependent on the previous bcm_kona_wdt series:

http://www.spinics.net/lists/linux-watchdog/msg03328.html

Regards,
-Markus


Markus Mayer (2):
  watchdog: Add support to set timer resolution
  watchdog: bcm281xx: Support for timer resolution ioctls

 drivers/watchdog/bcm_kona_wdt.c  |   57 +++++++++++++++++++++-----------------
 drivers/watchdog/watchdog_core.c |   14 ++++++++++
 drivers/watchdog/watchdog_dev.c  |   41 +++++++++++++++++++++++++++
 include/linux/watchdog.h         |   12 ++++++++
 include/uapi/linux/watchdog.h    |    3 ++
 5 files changed, 101 insertions(+), 26 deletions(-)

-- 
1.7.9.5

[RFC 1/2] watchdog: Add support to set timer resolution

[Markus Mayer]

This change adds support for watchdog timer resolution to the watchdog
framework. Two new ioctls are introduced:

  - WDIOC_GETRESOLUTION
  - WDIOC_SETRESOLUTION

They allow userland applications to retreive and program the watchdog
timer's resolution should the hardware and the driver support it.
---
 drivers/watchdog/watchdog_core.c |   14 +++++++++++++
 drivers/watchdog/watchdog_dev.c  |   41 ++++++++++++++++++++++++++++++++++++++
 include/linux/watchdog.h         |   12 +++++++++++
 include/uapi/linux/watchdog.h    |    3 +++
 4 files changed, 70 insertions(+)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 461336c..ca29e64 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -56,6 +56,19 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
 	}
 }
 
+static void watchdog_check_min_max_resolution(struct watchdog_device *wdd)
+{
+	/*
+	 * Check that we have valid min and max timeout values, if
+	 * not reset them both to 0 (=not used or unknown)
+	 */
+	if (wdd->min_resolution > wdd->max_resolution) {
+		pr_info("Invalid min & max resolution values, setting to 0!\n");
+		wdd->min_resolution = 0;
+		wdd->max_resolution = 0;
+	}
+}
+
 /**
  * watchdog_init_timeout() - initialize the timeout field
  * @timeout_parm: timeout module parameter
@@ -120,6 +133,7 @@ int watchdog_register_device(struct watchdog_device *wdd)
 		return -EINVAL;
 
 	watchdog_check_min_max_timeout(wdd);
+	watchdog_check_min_max_resolution(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..fd4ea61 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -218,6 +218,37 @@ out_timeout:
 }
 
 /*
+ *	watchdog_set_resolution: set the watchdog timer resolution
+ *	@wddev: the watchdog device to set the resolution for
+ *	@resolution: timer resolution (in native format)
+ */
+static int watchdog_set_resolution(struct watchdog_device *wddev,
+							unsigned int resolution)
+{
+	int err;
+
+	if ((wddev->ops->set_resolution == NULL) ||
+	    !(wddev->info->options & WDIOF_SETRESOLUTION))
+		return -EOPNOTSUPP;
+
+	if (watchdog_resolution_invalid(wddev, resolution))
+		return -EINVAL;
+
+	mutex_lock(&wddev->lock);
+
+	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+		err = -ENODEV;
+		goto out_resolution;
+	}
+
+	err = wddev->ops->set_resolution(wddev, resolution);
+
+out_resolution:
+	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
@@ -393,6 +424,16 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 		if (err)
 			return err;
 		return put_user(val, p);
+	case WDIOC_SETRESOLUTION:
+		if (get_user(val, p))
+			return -EFAULT;
+		err = watchdog_set_resolution(wdd, val);
+		if (err < 0)
+			return err;
+		watchdog_ping(wdd);
+		return 0;
+	case WDIOC_GETRESOLUTION:
+		return put_user(wdd->resolution, p);
 	default:
 		return -ENOTTY;
 	}
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 2a3038e..c9b0a4dc 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_resolution:The routine for setting the timer resolution 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_resolution)(struct watchdog_device *, unsigned int);
 	unsigned int (*get_timeleft)(struct watchdog_device *);
 	void (*ref)(struct watchdog_device *);
 	void (*unref)(struct watchdog_device *);
@@ -86,6 +88,9 @@ struct watchdog_device {
 	unsigned int timeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
+	unsigned int resolution;
+	unsigned int min_resolution;
+	unsigned int max_resolution;
 	void *driver_data;
 	struct mutex lock;
 	unsigned long status;
@@ -125,6 +130,13 @@ 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 resolution value is invalid */
+static inline bool watchdog_resolution_invalid(struct watchdog_device *wdd, unsigned int r)
+{
+	return ((wdd->max_resolution != 0) &&
+		(r < wdd->min_resolution || r > wdd->max_resolution));
+}
+
 /* Use the following functions to manipulate watchdog driver specific data */
 static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data)
 {
diff --git a/include/uapi/linux/watchdog.h b/include/uapi/linux/watchdog.h
index 2babe72..15cab9b 100644
--- a/include/uapi/linux/watchdog.h
+++ b/include/uapi/linux/watchdog.h
@@ -31,6 +31,8 @@ struct watchdog_info {
 #define	WDIOC_SETPRETIMEOUT	_IOWR(WATCHDOG_IOCTL_BASE, 8, int)
 #define	WDIOC_GETPRETIMEOUT	_IOR(WATCHDOG_IOCTL_BASE, 9, int)
 #define	WDIOC_GETTIMELEFT	_IOR(WATCHDOG_IOCTL_BASE, 10, int)
+#define	WDIOC_GETRESOLUTION	_IOR(WATCHDOG_IOCTL_BASE, 11, int)
+#define	WDIOC_SETRESOLUTION	_IOWR(WATCHDOG_IOCTL_BASE, 12, int)
 
 #define	WDIOF_UNKNOWN		-1	/* Unknown flag error */
 #define	WDIOS_UNKNOWN		-1	/* Unknown status error */
@@ -47,6 +49,7 @@ struct watchdog_info {
 #define	WDIOF_PRETIMEOUT	0x0200  /* Pretimeout (in seconds), get/set */
 #define	WDIOF_ALARMONLY		0x0400	/* Watchdog triggers a management or
 					   other external alarm not a reboot */
+#define	WDIOF_SETRESOLUTION	0x0800  /* Set resolution */
 #define	WDIOF_KEEPALIVEPING	0x8000	/* Keep alive ping reply */
 
 #define	WDIOS_DISABLECARD	0x0001	/* Turn off the watchdog timer */
-- 
1.7.9.5

[RFC 2/2] watchdog: bcm281xx: Support for timer resolution ioctls

[Markus Mayer]

This change makes use of the watchdog timer resolution support that is
present in the watchdog framework. It depends on the following patch series:
http://www.spinics.net/lists/linux-watchdog/msg03328.html

A userland application can set or retrieve the watchdog timer's
resolution using code like this:

	/* Set the watchdog timer resolution */
	resolution = ... ;
	printf("setting watchdog timer resolution to %d\n", resolution);
	ret = ioctl(fd, WDIOC_SETRESOLUTION, &resolution);
	if (ret < 0)
		fprintf(stderr, "%s: ioctl failed -- %s\n", prg,
						strerror(errno));

	/* Retrieve the watchdog timer resolution */
	ret = ioctl(fd, WDIOC_GETRESOLUTION, &resolution);
	if (ret < 0)
		fprintf(stderr, "%s: ioctl failed -- %s\n", prg,
						strerror(errno));
	else
		printf("resolution: %d\n",resolution);
	break;
---
 drivers/watchdog/bcm_kona_wdt.c |   57 +++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/drivers/watchdog/bcm_kona_wdt.c b/drivers/watchdog/bcm_kona_wdt.c
index 9fe174b..30e4e9b 100644
--- a/drivers/watchdog/bcm_kona_wdt.c
+++ b/drivers/watchdog/bcm_kona_wdt.c
@@ -54,7 +54,6 @@ struct bcm_kona_wdt {
 	 * us a maximum of about 18 hours and 12 minutes before the watchdog
 	 * times out.
 	 */
-	int resolution;
 	spinlock_t lock;
 #ifdef CONFIG_BCM_KONA_WDT_DEBUG
 	struct dentry *debugfs;
@@ -104,9 +103,10 @@ static int bcm_kona_wdt_dbg_show(struct seq_file *s, void *data)
 {
 	int ctl_val, cur_val, ret;
 	unsigned long flags;
-	struct bcm_kona_wdt *wdt = s->private;
+	struct watchdog_device *wdd = s->private;
+	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdd);
 
-	if (!wdt)
+	if (!wdd || !wdt)
 		return seq_puts(s, "No device pointer\n");
 
 	spin_lock_irqsave(&wdt->lock, flags);
@@ -122,13 +122,13 @@ static int bcm_kona_wdt_dbg_show(struct seq_file *s, void *data)
 		ctl = ctl_val & SECWDOG_COUNT_MASK;
 		res = (ctl_val & SECWDOG_RES_MASK) >> SECWDOG_CLKS_SHIFT;
 		cur = cur_val & SECWDOG_COUNT_MASK;
-		ctl_sec = TICKS_TO_SECS(ctl, wdt);
-		cur_sec = TICKS_TO_SECS(cur, wdt);
+		ctl_sec = TICKS_TO_SECS(ctl, wdd);
+		cur_sec = TICKS_TO_SECS(cur, wdd);
 		ret = seq_printf(s, "Resolution: %d / %d\n"
 				"Control: %d s / %d (%#x) ticks\n"
 				"Current: %d s / %d (%#x) ticks\n"
 				"Busy count: %lu\n", res,
-				wdt->resolution, ctl_sec, ctl, ctl, cur_sec,
+				wdd->resolution, ctl_sec, ctl, ctl, cur_sec,
 				cur, cur, busy_count);
 	}
 
@@ -156,7 +156,7 @@ static struct dentry *bcm_kona_wdt_debugfs_init(struct bcm_kona_wdt *wdt,
 	if (IS_ERR_OR_NULL(dir))
 		return NULL;
 
-	if (debugfs_create_file("info", S_IFREG | S_IRUGO, dir, wdt,
+	if (debugfs_create_file("info", S_IFREG | S_IRUGO, dir, wdd,
 				&bcm_kona_dbg_operations))
 		return dir;
 
@@ -195,13 +195,14 @@ static int bcm_kona_wdt_ctrl_reg_modify(struct bcm_kona_wdt *wdt,
 	return ret;
 }
 
-static int bcm_kona_wdt_set_resolution_reg(struct bcm_kona_wdt *wdt)
+static int bcm_kona_wdt_set_resolution(struct watchdog_device *wdog,
+	unsigned r)
 {
-	if (wdt->resolution > SECWDOG_MAX_RES)
-		return -EINVAL;
+	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
 
+	wdog->resolution = r;
 	return bcm_kona_wdt_ctrl_reg_modify(wdt, SECWDOG_RES_MASK,
-					wdt->resolution << SECWDOG_CLKS_SHIFT);
+					r << SECWDOG_CLKS_SHIFT);
 }
 
 static int bcm_kona_wdt_set_timeout_reg(struct watchdog_device *wdog,
@@ -210,7 +211,7 @@ static int bcm_kona_wdt_set_timeout_reg(struct watchdog_device *wdog,
 	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
 
 	return bcm_kona_wdt_ctrl_reg_modify(wdt, SECWDOG_COUNT_MASK,
-					SECS_TO_TICKS(wdog->timeout, wdt) |
+					SECS_TO_TICKS(wdog->timeout, wdog) |
 					watchdog_flags);
 }
 
@@ -234,7 +235,7 @@ static unsigned int bcm_kona_wdt_get_timeleft(struct watchdog_device *wdog)
 	if (val < 0)
 		return val;
 
-	return TICKS_TO_SECS(val & SECWDOG_COUNT_MASK, wdt);
+	return TICKS_TO_SECS(val & SECWDOG_COUNT_MASK, wdog);
 }
 
 static int bcm_kona_wdt_start(struct watchdog_device *wdog)
@@ -252,16 +253,17 @@ static int bcm_kona_wdt_stop(struct watchdog_device *wdog)
 }
 
 static struct watchdog_ops bcm_kona_wdt_ops = {
-	.owner =	THIS_MODULE,
-	.start =	bcm_kona_wdt_start,
-	.stop =		bcm_kona_wdt_stop,
-	.set_timeout =	bcm_kona_wdt_set_timeout,
-	.get_timeleft =	bcm_kona_wdt_get_timeleft,
+	.owner =		THIS_MODULE,
+	.start =		bcm_kona_wdt_start,
+	.stop =			bcm_kona_wdt_stop,
+	.set_timeout =		bcm_kona_wdt_set_timeout,
+	.get_timeleft =		bcm_kona_wdt_get_timeleft,
+	.set_resolution =	bcm_kona_wdt_set_resolution,
 };
 
 static struct watchdog_info bcm_kona_wdt_info = {
-	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
-			WDIOF_KEEPALIVEPING,
+	.options =	WDIOF_SETTIMEOUT | WDIOF_SETRESOLUTION |
+			WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
 	.identity =	"Broadcom Kona Watchdog Timer",
 };
 
@@ -271,6 +273,9 @@ static struct watchdog_device bcm_kona_wdt_wdd = {
 	.min_timeout =	1,
 	.max_timeout =	SECWDOG_MAX_COUNT >> SECWDOG_DEFAULT_RESOLUTION,
 	.timeout =	SECWDOG_MAX_COUNT >> SECWDOG_DEFAULT_RESOLUTION,
+	.min_resolution = 0,
+	.max_resolution = SECWDOG_MAX_RES,
+	.resolution = SECWDOG_DEFAULT_RESOLUTION
 };
 
 static void bcm_kona_wdt_shutdown(struct platform_device *pdev)
@@ -294,17 +299,17 @@ static int bcm_kona_wdt_probe(struct platform_device *pdev)
 	if (IS_ERR(wdt->base))
 		return -ENODEV;
 
-	wdt->resolution = SECWDOG_DEFAULT_RESOLUTION;
-	ret = bcm_kona_wdt_set_resolution_reg(wdt);
+	spin_lock_init(&wdt->lock);
+	platform_set_drvdata(pdev, wdt);
+	watchdog_set_drvdata(&bcm_kona_wdt_wdd, wdt);
+
+	ret = bcm_kona_wdt_set_resolution(&bcm_kona_wdt_wdd,
+				bcm_kona_wdt_wdd.resolution);
 	if (ret) {
 		dev_err(dev, "Failed to set resolution (error: %d)", ret);
 		return ret;
 	}
 
-	spin_lock_init(&wdt->lock);
-	platform_set_drvdata(pdev, wdt);
-	watchdog_set_drvdata(&bcm_kona_wdt_wdd, wdt);
-
 	ret = bcm_kona_wdt_set_timeout_reg(&bcm_kona_wdt_wdd, 0);
 	if (ret) {
 		dev_err(dev, "Failed set watchdog timeout");
-- 
1.7.9.5

Re: [RFC 0/2] Proposal for Watchdog Timer Resolution

[Guenter Roeck]

On 11/29/2013 01:20 PM, Markus Mayer wrote:
> I would like to propose the following extension to the watchdog
> framework to allow userland programs access to the resolution of the
> watchdog timer should the hardware support such a feature. The first
> driver to make use of this feature would be the BCM Kona watchdog
> driver.
>
> I would also like to propose introducing a sysfs interface to the
> watchdog framework in a later phase of updating the watchdog framework.
> The sysfs interface would exist in addition to the existing ioctl
> interface. In the future, the ioctl interface could be deprecated in
> favour of the sysfs interface or both interfaces could be kept
> indefinitely.
>
> Phase 1:
>
> Add get resolution/set resolution ioctls.

Hi Markus,

I am not entirely sure I understand the rationale for this one, or if it is really
valuable for the user.

One example where a resolution might be applicable is the w83627hf_wdt.c driver.
The Winbond/Nuvoton chips support watchdog timeouts either in seconds or in
minutes, based on some configuration register. Currently the driver supports
only seconds (1..255). I had thought about extending it to minutes, but my
approach would have been to automatically select minutes-based timeouts if the
requested timeout is too large (> 255 seconds).

The same approach should be possible for the bcm_kona driver as well; just select
the optimal resolution based on the requested timeout. I think this would make more
sense than manual configuration from a user perspective, to keep the API as simple
as possible while supporting a wide range of timeouts.

On a side note, I don't think you specify anywhere what "resolution" actually means;
it is just a number which, as far as I can see, does not seem to resemble a specific
time. In the case of the bcm_kona driver, it appears to be that values of 0..15 map
to 30us .. 1s in reverse order, which translates to a maximum timeout of anywhere
between 32s and about 12 days.

Based on this, you could set the maximum permitted timeout for the driver to 12 days
or 748800 seconds (or , and then calculate the required resolution from the actually
selected timeout.

Example:
	requested timeout: 24 x 3600 = 86400 seconds, ie 1 day
	Required resolution: 86400 s / 0xfffff = 82.4 ms ==> resolution = 3.

On the other side, I am not sure if all this complexity is really needed.
Even at maximum resolution of 1s you can always provide the remaining timeout
exactly down to the second to the user, which is the granularity provided
by the watchdog subsystem. So, if you want to be able to support 12-day
timeouts, why don't you just set the resolution to the maximum supported ?

> Add necessary flags and call-back functions.
> Extend bcm_kona_wdt.c to make use of the new functionality.
>
> Phase 2:
>
> Abstract the ioctl interface (into watchdog_ioctl.c) with a new set of
> per-driver ops, similar to what other frameworks are already doing.
>
> Phase 3:
>
> Introduce a sysfs interface (watchdog_sysfs.c) hooking into the already
> established per-driver ops. ioctl and sysfs calls would use the same
> framework and driver functions, requiring no code duplication.
>
> Please let me know if this sounds sensible or whether an alternate
> approach would be preferred.
>
I would like to see a sysfs interface, so I would like to see phase 3.
I'd have to see a patch set for phase 2; not really sure what this entails.

Thanks,
Guenter