linux-kernel - Re: [PATCH 06/18] i2c: i801: remove printout on handled timeouts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <zmkluzi3ncze67wei6eccd67cpuab2k7qw7cdgju4tg7rermv2@hw6ukejz4cvy>
Date: Thu, 11 Apr 2024 09:02:52 +0200
From: Wolfram Sang <wsa+renesas@...g-engineering.com>
To: Andi Shyti <andi.shyti@...nel.org>
Cc: linux-i2c@...r.kernel.org, Jean Delvare <jdelvare@...e.com>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 06/18] i2c: i801: remove printout on handled timeouts

On Wed, Apr 10, 2024 at 02:21:58PM +0200, Andi Shyti wrote:
> Hi Wolfram,
> 
> 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@...g-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'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.

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

Happy hacking,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)