Re: [PATCH v9 4/4] i2c: qcom-geni: Support multi-owner controllers in GPI mode

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

New issues:
- [High] Premature hardware lock release breaks multi-owner transaction atomicity
- [Medium] PM runtime usage counter leak on `geni_i2c_init` error path

Pre-existing issues:
- [Critical] Double free and use-after-free of DMA bounce buffers in `geni_i2c_gpi_xfer()`
- [High] Memory leak of DMA tracking arrays on multi-descriptor transfer error paths
--

--- Patch [4]: [PATCH v9 4/4] i2c: qcom-geni: Support multi-owner controllers in GPI mode ---
commit e932d5c4bcfb356d1e0cbaf5c247b0bd50b4e7e8
Author: Mukesh Kumar Savaliya <[email protected]>

i2c: qcom-geni: Support multi-owner controllers in GPI mode
    
Some platforms use a QUP-based I2C controller in a configuration where the
controller is shared with another system processor. In this setup the
operating system must not assume exclusive ownership of the controller or
its associated pins.
    
Add support for enabling multi-owner operation when DeviceTree specifies
qcom,qup-multi-owner.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -829,6 +829,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>  	peripheral.clk_div = itr->clk_div;
>  	peripheral.set_config = 1;
>  	peripheral.multi_msg = false;
> +	peripheral.multi_owner = gi2c->se.multi_owner;

[Severity: High]
Does this setup cause premature hardware lock releases, breaking the
multi-owner atomicity?

In geni_i2c_gpi(), the interrupt flag is unconditionally set for sequential
transfers:

geni_i2c_gpi() {
    ...
    } else {
        flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
    }
    ...
}

Since the GPI driver inserts an UNLOCK TRE when DMA_PREP_INTERRUPT is set,
would a multi-message sequence like Write-then-Read drop the hardware lock
mid-transaction while the bus is stretched?

[Severity: Critical]
This is a pre-existing issue, but does geni_i2c_gpi_xfer() have a double free
bug on its DMA bounce buffers?

The tx_buf and rx_buf variables are initialized outside the loop, but are not
reset at the top of each iteration:

geni_i2c_gpi_xfer() {
    void *tx_buf = NULL, *rx_buf = NULL;
    ...
    for (i = 0; i < num; i++) {
        ...
        if (msgs[i].flags & I2C_M_RD) {
            geni_i2c_gpi(..., &rx_buf, ...);
        }
        ...
        geni_i2c_gpi_unmap(..., rx_buf, ...);
    }
}

If a message sequence contains a Read followed by a Write, rx_buf is mapped
in the Read iteration and then freed in geni_i2c_gpi_unmap(). In the next
Write iteration, rx_buf is not reset to NULL and not overwritten, causing
geni_i2c_gpi_unmap() to use and free the stale pointer again:

geni_i2c_gpi_unmap() {
    ...
    if (rx_buf) {
        dma_unmap_single(...);
        i2c_put_dma_safe_msg_buf(rx_buf, ...);
    }
}

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

If a multi-descriptor transfer encounters an error (like a device NACK),
gi2c->err is set and the cleanup loop in geni_i2c_gpi_multi_desc_unmap()
aborts early:

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;
    }
    ...
}

Since unmap_msg_cnt never reaches num_msgs - 1 on an early error path, are
dma_buf and dma_addr permanently leaked?

[ ... ]

> @@ -1075,6 +1080,11 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
>  		dev_dbg(gi2c->se.dev, "Using GPI DMA mode for I2C\n");
>  	} else {
>  		gi2c->gpi_mode = false;
> +
> +		if (gi2c->se.multi_owner)
> +			return dev_err_probe(gi2c->se.dev, -EINVAL,
> +					     "I2C sharing not supported in non-GSI mode\n");
> +

[Severity: Medium]
Does this early return leak the PM runtime usage counter?

By returning directly here, it seems to bypass the pm_runtime_put() cleanup
step at the err: label that the rest of the function uses on failure.

>  		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4