Received-SPF: pass (google.com: domain of linux-kernel+bounces-158853-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 25 Apr 2024 17:58:19 +0200
From: Jean Delvare <jdelvare@suse.de>
To: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: linux-i2c@vger.kernel.org, Andi Shyti <andi.shyti@kernel.org>,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/4] i2c: ali1535: remove printout on handled timeouts
Message-ID: <20240425175819.20c1d9aa@endymion.delvare>
In-Reply-To: <20240423121322.28460-3-wsa+renesas@sang-engineering.com>
References: <20240423121322.28460-1-wsa+renesas@sang-engineering.com>
	<20240423121322.28460-3-wsa+renesas@sang-engineering.com>
Organization: SUSE Linux
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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