[PATCH] i2c: imx: test the block read flag of the message that is read

[PATCH] i2c: imx: test the block read flag of the message that is read

[Mehmet Fide]

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

i2c_imx_xfer_common() decides between the DMA and the interrupt driven
read with I2C_M_RECV_LEN of the first message of the transfer instead of
the message at hand. A block read of DMA_THRESHOLD bytes or more that
follows another message is therefore done with DMA, which reads the
initial length and does not look at the count byte.

Fixes: b460b15b3cc2 ("i2c: imx: separate atomic, dma and non-dma use case")
Cc: stable@vger.kernel.org
Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---
Reported by the Sashiko review of another i2c-imx patch:
https://sashiko.dev/#/bug/linux-f62df5ed-ad96-43f2-872a-ae2c0100b73f

Tested on a Colibri VF50 (i2c0 with DMA) with a v6.12.117 kernel that
carries the i2c-imx changes of v6.13 and later as backports; on this
base compile tested only. An I2C_RDWR transfer of a one byte write
followed by a read with I2C_M_RECV_LEN and an initial length of 16:
without the patch 16 bytes are read and the count byte is taken as data,
with it the count byte is evaluated (here -EPROTO, the device answers
with 0x25).

The line is from v6.13, the patch applies to 6.18.y and 7.3.y.

 drivers/i2c/busses/i2c-imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index ccf08cd63151..9abe0a26c877 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1713,7 +1713,7 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
 			msgs[i].flags & I2C_M_DMA_SAFE &&
 			!(msgs[i].flags & I2C_M_NOSTART);
 		if (msgs[i].flags & I2C_M_RD) {
-			int block_data = msgs->flags & I2C_M_RECV_LEN;
+			int block_data = msgs[i].flags & I2C_M_RECV_LEN;
 
 			if (atomic)
 				result = i2c_imx_atomic_read(i2c_imx, &msgs[i], is_lastmsg);

base-commit: 25e7ab89f4774b0f842cee0e8a20c11f755924d5
-- 
2.56.0

Re: [PATCH] i2c: imx: test the block read flag of the message that is read

[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: uncleared I2CR_TXAK for I2C_M_RECV_LEN in
  i2c_imx_prepare_read() [1]
--

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

i2c: imx: test the block read flag of the message that is read

This commit changes i2c_imx_xfer_common() to check I2C_M_RECV_LEN on the
current message (msgs[i]) rather than the first message of the transfer
when deciding whether to use DMA for a read.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index ccf08cd63151f..9abe0a26c877a 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1713,7 +1713,7 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
>  			msgs[i].flags & I2C_M_DMA_SAFE &&
>  			!(msgs[i].flags & I2C_M_NOSTART);
>  		if (msgs[i].flags & I2C_M_RD) {
> -			int block_data = msgs->flags & I2C_M_RECV_LEN;
> +			int block_data = msgs[i].flags & I2C_M_RECV_LEN;
>  
>  			if (atomic)
>  				result = i2c_imx_atomic_read(i2c_imx, &msgs[i], is_lastmsg);

[Severity: High]
This problem wasn't introduced by this patch, but in the
i2c_imx_atomic_read() -> i2c_imx_prepare_read() path just below, does
i2c_imx_prepare_read() fail to clear I2CR_TXAK for an SMBus block read
([1])?

drivers/i2c/busses/i2c-imx.c:i2c_imx_prepare_read() {
	...
	/*
	 * Reset the I2CR_TXAK flag initially for SMBus block read since the
	 * length is unknown
	 */
	if (msgs->len - 1)
		temp &= ~I2CR_TXAK;
	...
}

i2c_smbus_xfer_emulated() initializes I2C_M_RECV_LEN read messages with
len = 1, and i2c_imx_start() sets I2CR_TXAK at the start of the transfer.
Unlike i2c_imx_isr_read(), i2c_imx_prepare_read() does not check
msgs->flags & I2C_M_RECV_LEN, so I2CR_TXAK stays set when the dummy read
of IMX_I2C_I2DR starts reception.  The controller then sends a NACK after
the initial length byte, causing the target device to abort the transfer
while the remaining payload bytes read as 0xff.

[1] https://sashiko.dev/#/bug/linux-09e3015b-fc03-4410-809b-82964cdfac9c

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