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
