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

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

On 11/08/2021 13.49, Stefan Roese wrote:
> Hi Rasmus,
> 
> On 11.08.21 13:32, Rasmus Villemoes wrote:
>> On 03/08/2021 10.28, Stefan Roese wrote:
>>> Hi Rasmus,
>>>
>>>>    #endif
>>>> diff --git a/include/asm-generic/global_data.h
>>>> b/include/asm-generic/global_data.h
>>>> index e55070303f..28d749538c 100644
>>>> --- a/include/asm-generic/global_data.h
>>>> +++ b/include/asm-generic/global_data.h
>>>> @@ -447,12 +447,6 @@ struct global_data {
>>>>         */
>>>>        fdt_addr_t translation_offset;
>>>>    #endif
>>>> -#if CONFIG_IS_ENABLED(WDT)
>>>> -    /**
>>>> -     * @watchdog_dev: watchdog device
>>>> -     */
>>>> -    struct udevice *watchdog_dev;
>>>> -#endif
>>>
>>> After applying the patchset v4 to current master, I see this error when
>>> building for "x530":
>>
>> I'm not sure how I missed this, I was convinced I had done a grep and
>> seen that all references to ->watchdog_dev were gone.
>>
>>> board/alliedtelesis/x530/x530.c: In function 'arch_preboot_os':
>>> board/alliedtelesis/x530/x530.c:125:13: error: 'gd_t' {aka 'volatile
>>> struct global_data'} has no member named 'watchdog_dev'
>>>    125 |  wdt_stop(gd->watchdog_dev);
>>>        |             ^~
>>> make[1]: *** [scripts/Makefile.build:254:
>>> board/alliedtelesis/x530/x530.o] Error 1
>>>
>>> Perhaps we need a common function now to stop all watchdogs, which can
>>> be called from such places?
>>
>> Yes, I think that's the right thing, even if there's just one single
>> caller. Dead code elimination should remove that global function for all
>> other boards. Here is what I have right now:
>>
>>      watchdog: wdt-uclass: add wdt_stop_all() helper
>>
>>      Since the watchdog_dev member of struct global_data is going away in
>>      favor of the wdt-uclass handling all watchdog devices, prepare for
>>      that by adding a helper to call wdt_stop() on all known devices.
>>
>>      Initially, this will only be used in one single
>>      place (board/alliedtelesis/x530/x530.c), and that user currently
>>      ignores the return value of wdt_stop(). It's not clear what one
>> should
>>      do if we encounter an error (remember the error but still stop the
>>      remaining ones? return immediately? try to unwind and
>>      restart the devices already stopped?). Moreover, some watchdogs
>> are by
>>      design always-running (and don't have a ->stop method at all), so at
>>      the very least some exception for -ENOSYS would be in order.
>>
>>      So for now, and until a real use case appears from which we can
>> decide
>>      the desired semantics, have the function return void and just emit a
>>      log_debug() if an error is encountered.
>>
>> diff --git a/drivers/watchdog/wdt-uclass.c
>> b/drivers/watchdog/wdt-uclass.c
>> index 358fc68e27..75ff4c2a6c 100644
>> --- a/drivers/watchdog/wdt-uclass.c
>> +++ b/drivers/watchdog/wdt-uclass.c
>> @@ -116,6 +116,31 @@ int wdt_stop(struct udevice *dev)
>>          return ret;
>>   }
>>
>> +void wdt_stop_all(void)
>> +{
>> +       struct wdt_priv *priv;
>> +       struct udevice *dev;
>> +       struct uclass *uc;
>> +       int ret;
>> +
>> +       ret = uclass_get(UCLASS_WDT, &uc);
>> +       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.

>>   int wdt_reset(struct udevice *dev)
>>   {
>>          const struct wdt_ops *ops = device_get_ops(dev);
>>
>> (along with a declaration in wdt.h, and another patch for x530 to make
>> use of it). Something like that?
> 
> 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.).

> A follow-up patch on-top your patchset v4 will cause git bisect
> problems. So you need to integrate this into your patches and send a
> v5 I'm afraid.

Definitely, it's already reworked in between 6 and 7 in my local branch,
I just didn't want to send a 12-patch series without some "yeah,
something like that would work".

Rasmus