Re: [PATCH v3 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] `i2c_imx_prepare_read()` fails to clear `I2CR_TXAK` for SMBus block reads, causing the block length byte to be incorrectly NACKed.
- [High] The interrupt-driven SMBus block read path (`i2c_imx_isr_read_block_data_len`) fails to properly terminate the transaction when receiving an invalid length (0 or > I2C_SMBUS_BLOCK_MAX), leaving the I2C bus stuck.
--

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

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

SMBus 3.1 6.5.7 allows a Block Read byte count of 0, but the atomic
path rejects it as -EPROTO and returns without sending a nack and stop.
This patch properly handles the in-flight dummy byte by extending the
message length to emit a stop, releasing the bus.

> SMBus 3.1 6.5.7 allows a Block Read byte count of 0, but the atomic
> (polling) path 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 for
> this i2c controller.
> 
> Reading I2DR to obtain the count likewise arms the next byte on the
> count > I2C_SMBUS_BLOCK_MAX path, which also returned -EPROTO directly
> and left the bus held.
> 
> Handle both: NACK the in-flight dummy byte (TXAK) and extend msgs->len so
> the existing last-byte handling emits STOP; the dummy byte is discarded.
> A count of 0 is a valid empty block read; a count above
> I2C_SMBUS_BLOCK_MAX is still reported as -EPROTO, but only after the bus
> has been released.
> 
> The interrupt-driven path has the same flaw from a later commit and is
> fixed separately, as it carries a different Fixes: tag and stable range.

[Severity: High]
This isn't a bug introduced by this patch, but does the interrupt-driven
SMBus block read path in i2c_imx_isr_read_block_data_len() fail to properly
terminate the transaction when receiving an invalid length?

When a length of 0 or greater than I2C_SMBUS_BLOCK_MAX is returned:

	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);
	}

This aborts the transfer and wakes up the thread to clear MSTA without
gracefully nacking the in-flight dummy byte. Clearing MSTA while a
byte reception is in flight causes the controller to drop its master
state while the bus remains physically locked with SCL and SDA held low.

I see the commit message acknowledges this flaw exists in the interrupt
path but defers fixing it to a separate patch. Since that patch is not
included in this series, the hang remains active in the codebase.

> 
> Fixes: 8e8782c71595 ("i2c: imx: add SMBus block read support")
> Cc: stable@vger.kernel.org # v3.16+
> Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Acked-by: Carlos Song <carlos.song@nxp.com>
> Reviewed-by: Stefan Eichenberger <eichest@gmail.com>
> Signed-off-by: Vincent Jardin <vjardin@free.fr>
>
> ---
> drivers/i2c/busses/i2c-imx.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 28313d0fad37e..cfd1e63359e75 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1415,6 +1415,7 @@ static int i2c_imx_atomic_read(struct imx_i2c_struct *i2c_imx,
>  	int i, result;
>  	unsigned int temp;
>  	int block_data = msgs->flags & I2C_M_RECV_LEN;
> +	int block_err = 0;
>  
>  	result = i2c_imx_prepare_read(i2c_imx, msgs, false);

[Severity: High]
This is a pre-existing issue, but does i2c_imx_prepare_read() fail to clear
I2CR_TXAK for SMBus block reads?

In i2c_imx_prepare_read(), there is no check for I2C_M_RECV_LEN:

	if (msgs->len - 1)
		temp &= ~I2CR_TXAK;

For an I2C_M_RECV_LEN transaction without PEC, the core initializes
msgs->len to 1. This evaluates to 0, which leaves TXAK set and causes the
controller to nack the very first byte (the length byte). 

A compliant SMBus slave will immediately abort the transaction upon receiving
a nack, causing all subsequent data bytes to read as 0xFF. Should this check
include the I2C_M_RECV_LEN flag to properly clear TXAK?

>  	if (result)
[ ... ]

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