[PATCH v2 0/2] watchdog: add get/set peretimeout UAPI and internals

[PATCH v2 0/2] watchdog: add get/set peretimeout UAPI and internals

[Vladimir Zapolskiy]

The change from Wolfram based on Robin's work adds two new watchdog
ioctls WDIOC_GETPRETIMEOUT and WDIOC_SETPRETIMEOUT, both interfaces
are already described in Documentation/watchdog/watchdog-api.txt

Along with UAPI update a new internal interface for watchdog drivers
set_pretimeout() is introduced, it allows to update the state of a controller.

The handling of a pretimeout event is a task delegated to watchdog drivers.

My contribution to the change is negligible, apart of review and testing:
* added visibility of a pretimeout sysfs attribute based on
  set WDIOF_PRETIMEOUT watchdog option,

Wolfram's patch "watchdog: softdog: implement pretimeout support"
potentially can be added to the series with a minor modification:

-	static struct watchdog_device softdog_dev;
 	.....
-	watchdog_notify_pretimeout(&softdog_dev);
+	panic("watchdog pretimeout event");

but I'm hesitating to do it.

Changes from v1 to v2:
* removed my wrong generalization of wdd->pretimeout update
* added seconds unit of time to describe internal kernel API

The change adds all necessary bits to proceed with a watchdog pretimeout
framework devlopment.

Wolfram Sang (2):
  watchdog: add pretimeout support to the core
  fs: compat_ioctl: add pretimeout functions for watchdogs

 Documentation/watchdog/watchdog-kernel-api.txt | 20 +++++++++
 drivers/watchdog/watchdog_dev.c                | 56 +++++++++++++++++++++++++-
 fs/compat_ioctl.c                              |  2 +
 include/linux/watchdog.h                       | 11 +++++
 4 files changed, 87 insertions(+), 2 deletions(-)

-- 
2.8.1

[PATCH v2 1/2] watchdog: add pretimeout support to the core

[Vladimir Zapolskiy]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Since the watchdog framework centrializes the IOCTL interfaces of device
drivers now, SETPRETIMEOUT and GETPRETIMEOUT need to be added in the
common code.

Signed-off-by: Robin Gong <b38343@freescale.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
[vzapolskiy: added conditional pretimeout sysfs attribute visibility]
Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
Changes from v1 to v2:
* removed my wrong generalization of wdd->pretimeout update
* added seconds unit of time to describe internal kernel API

 Documentation/watchdog/watchdog-kernel-api.txt | 20 +++++++++
 drivers/watchdog/watchdog_dev.c                | 56 +++++++++++++++++++++++++-
 include/linux/watchdog.h                       | 11 +++++
 3 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 917eeea..a8ef8fa 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -50,6 +50,7 @@ struct watchdog_device {
 	const struct watchdog_ops *ops;
 	unsigned int bootstatus;
 	unsigned int timeout;
+	unsigned int pretimeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
 	unsigned int min_hw_heartbeat_ms;
@@ -77,6 +78,7 @@ It contains following fields:
 * 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.
+* pretimeout: the watchdog timer's pretimeout value (in seconds).
 * min_timeout: the watchdog timer's minimum timeout value (in seconds).
   If set, the minimum configurable value for 'timeout'.
 * max_timeout: the watchdog timer's maximum timeout value (in seconds),
@@ -120,6 +122,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 *);
 	int (*restart)(struct watchdog_device *);
 	void (*ref)(struct watchdog_device *) __deprecated;
@@ -183,6 +186,23 @@ they are supported. These optional routines/operations are:
   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.
