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

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2631 bytes --]

Hi Carlos,

On Thu Nov 5, 2026 at 9:29 AM CET, Carlos Song wrote:
>
>
>> 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?


Yes exactly.

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]