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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 03.12.2021 13:55, Heiner Kallweit wrote:
> 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.

I mixed something up, START is needed only once.

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