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

[Vincent Jardin <vjardin@free.fr>]

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