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

[Doug Anderson]

Guenter,

Thanks for your reviews!

On Tue, Nov 26, 2013 at 10:58 AM, Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/