| Message ID | 20240531065723.1085423-4-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: rzg2l_wdt: Add support for RZ/G3S | expand |
Hi Claudiu, > -----Original Message----- > From: Claudiu <claudiu.beznea@tuxon.dev> > Sent: Friday, May 31, 2024 7:57 AM > Subject: [PATCH v9 3/9] watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get() > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > pm_runtime_get_sync() may return with error. In case it returns with error > dev->power.usage_count needs to be decremented. > dev->pm_runtime_resume_and_get() > takes care of this. Thus use it. > > Along with it the rzg2l_wdt_set_timeout() function was updated to propagate the result of > rzg2l_wdt_start() to its caller. > > Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L") > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > > Changes in v9: > - collected Guenter's Rb tag > > Changes in v8: > - none > > Changes in v7: > - none > > Changes in v6: > - none > > Changes in v5: > - none > > Changes in v4: > - none > > Changes in v3: > - none > > Changes in v2: > - propagate the return code of rzg2l_wdt_start() to it's callers > > > drivers/watchdog/rzg2l_wdt.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index > 1741f98ca67c..d87d4f50180c 100644 > --- a/drivers/watchdog/rzg2l_wdt.c > +++ b/drivers/watchdog/rzg2l_wdt.c > @@ -123,8 +123,11 @@ static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev) static int > rzg2l_wdt_start(struct watchdog_device *wdev) { > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > + int ret; > > - pm_runtime_get_sync(wdev->parent); > + ret = pm_runtime_resume_and_get(wdev->parent); > + if (ret) > + return ret; Checking ret will lead to imbalance. It should be ret < 0 as ret = 1 corresponds to RPM_ACTIVE and the API does not call put() when ret = 1; see [1] and [2] [1] https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/base/power/runtime.c#L778 [2] https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/pm_runtime.h#L431 Cheers, Biju
> Subject: RE: [PATCH v9 3/9] watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get() > > Hi Claudiu, > > > -----Original Message----- > > From: Claudiu <claudiu.beznea@tuxon.dev> > > Sent: Friday, May 31, 2024 7:57 AM > > Subject: [PATCH v9 3/9] watchdog: rzg2l_wdt: Use > > pm_runtime_resume_and_get() > > > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > pm_runtime_get_sync() may return with error. In case it returns with > > error > > dev->power.usage_count needs to be decremented. > > dev->pm_runtime_resume_and_get() > > takes care of this. Thus use it. > > > > Along with it the rzg2l_wdt_set_timeout() function was updated to > > propagate the result of > > rzg2l_wdt_start() to its caller. > > > > Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L") > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > --- > > > > Changes in v9: > > - collected Guenter's Rb tag > > > > Changes in v8: > > - none > > > > Changes in v7: > > - none > > > > Changes in v6: > > - none > > > > Changes in v5: > > - none > > > > Changes in v4: > > - none > > > > Changes in v3: > > - none > > > > Changes in v2: > > - propagate the return code of rzg2l_wdt_start() to it's callers > > > > > > drivers/watchdog/rzg2l_wdt.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/watchdog/rzg2l_wdt.c > > b/drivers/watchdog/rzg2l_wdt.c index 1741f98ca67c..d87d4f50180c 100644 > > --- a/drivers/watchdog/rzg2l_wdt.c > > +++ b/drivers/watchdog/rzg2l_wdt.c > > @@ -123,8 +123,11 @@ static void rzg2l_wdt_init_timeout(struct > > watchdog_device *wdev) static int rzg2l_wdt_start(struct watchdog_device *wdev) { > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > + int ret; > > > > - pm_runtime_get_sync(wdev->parent); > > + ret = pm_runtime_resume_and_get(wdev->parent); > > + if (ret) > > + return ret; > > Checking ret will lead to imbalance. It should be ret < 0 as ret = 1 corresponds to RPM_ACTIVE and > the API does not call put() when ret = 1; see [1] and [2] > > [1] https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/base/power/runtime.c#L778 > > [2] https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/pm_runtime.h#L431 Please ignore me. Sorry for the noise. the code for pm_runtime_resume_and_get() seems to be correct. https://elixir.bootlin.com/linux/latest/source/include/linux/pm_runtime.h#L436 Cheers, Biju
diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index 1741f98ca67c..d87d4f50180c 100644 --- a/drivers/watchdog/rzg2l_wdt.c +++ b/drivers/watchdog/rzg2l_wdt.c @@ -123,8 +123,11 @@ static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev) static int rzg2l_wdt_start(struct watchdog_device *wdev) { struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); + int ret; - pm_runtime_get_sync(wdev->parent); + ret = pm_runtime_resume_and_get(wdev->parent); + if (ret) + return ret; /* Initialize time out */ rzg2l_wdt_init_timeout(wdev); @@ -150,6 +153,8 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev) static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout) { + int ret = 0; + wdev->timeout = timeout; /* @@ -159,10 +164,10 @@ static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int time */ if (watchdog_active(wdev)) { rzg2l_wdt_stop(wdev); - rzg2l_wdt_start(wdev); + ret = rzg2l_wdt_start(wdev); } - return 0; + return ret; } static int rzg2l_wdt_restart(struct watchdog_device *wdev,