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

[Guenter Roeck <linux@roeck-us.net>]

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