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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The driver uses the internal `__clk_lookup` function, which is not exported to modules, causing a linker error.
- [High] The reset controller operations lack locking, leading to read-modify-write race conditions on shared registers.
- [High] The clock provider is registered using `of_clk_add_hw_provider()` but never deregistered, leading to a Use-After-Free vulnerability.
- [High] Reset controller callbacks dereference unpopulated reset IDs without validation, leading to a NULL pointer dereference.
- [High] Incorrect error checking for `devm_platform_ioremap_resource()` leads to an unhandled `ERR_PTR` dereference.
- [High] The reset controller's `.status` callback returns inverted logic compared to the reset API requirements.
--

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

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

This commit introduces a driver for the ZTE zx297520v3 top clocks and
resets, which controls core devices like PLLs, the AHB bus, and timers.

> diff --git a/drivers/clk/zte/clk-zx297520v3.c b/drivers/clk/zte/clk-zx297520v3.c
> new file mode 100644
> index 0000000000000..f73f5c006641f
> --- /dev/null
> +++ b/drivers/clk/zte/clk-zx297520v3.c

[ ... ]

> +static int zx297520v3_rst_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct zx29_clk_controller *data = container_of(rcdev, struct zx29_clk_controller, rcdev);
> +	u32 val;
> +
> +	val = readl(data->resets[id].reg);

Does this code safely handle unpopulated reset IDs?

The resets array is zero-initialized via devm_kzalloc(), and only specific IDs
have their .reg address populated in the probe function. If a consumer requests
an unpopulated ID, could data->resets[id].reg be NULL, leading to a NULL
pointer dereference here?

Additionally, does this read-modify-write sequence need locking?

Several reset IDs share the same physical register (e.g., offset 0x74 is used
by PMM, TIMER_T12, and others). If concurrent reset operations occur for
different devices on the same register, could the read-modify-write cycles
overlap and corrupt the register state?

> +	val &= ~data->resets[id].mask;
> +	writel(val, data->resets[id].reg);
> +
> +	return 0;
> +}

[ ... ]

> +static int zx297520v3_rst_status(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct zx29_clk_controller *data = container_of(rcdev, struct zx29_clk_controller, rcdev);
> +	u32 val;
> +
> +	val = readl(data->resets[id].reg);
> +
> +	return (val & data->resets[id].mask) == data->resets[id].mask;
> +}

Is the return value of this status callback inverted?

In zx297520v3_rst_assert(), the reset is asserted by clearing the mask bits.
However, this status function returns 1 when the bits are set (deasserted) and
0 when the bits are cleared (asserted). The reset API expects the status
callback to return a positive value when the reset line is asserted.

[ ... ]

> +static int zx297520_topclk_probe(struct platform_device *pdev)
> +{
> +	struct zx29_clk_controller *top;
> +	struct device *dev = &pdev->dev;
> +	struct clk_hw *hw;
> +	struct clk *clk;
> +	int res;

[ ... ]

> +	top->base = devm_platform_ioremap_resource(pdev, 0);
> +	WARN_ON(!top->base);

Does devm_platform_ioremap_resource() return NULL on failure?

This function returns an ERR_PTR (such as ERR_PTR(-EINVAL)) on failure, not
NULL. If mapping fails, the WARN_ON check will pass and the error pointer will
be used as a base address in subsequent calls like zx297520v3_pll(), causing
an invalid memory access. Should this be checked with IS_ERR() instead?

[ ... ]

> +	/* The documentation says 491.52 MHz and measurement with the LSP TDM device supports this.
> +	 * The default value is 0x480C2011. To get to 491.52 with these settings it needs a 23.04
> +	 * MHz reference clock, which matches unknownpll_d2. If unknownpll is disabled, dpll loses
> +	 * its lock.
> +	 *
> +	 * The proprietary LTE driver or coproc enables and disables it. TDM and I2S can use it.
> +	 *
> +	 * FIXME: Isn't there a nicer way to get the struct clk for unknownpll_d2? I don't want to
> +	 * return all generated clocks from zx297520v3_pll or store them in the controller because
> +	 * I need one of them here. I could always pass the parent by name though.
> +	 */
> +	res = zx297520v3_pll(dev, top->base + 0x18, "dpll", __clk_lookup("unknownpll_d2"));

Will this cause a linker error if the driver is built as a module?

The Kconfig defines COMMON_CLK_ZX297520V3 as a tristate option, but
__clk_lookup() is an internal clock framework function that is not exported.
If the driver is compiled as a module, this might cause an undefined symbol
linker error for __clk_lookup.

[ ... ]

> +	of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, top->clocks);

Is there a missing cleanup for this clock provider?

The driver registers the clock provider with of_clk_add_hw_provider(), but
doesn't seem to use the managed devm_of_clk_add_hw_provider() or provide a
.remove callback to deregister it.

If a later probe step (like devm_reset_controller_register()) fails, or if the
driver is unbound, top->clocks will be freed by devres. Could this leave a
dangling pointer in the global of_clk_providers list and cause a
use-after-free?

[ ... ]

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