[PATCH 1/3] thermal/core: Encapsulate the set_cur_state function

[PATCH 1/3] thermal/core: Encapsulate the set_cur_state function

[Daniel Lezcano]

Concentrate the actions in a single place when a cooling device state
is changed. Provide a function to do that instead of calling the
underlying ops.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.h    |  1 +
 drivers/thermal/thermal_helpers.c | 32 ++++++++++++++++++++++++-------
 drivers/thermal/thermal_sysfs.c   |  7 ++-----
 3 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 726e327b4205..4689f6cf898f 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -65,6 +65,7 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
 		cdev->ops->power2state;
 }
 
+int thermal_cdev_set_state(struct thermal_cooling_device *cdev, int state);
 void thermal_cdev_update(struct thermal_cooling_device *);
 void __thermal_cdev_update(struct thermal_cooling_device *cdev);
 
diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
index 3edd047e144f..d5f162fad1ab 100644
--- a/drivers/thermal/thermal_helpers.c
+++ b/drivers/thermal/thermal_helpers.c
@@ -182,14 +182,32 @@ void thermal_set_delay_jiffies(unsigned long *delay_jiffies, int delay_ms)
 		*delay_jiffies = round_jiffies(*delay_jiffies);
 }
 
-static void thermal_cdev_set_cur_state(struct thermal_cooling_device *cdev,
-				       int target)
+/**
+ * thermal_cdev_set_state - set the cooling device state
+ * @cdev: a pointer to a thermal_cooling_device
+ * @state: the target state
+ *
+ * Set the state of the cooling device passed as parameter. The
+ * cooling device lock must be held when calling this function.
+ *
+ * Return: 0 in case of success, otherwise the return value is the one
+ * returned by the backend for the ops
+ */
+int thermal_cdev_set_state(struct thermal_cooling_device *cdev, int state)
 {
-	if (cdev->ops->set_cur_state(cdev, target))
-		return;
+	int ret;
 
-	thermal_notify_cdev_state_update(cdev->id, target);
-	thermal_cooling_device_stats_update(cdev, target);
+	/*
+	 * No check is needed for the ops->set_cur_state as the
+	 * registering function checked the ops are correctly set
+	 */
+	ret = cdev->ops->set_cur_state(cdev, state);
+	if (!ret) {
+		thermal_notify_cdev_state_update(cdev->id, state);
+		thermal_cooling_device_stats_update(cdev, state);
+	}
+
+	return ret;
 }
 
 void __thermal_cdev_update(struct thermal_cooling_device *cdev)
@@ -207,7 +225,7 @@ void __thermal_cdev_update(struct thermal_cooling_device *cdev)
 			target = instance->target;
 	}
 
-	thermal_cdev_set_cur_state(cdev, target);
+	thermal_cdev_set_state(cdev, target);
 
 	trace_cdev_update(cdev, target);
 	dev_dbg(&cdev->device, "set to state %lu\n", target);
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 1c4aac8464a7..935e79909121 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -617,12 +617,9 @@ cur_state_store(struct device *dev, struct device_attribute *attr,
 		return -EINVAL;
 
 	mutex_lock(&cdev->lock);
-
-	result = cdev->ops->set_cur_state(cdev, state);
-	if (!result)
-		thermal_cooling_device_stats_update(cdev, state);
-
+	result = thermal_cdev_set_state(cdev, state);
 	mutex_unlock(&cdev->lock);
+
 	return result ? result : count;
 }
 
-- 
2.25.1

[PATCH 2/3] thermal/debugfs: Add debugfs information

[Daniel Lezcano]

The thermal framework does not have any debug information except a
sysfs stat which is a bit controversial. This one allocates big chunks
of memory for every cooling devices with a high number of states and
could represent on some systems in production several megabytes of
memory for just a portion of it. As the syfs is limited to a page
size, the output is not exploitable with large data array and gets
truncated.

The patch provides the same information than sysfs except the
transitions are dynamically allocated, thus they won't represent more
events than the ones which actually occured. There is no longer a size
limitation and it opens the field for more debugging information where
the debugfs is designed for, not sysfs.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/Kconfig           |   7 +
 drivers/thermal/Makefile          |   3 +
 drivers/thermal/thermal_core.c    |  10 +
 drivers/thermal/thermal_core.h    |   1 +
 drivers/thermal/thermal_debugfs.c | 362 ++++++++++++++++++++++++++++++
 drivers/thermal/thermal_debugfs.h |  17 ++
 drivers/thermal/thermal_helpers.c |   1 +
 include/linux/thermal.h           |   7 +
 8 files changed, 408 insertions(+)
 create mode 100644 drivers/thermal/thermal_debugfs.c
 create mode 100644 drivers/thermal/thermal_debugfs.h

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 0e5cc948373c..42400c6aed61 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -33,6 +33,13 @@ config THERMAL_STATISTICS
 
 	  If in doubt, say N.
 
+config THERMAL_DEBUGFS
+	bool "Thermal debugging file system"
+	depends on DEBUG_FS
+	help
+	  This option provides a debugfs entry giving useful
+	  information about the thermal framework internals.
+
 config THERMAL_EMERGENCY_POWEROFF_DELAY_MS
 	int "Emergency poweroff delay in milli-seconds"
 	default 0
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index def8e1a0399c..1d498fa1beba 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -10,6 +10,9 @@ thermal_sys-y			+= thermal_core.o thermal_sysfs.o \
 # netlink interface to manage the thermal framework
 thermal_sys-$(CONFIG_THERMAL_NETLINK)		+= thermal_netlink.o
 
+# debugfs interface to investigate the behavior and statistics
+thermal_sys-$(CONFIG_THERMAL_DEBUGFS)	+= thermal_debugfs.o
+
 # interface to/from other layers providing sensors
 thermal_sys-$(CONFIG_THERMAL_HWMON)		+= thermal_hwmon.o
 thermal_sys-$(CONFIG_THERMAL_OF)		+= thermal_of.o
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index cdc0552e8c42..a2d6eb0d0895 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -944,6 +944,8 @@ __thermal_cooling_device_register(struct device_node *np,
 						   THERMAL_EVENT_UNSPECIFIED);
 	mutex_unlock(&thermal_list_lock);
 
+	thermal_debugfs_cdev_register(cdev);
+	
 	return cdev;
 
 out_kfree_type:
@@ -1079,6 +1081,8 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
 	if (!cdev)
 		return;
 
