Re: [PATCH v2 9/9] i2c: qcom-geni: Use geni_se_set_rate() for setting source clock frequency

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing pm_runtime_put() on error path in geni_i2c_xfer() causes PM resource leak.
- [High] Memory leak of `dma_buf` and `dma_addr` arrays on initialization error paths in multi-descriptor GPI transfers.
--

commit 94f912454825d4e32acc9ffbdaf4425dd413984d
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

i2c: qcom-geni: Use geni_se_set_rate() for setting source clock frequency

This patch refactors the GENI I2C driver to use geni_se_set_rate() for
source clock programming rather than relying on platform-specific callbacks.
This allows the GENI core to abstract resource-management mechanisms and
enables a common code path.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 0b178701e1d1e..deb0d822290c3 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
[ ... ]
> @@ -985,7 +984,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>  		return ret;
>  	}
>  
> -	ret = gi2c->dev_data->set_rate(&gi2c->se, gi2c->clk_freq_out);
> +	ret = qcom_geni_i2c_conf(&gi2c->se);
>  	if (ret)
>  		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does the error path here
permanently leak the PM runtime reference?

Earlier in geni_i2c_xfer(), we acquire a reference:

	ret = pm_runtime_get_sync(gi2c->se.dev);
	if (ret < 0) {
		...
	}

If qcom_geni_i2c_conf() fails, the code returns directly without dropping the 
PM usage counter or restoring the suspended state, which keeps the device and 
its parent power domains awake indefinitely. Should there be a 
pm_runtime_put() call before returning here?

[ ... ]


While reviewing the driver, I also noticed a pre-existing issue in 
geni_i2c_gpi_xfer() and geni_i2c_gpi_multi_desc_unmap().

[Severity: High]
This is a pre-existing issue, but does geni_i2c_gpi_multi_desc_unmap() 
leak the dma_buf and dma_addr arrays on initialization error paths?

In geni_i2c_gpi_xfer(), the arrays are allocated:

	tx_multi_xfer->dma_buf = kcalloc(num, sizeof(void *), GFP_KERNEL);
	tx_multi_xfer->dma_addr = kzalloc_objs(dma_addr_t, num);
	if (!tx_multi_xfer->dma_buf || !tx_multi_xfer->dma_addr) {
		ret = -ENOMEM;
		goto err;
	}

If the dma_addr allocation fails (or if an early mapping call fails), 
the function jumps to the err label and calls geni_i2c_gpi_multi_desc_unmap() 
with msg_xfer_cnt evaluated to 0. 

Inside geni_i2c_gpi_multi_desc_unmap():

	for (; tx_multi_xfer->unmap_msg_cnt < msg_xfer_cnt; tx_multi_xfer->unmap_msg_cnt++) {
		wr_idx = 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 msg_xfer_cnt is 0, the loop condition evaluates to false immediately, 
and the kfree statements are skipped. Does this permanently leak the 
allocated dma_buf array under memory pressure or DMA mapping exhaustion?

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