[PATCH 0/6] watchdog: Add millisecond-level capabilities

[PATCH 0/6] watchdog: Add millisecond-level capabilities

[minyard]

As mentioned before in the list, this is work for the ELISA (Linux in
safety critical systems) group.  Some use cases require tight watchdog
times so a system can recover quickly on a failure.

This patch series adds millisecond level capabilites that devices can
use if they wish, new ioctls to use those interfaces, new device
attributes with the higher precision, documentation, and it converts the
i6300 and softdog watchdog to use milliseconds.

All existing user APIs should work as before.  The interface between the
watchdog subsystem and the driver is converted to be modal.  If a flag
is set in the options then all interactions are in milliseconds.  If the
flag is not set then they are in seconds.

It's all pretty straightforward except for the calculations in the
i6300 driver, which required some thought.

Note that checkpatch gives warnings about there being no identifiers in
the function arguments in the watchdog ops structure, but I tried to
keep the style the same.

Thanks,

-corey

[PATCH 1/6] watchdog: Allow a driver to use milliseconds instead of seconds

[minyard]

From: Corey Minyard <cminyard@mvista.com>

If the WDIOF_MSECTIMER is set, then all the timeouts in the watchdog
structure are expected to be in milliseconds.  Add the flag and the
various conversions.  This should have no effect on existing drivers.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/watchdog/watchdog_core.c | 30 +++++++++++++-------
 drivers/watchdog/watchdog_dev.c  | 47 ++++++++++++++++++++++++++------
 include/linux/watchdog.h         | 29 +++++++++++++++-----
 include/uapi/linux/watchdog.h    |  1 +
 4 files changed, 82 insertions(+), 25 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 423844757812..b54451a9a336 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -116,17 +116,17 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 {
 	const char *dev_str = wdd->parent ? dev_name(wdd->parent) :
 			      (const char *)wdd->info->identity;
-	unsigned int t = 0;
 	int ret = 0;
 
 	watchdog_check_min_max_timeout(wdd);
 
 	/* check the driver supplied value (likely a module parameter) first */
 	if (timeout_parm) {
-		if (!watchdog_timeout_invalid(wdd, timeout_parm)) {
-			wdd->timeout = timeout_parm;
-			return 0;
-		}
+		if (wdd->info->options & WDIOF_MSECTIMER) {
+			if (!_watchdog_timeout_invalid(wdd, timeout_parm))
+				goto set_timeout;
+		} else if (!watchdog_timeout_invalid(wdd, timeout_parm))
+			goto set_timeout;
 		pr_err("%s: driver supplied timeout (%u) out of range\n",
 			dev_str, timeout_parm);
 		ret = -EINVAL;
@@ -134,12 +134,18 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 
 	/* try to get the timeout_sec property */
 	if (dev && dev->of_node &&
-	    of_property_read_u32(dev->of_node, "timeout-sec", &t) == 0) {
-		if (t && !watchdog_timeout_invalid(wdd, t)) {
-			wdd->timeout = t;
-			return 0;
+	    of_property_read_u32(dev->of_node, "timeout-sec",
+				 &timeout_parm) == 0) {
+		if (timeout_parm &&
+		    !watchdog_timeout_invalid(wdd, timeout_parm)) {
+			if (!(wdd->info->options & WDIOF_MSECTIMER))
+				/* Convert to msecs if not already so. */
+				timeout_parm *= 1000;
+			goto set_timeout;
 		}
-		pr_err("%s: DT supplied timeout (%u) out of range\n", dev_str, t);
+
+		pr_err("%s: DT supplied timeout (%u) out of range\n", dev_str,
+		       timeout_parm);
 		ret = -EINVAL;
 	}
 
@@ -148,6 +154,10 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 			wdd->timeout);
 
 	return ret;
+
+set_timeout:
+	wdd->timeout = timeout_parm;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(watchdog_init_timeout);
 
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 7e4cd34a8c20..480460b89c16 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -99,7 +99,11 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 {
 	/* All variables in milli-seconds */
 	unsigned int hm = wdd->max_hw_heartbeat_ms;
-	unsigned int t = wdd->timeout * 1000;
+	unsigned int t = wdd->timeout;
+
+	if (!(wdd->info->options & WDIOF_MSECTIMER))
+		/* Convert to msecs if not already so. */
+		t *= 1000;
 
 	/*
 	 * A worker to generate heartbeat requests is needed if all of the
@@ -121,12 +125,16 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
 {
 	struct watchdog_core_data *wd_data = wdd->wd_data;
-	unsigned int timeout_ms = wdd->timeout * 1000;
+	unsigned int timeout_ms = wdd->timeout;
 	ktime_t keepalive_interval;
 	ktime_t last_heartbeat, latest_heartbeat;
 	ktime_t virt_timeout;
 	unsigned int hw_heartbeat_ms;
 
+	if (!(wdd->info->options & WDIOF_MSECTIMER))
+		/* Convert to msecs if not already so. */
+		timeout_ms *= 1000;
+
 	if (watchdog_active(wdd))
 		virt_timeout = ktime_add(wd_data->last_keepalive,
 					 ms_to_ktime(timeout_ms));
@@ -137,7 +145,7 @@ static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
 	keepalive_interval = ms_to_ktime(hw_heartbeat_ms / 2);
 
 	/*
-	 * To ensure that the watchdog times out wdd->timeout seconds
+	 * To ensure that the watchdog times out wdd->timeout seconds/msecs
 	 * after the most recent ping from userspace, the last
 	 * worker ping has to come in hw_heartbeat_ms before this timeout.
 	 */
@@ -382,6 +390,8 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
 	if (watchdog_timeout_invalid(wdd, timeout))
 		return -EINVAL;
 
+	if (wdd->info->options & WDIOF_MSECTIMER)
+		timeout *= 1000;
 	if (wdd->ops->set_timeout) {
 		err = wdd->ops->set_timeout(wdd, timeout);
 	} else {
@@ -413,6 +423,8 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
 	if (watchdog_pretimeout_invalid(wdd, timeout))
 		return -EINVAL;
 
+	if (wdd->info->options & WDIOF_MSECTIMER)
+		timeout *= 1000;
 	if (wdd->ops->set_pretimeout)
 		err = wdd->ops->set_pretimeout(wdd, timeout);
 	else
@@ -440,6 +452,8 @@ static int watchdog_get_timeleft(struct watchdog_device *wdd,
 		return -EOPNOTSUPP;
 
 	*timeleft = wdd->ops->get_timeleft(wdd);
+	if (wdd->info->options & WDIOF_MSECTIMER)
+		*timeleft /= 1000;
 
 	return 0;
 }
@@ -508,8 +522,11 @@ static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
 	mutex_lock(&wd_data->lock);
 	status = watchdog_get_timeleft(wdd, &val);
 	mutex_unlock(&wd_data->lock);
-	if (!status)
+	if (!status) {
+		if (wdd->info->options & WDIOF_MSECTIMER)
+			val /= 1000;
 		status = sprintf(buf, "%u\n", val);
+	}
 
 	return status;
 }
@@ -519,8 +536,12 @@ static ssize_t timeout_show(struct device *dev, struct device_attribute *attr,
 				char *buf)
 {
 	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	unsigned int t = wdd->timeout;
+
+	if (wdd->info->options & WDIOF_MSECTIMER)
+		t /= 1000;
 
-	return sprintf(buf, "%u\n", wdd->timeout);
+	return sprintf(buf, "%u\n", t);
 }
 static DEVICE_ATTR_RO(timeout);
 
@@ -528,8 +549,12 @@ static ssize_t pretimeout_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
 	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	unsigned int t = wdd->pretimeout;
 
-	return sprintf(buf, "%u\n", wdd->pretimeout);
+	if (wdd->info->options & WDIOF_MSECTIMER)
+		t /= 1000;
+
+	return sprintf(buf, "%u\n", t);
 }
 static DEVICE_ATTR_RO(pretimeout);
 
@@ -783,7 +808,10 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 			err = -EOPNOTSUPP;
 			break;
 		}
-		err = put_user(wdd->timeout, p);
+		val = wdd->timeout;
+		if (wdd->info->options & WDIOF_MSECTIMER)
+			val /= 1000;
+		err = put_user(val, p);
 		break;
 	case WDIOC_GETTIMELEFT:
 		err = watchdog_get_timeleft(wdd, &val);
@@ -799,7 +827,10 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 		err = watchdog_set_pretimeout(wdd, val);
 		break;
 	case WDIOC_GETPRETIMEOUT:
-		err = put_user(wdd->pretimeout, p);
+		val = wdd->pretimeout;
+		if (wdd->info->options & WDIOF_MSECTIMER)
+			val /= 1000;
+		err = put_user(val, p);
 		break;
 	default:
 		err = -ENOTTY;
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 1464ce6ffa31..49bfaf986b37 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -55,7 +55,9 @@ struct watchdog_ops {
 	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
 };
 
-/** struct watchdog_device - The structure that defines a watchdog device
+/** struct watchdog_device - The structure that defines a watchdog device.
+ * Unless otherwise specified, all timeouts are in seconds unless
+ * WDIOF_MSECTIMER is set, then they are in milliseconds.
  *
  * @id:		The watchdog's ID. (Allocated by watchdog_register_device)
  * @parent:	The parent bus device
@@ -65,10 +67,10 @@ struct watchdog_ops {
  * @ops:	Pointer to the list of watchdog operations.
  * @gov:	Pointer to watchdog pretimeout governor.
  * @bootstatus:	Status of the watchdog device at boot.
- * @timeout:	The watchdog devices timeout value (in seconds).
+ * @timeout:	The watchdog devices timeout value.
  * @pretimeout: The watchdog devices pre_timeout value.
- * @min_timeout:The watchdog devices minimum timeout value (in seconds).
- * @max_timeout:The watchdog devices maximum timeout value (in seconds)
+ * @min_timeout:The watchdog devices minimum timeout value.
+ * @max_timeout:The watchdog devices maximum timeout value
  *		as configurable from user space. Only relevant if
  *		max_hw_heartbeat_ms is not provided.
  * @min_hw_heartbeat_ms:
@@ -156,6 +158,17 @@ static inline void watchdog_stop_on_unregister(struct watchdog_device *wdd)
 	set_bit(WDOG_STOP_ON_UNREGISTER, &wdd->status);
 }
 
+/*
+ * Use the following function to check if a timeout value is
+ * internally consistent with the range parameters.  t is in milliseconds.
+ */
+static inline bool _watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
+{
+	return  t < wdd->min_timeout ||
+		(!wdd->max_hw_heartbeat_ms && wdd->max_timeout &&
+		 t > wdd->max_timeout);
+}
+
 /* Use the following function to check if a timeout value is invalid */
 static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
 {
@@ -170,9 +183,11 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
 	 *   is configured, and the requested value is larger than the
 	 *   configured maximum timeout.
 	 */
-	return t > UINT_MAX / 1000 || t < wdd->min_timeout ||
-		(!wdd->max_hw_heartbeat_ms && wdd->max_timeout &&
-		 t > wdd->max_timeout);
+	if (t > UINT_MAX / 1000)
+		return true;
+	if (wdd->info->options & WDIOF_MSECTIMER)
+		t *= 1000;
+	return _watchdog_timeout_invalid(wdd, t);
 }
 
 /* Use the following function to check if a pretimeout value is invalid */
diff --git a/include/uapi/linux/watchdog.h b/include/uapi/linux/watchdog.h
index b15cde5c9054..feb3bcc46993 100644
--- a/include/uapi/linux/watchdog.h
+++ b/include/uapi/linux/watchdog.h
@@ -48,6 +48,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_MSECTIMER		0x0800	/* Driver can use milliseconds for timeouts */
 #define	WDIOF_KEEPALIVEPING	0x8000	/* Keep alive ping reply */
 
 #define	WDIOS_DISABLECARD	0x0001	/* Turn off the watchdog timer */
-- 
2.17.1

Re: [PATCH 1/6] watchdog: Allow a driver to use milliseconds instead of seconds

[Guenter Roeck]

On 6/20/20 10:33 AM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> If the WDIOF_MSECTIMER is set, then all the timeouts in the watchdog
> structure are expected to be in milliseconds.  Add the flag and the
> various conversions.  This should have no effect on existing drivers.
> 

For this to work, the entire watchdog core code has to be converted to be based
on milliseconds, with API functions converting from s to ms on incoming calls
and from ms to s on outgoing calls. At the same time, the existing UAPI must not
be changed and still be based on seconds. Milli-second functionality such as
milli-second based sysfs attributes or milli-second based ioctls can be added
separately.

That means functions such as watchdog_need_worker() must be completely based
on milli-seconds and not make an on-the-fly conversion on each call.
That is just one example.

I'd give it a try myself, but unfortunately I just don't have the time.

Guenter

> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/watchdog/watchdog_core.c | 30 +++++++++++++-------
>  drivers/watchdog/watchdog_dev.c  | 47 ++++++++++++++++++++++++++------
>  include/linux/watchdog.h         | 29 +++++++++++++++-----
>  include/uapi/linux/watchdog.h    |  1 +
>  4 files changed, 82 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 423844757812..b54451a9a336 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -116,17 +116,17 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>  {
>  	const char *dev_str = wdd->parent ? dev_name(wdd->parent) :
>  			      (const char *)wdd->info->identity;
> -	unsigned int t = 0;
>  	int ret = 0;
>  
>  	watchdog_check_min_max_timeout(wdd);
>  
>  	/* check the driver supplied value (likely a module parameter) first */
>  	if (timeout_parm) {
> -		if (!watchdog_timeout_invalid(wdd, timeout_parm)) {
> -			wdd->timeout = timeout_parm;
> -			return 0;
> -		}
> +		if (wdd->info->options & WDIOF_MSECTIMER) {
> +			if (!_watchdog_timeout_invalid(wdd, timeout_parm))
> +				goto set_timeout;
> +		} else if (!watchdog_timeout_invalid(wdd, timeout_parm))
> +			goto set_timeout;
>  		pr_err("%s: driver supplied timeout (%u) out of range\n",
>  			dev_str, timeout_parm);
>  		ret = -EINVAL;
> @@ -134,12 +134,18 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>  
>  	/* try to get the timeout_sec property */
>  	if (dev && dev->of_node &&
> -	    of_property_read_u32(dev->of_node, "timeout-sec", &t) == 0) {
> -		if (t && !watchdog_timeout_invalid(wdd, t)) {
> -			wdd->timeout = t;
> -			return 0;
> +	    of_property_read_u32(dev->of_node, "timeout-sec",
> +				 &timeout_parm) == 0) {
> +		if (timeout_parm &&
> +		    !watchdog_timeout_invalid(wdd, timeout_parm)) {
> +			if (!(wdd->info->options & WDIOF_MSECTIMER))
> +				/* Convert to msecs if not already so. */
> +				timeout_parm *= 1000;
> +			goto set_timeout;
>  		}
> -		pr_err("%s: DT supplied timeout (%u) out of range\n", dev_str, t);
> +
> +		pr_err("%s: DT supplied timeout (%u) out of range\n", dev_str,
> +		       timeout_parm);
>  		ret = -EINVAL;
>  	}
>  
> @@ -148,6 +154,10 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>  			wdd->timeout);
>  
>  	return ret;
> +
> +set_timeout:
> +	wdd->timeout = timeout_parm;
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>  
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 7e4cd34a8c20..480460b89c16 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -99,7 +99,11 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>  {
>  	/* All variables in milli-seconds */
>  	unsigned int hm = wdd->max_hw_heartbeat_ms;
> -	unsigned int t = wdd->timeout * 1000;
> +	unsigned int t = wdd->timeout;
> +
> +	if (!(wdd->info->options & WDIOF_MSECTIMER))
> +		/* Convert to msecs if not already so. */
> +		t *= 1000;
>  
>  	/*
>  	 * A worker to generate heartbeat requests is needed if all of the
> @@ -121,12 +125,16 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>  static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
>  {
>  	struct watchdog_core_data *wd_data = wdd->wd_data;
> -	unsigned int timeout_ms = wdd->timeout * 1000;
> +	unsigned int timeout_ms = wdd->timeout;
>  	ktime_t keepalive_interval;
>  	ktime_t last_heartbeat, latest_heartbeat;
>  	ktime_t virt_timeout;
>  	unsigned int hw_heartbeat_ms;
>  
> +	if (!(wdd->info->options & WDIOF_MSECTIMER))
> +		/* Convert to msecs if not already so. */
> +		timeout_ms *= 1000;
> +
>  	if (watchdog_active(wdd))
>  		virt_timeout = ktime_add(wd_data->last_keepalive,
>  					 ms_to_ktime(timeout_ms));
> @@ -137,7 +145,7 @@ static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
>  	keepalive_interval = ms_to_ktime(hw_heartbeat_ms / 2);
>  
>  	/*
> -	 * To ensure that the watchdog times out wdd->timeout seconds
> +	 * To ensure that the watchdog times out wdd->timeout seconds/msecs
>  	 * after the most recent ping from userspace, the last
>  	 * worker ping has to come in hw_heartbeat_ms before this timeout.
>  	 */
> @@ -382,6 +390,8 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
>  	if (watchdog_timeout_invalid(wdd, timeout))
>  		return -EINVAL;
>  
> +	if (wdd->info->options & WDIOF_MSECTIMER)
> +		timeout *= 1000;
>  	if (wdd->ops->set_timeout) {
>  		err = wdd->ops->set_timeout(wdd, timeout);
>  	} else {
> @@ -413,6 +423,8 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
>  	if (watchdog_pretimeout_invalid(wdd, timeout))
>  		return -EINVAL;
>  
> +	if (wdd->info->options & WDIOF_MSECTIMER)
> +		timeout *= 1000;
>  	if (wdd->ops->set_pretimeout)
>  		err = wdd->ops->set_pretimeout(wdd, timeout);
>  	else
> @@ -440,6 +452,8 @@ static int watchdog_get_timeleft(struct watchdog_device *wdd,
>  		return -EOPNOTSUPP;
>  
>  	*timeleft = wdd->ops->get_timeleft(wdd);
> +	if (wdd->info->options & WDIOF_MSECTIMER)
> +		*timeleft /= 1000;
>  
>  	return 0;
>  }
> @@ -508,8 +522,11 @@ static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
>  	mutex_lock(&wd_data->lock);
>  	status = watchdog_get_timeleft(wdd, &val);
>  	mutex_unlock(&wd_data->lock);
> -	if (!status)
> +	if (!status) {
> +		if (wdd->info->options & WDIOF_MSECTIMER)
> +			val /= 1000;
>  		status = sprintf(buf, "%u\n", val);
> +	}
>  
>  	return status;
>  }
> @@ -519,8 +536,12 @@ static ssize_t timeout_show(struct device *dev, struct device_attribute *attr,
>  				char *buf)
>  {
>  	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +	unsigned int t = wdd->timeout;
> +
> +	if (wdd->info->options & WDIOF_MSECTIMER)
> +		t /= 1000;
>  
> -	return sprintf(buf, "%u\n", wdd->timeout);
> +	return sprintf(buf, "%u\n", t);
>  }
>  static DEVICE_ATTR_RO(timeout);
>  
> @@ -528,8 +549,12 @@ static ssize_t pretimeout_show(struct device *dev,
>  			       struct device_attribute *attr, char *buf)
>  {
>  	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +	unsigned int t = wdd->pretimeout;
>  
> -	return sprintf(buf, "%u\n", wdd->pretimeout);
> +	if (wdd->info->options & WDIOF_MSECTIMER)
> +		t /= 1000;
> +
> +	return sprintf(buf, "%u\n", t);
>  }
>  static DEVICE_ATTR_RO(pretimeout);
>  
> @@ -783,7 +808,10 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>  			err = -EOPNOTSUPP;
>  			break;
>  		}
> -		err = put_user(wdd->timeout, p);
> +		val = wdd->timeout;
> +		if (wdd->info->options & WDIOF_MSECTIMER)
> +			val /= 1000;
> +		err = put_user(val, p);
>  		break;
>  	case WDIOC_GETTIMELEFT:
>  		err = watchdog_get_timeleft(wdd, &val);
> @@ -799,7 +827,10 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>  		err = watchdog_set_pretimeout(wdd, val);
>  		break;
>  	case WDIOC_GETPRETIMEOUT:
> -		err = put_user(wdd->pretimeout, p);
> +		val = wdd->pretimeout;
> +		if (wdd->info->options & WDIOF_MSECTIMER)
> +			val /= 1000;
> +		err = put_user(val, p);
>  		break;
>  	default:
>  		err = -ENOTTY;
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 1464ce6ffa31..49bfaf986b37 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -55,7 +55,9 @@ struct watchdog_ops {
>  	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
>  };
>  
> -/** struct watchdog_device - The structure that defines a watchdog device
> +/** struct watchdog_device - The structure that defines a watchdog device.
> + * Unless otherwise specified, all timeouts are in seconds unless
> + * WDIOF_MSECTIMER is set, then they are in milliseconds.
>   *
>   * @id:		The watchdog's ID. (Allocated by watchdog_register_device)
>   * @parent:	The parent bus device
> @@ -65,10 +67,10 @@ struct watchdog_ops {
>   * @ops:	Pointer to the list of watchdog operations.
>   * @gov:	Pointer to watchdog pretimeout governor.
>   * @bootstatus:	Status of the watchdog device at boot.
> - * @timeout:	The watchdog devices timeout value (in seconds).
> + * @timeout:	The watchdog devices timeout value.
>   * @pretimeout: The watchdog devices pre_timeout value.
> - * @min_timeout:The watchdog devices minimum timeout value (in seconds).
> - * @max_timeout:The watchdog devices maximum timeout value (in seconds)
> + * @min_timeout:The watchdog devices minimum timeout value.
> + * @max_timeout:The watchdog devices maximum timeout value
>   *		as configurable from user space. Only relevant if
>   *		max_hw_heartbeat_ms is not provided.
>   * @min_hw_heartbeat_ms:
> @@ -156,6 +158,17 @@ static inline void watchdog_stop_on_unregister(struct watchdog_device *wdd)
>  	set_bit(WDOG_STOP_ON_UNREGISTER, &wdd->status);
>  }
>  
> +/*
> + * Use the following function to check if a timeout value is
> + * internally consistent with the range parameters.  t is in milliseconds.
> + */
> +static inline bool _watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
> +{
> +	return  t < wdd->min_timeout ||
> +		(!wdd->max_hw_heartbeat_ms && wdd->max_timeout &&
> +		 t > wdd->max_timeout);
> +}
> +
>  /* Use the following function to check if a timeout value is invalid */
>  static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
>  {
> @@ -170,9 +183,11 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
>  	 *   is configured, and the requested value is larger than the
>  	 *   configured maximum timeout.
>  	 */
> -	return t > UINT_MAX / 1000 || t < wdd->min_timeout ||
> -		(!wdd->max_hw_heartbeat_ms && wdd->max_timeout &&
> -		 t > wdd->max_timeout);
> +	if (t > UINT_MAX / 1000)
> +		return true;
> +	if (wdd->info->options & WDIOF_MSECTIMER)
> +		t *= 1000;
> +	return _watchdog_timeout_invalid(wdd, t);
>  }
>  
>  /* Use the following function to check if a pretimeout value is invalid */
> diff --git a/include/uapi/linux/watchdog.h b/include/uapi/linux/watchdog.h
> index b15cde5c9054..feb3bcc46993 100644
> --- a/include/uapi/linux/watchdog.h
> +++ b/include/uapi/linux/watchdog.h
> @@ -48,6 +48,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_MSECTIMER		0x0800	/* Driver can use milliseconds for timeouts */
>  #define	WDIOF_KEEPALIVEPING	0x8000	/* Keep alive ping reply */
>  
>  #define	WDIOS_DISABLECARD	0x0001	/* Turn off the watchdog timer */
> 

Re: [PATCH 1/6] watchdog: Allow a driver to use milliseconds instead of seconds

[Corey Minyard]

On Fri, Jun 26, 2020 at 04:10:41PM -0700, Guenter Roeck wrote:
> On 6/20/20 10:33 AM, minyard@acm.org wrote:
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > If the WDIOF_MSECTIMER is set, then all the timeouts in the watchdog
> > structure are expected to be in milliseconds.  Add the flag and the
> > various conversions.  This should have no effect on existing drivers.
> > 
> 
> For this to work, the entire watchdog core code has to be converted to be based
> on milliseconds, with API functions converting from s to ms on incoming calls
> and from ms to s on outgoing calls. At the same time, the existing UAPI must not
> be changed and still be based on seconds. Milli-second functionality such as
> milli-second based sysfs attributes or milli-second based ioctls can be added
> separately.
> 
> That means functions such as watchdog_need_worker() must be completely based
> on milli-seconds and not make an on-the-fly conversion on each call.
> That is just one example.
> 
> I'd give it a try myself, but unfortunately I just don't have the time.

The patches in this series should do all this.  For instance:

@@ -99,7 +99,11 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 {
        /* All variables in milli-seconds */
        unsigned int hm = wdd->max_hw_heartbeat_ms;
-       unsigned int t = wdd->timeout * 1000;
+       unsigned int t = wdd->timeout;
+
+       if (!(wdd->info->options & WDIOF_MSECTIMER))
+               /* Convert to msecs if not already so. */
+               t *= 1000;

        /*
         * A worker to generate heartbeat requests is needed if all of the

Basically, the changes keep the timeout in milliseconds if the lower
level driver uses milliseconds, and seconds if the lower level driver
uses seconds.  The watchdog cores do the conversions as necessary.
All the UAPIs do conversion, they work unchanged.

That's really the only way you can do this without changing all the low
level drivers, since they often reference the timeout field.

-corey

> 
> Guenter
> 
> > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > ---
> >  drivers/watchdog/watchdog_core.c | 30 +++++++++++++-------
> >  drivers/watchdog/watchdog_dev.c  | 47 ++++++++++++++++++++++++++------
> >  include/linux/watchdog.h         | 29 +++++++++++++++-----
> >  include/uapi/linux/watchdog.h    |  1 +
> >  4 files changed, 82 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> > index 423844757812..b54451a9a336 100644
> > --- a/drivers/watchdog/watchdog_core.c
> > +++ b/drivers/watchdog/watchdog_core.c
> > @@ -116,17 +116,17 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
> >  {
> >  	const char *dev_str = wdd->parent ? dev_name(wdd->parent) :
> >  			      (const char *)wdd->info->identity;
> > -	unsigned int t = 0;
> >  	int ret = 0;
> >  
> >  	watchdog_check_min_max_timeout(wdd);
> >  
> >  	/* check the driver supplied value (likely a module parameter) first */
> >  	if (timeout_parm) {
> > -		if (!watchdog_timeout_invalid(wdd, timeout_parm)) {
> > -			wdd->timeout = timeout_parm;
> > -			return 0;
> > -		}
> > +		if (wdd->info->options & WDIOF_MSECTIMER) {
> > +			if (!_watchdog_timeout_invalid(wdd, timeout_parm))
> > +				goto set_timeout;
> > +		} else if (!watchdog_timeout_invalid(wdd, timeout_parm))
> > +			goto set_timeout;
> >  		pr_err("%s: driver supplied timeout (%u) out of range\n",
> >  			dev_str, timeout_parm);
> >  		ret = -EINVAL;
> > @@ -134,12 +134,18 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
> >  
> >  	/* try to get the timeout_sec property */
> >  	if (dev && dev->of_node &&
> > -	    of_property_read_u32(dev->of_node, "timeout-sec", &t) == 0) {
> > -		if (t && !watchdog_timeout_invalid(wdd, t)) {
> > -			wdd->timeout = t;
> > -			return 0;
> > +	    of_property_read_u32(dev->of_node, "timeout-sec",
> > +				 &timeout_parm) == 0) {
> > +		if (timeout_parm &&
> > +		    !watchdog_timeout_invalid(wdd, timeout_parm)) {
> > +			if (!(wdd->info->options & WDIOF_MSECTIMER))
> > +				/* Convert to msecs if not already so. */
> > +				timeout_parm *= 1000;
> > +			goto set_timeout;
> >  		}
> > -		pr_err("%s: DT supplied timeout (%u) out of range\n", dev_str, t);
> > +
> > +		pr_err("%s: DT supplied timeout (%u) out of range\n", dev_str,
> > +		       timeout_parm);
> >  		ret = -EINVAL;
> >  	}
> >  
> > @@ -148,6 +154,10 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
> >  			wdd->timeout);
> >  
> >  	return ret;
> > +
> > +set_timeout:
> > +	wdd->timeout = timeout_parm;
> > +	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(watchdog_init_timeout);
> >  
> > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> > index 7e4cd34a8c20..480460b89c16 100644
> > --- a/drivers/watchdog/watchdog_dev.c
> > +++ b/drivers/watchdog/watchdog_dev.c
> > @@ -99,7 +99,11 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
> >  {
> >  	/* All variables in milli-seconds */
> >  	unsigned int hm = wdd->max_hw_heartbeat_ms;
> > -	unsigned int t = wdd->timeout * 1000;
> > +	unsigned int t = wdd->timeout;
> > +
> > +	if (!(wdd->info->options & WDIOF_MSECTIMER))
> > +		/* Convert to msecs if not already so. */
> > +		t *= 1000;
> >  
> >  	/*
> >  	 * A worker to generate heartbeat requests is needed if all of the
> > @@ -121,12 +125,16 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
> >  static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
> >  {
> >  	struct watchdog_core_data *wd_data = wdd->wd_data;
> > -	unsigned int timeout_ms = wdd->timeout * 1000;
> > +	unsigned int timeout_ms = wdd->timeout;
> >  	ktime_t keepalive_interval;
> >  	ktime_t last_heartbeat, latest_heartbeat;
> >  	ktime_t virt_timeout;
> >  	unsigned int hw_heartbeat_ms;
> >  
> > +	if (!(wdd->info->options & WDIOF_MSECTIMER))
> > +		/* Convert to msecs if not already so. */
> > +		timeout_ms *= 1000;
> > +
> >  	if (watchdog_active(wdd))
> >  		virt_timeout = ktime_add(wd_data->last_keepalive,
> >  					 ms_to_ktime(timeout_ms));
> > @@ -137,7 +145,7 @@ static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
> >  	keepalive_interval = ms_to_ktime(hw_heartbeat_ms / 2);
> >  
> >  	/*
> > -	 * To ensure that the watchdog times out wdd->timeout seconds
> > +	 * To ensure that the watchdog times out wdd->timeout seconds/msecs
> >  	 * after the most recent ping from userspace, the last
> >  	 * worker ping has to come in hw_heartbeat_ms before this timeout.
> >  	 */
> > @@ -382,6 +390,8 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
> >  	if (watchdog_timeout_invalid(wdd, timeout))
> >  		return -EINVAL;
> >  
> > +	if (wdd->info->options & WDIOF_MSECTIMER)
> > +		timeout *= 1000;
> >  	if (wdd->ops->set_timeout) {
> >  		err = wdd->ops->set_timeout(wdd, timeout);
> >  	} else {
> > @@ -413,6 +423,8 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
> >  	if (watchdog_pretimeout_invalid(wdd, timeout))
> >  		return -EINVAL;
> >  
> > +	if (wdd->info->options & WDIOF_MSECTIMER)
> > +		timeout *= 1000;
> >  	if (wdd->ops->set_pretimeout)
> >  		err = wdd->ops->set_pretimeout(wdd, timeout);
> >  	else
> > @@ -440,6 +452,8 @@ static int watchdog_get_timeleft(struct watchdog_device *wdd,
> >  		return -EOPNOTSUPP;
> >  
> >  	*timeleft = wdd->ops->get_timeleft(wdd);
> > +	if (wdd->info->options & WDIOF_MSECTIMER)
> > +		*timeleft /= 1000;
> >  
> >  	return 0;
> >  }
> > @@ -508,8 +522,11 @@ static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
> >  	mutex_lock(&wd_data->lock);
> >  	status = watchdog_get_timeleft(wdd, &val);
> >  	mutex_unlock(&wd_data->lock);
> > -	if (!status)
> > +	if (!status) {
> > +		if (wdd->info->options & WDIOF_MSECTIMER)
> > +			val /= 1000;
> >  		status = sprintf(buf, "%u\n", val);
> > +	}
> >  
> >  	return status;
> >  }
> > @@ -519,8 +536,12 @@ static ssize_t timeout_show(struct device *dev, struct device_attribute *attr,
> >  				char *buf)
> >  {
> >  	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > +	unsigned int t = wdd->timeout;
> > +
> > +	if (wdd->info->options & WDIOF_MSECTIMER)
> > +		t /= 1000;
> >  
> > -	return sprintf(buf, "%u\n", wdd->timeout);
> > +	return sprintf(buf, "%u\n", t);
> >  }
> >  static DEVICE_ATTR_RO(timeout);
> >  
> > @@ -528,8 +549,12 @@ static ssize_t pretimeout_show(struct device *dev,
> >  			       struct device_attribute *attr, char *buf)
> >  {
> >  	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > +	unsigned int t = wdd->pretimeout;
> >  
> > -	return sprintf(buf, "%u\n", wdd->pretimeout);
> > +	if (wdd->info->options & WDIOF_MSECTIMER)
> > +		t /= 1000;
> > +
> > +	return sprintf(buf, "%u\n", t);
> >  }
> >  static DEVICE_ATTR_RO(pretimeout);
> >  
> > @@ -783,7 +808,10 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
> >  			err = -EOPNOTSUPP;
> >  			break;
> >  		}
> > -		err = put_user(wdd->timeout, p);
> > +		val = wdd->timeout;
> > +		if (wdd->info->options & WDIOF_MSECTIMER)
> > +			val /= 1000;
> > +		err = put_user(val, p);
> >  		break;
> >  	case WDIOC_GETTIMELEFT:
> >  		err = watchdog_get_timeleft(wdd, &val);
> > @@ -799,7 +827,10 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
> >  		err = watchdog_set_pretimeout(wdd, val);
> >  		break;
> >  	case WDIOC_GETPRETIMEOUT:
> > -		err = put_user(wdd->pretimeout, p);
> > +		val = wdd->pretimeout;
> > +		if (wdd->info->options & WDIOF_MSECTIMER)
> > +			val /= 1000;
> > +		err = put_user(val, p);
> >  		break;
> >  	default:
> >  		err = -ENOTTY;
> > diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> > index 1464ce6ffa31..49bfaf986b37 100644
> > --- a/include/linux/watchdog.h
> > +++ b/include/linux/watchdog.h
> > @@ -55,7 +55,9 @@ struct watchdog_ops {
> >  	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
> >  };
> >  
> > -/** struct watchdog_device - The structure that defines a watchdog device
> > +/** struct watchdog_device - The structure that defines a watchdog device.
> > + * Unless otherwise specified, all timeouts are in seconds unless
> > + * WDIOF_MSECTIMER is set, then they are in milliseconds.
> >   *
> >   * @id:		The watchdog's ID. (Allocated by watchdog_register_device)
> >   * @parent:	The parent bus device
> > @@ -65,10 +67,10 @@ struct watchdog_ops {
> >   * @ops:	Pointer to the list of watchdog operations.
> >   * @gov:	Pointer to watchdog pretimeout governor.
> >   * @bootstatus:	Status of the watchdog device at boot.
> > - * @timeout:	The watchdog devices timeout value (in seconds).
> > + * @timeout:	The watchdog devices timeout value.
> >   * @pretimeout: The watchdog devices pre_timeout value.
> > - * @min_timeout:The watchdog devices minimum timeout value (in seconds).
> > - * @max_timeout:The watchdog devices maximum timeout value (in seconds)
> > + * @min_timeout:The watchdog devices minimum timeout value.
> > + * @max_timeout:The watchdog devices maximum timeout value
> >   *		as configurable from user space. Only relevant if
> >   *		max_hw_heartbeat_ms is not provided.
> >   * @min_hw_heartbeat_ms:
> > @@ -156,6 +158,17 @@ static inline void watchdog_stop_on_unregister(struct watchdog_device *wdd)
> >  	set_bit(WDOG_STOP_ON_UNREGISTER, &wdd->status);
> >  }
> >  
> > +/*
> > + * Use the following function to check if a timeout value is
> > + * internally consistent with the range parameters.  t is in milliseconds.
> > + */
> > +static inline bool _watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
> > +{
> > +	return  t < wdd->min_timeout ||
> > +		(!wdd->max_hw_heartbeat_ms && wdd->max_timeout &&
> > +		 t > wdd->max_timeout);
> > +}
> > +
> >  /* Use the following function to check if a timeout value is invalid */
> >  static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
> >  {
> > @@ -170,9 +183,11 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
> >  	 *   is configured, and the requested value is larger than the
> >  	 *   configured maximum timeout.
> >  	 */
> > -	return t > UINT_MAX / 1000 || t < wdd->min_timeout ||
> > -		(!wdd->max_hw_heartbeat_ms && wdd->max_timeout &&
> > -		 t > wdd->max_timeout);
> > +	if (t > UINT_MAX / 1000)
> > +		return true;
> > +	if (wdd->info->options & WDIOF_MSECTIMER)
> > +		t *= 1000;
> > +	return _watchdog_timeout_invalid(wdd, t);
> >  }
> >  
> >  /* Use the following function to check if a pretimeout value is invalid */
> > diff --git a/include/uapi/linux/watchdog.h b/include/uapi/linux/watchdog.h
> > index b15cde5c9054..feb3bcc46993 100644
> > --- a/include/uapi/linux/watchdog.h
> > +++ b/include/uapi/linux/watchdog.h
> > @@ -48,6 +48,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_MSECTIMER		0x0800	/* Driver can use milliseconds for timeouts */
> >  #define	WDIOF_KEEPALIVEPING	0x8000	/* Keep alive ping reply */
> >  
> >  #define	WDIOS_DISABLECARD	0x0001	/* Turn off the watchdog timer */
> > 
> 

Re: [PATCH 1/6] watchdog: Allow a driver to use milliseconds instead of seconds

[Guenter Roeck]

On 6/26/20 7:18 PM, Corey Minyard wrote:
> On Fri, Jun 26, 2020 at 04:10:41PM -0700, Guenter Roeck wrote:
>> On 6/20/20 10:33 AM, minyard@acm.org wrote:
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> If the WDIOF_MSECTIMER is set, then all the timeouts in the watchdog
>>> structure are expected to be in milliseconds.  Add the flag and the
>>> various conversions.  This should have no effect on existing drivers.
>>>
>>
>> For this to work, the entire watchdog core code has to be converted to be based
>> on milliseconds, with API functions converting from s to ms on incoming calls
>> and from ms to s on outgoing calls. At the same time, the existing UAPI must not
>> be changed and still be based on seconds. Milli-second functionality such as
>> milli-second based sysfs attributes or milli-second based ioctls can be added
>> separately.
>>
>> That means functions such as watchdog_need_worker() must be completely based
>> on milli-seconds and not make an on-the-fly conversion on each call.
>> That is just one example.
>>
>> I'd give it a try myself, but unfortunately I just don't have the time.
> 
> The patches in this series should do all this.  For instance:
> 
> @@ -99,7 +99,11 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>  {
>         /* All variables in milli-seconds */
>         unsigned int hm = wdd->max_hw_heartbeat_ms;
> -       unsigned int t = wdd->timeout * 1000;
> +       unsigned int t = wdd->timeout;
> +
> +       if (!(wdd->info->options & WDIOF_MSECTIMER))
> +               /* Convert to msecs if not already so. */
> +               t *= 1000;
> 
>         /*
>          * A worker to generate heartbeat requests is needed if all of the
> 
> Basically, the changes keep the timeout in milliseconds if the lower
> level driver uses milliseconds, and seconds if the lower level driver
> uses seconds.  The watchdog cores do the conversions as necessary.
> All the UAPIs do conversion, they work unchanged.
> 
> That's really the only way you can do this without changing all the low
> level drivers, since they often reference the timeout field.
> 

I disagree. The above is only necessary because wdd->timeout is still used
and in seconds. As result the code is littered with runtime conversions,
instead of a single conversion when the set_timeout() function call
to the driver returns.

Maybe the code is just just too messy. We may have to find a better
solution. Maybe we will need a Coccinelle script which changes all drivers
in one go, or maybe we will need separate callback and ops functions.
As it is, it looks just too messy to me.

Guenter

> -corey
> 
>>
>> Guenter
>>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> ---
>>>  drivers/watchdog/watchdog_core.c | 30 +++++++++++++-------
>>>  drivers/watchdog/watchdog_dev.c  | 47 ++++++++++++++++++++++++++------
>>>  include/linux/watchdog.h         | 29 +++++++++++++++-----
>>>  include/uapi/linux/watchdog.h    |  1 +
>>>  4 files changed, 82 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
>>> index 423844757812..b54451a9a336 100644
>>> --- a/drivers/watchdog/watchdog_core.c
>>> +++ b/drivers/watchdog/watchdog_core.c
>>> @@ -116,17 +116,17 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>>>  {
>>>  	const char *dev_str = wdd->parent ? dev_name(wdd->parent) :
>>>  			      (const char *)wdd->info->identity;
>>> -	unsigned int t = 0;
>>>  	int ret = 0;
>>>  
>>>  	watchdog_check_min_max_timeout(wdd);
>>>  
>>>  	/* check the driver supplied value (likely a module parameter) first */
>>>  	if (timeout_parm) {
>>> -		if (!watchdog_timeout_invalid(wdd, timeout_parm)) {
>>> -			wdd->timeout = timeout_parm;
>>> -			return 0;
>>> -		}
>>> +		if (wdd->info->options & WDIOF_MSECTIMER) {
>>> +			if (!_watchdog_timeout_invalid(wdd, timeout_parm))
>>> +				goto set_timeout;
>>> +		} else if (!watchdog_timeout_invalid(wdd, timeout_parm))
>>> +			goto set_timeout;
>>>  		pr_err("%s: driver supplied timeout (%u) out of range\n",
>>>  			dev_str, timeout_parm);
>>>  		ret = -EINVAL;
>>> @@ -134,12 +134,18 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>>>  
>>>  	/* try to get the timeout_sec property */
>>>  	if (dev && dev->of_node &&
>>> -	    of_property_read_u32(dev->of_node, "timeout-sec", &t) == 0) {
>>> -		if (t && !watchdog_timeout_invalid(wdd, t)) {
>>> -			wdd->timeout = t;
>>> -			return 0;
>>> +	    of_property_read_u32(dev->of_node, "timeout-sec",
>>> +				 &timeout_parm) == 0) {
>>> +		if (timeout_parm &&
>>> +		    !watchdog_timeout_invalid(wdd, timeout_parm)) {
>>> +			if (!(wdd->info->options & WDIOF_MSECTIMER))
>>> +				/* Convert to msecs if not already so. */
>>> +				timeout_parm *= 1000;
>>> +			goto set_timeout;
>>>  		}
>>> -		pr_err("%s: DT supplied timeout (%u) out of range\n", dev_str, t);
>>> +
>>> +		pr_err("%s: DT supplied timeout (%u) out of range\n", dev_str,
>>> +		       timeout_parm);
>>>  		ret = -EINVAL;
>>>  	}
>>>  
>>> @@ -148,6 +154,10 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>>>  			wdd->timeout);
>>>  
>>>  	return ret;
>>> +
>>> +set_timeout:
>>> +	wdd->timeout = timeout_parm;
>>> +	return 0;
>>>  }
>>>  EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>>>  
>>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
>>> index 7e4cd34a8c20..480460b89c16 100644
>>> --- a/drivers/watchdog/watchdog_dev.c
>>> +++ b/drivers/watchdog/watchdog_dev.c
>>> @@ -99,7 +99,11 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>>>  {
>>>  	/* All variables in milli-seconds */
>>>  	unsigned int hm = wdd->max_hw_heartbeat_ms;
>>> -	unsigned int t = wdd->timeout * 1000;
>>> +	unsigned int t = wdd->timeout;
>>> +
>>> +	if (!(wdd->info->options & WDIOF_MSECTIMER))
>>> +		/* Convert to msecs if not already so. */
>>> +		t *= 1000;
>>>  
>>>  	/*
>>>  	 * A worker to generate heartbeat requests is needed if all of the
>>> @@ -121,12 +125,16 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>>>  static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
>>>  {
>>>  	struct watchdog_core_data *wd_data = wdd->wd_data;
>>> -	unsigned int timeout_ms = wdd->timeout * 1000;
>>> +	unsigned int timeout_ms = wdd->timeout;
>>>  	ktime_t keepalive_interval;
>>>  	ktime_t last_heartbeat, latest_heartbeat;
>>>  	ktime_t virt_timeout;
>>>  	unsigned int hw_heartbeat_ms;
>>>  
>>> +	if (!(wdd->info->options & WDIOF_MSECTIMER))
>>> +		/* Convert to msecs if not already so. */
>>> +		timeout_ms *= 1000;
>>> +
>>>  	if (watchdog_active(wdd))
>>>  		virt_timeout = ktime_add(wd_data->last_keepalive,
>>>  					 ms_to_ktime(timeout_ms));
>>> @@ -137,7 +145,7 @@ static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
>>>  	keepalive_interval = ms_to_ktime(hw_heartbeat_ms / 2);
>>>  
>>>  	/*
>>> -	 * To ensure that the watchdog times out wdd->timeout seconds
>>> +	 * To ensure that the watchdog times out wdd->timeout seconds/msecs
>>>  	 * after the most recent ping from userspace, the last
>>>  	 * worker ping has to come in hw_heartbeat_ms before this timeout.
>>>  	 */
>>> @@ -382,6 +390,8 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
>>>  	if (watchdog_timeout_invalid(wdd, timeout))
>>>  		return -EINVAL;
>>>  
>>> +	if (wdd->info->options & WDIOF_MSECTIMER)
>>> +		timeout *= 1000;
>>>  	if (wdd->ops->set_timeout) {
>>>  		err = wdd->ops->set_timeout(wdd, timeout);
>>>  	} else {
>>> @@ -413,6 +423,8 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
>>>  	if (watchdog_pretimeout_invalid(wdd, timeout))
>>>  		return -EINVAL;
>>>  
>>> +	if (wdd->info->options & WDIOF_MSECTIMER)
>>> +		timeout *= 1000;
>>>  	if (wdd->ops->set_pretimeout)
>>>  		err = wdd->ops->set_pretimeout(wdd, timeout);
>>>  	else
>>> @@ -440,6 +452,8 @@ static int watchdog_get_timeleft(struct watchdog_device *wdd,
>>>  		return -EOPNOTSUPP;
>>>  
>>>  	*timeleft = wdd->ops->get_timeleft(wdd);
>>> +	if (wdd->info->options & WDIOF_MSECTIMER)
>>> +		*timeleft /= 1000;
>>>  
>>>  	return 0;
>>>  }
>>> @@ -508,8 +522,11 @@ static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
>>>  	mutex_lock(&wd_data->lock);
>>>  	status = watchdog_get_timeleft(wdd, &val);
>>>  	mutex_unlock(&wd_data->lock);
>>> -	if (!status)
>>> +	if (!status) {
>>> +		if (wdd->info->options & WDIOF_MSECTIMER)
>>> +			val /= 1000;
>>>  		status = sprintf(buf, "%u\n", val);
>>> +	}
>>>  
>>>  	return status;
>>>  }
>>> @@ -519,8 +536,12 @@ static ssize_t timeout_show(struct device *dev, struct device_attribute *attr,
>>>  				char *buf)
>>>  {
>>>  	struct watchdog_device *wdd = dev_get_drvdata(dev);
>>> +	unsigned int t = wdd->timeout;
>>> +
>>> +	if (wdd->info->options & WDIOF_MSECTIMER)
>>> +		t /= 1000;
>>>  
>>> -	return sprintf(buf, "%u\n", wdd->timeout);
>>> +	return sprintf(buf, "%u\n", t);
>>>  }
>>>  static DEVICE_ATTR_RO(timeout);
>>>  
>>> @@ -528,8 +549,12 @@ static ssize_t pretimeout_show(struct device *dev,
>>>  			       struct device_attribute *attr, char *buf)
>>>  {
>>>  	struct watchdog_device *wdd = dev_get_drvdata(dev);
>>> +	unsigned int t = wdd->pretimeout;
>>>  
>>> -	return sprintf(buf, "%u\n", wdd->pretimeout);
>>> +	if (wdd->info->options & WDIOF_MSECTIMER)
>>> +		t /= 1000;
>>> +
>>> +	return sprintf(buf, "%u\n", t);
>>>  }
>>>  static DEVICE_ATTR_RO(pretimeout);
>>>  
>>> @@ -783,7 +808,10 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>>>  			err = -EOPNOTSUPP;
>>>  			break;
>>>  		}
>>> -		err = put_user(wdd->timeout, p);
>>> +		val = wdd->timeout;
>>> +		if (wdd->info->options & WDIOF_MSECTIMER)
>>> +			val /= 1000;
>>> +		err = put_user(val, p);
>>>  		break;
>>>  	case WDIOC_GETTIMELEFT:
>>>  		err = watchdog_get_timeleft(wdd, &val);
>>> @@ -799,7 +827,10 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>>>  		err = watchdog_set_pretimeout(wdd, val);
>>>  		break;
>>>  	case WDIOC_GETPRETIMEOUT:
>>> -		err = put_user(wdd->pretimeout, p);
>>> +		val = wdd->pretimeout;
>>> +		if (wdd->info->options & WDIOF_MSECTIMER)
>>> +			val /= 1000;
>>> +		err = put_user(val, p);
>>>  		break;
>>>  	default:
>>>  		err = -ENOTTY;
>>> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
>>> index 1464ce6ffa31..49bfaf986b37 100644
>>> --- a/include/linux/watchdog.h
>>> +++ b/include/linux/watchdog.h
>>> @@ -55,7 +55,9 @@ struct watchdog_ops {
>>>  	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
>>>  };
>>>  
>>> -/** struct watchdog_device - The structure that defines a watchdog device
>>> +/** struct watchdog_device - The structure that defines a watchdog device.
>>> + * Unless otherwise specified, all timeouts are in seconds unless
>>> + * WDIOF_MSECTIMER is set, then they are in milliseconds.
>>>   *
>>>   * @id:		The watchdog's ID. (Allocated by watchdog_register_device)
>>>   * @parent:	The parent bus device
>>> @@ -65,10 +67,10 @@ struct watchdog_ops {
>>>   * @ops:	Pointer to the list of watchdog operations.
>>>   * @gov:	Pointer to watchdog pretimeout governor.
>>>   * @bootstatus:	Status of the watchdog device at boot.
>>> - * @timeout:	The watchdog devices timeout value (in seconds).
>>> + * @timeout:	The watchdog devices timeout value.
>>>   * @pretimeout: The watchdog devices pre_timeout value.
>>> - * @min_timeout:The watchdog devices minimum timeout value (in seconds).
>>> - * @max_timeout:The watchdog devices maximum timeout value (in seconds)
>>> + * @min_timeout:The watchdog devices minimum timeout value.
>>> + * @max_timeout:The watchdog devices maximum timeout value
>>>   *		as configurable from user space. Only relevant if
>>>   *		max_hw_heartbeat_ms is not provided.
>>>   * @min_hw_heartbeat_ms:
>>> @@ -156,6 +158,17 @@ static inline void watchdog_stop_on_unregister(struct watchdog_device *wdd)
>>>  	set_bit(WDOG_STOP_ON_UNREGISTER, &wdd->status);
>>>  }
>>>  
>>> +/*
>>> + * Use the following function to check if a timeout value is
>>> + * internally consistent with the range parameters.  t is in milliseconds.
>>> + */
>>> +static inline bool _watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
>>> +{
>>> +	return  t < wdd->min_timeout ||
>>> +		(!wdd->max_hw_heartbeat_ms && wdd->max_timeout &&
>>> +		 t > wdd->max_timeout);
>>> +}
>>> +
>>>  /* Use the following function to check if a timeout value is invalid */
>>>  static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
>>>  {
>>> @@ -170,9 +183,11 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
>>>  	 *   is configured, and the requested value is larger than the
>>>  	 *   configured maximum timeout.
>>>  	 */
>>> -	return t > UINT_MAX / 1000 || t < wdd->min_timeout ||
>>> -		(!wdd->max_hw_heartbeat_ms && wdd->max_timeout &&
>>> -		 t > wdd->max_timeout);
>>> +	if (t > UINT_MAX / 1000)
>>> +		return true;
>>> +	if (wdd->info->options & WDIOF_MSECTIMER)
>>> +		t *= 1000;
>>> +	return _watchdog_timeout_invalid(wdd, t);
>>>  }
>>>  
>>>  /* Use the following function to check if a pretimeout value is invalid */
>>> diff --git a/include/uapi/linux/watchdog.h b/include/uapi/linux/watchdog.h
>>> index b15cde5c9054..feb3bcc46993 100644
>>> --- a/include/uapi/linux/watchdog.h
>>> +++ b/include/uapi/linux/watchdog.h
>>> @@ -48,6 +48,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_MSECTIMER		0x0800	/* Driver can use milliseconds for timeouts */
>>>  #define	WDIOF_KEEPALIVEPING	0x8000	/* Keep alive ping reply */
>>>  
>>>  #define	WDIOS_DISABLECARD	0x0001	/* Turn off the watchdog timer */
>>>
>>

Re: [PATCH 1/6] watchdog: Allow a driver to use milliseconds instead of seconds

[Guenter Roeck]

On Sat, Jun 20, 2020 at 12:33:46PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> If the WDIOF_MSECTIMER is set, then all the timeouts in the watchdog
> structure are expected to be in milliseconds.  Add the flag and the
> various conversions.  This should have no effect on existing drivers.
> 

I have decided to go another route: Instead of using a WDIOF_MSECTIMER
flag, provide new callback functions. That increases structure sizes,
but ultimately I think it is cleaner.

I implemented a prototype, but didn't have a chance to test it yet.

Guenter

Re: [PATCH 1/6] watchdog: Allow a driver to use milliseconds instead of seconds

[Corey Minyard]

On Sun, Jun 28, 2020 at 07:54:20AM -0700, Guenter Roeck wrote:
> On Sat, Jun 20, 2020 at 12:33:46PM -0500, minyard@acm.org wrote:
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > If the WDIOF_MSECTIMER is set, then all the timeouts in the watchdog
> > structure are expected to be in milliseconds.  Add the flag and the
> > various conversions.  This should have no effect on existing drivers.
> > 
> 
> I have decided to go another route: Instead of using a WDIOF_MSECTIMER
> flag, provide new callback functions. That increases structure sizes,
> but ultimately I think it is cleaner.
> 
> I implemented a prototype, but didn't have a chance to test it yet.

Oh, ok.  I was looking at going through all the existing drivers and
changing the direct references to timeout and pretimeout to use accessor
functions.  That what what I thought you were hinting at, and I thought
that was a good plan.  My original idea was to provide the ability,
change everything over, then remove the messy cruft from the core
driver.

If you want to send something, I can try it out.

-corey

> 
> Guenter

Re: [PATCH 1/6] watchdog: Allow a driver to use milliseconds instead of seconds

[Corey Minyard]

On Sun, Jun 28, 2020 at 07:54:20AM -0700, Guenter Roeck wrote:
> On Sat, Jun 20, 2020 at 12:33:46PM -0500, minyard@acm.org wrote:
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > If the WDIOF_MSECTIMER is set, then all the timeouts in the watchdog
> > structure are expected to be in milliseconds.  Add the flag and the
> > various conversions.  This should have no effect on existing drivers.
> > 
> 
> I have decided to go another route: Instead of using a WDIOF_MSECTIMER
> flag, provide new callback functions. That increases structure sizes,
> but ultimately I think it is cleaner.
> 
> I implemented a prototype, but didn't have a chance to test it yet.
> 
> Guenter

I was wondering if you had progressed with this.  I'm happy to help with
it, if there's anything I can do.

Regards,

-corey

Re: [PATCH 1/6] watchdog: Allow a driver to use milliseconds instead of seconds

[Guenter Roeck]

On Tue, Dec 01, 2020 at 04:54:37PM -0600, Corey Minyard wrote:
> On Sun, Jun 28, 2020 at 07:54:20AM -0700, Guenter Roeck wrote:
> > On Sat, Jun 20, 2020 at 12:33:46PM -0500, minyard@acm.org wrote:
> > > From: Corey Minyard <cminyard@mvista.com>
> > > 
> > > If the WDIOF_MSECTIMER is set, then all the timeouts in the watchdog
> > > structure are expected to be in milliseconds.  Add the flag and the
> > > various conversions.  This should have no effect on existing drivers.
> > > 
> > 
> > I have decided to go another route: Instead of using a WDIOF_MSECTIMER
> > flag, provide new callback functions. That increases structure sizes,
> > but ultimately I think it is cleaner.
> > 
> > I implemented a prototype, but didn't have a chance to test it yet.
> > 
> > Guenter
> 
> I was wondering if you had progressed with this.  I'm happy to help with
> it, if there's anything I can do.
> 

Unfortunately I got interrupted, and right now I am so deeply buried in work
that I don't think I'll get to it anytime soon. Major problem I had before I
had to give up is that everything I came up with was way too invasive for
my liking.

Guenter

[PATCH 2/6] watchdog: Add ioctls for millisecond timeout handling

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Add millisecond ioctls for the five timeout ioctls in the watchdog.  If
the driver being used doesn't support milliseconds, the appropriate
conversions are done.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/watchdog/watchdog_core.c |   5 +-
 drivers/watchdog/watchdog_core.h |   2 +
 drivers/watchdog/watchdog_dev.c  | 123 ++++++++++++++++++++++---------
 include/uapi/linux/watchdog.h    |   5 ++
 4 files changed, 98 insertions(+), 37 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index b54451a9a336..9c531ae05c46 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -138,9 +138,8 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 				 &timeout_parm) == 0) {
 		if (timeout_parm &&
 		    !watchdog_timeout_invalid(wdd, timeout_parm)) {
-			if (!(wdd->info->options & WDIOF_MSECTIMER))
-				/* Convert to msecs if not already so. */
-				timeout_parm *= 1000;
+			timeout_parm = watchdog_timeout_tointernal(wdd, false,
+							timeout_parm);
 			goto set_timeout;
 		}
 
diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
index a5062e8e0d13..151806073d58 100644
--- a/drivers/watchdog/watchdog_core.h
+++ b/drivers/watchdog/watchdog_core.h
@@ -31,3 +31,5 @@ extern int watchdog_dev_register(struct watchdog_device *);
 extern void watchdog_dev_unregister(struct watchdog_device *);
 extern int __init watchdog_dev_init(void);
 extern void __exit watchdog_dev_exit(void);
+extern unsigned int watchdog_timeout_tointernal(struct watchdog_device *,
+						bool, unsigned int);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 480460b89c16..eca13ce1dc91 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -371,6 +371,56 @@ static unsigned int watchdog_get_status(struct watchdog_device *wdd)
 	return status;
 }
 
+/*
+ *	watchdog_timeout_tointernal: Convert to internal time representation
+ *	@wdd: the watchdog device
+ *	@in_msecs: false - timeout in seconds, true - timeout in milliseconds
+ *	@timeout: timeout to convert
+ *
+ *	Convert from an external representation of the timeout in
+ *	seconds (or milliseconds if in_msecs is true) to the internal
+ *	timeout in seconds or milliseconds, depending on
+ *	WDIOF_MSECTIMER.
+ */
+unsigned int watchdog_timeout_tointernal(struct watchdog_device *wdd,
+					 bool in_msecs,
+					 unsigned int timeout)
+{
+	if (wdd->info->options & WDIOF_MSECTIMER) {
+		if (!in_msecs)
+			timeout *= 1000;
+	} else if (in_msecs) {
+		/* Truncate up. */
+		timeout = (timeout + 999) / 1000;
+	}
+	return timeout;
+}
+
+/*
+ *	watchdog_timeout_toexternal: Convert to external time representation
+ *	@wdd: the watchdog device
+ *	@in_msecs: false - returns seconds, true - returns milliseconds
+ *	@timeout: timeout in seconds (or milliseconds if WDIOF_MSECTIMER is set)
+ *
+ *	Convert from an internal representation of the timeout in
+ *	seconds (or milliseconds if WDIOF_MSECTIMER is set) to the
+ *	external timeout in seconds or milliseconds, depending on
+ *	in_msecs.
+ */
+static unsigned int watchdog_timeout_toexternal(struct watchdog_device *wdd,
+						bool in_msecs,
+						unsigned int timeout)
+{
+	if (wdd->info->options & WDIOF_MSECTIMER) {
+		if (!in_msecs)
+			timeout /= 1000;
+	} else if (in_msecs) {
+		timeout *= 1000;
+	}
+
+	return timeout;
+}
+
 /*
  *	watchdog_set_timeout: set the watchdog timer timeout
  *	@wdd: the watchdog device to set the timeout for
@@ -379,7 +429,7 @@ static unsigned int watchdog_get_status(struct watchdog_device *wdd)
  *	The caller must hold wd_data->lock.
  */
 
-static int watchdog_set_timeout(struct watchdog_device *wdd,
+static int watchdog_set_timeout(struct watchdog_device *wdd, bool in_msecs,
 							unsigned int timeout)
 {
 	int err = 0;
@@ -387,11 +437,13 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
 	if (!(wdd->info->options & WDIOF_SETTIMEOUT))
 		return -EOPNOTSUPP;
 
-	if (watchdog_timeout_invalid(wdd, timeout))
+	if (in_msecs) {
+		if (_watchdog_timeout_invalid(wdd, timeout))
+			return -EINVAL;
+	} else if (watchdog_timeout_invalid(wdd, timeout))
 		return -EINVAL;
 
-	if (wdd->info->options & WDIOF_MSECTIMER)
-		timeout *= 1000;
+	timeout = watchdog_timeout_tointernal(wdd, in_msecs, timeout);
 	if (wdd->ops->set_timeout) {
 		err = wdd->ops->set_timeout(wdd, timeout);
 	} else {
@@ -413,6 +465,7 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
  */
 
 static int watchdog_set_pretimeout(struct watchdog_device *wdd,
+				   bool in_msecs,
 				   unsigned int timeout)
 {
 	int err = 0;
@@ -423,8 +476,7 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
 	if (watchdog_pretimeout_invalid(wdd, timeout))
 		return -EINVAL;
 
-	if (wdd->info->options & WDIOF_MSECTIMER)
-		timeout *= 1000;
+	timeout = watchdog_timeout_tointernal(wdd, in_msecs, timeout);
 	if (wdd->ops->set_pretimeout)
 		err = wdd->ops->set_pretimeout(wdd, timeout);
 	else
@@ -443,7 +495,7 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
  *	Get the time before a watchdog will reboot (if not pinged).
  */
 
-static int watchdog_get_timeleft(struct watchdog_device *wdd,
+static int watchdog_get_timeleft(struct watchdog_device *wdd, bool in_msecs,
 							unsigned int *timeleft)
 {
 	*timeleft = 0;
@@ -452,8 +504,7 @@ static int watchdog_get_timeleft(struct watchdog_device *wdd,
 		return -EOPNOTSUPP;
 
 	*timeleft = wdd->ops->get_timeleft(wdd);
-	if (wdd->info->options & WDIOF_MSECTIMER)
-		*timeleft /= 1000;
+	*timeleft = watchdog_timeout_toexternal(wdd, in_msecs, *timeleft);
 
 	return 0;
 }
@@ -520,13 +571,11 @@ static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
 	unsigned int val;
 
 	mutex_lock(&wd_data->lock);
-	status = watchdog_get_timeleft(wdd, &val);
+	status = watchdog_get_timeleft(wdd, false, &val);
 	mutex_unlock(&wd_data->lock);
-	if (!status) {
-		if (wdd->info->options & WDIOF_MSECTIMER)
-			val /= 1000;
-		status = sprintf(buf, "%u\n", val);
-	}
+	if (!status)
+		status = sprintf(buf, "%u\n",
+				 watchdog_timeout_toexternal(wdd, false, val));
 
 	return status;
 }
@@ -536,12 +585,9 @@ static ssize_t timeout_show(struct device *dev, struct device_attribute *attr,
 				char *buf)
 {
 	struct watchdog_device *wdd = dev_get_drvdata(dev);
-	unsigned int t = wdd->timeout;
 
-	if (wdd->info->options & WDIOF_MSECTIMER)
-		t /= 1000;
-
-	return sprintf(buf, "%u\n", t);
+	return sprintf(buf, "%u\n",
+		       watchdog_timeout_toexternal(wdd, false, wdd->timeout));
 }
 static DEVICE_ATTR_RO(timeout);
 
@@ -549,12 +595,10 @@ static ssize_t pretimeout_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
 	struct watchdog_device *wdd = dev_get_drvdata(dev);
-	unsigned int t = wdd->pretimeout;
-
-	if (wdd->info->options & WDIOF_MSECTIMER)
-		t /= 1000;
 
-	return sprintf(buf, "%u\n", t);
+	return sprintf(buf, "%u\n",
+		       watchdog_timeout_toexternal(wdd, false,
+						   wdd->pretimeout));
 }
 static DEVICE_ATTR_RO(pretimeout);
 
@@ -788,11 +832,13 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 		err = watchdog_ping(wdd);
 		break;
 	case WDIOC_SETTIMEOUT:
+	case WDIOC_SETTIMEOUT_MS:
 		if (get_user(val, p)) {
 			err = -EFAULT;
 			break;
 		}
-		err = watchdog_set_timeout(wdd, val);
+		err = watchdog_set_timeout(wdd, cmd == WDIOC_SETTIMEOUT_MS,
+					   val);
 		if (err < 0)
 			break;
 		/* If the watchdog is active then we send a keepalive ping
@@ -803,33 +849,42 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 			break;
 		/* fall through */
 	case WDIOC_GETTIMEOUT:
+	case WDIOC_GETTIMEOUT_MS:
 		/* timeout == 0 means that we don't know the timeout */
 		if (wdd->timeout == 0) {
 			err = -EOPNOTSUPP;
 			break;
 		}
-		val = wdd->timeout;
-		if (wdd->info->options & WDIOF_MSECTIMER)
-			val /= 1000;
+		val = watchdog_timeout_toexternal(wdd,
+					  (cmd == WDIOC_SETTIMEOUT_MS ||
+					   cmd == WDIOC_GETTIMEOUT_MS),
+					  wdd->timeout);
 		err = put_user(val, p);
 		break;
 	case WDIOC_GETTIMELEFT:
-		err = watchdog_get_timeleft(wdd, &val);
+	case WDIOC_GETTIMELEFT_MS:
+		err = watchdog_get_timeleft(wdd, cmd == WDIOC_GETTIMELEFT_MS,
+					    &val);
 		if (err < 0)
 			break;
 		err = put_user(val, p);
 		break;
 	case WDIOC_SETPRETIMEOUT:
+	case WDIOC_SETPRETIMEOUT_MS:
 		if (get_user(val, p)) {
 			err = -EFAULT;
 			break;
 		}
-		err = watchdog_set_pretimeout(wdd, val);
+		err = watchdog_set_pretimeout(wdd,
+					      cmd == WDIOC_SETPRETIMEOUT_MS,
+					      val);
 		break;
 	case WDIOC_GETPRETIMEOUT:
-		val = wdd->pretimeout;
-		if (wdd->info->options & WDIOF_MSECTIMER)
-			val /= 1000;
+	case WDIOC_GETPRETIMEOUT_MS:
+		val = watchdog_timeout_toexternal(wdd,
+					  (cmd == WDIOC_SETPRETIMEOUT_MS ||
+					   cmd == WDIOC_GETPRETIMEOUT_MS),
+					  wdd->pretimeout);
 		err = put_user(val, p);
 		break;
 	default:
diff --git a/include/uapi/linux/watchdog.h b/include/uapi/linux/watchdog.h
index feb3bcc46993..3df7880c6fca 100644
--- a/include/uapi/linux/watchdog.h
+++ b/include/uapi/linux/watchdog.h
@@ -32,6 +32,11 @@ 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_SETTIMEOUT_MS	_IOWR(WATCHDOG_IOCTL_BASE, 11, int)
+#define	WDIOC_GETTIMEOUT_MS	_IOR(WATCHDOG_IOCTL_BASE, 12, int)
+#define	WDIOC_SETPRETIMEOUT_MS	_IOWR(WATCHDOG_IOCTL_BASE, 13, int)
+#define	WDIOC_GETPRETIMEOUT_MS	_IOR(WATCHDOG_IOCTL_BASE, 14, int)
+#define	WDIOC_GETTIMELEFT_MS	_IOR(WATCHDOG_IOCTL_BASE, 15, int)
 
 #define	WDIOF_UNKNOWN		-1	/* Unknown flag error */
 #define	WDIOS_UNKNOWN		-1	/* Unknown status error */
-- 
2.17.1

[PATCH 3/6] watchdog: Add millisecond precision device attributes

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Add timeleft_ms, timeout_ms, and pretimeout_ms that print microsecond
values.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/watchdog/watchdog_dev.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index eca13ce1dc91..b82049896204 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -562,6 +562,21 @@ static ssize_t bootstatus_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(bootstatus);
 
+/*
+ * If _ms is in the attribute name, return milliseconds, otherwise
+ * return seconds.
+ */
+static unsigned int conv_time_to_attr_display(struct watchdog_device *wdd,
+					      struct device_attribute *attr,
+					      unsigned int val)
+{
+	bool in_msec = false;
+
+	if (strstr(attr->attr.name, "_ms"))
+		in_msec = true;
+	return watchdog_timeout_toexternal(wdd, in_msec, val);
+}
+
 static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
 				char *buf)
 {
@@ -575,11 +590,12 @@ static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
 	mutex_unlock(&wd_data->lock);
 	if (!status)
 		status = sprintf(buf, "%u\n",
-				 watchdog_timeout_toexternal(wdd, false, val));
+				 conv_time_to_attr_display(wdd, attr, val));
 
 	return status;
 }
 static DEVICE_ATTR_RO(timeleft);
+static DEVICE_ATTR(timeleft_ms, 0444, timeleft_show, NULL);
 
 static ssize_t timeout_show(struct device *dev, struct device_attribute *attr,
 				char *buf)
@@ -587,9 +603,10 @@ static ssize_t timeout_show(struct device *dev, struct device_attribute *attr,
 	struct watchdog_device *wdd = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%u\n",
-		       watchdog_timeout_toexternal(wdd, false, wdd->timeout));
+		       conv_time_to_attr_display(wdd, attr, wdd->timeout));
 }
 static DEVICE_ATTR_RO(timeout);
+static DEVICE_ATTR(timeout_ms, 0444, timeout_show, NULL);
 
 static ssize_t pretimeout_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
@@ -597,10 +614,10 @@ static ssize_t pretimeout_show(struct device *dev,
 	struct watchdog_device *wdd = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%u\n",
-		       watchdog_timeout_toexternal(wdd, false,
-						   wdd->pretimeout));
+		       conv_time_to_attr_display(wdd, attr, wdd->pretimeout));
 }
 static DEVICE_ATTR_RO(pretimeout);
+static DEVICE_ATTR(pretimeout_ms, 0444, pretimeout_show, NULL);
 
 static ssize_t identity_show(struct device *dev, struct device_attribute *attr,
 				char *buf)
@@ -677,8 +694,11 @@ static struct attribute *wdt_attrs[] = {
 	&dev_attr_state.attr,
 	&dev_attr_identity.attr,
 	&dev_attr_timeout.attr,
+	&dev_attr_timeout_ms.attr,
 	&dev_attr_pretimeout.attr,
+	&dev_attr_pretimeout_ms.attr,
 	&dev_attr_timeleft.attr,
+	&dev_attr_timeleft_ms.attr,
 	&dev_attr_bootstatus.attr,
 	&dev_attr_status.attr,
 	&dev_attr_nowayout.attr,
-- 
2.17.1

[PATCH 4/6] watchdog: Add documentation for millisecond interfaces

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Document the new interfaces and semantics of how this works.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 Documentation/watchdog/watchdog-api.rst       | 59 +++++++++++++++++++
 .../watchdog/watchdog-kernel-api.rst          | 30 ++++++----
 2 files changed, 78 insertions(+), 11 deletions(-)

diff --git a/Documentation/watchdog/watchdog-api.rst b/Documentation/watchdog/watchdog-api.rst
index c6c1e9fa9f73..9f9aa1cd7310 100644
--- a/Documentation/watchdog/watchdog-api.rst
+++ b/Documentation/watchdog/watchdog-api.rst
@@ -112,6 +112,39 @@ current timeout using the GETTIMEOUT ioctl::
     ioctl(fd, WDIOC_GETTIMEOUT, &timeout);
     printf("The timeout was is %d seconds\n", timeout);
 
+There is also millisecond-level ioctls for setting timeouts in
+millisecond values.  These will work against a watchdog driver that
+only supports seconds, but values will be truncated up on setting and
+truncated on fetching.  So, for instance:
+
+    int timeout = 44001;
+    ioctl(fd, WDIOC_SETTIMEOUT_MS, &timeout);
+    printf("The timeout was set to %d milliseconds\n", timeout);
+
+If the driver only supports seconds, the timeout will be set to 45
+seconds because it's truncated up.
+
+Fetching does similar conversions.  On a driver that supports
+milliseconds, if the current value is 39603 milliseconds:
+
+    ioctl(fd, WDIOC_GETTIMEOUT, &timeout);
+    printf("The timeout was is %d seconds\n", timeout);
+    ioctl(fd, WDIOC_GETTIMEOUT_MS, &timeout);
+    printf("The timeout was is %d milliseconds\n", timeout);
+
+will print 39 seconds and 39603 milliseconds.
+
+If a driver supports millisecond level precision, it will have the
+WDIOF_MSECTIMER flag set in its option field.  Note that does not mean
+that the driver has millisecond level accuracy.  For instance, a
+device might have a 10Hz clock, giving 100ms accuracy.  The driver
+should set the return timeout for WDIOC_SETTIMEOUT_MS to the actual
+setting of the timeout, so you can verify the value.
+
+It would be nice to have a granularity field, but some devices may not
+be linear.  So a granularity is not a general thing that could be
+done.
+
 Pretimeouts
 ===========
 
@@ -137,6 +170,16 @@ There is also a get function for getting the pretimeout::
 
 Not all watchdog drivers will support a pretimeout.
 
+Like timeouts, pretimeouts also have millisecond-level ioctls:
+
+    pretimeout = 10000;
+    ioctl(fd, WDIOC_SETPRETIMEOUT_MS, &pretimeout);
+    ioctl(fd, WDIOC_GETPRETIMEOUT_NS, &pretimeout);
+    printf("The pretimeout was is %d milliseconds\n", pretimeout);
+
+These work just like the timeouts, see that discussion for how
+conversions are done.
+
 Get the number of seconds before reboot
 =======================================
 
@@ -147,6 +190,14 @@ that returns the number of seconds before reboot::
     ioctl(fd, WDIOC_GETTIMELEFT, &timeleft);
     printf("The timeout was is %d seconds\n", timeleft);
 
+There is also a millisecond-level version:
+
+    ioctl(fd, WDIOC_GETTIMELEFT_MS, &timeleft);
+    printf("The timeout was is %d milliseconds\n", timeleft);
+
+If the driver only supports seconds, then the value returns is just
+1000 times the seconds value.
+
 Environmental monitoring
 ========================
 
@@ -223,6 +274,14 @@ sense.
 
 The watchdog saw a keepalive ping since it was last queried.
 
+	===============		========================================
+	WDIOF_MSECTIMER		Driver can use milliseconds for timeouts
+	===============		========================================
+
+The driver can do millisecond-level timeouts.  The seconds-level
+interfaces still work, but setting values in milliseconds can result
+in finer granularity.
+
 	================	=======================
 	WDIOF_SETTIMEOUT	Can set/get the timeout
 	================	=======================
diff --git a/Documentation/watchdog/watchdog-kernel-api.rst b/Documentation/watchdog/watchdog-kernel-api.rst
index 068a55ee0d4a..bee60e6ae274 100644
--- a/Documentation/watchdog/watchdog-kernel-api.rst
+++ b/Documentation/watchdog/watchdog-kernel-api.rst
@@ -80,13 +80,13 @@ 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.
 * gov: a pointer to the assigned watchdog device pretimeout governor or NULL.
-* timeout: the watchdog timer's timeout value (in seconds).
+* timeout: the watchdog timer's timeout value.
   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).
+* pretimeout: the watchdog timer's pretimeout value.
+* min_timeout: the watchdog timer's minimum timeout value.
   If set, the minimum configurable value for 'timeout'.
-* max_timeout: the watchdog timer's maximum timeout value (in seconds),
+* max_timeout: the watchdog timer's maximum timeout value,
   as seen from userspace. If set, the maximum configurable value for
   'timeout'. Not used if max_hw_heartbeat_ms is non-zero.
 * min_hw_heartbeat_ms: Hardware limit for minimum time between heartbeats,
@@ -96,7 +96,7 @@ It contains following fields:
   If set, the infrastructure will send heartbeats to the watchdog driver
   if 'timeout' is larger than max_hw_heartbeat_ms, unless WDOG_ACTIVE
   is set and userspace failed to send a heartbeat for at least 'timeout'
-  seconds. max_hw_heartbeat_ms must be set if a driver does not implement
+  time. max_hw_heartbeat_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
@@ -117,6 +117,13 @@ It contains following fields:
 * deferred: entry in wtd_deferred_reg_list which is used to
   register early initialized watchdogs.
 
+The time value used to interact with the device can either be in
+seconds or milli-seconds except for min_hw_heartbeat_ms and
+max_hw_heartbeat_ms, which are always in milli-seconds. If the driver
+sets WDIOF_MSECTIMER in the driver info flags, then the time values
+will be in milli-seconds. If it is not set, then the time values will
+be in seconds.
+
 The list of watchdog operations is defined as::
 
   struct watchdog_ops {
@@ -212,12 +219,13 @@ they are supported. These optional routines/operations are:
   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.
+* 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 time 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).
-- 
2.17.1

[PATCH 5/6] watchdog:i6300: Convert to a millisecond watchdog device

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The i6300 clock runs at 1Khz, so it's an easy conversion to make it a
millisecond timer.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/watchdog/i6300esb.c | 66 +++++++++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index a30835f547b3..9f5afe6ee622 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -69,12 +69,40 @@
 #define ESB_UNLOCK1     0x80            /* Step 1 to unlock reset registers  */
 #define ESB_UNLOCK2     0x86            /* Step 2 to unlock reset registers  */
 
-/* module parameters */
-/* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
+/*
+ * Timer clock is driven by a 30ns clock divided by 32768, giving a
+ * 983.04usec clock.  Not really close enough to say it's 1Khz.  But
+ * if we multiply by 101725261 / 100000000, that would give us a
+ * 1000.0000057 usec per increment of time, which is very close and
+ * slightly slow, which is preferred to slightly fast.  If there is a
+ * remainder from that calculation, then round up.  So if 1 comes in
+ * for the time, we will have (1 * 101725261) / 100000000 = 1, and
+ * (1 * 101725261) % 100000000 = 1725261, so we round up the 1 to a 2, which
+ * will result in 1.96608msecs.  Remember, better too long than too short.
+ * All the arithmetic has to be 64 bit to avoid overflow.
+ *
+ * The error gets better as the numbers increase to more reasonable
+ * values.  For 30 seconds, for instance, we get a count of 30518,
+ * which is 30.0004 seconds.  Close enough :).
+ *
+ * The 2061582 max time comes in because we have 2 20 bit registers
+ * that count down.  This means that the maximum timeout value we can
+ * put in the registers is 0x1ffffe.  Run that through the calculation
+ * backwards and we get 0x1ffffe * 100000000 / 101725261 = 2061582.  If we
+ * put that into our calculation, we get 2061582 * 101725261 / 100000000 =
+ * 0x1ffffd which will round up to 0x1ffffe.
+ *
+ * These numbers should never result in an error more than 1ms, so
+ * there is no need to be more accurate.  This is been tested
+ * exhaustively.
+ */
 #define ESB_HEARTBEAT_MIN	1
-#define ESB_HEARTBEAT_MAX	2046
-#define ESB_HEARTBEAT_DEFAULT	30
-#define ESB_HEARTBEAT_RANGE __MODULE_STRING(ESB_HEARTBEAT_MIN) \
+#define ESB_HEARTBEAT_MAX	2061582
+/* 30 sec default heartbeat */
+#define ESB_HEARTBEAT_DEFAULT	30000
+
+/* module parameters */
+#define ESB_HEARTBEAT_RANGE __MODULE_STRING(ESB_HEARTBEAT_MIN)	\
 	"<heartbeat<" __MODULE_STRING(ESB_HEARTBEAT_MAX)
 static int heartbeat; /* in seconds */
 module_param(heartbeat, int, 0);
@@ -158,17 +186,34 @@ static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
 {
 	struct esb_dev *edev = to_esb_dev(wdd);
 	u32 val;
+	u64 ttime, ctime;
+
+	/* See comments above ESB_HEARTBEAT_xxx for details on this. */
+	ttime = (u64) time * 101725261ULL;
+	ctime = div_u64(ttime, 100000000);
+	/*
+	 * You might think that a "if (ttime % 100000000ULL)" would be
+	 * required here so we don't round up if the time is exact,
+	 * but with these numbers, there is never a time value that
+	 * will come in that will result in a zero remainder.  So no
+	 * need to check.  We round up, as it's better to be a little
+	 * long than a little short.
+	 */
+	ctime += 1;
 
-	/* We shift by 9, so if we are passed a value of 1 sec,
-	 * val will be 1 << 9 = 512, then write that to two
-	 * timers => 2 * 512 = 1024 (which is decremented at 1KHz)
+	/*
+	 * We have two registers that have to count down, so each gets
+	 * loaded with half the time.
 	 */
-	val = time << 9;
+	val = ctime / 2;
 
 	/* Write timer 1 */
 	esb_unlock_registers(edev);
 	writel(val, ESB_TIMER1_REG(edev));
 
+	/* If the time was odd, add the extra tick to the second register. */
+	val += ctime % 2;
+
 	/* Write timer 2 */
 	esb_unlock_registers(edev);
 	writel(val, ESB_TIMER2_REG(edev));
@@ -190,7 +235,8 @@ static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
 
 static struct watchdog_info esb_info = {
 	.identity = ESB_MODULE_NAME,
-	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+	.options = (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE |
+		    WDIOF_MSECTIMER),
 };
 
 static const struct watchdog_ops esb_ops = {
-- 
2.17.1

[PATCH 6/6] watchdog:softdog: Convert to a millisecond watchdog

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Use milliseconds for the softdog timer to support a higher resolution
timeout.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/watchdog/softdog.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
index 3e4885c1545e..794f552db53b 100644
--- a/drivers/watchdog/softdog.c
+++ b/drivers/watchdog/softdog.c
@@ -82,13 +82,13 @@ static int softdog_ping(struct watchdog_device *w)
 {
 	if (!hrtimer_active(&softdog_ticktock))
 		__module_get(THIS_MODULE);
-	hrtimer_start(&softdog_ticktock, ktime_set(w->timeout, 0),
+	hrtimer_start(&softdog_ticktock, ms_to_ktime(w->timeout),
 		      HRTIMER_MODE_REL);
 
 	if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT)) {
 		if (w->pretimeout)
 			hrtimer_start(&softdog_preticktock,
-				      ktime_set(w->timeout - w->pretimeout, 0),
+				      ms_to_ktime(w->timeout - w->pretimeout),
 				      HRTIMER_MODE_REL);
 		else
 			hrtimer_cancel(&softdog_preticktock);
@@ -110,7 +110,8 @@ static int softdog_stop(struct watchdog_device *w)
 
 static struct watchdog_info softdog_info = {
 	.identity = "Software Watchdog",
-	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+	.options = (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE |
+		    WDIOF_MSECTIMER),
 };
 
 static const struct watchdog_ops softdog_ops = {
@@ -123,15 +124,15 @@ static struct watchdog_device softdog_dev = {
 	.info = &softdog_info,
 	.ops = &softdog_ops,
 	.min_timeout = 1,
-	.max_timeout = 65535,
-	.timeout = TIMER_MARGIN,
+	.max_timeout = 65535000,
+	.timeout = TIMER_MARGIN * 1000,
 };
 
 static int __init softdog_init(void)
 {
 	int ret;
 
-	watchdog_init_timeout(&softdog_dev, soft_margin, NULL);
+	watchdog_init_timeout(&softdog_dev, soft_margin * 1000, NULL);
 	watchdog_set_nowayout(&softdog_dev, nowayout);
 	watchdog_stop_on_reboot(&softdog_dev);
 
-- 
2.17.1