Re: [PATCH V1] i2c: imx: Fix SMBus block read hang on zero length

[Andi Shyti <andi.shyti@kernel.org>]

Hi,

...

> > > 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@leica-geosystems.com.cn>
> > 
> > Is this a fix?
> 
> Yes, this is a fix. Without this patch, zero-length SMBus block reads cause a system crash 
> and permanent bus lockup on i.MX 8M Plus. The fix ensures proper STOP generation and 
> prevents buffer overruns.

Then, please add the Fixes tag.

> > > ---
> > >  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;
> > > +             if ((temp & I2CR_MSTA) && !(temp & I2CR_MTX)) {
> > > +                     (void)imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> > 
> > why do we need a cast here?
> 
> The void cast explicitly marks this as a dummy read. We're reading I2DR solely to drain the
> receive buffer (a required side effect before mode switching), not because we need the value.
> This makes the intent clear to both the compiler and code reviewers.

Please drop it.

> > > +                     temp |= I2CR_MTX;
> > > +                     imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > > +                     if (atomic)
> > > +                             udelay(25);
> > 
> > where is this 25 coming from?
> 
> It's a conservative value derived from practical testing. at standard mode (100kHz),
> this equals ~2.5 I2C clock periods; at fast mode (400kHz), ~10 periods

Please add a comment.

Andi