Re: [PATCH v2 3/9] watchdog: msc313e: Fix clock leak and spurious timer in settimeout()

[Guenter Roeck]

On Sat, Aug 29, 2026 at 12:13:42AM +0800, Tzung-Bi Shih wrote:
> msc313e_wdt_settimeout() unconditionally calls msc313e_wdt_start() which
> introduces two severe bugs:
> 
> 1. If the watchdog is already active, calling start() again will
>    increase the reference count of the clock again.  However stop() is
>    only called once, the reference count is unbalance.
> 2. If the watchdog is stopped, calling settimeout() will start
>    the hardware timer accidentally.
> 
> Factor out the register-writing logic into a helper function.  Only call
> it in settimeout() if the watchdog is running.  Otherwise, simply update
> `wdev->timeout`.
> 
> Fixes: e9800b799464 ("watchdog: Add Mstar MSC313e WDT driver")
> Signed-off-by: Tzung-Bi Shih <tzungbi@xxxxxxxxxx>

Applied. Comment below, though.

Thanks,
Guenter

> ---
> v2:
> - New to the series.
> ---
>  drivers/watchdog/msc313e_wdt.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index c3018b970164..8ce24df8e338 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -31,20 +31,26 @@ struct msc313e_wdt_priv {
>  	struct clk *clk;
>  };
>  
> +static void msc313e_wdt_set_hw_timeout(struct msc313e_wdt_priv *priv,
> +				       unsigned int timeout)
> +{
> +	u32 t = timeout * clk_get_rate(priv->clk);
> +
> +	writew(t & 0xffff, priv->base + REG_WDT_MAX_PRD_L);
> +	writew((t >> 16) & 0xffff, priv->base + REG_WDT_MAX_PRD_H);
> +	writew(1, priv->base + REG_WDT_CLR);

I wonder if the write to REG_WDT_CLR can come first, to fix the
problem outlined by Sashiko in one of the subsequent patches.


> +}
> +
>  static int msc313e_wdt_start(struct watchdog_device *wdev)
>  {
>  	struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
> -	u32 timeout;
>  	int err;
>  
>  	err = clk_prepare_enable(priv->clk);
>  	if (err)
>  		return err;
>  
> -	timeout = wdev->timeout * clk_get_rate(priv->clk);
> -	writew(timeout & 0xffff, priv->base + REG_WDT_MAX_PRD_L);
> -	writew((timeout >> 16) & 0xffff, priv->base + REG_WDT_MAX_PRD_H);
> -	writew(1, priv->base + REG_WDT_CLR);
> +	msc313e_wdt_set_hw_timeout(priv, wdev->timeout);
>  	return 0;
>  }
>  
> @@ -69,9 +75,13 @@ static int msc313e_wdt_stop(struct watchdog_device *wdev)
>  
>  static int msc313e_wdt_settimeout(struct watchdog_device *wdev, unsigned int new_time)
>  {
> +	struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +
>  	wdev->timeout = new_time;
>  
> -	return msc313e_wdt_start(wdev);
> +	if (watchdog_hw_running(wdev) || watchdog_active(wdev))
> +		msc313e_wdt_set_hw_timeout(priv, wdev->timeout);
> +	return 0;
>  }
>  
>  static const struct watchdog_info msc313e_wdt_ident = {