| 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
| ||
|
Message-ID:
<TY3PR01MB1134654A11467C617607E2D6E86942@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Tue, 27 Aug 2024 13:51: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.uj@...renesas.com>
Subject: RE: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in
the restart handler
Hi Claudiu,
> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@...on.dev>
> Sent: Tuesday, August 27, 2024 2:35 PM
> Subject: Re: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler
>
>
>
> On 27.08.2024 15:51, Biju Das wrote:
> >
> >
> >> -----Original Message-----
> >> From: claudiu beznea <claudiu.beznea@...on.dev>
> >> Sent: Tuesday, August 27, 2024 1:33 PM
> >> To: Biju Das <biju.das.jz@...renesas.com>; geert+renesas@...der.be;
> >> mturquette@...libre.com; sboyd@...nel.org; wim@...ux-watchdog.org;
> >> linux@...ck-us.net; ulf.hansson@...aro.org
> >> Cc: 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 3/3] watchdog: rzg2l_wdt: Power on the watchdog
> >> domain in the restart handler
> >>
> >> Hi, Biju,
> >>
> >> On 27.08.2024 15:15, Biju Das wrote:
> >>> 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.
> >>
> >> OK, but I said "RZ/G2 devices that uses this driver". Here are included RZ/{G2L,G2LC,G2UL,V2L}
> AFAICT.
> >
> > OK. Not sure you missed the same terminology on comment section, see below??
> >
> >>
> >>>
> >>>
> >>>> 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??
> >>
> >> Not sure... I thought about it, too. I chose to have it like this
> >> thinking
> >> that:
> >>
> >> 1/ that may not apply cleanly as it depends on other cleanups done on this
> >> driver, e.g. commit d8997ed79ed7 ("watchdog: rzg2l_wdt: Rely on the
> >> reset driver for doing proper reset") so it may be worthless for
> >> backport machinery
> >> 2/ There is actually no seen bug reported by lockdep (as the clk_prepare()
> >> doesn't handle it)
> >>
> >> Don't know, I can reply here and add it. Applying this patch with b4
> >> will take care of it. But not sure about it.
> >
> > Maybe leave it.
> >
> >>>
> >>>>
> >>>> 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
> >
> > NitPick: For the rest of RZ/G2 devices that uses this driver (This
> > will make sure It is not meant for RZ/G2{H,M,N,E} devices)
>
> If one considers this driver is used by RZ/G2{H,M,N,E} when reaching this point then surely is in the
> wrong place.
So strictly speaking, then it is RZ/G3S and rest of the devices.
as RZ/G2 is not applicable here as we have RZ/V2L and RZ/Five apart
from RZ/{G2L,G2LC,G2UL}.
>
> RZ/Five is also uses this driver. Later, maybe devices compatible with this driver will be added and
> this comment will not fit. Later, will we be updating the comment for that? I'm not a fan of it.
The comment RZ/G2 made confusion as the driver supports RZ/V2L and RZ/Five SoCs as well.
Also, it gives an impression about RZ/G2{H,M,N,E} devices.
Maybe replace "For the rest of RZ/G2 devices"->"For the rest of devices" should be sufficient.
as it covers RZ/{G2L,G2LC,G2UL}, RZ/V2L and RZ/Five SoCs.
Cheers.
Biju
> >
> >
> >
> >>>> + * 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
> >>>>
> >>>
Powered by blists - more mailing lists