| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aWEIQ2X9nWbuJpeH@pengutronix.de>
Date: Fri, 9 Jan 2026 14:53:07 +0100
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: LI Qingwu <Qing-wu.Li@...ca-geosystems.com.cn>
Cc: kernel@...gutronix.de, andi.shyti@...nel.org, shawnguo@...nel.org,
s.hauer@...gutronix.de, festevam@...il.com,
linux-i2c@...r.kernel.org, imx@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
bsp-development.geo@...ca-geosystems.com
Subject: Re: [PATCH V1] i2c: imx: Fix SMBus block read hang on zero length
Hi,
On Mon, Dec 29, 2025 at 08:16:29AM +0000, LI Qingwu wrote:
> SMBus block read transfers encode the payload length in the first data
> byte. When this first byte is zero, there is no payload and the
> transaction should terminate immediately.
>
> On i.MX, if the first byte of an SMBus block read is zero, the driver
> unconditionally overwrites the state with IMX_I2C_STATE_READ_CONTINUE.
> This causes the state machine to enter an endless read loop, eventually
> overrunning internal buffers and leading to a crash.
>
> At the same time, the controller remains in master receive mode and
> never generates a proper STOP condition, leaving the I2C bus permanently
> busy and preventing any further transfers on the bus.
>
> Fix this by handling the zero-length case explicitly: when the first
> byte is zero, ensure that a clean STOP is generated. In this situation
> the controller is in master receive mode, so it must be switched to
> master transmit mode before stopping. This is done by draining the
> pending received byte from I2DR, setting I2CR_MTX to enter transmit
> mode, waiting briefly for the mode change, and then proceeding with the
> normal STOP sequence.
>
> This change has been tested on i.MX 8M Plus platform.
>
> Signed-off-by: LI Qingwu <Qing-wu.Li@...ca-geosystems.com.cn>
Sorry for delay and thank you for your work.
> ---
> 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 dcce882f3eba..f40deecf0f66 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -735,6 +735,16 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx, bool atomic)
> temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> if (!(temp & I2CR_MSTA))
> i2c_imx->stopped = 1;
I would love to have more comments, so I'll add some. Otherwise it is hard
to recall everything needed with my cold cache :)
/*
* Condition: We are in Master Mode (MSTA=1) AND Receive Mode (MTX=0).
*
* Ref: IMX8MPRM Rev. 1, 06/2021: 17.1.6.3 I2Cx_I2CR:
* - Bit 5 (MSTA): 1 = Master Mode.
* - Bit 4 (MTX): 0 = Receive.
*/
> + if ((temp & I2CR_MSTA) && !(temp & I2CR_MTX)) {
/*
* Dummy read of I2C Data Register (I2DR).
*
* Ref: IMX8MPRM Rev. 1, 06/2021: 17.1.6.5 I2C Data I/O Register (I2Cx_I2DR):
* "Reading the data register... initiates the next byte to be received."
*
* Ref: IMX8MPRM Rev. 1, 06/2021: 17.1.6.4 I2C Status Register (I2Cx_I2SR) -> ICF (Bit 7):
* "The data transferring bit (ICF) is cleared... by reading from I2C_I2DR in Receive mode."
*
* This dummy read is essential to clear the 'Transfer Complete' (ICF)
* flag and release the SCL line if the hardware
* was stretching the clock waiting for a read.
*/
> + (void)imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
/*
* Force the controller into Master Transmit Mode (MTX=1).
*
* Ref: IMX8MPRM Rev. 1, 06/2021: 17.1.6.3 I2Cx_I2CR -> MTX (Bit 4):
* "1 = Transmit"
*
* We cannot safely STOP while waiting for data (RX). By switching to TX,
* the Master asserts control over SDA to generate the STOP condition
* without the ambiguity of an expected incoming byte.
*/
> + temp |= I2CR_MTX;
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
/*
* Wait for the mode switch to settle.
*
* Ref: IMX8MPRM Rev. 1, 06/2021: 17.1.6.4 Note on Timeout:
* "The minimum timeout... is 25 us".
*
* The state machine needs time to latch the new mode (TX) before
* we immediately command it to STOP. 25us is the documented safe lower bound
* for I2C bus event processing at 400kHz.
* T_min = 10/F_SCL
* T_min = 10 / 400000 Hz = 0,00002 Sec
*/
I would recommend here to calculate delay based on current clock,
probably i2c_imx->cur_clk is the right variable.
May be i2c_imx->disable_delay should be used? Cirrently it is used only
for imx1.
I'm still not sure about impact of this delay withing this context.
> + if (atomic)
> + udelay(25);
> + else
> + usleep_range(25, 50);
/*
* Read back the control register to ensure the write persisted and
* to have the freshest state for the final STOP command?
*/
> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + }
> temp &= ~(I2CR_MSTA | I2CR_MTX);
> if (i2c_imx->dma)
> temp &= ~I2CR_DMAEN;
> @@ -1103,7 +1113,8 @@ static irqreturn_t i2c_imx_master_isr(struct imx_i2c_struct *i2c_imx, unsigned i
>
> case IMX_I2C_STATE_READ_BLOCK_DATA_LEN:
> i2c_imx_isr_read_block_data_len(i2c_imx);
> - i2c_imx->state = IMX_I2C_STATE_READ_CONTINUE;
I ques, this part can go as separate directly upstream to the stable.
> + if (i2c_imx->state == IMX_I2C_STATE_READ_BLOCK_DATA_LEN)
> + i2c_imx->state = IMX_I2C_STATE_READ_CONTINUE;
> break;
>
> case IMX_I2C_STATE_WRITE:
> --
> 2.43.0
>
>
Best Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Powered by blists - more mailing lists