[PATCH 1/2] watchdog: core: Make dt "timeout-sec" property work on drivers w/out min/max

[PATCH 1/2] watchdog: core: Make dt "timeout-sec" property work on drivers w/out min/max

[Doug Anderson]

It is valid for a watchdog driver to have 0 for a "min" and "max"
timeout if the driver doesn't need the core to enforce the concepts of
min and max.  The s3c2410_wdt driver is one such driver.  Specifically
it can be hard for that driver to come up with a static "max" on all
platforms without a lot more information since the input clock on
S3C2410 and S3C2440 can change with DVFS.

As written, watchdog_init_timeout() will not ever read "timeout-sec"
on these drivers since watchdog_timeout_invalid() will _never_ return
true.  Change to not consider a timeout_parm of 0 as valid even if
min/max aren't specified by the driver.  Also handle the case when
there is no min/max and no "timeout-sec" property.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/watchdog/watchdog_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 461336c..cec9b55 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -78,7 +78,7 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 	watchdog_check_min_max_timeout(wdd);
 
 	/* try to get the timeout module parameter first */
-	if (!watchdog_timeout_invalid(wdd, timeout_parm)) {
+	if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm) {
 		wdd->timeout = timeout_parm;
 		return ret;
 	}
@@ -89,7 +89,7 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 	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))
+	if (!watchdog_timeout_invalid(wdd, t) && t)
 		wdd->timeout = t;
 	else
 		ret = -EINVAL;
-- 
1.8.4.1

[PATCH 2/2] watchdog: core: Fix watchdog_init_timeout() when invalid param / valid dt

[Doug Anderson]

There was a minor bug in watchdog_init_timeout() where it would return
an error code if someone specified an invalid parameter on the
command line but then there was a valid parameter in the device tree
as "timeout-sec".

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/watchdog/watchdog_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index cec9b55..8d27753 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -82,12 +82,12 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 		wdd->timeout = timeout_parm;
 		return ret;
 	}
-	if (timeout_parm)
-		ret = -EINVAL;
 
-	/* try to get the timeout_sec property */
+	/* if no device tree then we're done */
 	if (dev == NULL || dev->of_node == NULL)
-		return ret;
+		return (timeout_parm) ? -EINVAL : ret;
+
+	/* try to get the timeout_sec property */
 	of_property_read_u32(dev->of_node, "timeout-sec", &t);
 	if (!watchdog_timeout_invalid(wdd, t) && t)
 		wdd->timeout = t;
-- 
1.8.4.1

Re: [PATCH 2/2] watchdog: core: Fix watchdog_init_timeout() when invalid param / valid dt

[Guenter Roeck]

On 11/26/2013 10:22 AM, Doug Anderson wrote:
> There was a minor bug in watchdog_init_timeout() where it would return
> an error code if someone specified an invalid parameter on the
> command line but then there was a valid parameter in the device tree
> as "timeout-sec".
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>

I thought that was on purpose.

Problem as I see it is that users would expect the timeout to be set to
the provided parameter, which would be silently ignored and replaced
by timeout-sec if the parameter is wrong and timeout-sec is specified.
Seems to me that the user should be informed about the problem,
and not be permitted to provide invalid parameters.

Thanks,
Guenter

> ---
>   drivers/watchdog/watchdog_core.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index cec9b55..8d27753 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -82,12 +82,12 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>   		wdd->timeout = timeout_parm;
>   		return ret;
>   	}
> -	if (timeout_parm)
> -		ret = -EINVAL;
>
> -	/* try to get the timeout_sec property */
> +	/* if no device tree then we're done */
>   	if (dev == NULL || dev->of_node == NULL)
> -		return ret;
> +		return (timeout_parm) ? -EINVAL : ret;

( ) is unnecessary.

> +
> +	/* try to get the timeout_sec property */
>   	of_property_read_u32(dev->of_node, "timeout-sec", &t);
>   	if (!watchdog_timeout_invalid(wdd, t) && t)
>   		wdd->timeout = t;
>

Re: [PATCH 1/2] watchdog: core: Make dt "timeout-sec" property work on drivers w/out min/max

[Guenter Roeck]

On 11/26/2013 10:22 AM, Doug Anderson wrote:
> It is valid for a watchdog driver to have 0 for a "min" and "max"
> timeout if the driver doesn't need the core to enforce the concepts of
> min and max.  The s3c2410_wdt driver is one such driver.  Specifically
> it can be hard for that driver to come up with a static "max" on all
> platforms without a lot more information since the input clock on
> S3C2410 and S3C2440 can change with DVFS.
>
> As written, watchdog_init_timeout() will not ever read "timeout-sec"
> on these drivers since watchdog_timeout_invalid() will _never_ return
> true.  Change to not consider a timeout_parm of 0 as valid even if
> min/max aren't specified by the driver.  Also handle the case when
> there is no min/max and no "timeout-sec" property.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Makes sense to me.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Guenter

