[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 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

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

[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

[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

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

[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 --]