linux-kernel - Re: [PATCH v3 1/5] clk: Respect CLK_OPS_PARENT

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <3342669.irdbgypaU6@workhorse>
Date: Fri, 17 Oct 2025 14:21:55 +0200
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Brian Masney <bmasney@...hat.com>
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>,
 Dong Aisheng <aisheng.dong@....com>,
 Matthias Brugger <matthias.bgg@...il.com>,
 Yassine Oudjana <y.oudjana@...tonmail.com>,
 Laura Nao <laura.nao@...labora.com>,
 Nícolas F. R. A. Prado <nfraprado@...labora.com>,
 Chia-I Wu <olvaffe@...il.com>, Chen-Yu Tsai <wenst@...omium.org>,
 kernel@...labora.com, linux-clk@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v3 1/5] clk: Respect CLK_OPS_PARENT_ENABLE during recalc

On Thursday, 16 October 2025 22:52:30 Central European Summer Time Brian Masney wrote:
> Hi Nicolas,
> 
> On Fri, Oct 10, 2025 at 10:47:09PM +0200, Nicolas Frattaroli wrote:
> > When CLK_OPS_PARENT_ENABLE was introduced, it guarded various clock
> > operations, such as setting the rate or switching parents. However,
> > another operation that can and often does touch actual hardware state is
> > recalc_rate, which may also be affected by such a dependency.
> > 
> > Add parent enables/disables where the recalc_rate op is called directly.
> > 
> > Fixes: fc8726a2c021 ("clk: core: support clocks which requires parents enable (part 2)")
> > Fixes: a4b3518d146f ("clk: core: support clocks which requires parents enable (part 1)")
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
> > Reviewed-by: Chen-Yu Tsai <wenst@...omium.org>
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
> > ---
> >  drivers/clk/clk.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 85d2f2481acf360f0618a4a382fb51250e9c2fc4..1b0f9d567f48e003497afc98df0c0d2ad244eb90 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1921,7 +1921,14 @@ static unsigned long clk_recalc(struct clk_core *core,
> >  	unsigned long rate = parent_rate;
> >  
> >  	if (core->ops->recalc_rate && !clk_pm_runtime_get(core)) {
> > +		if (core->flags & CLK_OPS_PARENT_ENABLE)
> > +			clk_core_prepare_enable(core->parent);
> > +
> >  		rate = core->ops->recalc_rate(core->hw, parent_rate);
> > +
> > +		if (core->flags & CLK_OPS_PARENT_ENABLE)
> > +			clk_core_disable_unprepare(core->parent);
> > +
> >  		clk_pm_runtime_put(core);
> >  	}
> >  	return rate;
> 
> clk_change_rate() has the following code:
> 
> 
>         if (core->flags & CLK_OPS_PARENT_ENABLE)
>                 clk_core_prepare_enable(parent);
> 
> 	...
> 
>         core->rate = clk_recalc(core, best_parent_rate);
> 
> 	...
> 
>         if (core->flags & CLK_OPS_PARENT_ENABLE)
>                 clk_core_disable_unprepare(parent);
> 
> clk_change_rate() ultimately is called by various clk_set_rate
> functions. Will that be a problem for the double calls to
> clk_core_prepare_enable()?

I don't see how multiple prepares are a problem as long as they're
balanced.

> 
> Fanning this out to the edge further is going to make the code even
> more complicated. What do you think about moving this to
> clk_core_enable_lock()? I know the set_parent operation has a special
> case that would need to be worked around.

__clk_core_init also needs special code in that case, as it calls the
bare recalc_rate op with no clk_core_enable_lock beforehand. It's also
wrong, in that recalc_rate does not necessitate the clock being on as
far as I'm aware. (if it did, this wouldn't be a problem in the first
place, as enabling it would enable the parent as well). Changing the
semantics of clk_recalc, and therefore clk_get_rate, to also enable
the clock, would be a major change in how the common clock framework
functions.

In my case, the __clk_core_init callback was the one that crashed,
so it really needs to happen there, and I really don't want to
refactor every location where `CLK_OPS_PARENT_ENABLE` is used for
a bugfix just to avoid potentially checking the same flag twice.

Having `CLK_OPS_PARENT_ENABLE` cleaned up such that every clk op
that has potential register access is never directly called by the
clk core except for one place, an accessor function that does both
pmdomain and `CLK_OPS_PARENT_ENABLE` checks, would be nice, e.g.
by keeping the clk_recalc change and then having __clk_core_init
call clk_recalc instead of the recalc op directly. But then the
__clk_core_init logic needs further refactoring as well.

I'm not sure I want to do that in this series, because it's quite
a bit different from just adding the missing check and parent
toggling, and has the chance of me introducing subtle logic bugs
in what is supposed to be a bugfix.

> 
> Brian
> 
> 

Kind regards,
Nicolas Frattaroli