Re: [PATCH 05/12] clk: qcom: ipq-cmn-pll: Add PPE clock support

On 7/24/2026 11:19 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Using DIV_ROUND_CLOSEST_ULL() with a 64-bit divisor causes a divide-by-zero kernel panic due to 32-bit truncation.
> --
> 
> --- Patch [5]: [PATCH 05/12] clk: qcom: ipq-cmn-pll: Add PPE clock support ---
> commit aed5f1938300824fb26726205b9e4ce2ebf415fa
> Author: Luo Jie <[email protected]>
> 
> clk: qcom: ipq-cmn-pll: Add PPE clock support
> 
> This commit adds support for the PPE (packet process engine) clock,
> which is derived from the CMN PLL output. It uses a configurable 6-bit
> divider and shares a frequency control register with the NSS clock.
> 
>> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
>> index 3df326c22fbac..020ad12ca1f2b 100644
>> --- a/drivers/clk/qcom/ipq-cmn-pll.c
>> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
> 
> [ ... ]
> 
>> @@ -462,6 +463,83 @@ static struct clk_hw *ipq_cmn_pll_nss_register(struct platform_device *pdev,
>>  	return &nss_clk->hw;
>>  }
>>  
>> +/*
>> + * PPE (Packet Process Engine) clock operations.
>> + * The PPE clock is derived from CMN PLL rate / 2, then divided by
>> + * a configurable 6-bit divider (8-63).
>> + */
> 
> [ ... ]
> 
>> +static int clk_ppe_set_rate(struct clk_hw *hw, unsigned long rate,
>> +			    unsigned long parent_rate)
>> +{
>> +	struct clk_cmn_pll *ppe_clk = to_clk_cmn_pll(hw);
>> +	unsigned long div;
>> +	int ret;
>> +
>> +	if (rate == 0)
>> +		return -EINVAL;
>> +
>> +	div = DIV_ROUND_CLOSEST_ULL((u64)parent_rate, 2ULL * rate);
> 
> [Severity: High]
> Does this code overflow the 32-bit divisor limit of DIV_ROUND_CLOSEST_ULL?
> 
> DIV_ROUND_CLOSEST_ULL relies on the kernel do_div() macro, which strictly
> truncates the divisor to 32 bits on all architectures. If a caller requests
> a large rate (e.g. 2,147,483,648 Hz), 2ULL * rate will have its lower 32
> bits as 0. Could this regression cause the divisor to truncate to 0 and
> trigger a direct divide-by-zero kernel panic?

Good catch, DIV_ROUND_CLOSEST_ULL internally calls do_div(), which
silently truncates the divisor to the lower 32 bits on all
architectures. If 2ULL * rate exceeds U32_MAX (e.g. for rates ≥ ~2.15
GHz), the truncated divisor could wrap to zero, triggering a divide-by-
zero kernel panic.

Replacing it with DIV64_U64_ROUND_CLOSEST() is the correct fix — it uses
div64_u64() under the hood, which handles a full 64-bit divisor safely.
I'll update it accordingly.

> 
>> +	if (div < CMN_PLL_NSS_PPE_DIV_MIN || div > CMN_PLL_NSS_PPE_DIV_MAX)
>> +		return -EINVAL;
>> +
> 
> [ ... ]
> 
>> +static const struct clk_ops clk_ppe_ops = {
>> +	.recalc_rate = clk_ppe_recalc_rate,
>> +	.determine_rate = clk_nss_ppe_determine_rate,
>> +	.set_rate = clk_ppe_set_rate,
>> +};
> 
> While clk_ppe_set_rate might be protected if the clock framework previously
> clamps the rate via determine_rate, this patch also points to the shared
> clk_nss_ppe_determine_rate. 
> 
> Can unconstrained rates passed into clk_nss_ppe_determine_rate from the
> clock framework (such as via debugfs or a malformed device tree) cause a
> similar truncation and kernel panic during rate calculation?

Yes, DIV64_U64_ROUND_CLOSEST() should be also used to cover this similar
truncation issue.

> 
>> +static struct clk_hw *ipq_cmn_pll_ppe_register(struct platform_device *pdev,
>> +					       struct regmap *regmap,
>> +					       struct clk_hw *cmn_pll_hw)
>> +{
> 
> [ ... ]
>