Re: [PATCH RESEND v2] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] i2c: imx: premature NACK on read messages followed by I2C_M_RD | I2C_M_NOSTART
--

commit cc53d2cc1adbe1a23f92c25512b4a73a38c5f255
Author: Vincent Jardin <vjardin@free.fr>

i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART

This commit adds support for I2C_M_REV_DIR_ADDR and I2C_M_NOSTART to the
i2c-imx driver so that devices such as the Realtek RTL8366SE switch can
perform SMI read frames. It also adds message validation in
i2c_imx_check_msgs() and advertises I2C_FUNC_NOSTART and
I2C_FUNC_PROTOCOL_MANGLING.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 9477d814fde94..7dc9c43673eee 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c

[ ... ]

> @@ -1529,22 +1561,29 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
>  
>  	dev_dbg(&i2c_imx->adapter.dev,
>  		"<%s> write slave address: addr=0x%x\n",
> -		__func__, i2c_8bit_addr_from_msg(msgs));
> +		__func__, i2c_imx_addr_byte(msgs));
>  
>  	i2c_imx->is_lastmsg = is_lastmsg;
>  
> -	if (block_data)
> -		i2c_imx->state = IMX_I2C_STATE_READ_BLOCK_DATA;
> -	else
> -		i2c_imx->state = IMX_I2C_STATE_READ;
>  	i2c_imx->msg = msgs;
>  	i2c_imx->msg_buf_idx = 0;
>  
> -	/*
> -	 * By writing the device address we start the state machine in the ISR.
> -	 * The ISR will report when it is done or when it fails.
> -	 */
> -	imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
> +	if (msgs->flags & I2C_M_NOSTART) {
> +		i2c_imx->state = block_data ? IMX_I2C_STATE_READ_BLOCK_DATA_LEN
> +					    : IMX_I2C_STATE_READ_CONTINUE;
> +		i2c_imx_setup_read(i2c_imx);
> +	} else {
> +		if (block_data)
> +			i2c_imx->state = IMX_I2C_STATE_READ_BLOCK_DATA;
> +		else
> +			i2c_imx->state = IMX_I2C_STATE_READ;
> +
> +		/*
> +		 * By writing the device address we start the state machine in the ISR.
> +		 * The ISR will report when it is done or when it fails.
> +		 */
> +		imx_i2c_write_reg(i2c_imx_addr_byte(msgs), i2c_imx, IMX_I2C_I2DR);
> +	}

[ ... ]

> @@ -1574,6 +1613,33 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
>  	return ret;
>  }
>  
> +#define I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS \
> +	(I2C_M_IGNORE_NAK | I2C_M_NO_RD_ACK | I2C_M_STOP)
> +
> +static int i2c_imx_check_msgs(struct i2c_msg *msgs, int num)
> +{
> +	int i;
> +
> +	for (i = 0; i < num; i++) {
> +		/* Reject rather than silently transfer */
> +		if (msgs[i].flags & I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS)
> +			return -EOPNOTSUPP;
> +
> +		if (msgs[i].flags & I2C_M_NOSTART) {
> +			/*
> +			 * NOSTART continues an already open frame. The first
> +			 * message has nothing to continue from,
> +			 * a 0 length continuation leaves the controller with
> +			 * no byte to clock.
> +			 */
> +			if (i == 0 || msgs[i].len == 0)
> +				return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
What happens if a caller splits a read across two messages, passing
msgs[i - 1] with I2C_M_RD followed by msgs[i] with I2C_M_RD | I2C_M_NOSTART?

In i2c_imx_check_msgs(), that sequence is accepted as long as i > 0 and
msgs[i].len > 0. However, the read paths for msgs[i - 1]
(i2c_imx_setup_read(), i2c_imx_isr_read_continue(), i2c_imx_prepare_read(),
i2c_imx_atomic_read(), and i2c_imx_dma_read()) unconditionally set I2CR_TXAK
on the last byte of msgs[i - 1] and set I2CR_MTX before reading the final byte
from IMX_I2C_I2DR:

drivers/i2c/busses/i2c-imx.c:i2c_imx_isr_read_continue() {
	if ((i2c_imx->msg->len - 1) == i2c_imx->msg_buf_idx) {
		...
		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
		temp |= I2CR_MTX;
		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
		next_state = IMX_I2C_STATE_DONE;
	} else if (i2c_imx->msg_buf_idx == (i2c_imx->msg->len - 2)) {
		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
		temp |= I2CR_TXAK;
		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
	}
}

Because the controller sends a NACK at the end of msgs[i - 1], the target
device aborts transmission and releases SDA. When msgs[i] then runs
i2c_imx_setup_read() (or i2c_imx_prepare_read() in atomic mode), it clears
I2CR_MTX, performs another dummy read of IMX_I2C_I2DR, and clocks in 0xff for
the remainder of the transfer.

Should i2c_imx_check_msgs() reject I2C_M_RD | I2C_M_NOSTART when preceded by
an I2C_M_RD message (or conversely, when a non-final read message is followed
by I2C_M_NOSTART), or should the read state machine suppress I2CR_TXAK,
I2CR_MTX, and the second dummy read across the message boundary?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-for-upstream-i2c-imx-lx2160-reverse-v2-1-03afae754886@free.fr?part=1