Re: [PATCH v7 4/6] i2c: qcom-geni: Use resources helper APIs in runtime PM functions

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch calls undefined helper functions `geni_se_resources_activate`, `geni_se_resources_deactivate`, and `geni_se_resources_init`, leading to implicit function declarations and link failures.

Pre-existing issues:
- [Critical] Stale `gi2c->dma_buf` pointer causes use-after-free and double-free on DMA mapping failure.
- [High] Pending asynchronous runtime suspend is cancelled by `pm_runtime_disable()`, permanently leaking hardware resources (clocks, interconnects).
- [High] Memory leak of multi-descriptor DMA arrays on transfer error.
- [High] The system suspend callback executes a sleeping function while interrupts are disabled, causing a BUG.
--

commit 495d087d0c5bbbc5ad2b355b642cfdc5ae9439b6
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

i2c: qcom-geni: Use resources helper APIs in runtime PM functions

To manage GENI serial engine resources during runtime power management,
drivers currently need to call functions for ICC, clock, and
SE resource operations in both suspend and resume paths, resulting in
code duplication across drivers.

The new geni_se_resources_activate() and geni_se_resources_deactivate()
helper APIs addresses this issue by providing a streamlined method to
enable or disable all resources based, thereby eliminating redundancy
across drivers.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index a7672b6c2bc07..8f8b74c4e88a4 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -1172,15 +1172,14 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>  	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>  
>  	disable_irq(gi2c->irq);

[Severity: High]
This isn't a bug introduced by this patch, but does calling disable_irq() here
cause a BUG when suspending the system?

Looking at the suspend paths, geni_i2c_suspend_noirq() runs in the noirq
phase where local interrupts are globally disabled. It calls
pm_runtime_force_suspend(), which invokes this geni_i2c_runtime_suspend()
function.

Calling disable_irq() explicitly invokes might_sleep(), which triggers a
"BUG: sleeping function called from invalid context" when called with
interrupts disabled.

> -	ret = geni_se_resources_off(&gi2c->se);
> +	ret = geni_se_resources_deactivate(&gi2c->se);

[Severity: Low]
Are the implementations for geni_se_resources_deactivate() and
geni_se_resources_activate() missing from this patch series?

These functions appear to be completely missing from the kernel tree at this
revision. Does this lead to implicit function declarations and build failures?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
geni_i2c_probe() and the driver removal path in geni_i2c_remove()
permanently leak hardware resources?

In geni_i2c_init(), an asynchronous pm_runtime_put() is called. If an error
occurs later in geni_i2c_probe(), or during geni_i2c_remove(), the code
immediately calls pm_runtime_disable():

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_probe() {
    ...
    pm_runtime_disable(gi2c->se.dev);
    ...
}

Calling pm_runtime_disable() invokes __pm_runtime_barrier(), which cancels
any pending asynchronous suspend requests instead of waiting for them to
complete. Since the suspend is cancelled and the device remains in the
RPM_ACTIVE state after the driver unbinds, do its clocks and power domains
leak indefinitely?

[Severity: Critical]
This is a pre-existing issue, but can a stale dma_buf pointer cause a
use-after-free and double-free if DMA mapping fails?

If geni_se_rx_dma_prep() fails in geni_i2c_rx_one_msg(), the local dma_buf
is freed and set to NULL, but the device struct's gi2c->dma_buf retains the
stale pointer from a previous successful transfer:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_rx_one_msg() {
    ...
	if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) {
		geni_se_select_mode(se, GENI_SE_FIFO);
		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
		dma_buf = NULL;
	} else {
    ...
}

During cleanup, the stale pointer is evaluated as true:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_rx_msg_cleanup() {
    ...
	if (gi2c->dma_buf) {
		if (gi2c->err)
			geni_i2c_rx_fsm_rst(gi2c);
		geni_se_rx_dma_unprep(&gi2c->se, gi2c->dma_addr, gi2c->xfer_len);
		i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err);
	}
    ...
}

Does this unmap the stale DMA address, copy data from the freed bounce buffer,
and then double-free the kalloc'd bounce buffer?

[Severity: High]
This is a pre-existing issue, but is there a memory leak of the
multi-descriptor DMA arrays when a transfer error occurs?

In geni_i2c_gpi_multi_desc_unmap(), the arrays are only freed if
unmap_msg_cnt reaches gi2c->num_msgs - 1:

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

If an error occurs during the multi-message transfer, msg_xfer_cnt is less
than num_msgs, so the loop terminates early. Does this bypass the kfree()
calls and leak both tracking arrays, potentially allowing an attacker to
cause an Out-Of-Memory condition?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com?part=4