Re: [PATCH 2/2] watchdog: core: Fix watchdog_init_timeout() wheninvalid 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@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 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

--
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/