+	thermal_debugfs_cdev_unregister(cdev);
+
 	mutex_lock(&thermal_list_lock);
 	list_for_each_entry(pos, &thermal_cdev_list, node)
 		if (pos == cdev)
@@ -1311,6 +1315,8 @@ thermal_zone_device_register(const char *type, int trips, int mask,
 
 	thermal_notify_tz_create(tz->id, tz->type);
 
+	thermal_debugfs_tz_register(tz);
+
 	return tz;
 
 unregister:
@@ -1340,6 +1346,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 	if (!tz)
 		return;
 
+	thermal_debugfs_tz_unregister(tz);
+	
 	tzp = tz->tzp;
 	tz_id = tz->id;
 
@@ -1485,6 +1493,8 @@ static int __init thermal_init(void)
 		pr_warn("Thermal: Can not register suspend notifier, return %d\n",
 			result);
 
+	thermal_debugfs_init();
+	
 	return 0;
 
 unregister_class:
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 4689f6cf898f..49331b8a5404 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -13,6 +13,7 @@
 #include <linux/thermal.h>
 
 #include "thermal_netlink.h"
+#include "thermal_debugfs.h"
 
 /* Default Thermal Governor */
 #if defined(CONFIG_THERMAL_DEFAULT_GOV_STEP_WISE)
diff --git a/drivers/thermal/thermal_debugfs.c b/drivers/thermal/thermal_debugfs.c
new file mode 100644
index 000000000000..d22558d06da0
--- /dev/null
+++ b/drivers/thermal/thermal_debugfs.c
@@ -0,0 +1,362 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2022 Linaro Limited
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ * Debug filesystem for thermal framework
+ */
+#include <linux/debugfs.h>
+#include <linux/ktime.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/thermal.h>
+
+static struct dentry *rootd;
+static struct dentry *cdevd;
+static struct dentry *tzd;
+
+/*
+ * Length of the string containing the thermal zone id, including the
+ * ending null character. We can reasonably assume there won't be more
+ * than 256 thermal zones as the maximum observed today is around 32.
+ */
+#define IDSLENGTH 4
+
+/*
+ * The cooling device transition list is stored in a hash table where
+ * the size is CDEVSTATS_HASH_SIZE. The majority of cooling devices
+ * have dozen of states but some can have much more, so a hash table
+ * is more adequate in this case because browsing the entire list when
+ * storing the transitions could have a non neglictible cost
+ */
+#define CDEVSTATS_HASH_SIZE 16
+
+struct cdev_value {
+	struct list_head list;
+	int id;
+	u64 value;
+};
+
+struct cdev_debugfs {
+	u32 total;
+	int current_state;
+	ktime_t timestamp;
+	struct mutex lock;
+	struct list_head trans_list[CDEVSTATS_HASH_SIZE];
+	struct list_head duration_list[CDEVSTATS_HASH_SIZE];
+};
+
+struct thermal_debugfs {
+	struct dentry *d_top;
+	union {
+		struct cdev_debugfs cdev;
+	};
+};
+
+void thermal_debugfs_init(void)
+{
+	rootd = debugfs_create_dir("thermal", NULL);
+	if (!rootd)
+		return;
+
+	cdevd = debugfs_create_dir("cooling_devices", rootd);
+	if (!cdevd)
+		return;
+
+	tzd = debugfs_create_dir("thermal_zones", rootd);
+}
+
+static struct thermal_debugfs *thermal_debugfs_add_id(struct dentry *d, int id)
+{
+	struct thermal_debugfs *dfs;
+	char ids[IDSLENGTH];
+
+	dfs = kzalloc(sizeof(*dfs), GFP_KERNEL);
+	if (!dfs)
+		return NULL;
+
+	snprintf(ids, IDSLENGTH, "%d", id);
+
+	dfs->d_top = debugfs_create_dir(ids, d);
+	if (!dfs->d_top) {
+		kfree(dfs);
+		return NULL;
+	}
+
+	return dfs;
+}
+
+static void thermal_debugfs_remove_id(struct thermal_debugfs *dfs)
+{
+	if (!dfs)
+		return;
+
+	debugfs_remove(dfs->d_top);
+	kfree(dfs);
+}
+
+static struct cdev_value *thermal_debugfs_cdev_value_alloc(int id)
+{
+	struct cdev_value *cdev_value;
+
+	cdev_value = kzalloc(sizeof(*cdev_value), GFP_KERNEL);
+	if (cdev_value) {
+		cdev_value->id = id;
+		INIT_LIST_HEAD(&cdev_value->list);
+	}
+
+	return cdev_value;
+}
+
+static struct cdev_value *thermal_debugfs_cdev_value_find(struct thermal_debugfs *dfs,
+							  struct list_head *list, int id)
+{
+	struct cdev_value *pos;
+
+	list_for_each_entry(pos, &list[id % CDEVSTATS_HASH_SIZE], list)
+		if (pos->id == id)
+			return pos;
+
+	return NULL;
+}
+
+static void thermal_debugfs_cdev_value_insert(struct thermal_debugfs *dfs,
+					      struct list_head *list,
+					      struct cdev_value *cdev_value)
+{
+	list_add_tail(&cdev_value->list, &list[cdev_value->id % CDEVSTATS_HASH_SIZE]);
+}
+
+struct cdev_value *thermal_debugfs_cdev_value_get(struct thermal_debugfs *dfs,
+						  struct list_head *list, int id)
+{
+	struct cdev_value *cdev_value;
+
+	cdev_value = thermal_debugfs_cdev_value_find(dfs, list, id);
+	if (cdev_value)
+		return cdev_value;
+
+	cdev_value = thermal_debugfs_cdev_value_alloc(id);
+	if (cdev_value)
+		thermal_debugfs_cdev_value_insert(dfs, list, cdev_value);
+
+	return cdev_value;
+}
+
+static void thermal_debugfs_cdev_reset(struct cdev_debugfs *cfs)
+{
+	int i;
+	struct cdev_value *pos, *tmp;
+
+	for (i = 0; i < CDEVSTATS_HASH_SIZE; i++) {
+
+		list_for_each_entry_safe(pos, tmp, &cfs->trans_list[i], list) {
+			list_del(&pos->list);
+			kfree(pos);
+		}
+
+		list_for_each_entry_safe(pos, tmp, &cfs->duration_list[i], list) {
+			list_del(&pos->list);
+			kfree(pos);
+		}
+	}
+
+	cfs->total = 0;
+}
+
+void thermal_debugfs_cdev_transition(struct thermal_cooling_device *cdev, int to)
+{
+	struct thermal_debugfs *dfs = cdev->debugfs;
+	struct cdev_debugfs *cfs;
+	struct cdev_value *cdev_value;
+	ktime_t now = ktime_get();
+	int transition, from;
+
+	if (!dfs || (dfs->cdev.current_state == to))
+		return;
+
+	cfs = &dfs->cdev;
+
+	mutex_lock(&cfs->lock);
+
+	from = cfs->current_state;
+	cfs->current_state = to;
+	transition = (from << 16) | to;
+
+	cdev_value = thermal_debugfs_cdev_value_get(dfs, cfs->duration_list, from);
+	if (cdev_value) {
+		cdev_value->value += ktime_ms_delta(now, cfs->timestamp);
+		cfs->timestamp = now;
+	}
+
+	cdev_value = thermal_debugfs_cdev_value_get(dfs, cfs->trans_list, transition);
+	if (cdev_value)
+		cdev_value->value++;
+
+	cfs->total++;
+
+	mutex_unlock(&cfs->lock);
+}
+
+static void *cdev_seq_start(struct seq_file *s, loff_t *pos)
+{
+	struct cdev_debugfs *cfs =  s->private;
+
+	mutex_lock(&cfs->lock);
+
+	return (*pos < CDEVSTATS_HASH_SIZE) ? pos : NULL;
+}
+
+static void *cdev_seq_next(struct seq_file *s, void *v, loff_t *pos)
+{
+	(*pos)++;
+
+	return (*pos < CDEVSTATS_HASH_SIZE) ? pos : NULL;
+}
+
+static void cdev_seq_stop(struct seq_file *s, void *v)
+{
+	struct cdev_debugfs *cfs =  s->private;
+
+	mutex_unlock(&cfs->lock);
+}
+
+static int cdev_tt_seq_show(struct seq_file *s, void *v)
+{
+	struct cdev_debugfs *cfs =  s->private;
+	struct list_head *trans_list = cfs->trans_list;
+	struct cdev_value *pos;
+	char buffer[11];
+	int i = *(loff_t *)v;
+
+	if (!i)
+		seq_puts(s, "Transition\tHits\n");
+
+	list_for_each_entry(pos, &trans_list[i], list) {
+
+		snprintf(buffer, ARRAY_SIZE(buffer) - 1, "%d->%d",
+			 pos->id >> 16, pos->id & 0xFFFF);
+
+		seq_printf(s, "%-10s\t%-10llu\n", buffer, pos->value);
+	}
+
+	return 0;
+}
+
+static const struct seq_operations tt_sops = {
+	.start = cdev_seq_start,
+	.next = cdev_seq_next,
+	.stop = cdev_seq_stop,
+	.show = cdev_tt_seq_show,
+};
+
+DEFINE_SEQ_ATTRIBUTE(tt);
+
+static int cdev_dt_seq_show(struct seq_file *s, void *v)
+{
+	struct cdev_debugfs *cfs = s->private;
+	struct list_head *duration_list = cfs->duration_list;
+	struct cdev_value *pos;
+	int i = *(loff_t *)v;
+
+	if (!i)
+		seq_puts(s, "State\tTime\n");
+
+	list_for_each_entry(pos, &duration_list[i], list) {
+		s64 duration = pos->value;
+
+		if (pos->id == cfs->current_state)
+			duration += ktime_ms_delta(ktime_get(), cfs->timestamp);
+
+		seq_printf(s, "%-5d\t%-10llu\n", pos->id, duration);
+	}
+
+	return 0;
+}
+
+static const struct seq_operations dt_sops = {
+	.start = cdev_seq_start,
+	.next = cdev_seq_next,
+	.stop = cdev_seq_stop,
+	.show = cdev_dt_seq_show,
+};
+
+DEFINE_SEQ_ATTRIBUTE(dt);
+
+static int cdev_reset_set(void *data, u64 val)
+{
+	struct cdev_debugfs *cfs = data;
+
+	if (!val)
+		return -EINVAL;
+
+	thermal_debugfs_cdev_reset(cfs);
+
+	return 0;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(cdev_reset_fops, NULL, cdev_reset_set, "%llu\n");
+
+void thermal_debugfs_cdev_register(struct thermal_cooling_device *cdev)
+{
+	struct thermal_debugfs *dfs;
+	struct cdev_debugfs *cfs;
+	int i;
+
+	if (!cdevd)
+		return;
+
+	dfs = thermal_debugfs_add_id(cdevd, cdev->id);
+	if (!dfs)
+		return;
+
+	cfs = &dfs->cdev;
+
+	for (i = 0; i < CDEVSTATS_HASH_SIZE; i++) {
+		INIT_LIST_HEAD(&cfs->trans_list[i]);
+		INIT_LIST_HEAD(&cfs->duration_list[i]);
+	}
+
+	mutex_init(&cfs->lock);
+	cfs->current_state = 0;
+	cfs->timestamp = ktime_get();
+
+	debugfs_create_file("trans_table", 0400, dfs->d_top, cfs, &tt_fops);
+
+	debugfs_create_file("time_in_state_ms", 0400, dfs->d_top, cfs, &dt_fops);
+
+	debugfs_create_file("reset", 0200, dfs->d_top, cfs, &cdev_reset_fops);
+
+	debugfs_create_u32("total_trans", 0400, dfs->d_top, &cfs->total);
+
+	cdev->debugfs = dfs;
+}
+
+void thermal_debugfs_cdev_unregister(struct thermal_cooling_device *cdev)
+{
+	struct thermal_debugfs *dfs = cdev->debugfs;
+
+	if (!dfs)
+		return;
+
+	debugfs_remove(dfs->d_top);
+	thermal_debugfs_cdev_reset(&dfs->cdev);
+	cdev->debugfs = NULL;
+	kfree(dfs);
+}
+
+void thermal_debugfs_tz_register(struct thermal_zone_device *tz)
+{
+	if (!tz)
+		return;
+
+	tz->debugfs = thermal_debugfs_add_id(tzd, tz->id);
+}
+
+void thermal_debugfs_tz_unregister(struct thermal_zone_device *tz)
+{
+	thermal_debugfs_remove_id(tz->debugfs);
+
+	tz->debugfs = NULL;
+}
diff --git a/drivers/thermal/thermal_debugfs.h b/drivers/thermal/thermal_debugfs.h
new file mode 100644
index 000000000000..529f5bda931b
--- /dev/null
+++ b/drivers/thermal/thermal_debugfs.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifdef CONFIG_THERMAL_DEBUGFS
+void thermal_debugfs_init(void);
+void thermal_debugfs_cdev_register(struct thermal_cooling_device *cdev);
+void thermal_debugfs_cdev_unregister(struct thermal_cooling_device *cdev);
+void thermal_debugfs_tz_register(struct thermal_zone_device *tz);
+void thermal_debugfs_tz_unregister(struct thermal_zone_device *tz);
+void thermal_debugfs_cdev_transition(struct thermal_cooling_device *cdev, int state);
+#else
+static inline void thermal_debugfs_init(void);
+static inline void thermal_debugfs_cdev_register(struct thermal_cooling_device *cdev) {}
+static inline void thermal_debugfs_cdev_unregister(struct thermal_cooling_device *cdev) {}
+static inline void thermal_debugfs_tz_register(struct thermal_zone_device *tz) {}
+static inline void thermal_debugfs_tz_unregister(struct thermal_zone_device *tz) {}
+static inline void thermal_debugfs_cdev_transition(struct thermal_cooling_device *cdev, int state) {}
+#endif /* CONFIG_THERMAL_DEBUGFS */
diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
index d5f162fad1ab..040d9e9b55e1 100644
--- a/drivers/thermal/thermal_helpers.c
+++ b/drivers/thermal/thermal_helpers.c
@@ -205,6 +205,7 @@ int thermal_cdev_set_state(struct thermal_cooling_device *cdev, int state)
 	if (!ret) {
 		thermal_notify_cdev_state_update(cdev->id, state);
 		thermal_cooling_device_stats_update(cdev, state);
+		thermal_debugfs_cdev_transition(cdev, state);
 	}
 
 	return ret;
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 365733b428d8..4a69e8a6868e 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -34,6 +34,7 @@
 struct thermal_zone_device;
 struct thermal_cooling_device;
 struct thermal_instance;
+struct thermal_debugfs;
 struct thermal_attr;
 
 enum thermal_trend {
@@ -101,6 +102,9 @@ struct thermal_cooling_device {
 	struct mutex lock; /* protect thermal_instances list */
 	struct list_head thermal_instances;
 	struct list_head node;
+#ifdef CONFIG_THERMAL_DEBUGFS
+	struct thermal_debugfs *debugfs;
+#endif
 };
 
 /**
@@ -174,6 +178,9 @@ struct thermal_zone_device {
 	struct list_head node;
 	struct delayed_work poll_queue;
 	enum thermal_notify_event notify_event;
+#ifdef CONFIG_THERMAL_DEBUGFS
+	struct thermal_debugfs *debugfs;
+#endif
 };
 
 /**
-- 
2.25.1

Re: [PATCH 2/3] thermal/debugfs: Add debugfs information

[kernel test robot]

Hi Daniel,

I love your patch! Yet something to improve:

[auto build test ERROR on rafael-pm/thermal]
[also build test ERROR on v5.18 next-20220601]
[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/intel-lab-lkp/linux/commits/Daniel-Lezcano/thermal-core-Encapsulate-the-set_cur_state-function/20220601-231733
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git thermal
config: x86_64-randconfig-a011 (https://download.01.org/0day-ci/archive/20220602/202206020607.DHohHTFP-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/04c295f7e8b49af742179609949736f6f056b49c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Daniel-Lezcano/thermal-core-Encapsulate-the-set_cur_state-function/20220601-231733
        git checkout 04c295f7e8b49af742179609949736f6f056b49c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

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

All errors (new ones prefixed by >>):

   ld: drivers/thermal/thermal_core.o: in function `thermal_init':
>> drivers/thermal/thermal_core.c:1496: undefined reference to `thermal_debugfs_init'


vim +1496 drivers/thermal/thermal_core.c

  1470	
  1471	static int __init thermal_init(void)
  1472	{
  1473		int result;
  1474	
  1475		result = thermal_netlink_init();
  1476		if (result)
  1477			goto error;
  1478	
  1479		result = thermal_register_governors();
  1480		if (result)
  1481			goto error;
  1482	
  1483		result = class_register(&thermal_class);
  1484		if (result)
  1485			goto unregister_governors;
  1486	
  1487		result = of_parse_thermal_zones();
  1488		if (result)
  1489			goto unregister_class;
  1490	
  1491		result = register_pm_notifier(&thermal_pm_nb);
  1492		if (result)
  1493			pr_warn("Thermal: Can not register suspend notifier, return %d\n",
  1494				result);
  1495	
> 1496		thermal_debugfs_init();

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/3] thermal/debugfs: Add debugfs information

[kernel test robot]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on rafael-pm/thermal]
[also build test WARNING on v5.18 next-20220601]
[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/intel-lab-lkp/linux/commits/Daniel-Lezcano/thermal-core-Encapsulate-the-set_cur_state-function/20220601-231733
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git thermal
config: x86_64-randconfig-a016 (https://download.01.org/0day-ci/archive/20220602/202206020816.0blG5XwC-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c825abd6b0198fb088d9752f556a70705bc99dfd)
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/intel-lab-lkp/linux/commit/04c295f7e8b49af742179609949736f6f056b49c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Daniel-Lezcano/thermal-core-Encapsulate-the-set_cur_state-function/20220601-231733
        git checkout 04c295f7e8b49af742179609949736f6f056b49c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/

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

All warnings (new ones prefixed by >>):

   In file included from drivers/thermal/thermal_core.c:27:
   In file included from drivers/thermal/thermal_core.h:16:
>> drivers/thermal/thermal_debugfs.h:11:20: warning: function 'thermal_debugfs_init' has internal linkage but is not defined [-Wundefined-internal]
   static inline void thermal_debugfs_init(void);
                      ^
   drivers/thermal/thermal_core.c:1496:2: note: used here
           thermal_debugfs_init();
           ^
   1 warning generated.


vim +/thermal_debugfs_init +11 drivers/thermal/thermal_debugfs.h

     2	
     3	#ifdef CONFIG_THERMAL_DEBUGFS
     4	void thermal_debugfs_init(void);
     5	void thermal_debugfs_cdev_register(struct thermal_cooling_device *cdev);
     6	void thermal_debugfs_cdev_unregister(struct thermal_cooling_device *cdev);
     7	void thermal_debugfs_tz_register(struct thermal_zone_device *tz);
     8	void thermal_debugfs_tz_unregister(struct thermal_zone_device *tz);
     9	void thermal_debugfs_cdev_transition(struct thermal_cooling_device *cdev, int state);
    10	#else
  > 11	static inline void thermal_debugfs_init(void);

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/3] thermal/debugfs: Add debugfs information

[kernel test robot]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on rafael-pm/thermal]
[also build test WARNING on v5.18 next-20220602]
[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/intel-lab-lkp/linux/commits/Daniel-Lezcano/thermal-core-Encapsulate-the-set_cur_state-function/20220601-231733
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git thermal
config: i386-randconfig-s001 (https://download.01.org/0day-ci/archive/20220603/202206030425.4QFicgCF-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-14-g5a0004b5-dirty
        # https://github.com/intel-lab-lkp/linux/commit/04c295f7e8b49af742179609949736f6f056b49c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Daniel-Lezcano/thermal-core-Encapsulate-the-set_cur_state-function/20220601-231733
        git checkout 04c295f7e8b49af742179609949736f6f056b49c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash

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


sparse warnings: (new ones prefixed by >>)
   drivers/thermal/thermal_core.c: note: in included file (through drivers/thermal/thermal_core.h):
>> drivers/thermal/thermal_debugfs.h:11:40: sparse: sparse: marked inline, but without a definition

vim +11 drivers/thermal/thermal_debugfs.h

     2	
     3	#ifdef CONFIG_THERMAL_DEBUGFS
     4	void thermal_debugfs_init(void);
     5	void thermal_debugfs_cdev_register(struct thermal_cooling_device *cdev);
     6	void thermal_debugfs_cdev_unregister(struct thermal_cooling_device *cdev);
     7	void thermal_debugfs_tz_register(struct thermal_zone_device *tz);
     8	void thermal_debugfs_tz_unregister(struct thermal_zone_device *tz);
     9	void thermal_debugfs_cdev_transition(struct thermal_cooling_device *cdev, int state);
    10	#else
  > 11	static inline void thermal_debugfs_init(void);

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH 3/3] thermal/sysfs: Remove cooling device sysfs statistics

[Daniel Lezcano]

The statistics are for debugging purpose and belong to debugfs rather
than sysfs. As the previous changes introduced the same statistics in
debugfs, those in sysfs are no longer needed and can be removed.

That solves a couple of problems reported by different SoC vendors:

 - Extra memory used in low profile boards where the memory is limited
   resource

- Truncated information and memory allocation failure with cooling
   devices having a large number of states

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/Kconfig           |   7 -
 drivers/thermal/thermal_core.c    |   2 -
 drivers/thermal/thermal_core.h    |  10 --
 drivers/thermal/thermal_helpers.c |   1 -
 drivers/thermal/thermal_sysfs.c   | 217 ------------------------------
 include/linux/thermal.h           |   1 -
 6 files changed, 238 deletions(-)

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 42400c6aed61..0c7b776462a9 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -26,13 +26,6 @@ config THERMAL_NETLINK
 	  trip point crossed, cooling device update or governor
 	  change. It is recommended to enable the feature.
 
-config THERMAL_STATISTICS
-	bool "Thermal state transition statistics"
-	help
-	  Export thermal state transition statistics information through sysfs.
-
-	  If in doubt, say N.
-
 config THERMAL_DEBUGFS
 	bool "Thermal debugging file system"
 	depends on DEBUG_FS
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index a2d6eb0d0895..4851f2c7910f 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -949,7 +949,6 @@ __thermal_cooling_device_register(struct device_node *np,
 	return cdev;
 
 out_kfree_type:
-	thermal_cooling_device_destroy_sysfs(cdev);
 	kfree(cdev->type);
 	put_device(&cdev->device);
 	cdev = NULL;
@@ -1117,7 +1116,6 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
 
 	ida_simple_remove(&thermal_cdev_ida, cdev->id);
 	device_del(&cdev->device);
-	thermal_cooling_device_destroy_sysfs(cdev);
 	kfree(cdev->type);
 	put_device(&cdev->device);
 }
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 49331b8a5404..dcae52f20258 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -134,22 +134,12 @@ void thermal_set_delay_jiffies(unsigned long *delay_jiffies, int delay_ms);
 int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
 void thermal_zone_destroy_device_groups(struct thermal_zone_device *);
 void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
-void thermal_cooling_device_destroy_sysfs(struct thermal_cooling_device *cdev);
 /* used only at binding time */
 ssize_t trip_point_show(struct device *, struct device_attribute *, char *);
 ssize_t weight_show(struct device *, struct device_attribute *, char *);
 ssize_t weight_store(struct device *, struct device_attribute *, const char *,
 		     size_t);
 
-#ifdef CONFIG_THERMAL_STATISTICS
-void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
-					 unsigned long new_state);
-#else
-static inline void
-thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
-				    unsigned long new_state) {}
-#endif /* CONFIG_THERMAL_STATISTICS */
-
 /* device tree support */
 #ifdef CONFIG_THERMAL_OF
 int of_parse_thermal_zones(void);
diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
index 040d9e9b55e1..81185bb0d526 100644
--- a/drivers/thermal/thermal_helpers.c
+++ b/drivers/thermal/thermal_helpers.c
@@ -204,7 +204,6 @@ int thermal_cdev_set_state(struct thermal_cooling_device *cdev, int state)
 	ret = cdev->ops->set_cur_state(cdev, state);
 	if (!ret) {
 		thermal_notify_cdev_state_update(cdev->id, state);
-		thermal_cooling_device_stats_update(cdev, state);
 		thermal_debugfs_cdev_transition(cdev, state);
 	}
 
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 935e79909121..e48c5ad26611 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -641,231 +641,14 @@ static const struct attribute_group cooling_device_attr_group = {
 
 static const struct attribute_group *cooling_device_attr_groups[] = {
 	&cooling_device_attr_group,
-	NULL, /* Space allocated for cooling_device_stats_attr_group */
 	NULL,
 };
 
-#ifdef CONFIG_THERMAL_STATISTICS
-struct cooling_dev_stats {
-	spinlock_t lock;
-	unsigned int total_trans;
-	unsigned long state;
-	unsigned long max_states;
-	ktime_t last_time;
-	ktime_t *time_in_state;
-	unsigned int *trans_table;
-};
-
-static void update_time_in_state(struct cooling_dev_stats *stats)
-{
-	ktime_t now = ktime_get(), delta;
-
-	delta = ktime_sub(now, stats->last_time);
-	stats->time_in_state[stats->state] =
-		ktime_add(stats->time_in_state[stats->state], delta);
-	stats->last_time = now;
-}
-
-void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
-					 unsigned long new_state)
-{
-	struct cooling_dev_stats *stats = cdev->stats;
-
-	if (!stats)
-		return;
-
-	spin_lock(&stats->lock);
-
-	if (stats->state == new_state)
-		goto unlock;
-
-	update_time_in_state(stats);
-	stats->trans_table[stats->state * stats->max_states + new_state]++;
-	stats->state = new_state;
-	stats->total_trans++;
-
-unlock:
-	spin_unlock(&stats->lock);
-}
-
-static ssize_t total_trans_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	struct thermal_cooling_device *cdev = to_cooling_device(dev);
-	struct cooling_dev_stats *stats = cdev->stats;
-	int ret;
-
-	spin_lock(&stats->lock);
-	ret = sprintf(buf, "%u\n", stats->total_trans);
-	spin_unlock(&stats->lock);
-
-	return ret;
-}
-
-static ssize_t
-time_in_state_ms_show(struct device *dev, struct device_attribute *attr,
-		      char *buf)
-{
-	struct thermal_cooling_device *cdev = to_cooling_device(dev);
-	struct cooling_dev_stats *stats = cdev->stats;
-	ssize_t len = 0;
-	int i;
-
-	spin_lock(&stats->lock);
-	update_time_in_state(stats);
-
-	for (i = 0; i < stats->max_states; i++) {
-		len += sprintf(buf + len, "state%u\t%llu\n", i,
-			       ktime_to_ms(stats->time_in_state[i]));
-	}
-	spin_unlock(&stats->lock);
-
-	return len;
-}
-
-static ssize_t
-reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
-	    size_t count)
-{
-	struct thermal_cooling_device *cdev = to_cooling_device(dev);
-	struct cooling_dev_stats *stats = cdev->stats;
-	int i, states = stats->max_states;
-
-	spin_lock(&stats->lock);
-
-	stats->total_trans = 0;
-	stats->last_time = ktime_get();
-	memset(stats->trans_table, 0,
-	       states * states * sizeof(*stats->trans_table));
-
-	for (i = 0; i < stats->max_states; i++)
-		stats->time_in_state[i] = ktime_set(0, 0);
-
-	spin_unlock(&stats->lock);
-
-	return count;
-}
-
-static ssize_t trans_table_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	struct thermal_cooling_device *cdev = to_cooling_device(dev);
-	struct cooling_dev_stats *stats = cdev->stats;
-	ssize_t len = 0;
-	int i, j;
-
-	len += snprintf(buf + len, PAGE_SIZE - len, " From  :    To\n");
-	len += snprintf(buf + len, PAGE_SIZE - len, "       : ");
-	for (i = 0; i < stats->max_states; i++) {
-		if (len >= PAGE_SIZE)
-			break;
-		len += snprintf(buf + len, PAGE_SIZE - len, "state%2u  ", i);
-	}
-	if (len >= PAGE_SIZE)
-		return PAGE_SIZE;
-
-	len += snprintf(buf + len, PAGE_SIZE - len, "\n");
-
-	for (i = 0; i < stats->max_states; i++) {
-		if (len >= PAGE_SIZE)
-			break;
-
-		len += snprintf(buf + len, PAGE_SIZE - len, "state%2u:", i);
-
-		for (j = 0; j < stats->max_states; j++) {
-			if (len >= PAGE_SIZE)
-				break;
-			len += snprintf(buf + len, PAGE_SIZE - len, "%8u ",
-				stats->trans_table[i * stats->max_states + j]);
-		}
-		if (len >= PAGE_SIZE)
-			break;
-		len += snprintf(buf + len, PAGE_SIZE - len, "\n");
-	}
-
-	if (len >= PAGE_SIZE) {
-		pr_warn_once("Thermal transition table exceeds PAGE_SIZE. Disabling\n");
-		return -EFBIG;
-	}
-	return len;
-}
-
-static DEVICE_ATTR_RO(total_trans);
-static DEVICE_ATTR_RO(time_in_state_ms);
-static DEVICE_ATTR_WO(reset);
-static DEVICE_ATTR_RO(trans_table);
-
-static struct attribute *cooling_device_stats_attrs[] = {
-	&dev_attr_total_trans.attr,
-	&dev_attr_time_in_state_ms.attr,
-	&dev_attr_reset.attr,
-	&dev_attr_trans_table.attr,
-	NULL
-};
-
-static const struct attribute_group cooling_device_stats_attr_group = {
-	.attrs = cooling_device_stats_attrs,
-	.name = "stats"
-};
-
-static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
-{
-	struct cooling_dev_stats *stats;
-	unsigned long states;
-	int var;
-
-	if (cdev->ops->get_max_state(cdev, &states))
-		return;
-
-	states++; /* Total number of states is highest state + 1 */
-
-	var = sizeof(*stats);
-	var += sizeof(*stats->time_in_state) * states;
-	var += sizeof(*stats->trans_table) * states * states;
-
-	stats = kzalloc(var, GFP_KERNEL);
-	if (!stats)
-		return;
-
-	stats->time_in_state = (ktime_t *)(stats + 1);
-	stats->trans_table = (unsigned int *)(stats->time_in_state + states);
-	cdev->stats = stats;
-	stats->last_time = ktime_get();
-	stats->max_states = states;
-
-	spin_lock_init(&stats->lock);
-
-	/* Fill the empty slot left in cooling_device_attr_groups */
-	var = ARRAY_SIZE(cooling_device_attr_groups) - 2;
-	cooling_device_attr_groups[var] = &cooling_device_stats_attr_group;
-}
-
-static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev)
-{
-	kfree(cdev->stats);
-	cdev->stats = NULL;
-}
-
-#else
-
-static inline void
-cooling_device_stats_setup(struct thermal_cooling_device *cdev) {}
-static inline void
-cooling_device_stats_destroy(struct thermal_cooling_device *cdev) {}
-
-#endif /* CONFIG_THERMAL_STATISTICS */
-
 void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *cdev)
 {
-	cooling_device_stats_setup(cdev);
 	cdev->device.groups = cooling_device_attr_groups;
 }
 
