Re: [PATCH v3 7/8] i2c: qcom-geni: Use common GENI resource initialization helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i2c: qcom-geni: uninitialized src_freq bypasses OPP vote before hw access
- [High] i2c: qcom-geni: severe lock contention by calling icc_set_bw per-transfer
- [Medium] i2c: qcom-geni: leak of GPI DMA channels on clk map error path
--

commit 78a289de1b8e5415a287115753c631aba3c4355a
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

i2c: qcom-geni: Use common GENI resource initialization helper

This commit replaces a custom resource initialization callback with
geni_se_resources_init(). It moves clock frequency validation and ICC
bandwidth vote programming to probe and qcom_geni_i2c_conf() respectively.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index a6fe89d0934f6..dbe344719b86c 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -275,7 +275,9 @@ static int qcom_geni_i2c_conf(struct geni_se *se, unsigned long freq)
>  	trace_geni_i2c_bus_setup(gi2c->se.dev, gi2c->clk_freq_out,
>  				 itr->clk_div, itr->t_high_cnt,
>  				 itr->t_low_cnt, itr->t_cycle_cnt);
> -	return 0;
> +
> +	return geni_icc_set_bw_ab(&gi2c->se, GENI_DEFAULT_BW, GENI_DEFAULT_BW,
> +				  Bps_to_icc(gi2c->clk_freq_out));
>  }

[Severity: High]
Could this introduce severe lock contention?

The geni_i2c_xfer() handler invokes this configuration function on every batch
of messages. Moving geni_icc_set_bw_ab() into qcom_geni_i2c_conf() means
icc_set_bw() is now called on every I2C transfer.

Because icc_set_bw() unconditionally acquires the global icc_bw_lock mutex
and iterates through interconnect nodes, will this rapidly acquire and
release a global interconnect lock during normal I2C operations, throttling
the rest of the system?

[ ... ]

> @@ -1251,6 +1237,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = geni_i2c_clk_map_idx(gi2c);

[Severity: High]
Does moving geni_i2c_clk_map_idx() after geni_i2c_init() leave gi2c->src_freq
uninitialized when hardware is first accessed?

When geni_i2c_init() calls pm_runtime_resume_and_get(), it triggers
geni_i2c_runtime_resume() which checks gi2c->src_freq:

	if (gi2c->src_freq) {
		ret = dev_pm_opp_set_rate(dev, gi2c->src_freq);
		if (ret)
			return ret;
	}

Because gi2c->src_freq is 0 at this point, the OPP performance state vote is
silently bypassed. geni_i2c_init() then immediately accesses hardware registers.
On Qualcomm platforms, could this un-voted access result in a NoC timeout or
system crash?

> +	if (ret)
> +		return dev_err_probe(gi2c->se.dev, ret, "Invalid clk frequency %d Hz\n",
> +				     gi2c->clk_freq_out);

[Severity: Medium]
Does this path leak the GPI DMA channels?

geni_i2c_init() allocates RX and TX channels via setup_gpi_dma() when the
device is in GPI mode. When geni_i2c_clk_map_idx() fails here, we return
the error directly without calling release_gpi_dma() to clean up the
allocated channels, unlike the i2c_add_adapter() error path directly below it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-derive_clk_perf_tbl_from_perf_domain_opp_table-v3-0-5da1096243c9@oss.qualcomm.com?part=7