Re: [PATCH v3 2/2] i2c: imx: fix locked bus on SMBus block-read of 0 (IRQ)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] SMBus block read with a received length of 1 fails to NACK the final data byte in the ISR state machine, potentially causing a bus hang.
--

--- Patch [2]: [PATCH v3 2/2] i2c: imx: fix locked bus on SMBus block-read of 0 (IRQ) ---
commit 2303c0a2b014ac50e9fdc1a3b61bbfbe99012c6a
Author: Vincent Jardin <vjardin@free.fr>

i2c: imx: fix locked bus on SMBus block-read of 0 (IRQ)

SMBus 3.1 6.5.7 allows a Block Read byte count of 0, but the
interrupt-driven block-read state machine rejects it as -EPROTO. Worse,
it returns without a NACK+STOP: the next receive cycle has already
started, so the target keeps holding SDA and the bus stays stuck until a
power cycle of this i2c controller.

> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1061,11 +1061,28 @@ static inline enum imx_i2c_state i2c_imx_isr_read_continue(struct imx_i2c_struct
>  static inline void i2c_imx_isr_read_block_data_len(struct imx_i2c_struct *i2c_imx)
>  {
>  	u8 len = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> +	unsigned int temp;
>  
>  	if (len == 0 || len > I2C_SMBUS_BLOCK_MAX) {

[ ... ]

>  		i2c_imx->isr_result = -EPROTO;
>  		i2c_imx->state = IMX_I2C_STATE_FAILED;
>  		wake_up(&i2c_imx->queue);
> +		return;
>  	}
>  	i2c_imx->msg->len += len;
>  	i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = len;

[Severity: High]
This is a pre-existing issue, but does this code properly handle the case where
the received length is 1?

When len is 1, the length is updated but we fail to set the I2CR_TXAK bit. Since
reading I2DR at the start of i2c_imx_isr_read_block_data_len() already initiated
reception of the final byte, doesn't the master end up ACKing the final byte
instead of NACKing it?

If the slave holds SDA low, and we later try to generate a STOP condition in
i2c_imx_isr_read_continue():

	if ((i2c_imx->msg->len - 1) == i2c_imx->msg_buf_idx) {
		if (i2c_imx->is_lastmsg) {
			/* ... */
			temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
			if (!(temp & I2CR_MSTA))
				i2c_imx->stopped =  1;
			temp &= ~(I2CR_MSTA | I2CR_MTX);
			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);

Will the STOP fail and permanently hang the bus because SDA is held low?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-for-upstream-i2c-lx2160-fix-v1-v3-0-073ac9e103a5@free.fr?part=2