Re: [PATCH RFC v2 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 handling for devm_platform_ioremap_resource() causes invalid pointer dereference.
- [High] Missing OF clock provider unregistration causes a Use-After-Free.
- [High] PCLK and WCLK gating shift bits are swapped in the LSP_CLOCK macro.
- [Low] Redundant clock request for "pclk" wastes devres memory.
- [Low] Missing dev_err_probe() for parent clock retrieval.
--

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

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

This commit introduces a new clock and reset driver for the ZTE
zx297520v3 low speed peripherals (LSP) block, which includes UART,
timers, watchdogs, I2C, SPI, and other proprietary controllers.

> diff --git a/drivers/clk/zte/clk-zx297520v3.c b/drivers/clk/zte/clk-zx297520v3.c
> index d4b683cb63544..05b02c8f266be 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 'Bit 0 for PCLK, Bit 1 for WCLK'.

Based on the struct layout:

struct zx297520v3_composite {
    ...
    u32 gate_reg, wclk_gate_shift, pclk_gate_shift;
    ...
}

The macro assigns 0 to wclk_gate_shift and 1 to pclk_gate_shift. Are the gating
shift bits for the peripheral and work clocks swapped here, preventing the
hardware clocks from being enabled correctly?

[ ... ]
> +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" };
> +
> +	struct zx29_clk_controller *lsp;
[ ... ]
> +	lsp->base = devm_platform_ioremap_resource(pdev, 0);
> +	WARN_ON(!lsp->base);

Does this correctly handle the error return value?

devm_platform_ioremap_resource() returns an ERR_PTR on failure, not NULL. Will
this check evaluate to false for an error pointer, causing an invalid pointer
dereference when lsp->base is used later in zx297520v3_composite()?

> +
> +	/* TODO: Technically we can disable the pclk if all LSP devices are shut down, but that
> +	 * needs custom clk ops to tiptoe around a disabled LSP pclk before attempting to access
> +	 * the actual clock. In normal operation it is unlikely that all LSP devices are shut down
> +	 * simultaneously though as UART and NAND are located here.
> +	 */
> +	parent = devm_clk_get_enabled(dev, "pclk");
> +	if (IS_ERR(parent)) {
> +		dev_err(dev, "failed to find lsp pclk\n");
> +		return PTR_ERR(parent);
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(parent_names); ++i) {
> +		parent = devm_clk_get(dev, parent_names[i]);

This isn't a bug, but pclk was already requested and enabled earlier via
devm_clk_get_enabled().

Since pclk is also included in the parent_names array, it will be requested a
second time here. Is it intended to take a second devres reference to this
clock?

> +		if (IS_ERR(parent)) {
> +			dev_err(dev, "failed to find lsp %s clock\n", parent_names[i]);

Since devm_clk_get() can return -EPROBE_DEFER if a parent clock is not yet
initialized, will using dev_err() here spam the kernel log during normal probe
deferral?

Would dev_err_probe() be a better fit to silently handle deferrals?

> +			return PTR_ERR(parent);
> +		}
> +	}
> +
> +	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);
> +	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);
> +}

If devm_reset_controller_register() fails, or if the driver is unbound later,
does this leave a dangling pointer in the common clock framework?

The lsp->clocks array is managed by devres and will be freed, but the clock
provider registered with of_clk_add_hw_provider() is not automatically
unregistered.

Would using devm_of_clk_add_hw_provider() or adding an error path and a .remove
callback prevent a potential use-after-free when another device attempts to
access the clock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-zx29clk-v2-0-29f0edc300f5@gmail.com?part=4