mbox series

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

Message ID 20240410112418.6400-20-wsa+renesas@sang-engineering.com
Headers show
Series i2c: remove printout on handled timeouts | expand

Message

Wolfram Sang April 10, 2024, 11:24 a.m. UTC
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(-)

Comments

Heiko Stuebner April 13, 2024, 7:58 a.m. UTC | #1
Am Samstag, 13. April 2024, 08:44:41 CEST schrieb Dragan Simic:
> On 2024-04-13 08:38, Wolfram Sang wrote:
> >> Maybe it would be good to turn it into a debug message, instead of
> >> simply removing it?  Maybe not all client drivers handle it correctly,
> >> in which case having an easy way for debugging would be beneficial.
> > 
> > Hmm, but it still returns -ETIMEDOUT to distinguish cases?
> 
> Sure, but I think that having such an additional debug facility
> can only help and save the people from adding temporary printk()s
> while debugging.

Also we're talking about two lines of code, I wouldn't call that bloat ;-)
I was thinking about dev_dbg vs. removal too, but hadn't a clear
favorite.

So essentially Dragan is tipping the scale and I guess dev_dbg might be
the nicer way to go.


Heiko
Dragan Simic April 13, 2024, 8:12 a.m. UTC | #2
Hello Heiko,

On 2024-04-13 09:58, Heiko Stübner wrote:
> Am Samstag, 13. April 2024, 08:44:41 CEST schrieb Dragan Simic:
>> On 2024-04-13 08:38, Wolfram Sang wrote:
>> >> Maybe it would be good to turn it into a debug message, instead of
>> >> simply removing it?  Maybe not all client drivers handle it correctly,
>> >> in which case having an easy way for debugging would be beneficial.
>> >
>> > Hmm, but it still returns -ETIMEDOUT to distinguish cases?
>> 
>> Sure, but I think that having such an additional debug facility
>> can only help and save the people from adding temporary printk()s
>> while debugging.
> 
> Also we're talking about two lines of code, I wouldn't call that bloat 
> ;-)
> I was thinking about dev_dbg vs. removal too, but hadn't a clear
> favorite.
> 
> So essentially Dragan is tipping the scale and I guess dev_dbg might be
> the nicer way to go.

Yes, the code for printing the message is already there and it's only
a couple of lines, so it might be a good idea to recycle it. :)
Wolfram Sang April 13, 2024, 2:35 p.m. UTC | #3
> Also we're talking about two lines of code, I wouldn't call that bloat ;-)

With this patch, yes. But once you allow debug code, it is hard to draw
a line which debug is still okay and which is too fine-grained. And then
you end up with a lot. Over the years, I developed the tendency to try
to have less but meaningful error printouts. But I don't enforce it
strictly because it is too much bike-shedding discussion.

In case of this error printout here, it is just wrong. But, see, it also
came from this tendency I don't like to have printouts for every error.
Andi Shyti April 16, 2024, 7:02 p.m. UTC | #4
Hi,

On Sat, Apr 13, 2024 at 04:35:06PM +0200, Wolfram Sang wrote:
> > Also we're talking about two lines of code, I wouldn't call that bloat ;-)
> 
> With this patch, yes. But once you allow debug code, it is hard to draw
> a line which debug is still okay and which is too fine-grained. And then
> you end up with a lot. Over the years, I developed the tendency to try
> to have less but meaningful error printouts. But I don't enforce it
> strictly because it is too much bike-shedding discussion.
> 
> In case of this error printout here, it is just wrong. But, see, it also
> came from this tendency I don't like to have printouts for every error.

I agree with Wolfram here. Debug messages are OK if they are
providing real useful information to a final product.

Besides, as I explained earlier, the patter:

	dev_dbg("timed out")
	return -ETIMEDOUT;

(print a debug but return error) doesn't make much sense.

But, I this is all personal preference. So that I can also leave
it to the specific maintainer.
Andy Shevchenko April 24, 2024, 12:44 p.m. UTC | #5
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.
Wolfram Sang April 27, 2024, 6:03 p.m. UTC | #6
> 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.