Re: Potential bug in SMBus kernel module

Re: Potential bug in SMBus kernel module

[Jean Delvare]

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

Re: Potential bug in SMBus kernel module

[Alexander Sverdlin]

Hi Jean, thanks for including me!

On Tue, 2024-02-13 at 12:05 +0100, Jean Delvare 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.

While I'm trying to remember the details of the story ;-), could
you please point me to the full bug report? I suspect it has not actually
been posted to linux-i2c?

-- 
Alexander Sverdlin.

Re: Potential bug in SMBus kernel module

[Alexander Sverdlin]

Hi Jean,

On Tue, 2024-02-13 at 12:05 +0100, Jean Delvare wrote:
> 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?

from what I can tell, back than it was
"Intel® Xeon® D-1500 product family Integrated PCH Logic".

I'm not sure if it was WILDCATPOINT_SMBUS or LYNXPOINT_SMBUS,
it's nevertheless FEATURES_ICH5, means FEATURE_BLOCK_BUFFER.

Unfortunately, I cannot tell you the details of the tests, but the reason
is that I had to rely on a colleague for this, so I cannot explain
how this went unnoticed back than. Maybe the (incorrect) returned data
was irrelevant for his tests, I can only speculate.

I don't think there was a non-published downstream patch in play.

-- 
Alexander Sverdlin.