linux-i2c.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH WIP v2] i2c: add tracepoints to aid debugging in i2c-core-base
@ 2025-08-17  7:55 Mohammad Gomaa
  2025-08-18  7:19 ` Dan Carpenter
  2025-08-19 16:09 ` Steven Rostedt
  0 siblings, 2 replies; 3+ messages in thread
From: Mohammad Gomaa @ 2025-08-17  7:55 UTC (permalink / raw)
  To: Wolfram Sang, Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers
  Cc: linux-i2c, linux-kernel, linux-trace-kernel, kenalba, hbarnor,
	rayxu, Mohammad Gomaa

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@gmail.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.
---
Changes in v2:
- Refactored i2c_device_probe_failed to i2c_device_probe_complete
  to support both successful and failed probes.
- Fixed formatting for TRACE_EVENT().
- Used enum instead of string variable for "reason".
- Link to v1: https://lore.kernel.org/r/20250806-refactor-add-i2c-tracepoints-v1-1-d1d39bd6fb57@gmail.com
---
 drivers/i2c/i2c-core-base.c | 63 ++++++++++++++++++++++++--------
 include/trace/events/i2c.h  | 88 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 137 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index ecca8c006b020379fb53293b20ab09ba25314609..7ca22759d26e85ee51bea60f414ed014e9bcec40 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -495,6 +495,8 @@ static int i2c_device_probe(struct device *dev)
 	struct i2c_driver	*driver;
 	bool do_power_on;
 	int status;
+	int err_reason;
+	bool has_id_table, has_acpi_match, has_of_match;
 
 	if (!client)
 		return 0;
@@ -509,38 +511,54 @@ static int i2c_device_probe(struct device *dev)
 			/* Keep adapter active when Host Notify is required */
 			pm_runtime_get_sync(&client->adapter->dev);
 			irq = i2c_smbus_host_notify_to_irq(client);
+			err_reason = I2C_TRACE_REASON_HOST_NOTIFY;
 		} else if (is_of_node(fwnode)) {
 			irq = fwnode_irq_get_byname(fwnode, "irq");
 			if (irq == -EINVAL || irq == -ENODATA)
 				irq = fwnode_irq_get(fwnode, 0);
+			err_reason = I2C_TRACE_REASON_FROM_OF;
 		} else if (is_acpi_device_node(fwnode)) {
 			bool wake_capable;
 
 			irq = i2c_acpi_get_irq(client, &wake_capable);
 			if (irq > 0 && wake_capable)
 				client->flags |= I2C_CLIENT_WAKE;
+			err_reason = I2C_TRACE_REASON_FROM_ACPI;
 		}
 		if (irq == -EPROBE_DEFER) {
 			status = dev_err_probe(dev, irq, "can't get irq\n");
+			err_reason = I2C_TRACE_REASON_EPROBE_DEFER_IRQ;
 			goto put_sync_adapter;
 		}
 
-		if (irq < 0)
+		if (irq < 0) {
+			trace_i2c_device_probe_debug(dev, err_reason);
 			irq = 0;
+		}
 
 		client->irq = irq;
 	}
 
 	driver = to_i2c_driver(dev->driver);
 
+	has_id_table = driver->id_table;
+	has_acpi_match = acpi_driver_match_device(dev, dev->driver);
+	has_of_match = i2c_of_match_device(dev->driver->of_match_table, client);
+
+	if (!has_id_table)
+		trace_i2c_device_probe_debug(dev, I2C_TRACE_REASON_NO_I2C_ID_TABLE);
+	if (!has_acpi_match)
+		trace_i2c_device_probe_debug(dev, I2C_TRACE_REASON_ACPI_ID_MISMATCH);
+	if (!has_of_match)
+		trace_i2c_device_probe_debug(dev, I2C_TRACE_REASON_OF_ID_MISMATCH);
+
 	/*
 	 * An I2C ID table is not mandatory, if and only if, a suitable OF
 	 * or ACPI ID table is supplied for the probing device.
 	 */
