Re: [PATCH v3 2/3] clk: socfpga: agilex5: register the SP timer clocks early

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: data race and missing barriers when mutating exposed hws array
--

commit b3256fb3551a9baf9b48df9a42da38b26efc5524
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

clk: socfpga: agilex5: register the SP timer clocks early

The DW APB timers are probed through TIMER_OF_DECLARE() from time_init()
and cannot defer, but the clock manager is a platform driver registered
at core_initcall(). The timer's clk_get() therefore fails and the timer
is never brought up.

Register only l4_sp_clk and its parents (boot_clk, main_pll, periph_pll,
main_pll_c3, peri_pll_c1 and noc_free_clk) from of_clk_init() using
CLK_OF_DECLARE_DRIVER(), with every other clock left as -EPROBE_DEFER.
The remaining clocks are still registered when the platform driver
probes, which reuses the early provider.

> diff --git a/drivers/clk/socfpga/clk-agilex5.c b/drivers/clk/socfpga/clk-agilex5.c
> index f7f0ad884f645..50dba11ab45ec 100644
> --- a/drivers/clk/socfpga/clk-agilex5.c
> +++ b/drivers/clk/socfpga/clk-agilex5.c

[ ... ]

> @@ -599,22 +620,25 @@ static int agilex5_clkmgr_init(struct platform_device *pdev)
>  {
>  	struct stratix10_clock_data *clk_data;
>  	int i, ret;
>  
>  	if (!agilex5_clk_data) {
>  		ret = agilex5_clkmgr_setup(pdev->dev.of_node);
>  		if (ret)
>  			return ret;
>  	}
>  
>  	clk_data = agilex5_clk_data;
>  
>  	agilex5_clk_register_c_perip(agilex5_main_perip_c_clks,
>  				     ARRAY_SIZE(agilex5_main_perip_c_clks),
>  				     clk_data);
>  
>  	agilex5_clk_register_cnt_perip(agilex5_main_perip_cnt_clks,
>  				       ARRAY_SIZE(agilex5_main_perip_cnt_clks),
>  				       clk_data);
>  
>  	agilex5_clk_register_gate(agilex5_gate_clks,
>  				  ARRAY_SIZE(agilex5_gate_clks), clk_data);

[Severity: High]
Since agilex5_clkmgr_early_init() publishes the hws array early via
of_clk_add_hw_provider(), can concurrent clock consumers access this array
while agilex5_clkmgr_init() is mutating it?

When agilex5_clk_register_c_perip() and agilex5_clk_register_gate() are
called here, they write newly allocated clocks directly into the shared
array:

agilex5_clk_register_c_perip() {
    ...
    data->clk_data.hws[clks[i].id] = hw_clk;
    ...
}

Because these assignments lack smp_store_release() and are not protected by
of_clk_mutex, could a consumer's of_clk_hw_onecell_get() observe the hw_clk
pointer before its initialization stores are visible in memory, leading to a
NULL pointer dereference or memory corruption?

> -	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
> -	return 0;
> +	for (i = 0; i < AGILEX5_NUM_CLKS; i++)
> +		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
> +			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);

[Severity: High]
Can this loop mutate the exposed array concurrently with potential readers,
since it doesn't hold of_clk_mutex?

> +
> +	return agilex5_clkmgr_add_provider(pdev->dev.of_node);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791871090.git.adrian.ho.yin.ng@altera.com?part=2