[PATCH v4 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Stefan Roese]

On 12.08.21 15:48, Tom Rini wrote:
> On Thu, Aug 12, 2021 at 08:40:21AM +0200, Wolfgang Denk wrote:
>> Dear Tom,
>>
>> In message <20210811124318.GT858 at bill-the-cat> you wrote:
>>>
>>>> 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.
>>
>> Letting errors slip through unnoticed when there is a trival way to
>> at least inform the user of the problem is simply unacceptable.
>>
>> Please do not let U-Boot degrade into such a crappy piece of code.
>>
>> There are tons of other places where we don't even mention code
>> size, so if you want to save on that, there are many bette4r places
>> to save than error handling.
> 
> Alright, lets take a look at what kind of area of the code we're talking
> about.  uclass_get is a pretty fundamental thing.  If that fails, your
> system is on fire.  Things are massively corrupt.  Lets look at other
> existing callers to see what happens.  Most callers check the return
> code, like you need to, and pass it up the chain to deal with.  We have
> a few board specific ones such as
> board/Marvell/octeontx2/board.c::board_quiesce_devices() that is also
> conceptually like the x530 case in the next part of the series.  That
> does print on failure.  The rest of the ones that print an error message
> are about commands and it's somewhat helpful there.
> 
> So yes, return codes need to be checked and passed.  But no, not every
> single error path needs to print to the user along every part of an
> error path either.
> 
>>> 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.
>>
>> It does not matter where the error is reported - in the called
>> function, or in some caller firther up the call tree.  But it _must_
>> be reportet at least once.
>>
>> So if we don't issue an error message here, we need to check and fix
>> the callers, too.
> 
> That would be the next patch in the series where the BSP author isn't
> currently checking the return value, and this series doesn't change
> that.  Perhaps it should, and CC the maintainer.  But I think has been
> said a few times over the course of this series, what exactly is one
> going to do about the failure?  Getting specific for a moment, if you're
> in the case of "shutdown the watchdog" and the watchdog doesn't shutdown
> like you want it to, do you hang and hope the watchdog is alive to kick
> things still?  hang and lock the system?  Figure the system is on fire
> anyhow but add another message to the failure spew?
> 
> Again, I think the change that's needed to this patch is to make it
> return the error code to the caller.  Let the caller decide.  And make
> sure to CC the board maintainer on the next go-round so they can chime
> in about that.

Getting back to this to hopefully get this decided:

It seems that we (most of us?) agree on this change, that wdt_stop_all()
shall be changed to return an error code and the caller can decide what
to do with it?

If yes, then Rasmus, could you please re-spin this patchset accordingly
and send v6?

Thanks,
Stefan