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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3745487.e9J7NaK4W3@workhorse>
Date: Wed, 28 Jan 2026 17:04:55 +0100
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Mark Brown <broonie@...nel.org>
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 Wednesday, 28 January 2026 15:47:08 Central European Standard Time Mark Brown wrote:
> On Wed, Jan 28, 2026 at 03:09:46PM +0100, Nicolas Frattaroli wrote:
> > On Wednesday, 28 January 2026 00:14:29 Central European Standard Time Mark Brown wrote:
> 
> > > Do you have a debugging patch we could run which would say which clocks
> > > are impacted?  I guess it's more of an issue for the platforms that give
> > > no output but for at least Avenger96 I was getting earlycon output.
> 
> > Try this one
> 
> For the Avenger96 that says:
> 
> [    0.347816] __clk_core_init: enabling parent ck_hse for ck_per
> [    0.352230] __clk_core_init: disabling parent ck_hse for ck_per
> [    0.358207] __clk_core_init: enabling parent pll1_p for ck_mpu
> [    0.364005] __clk_core_init: disabling parent pll1_p for ck_mpu
> 
> https://lava.sirena.org.uk/scheduler/job/2412562#L563
> 

Okay, on this one, there may be a problem in the clock tree.
ck_mpu is marked CLK_IS_CRITICAL, but its parent, pll1_p, is not. Clock
core doesn't seem to check whether any children of a clock are marked as
critical before disabling it.

I'm not super familiar with the intended semantics of critical clocks.
If we need to manually mark all parents of critical clocks as critical
as well, then a (potentially partial) fix for the Avenger96 might be:

---
diff --git a/drivers/clk/stm32/clk-stm32mp1.c b/drivers/clk/stm32/clk-stm32mp1.c
index 2d9ccd96ec98..6bf3fad1e0dc 100644
--- a/drivers/clk/stm32/clk-stm32mp1.c
+++ b/drivers/clk/stm32/clk-stm32mp1.c
@@ -1783,12 +1783,12 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
 	PLL(PLL4, "pll4", ref4_parents, 0, RCC_PLL4CR, RCC_RCK4SELR),

 	/* ODF */
-	COMPOSITE(PLL1_P, "pll1_p", PARENT("pll1"), 0,
+	COMPOSITE(PLL1_P, "pll1_p", PARENT("pll1"), CLK_IS_CRITICAL,
 		  _GATE(RCC_PLL1CR, 4, 0),
 		  _NO_MUX,
 		  _DIV(RCC_PLL1CFGR2, 0, 7, 0, NULL)),

-	COMPOSITE(PLL2_P, "pll2_p", PARENT("pll2"), 0,
+	COMPOSITE(PLL2_P, "pll2_p", PARENT("pll2"), CLK_IS_CRITICAL,
 		  _GATE(RCC_PLL2CR, 4, 0),
 		  _NO_MUX,
 		  _DIV(RCC_PLL2CFGR2, 0, 7, 0, NULL)),
@@ -1803,7 +1803,7 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
 		  _NO_MUX,
 		  _DIV(RCC_PLL2CFGR2, 16, 7, 0, NULL)),

-	COMPOSITE(PLL3_P, "pll3_p", PARENT("pll3"), 0,
+	COMPOSITE(PLL3_P, "pll3_p", PARENT("pll3"), CLK_IS_CRITICAL,
 		  _GATE(RCC_PLL3CR, 4, 0),
 		  _NO_MUX,
 		  _DIV(RCC_PLL3CFGR2, 0, 7, 0, NULL)),
---

I just looked for CLK_IS_CRITICAL clocks in that file that have the
CLK_OPS_PARENT_ENABLE flag, and marked their PLL parents as critical
as well.

An alternate approach would be to skip the parent enable/disable pair
in the problematic patch in __clk_core_init for clocks that are marked
as critical, because if the parent wasn't on for a critical clock then
we wouldn't be in that function, we would be dead.

Kind regards,
Nicolas Frattaroli