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: <uvnhbxkhj4skur5uhmbdtmbc4ebodrdujfzqmrv6tjejwvjrxk@xvad5h5ciiay>
Date: Thu, 11 Apr 2024 11:16:18 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: Wolfram Sang <wsa+renesas@...g-engineering.com>, 
	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

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