Re: [PATCH RFC v4 07/12] clk: zte: Introduce a driver for zx297520v3 top clocks

[sashiko-bot@kernel.org]

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

New issues:
- [High] The 'dpll' and 'unknownpll' clocks are incorrectly configured with a single parent, leading to incorrect rate calculations and potential clock orphaning.

Pre-existing issues:
- [High] The `struct auxiliary_device` is allocated with `devm_kzalloc`, decoupling its memory lifecycle from its embedded kobject reference count.
- [High] The driver fails to properly validate the return value of `device_node_to_regmap()`, resulting in an invalid pointer dereference if the regmap cannot be obtained.
--

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

clk: zte: Introduce a driver for zx297520v3 top clocks

This commit introduces a new clock driver for the ZTE zx297520v3 SoC.
It adds support for the top clocks, including various PLLs, AHB, timers,
and USB controllers. The patch wires up the clock configuration structures
and registers the driver via the zx_clk_probe framework.

Signed-off-by: Stefan Dösinger <stefandoesinger@gmail.com>

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

[ ... ]

> +	/* Default value 0x4834902d. Feeds dpll. 46.08 MHz. Bit 25 can be set, so two parents are
> +	 * possible. It looks like both values select the 26 MHz oscillator though.
> +	 */
> +	{
> +		.id = 0,
> +		.name = "unknownpll",
> +		.parents = clk_main,
> +		.num_parents = ARRAY_SIZE(clk_main),

[Severity: High]
The documentation block here states this PLL has two possible parents, but it
is restricted to the single parent in clk_main. Does hardcoding a single parent
cause an out-of-bounds index lookup if the hardware selects a parent index > 0?

> +		.rate = 0,
> +		.postdivs = unknownpll_postdivs,
> +		.num_postdivs = ARRAY_SIZE(unknownpll_postdivs),
> +		.reg = 0x100,
> +	},
> +
> +	/* The documentation says 491.52 MHz and measurement with the LSP TDM device supports this.
> +	 * The default value is 0x480c2011, but not all boot loaders set it up. 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. We set the frequency on this PLL if we
> +	 * find it is not enabled by the boot loader.
> +	 *
> +	 * The proprietary LTE driver or coproc enables and disables it. TDM and I2S can use it.
> +	 *
> +	 * It accepts parent values 0, 1, 2 and 3. Parent 0 is unknownpll_d2. The others look like
> +	 * osc26m. With a parent != 0 dpll never loses its lock even when all other PLLs are off
> +	 * and the TDM counter register increases at a rate consistent with a 26.0/23.04 clock
> +	 * increase.
> +	 */
> +	{
> +		.id = ZX297520V3_DPLL,
> +		.name = "dpll",
> +		.parents = clk_main,
> +		.num_parents = ARRAY_SIZE(clk_main),

[Severity: High]
Similarly, the comment states this PLL accepts parent values 0, 1, 2, and 3,
with parent 0 being unknownpll_d2. Since clk_main only contains osc26m, does
restricting this to a single parent cause the clock framework to erroneously
compute all DPLL-derived frequencies using 26 MHz instead of 23.04 MHz?

> +		.rate = 491520000,
> +		.postdivs = pll_postdivs,
> +		.num_postdivs = ARRAY_SIZE(pll_postdivs),
> +		.reg = 0x18,
> +	},

[ ... ]

> +static struct platform_driver clk_zx297520v3 = {
> +	.probe = zx_clk_probe,

[Severity: High]
This is a pre-existing issue, but I noticed two concerns in zx_clk_probe()
located in clk-zx.c:

First, zx_clk_probe() calls device_node_to_regmap(dev->of_node) which
returns an ERR_PTR on failure. The code checks for failure using:

    if (!map)
        return -EINVAL;

Since an error pointer is non-null, will this check evaluate to false and
allow an invalid pointer to be dereferenced later?

Second, zx_clk_probe() allocates the auxiliary device using devm_kzalloc().
When the driver unbinds, devres executes zx_adev_unregister() to uninitialize
the device before freeing the devm-managed memory.

If an external entity holds an active reference to the auxiliary device,
the reference count will remain non-zero. Could this result in a
use-after-free when the external reference is dropped and put_device() tries
to access the already-freed memory?

> +	.driver = {
> +		.name = "clk-zx297520v3",
> +		.of_match_table = of_match_zx297520v3,
> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-zx29clk-v4-0-ca994bd22e9d@gmail.com?part=7