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

[Vincent Jardin <vjardin@free.fr>]

Hi M. Bot,


> > +}
> 
> [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?

hmm, ok, updated into the v3

thank you,