Re: [PATCH] i2c: i801: Don't clear status flags twice in interrupt mode

[Heiner Kallweit <hkallweit1@gmail.com>]

On 03.12.2021 11:41, Jean Delvare wrote:
> Hi Heiner,
> 
> On Thu, 02 Dec 2021 10:57:43 +0100, Heiner Kallweit wrote:
>> In interrupt mode we clear the status flags twice, in the interrupt
>> handler and in i801_check_post(). Remove clearing the status flags
>> from i801_check_post() and let i801_wait_intr() clear them in
>> polling mode. Another benefit is that now only checks for error
>> conditions are left in i801_check_post(), thus better matching the
>> function name.
>>
>> Note: There's a comment in i801_check_post() that i801_wait_intr()
>> clears the error status bits. Actually this hasn't been true until
>> this change.
> 
> While reviewing this, I have noticed other comments which seem outdated:
> 
> * In i801_isr(), "->status must be cleared before the next transaction
>   is started." We no longer do this since
>   1de93d5d521717cbb77cc9796a4df141d800d608, and things are working
>   well, so clearly this is no longer necessary and the comment could be
>   removed.
> * The comment before i801_check_post() is obsoleted by your
>   proposed change.
> 
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 7446bed78..82553e0cb 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -438,9 +438,6 @@ static int i801_check_post(struct i801_priv *priv, int status)
>>  		dev_dbg(&priv->pci_dev->dev, "Lost arbitration\n");
>>  	}
>>  
>> -	/* Clear status flags except BYTE_DONE, to be cleared by caller */
>> -	outb_p(status, SMBHSTSTS(priv));
>> -
>>  	return result;
>>  }
>>  
>> @@ -455,8 +452,10 @@ static int i801_wait_intr(struct i801_priv *priv)
>>  		status = inb_p(SMBHSTSTS(priv));
>>  		busy = status & SMBHSTSTS_HOST_BUSY;
>>  		status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
>> -		if (!busy && status)
>> +		if (!busy && status) {
>> +			outb_p(status, SMBHSTSTS(priv));
>>  			return status;
>> +		}
>>  	} while (time_is_after_eq_jiffies(timeout));
>>  
>>  	return -ETIMEDOUT;
> 
> I like the idea to make things consistent between the interrupt and
> poll modes, however I'm afraid there's one code path not covered by
> your patch. If i801_wait_byte_done() returns an error flag in
> i801_block_transaction_byte_by_byte(), we jump directly to
> i801_check_post() without calling i801_wait_intr(). In that case, the
> error flags are not cleared, right? Doesn't look good.
> 
Indeed, good point. I have an idea, I'll propose a patch that we can
use as discussion basis.

> Makes me wonder if we shouldn't actually let i801_check_post() clear
> the flags and remove that piece of code everywhere else? Or is there a
> reason why it has to be done earlier in i801_isr(), that I do not
> remember?
> 
I'd like to achieve that check_pre/post() only check and handle error
conditions. Both functions shouldn't be involved in the normal flow.

> I'm also wondering if there's actually any reason to clear the flags at
> all at the end of the transaction, considering that we'll be doing it
> again at the beginning of the next transaction anyway to be on the safe
> side.
> 
Clearing flags in check_pre() should just be a last resort (IMO).
The regular flow should leave the flags in a clean state.