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

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

[Vincent Jardin]

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

It was triggered by an MPQ8785 PMBus regulator on a LX2160A i2c
bus: when the kernel binds it using the pmbus/hwmon framework, the bus
locks up and it does never recovers. It was confirmed with a scope, with
and without the fix.

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 v3:
- no functional change; collected the review tags received on v2:
  Acked-by Oleksij Rempel, Acked-by Carlos Song, Reviewed-by Stefan
  Eichenberger (both patches)
- cover letter: add the real-world trigger (MPQ8785 PMBus regulator
  on the LX2160A) and how the fix was validated, asked by Carlos Song
- resend as a new thread, per Andi Shyti's request
- Link to v2: https://lore.kernel.org/r/20260525-for-upstream-i2c-lx2160-fix-v1-v2-0-26a3cc8cd055@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: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
change-id: 20260525-for-upstream-i2c-lx2160-fix-v1-0cba0a0093e5

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

[PATCH v3 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+
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 28313d0fad37..cfd1e63359e7 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 v3 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] `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

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

[Vincent Jardin]

Hi Bot,

Le 13/07/26 18:26, sashiko-bot@kernel.org a écrit :
> - [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.

Both topics were already raised on v1 and v2 and answered. No change with v3,
so no need to reopend those threads.

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

See 2/2 of this serie.

Same for v1: https://lore.kernel.org/all/ahRoKMLtrirbowbj@L30177.local/
and v2: https://lore.kernel.org/all/ahVZPLHymdsZtgg5@L30177.local/

> [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;
 [...}
> 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?

Already explained with v1:
  https://lore.kernel.org/all/ahRmjr9cxYmkYvm5@L30177.local/

Best regards,
  Vincent

[PATCH v3 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+
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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index cfd1e63359e7..d5e6e2eca3b3 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 v3 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] 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

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

[Vincent Jardin]

Hi Bot,

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

You have a point, but it is a pre-existing side issue that is not the focus of this serie.

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

It seems to be an OK argument.

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

it was pre-existing. It is another issue to be checked, but I'd like to avoid
widening the scope of this serie since it was already widely reviewed.

I'll check for another specific patch for this case as a follow up. First, let's
finalize this serie.

Thanks,
  Vincent

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

[Andi Shyti]

Hi Vincent,

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

merged to i2c/i2c-fixes.

Thanks,
Andi

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

[Vincent Jardin]

Hi Andi,

> merged to i2c/i2c-fixes.

big thanks

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

[Wolfram Sang]

On Mon, Jul 13, 2026 at 08:11:58PM +0200, Vincent Jardin wrote:
> i2c-imx rejects an SMBus Block Read byte count of 0 (valid per SMBus 3.1
> 6.5.7) as -EPROTO and returns without emitting a NACK + STOP, leaving the
> target holding SDA so the bus stays stuck until a power cycle.

Bigger picture: Linux does not support SMBus3 which also allows byte
counts of up to 255. I started sketching support for all that but could
never implement it.

That being said, despite no SMBus3 support, it should not hang the bus
like here.