Re: [PATCH RFC v2 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 7 potential issue(s) to consider:
- [High] The driver cannot be loaded as a module because it uses the unexported internal function `__clk_lookup()`.
- [High] The driver leaks the OF clock provider registration on probe failure or module unload, causing a Use-After-Free.
- [High] The driver checks for MMIO remapping failure using `!top->base` instead of `IS_ERR()`, leading to a guaranteed kernel crash if mapping fails.
- [High] The reset controller's assert and deassert operations perform unprotected read-modify-write (RMW) operations on shared hardware registers.
- [High] The reset operations do not verify if a requested reset ID corresponds to an initialized register pointer before dereferencing it.
- [Medium] The `zx297520v3_rst_status` function returns inverted status logic.
- [Medium] The Kconfig symbol `COMMON_CLK_ZX297520V3` depends on and selects non-existent architecture and SoC configurations.
--

commit 907b20d81591d5c194abf35b53c48cd6ec44122c
Author: Stefan Dösinger <stefandoesinger@gmail.com>
clk: zte: Introduce a driver for zx297520v3 top clocks and resets.

This patch introduces a clock and reset driver for the ZTE zx297520v3 SoC top
clocks. It provides basic clock gating, multiplexing, and reset controls for
various core devices like PLLs, timers, and USB. The driver also registers a
restart handler for the board.

> diff --git a/drivers/clk/zte/Kconfig b/drivers/clk/zte/Kconfig
> new file mode 100644
> index 0000000000000..e7acd28832cd7
> --- /dev/null
> +++ b/drivers/clk/zte/Kconfig
> @@ -0,0 +1,18 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# ZTE Clock Drivers
> +#
> +menu "Clock driver for ZTE SoC"
> +	depends on ARCH_ZTE || COMPILE_TEST
> +
> +config COMMON_CLK_ZX297520V3
> +	tristate "Clock driver for ZTE zx297520v3"
> +	default SOC_ZX297520V3

Are ARCH_ZTE and SOC_ZX297520V3 defined anywhere in the kernel tree? It
appears these config symbols do not exist, which might hide this driver unless
COMPILE_TEST is explicitly enabled.

> diff --git a/drivers/clk/zte/clk-zx297520v3.c b/drivers/clk/zte/clk-zx297520v3.c
> new file mode 100644
> index 0000000000000..aa304dd34b7ba
> --- /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);

Could an unpopulated reset ID cause a NULL pointer dereference here? Only a
subset of reset IDs seem to be populated with register pointers in the probe
function. If a consumer requests an uninitialized ID, this might pass a NULL
pointer to readl().

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

Can these read-modify-write operations race with clock gate operations? It
looks like multiple reset components and clock gates share the same physical
register (e.g. offset 0x74), but these reset callbacks don't acquire reg_lock
before modifying the register.

[ ... ]
> +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 status logic inverted here? The reset controller API expects the status
callback to return a non-zero value when the reset line is asserted, but this
appears to return 1 when the reset is deasserted based on the logic in the
assert and deassert callbacks.

[ ... ]
> +static int zx297520_topclk_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	top->base = devm_platform_ioremap_resource(pdev, 0);
> +	WARN_ON(!top->base);

Does devm_platform_ioremap_resource() return an error pointer on failure
instead of NULL?

If it fails, checking for !top->base will evaluate to false and the driver
will proceed to pass an error pointer to zx297520v3_pll(), which might
cause a crash when readl() dereferences it.

[ ... ]
> +	/* 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 when built as a module? The driver is
configured as tristate, but __clk_lookup() is an internal function not
exported to modules.

Additionally, should this be called with the clock framework prepare_lock
held to avoid races against concurrent clock registrations?

[ ... ]
> +	hw = devm_clk_hw_register_mux(dev, "m0_wclk", m0_sel, ARRAY_SIZE(m0_sel),
> +				      0, top->base + 0x38, 0, 2, 0, &reg_lock);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +	top->clocks->hws[ZX297520V3_M0_WCLK] = hw;
> +
> +	of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, top->clocks);

Does this code leak the OF clock provider on probe failure or module unload?

Since it uses of_clk_add_hw_provider() instead of the managed
devm_of_clk_add_hw_provider(), a failure later in the probe function might
leave a dangling pointer in the system's global clock provider list, resulting
in a use-after-free during subsequent clock lookups.

> +
> +	res = devm_register_restart_handler(dev, zx_restart_handle, top);

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