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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Atomic SMBus block reads without PEC fail because `i2c_imx_prepare_read` incorrectly NACKs the length byte.
- [High] Returning `-EPROTO` directly when `len > I2C_SMBUS_BLOCK_MAX` abandons an in-flight receive cycle, leaving the I2C bus permanently stuck.
--

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

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

This commit fixes a locked I2C bus issue in atomic mode when an SMBus
block-read has a byte count of 0. It handles count=0 by NACKing the
in-flight dummy byte and adjusting the message length to emit a STOP
condition properly.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1436,8 +1436,19 @@ static int i2c_imx_atomic_read(struct imx_i2c_struct *i2c_imx,
>  		 */
>  		if ((!i) && block_data) {

[Severity: High]
This is a pre-existing issue, but does i2c_imx_prepare_read() incorrectly
NACK the length byte for atomic SMBus block reads without PEC?

When i2c_imx_prepare_read() is called earlier in this function to clear the
I2CR_TXAK flag and ACK the initial bytes, it uses a check for (msgs->len - 1).
For SMBus block reads (I2C_M_RECV_LEN), the initial msgs->len provided by the
I2C core is 1.

This causes msgs->len - 1 to evaluate to 0, which leaves TXAK set to 1. The
controller then NACKs the very first length byte, causing the target to abort
the transfer and immediately fail atomic SMBus block reads.

>  			len = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> -			if ((len == 0) || (len > I2C_SMBUS_BLOCK_MAX))
> +			if (len > I2C_SMBUS_BLOCK_MAX)
>  				return -EPROTO;

[Severity: High]
This is a pre-existing issue, but does returning -EPROTO directly here
abandon an in-flight receive cycle and leave the bus permanently stuck?

When reading the length byte via imx_i2c_read_reg(..., IMX_I2C_I2DR), the
hardware automatically initiates the reception of the next byte. If the length
exceeds I2C_SMBUS_BLOCK_MAX, returning directly without generating a NACK or
STOP condition abandons the transfer.

Because the target device remains in the middle of a transmit cycle holding
SDA low waiting for clock pulses, and atomic polling mode disables sleepable
GPIO bus recovery functions, does this cause the I2C bus to wedge until a
power cycle?

Should the same graceful abort sequence used for len == 0 (NACKing the
in-flight dummy byte and emitting a STOP) be applied here as well?

> +			if (len == 0) {
> +				/*
> +				 * SMBus 3.1 6.5.7: support count byte of 0.
> +				 */
> +				temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +				temp |= I2CR_TXAK;
> +				imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +				msgs->buf[0] = 0;
> +				msgs->len = 2;
> +				continue;
> +			}
>  			dev_dbg(&i2c_imx->adapter.dev,
>  				"<%s> read length: 0x%X\n",
>  				__func__, len);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525-for-upstream-i2c-lx2160-fix-v1-v1-0-f30ab53dd97c@free.fr?part=1