[RFC Patch v2 0/3] I2C statistics as sysfs attributes

[RFC Patch v2 0/3] I2C statistics as sysfs attributes

[Sui Chen]

Add I2C statistics such as Bus Error counts and NACK counts as sysfs
attributes so they don't need to live in debugfs.

There are a few I2C statistics that are implemented in many I2C
controllers, such as bus error counts and NACK counts. Having those
statistics in sysfs will 1) allow for a unified definition across
various I2C drivers, and 2) make the statistics more ABI-stable and
available on devices with the debugfs disabled.

Overall the patch works as the following way:
1) An I2C statistics sysfs directory is created.
2) Each specific I2C driver is responsible for instantiating the
statistics available.

Test Process:
1. Clone the OpenBMC repository
2. `devtool modify`and apply patch to the linux-nuvoton recipe
3. Build image for quanta-gsj
4. Build QEMU
5. Run the image-bmc image in QEMU

Results:
root@gsj:/sys/class/i2c-adapter/i2c-1/stats# ls
ber_cnt          i2c_speed        nack_cnt         rec_fail_cnt
rec_succ_cnt     timeout_cnt      tx_complete_cnt
root@gsj:/sys/class/i2c-adapter/i2c-1/stats# cat *
0
100000
0
0
0
0
53

Sui Chen (2):
  i2c debug counters as sysfs attributes
  add npcm7xx debug counters as sysfs attributes

Tali Perry (1):
  i2c: npcm7xx: add tx_complete counter

 drivers/i2c/busses/i2c-npcm7xx.c |  13 ++++
 drivers/i2c/i2c-core-base.c      |   2 +
 drivers/i2c/i2c-dev.c            | 103 +++++++++++++++++++++++++++++++
 include/linux/i2c.h              |  27 ++++++++
 4 files changed, 145 insertions(+)

-- 
2.34.0.384.gca35af8252-goog

[RFC Patch v2 0/3] I2C statistics as sysfs attributes

[Sui Chen]

Add I2C statistics such as Bus Error counts and NACK counts as sysfs
attributes so they don't need to live in debugfs.

There are a few I2C statistics that are implemented in many I2C
controllers, such as bus error counts and NACK counts. Having those
statistics in sysfs will 1) allow for a unified definition across
various I2C drivers, and 2) make the statistics more ABI-stable and
available on devices with the debugfs disabled.

Overall the patch works as the following way:
1) An I2C statistics sysfs directory is created.
2) Each specific I2C driver is responsible for instantiating the
statistics available.

Test Process:
1. Clone the OpenBMC repository
2. `devtool modify`and apply patch to the linux-nuvoton recipe
3. Build image for quanta-gsj
4. Build QEMU
5. Run the image-bmc image in QEMU

Results:
root@gsj:/sys/class/i2c-adapter/i2c-1/stats# ls
ber_cnt          i2c_speed        nack_cnt         rec_fail_cnt
rec_succ_cnt     timeout_cnt      tx_complete_cnt
root@gsj:/sys/class/i2c-adapter/i2c-1/stats# cat *
0
100000
0
0
0
0
53

Sui Chen (2):
  i2c debug counters as sysfs attributes
  add npcm7xx debug counters as sysfs attributes

Tali Perry (1):
  i2c: npcm7xx: add tx_complete counter

 drivers/i2c/busses/i2c-npcm7xx.c |  13 ++++
 drivers/i2c/i2c-core-base.c      |   2 +
 drivers/i2c/i2c-dev.c            | 103 +++++++++++++++++++++++++++++++
 include/linux/i2c.h              |  27 ++++++++
 4 files changed, 145 insertions(+)

-- 
2.34.0.384.gca35af8252-goog

[RFC Patch v2 1/3] i2c debug counters as sysfs attributes

[Sui Chen]

This change adds a few example I2C debug counters as sysfs attributes:
- ber_cnt (bus error count)
- nack_cnt (NACK count)
- rec_fail_cnt, rec_succ_cnt (recovery failure/success count)
- timeout_cnt (timeout count)
- i2c_speed (bus frequency)
- tx_complete_cnt (transaction completed, including both as an initiator
  and as a target)

The function i2c_adapter_create_stats_folder creates a stats directory
in the device's sysfs directory to hold the debug counters. The platform
drivers are responsible for instantiating the counters in the stats
directory if applicable.

Signed-off-by: Sui Chen <suichen@google.com>
---
 drivers/i2c/i2c-core-base.c |   2 +
 drivers/i2c/i2c-dev.c       | 103 ++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         |  27 ++++++++++
 3 files changed, 132 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 84f12bf90644a..c42113daf32a7 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1610,6 +1610,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	bus_for_each_drv(&i2c_bus_type, NULL, adap, __process_new_adapter);
 	mutex_unlock(&core_lock);
 
+	i2c_adapter_create_stats_folder(adap);
+
 	return 0;
 
 out_reg:
diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 77f576e516522..75f5e25400fdb 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -767,6 +767,109 @@ 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 kobject*)pdev)->parent;
+	struct device* dev = container_of(dev_kobj, struct device, kobj);
+	return to_i2c_adapter(dev);
+}
+
+static ssize_t ber_cnt_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* ber_cnt = kobj_to_adapter(pdev)->stats->ber_cnt;
+	if (ber_cnt == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *ber_cnt);
+}
+DEVICE_ATTR_RO(ber_cnt);
+
+ssize_t nack_cnt_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* nack_cnt = kobj_to_adapter(pdev)->stats->nack_cnt;
+	if (nack_cnt == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *nack_cnt);
+}
+DEVICE_ATTR_RO(nack_cnt);
+
+ssize_t rec_succ_cnt_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* rec_succ_cnt = kobj_to_adapter(pdev)->stats->rec_succ_cnt;
+	if (rec_succ_cnt == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *rec_succ_cnt);
+}
+DEVICE_ATTR_RO(rec_succ_cnt);
+
+ssize_t rec_fail_cnt_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* rec_fail_cnt = kobj_to_adapter(pdev)->stats->rec_fail_cnt;
+	if (rec_fail_cnt == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *rec_fail_cnt);
+}
+DEVICE_ATTR_RO(rec_fail_cnt);
+
+ssize_t timeout_cnt_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* timeout_cnt = kobj_to_adapter(pdev)->stats->timeout_cnt;
+	if (timeout_cnt == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *timeout_cnt);
+}
+DEVICE_ATTR_RO(timeout_cnt);
+
+ssize_t i2c_speed_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* i2c_speed = kobj_to_adapter(pdev)->stats->i2c_speed;
+	if (i2c_speed == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *i2c_speed);
+}
+DEVICE_ATTR_RO(i2c_speed);
+
+ssize_t tx_complete_cnt_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* tx_complete_cnt = kobj_to_adapter(pdev)->stats->tx_complete_cnt;
+	if (tx_complete_cnt == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *tx_complete_cnt);
+}
+DEVICE_ATTR_RO(tx_complete_cnt);
+
+void i2c_adapter_create_stats_folder(struct i2c_adapter* adapter) {
+	adapter->stats = kzalloc(sizeof(struct i2c_adapter_stats), GFP_KERNEL);
+	adapter->stats->kobj = kobject_create_and_add("stats", &adapter->dev.kobj);;
+}
+
+void i2c_adapter_stats_register_counter(struct i2c_adapter* adapter,
+	const char* counter_name, void* data_source) {
+	int ret;
+	if (adapter->stats == NULL) {
+		i2c_adapter_create_stats_folder(adapter);
+	}
+
+	if (!strcmp(counter_name, "ber_cnt")) {
+		adapter->stats->ber_cnt = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_ber_cnt.attr);
+	} else if (!strcmp(counter_name, "nack_cnt")) {
+		adapter->stats->nack_cnt = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_nack_cnt.attr);
+	} else if (!strcmp(counter_name, "rec_succ_cnt")) {
+		adapter->stats->rec_succ_cnt = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_rec_succ_cnt.attr);
+	} else if (!strcmp(counter_name, "rec_fail_cnt")) {
+		adapter->stats->rec_fail_cnt = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_rec_fail_cnt.attr);
+	} else if (!strcmp(counter_name, "timeout_cnt")) {
+		adapter->stats->timeout_cnt = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_timeout_cnt.attr);
+	} else if (!strcmp(counter_name, "i2c_speed")) {
+		adapter->stats->i2c_speed = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_i2c_speed.attr);
+	} else if (!strcmp(counter_name, "tx_complete_cnt")) {
+		adapter->stats->tx_complete_cnt = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_tx_complete_cnt.attr);
+	}
+
+	if (ret) {
+		printk("Failed to create sysfs file for %s", counter_name);
+	}
+}
+
 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 3eb60a2e9e618..448249eb6ca2e 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -21,6 +21,7 @@
 #include <linux/of.h>		/* for struct device_node */
 #include <linux/swab.h>		/* for swab16 */
 #include <uapi/linux/i2c.h>
+#include <linux/slab.h> /* for kzalloc */
 
 extern struct bus_type i2c_bus_type;
 extern struct device_type i2c_adapter_type;
@@ -684,6 +685,27 @@ struct i2c_adapter_quirks {
 	u16 max_comb_2nd_msg_len;
 };
 
+/**
+ * I2C statistics
+ * The list of statistics are currently copied from npcm7xx.
+ * Perhaps a more universal set of statistics can be used.
+ *
+ * The stats are currently modeled as pointers to members in the bus drivers.
+ * A null pointer indicates the counter is not supported by the bus driver.
+ */
+struct i2c_adapter_stats {
+	struct kobject* kobj;
+
+	/* a NULL value means the counter is not available */
+	u64* tx_complete_cnt;
+	u64* ber_cnt;
+	u64* nack_cnt;
+	u64* rec_succ_cnt;
+	u64* rec_fail_cnt;
+	u64* timeout_cnt;
+	u64* i2c_speed;
+};
+
 /* 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 */
@@ -735,12 +757,17 @@ struct i2c_adapter {
 
 	struct i2c_bus_recovery_info *bus_recovery_info;
 	const struct i2c_adapter_quirks *quirks;
+	struct i2c_adapter_stats* stats;
 
 	struct irq_domain *host_notify_domain;
 	struct regulator *bus_regulator;
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
+void i2c_adapter_create_stats_folder(struct i2c_adapter* adapter);
+void i2c_adapter_stats_register_counter(struct i2c_adapter* adapter,
+	const char* counter_name, void* data_source);
+
 static inline void *i2c_get_adapdata(const struct i2c_adapter *adap)
 {
 	return dev_get_drvdata(&adap->dev);
-- 
2.34.0.384.gca35af8252-goog

[RFC Patch v2 1/3] i2c debug counters as sysfs attributes

[Sui Chen]

This change adds a few example I2C debug counters as sysfs attributes:
- ber_cnt (bus error count)
- nack_cnt (NACK count)
- rec_fail_cnt, rec_succ_cnt (recovery failure/success count)
- timeout_cnt (timeout count)
- i2c_speed (bus frequency)
- tx_complete_cnt (transaction completed, including both as an initiator
  and as a target)

The function i2c_adapter_create_stats_folder creates a stats directory
in the device's sysfs directory to hold the debug counters. The platform
drivers are responsible for instantiating the counters in the stats
directory if applicable.

Signed-off-by: Sui Chen <suichen@google.com>
---
 drivers/i2c/i2c-core-base.c |   2 +
 drivers/i2c/i2c-dev.c       | 103 ++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         |  27 ++++++++++
 3 files changed, 132 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 84f12bf90644a..c42113daf32a7 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1610,6 +1610,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	bus_for_each_drv(&i2c_bus_type, NULL, adap, __process_new_adapter);
 	mutex_unlock(&core_lock);
 
+	i2c_adapter_create_stats_folder(adap);
+
 	return 0;
 
 out_reg:
diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 77f576e516522..75f5e25400fdb 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -767,6 +767,109 @@ 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 kobject*)pdev)->parent;
+	struct device* dev = container_of(dev_kobj, struct device, kobj);
+	return to_i2c_adapter(dev);
+}
+
+static ssize_t ber_cnt_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* ber_cnt = kobj_to_adapter(pdev)->stats->ber_cnt;
+	if (ber_cnt == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *ber_cnt);
+}
+DEVICE_ATTR_RO(ber_cnt);
+
+ssize_t nack_cnt_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* nack_cnt = kobj_to_adapter(pdev)->stats->nack_cnt;
+	if (nack_cnt == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *nack_cnt);
+}
+DEVICE_ATTR_RO(nack_cnt);
+
+ssize_t rec_succ_cnt_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* rec_succ_cnt = kobj_to_adapter(pdev)->stats->rec_succ_cnt;
+	if (rec_succ_cnt == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *rec_succ_cnt);
+}
+DEVICE_ATTR_RO(rec_succ_cnt);
+
+ssize_t rec_fail_cnt_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* rec_fail_cnt = kobj_to_adapter(pdev)->stats->rec_fail_cnt;
+	if (rec_fail_cnt == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *rec_fail_cnt);
+}
+DEVICE_ATTR_RO(rec_fail_cnt);
+
+ssize_t timeout_cnt_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* timeout_cnt = kobj_to_adapter(pdev)->stats->timeout_cnt;
+	if (timeout_cnt == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *timeout_cnt);
+}
+DEVICE_ATTR_RO(timeout_cnt);
+
+ssize_t i2c_speed_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* i2c_speed = kobj_to_adapter(pdev)->stats->i2c_speed;
+	if (i2c_speed == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *i2c_speed);
+}
+DEVICE_ATTR_RO(i2c_speed);
+
+ssize_t tx_complete_cnt_show(struct device* pdev,
+	struct device_attribute* attr, char* buf) {
+	u64* tx_complete_cnt = kobj_to_adapter(pdev)->stats->tx_complete_cnt;
+	if (tx_complete_cnt == NULL) return 0;
+	return sysfs_emit(buf, "%llu\n", *tx_complete_cnt);
+}
+DEVICE_ATTR_RO(tx_complete_cnt);
+
+void i2c_adapter_create_stats_folder(struct i2c_adapter* adapter) {
+	adapter->stats = kzalloc(sizeof(struct i2c_adapter_stats), GFP_KERNEL);
+	adapter->stats->kobj = kobject_create_and_add("stats", &adapter->dev.kobj);;
+}
+
+void i2c_adapter_stats_register_counter(struct i2c_adapter* adapter,
+	const char* counter_name, void* data_source) {
+	int ret;
+	if (adapter->stats == NULL) {
+		i2c_adapter_create_stats_folder(adapter);
+	}
+
+	if (!strcmp(counter_name, "ber_cnt")) {
+		adapter->stats->ber_cnt = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_ber_cnt.attr);
+	} else if (!strcmp(counter_name, "nack_cnt")) {
+		adapter->stats->nack_cnt = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_nack_cnt.attr);
+	} else if (!strcmp(counter_name, "rec_succ_cnt")) {
+		adapter->stats->rec_succ_cnt = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_rec_succ_cnt.attr);
+	} else if (!strcmp(counter_name, "rec_fail_cnt")) {
+		adapter->stats->rec_fail_cnt = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_rec_fail_cnt.attr);
+	} else if (!strcmp(counter_name, "timeout_cnt")) {
+		adapter->stats->timeout_cnt = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_timeout_cnt.attr);
+	} else if (!strcmp(counter_name, "i2c_speed")) {
+		adapter->stats->i2c_speed = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_i2c_speed.attr);
+	} else if (!strcmp(counter_name, "tx_complete_cnt")) {
+		adapter->stats->tx_complete_cnt = data_source;
+		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_tx_complete_cnt.attr);
+	}
+
+	if (ret) {
+		printk("Failed to create sysfs file for %s", counter_name);
+	}
+}
+
 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 3eb60a2e9e618..448249eb6ca2e 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -21,6 +21,7 @@
 #include <linux/of.h>		/* for struct device_node */
 #include <linux/swab.h>		/* for swab16 */
 #include <uapi/linux/i2c.h>