-void thermal_cooling_device_destroy_sysfs(struct thermal_cooling_device *cdev)
-{
-	cooling_device_stats_destroy(cdev);
-}
-
 /* these helper will be used only at the time of bindig */
 ssize_t
 trip_point_show(struct device *dev, struct device_attribute *attr, char *buf)
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 4a69e8a6868e..522c9180a08d 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -96,7 +96,6 @@ struct thermal_cooling_device {
 	struct device device;
 	struct device_node *np;
 	void *devdata;
-	void *stats;
 	const struct thermal_cooling_device_ops *ops;
 	bool updated; /* true if the cooling device does not need update */
 	struct mutex lock; /* protect thermal_instances list */
-- 
2.25.1

Re: [PATCH 3/3] thermal/sysfs: Remove cooling device sysfs statistics

[Lukasz Luba]

Hi Daniel,


On 6/1/22 16:14, Daniel Lezcano wrote:
> The statistics are for debugging purpose and belong to debugfs rather
> than sysfs. As the previous changes introduced the same statistics in
> debugfs, those in sysfs are no longer needed and can be removed.

I just want to let you know that in current Android kernels we cannot
even compile the kernel with CONFIG_DEBUG_FS. I have this pain with
Energy Model there... Some vendors might see useful info via this
sysfs interface in bring-up of the SoC.

I don't know if there are user-space tools tracking this
information via sysfs. We probably should check that.

I agree that these statistics look more like debug info, rather than
something useful for control.

Furthermore, we have trace events for the cooling state changes, which
should be good enough for bring-up and experiments.

I don't have strong preferences here. I tend to agree to remove this
interface if there are no user-space tools using it.

Regards,
Lukasz

Re: [PATCH 3/3] thermal/sysfs: Remove cooling device sysfs statistics

[Daniel Lezcano]

Hi Lukasz,

[Adding Todd]

On 01/06/2022 17:33, Lukasz Luba wrote:
> Hi Daniel,
> 
> 
> On 6/1/22 16:14, Daniel Lezcano wrote:
>> The statistics are for debugging purpose and belong to debugfs rather
>> than sysfs. As the previous changes introduced the same statistics in
>> debugfs, those in sysfs are no longer needed and can be removed.
> 
> I just want to let you know that in current Android kernels we cannot
> even compile the kernel with CONFIG_DEBUG_FS.

Right, it makes sense. Precisely, with the sysfs stats they are always 
compiled in for the Android kernel and is a problem for low memory 
systems. While debugfs can fulfill its purpose in the developement and 
will be removed in production systems.

> I have this pain with
> Energy Model there... Some vendors might see useful info via this
> sysfs interface in bring-up of the SoC.

Well alternatively, information can be extracted from procfs in the 
device-tree description.

What prevents to add energy information in sysfs now that the energy 
model is per device ?

> I don't know if there are user-space tools tracking this
> information via sysfs. We probably should check that.
> 
> I agree that these statistics look more like debug info, rather than
> something useful for control.
> 
> Furthermore, we have trace events for the cooling state changes, which
> should be good enough for bring-up and experiments.
> 
> I don't have strong preferences here. I tend to agree to remove this
> interface if there are no user-space tools using it.

I agree userspace can also get information about the transition but the 
goal of the debugfs is also add information about thermal internals like 
average temperature at mitigation time, min and max, timings, etc ...


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 3/3] thermal/sysfs: Remove cooling device sysfs statistics

