| 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
| ||
|
Date: Tue, 30 Aug 2022 21:40:53 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Alexander Stein <alexander.stein@...tq-group.com>
Cc: Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>,
Nícolas F . R . A . Prado
<nfraprado@...labora.com>, linux-clk@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND v2 1/2] clk: core: Honor CLK_OPS_PARENT_ENABLE for
clk gate ops
On Tue, Aug 30, 2022 at 8:37 PM Alexander Stein
<alexander.stein@...tq-group.com> wrote:
>
> Hi,
>
> Am Montag, 29. August 2022, 11:22:16 CEST schrieb Chen-Yu Tsai:
> > Hi,
> >
> > On Fri, Aug 26, 2022 at 8:28 PM Alexander Stein
> >
> > <alexander.stein@...tq-group.com> wrote:
> > > Hi everybody,
> > >
> > > Am Montag, 22. August 2022, 10:14:23 CEST schrieb Chen-Yu Tsai:
> > > > In the previous commits that added CLK_OPS_PARENT_ENABLE, support for
> > > > this flag was only added to rate change operations (rate setting and
> > > > reparent) and disabling unused subtree. It was not added to the
> > > > clock gate related operations. Any hardware driver that needs it for
> > > > these operations will either see bogus results, or worse, hang.
> > > >
> > > > This has been seen on MT8192 and MT8195, where the imp_ii2_* clk
> > > > drivers set this, but dumping debugfs clk_summary would cause it
> > > > to hang.
> > > >
> > > > Fixes: fc8726a2c021 ("clk: core: support clocks which requires parents
> > > > enable (part 2)") Fixes: a4b3518d146f ("clk: core: support clocks which
> > > > requires parents enable (part 1)") Signed-off-by: Chen-Yu Tsai
> > > > <wenst@...omium.org>
> > > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> > > > Tested-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> > > > ---
> > > >
> > > > drivers/clk/clk.c | 28 ++++++++++++++++++++++++++++
> > > > 1 file changed, 28 insertions(+)
> > > >
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index 7fc191c15507..9b365cd6d14b 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -196,6 +196,9 @@ static bool clk_core_rate_is_protected(struct
> > > > clk_core
> > > > *core) return core->protect_count;
> > > >
> > > > }
> > > >
> > > > +static int clk_core_prepare_enable(struct clk_core *core);
> > > > +static void clk_core_disable_unprepare(struct clk_core *core);
> > > > +
> > > >
> > > > static bool clk_core_is_prepared(struct clk_core *core)
> > > > {
> > > >
> > > > bool ret = false;
> > > >
> > > > @@ -208,7 +211,11 @@ static bool clk_core_is_prepared(struct clk_core
> > > > *core) return core->prepare_count;
> > > >
> > > > if (!clk_pm_runtime_get(core)) {
> > > >
> > > > + if (core->flags & CLK_OPS_PARENT_ENABLE)
> > > > + clk_core_prepare_enable(core->parent);
> > > >
> > > > ret = core->ops->is_prepared(core->hw);
> > > >
> > > > + if (core->flags & CLK_OPS_PARENT_ENABLE)
> > > > + clk_core_disable_unprepare(core->parent);
> > > >
> > > > clk_pm_runtime_put(core);
> > > >
> > > > }
> > > >
> > > > @@ -244,7 +251,13 @@ static bool clk_core_is_enabled(struct clk_core
> > > > *core)
> > > >
> > > > }
> > > >
> > > > }
> > > >
> > > > + if (core->flags & CLK_OPS_PARENT_ENABLE)
> > > > + clk_core_prepare_enable(core->parent);
> > > > +
> > > >
> > > > ret = core->ops->is_enabled(core->hw);
> > > >
> > > > +
> > > > + if (core->flags & CLK_OPS_PARENT_ENABLE)
> > > > + clk_core_disable_unprepare(core->parent);
> > > >
> > > > done:
> > > > if (core->rpm_enabled)
> > > >
> > > > pm_runtime_put(core->dev);
> > > >
> > > > @@ -812,6 +825,9 @@ int clk_rate_exclusive_get(struct clk *clk)
> > > >
> > > > }
> > > > EXPORT_SYMBOL_GPL(clk_rate_exclusive_get);
> > > >
> > > > +static int clk_core_enable_lock(struct clk_core *core);
> > > > +static void clk_core_disable_lock(struct clk_core *core);
> > > > +
> > > >
> > > > static void clk_core_unprepare(struct clk_core *core)
> > > > {
> > > >
> > > > lockdep_assert_held(&prepare_lock);
> > > >
> > > > @@ -835,6 +851,9 @@ static void clk_core_unprepare(struct clk_core
> > > > *core)
> > > >
> > > > WARN(core->enable_count > 0, "Unpreparing enabled %s\n", core-
> > > >
> > > >name);
> > > >
> > > > + if (core->flags & CLK_OPS_PARENT_ENABLE)
> > > > + clk_core_enable_lock(core->parent);
> > > > +
> > > >
> > > > trace_clk_unprepare(core);
> > > >
> > > > if (core->ops->unprepare)
> > > >
> > > > @@ -843,6 +862,9 @@ static void clk_core_unprepare(struct clk_core
> > > > *core)
> > > >
> > > > clk_pm_runtime_put(core);
> > > >
> > > > trace_clk_unprepare_complete(core);
> > > >
> > > > +
> > > > + if (core->flags & CLK_OPS_PARENT_ENABLE)
> > > > + clk_core_disable_lock(core->parent);
> > > >
> > > > clk_core_unprepare(core->parent);
> > > >
> > > > }
> > > >
> > > > @@ -891,6 +913,9 @@ static int clk_core_prepare(struct clk_core *core)
> > > >
> > > > if (ret)
> > > >
> > > > goto runtime_put;
> > > >
> > > > + if (core->flags & CLK_OPS_PARENT_ENABLE)
> > > > + clk_core_enable_lock(core->parent);
> > > > +
> > > >
> > > > trace_clk_prepare(core);
> > > >
> > > > if (core->ops->prepare)
> > > >
> > > > @@ -898,6 +923,9 @@ static int clk_core_prepare(struct clk_core *core)
> > > >
> > > > trace_clk_prepare_complete(core);
> > > >
> > > > + if (core->flags & CLK_OPS_PARENT_ENABLE)
> > > > + clk_core_disable_lock(core->parent);
> > > > +
> > > >
> > > > if (ret)
> > > >
> > > > goto unprepare;
> > > >
> > > > }
> > >
> > > Unfortunately this completely locks up my i.MX8M Plus based board during
> > > early boot.
> > > I'm currently running on next-20220826 using
> > > arch/arm64/boot/dts/freescale/
> > > imx8mp-tqma8mpql-mba8mpxl.dts
> > > Reverting this patch gets my board booting again. dmesg until hard lockup
> > > below.
> >
> > The standard logs don't have anything to go on. Could you add some printk
> > calls to the clk core around the areas this patch touchs? That would help.
> >
> > Could you also provide a dump of /sys/kernel/debug/clk/clk_summary? That
> > would help to understand the clock tree.
>
> Sure,
>
> These are the last kernel log lines before hard lockup:
> [ 0.686357] io scheduler mq-deadline registered
> [ 0.690907] io scheduler kyber registered
> [ 0.699275] clk_core_prepare: clk: main_axi CLK_OPS_PARENT_ENABLE
>
> main_axi is also the only debug output up to this point. This is the used
> patch for debugging:
> ---8<---
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -211,8 +211,10 @@ static bool clk_core_is_prepared(struct clk_core *core)
> return core->prepare_count;
>
> if (!clk_pm_runtime_get(core)) {
> - if (core->flags & CLK_OPS_PARENT_ENABLE)
> + if (core->flags & CLK_OPS_PARENT_ENABLE) {
> + pr_info("%s: clk: %s CLK_OPS_PARENT_ENABLE\n",
> __func__, core->name);
> clk_core_prepare_enable(core->parent);
> + }
> ret = core->ops->is_prepared(core->hw);
> if (core->flags & CLK_OPS_PARENT_ENABLE)
> clk_core_disable_unprepare(core->parent);
> @@ -251,8 +253,10 @@ static bool clk_core_is_enabled(struct clk_core *core)
> }
> }
>
> - if (core->flags & CLK_OPS_PARENT_ENABLE)
> + if (core->flags & CLK_OPS_PARENT_ENABLE) {
> + pr_info("%s: clk: %s CLK_OPS_PARENT_ENABLE\n", __func__, core-
> >name);
> clk_core_prepare_enable(core->parent);
> + }
>
> ret = core->ops->is_enabled(core->hw);
>
> @@ -851,8 +855,10 @@ static void clk_core_unprepare(struct clk_core *core)
>
> WARN(core->enable_count > 0, "Unpreparing enabled %s\n", core->name);
>
> - if (core->flags & CLK_OPS_PARENT_ENABLE)
> + if (core->flags & CLK_OPS_PARENT_ENABLE) {
> + pr_info("%s: clk: %s CLK_OPS_PARENT_ENABLE\n", __func__, core-
> >name);
> clk_core_enable_lock(core->parent);
> + }
>
> trace_clk_unprepare(core);
>
> @@ -912,8 +918,10 @@ static int clk_core_prepare(struct clk_core *core)
> if (ret)
> goto runtime_put;
>
> - if (core->flags & CLK_OPS_PARENT_ENABLE)
> + if (core->flags & CLK_OPS_PARENT_ENABLE) {
> + pr_info("%s: clk: %s CLK_OPS_PARENT_ENABLE\n",
> __func__, core->name);
> clk_core_enable_lock(core->parent);
> + }
>
> trace_clk_prepare(core);
>
>
> ---8<---
Thanks. So the part of the clock tree that's problematic is this:
osc_24m (fixed)
sys_pll1_ref_sel (mux)
sys_pll1 (imx pll14xx)
sys_pll1_bypass (mux)
sys_pll1_out (gate)
sys_pll1_800m (fixed factor)
main_axi (composite CLK_IS_CRITICAL)
Would it be possible for you to produce a stack dump as well? And also
enable lock debugging? This likely still won't catch anything if it's
a spinlock deadlock though.
Thanks
ChenYu
Powered by blists - more mailing lists