[PATCH 00/18] i2c: remove printout on handled timeouts

[PATCH 00/18] i2c: remove printout on handled timeouts

[Wolfram Sang]

While working on another cleanup series, I stumbled over the fact that
some drivers print an error on I2C or SMBus related timeouts. This is
wrong because it may be an expected state. The client driver on top
knows this, so let's keep error handling on this level and remove the
prinouts from controller drivers.

Looking forward to comments,

   Wolfram


Wolfram Sang (18):
  i2c: at91-master: remove printout on handled timeouts
  i2c: bcm-iproc: remove printout on handled timeouts
  i2c: bcm2835: remove printout on handled timeouts
  i2c: cadence: remove printout on handled timeouts
  i2c: davinci: remove printout on handled timeouts
  i2c: i801: remove printout on handled timeouts
  i2c: img-scb: remove printout on handled timeouts
  i2c: ismt: remove printout on handled timeouts
  i2c: nomadik: remove printout on handled timeouts
  i2c: omap: remove printout on handled timeouts
  i2c: qcom-geni: remove printout on handled timeouts
  i2c: qup: remove printout on handled timeouts
  i2c: rk3x: remove printout on handled timeouts
  i2c: sh_mobile: remove printout on handled timeouts
  i2c: st: remove printout on handled timeouts
  i2c: tegra: remove printout on handled timeouts
  i2c: uniphier-f: remove printout on handled timeouts
  i2c: uniphier: remove printout on handled timeouts

 drivers/i2c/busses/i2c-at91-master.c | 1 -
 drivers/i2c/busses/i2c-bcm-iproc.c   | 2 --
 drivers/i2c/busses/i2c-bcm2835.c     | 1 -
 drivers/i2c/busses/i2c-cadence.c     | 2 --
 drivers/i2c/busses/i2c-davinci.c     | 1 -
 drivers/i2c/busses/i2c-i801.c        | 4 ++--
 drivers/i2c/busses/i2c-img-scb.c     | 5 +----
 drivers/i2c/busses/i2c-ismt.c        | 1 -
 drivers/i2c/busses/i2c-nomadik.c     | 7 ++-----
 drivers/i2c/busses/i2c-omap.c        | 1 -
 drivers/i2c/busses/i2c-qcom-geni.c   | 5 +----
 drivers/i2c/busses/i2c-qup.c         | 4 +---
 drivers/i2c/busses/i2c-rk3x.c        | 3 ---
 drivers/i2c/busses/i2c-sh_mobile.c   | 1 -
 drivers/i2c/busses/i2c-st.c          | 5 +----
 drivers/i2c/busses/i2c-tegra.c       | 2 --
 drivers/i2c/busses/i2c-uniphier-f.c  | 1 -
 drivers/i2c/busses/i2c-uniphier.c    | 4 +---
 18 files changed, 9 insertions(+), 41 deletions(-)

-- 
2.43.0

[PATCH 14/18] i2c: sh_mobile: remove printout on handled timeouts