[Lukasz Luba]

On 6/2/22 09:37, Daniel Lezcano wrote:
> 
> Hi Lukasz,
> 
> [Adding Todd]
> 
> On 01/06/2022 17:33, Lukasz Luba wrote:
>> Hi Daniel,
>>
>>
>> On 6/1/22 16:14, Daniel Lezcano wrote:
>>> The statistics are for debugging purpose and belong to debugfs rather
>>> than sysfs. As the previous changes introduced the same statistics in
>>> debugfs, those in sysfs are no longer needed and can be removed.
>>
>> I just want to let you know that in current Android kernels we cannot
>> even compile the kernel with CONFIG_DEBUG_FS.
> 
> Right, it makes sense. Precisely, with the sysfs stats they are always 
> compiled in for the Android kernel and is a problem for low memory 
> systems. While debugfs can fulfill its purpose in the developement and 
> will be removed in production systems.

True.

> 
>> I have this pain with
>> Energy Model there... Some vendors might see useful info via this
>> sysfs interface in bring-up of the SoC.
> 
> Well alternatively, information can be extracted from procfs in the 
> device-tree description.
> 
> What prevents to add energy information in sysfs now that the energy 
> model is per device ?

Probably nothing, but we need strong need. I have proposed this
a few times internally, but this must have a requirement.
If a user-space tool would ask for it, then I could send a patch
exposing the sysfs. So far we have only one user-space tool, which
suffers the missing debugfs EM dir: LISA (but we are working on a
workaround for it).
If you have a tool or plan to have such, which uses EM, please let
me know. I'm gathering the requirements.

