diff mbox series

[RFC,2/3] watchdog: rzg2l_wdt: Keep the clocks prepared

Message ID 20240619120920.2703605-3-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State New
Headers show
Series watchdog: rzg2l_wdt: Enable properly the watchdog clocks and power domain | expand

Commit Message

Claudiu Beznea June 19, 2024, 12:09 p.m. UTC
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The watchdog restart handler is called with interrupts disabled. In
rzg2l_wdt_restart() we call clk_prepare_enable() to enable the watchdog
clocks. The prepare part of clk_prepare_enable() may sleep. Sleep in
atomic context should not happen. The clock drivers for all the
micro-architectures where the RZ/G2L watchdog driver is used are not
implementing struct clk_ops::prepare(). Even so, to be sure we are
not hitted by this at some point, keep the watchdog clocks prepared
and only enable them in restart handler. It is guaranteed that
clk_enable() can be called in atomic context.

Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
Closes: https://lore.kernel.org/all/CAPDyKFq1+cL1M9qGY0P58ETHUZHGymxQL0w92emUJPMe7a_GxA@mail.gmail.com
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/watchdog/rzg2l_wdt.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

Comments

Lad, Prabhakar June 20, 2024, 3:31 p.m. UTC | #1
Hi Claudiu,

Thank you for the patch.

On Thu, Jun 20, 2024 at 9:29 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The watchdog restart handler is called with interrupts disabled. In
> rzg2l_wdt_restart() we call clk_prepare_enable() to enable the watchdog
> clocks. The prepare part of clk_prepare_enable() may sleep. Sleep in
> atomic context should not happen. The clock drivers for all the
> micro-architectures where the RZ/G2L watchdog driver is used are not
> implementing struct clk_ops::prepare(). Even so, to be sure we are
> not hitted by this at some point, keep the watchdog clocks prepared
> and only enable them in restart handler. It is guaranteed that
> clk_enable() can be called in atomic context.
>
> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> Closes: https://lore.kernel.org/all/CAPDyKFq1+cL1M9qGY0P58ETHUZHGymxQL0w92emUJPMe7a_GxA@mail.gmail.com
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/watchdog/rzg2l_wdt.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index 2a35f890a288..6e3d7512f38c 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -166,8 +166,8 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>         struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>         int ret;
>
> -       clk_prepare_enable(priv->pclk);
> -       clk_prepare_enable(priv->osc_clk);
> +       clk_enable(priv->pclk);
> +       clk_enable(priv->osc_clk);
>
I think we need to add a check before enabling the clocks:

if (!watchdog_active(wdev)) {
         clk_enable(priv->pclk);
         clk_enable(priv->osc_clk);
}

>         if (priv->devtype == WDT_RZG2L) {
>                 ret = reset_control_deassert(priv->rstc);
> @@ -226,11 +226,28 @@ static const struct watchdog_ops rzg2l_wdt_ops = {
>         .restart = rzg2l_wdt_restart,
>  };
>
> +static int rzg2l_clks_prepare(struct rzg2l_wdt_priv *priv)
> +{
> +       int ret;
> +
> +       ret = clk_prepare(priv->pclk);
> +       if (ret)
> +               return ret;
> +
> +       ret = clk_prepare(priv->osc_clk);
> +       if (ret)
> +               clk_unprepare(priv->pclk);
> +
> +       return ret;
> +}
> +
>  static void rzg2l_wdt_pm_disable(void *data)
>  {
> -       struct watchdog_device *wdev = data;
> +       struct rzg2l_wdt_priv *priv = data;
>
> -       pm_runtime_disable(wdev->parent);
> +       pm_runtime_disable(priv->wdev.parent);
> +       clk_unprepare(priv->osc_clk);
> +       clk_unprepare(priv->pclk);
>  }
>
All the above chunk can go away if we use devm_clk_get_prepared()
while requesting the clocks in the probe.

Cheers,
Prabhakar

>  static int rzg2l_wdt_probe(struct platform_device *pdev)
> @@ -275,6 +292,10 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
>
>         priv->devtype = (uintptr_t)of_device_get_match_data(dev);
>
> +       ret = rzg2l_clks_prepare(priv);
> +       if (ret)
> +               return ret;
> +
>         pm_runtime_enable(&pdev->dev);
>
>         priv->wdev.info = &rzg2l_wdt_ident;
> @@ -287,7 +308,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
>
>         watchdog_set_drvdata(&priv->wdev, priv);
>         dev_set_drvdata(dev, priv);
> -       ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv->wdev);
> +       ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv);
>         if (ret)
>                 return ret;
>
> --
> 2.39.2
>
>
Claudiu Beznea June 21, 2024, 6:16 a.m. UTC | #2
Hi, Prabhakar,

