Re: Potential bug in SMBus kernel module

[Jean Delvare <jdelvare@suse.de>]

Hi Piotr,

Thanks for your bug report.

On Mon, 12 Feb 2024 12:44:35 +0000, Zakowski, Piotr wrote:
> 4. Observed bug
> 
> In the "Block Write-Block Read Process Call", the FIFO index should be cleared twice.
> Testing of the Linux driver for the above scenario has shown that it only clears once during the write part (writing data to the FIFO buffer) and does not clear before the read part (reading data from the FIFO buffer).
> As a result, the command only returns a portion of the data (N-M) and leaves behind residual data from previous SMBus commands that used the FIFO buffer.

Support for the Block Write-Block Read Process Call command was added
to the i2c-i801 driver by Alexander Sverdlin in June 2019. I'm adding
him to Cc, but he changed addresses meanwhile, so I hope I got the
right Alexander.

The i2c-i801 driver received a lot of changes meanwhile, however as far
as I can see this implementation detail did not change, the data buffer
index is still only reset once at the beginning of
i801_block_transaction_by_block(), as it was back then.

Alexander, back when you contributed the code, you said the new command
was long-time tested, so it's hard to believe it includes the bug
reported by Piotr. Do you remember which Intel chipset you were using?
Is the code you submitted exactly what you were using on the hardware,
or is there a chance that you forgot one change when preparing the
upstream submission?

Piotr, which Intel chipset have you been testing? Is there a chance
that different Intel chipsets may behave differently in this respect?
Out of curiosity, how are you testing the Block Write-Block Read
Process Call command? Very few target devices support this command.

I looked at the ICH9 datasheet (Intel document number 316972-002) and
in the description of the "Block Write–Block Read Process Call" command
protocol (section 5.20.1.1, page 217) it is clearly stated:

  "Software must do a read to the command register (offset 2h) to reset
  the 32 byte buffer pointer prior to reading the block data register."

So I guess the driver should indeed be doing that. But I'd really like
to understand how this went unnoticed so far.

-- 
Jean Delvare
SUSE L3 Support