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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] i2c: imx: I2C_M_STOP accepted on non-final messages in i2c_imx_check_msgs()
--

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

i2c: imx: honour I2C_M_IGNORE_NAK

This commit adds support for I2C_M_IGNORE_NAK on PIO transfers in the i.MX
I2C controller driver while keeping flagged messages out of the DMA path and
rejecting zero-length reads with I2C_M_IGNORE_NAK.

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

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

[ ... ]

> @@ -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;

[Severity: Medium]
Does removing I2C_M_STOP from I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS
cause i2c_imx_check_msgs() to accept multi-message transfers that set
I2C_M_STOP on a non-final message (i < num - 1) without actually generating
an inter-message STOP condition?

In i2c_imx_xfer_common(), is_lastmsg is only set when i == num - 1, and
i2c_imx_stop() is only called at the end of the entire transfer. For
intermediate messages, the controller keeps I2CR_MSTA asserted and emits a
repeated START on the next message:

drivers/i2c/busses/i2c-imx.c:i2c_imx_xfer_common() {
	for (i = 0; i < num; i++) {
		if (i == num - 1)
			is_lastmsg = true;

		if (i && !(msgs[i].flags & I2C_M_NOSTART)) {
			...
			temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
			temp |= I2CR_RSTA;
			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
			...
		}
}

When callers such as SCCB client drivers (via I2C_CLIENT_SCCB in
i2c_smbus_xfer_emulated(), which sets I2C_M_STOP | I2C_M_IGNORE_NAK on msg[0]
of a two-message read) or userspace I2C_RDWR ioctls pass I2C_M_STOP on an
intermediate message, shouldn't i2c_imx_check_msgs() still reject I2C_M_STOP
when i < num - 1 unless i2c_imx_xfer_common() emits a STOP followed by a new
START between those messages?

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