Re: [PATCH 2/2] watchdog: msc313e: Enable clock before accessing hardware registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] Clock reference count is leaked on the probe error path if watchdog registration fails.

Pre-existing issues:
- [High] Repeatedly setting the watchdog timeout leaks clock prepare and enable references, potentially leading to refcount overflow.
- [High] System will unexpectedly reset during suspend if the hardware watchdog is running but was never opened by userspace.
- [High] Potential division by zero if clk_get_rate() returns 0.
--

commit 363d0038f19004fef94c7716c644171a89070cb1
Author: Tzung-Bi Shih <tzungbi@kernel.org>
Subject: watchdog: msc313e: Enable clock before accessing hardware registers

msc313e_wdt_probe() reads from hardware registers without ensuring the
required clock is enabled. This patch calls clk_prepare_enable() before
reading WDT registers and leaves the clock enabled if the WDT is running.

> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index 3b62650375628..b89b1cddaa5e8 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -98,6 +98,7 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct msc313e_wdt_priv *priv;
> +	int ret;
>  
>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)

[ ... ]

> @@ -120,9 +121,21 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
>  	priv->wdev.max_timeout = U32_MAX / clk_get_rate(priv->clk);

[Severity: High]
This isn't a bug introduced by this patch, but does this code cause a division
by zero panic during device probe if clk_get_rate() returns 0?

>  	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);

[Severity: Medium]
If the hardware watchdog is already running at boot, the driver intentionally
leaves the clock enabled here. However, if devm_watchdog_register_device()
subsequently fails:

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_probe() {
    ...
	return devm_watchdog_register_device(dev, &priv->wdev);
}

Does this regression leak the clock reference count on the error path if
device registration fails, since the prepared clock is never disabled?


[Severity: High]
This is a pre-existing issue, but repeatedly setting the watchdog timeout
leaks clock prepare and enable references, potentially leading to refcount
overflow.

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_settimeout() {
    ...
	return msc313e_wdt_start(wdev);
}

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_start() {
    ...
	err = clk_prepare_enable(priv->clk);
    ...
}

Since the set_timeout callback directly wraps msc313e_wdt_start(), does this
sequence unconditionally increment the clock refcount every time the timeout
is updated, without a corresponding decrement?


[Severity: High]
This isn't a bug introduced by this patch, but will the system unexpectedly
reset during suspend if the hardware watchdog is running but was never opened
by userspace?

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_suspend() {
    ...
	if (watchdog_active(&priv->wdev))
		msc313e_wdt_stop(&priv->wdev);
    ...
}

If the watchdog was left running by the bootloader (WDOG_HW_RUNNING is set)
but never opened by userspace, watchdog_active() is false and the driver
skips stopping it. Meanwhile, the watchdog core suspends its background ping
worker.

Since the hardware timer continues counting down without being pinged, will
it expire and reset the suspended system?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827044700.554333-1-tzungbi@kernel.org?part=2