+#include <linux/slab.h> /* for kzalloc */
 
 extern struct bus_type i2c_bus_type;
 extern struct device_type i2c_adapter_type;
@@ -684,6 +685,27 @@ struct i2c_adapter_quirks {
 	u16 max_comb_2nd_msg_len;
 };
 
+/**
+ * I2C statistics
+ * The list of statistics are currently copied from npcm7xx.
+ * Perhaps a more universal set of statistics can be used.
+ *
+ * The stats are currently modeled as pointers to members in the bus drivers.
+ * A null pointer indicates the counter is not supported by the bus driver.
+ */
+struct i2c_adapter_stats {
+	struct kobject* kobj;
+
+	/* a NULL value means the counter is not available */
+	u64* tx_complete_cnt;
+	u64* ber_cnt;
+	u64* nack_cnt;
+	u64* rec_succ_cnt;
+	u64* rec_fail_cnt;
+	u64* timeout_cnt;
+	u64* i2c_speed;
+};
+
 /* 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 */
@@ -735,12 +757,17 @@ struct i2c_adapter {
 
 	struct i2c_bus_recovery_info *bus_recovery_info;
 	const struct i2c_adapter_quirks *quirks;
+	struct i2c_adapter_stats* stats;
 
 	struct irq_domain *host_notify_domain;
 	struct regulator *bus_regulator;
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
+void i2c_adapter_create_stats_folder(struct i2c_adapter* adapter);
+void i2c_adapter_stats_register_counter(struct i2c_adapter* adapter,
+	const char* counter_name, void* data_source);
+
 static inline void *i2c_get_adapdata(const struct i2c_adapter *adap)
 {
 	return dev_get_drvdata(&adap->dev);
-- 
2.34.0.384.gca35af8252-goog

[RFC Patch v2 2/3] i2c: npcm7xx: add tx_complete counter

[Sui Chen]

From: Tali Perry <tali.perry1@gmail.com>

Add tx_copmplete counter which increments when a
valid transaction completes.

Signed-off-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 2ad166355ec9b..0b87706de31d7 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -314,6 +314,7 @@ struct npcm_i2c {
 	u64 rec_fail_cnt;
 	u64 nack_cnt;
 	u64 timeout_cnt;
+	u64 tx_complete_cnt;
 };
 
 static inline void npcm_i2c_select_bank(struct npcm_i2c *bus,
@@ -684,6 +685,8 @@ static void npcm_i2c_callback(struct npcm_i2c *bus,
 	switch (op_status) {
 	case I2C_MASTER_DONE_IND:
 		bus->cmd_err = bus->msgs_num;
+		if (bus->tx_complete_cnt < ULLONG_MAX)
+            bus->tx_complete_cnt++;
 		fallthrough;
 	case I2C_BLOCK_BYTES_ERR_IND:
 		/* Master tx finished and all transmit bytes were sent */
@@ -2223,6 +2226,7 @@ static void npcm_i2c_init_debugfs(struct platform_device *pdev,
 	debugfs_create_u64("rec_succ_cnt", 0444, d, &bus->rec_succ_cnt);
 	debugfs_create_u64("rec_fail_cnt", 0444, d, &bus->rec_fail_cnt);
 	debugfs_create_u64("timeout_cnt", 0444, d, &bus->timeout_cnt);
+	debugfs_create_u64("tx_complete_cnt", 0444, d, &bus->tx_complete_cnt);
 
 	bus->debugfs = d;
 }
-- 
2.34.0.384.gca35af8252-goog

[RFC Patch v2 2/3] i2c: npcm7xx: add tx_complete counter

[Sui Chen]

From: Tali Perry <tali.perry1@gmail.com>

Add tx_copmplete counter which increments when a
valid transaction completes.

Signed-off-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 2ad166355ec9b..0b87706de31d7 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -314,6 +314,7 @@ struct npcm_i2c {
 	u64 rec_fail_cnt;
 	u64 nack_cnt;
 	u64 timeout_cnt;
+	u64 tx_complete_cnt;
 };
 
 static inline void npcm_i2c_select_bank(struct npcm_i2c *bus,
@@ -684,6 +685,8 @@ static void npcm_i2c_callback(struct npcm_i2c *bus,
 	switch (op_status) {
 	case I2C_MASTER_DONE_IND:
 		bus->cmd_err = bus->msgs_num;
+		if (bus->tx_complete_cnt < ULLONG_MAX)
+            bus->tx_complete_cnt++;
 		fallthrough;
 	case I2C_BLOCK_BYTES_ERR_IND:
 		/* Master tx finished and all transmit bytes were sent */
@@ -2223,6 +2226,7 @@ static void npcm_i2c_init_debugfs(struct platform_device *pdev,
 	debugfs_create_u64("rec_succ_cnt", 0444, d, &bus->rec_succ_cnt);
 	debugfs_create_u64("rec_fail_cnt", 0444, d, &bus->rec_fail_cnt);
 	debugfs_create_u64("timeout_cnt", 0444, d, &bus->timeout_cnt);
+	debugfs_create_u64("tx_complete_cnt", 0444, d, &bus->tx_complete_cnt);
 
 	bus->debugfs = d;
 }
-- 
2.34.0.384.gca35af8252-goog

[RFC Patch v2 3/3] add npcm7xx debug counters as sysfs attributes

[Sui Chen]

This change adds npcm7xx debug counters as sysfs attributes using the
i2c_adapter_stats_register_counter function.

Signed-off-by: Sui Chen <suichen@google.com>
Reviewed-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 0b87706de31d7..1268b2d71ca0a 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2228,6 +2228,15 @@ static void npcm_i2c_init_debugfs(struct platform_device *pdev,
 	debugfs_create_u64("timeout_cnt", 0444, d, &bus->timeout_cnt);
 	debugfs_create_u64("tx_complete_cnt", 0444, d, &bus->tx_complete_cnt);
 
+	/* register debug counters in sysfs */
+	i2c_adapter_stats_register_counter(&bus->adap, "ber_cnt", &bus->ber_cnt);
+	i2c_adapter_stats_register_counter(&bus->adap, "nack_cnt", &bus->nack_cnt);
+	i2c_adapter_stats_register_counter(&bus->adap, "rec_succ_cnt", &bus->rec_succ_cnt);
+	i2c_adapter_stats_register_counter(&bus->adap, "rec_fail_cnt", &bus->rec_fail_cnt);
+	i2c_adapter_stats_register_counter(&bus->adap, "timeout_cnt", &bus->timeout_cnt);
+	i2c_adapter_stats_register_counter(&bus->adap, "i2c_speed", &bus->bus_freq);
+	i2c_adapter_stats_register_counter(&bus->adap, "tx_complete_cnt", &bus->tx_complete_cnt);
+
 	bus->debugfs = d;
 }
 
-- 
2.34.0.384.gca35af8252-goog

[RFC Patch v2 3/3] add npcm7xx debug counters as sysfs attributes

[Sui Chen]

This change adds npcm7xx debug counters as sysfs attributes using the
i2c_adapter_stats_register_counter function.

Signed-off-by: Sui Chen <suichen@google.com>
Reviewed-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 0b87706de31d7..1268b2d71ca0a 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2228,6 +2228,15 @@ static void npcm_i2c_init_debugfs(struct platform_device *pdev,
 	debugfs_create_u64("timeout_cnt", 0444, d, &bus->timeout_cnt);
 	debugfs_create_u64("tx_complete_cnt", 0444, d, &bus->tx_complete_cnt);
 
+	/* register debug counters in sysfs */
+	i2c_adapter_stats_register_counter(&bus->adap, "ber_cnt", &bus->ber_cnt);
+	i2c_adapter_stats_register_counter(&bus->adap, "nack_cnt", &bus->nack_cnt);
+	i2c_adapter_stats_register_counter(&bus->adap, "rec_succ_cnt", &bus->rec_succ_cnt);
+	i2c_adapter_stats_register_counter(&bus->adap, "rec_fail_cnt", &bus->rec_fail_cnt);
+	i2c_adapter_stats_register_counter(&bus->adap, "timeout_cnt", &bus->timeout_cnt);
+	i2c_adapter_stats_register_counter(&bus->adap, "i2c_speed", &bus->bus_freq);
+	i2c_adapter_stats_register_counter(&bus->adap, "tx_complete_cnt", &bus->tx_complete_cnt);
+
 	bus->debugfs = d;
 }
 
-- 
2.34.0.384.gca35af8252-goog

Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes

[Joe Perches]

On Thu, 2021-12-02 at 18:37 -0800, Sui Chen wrote:
> This change adds a few example I2C debug counters as sysfs attributes:
> - ber_cnt (bus error count)
> - nack_cnt (NACK count)
> - rec_fail_cnt, rec_succ_cnt (recovery failure/success count)
> - timeout_cnt (timeout count)
> - i2c_speed (bus frequency)
> - tx_complete_cnt (transaction completed, including both as an initiator
>   and as a target)
> 
> The function i2c_adapter_create_stats_folder creates a stats directory
> in the device's sysfs directory to hold the debug counters. The platform
> drivers are responsible for instantiating the counters in the stats
> directory if applicable.

Please try to use scripts/checkpatch.pl on your patches and see if
you should be more 'typical kernel style' compliant.

Ideally, use the --strict option too.

Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes

[Joe Perches]

On Thu, 2021-12-02 at 18:37 -0800, Sui Chen wrote:
> This change adds a few example I2C debug counters as sysfs attributes:
> - ber_cnt (bus error count)
> - nack_cnt (NACK count)
> - rec_fail_cnt, rec_succ_cnt (recovery failure/success count)
> - timeout_cnt (timeout count)
> - i2c_speed (bus frequency)
> - tx_complete_cnt (transaction completed, including both as an initiator
>   and as a target)
> 
> The function i2c_adapter_create_stats_folder creates a stats directory
> in the device's sysfs directory to hold the debug counters. The platform
> drivers are responsible for instantiating the counters in the stats
> directory if applicable.

Please try to use scripts/checkpatch.pl on your patches and see if
you should be more 'typical kernel style' compliant.

Ideally, use the --strict option too.

Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes

[Sui Chen]

On 12/2/21 6:50 PM, Joe Perches wrote:
> On Thu, 2021-12-02 at 18:37 -0800, Sui Chen wrote:
>> This change adds a few example I2C debug counters as sysfs attributes:
>> - ber_cnt (bus error count)
>> - nack_cnt (NACK count)
>> - rec_fail_cnt, rec_succ_cnt (recovery failure/success count)
>> - timeout_cnt (timeout count)
>> - i2c_speed (bus frequency)
>> - tx_complete_cnt (transaction completed, including both as an initiator
>>    and as a target)
>>
>> The function i2c_adapter_create_stats_folder creates a stats directory
>> in the device's sysfs directory to hold the debug counters. The platform
>> drivers are responsible for instantiating the counters in the stats
>> directory if applicable.
> 
> Please try to use scripts/checkpatch.pl on your patches and see if
> you should be more 'typical kernel style' compliant.
> 
> Ideally, use the --strict option too.
> 

Hello Joe,

I thank and really appreciate your spending time commenting on the 
patch, and on its previous version too. I ran checkpatch.pl and found a 
few code style fixes on patches 1 and 2.
Sorry for not checking the format before sending the email, I will 
definitely do the format check next time.

Regarding the patch itself, code style aside, we're wondering if this 
idea of exporting I2C statistics to sysfs looks reasonable? Do we need 
to accompany this change with design documents too (similar to PCIe AER 
reporting?)

We have done some more I2C-related performance and reliability tests; 
however it might take some more efforts to explore those ideas and 
summarize them into patches/documents. For now we would like to know 
about the comments on this sysfs attribute change first, since it is the 
initial step to the larger effort. Any comments will be greatly appreciated.

Thanks,
Sui

Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes

[Sui Chen]

On 12/2/21 6:50 PM, Joe Perches wrote:
> On Thu, 2021-12-02 at 18:37 -0800, Sui Chen wrote:
>> This change adds a few example I2C debug counters as sysfs attributes:
>> - ber_cnt (bus error count)
>> - nack_cnt (NACK count)
>> - rec_fail_cnt, rec_succ_cnt (recovery failure/success count)
>> - timeout_cnt (timeout count)
>> - i2c_speed (bus frequency)
>> - tx_complete_cnt (transaction completed, including both as an initiator
>>    and as a target)
>>
>> The function i2c_adapter_create_stats_folder creates a stats directory
>> in the device's sysfs directory to hold the debug counters. The platform
>> drivers are responsible for instantiating the counters in the stats
>> directory if applicable.
> 
> Please try to use scripts/checkpatch.pl on your patches and see if
> you should be more 'typical kernel style' compliant.
> 
> Ideally, use the --strict option too.
> 

Hello Joe,

I thank and really appreciate your spending time commenting on the 
patch, and on its previous version too. I ran checkpatch.pl and found a 
few code style fixes on patches 1 and 2.
Sorry for not checking the format before sending the email, I will 
definitely do the format check next time.

Regarding the patch itself, code style aside, we're wondering if this 
idea of exporting I2C statistics to sysfs looks reasonable? Do we need 
to accompany this change with design documents too (similar to PCIe AER 
reporting?)

We have done some more I2C-related performance and reliability tests; 
however it might take some more efforts to explore those ideas and 
summarize them into patches/documents. For now we would like to know 
about the comments on this sysfs attribute change first, since it is the 
initial step to the larger effort. Any comments will be greatly appreciated.

Thanks,
Sui

Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes

[Joe Perches]

On Thu, 2021-12-02 at 18:37 -0800, Sui Chen wrote:
> This change adds a few example I2C debug counters as sysfs attributes:
> - ber_cnt (bus error count)
> - nack_cnt (NACK count)
> - rec_fail_cnt, rec_succ_cnt (recovery failure/success count)
> - timeout_cnt (timeout count)
> - i2c_speed (bus frequency)
> - tx_complete_cnt (transaction completed, including both as an initiator
>   and as a target)
> 
> The function i2c_adapter_create_stats_folder creates a stats directory
> in the device's sysfs directory to hold the debug counters. The platform
> drivers are responsible for instantiating the counters in the stats
> directory if applicable.
[]
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
[]
> +void i2c_adapter_create_stats_folder(struct i2c_adapter* adapter) {
> +	adapter->stats = kzalloc(sizeof(struct i2c_adapter_stats), GFP_KERNEL);

unchecked alloc, could fail.

> +	adapter->stats->kobj = kobject_create_and_add("stats", &adapter->dev.kobj);;
> +}
> +
> +void i2c_adapter_stats_register_counter(struct i2c_adapter* adapter,
> +	const char* counter_name, void* data_source) {
> +	int ret;
> +	if (adapter->stats == NULL) {
> +		i2c_adapter_create_stats_folder(adapter);
> +	}

So all of these adapter->stats dereferences could oops.

> +	if (!strcmp(counter_name, "ber_cnt")) {
> +		adapter->stats->ber_cnt = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_ber_cnt.attr);
> +	} else if (!strcmp(counter_name, "nack_cnt")) {
> +		adapter->stats->nack_cnt = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_nack_cnt.attr);
> +	} else if (!strcmp(counter_name, "rec_succ_cnt")) {
> +		adapter->stats->rec_succ_cnt = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_rec_succ_cnt.attr);
> +	} else if (!strcmp(counter_name, "rec_fail_cnt")) {
> +		adapter->stats->rec_fail_cnt = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_rec_fail_cnt.attr);
> +	} else if (!strcmp(counter_name, "timeout_cnt")) {
> +		adapter->stats->timeout_cnt = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_timeout_cnt.attr);
> +	} else if (!strcmp(counter_name, "i2c_speed")) {
> +		adapter->stats->i2c_speed = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_i2c_speed.attr);
> +	} else if (!strcmp(counter_name, "tx_complete_cnt")) {
> +		adapter->stats->tx_complete_cnt = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_tx_complete_cnt.attr);
> +	}

and if none of the strcmp comparisons match, ret is uninitialized.

> +
> +	if (ret) {
> +		printk("Failed to create sysfs file for %s", counter_name);

pr_<level> and should have a terminating newline

Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes

[Joe Perches]

On Thu, 2021-12-02 at 18:37 -0800, Sui Chen wrote:
> This change adds a few example I2C debug counters as sysfs attributes:
> - ber_cnt (bus error count)
> - nack_cnt (NACK count)
> - rec_fail_cnt, rec_succ_cnt (recovery failure/success count)
> - timeout_cnt (timeout count)
> - i2c_speed (bus frequency)
> - tx_complete_cnt (transaction completed, including both as an initiator
>   and as a target)
> 
> The function i2c_adapter_create_stats_folder creates a stats directory
> in the device's sysfs directory to hold the debug counters. The platform
> drivers are responsible for instantiating the counters in the stats
> directory if applicable.
[]
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
[]
> +void i2c_adapter_create_stats_folder(struct i2c_adapter* adapter) {
> +	adapter->stats = kzalloc(sizeof(struct i2c_adapter_stats), GFP_KERNEL);

unchecked alloc, could fail.

> +	adapter->stats->kobj = kobject_create_and_add("stats", &adapter->dev.kobj);;
> +}
> +
> +void i2c_adapter_stats_register_counter(struct i2c_adapter* adapter,
> +	const char* counter_name, void* data_source) {
> +	int ret;
> +	if (adapter->stats == NULL) {
> +		i2c_adapter_create_stats_folder(adapter);
> +	}

So all of these adapter->stats dereferences could oops.

> +	if (!strcmp(counter_name, "ber_cnt")) {
> +		adapter->stats->ber_cnt = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_ber_cnt.attr);
> +	} else if (!strcmp(counter_name, "nack_cnt")) {
> +		adapter->stats->nack_cnt = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_nack_cnt.attr);
> +	} else if (!strcmp(counter_name, "rec_succ_cnt")) {
> +		adapter->stats->rec_succ_cnt = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_rec_succ_cnt.attr);
> +	} else if (!strcmp(counter_name, "rec_fail_cnt")) {
> +		adapter->stats->rec_fail_cnt = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_rec_fail_cnt.attr);
> +	} else if (!strcmp(counter_name, "timeout_cnt")) {
> +		adapter->stats->timeout_cnt = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_timeout_cnt.attr);
> +	} else if (!strcmp(counter_name, "i2c_speed")) {
> +		adapter->stats->i2c_speed = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_i2c_speed.attr);
> +	} else if (!strcmp(counter_name, "tx_complete_cnt")) {
> +		adapter->stats->tx_complete_cnt = data_source;
> +		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_tx_complete_cnt.attr);
> +	}