> 
>> I don't know if there are user-space tools tracking this
>> information via sysfs. We probably should check that.
>>
>> I agree that these statistics look more like debug info, rather than
>> something useful for control.
>>
>> Furthermore, we have trace events for the cooling state changes, which
>> should be good enough for bring-up and experiments.
>>
>> I don't have strong preferences here. I tend to agree to remove this
>> interface if there are no user-space tools using it.
> 
> I agree userspace can also get information about the transition but the 
> goal of the debugfs is also add information about thermal internals like 
> average temperature at mitigation time, min and max, timings, etc ...
> 
> 

I see, it makes sense. Let's see if Todd and Android folks don't
use this thermal sysfs stats, so we could remove them.

Re: [PATCH 3/3] thermal/sysfs: Remove cooling device sysfs statistics

[Todd Kjos]

On Thu, Jun 2, 2022 at 2:16 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 6/2/22 09:37, Daniel Lezcano wrote:
> >
> > Hi Lukasz,
> >
> > [Adding Todd]
> >
> > On 01/06/2022 17:33, Lukasz Luba wrote:
> >> Hi Daniel,
> >>
> >>
> >> On 6/1/22 16:14, Daniel Lezcano wrote:
> >>> The statistics are for debugging purpose and belong to debugfs rather
> >>> than sysfs. As the previous changes introduced the same statistics in
> >>> debugfs, those in sysfs are no longer needed and can be removed.
> >>
> >> I just want to let you know that in current Android kernels we cannot
> >> even compile the kernel with CONFIG_DEBUG_FS.
> >
> > Right, it makes sense. Precisely, with the sysfs stats they are always
> > compiled in for the Android kernel and is a problem for low memory
> > systems. While debugfs can fulfill its purpose in the developement and
> > will be removed in production systems.
>
> True.
>
> >
> >> I have this pain with
> >> Energy Model there... Some vendors might see useful info via this
> >> sysfs interface in bring-up of the SoC.
> >
> > Well alternatively, information can be extracted from procfs in the
> > device-tree description.
> >
> > What prevents to add energy information in sysfs now that the energy
> > model is per device ?
>
> Probably nothing, but we need strong need. I have proposed this
> a few times internally, but this must have a requirement.
> If a user-space tool would ask for it, then I could send a patch
> exposing the sysfs. So far we have only one user-space tool, which
> suffers the missing debugfs EM dir: LISA (but we are working on a
> workaround for it).
> If you have a tool or plan to have such, which uses EM, please let
> me know. I'm gathering the requirements.
>
> >
> >> I don't know if there are user-space tools tracking this
> >> information via sysfs. We probably should check that.
> >>
> >> I agree that these statistics look more like debug info, rather than
> >> something useful for control.
> >>
> >> Furthermore, we have trace events for the cooling state changes, which
> >> should be good enough for bring-up and experiments.
> >>
> >> I don't have strong preferences here. I tend to agree to remove this
> >> interface if there are no user-space tools using it.
> >
> > I agree userspace can also get information about the transition but the
> > goal of the debugfs is also add information about thermal internals like
> > average temperature at mitigation time, min and max, timings, etc ...
> >
> >
>
> I see, it makes sense. Let's see if Todd and Android folks don't
> use this thermal sysfs stats, so we could remove them.

