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