linux-kernel - RE: [PATCH 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:
 <TY3PR01MB11346A223DA7462799B9D103786942@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Tue, 27 Aug 2024 12:15:29 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Claudiu.Beznea <claudiu.beznea@...on.dev>, "geert+renesas@...der.be"
	<geert+renesas@...der.be>, "mturquette@...libre.com"
	<mturquette@...libre.com>, "sboyd@...nel.org" <sboyd@...nel.org>,
	"wim@...ux-watchdog.org" <wim@...ux-watchdog.org>, "linux@...ck-us.net"
	<linux@...ck-us.net>, "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>
CC: "linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
	"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>, Claudiu.Beznea
	<claudiu.beznea@...on.dev>, Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: RE: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in
 the restart handler

Hi Claudiu,

Thanks for the feedback.

> -----Original Message-----
> From: Claudiu <claudiu.beznea@...on.dev>
> Sent: Monday, August 26, 2024 4:25 PM
> Subject: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler
> 
> 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 of RZ/G2 devices that uses

RZ/G2L alike devices or be specific RZ/{G2L,G2LC,G2UL,V2L} as it is not applicable for
RZ/G2{H,M,N,E}devices.


> 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.

Can this patch be fix for Commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
> context'") on RZ/{G2L,G2LC,G2UL,V2L} SoC??

Cheers,
Biju

> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> ---
>  drivers/watchdog/rzg2l_wdt.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index
> 2a35f890a288..e9e0408c96f7 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,23 @@ 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 RZ/G2 devices (and for RZ/G3S with old device trees
> +	 * where PM domains are registered like on RZ/G2 devices) 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 +291,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
>