Re: [PATCH v1 3/3] i2c: qcom-geni: Add bus recovery support for GPI DMA mode

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Double free and invalid DMA unmap in geni_i2c_gpi_xfer() due to uninitialized rx_buf across loop iterations.
--

commit 1ca4e6273080dc8f5231d9130b61334c26638357
Author: Aniket Randive <aniket.randive@oss.qualcomm.com>

i2c: qcom-geni: Add bus recovery support for GPI DMA mode

This commit adds bus recovery support for GPI DMA mode in the QCOM GENI I2C
driver. It implements I2C_BUS_CLEAR and I2C_STOP_ON_BUS opcodes via the
GPI TX DMA channel and updates the recovery trigger logic.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 9fa1a8ac400c3..85cb1367958c6 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

[ ... ]

> @@ -1066,14 +1136,18 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>  	else
>  		ret = geni_i2c_fifo_xfer(gi2c, msgs, num);

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential double-free and
invalid DMA unmap in geni_i2c_gpi_xfer() during multi-message transfers.

Looking at geni_i2c_gpi_xfer() which is called just above this context,
the rx_buf pointer is initialized outside the message processing loop:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_gpi_xfer() {
    void *tx_buf = NULL, *rx_buf = NULL;
    ...
    for (i = 0; i < num; i++) {
        ...
        if (msgs[i].flags & I2C_M_RD) {
            ret =  geni_i2c_gpi(gi2c, msgs, &config,
                                &rx_addr, &rx_buf, I2C_READ, gi2c->rx_c);
        ...
        if (!gi2c->is_tx_multi_desc_xfer)
            geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
}

If a multi-message transfer contains a read message followed by a write
message, rx_buf is allocated and set during the read message. During the
subsequent write message, the I2C_M_RD block is skipped, meaning rx_buf
is never reset to NULL.

The stale rx_buf pointer is then passed again to geni_i2c_gpi_unmap() at
the end of the write iteration:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_gpi_unmap() {
    ...
    if (rx_buf) {
        dma_unmap_single(gi2c->se.dev->parent, rx_addr, msg->len, DMA_FROM_DEVICE);
        i2c_put_dma_safe_msg_buf(rx_buf, msg, !gi2c->err);
    }
}

Could this lead to a double-free of the bounce buffer and an invalid DMA
unmap since rx_buf retains its value from the previous read iteration?

> -	if (!gi2c->gpi_mode &&
> -	    (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN)) {
> +	if (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN ||
> +	    (gi2c->gpi_mode && ret == -EIO)) {
>  		/*
>  		 * Only attempt recovery if SDA is stuck low. -EPROTO and
>  		 * -ETIMEDOUT indicate bus errors where the target may be

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-i2c_bus_recovery-v1-0-203f0ec76f84@oss.qualcomm.com?part=3