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

[Vincent Jardin <vjardin@free.fr>]

Hi Carlos,

> Thanks for working on this fix, this looks good to me.

thanks for checking it. It took some times to isolate this issue and
then find a fix.

> SMBus block reads with a length of 0 seem quite uncommon in practice.
> Was this triggered by a specific device behavior, or mainly found
> during boundary / compliance testing?

It is trigger by the usage of a mpq8785 on the i2c bus: when the kernel attaches
on it using its pmsbus/hwmon framework, then the i2c bus get locked on lx2160 !

> Regarding the handling of len == 0,
> I see that the patch sets:
> 
>     msg->buf[0] = 0;
>     msg->len = 2;
> 
> It relies on the last-byte STOP handling together with TXAK. It will help I2C-IMX generate NACK + STOP and
> release the bus, right?

Yes, exactly. Reading I2DR for the length byte has already armed the
next byte, so we set TXAK to NACK it and extend msg->len to 2.
Next then i2c_imx_isr_read_continue() at msg_buf_idx == msg->len - 1,
ie the normal last-byte path, which clears MSTA to emit STOP. So NACK + STOP,
and THEN the bus is released. I do not see any other means to handle it.

> len = 0 is a legal behavior, So it go into a successful path.

Yes. count == 0 is legal (SMBus 3.1 6.5.7), so the transfer reaches
STATE_DONE and returns success.

> But len > I2C_SMBUS_BLOCK_MAX is abnormal behavior. So it go into a fail path.

Correct, and it is a protocol error, so it needs to end up with a -EPROTO while
a count of 0 is an ok case.

> Do I understand it right?

yes. I do not see any other means to handle it.

> Also, if possible could you briefly describe how you validated this change
> (e.g. test setup or steps, with and without the fix)?

On a lx2160a board, on its i2c, bind a mpq8785, and enable the Kernel pmbus/hwmon
framework, then the i2c bus becomes un-useable. Using a scope, we can confirm that
the lx21260a i2c cannot recover.

Best regards,
  Vincent