Re: [PATCH v2 07/12] clk: qcom: ipq-cmn-pll: Add PON reference clock support

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Integer Overflow / Division by Zero when requesting a specific large clock rate.
- [Medium] Non-atomic update of the clock mode and divider creates an invalid intermediate hardware state.
- [Medium] Misuse of `WARN_ON_ONCE` for valid framework queries and uninitialized hardware states.
--

--- Patch [7]: [PATCH v2 07/12] clk: qcom: ipq-cmn-pll: Add PON reference clock support ---
commit bc0bab1980249d4673b3cc332facefa3306c107a
Author: Luo Jie <[email protected]>

clk: qcom: ipq-cmn-pll: Add PON reference clock support

This patch adds support for the PON reference clock in the CMN PLL driver.
The clock is derived from the CMN PLL output divided by 2 and then by a
configurable 8-bit divider. It supports both a fixed 31.25 MHz UNIPHY mode
and a configurable PON mode.

> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
> index a24e57b33c6b3..356988c03a7ae 100644
> --- a/drivers/clk/qcom/ipq-cmn-pll.c
> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
[ ... ]
> +static unsigned long clk_pon_refclk_recalc_rate(struct clk_hw *hw,
> +						unsigned long parent_rate)
> +{
> +	struct clk_cmn_pll *pon_clk = to_clk_cmn_pll(hw);
> +	u32 val, div;
> +	int ret;
> +
> +	ret = regmap_read(pon_clk->regmap, CMN_PLL_PON_CONFIG, &val);
> +	if (WARN_ON_ONCE(ret))
> +		return 0;
> +
> +	/* Check if in UNIPHY mode (bit 9 = 0) - fixed 31.25 MHz */
> +	if (!(val & CMN_PLL_PON_MODE_SEL))
> +		return 31250000UL;
> +
> +	/* PON mode: calculate from divider */
> +	div = FIELD_GET(CMN_PLL_PON_DIV_CTRL, val);
> +	if (WARN_ON_ONCE(!div))
> +		return 0;

[Severity: Medium]
Is it safe to use WARN_ON_ONCE() for this check?

If the bootloader leaves the PON hardware uninitialized with a divider of 0,
this will throw a stack trace during boot when the clock framework initially
registers the clock.

Returning 0 without the warning might be safer.

> +
> +	return DIV_ROUND_CLOSEST_ULL((u64)parent_rate, 2ULL * div);
> +}
> +
> +static int clk_pon_refclk_determine_rate(struct clk_hw *hw,
> +					 struct clk_rate_request *req)
> +{
> +	unsigned long div, pon_rate, uniphy_rate = 31250000UL;
> +
> +	if (WARN_ON_ONCE(!req->rate))
> +		return -EINVAL;

[Severity: Medium]
Will this throw a stack trace for legitimate clock framework queries?

A user can write 0 to the clock's debugfs clk_rate file, or a consumer
driver can call clk_round_rate(clk, 0) to discover the minimum rate, which
are legal queries.

> +
> +	div = DIV64_U64_ROUND_CLOSEST((u64)req->best_parent_rate, 2ULL * req->rate);

[Severity: High]
Can this multiplication overflow and cause a divide-by-zero regression?

If req->rate is set to an exceptionally large value like 1ULL << 63 (which
can be requested by writing to the clock framework's debugfs interface),
the multiplication 2ULL * req->rate evaluates to 0 due to 64-bit unsigned
integer wrapping.

This 0 is then passed as the divisor to DIV64_U64_ROUND_CLOSEST.

> +
> +	/* Clamp to valid range (1-255) */
> +	div = clamp_t(unsigned long, div, 1, 255);
[ ... ]
> +static int clk_pon_refclk_set_rate(struct clk_hw *hw, unsigned long rate,
> +				   unsigned long parent_rate)
> +{
> +	struct clk_cmn_pll *pon_clk = to_clk_cmn_pll(hw);
> +	unsigned long div;
> +	int ret;
> +
> +	if (rate == 0)
> +		return -EINVAL;
[ ... ]
> +	div = DIV64_U64_ROUND_CLOSEST((u64)parent_rate, 2ULL * rate);

[Severity: High]
Might this also trigger a divide-by-zero regression for the same reason?

If rate wraps when multiplied by 2, passing 0 as the divisor to
DIV64_U64_ROUND_CLOSEST will result in a division by zero.

> +	if (div == 0 || div > 255)
> +		return -EINVAL;
> +
> +	/* Switch to PON mode */
> +	ret = regmap_set_bits(pon_clk->regmap, CMN_PLL_PON_CONFIG,
> +			      CMN_PLL_PON_MODE_SEL);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(pon_clk->regmap, CMN_PLL_PON_CONFIG,
> +				  CMN_PLL_PON_DIV_CTRL,
> +				  FIELD_PREP(CMN_PLL_PON_DIV_CTRL, div));
> +}

[Severity: Medium]
Does this non-atomic update create an invalid intermediate hardware state?

When changing from UNIPHY mode to PON mode, the clock mode is switched
before the new divider is programmed.

If the old divider was zero or invalid, the hardware might momentarily
output an out-of-spec frequency or lock up, especially since this clock
lacks the CLK_SET_RATE_GATE flag and rate changes can happen while active.

Could both fields be updated atomically in a single regmap_update_bits()
call?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7