Re: [PATCH v2 4/9] watchdog: msc313e: Enable clock before accessing hardware registers

[Guenter Roeck <linux@roeck-us.net>]

On Sat, Aug 29, 2026 at 12:13:43AM +0800, Tzung-Bi Shih wrote:
> msc313e_wdt_probe() reads from hardware registers without ensuring the
> required clock is enabled.  Furthermore, if the bootloader leaves the
> watchdog running, msc313e_wdt_probe() sets WDOG_HW_RUNNING without
> increasing the clock's reference count.
> 
> While the clock is currently supplied as a fixed clock by the device
> tree (`xtal_div2` in arch/arm/boot/dts/sigmastar/mstar-v7.dtsi) which
> masks the physical issue, this still violates the API usage.
> 
> Call clk_prepare_enable() before reading WDT registers.  If the WDT is
> running, leave the clock enabled so the CCF reference counter is
> balanced.
> 
> Fixes: ffd264bd152c ("watchdog: msc313e: Check if the WDT was running at boot")
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Applied.

Comment below.

> ---
> v2:
> - Fix a clock leak issue in the error handling path (Sashiko reported).
> 
> v1: https://lore.kernel.org/all/20260827044700.554333-3-tzungbi@kernel.org
> ---
>  drivers/watchdog/msc313e_wdt.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index 8ce24df8e338..7c4593566781 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -108,6 +108,7 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct msc313e_wdt_priv *priv;
>  	unsigned long rate;
> +	int ret;
>  
>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> @@ -133,9 +134,21 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
>  	priv->wdev.max_timeout = U32_MAX / rate;
>  	priv->wdev.timeout = MSC313E_WDT_DEFAULT_TIMEOUT;
>  
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return ret;
> +
>  	/* If the period is non-zero the WDT is running */
> -	if (readw(priv->base + REG_WDT_MAX_PRD_L) | (readw(priv->base + REG_WDT_MAX_PRD_H) << 16))
> +	if (readw(priv->base + REG_WDT_MAX_PRD_L) | (readw(priv->base + REG_WDT_MAX_PRD_H) << 16)) {
>  		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
> +		/*
> +		 * Keep the clock enabled. The watchdog core will skip the next
> +		 * start() and a future stop() will balance the CCF reference
> +		 * count.
> +		 */
> +	} else {
> +		clk_disable_unprepare(priv->clk);
> +	}
>  
>  	watchdog_set_drvdata(&priv->wdev, priv);
>  	platform_set_drvdata(pdev, priv);
> @@ -144,7 +157,13 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
>  	watchdog_stop_on_reboot(&priv->wdev);
>  	watchdog_stop_on_unregister(&priv->wdev);
>  
> -	return devm_watchdog_register_device(dev, &priv->wdev);
> +	ret = devm_watchdog_register_device(dev, &priv->wdev);
> +
> +	/* If the WDT is running and anything goes wrong, disable the clock. */
> +	if (ret && test_bit(WDOG_HW_RUNNING, &priv->wdev.status))
> +		clk_disable_unprepare(priv->clk);

Curious. Does this mean that Sashiko complains either way ?

Thanks,
Guenter

> +
> +	return ret;
>  }
>  
>  static int __maybe_unused msc313e_wdt_suspend(struct device *dev)