Re: [PATCH v3] watchdog: msc313e: Propagate error code in resume()

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

On Wed, Sep 16, 2026 at 11:45:57AM +0700, Triet Hoang wrote:
> Sat, 12 Sep 2026 16:42:21 +0000 Sashiko wrote:
> 
> > [Severity: High]
> > This is a pre-existing issue, but does leaving the watchdog framework state
> > flags intact when the clock fails to enable cause a system crash?
> > 
> > If msc313e_wdt_start() fails, the clock remains disabled, but WDOG_ACTIVE
> > and WDOG_HW_RUNNING are still set. The Linux PM core ignores device resume 
> > errors and proceeds with system resume.

Another example that Sashiko complains either way.  See also v2 [1].

[1] https://lore.kernel.org/all/20260828162703.9F9DF1F00A3E@smtp.kernel.org

> > 
> > The watchdog core's PM notifier then calls watchdog_dev_resume(), which
> > immediately invokes msc313e_wdt_ping(). This unconditionally writes to a 
> > memory-mapped register:
> > 
> > drivers/watchdog/msc313e_wdt.c:msc313e_wdt_ping() {
> > 	...
> > 	writew(1, priv->base + REG_WDT_CLR);
> > 	return 0;
> > }
> > 
> > Can accessing these registers with the bus clock disabled on this platform 
> > trigger a synchronous external bus abort and a kernel panic?
> > 
> > Additionally, a subsequent stop operation unconditionally disables the clock:
> > 
> > drivers/watchdog/msc313e_wdt.c:msc313e_wdt_stop() {
> > 	...
> > 	writew(0, priv->base + REG_WDT_CLR);
> > 	clk_disable_unprepare(priv->clk);
> > 	return 0;
> > }
> > 
> > Since the clock enable previously failed, would this extra 
> > clk_disable_unprepare() cause a clock reference count underflow warning?
> 
> Hi Tzung-Bi,
> 
> I think we can address the problem Sashiko mentioned by making the clock ownership 
> explicit in the driver.
> 
> If msc313e_wdt_start() fails to enable the clock, the driver should not subsequently access
> the watchdog registers or call clk_disable_unprepare() as if the clock had been successfully enabled.
> In particular, we can track whether the clock was successfully acquired and make ping() and stop()
> operate only when the clock is enabled.
> 
> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index 4a5cce2a16b1..437b8ace0a00 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -29,6 +29,7 @@ struct msc313e_wdt_priv {
>         void __iomem *base;
>         struct watchdog_device wdev;
>         struct clk *clk;
> +       bool clk_enabled;
>  };
>  
>  static u32 msc313e_wdt_get_hw_timeout(struct msc313e_wdt_priv *priv)
> @@ -60,6 +61,7 @@ static int msc313e_wdt_start(struct watchdog_device *wdev)
>         if (err)
>                 return err;
>  
> +       priv->clk_enabled = true;
>         msc313e_wdt_set_hw_timeout(priv, wdev->timeout);
>         return 0;
>  }
> @@ -68,6 +70,9 @@ static int msc313e_wdt_ping(struct watchdog_device *wdev)
>  {
>         struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
>  
> +       if (!priv->clk_enabled)
> +               return -EIO;
> +
>         writew(1, priv->base + REG_WDT_CLR);
>         return 0;
>  }
> @@ -79,7 +84,10 @@ static int msc313e_wdt_stop(struct watchdog_device *wdev)
>         writew(0, priv->base + REG_WDT_MAX_PRD_L);
>         writew(0, priv->base + REG_WDT_MAX_PRD_H);
>         writew(0, priv->base + REG_WDT_CLR);
> -       clk_disable_unprepare(priv->clk);
> +       if (priv->clk_enabled) {
> +               clk_disable_unprepare(priv->clk);
> +               priv->clk_enabled = false;
> +       }
>         return 0;
>  }
> 
> How do you think about this idea?

Hi Triet,

Thanks for the suggestion.

Fundamentally, if msc313e_wdt_start() fails due to a clk_prepare_enable()
failure, it indicates a much more severe system-level issue.  In such a
scenario, propagating the error to the PM core is likely our best bet rather
than trying to gracefully manage it with additional state tracking.

Also, regarding Sashiko's concern about the bus abort and kernel panic:
the driver enables and disables this clock primarily for CCF reference
counting.  It is actually a "fixed-clock" (see xtal_div2 in mstar-v7.dtsi).
Because it is a fixed clock, there is no physical clock gating that would
block bus access and cause the synchronous external bus abort that the bot
is warning about.

Given that, I think it's better to avoid adding extra state complexity to
the driver just to silence the warning.