-	if (!driver->id_table &&
-	    !acpi_driver_match_device(dev, dev->driver) &&
-	    !i2c_of_match_device(dev->driver->of_match_table, client)) {
+	if (!has_id_table && !has_acpi_match && !has_of_match) {
 		status = -ENODEV;
+		err_reason = I2C_TRACE_REASON_NO_ID_MATCH;
 		goto put_sync_adapter;
 	}
 
@@ -550,47 +568,60 @@ static int i2c_device_probe(struct device *dev)
 		wakeirq = fwnode_irq_get_byname(fwnode, "wakeup");
 		if (wakeirq == -EPROBE_DEFER) {
 			status = dev_err_probe(dev, wakeirq, "can't get wakeirq\n");
+			err_reason = I2C_TRACE_REASON_EPROBE_DEFER_WAKEIRQ;
 			goto put_sync_adapter;
 		}
 
 		device_init_wakeup(&client->dev, true);
 
-		if (wakeirq > 0 && wakeirq != client->irq)
+		if (wakeirq > 0 && wakeirq != client->irq) {
 			status = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
-		else if (client->irq > 0)
+			err_reason = I2C_TRACE_REASON_SET_DED_WAKE_FAILED;
+		} else if (client->irq > 0) {
 			status = dev_pm_set_wake_irq(dev, client->irq);
-		else
+			err_reason = I2C_TRACE_REASON_SET_WAKE_FAILED;
+		} else {
 			status = 0;
+			err_reason = I2C_TRACE_REASON_NO_IRQ;
+		}
 
-		if (status)
+		if (status) {
 			dev_warn(&client->dev, "failed to set up wakeup irq\n");
+			trace_i2c_device_probe_debug(&client->dev, err_reason);
+		}
 	}
 
 	dev_dbg(dev, "probe\n");
 
 	status = of_clk_set_defaults(to_of_node(fwnode), false);
-	if (status < 0)
+	if (status < 0) {
+		err_reason = I2C_TRACE_REASON_SET_DEF_CLOCKS;
 		goto err_clear_wakeup_irq;
-
+	}
 	do_power_on = !i2c_acpi_waive_d0_probe(dev);
 	status = dev_pm_domain_attach(&client->dev, do_power_on ? PD_FLAG_ATTACH_POWER_ON : 0);
-	if (status)
+	if (status) {
+		err_reason = I2C_TRACE_REASON_ATTACH_PM_DOMAIN;
 		goto err_clear_wakeup_irq;
-
+	}
 	client->devres_group_id = devres_open_group(&client->dev, NULL,
 						    GFP_KERNEL);
 	if (!client->devres_group_id) {
 		status = -ENOMEM;
+		err_reason = I2C_TRACE_REASON_OPEN_DEVRES_GROUP;
 		goto err_detach_pm_domain;
 	}
 
 	client->debugfs = debugfs_create_dir(dev_name(&client->dev),
 					     client->adapter->debugfs);
 
-	if (driver->probe)
+	if (driver->probe) {
 		status = driver->probe(client);
-	else
+		err_reason = I2C_TRACE_REASON_PROBE_FAILED;
+	} else {
 		status = -EINVAL;
+		err_reason = I2C_TRACE_REASON_NO_PROBE;
+	}
 
 	/*
 	 * Note that we are not closing the devres group opened above so
@@ -603,6 +634,8 @@ static int i2c_device_probe(struct device *dev)
 	if (status)
 		goto err_release_driver_resources;
 
+	trace_i2c_device_probe_complete(&client->dev, status, err_reason);
+
 	return 0;
 
 err_release_driver_resources:
@@ -617,6 +650,8 @@ static int i2c_device_probe(struct device *dev)
 	if (client->flags & I2C_CLIENT_HOST_NOTIFY)
 		pm_runtime_put_sync(&client->adapter->dev);
 
+	trace_i2c_device_probe_complete(&client->dev, status, err_reason);
+
 	return status;
 }
 
diff --git a/include/trace/events/i2c.h b/include/trace/events/i2c.h
index 142a23c6593c611de9abc2a89a146b95550b23cd..b00650ceba0a96a7cc538991ce5d5a45ea553715 100644
--- a/include/trace/events/i2c.h
+++ b/include/trace/events/i2c.h
@@ -16,6 +16,94 @@
 /*
  * drivers/i2c/i2c-core-base.c
  */
