[PATCH] clk: mediatek: mt7629: Properly use CLK_IS_CRITICAL flag

[PATCH] clk: mediatek: mt7629: Properly use CLK_IS_CRITICAL flag

[Ruoyu Wang]

The MT7629 clock driver calls clk_prepare_enable() for six clocks that
must remain running. These calls create permanent references owned by
the driver. In topckgen and apmixedsys, provider registration can fail
after the references are acquired, leaving them unreleased.

Mark ARMPLL, main_core_en, the AXI, memory and DDRPHY selectors, and the
UART0 gate with CLK_IS_CRITICAL, and remove the manual enable calls. The
common clock framework then keeps these clocks enabled without
driver-owned references. This preserves their always-on behavior and
avoids leaking references when provider registration fails.

This issue was found by a static analysis checker and confirmed by manual
source review.

Fixes: 3b5e748615e7 ("clk: mediatek: add clock support for MT7629 SoC")
Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>
---
 drivers/clk/mediatek/clk-mt7629.c | 36 ++++++++++++++-----------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt7629.c b/drivers/clk/mediatek/clk-mt7629.c
index e154771b1b8bba..627e8017fa642f 100644
--- a/drivers/clk/mediatek/clk-mt7629.c
+++ b/drivers/clk/mediatek/clk-mt7629.c
@@ -48,8 +48,9 @@
 		_pd_reg, _pd_shift, _tuner_reg, _pcw_reg, _pcw_shift,	\
 		NULL, "clk20m")
 
-#define GATE_APMIXED(_id, _name, _parent, _shift)			\
-	GATE_MTK(_id, _name, _parent, &apmixed_cg_regs, _shift, &mtk_clk_gate_ops_no_setclr_inv)
+#define GATE_APMIXED_AO(_id, _name, _parent, _shift)			\
+	GATE_MTK_FLAGS(_id, _name, _parent, &apmixed_cg_regs, _shift,	\
+		 &mtk_clk_gate_ops_no_setclr_inv, CLK_IS_CRITICAL)
 
 #define GATE_INFRA(_id, _name, _parent, _shift)				\
 	GATE_MTK(_id, _name, _parent, &infra_cg_regs, _shift, &mtk_clk_gate_ops_setclr)
@@ -57,6 +58,10 @@
 #define GATE_PERI0(_id, _name, _parent, _shift)				\
 	GATE_MTK(_id, _name, _parent, &peri0_cg_regs, _shift, &mtk_clk_gate_ops_setclr)
 
+#define GATE_PERI0_AO(_id, _name, _parent, _shift)			\
+	GATE_MTK_FLAGS(_id, _name, _parent, &peri0_cg_regs, _shift,	\
+		 &mtk_clk_gate_ops_setclr, CLK_IS_CRITICAL)
+
 #define GATE_PERI1(_id, _name, _parent, _shift)				\
 	GATE_MTK(_id, _name, _parent, &peri1_cg_regs, _shift, &mtk_clk_gate_ops_setclr)
 