On 20.06.2024 18:31, Lad, Prabhakar wrote:
> Hi Claudiu,
> 
> Thank you for the patch.
> 
> On Thu, Jun 20, 2024 at 9:29 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The watchdog restart handler is called with interrupts disabled. In
>> rzg2l_wdt_restart() we call clk_prepare_enable() to enable the watchdog
>> clocks. The prepare part of clk_prepare_enable() may sleep. Sleep in
>> atomic context should not happen. The clock drivers for all the
>> micro-architectures where the RZ/G2L watchdog driver is used are not
>> implementing struct clk_ops::prepare(). Even so, to be sure we are
>> not hitted by this at some point, keep the watchdog clocks prepared
>> and only enable them in restart handler. It is guaranteed that
>> clk_enable() can be called in atomic context.
>>
>> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Closes: https://lore.kernel.org/all/CAPDyKFq1+cL1M9qGY0P58ETHUZHGymxQL0w92emUJPMe7a_GxA@mail.gmail.com
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/watchdog/rzg2l_wdt.c | 31 ++++++++++++++++++++++++++-----
>>  1 file changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
>> index 2a35f890a288..6e3d7512f38c 100644
>> --- a/drivers/watchdog/rzg2l_wdt.c
>> +++ b/drivers/watchdog/rzg2l_wdt.c
>> @@ -166,8 +166,8 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>>         struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>         int ret;
>>
>> -       clk_prepare_enable(priv->pclk);
>> -       clk_prepare_enable(priv->osc_clk);
>> +       clk_enable(priv->pclk);
>> +       clk_enable(priv->osc_clk);
>>
> I think we need to add a check before enabling the clocks:
> 
> if (!watchdog_active(wdev)) {

Agree, this should be better.

>          clk_enable(priv->pclk);
>          clk_enable(priv->osc_clk);
> }
> 
>>         if (priv->devtype == WDT_RZG2L) {
>>                 ret = reset_control_deassert(priv->rstc);
>> @@ -226,11 +226,28 @@ static const struct watchdog_ops rzg2l_wdt_ops = {
>>         .restart = rzg2l_wdt_restart,
>>  };
>>
>> +static int rzg2l_clks_prepare(struct rzg2l_wdt_priv *priv)
>> +{
>> +       int ret;
>> +
>> +       ret = clk_prepare(priv->pclk);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = clk_prepare(priv->osc_clk);
>> +       if (ret)
>> +               clk_unprepare(priv->pclk);
>> +
>> +       return ret;
>> +}
>> +
>>  static void rzg2l_wdt_pm_disable(void *data)
>>  {
>> -       struct watchdog_device *wdev = data;
>> +       struct rzg2l_wdt_priv *priv = data;
>>
>> -       pm_runtime_disable(wdev->parent);
>> +       pm_runtime_disable(priv->wdev.parent);
>> +       clk_unprepare(priv->osc_clk);
>> +       clk_unprepare(priv->pclk);
>>  }
>>
> All the above chunk can go away if we use devm_clk_get_prepared()
> while requesting the clocks in the probe.

Indeed, I missed devm_clk_get_prepared().

Thank you for your review,
Claudiu Beznea

> 
> Cheers,
> Prabhakar
> 
>>  static int rzg2l_wdt_probe(struct platform_device *pdev)
>> @@ -275,6 +292,10 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
>>
>>         priv->devtype = (uintptr_t)of_device_get_match_data(dev);
>>
>> +       ret = rzg2l_clks_prepare(priv);
>> +       if (ret)
>> +               return ret;
>> +
>>         pm_runtime_enable(&pdev->dev);
>>
>>         priv->wdev.info = &rzg2l_wdt_ident;
>> @@ -287,7 +308,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
>>
>>         watchdog_set_drvdata(&priv->wdev, priv);
>>         dev_set_drvdata(dev, priv);
>> -       ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv->wdev);
>> +       ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv);
>>         if (ret)
>>                 return ret;
>>
>> --
>> 2.39.2
>>
>>
diff mbox series

Patch

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 2a35f890a288..6e3d7512f38c 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -166,8 +166,8 @@  static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
 	int ret;
 
-	clk_prepare_enable(priv->pclk);
-	clk_prepare_enable(priv->osc_clk);
+	clk_enable(priv->pclk);
+	clk_enable(priv->osc_clk);
 
 	if (priv->devtype == WDT_RZG2L) {
 		ret = reset_control_deassert(priv->rstc);
@@ -226,11 +226,28 @@  static const struct watchdog_ops rzg2l_wdt_ops = {
 	.restart = rzg2l_wdt_restart,
 };
 
+static int rzg2l_clks_prepare(struct rzg2l_wdt_priv *priv)
+{
+	int ret;
+
+	ret = clk_prepare(priv->pclk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare(priv->osc_clk);
+	if (ret)
+		clk_unprepare(priv->pclk);
+
+	return ret;
+}
+
 static void rzg2l_wdt_pm_disable(void *data)
 {
-	struct watchdog_device *wdev = data;
+	struct rzg2l_wdt_priv *priv = data;
 
-	pm_runtime_disable(wdev->parent);
+	pm_runtime_disable(priv->wdev.parent);
+	clk_unprepare(priv->osc_clk);
+	clk_unprepare(priv->pclk);
 }
 
 static int rzg2l_wdt_probe(struct platform_device *pdev)
@@ -275,6 +292,10 @@  static int rzg2l_wdt_probe(struct platform_device *pdev)
 
 	priv->devtype = (uintptr_t)of_device_get_match_data(dev);
 
+	ret = rzg2l_clks_prepare(priv);
+	if (ret)
+		return ret;
+
 	pm_runtime_enable(&pdev->dev);
 
 	priv->wdev.info = &rzg2l_wdt_ident;
@@ -287,7 +308,7 @@  static int rzg2l_wdt_probe(struct platform_device *pdev)
 
 	watchdog_set_drvdata(&priv->wdev, priv);
 	dev_set_drvdata(dev, priv);
-	ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv->wdev);
+	ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv);
 	if (ret)
 		return ret;