+#ifndef I2C_TRACE_REASON_ENUM_DEFINED
+#define I2C_TRACE_REASON_ENUM_DEFINED
+
+#define I2C_TRACE_REASON \
+EM(HOST_NOTIFY,			"IRQ 0: could not get irq from Host Notify") \
+EM(FROM_OF,			"IRQ 0: could not get irq from OF") \
+EM(FROM_ACPI,			"IRQ 0: could not get irq from ACPI") \
+EM(EPROBE_DEFER_IRQ,		"IRQ 0: could not get IRQ due to EPROBE_DEFER") \
+EM(NO_I2C_ID_TABLE,		"no I2C ID table") \
+EM(ACPI_ID_MISMATCH,		"ACPI ID table mismatch") \
+EM(OF_ID_MISMATCH,		"OF ID table mismatch") \
+EM(NO_ID_MATCH,		"no I2C ID table and no ACPI/OF match") \
+EM(EPROBE_DEFER_WAKEIRQ,	"get wake IRQ due to EPROBE_DEFER") \
+EM(SET_DED_WAKE_FAILED,	"failed to set dedicated wakeup IRQ") \
+EM(SET_WAKE_FAILED,		"failed to set wakeup IRQ") \
+EM(NO_IRQ,			"no IRQ") \
+EM(SET_DEF_CLOCKS,		"set default clocks") \
+EM(ATTACH_PM_DOMAIN,		"attach PM domain") \
+EM(OPEN_DEVRES_GROUP,		"open devres group") \
+EM(PROBE_FAILED,		"specific driver probe failed") \
+EMe(NO_PROBE,			"no probe defined")
+
+#undef EM
+#undef EMe
+#define EM(a, b)	I2C_TRACE_REASON_##a,
+#define EMe(a, b)	I2C_TRACE_REASON_##a
+enum {
+	I2C_TRACE_REASON
+};
+
+#undef EM
+#undef EMe
+#define EM(a, b)	TRACE_DEFINE_ENUM(I2C_TRACE_REASON_##a);
+#define EMe(a, b)	TRACE_DEFINE_ENUM(I2C_TRACE_REASON_##a);
+I2C_TRACE_REASON
+
+#undef EM
+#undef EMe
+#define EM(a, b)		{ I2C_TRACE_REASON_##a, b },
+#define EMe(a, b)	{ I2C_TRACE_REASON_##a, b }
+
+#endif /* I2C_TRACE_REASON_ENUM_DEFINED */
+
+TRACE_EVENT(i2c_device_probe_debug,
+
+	TP_PROTO(struct device *dev, int err_reason),
+
+	TP_ARGS(dev, err_reason),
+
+	TP_STRUCT__entry(
+		__string(	devname,	dev_name(dev)	)
+		__field(	int,		err_reason	)
+	),
+
+	TP_fast_assign(
+		__assign_str(devname);
+		__entry->err_reason = err_reason;
+	),
+
+	TP_printk("device=%s: %s",
+		__get_str(devname),
+		__print_symbolic(__entry->err_reason, I2C_TRACE_REASON))
+);
+
+TRACE_EVENT(i2c_device_probe_complete,
+
+	TP_PROTO(struct device *dev, int status, int err_reason),
+
+	TP_ARGS(dev, status, err_reason),
+
+	TP_STRUCT__entry(
+		__string(	dev_name,	dev_name(dev)	)
+		__field(	int,		status		)
+		__field(	int,		err_reason	)
+	),
+
+	TP_fast_assign(
+		__assign_str(dev_name);
+		__entry->status = status;
+		__entry->err_reason = err_reason;
+	),
+
+	TP_printk("%s probe %s: %s",
+		__get_str(dev_name),
+		__entry->status ? "failed" : "succeeded",
+		__entry->status ? __print_symbolic(__entry->err_reason, I2C_TRACE_REASON) : "")
+);
+
 extern int i2c_transfer_trace_reg(void);
 extern void i2c_transfer_trace_unreg(void);
 

---
base-commit: 7e161a991ea71e6ec526abc8f40c6852ebe3d946
change-id: 20250806-refactor-add-i2c-tracepoints-b6e2b92d4cd9

Best regards,
-- 
Mohammad Gomaa <midomaxgomaa@gmail.com>


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH WIP v2] i2c: add tracepoints to aid debugging in i2c-core-base
  2025-08-17  7:55 [PATCH WIP v2] i2c: add tracepoints to aid debugging in i2c-core-base Mohammad Gomaa
@ 2025-08-18  7:19 ` Dan Carpenter
  2025-08-19 16:09 ` Steven Rostedt
  1 sibling, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2025-08-18  7:19 UTC (permalink / raw)
  To: oe-kbuild, Mohammad Gomaa, Wolfram Sang, Steven Rostedt,
	Masami Hiramatsu, Mathieu Desnoyers
  Cc: lkp, oe-kbuild-all, linux-i2c, linux-kernel, linux-trace-kernel,
	kenalba, hbarnor, rayxu, Mohammad Gomaa

Hi Mohammad,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Mohammad-Gomaa/i2c-add-tracepoints-to-aid-debugging-in-i2c-core-base/20250817-155936
base:   7e161a991ea71e6ec526abc8f40c6852ebe3d946
patch link:    https://lore.kernel.org/r/20250817-refactor-add-i2c-tracepoints-v2-1-c0bad299e02e%40gmail.com
patch subject: [PATCH WIP v2] i2c: add tracepoints to aid debugging in i2c-core-base
config: x86_64-randconfig-161-20250818 (https://download.01.org/0day-ci/archive/20250818/202508181525.GKAXMVYv-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202508181525.GKAXMVYv-lkp@intel.com/

smatch warnings:
drivers/i2c/i2c-core-base.c:535 i2c_device_probe() error: uninitialized symbol 'err_reason'.

vim +/err_reason +535 drivers/i2c/i2c-core-base.c

f37dd80ac2a67e4 drivers/i2c/i2c-core.c      David Brownell           2007-02-13  491  static int i2c_device_probe(struct device *dev)
^1da177e4c3f415 drivers/i2c/i2c-core.c      Linus Torvalds           2005-04-16  492  {
5763a474c887d4a drivers/i2c/i2c-core-base.c Andy Shevchenko          2025-04-16  493  	struct fwnode_handle	*fwnode = dev_fwnode(dev);
51298d1257b9f0a drivers/i2c/i2c-core.c      Jean Delvare             2009-09-18  494  	struct i2c_client	*client = i2c_verify_client(dev);
51298d1257b9f0a drivers/i2c/i2c-core.c      Jean Delvare             2009-09-18  495  	struct i2c_driver	*driver;
79ece9b292af6b0 drivers/i2c/i2c-core-base.c Ricardo Ribalda          2022-11-14  496  	bool do_power_on;
50c3304a5e1e521 drivers/i2c/i2c-core.c      Hans Verkuil             2008-03-12  497  	int status;
dc94c3bf033524a drivers/i2c/i2c-core-base.c Mohammad Gomaa           2025-08-17  498  	int err_reason;
dc94c3bf033524a drivers/i2c/i2c-core-base.c Mohammad Gomaa           2025-08-17  499  	bool has_id_table, has_acpi_match, has_of_match;
7b4fbc50fabb810 drivers/i2c/i2c-core.c      David Brownell           2007-05-01  500  
51298d1257b9f0a drivers/i2c/i2c-core.c      Jean Delvare             2009-09-18  501  	if (!client)
51298d1257b9f0a drivers/i2c/i2c-core.c      Jean Delvare             2009-09-18  502  		return 0;
51298d1257b9f0a drivers/i2c/i2c-core.c      Jean Delvare             2009-09-18  503  
6e76cb7dfd34a2e drivers/i2c/i2c-core-base.c Charles Keepax           2019-06-27  504  	client->irq = client->init_irq;
6e76cb7dfd34a2e drivers/i2c/i2c-core-base.c Charles Keepax           2019-06-27  505  
0c2a34937f7e4c4 drivers/i2c/i2c-core-base.c Wolfram Sang             2020-06-30  506  	if (!client->irq) {
845c877009cf014 drivers/i2c/i2c-core.c      Mika Westerberg          2015-05-06  507  		int irq = -ENOENT;
845c877009cf014 drivers/i2c/i2c-core.c      Mika Westerberg          2015-05-06  508  
331c34255293cd0 drivers/i2c/i2c-core.c      Dmitry Torokhov          2017-01-04  509  		if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
331c34255293cd0 drivers/i2c/i2c-core.c      Dmitry Torokhov          2017-01-04  510  			dev_dbg(dev, "Using Host Notify IRQ\n");
72bfcee11cf8950 drivers/i2c/i2c-core-base.c Jarkko Nikula            2019-04-30  511  			/* Keep adapter active when Host Notify is required */
72bfcee11cf8950 drivers/i2c/i2c-core-base.c Jarkko Nikula            2019-04-30  512  			pm_runtime_get_sync(&client->adapter->dev);
331c34255293cd0 drivers/i2c/i2c-core.c      Dmitry Torokhov          2017-01-04  513  			irq = i2c_smbus_host_notify_to_irq(client);
dc94c3bf033524a drivers/i2c/i2c-core-base.c Mohammad Gomaa           2025-08-17  514  			err_reason = I2C_TRACE_REASON_HOST_NOTIFY;
5763a474c887d4a drivers/i2c/i2c-core-base.c Andy Shevchenko          2025-04-16  515  		} else if (is_of_node(fwnode)) {
5d9424b00b577b6 drivers/i2c/i2c-core-base.c Andy Shevchenko          2025-04-16  516  			irq = fwnode_irq_get_byname(fwnode, "irq");
3fffd1283927342 drivers/i2c/i2c-core.c      Dmitry Torokhov          2015-08-17  517  			if (irq == -EINVAL || irq == -ENODATA)
5d9424b00b577b6 drivers/i2c/i2c-core-base.c Andy Shevchenko          2025-04-16  518  				irq = fwnode_irq_get(fwnode, 0);
dc94c3bf033524a drivers/i2c/i2c-core-base.c Mohammad Gomaa           2025-08-17  519  			err_reason = I2C_TRACE_REASON_FROM_OF;
5763a474c887d4a drivers/i2c/i2c-core-base.c Andy Shevchenko          2025-04-16  520  		} else if (is_acpi_device_node(fwnode)) {
b38f2d5d9615cf9 drivers/i2c/i2c-core-base.c Raul E Rangel            2022-09-29  521  			bool wake_capable;
b38f2d5d9615cf9 drivers/i2c/i2c-core-base.c Raul E Rangel            2022-09-29  522  
b38f2d5d9615cf9 drivers/i2c/i2c-core-base.c Raul E Rangel            2022-09-29  523  			irq = i2c_acpi_get_irq(client, &wake_capable);
b38f2d5d9615cf9 drivers/i2c/i2c-core-base.c Raul E Rangel            2022-09-29  524  			if (irq > 0 && wake_capable)
b38f2d5d9615cf9 drivers/i2c/i2c-core-base.c Raul E Rangel            2022-09-29  525  				client->flags |= I2C_CLIENT_WAKE;
dc94c3bf033524a drivers/i2c/i2c-core-base.c Mohammad Gomaa           2025-08-17  526  			err_reason = I2C_TRACE_REASON_FROM_ACPI;
3fffd1283927342 drivers/i2c/i2c-core.c      Dmitry Torokhov          2015-08-17  527  		}

err_reason not set on else { path.

3c3dd56f760da05 drivers/i2c/i2c-core-base.c Alain Volmat             2020-04-30  528  		if (irq == -EPROBE_DEFER) {
5c52473b4496b35 drivers/i2c/i2c-core-base.c Xu Yang                  2025-05-07  529  			status = dev_err_probe(dev, irq, "can't get irq\n");
dc94c3bf033524a drivers/i2c/i2c-core-base.c Mohammad Gomaa           2025-08-17  530  			err_reason = I2C_TRACE_REASON_EPROBE_DEFER_IRQ;
3c3dd56f760da05 drivers/i2c/i2c-core-base.c Alain Volmat             2020-04-30  531  			goto put_sync_adapter;
3c3dd56f760da05 drivers/i2c/i2c-core-base.c Alain Volmat             2020-04-30  532  		}
331c34255293cd0 drivers/i2c/i2c-core.c      Dmitry Torokhov          2017-01-04  533  
dc94c3bf033524a drivers/i2c/i2c-core-base.c Mohammad Gomaa           2025-08-17  534  		if (irq < 0) {
dc94c3bf033524a drivers/i2c/i2c-core-base.c Mohammad Gomaa           2025-08-17 @535  			trace_i2c_device_probe_debug(dev, err_reason);
                                                                                                                                          ^^^^^^^^^^

6f34be7400c68d3 drivers/i2c/i2c-core.c      Geert Uytterhoeven       2014-11-17  536  			irq = 0;
dc94c3bf033524a drivers/i2c/i2c-core-base.c Mohammad Gomaa           2025-08-17  537  		}
2fd36c55264926e drivers/i2c/i2c-core.c      Laurent Pinchart         2014-10-30  538  
2fd36c55264926e drivers/i2c/i2c-core.c      Laurent Pinchart         2014-10-30  539  		client->irq = irq;
2fd36c55264926e drivers/i2c/i2c-core.c      Laurent Pinchart         2014-10-30  540  	}
2fd36c55264926e drivers/i2c/i2c-core.c      Laurent Pinchart         2014-10-30  541  
0c2a34937f7e4c4 drivers/i2c/i2c-core-base.c Wolfram Sang             2020-06-30  542  	driver = to_i2c_driver(dev->driver);
0c2a34937f7e4c4 drivers/i2c/i2c-core-base.c Wolfram Sang             2020-06-30  543  
dc94c3bf033524a drivers/i2c/i2c-core-base.c Mohammad Gomaa           2025-08-17  544  	has_id_table = driver->id_table;

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH WIP v2] i2c: add tracepoints to aid debugging in i2c-core-base
  2025-08-17  7:55 [PATCH WIP v2] i2c: add tracepoints to aid debugging in i2c-core-base Mohammad Gomaa
  2025-08-18  7:19 ` Dan Carpenter
@ 2025-08-19 16:09 ` Steven Rostedt
  1 sibling, 0 replies; 3+ messages in thread
From: Steven Rostedt @ 2025-08-19 16:09 UTC (permalink / raw)
  To: Mohammad Gomaa
  Cc: Wolfram Sang, Masami Hiramatsu, Mathieu Desnoyers, linux-i2c,
	linux-kernel, linux-trace-kernel, kenalba, hbarnor, rayxu

On Sun, 17 Aug 2025 10:55:14 +0300
Mohammad Gomaa <midomaxgomaa@gmail.com> wrote:

> 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 Mohammad!

>  	driver = to_i2c_driver(dev->driver);
>  

I'll let those that own this code discuss the merits of this, and if
there's a better way to achieve this. But I'll comment only on the tracing
aspect of this change.

> +	has_id_table = driver->id_table;
> +	has_acpi_match = acpi_driver_match_device(dev, dev->driver);
> +	has_of_match = i2c_of_match_device(dev->driver->of_match_table, client);
> +
> +	if (!has_id_table)
> +		trace_i2c_device_probe_debug(dev, I2C_TRACE_REASON_NO_I2C_ID_TABLE);
> +	if (!has_acpi_match)
> +		trace_i2c_device_probe_debug(dev, I2C_TRACE_REASON_ACPI_ID_MISMATCH);
> +	if (!has_of_match)
> +		trace_i2c_device_probe_debug(dev, I2C_TRACE_REASON_OF_ID_MISMATCH);

The above adds if statements into the running code when tracing is disabled
and this causes pressure on the branch prediction and should be avoided. To
avoid this, you could use the trace_<tracepoint>_enabled() helper:

	if (trace_i2c_device_probe_debug_enabled()) {
		has_id_table = driver->id_table;
		has_acpi_match = acpi_driver_match_device(dev, dev->driver);
		has_of_match = i2c_of_match_device(dev->driver->of_match_table, client);

		if (!has_id_table)
			trace_i2c_device_probe_debug(dev, I2C_TRACE_REASON_NO_I2C_ID_TABLE);
		if (!has_acpi_match)
			trace_i2c_device_probe_debug(dev, I2C_TRACE_REASON_ACPI_ID_MISMATCH);
		if (!has_of_match)
			trace_i2c_device_probe_debug(dev, I2C_TRACE_REASON_OF_ID_MISMATCH);
	}

But I suspect there's a better way to record this information. I just
wanted to inform you on the trace_<tracepoint>_enabled() logic. What it
does is to add a static branch (jump label) where there's no "if"
statement. It's either a nop that skips this code altogether, or it's a jmp
to the code that will do the logic as well as the tracing and then jump
back to where it left off.

It's much more efficient to use this and it doesn't add anything to the
branch prediction cache.

-- Steve

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2025-08-19 16:09 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-17  7:55 [PATCH WIP v2] i2c: add tracepoints to aid debugging in i2c-core-base Mohammad Gomaa
2025-08-18  7:19 ` Dan Carpenter
2025-08-19 16:09 ` Steven Rostedt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).