Re: [PATCH v2 2/9] soc: qcom: geni-se: Populate clk_perf_tbl with SE source clock frequencies from perf OPP table

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Populating `clk_perf_tbl` with OPP performance levels instead of frequencies breaks clock divider math, introduces division-by-zero risks, and fails `dev_pm_opp_set_rate()`.
- [High] Fragile OPP iteration loop will fail the probe if the OPP table contains duplicate levels.
--

commit f430a1f0dd5802c00d71aa7a1d541115aa32fe57
Author: Praveen Talari <[email protected]>

soc: qcom: geni-se: Populate clk_perf_tbl with SE source clock frequencies from perf OPP table

This commit populates clk_perf_tbl with SE source clock frequencies
from the perf OPP table. It treats the OPP table as the actual SE hardware
supported source clock frequencies, allowing protocol drivers to compute
the correct source clock and divider.

> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 873bfbd6b2b7b..447b9cc6babf6 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
[ ... ]
> @@ -1175,6 +1181,31 @@ int geni_se_domain_attach(struct geni_se *se)
>  	else if (ret < 0)
>  		return ret;
>  
> +	perf_dev = se->pd_list->pd_devs[DOMAIN_IDX_PERF];
> +
> +	num_opps = dev_pm_opp_get_opp_count(perf_dev);
> +	if (num_opps < 0)
> +		return num_opps;
> +	if (num_opps == 0)
> +		return -ENODEV;
> +
> +	se->clk_perf_tbl = devm_kcalloc(se->dev, num_opps,
> +					sizeof(*se->clk_perf_tbl),
> +					GFP_KERNEL);
> +	if (!se->clk_perf_tbl)
> +		return -ENOMEM;
> +
> +	for (i = 0, level = 0; i < num_opps; i++, level++) {
> +		opp = dev_pm_opp_find_level_ceil(perf_dev, &level);
> +		if (IS_ERR(opp))
> +			return PTR_ERR(opp);

[Severity: High]
Does this iteration loop safely handle duplicate or missing OPP levels?

If the OPP table contains duplicate levels (e.g., same performance state but
different voltages), or if opp-level is unpopulated for some entries,
dev_pm_opp_find_level_ceil() will skip the duplicates.

This means the loop will run out of unique levels before 'i' reaches
'num_opps', returning -ERANGE or -ENODEV and failing the entire GENI SE probe.

[Severity: Critical]
Does fetching the OPP performance level here instead of the clock frequency
break the clock divider math later on?

When this loop uses dev_pm_opp_find_level_ceil() instead of
dev_pm_opp_find_freq_ceil(), it retrieves abstract performance levels
(which can be 0 for OFF/minimum states) rather than actual Hz frequencies.

> +
> +		se->clk_perf_tbl[i] = level;

These performance levels are then stored in se->clk_perf_tbl, which is meant
for Hz frequencies.

When protocol drivers call geni_se_clk_freq_match() to configure their source
clock, the following math is used:

	divider = DIV_ROUND_UP(tbl[i], req_freq);
	new_delta = req_freq - tbl[i] / divider;

If a performance level in tbl[i] is 0, 'divider' evaluates to 0, which leads
to a divide-by-zero kernel panic.

Even if the level is non-zero, it is treated as a tiny frequency, causing
dev_pm_opp_set_rate() to either fail with -ERANGE or force the clock to its
minimum state, breaking serial communication.

Should this use dev_pm_opp_find_freq_ceil() to populate the table with
opp-hz instead?

> +		dev_pm_opp_put(opp);
> +	}
> +	se->num_clk_levels = num_opps;
> +	se->has_opp = true;
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-derive_clk_perf_tbl_from_perf_domain_opp_table-v2-0-091697dbeb02@oss.qualcomm.com?part=2