[PATCH v4] i2c: imx: honour I2C_M_IGNORE_NAK

[PATCH v4] i2c: imx: honour I2C_M_IGNORE_NAK

[Haobin Jiang]

I2C_M_IGNORE_NAK is removed from the flags rejected by
i2c_imx_check_msgs(), so the flag is honoured as documented.
I2C_M_STOP stays unhandled: the controller emits a STOP after the
final message and repeated STARTs between messages, so rejecting it
would only break the existing ov2659/SCCB use case. I2C_M_NO_RD_ACK
stays rejected: the controller always clocks the ninth acknowledge
bit, so it cannot be implemented.

This applies on top of Vincent Jardin's I2C_M_REV_DIR_ADDR and
I2C_M_NOSTART support [1], merged as c2867bb0fa1d ("i2c: imx: add
I2C_M_REV_DIR_ADDR and I2C_M_NOSTART").

Tested on an ALIENTEK i.MX6ULL mini board (v6.6.44, equivalent
backport) with a logic analyzer: with the flag, a 3-byte write to an
unpopulated address clocks out every byte (each NAK'd) and returns 0;
without the flag it aborts with -ENXIO after the address byte. A
4-byte read with the flag completes and returns 0 as well. Waveforms
and test logs: https://github.com/JHB11Hinson/i2c-imx-ignore-nak-test

Note: part of the code was generated with the assistance
of a generative AI tool (GLM 5.3) from the author's description of
the problem, then reviewed and verified by the author.

Link: https://patch.msgid.link/20261023-for-upstream-i2c-imx-lx2160-reverse-v3-1-a07b249df381@free.fr # [1]
Link: https://lore.kernel.org/linux-i2c/tencent_342A87BFCECA70C9982E68A79AAD878B6909@qq.com/ # [2]

Signed-off-by: Haobin Jiang <lemonoutput@foxmail.com>

---
v3 -> v4:
	- Rebase onto i2c/i2c after Vincent Jardin's I2C_M_REV_DIR_ADDR
	  and I2C_M_NOSTART series was merged as c2867bb0fa1d [1].
	- Previous version: [2].
	- No functional change from v3.
---
 drivers/i2c/busses/i2c-imx.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index ccf08cd63..8e422b45d 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -612,9 +612,12 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx, bool atomic)
 	return 0;
 }
 
-static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx)
+static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx, bool ignore_nak)
 {
 	if (imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR) & I2SR_RXAK) {
+		if (ignore_nak)
+			return 0;
+
 		dev_dbg(&i2c_imx->adapter.dev, "<%s> No ACK\n", __func__);
 		return -ENXIO;  /* No ACK */
 	}
@@ -971,11 +974,15 @@ static int i2c_imx_unreg_slave(struct i2c_client *client)
 	return ret;
 }
 
-static inline int i2c_imx_isr_acked(struct imx_i2c_struct *i2c_imx)
+static inline int i2c_imx_isr_acked(struct imx_i2c_struct *i2c_imx,
+				    bool ignore_nak)
 {
 	i2c_imx->isr_result = 0;
 
 	if (imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR) & I2SR_RXAK) {
+		if (ignore_nak)
+			return 0;
+
 		i2c_imx->state = IMX_I2C_STATE_FAILED;
 		i2c_imx->isr_result = -ENXIO;
 		wake_up(&i2c_imx->queue);
@@ -988,7 +995,7 @@ static inline int i2c_imx_isr_write(struct imx_i2c_struct *i2c_imx)
 {
 	int result;
 
-	result = i2c_imx_isr_acked(i2c_imx);
+	result = i2c_imx_isr_acked(i2c_imx, i2c_imx->msg->flags & I2C_M_IGNORE_NAK);
 	if (result)
 		return result;
 
@@ -1017,7 +1024,7 @@ static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
 {
 	int result;
 
-	result = i2c_imx_isr_acked(i2c_imx);
+	result = i2c_imx_isr_acked(i2c_imx, i2c_imx->msg->flags & I2C_M_IGNORE_NAK);
 	if (result)
 		return result;
 
@@ -1249,7 +1256,8 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
 	if (result)
 		return result;
 
-	return i2c_imx_acked(i2c_imx);
+	/* I2C_M_IGNORE_NAK messages never take the DMA path. */
+	return i2c_imx_acked(i2c_imx, false);
 }
 
 static int i2c_imx_prepare_read(struct imx_i2c_struct *i2c_imx,
@@ -1268,7 +1276,7 @@ static int i2c_imx_prepare_read(struct imx_i2c_struct *i2c_imx,
 		result = i2c_imx_trx_complete(i2c_imx, !use_dma);
 		if (result)
 			return result;
-		result = i2c_imx_acked(i2c_imx);
+		result = i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
 		if (result)
 			return result;
 	}
@@ -1401,7 +1409,7 @@ static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
 		result = i2c_imx_trx_complete(i2c_imx, true);
 		if (result)
 			return result;
-		result = i2c_imx_acked(i2c_imx);
+		result = i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
 		if (result)
 			return result;
 	}
@@ -1416,7 +1424,7 @@ static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
 		result = i2c_imx_trx_complete(i2c_imx, true);
 		if (result)
 			return result;
-		result = i2c_imx_acked(i2c_imx);
+		result = i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
 		if (result)
 			return result;
 	}
@@ -1613,8 +1621,9 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
 	return ret;
 }
 
+/* I2C_M_STOP is the controller's native behaviour; don't reject it. */
 #define I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS \
