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

[sashiko-bot@kernel.org]

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 <jie.luo@oss.qualcomm.com>

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?

> +	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?

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

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-qcom_ipq5210_cmnpll-v1-0-a8b8679d215f@oss.qualcomm.com?part=5