Re: [PATCH v3 3/3] clk: socfpga: stratix10: 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:
- [Medium] clk: socfpga: data race in early provider hws array modification
--

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

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

This commit changes the Stratix10 clock manager to register the l4_sp_clk
and its parents early via CLK_OF_DECLARE_DRIVER(). The remaining clocks
are deferred and registered later when the platform driver probes. The clock
manager structure, including the hardware clock array, is reused between
the early and late init phases.

> diff --git a/drivers/clk/socfpga/clk-s10.c b/drivers/clk/socfpga/clk-s10.c
> --- a/drivers/clk/socfpga/clk-s10.c
> +++ b/drivers/clk/socfpga/clk-s10.c

[ ... ]

> +static int s10_clkmgr_init(struct platform_device *pdev)
> +{
> +	struct stratix10_clock_data *clk_data;
> +	int i, ret;
> +
> +	if (!s10_clk_data) {
> +		ret = s10_clkmgr_setup(pdev->dev.of_node);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	clk_data = s10_clk_data;
>  
> +	s10_clk_register_c_perip(s10_main_perip_c_clks,
> +				 ARRAY_SIZE(s10_main_perip_c_clks), clk_data);

[Severity: Medium]
Does this introduce a data race with concurrent clock consumers?

Because s10_clkmgr_early_init() previously exposed the clock provider array
by calling of_clk_add_hw_provider(), the array can now be read locklessly
by other threads via of_clk_hw_onecell_get() while consumer drivers probe.

When s10_clk_register_c_perip() runs here, it writes newly registered clock
pointers directly into the array:

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

Modifying this array using standard C assignments without annotations like
WRITE_ONCE() or a lock while it is exposed to readers constitutes an
unannotated data race under the Linux Kernel Memory Model. This could 
potentially lead to compiler tearing or KCSAN splats.

[ ... ]

> +	for (i = 0; i < STRATIX10_NUM_CLKS; i++)
> +		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
> +			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);

[Severity: Medium]
Could modifying these deferral pointers concurrently cause issues for readers?

The same exposed clk_data->clk_data.hws array is modified directly without
WRITE_ONCE(). A concurrent reader accessing the array via
of_clk_hw_onecell_get() could observe partial updates or inconsistent states.

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

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