[Wolfram Sang]

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-sh_mobile.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index c65ac3d7eadc..f86c29737df1 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -688,7 +688,6 @@ static int sh_mobile_xfer(struct sh_mobile_i2c_data *pd,
 		}
 
 		if (!time_left) {
-			dev_err(pd->dev, "Transfer request timed out\n");
 			if (pd->dma_direction != DMA_NONE)
 				sh_mobile_i2c_cleanup_dma(pd, true);
 
-- 
2.43.0

Re: [PATCH 00/18] i2c: remove printout on handled timeouts

[Andi Shyti]

Hi Wolfram,

On Wed, Apr 10, 2024 at 01:24:14PM +0200, Wolfram Sang wrote:
> While working on another cleanup series, I stumbled over the fact that
> some drivers print an error on I2C or SMBus related timeouts. This is
> wrong because it may be an expected state. The client driver on top
> knows this, so let's keep error handling on this level and remove the
> prinouts from controller drivers.
> 
> Looking forward to comments,
> 
>    Wolfram

Applyed everything but patch 6 in i2c/i2c-host on

git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git

Thank you,
Andi

Patches applied
===============
[01/18] i2c: at91-master: remove printout on handled timeouts
        commit: 74cce8ed33aeac91f397d642901c94520e574f8b
[02/18] i2c: bcm-iproc: remove printout on handled timeouts
        commit: 9f98914320f3e332487042aa73bbbfacc1dc9896
[03/18] i2c: bcm2835: remove printout on handled timeouts
        commit: ab17612ffb60bf07e4268448e022576d42833bf7
[04/18] i2c: cadence: remove printout on handled timeouts
        commit: 7aaff22d3e939c5187512188d7e27eb5e93ae41e
[05/18] i2c: davinci: remove printout on handled timeouts
        commit: dc72daa5cdf1c6ffebaef0c6df1f4cdeb15cadd6
[07/18] i2c: img-scb: remove printout on handled timeouts
        commit: 3e720ba5e30d6dd1b22e0f8a23f1697d438092b8
[08/18] i2c: ismt: remove printout on handled timeouts
        commit: 800a297370161bda70a34cb00eb0fa2f0345b75f
[09/18] i2c: nomadik: remove printout on handled timeouts
        commit: 26fbd3025cbce49cb3dd71f3a10239f69546b3c2
[10/18] i2c: omap: remove printout on handled timeouts
        commit: d3f24197d8125b2bf75162ec5cc270fd68f894f4
[11/18] i2c: qcom-geni: remove printout on handled timeouts
        commit: 4677d9f5c98f1c2825de142de5df08621ea340b3
[12/18] i2c: qup: remove printout on handled timeouts
        commit: e28ec7512496848e8a340889c512a0167949dc8f
[13/18] i2c: rk3x: remove printout on handled timeouts
        commit: 1cf7a7b3c944f727f34453a132b8899685e32f81
[14/18] i2c: sh_mobile: remove printout on handled timeouts
        commit: 31fb960bf8a424c47a5bf4568685e058c9d6f24d
[15/18] i2c: st: remove printout on handled timeouts
        commit: bff862e67260f779b2188e4b39c1a9f9989532ee
[16/18] i2c: tegra: remove printout on handled timeouts
        commit: 5ea641d9ea5ee1b3536f8b75e658e3bf2c2a548e
[17/18] i2c: uniphier-f: remove printout on handled timeouts
        commit: c31bc8e162890cda38d045e73ff0004119ab28e7
[18/18] i2c: uniphier: remove printout on handled timeouts
        commit: 507a2da9539cdb839a1a2e57bfcca644bcfe0f03

Re: [PATCH 00/18] i2c: remove printout on handled timeouts

[Andy Shevchenko]

Wed, Apr 10, 2024 at 01:24:14PM +0200, Wolfram Sang kirjoitti:
> While working on another cleanup series, I stumbled over the fact that
> some drivers print an error on I2C or SMBus related timeouts. This is
> wrong because it may be an expected state. The client driver on top
> knows this, so let's keep error handling on this level and remove the
> prinouts from controller drivers.
> 
> Looking forward to comments,

I do not see an equivalent change in I²C core.

IIRC in our case (DW or i801 or iSMT) we often have this message as the only
one that points to the issues (on non-debug level), it will be much harder to
debug for our customers with this going away.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/18] i2c: remove printout on handled timeouts

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

Hi Andy,

On Wed, Apr 24, 2024 at 03:08:14AM +0300, Andy Shevchenko wrote:
> Wed, Apr 10, 2024 at 01:24:14PM +0200, Wolfram Sang kirjoitti:
> > While working on another cleanup series, I stumbled over the fact that
> > some drivers print an error on I2C or SMBus related timeouts. This is
> > wrong because it may be an expected state. The client driver on top
> > knows this, so let's keep error handling on this level and remove the
> > prinouts from controller drivers.
> > 
> > Looking forward to comments,
> 
> I do not see an equivalent change in I²C core.

There shouldn't be. The core neither knows if it is okay or not. The
client driver knows.

> IIRC in our case (DW or i801 or iSMT) we often have this message as the only

Often? How often?

> one that points to the issues (on non-debug level), it will be much harder to
> debug for our customers with this going away.

The proper fix is to print the error in the client driver. Maybe this
needs a helper for client drivers which they can use like:

	i2c_report_error(client, retval, flags);

The other thing which is also more helpful IMO is that we have
trace_events for __i2c_transfer. There, you can see what was happening
on the bus before the timeout. It can easily be that, if device X
times out, it was because of the transfer before to device Y which locks
up the bus. A simple "Bus timed out" message will not help you a lot
there.

