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

[Stefan Roese]

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?

Thanks,
Stefan

> Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
> ---
>   drivers/watchdog/wdt-uclass.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 417e8d7eef9..caa1567e0c3 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -122,10 +122,11 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>   		struct wdt_priv *priv = dev_get_uclass_priv(dev);
>   		char str[16];
>   
> -		priv->running = true;
> -
>   		memset(str, 0, 16);
>   		if (IS_ENABLED(CONFIG_WATCHDOG)) {
> +			if (priv->running)
> +				cyclic_unregister(priv->cyclic);
> +
>   			/* Register the watchdog driver as a cyclic function */
>   			priv->cyclic = cyclic_register(wdt_cyclic,
>   						       priv->reset_period * 1000,
> @@ -140,6 +141,7 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>   			}
>   		}
>   
> +		priv->running = true;
>   		printf("WDT:   Started %s with%s servicing %s (%ds timeout)\n",
>   		       dev->name, IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out",
>   		       str, (u32)lldiv(timeout_ms, 1000));
> @@ -160,6 +162,9 @@ int wdt_stop(struct udevice *dev)
>   	if (ret == 0) {
>   		struct wdt_priv *priv = dev_get_uclass_priv(dev);
>   
> +		if (IS_ENABLED(CONFIG_WATCHDOG) && priv->running)
> +			cyclic_unregister(priv->cyclic);
> +
>   		priv->running = false;
>   	}
>   

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de