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

[Rasmus Villemoes]

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.

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().

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.

Rasmus