[PATCH v5] i2c: imx: honour I2C_M_IGNORE_NAK

[PATCH v5] i2c: imx: honour I2C_M_IGNORE_NAK

[Haobin Jiang]

Honour I2C_M_IGNORE_NAK for supported PIO transfers and keep flagged
messages out of DMA.

Following Sashiko's review, reject zero-length reads carrying
I2C_M_IGNORE_NAK before START. This prevents the newly supported flag
from exposing an existing receive-path bug, without changing unflagged
zero-length reads.

Link: https://lore.kernel.org/linux-i2c/tencent_877C99956190215D644EB959DF1663542A0A@qq.com/ # [1]

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

---
v4 -> v5:
	- Add a scoped zero-length read guard following Sashiko's review.
	- Previous version: [1].
---
 drivers/i2c/busses/i2c-imx.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index ccf08cd63151f25581dd3059ee874b606d0dc94c..1e2fd699f4fc658f4a9298cd1852455966018e37 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)
 {
@@ -1625,6 +1634,12 @@ static int i2c_imx_check_msgs(struct i2c_msg *msgs, int num)
 		if (msgs[i].flags & I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS)
 			return -EOPNOTSUPP;
 
+		/* The receive path cannot handle zero-length reads. */
+		if ((msgs[i].flags & I2C_M_RD) &&
+		    (msgs[i].flags & I2C_M_IGNORE_NAK) &&
+		    msgs[i].len == 0)
+			return -EOPNOTSUPP;
+
 		if (msgs[i].flags & I2C_M_NOSTART) {
 			/*
 			 * NOSTART continues an already open frame. The first
@@ -1712,6 +1727,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

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

[Carlos Song]

Hi,

Why not follow community rule to make patch...?

At higher version, better commit log and put change to commit log bottom
with extra lines like your V3 patch[2].

I think V5 patch commit log should be better with shorter and clearer
problem description, reproduction, resolution, verification based on V2
patch[1] and V3 patch[2].

But abviously, this patch is generated by AI, and it dropped most commit
and discussion details from previous patches. No one wants to review
such a haphazard submission.

AI-generated patches should be subject to more rigorous review and
testing. Remember that AI is a tool, the AI patches should has higher
Standards and greater Professionalism than patch created by humans.
Wharever humans must always remain in control in whole flow.

High quality over high speed!

[1]
https://lore.kernel.org/linux-i2c/tencent_E6A52FBBB25132C90B8E296FEB96E2C8CA07@qq.com/
[2]
https://lore.kernel.org/linux-i2c/tencent_342A87BFCECA70C9982E68A79AAD878B6909@qq.com/

Thanks,
Carlos Song


> Honour I2C_M_IGNORE_NAK for supported PIO transfers and keep flagged
> messages out of DMA.
> 
> Following Sashiko's review, reject zero-length reads carrying
> I2C_M_IGNORE_NAK before START. This prevents the newly supported flag
> from exposing an existing receive-path bug, without changing unflagged
> zero-length reads.
> 
> Link: https://lore.kernel.org/linux-i2c/tencent_877C99956190215D644EB959DF1663542A0A@qq.com/ # [1]
> 
> Signed-off-by: Haobin Jiang <lemonoutput@foxmail.com>
> 
> ---
> v4 -> v5:
> 	- Add a scoped zero-length read guard following Sashiko's review.
> 	- Previous version: [1].
> ---
>  drivers/i2c/busses/i2c-imx.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index ccf08cd63151f25581dd3059ee874b606d0dc94c..1e2fd699f4fc658f4a9298cd1852455966018e37 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)
>  {
> @@ -1625,6 +1634,12 @@ static int i2c_imx_check_msgs(struct i2c_msg *msgs, int num)
>  		if (msgs[i].flags & I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS)
>  			return -EOPNOTSUPP;
>  
> +		/* The receive path cannot handle zero-length reads. */
> +		if ((msgs[i].flags & I2C_M_RD) &&
> +		    (msgs[i].flags & I2C_M_IGNORE_NAK) &&
> +		    msgs[i].len == 0)
> +			return -EOPNOTSUPP;
> +
>  		if (msgs[i].flags & I2C_M_NOSTART) {
>  			/*
>  			 * NOSTART continues an already open frame. The first
> @@ -1712,6 +1727,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

-- 
Best Regards,
Carlos

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

[Haobin Jiang]

Hi Carlos,

> Why not follow community rule to make patch...?

You are right, and thank you for taking the time to point it out.

> At higher version, better commit log and put change to commit log bottom
> with extra lines like your V3 patch[2].

Agreed. When I prepared V5, I regenerated the log from the current diff
and did not carry forward the problem description, the test evidence, or
the review history from V2/V3/V4. That is my responsibility as the
submitter.

> I think V5 patch commit log should be better with shorter and clearer
> problem description, reproduction, resolution, verification based on V2
> patch[1] and V3 patch[2].

I will rewrite it that way for V6. The commit log will cover the
problem, the resolution, the reproduction/verification, and the review
history with links to V1-V5. I will put the changelog below a single
`---` line, like V3.

> But abviously, this patch is generated by AI, and it dropped most commit
> and discussion details from previous patches.

That is a fair criticism of V5. The tool produced a shorter log; I should
have caught that before sending. I will restore the AI-assistance note
from V3/V4 and describe which parts were tool-assisted and how I verified
them. I will not resend until the V6 log is self-contained and I have
re-checked the new zero-length guard. I will state clearly what was and
was not tested.

One point I want to be explicit about: the V5 guard only rejects
I2C_M_RD + I2C_M_IGNORE_NAK + len == 0. It does not fix the pre-existing
unflagged zero-length read path. That scope is a deliberate choice, and
I will document it in V6 rather than imply the general issue is fixed.

> AI-generated patches should be subject to more rigorous review and
> testing... humans must always remain in control in whole flow.

Understood. I will keep the human review and test evidence in the
changelog, and I will be the one accountable for the content.

Thanks,
Haobin