Re: [PATCH RFC v5 06/12] clk: zte: Add zx PLL support infrastructure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Calling `clk_hw_get_rate(clk_hw_get_parent(&pll->hw))` inside `zx29_pll_determine_rate` will cause a NULL pointer dereference if the clock is an orphan.
- [Medium] `zx29_pll_determine_rate` reads the currently committed hardware parent rate instead of using the CCF transaction state, breaking rate propagation.
- [Low] Using `abs(out - rate)` with an `unsigned long` rate causes a compilation error and logic failure due to unsigned integer promotion.
--

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

clk: zte: Add zx PLL support infrastructure

I am guessing how much of this is reusable among other zx chips or even
differently named ZTE platforms (if there are any). From reading the old
zx2967 code, I think the PLL code would be reusable there, maybe with
platform specific bitmasks but otherwise the same logic.

> diff --git a/drivers/clk/zte/pll-zx.c b/drivers/clk/zte/pll-zx.c
> index c0475d5441fbc..1482caeea9995 100644
> --- a/drivers/clk/zte/pll-zx.c
> +++ b/drivers/clk/zte/pll-zx.c
[ ... ]
> +static u32 zx29_pll_calc_values(const struct zx29_clk_pll *pll, unsigned long parent_rate,
> +				unsigned long rate)
> +{
[ ... ]
> +			setting |= i << ZX29_PLL_POSTDIV1_SHIFT;
> +			setting |= j << ZX29_PLL_POSTDIV2_SHIFT;
> +			out = zx29_pll_get_rate(pll, parent_rate, setting);
> +
> +			if (abs(out - rate) > best)

[Severity: Low]
Could this fail to compile or produce a logic error due to unsigned integer
promotion? 

The abs() macro in the kernel evaluates to ((void)0) when passed an
unsigned type like rate, which would result in a build failure.

If it were to compile, the unsigned subtraction out - rate could wrap around
for values below the target rate, causing valid frequencies to be incorrectly
discarded.

> +				continue;
> +
> +			if (abs(out - rate) < best) {
> +				postdiv1 = i;
> +				postdiv2 = j;
[ ... ]
> +static int zx29_pll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> +{
> +	struct zx29_clk_pll *pll = to_zx29_clk_pll(hw);
> +	unsigned long new_rate, parent_rate = clk_hw_get_rate(clk_hw_get_parent(&pll->hw));

[Severity: High]
Is it possible to trigger a NULL pointer dereference here if the clock is an
orphan? 

If the parent is not yet registered during system initialization,
clk_hw_get_parent() returns NULL, which clk_hw_get_rate() unconditionally
dereferences.

[Severity: Medium]
Additionally, does reading the currently committed hardware parent rate break
rate propagation?

By calling clk_hw_get_rate() instead of using req->best_parent_rate, the driver
reads the old hardware rate of the parent rather than the proposed transaction
state. This bypasses the Common Clock Framework's two-phase transactional model
and could calculate incorrect hardware dividers when the clock tree is being
reparented or rate-adjusted.

> +	u32 setting;
> +
> +	setting = zx29_pll_calc_values(pll, parent_rate, req->rate);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628-zx29clk-v5-0-79ff044e4192@gmail.com?part=6