[PATCH 2/3] wdt-uclass: prevent multiple cyclic_register calls

[Stefan Roese]

Hi Rasmus,

On 5/13/24 13:09, Rasmus Villemoes wrote:
> On 13/05/2024 12.40, Stefan Roese wrote:
>> On 5/9/24 02:47, Rasmus Villemoes wrote:
>>> Currently, the cyclic_register() done in wdt_start() is not undone in
>>> wdt_stop(). Moreover, calling wdt_start multiple times (which is
>>> perfectly allowed on an already started device, e.g. to change the
>>> timeout value) will result in another struct cyclic_info being
>>> registered, referring to the same watchdog device.
>>>
>>> This can easily be seen on e.g. a wandboard:
>>>
>>> => cyclic list
>>> function: watchdog at 20bc000, cpu-time: 22 us, frequency: 1.01 times/s
>>> => wdt list
>>> watchdog at 20bc000 (imx_wdt)
>>> => wdt dev watchdog at 20bc000
>>> => wdt start 50000
>>> WDT:   Started watchdog at 20bc000 with servicing every 1000ms (50s timeout)
>>> => cyclic list
>>> function: watchdog at 20bc000, cpu-time: 37 us, frequency: 1.03 times/s
>>> function: watchdog at 20bc000, cpu-time: 241 us, frequency: 1.01 times/s
>>> => wdt start 12345
>>> WDT:   Started watchdog at 20bc000 with servicing every 1000ms (12s timeout)
>>> => cyclic list
>>> function: watchdog at 20bc000, cpu-time: 36 us, frequency: 1.03 times/s
>>> function: watchdog at 20bc000, cpu-time: 100 us, frequency: 1.04 times/s
>>> function: watchdog at 20bc000, cpu-time: 299 us, frequency: 1.00 times/s
>>>
>>> So properly unregister the watchdog device from the cyclic framework
>>> in wdt_stop(). In wdt_start(), we cannot just skip the registration,
>>> as the (new) timeout value may mean that we have to ask the cyclic
>>> framework to call us more often. So if we're already running,
>>> first unregister the old cyclic instance.
>>
>> Thanks again for all your valuable work here. I do have a question
>> regarding this patch though. AFAIU, it's a valid use-case to enable
>> 2 different watchdog devices. And this patch will prevent such a
>> setup. Or do I misunderstand this?
> 
> No, that shouldn't prevent enabling or petting two different watchdogs
> at all, that should work just as well as today. All the state I'm
> looking at and modifying belongs to one specific device, it's not
> wdt-uclass-global state, it's wdt-uclass-per-device state.

That's what I was hoping, but was too lazy to figure out myself.

> But with the existing code, if one calls wdt_start() a second time on an
> already started device, the existing priv->cyclic pointer is dropped on
> the floor (it gets unconditionally overwritten by the new
> cyclic_register() return value), so one gets the situation that the same
> device has multiple cyclic callbacks registered. Also, we're not
> unregistering the device from the cyclic framework upon ->stop().

Understood.

> Today, that's "mostly harmless". It does mean 'cyclic list' becomes
> cluttered, and is a memory leak, so you could exhaust memory by doing
> wdt_start in a loop, and cyclic_run() ends up doing extra callbacks to
> the same device (all but one will be no-ops due to the per-device
> rate-limiting of ->reset callbacks). In practice there's no chance of
> memory corruption or other failures. However, that changes with 3/3,
> where it would be catastrophic to call cyclic_register() with an already
> registered cyclic_info instance, as the hlist would become corrupt. I
> think it's worth fixing regardless of whether 3/3 is accepted.

Agreed.

Reviewed-by: Stefan Roese <sr at denx.de>

Thanks,
Stefan