Messages in this thread | | | Date | Tue, 26 Nov 2013 11:51:49 -0800 | From | Guenter Roeck <> | Subject | Re: [PATCH 2/2] watchdog: core: Fix watchdog_init_timeout() when invalid param / valid dt |
| |
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
| |