Re: [PATCH 1/5] watchdog: starfive-wdt: Drop runtime PM reference if starting watchdog fails

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

On Thu, Nov 05, 2026 at 06:52:29AM -0800, Guenter Roeck wrote:
> On Thu, Nov 05, 2026 at 06:49:26AM -0800, Guenter Roeck wrote:
> > On Thu, Nov 05, 2026 at 01:46:40PM +0000, Tzung-Bi Shih wrote:
> > > On Wed, Nov 04, 2026 at 08:44:41AM -0800, Guenter Roeck wrote:
> > > > Commit 8f0ca55016a7 ("watchdog: starfive-wdt: Fix runtime PM leak in
> > > > starfive_wdt_pm_start()") switched starfive_wdt_pm_start() to
> > > > pm_runtime_resume_and_get() so the runtime PM usage counter is not
> > > > leaked when resuming the device fails. However, if starfive_wdt_start()
> > > > subsequently fails (for example, on a timeout in starfive_wdt_int_clr()),
> > > > starfive_wdt_pm_start() returns the error without releasing the runtime
> > > > PM reference acquired by pm_runtime_resume_and_get(). Because the
> > > > watchdog core does not invoke the stop callback when start fails, the
> > > > usage counter remains permanently elevated.
> > > > 
> > > > Drop the runtime PM reference with pm_runtime_put_sync() if
> > > > starfive_wdt_start() fails.
> > > > 
> > > > Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
> > > > Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > Cc: Ziv Xu <ziv.xu@starfivetech.com>
> > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > 
> > > With a nit,
> > > 
> > > Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
> > > 
> > > > @@ -372,12 +372,17 @@ static void starfive_wdt_stop(struct starfive_wdt *wdt)
> > > >  static int starfive_wdt_pm_start(struct watchdog_device *wdd)
> > > >  {
> > > >  	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> > > > -	int ret = pm_runtime_resume_and_get(wdd->parent);
> > > > +	int ret;
> > > >  
> > > > +	ret = pm_runtime_resume_and_get(wdd->parent);
> > > >  	if (ret < 0)
> > > >  		return ret;
> > > >  
> > > > -	return starfive_wdt_start(wdt);
> > > > +	ret = starfive_wdt_start(wdt);
> > > > +	if (ret)
> > > > +		pm_runtime_put_sync(wdd->parent);
> > > > +
> > > > +	return ret;
> > > 
> > > Or just `return 0;`.
> > > 
> > 
> > Good idea. I'll change that.
> 
> Ah, no, we still want to return the error. Difference is that
> pm_runtime_put_sync() is only called if there was an error,
> but the error must still be returned to the caller.

Ah...correct, I overlooked that.