Android HALs do use the thermal sysfs stats. debugfs isn't a viable
replacement since debugfs must not be mounted during normal operation.

Re: [PATCH 3/3] thermal/sysfs: Remove cooling device sysfs statistics

[Daniel Lezcano]

Hi Todd,

[adding Wei]

On 02/06/2022 21:02, Todd Kjos wrote:
> On Thu, Jun 2, 2022 at 2:16 AM Lukasz Luba <lukasz.luba@arm.com> wrote:

[ ... ]

>> I see, it makes sense. Let's see if Todd and Android folks don't
>> use this thermal sysfs stats, so we could remove them.
> 
> Android HALs do use the thermal sysfs stats. debugfs isn't a viable
> replacement since debugfs must not be mounted during normal operation.

Thanks for your answer.

I'm curious, what is the purpose of getting the statistics, especially 
the transitions stats from normal operation?

There were some complains about systems having a high number of cooling 
devices with a lot of states. The state transitions are represented as a 
matrix and result in up to hundred of megabytes of memory wasted.

Moreover, sysfs being limited a page size, the output is often truncated.

As it is automatically enabled for GKI, this waste of memory which is 
not negligible for system with low memory can not be avoided.

I went through the thermal HAL but did not find an usage of these 
statistics, do you have a pointer to the code using them ?

