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

[Carlos Song <carlos.song@oss.nxp.com>]

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