Re: [PATCH v7 04/10] clk: realtek: Add support for phase locked loops (PLLs)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing spinlock release and IRQ restore in `clk_pll_recalc_rate()` error path.
- [High] Severe scheduling latency due to up to 2ms delay in atomic context with interrupts disabled.
- [High] `is_power_on()` masks hardware read errors, leading to silent hardware failures.
- [High] Valid register offset `0x0` is treated as 'no register' in `is_power_on()`.
- [Medium] `clk_pll_determine_rate()` fails to enforce frequency bounds.
--

commit df23b935aa675a5465a9a387a1eecbe47915033e
Author: Cheng-Yu Lee <cylee12@realtek.com>
clk: realtek: Add support for phase locked loops (PLLs)

This commit introduces support for realtek programmable and fixed or
hardware-managed PLLs. It provides operations for programmable PLLs and
a read-only variant for fixed or hardware-managed PLLs.

> diff --git a/drivers/clk/realtek/clk-pll.c b/drivers/clk/realtek/clk-pll.c
> new file mode 100644
> index 0000000000000..b63f3aa92d3f4
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-pll.c

[ ... ]

> +static int wait_freq_ready(struct clk_pll *clkp)
> +{
> +	u32 pollval;
> +
> +	if (!clkp->freq_ready_valid)
> +		return 0;
> +
> +	return regmap_read_poll_timeout_atomic(clkp->clkr.regmap, clkp->freq_ready_reg, pollval,
> +		(pollval & clkp->freq_ready_mask) == clkp->freq_ready_val, 1, TIMEOUT);
> +}

Is a 2000 microsecond (2ms) timeout safe in an atomic context? wait_freq_ready()
is called from clk_pll_enable() and clk_pll_set_rate(), both of which hold 
clkp->lock and disable interrupts via spin_lock_irqsave(). Spinning for 2ms 
with local interrupts disabled could degrade system responsiveness and trigger 
NMI watchdogs.

Should multi-millisecond hardware waits be moved to sleepable callbacks (like
.prepare() instead of .enable()) using mutexes?

> +static bool is_power_on(struct clk_pll *clkp)
> +{
> +	u32 val;
> +
> +	if (!clkp->power_reg)
> +		return true;

Does this check inadvertently treat the valid register offset 0x0 as 'no
register'?

In regmap-based MMIO devices, offset 0x0 is a common and valid register address.
If a PLL maps its power register to offset 0, will the driver skip reading it
and incorrectly assume the clock is powered on?

> +
> +	if (regmap_read(clkp->clkr.regmap, clkp->power_reg, &val))
> +		return true;

Does returning true on a regmap_read() error mask hardware read failures?

If this fails, clk_pll_enable() will interpret the clock as already powered
on, skip the seq_power_on sequence, and return success while the hardware
remains unpowered. Could this lead to silent failures and crashes when
dependent drivers access unclocked registers?

> +
> +	return (val & clkp->power_mask) == clkp->power_val_on;
> +}

[ ... ]

> +static int clk_pll_determine_rate(struct clk_hw *hw,
> +				  struct clk_rate_request *req)
> +{
> +	struct clk_pll *clkp = to_clk_pll(hw);
> +	const struct freq_table *ftblv = NULL;
> +
> +	ftblv = ftbl_find_by_rate(clkp->freq_tbl, req->rate);
> +	if (!ftblv)
> +		return -EINVAL;
> +
> +	req->rate = ftblv->rate;

Does this callback need to enforce frequency bounds?

The determine_rate callback is generally expected to select a valid frequency
that satisfies req->min_rate <= rate <= req->max_rate. It looks like the closest
rate is assigned without checking if it falls within the requested bounds.

> +
> +	return 0;
> +}
> +
> +static unsigned long clk_pll_recalc_rate(struct clk_hw *hw,
> +					 unsigned long parent_rate)
> +{
> +	struct clk_pll *clkp = to_clk_pll(hw);
> +	const struct freq_table *fv;
> +	unsigned long flags;
> +	u32 freq_val;
> +
> +	spin_lock_irqsave(&clkp->lock, flags);
> +
> +	if (regmap_read(clkp->clkr.regmap, clkp->freq_reg, &freq_val))
> +		return 0;

Is there a missing spin_unlock_irqrestore() in this error path?

If regmap_read() fails, the function returns immediately, leaving the spinlock
acquired and interrupts disabled. Any future attempt to acquire this lock would
deadlock.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508111641.3192177-1-eleanor.lin@realtek.com?part=4