@@ -311,7 +316,7 @@ static const struct mtk_gate_regs peri1_cg_regs = {
 
 static const struct mtk_pll_data plls[] = {
 	PLL(CLK_APMIXED_ARMPLL, "armpll", 0x0200, 0x020C, 0,
-	    0, 21, 0x0204, 24, 0, 0x0204, 0),
+	    PLL_AO, 21, 0x0204, 24, 0, 0x0204, 0),
 	PLL(CLK_APMIXED_MAINPLL, "mainpll", 0x0210, 0x021C, 0,
 	    HAVE_RST_BAR, 21, 0x0214, 24, 0, 0x0214, 0),
 	PLL(CLK_APMIXED_UNIV2PLL, "univ2pll", 0x0220, 0x022C, 0,
@@ -325,7 +330,7 @@ static const struct mtk_pll_data plls[] = {
 };
 
 static const struct mtk_gate apmixed_clks[] = {
-	GATE_APMIXED(CLK_APMIXED_MAIN_CORE_EN, "main_core_en", "mainpll", 5),
+	GATE_APMIXED_AO(CLK_APMIXED_MAIN_CORE_EN, "main_core_en", "mainpll", 5),
 };
 
 static const struct mtk_gate infra_clks[] = {
@@ -437,7 +442,7 @@ static const struct mtk_gate peri_clks[] = {
 	GATE_PERI0(CLK_PERI_PWM_PD, "peri_pwm_pd", "pwm_qtr_26m", 9),
 	GATE_PERI0(CLK_PERI_AP_DMA_PD, "peri_ap_dma_pd", "faxi", 12),
 	GATE_PERI0(CLK_PERI_MSDC30_1_PD, "peri_msdc30_1", "msdc30_1", 14),
-	GATE_PERI0(CLK_PERI_UART0_PD, "peri_uart0_pd", "faxi", 17),
+	GATE_PERI0_AO(CLK_PERI_UART0_PD, "peri_uart0_pd", "faxi", 17),
 	GATE_PERI0(CLK_PERI_UART1_PD, "peri_uart1_pd", "faxi", 18),
 	GATE_PERI0(CLK_PERI_UART2_PD, "peri_uart2_pd", "faxi", 19),
 	GATE_PERI0(CLK_PERI_UART3_PD, "peri_uart3_pd", "faxi", 20),
@@ -459,12 +464,12 @@ static struct mtk_composite infra_muxes[] = {
 
 static struct mtk_composite top_muxes[] = {
 	/* CLK_CFG_0 */
-	MUX_GATE(CLK_TOP_AXI_SEL, "axi_sel", axi_parents,
-		 0x040, 0, 3, 7),
-	MUX_GATE(CLK_TOP_MEM_SEL, "mem_sel", mem_parents,
-		 0x040, 8, 1, 15),
-	MUX_GATE(CLK_TOP_DDRPHYCFG_SEL, "ddrphycfg_sel", ddrphycfg_parents,
-		 0x040, 16, 1, 23),
+	MUX_GATE_FLAGS(CLK_TOP_AXI_SEL, "axi_sel", axi_parents,
+		       0x040, 0, 3, 7, CLK_IS_CRITICAL),
+	MUX_GATE_FLAGS(CLK_TOP_MEM_SEL, "mem_sel", mem_parents,
+		       0x040, 8, 1, 15, CLK_IS_CRITICAL),
+	MUX_GATE_FLAGS(CLK_TOP_DDRPHYCFG_SEL, "ddrphycfg_sel", ddrphycfg_parents,
+		       0x040, 16, 1, 23, CLK_IS_CRITICAL),
 	MUX_GATE(CLK_TOP_ETH_SEL, "eth_sel", eth_parents,
 		 0x040, 24, 3, 31),
 	/* CLK_CFG_1 */
@@ -568,10 +573,6 @@ static int mtk_topckgen_init(struct platform_device *pdev)
 				    ARRAY_SIZE(top_muxes), base,
 				    &mt7629_clk_lock, clk_data);
 
-	clk_prepare_enable(clk_data->hws[CLK_TOP_AXI_SEL]->clk);
-	clk_prepare_enable(clk_data->hws[CLK_TOP_MEM_SEL]->clk);
-	clk_prepare_enable(clk_data->hws[CLK_TOP_DDRPHYCFG_SEL]->clk);
-
 	return of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
 }
 
@@ -620,8 +621,6 @@ static int mtk_pericfg_init(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	clk_prepare_enable(clk_data->hws[CLK_PERI_UART0_PD]->clk);
-
 	return 0;
 }
 
@@ -640,9 +639,6 @@ static int mtk_apmixedsys_init(struct platform_device *pdev)
 	mtk_clk_register_gates(&pdev->dev, node, apmixed_clks,
 			       ARRAY_SIZE(apmixed_clks), clk_data);
 
-	clk_prepare_enable(clk_data->hws[CLK_APMIXED_ARMPLL]->clk);
-	clk_prepare_enable(clk_data->hws[CLK_APMIXED_MAIN_CORE_EN]->clk);
-
 	return of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
 }
 
-- 
2.51.0

Re: [PATCH] clk: mediatek: mt7629: Properly use CLK_IS_CRITICAL flag

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Replacing MUX_GATE with MUX_GATE_FLAGS to add CLK_IS_CRITICAL accidentally drops the CLK_SET_RATE_PARENT flag.

Pre-existing issues:
- [High] Pre-existing probe error paths leave registered composite clocks pointing to unmapped devm MMIO memory.
--

--- Patch [1]: [PATCH] clk: mediatek: mt7629: Properly use CLK_IS_CRITICAL flag ---
commit 737e44fe2aa5421b872238fdc129088bbb8750ca
Author: Ruoyu Wang <ruoyuw560@gmail.com>

clk: mediatek: mt7629: Properly use CLK_IS_CRITICAL flag

The MT7629 clock driver previously used manual clk_prepare_enable()
calls to keep essential clocks running. This patch replaces those calls
with the CLK_IS_CRITICAL flag to prevent references from leaking if
provider registration fails.

> diff --git a/drivers/clk/mediatek/clk-mt7629.c b/drivers/clk/mediatek/clk-mt7629.c
> index e154771b1b8bb..627e8017fa642 100644
> --- a/drivers/clk/mediatek/clk-mt7629.c
> +++ b/drivers/clk/mediatek/clk-mt7629.c

[ ... ]

> @@ -459,12 +464,12 @@ static struct mtk_composite infra_muxes[] = {
>  
>  static struct mtk_composite top_muxes[] = {
>  	/* CLK_CFG_0 */
> -	MUX_GATE(CLK_TOP_AXI_SEL, "axi_sel", axi_parents,
> -		 0x040, 0, 3, 7),
> -	MUX_GATE(CLK_TOP_MEM_SEL, "mem_sel", mem_parents,
> -		 0x040, 8, 1, 15),
> -	MUX_GATE(CLK_TOP_DDRPHYCFG_SEL, "ddrphycfg_sel", ddrphycfg_parents,
> -		 0x040, 16, 1, 23),
> +	MUX_GATE_FLAGS(CLK_TOP_AXI_SEL, "axi_sel", axi_parents,
> +		       0x040, 0, 3, 7, CLK_IS_CRITICAL),

[Severity: High]
Does this unintentionally drop the CLK_SET_RATE_PARENT flag?

Looking at the macro definitions, MUX_GATE implicitly includes
CLK_SET_RATE_PARENT:

#define MUX_GATE(_id, _name, _parents, _reg, _shift, _width, _gate) \
	MUX_GATE_FLAGS(_id, _name, _parents, _reg, _shift, _width,  \
		_gate, CLK_SET_RATE_PARENT)

By replacing MUX_GATE with MUX_GATE_FLAGS and passing only CLK_IS_CRITICAL,
the CLK_SET_RATE_PARENT flag is no longer applied. Could this prevent 
frequency scaling requests from propagating to the parent PLLs for these 
critical top-level buses?

> +	MUX_GATE_FLAGS(CLK_TOP_MEM_SEL, "mem_sel", mem_parents,
> +		       0x040, 8, 1, 15, CLK_IS_CRITICAL),
> +	MUX_GATE_FLAGS(CLK_TOP_DDRPHYCFG_SEL, "ddrphycfg_sel", ddrphycfg_parents,
> +		       0x040, 16, 1, 23, CLK_IS_CRITICAL),
>  	MUX_GATE(CLK_TOP_ETH_SEL, "eth_sel", eth_parents,
>  		 0x040, 24, 3, 31),

[ ... ]

> @@ -568,10 +573,6 @@ static int mtk_topckgen_init(struct platform_device *pdev)
>  				    ARRAY_SIZE(top_muxes), base,
>  				    &mt7629_clk_lock, clk_data);
>  
> -	clk_prepare_enable(clk_data->hws[CLK_TOP_AXI_SEL]->clk);
> -	clk_prepare_enable(clk_data->hws[CLK_TOP_MEM_SEL]->clk);
> -	clk_prepare_enable(clk_data->hws[CLK_TOP_DDRPHYCFG_SEL]->clk);
> -
>  	return of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
>  }

[Severity: High]
This is a pre-existing issue, but does this error path leak registered
composite clocks pointing to unmapped memory?

If of_clk_add_hw_provider() fails, mtk_topckgen_init() returns an error,
which causes driver probe to fail. The devres framework will then unmap
the IO memory (base).

However, the clocks registered via mtk_clk_register_composites() earlier in
the function are never unregistered. Will subsequent interactions with the
Common Clock Framework (like reading /sys/kernel/debug/clk/clk_summary)
access the now unmapped IO memory and cause a page fault?

This exact same pattern also appears in mtk_pericfg_init().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813153113.3952621-1-ruoyuw560@gmail.com?part=1