Re: [PATCH 2/2] i2c: imx: return -ENXIO at once when a DMA write is not acknowledged

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] i2c: imx: DMA mapping leak in i2c_imx_dma_write() on I2SR_RXAK
- [Medium] i2c: imx: missed NACK on last DMA byte in i2c_imx_dma_write()
--

commit 466099063994a0c4c92a4a0f7a1a450fa666895b
Author: Mehmet Fide <mehmet.fide@screeningeagle.com>

i2c: imx: return -ENXIO at once when a DMA write is not acknowledged

This commit completes the DMA wait in the interrupt handler when a
missing acknowledge is detected during a DMA write and returns -ENXIO
from i2c_imx_dma_write().

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 1987a0fd81329..09934f3b13efa 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
[ ... ]
> @@ -1225,6 +1228,11 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
>  		return -ETIMEDOUT;
>  	}
>  
> +	if (i2c_imx->i2csr & I2SR_RXAK) {
> +		dmaengine_terminate_sync(dma->chan_using);
> +		return -ENXIO;
> +	}
> +
>  	/* Waiting for transfer complete. */
>  	while (1) {
>  		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);

[Severity: High]
Does this error path leak the DMA mapping in dma->dma_buf?

i2c_imx_dma_xfer() maps msgs->buf with dma_map_single(), and outside
submission errors the buffer is only unmapped in i2c_imx_dma_callback():

drivers/i2c/busses/i2c-imx.c:i2c_imx_dma_callback() {
	struct imx_i2c_dma *dma = (struct imx_i2c_dma *)arg;

	dma_unmap_single(dma->chan_using->device->dev, dma->dma_buf,
			dma->dma_len, dma->dma_data_dir);
	complete(&dma->cmd_complete);
}

When a write is not acknowledged, the ISR completes dma->cmd_complete and
dmaengine_terminate_sync() aborts the pending DMA transfer without running
i2c_imx_dma_callback().  The mapping is never unmapped, and callers free
the message buffer while it is still DMA-mapped.  Moving dma_unmap_single()
out of i2c_imx_dma_callback() and into i2c_imx_dma_write() and
i2c_imx_dma_read() after waiting and any dmaengine_terminate_sync() call
would avoid both the leak and a double unmap if the callback already ran.

[Severity: Medium]
Can a NACK on the last DMA-transferred byte (msgs->buf[msgs->len - 2]) be
missed because I2SR_RXAK is checked before waiting for I2SR_ICF?

The DMA engine signals dma->cmd_complete as soon as it writes
msgs->buf[msgs->len - 2] into IMX_I2C_I2DR, while that byte is still
shifting out on the bus (I2SR_ICF == 0).  i2c_imx_dma_write() wakes up,
checks i2c_imx->i2csr & I2SR_RXAK before the byte finishes transmitting,
and enters the I2SR_ICF polling loop.

If the target NACKs msgs->buf[msgs->len - 2] when it finishes shifting
out, the controller raises an interrupt that stores I2SR_IIF | I2SR_RXAK
into i2c_imx->i2csr and sets I2SR_ICF.  The loop then exits without
rechecking I2SR_RXAK, writes the final byte msgs->buf[msgs->len - 1] to
IMX_I2C_I2DR, and calls i2c_imx_trx_complete(), which returns immediately
by consuming the stale I2SR_IIF from the earlier NACK while the final byte
is still on the wire.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113073250.1037215-1-mehmet.fide@gmail.com?part=2