Thanks

   -- Daniel



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 3/3] thermal/sysfs: Remove cooling device sysfs statistics

[Wei Wang]

On Fri, Jun 3, 2022 at 4:04 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
>
> Hi Todd,
>
> [adding Wei]
>
> On 02/06/2022 21:02, Todd Kjos wrote:
> > On Thu, Jun 2, 2022 at 2:16 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> [ ... ]
>
> >> I see, it makes sense. Let's see if Todd and Android folks don't
> >> use this thermal sysfs stats, so we could remove them.
> >
> > Android HALs do use the thermal sysfs stats. debugfs isn't a viable
> > replacement since debugfs must not be mounted during normal operation.
>
> Thanks for your answer.
>
> I'm curious, what is the purpose of getting the statistics, especially
> the transitions stats from normal operation?
>
> There were some complains about systems having a high number of cooling
> devices with a lot of states. The state transitions are represented as a
> matrix and result in up to hundred of megabytes of memory wasted.
>
> Moreover, sysfs being limited a page size, the output is often truncated.
>
> As it is automatically enabled for GKI, this waste of memory which is
> not negligible for system with low memory can not be avoided.
>
> I went through the thermal HAL but did not find an usage of these
> statistics, do you have a pointer to the code using them ?
>
> Thanks
>
>    -- Daniel
>
>

