Hi Guenter,
Great thanks for your review,
On 3 June 2015 at 00:12, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On 06/01/2015 09:05 PM, fu.wei@xxxxxxxxxx wrote:
>>
>> From: Fu Wei <fu.wei@xxxxxxxxxx>
>>
>> Also update Documentation/watchdog/watchdog-kernel-api.txt to
>> introduce:
>> (1)the new elements in the watchdog_device and watchdog_ops struct;
>> (2)the new API "watchdog_init_timeouts"
>>
>> Reasons:
>> (1)kernel already has two watchdog drivers are using "pretimeout":
>> drivers/char/ipmi/ipmi_watchdog.c
>> drivers/watchdog/kempld_wdt.c(but the definition is different)
>> (2)some other drivers are going to use this: ARM SBSA Generic Watchdog
>>
>> Signed-off-by: Fu Wei <fu.wei@xxxxxxxxxx>
>> ---
>> Documentation/watchdog/watchdog-kernel-api.txt | 47 ++++++++--
>> drivers/watchdog/watchdog_core.c | 115
>> +++++++++++++++++++------
>> drivers/watchdog/watchdog_dev.c | 53 ++++++++++++
>> include/linux/watchdog.h | 33 ++++++-
>> 4 files changed, 212 insertions(+), 36 deletions(-)
>>
>> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt
>> b/Documentation/watchdog/watchdog-kernel-api.txt
>> index a0438f3..95b355d 100644
>> --- a/Documentation/watchdog/watchdog-kernel-api.txt
>> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
>> @@ -49,6 +49,9 @@ struct watchdog_device {
>> unsigned int timeout;
>> unsigned int min_timeout;
>> unsigned int max_timeout;
>> + unsigned int pretimeout;
>> + unsigned int min_pretimeout;
>> + unsigned int max_pretimeout;
>> void *driver_data;
>> struct mutex lock;
>> unsigned long status;
>> @@ -70,6 +73,9 @@ It contains following fields:
>> * timeout: the watchdog timer's timeout value (in seconds).
>> * min_timeout: the watchdog timer's minimum timeout value (in seconds).
>> * max_timeout: the watchdog timer's maximum timeout value (in seconds).
>> +* pretimeout: the watchdog timer's pretimeout value (in seconds).
>> +* min_pretimeout: the watchdog timer's minimum pretimeout value (in
>> seconds).
>> +* max_pretimeout: the watchdog timer's maximum pretimeout value (in
>> seconds).
>> * bootstatus: status of the device after booting (reported with watchdog
>> WDIOF_* status bits).
>> * driver_data: a pointer to the drivers private data of a watchdog
>> device.
>> @@ -92,6 +98,7 @@ struct watchdog_ops {
>> int (*ping)(struct watchdog_device *);
>> unsigned int (*status)(struct watchdog_device *);
>> int (*set_timeout)(struct watchdog_device *, unsigned int);
>> + int (*set_pretimeout)(struct watchdog_device *, unsigned int);
>> unsigned int (*get_timeleft)(struct watchdog_device *);
>> void (*ref)(struct watchdog_device *);
>> void (*unref)(struct watchdog_device *);
>> @@ -153,9 +160,19 @@ they are supported. These optional
>> routines/operations are:
>> and -EIO for "could not write value to the watchdog". On success this
>> routine should set the timeout value of the watchdog_device to the
>> achieved timeout value (which may be different from the requested one
>> - because the watchdog does not necessarily has a 1 second resolution).
>> + because the watchdog does not necessarily has a 1 second resolution;
>> + If the driver supports pretimeout, then the timeout value must be
>> greater
>> + than that).
>> (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of
>> the
>> watchdog's info structure).
>> +* set_pretimeout: this routine checks and changes the pretimeout of the
>> + watchdog timer device. It returns 0 on success, -EINVAL for "parameter
>> out of
>> + range" and -EIO for "could not write value to the watchdog". On success
>> this
>> + routine should set the pretimeout value of the watchdog_device to the
>> + achieved pretimeout value (which may be different from the requested
>> one
>> + because the watchdog does not necessarily has a 1 second resolution).
>> + (Note: the WDIOF_PRETIMEOUT needs to be set in the options field of the
>> + watchdog's info structure).
>> * get_timeleft: this routines returns the time that's left before a
>> reset.
>> * ref: the operation that calls kref_get on the kref of a dynamically
>> allocated watchdog_device struct.
>> @@ -219,8 +236,28 @@ extern int watchdog_init_timeout(struct
>> watchdog_device *wdd,
>> unsigned int timeout_parm, struct
>> device *dev);
>>
>> The watchdog_init_timeout function allows you to initialize the timeout
>> field
>> -using the module timeout parameter or by retrieving the timeout-sec
>> property from
>> -the device tree (if the module timeout parameter is invalid). Best
>> practice is
>> -to set the default timeout value as timeout value in the watchdog_device
>> and
>> -then use this function to set the user "preferred" timeout value.
>> +using the module timeout parameter or by retrieving the first element of
>> +the timeout-sec property from the device tree (if the module timeout
>> parameter
>> +is invalid). Best practice is to set the default timeout value as timeout
>> value
>> +in the watchdog_device and then use this function to set the user
>> "preferred"
>> +timeout value.
>> +This routine returns zero on success and a negative errno code for
>> failure.
>> +
>> +Some watchdog timers have two stage of timeouts(timeout and pretimeout),
>> +to initialize the timeout and pretimeout fields at the same time, the
>> following
>> +function can be used:
>> +
>> +extern int watchdog_init_timeouts(struct watchdog_device *wdd,
>> + unsigned int pretimeout_parm,
>> + unsigned int timeout_parm,
>> + struct device *dev);
>> +
>> +The watchdog_init_timeouts function allows you to initialize the
>> pretimeout and
>> +timeout fields using the module pretimeout and timeout parameter or by
>> +retrieving the elements in the timeout-sec property(the first element is
>> for
>> +timeout, the second one is for pretimeout) from the device tree(if the
>> module
>> +pretimeout and timeout parameter are invalid).
>> +Best practice is to set the default pretimeout and timeout value as
>> pretimeout
>> +and timeout value in the watchdog_device and then use this function to
>> set the
>> +user "preferred" pretimeout value.
>> This routine returns zero on success and a negative errno code for
>> failure.
>> diff --git a/drivers/watchdog/watchdog_core.c
>> b/drivers/watchdog/watchdog_core.c
>> index cec9b55..ff18db3 100644
>> --- a/drivers/watchdog/watchdog_core.c
>> +++ b/drivers/watchdog/watchdog_core.c
>> @@ -43,60 +43,119 @@
>> static DEFINE_IDA(watchdog_ida);
>> static struct class *watchdog_class;
>>
>> -static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
>> +static void watchdog_check_min_max_timeouts(struct watchdog_device *wdd)
>> {
>> /*
>> - * Check that we have valid min and max timeout values, if
>> - * not reset them both to 0 (=not used or unknown)
>> + * Check that we have valid min and max pretimeout and timeout
>> values,
>> + * if not reset them both to 0 (=not used or unknown)
>> */
>> + if (wdd->min_pretimeout > wdd->max_pretimeout) {
>> + pr_info("Invalid min and max pretimeout, resetting to
>> 0\n");
>> + wdd->min_pretimeout = 0;
>> + wdd->max_pretimeout = 0;
>> + }
>> if (wdd->min_timeout > wdd->max_timeout) {
>> pr_info("Invalid min and max timeout values, resetting to
>> 0!\n");
>> wdd->min_timeout = 0;
>> wdd->max_timeout = 0;
>> }
>> + /*
>> + * Check that we have valid min timeout and max pretimeout values,
>> + * if not reset them.
>> + */
>> + if (wdd->min_pretimeout && wdd->min_timeout < wdd->min_pretimeout)
>> {
>> + pr_info("Invalid min_timeout, resetting to
>> min_pretimeout+1\n");
>> + wdd->min_timeout = wdd->min_pretimeout + 1;
>> + }
>
>
> min_timeout = 10
> max_timeout = 20
> min_pretimeout = 30
> max_pretimeout = 40
>
> will result in min_timeout set to 31.
>
>> + if (wdd->max_pretimeout && wdd->max_timeout < wdd->max_pretimeout)
>> {
>> + pr_info("Invalid max_pretimeout, resetting to
>> max_timeout-1\n");
>> + wdd->max_pretimeout = wdd->max_timeout - 1;
>
>
> and then you set max_pretimeout to 19.
>
> So we'll end up with
>
> min_timeout = 31
> max_timeout = 20
> min_pretimeout = 30
> max_pretimeout = 19
>
> Another example: max_pretimeout = 10, max_timeout = 0 -> max_pretimeout =
> -1.
>
>> + }
>
>
> You'll need to determine valid ranges and then enforce those.
> Maybe something like
> min_pretimeout < min_timeout < max_timeout
> and
> min_pretimeout < max_pretimeout < max_timeout
>
> Not sure if we should adjust anything to a value different to 0.
> Seems to me this is just asking for trouble.
yes, this is a new problem, but let me fix this in my new patchset:
(1)these values is set by driver in the initialization procedure,
according to hardware info.
(2) I thinks there is not any driver should call this function after
initialization.
So if we find any incorrect value, I think the hardware info we got
is wrong , maybe we can just give up
any suggestion ?
>
>> }
>>
>> /**
>> - * watchdog_init_timeout() - initialize the timeout field
>> + * watchdog_init_timeouts() - initialize the pretimeout and timeout field
>> + * @pretimeout_parm: pretimeout module parameter
>> * @timeout_parm: timeout module parameter
>> * @dev: Device that stores the timeout-sec property
>> *
>> - * Initialize the timeout field of the watchdog_device struct with either
>> the
>> - * timeout module parameter (if it is valid value) or the timeout-sec
>> property
>> - * (only if it is a valid value and the timeout_parm is out of bounds).
>> - * If none of them are valid then we keep the old value (which should
>> normally
>> - * be the default timeout value.
>> + * Initialize the pretimeout and timeout field of the watchdog_device
>> struct
>> + * with either the pretimeout and timeout module parameter (if it is
>> valid
>> + * value) or the timeout-sec property (only if it is a valid value and
>> the
>
>
> just 'if it is valid', or 'if it is a valid value' in both cases.
OK. np,. Thanks
>
>> + * pretimeout_parm and timeout_parm is out of bounds). If none of them
>> are
>
>
> s/and/or/
Thanks , fixed
>
>
>> + * valid, then we keep the old value (which should normally be the
>> default
>> + * timeout value).
>> *
>> * A zero is returned on success and -EINVAL for failure.
>> */
>> -int watchdog_init_timeout(struct watchdog_device *wdd,
>> - unsigned int timeout_parm, struct device
>> *dev)
>> +int watchdog_init_timeouts(struct watchdog_device *wdd,
>> + unsigned int pretimeout_parm,
>> + unsigned int timeout_parm,
>> + struct device *dev)
>> {
>> - unsigned int t = 0;
>> - int ret = 0;
>> + int ret = 0, length = 0;
>> + u32 timeouts[2] = {0};
>>
>> - watchdog_check_min_max_timeout(wdd);
>> + watchdog_check_min_max_timeouts(wdd);
>>
>> - /* try to get the timeout module parameter first */
>> - if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm)
>> {
>> - wdd->timeout = timeout_parm;
>> - return ret;
>> + /*
>> + * Try to get the pretimeout module parameter first,
>> + * but it can be "0", that means we don't use pretimeout.
>> + */
>> + if (pretimeout_parm) {
>> + if (!watchdog_pretimeout_invalid(wdd, pretimeout_parm))
>> + timeouts[1] = pretimeout_parm;
>> + ret = -EINVAL; /* pretimeout_parm is not "0", and invalid
>> */
>
>
> pretimeout_parm is always invalid ? Why ?
Sorry, I lost a "else" , this is a bug/typo
>
>> }
>> - if (timeout_parm)
>> +
>> + /*
>> + * Try to get the timeout module parameter,
>> + * if it's valid and pretimeout is ont invalid(ret == 0),
>
>
> s/ont/not/
Thanks , fixed
>
>> + * assignment and return zero. Otherwise, try dtb.
>> + */
>> + if (timeout_parm) {
>> + if (!watchdog_timeout_invalid(wdd, timeout_parm) && !ret)
>> {
>
>
> Unless I am missing something, you'll never get here if the pretimeout
> module
> parameter is set.
yes, because of the bug above.
Will fix it in my next patchset , thanks :-)
>
>> + wdd->timeout = timeout_parm;
>> + wdd->pretimeout = timeouts[1];
>> + return 0;
>> + }
>> ret = -EINVAL;
>> + }
>>
>> - /* try to get the timeout_sec property */
>> + /*
>> + * We get here, the situation is one of them:
>> + * (1)at least one of parameters is invalid(ret = -EINVAL);
>> + * (2)both of them are 0.(ret = 0)
>> + * So give up the module parameter(at least drop the invalid one),
>> + * try to get the timeout_sec property from dtb.
>
>
> Please simplify the comment.
>
> Either at least one of the module parameters is invalid,
> or both of them are 0. Try to get the timeout_sec property.
Thanks , will do.
>
>> + */
>> if (dev == NULL || dev->of_node == NULL)
>> return ret;
>> - of_property_read_u32(dev->of_node, "timeout-sec", &t);
>> - if (!watchdog_timeout_invalid(wdd, t) && t)
>> - wdd->timeout = t;
>> - else
>> - ret = -EINVAL;
>>
>> - return ret;
>> + /*
>> + * We get here, that means maybe we can get timeouts from dtb,
>> + * if "timeout-sec" is available and the data is valid.
>> + */
>
>
> That comment is not needed; it is obvious.
OK, will remove it
>
>> + of_find_property(dev->of_node, "timeout-sec", &length);
>> + if (length > 0 && length <= sizeof(u32) * 2) {
>
>
> You need to check the return value of of_find_property() instead of assuming
> that length will be 0 if it is not found.
you are right, will do, thanks
>
>> + of_property_read_u32_array(dev->of_node,
>> + "timeout-sec", timeouts,
>> + length / sizeof(u32));
>> + if (length == sizeof(u32) * 2 && timeouts[1] &&
>> + watchdog_pretimeout_invalid(wdd, timeouts[1]))
>> + return -EINVAL; /* pretimeout is invalid
>> */
>
>
> Obvious comment.
OK, will remove it
>
>
>> +
>> + if (!watchdog_timeout_invalid(wdd, timeouts[0]) &&
>> + timeouts[0]) {
>> + wdd->timeout = timeouts[0];
>> + wdd->pretimeout = timeouts[1];
>> + return 0;
>> + }
>> + }
>> +
>> + return -EINVAL;
>> }
>> -EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>> +EXPORT_SYMBOL_GPL(watchdog_init_timeouts);
>>
>> /**
>> * watchdog_register_device() - register a watchdog device
>> @@ -119,7 +178,7 @@ int watchdog_register_device(struct watchdog_device
>> *wdd)
>> if (wdd->ops->start == NULL || wdd->ops->stop == NULL)
>> return -EINVAL;
>>
>> - watchdog_check_min_max_timeout(wdd);
>> + watchdog_check_min_max_timeouts(wdd);
>>
>> /*
>> * Note: now that all watchdog_device data has been verified, we
>> diff --git a/drivers/watchdog/watchdog_dev.c
>> b/drivers/watchdog/watchdog_dev.c
>> index 6aaefba..af0777e 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -218,6 +218,38 @@ out_timeout:
>> }
>>
>> /*
>> + * watchdog_set_pretimeout: set the watchdog timer pretimeout
>> + * @wddev: the watchdog device to set the timeout for
>> + * @pretimeout: pretimeout to set in seconds
>> + */
>> +
>> +static int watchdog_set_pretimeout(struct watchdog_device *wddev,
>> + unsigned int pretimeout)
>> +{
>> + int err;
>> +
>> + if (!wddev->ops->set_pretimeout ||
>> + !(wddev->info->options & WDIOF_PRETIMEOUT))
>> + return -EOPNOTSUPP;
>> +
>> + if (watchdog_pretimeout_invalid(wddev, pretimeout))
>> + return -EINVAL;
>> +
>> + mutex_lock(&wddev->lock);
>> +
>> + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
>> + err = -ENODEV;
>> + goto out_pretimeout;
>> + }
>> +
>> + err = wddev->ops->set_pretimeout(wddev, pretimeout);
>> +
>> +out_pretimeout:
>> + mutex_unlock(&wddev->lock);
>> + return err;
>> +}
>> +
>> +/*
>> * watchdog_get_timeleft: wrapper to get the time left before a
>> reboot
>> * @wddev: the watchdog device to get the remaining time from
>> * @timeleft: the time that's left
>> @@ -388,6 +420,27 @@ static long watchdog_ioctl(struct file *file,
>> unsigned int cmd,
>> if (wdd->timeout == 0)
>> return -EOPNOTSUPP;
>> return put_user(wdd->timeout, p);
>> + case WDIOC_SETPRETIMEOUT:
>> + /* check if we support the pretimeout */
>> + if (!(wdd->info->options & WDIOF_PRETIMEOUT))
>> + return -EOPNOTSUPP;
>> + if (get_user(val, p))
>> + return -EFAULT;
>> + err = watchdog_set_pretimeout(wdd, val);
>> + if (err < 0)
>> + return err;
>> + /*
>> + * If the watchdog is active then we send a keepalive ping
>> + * to make sure that the watchdog keeps running (and if
>> + * possible that it takes the new pretimeout)
>> + */
>> + watchdog_ping(wdd);
>> + /* Fall */
>> + case WDIOC_GETPRETIMEOUT:
>> + /* check if we support the pretimeout */
>> + if (wdd->info->options & WDIOF_PRETIMEOUT)
>> + return put_user(wdd->pretimeout, p);
>> + return -EOPNOTSUPP;
>> case WDIOC_GETTIMELEFT:
>> err = watchdog_get_timeleft(wdd, &val);
>> if (err)
>> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
>> index a746bf5..b1e325d 100644
>> --- a/include/linux/watchdog.h
>> +++ b/include/linux/watchdog.h
>> @@ -25,6 +25,7 @@ struct watchdog_device;
>> * @ping: The routine that sends a keepalive ping to the watchdog
>> device.
>> * @status: The routine that shows the status of the watchdog device.
>> * @set_timeout:The routine for setting the watchdog devices timeout
>> value.
>> + * @set_pretimeout:The routine for setting the watchdog devices
>> pretimeout value
>> * @get_timeleft:The routine that get's the time that's left before a
>> reset.
>> * @ref: The ref operation for dyn. allocated watchdog_device
>> structs
>> * @unref: The unref operation for dyn. allocated watchdog_device
>> structs
>> @@ -44,6 +45,7 @@ struct watchdog_ops {
>> int (*ping)(struct watchdog_device *);
>> unsigned int (*status)(struct watchdog_device *);
>> int (*set_timeout)(struct watchdog_device *, unsigned int);
>> + int (*set_pretimeout)(struct watchdog_device *, unsigned int);
>> unsigned int (*get_timeleft)(struct watchdog_device *);
>> void (*ref)(struct watchdog_device *);
>> void (*unref)(struct watchdog_device *);
>> @@ -62,6 +64,9 @@ struct watchdog_ops {
>> * @timeout: The watchdog devices timeout value.
>> * @min_timeout:The watchdog devices minimum timeout value.
>> * @max_timeout:The watchdog devices maximum timeout value.
>> + * @pretimeout: The watchdog devices pretimeout value.
>> + * @min_pretimeout:The watchdog devices minimum pretimeout value.
>> + * @max_pretimeout:The watchdog devices maximum pretimeout value.
>> * @driver-data:Pointer to the drivers private data.
>> * @lock: Lock for watchdog core internal use only.
>> * @status: Field that contains the devices internal status bits.
>> @@ -86,6 +91,9 @@ struct watchdog_device {
>> unsigned int timeout;
>> unsigned int min_timeout;
>> unsigned int max_timeout;
>> + unsigned int pretimeout;
>> + unsigned int min_pretimeout;
>> + unsigned int max_pretimeout;
>> void *driver_data;
>> struct mutex lock;
>> unsigned long status;
>> @@ -117,7 +125,17 @@ static inline void watchdog_set_nowayout(struct
>> watchdog_device *wdd, bool noway
>> static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd,
>> unsigned int t)
>> {
>> return ((wdd->max_timeout != 0) &&
>> - (t < wdd->min_timeout || t > wdd->max_timeout));
>> + (t < wdd->min_timeout || t > wdd->max_timeout ||
>> + t <= wdd->pretimeout));
>> +}
>> +
>> +/* Use the following function to check if a pretimeout value is invalid
>> */
>> +static inline bool watchdog_pretimeout_invalid(struct watchdog_device
>> *wdd,
>> + unsigned int t)
>> +{
>> + return (wdd->max_pretimeout != 0 &&
>> + (t < wdd->min_pretimeout || t > wdd->max_pretimeout ||
>> + (wdd->timeout != 0 && t >= wdd->timeout)));
>
>
> This validates a pretimeout as valid if max_pretimeout == 0,
> no matter what timeout is set to.
>
> Do we really need to check if timeout == 0 ? Can that ever happen ?
(wdd->timeout != 0 && t >= wdd->timeout)
if we have set timeout(wdd->timeout != 0), and t(pretimeout) >
wdd->timeout, t is a invalid value.
Am I correct?
the reason of adding "wdd->timeout != 0": if driver calls this
function before setting up wdd->timeout.
>
>> }
>>
>> /* Use the following functions to manipulate watchdog driver specific
>> data */
>> @@ -132,11 +150,20 @@ static inline void *watchdog_get_drvdata(struct
>> watchdog_device *wdd)
>> }
>>
>> /* drivers/watchdog/watchdog_core.c */
>> -extern int watchdog_init_timeout(struct watchdog_device *wdd,
>> - unsigned int timeout_parm, struct device
>> *dev);
>> +extern int watchdog_init_timeouts(struct watchdog_device *wdd,
>> + unsigned int pretimeout_parm,
>> + unsigned int timeout_parm,
>> + struct device *dev);
>
>
> No 'extern' here, please. Yes, we'll need to fix that for the other
> function declarations, but that is not a reason to introduce new ones.
OK, I do agree with you on this, have fixed it in my next patchset.
>
> Thanks,
> Guenter
>
--
Best regards,
Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
