Message-ID: <CAKRcmag2naB67VTJj+xdyJGN3i6Q9a9EmF1qamfZSFs-w1RkMQ@mail.gmail.com>
Date: Sat, 6 Sep 2025 10:10:51 +0300
From: Mohammad Gomaa <midomaxgomaa@...il.com>
To: Wolfram Sang <wsa+renesas@...g-engineering.com>
Cc: Steven Rostedt <rostedt@...dmis.org>, Masami Hiramatsu <mhiramat@...nel.org>, 
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, linux-i2c@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org, 
	kenalba@...gle.com, hbarnor@...omium.org, rayxu@...gle.com
Subject: Re: [PATCH WIP v2] i2c: add tracepoints to aid debugging in i2c-core-base

Hello everyone,

Thanks for your replies and guidance, I truly appreciate it.

ChromiumOS had a small subset of Chromebooks that had failing input devices
without any useful information in the Kernel logs, initially we
thought it had something
to do with the i2c probing logic, which led to us trying to improve
logging in the i2c probe function.

We started by adding a bunch of `dev_err` and `dev_debug` then we moved
to traces when we saw this Kernel Patch (note: it didn't get merged):
https://patchwork.kernel.org/project/linux-input/patch/C1C54D7FA3DF3958+20250710073148.3994900-1-wangyuli@uniontech.com/

My point is: we can always circle back to using `dev_err` and `dev_debug`,
while focusing on making the least amount of changes.

Few examples of where we think adding an extra log line would be beneficial:
- https://elixir.bootlin.com/linux/v6.6.94/source/drivers/i2c/i2c-core-base.c#L539-L544
(log that no driver were matched)
- https://elixir.bootlin.com/linux/v6.6.94/source/drivers/i2c/i2c-core-base.c#L614
(log that probing failed)

I hope this makes more sense, what do you think?

Thanks,
Mohammad Gomaa



On Thu, Sep 4, 2025 at 11:04 PM Wolfram Sang
<wsa+renesas@...g-engineering.com> wrote:
>
> Hi Mohammad,
>
> On Sun, Aug 17, 2025 at 10:55:14AM +0300, Mohammad Gomaa wrote:
> > Add tracepoints to i2c-core-base.c file to help trace
> > and debug I2C device probe failures.
> >
> > The new trace points are:
> > - i2c_device_probe_debug: records non-failure routines
> >   e.g. IRQ 0.
> > - i2c_device_probe_complete: records failed & successful
> >   probbes with appropriate trace message.
> >
> > To support operation of these tracepoints an enum
> > was added that stores log message for debug and failure.
> >
> > Signed-off-by: Mohammad Gomaa <midomaxgomaa@...il.com>
> > ---
> > Hello,
> >
> > This patch adds tracepoints to i2c-core-base to aid with debugging I2C probing failrues.
> >
> > The motivation for this comes from my work in Google Summer of Code (GSoC) 2025:
> > "ChromeOS Platform Input Device Quality Monitoring"
> > https://summerofcode.withgoogle.com/programs/2025/projects/uCdIgK7K
> >
> > This is my first submission to the Linux kernel, so any feedback is welcome.
>
> Welcome to Kernel hacking!
>
> I understand from the link above that this patch is intended for quality
> assurance. With automatic testing, you can find regression of firmware
> updates much easier.
>
> The drawback is that it is quite some code for such a niche use case.
> Also, I would think this code is very fragile because whenever the code
> path in probe changes, one must ensure that err_reason is still set
> accordingly. This adds maintenance burden.
>
> I wonder if you can't use the output in the kernel log to identify
> problems? We could add some if they are really missing. I know such
> strings are not static. Still, my gut feeling says some application
> should handle the complexity, not the kernel. Wouldn't we need a patch
> like this for each and every subsystem if we follow that route?
>
> Happy hacking,
>
>    Wolfram

Powered by blists - more mailing lists