Re: [PATCH] i2c: i801: Don't read back cleared status in i801_check_pre()

[Jean Delvare <jdelvare@suse.de>]

Hi Heiner,

On Thu, 02 Dec 2021 10:53:05 +0100, Heiner Kallweit wrote:
> I see no need to read back the registers to verify that the bits
> have actually been cleared. I can't imagine any scenario where
> the bits would remain set after a write to them.

This happened at least once in the past. See this archived message:

https://www.spinics.net/lists/linux-i2c/msg02651.html

This was in i801_check_post(), not i801_check_pre(), but that was the
same code. Which was removed in
6cad93c4bbd62ecfa2e1b3a95c1ac4f6f27764c7 because there was little point
in checking the same condition twice.

Unfortunately it seems that the error messages were copied manually so
we lack the details of which status bit couldn't be cleared exactly.

Granted, it was caused by a driver bug, which was fixed since (commit
c074c39d62306efa5ba7c69c1a1531bc7333d252) but this shows that the
condition can actually trigger.

> Whilst at it, change involved syslog messages to use pci_dbg() et al.
> to simplify them.

Fine with me.

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c
> b/drivers/i2c/busses/i2c-i801.c index 720f7e9d0..a82aaef27 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -328,22 +328,14 @@ static int i801_check_pre(struct i801_priv
> *priv) 
>  	status = inb_p(SMBHSTSTS(priv));
>  	if (status & SMBHSTSTS_HOST_BUSY) {
> -		dev_err(&priv->pci_dev->dev, "SMBus is busy, can't use it!\n");
> +		pci_err(priv->pci_dev, "SMBus is busy, can't use it!\n");
>  		return -EBUSY;
>  	}
>  
>  	status &= STATUS_FLAGS;
>  	if (status) {
> -		dev_dbg(&priv->pci_dev->dev, "Clearing status flags (%02x)\n",
> -			status);
> +		pci_dbg(priv->pci_dev, "Clearing status flags (%02x)\n", status);
>  		outb_p(status, SMBHSTSTS(priv));
> -		status = inb_p(SMBHSTSTS(priv)) & STATUS_FLAGS;
> -		if (status) {
> -			dev_err(&priv->pci_dev->dev,
> -				"Failed clearing status flags (%02x)\n",
> -				status);
> -			return -EBUSY;
> -		}
>  	}
>  
>  	/*
> @@ -356,16 +348,8 @@ static int i801_check_pre(struct i801_priv *priv)
>  	if (priv->features & FEATURE_SMBUS_PEC) {
>  		status = inb_p(SMBAUXSTS(priv)) & SMBAUXSTS_CRCE;
>  		if (status) {
> -			dev_dbg(&priv->pci_dev->dev,
> -				"Clearing aux status flags (%02x)\n", status);
> +			pci_dbg(priv->pci_dev, "Clearing aux status flags (%02x)\n", status);
>  			outb_p(status, SMBAUXSTS(priv));
> -			status = inb_p(SMBAUXSTS(priv)) & SMBAUXSTS_CRCE;
> -			if (status) {
> -				dev_err(&priv->pci_dev->dev,
> -					"Failed clearing aux status flags (%02x)\n",
> -					status);
> -				return -EBUSY;
> -			}
>  		}
>  	}
>  

So I'm not too sure what to do with this. On the one hand, the code you
want to remove could be useful to catch and investigate future bugs.
The rest of the code does assume that the status bits are properly
cleared before starting a new transaction. On the other hand, it is
slowing down the driver a bit when all is fine. Is that really worth
optimizing?

-- 
Jean Delvare
SUSE L3 Support