+  If the pretimeout feature is used (WDIOF_PRETIMEOUT), then set_timeout must
+  also take care of checking if pretimeout is still valid and set up the timer
+  accordingly. This can't be done in the core without races, so it is the
+  duty of the driver.
+* set_pretimeout: this routine checks and changes the pretimeout value of
+  the watchdog. It is optional because not all watchdogs support pretimeout
+  notification. The timeout value is not an absolute time, but the number of
+  seconds before the actual timeout would happen. It returns 0 on success,
+  -EINVAL for "parameter out of range" and -EIO for "could not write value to
+  the watchdog". A value of 0 disables pretimeout notification.
+  (Note: the WDIOF_PRETIMEOUT 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.pretimeout, this callback can be omitted. That means if
+  set_pretimeout is not provided but WDIOF_PRETIMEOUT is set, the watchdog
+  infrastructure updates the pretimeout 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 3595cff..5c24732 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -307,10 +307,14 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
 	if (watchdog_timeout_invalid(wdd, timeout))
 		return -EINVAL;
 
-	if (wdd->ops->set_timeout)
+	if (wdd->ops->set_timeout) {
 		err = wdd->ops->set_timeout(wdd, timeout);
-	else
+	} else {
 		wdd->timeout = timeout;
+		/* Disable pretimeout if it doesn't fit the new timeout */
+		if (wdd->pretimeout > wdd->timeout)
+			wdd->pretimeout = 0;
+	}
 
 	watchdog_update_worker(wdd);
 
@@ -318,6 +322,31 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
 }
 
 /*
+ *	watchdog_set_pretimeout: set the watchdog timer pretimeout
+ *	@wdd: the watchdog device to set the timeout for
+ *	@timeout: pretimeout to set in seconds
+ */
+
+static int watchdog_set_pretimeout(struct watchdog_device *wdd,
+				   unsigned int timeout)
+{
+	int err = 0;
+
+	if (!(wdd->info->options & WDIOF_PRETIMEOUT))
+		return -EOPNOTSUPP;
+
+	if (watchdog_pretimeout_invalid(wdd, timeout))
+		return -EINVAL;
+
+	if (wdd->ops->set_pretimeout)
+		err = wdd->ops->set_pretimeout(wdd, timeout);
+	else
+		wdd->pretimeout = timeout;
+
+	return err;
+}
+
+/*
  *	watchdog_get_timeleft: wrapper to get the time left before a reboot
  *	@wdd: the watchdog device to get the remaining time from
  *	@timeleft: the time that's left
@@ -401,6 +430,15 @@ static ssize_t timeout_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(timeout);
 
+static ssize_t pretimeout_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", wdd->pretimeout);
+}
+static DEVICE_ATTR_RO(pretimeout);
+
 static ssize_t identity_show(struct device *dev, struct device_attribute *attr,
 				char *buf)
 {
@@ -433,6 +471,9 @@ static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
 		mode = 0;
 	else if (attr == &dev_attr_timeleft.attr && !wdd->ops->get_timeleft)
 		mode = 0;
+	else if (attr == &dev_attr_pretimeout.attr &&
+		 !(wdd->info->options & WDIOF_PRETIMEOUT))
+		mode = 0;
 
 	return mode;
 }
@@ -440,6 +481,7 @@ static struct attribute *wdt_attrs[] = {
 	&dev_attr_state.attr,
 	&dev_attr_identity.attr,
 	&dev_attr_timeout.attr,
+	&dev_attr_pretimeout.attr,
 	&dev_attr_timeleft.attr,
 	&dev_attr_bootstatus.attr,
 	&dev_attr_status.attr,
@@ -620,6 +662,16 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 			break;
 		err = put_user(val, p);
 		break;
+	case WDIOC_SETPRETIMEOUT:
+		if (get_user(val, p)) {
+			err = -EFAULT;
+			break;
+		}
+		err = watchdog_set_pretimeout(wdd, val);
+		break;
+	case WDIOC_GETPRETIMEOUT:
+		err = put_user(wdd->pretimeout, p);
+		break;
 	default:
 		err = -ENOTTY;
 		break;
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 51732d6..bb8815a 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -28,6 +28,7 @@ struct watchdog_core_data;
  * @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 (in seconds).
+ * @set_pretimeout:The routine for setting the watchdog devices pretimeout (in seconds).
  * @get_timeleft:The routine that gets the time left before a reset (in seconds).
  * @restart:	The routine for restarting the machine.
  * @ioctl:	The routines that handles extra ioctl calls.
@@ -46,6 +47,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 *);
 	int (*restart)(struct watchdog_device *, unsigned long, void *);
 	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
@@ -61,6 +63,7 @@ struct watchdog_ops {
  * @ops:	Pointer to the list of watchdog operations.
  * @bootstatus:	Status of the watchdog device at boot.
  * @timeout:	The watchdog devices timeout value (in seconds).
+ * @pretimeout: The watchdog devices pre_timeout value (in seconds).
  * @min_timeout:The watchdog devices minimum timeout value (in seconds).
  * @max_timeout:The watchdog devices maximum timeout value (in seconds)
  *		as configurable from user space. Only relevant if
@@ -95,6 +98,7 @@ struct watchdog_device {
 	const struct watchdog_ops *ops;
 	unsigned int bootstatus;
 	unsigned int timeout;
+	unsigned int pretimeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
 	unsigned int min_hw_heartbeat_ms;
@@ -162,6 +166,13 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
 		 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 t && wdd->timeout && t >= wdd->timeout;
+}
+
 /* Use the following functions to manipulate watchdog driver specific data */
 static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data)
 {
-- 
2.8.1

[PATCH v2 2/2] fs: compat_ioctl: add pretimeout functions for watchdogs

[Vladimir Zapolskiy]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Watchdog core now handles those ioctls centrally, so we want 64 bit
support, too.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
Changes from v1 to v2:
* none

 fs/compat_ioctl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index bd01b92..914bfc5 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -1208,6 +1208,8 @@ COMPATIBLE_IOCTL(WDIOC_SETOPTIONS)
 COMPATIBLE_IOCTL(WDIOC_KEEPALIVE)
 COMPATIBLE_IOCTL(WDIOC_SETTIMEOUT)
 COMPATIBLE_IOCTL(WDIOC_GETTIMEOUT)
+COMPATIBLE_IOCTL(WDIOC_SETPRETIMEOUT)
+COMPATIBLE_IOCTL(WDIOC_GETPRETIMEOUT)
 /* Big R */
 COMPATIBLE_IOCTL(RNDGETENTCNT)
 COMPATIBLE_IOCTL(RNDADDTOENTCNT)
-- 
2.8.1

Re: [PATCH v2 1/2] watchdog: add pretimeout support to the core

[Guenter Roeck]

On 06/25/2016 06:01 PM, Vladimir Zapolskiy wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Since the watchdog framework centrializes the IOCTL interfaces of device
> drivers now, SETPRETIMEOUT and GETPRETIMEOUT need to be added in the
> common code.
>
> Signed-off-by: Robin Gong <b38343@freescale.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> [vzapolskiy: added conditional pretimeout sysfs attribute visibility]
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---

[ ... ]

> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -307,10 +307,14 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
>   	if (watchdog_timeout_invalid(wdd, timeout))
>   		return -EINVAL;
>
> -	if (wdd->ops->set_timeout)
> +	if (wdd->ops->set_timeout) {
>   		err = wdd->ops->set_timeout(wdd, timeout);
> -	else
> +	} else {
>   		wdd->timeout = timeout;
> +		/* Disable pretimeout if it doesn't fit the new timeout */
> +		if (wdd->pretimeout > wdd->timeout)

Nitpick: If we consider pretimeout == timeout to be invalid, we should reject it here
as well. So this should be >=.

Otherwise lgtm, so feel free to add

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

to the next version. I'd suggest to wait a bit to give Wolfram time to provide input,
though.

Thanks,
Guenter

Re: [PATCH v2 2/2] fs: compat_ioctl: add pretimeout functions for watchdogs

[Guenter Roeck]

On 06/25/2016 06:01 PM, Vladimir Zapolskiy wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Watchdog core now handles those ioctls centrally, so we want 64 bit
> support, too.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

> ---
> Changes from v1 to v2:
> * none
>
>   fs/compat_ioctl.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
> index bd01b92..914bfc5 100644
> --- a/fs/compat_ioctl.c
> +++ b/fs/compat_ioctl.c
> @@ -1208,6 +1208,8 @@ COMPATIBLE_IOCTL(WDIOC_SETOPTIONS)
>   COMPATIBLE_IOCTL(WDIOC_KEEPALIVE)
>   COMPATIBLE_IOCTL(WDIOC_SETTIMEOUT)
>   COMPATIBLE_IOCTL(WDIOC_GETTIMEOUT)
> +COMPATIBLE_IOCTL(WDIOC_SETPRETIMEOUT)
> +COMPATIBLE_IOCTL(WDIOC_GETPRETIMEOUT)
>   /* Big R */
>   COMPATIBLE_IOCTL(RNDGETENTCNT)
>   COMPATIBLE_IOCTL(RNDADDTOENTCNT)
>

Re: [PATCH v2 1/2] watchdog: add pretimeout support to the core

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 541 bytes --]


> Nitpick: If we consider pretimeout == timeout to be invalid, we should reject it here
> as well. So this should be >=.

I agree to that.

> to the next version. I'd suggest to wait a bit to give Wolfram time to provide input,
> though.

I liked the is_visible() change right away, so these patches are fine.
Guenter, what is your take on upstreaming the softdog pretimeout
support? I'd think it would be good to have for testing purposes. It
would also help me to upstream the busybox patch for the watchdog
applet.

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/2] watchdog: add pretimeout support to the core

