Re: [PATCH v2 28/35] clk: mediatek: mt8173-infracfg: Switch to common probe/remove helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: mediatek: early timer initialization breaks due to removal of CLK_OF_DECLARE_DRIVER
- [High] clk: mediatek: out-of-bounds array write in mtk_clk_simple_probe due to sparse IDs
--

commit cabfa228cff0aa82afbc9489448b517cc6478e89
Author: Akari Tsuyukusa <akkun11.open@gmail.com>

clk: mediatek: mt8173-infracfg: Switch to common probe/remove helpers

The MT8173 infracfg driver previously needed custom probe/remove
callbacks to register and unregister cpumux. With cpumux support now
available in the common helpers, the driver can use
mtk_clk_simple_probe() and mtk_clk_simple_remove().

> diff --git a/drivers/clk/mediatek/clk-mt8173-infracfg.c b/drivers/clk/mediatek/clk-mt8173-infracfg.c
> index 9b8c358977891..fdd994736e833 100644
> --- a/drivers/clk/mediatek/clk-mt8173-infracfg.c
> +++ b/drivers/clk/mediatek/clk-mt8173-infracfg.c

[ ... ]

> @@ -70,95 +68,29 @@ static const struct mtk_clk_rst_desc clk_rst_desc = {
>  	.rst_bank_nr = ARRAY_SIZE(infrasys_rst_ofs),
>  };
>  
> +static const struct mtk_clk_desc infracfg_desc = {
> +	.clks = infra_gates,
> +	.num_clks = ARRAY_SIZE(infra_gates),
> +	.factor_clks = infra_divs,
> +	.num_factor_clks = ARRAY_SIZE(infra_divs),
> +	.cpumuxes = cpu_muxes,
> +	.num_cpumuxes = ARRAY_SIZE(cpu_muxes),
> +	.rst_desc = &clk_rst_desc,
> +};

[Severity: High]
Does this code under-allocate the clock array by using the sum of element
counts rather than the maximum clock ID?

The transition to mtk_clk_simple_probe() calculates num_clks by summing the
array sizes in infracfg_desc (11 gates + 1 factor + 2 cpumuxes = 14 elements).
Consequently, mtk_alloc_clk_data(14) allocates exactly 14 elements.

However, mt8173-infracfg uses sparse IDs. For example, CLK_INFRA_CA72SEL is
defined as 14. When mtk_clk_register_cpumuxes() registers this cpumux, it
attempts to assign the clock pointer to clk_data->hws[14]:

drivers/clk/mediatek/clk-cpumux.c:mtk_clk_register_cpumuxes() {
    ...
		clk_data->hws[mux->id] = hw;
    ...
}

This performs an out-of-bounds write past the end of the allocation. The
previous custom probe avoided this by explicitly allocating CLK_INFRA_NR_CLK
elements.

>  static const struct of_device_id of_match_clk_mt8173_infracfg[] = {
> -	{ .compatible = "mediatek,mt8173-infracfg" },
> +	{ .compatible = "mediatek,mt8173-infracfg", .data = &infracfg_desc },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, of_match_clk_mt8173_infracfg);
>  
> -static void clk_mt8173_infra_init_early(struct device_node *node)
> -{
> -	int i;
> -
> -	infra_clk_data = mtk_alloc_clk_data(CLK_INFRA_NR_CLK);
> -	if (!infra_clk_data)
> -		return;
> -
> -	for (i = 0; i < CLK_INFRA_NR_CLK; i++)
> -		infra_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
> -
> -	mtk_clk_register_factors(infra_early_divs,
> -				 ARRAY_SIZE(infra_early_divs), infra_clk_data);
> -
> -	of_clk_add_hw_provider(node, of_clk_hw_onecell_get, infra_clk_data);
> -}
> -CLK_OF_DECLARE_DRIVER(mtk_infrasys, "mediatek,mt8173-infracfg",
> -		      clk_mt8173_infra_init_early);

[Severity: High]
Does removing CLK_OF_DECLARE_DRIVER break early timer initialization?

The MT8173 device tree defines a system timer matching mediatek,mt6577-timer,
which depends on CLK_INFRA_CLK_13M. This timer is initialized during early boot
via TIMER_OF_DECLARE running synchronously from timer_probe().

By removing CLK_OF_DECLARE_DRIVER from mt8173-infracfg, the infracfg clock
provider is deferred to the standard platform module probe phase. When
timer_probe() executes early in boot, of_clk_get() for CLK_INFRA_CLK_13M
returns -EPROBE_DEFER.

Since timer_probe() completely ignores deferred probing and never retries:

drivers/clocksource/timer-probe.c:timer_probe() {
    ...
		ret = init_func_ret(np);
		if (ret) {
			if (ret != -EPROBE_DEFER)
				pr_err("Failed to initialize '%pOF': %d\n", np, ret);
			continue;
		}
    ...
}

This causes the system to permanently lose its mt6577-timer (clocksource,
clockevents, and sched_clock), leading to a boot regression.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013151734.3177690-1-akkun11.open@gmail.com?part=28