On Wed, Aug 11, 2021 at 02:29:12PM +0200, Wolfgang Denk wrote:
Dear Rasmus,
In message 3d48015a-07d3-e296-b9ba-a1edd455ce9e@prevas.dk you wrote:
+ if (ret) { + log_debug("Error getting UCLASS_WDT: %d\n", ret);
Perhaps log_err()?
No, we've already been over this in earlier discussions (it's the exact same pattern and reasoning as initr_watchdog). If I made it log_err(), it would cost .text for something that never-ever happens in practice, while log_debug() is usually a no-op, but can be compiled in if something truly fishy seems to be going on.
This argument fits on all types or effors: they are supposed to never ever happen - at least in theory; in reality they do, and more often than we like.
And a proper error message is mandatory for correct error handling.
Error messages are a fine line to walk. We've got to have been very badly corrupted to go down this error path. There's going to be lots of other error messages popping out. Saving a bit of .text is good. It makes it easier to justify spending a little .text later.
Looks good, thanks for quickly working on this. Not sure, if this new function should be "void" or better "int" so that the error can be returned.
That's why I included my tentative commit log, so you could see my explanation for why I made it void. Until some user shows up that _wants_ a return value, there's no point making it return int. When that user shows up, we can discuss which int (return early on failure? remember that an error was seen but still call wdt_stop on remaining devices? etc. etc.).
Returning an error code is always a good ide, no matter if current users check it or not.
And here I agree, catch an error code, pass the error code back to the caller. That's far more important than making sure that every error code we catch logs a message by default every time.