Re: [PATCH RFC 4/4] clk: zte: Introduce a driver for zx297520v3 LSP clocks and resets.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Incorrect error checking for devm_platform_ioremap_resource() leading to kernel panic.
- [High] The clock provider is registered without devres, leading to a Use-After-Free.
- [High] PCLK and WCLK gate shifts are swapped in the LSP_CLOCK macro.
- [High] Unprotected read-modify-write data race on shared LSP hardware registers.
- [Low] Incompatible pointer type for parent_names array.
--

commit a23190a3a7795f784d87109b7d775267f0845b7b
Author: Stefan Dösinger <stefandoesinger@gmail.com>

clk: zte: Introduce a driver for zx297520v3 LSP clocks and resets.

"LSP" is ZTE's term for this part of the SoC, I suspect it stands for
"low speed peripherals". The main UART is here, together with the flash
controller and more surplus proprietary timers.

> diff --git a/drivers/clk/zte/clk-zx297520v3.c b/drivers/clk/zte/clk-zx297520v3.c
> index 0c06add433ee9..e69be686c1b1d 100644
> --- a/drivers/clk/zte/clk-zx297520v3.c
> +++ b/drivers/clk/zte/clk-zx297520v3.c
[ ... ]
> +/* LSP clock entries have a common pattern: Bit 0 for PCLK, Bit 1 for WCLK. Bit 4 (and sometimes
> + * more) for WCLK mux.
[ ... ]
> +#define LSP_CLOCK(offset, name, mux, div_shift, div_size) {\
> +		ZX297520V3_##name##_RESET, ZX297520V3_##name##_WCLK, ZX297520V3_##name##_PCLK,\
> +		#name, offset, 8, offset, 0, 1, "lsp_pclk", offset, 4, 4, mux, ARRAY_SIZE(mux),\
> +		offset, div_shift, div_size, 0}

The comment above states that Bit 0 is for PCLK and Bit 1 is for WCLK.
However, passing 0 and 1 to this macro seems to map to wclk_gate_shift and
pclk_gate_shift in struct zx297520v3_composite.

Does this accidentally assign wclk_gate_shift to 0 and pclk_gate_shift to 1,
swapping the gate controls?

Also, the clock gate, mux, and reset controls are assigned to the exact same
memory offset (a shared 32-bit register).

The clock framework uses reg_lock for read-modify-write operations. Do the
reset controller operations (like zx297520v3_rst_assert) use the same lock?
If they read and write these registers locklessly, could concurrent clock
and reset operations cause a data race resulting in lost writes?

> +static const struct zx297520v3_composite lsp_clocks[] =  {
[ ... ]
> +};
> +
> +#undef LSP_CLOCK
> +
> +static int zx297520_lspclk_probe(struct platform_device *pdev)
> +{
> +	static const char * const *parent_names[] = { "mpll_d5", "mpll_d4", "mpll_d6", "mpll_d8",
> +						      "mpll_d12", "osc26m", "osc32k", "pclk" };

Does this array declaration use an incompatible pointer type?

The array is declared as an array of pointers to const char *, but is
initialized directly with string literals. When parent_names[i] is passed to
devm_clk_get() below, it passes a pointer-to-pointer instead of a string.
This can trigger build errors on compilers like GCC 14.

Should this be declared as static const char * const parent_names[] instead?

> +
> +	struct zx29_clk_controller *lsp;
[ ... ]
> +	lsp->clocks = devm_kzalloc(dev, struct_size(lsp->clocks, hws,
> +				   ZX297520V3_LSPCLK_END), GFP_KERNEL);
> +	if (!lsp->clocks)
> +		return -ENOMEM;
> +	lsp->clocks->num = ZX297520V3_LSPCLK_END;
> +
> +	lsp->base = devm_platform_ioremap_resource(pdev, 0);
> +	WARN_ON(!lsp->base);

Since devm_platform_ioremap_resource() returns an ERR_PTR on failure rather
than NULL, this WARN_ON() will not catch failures because an ERR_PTR
evaluates to true.

If the invalid error pointer is then passed into zx297520v3_composite()
as the base address, will this lead to a kernel panic when read/write
operations occur?

Should this use IS_ERR() and return PTR_ERR() instead?

> +
> +	/* TODO: Technically we can disable the pclk if all LSP devices are shut down, but that
[ ... ]
> +	res = zx297520v3_composite(dev, lsp->base, lsp->clocks, lsp->resets,
> +				 lsp_clocks, ARRAY_SIZE(lsp_clocks));
> +	if (res)
> +		return res;
> +
> +	res = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, lsp->clocks);

Since lsp->clocks is allocated using devm_kzalloc(), it will be
automatically freed if devm_reset_controller_register() fails later or if
the driver is unbound.

Because the clock provider is registered with of_clk_add_hw_provider()
instead of the managed devm_of_clk_add_hw_provider(), could the provider
remain registered to the device node after lsp->clocks is freed?

Will this leave a dangling pointer that triggers a use-after-free upon the
next clock lookup?

> +	if (res)
> +		return res;
> +
> +	lsp->rcdev.owner = THIS_MODULE;
> +	lsp->rcdev.nr_resets = ZX297520V3_LSPRST_END;
> +	lsp->rcdev.ops = &zx297520v3_rst_ops;
> +	lsp->rcdev.of_node = dev->of_node;
> +	return devm_reset_controller_register(dev, &lsp->rcdev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-zx29clk-v1-0-e1bacfffe967@gmail.com?part=4