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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: socfpga: lockless publication of clk_hw pointers without memory barriers
- [Medium] clk: socfpga: silent failure on clock registration OOM due to missing NULL check
--

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

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

The DW APB timers are probed from time_init() and cannot defer. This commit
registers l4_sp_clk and its parents early using CLK_OF_DECLARE_DRIVER(),
leaving the rest as -EPROBE_DEFER, which are subsequently registered when
the platform driver probes.

> diff --git a/drivers/clk/socfpga/clk-agilex.c b/drivers/clk/socfpga/clk-agilex.c
> index 2bdea1997b5ef..445d814e2f3ac 100644
> --- a/drivers/clk/socfpga/clk-agilex.c
> +++ b/drivers/clk/socfpga/clk-agilex.c
[ ... ]
> @@ -454,33 +472,166 @@ static int n5x_clk_register_pll(const struct stratix10_pll_clock *clks,
[ ... ]
> +static int agilex_clkmgr_add_provider(struct device_node *np)
> +{
> +	static bool added;
> +	int ret;
> +
> +	if (added)
> +		return 0;
> +
> +	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
> +				     &agilex_clk_data->clk_data);

[Severity: High]
Does exposing the clock data array here during early boot allow
concurrent readers to see uninitialized memory?

The global array is exposed via of_clk_add_hw_provider() before all
clocks are populated. Later, during the platform driver probe in
agilex_clkmgr_init(), the remaining clocks are initialized and their
pointers are published locklessly:

drivers/clk/socfpga/clk-agilex.c:agilex_clk_register_c_perip() {
	...
	hw_clk = s10_register_periph(&clks[i], base);
	...
	data->clk_data.hws[clks[i].id] = hw_clk;
}

Since the array is mutated without memory barriers like smp_store_release()
or shared locks, could weakly ordered architectures reorder the publication
of the pointer before the initialization of the memory?

If a concurrent consumer requests a clock during this window, they might read
the new hw_clk pointer but observe uninitialized memory inside the clk_hw
structure, potentially leading to data corruption or a kernel panic.

[ ... ]
> +static int agilex_clkmgr_finish(struct platform_device *pdev,
> +				struct stratix10_clock_data *clk_data)
> +{
> +	int i;
> +
> +	for (i = 0; i < AGILEX_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]
Is it safe to mutate the globally exposed hws array locklessly here?

This loop is running during the delayed platform probe, modifying the array
while consumers might be concurrently accessing it.

> +
> +	return agilex_clkmgr_add_provider(pdev->dev.of_node);
> +}
> +
> +static int agilex_clkmgr_init(struct platform_device *pdev)
> +{
> +	struct stratix10_clock_data *clk_data;
> +
> +	clk_data = agilex_clkmgr_get_clk_data(pdev, agilex_clk_register_early);
> +	if (IS_ERR(clk_data))
> +		return PTR_ERR(clk_data);
> +
>  	agilex_clk_register_c_perip(agilex_main_perip_c_clks,
>  				 ARRAY_SIZE(agilex_main_perip_c_clks), clk_data);

[Severity: Medium]
Does the missing NULL check in agilex_clk_register_c_perip() cause a silent
failure on memory allocation errors?

In agilex_clk_register_c_perip(), the return value of s10_register_periph()
is checked with IS_ERR():

drivers/clk/socfpga/clk-agilex.c:agilex_clk_register_c_perip() {
	...
	hw_clk = s10_register_periph(&clks[i], base);
	if (IS_ERR(hw_clk)) {
		pr_err("%s: failed to register clock %s\n",
		       __func__, clks[i].name);
		continue;
	}
	data->clk_data.hws[clks[i].id] = hw_clk;
}

However, s10_register_periph() returns NULL on memory allocation failure.
This means the IS_ERR() check evaluates to false, failing to catch the error.

Could this cause NULL to silently overwrite the original -EPROBE_DEFER in the
hws array? If consumers request this clock, they would receive a NULL clock
pointer, which the common clock framework treats as a valid optional dummy
clock. This masks the initialization failure and might allow dependent hardware
to proceed without being clocked.

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