* [RFC Patch v2 0/3] I2C statistics as sysfs attributes
@ 2021-12-03 2:37 ` Sui Chen
0 siblings, 0 replies; 24+ messages in thread
From: Sui Chen @ 2021-12-03 2:37 UTC (permalink / raw)
To: linux-kernel
Cc: benjaminfair, andrew, openbmc, tali.perry1, krellan, linux-i2c,
joe, 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFC Patch v2 1/3] i2c debug counters as sysfs attributes
2021-12-03 2:37 ` Sui Chen
@ 2021-12-03 2:37 ` Sui Chen
-1 siblings, 0 replies; 24+ messages in thread
From: Sui Chen @ 2021-12-03 2:37 UTC (permalink / raw)
To: linux-kernel
Cc: openbmc, linux-i2c, joel, andrew, tali.perry1, benjaminfair,
krellan, joe, 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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFC Patch v2 1/3] i2c debug counters as sysfs attributes
@ 2021-12-03 2:37 ` Sui Chen
0 siblings, 0 replies; 24+ messages in thread
From: Sui Chen @ 2021-12-03 2:37 UTC (permalink / raw)
To: linux-kernel
Cc: benjaminfair, andrew, openbmc, tali.perry1, krellan, linux-i2c,
joe, 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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes
2021-12-03 2:37 ` Sui Chen
@ 2021-12-03 2:50 ` Joe Perches
-1 siblings, 0 replies; 24+ messages in thread
From: Joe Perches @ 2021-12-03 2:50 UTC (permalink / raw)
To: Sui Chen, linux-kernel
Cc: openbmc, linux-i2c, joel, andrew, tali.perry1, benjaminfair, krellan
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes
@ 2021-12-03 2:50 ` Joe Perches
0 siblings, 0 replies; 24+ messages in thread
From: Joe Perches @ 2021-12-03 2:50 UTC (permalink / raw)
To: Sui Chen, linux-kernel
Cc: benjaminfair, andrew, openbmc, tali.perry1, krellan, linux-i2c
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes
2021-12-03 2:50 ` Joe Perches
@ 2021-12-03 5:35 ` Sui Chen
-1 siblings, 0 replies; 24+ messages in thread
From: Sui Chen @ 2021-12-03 5:35 UTC (permalink / raw)
To: Joe Perches, linux-kernel
Cc: openbmc, linux-i2c, joel, andrew, tali.perry1, benjaminfair, krellan
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes
@ 2021-12-03 5:35 ` Sui Chen
0 siblings, 0 replies; 24+ messages in thread
From: Sui Chen @ 2021-12-03 5:35 UTC (permalink / raw)
To: Joe Perches, linux-kernel
Cc: benjaminfair, andrew, openbmc, tali.perry1, krellan, linux-i2c
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes
2021-12-03 2:37 ` Sui Chen
@ 2021-12-03 5:54 ` Joe Perches
-1 siblings, 0 replies; 24+ messages in thread
From: Joe Perches @ 2021-12-03 5:54 UTC (permalink / raw)
To: Sui Chen, linux-kernel
Cc: openbmc, linux-i2c, joel, andrew, tali.perry1, benjaminfair, krellan
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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes
@ 2021-12-03 5:54 ` Joe Perches
0 siblings, 0 replies; 24+ messages in thread
From: Joe Perches @ 2021-12-03 5:54 UTC (permalink / raw)
To: Sui Chen, linux-kernel
Cc: benjaminfair, andrew, openbmc, tali.perry1, krellan, linux-i2c
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes
2021-12-03 2:37 ` Sui Chen
` (2 preceding siblings ...)
(?)
@ 2021-12-03 12:36 ` kernel test robot
-1 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-12-03 12:36 UTC (permalink / raw)
To: kbuild-all
[-- 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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFC Patch v2 1/3] i2c debug counters as sysfs attributes
2021-12-03 2:37 ` Sui Chen
` (3 preceding siblings ...)
(?)
@ 2021-12-03 14:18 ` kernel test robot
-1 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-12-03 14:18 UTC (permalink / raw)
To: kbuild-all
[-- 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFC Patch v2 2/3] i2c: npcm7xx: add tx_complete counter
2021-12-03 2:37 ` Sui Chen
@ 2021-12-03 2:37 ` Sui Chen
-1 siblings, 0 replies; 24+ messages in thread
From: Sui Chen @ 2021-12-03 2:37 UTC (permalink / raw)
To: linux-kernel
Cc: openbmc, linux-i2c, joel, andrew, tali.perry1, benjaminfair,
krellan, joe
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFC Patch v2 2/3] i2c: npcm7xx: add tx_complete counter
@ 2021-12-03 2:37 ` Sui Chen
0 siblings, 0 replies; 24+ messages in thread
From: Sui Chen @ 2021-12-03 2:37 UTC (permalink / raw)
To: linux-kernel
Cc: benjaminfair, andrew, openbmc, tali.perry1, krellan, linux-i2c, joe
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC Patch v2 3/3] add npcm7xx debug counters as sysfs attributes
2021-12-03 2:37 ` Sui Chen
@ 2021-12-03 2:37 ` Sui Chen
-1 siblings, 0 replies; 24+ messages in thread
From: Sui Chen @ 2021-12-03 2:37 UTC (permalink / raw)
To: linux-kernel
Cc: openbmc, linux-i2c, joel, andrew, tali.perry1, benjaminfair,
krellan, joe, 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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFC Patch v2 3/3] add npcm7xx debug counters as sysfs attributes
@ 2021-12-03 2:37 ` Sui Chen
0 siblings, 0 replies; 24+ messages in thread
From: Sui Chen @ 2021-12-03 2:37 UTC (permalink / raw)
To: linux-kernel
Cc: benjaminfair, andrew, openbmc, tali.perry1, krellan, linux-i2c,
joe, 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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [RFC Patch v2 3/3] add npcm7xx debug counters as sysfs attributes
2021-12-03 2:37 ` Sui Chen
(?)
@ 2021-12-03 21:15 ` kernel test robot
-1 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-12-03 21:15 UTC (permalink / raw)
To: kbuild-all
[-- 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC Patch v2 0/3] I2C statistics as sysfs attributes
2021-12-03 2:37 ` Sui Chen
@ 2021-12-03 16:37 ` Wolfram Sang
-1 siblings, 0 replies; 24+ messages in thread
From: Wolfram Sang @ 2021-12-03 16:37 UTC (permalink / raw)
To: Sui Chen
Cc: linux-kernel, openbmc, linux-i2c, joel, andrew, tali.perry1,
benjaminfair, krellan, joe
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC Patch v2 0/3] I2C statistics as sysfs attributes
@ 2021-12-03 16:37 ` Wolfram Sang
0 siblings, 0 replies; 24+ messages in thread
From: Wolfram Sang @ 2021-12-03 16:37 UTC (permalink / raw)
To: Sui Chen
Cc: benjaminfair, andrew, openbmc, linux-kernel, tali.perry1,
krellan, joe, linux-i2c
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC Patch v2 0/3] I2C statistics as sysfs attributes
2021-12-03 16:37 ` Wolfram Sang
(?)
@ 2021-12-07 16:00 ` Sui Chen
2022-01-03 9:44 ` Wolfram Sang
-1 siblings, 1 reply; 24+ messages in thread
From: Sui Chen @ 2021-12-07 16:00 UTC (permalink / raw)
To: Wolfram Sang, linux-kernel, openbmc, linux-i2c, joel, andrew,
tali.perry1, benjaminfair, krellan, joe
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC Patch v2 0/3] I2C statistics as sysfs attributes
2021-12-07 16:00 ` Sui Chen
@ 2022-01-03 9:44 ` Wolfram Sang
0 siblings, 0 replies; 24+ messages in thread
From: Wolfram Sang @ 2022-01-03 9:44 UTC (permalink / raw)
To: Sui Chen
Cc: linux-kernel, openbmc, linux-i2c, joel, andrew, tali.perry1,
benjaminfair, krellan, joe
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC Patch v2 0/3] I2C statistics as sysfs attributes
@ 2022-01-03 9:44 ` Wolfram Sang
0 siblings, 0 replies; 24+ messages in thread
From: Wolfram Sang @ 2022-01-03 9:44 UTC (permalink / raw)
To: Sui Chen
Cc: benjaminfair, andrew, openbmc, linux-kernel, tali.perry1,
krellan, joe, linux-i2c
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread