Re: [PATCH V3 1/2] i2c: imx: preserve error state in block data length handler

[Stefan Eichenberger <eichest@gmail.com>]

Hi Andi and Li,

On Thu, Jan 22, 2026 at 11:48:54AM +0100, Andi Shyti wrote:
> Hi Li,
> 
> I'm adding also Stefan in the Cc list as he has authored the
> lines you are changing.
> 
> On Fri, Jan 16, 2026 at 11:19:05AM +0000, LI Qingwu wrote:
> > When a block read returns an invalid length, zero or >I2C_SMBUS_BLOCK_MAX,
> > the length handler sets the state to IMX_I2C_STATE_FAILED. However,
> > i2c_imx_master_isr() unconditionally overwrites this with
> > IMX_I2C_STATE_READ_CONTINUE, causing an endless read loop that overruns
> > buffers and crashes the system.
> > 
> > Guard the state transition to preserve error states set by the length
> > handler.
> > 
> > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> 
> I asked you to add the Fixes tag here. Perhaps you need to add:
> 
> Fixes: 5f5c2d4579ca ("i2c: imx: prevent rescheduling in non dma mode")
> Cc Stefan Eichenberger <stefan.eichenberger@toradex.com>
> Cc: <stable@vger.kernel.org> # v6.13+
> 
> You can get the information above with "git blame".
> 
> I'll wait for comments from Oleksij and/or Stefan here.
> 
> Andi
> 
> > ---
> >  drivers/i2c/busses/i2c-imx.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > index 205cc132fdec..05ba41144648 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > @@ -1102,7 +1102,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;
> > +		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
> > 

It looks good to me, thanks for the fix. I wonder if the functions in
the isr should instead better return a status and based on that we
decide if we have to change the state or not. Then we would have the
decision to what state we swtich at one place. However, this would
probably be too much rework for that fix.

Therefore, if you add the fixes tag suggested by Andi:
Reviewed-by: Stefan Eichenberger <eichest@gmail.com>