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@xxxxxxx wrote:
>>> From: Corey Minyard <cminyard@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> ---
>>> 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 */
>>>
>>
