[PATCH 0/2] i2c: imx: fix SMBus block-read of 0 locking the bus

[PATCH 0/2] i2c: imx: fix SMBus block-read of 0 locking the bus

[Vincent Jardin]

i2c-imx rejects a SMBus Block Read byte count of 0 (valid per SMBus 3.1
6.5.7) and it returns without a NACK+STOP, leaving the target
holding SDA so the bus is stuck until a power cycle occur.

The same bug is occuring with two independently introduced spots, so the
fix is two patches with their respective Fixes: tags and backport ranges:

  1/2  atomic/polling path       Fixes: 8e8782c71595   v3.16+
  2/2  IRQ-driven state machine  Fixes: 5f5c2d4579ca   v6.13+

Signed-off-by: Vincent Jardin <vjardin@free.fr>
---
Vincent Jardin (2):
      i2c: imx: fix locked bus on SMBus block-read of 0 (atomic)
      i2c: imx: fix locked bus on SMBus block-read of 0 (IRQ)

 drivers/i2c/busses/i2c-imx.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)
---
base-commit: 6916d5703ddf9a38f1f6c2cc793381a24ee914c6
change-id: 20260525-for-upstream-i2c-lx2160-fix-v1-0cba0a0093e5

Best regards,
-- 
Vincent Jardin <vjardin@free.fr>

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

[Vincent Jardin]

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.

Accept count=0: NACK the in-flight dummy byte (TXAK) and extend msgs->len
so the existing last-byte handling emits STOP. The dummy byte is
discarded; block-read callers only consume buf[0..count-1].

The interrupt-driven path has the same flaw from a later commit and is
fixed separately, as it carries a different Fixes:

Fixes: 8e8782c71595 ("i2c: imx: add SMBus block read support")
Cc: <stable@vger.kernel.org> # v3.16+
Signed-off-by: Vincent Jardin <vjardin@free.fr>
---
 drivers/i2c/busses/i2c-imx.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index a208fefd3c3b..0cd4f5892591 100644
--- 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) {
 			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;
+			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);

-- 
2.43.0

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

[sashiko-bot]

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

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

[Vincent Jardin]

Hi bot,