-	(I2C_M_IGNORE_NAK | I2C_M_NO_RD_ACK | I2C_M_STOP)
+	I2C_M_NO_RD_ACK
 
 static int i2c_imx_check_msgs(struct i2c_msg *msgs, int num)
 {
@@ -1712,6 +1721,10 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
 		use_dma = i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD &&
 			msgs[i].flags & I2C_M_DMA_SAFE &&
 			!(msgs[i].flags & I2C_M_NOSTART);
+
+		/* A NAK'd byte may not generate a further DMA request. */
+		if (msgs[i].flags & I2C_M_IGNORE_NAK)
+			use_dma = false;
 		if (msgs[i].flags & I2C_M_RD) {
 			int block_data = msgs->flags & I2C_M_RECV_LEN;
 

base-commit: 123cffeca4a33d770c29ff8d3f22453a5fd59c32
-- 
2.34.1

Re: [PATCH v4] i2c: imx: honour I2C_M_IGNORE_NAK

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] i2c: imx: out-of-bounds write on zero-length read in i2c_imx_isr_read()
--

commit 9dd0c3d38f0968bb16ae7d2a6e9822f507de880e
Author: Haobin Jiang <lemonoutput@foxmail.com>

i2c: imx: honour I2C_M_IGNORE_NAK

This commit honours I2C_M_IGNORE_NAK in the i.MX I2C controller driver by
removing I2C_M_IGNORE_NAK and I2C_M_STOP from the unsupported protocol
mangling flags and ignoring missing ACKs when I2C_M_IGNORE_NAK is set.
Messages with I2C_M_IGNORE_NAK are also forced to use the non-DMA transfer
path.

Link: https://patch.msgid.link/20261023-for-upstream-i2c-imx-lx2160-reverse-v3-1-a07b249df381@free.fr # [1]
Link: https://lore.kernel.org/linux-i2c/tencent_342A87BFCECA70C9982E68A79AAD878B6909@qq.com/ # [2]

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index ccf08cd63151f..8e422b45dc2bb 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c

[ ... ]

> @@ -988,7 +995,7 @@ static inline int i2c_imx_isr_write(struct imx_i2c_struct *i2c_imx)
>  {
>  	int result;
>  
> -	result = i2c_imx_isr_acked(i2c_imx);
> +	result = i2c_imx_isr_acked(i2c_imx, i2c_imx->msg->flags & I2C_M_IGNORE_NAK);
>  	if (result)
>  		return result;
>  
> @@ -1017,7 +1024,7 @@ static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
>  {
>  	int result;
>  
> -	result = i2c_imx_isr_acked(i2c_imx);
> +	result = i2c_imx_isr_acked(i2c_imx, i2c_imx->msg->flags & I2C_M_IGNORE_NAK);
>  	if (result)
>  		return result;
>  

[Severity: High]
What happens here if a zero-length read message (msg->len == 0 with
I2C_M_RD) is transferred, particularly now that I2C_M_IGNORE_NAK allows
i2c_imx_isr_acked() to succeed even when no device ACKs the address byte?

In i2c_imx_check_msgs(), zero-length messages are only rejected when
I2C_M_NOSTART is set:

drivers/i2c/busses/i2c-imx.c:i2c_imx_check_msgs() {
	...
		if (msgs[i].flags & I2C_M_NOSTART) {
			if (i == 0 || msgs[i].len == 0)
				return -EOPNOTSUPP;
	...
}

Unlike i2c_imx_isr_write(), which checks whether i2c_imx->msg->len ==
i2c_imx->msg_buf_idx after i2c_imx_isr_acked(), i2c_imx_isr_read()
unconditionally calls i2c_imx_setup_read() and transitions the state machine
in i2c_imx_master_isr() to IMX_I2C_STATE_READ_CONTINUE:

drivers/i2c/busses/i2c-imx.c:i2c_imx_setup_read() {
	...
	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
	temp &= ~I2CR_MTX;
	if ((i2c_imx->msg->len - 1) || (i2c_imx->msg->flags & I2C_M_RECV_LEN))
		temp &= ~I2CR_TXAK;

	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
	imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
}

When msg->len is 0, (i2c_imx->msg->len - 1) is non-zero, so
i2c_imx_setup_read() clears I2CR_MTX and I2CR_TXAK and performs a dummy read
of IMX_I2C_I2DR, causing the controller to clock in a data byte. When that
byte completes, i2c_imx_master_isr() -> i2c_imx_isr_read_continue() runs:

drivers/i2c/busses/i2c-imx.c:i2c_imx_isr_read_continue() {
	...
	if ((i2c_imx->msg->len - 1) == i2c_imx->msg_buf_idx) {
		...
	} else if (i2c_imx->msg_buf_idx == (i2c_imx->msg->len - 2)) {
		...
	}

	i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
	return next_state;
}

Because msg->len is 0, neither (msg->len - 1) nor (msg->len - 2) matches
msg_buf_idx, so i2c_imx_isr_read_continue() writes out of bounds into
i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] on every interrupt (dereferencing
ZERO_SIZE_PTR when called via I2C_RDWR ioctl with len == 0, or overflowing
msgbuf0 on the stack during an emulated SMBus Quick Read). Similarly, in
i2c_imx_atomic_read() -> i2c_imx_prepare_read(), the controller is switched
to receive mode and performs a dummy read even when msgs->len is 0.

Should zero-length reads be handled before calling i2c_imx_setup_read(), or
rejected via adapter quirks / i2c_imx_check_msgs()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_877C99956190215D644EB959DF1663542A0A@qq.com?part=1