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

[sashiko-bot@kernel.org]

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