[Guenter Roeck]

On 06/27/2016 02:37 AM, Wolfram Sang wrote:
>
>> Nitpick: If we consider pretimeout == timeout to be invalid, we should reject it here
>> as well. So this should be >=.
>
> I agree to that.
>
>> to the next version. I'd suggest to wait a bit to give Wolfram time to provide input,
>> though.
>
> I liked the is_visible() change right away, so these patches are fine.
> Guenter, what is your take on upstreaming the softdog pretimeout
> support? I'd think it would be good to have for testing purposes. It
> would also help me to upstream the busybox patch for the watchdog
> applet.
>

Sure, but the current patch series doesn't include watchdog_notify_pretimeout(),
or am I missing something ?

Guenter

Re: [PATCH v2 1/2] watchdog: add pretimeout support to the core

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 492 bytes --]


> >I liked the is_visible() change right away, so these patches are fine.
> >Guenter, what is your take on upstreaming the softdog pretimeout
> >support? I'd think it would be good to have for testing purposes. It
> >would also help me to upstream the busybox patch for the watchdog
> >applet.
> >
> 
> Sure, but the current patch series doesn't include watchdog_notify_pretimeout(),
> or am I missing something ?

It would be hardcoded to "panic" for now and later converted.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/2] watchdog: add pretimeout support to the core

[Guenter Roeck]

On Mon, Jun 27, 2016 at 05:33:56PM +0200, Wolfram Sang wrote:
> 
> > >I liked the is_visible() change right away, so these patches are fine.
> > >Guenter, what is your take on upstreaming the softdog pretimeout
> > >support? I'd think it would be good to have for testing purposes. It
> > >would also help me to upstream the busybox patch for the watchdog
> > >applet.
> > >
> > 
> > Sure, but the current patch series doesn't include watchdog_notify_pretimeout(),
> > or am I missing something ?
> 
> It would be hardcoded to "panic" for now and later converted.
> 

sgtm

Guenter