and if none of the strcmp comparisons match, ret is uninitialized.

> +
> +	if (ret) {
> +		printk("Failed to create sysfs file for %s", counter_name);

pr_<level> and should have a terminating newline

Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]

Hi Sui,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on linux/master linus/master v5.16-rc3 next-20211203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sui-Chen/I2C-statistics-as-sysfs-attributes/20211203-103913
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: arm-randconfig-r011-20211203 (https://download.01.org/0day-ci/archive/20211203/202112032029.jJ4ZTWpT-lkp(a)intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/16ccbe6d6d8b9a6e81ba9f40b70d3af92d04776e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sui-Chen/I2C-statistics-as-sysfs-attributes/20211203-103913
        git checkout 16ccbe6d6d8b9a6e81ba9f40b70d3af92d04776e
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: drivers/i2c/i2c-core-base.o: in function `i2c_register_adapter':
>> i2c-core-base.c:(.text+0x28e2): undefined reference to `i2c_adapter_create_stats_folder'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1981 bytes --]

Hi Sui,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on linux/master linus/master v5.16-rc3 next-20211203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sui-Chen/I2C-statistics-as-sysfs-attributes/20211203-103913
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: arc-nsimosci_defconfig (https://download.01.org/0day-ci/archive/20211203/202112032229.MtipbaGi-lkp(a)intel.com/config)
compiler: arc-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/16ccbe6d6d8b9a6e81ba9f40b70d3af92d04776e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sui-Chen/I2C-statistics-as-sysfs-attributes/20211203-103913
        git checkout 16ccbe6d6d8b9a6e81ba9f40b70d3af92d04776e
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arc-elf-ld: drivers/i2c/i2c-core-base.o: in function `i2c_register_adapter':
   i2c-core-base.c:(.text+0x1cbe): undefined reference to `i2c_adapter_create_stats_folder'
>> arc-elf-ld: i2c-core-base.c:(.text+0x1cbe): undefined reference to `i2c_adapter_create_stats_folder'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [RFC Patch v2 0/3] I2C statistics as sysfs attributes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 296 bytes --]

On Thu, Dec 02, 2021 at 06:37:25PM -0800, Sui Chen wrote:
> Add I2C statistics such as Bus Error counts and NACK counts as sysfs
> attributes so they don't need to live in debugfs.

What has changed since v1?

From a glimpse, none of my questions to v1 have been answered or
addressed?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC Patch v2 0/3] I2C statistics as sysfs attributes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 296 bytes --]

On Thu, Dec 02, 2021 at 06:37:25PM -0800, Sui Chen wrote:
> Add I2C statistics such as Bus Error counts and NACK counts as sysfs
> attributes so they don't need to live in debugfs.

What has changed since v1?

From a glimpse, none of my questions to v1 have been answered or
addressed?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC Patch v2 3/3] add npcm7xx debug counters as sysfs attributes

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]

Hi Sui,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on linux/master linus/master v5.16-rc3 next-20211203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sui-Chen/I2C-statistics-as-sysfs-attributes/20211203-103913
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: arc-buildonly-randconfig-r005-20211203 (https://download.01.org/0day-ci/archive/20211204/202112040522.ItSpbl40-lkp(a)intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/a4ab6f0c06f5a80876ca7f818e77e2df31f1b8da
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sui-Chen/I2C-statistics-as-sysfs-attributes/20211203-103913
        git checkout a4ab6f0c06f5a80876ca7f818e77e2df31f1b8da
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arceb-elf-ld: drivers/i2c/i2c-core-base.o: in function `i2c_register_adapter':
   i2c-core-base.c:(.text+0x24de): undefined reference to `i2c_adapter_create_stats_folder'
   arceb-elf-ld: i2c-core-base.c:(.text+0x24de): undefined reference to `i2c_adapter_create_stats_folder'
   arceb-elf-ld: drivers/i2c/busses/i2c-npcm7xx.o: in function `npcm_i2c_init_debugfs':
>> i2c-npcm7xx.c:(.text+0x29a): undefined reference to `i2c_adapter_stats_register_counter'
>> arceb-elf-ld: i2c-npcm7xx.c:(.text+0x29a): undefined reference to `i2c_adapter_stats_register_counter'
   arceb-elf-ld: i2c-npcm7xx.c:(.text+0x2a8): undefined reference to `i2c_adapter_stats_register_counter'
   arceb-elf-ld: i2c-npcm7xx.c:(.text+0x2a8): undefined reference to `i2c_adapter_stats_register_counter'
   arceb-elf-ld: i2c-npcm7xx.c:(.text+0x2b6): undefined reference to `i2c_adapter_stats_register_counter'
   arceb-elf-ld: drivers/i2c/busses/i2c-npcm7xx.o:i2c-npcm7xx.c:(.text+0x2b6): more undefined references to `i2c_adapter_stats_register_counter' follow

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4459 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20211203023728.3699610-2-suichen@google.com>
References: <20211203023728.3699610-2-suichen@google.com>
TO: Sui Chen <suichen@google.com>

Hi Sui,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on linux/master linus/master v5.16-rc3 next-20211203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sui-Chen/I2C-statistics-as-sysfs-attributes/20211203-103913
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: x86_64-randconfig-m001-20211203 (https://download.01.org/0day-ci/archive/20211205/202112050811.jZxEi6SN-lkp(a)intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/i2c/i2c-dev.c:866 i2c_adapter_stats_register_counter() error: uninitialized symbol 'ret'.

vim +/ret +866 drivers/i2c/i2c-dev.c

16ccbe6d6d8b9a Sui Chen 2021-12-02  835  
16ccbe6d6d8b9a Sui Chen 2021-12-02  836  void i2c_adapter_stats_register_counter(struct i2c_adapter* adapter,
16ccbe6d6d8b9a Sui Chen 2021-12-02  837  	const char* counter_name, void* data_source) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  838  	int ret;
16ccbe6d6d8b9a Sui Chen 2021-12-02  839  	if (adapter->stats == NULL) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  840  		i2c_adapter_create_stats_folder(adapter);
16ccbe6d6d8b9a Sui Chen 2021-12-02  841  	}
16ccbe6d6d8b9a Sui Chen 2021-12-02  842  
16ccbe6d6d8b9a Sui Chen 2021-12-02  843  	if (!strcmp(counter_name, "ber_cnt")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  844  		adapter->stats->ber_cnt = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  845  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_ber_cnt.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  846  	} else if (!strcmp(counter_name, "nack_cnt")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  847  		adapter->stats->nack_cnt = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  848  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_nack_cnt.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  849  	} else if (!strcmp(counter_name, "rec_succ_cnt")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  850  		adapter->stats->rec_succ_cnt = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  851  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_rec_succ_cnt.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  852  	} else if (!strcmp(counter_name, "rec_fail_cnt")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  853  		adapter->stats->rec_fail_cnt = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  854  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_rec_fail_cnt.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  855  	} else if (!strcmp(counter_name, "timeout_cnt")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  856  		adapter->stats->timeout_cnt = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  857  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_timeout_cnt.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  858  	} else if (!strcmp(counter_name, "i2c_speed")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  859  		adapter->stats->i2c_speed = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  860  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_i2c_speed.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  861  	} else if (!strcmp(counter_name, "tx_complete_cnt")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  862  		adapter->stats->tx_complete_cnt = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  863  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_tx_complete_cnt.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  864  	}
16ccbe6d6d8b9a Sui Chen 2021-12-02  865  
16ccbe6d6d8b9a Sui Chen 2021-12-02 @866  	if (ret) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  867  		printk("Failed to create sysfs file for %s", counter_name);
16ccbe6d6d8b9a Sui Chen 2021-12-02  868  	}
16ccbe6d6d8b9a Sui Chen 2021-12-02  869  }
16ccbe6d6d8b9a Sui Chen 2021-12-02  870  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 3827 bytes --]

Hi Sui,

url:    https://github.com/0day-ci/linux/commits/Sui-Chen/I2C-statistics-as-sysfs-attributes/20211203-103913
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: x86_64-randconfig-m001-20211203 (https://download.01.org/0day-ci/archive/20211205/202112050811.jZxEi6SN-lkp(a)intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/i2c/i2c-dev.c:866 i2c_adapter_stats_register_counter() error: uninitialized symbol 'ret'.

vim +/ret +866 drivers/i2c/i2c-dev.c

16ccbe6d6d8b9a Sui Chen 2021-12-02  836  void i2c_adapter_stats_register_counter(struct i2c_adapter* adapter,
16ccbe6d6d8b9a Sui Chen 2021-12-02  837  	const char* counter_name, void* data_source) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  838  	int ret;
16ccbe6d6d8b9a Sui Chen 2021-12-02  839  	if (adapter->stats == NULL) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  840  		i2c_adapter_create_stats_folder(adapter);
16ccbe6d6d8b9a Sui Chen 2021-12-02  841  	}
16ccbe6d6d8b9a Sui Chen 2021-12-02  842  
16ccbe6d6d8b9a Sui Chen 2021-12-02  843  	if (!strcmp(counter_name, "ber_cnt")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  844  		adapter->stats->ber_cnt = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  845  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_ber_cnt.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  846  	} else if (!strcmp(counter_name, "nack_cnt")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  847  		adapter->stats->nack_cnt = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  848  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_nack_cnt.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  849  	} else if (!strcmp(counter_name, "rec_succ_cnt")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  850  		adapter->stats->rec_succ_cnt = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  851  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_rec_succ_cnt.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  852  	} else if (!strcmp(counter_name, "rec_fail_cnt")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  853  		adapter->stats->rec_fail_cnt = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  854  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_rec_fail_cnt.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  855  	} else if (!strcmp(counter_name, "timeout_cnt")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  856  		adapter->stats->timeout_cnt = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  857  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_timeout_cnt.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  858  	} else if (!strcmp(counter_name, "i2c_speed")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  859  		adapter->stats->i2c_speed = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  860  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_i2c_speed.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  861  	} else if (!strcmp(counter_name, "tx_complete_cnt")) {
16ccbe6d6d8b9a Sui Chen 2021-12-02  862  		adapter->stats->tx_complete_cnt = data_source;
16ccbe6d6d8b9a Sui Chen 2021-12-02  863  		ret = sysfs_create_file(adapter->stats->kobj, &dev_attr_tx_complete_cnt.attr);
16ccbe6d6d8b9a Sui Chen 2021-12-02  864  	}

No else path.

16ccbe6d6d8b9a Sui Chen 2021-12-02  865  
16ccbe6d6d8b9a Sui Chen 2021-12-02 @866  	if (ret) {
                                                    ^^^
16ccbe6d6d8b9a Sui Chen 2021-12-02  867  		printk("Failed to create sysfs file for %s", counter_name);
16ccbe6d6d8b9a Sui Chen 2021-12-02  868  	}
16ccbe6d6d8b9a Sui Chen 2021-12-02  869  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [RFC Patch v2 0/3] I2C statistics as sysfs attributes

[Sui Chen]

On 12/3/21 8:37 AM, Wolfram Sang wrote:
> On Thu, Dec 02, 2021 at 06:37:25PM -0800, Sui Chen wrote:
>> Add I2C statistics such as Bus Error counts and NACK counts as sysfs
>> attributes so they don't need to live in debugfs.
>
> What has changed since v1?
>
>  From a glimpse, none of my questions to v1 have been answered or
> addressed?
>

Apologies for missing the first email, I double-checked my mailbox
and saw it. Difference in v2 is not much, with just the
tx_complete_cnt added.

Before answering the questions please let me give some
background info:

The motivation starts with monitoring the operation of BMCs and then
applying what we learned to monitoring BMC health at scale. The BMCs
we're interested in run the OpenBMC distribution, which is characterized
by its extensive use of DBus APIs in the sensor stack and everywhere
else in the system. We had been working on some tools centered around
DBus and had discovered issues on certain systems by looking at related
metrics.

A snapshot of the data we look into on a running OpenBMC system may
look like the following (a screenshot from one of the tools we're
working on):

+-------dbus-top v0.xx-----------------------------------------+
|Message Type          | msg/s |Method Call Time (us) Histogram|
|Method Call            282.91 |  1387-:                       |
|Method Return          282.91 |   227-::.                     |
|Signal                  56.98 |    37-::::::              ::  |
|Error                    0.00 |     6-::::::: ..::....::..::: |
|Total                  622.79 |1%-99% 57.00          23899.00 |
+-------------------------------------------------------+------+
|History     (Total msg/s)                              |
|-                                           -700       |
|-                                          :-525       |
|-                                     .  : :-350       |
|-                         .  ::       :  : :-175       |
|-                     .:::::::::::::::::::::-0         |
+-------------------------------------------------------+---+
| Columns 1-3 of 6                           200 sensors    |
|   mobo_pch.......   Core_xx_CPUX...   Core_xx_CPUX      > |
|   mobo_pch.......   Core_xx_CPUX...   Core_xx_CPUX      > |
|   mobo_pch.......   Core_xx_CPUX      Core_XX_CPUX      > |
|   mobo_pch.......   Core_xx_CPUX      Core_XX_CPUX ..   > |
|   cpuX_tem.......   Core_xx_CPUX      Core_XX_CPUX      > |
*************************************************************
* Destination     Sender I2C Tx/s     Sender CMD            *
* systemd         n/a                 /usr/bin/cpusensor    *
* :1.78           n/a                 (unknown)             *
* systemd         172.44              (unknown)             *
* systemd         n/a                 /usr/bin/externalsenso*
* systemd         658.28              /usr/bin/fansensor    *
* systemd         167.94              /usr/bin/hwmontempsens*
* systemd         10345.07            /usr/bin/psusensor    *
* :1.15           n/a                 ipmid                 *
* :1.59           n/a                 ipmid                 *
* systemd         n/a                 ipmid                 *
* :1.67           n/a                 /usr/libexec/kcsbridg *
* :1.26166        n/a                 (unknown)             *
* systemd         n/a                 (unknown)             *
*************************************************************

As one can see from the figure above, there are a few hundred DBus
messages flowing through different DBus peers on the system at
any given moment, with a few daemons reading sensor data via I2C and
publishing them on DBus. We observe I2C to be a large chunk of the work
the BMC is constantly doing, and actually it has been the source of a
few problems we had seen and fixed.
By the time we started  this tool, we have had enough experience
and confidence to say that I2C would be interesting for both development
and monitoring at-scale and is worth investing in.

The requirement for a stable API/ABI is mostly relevant to the
"at-scale" monitoring part. Here are the answers to the questions:

> 1) Why do you need this information? I don't think values like NACK
> count describe the health of a system. NACKs are perfectly OK in
> regular I2C communication. So, for now, I think debugfs is the better
> place. An exception might be the bus speed. Some people already had an
> interest in this.

- Because we're targeting at-scale monitoring of fleets of machines
   in large numbers with identical hardware configuration, the
   I2C counters (including NACK) can be used for apples-to-apples
   comparison, for detecting anomaly, etc., while this may not be
   applicable to a single machine.

> 2) Even when this information is kept in debugfs, we can still add
> some core helper to have a standardized structure for the created
> files. This is independent from sysfs. I don't think I want this a
> standardized ABI, currently. Unless you explain good reasons to me

- The monitoring is done in a production environment, so it is not a
   "debug" usage. Android 11 removed support for debugfs [1] due to
   unstable and undocumented API, code quality and vulnerability issues.
   We would like to follow a similar rationale for the I2C counters.

- debugfs paths may be vendor-specific, so a monitoring framework would
   need different code paths for different vendors without a stable
   method for obtaining those counters. This may be resolved by the core
   helpers, but our intention is to not use debugfs if possible.

- A monitoring system includes not only lower levels such as the kernel,
   but also higher-level standards such as Redfish, and also the OpenBMC-
   wide DBus interfaces and C++ bindings. Redfish has already committed
   to creating a new Schema that contains similar metrics; the Schemas
   is expected to be stable for a considerable amount of time. OpenBMC is
   considering adding its system-wide DBus interfaces for the I2C
   definitions to reflect the relatively stable Redfish bindings too.
   Therefore we kindly hope that the kernel interface for I2Cs can
   be similarly stable just like the other two.

For the errors found in the testing, and other potential errors in the
code: we will look into them and correct them.

Hope this may be useful,
Thanks
Sui

[1] https://source.android.com/setup/start/android-11-release#debugfs

Re: [RFC Patch v2 0/3] I2C statistics as sysfs attributes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]


> - Because we're targeting at-scale monitoring of fleets of machines
>    in large numbers with identical hardware configuration, the
>    I2C counters (including NACK) can be used for apples-to-apples
>    comparison, for detecting anomaly, etc., while this may not be
>    applicable to a single machine.

Okay, I buy this argument. Let's work on this during the next cycle.
From a glimpse, we need some refactoring, like moving stuff away from
i2c-dev into the core and probably add a generic stats-struct which can
be attached to i2c_adapter. But I'll respond to that in more detail to
v3 when I have a better picture.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC Patch v2 0/3] I2C statistics as sysfs attributes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]


> - Because we're targeting at-scale monitoring of fleets of machines
>    in large numbers with identical hardware configuration, the
>    I2C counters (including NACK) can be used for apples-to-apples
>    comparison, for detecting anomaly, etc., while this may not be
>    applicable to a single machine.

Okay, I buy this argument. Let's work on this during the next cycle.
From a glimpse, we need some refactoring, like moving stuff away from
i2c-dev into the core and probably add a generic stats-struct which can
be attached to i2c_adapter. But I'll respond to that in more detail to
v3 when I have a better picture.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]