Re: [PATCH RFC v3 3/5] 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] Calling a sleeping function `readl_poll_timeout` while holding a spinlock with interrupts disabled.
- [High] `determine_rate` callback fails to update `req->rate`, breaking clock rate negotiation.
- [High] Missing spinlock protection for shared PLL registers leads to concurrent read-modify-write corruption.
- [High] Inverted logic in the reset controller `.status` callback.
- [High] Multiple paths lead to a division by zero kernel panic during PLL rate calculations.
- [Medium] Holding a spinlock with interrupts disabled across a 100us delay.
- [Low] Makefile dependency prevents COMPILE_TEST builds on non-ZTE architectures.
--

commit 762483ab5f3f25d2d8b51f77a303caa010237fbd
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 controller driver for the
ZTE zx297520v3 SoC, handling core devices like PLLs, timers, and USB.

> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> [ ... ]
> @@ -167,5 +167,6 @@ ifeq ($(CONFIG_COMMON_CLK), y)
>  obj-$(CONFIG_X86)			+= x86/
>  endif
>  obj-y					+= xilinx/
> +obj-$(CONFIG_ARCH_ZTE)			+= zte/

[Severity: Low]
Will this obj-$(CONFIG_ARCH_ZTE) prevent COMPILE_TEST builds from
traversing into the zte/ directory when CONFIG_ARCH_ZTE is not set?

> --- /dev/null
> +++ b/drivers/clk/zte/clk-zx297520v3.c
> @@ -0,0 +1,775 @@
> [ ... ]
> +static int __zx297520v3_rst_deassert(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);
> +	val |= data->resets[id].mask;
> +	writel(val, data->resets[id].reg);
> +
> +	/* This is a special case used only by USB reset */
> +	if (data->resets[id].wait_mask) {
> +		return readl_poll_timeout(data->resets[id].reg + 4, val,
> +					  val & data->resets[id].wait_mask, 1, 100);
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Is it safe to call readl_poll_timeout() here? Since it is called from
zx297520v3_rst_deassert() and zx297520v3_rst_reset() while holding the
reg_lock spinlock with interrupts disabled, the usleep_range() inside
readx_poll_timeout() might sleep in an atomic context.

Should readl_poll_timeout_atomic() be used instead?

> [ ... ]
> +static int zx297520v3_rst_reset(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	unsigned long flags;
> +	int res;
> +
> +	spin_lock_irqsave(&reg_lock, flags);
> +
> +	res = __zx297520v3_rst_assert(rcdev, id);
> +	if (res)
> +		goto unlock;
> +	udelay(100);

[Severity: Medium]
Does holding a spinlock with local IRQs disabled across a 100-microsecond
delay risk causing severe scheduling latency spikes or triggering
soft-lockup detectors? Could the lock be dropped during the delay?

> +	res = __zx297520v3_rst_deassert(rcdev, id);
> +
> +unlock:
> +	spin_unlock_irqrestore(&reg_lock, flags);
> +	return res;
> +}
> +
> +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;
> +}

[Severity: High]
Does this return value invert the reset controller framework's semantics?

Since __zx297520v3_rst_assert() clears the bit and
__zx297520v3_rst_deassert() sets the bit, the reset appears to be active-low.
Returning val & mask produces a non-zero value when the line is
deasserted, but the framework expects .status to return non-zero when
asserted.

> --- /dev/null
> +++ b/drivers/clk/zte/pll.c
> @@ -0,0 +1,450 @@
> [ ... ]
> +static int zx29_pll_enable(struct clk_hw *hw)
> +{
> +	struct zx29_clk_pll *pll = to_zx29_clk_pll(hw);
> +	u32 val;
> +
> +	val = readl(pll->base);
> +	val &= ~ZX29_PLL_BYPASS;
> +	writel(val, pll->base);
> +
> +	val = readl(pll->base + ZX29_PLL_REG2_OFFSET);
> +	val &= ~ZX29_PLL_POSTDIV_OUT_DISABLE;
> +	writel(val, pll->base + ZX29_PLL_REG2_OFFSET);
> +
> +	return 0;
> +}

[Severity: High]
Are these read-modify-write sequences safe against concurrent
modifications?

The Common Clock Framework protects .enable/.disable with
a global spinlock and .prepare/.set_rate with a mutex. Since callbacks
like zx29_pll_enable() and zx29_pll_set_rate() can run concurrently on the
same hardware register, is a driver-specific spinlock needed here?

> [ ... ]
> +static unsigned long zx29_pll_get_rate(const struct zx29_clk_pll *pll, unsigned long parent_rate,
> +				       u32 setting)
> +{
> +	unsigned long refdiv, fbdiv, postdiv1, postdiv2, freq;
> +	const char *name = clk_hw_get_name(&pll->hw);
> +	u64 vco;
> +
> +	refdiv = (setting & ZX29_PLL_REFDIV_MASK) >> ZX29_PLL_REFDIV_SHIFT;
> +	fbdiv = (setting & ZX29_PLL_FBDIV_MASK) >> ZX29_PLL_FBDIV_SHIFT;
> +	postdiv1 = (setting & ZX29_PLL_POSTDIV1_MASK) >> ZX29_PLL_POSTDIV1_SHIFT;
> +	postdiv2 = (setting & ZX29_PLL_POSTDIV2_MASK) >> ZX29_PLL_POSTDIV2_SHIFT;
> +	dev_dbg(pll->dev, "%s: reference clock %lu HZ, PLL setting 0x%08x\n",
> +		name, parent_rate, setting);
> +
> +	vco = div_u64((u64)parent_rate * fbdiv, refdiv);
> +	freq = div_u64(div_u64(vco, postdiv1), postdiv2);

[Severity: High]
Can this sequence cause a divide-by-zero kernel panic?

If the hardware register initially contains 0, or if zx29_pll_calc_values()
returns 0, refdiv, postdiv1, and postdiv2 could evaluate to 0, which would be
passed as divisors to div_u64().

> [ ... ]
> +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));
> +	u32 setting;
> +
> +	setting = zx29_pll_calc_values(pll, parent_rate, req->rate);
> +	if (!setting)
> +		return -EINVAL;
> +
> +	new_rate = zx29_pll_get_rate(pll, parent_rate, setting);
> +	if (new_rate != req->rate) {
> +		dev_warn(pll->dev, "Did not find an exact match. Want %lu, got %lu\n",
> +			 req->rate, new_rate);
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Does this callback need to assign the negotiated new_rate back to
req->rate?

The Common Clock Framework uses the rate updated in req->rate
when executing .set_rate. Without this assignment, the framework might
request the exact original rate, which zx29_pll_set_rate() strictly
validates against and could reject with -EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-zx29clk-v3-0-c7fe54ea388f@gmail.com?part=3