Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: minyard@acm.org, Wim Van Sebroeck <wim@linux-watchdog.org>
Cc: linux-watchdog@vger.kernel.org,
	Gabriele Paoloni <gabriele.paoloni@intel.com>,
	Corey Minyard <cminyard@mvista.com>
Subject: Re: [PATCH 1/6] watchdog: Allow a driver to use milliseconds instead of seconds
Date: Fri, 26 Jun 2020 16:10:41 -0700
Message-ID: <c29b1018-52ba-f118-b955-9df07f2357b9@roeck-us.net> (raw)
In-Reply-To: <20200620173351.18752-2-minyard@acm.org>

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 */
> 


  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-20 17:33 [PATCH 0/6] watchdog: Add millisecond-level capabilities minyard
2020-06-20 17:33 ` [PATCH 1/6] watchdog: Allow a driver to use milliseconds instead of seconds minyard
2020-06-26 23:10   ` Guenter Roeck [this message]
2020-06-27  2:18     ` Corey Minyard
2020-06-27  4:08       ` Guenter Roeck
2020-06-28 14:54   ` Guenter Roeck
2020-06-28 17:56     ` Corey Minyard
2020-06-20 17:33 ` [PATCH 2/6] watchdog: Add ioctls for millisecond timeout handling minyard
2020-06-20 17:33 ` [PATCH 3/6] watchdog: Add millisecond precision device attributes minyard
2020-06-20 17:33 ` [PATCH 4/6] watchdog: Add documentation for millisecond interfaces minyard
2020-06-20 17:33 ` [PATCH 5/6] watchdog:i6300: Convert to a millisecond watchdog device minyard
2020-06-20 17:33 ` [PATCH 6/6] watchdog:softdog: Convert to a millisecond watchdog minyard

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c29b1018-52ba-f118-b955-9df07f2357b9@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=cminyard@mvista.com \
    --cc=gabriele.paoloni@intel.com \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=minyard@acm.org \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git