And, keep in mind the false positives I mentioned in the coverletter.

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 00/18] i2c: remove printout on handled timeouts

[Andy Shevchenko]

On Wed, Apr 24, 2024 at 12:00 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> On Wed, Apr 24, 2024 at 03:08:14AM +0300, Andy Shevchenko wrote:
> > Wed, Apr 10, 2024 at 01:24:14PM +0200, Wolfram Sang kirjoitti:
> > > While working on another cleanup series, I stumbled over the fact that
> > > some drivers print an error on I2C or SMBus related timeouts. This is
> > > wrong because it may be an expected state. The client driver on top
> > > knows this, so let's keep error handling on this level and remove the
> > > prinouts from controller drivers.
> > >
> > > Looking forward to comments,
> >
> > I do not see an equivalent change in I²C core.
>
> There shouldn't be. The core neither knows if it is okay or not. The
> client driver knows.
>
> > IIRC in our case (DW or i801 or iSMT) we often have this message as the only
>
> Often? How often?

Once in a couple of months I assume. Last time it was a few weeks ago
that there was a report and they pointed to this very message which
was helpful.

> > one that points to the issues (on non-debug level), it will be much harder to
> > debug for our customers with this going away.
>
> The proper fix is to print the error in the client driver. Maybe this
> needs a helper for client drivers which they can use like:
>
>         i2c_report_error(client, retval, flags);
>
> The other thing which is also more helpful IMO is that we have
> trace_events for __i2c_transfer. There, you can see what was happening
> on the bus before the timeout. It can easily be that, if device X
> times out, it was because of the transfer before to device Y which locks
> up the bus. A simple "Bus timed out" message will not help you a lot
> there.

The trace events are good to have, not sure if production kernels have
them enabled, though.

> And, keep in mind the false positives I mentioned in the coverletter.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/18] i2c: remove printout on handled timeouts

[Andy Shevchenko]

On Wed, Apr 24, 2024 at 3:41 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Apr 24, 2024 at 12:00 PM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > On Wed, Apr 24, 2024 at 03:08:14AM +0300, Andy Shevchenko wrote:
> > > Wed, Apr 10, 2024 at 01:24:14PM +0200, Wolfram Sang kirjoitti:
> > > > While working on another cleanup series, I stumbled over the fact that
> > > > some drivers print an error on I2C or SMBus related timeouts. This is
> > > > wrong because it may be an expected state. The client driver on top
> > > > knows this, so let's keep error handling on this level and remove the
> > > > prinouts from controller drivers.
> > > >
> > > > Looking forward to comments,
> > >
> > > I do not see an equivalent change in I²C core.
> >
> > There shouldn't be. The core neither knows if it is okay or not. The
> > client driver knows.
> >
> > > IIRC in our case (DW or i801 or iSMT) we often have this message as the only
> >
> > Often? How often?
>
> Once in a couple of months I assume. Last time it was a few weeks ago
> that there was a report and they pointed to this very message which
> was helpful.
>
> > > one that points to the issues (on non-debug level), it will be much harder to
> > > debug for our customers with this going away.
> >
> > The proper fix is to print the error in the client driver. Maybe this
> > needs a helper for client drivers which they can use like:
> >
> >         i2c_report_error(client, retval, flags);
> >
> > The other thing which is also more helpful IMO is that we have
> > trace_events for __i2c_transfer. There, you can see what was happening
> > on the bus before the timeout. It can easily be that, if device X
> > times out, it was because of the transfer before to device Y which locks
> > up the bus. A simple "Bus timed out" message will not help you a lot
> > there.
>
> The trace events are good to have, not sure if production kernels have
> them enabled, though.

Ah, and to accent on the usefulness of the message that happens before
one thinks about enabling trace events. How usual is that we _expect_
something to fail? Deeper debugging usually happens after we have
noticed the issue. I'm not sure if there is an equivalent to signal
about a problem without expecting it to happen. Is that -ETIMEDOUT
being converted to some message somewhere?

> > And, keep in mind the false positives I mentioned in the coverletter.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/18] i2c: remove printout on handled timeouts

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 186 bytes --]


> about a problem without expecting it to happen. Is that -ETIMEDOUT
> being converted to some message somewhere?

As said initially, the place for that is the client driver, I'd say.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]