Re: [PATCH 2/2] watchdog: core: Fix watchdog_init_timeout() when invalid param / valid dt

[Doug Anderson]

Guenter,

Thanks for your reviews!

On Tue, Nov 26, 2013 at 10:58 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 11/26/2013 10:22 AM, Doug Anderson wrote:
>>
>> There was a minor bug in watchdog_init_timeout() where it would return
>> an error code if someone specified an invalid parameter on the
>> command line but then there was a valid parameter in the device tree
>> as "timeout-sec".
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>
>
> I thought that was on purpose.
>
> Problem as I see it is that users would expect the timeout to be set to
> the provided parameter, which would be silently ignored and replaced
> by timeout-sec if the parameter is wrong and timeout-sec is specified.
> Seems to me that the user should be informed about the problem,
> and not be permitted to provide invalid parameters.

Wow, really?  ...so it's on purpose that the function will properly
read the device tree entry and fill it in but still return an error?

I guess that can make some sense (treating device tree as an extension
of the "default"), though it's non-obvious enough to me that it feels
like it deserves some documentation.  I'd also question the value of
the return code from this function anyway.  I'd vote for:

1. If param is non-zero and invalid, dev_warn() in this function.

2. If "timeout-sec" is specified in device tree and invalid,
dev_warn() in this function.

Function doesn't need to return an error code.  ...or if we keep it
then nobody should be looking at it.  They should be putting their
default in "timeout" before calling the function and trusting that the
function will do the right thing and update it as needed.


In practice only one caller ever checks this result in the code I'm
looking at (at91sam9_wdt) and it's a little confusing what that's
trying to do.  It does look like it would be broken by my suggestions
above.  I guess it's trying to do:
1. device tree first (always passes 0 as the "param")
2. a value based on the patting heartbeat second.
3. the value WDT_HEARTBEAT third (starts in ->timeout)


In any case I'm OK with just dropping this patch.  The code looked
wrong and so I thought I'd fix it up, but I have no real need to see
it land (we don't use kernel parameters for things like this) in
Chrome OS.  I'm also happy to spin it if there is some interest.


-Doug

Re: [PATCH 2/2] watchdog: core: Fix watchdog_init_timeout() when invalid param / valid dt

[Guenter Roeck]

On 11/26/2013 11:23 AM, Doug Anderson wrote:
> Guenter,
>
> Thanks for your reviews!
>
> On Tue, Nov 26, 2013 at 10:58 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 11/26/2013 10:22 AM, Doug Anderson wrote:
>>>
>>> There was a minor bug in watchdog_init_timeout() where it would return
>>> an error code if someone specified an invalid parameter on the
>>> command line but then there was a valid parameter in the device tree
>>> as "timeout-sec".
>>>
>>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>>
>>
>> I thought that was on purpose.
>>
>> Problem as I see it is that users would expect the timeout to be set to
>> the provided parameter, which would be silently ignored and replaced
>> by timeout-sec if the parameter is wrong and timeout-sec is specified.
>> Seems to me that the user should be informed about the problem,
>> and not be permitted to provide invalid parameters.
>
> Wow, really?  ...so it's on purpose that the function will properly
> read the device tree entry and fill it in but still return an error?
>
I said "I thought ...", which wasn't meant to imply that I know.
Maybe Wim should comment and provide directions.

> I guess that can make some sense (treating device tree as an extension
> of the "default"), though it's non-obvious enough to me that it feels
> like it deserves some documentation.  I'd also question the value of
> the return code from this function anyway.  I'd vote for:
>
> 1. If param is non-zero and invalid, dev_warn() in this function.
>
> 2. If "timeout-sec" is specified in device tree and invalid,
> dev_warn() in this function.
>
Makes sense to me. Again up to Wim to provide direction.

> Function doesn't need to return an error code.  ...or if we keep it
> then nobody should be looking at it.  They should be putting their
> default in "timeout" before calling the function and trusting that the
> function will do the right thing and update it as needed.
>
>
> In practice only one caller ever checks this result in the code I'm
> looking at (at91sam9_wdt) and it's a little confusing what that's
> trying to do.  It does look like it would be broken by my suggestions
> above.  I guess it's trying to do:
> 1. device tree first (always passes 0 as the "param")
> 2. a value based on the patting heartbeat second.
> 3. the value WDT_HEARTBEAT third (starts in ->timeout)
>
I thought the idea was to give drivers the ability to handle errors
this way. Notice the "thought" ... I may be wrong.

>
> In any case I'm OK with just dropping this patch.  The code looked
> wrong and so I thought I'd fix it up, but I have no real need to see
> it land (we don't use kernel parameters for things like this) in
> Chrome OS.  I'm also happy to spin it if there is some interest.
>

It is really be up to Wim to decide what to do, so I'll defer to him.

Thanks,
Guenter