Re: [PATCH v6 2/3] clk: en7523: generalize register clocks function

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Fragile loop boundary logic using `num_clocks - 1` risks out-of-bounds array access.

Pre-existing issues:
- [High] The PCIe clock gate is registered with an unmanaged API, leading to a Use-After-Free if probe fails.
- [High] Missing spinlock in reset controller update leads to data races and lost writes.
--

--- Patch [2]: [PATCH v6 2/3] clk: en7523: generalize register clocks function ---
commit c89fd9565e1664803408c338e740e001e5bb4ff0
Author: Christian Marangi <[email protected]>

clk: en7523: generalize register clocks function

This commit generalizes the clock registration logic for the EN7581 SoC
so it can be reused by the upcoming AN7583 SoC. It achieves this by
moving the base clocks array definition into the soc_data structure.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 1ab0e2eca5d32..087ff4568124f 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c

[ ... ]

> @@ -711,12 +712,15 @@ static int en7523_clk_hw_init(struct platform_device *pdev,
>  static void en7581_register_clocks(struct device *dev, struct clk_hw_onecell_data *clk_data,
>  				   struct regmap *map, void __iomem *base)
>  {
> +	const struct en_clk_soc_data *soc_data;
>  	struct clk_hw *hw;
>  	u32 rate;
>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(en7581_base_clks); i++) {
> -		const struct en_clk_desc *desc = &en7581_base_clks[i];
> +	soc_data = device_get_match_data(dev);
> +
> +	for (i = 0; i < soc_data->num_clocks - 1; i++) {
> +		const struct en_clk_desc *desc = &soc_data->base_clks[i];

[Severity: Medium]
If a future SoC defines a clock provider count (num_clocks) that doesn't
exactly equal the number of elements in base_clks plus one, could this
loop iterate past the end of the base_clks array?

It seems this loop derives the base_clks array length from
soc_data->num_clocks - 1, mathematically conflating the total clock count
with the base clock array size.

While reviewing this, I noticed a couple of pre-existing issues in the same
file:

drivers/clk/clk-en7523.c:en7523_register_pcie_clk() {
    ...
	cg = devm_kzalloc(dev, sizeof(*cg), GFP_KERNEL);
	if (!cg)
		return NULL;
    ...
	if (clk_hw_register(dev, &cg->hw))
		return NULL;
    ...
}

[Severity: High]
This is a pre-existing issue, but since the clock gate structure is allocated
using devres, won't it be automatically freed if probe fails?

Because the clock is registered with the non-managed clk_hw_register() API,
if a downstream operation during probe fails, the driver's probe will return
an error and trigger devres cleanup. Could this leave a dangling pointer
inside the Common Clock Framework and lead to a Use-After-Free?


drivers/clk/clk-en7523.c:en7523_reset_update() {
    ...
	val = readl(addr);
	if (assert)
		val |= BIT(id % RST_NR_PER_BANK);
	else
		val &= ~BIT(id % RST_NR_PER_BANK);
	writel(val, addr);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to perform this
read-modify-write operation on a shared register without a spinlock?

The reset core does not serialize .assert and .deassert operations, so if
multiple consumer drivers concurrently assert or deassert resets within the
same 32-bit register bank, it seems this unprotected read-modify-write
could lead to data races and lost writes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2