[PATCH 0/4] i2c: remove printout on handled timeouts - part 2

[PATCH 0/4] i2c: remove printout on handled timeouts - part 2

[Wolfram Sang]

Here is the continuation of the previous series, largely applied by Andi
meanwhile (Thanks!). I improved the i801 driver (only one debug print
left) and added the three ali* drivers which I overlooked before. Hope
you'll like the patches...


Wolfram Sang (4):
  i2c: i801: remove printout on handled timeouts
  i2c: ali1535: remove printout on handled timeouts
  i2c: ali1563: remove printout on handled timeouts
  i2c: ali15x3: remove printout on handled timeouts

 drivers/i2c/busses/i2c-ali1535.c | 8 ++------
 drivers/i2c/busses/i2c-ali1563.c | 1 -
 drivers/i2c/busses/i2c-ali15x3.c | 4 +---
 drivers/i2c/busses/i2c-i801.c    | 4 +---
 4 files changed, 4 insertions(+), 13 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] i2c: i801: remove printout on handled timeouts

[Wolfram Sang]

I2C and SMBus timeouts are not something the user needs to be informed
about on controller level. The client driver may know if that really is
a problem and give more detailed information to the user. The controller
should just pass this information upwards. Remove the printout and turn
the SMBus-specific termination message to debug.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-i801.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index e577abc776c1..d2d2a6dbe29f 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -399,9 +399,7 @@ static int i801_check_post(struct i801_priv *priv, int status)
 	 * If the SMBus is still busy, we give up
 	 */
 	if (unlikely(status < 0)) {
-		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
 		/* try to stop the current command */
-		dev_dbg(&priv->pci_dev->dev, "Terminating the current operation\n");
 		outb_p(SMBHSTCNT_KILL, SMBHSTCNT(priv));
 		usleep_range(1000, 2000);
 		outb_p(0, SMBHSTCNT(priv));
@@ -410,7 +408,7 @@ static int i801_check_post(struct i801_priv *priv, int status)
 		status = inb_p(SMBHSTSTS(priv));
 		if ((status & SMBHSTSTS_HOST_BUSY) ||
 		    !(status & SMBHSTSTS_FAILED))
-			dev_err(&priv->pci_dev->dev,
+			dev_dbg(&priv->pci_dev->dev,
 				"Failed terminating the transaction\n");
 		return -ETIMEDOUT;
 	}
-- 
2.43.0

[PATCH 2/4] i2c: ali1535: remove printout on handled timeouts

[Wolfram Sang]

I2C and SMBus timeouts are not something the user needs to be informed
about on controller level. The client driver may know if that really is
a problem and give more detailed information to the user. The controller
should just pass this information upwards. Remove the printout.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-ali1535.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ali1535.c b/drivers/i2c/busses/i2c-ali1535.c
index 461eb23f9d47..9d7b4efe26ad 100644
--- a/drivers/i2c/busses/i2c-ali1535.c
+++ b/drivers/i2c/busses/i2c-ali1535.c
@@ -285,10 +285,8 @@ static int ali1535_transaction(struct i2c_adapter *adap)
 		 && (timeout++ < MAX_TIMEOUT));
 
 	/* If the SMBus is still busy, we give up */
-	if (timeout > MAX_TIMEOUT) {
+	if (timeout > MAX_TIMEOUT)
 		result = -ETIMEDOUT;
-		dev_err(&adap->dev, "SMBus Timeout!\n");
-	}
 
 	if (temp & ALI1535_STS_FAIL) {
 		result = -EIO;
@@ -313,10 +311,8 @@ static int ali1535_transaction(struct i2c_adapter *adap)
 	}
 
 	/* check to see if the "command complete" indication is set */
