linux-kernel - Re: [PATCH v2 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler

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]

Message-ID: <CAPDyKFrGGKU034C81Q_1AvwVLobKXrwbkgGXL83HgJW68-h+Sg@mail.gmail.com>
Date: Thu, 29 Aug 2024 16:46:49 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: geert+renesas@...der.be, mturquette@...libre.com, sboyd@...nel.org, 
	wim@...ux-watchdog.org, linux@...ck-us.net, linux-renesas-soc@...r.kernel.org, 
	linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-watchdog@...r.kernel.org, linux-pm@...r.kernel.org, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH v2 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain
 in the restart handler

On Wed, 28 Aug 2024 at 16:06, Claudiu <claudiu.beznea@...on.dev> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>
> On RZ/G3S the watchdog can be part of a software-controlled PM domain. In
> this case, the watchdog device need to be powered on in
> struct watchdog_ops::restart API. This can be done though
> pm_runtime_resume_and_get() API if the watchdog PM domain and watchdog
> device are marked as IRQ safe. We mark the watchdog PM domain as IRQ safe
> with GENPD_FLAG_IRQ_SAFE when the watchdog PM domain is registered and the
> watchdog device though pm_runtime_irq_safe().
>
> Before commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
> context'") pm_runtime_get_sync() was used in watchdog restart handler
> (which is similar to pm_runtime_resume_and_get() except the later one
> handles the runtime resume errors).
>
> Commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
> context'") dropped the pm_runtime_get_sync() and replaced it with
> clk_prepare_enable() to avoid invalid wait context due to genpd_lock()
> in genpd_runtime_resume() being called from atomic context. But
> clk_prepare_enable() doesn't fit for this either (as reported by
> Ulf Hansson) as clk_prepare() can also sleep (it just not throw invalid
> wait context warning as it is not written for this).
>
> Because the watchdog device is marked now as IRQ safe (though this patch)
> the irq_safe_dev_in_sleep_domain() call from genpd_runtime_resume() returns
> 1 for devices not registering an IRQ safe PM domain for watchdog (as the
> watchdog device is IRQ safe, PM domain is not and watchdog PM domain is
> always-on), this being the case for RZ/G3S with old device trees and
> the rest of the SoCs that use this driver, we can now drop also the
> clk_prepare_enable() calls in restart handler and rely on
> pm_runtime_resume_and_get().
>
> Thus, drop clk_prepare_enable() and use pm_runtime_resume_and_get() in
> watchdog restart handler.
>
> Acked-by: Guenter Roeck <linux@...ck-us.net>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>

Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org>

Kind regards
Uffe

> ---
>
> Changes in v2:
> - adjusted patch description and comment from code
> - collected tags
>
> Changes since RFC:
> - use pm_runtime_resume_and_get() and pm_runtime_irq_safe()
> - drop clock prepare in probe
>
>  drivers/watchdog/rzg2l_wdt.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index 2a35f890a288..11bbe48160ec 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -12,6 +12,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/reset.h>
>  #include <linux/units.h>
> @@ -166,8 +167,22 @@ 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);
> +       /*
> +        * In case of RZ/G3S the watchdog device may be part of an IRQ safe power
> +        * domain that is currently powered off. In this case we need to power
> +        * it on before accessing registers. Along with this the clocks will be
> +        * enabled. We don't undo the pm_runtime_resume_and_get() as the device
> +        * need to be on for the reboot to happen.
> +        *
> +        * For the rest of SoCs not registering a watchdog IRQ safe power
> +        * domain it is safe to call pm_runtime_resume_and_get() as the
> +        * irq_safe_dev_in_sleep_domain() call in genpd_runtime_resume()
> +        * returns non zero value and the genpd_lock() is avoided, thus, there
> +        * will be no invalid wait context reported by lockdep.
> +        */
> +       ret = pm_runtime_resume_and_get(wdev->parent);
> +       if (ret)
> +               return ret;
>
>         if (priv->devtype == WDT_RZG2L) {
>                 ret = reset_control_deassert(priv->rstc);
> @@ -275,6 +290,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
>
>         priv->devtype = (uintptr_t)of_device_get_match_data(dev);
>
> +       pm_runtime_irq_safe(&pdev->dev);
>         pm_runtime_enable(&pdev->dev);
>
>         priv->wdev.info = &rzg2l_wdt_ident;
> --
> 2.39.2
>