| Message ID | 20200620173351.18752-2-minyard@acm.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: Add millisecond-level capabilities | expand |
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 */ >
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 */ > > >
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 */ >>> >>
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
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
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
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
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 */