Re: [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Heiner Kallweit <hkallweit1@gmail.com>]

On 28.08.2023 15:27, Jean Delvare wrote:
> Hi Heiner,
> 
> On Sun, 27 Aug 2023 19:14:38 +0200, Heiner Kallweit wrote:
>> On 27.06.2023 15:46, Jean Delvare wrote:
>>> Hi Heiner, Andi,
>>>
>>> On Sat, 04 Mar 2023 22:36:34 +0100, Heiner Kallweit wrote:  
>>>> Here we don't have to write SMBHSTCNT in each iteration of the loop.
>>>> Bit SMBHSTCNT_START is internally cleared immediately, therefore
>>>> we don't have to touch the value of SMBHSTCNT until the last byte.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/i2c/busses/i2c-i801.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>>>> index 7641bd0ac..e1350a8cc 100644
>>>> --- a/drivers/i2c/busses/i2c-i801.c
>>>> +++ b/drivers/i2c/busses/i2c-i801.c
>>>> @@ -677,11 +677,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>>>  	for (i = 1; i <= len; i++) {
>>>>  		if (i == len && read_write == I2C_SMBUS_READ)
>>>>  			smbcmd |= SMBHSTCNT_LAST_BYTE;
>>>> -		outb_p(smbcmd, SMBHSTCNT(priv));
>>>>  
>>>>  		if (i == 1)
>>>> -			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
>>>> -			       SMBHSTCNT(priv));
>>>> +			outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
>>>> +		else if (smbcmd & SMBHSTCNT_LAST_BYTE)
>>>> +			outb_p(smbcmd, SMBHSTCNT(priv));
>>>>  
>>>>  		status = i801_wait_byte_done(priv);
>>>>  		if (status)  
>>>
>>> I tested this and it works, but I don't understand how.
>>>
>>> I thought that writing to SMBHSTCNT was what was telling the host
>>> controller to proceed with the next byte. If writing to SMBHSTCNT for
>>> each byte isn't needed, then what causes the next byte to be processed?
>>> Does this happen as soon as SMBHSTSTS_BYTE_DONE is written? If so, then
>>> what guarantees that we set SMBHSTCNT_LAST_BYTE *before* the last byte
>>> is actually processed?
>>
>> It's my understanding that writing SMBHSTSTS_BYTE_DONE tells the host to
>> continue with the next byte.
> 
> That's indeed possible, and quite likely, considering that your patch
> works.
> 

This understanding is backed by the following from Byte Done Status
description in (at least) ICH9 specification:
When not using the 32 Byte Buffer, hardware will drive the SMBCLK signal
low when the DS bit is set until SW clears the bit.

>> We set SMBHSTCNT_LAST_BYTE whilst the host is receiving the last byte.
>> Apparently the host checks for SMBHSTCNT_LAST_BYTE once it received
>> a byte, in order to determine whether to ack the byte or not.
>> So SMBHSTCNT_LAST_BYTE doesn't have to be set before the host starts
>> receiving the last byte.
> 
> How is this not racy?
> 
> In the interrupt-driven case, at the end of a block read transaction,
> we set SMBHSTCNT_LAST_BYTE at the end of i801_isr_byte_done(), then
> return to i801_isr() where we write 1 to SMBHSTSTS_BYTE_DONE to clear
> it. This lets the controller handle the last byte with the knowledge
> that this is the last byte.
> 
> However, in the poll-driven case, SMBHSTSTS_BYTE_DONE is being cleared
> at the end of the loop in i801_block_transaction_byte_by_byte(), then
> at the beginning of the next iteration, we write SMBHSTCNT_LAST_BYTE,
> then wait for completion. If the controller is super fast (or, to be
> more realistic, the i2c-i801 driver gets preempted between writing
> SMBHSTSTS_BYTE_DONE and writing SMBHSTCNT_LAST_BYTE) then the byte may
> have been already read and acked, before we have the time to let the
> controller know that no ACK should be sent. This looks racy. Am I
> missing something?
> 
> If nothing else, the fact that the order is different between the
> interrupt-driven and poll-driven cases is fishy.
> 
> I must add that the problem is not related to your patch, I just
> happened to notice it while reviewing your patch.
> 
>> For writes SMBHSTCNT_LAST_BYTE isn't used.
> 
> Agreed.
>