Received-SPF: pass (google.com: domain of linux-kernel+bounces-140150-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 11 Apr 2024 11:16:18 +0200
From: Andi Shyti <andi.shyti@kernel.org>
To: Wolfram Sang <wsa+renesas@sang-engineering.com>, 
	linux-i2c@vger.kernel.org, Jean Delvare <jdelvare@suse.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 06/18] i2c: i801: remove printout on handled timeouts
Message-ID: <uvnhbxkhj4skur5uhmbdtmbc4ebodrdujfzqmrv6tjejwvjrxk@xvad5h5ciiay>
References: <20240410112418.6400-20-wsa+renesas@sang-engineering.com>
 <20240410112418.6400-26-wsa+renesas@sang-engineering.com>
 <242ogjpole3ltk5nu53knbfsxmmwcqfrbcivjh7fnkngvrroq5@cwspwdrtepwh>
 <zmkluzi3ncze67wei6eccd67cpuab2k7qw7cdgju4tg7rermv2@hw6ukejz4cvy>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <zmkluzi3ncze67wei6eccd67cpuab2k7qw7cdgju4tg7rermv2@hw6ukejz4cvy>

Hi Wolfram,

On Thu, Apr 11, 2024 at 09:02:52AM +0200, Wolfram Sang wrote:
> On Wed, Apr 10, 2024 at 02:21:58PM +0200, Andi Shyti wrote:
> > On Wed, Apr 10, 2024 at 01:24:20PM +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. Turn all timeout related
> > > printouts to debug level.
> > > 
> > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > ---
> > > 
> > > Here, I did not delete the printout to support checking the termination
> > > process. The other drivers in this series do not have this SMBus
> > > specific termination step.
> > > 
> > >  drivers/i2c/busses/i2c-i801.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > > index 4294c0c63cef..a42b5152f9bd 100644
> > > --- a/drivers/i2c/busses/i2c-i801.c
> > > +++ b/drivers/i2c/busses/i2c-i801.c
> > > @@ -400,7 +400,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");
> > > +		dev_dbg(&priv->pci_dev->dev, "Transaction timeout\n");
> > 
> > why after 5 patches of removing dev_err's, here you are changing
> > them to dev_dbg?
> 
> The reasoning was explained above:
> 
> > > Here, I did not delete the printout to support checking the termination
> > > process. The other drivers in this series do not have this SMBus
> > > specific termination step.
> 
> This is also why I converted two calls here to dev_dbg. But read on
> first.

It would make sense if the debug would give some more
information...

> > It's still good, but if we want to be strict, errors should
> > print errors: as we are returning -ETIMEDOUT, then we are
> > treating the case as an error and we should print error.
> 
> I strongly disagree. While we use an errno, we don't know if this is a
> real error yet. It is more a return value saying that the transfer timed
> out. The client driver knows. For some I2C clients this may be an error,
> but for an EEPROM this might be an "oh, it might still be erasing a
> page, let's try again after some defined delay".
> 
> Think of 'i2cdetect': If we printout something in the -ENXIO case (no
> device responded to the address), the log file would have more than 100
> entries on a typical I2C bus. Although we know that -ENXIO will be the
> majority of cases and are fine with it.
> 
> > As you did before, I would just remove the printout here.
> 
> Maybe we could because there is still the "Terminating the current
> operation" string as debug message making the code flow still clear.

.. e.g. for me it's not totally right if we do:

	dev_dbg("timed out")
	return -ETIMEDOUT;

Considering that this might not be a real error I would let the
calling function decide and print. Indeed i801_access() is not
even checking the error but letting the caller of smbus_xfer()
decide.

It would make more sense if we provide more information like:

	dev_dbg("Terminating current operation because the bus is busy and we timed out");

Just merged the two consecutive messages (we could still trim it
a bit and reduce dmesg spam).

The second /dev_err/dev_dbg/ in this file to me is fine (even
though it's not really self explaining).

> > I will wait a bit for more comments and take patches 1 to 5 so
> > that I can unburden you a little from them.
> 
> The patches have no dependencies. To keep mail traffic low, I suggest
> you continue reviewing and I only resend the i801 patch?

Yeah... I'll wait a few more days and give more time for reviews
and comments. I checked the rest of the series and it's fine.

If you are willing to send a V2 you could send it as reply to
this mail instead of resending everything.

Thanks,
Andi