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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Severe performance overhead by executing heavy interconnect calls on every I2C transfer.
- [High] DMA channels allocated during `geni_i2c_init()` are leaked if subsequent probe steps fail.

Pre-existing issues:
- [High] PM runtime usage counter leak in `geni_i2c_xfer()` on `.set_rate()` failure.
- [High] Memory leak of multi-descriptor DMA buffer tracking arrays on transfer failure.
--

commit 70cf5551bb430cb5f179e24ea3c574d784037179
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

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

The driver implements a custom resources_init() callback for clock
frequency validation and bandwidth vote programming. This commit moves
the clock validation to the probe function and programs the bandwidth
vote from qcom_geni_i2c_conf(). This allows the driver to use
geni_se_resources_init() directly.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index a3b7609e3cf95..0b178701e1d1e 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -242,7 +242,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]
Does moving this interconnect vote to qcom_geni_i2c_conf() introduce a
regression that causes severe performance degradation?

Because this function is used as the .set_rate callback, it is invoked by
geni_i2c_xfer() on every single I2C message transfer. Executing an
interconnect vote unconditionally on every transfer forces expensive SoC
fabric updates constantly.

[Severity: High]
This is a pre-existing issue, but does a failure during the rate setting phase
leak the PM runtime usage counter?

If .set_rate() (which now includes the fallible interconnect vote) fails
during an I2C transfer, the error path in geni_i2c_xfer() returns directly:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_xfer() {
    ...
	ret = pm_runtime_get_sync(gi2c->se.dev);
    ...
	ret = gi2c->dev_data->set_rate(&gi2c->se, gi2c->clk_freq_out);
	if (ret)
		return ret;
    ...
	pm_runtime_put_autosuspend(gi2c->se.dev);
    ...
}

Returning early here entirely skips the pm_runtime_put_autosuspend() call,
permanently preventing the device and its enclosing power domain from
suspending.

[ ... ]

> @@ -1199,6 +1183,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = geni_i2c_clk_map_idx(gi2c);
> +	if (ret)
> +		return dev_err_probe(gi2c->se.dev, ret, "Invalid clk frequency %d Hz\n",
> +				     gi2c->clk_freq_out);
> +
>  	ret = i2c_add_adapter(&gi2c->adap);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Error adding i2c adapter\n");

[Severity: High]
Does this error path introduce a regression by leaking the DMA channels
allocated earlier in probe?

The geni_i2c_init() function successfully requests DMA channels via
setup_gpi_dma() before this point. If geni_i2c_clk_map_idx() fails here
(e.g., due to an invalid DT clock configuration), the probe function returns
the error directly, bypassing release_gpi_dma() and leaking the channels.

[ ... ]

[Severity: High]
This is a pre-existing issue, but is it possible for the multi-descriptor
DMA buffer tracking arrays to leak on transfer failure?

During multi-message transfers, geni_i2c_gpi_xfer() dynamically allocates
tracking arrays. If an error occurs (such as an I2C NACK from a target
device), the transfer loop terminates early. 

Inside the cleanup function geni_i2c_gpi_multi_desc_unmap():

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_gpi_multi_desc_unmap() {
    ...
	for (; tx_multi_xfer->unmap_msg_cnt < msg_xfer_cnt; tx_multi_xfer->unmap_msg_cnt++) {
        ...
		if (tx_multi_xfer->unmap_msg_cnt == gi2c->num_msgs - 1) {
			kfree(tx_multi_xfer->dma_buf);
			kfree(tx_multi_xfer->dma_addr);
			break;
		}
	}
    ...
}

Because the error aborted the transfer early, msg_xfer_cnt is smaller than
num_msgs. The kfree() condition is never met, resulting in a permanent
memory leak of the kcalloc() allocated arrays.

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