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

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

[Heiner Kallweit]

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.

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

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;
-			}
 		}
 	}
 
-- 
2.34.1

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

[Jean Delvare]

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

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

[Heiner Kallweit]

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

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

[Heiner Kallweit]

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

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

[Jean Delvare]

Hi Heiner,

On Fri, 3 Dec 2021 13:55:30 +0100, Heiner Kallweit wrote:
> On 03.12.2021 10:59, Jean Delvare wrote:
> > 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 ..

I definitely do not remember, so all the information available now is
what can be read in this archived thread.

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

Sorry for the confusion, Felix was the guy hitting the bug, and I was
helping him understand what was happening and how to fix it.

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

Most probably yes.

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

The important bit was probably SMBHSTSTS_BYTE_DONE. The driver set the
block buffer mode, expecting the hardware to support that, but the
hardware didn't and thus expected a byte-by-byte block transaction,
which requires ack-ing every byte by clearing SMBHSTSTS_BYTE_DONE.

> > (...)
> > 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?

As I got some time to think about it, answering myself: I'm fine
removing the checks. If we ever hit the problem (unable to clear the
error flags), it means that something went wrong _before_, and we must
have logged these problems already. As a matter of fact, that was
exactly the situation for Felix, the message you want to remove was the
4th error message logged, so in fact it did not really add any value.

-- 
Jean Delvare
SUSE L3 Support

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 455 bytes --]


> As I got some time to think about it, answering myself: I'm fine
> removing the checks. If we ever hit the problem (unable to clear the
> error flags), it means that something went wrong _before_, and we must
> have logged these problems already. As a matter of fact, that was
> exactly the situation for Felix, the message you want to remove was the
> 4th error message logged, so in fact it did not really add any value.

May I read this as an ack?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jean Delvare]

On Thu, 9 Dec 2021 10:16:45 +0100, Wolfram Sang wrote:
> > As I got some time to think about it, answering myself: I'm fine
> > removing the checks. If we ever hit the problem (unable to clear the
> > error flags), it means that something went wrong _before_, and we must
> > have logged these problems already. As a matter of fact, that was
> > exactly the situation for Felix, the message you want to remove was the
> > 4th error message logged, so in fact it did not really add any value.  
> 
> May I read this as an ack?

Yes, sorry ^^

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Tested-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

On Thu, Dec 02, 2021 at 10:53:05AM +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.
> 
> Whilst at it, change involved syslog messages to use pci_dbg() et al.
> to simplify them.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]