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

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Fri, Aug 28, 2026 at 07:48:18AM -0700, Guenter Roeck wrote:
> On 8/26/26 22:01, sashiko-bot@kernel.org wrote:
> > 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.
> > 
> 
> That seems real.

Fixed it in v2.

https://lore.kernel.org/all/20260828161348.13212-5-tzungbi@kernel.org

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

Proposed fixes for them in the series [1].

[1] https://lore.kernel.org/all/20260828161348.13212-1-tzungbi@kernel.org

> > > @@ -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?

https://lore.kernel.org/all/20260828161348.13212-3-tzungbi@kernel.org

> > 
> > >   	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?
> > 
> Yes, that will need to be fixed.

https://lore.kernel.org/all/20260828161348.13212-5-tzungbi@kernel.org

> > [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?
> > 
> 
> Worse, it starts the watchdog even if it isn't running. Separate problem, though.

https://lore.kernel.org/all/20260828161348.13212-4-tzungbi@kernel.org

> > [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?

https://lore.kernel.org/all/20260828161348.13212-6-tzungbi@kernel.org