Le 25/05/26 12:04, sashiko-bot@kernel.org a écrit :
> [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.

You're right that the guard is not consistent with its the comment. The
core hands a block read in with msgs->len == 1 (I2C_M_RECV_LEN;
i2c-core-smbus.c sets "msg[1].len = 1"), so "if (msgs->len - 1)" is false
and TXAK is not cleared, even though the comment right above says it
should be for block reads.

Maybe, a better design would be closer to "if (block_data || msgs->len > 1)". In
practice it is a latent topic only: TXAK is already 0 at that point, which
is why non-zero-count block reads work today. That is confusing.

In fact fixing that guard would not include the block-read-of-0 case because
the two act on different bytes:

  - the guard decides the ACK/NACK of the length byte (byte 0);

  - the count=0 lock/wedge is caused by the byte AFTER it. Reading I2DR to
    obtain the count already clocks in the next byte, so when the count
    is 0 a dummy byte is already in flight and must be NACKed + STOPed
    regardless of how the length byte was acked.

There is also an ordering constraint: we cannot emit STOP before reading
the length byte (we need it to know/learn the count is 0), so once it is read
the prefetch is unavoidably armed and needs the explicit NACK+STOP. So
the length-byte guard is a separate, nice-to-have cleanup.

> >  			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?

Good catch, and yes: it is the same root cause as count=0, once I2DR
has been read, the next byte is armed, so the bare "return -EPROTO" on
the len > I2C_SMBUS_BLOCK_MAX path has the same lock/wedge potential issue.
A v2 will fix both cases (count == 0 and count > I2C_SMBUS_BLOCK_MAX) with
the same NACK (TXAK) + STOP drain before returning, so neither leaves the
bus locked.

thanks,
  Vincent

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

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

[Vincent Jardin]

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.

Accept count=0: NACK the in-flight dummy byte (TXAK) and set msg->len to
2 so i2c_imx_isr_read_continue() emits STOP via its normal last-byte
path. The dummy byte is discarded; block-read callers only consume
buf[0..count-1].

While here, return early on the I2C_SMBUS_BLOCK_MAX error path instead
of falling through and overwriting msg->len/msg->buf with the rejected
count byte.

The atomic path regressed earlier (v3.16) and is fixed separately; this
patch covers only the v6.13 state-machine rework.

Fixes: 5f5c2d4579ca ("i2c: imx: prevent rescheduling in non dma mode")
Cc: <stable@vger.kernel.org> # v6.13+
Signed-off-by: Vincent Jardin <vjardin@free.fr>
---
 drivers/i2c/busses/i2c-imx.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 0cd4f5892591..8792cb5cb9a8 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1061,11 +1061,26 @@ 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) {
+	if (len > I2C_SMBUS_BLOCK_MAX) {
 		i2c_imx->isr_result = -EPROTO;
 		i2c_imx->state = IMX_I2C_STATE_FAILED;
 		wake_up(&i2c_imx->queue);
+		return;
+	}
+
+	if (len == 0) {
+		/*
+		 * SMBus 3.1 6.5.7 "Block Write/Read": byte count can be 0
+		 */
+		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+		temp |= I2CR_TXAK;
+		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
+		i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = 0;
+		i2c_imx->msg->len = 2;
+		return;
 	}
 	i2c_imx->msg->len += len;
 	i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = len;

-- 
2.43.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing I2C NACK on Error Path (Bus Lockup)
- [High] Missing I2C NACK on Block Read of 1 Byte
- [High] Race Condition in Lockless Register Access
--

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

i2c: imx: fix locked bus on SMBus block-read of 0 (IRQ)
    
This patch allows an SMBus Block Read with a byte count of 0. It fixes a
bus lockup by accepting a count of 0, NACKing the in-flight dummy byte
(TXAK), and emitting a STOP condition. It also adds an early return on the
error path when the count exceeds the maximum.

> 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
> @@ -1061,11 +1061,26 @@ 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) {
> +	if (len > I2C_SMBUS_BLOCK_MAX) {
>  		i2c_imx->isr_result = -EPROTO;
>  		i2c_imx->state = IMX_I2C_STATE_FAILED;
>  		wake_up(&i2c_imx->queue);
> +		return;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but does the error path for an
oversized length need to set the TXAK bit before aborting the transaction?

Because reading I2DR automatically initiates the reception of the next byte
with ACK enabled, returning early without setting I2CR_TXAK causes the
controller to ACK the in-flight byte.

If the target device drives SDA low, could the subsequent cleanup via
i2c_imx_stop() fail to emit a STOP condition and leave the bus permanently
locked, similar to the zero-length case this patch addresses?

> +
> +	if (len == 0) {
> +		/*
> +		 * SMBus 3.1 6.5.7 "Block Write/Read": byte count can be 0
> +		 */
> +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +		temp |= I2CR_TXAK;
> +		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);

[Severity: High]
This is a pre-existing issue, but is there a potential read-modify-write
race condition on the IMX_I2C_I2CR register here?

If an I2C transaction times out, the process context calls i2c_imx_stop(),
which locklessly clears the I2CR_MSTA bit.

If the hardware interrupt fires concurrently and the ISR reads the register
before i2c_imx_stop() clears MSTA, then writes it back afterward, could the
ISR overwrite the register with MSTA=1? Would this transition from 0 back to
1 cause the controller to erroneously generate a new START condition?

> +
> +		i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = 0;
> +		i2c_imx->msg->len = 2;
> +		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 the master fail to NACK the final data
byte when performing an SMBus block read with exactly 1 byte of data?

Reading the length byte from I2DR triggers the reception of the single data
byte, but the code does not appear to set I2CR_TXAK when the length is exactly
1.

During the next interrupt, when i2c_imx_isr_read_continue() attempts to issue
a STOP condition, could the target still be expecting to send more data and
drive SDA low, preventing the STOP condition from being generated?

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

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

[Vincent Jardin]

Hi again bot,

Le 25/05/26 13:33, sashiko-bot@kernel.org a écrit :
> [Severity: High]
> This isn't a bug introduced by this patch, but does the error path for an
> oversized length need to set the TXAK bit before aborting the transaction?
> 
> Because reading I2DR automatically initiates the reception of the next byte
> with ACK enabled, returning early without setting I2CR_TXAK causes the
> controller to ACK the in-flight byte.
> 
> If the target device drives SDA low, could the subsequent cleanup via
> i2c_imx_stop() fail to emit a STOP condition and leave the bus permanently
> locked, similar to the zero-length case this patch addresses?

Yes, same as the former email on 1/2: the len > I2C_SMBUS_BLOCK_MAX path
sets STATE_FAILED and returns without a NACK+STOP, while reading I2DR has
already started the next byte.

I'll fix it in v2 by NACKing the in-flight byte and letting the normal
last-byte STOP sequence run before failing, matching the count=0 handling.

> 
> > +
> > +	if (len == 0) {
> > +		/*
> > +		 * SMBus 3.1 6.5.7 "Block Write/Read": byte count can be 0
> > +		 */
> > +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > +		temp |= I2CR_TXAK;
> > +		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> 
> [Severity: High]
> This is a pre-existing issue, but is there a potential read-modify-write
> race condition on the IMX_I2C_I2CR register here?
> 
> If an I2C transaction times out, the process context calls i2c_imx_stop(),
> which locklessly clears the I2CR_MSTA bit.
> 
> If the hardware interrupt fires concurrently and the ISR reads the register
> before i2c_imx_stop() clears MSTA, then writes it back afterward, could the
> ISR overwrite the register with MSTA=1? Would this transition from 0 back to
> 1 cause the controller to erroneously generate a new START condition?

The count=0 branch uses the same read/modify/write of I2CR than every other
ISR path in this driver already does. If there is a real ISR-vs-i2c_imx_stop()
race on MSTA it should be fixed across the driver in its own patch; it is out of
scope here and it is not introduced by this code.

> 
> > +
> > +		i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = 0;
> > +		i2c_imx->msg->len = 2;
> > +		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 the master fail to NACK the final data
> byte when performing an SMBus block read with exactly 1 byte of data?
> 
> Reading the length byte from I2DR triggers the reception of the single data
> byte, but the code does not appear to set I2CR_TXAK when the length is exactly
> 1.
> 
> During the next interrupt, when i2c_imx_isr_read_continue() attempts to issue
> a STOP condition, could the target still be expecting to send more data and
> drive SDA low, preventing the STOP condition from being generated?

This looks like a genuine pre-existing bug, but it is independent of the
block-read-of-0 fix:
for count == 1, msg->len becomes 2 and i2c_imx_isr_read_continue() enters
at msg_buf_idx == len - 1, taking the STOP branch and skipping the
(len - 2) TXAK branch,
so the single data byte is ACK'd rather than NACK'd.

It is a topic for another thread/serie.

A v2 will be sent.

thanks,
  Vincent

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

[PATCH v2 0/2] i2c: imx: fix SMBus block-read of 0 locking the bus

[Vincent Jardin]

i2c-imx rejects a SMBus Block Read byte count of 0 (valid per SMBus 3.1
6.5.7) and it returns without a NACK+STOP, leaving the target
holding SDA so the bus is stuck until a power cycle occur.

The same bug is occuring with two independently introduced spots, so the
fix is two patches with their respective Fixes: tags and backport ranges:

  1/2  atomic/polling path       Fixes: 8e8782c71595   v3.16+
  2/2  IRQ-driven state machine  Fixes: 5f5c2d4579ca   v6.13+

Signed-off-by: Vincent Jardin <vjardin@free.fr>
---
Changes in v2:
- Handle when count > I2C_SMBUS_BLOCK_MAX the same way as count == 0
  Reported by the Sashiko AI review on v1.

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

 drivers/i2c/busses/i2c-imx.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)
---
base-commit: 6916d5703ddf9a38f1f6c2cc793381a24ee914c6
change-id: 20260525-for-upstream-i2c-lx2160-fix-v1-0cba0a0093e5

Best regards,
-- 
Vincent Jardin <vjardin@free.fr>

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

[Vincent Jardin]

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.

Fixes: 8e8782c71595 ("i2c: imx: add SMBus block read support")
Cc: <stable@vger.kernel.org> # v3.16+
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 a208fefd3c3b..14107e1ad413 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);
 	if (result)
@@ -1436,8 +1437,20 @@ static int i2c_imx_atomic_read(struct imx_i2c_struct *i2c_imx,
 		 */
 		if ((!i) && block_data) {
 			len = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
-			if ((len == 0) || (len > I2C_SMBUS_BLOCK_MAX))
-				return -EPROTO;
+			if ((len == 0) || (len > I2C_SMBUS_BLOCK_MAX)) {
+				/*
+				 * SMBus 3.1 6.5.7: support count byte of 0.
+				 * I2C_SMBUS_BLOCK_MAX case should not hold the SDA either.
+				 */
+				if (len > I2C_SMBUS_BLOCK_MAX)
+					block_err = -EPROTO;
+				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);
@@ -1485,7 +1498,7 @@ static int i2c_imx_atomic_read(struct imx_i2c_struct *i2c_imx,
 			"<%s> read byte: B%d=0x%X\n",
 			__func__, i, msgs->buf[i]);
 	}
-	return 0;
+	return block_err;
 }
 
 static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,

-- 
2.43.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The atomic read setup function `i2c_imx_prepare_read` fails to clear the TXAK (NACK) flag for SMBus block reads, causing the controller to incorrectly NACK the initial length byte.
--

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

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

This patch fixes a locked bus issue during SMBus block reads of 0 in the
atomic polling path by NACKing the in-flight dummy byte and extending the
message length to correctly emit a STOP. It also handles the case where
the byte count exceeds I2C_SMBUS_BLOCK_MAX.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index a208fefd3c3b3..14107e1ad4138 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 isn't a bug introduced by this patch, but does the atomic read setup
fail to clear the TXAK flag for SMBus block reads here?

When i2c_imx_prepare_read() is called for SMBus block reads 
(I2C_M_RECV_LEN), the I2C core initially sets msgs->len to 1. In 
i2c_imx_prepare_read(), we have:

drivers/i2c/busses/i2c-imx.c:i2c_imx_prepare_read() {
    ...
    /*
     * Reset the I2CR_TXAK flag initially for SMBus block read since the
     * length is unknown
     */
    if (msgs->len - 1)
        temp &= ~I2CR_TXAK;
    ...
}

Because msgs->len is 1, msgs->len - 1 evaluates to 0, and I2CR_TXAK is not
cleared. If prior reads left TXAK set, the master will immediately NACK the
length byte. A NACK on the length byte signals the target to abort the
transfer, so no subsequent data bytes are transmitted.

A similar issue was previously identified and fixed in the interrupt-driven
path by explicitly checking the I2C_M_RECV_LEN flag. Could
i2c_imx_prepare_read() be missing the same check?

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

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

[Vincent Jardin]

Hi bot,

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

> [Severity: High]
> This isn't a bug introduced by this patch, but does the atomic read setup
> fail to clear the TXAK flag for SMBus block reads here?
> 
> When i2c_imx_prepare_read() is called for SMBus block reads 
> (I2C_M_RECV_LEN), the I2C core initially sets msgs->len to 1. In 
> i2c_imx_prepare_read(), we have:
> 
> drivers/i2c/busses/i2c-imx.c:i2c_imx_prepare_read() {
>     ...
>     /*
>      * Reset the I2CR_TXAK flag initially for SMBus block read since the
>      * length is unknown
>      */
>     if (msgs->len - 1)
>         temp &= ~I2CR_TXAK;
>     ...
> }
> 
> Because msgs->len is 1, msgs->len - 1 evaluates to 0, and I2CR_TXAK is not
> cleared. If prior reads left TXAK set, the master will immediately NACK the
> length byte. A NACK on the length byte signals the target to abort the
> transfer, so no subsequent data bytes are transmitted.
> 
> A similar issue was previously identified and fixed in the interrupt-driven
> path by explicitly checking the I2C_M_RECV_LEN flag. Could
> i2c_imx_prepare_read() be missing the same check?

See,
  https://lore.kernel.org/all/ahRmjr9cxYmkYvm5@L30177.local/

it was already explained: this serie fixes the block read of 0 locking of the
i2c, and this v2 just add the symmetric for count > I2C_SMBUS_BLOCK_MAX case.

For this topic and the analysis see the former email that replied about the same
topic on the v1.

regards,
  Vincent

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

[Vincent Jardin]

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.

Accept count=0: NACK the in-flight dummy byte (TXAK) and set msg->len to
2 so i2c_imx_isr_read_continue() emits STOP via its normal last-byte
path. The dummy byte is discarded; block-read callers only consume
buf[0..count-1].

Reading I2DR has likewise already armed the next byte on the
count > I2C_SMBUS_BLOCK_MAX error path, so NACK it (TXAK) before aborting
with -EPROTO; otherwise the failing transfer's STOP cannot complete and
the bus stays held.

The atomic path regressed earlier (v3.16) and is fixed separately; this
patch covers only the v6.13 state-machine rework.

Fixes: 5f5c2d4579ca ("i2c: imx: prevent rescheduling in non dma mode")
Cc: <stable@vger.kernel.org> # v6.13+
Signed-off-by: Vincent Jardin <vjardin@free.fr>
---
 drivers/i2c/busses/i2c-imx.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 14107e1ad413..8db8d2e10f5c 100644
--- 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) {
+		/*
+		 * SMBus 3.1 6.5.7: support count byte of 0.
+		 * I2C_SMBUS_BLOCK_MAX case should not hold the SDA either.
+		 * So NACK it (TXAK) to not hold the bus.
+		 */
+		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+		temp |= I2CR_TXAK;
+		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
+		if (len == 0) {
+			i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = 0;
+			i2c_imx->msg->len = 2;
+			return;
+		}
+
 		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;

-- 
2.43.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver fails to set the NACK flag (`TXAK`) when an SMBus block read returns a length of 1, which can cause the I2C bus to hang, similar to the bug fixed by this patch for length 0.
--

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

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

This commit updates the interrupt-driven SMBus block read state machine to
support a length of 0 bytes, setting the NACK bit for the in-flight dummy
byte to avoid bus hangs. It also safely aborts reads larger than the maximum
allowed size by setting the NACK bit before returning an error.

Cc: <stable@vger.kernel.org> # v6.13+

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 14107e1ad4138..8db8d2e10f5c5 100644
> --- 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 function also need to set the
TXAK bit when len is exactly 1?

When the length byte is read here, the hardware immediately starts clocking
in the next byte, which is the first data byte. If the block read length is
exactly 1, this first data byte is also the last byte of the transfer.

Because TXAK is explicitly cleared earlier in i2c_imx_isr_read for length
transfers, the controller will ACK this final data byte if TXAK is not set
at this point.

If the slave holds SDA low as a result of receiving an ACK instead of a NACK
on its final byte, would the subsequent attempt to generate a STOP condition
in i2c_imx_isr_read_continue fail and permanently hang the bus?

The atomic path in i2c_imx_atomic_read appears to handle this safely by
setting TXAK when reading the second-to-last byte.

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

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

[Vincent Jardin]

Hi bot,


> i2c: imx: fix locked bus on SMBus block-read of 0 (IRQ)
> [Severity: High]
> This is a pre-existing issue, but does this function also need to set the
> TXAK bit when len is exactly 1?
> 
> When the length byte is read here, the hardware immediately starts clocking
> in the next byte, which is the first data byte. If the block read length is
> exactly 1, this first data byte is also the last byte of the transfer.
> 
> Because TXAK is explicitly cleared earlier in i2c_imx_isr_read for length
> transfers, the controller will ACK this final data byte if TXAK is not set
> at this point.
> 
> If the slave holds SDA low as a result of receiving an ACK instead of a NACK
> on its final byte, would the subsequent attempt to generate a STOP condition
> in i2c_imx_isr_read_continue fail and permanently hang the bus?
> 
> The atomic path in i2c_imx_atomic_read appears to handle this safely by
> setting TXAK when reading the second-to-last byte.

once again, see the answers on v1:
  https://lore.kernel.org/all/ahRoKMLtrirbowbj@L30177.local/

regards,
  Vincent

Re: [PATCH v2 0/2] i2c: imx: fix SMBus block-read of 0 locking the bus

[Vincent Jardin]

Hi Carlos,

> Thanks for working on this fix, this looks good to me.

thanks for checking it. It took some times to isolate this issue and
then find a fix.

> SMBus block reads with a length of 0 seem quite uncommon in practice.
> Was this triggered by a specific device behavior, or mainly found
> during boundary / compliance testing?

It is trigger by the usage of a mpq8785 on the i2c bus: when the kernel attaches
on it using its pmsbus/hwmon framework, then the i2c bus get locked on lx2160 !

> Regarding the handling of len == 0,
> I see that the patch sets:
> 
>     msg->buf[0] = 0;
>     msg->len = 2;
> 
> It relies on the last-byte STOP handling together with TXAK. It will help I2C-IMX generate NACK + STOP and
> release the bus, right?

Yes, exactly. Reading I2DR for the length byte has already armed the
next byte, so we set TXAK to NACK it and extend msg->len to 2.
Next then i2c_imx_isr_read_continue() at msg_buf_idx == msg->len - 1,
ie the normal last-byte path, which clears MSTA to emit STOP. So NACK + STOP,
and THEN the bus is released. I do not see any other means to handle it.

> len = 0 is a legal behavior, So it go into a successful path.

Yes. count == 0 is legal (SMBus 3.1 6.5.7), so the transfer reaches
STATE_DONE and returns success.

> But len > I2C_SMBUS_BLOCK_MAX is abnormal behavior. So it go into a fail path.

Correct, and it is a protocol error, so it needs to end up with a -EPROTO while
a count of 0 is an ok case.

> Do I understand it right?

yes. I do not see any other means to handle it.

> Also, if possible could you briefly describe how you validated this change
> (e.g. test setup or steps, with and without the fix)?

On a lx2160a board, on its i2c, bind a mpq8785, and enable the Kernel pmbus/hwmon
framework, then the i2c bus becomes un-useable. Using a scope, we can confirm that
the lx21260a i2c cannot recover.

Best regards,
  Vincent 

RE: [PATCH v2 0/2] i2c: imx: fix SMBus block-read of 0 locking the bus

[Carlos Song (OSS)]

> -----Original Message-----
> From: Vincent Jardin <vjardin@free.fr>
> Sent: Tuesday, May 26, 2026 4:12 PM
> To: Carlos Song (OSS) <carlos.song@oss.nxp.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Andi Shyti <andi.shyti@kernel.org>; Frank Li
> <frank.li@nxp.com>; Sascha Hauer <s.hauer@pengutronix.de>; Fabio
> Estevam <festevam@gmail.com>; Wolfram Sang <wsa@kernel.org>; Kaushal
> Butala <kaushalkernelmailinglist@gmail.com>; Shawn Guo
> <shawn.guo@freescale.com>; Stefan Eichenberger
> <stefan.eichenberger@toradex.com>; linux-i2c@vger.kernel.org;
> imx@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: Re: [PATCH v2 0/2] i2c: imx: fix SMBus block-read of 0 locking the bus
> 
> [You don't often get email from vjardin@free.fr. Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hi Carlos,
> 
> > Thanks for working on this fix, this looks good to me.
> 
> thanks for checking it. It took some times to isolate this issue and then find a
> fix.
> 
> > SMBus block reads with a length of 0 seem quite uncommon in practice.
> > Was this triggered by a specific device behavior, or mainly found
> > during boundary / compliance testing?
> 
> It is trigger by the usage of a mpq8785 on the i2c bus: when the kernel
> attaches on it using its pmsbus/hwmon framework, then the i2c bus get
> locked on lx2160 !
> 
> > Regarding the handling of len == 0,
> > I see that the patch sets:
> >
> >     msg->buf[0] = 0;
> >     msg->len = 2;
> >
> > It relies on the last-byte STOP handling together with TXAK. It will
> > help I2C-IMX generate NACK + STOP and release the bus, right?
> 
> Yes, exactly. Reading I2DR for the length byte has already armed the next byte,
> so we set TXAK to NACK it and extend msg->len to 2.
> Next then i2c_imx_isr_read_continue() at msg_buf_idx == msg->len - 1, ie the
> normal last-byte path, which clears MSTA to emit STOP. So NACK + STOP, and
> THEN the bus is released. I do not see any other means to handle it.
> 
> > len = 0 is a legal behavior, So it go into a successful path.
> 
> Yes. count == 0 is legal (SMBus 3.1 6.5.7), so the transfer reaches
> STATE_DONE and returns success.
> 
> > But len > I2C_SMBUS_BLOCK_MAX is abnormal behavior. So it go into a fail
> path.
> 
> Correct, and it is a protocol error, so it needs to end up with a -EPROTO while
> a count of 0 is an ok case.
> 
> > Do I understand it right?
> 
> yes. I do not see any other means to handle it.
> 
> > Also, if possible could you briefly describe how you validated this
> > change (e.g. test setup or steps, with and without the fix)?
> 
> On a lx2160a board, on its i2c, bind a mpq8785, and enable the Kernel
> pmbus/hwmon framework, then the i2c bus becomes un-useable. Using a
> scope, we can confirm that the lx21260a i2c cannot recover.
> 
Hi, Vincent

Thank you very much!

Acked-by: Carlos Song <carlos.song@nxp.com>

> Best regards,
>   Vincent