Re: [PATCH] watchdog: sunxi_wdt: preserve boot-enabled watchdog

[Guenter Roeck]

On 8/27/26 16:42, James Hilliard wrote:
> sunxi_wdt_probe() unconditionally stops the watchdog even when firmware
> left it running. This opens an unprotected interval during boot and
> prevents CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED from taking over the active
> watchdog.
>
> Detect an enabled watchdog and decode its programmed interval. Preserve
> that timeout while setting the Linux reset mode and pinging the watchdog;
> use the configured timeout only when the hardware interval cannot be
> represented. The enable bit remains set throughout. Mark it as
> hardware-running before registration so the watchdog core services it
> until userspace takes control. Keep the existing stop operation when the
> watchdog was disabled.
>
> Fixes: d00680ed0026 ("watchdog: sunxi: New watchdog driver for Allwinner A10/A13")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: James Hilliard <james.hilliard1@xxxxxxxxx>
> ---
>   drivers/watchdog/sunxi_wdt.c | 47 +++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
> index b6c761acc3de..1f3da1d47fa5 100644
> --- a/drivers/watchdog/sunxi_wdt.c
> +++ b/drivers/watchdog/sunxi_wdt.c
> @@ -128,6 +128,38 @@ static int sunxi_wdt_ping(struct watchdog_device *wdt_dev)
>   	return 0;
>   }
>   
> +static bool sunxi_wdt_is_running(struct watchdog_device *wdt_dev)
> +{
> +	struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> +	const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
> +
> +	return readl(sunxi_wdt->wdt_base + regs->wdt_mode) & WDT_MODE_EN;
> +}
> +
> +static unsigned int sunxi_wdt_get_timeout(struct watchdog_device *wdt_dev)
> +{
> +	struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> +	const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
> +	unsigned int timeout;
> +	u32 interval;
> +
> +	interval = readl(sunxi_wdt->wdt_base + regs->wdt_mode);
> +	interval >>= regs->wdt_timeout_shift;
> +	interval &= WDT_TIMEOUT_MASK;
> +	/* The 0.5-second interval cannot be represented by wdt_dev->timeout. */
> +	if (!interval)
> +		return 0;
> +

Why 0 and not 1 ? If I understand correctly, this sets the timeout
to  WDT_MAX_TIMEOUT or 16 seconds. It is not immediately obvious why
that would be desirable.

> +	for (timeout = WDT_MIN_TIMEOUT;
> +	     timeout < ARRAY_SIZE(wdt_timeout_map); timeout++) {
> +		if (wdt_timeout_map[timeout] == interval)
> +			return timeout;
> +	}
> +
> +	/* Reserved interval encoding. */
> +	return 0;
> +}
> +
>   static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
>   		unsigned int timeout)
>   {
> @@ -259,6 +291,7 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct sunxi_wdt_dev *sunxi_wdt;
> +	unsigned int running_timeout;
>   	int err;
>   
>   	sunxi_wdt = devm_kzalloc(dev, sizeof(*sunxi_wdt), GFP_KERNEL);
> @@ -286,7 +319,19 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
>   
>   	watchdog_set_drvdata(&sunxi_wdt->wdt_dev, sunxi_wdt);
>   
> -	sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
> +	if (sunxi_wdt_is_running(&sunxi_wdt->wdt_dev)) {
> +		running_timeout = sunxi_wdt_get_timeout(&sunxi_wdt->wdt_dev);
> +		if (running_timeout)
> +			sunxi_wdt->wdt_dev.timeout = running_timeout;
> +
> +		err = sunxi_wdt_start(&sunxi_wdt->wdt_dev);
> +		if (err)
> +			return err;
> +
> +		set_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status);
> +	} else {
> +		sunxi_wdt_stop(&sunxi_wdt->wdt_dev);

Why is this necessary ? The watchdog is known to be stopped, why
stop it again ?

Thanks,
Guenter

> +	}
>   
>   	watchdog_stop_on_reboot(&sunxi_wdt->wdt_dev);
>   	err = devm_watchdog_register_device(dev, &sunxi_wdt->wdt_dev);
>
> ---
> base-commit: 1b78070aaef63512688aebfbc82365ef9d6660f1
> change-id: 20260827-submit-sunxi-wdt-boot-enabled-v1-94cd6be885d2
>
> Best regards,
> --
> James Hilliard <james.hilliard1@xxxxxxxxx>