| Message ID | 20211211212617.19639-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4] watchdog: rzg2l_wdt: Fix 32bit overflow issue | expand |
On 12/11/21 1:26 PM, Biju Das wrote: > If reset_control_deassert()/reset_control_reset() fails, then we > won't be able to access the device registers. Therefore check the > return code of reset_control_deassert()/reset_control_reset() and > return the error code to caller in case of error. > > While at it remove the unnecessary pm_runtime_resume_and_get() > from probe(), as it turns on the clocks. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> See note below, though. > --- > drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++----------------- > 1 file changed, 20 insertions(+), 17 deletions(-) > > diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c > index 96f2a018ab62..58fe4efd9a89 100644 > --- a/drivers/watchdog/rzg2l_wdt.c > +++ b/drivers/watchdog/rzg2l_wdt.c > @@ -85,8 +85,14 @@ 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; > + > + ret = reset_control_deassert(priv->rstc); > + if (ret) { > + dev_err(wdev->parent, "failed to deassert"); > + return ret; > + } > This patch introduces an error return into rzg2l_wdt_start(). If it is indeed necessary to call this function when setting the timeout, the error return needs to be checked there. Guenter > - reset_control_deassert(priv->rstc); > pm_runtime_get_sync(wdev->parent); > > /* Initialize time out */ > @@ -115,9 +121,15 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, > unsigned long action, void *data) > { > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > + int ret; > > /* Reset the module before we modify any register */ > - reset_control_reset(priv->rstc); > + ret = reset_control_reset(priv->rstc); > + if (ret) { > + dev_err(wdev->parent, "failed to reset"); > + return ret; > + } > + > pm_runtime_get_sync(wdev->parent); > > /* smallest counter value to reboot soon */ > @@ -151,12 +163,11 @@ static const struct watchdog_ops rzg2l_wdt_ops = { > .restart = rzg2l_wdt_restart, > }; > > -static void rzg2l_wdt_reset_assert_pm_disable_put(void *data) > +static void rzg2l_wdt_reset_assert_pm_disable(void *data) > { > struct watchdog_device *wdev = data; > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > - pm_runtime_put(wdev->parent); > pm_runtime_disable(wdev->parent); > reset_control_assert(priv->rstc); > } > @@ -204,13 +215,11 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) > return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc), > "failed to get cpg reset"); > > - reset_control_deassert(priv->rstc); > + ret = reset_control_deassert(priv->rstc); > + if (ret) > + return dev_err_probe(dev, ret, "failed to deassert"); > + > pm_runtime_enable(&pdev->dev); > - ret = pm_runtime_resume_and_get(&pdev->dev); > - if (ret < 0) { > - dev_err(dev, "pm_runtime_resume_and_get failed ret=%pe", ERR_PTR(ret)); > - goto out_pm_get; > - } > > priv->wdev.info = &rzg2l_wdt_ident; > priv->wdev.ops = &rzg2l_wdt_ops; > @@ -222,7 +231,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) > > watchdog_set_drvdata(&priv->wdev, priv); > ret = devm_add_action_or_reset(&pdev->dev, > - rzg2l_wdt_reset_assert_pm_disable_put, > + rzg2l_wdt_reset_assert_pm_disable, > &priv->wdev); > if (ret < 0) > return ret; > @@ -235,12 +244,6 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) > dev_warn(dev, "Specified timeout invalid, using default"); > > return devm_watchdog_register_device(&pdev->dev, &priv->wdev); > - > -out_pm_get: > - pm_runtime_disable(dev); > - reset_control_assert(priv->rstc); > - > - return ret; > } > > static const struct of_device_id rzg2l_wdt_ids[] = { >
Hi Guenter Roeck, > Subject: Re: [PATCH 2/4] watchdog: rzg2l_wdt: Add error check for > reset_control_{deassert/reset} > > On 12/11/21 1:26 PM, Biju Das wrote: > > If reset_control_deassert()/reset_control_reset() fails, then we won't > > be able to access the device registers. Therefore check the return > > code of reset_control_deassert()/reset_control_reset() and return the > > error code to caller in case of error. > > > > While at it remove the unnecessary pm_runtime_resume_and_get() from > > probe(), as it turns on the clocks. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > See note below, though. OK. > > > --- > > drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++----------------- > > 1 file changed, 20 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/watchdog/rzg2l_wdt.c > > b/drivers/watchdog/rzg2l_wdt.c index 96f2a018ab62..58fe4efd9a89 100644 > > --- a/drivers/watchdog/rzg2l_wdt.c > > +++ b/drivers/watchdog/rzg2l_wdt.c > > @@ -85,8 +85,14 @@ 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; > > + > > + ret = reset_control_deassert(priv->rstc); > > + if (ret) { > > + dev_err(wdev->parent, "failed to deassert"); > > + return ret; > > + } > > > This patch introduces an error return into rzg2l_wdt_start(). > If it is indeed necessary to call this function when setting the timeout, > the error return needs to be checked there. OK will do. Basically once watchdog is started it can be stopped only by reset. So for stopping WDT, we need to assert the reset. Also once WDT is started, Updation of WDT cycle setting register(WDTSET) is possible only by resetting WDT, that is assert/deassert and reconfigure WDTSET with new values. Regards, Biju > > Guenter > > > - reset_control_deassert(priv->rstc); > > pm_runtime_get_sync(wdev->parent); > > > > /* Initialize time out */ > > @@ -115,9 +121,15 @@ static int rzg2l_wdt_restart(struct watchdog_device > *wdev, > > unsigned long action, void *data) > > { > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > + int ret; > > > > /* Reset the module before we modify any register */ > > - reset_control_reset(priv->rstc); > > + ret = reset_control_reset(priv->rstc); > > + if (ret) { > > + dev_err(wdev->parent, "failed to reset"); > > + return ret; > > + } > > + > > pm_runtime_get_sync(wdev->parent); > > > > /* smallest counter value to reboot soon */ @@ -151,12 +163,11 @@ > > static const struct watchdog_ops rzg2l_wdt_ops = { > > .restart = rzg2l_wdt_restart, > > }; > > > > -static void rzg2l_wdt_reset_assert_pm_disable_put(void *data) > > +static void rzg2l_wdt_reset_assert_pm_disable(void *data) > > { > > struct watchdog_device *wdev = data; > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > - pm_runtime_put(wdev->parent); > > pm_runtime_disable(wdev->parent); > > reset_control_assert(priv->rstc); > > } > > @@ -204,13 +215,11 @@ static int rzg2l_wdt_probe(struct platform_device > *pdev) > > return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc), > > "failed to get cpg reset"); > > > > - reset_control_deassert(priv->rstc); > > + ret = reset_control_deassert(priv->rstc); > > + if (ret) > > + return dev_err_probe(dev, ret, "failed to deassert"); > > + > > pm_runtime_enable(&pdev->dev); > > - ret = pm_runtime_resume_and_get(&pdev->dev); > > - if (ret < 0) { > > - dev_err(dev, "pm_runtime_resume_and_get failed ret=%pe", > ERR_PTR(ret)); > > - goto out_pm_get; > > - } > > > > priv->wdev.info = &rzg2l_wdt_ident; > > priv->wdev.ops = &rzg2l_wdt_ops; > > @@ -222,7 +231,7 @@ static int rzg2l_wdt_probe(struct platform_device > > *pdev) > > > > watchdog_set_drvdata(&priv->wdev, priv); > > ret = devm_add_action_or_reset(&pdev->dev, > > - rzg2l_wdt_reset_assert_pm_disable_put, > > + rzg2l_wdt_reset_assert_pm_disable, > > &priv->wdev); > > if (ret < 0) > > return ret; > > @@ -235,12 +244,6 @@ static int rzg2l_wdt_probe(struct platform_device > *pdev) > > dev_warn(dev, "Specified timeout invalid, using default"); > > > > return devm_watchdog_register_device(&pdev->dev, &priv->wdev); > > - > > -out_pm_get: > > - pm_runtime_disable(dev); > > - reset_control_assert(priv->rstc); > > - > > - return ret; > > } > > > > static const struct of_device_id rzg2l_wdt_ids[] = { > >
Hi Guenter Roeck, > Subject: Re: [PATCH 2/4] watchdog: rzg2l_wdt: Add error check for > reset_control_{deassert/reset} > > On 12/11/21 1:26 PM, Biju Das wrote: > > If reset_control_deassert()/reset_control_reset() fails, then we won't > > be able to access the device registers. Therefore check the return > > code of reset_control_deassert()/reset_control_reset() and return the > > error code to caller in case of error. > > > > While at it remove the unnecessary pm_runtime_resume_and_get() from > > probe(), as it turns on the clocks. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > See note below, though. > > > --- > > drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++----------------- > > 1 file changed, 20 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/watchdog/rzg2l_wdt.c > > b/drivers/watchdog/rzg2l_wdt.c index 96f2a018ab62..58fe4efd9a89 100644 > > --- a/drivers/watchdog/rzg2l_wdt.c > > +++ b/drivers/watchdog/rzg2l_wdt.c > > @@ -85,8 +85,14 @@ 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; > > + > > + ret = reset_control_deassert(priv->rstc); > > + if (ret) { > > + dev_err(wdev->parent, "failed to deassert"); > > + return ret; > > + } > > > This patch introduces an error return into rzg2l_wdt_start(). > If it is indeed necessary to call this function when setting the timeout, > the error return needs to be checked there. Good point. After rechecking, this call is not at all needed. For WDT stop/ settime we need to reset the module. So we should use reset_control_reset() instead. From platform point, it just asserts and then de-assert the module[1]. Will add checks for this call in stop/settime and return error code to caller. I will send V2 with changes. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/renesas/rzg2l-cpg.c?h=v5.16-rc4#n685 Regards, Biju > > Guenter > > > - reset_control_deassert(priv->rstc); > > pm_runtime_get_sync(wdev->parent); > > > > /* Initialize time out */ > > @@ -115,9 +121,15 @@ static int rzg2l_wdt_restart(struct watchdog_device > *wdev, > > unsigned long action, void *data) > > { > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > + int ret; > > > > /* Reset the module before we modify any register */ > > - reset_control_reset(priv->rstc); > > + ret = reset_control_reset(priv->rstc); > > + if (ret) { > > + dev_err(wdev->parent, "failed to reset"); > > + return ret; > > + } > > + > > pm_runtime_get_sync(wdev->parent); > > > > /* smallest counter value to reboot soon */ @@ -151,12 +163,11 @@ > > static const struct watchdog_ops rzg2l_wdt_ops = { > > .restart = rzg2l_wdt_restart, > > }; > > > > -static void rzg2l_wdt_reset_assert_pm_disable_put(void *data) > > +static void rzg2l_wdt_reset_assert_pm_disable(void *data) > > { > > struct watchdog_device *wdev = data; > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > - pm_runtime_put(wdev->parent); > > pm_runtime_disable(wdev->parent); > > reset_control_assert(priv->rstc); > > } > > @@ -204,13 +215,11 @@ static int rzg2l_wdt_probe(struct platform_device > *pdev) > > return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc), > > "failed to get cpg reset"); > > > > - reset_control_deassert(priv->rstc); > > + ret = reset_control_deassert(priv->rstc); > > + if (ret) > > + return dev_err_probe(dev, ret, "failed to deassert"); > > + > > pm_runtime_enable(&pdev->dev); > > - ret = pm_runtime_resume_and_get(&pdev->dev); > > - if (ret < 0) { > > - dev_err(dev, "pm_runtime_resume_and_get failed ret=%pe", > ERR_PTR(ret)); > > - goto out_pm_get; > > - } > > > > priv->wdev.info = &rzg2l_wdt_ident; > > priv->wdev.ops = &rzg2l_wdt_ops; > > @@ -222,7 +231,7 @@ static int rzg2l_wdt_probe(struct platform_device > > *pdev) > > > > watchdog_set_drvdata(&priv->wdev, priv); > > ret = devm_add_action_or_reset(&pdev->dev, > > - rzg2l_wdt_reset_assert_pm_disable_put, > > + rzg2l_wdt_reset_assert_pm_disable, > > &priv->wdev); > > if (ret < 0) > > return ret; > > @@ -235,12 +244,6 @@ static int rzg2l_wdt_probe(struct platform_device > *pdev) > > dev_warn(dev, "Specified timeout invalid, using default"); > > > > return devm_watchdog_register_device(&pdev->dev, &priv->wdev); > > - > > -out_pm_get: > > - pm_runtime_disable(dev); > > - reset_control_assert(priv->rstc); > > - > > - return ret; > > } > > > > static const struct of_device_id rzg2l_wdt_ids[] = { > >
diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index 96f2a018ab62..58fe4efd9a89 100644 --- a/drivers/watchdog/rzg2l_wdt.c +++ b/drivers/watchdog/rzg2l_wdt.c @@ -85,8 +85,14 @@ 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; + + ret = reset_control_deassert(priv->rstc); + if (ret) { + dev_err(wdev->parent, "failed to deassert"); + return ret; + } - reset_control_deassert(priv->rstc); pm_runtime_get_sync(wdev->parent); /* Initialize time out */ @@ -115,9 +121,15 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, unsigned long action, void *data) { struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); + int ret; /* Reset the module before we modify any register */ - reset_control_reset(priv->rstc); + ret = reset_control_reset(priv->rstc); + if (ret) { + dev_err(wdev->parent, "failed to reset"); + return ret; + } + pm_runtime_get_sync(wdev->parent); /* smallest counter value to reboot soon */ @@ -151,12 +163,11 @@ static const struct watchdog_ops rzg2l_wdt_ops = { .restart = rzg2l_wdt_restart, }; -static void rzg2l_wdt_reset_assert_pm_disable_put(void *data) +static void rzg2l_wdt_reset_assert_pm_disable(void *data) { struct watchdog_device *wdev = data; struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); - pm_runtime_put(wdev->parent); pm_runtime_disable(wdev->parent); reset_control_assert(priv->rstc); } @@ -204,13 +215,11 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc), "failed to get cpg reset"); - reset_control_deassert(priv->rstc); + ret = reset_control_deassert(priv->rstc); + if (ret) + return dev_err_probe(dev, ret, "failed to deassert"); + pm_runtime_enable(&pdev->dev); - ret = pm_runtime_resume_and_get(&pdev->dev); - if (ret < 0) { - dev_err(dev, "pm_runtime_resume_and_get failed ret=%pe", ERR_PTR(ret)); - goto out_pm_get; - } priv->wdev.info = &rzg2l_wdt_ident; priv->wdev.ops = &rzg2l_wdt_ops; @@ -222,7 +231,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) watchdog_set_drvdata(&priv->wdev, priv); ret = devm_add_action_or_reset(&pdev->dev, - rzg2l_wdt_reset_assert_pm_disable_put, + rzg2l_wdt_reset_assert_pm_disable, &priv->wdev); if (ret < 0) return ret; @@ -235,12 +244,6 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) dev_warn(dev, "Specified timeout invalid, using default"); return devm_watchdog_register_device(&pdev->dev, &priv->wdev); - -out_pm_get: - pm_runtime_disable(dev); - reset_control_assert(priv->rstc); - - return ret; } static const struct of_device_id rzg2l_wdt_ids[] = {
If reset_control_deassert()/reset_control_reset() fails, then we won't be able to access the device registers. Therefore check the return code of reset_control_deassert()/reset_control_reset() and return the error code to caller in case of error. While at it remove the unnecessary pm_runtime_resume_and_get() from probe(), as it turns on the clocks. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-)