| Message ID | 20240410112418.6400-26-wsa+renesas@sang-engineering.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: remove printout on handled timeouts | expand |
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@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'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
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
Wed, Apr 10, 2024 at 01:24:20PM +0200, Wolfram Sang kirjoitti: > 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. As I mentioned in reply to cover letter this change does not seem to me right. If you provide an equivalent dev_err() in the core, I'll be totally fine with it.
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"); /* try to stop the current command */ dev_dbg(&priv->pci_dev->dev, "Terminating the current operation\n"); outb_p(SMBHSTCNT_KILL, SMBHSTCNT(priv)); @@ -411,7 +411,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; }
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(-)