linux-kernel - Re: [PATCH RFC 2/3] watchdog: rzg2l

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 20 Jun 2024 16:31:04 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: ulf.hansson@...aro.org, wim@...ux-watchdog.org, linux@...ck-us.net, 
	rafael@...nel.org, linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-watchdog@...r.kernel.org, geert+renesas@...der.be, 
	linux-renesas-soc@...r.kernel.org, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH RFC 2/3] watchdog: rzg2l_wdt: Keep the clocks prepared

Hi Claudiu,

Thank you for the patch.

On Thu, Jun 20, 2024 at 9:29 AM Claudiu <claudiu.beznea@...on.dev> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@...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@...aro.org>
> Closes: https://lore.kernel.org/all/CAPDyKFq1+cL1M9qGY0P58ETHUZHGymxQL0w92emUJPMe7a_GxA@mail.gmail.com
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...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
>
>