Sorry for the late reply, trying to catch up on emails after sick
recovery. We use it for stats collection to understand thermal
residency, and it is not in the HAL code, we don't use the transition
table heavily though. Are some of the devices having too many cooling
devices? Can we have a config to enable stats for a given cooling
device?

Thanks!
-Wei

>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 3/3] thermal/sysfs: Remove cooling device sysfs statistics

[Daniel Lezcano]

On 24/06/2022 08:02, Wei Wang wrote:
> On Fri, Jun 3, 2022 at 4:04 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>>
>> Hi Todd,
>>
>> [adding Wei]
>>
>> On 02/06/2022 21:02, Todd Kjos wrote:
>>> On Thu, Jun 2, 2022 at 2:16 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> [ ... ]
>>
>>>> I see, it makes sense. Let's see if Todd and Android folks don't
>>>> use this thermal sysfs stats, so we could remove them.
>>>
>>> Android HALs do use the thermal sysfs stats. debugfs isn't a viable
>>> replacement since debugfs must not be mounted during normal operation.
>>
>> Thanks for your answer.
>>
>> I'm curious, what is the purpose of getting the statistics, especially
>> the transitions stats from normal operation?
>>
>> There were some complains about systems having a high number of cooling
>> devices with a lot of states. The state transitions are represented as a
>> matrix and result in up to hundred of megabytes of memory wasted.
>>
>> Moreover, sysfs being limited a page size, the output is often truncated.
>>
>> As it is automatically enabled for GKI, this waste of memory which is
>> not negligible for system with low memory can not be avoided.
>>
>> I went through the thermal HAL but did not find an usage of these
>> statistics, do you have a pointer to the code using them ?
>>
>> Thanks
>>
>>     -- Daniel
>>
>>
> 
> Sorry for the late reply, trying to catch up on emails after sick
> recovery. We use it for stats collection to understand thermal
> residency, and it is not in the HAL code, we don't use the transition
> table heavily though. Are some of the devices having too many cooling
> devices? Can we have a config to enable stats for a given cooling
> device?


The stats table is bogus. As soon as the combination of the states leads 
to a size greater than a page size, then the result is truncated.

As the cooling devices is also abused to mimic power capping capable 
device, we are ending up to 1024 states sometimes.

Moreover, having the option set also create tables which take MB of 
memory for nothing.

The option only enables the stats for all the cooling device stats.

As you mentioned, it seems to you are using the stats for debugging 
purpose. The debugfs provides the same information, except it does only 
show the transitions would actually happened and we are no longer 
limited to a page size.

Would it be acceptable to remove the sysfs stats ?



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog