* [RFC Patch v5 Resend 1/1] i2c debug counters as sysfs attributes
2022-08-10 7:20 [RFC Patch v5 Resend 0/1] i2c counters as sysfs attributes Sui Chen
@ 2022-08-10 7:20 ` Sui Chen
0 siblings, 0 replies; 2+ messages in thread
From: Sui Chen @ 2022-08-10 7:20 UTC (permalink / raw)
To: linux-kernel, linux-i2c, wsa, openbmc, tali.perry1
Cc: joel, andrew, benjaminfair, krellan, Sui Chen
This change renames the I2C debug counters as suggested, and makes them
available to i2c_adapter's and i2c_client's:
- bus_errors
- transfers (only applicable to i2c_adapter)
- messages (only applicable to i2c_client)
- nacks
- timeouts
- recovery_successes (only applicable to i2c_adapter)
- recovery_failures (only applicable to i2c_adapter)
The above stats are located in the sysfs path "stats". The kobj for
"stats" for a i2c-device or an i2c-adapter are added as children of
the device's and adapter's kobj.
Did some quick tests with QEMU using a program that saves/replays I2C
trace by reading hwmon sensors (the program is located at
https://gerrit.openbmc-project.xyz/c/openbmc/openbmc-tools/+/52527)
(normal read)
root@gsj:~# cat /sys/class/i2c-adapter/i2c-1/stats/{transfers,nacks}\
/sys/class/i2c-adapter/i2c-1/1-005c/stats/{messages,nacks} && \
sleep 1 && ./i2c_bmk_bmc 0
46
0
92
0
/sys/class/hwmon/hwmon0/temp1_input: 0
[FindTraceEntries] t0=111.000000 t1=113.000000
Found 4 interesting I2C trace entries:
i2c_write: i2c-1 #0 a=05c f=0000 l=1 [00]
i2c_read: i2c-1 #1 a=05c f=0001 l=2
i2c_reply: i2c-1 #1 a=05c f=0001 l=2 [00-00]
i2c_result: i2c-1 n=2 ret=2
root@gsj:~# cat /sys/class/i2c-adapter/i2c-1/stats/{transfers,nacks} \
/sys/class/i2c-adapter/i2c-1/1-005c/stats/{messages,nacks}
47 <-- 1 more transfer on i2c-1
0
94 <-- 2 more messages sent to 1-005c
0
(deliberately nack)
root@gsj:~# sleep 1 && ./i2c_bmk_bmc 0
(injects 1 NACK using QEMU)
/sys/class/hwmon/hwmon0/temp1_input: 0
[FindTraceEntries] t0=116.000000 t1=118.000000
Found 3 interesting I2C trace entries:
i2c_write: i2c-1 #0 a=05c f=0000 l=1 [00]
i2c_read: i2c-1 #1 a=05c f=0001 l=2
i2c_result: i2c-1 n=2 ret=-6
root@gsj:~# cat /sys/class/i2c-adapter/i2c-1/stats/{transfers,nacks} \
/sys/class/i2c-adapter/i2c-1/1-005c/stats/{messages,nacks}
47
1 <-- 1 more NACK on i2c-1
94
1 <-- 1 more NACK attributed to 1-005c
Signed-off-by: Sui Chen <suichen@google.com>
---
drivers/i2c/i2c-core-base.c | 240 +++++++++++++++++++++++++++++++++++-
drivers/i2c/i2c-dev.c | 94 ++++++++++++++
include/linux/i2c.h | 41 ++++++
3 files changed, 374 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 84f12bf90644..4e7bd849f127 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -270,11 +270,22 @@ EXPORT_SYMBOL_GPL(i2c_generic_scl_recovery);
int i2c_recover_bus(struct i2c_adapter *adap)
{
+ if (!adap->stats)
+ i2c_adapter_create_stats_directory(adap);
+
if (!adap->bus_recovery_info)
return -EBUSY;
dev_dbg(&adap->dev, "Trying i2c bus recovery\n");
- return adap->bus_recovery_info->recover_bus(adap);
+ int ret = adap->bus_recovery_info->recover_bus(adap);
+
+ if (ret == 0)
+ ++(adap->stats->recovery_successes);
+ else
+ ++(adap->stats->recovery_failures);
+
+ return ret;
+
}
EXPORT_SYMBOL_GPL(i2c_recover_bus);
@@ -993,6 +1004,78 @@ int i2c_dev_irq_from_resources(const struct resource *resources,
return 0;
}
+ssize_t i2c_client_stats_messages_show(struct kobject *kobj, struct kobj_attribute *addr,
+ char *buf)
+{
+ return sysfs_emit(buf, "%llu\n",
+ container_of(kobj, struct i2c_client_stats, kobj)->messages);
+}
+
+struct kobj_attribute i2c_client_stats_messages_attr = {
+ .attr = { .name = "messages", .mode = 0444 },
+ .show = i2c_client_stats_messages_show,
+};
+
+ssize_t i2c_client_stats_bus_errors_show(struct kobject *kobj, struct kobj_attribute *addr,
+ char *buf)
+{
+ return sysfs_emit(buf, "%llu\n",
+ container_of(kobj, struct i2c_client_stats, kobj)->bus_errors);
+}
+
+struct kobj_attribute i2c_client_stats_bus_errors_attr = {
+ .attr = { .name = "bus_errors", .mode = 0444 },
+ .show = i2c_client_stats_bus_errors_show,
+};
+
+ssize_t i2c_client_stats_nacks_show(struct kobject *kobj, struct kobj_attribute *addr,
+ char *buf)
+{
+ return sysfs_emit(buf, "%llu\n",
+ container_of(kobj, struct i2c_client_stats, kobj)->nacks);
+}
+
+struct kobj_attribute i2c_client_stats_nacks_attr = {
+ .attr = { .name = "nacks", .mode = 0444 },
+ .show = i2c_client_stats_nacks_show,
+};
+
+ssize_t i2c_client_stats_timeouts_show(struct kobject *kobj, struct kobj_attribute *addr,
+ char *buf)
+{
+ return sysfs_emit(buf, "%llu\n",
+ container_of(kobj, struct i2c_client_stats, kobj)->timeouts);
+}
+
+struct kobj_attribute i2c_client_stats_timeouts_attr = {
+ .attr = { .name = "timeouts", .mode = 0444 },
+ .show = i2c_client_stats_timeouts_show,
+};
+
+static struct attribute *i2c_client_stats_attrs[] = {
+ &i2c_client_stats_messages_attr.attr,
+ &i2c_client_stats_bus_errors_attr.attr,
+ &i2c_client_stats_nacks_attr.attr,
+ &i2c_client_stats_timeouts_attr.attr,
+ NULL
+};
+
+static struct kobj_type i2c_client_stats_ktype = {
+ .sysfs_ops = &kobj_sysfs_ops,
+ .default_attrs = i2c_client_stats_attrs,
+};
+
+/**
+ * Functions for maintaining the adapter -> client stats lookup structure
+ */
+static struct i2c_client_stats *__get_i2c_client_stats(struct i2c_adapter_stats *stats,
+ int i2c_addr);
+static void __insert_i2c_client_stats(struct i2c_adapter_stats *stats,
+ struct i2c_client_stats *client_stats,
+ int i2c_addr);
+static void __remove_i2c_client_stats(struct i2c_adapter_stats *stats, int i2c_addr);
+static int __count_i2c_client_stats(struct i2c_adapter_stats *stats);
+
/**
* i2c_new_client_device - instantiate an i2c device
* @adap: the adapter managing the device
@@ -1014,6 +1097,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
{
struct i2c_client *client;
int status;
+ struct i2c_client_stats *client_stats;
client = kzalloc(sizeof *client, GFP_KERNEL);
if (!client)
@@ -1069,6 +1153,20 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n",
client->name, dev_name(&client->dev));
+ client_stats = kzalloc(sizeof(*client_stats), GFP_KERNEL);
+ client->stats = client_stats;
+ client_stats->address = info->addr;
+
+ kobject_init_and_add(&client_stats->kobj, &i2c_client_stats_ktype,
+ &client->dev.kobj, "stats");
+
+ if (!adap->stats)
+ i2c_adapter_create_stats_directory(adap);
+
+ __insert_i2c_client_stats(adap->stats, client_stats, info->addr);
+ dev_info(&adap->dev, "Added i2c_client_stats, adapter has %d client stats now",
+ __count_i2c_client_stats(adap->stats));
+
return client;
out_remove_swnode:
@@ -1103,6 +1201,12 @@ void i2c_unregister_device(struct i2c_client *client)
if (ACPI_COMPANION(&client->dev))
acpi_device_clear_enumerated(ACPI_COMPANION(&client->dev));
device_remove_software_node(&client->dev);
+
+ __remove_i2c_client_stats(client->adapter->stats, client->addr);
+ dev_info(&client->adapter->dev, "Removed i2c_client_stats, adapter has %d client stats now",
+ __count_i2c_client_stats(client->adapter->stats));
+ kfree(client->stats);
+
device_unregister(&client->dev);
}
EXPORT_SYMBOL_GPL(i2c_unregister_device);
@@ -2176,6 +2280,8 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
{
unsigned long orig_jiffies;
int ret, try;
+ int i, j, addr;
+ struct i2c_client_stats *cs;
if (WARN_ON(!msgs || num < 1))
return -EINVAL;
@@ -2223,6 +2329,50 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
trace_i2c_result(adap, num, ret);
}
+ // Per-adapter I2C stats
+ if (!adap->stats)
+ i2c_adapter_create_stats_directory(adap);
+ if (ret < 0) {
+ if (ret == -ENXIO)
+ ++(adap->stats->nacks);
+ else if (ret == -ETIMEDOUT)
+ ++(adap->stats->timeouts);
+ else
+ ++(adap->stats->bus_errors);
+ } else if (num > 0) {
+ ++(adap->stats->transfers);
+ }
+
+ // Per-address I2C stats
+ // If no errors, incerement the message count per client
+ if (ret >= 0) {
+ for (i = 0; i < ret; ++i) {
+ addr = msgs[i].addr;
+ cs = __get_i2c_client_stats(adap->stats, addr);
+ if (!cs) // For muxed devices, their stats are at the leaf level
+ continue;
+ if (num > 0)
+ ++(cs->messages);
+ }
+ } else { // Otherwise, attribute the error to each of the distinct addresses
+ for (i = 0; i < num; ++i) {
+ for (j = 0; j < i; ++j) { // de-duplicate
+ if (msgs[i].addr == msgs[j].addr)
+ goto done;
+ }
+ cs = __get_i2c_client_stats(adap->stats, msgs[i].addr);
+ if (!cs)
+ continue;
+ if (ret == -ENXIO)
+ ++(cs->nacks);
+ else if (ret == -ETIMEDOUT)
+ ++(cs->timeouts);
+ else
+ ++(cs->bus_errors);
+done:
+ {}
+ }
+ }
return ret;
}
EXPORT_SYMBOL(__i2c_transfer);
@@ -2619,6 +2769,94 @@ void i2c_put_dma_safe_msg_buf(u8 *buf, struct i2c_msg *msg, bool xferred)
}
EXPORT_SYMBOL_GPL(i2c_put_dma_safe_msg_buf);
+static struct i2c_client_stats *__get_i2c_client_stats(struct i2c_adapter_stats *stats,
+ int i2c_addr)
+{
+ int addr;
+ struct rb_node *n, *parent;
+ struct i2c_client_stats_rbnode *cs_node;
+
+ n = stats->i2c_client_stats_root.rb_node;
+ parent = NULL;
+ while (n) {
+ parent = n;
+ cs_node = rb_entry(parent, struct i2c_client_stats_rbnode, rb_node);
+ addr = cs_node->stats->address;
+ if (addr > i2c_addr)
+ n = n->rb_left;
+ else if (addr < i2c_addr)
+ n = n->rb_right;
+ else
+ return cs_node->stats;
+ }
+ return NULL;
+}
+
+static void __insert_i2c_client_stats(struct i2c_adapter_stats *stats,
+ struct i2c_client_stats *client_stats, int i2c_addr)
+{
+ char buf[32];
+ int ret, addr;
+ struct rb_node **link, *parent;
+ struct i2c_client_stats_rbnode *cs_node;
+
+ ret = 1;
+ link = &stats->i2c_client_stats_root.rb_node;
+ parent = NULL;
+ while (*link) {
+ parent = *link;
+ cs_node = rb_entry(parent, struct i2c_client_stats_rbnode, rb_node);
+ addr = cs_node->stats->address;
+ if (addr > i2c_addr)
+ link = &(*link)->rb_left;
+ else if (addr < i2c_addr)
+ link = &(*link)->rb_right;
+ else
+ return; // Already exists
+ }
+
+ cs_node = kzalloc(sizeof(*cs_node), GFP_KERNEL);
+
+ cs_node->stats = client_stats;
+ rb_link_node(&cs_node->rb_node, parent, link);
+ rb_insert_color(&cs_node->rb_node, &stats->i2c_client_stats_root);
+}
+
+static void __remove_i2c_client_stats(struct i2c_adapter_stats *stats, int i2c_addr)
+{
+ int addr;
+ struct rb_node *n, *parent;
+ struct i2c_client_stats_rbnode *cs_node;
+
+ n = stats->i2c_client_stats_root.rb_node;
+ parent = NULL;
+ while (n) {
+ parent = n;
+ cs_node = rb_entry(parent, struct i2c_client_stats_rbnode, rb_node);
+ addr = cs_node->stats->address;
+ if (addr > i2c_addr) {
+ n = n->rb_left;
+ } else if (addr < i2c_addr) {
+ n = n->rb_right;
+ } else {
+ rb_erase(&cs_node->rb_node, &stats->i2c_client_stats_root);
+ return;
+ }
+ }
+}
+
+static int __count_i2c_client_stats(struct i2c_adapter_stats *stats)
+{
+ struct rb_node *n = stats->i2c_client_stats_root.rb_node;
+ int ret = 0;
+
+ while (n) {
+ ++ret;
+ n = rb_next(n);
+ }
+ return ret;
+}
+
MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>");
MODULE_DESCRIPTION("I2C-Bus main module");
MODULE_LICENSE("GPL");
diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 77f576e51652..50141dd42796 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -767,6 +767,100 @@ static void __exit i2c_dev_exit(void)
unregister_chrdev_region(MKDEV(I2C_MAJOR, 0), I2C_MINORS);
}
+static struct i2c_adapter *kobj_to_adapter(struct device *pdev)
+{
+ struct kobject *dev_kobj;
+ struct device *dev;
+
+ dev_kobj = ((struct kobject *)pdev)->parent;
+ dev = container_of(dev_kobj, struct device, kobj);
+ return to_i2c_adapter(dev);
+}
+
+ssize_t bus_errors_show(struct device *pdev,
+ struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "%llu\n", kobj_to_adapter(pdev)->stats->bus_errors);
+}
+DEVICE_ATTR_RO(bus_errors);
+
+ssize_t transfers_show(struct device *pdev,
+ struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "%llu\n", kobj_to_adapter(pdev)->stats->transfers);
+}
+DEVICE_ATTR_RO(transfers);
+
+ssize_t nacks_show(struct device *pdev,
+ struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "%llu\n", kobj_to_adapter(pdev)->stats->nacks);
+}
+DEVICE_ATTR_RO(nacks);
+
+ssize_t recovery_successes_show(struct device *pdev,
+ struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "%llu\n", kobj_to_adapter(pdev)->stats->recovery_successes);
+}
+DEVICE_ATTR_RO(recovery_successes);
+
+ssize_t recovery_failures_show(struct device *pdev,
+ struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "%llu\n", kobj_to_adapter(pdev)->stats->recovery_failures);
+}
+DEVICE_ATTR_RO(recovery_failures);
+
+ssize_t timeouts_show(struct device *pdev,
+ struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "%llu\n", kobj_to_adapter(pdev)->stats->timeouts);
+}
+DEVICE_ATTR_RO(timeouts);
+
+/**
+ * i2c_adapter_create_stats_directory - creates folder for I2C statistics.
+ * @adapter: the i2c_adapter to create the stats directory for.
+ *
+ * Return: 0 if successful, 1 if failed.
+ */
+int i2c_adapter_create_stats_directory(struct i2c_adapter *adapter)
+{
+ struct i2c_adapter_stats *stats;
+ int ret = 1;
+
+ stats = kzalloc(sizeof(*stats), GFP_KERNEL);
+ if (!stats) {
+ adapter->stats = NULL;
+ return ret;
+ }
+ adapter->stats = stats;
+ adapter->stats->kobj = kobject_create_and_add("stats", &adapter->dev.kobj);
+
+ ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_transfers.attr);
+ if (ret)
+ dev_warn(&adapter->dev, "Failed to create sysfs file for tx_complete_cnt\n");
+
+ ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_bus_errors.attr);
+ if (ret)
+ dev_warn(&adapter->dev, "Failed to create sysfs file for bus_errors\n");
+
+ ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_nacks.attr);
+ if (ret)
+ dev_warn(&adapter->dev, "Failed to create sysfs file for nacks\n");
+
+ ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_recovery_successes.attr);
+ if (ret)
+ dev_warn(&adapter->dev, "Failed to create sysfs file for recovery_successes\n");
+
+ ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_recovery_failures.attr);
+ if (ret)
+ dev_warn(&adapter->dev, "Failed to create sysfs file for recovery_failures\n");
+
+ return ret;
+}
+
MODULE_AUTHOR("Frodo Looijaard <frodol@dds.nl>");
MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>");
MODULE_DESCRIPTION("I2C /dev entries driver");
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 3eb60a2e9e61..4547c4c782b6 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -20,6 +20,7 @@
#include <linux/irqdomain.h> /* for Host Notify IRQ */
#include <linux/of.h> /* for struct device_node */
#include <linux/swab.h> /* for swab16 */
+#include <linux/rbtree.h>
#include <uapi/linux/i2c.h>
extern struct bus_type i2c_bus_type;
@@ -297,6 +298,20 @@ struct i2c_driver {
};
#define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
+int i2c_adapter_create_stats_directory(struct i2c_adapter *adapter);
+
+void i2c_adapter_stats_register_counter(struct i2c_adapter *adapter,
+ const char *counter_name, void *data_source);
+
+struct i2c_client_stats {
+ struct kobject kobj;
+ int address;
+ u64 messages;
+ u64 bus_errors;
+ u64 nacks;
+ u64 timeouts;
+};
+
/**
* struct i2c_client - represent an I2C slave device
* @flags: see I2C_CLIENT_* for possible flags
@@ -342,6 +357,7 @@ struct i2c_client {
i2c_slave_cb_t slave_cb; /* callback for slave mode */
#endif
void *devres_group_id; /* ID of probe devres group */
+ struct i2c_client_stats *stats; /* i2c client stats */
};
#define to_i2c_client(d) container_of(d, struct i2c_client, dev)
@@ -684,6 +700,30 @@ struct i2c_adapter_quirks {
u16 max_comb_2nd_msg_len;
};
+/**
+ * I2C adapter statistics
+ */
+struct i2c_adapter_stats {
+ struct kobject *kobj;
+
+ u64 transfers;
+ u64 bus_errors;
+ u64 nacks;
+ u64 recovery_successes;
+ u64 recovery_failures;
+ u64 timeouts;
+
+ struct rb_root i2c_client_stats_root;
+};
+
+struct i2c_client_stats_rbnode {
+ struct i2c_client_stats *stats;
+ struct rb_node rb_node;
+};
+
+struct i2c_client_stats *__get_or_create_i2c_client_stats(struct i2c_adapter_stats *stats,
+ int i2c_addr);
+
/* enforce max_num_msgs = 2 and use max_comb_*_len for length checks */
#define I2C_AQ_COMB BIT(0)
/* first combined message must be write */
@@ -738,6 +778,7 @@ struct i2c_adapter {
struct irq_domain *host_notify_domain;
struct regulator *bus_regulator;
+ struct i2c_adapter_stats *stats;
};
#define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
--
2.37.1.559.g78731f0fdb-goog
^ permalink raw reply related [flat|nested] 2+ messages in thread