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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 03.12.2021 10:59, Jean Delvare wrote:
> 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
> 

"My last attempt locked the SMBus, but I was able to
recover by repeatedly writing to the HST_STS register, as may times as
the block length."

OK, this was 11 yrs ago, so at least I wouldn't be able to recall in
detail what happened back then ..

Question is how you did this "repeatedly writing to the HST_STS
register". Something like the following?

while (status = in (STATUS))
	out(STATUS, status);

Or maybe the driver started the loop to process the next byte?
I think it's not likely that when writing a status bit it
remained set. As we now know E32B is ignored in I2C mode, therefore
the chip can read/write only one byte in a row, and w/o setting
SMBHSTCNT_START in between it wouldn't touch the next byte.
Of course I may be wrong with my assumptions ..


> 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?
> 

In a follow-up mail in the thread you mentioned is the following.
I noticed the same (the 1ms delay is too short) and have related patches
in my tree. However I'd like to finalize the cleanups first.

"While working on this issue, I noticed that the piece of code which is
supposed to let the i2c-i801 driver recover in case of a transaction
timeout, did not always work."