-	if (!(temp & ALI1535_STS_DONE)) {
+	if (!(temp & ALI1535_STS_DONE))
 		result = -ETIMEDOUT;
-		dev_err(&adap->dev, "Error: command never completed\n");
-	}
 
 	dev_dbg(&adap->dev, "Transaction (post): STS=%02x, TYP=%02x, "
 		"CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n",
-- 
2.43.0

[PATCH 3/4] i2c: ali1563: remove printout on handled timeouts

[Wolfram Sang]

I2C and SMBus timeouts are not something the user needs to be informed
about on controller level. The client driver may know if that really is
a problem and give more detailed information to the user. The controller
should just pass this information upwards. Remove the printout.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-ali1563.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-ali1563.c b/drivers/i2c/busses/i2c-ali1563.c
index 307fb0666ecb..63897a89bb35 100644
--- a/drivers/i2c/busses/i2c-ali1563.c
+++ b/drivers/i2c/busses/i2c-ali1563.c
@@ -99,7 +99,6 @@ static int ali1563_transaction(struct i2c_adapter *a, int size)
 		return 0;
 
 	if (!timeout) {
-		dev_err(&a->dev, "Timeout - Trying to KILL transaction!\n");
 		/* Issue 'kill' to host controller */
 		outb_p(HST_CNTL2_KILL, SMB_HST_CNTL2);
 		data = inb_p(SMB_HST_STS);
-- 
2.43.0

[PATCH 4/4] i2c: ali15x3: remove printout on handled timeouts

[Wolfram Sang]

I2C and SMBus timeouts are not something the user needs to be informed
about on controller level. The client driver may know if that really is
a problem and give more detailed information to the user. The controller
should just pass this information upwards. Remove the printout.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-ali15x3.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ali15x3.c b/drivers/i2c/busses/i2c-ali15x3.c
index d2fa30deb054..956e5020d71e 100644
--- a/drivers/i2c/busses/i2c-ali15x3.c
+++ b/drivers/i2c/busses/i2c-ali15x3.c
@@ -294,10 +294,8 @@ static int ali15x3_transaction(struct i2c_adapter *adap)
 		 && (timeout++ < MAX_TIMEOUT));
 
 	/* If the SMBus is still busy, we give up */
-	if (timeout > MAX_TIMEOUT) {
+	if (timeout > MAX_TIMEOUT)
 		result = -ETIMEDOUT;
-		dev_err(&adap->dev, "SMBus Timeout!\n");
-	}
 
 	if (temp & ALI15X3_STS_TERM) {
 		result = -EIO;
-- 
2.43.0

Re: [PATCH 4/4] i2c: ali15x3: remove printout on handled timeouts

[Jean Delvare]

On Tue, 23 Apr 2024 14:13:21 +0200, Wolfram Sang wrote:
> I2C and SMBus timeouts are not something the user needs to be informed
> about on controller level. The client driver may know if that really is
> a problem and give more detailed information to the user. The controller
> should just pass this information upwards. Remove the printout.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-ali15x3.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-ali15x3.c b/drivers/i2c/busses/i2c-ali15x3.c
> index d2fa30deb054..956e5020d71e 100644
> --- a/drivers/i2c/busses/i2c-ali15x3.c
> +++ b/drivers/i2c/busses/i2c-ali15x3.c
> @@ -294,10 +294,8 @@ static int ali15x3_transaction(struct i2c_adapter *adap)
>  		 && (timeout++ < MAX_TIMEOUT));
>  
>  	/* If the SMBus is still busy, we give up */
> -	if (timeout > MAX_TIMEOUT) {
> +	if (timeout > MAX_TIMEOUT)
>  		result = -ETIMEDOUT;
> -		dev_err(&adap->dev, "SMBus Timeout!\n");
> -	}
>  
>  	if (temp & ALI15X3_STS_TERM) {
>  		result = -EIO;

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

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/4] i2c: ali1563: remove printout on handled timeouts

[Jean Delvare]

On Tue, 23 Apr 2024 14:13:20 +0200, Wolfram Sang wrote:
> I2C and SMBus timeouts are not something the user needs to be informed
> about on controller level. The client driver may know if that really is
> a problem and give more detailed information to the user. The controller
> should just pass this information upwards. Remove the printout.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-ali1563.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-ali1563.c b/drivers/i2c/busses/i2c-ali1563.c
> index 307fb0666ecb..63897a89bb35 100644
> --- a/drivers/i2c/busses/i2c-ali1563.c
> +++ b/drivers/i2c/busses/i2c-ali1563.c
> @@ -99,7 +99,6 @@ static int ali1563_transaction(struct i2c_adapter *a, int size)
>  		return 0;
>  
>  	if (!timeout) {
> -		dev_err(&a->dev, "Timeout - Trying to KILL transaction!\n");
>  		/* Issue 'kill' to host controller */
>  		outb_p(HST_CNTL2_KILL, SMB_HST_CNTL2);
>  		data = inb_p(SMB_HST_STS);

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

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 1/4] i2c: i801: remove printout on handled timeouts

[Jean Delvare]

On Tue, 23 Apr 2024 14:13:18 +0200, Wolfram Sang wrote:
> I2C and SMBus timeouts are not something the user needs to be informed
> about on controller level. The client driver may know if that really is
> a problem and give more detailed information to the user. The controller
> should just pass this information upwards. Remove the printout and turn
> the SMBus-specific termination message to debug.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index e577abc776c1..d2d2a6dbe29f 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -399,9 +399,7 @@ static int i801_check_post(struct i801_priv *priv, int status)
>  	 * If the SMBus is still busy, we give up
>  	 */
>  	if (unlikely(status < 0)) {
> -		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
>  		/* try to stop the current command */
> -		dev_dbg(&priv->pci_dev->dev, "Terminating the current operation\n");
>  		outb_p(SMBHSTCNT_KILL, SMBHSTCNT(priv));
>  		usleep_range(1000, 2000);
>  		outb_p(0, SMBHSTCNT(priv));
> @@ -410,7 +408,7 @@ static int i801_check_post(struct i801_priv *priv, int status)
>  		status = inb_p(SMBHSTSTS(priv));
>  		if ((status & SMBHSTSTS_HOST_BUSY) ||
>  		    !(status & SMBHSTSTS_FAILED))
> -			dev_err(&priv->pci_dev->dev,
> +			dev_dbg(&priv->pci_dev->dev,
>  				"Failed terminating the transaction\n");
>  		return -ETIMEDOUT;
>  	}

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

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/4] i2c: ali1535: remove printout on handled timeouts

[Jean Delvare]

Hi Wolfram,

On Tue, 23 Apr 2024 14:13:19 +0200, Wolfram Sang wrote:
> I2C and SMBus timeouts are not something the user needs to be informed
> about on controller level. The client driver may know if that really is
> a problem and give more detailed information to the user. The controller
> should just pass this information upwards. Remove the printout.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-ali1535.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-ali1535.c b/drivers/i2c/busses/i2c-ali1535.c
> index 461eb23f9d47..9d7b4efe26ad 100644
> --- a/drivers/i2c/busses/i2c-ali1535.c
> +++ b/drivers/i2c/busses/i2c-ali1535.c
> @@ -285,10 +285,8 @@ static int ali1535_transaction(struct i2c_adapter *adap)
>  		 && (timeout++ < MAX_TIMEOUT));
>  
>  	/* If the SMBus is still busy, we give up */
> -	if (timeout > MAX_TIMEOUT) {
> +	if (timeout > MAX_TIMEOUT)
>  		result = -ETIMEDOUT;
> -		dev_err(&adap->dev, "SMBus Timeout!\n");
> -	}
>  
>  	if (temp & ALI1535_STS_FAIL) {
>  		result = -EIO;
> @@ -313,10 +311,8 @@ static int ali1535_transaction(struct i2c_adapter *adap)
>  	}
>  
>  	/* check to see if the "command complete" indication is set */
> -	if (!(temp & ALI1535_STS_DONE)) {
> +	if (!(temp & ALI1535_STS_DONE))
>  		result = -ETIMEDOUT;
> -		dev_err(&adap->dev, "Error: command never completed\n");
> -	}
>  
>  	dev_dbg(&adap->dev, "Transaction (post): STS=%02x, TYP=%02x, "
>  		"CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n",

I'm skeptical about that one, although this might be mainly an issue
with the code flow rather than your proposed changes.

There are 2 conditions which cause result to be set to -ETIMEDOUT.
After removing the messages, there's no way to differentiate between
these two cases, which could make bug investigation more difficult.

Another concern is that it is possible (at least theoretically) to hit
the first timeout condition and NOT return -TIMEDOUT. This is because
the code flow tests a number of conditions in a non-exclusive way, so
errnos may overwrite each other. I don't like this design. The
consequence is that the calling device driver may not be able to report
the timeout, while this was the reason you gave for removing the
message.

That being said, this is a very old driver, maintained in best effort
mode, I actually very much doubt it has any user left, so there's
little point in spending too much time on this. My gut feeling is that
the first "result = -ETIMEDOUT" isn't actually needed in practice and
will always be overwritten by another errno later in the code flow
(possibly the second "result = -ETIMEDOUT"). So most likely your change
is safe.

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

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 0/4] i2c: remove printout on handled timeouts - part 2

[Andi Shyti]

Hi Wolfram,

On Tue, Apr 23, 2024 at 02:13:17PM +0200, Wolfram Sang wrote:
> Here is the continuation of the previous series, largely applied by Andi
> meanwhile (Thanks!). I improved the i801 driver (only one debug print
> left) and added the three ali* drivers which I overlooked before. Hope
> you'll like the patches...
> 
> 
> Wolfram Sang (4):
>   i2c: i801: remove printout on handled timeouts
>   i2c: ali1535: remove printout on handled timeouts
>   i2c: ali1563: remove printout on handled timeouts
>   i2c: ali15x3: remove printout on handled timeouts

Applied to i2c/i2c-host.

Andi