Re: [PATCH 1/2] Add a simple watchdog framework

[Juergen Beisert <jbe@pengutronix.de>]

Hi Sascha,

Sascha Hauer wrote:
> [...]
> > > > +
> > > > +	rc = watchdog_set_timeout(timeout);
> > > > +	if (rc == -EINVAL) {
> > >
> > > Why do you check for -EINVAL only? This way all other errors will be
> > > silently ignored.
> >
> > Are you sure we must handle other failure codes than -EINVAL here? The
> > called function is very simple and only must distinguish "timeout != 0"
> > and "timeout == 0". So, timeout can be "out of range" or - as you
> > mentioned - some platforms cannot disable the watchdog anymore once it is
> > enabled. Both results into -EINVAL, because the called function cannot
> > use the given value. What else can happen?
>
> Why should we only test for errors that we know and silently drop all
> others?

+1

> Somebody might think that -ENOSYS is the appropriate return value if the
> watchdog can't be disabled. When such a patch is added nobody will
> remember that our error handling only checks for a special error value.

If we accept every possible error value, then we also must move the readable 
error message into the called function (because only the called function 
knows the meaning of its returned value).
Otherwise we must force a specific return-value to give a correct error 
message to the user. But if we force a specific return-value (-EINVAL for 
example) then the -ENOSYS would violate the defined API.

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox