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

[Jean Delvare <jdelvare@suse.de>]

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