Re: [PATCH 1/2] i2c: imx-lpi2c: fix SMBus block read with PEC

[Carlos Song <carlos.song@oss.nxp.com>]

> If PEC is enabled, the I2C core sets msgs->len to 2 instead of 1. The
> length calculation for the second receive command assumes it's always
> 1. This will lead to a short read and eventually a timeout because the
> PEC byte is never received.
> 
> Take msgs->len into account when calculating the remaining bytes.
> 
> This was tested on a board which uses an IPMI SSIF BMC.
> 
> Fixes: efdc383d1cc2 ("i2c: imx-lpi2c: fix SMBus block read NACK after byte count")
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 7302464c371a..f57e800ac3b5 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -563,7 +563,7 @@ static unsigned int lpi2c_SMBus_block_read_length_byte(struct lpi2c_imx_struct *
>  static int lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
>  			       struct i2c_msg *msgs)
>  {
> -	unsigned int temp, val, block_len;
> +	unsigned int temp, val, block_len, remaining;
>  	int ret;
>  
>  	lpi2c_imx->rx_buf = msgs->buf;
> @@ -612,9 +612,13 @@ static int lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
>  		 * keep MTDR non-empty and ensuring continuous ACKs. Only update command register
>  		 * here. All block bytes will be read out at IRQ handler or lpi2c_imx_read_atomic()
>  		 * function.
> +		 *
> +		 * msgs->len is 1 - or if PEC enabled msgs->len is 2. Two bytes
> +		 * have already been requested.
>  		 */
> -		if (block_len > 1)
> -			writel((RECV_DATA << 8) | (block_len - 2), lpi2c_imx->base + LPI2C_MTDR);
> +		remaining = msgs->len + block_len - 2;
> +		if (remaining)
> +			writel((RECV_DATA << 8) | (remaining - 1), lpi2c_imx->base + LPI2C_MTDR);

Hi Michael,

Just to confirm my understanding:

For SMBus block read with PEC enabled, the PEC (CRC-8) byte is appended
at the end of the transfer. In that case the I2C core sets msgs->len to
2 instead of 1.

The previous code still calculated the remaining receive count assuming
msgs->len == 1, which means the PEC byte was not included in the total
number of bytes requested from the controller.

As a result, the last PEC byte was never read out, leading to a short
read and eventually a timeout.

Is that the issue this patch is fixing?

>  
>  		lpi2c_imx->msglen += block_len;
>  		msgs->len += block_len;

-- 
Best Regards,
Carlos