[PATCH 0/2] i2c: imx: two fixes for a DMA write that is not acknowledged

[PATCH 0/2] i2c: imx: two fixes for a DMA write that is not acknowledged

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

A write of DMA_THRESHOLD bytes or more to an address that does not
answer leaves the interrupt status behind, so that the next DMA read
returns 0xff for every byte without an error (patch 1), and it blocks
for DMA_TIMEOUT before it fails with -ETIMEDOUT instead of -ENXIO
(patch 2). Patch 2 needs patch 1.

Tested on a Colibri VF50 (i2c0 at 100 kHz, DMA) with v6.12.117, where
the touched code is the same; on this base compile tested only (W=1,
sparse). 200 rounds of failed write, 32 byte read and acknowledged
18 byte write: 200 times ENXIO at once, no wrong read, every write
acknowledged.

Mehmet Fide (2):
  i2c: imx: do not carry the interrupt status over to the next transfer
  i2c: imx: return -ENXIO at once when a DMA write is not acknowledged

 drivers/i2c/busses/i2c-imx.c | 10 ++++++++++
 1 file changed, 10 insertions(+)


base-commit: 25e7ab89f4774b0f842cee0e8a20c11f755924d5
-- 
2.56.0

[PATCH 1/2] i2c: imx: do not carry the interrupt status over to the next transfer

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

In DMA mode the interrupt handler stores I2SR in i2c_imx->i2csr and only
i2c_imx_trx_complete() clears it. A DMA transfer that fails returns
without doing so, for example a DMA write whose address is not
acknowledged.

The stale status stays until the next DMA read. There
i2c_imx_prepare_read() calls i2c_imx_trx_complete(), which finds IIF set
and returns before the address byte has been sent, so the read returns
0xff for every byte without an error:

  i2ctransfer -a -y 0 w20@0x03 0x00-     # unused address, -ETIMEDOUT
  i2ctransfer -y 0 w1@0x54 0x00 r32      # 32 times 0xff
  i2ctransfer -y 0 w1@0x54 0x00 r32      # correct data

Clear the status when a transfer starts.

Fixes: ce1a78840ff7 ("i2c: imx: add DMA support for freescale i2c driver")
Cc: stable@vger.kernel.org
Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---
 drivers/i2c/busses/i2c-imx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index ccf08cd63151..1987a0fd8132 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1657,6 +1657,8 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
 	if (result)
 		return result;
 
+	i2c_imx->i2csr = 0;
+
 	/* Start I2C transfer */
 	result = i2c_imx_start(i2c_imx, atomic);
 	if (result) {
-- 
2.56.0

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

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

When the address of a DMA write is not acknowledged the controller
raises its interrupt and no DMA request, so i2c_imx_dma_write() blocks
for DMA_TIMEOUT (1 s) and returns -ETIMEDOUT, where the interrupt driven
write returns -ENXIO.

Let the interrupt handler end the wait when it sees a missing
acknowledge during a DMA write, and return -ENXIO. No interrupt arrives
while the bytes of a DMA write are acknowledged, so that path does not
change.

Fixes: ce1a78840ff7 ("i2c: imx: add DMA support for freescale i2c driver")
Cc: stable@vger.kernel.org
Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---
 drivers/i2c/busses/i2c-imx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 1987a0fd8132..09934f3b13ef 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1108,6 +1108,9 @@ static irqreturn_t i2c_imx_master_isr(struct imx_i2c_struct *i2c_imx, unsigned i
 	switch (i2c_imx->state) {
 	case IMX_I2C_STATE_DMA:
 		i2c_imx->i2csr = status;
+		if ((status & I2SR_RXAK) &&
+		    i2c_imx->dma->dma_transfer_dir == DMA_MEM_TO_DEV)
+			complete(&i2c_imx->dma->cmd_complete);
 		wake_up(&i2c_imx->queue);
 		break;
 
@@ -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);
-- 
2.56.0

Re: [PATCH 1/2] i2c: imx: do not carry the interrupt status over to the next transfer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] i2c: imx: incorrect message index for I2C_M_RECV_LEN check in
  i2c_imx_xfer_common() [1]
--

commit 264afe43ed92a8a97214ca4df23166f59bf1e387
Author: Mehmet Fide <mehmet.fide@screeningeagle.com>

i2c: imx: do not carry the interrupt status over to the next transfer

This commit clears i2c_imx->i2csr at the start of i2c_imx_xfer_common() so
that stale interrupt status from a failed DMA transfer does not cause a
subsequent DMA read to complete prematurely.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index ccf08cd63151f..1987a0fd81329 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1657,6 +1657,8 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
>  	if (result)
>  		return result;
>  
> +	i2c_imx->i2csr = 0;
> +
>  	/* Start I2C transfer */
>  	result = i2c_imx_start(i2c_imx, atomic);
>  	if (result) {

[Severity: High]
This problem wasn't introduced by this patch, but should block_data check
msgs[i].flags instead of msgs->flags [1] in the message loop of
i2c_imx_xfer_common()?

drivers/i2c/busses/i2c-imx.c:i2c_imx_xfer_common() {
    ...
		if (msgs[i].flags & I2C_M_RD) {
			int block_data = msgs->flags & I2C_M_RECV_LEN;

			if (atomic)
				result = i2c_imx_atomic_read(i2c_imx, &msgs[i], is_lastmsg);
			else if (use_dma && !block_data)
				result = i2c_imx_dma_read(i2c_imx, &msgs[i], is_lastmsg);
    ...
}

In a combined transfer such as an SMBus block read where msgs[0] is a
command write and msgs[1] is a DMA-eligible read with I2C_M_RECV_LEN set,
msgs->flags tests msgs[0].flags, so block_data is 0 for msgs[1].  That
routes msgs[1] to i2c_imx_dma_read(), which does not parse the initial
SMBus block length byte and corrupts or fails the read.

[1] https://sashiko.dev/#/bug/linux-f62df5ed-ad96-43f2-872a-ae2c0100b73f

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

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

[sashiko-bot]

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

Re: [PATCH 0/2] i2c: imx: two fixes for a DMA write that is not acknowledged

[Mehmet Fide]

A correction to the test note in this cover letter.

The v6.12.117 kernel I tested with is not plain stable. It carries a
backport of commit 5f5c2d4579ca ("i2c: imx: prevent rescheduling in non
dma mode"), so its i2c-imx.c has the interrupt state machine of
mainline. "The touched code is the same" is true for that tree only.

For stable: both patches apply to 6.18.y and 7.3.y. On 6.12.y and older
patch 1 applies with fuzz and patch 2 does not apply, as the interrupt
handler is different there. I can send backports for those trees once
the patches are in mainline.