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

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

[Heiner Kallweit]

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.

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;
-- 
2.33.1

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

[Jean Delvare]

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.

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'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.

-- 
Jean Delvare
SUSE L3 Support

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

[Heiner Kallweit]

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.