[PATCH 0/7] thermal: enhancements on thermal stats

[PATCH 0/7] thermal: enhancements on thermal stats

[Eduardo Valentin]

Hello Rafael and Daniel

After a long hiatus, I am returning to more frequent contributions
to the thermal subsystems, as least until I drain some of the
commits I have in my trees.

This is a first series of several that will come as improvements
on the thermal subsystem that will enable using this subsystem
in the Baseboard Management Controller (BMC) space, as part
of the Nitro BMC project. To do so, there were a few improvements
and new features wrote.

In this series in particular, I present a set of enhancements
on how we are handling statistics. The cooling device stats
are awesome, but I added a few new entries there. I also
introduce stats per thermal zone here too.

I tried to keep documentation as current as possible.
I may have missed a thing or two, so please help me out here.
Testing/Examples are in each code.

Let me know any feeback,

BR,

Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
Cc: linux-pm@vger.kernel.org (open list:THERMAL)
Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
Cc: linux-kernel@vger.kernel.org (open list)

Eduardo Valentin (7):
  thermal: stats: track time each dev changes due to tz
  thermal: stats: track number of change requests due to tz
  thermal: stats: introduce thermal zone stats/ directory
  thermal: stats: introduce thermal zone stats/min_gradient
  thermal: stats: introduce tz time in trip
  ythermal: core: report errors to governors
  thermal: stats: add error accounting to thermal zone

 .../driver-api/thermal/sysfs-api.rst          |  10 +
 drivers/thermal/thermal_core.c                |  15 +-
 drivers/thermal/thermal_core.h                |  16 +
 drivers/thermal/thermal_helpers.c             |  11 +-
 drivers/thermal/thermal_sysfs.c               | 495 +++++++++++++++++-
 include/linux/thermal.h                       |   5 +
 6 files changed, 539 insertions(+), 13 deletions(-)

-- 
2.34.1

[PATCH 1/7] thermal: stats: track time each dev changes due to tz

[Eduardo Valentin]

From: Eduardo Valentin <eduval@amazon.com>

This patch improves the current cooling device
statistics by adding a new file under
cdev/stats/time_in_thermal_zone_ms
to represent the time in milliseconds
that the cooling device was driven by each
associated thermal zone.

The file format is:
thermal_zone: <type> <time_in_ms>

Samples:
$ cat /sys/class/thermal/cooling_device0/stats/time_in_thermal_zone_ms
thermal_zone: amb0	117496

Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
Cc: linux-pm@vger.kernel.org (open list:THERMAL)
Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
Cc: linux-kernel@vger.kernel.org (open list)

Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
 .../driver-api/thermal/sysfs-api.rst          |   2 +
 drivers/thermal/thermal_core.c                |   2 +-
 drivers/thermal/thermal_core.h                |   5 +
 drivers/thermal/thermal_helpers.c             |  11 +-
 drivers/thermal/thermal_sysfs.c               | 128 +++++++++++++++++-
 5 files changed, 139 insertions(+), 9 deletions(-)

diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
index 6c1175c6afba..caa50d61a5bc 100644
--- a/Documentation/driver-api/thermal/sysfs-api.rst
+++ b/Documentation/driver-api/thermal/sysfs-api.rst
@@ -367,6 +367,8 @@ Thermal cooling device sys I/F, created once it's registered::
     |---stats/time_in_state_ms:	Time (msec) spent in various cooling states
     |---stats/total_trans:	Total number of times cooling state is changed
     |---stats/trans_table:	Cooling state transition table
+    |---stats/time_in_thermal_zone_ms:	Time that this cooling device was driven
+                                each associated thermal zone.
 
 
 Then next two dynamic attributes are created/removed in pairs. They represent
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 842f678c1c3e..4bb77af6a6f4 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1078,7 +1078,7 @@ void thermal_cooling_device_update(struct thermal_cooling_device *cdev)
 	if (cdev->ops->get_cur_state(cdev, &state) || state > cdev->max_state)
 		goto unlock;
 
-	thermal_cooling_device_stats_update(cdev, state);
+	thermal_cooling_device_stats_update(cdev, NULL, state);
 
 unlock:
 	mutex_unlock(&cdev->lock);
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 3d4a787c6b28..3cce60c6e065 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -102,6 +102,9 @@ struct thermal_instance {
 	struct list_head cdev_node; /* node in cdev->thermal_instances */
 	unsigned int weight; /* The weight of the cooling device */
 	bool upper_no_limit;
+#if IS_ENABLED(CONFIG_THERMAL_STATISTICS)
+	ktime_t time_in; /* time spent in this instance */
+#endif
 };
 
 #define to_thermal_zone(_dev) \
@@ -137,10 +140,12 @@ ssize_t weight_store(struct device *, struct device_attribute *, const char *,
 
 #ifdef CONFIG_THERMAL_STATISTICS
 void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
+					 struct thermal_instance *instance,
 					 unsigned long new_state);
 #else
 static inline void
 thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
+				    struct thermal_instance *instance,
 				    unsigned long new_state) {}
 #endif /* CONFIG_THERMAL_STATISTICS */
 
diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
index cfba0965a22d..ec8e86394977 100644
--- a/drivers/thermal/thermal_helpers.c
+++ b/drivers/thermal/thermal_helpers.c
@@ -149,18 +149,19 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
 EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
 
 static void thermal_cdev_set_cur_state(struct thermal_cooling_device *cdev,
+				       struct thermal_instance *target_instance,
 				       int target)
 {
 	if (cdev->ops->set_cur_state(cdev, target))
 		return;
 
 	thermal_notify_cdev_state_update(cdev->id, target);
-	thermal_cooling_device_stats_update(cdev, target);
+	thermal_cooling_device_stats_update(cdev, target_instance, target);
 }
 
 void __thermal_cdev_update(struct thermal_cooling_device *cdev)
 {
-	struct thermal_instance *instance;
+	struct thermal_instance *instance, *target_instance = NULL;
 	unsigned long target = 0;
 
 	/* Make sure cdev enters the deepest cooling state */
@@ -169,11 +170,13 @@ void __thermal_cdev_update(struct thermal_cooling_device *cdev)
 			instance->tz->id, instance->target);
 		if (instance->target == THERMAL_NO_TARGET)
 			continue;
-		if (instance->target > target)
+		if (instance->target > target) {
 			target = instance->target;
+			target_instance = instance;
+		}
 	}
 
-	thermal_cdev_set_cur_state(cdev, target);
+	thermal_cdev_set_cur_state(cdev, target_instance, 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 6c20c9f90a05..a3b71f03db75 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -632,7 +632,7 @@ cur_state_store(struct device *dev, struct device_attribute *attr,
 
 	result = cdev->ops->set_cur_state(cdev, state);
 	if (!result)
-		thermal_cooling_device_stats_update(cdev, state);
+		thermal_cooling_device_stats_update(cdev, NULL, state);
 
 	mutex_unlock(&cdev->lock);
 	return result ? result : count;
@@ -661,6 +661,7 @@ static const struct attribute_group *cooling_device_attr_groups[] = {
 };
 
 #ifdef CONFIG_THERMAL_STATISTICS
+/* thermal cooling device statistics handling */
 struct cooling_dev_stats {
 	spinlock_t lock;
 	unsigned int total_trans;
@@ -668,9 +669,29 @@ struct cooling_dev_stats {
 	ktime_t last_time;
 	ktime_t *time_in_state;
 	unsigned int *trans_table;
+	struct thermal_instance *last_instance;
+	struct thermal_instance *curr_instance;
 };
 
-static void update_time_in_state(struct cooling_dev_stats *stats)
+static void update_time_in_instance(struct cooling_dev_stats *stats,
+				    struct thermal_instance *instance,
+				    ktime_t now, ktime_t delta)
+{
+	if (!instance)
+		return;
+
+	stats->last_instance = stats->curr_instance;
+	stats->curr_instance = instance;
+
+	if (!stats->last_instance)
+		stats->last_instance = instance;
+
+	stats->last_instance->time_in =
+			ktime_add(stats->last_instance->time_in, delta);
+}
+
+static void update_time_in_state(struct cooling_dev_stats *stats,
+				 struct thermal_instance *instance)
 {
 	ktime_t now = ktime_get(), delta;
 
@@ -678,9 +699,12 @@ static void update_time_in_state(struct cooling_dev_stats *stats)
 	stats->time_in_state[stats->state] =
 		ktime_add(stats->time_in_state[stats->state], delta);
 	stats->last_time = now;
+
+	update_time_in_instance(stats, instance, now, delta);
 }
 
 void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
+					 struct thermal_instance *instance,
 					 unsigned long new_state)
 {
 	struct cooling_dev_stats *stats = cdev->stats;
@@ -695,7 +719,7 @@ void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
 	if (stats->state == new_state)
 		goto unlock;
 
-	update_time_in_state(stats);
+	update_time_in_state(stats, instance);
 	stats->trans_table[stats->state * (cdev->max_state + 1) + new_state]++;
 	stats->state = new_state;
 	stats->total_trans++;
@@ -744,7 +768,7 @@ time_in_state_ms_show(struct device *dev, struct device_attribute *attr,
 
 	spin_lock(&stats->lock);
 
-	update_time_in_state(stats);
+	update_time_in_state(stats, stats->curr_instance);
 
 	for (i = 0; i <= cdev->max_state; i++) {
 		len += sprintf(buf + len, "state%u\t%llu\n", i,
@@ -758,12 +782,98 @@ time_in_state_ms_show(struct device *dev, struct device_attribute *attr,
 	return len;
 }
 
+struct cdev_thermal_zone_residency {
+	char thermal_zone[THERMAL_NAME_LENGTH];
+	ktime_t time_in;
+	unsigned long counter;
+	struct list_head node; /* we build this as we go */
+};
+
+static void
+build_cdev_thermal_zone_residency(struct list_head *list,
+				  struct thermal_cooling_device *cdev)
+{
+	struct cdev_thermal_zone_residency *res, *update_res;
+	struct thermal_instance *instance;
+
+	/*
+	 * Build an array of pairs <thermal zone, time> to represent
+	 * how this cooling device was driven by each thermal zone
+	 */
+	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
+		update_res = NULL;
+
+		list_for_each_entry(res, list, node) {
+			if (strncmp(res->thermal_zone, instance->tz->type,
+				    THERMAL_NAME_LENGTH) == 0)
+				update_res = res;
+		}
+		if (!update_res) {
+			update_res = kzalloc(sizeof(*update_res), GFP_KERNEL);
+			strscpy(update_res->thermal_zone,
+				instance->tz->type, THERMAL_NAME_LENGTH);
+			list_add_tail(&update_res->node, list);
+		}
+	}
+}
+
+static ssize_t
+time_in_thermal_zone_ms_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	LIST_HEAD(cdev_thermal_zone_list);
+	struct thermal_cooling_device *cdev = to_cooling_device(dev);
+	struct cooling_dev_stats *stats = cdev->stats;
+	struct cdev_thermal_zone_residency *res, *next;
+	struct thermal_instance *instance;
+	ssize_t len = 0, ret = 0;
+
+	mutex_lock(&cdev->lock);
+
+	spin_lock(&stats->lock);
+	update_time_in_state(stats, stats->curr_instance);
+	spin_unlock(&stats->lock);
+
+	build_cdev_thermal_zone_residency(&cdev_thermal_zone_list, cdev);
+
+	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node)
+		list_for_each_entry(res, &cdev_thermal_zone_list, node)
+			if (strncmp(res->thermal_zone, instance->tz->type,
+				    THERMAL_NAME_LENGTH) == 0)
+				res->time_in = ktime_add(res->time_in,
+							 instance->time_in);
+
+	mutex_unlock(&cdev->lock);
+
+	list_for_each_entry_safe(res, next, &cdev_thermal_zone_list, node) {
+		ret = snprintf(buf + len, PAGE_SIZE - len,
+			       "thermal_zone: %s\t%llu\n",
+			       res->thermal_zone, ktime_to_ms(res->time_in));
+
+		if (ret == 0)
+			ret = -EOVERFLOW;
+
+		if (ret < 0)
+			break;
+
+		len += ret;
+	}
+
+	list_for_each_entry_safe(res, next, &cdev_thermal_zone_list, node) {
+		list_del(&res->node);
+		kfree(res);
+	}
+
+	return ret < 0 ? ret : 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;
+	struct thermal_instance *instance;
 	int i, states;
 
 	mutex_lock(&cdev->lock);
@@ -774,6 +884,7 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
 
 	states = cdev->max_state + 1;
 
+	mutex_lock(&cdev->lock);
 	spin_lock(&stats->lock);
 
 	stats->total_trans = 0;
@@ -784,7 +895,14 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
 	for (i = 0; i < states; i++)
 		stats->time_in_state[i] = ktime_set(0, 0);
 
+	/* Make sure we reset all counters per instance */
+	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
+		instance->time_in = ktime_set(0, 0);
+	}
+
 	spin_unlock(&stats->lock);
+	mutex_unlock(&cdev->lock);
+
 
 unlock:
 	mutex_unlock(&cdev->lock);
@@ -852,12 +970,14 @@ static ssize_t trans_table_show(struct device *dev,
 
 static DEVICE_ATTR_RO(total_trans);
 static DEVICE_ATTR_RO(time_in_state_ms);
+static DEVICE_ATTR_RO(time_in_thermal_zone_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_time_in_thermal_zone_ms.attr,
 	&dev_attr_reset.attr,
 	&dev_attr_trans_table.attr,
 	NULL
-- 
2.34.1

[PATCH 2/7] thermal: stats: track number of change requests due to tz

[Eduardo Valentin]

From: Eduardo Valentin <eduval@amazon.com>

This patch improves the current cooling device
statistics by adding a new file under
cdev/stats/requests_of_thermal_zone

to represent the number of times each thermal zone
requested the cooling device to effectively change.
If the request associated was not serviced because
another thermal zone asked for a higher cooling level,
this counter does not increase.

The file format is:
thermal_zone: <type> <count>

Samples:
$ cat cdev0/stats/requests_of_thermal_zone
thermal_zone: amb0	2

In this example, it means the thermal zone 'amb0' has requested
2 times for cdev0 to change state.

Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
Cc: linux-pm@vger.kernel.org (open list:THERMAL)
Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
Cc: linux-kernel@vger.kernel.org (open list)

Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
 .../driver-api/thermal/sysfs-api.rst          |  2 +
 drivers/thermal/thermal_core.h                |  1 +
 drivers/thermal/thermal_sysfs.c               | 52 +++++++++++++++++++
 3 files changed, 55 insertions(+)

diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
index caa50d61a5bc..75309a51d9b3 100644
--- a/Documentation/driver-api/thermal/sysfs-api.rst
+++ b/Documentation/driver-api/thermal/sysfs-api.rst
@@ -369,6 +369,8 @@ Thermal cooling device sys I/F, created once it's registered::
     |---stats/trans_table:	Cooling state transition table
     |---stats/time_in_thermal_zone_ms:	Time that this cooling device was driven
                                 each associated thermal zone.
+    |---stats/requests_of_thermal_zone:	Total number of times this cooling device
+                                changed due to each associated thermal zone.
 
 
 Then next two dynamic attributes are created/removed in pairs. They represent
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 3cce60c6e065..ed6511c3b794 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -103,6 +103,7 @@ struct thermal_instance {
 	unsigned int weight; /* The weight of the cooling device */
 	bool upper_no_limit;
 #if IS_ENABLED(CONFIG_THERMAL_STATISTICS)
+	unsigned long total_requests;
 	ktime_t time_in; /* time spent in this instance */
 #endif
 };
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index a3b71f03db75..0bce1415f7e8 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -723,6 +723,7 @@ void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
 	stats->trans_table[stats->state * (cdev->max_state + 1) + new_state]++;
 	stats->state = new_state;
 	stats->total_trans++;
+	stats->curr_instance->total_requests++;
 
 unlock:
 	spin_unlock(&stats->lock);
@@ -867,6 +868,54 @@ time_in_thermal_zone_ms_show(struct device *dev, struct device_attribute *attr,
 	return ret < 0 ? ret : len;
 }
 
+static ssize_t
+requests_of_thermal_zone_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	LIST_HEAD(cdev_thermal_zone_list);
+	struct thermal_cooling_device *cdev = to_cooling_device(dev);
+	struct cooling_dev_stats *stats = cdev->stats;
+	struct cdev_thermal_zone_residency *res, *next;
+	struct thermal_instance *instance;
+	ssize_t len = 0, ret = 0;
+
+	mutex_lock(&cdev->lock);
+
+	spin_lock(&stats->lock);
+	update_time_in_state(stats, stats->curr_instance);
+	spin_unlock(&stats->lock);
+
+	build_cdev_thermal_zone_residency(&cdev_thermal_zone_list, cdev);
+
+	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node)
+		list_for_each_entry(res, &cdev_thermal_zone_list, node)
+			if (strncmp(res->thermal_zone, instance->tz->type,
+				    THERMAL_NAME_LENGTH) == 0)
+				res->counter += instance->total_requests;
+
+	mutex_unlock(&cdev->lock);
+
+	list_for_each_entry_safe(res, next, &cdev_thermal_zone_list, node) {
+		ret = sprintf(buf + len, "thermal_zone: %s\t%lu\n",
+			      res->thermal_zone, res->counter);
+
+		if (ret == 0)
+			ret = -EOVERFLOW;
+
+		if (ret < 0)
+			break;
+
+		len += ret;
+	}
+
+	list_for_each_entry_safe(res, next, &cdev_thermal_zone_list, node) {
+		list_del(&res->node);
+		kfree(res);
+	}
+
+	return ret < 0 ? ret : len;
+}
+
 static ssize_t
 reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
 	    size_t count)
@@ -897,6 +946,7 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
 
 	/* Make sure we reset all counters per instance */
 	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
+		instance->total_requests = 0;
 		instance->time_in = ktime_set(0, 0);
 	}
 
@@ -971,6 +1021,7 @@ static ssize_t trans_table_show(struct device *dev,
 static DEVICE_ATTR_RO(total_trans);
 static DEVICE_ATTR_RO(time_in_state_ms);
 static DEVICE_ATTR_RO(time_in_thermal_zone_ms);
+static DEVICE_ATTR_RO(requests_of_thermal_zone);
 static DEVICE_ATTR_WO(reset);
 static DEVICE_ATTR_RO(trans_table);
 
@@ -978,6 +1029,7 @@ static struct attribute *cooling_device_stats_attrs[] = {
 	&dev_attr_total_trans.attr,
 	&dev_attr_time_in_state_ms.attr,
 	&dev_attr_time_in_thermal_zone_ms.attr,
+	&dev_attr_requests_of_thermal_zone.attr,
 	&dev_attr_reset.attr,
 	&dev_attr_trans_table.attr,
 	NULL
-- 
2.34.1

[PATCH 3/7] thermal: stats: introduce thermal zone stats/ directory

[Eduardo Valentin]

From: Eduardo Valentin <eduval@amazon.com>

This patch adds a new sysfs interface to report
statistics of each thermal zone. This interface
follows the existing thermal statistics for cooling
devices design, in which it is provided a reset
mechanism. The patch adds a statistic to track
the maximum gradient (dT/dt) to the thermal zone
stats/ folder.

Sample, how gradient changes using emulation:

$ cat stats/max_gradient
0
$ echo 2000 > emul_temp
$ cat temp
2000
$ cat stats/max_gradient
2849
$ cat temp
2000

Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
Cc: linux-pm@vger.kernel.org (open list:THERMAL)
Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
Cc: linux-kernel@vger.kernel.org (open list)

Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
 .../driver-api/thermal/sysfs-api.rst          |   3 +
 drivers/thermal/thermal_core.c                |   1 +
 drivers/thermal/thermal_core.h                |   3 +
 drivers/thermal/thermal_sysfs.c               | 142 +++++++++++++++++-
 include/linux/thermal.h                       |   2 +
 5 files changed, 147 insertions(+), 4 deletions(-)

diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
index 75309a51d9b3..18140dbb1ce1 100644
--- a/Documentation/driver-api/thermal/sysfs-api.rst
+++ b/Documentation/driver-api/thermal/sysfs-api.rst
@@ -355,6 +355,9 @@ Thermal zone device sys I/F, created once it's registered::
     |---integral_cutoff:        Offset above which errors are accumulated
     |---slope:                  Slope constant applied as linear extrapolation
     |---offset:                 Offset constant applied as linear extrapolation
+    |---stats:			Directory containing thermal zone device's stats
+    |---stats/reset_tz_stats:	Writes to this file resets the statistics.
+    |---stats/max_gradient:	The maximum recorded dT/dt in uC/ms.
 
 Thermal cooling device sys I/F, created once it's registered::
 
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 4bb77af6a6f4..3ba970c0744f 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -385,6 +385,7 @@ static void update_temperature(struct thermal_zone_device *tz)
 
 	tz->last_temperature = tz->temperature;
 	tz->temperature = temp;
+	thermal_zone_device_stats_update(tz);
 
 	trace_thermal_temperature(tz);
 
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index ed6511c3b794..ef37b92bbb7c 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -140,10 +140,13 @@ ssize_t weight_store(struct device *, struct device_attribute *, const char *,
 		     size_t);
 
 #ifdef CONFIG_THERMAL_STATISTICS
+void thermal_zone_device_stats_update(struct thermal_zone_device *tz);
 void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
 					 struct thermal_instance *instance,
 					 unsigned long new_state);
 #else
+static inline
+void thermal_zone_device_stats_update(struct thermal_zone_device *tz) {}
 static inline void
 thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
 				    struct thermal_instance *instance,
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 0bce1415f7e8..aa28c1cae916 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -537,20 +537,147 @@ static void destroy_trip_attrs(struct thermal_zone_device *tz)
 	kfree(tz->trips_attribute_group.attrs);
 }
 
+#ifdef CONFIG_THERMAL_STATISTICS
+/* thermal zone device statistics handling */
+struct thermal_zone_device_stats {
+	spinlock_t lock; /* protects this struct */
+	s64 max_gradient;
+	ktime_t last_time;
+};
+
+#define DELTA_MILLI_C_TO_MICRO_C(t0, t1)		(((t0) - (t1)) * 1000)
+static void temperature_stats_update(struct thermal_zone_device *tz)
+{
+	struct thermal_zone_device_stats *stats = tz->stats;
+	ktime_t now = ktime_get(), delta;
+	s64 cur_gradient, delta_temp;
+
+	delta = ktime_sub(now, stats->last_time);
+	stats->last_time = now;
+
+	/* to see sub 1C/s, use uC/ms */
+	delta_temp = DELTA_MILLI_C_TO_MICRO_C(tz->temperature,
+					      tz->last_temperature);
+	cur_gradient = ktime_to_ms(delta);
+	if (cur_gradient)
+		cur_gradient = div64_s64(delta_temp, cur_gradient);
+
+	/* Corner case of initialization, first temp read */
+	if (tz->last_temperature == 0)
+		cur_gradient = 0;
+
+	/* update fastest temperature rise from our perspective */
+	if (cur_gradient > stats->max_gradient)
+		stats->max_gradient = cur_gradient;
+}
+
+void thermal_zone_device_stats_update(struct thermal_zone_device *tz)
+{
+	struct thermal_zone_device_stats *stats = tz->stats;
+
+	spin_lock(&stats->lock);
+	temperature_stats_update(tz);
+	spin_unlock(&stats->lock);
+}
+
+static ssize_t max_gradient_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	struct thermal_zone_device_stats *stats = tz->stats;
+	int ret;
+
+	spin_lock(&stats->lock);
+	temperature_stats_update(tz);
+	ret = snprintf(buf, PAGE_SIZE, "%lld\n", stats->max_gradient);
+	spin_unlock(&stats->lock);
+
+	return ret;
+}
+
+static ssize_t
+reset_tz_stats_store(struct device *dev, struct device_attribute *attr,
+		     const char *buf, size_t count)
+{
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	struct thermal_zone_device_stats *stats = tz->stats;
+
+	spin_lock(&stats->lock);
+
+	stats->max_gradient = 0;
+	stats->last_time = ktime_get();
+
+	spin_unlock(&stats->lock);
+
+	return count;
+}
+
+static DEVICE_ATTR_RO(max_gradient);
+static DEVICE_ATTR_WO(reset_tz_stats);
+
+static struct attribute *thermal_zone_device_stats_attrs[] = {
+	&dev_attr_max_gradient.attr,
+	&dev_attr_reset_tz_stats.attr,
+	NULL
+};
+
+static const struct attribute_group thermal_zone_device_stats_attr_group = {
+	.attrs = thermal_zone_device_stats_attrs,
+	.name = "stats"
+};
+
+static void
+thermal_zone_device_stats_setup(struct thermal_zone_device *tz,
+				const struct attribute_group **group)
+{
+	struct thermal_zone_device_stats *stats;
+	int var;
+
+	var = sizeof(*stats);
+
+	stats = kzalloc(var, GFP_KERNEL);
+	if (!stats)
+		return;
+
+	stats->last_time = ktime_get();
+	spin_lock_init(&stats->lock);
+	tz->stats = stats;
+
+	/* Fill the empty slot left in thermal_zone_device_attr_groups */
+	*group = &thermal_zone_device_stats_attr_group;
+}
+
+static void thermal_zone_device_stats_destroy(struct thermal_zone_device *tz)
+{
+	kfree(tz->stats);
+	tz->stats = NULL;
+}
+#else
+
+static inline void
+thermal_zone_device_stats_setup(struct thermal_zone_device *tz,
+				const struct attribute_group **group) {}
+static inline void
+thermal_zone_device_stats_destroy(struct thermal_zone_device *tz) {}
+
+#endif
+
 int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
 				      int mask)
 {
 	const struct attribute_group **groups;
-	int i, size, result;
+	int i, size, result, extras = 2;
 
+	if (IS_ENABLED(CONFIG_THERMAL_STATISTIC))
+		extras++;
 	/* we need one extra for trips and the NULL to terminate the array */
-	size = ARRAY_SIZE(thermal_zone_attribute_groups) + 2;
+	size = ARRAY_SIZE(thermal_zone_attribute_groups) + extras;
 	/* This also takes care of API requirement to be NULL terminated */
 	groups = kcalloc(size, sizeof(*groups), GFP_KERNEL);
 	if (!groups)
 		return -ENOMEM;
 
-	for (i = 0; i < size - 2; i++)
+	for (i = 0; i < size - extras; i++)
 		groups[i] = thermal_zone_attribute_groups[i];
 
 	if (tz->num_trips) {
@@ -561,9 +688,15 @@ int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
 			return result;
 		}
 
-		groups[size - 2] = &tz->trips_attribute_group;
+		groups[size - extras] = &tz->trips_attribute_group;
 	}
 
+	/*
+	 * This will be nop if CONFIG_THERMAL_STATISTICS is not defined,
+	 * otherwise, it is always the last entry in the group prior to NULL
+	 */
+	thermal_zone_device_stats_setup(tz, &groups[size - 2]);
+
 	tz->device.groups = groups;
 
 	return 0;
@@ -578,6 +711,7 @@ void thermal_zone_destroy_device_groups(struct thermal_zone_device *tz)
 		destroy_trip_attrs(tz);
 
 	kfree(tz->device.groups);
+	thermal_zone_device_stats_destroy(tz);
 }
 
 /* sys I/F for cooling device */
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 87837094d549..9dc8292f0314 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -121,6 +121,7 @@ struct thermal_cooling_device {
  * @trip_type_attrs:	attributes for trip points for sysfs: trip type
  * @trip_hyst_attrs:	attributes for trip points for sysfs: trip hysteresis
  * @mode:		current mode of this thermal zone
+ * @stats:		private pointer to stats collected on this thermal zone
  * @devdata:	private pointer for device private data
  * @trips:	an array of struct thermal_trip
  * @num_trips:	number of trip points the thermal zone supports
@@ -162,6 +163,7 @@ struct thermal_zone_device {
 	struct thermal_attr *trip_type_attrs;
 	struct thermal_attr *trip_hyst_attrs;
 	enum thermal_device_mode mode;
+	void *stats;
 	void *devdata;
 	struct thermal_trip *trips;
 	int num_trips;
-- 
2.34.1

[PATCH 4/7] thermal: stats: introduce thermal zone stats/min_gradient

[Eduardo Valentin]

From: Eduardo Valentin <eduval@amazon.com>

The patch adds a statistic to track
the minimum gradient (dT/dt) to the thermal zone
stats/ folder.

Samples:

$ echo 1000 > emul_temp
$ cat stats/min_gradient
0
$ echo 2000 > emul_temp
$ echo 1000 > emul_temp
$ cat stats/min_gradient
-3460

Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
Cc: linux-pm@vger.kernel.org (open list:THERMAL)
Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
Cc: linux-kernel@vger.kernel.org (open list)

Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
 .../driver-api/thermal/sysfs-api.rst          |  1 +
 drivers/thermal/thermal_sysfs.c               | 23 +++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
index 18140dbb1ce1..ed5e6ba4e0d7 100644
--- a/Documentation/driver-api/thermal/sysfs-api.rst
+++ b/Documentation/driver-api/thermal/sysfs-api.rst
@@ -358,6 +358,7 @@ Thermal zone device sys I/F, created once it's registered::
     |---stats:			Directory containing thermal zone device's stats
     |---stats/reset_tz_stats:	Writes to this file resets the statistics.
     |---stats/max_gradient:	The maximum recorded dT/dt in uC/ms.
+    |---stats/min_gradient:	The minimum recorded dT/dt in uC/ms.
 
 Thermal cooling device sys I/F, created once it's registered::
 
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index aa28c1cae916..f89ec9a7e8c8 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -542,6 +542,7 @@ static void destroy_trip_attrs(struct thermal_zone_device *tz)
 struct thermal_zone_device_stats {
 	spinlock_t lock; /* protects this struct */
 	s64 max_gradient;
+	s64 min_gradient;
 	ktime_t last_time;
 };
 
@@ -569,6 +570,10 @@ static void temperature_stats_update(struct thermal_zone_device *tz)
 	/* update fastest temperature rise from our perspective */
 	if (cur_gradient > stats->max_gradient)
 		stats->max_gradient = cur_gradient;
+
+	/* update fastest temperature decay from our perspective */
+	if (cur_gradient < stats->min_gradient)
+		stats->min_gradient = cur_gradient;
 }
 
 void thermal_zone_device_stats_update(struct thermal_zone_device *tz)
@@ -595,6 +600,21 @@ static ssize_t max_gradient_show(struct device *dev,
 	return ret;
 }
 
+static ssize_t min_gradient_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	struct thermal_zone_device_stats *stats = tz->stats;
+	int ret;
+
+	spin_lock(&stats->lock);
+	temperature_stats_update(tz);
+	ret = snprintf(buf, PAGE_SIZE, "%lld\n", stats->min_gradient);
+	spin_unlock(&stats->lock);
+
+	return ret;
+}
+
 static ssize_t
 reset_tz_stats_store(struct device *dev, struct device_attribute *attr,
 		     const char *buf, size_t count)
@@ -604,6 +624,7 @@ reset_tz_stats_store(struct device *dev, struct device_attribute *attr,
 
 	spin_lock(&stats->lock);
 
+	stats->min_gradient = 0;
 	stats->max_gradient = 0;
 	stats->last_time = ktime_get();
 
@@ -612,10 +633,12 @@ reset_tz_stats_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static DEVICE_ATTR_RO(min_gradient);
 static DEVICE_ATTR_RO(max_gradient);
 static DEVICE_ATTR_WO(reset_tz_stats);
 
 static struct attribute *thermal_zone_device_stats_attrs[] = {
+	&dev_attr_min_gradient.attr,
 	&dev_attr_max_gradient.attr,
 	&dev_attr_reset_tz_stats.attr,
 	NULL
-- 
2.34.1

[PATCH 5/7] thermal: stats: introduce tz time in trip

[Eduardo Valentin]

From: Eduardo Valentin <eduval@amazon.com>

This patch adds a statistic to report how long
the thermal zone spent on temperature intervals
created by each trip point. The first interval
is the range below the first trip point. All
subsequent intervals are accounted when temperature
is above the trip point temperature value.

Samples:
$ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
trip-1	0	0
trip0	-10000	35188
trip1	25000	0
$ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
trip-1	0	0
trip0	-10000	36901
trip1	25000	0
$ echo 25001 > /sys//class/thermal/thermal_zone0/emul_temp
$ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
trip-1	0	0
trip0	-10000	47810
trip1	25000	2259
$ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
trip-1	0	0
trip0	-10000	47810
trip1	25000	3224
$ echo 24001 > /sys//class/thermal/thermal_zone0/emul_temp
$ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
trip-1	0	0
trip0	-10000	48960
trip1	25000	10080
$ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
trip-1	0	0
trip0	-10000	49844
trip1	25000	10080

Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
Cc: linux-pm@vger.kernel.org (open list:THERMAL)
Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
Cc: linux-kernel@vger.kernel.org (open list)

Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
 .../driver-api/thermal/sysfs-api.rst          |  2 +
 drivers/thermal/thermal_sysfs.c               | 86 +++++++++++++++++++
 2 files changed, 88 insertions(+)

diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
index ed5e6ba4e0d7..4a2b92a7488c 100644
--- a/Documentation/driver-api/thermal/sysfs-api.rst
+++ b/Documentation/driver-api/thermal/sysfs-api.rst
@@ -359,6 +359,8 @@ Thermal zone device sys I/F, created once it's registered::
     |---stats/reset_tz_stats:	Writes to this file resets the statistics.
     |---stats/max_gradient:	The maximum recorded dT/dt in uC/ms.
     |---stats/min_gradient:	The minimum recorded dT/dt in uC/ms.
+    |---stats/time_in_trip_ms:	Time spent on each temperature interval of
+				trip points.
 
 Thermal cooling device sys I/F, created once it's registered::
 
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index f89ec9a7e8c8..25851fe073c3 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -544,6 +544,7 @@ struct thermal_zone_device_stats {
 	s64 max_gradient;
 	s64 min_gradient;
 	ktime_t last_time;
+	ktime_t *time_in_trip;
 };
 
 #define DELTA_MILLI_C_TO_MICRO_C(t0, t1)		(((t0) - (t1)) * 1000)
@@ -552,6 +553,7 @@ static void temperature_stats_update(struct thermal_zone_device *tz)
 	struct thermal_zone_device_stats *stats = tz->stats;
 	ktime_t now = ktime_get(), delta;
 	s64 cur_gradient, delta_temp;
+	int i, trip_id = -1;
 
 	delta = ktime_sub(now, stats->last_time);
 	stats->last_time = now;
@@ -567,6 +569,29 @@ static void temperature_stats_update(struct thermal_zone_device *tz)
 	if (tz->last_temperature == 0)
 		cur_gradient = 0;
 
+	for (i = 0; i < tz->num_trips; i++) {
+		struct thermal_trip trip;
+		int ret;
+
+		ret = __thermal_zone_get_trip(tz, i, &trip);
+		if (ret)
+			continue;
+
+		if (tz->temperature > trip.temperature)
+			trip_id = i;
+	}
+
+	/*
+	 * Update how long we spend in each temperature interval.
+	 * This array is like as follows:
+	 * time_in_trip[0] == time spent with temperature <= trip[0]
+	 * time_in_trip[1] == time spent with temperature > trip[0]
+	 * time_in_trip[2] == time spent with temperature > trip[1]
+	 * ...
+	 */
+	stats->time_in_trip[trip_id + 1] =
+		ktime_add(stats->time_in_trip[trip_id + 1], delta);
+
 	/* update fastest temperature rise from our perspective */
 	if (cur_gradient > stats->max_gradient)
 		stats->max_gradient = cur_gradient;
@@ -615,12 +640,66 @@ static ssize_t min_gradient_show(struct device *dev,
 	return ret;
 }
 
+static ssize_t
+time_in_trip_ms_show(struct device *dev, struct device_attribute *attr,
+		     char *buf)
+{
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	struct thermal_zone_device_stats *stats = tz->stats;
+	ssize_t len = 0, ret = 0;
+	int i;
+
+	spin_lock(&stats->lock);
+	temperature_stats_update(tz);
+
+	/*
+	 * This should look like this:
+	 * trip-1	X
+	 * trip0	Y
+	 * trip1	Z
+	 * ...
+	 * and the way to read is. This thermal zone temperature was seen
+	 * X ms with tz->temperature <= trip0, Y ms with tz->temperature > trip0
+	 * and Z ms with tz->temperature > trip1.
+	 */
+	for (i = 0; i <= tz->num_trips; i++) {
+		int trip_temp = 0, r = 0;
+
+		if (i > 0) {
+			struct thermal_trip trip;
+
+			r = __thermal_zone_get_trip(tz, i - 1, &trip);
+			if (r < 0) {
+				ret = r;
+				break;
+			}
+			trip_temp = trip.temperature;
+		}
+
+		ret = snprintf(buf + len, PAGE_SIZE - len, "trip%d\t%d\t%llu\n",
+			       i - 1, trip_temp,
+			       ktime_to_ms(stats->time_in_trip[i]));
+
+		if (ret == 0)
+			ret = -EOVERFLOW;
+
+		if (ret < 0)
+			break;
+
+		len += ret;
+	}
+	spin_unlock(&stats->lock);
+
+	return ret < 0 ? ret : len;
+}
+
 static ssize_t
 reset_tz_stats_store(struct device *dev, struct device_attribute *attr,
 		     const char *buf, size_t count)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
 	struct thermal_zone_device_stats *stats = tz->stats;
+	int i;
 
 	spin_lock(&stats->lock);
 
@@ -628,6 +707,9 @@ reset_tz_stats_store(struct device *dev, struct device_attribute *attr,
 	stats->max_gradient = 0;
 	stats->last_time = ktime_get();
 
+	for (i = 0; i <= tz->num_trips; i++)
+		stats->time_in_trip[i] = ktime_set(0, 0);
+
 	spin_unlock(&stats->lock);
 
 	return count;
@@ -635,11 +717,13 @@ reset_tz_stats_store(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR_RO(min_gradient);
 static DEVICE_ATTR_RO(max_gradient);
+static DEVICE_ATTR_RO(time_in_trip_ms);
 static DEVICE_ATTR_WO(reset_tz_stats);
 
 static struct attribute *thermal_zone_device_stats_attrs[] = {
 	&dev_attr_min_gradient.attr,
 	&dev_attr_max_gradient.attr,
+	&dev_attr_time_in_trip_ms.attr,
 	&dev_attr_reset_tz_stats.attr,
 	NULL
 };
@@ -657,11 +741,13 @@ thermal_zone_device_stats_setup(struct thermal_zone_device *tz,
 	int var;
 
 	var = sizeof(*stats);
+	var += sizeof(*stats->time_in_trip) * (tz->num_trips + 1);
 
 	stats = kzalloc(var, GFP_KERNEL);
 	if (!stats)
 		return;
 
+	stats->time_in_trip = (ktime_t *)(stats + 1);
 	stats->last_time = ktime_get();
 	spin_lock_init(&stats->lock);
 	tz->stats = stats;
-- 
2.34.1

[PATCH 6/7] ythermal: core: report errors to governors

[Eduardo Valentin]

From: Eduardo Valentin <eduval@amazon.com>

Currently the thermal governors are not allowed to
react on temperature error events as the thermal core
skips the handling and logs an error on kernel buffer.
This patch adds the opportunity to report the errors
when they happen to governors.

Now, if a governor wants to react on temperature read
errors, they can implement the .check_error() callback.

Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
Cc: linux-pm@vger.kernel.org (open list:THERMAL)
Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
Cc: linux-kernel@vger.kernel.org (open list)

Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
 drivers/thermal/thermal_core.c | 9 +++++++++
 include/linux/thermal.h        | 3 +++
 2 files changed, 12 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 3ba970c0744f..2ff7d9c7c973 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -313,6 +313,12 @@ static void handle_non_critical_trips(struct thermal_zone_device *tz, int trip)
 		       def_governor->throttle(tz, trip);
 }
 
+static void handle_error_temperature(struct thermal_zone_device *tz, int error)
+{
+	if (tz->governor && tz->governor->check_error)
+		tz->governor->check_error(tz, error);
+}
+
 void thermal_zone_device_critical(struct thermal_zone_device *tz)
 {
 	/*
@@ -380,6 +386,9 @@ static void update_temperature(struct thermal_zone_device *tz)
 			dev_warn(&tz->device,
 				 "failed to read out thermal zone (%d)\n",
 				 ret);
+		/* tell the governor its source is hosed */
+		handle_error_temperature(tz, ret);
+
 		return;
 	}
 
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 9dc8292f0314..82c8e09a63e0 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -199,6 +199,8 @@ struct thermal_zone_device {
  *			thermal zone.
  * @throttle:	callback called for every trip point even if temperature is
  *		below the trip point temperature
+ * @check_error:	callback called whenever temperature updates fail.
+ *		Opportunity for the governor to react on errors.
  * @governor_list:	node in thermal_governor_list (in thermal_core.c)
  */
 struct thermal_governor {
@@ -206,6 +208,7 @@ struct thermal_governor {
 	int (*bind_to_tz)(struct thermal_zone_device *tz);
 	void (*unbind_from_tz)(struct thermal_zone_device *tz);
 	int (*throttle)(struct thermal_zone_device *tz, int trip);
+	void (*check_error)(struct thermal_zone_device *tz, int error);
 	struct list_head	governor_list;
 };
 
-- 
2.34.1

[PATCH 7/7] thermal: stats: add error accounting to thermal zone

[Eduardo Valentin]

From: Eduardo Valentin <eduval@amazon.com>

This patch adds an extra stat to report how many
temperature update failures were detected.
Error count is increase whenever the thermal
driver returns an actual error or when the temperature
is non positive.

Sample:

$ cat /sys/class/thermal/thermal_zone0/stats/error_count
0
$ echo -1 > /sys/class/thermal/thermal_zone0/emul_temp
$ cat /sys/class/thermal/thermal_zone0/stats/error_count
3

Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
Cc: linux-pm@vger.kernel.org (open list:THERMAL)
Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
Cc: linux-kernel@vger.kernel.org (open list)

Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
 drivers/thermal/thermal_core.c  |  3 ++
 drivers/thermal/thermal_core.h  |  7 ++++
 drivers/thermal/thermal_sysfs.c | 64 +++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 2ff7d9c7c973..359e7b2ff0e3 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -389,6 +389,9 @@ static void update_temperature(struct thermal_zone_device *tz)
 		/* tell the governor its source is hosed */
 		handle_error_temperature(tz, ret);
 
+		/* book keeping */
+		thermal_zone_device_error_stats_update(tz, ret);
+
 		return;
 	}
 
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index ef37b92bbb7c..612f93e6c257 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -141,12 +141,19 @@ ssize_t weight_store(struct device *, struct device_attribute *, const char *,
 
 #ifdef CONFIG_THERMAL_STATISTICS
 void thermal_zone_device_stats_update(struct thermal_zone_device *tz);
+void thermal_zone_device_error_stats_update(struct thermal_zone_device *tz,
+					    int error);
 void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
 					 struct thermal_instance *instance,
 					 unsigned long new_state);
 #else
 static inline
 void thermal_zone_device_stats_update(struct thermal_zone_device *tz) {}
+static inline
+void thermal_zone_device_error_stats_update(struct thermal_zone_device *tz,
+					    int error)
+{
+}
 static inline void
 thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
 				    struct thermal_instance *instance,
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 25851fe073c3..e511042e9dab 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -541,12 +541,21 @@ static void destroy_trip_attrs(struct thermal_zone_device *tz)
 /* thermal zone device statistics handling */
 struct thermal_zone_device_stats {
 	spinlock_t lock; /* protects this struct */
+	unsigned int error_count; /* just account them */
+	int max_temperature;
 	s64 max_gradient;
 	s64 min_gradient;
 	ktime_t last_time;
 	ktime_t *time_in_trip;
 };
 
+static void error_stats_update(struct thermal_zone_device *tz, int error)
+{
+	struct thermal_zone_device_stats *stats = tz->stats;
+
+	stats->error_count++;
+}
+
 #define DELTA_MILLI_C_TO_MICRO_C(t0, t1)		(((t0) - (t1)) * 1000)
 static void temperature_stats_update(struct thermal_zone_device *tz)
 {
@@ -555,6 +564,15 @@ static void temperature_stats_update(struct thermal_zone_device *tz)
 	s64 cur_gradient, delta_temp;
 	int i, trip_id = -1;
 
+	if (tz->temperature <= 0) {
+		/* probably a wrong reading */
+		error_stats_update(tz, tz->temperature);
+		return;
+	}
+
+	if (tz->temperature > stats->max_temperature)
+		stats->max_temperature = tz->temperature;
+
 	delta = ktime_sub(now, stats->last_time);
 	stats->last_time = now;
 
@@ -610,6 +628,31 @@ void thermal_zone_device_stats_update(struct thermal_zone_device *tz)
 	spin_unlock(&stats->lock);
 }
 
+void thermal_zone_device_error_stats_update(struct thermal_zone_device *tz,
+					    int error)
+{
+	struct thermal_zone_device_stats *stats = tz->stats;
+
+	spin_lock(&stats->lock);
+	error_stats_update(tz, error);
+	spin_unlock(&stats->lock);
+}
+
+static ssize_t max_temperature_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	struct thermal_zone_device_stats *stats = tz->stats;
+	int ret;
+
+	spin_lock(&stats->lock);
+	temperature_stats_update(tz);
+	ret = snprintf(buf, PAGE_SIZE, "%d\n", stats->max_temperature);
+	spin_unlock(&stats->lock);
+
+	return ret;
+}
+
 static ssize_t max_gradient_show(struct device *dev,
 				 struct device_attribute *attr, char *buf)
 {
@@ -640,6 +683,21 @@ static ssize_t min_gradient_show(struct device *dev,
 	return ret;
 }
 
+static ssize_t error_count_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	struct thermal_zone_device_stats *stats = tz->stats;
+	int ret;
+
+	spin_lock(&stats->lock);
+	temperature_stats_update(tz);
+	ret = snprintf(buf, PAGE_SIZE, "%u\n", stats->error_count);
+	spin_unlock(&stats->lock);
+
+	return ret;
+}
+
 static ssize_t
 time_in_trip_ms_show(struct device *dev, struct device_attribute *attr,
 		     char *buf)
@@ -705,6 +763,8 @@ reset_tz_stats_store(struct device *dev, struct device_attribute *attr,
 
 	stats->min_gradient = 0;
 	stats->max_gradient = 0;
+	stats->max_temperature = 0;
+	stats->error_count = 0;
 	stats->last_time = ktime_get();
 
 	for (i = 0; i <= tz->num_trips; i++)
@@ -717,13 +777,17 @@ reset_tz_stats_store(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR_RO(min_gradient);
 static DEVICE_ATTR_RO(max_gradient);
+static DEVICE_ATTR_RO(max_temperature);
 static DEVICE_ATTR_RO(time_in_trip_ms);
+static DEVICE_ATTR_RO(error_count);
 static DEVICE_ATTR_WO(reset_tz_stats);
 
 static struct attribute *thermal_zone_device_stats_attrs[] = {
 	&dev_attr_min_gradient.attr,
 	&dev_attr_max_gradient.attr,
+	&dev_attr_max_temperature.attr,
 	&dev_attr_time_in_trip_ms.attr,
+	&dev_attr_error_count.attr,
 	&dev_attr_reset_tz_stats.attr,
 	NULL
 };
-- 
2.34.1

Re: [PATCH 0/7] thermal: enhancements on thermal stats

[Rafael J. Wysocki]

Hi Eduardo,

On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
>
> Hello Rafael and Daniel
>
> After a long hiatus, I am returning to more frequent contributions
> to the thermal subsystems, as least until I drain some of the
> commits I have in my trees.
>
> This is a first series of several that will come as improvements
> on the thermal subsystem that will enable using this subsystem
> in the Baseboard Management Controller (BMC) space, as part
> of the Nitro BMC project. To do so, there were a few improvements
> and new features wrote.
>
> In this series in particular, I present a set of enhancements
> on how we are handling statistics. The cooling device stats
> are awesome, but I added a few new entries there. I also
> introduce stats per thermal zone here too.
>
> I tried to keep documentation as current as possible.
> I may have missed a thing or two, so please help me out here.
> Testing/Examples are in each code.
>
> Let me know any feeback,
>
> BR,
>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
> Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
> Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
> Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> Cc: linux-pm@vger.kernel.org (open list:THERMAL)
> Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
> Cc: linux-kernel@vger.kernel.org (open list)
>
> Eduardo Valentin (7):
>   thermal: stats: track time each dev changes due to tz
>   thermal: stats: track number of change requests due to tz
>   thermal: stats: introduce thermal zone stats/ directory
>   thermal: stats: introduce thermal zone stats/min_gradient
>   thermal: stats: introduce tz time in trip
>   ythermal: core: report errors to governors
>   thermal: stats: add error accounting to thermal zone
>
>  .../driver-api/thermal/sysfs-api.rst          |  10 +
>  drivers/thermal/thermal_core.c                |  15 +-
>  drivers/thermal/thermal_core.h                |  16 +
>  drivers/thermal/thermal_helpers.c             |  11 +-
>  drivers/thermal/thermal_sysfs.c               | 495 +++++++++++++++++-
>  include/linux/thermal.h                       |   5 +
>  6 files changed, 539 insertions(+), 13 deletions(-)
>
> --

There are still some other things I need to take care of before I can
get to this series, but I will get to it.

Thanks!

Re: [PATCH 0/7] thermal: enhancements on thermal stats

[Eduardo Valentin]

On Wed, May 24, 2023 at 08:22:11PM +0200, Rafael J. Wysocki wrote:
> 
> There are still some other things I need to take care of before I can
> get to this series, but I will get to it.
> 
> Thanks!

Ok, no worries.


-- 
All the best,
Eduardo Valentin

Re: [PATCH 1/7] thermal: stats: track time each dev changes due to tz

[Rafael J. Wysocki]

On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
>
> From: Eduardo Valentin <eduval@amazon.com>
>
> This patch improves the current cooling device
> statistics by adding a new file under
> cdev/stats/time_in_thermal_zone_ms
> to represent the time in milliseconds
> that the cooling device was driven by each
> associated thermal zone.

Can you please explain the use case addressed by this?

>
> The file format is:
> thermal_zone: <type> <time_in_ms>

So there is the "one value per sysfs attribute" rule ...

> Samples:
> $ cat /sys/class/thermal/cooling_device0/stats/time_in_thermal_zone_ms
> thermal_zone: amb0      117496
>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
> Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
> Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
> Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> Cc: linux-pm@vger.kernel.org (open list:THERMAL)
> Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
> Cc: linux-kernel@vger.kernel.org (open list)
>
> Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> ---
>  .../driver-api/thermal/sysfs-api.rst          |   2 +
>  drivers/thermal/thermal_core.c                |   2 +-
>  drivers/thermal/thermal_core.h                |   5 +
>  drivers/thermal/thermal_helpers.c             |  11 +-
>  drivers/thermal/thermal_sysfs.c               | 128 +++++++++++++++++-
>  5 files changed, 139 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
> index 6c1175c6afba..caa50d61a5bc 100644
> --- a/Documentation/driver-api/thermal/sysfs-api.rst
> +++ b/Documentation/driver-api/thermal/sysfs-api.rst
> @@ -367,6 +367,8 @@ Thermal cooling device sys I/F, created once it's registered::
>      |---stats/time_in_state_ms:        Time (msec) spent in various cooling states
>      |---stats/total_trans:     Total number of times cooling state is changed
>      |---stats/trans_table:     Cooling state transition table
> +    |---stats/time_in_thermal_zone_ms: Time that this cooling device was driven
> +                                each associated thermal zone.

I think that "by" is missing from the above description, but in any
case I'm not quite sure what exactly it means.

A cooling device may be shared by multiple thermal zones which is what
instances are for IIUC, so is this going to measure how much time the
given thermal zone was that caused the cdev to stay in the given
state?  Like say there are two thermal zone sharing a cdev and one of
them says "don't care" and the other says "turn on", so the second one
causes the cdev to enter the "on" state?

But what if both thermal zones in this example say "turn on"?

>  Then next two dynamic attributes are created/removed in pairs. They represent
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 842f678c1c3e..4bb77af6a6f4 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1078,7 +1078,7 @@ void thermal_cooling_device_update(struct thermal_cooling_device *cdev)
>         if (cdev->ops->get_cur_state(cdev, &state) || state > cdev->max_state)
>                 goto unlock;
>
> -       thermal_cooling_device_stats_update(cdev, state);
> +       thermal_cooling_device_stats_update(cdev, NULL, state);
>
>  unlock:
>         mutex_unlock(&cdev->lock);
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 3d4a787c6b28..3cce60c6e065 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -102,6 +102,9 @@ struct thermal_instance {
>         struct list_head cdev_node; /* node in cdev->thermal_instances */
>         unsigned int weight; /* The weight of the cooling device */
>         bool upper_no_limit;
> +#if IS_ENABLED(CONFIG_THERMAL_STATISTICS)
> +       ktime_t time_in; /* time spent in this instance */
> +#endif
>  };
>
>  #define to_thermal_zone(_dev) \
> @@ -137,10 +140,12 @@ ssize_t weight_store(struct device *, struct device_attribute *, const char *,
>
>  #ifdef CONFIG_THERMAL_STATISTICS
>  void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
> +                                        struct thermal_instance *instance,
>                                          unsigned long new_state);
>  #else
>  static inline void
>  thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
> +                                   struct thermal_instance *instance,
>                                     unsigned long new_state) {}
>  #endif /* CONFIG_THERMAL_STATISTICS */
>
> diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
> index cfba0965a22d..ec8e86394977 100644
> --- a/drivers/thermal/thermal_helpers.c
> +++ b/drivers/thermal/thermal_helpers.c
> @@ -149,18 +149,19 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
>  EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>
>  static void thermal_cdev_set_cur_state(struct thermal_cooling_device *cdev,
> +                                      struct thermal_instance *target_instance,
>                                        int target)
>  {
>         if (cdev->ops->set_cur_state(cdev, target))
>                 return;
>
>         thermal_notify_cdev_state_update(cdev->id, target);
> -       thermal_cooling_device_stats_update(cdev, target);
> +       thermal_cooling_device_stats_update(cdev, target_instance, target);
>  }
>
>  void __thermal_cdev_update(struct thermal_cooling_device *cdev)
>  {
> -       struct thermal_instance *instance;
> +       struct thermal_instance *instance, *target_instance = NULL;
>         unsigned long target = 0;
>
>         /* Make sure cdev enters the deepest cooling state */
> @@ -169,11 +170,13 @@ void __thermal_cdev_update(struct thermal_cooling_device *cdev)
>                         instance->tz->id, instance->target);
>                 if (instance->target == THERMAL_NO_TARGET)
>                         continue;
> -               if (instance->target > target)
> +               if (instance->target > target) {
>                         target = instance->target;
> +                       target_instance = instance;
> +               }
>         }
>
> -       thermal_cdev_set_cur_state(cdev, target);
> +       thermal_cdev_set_cur_state(cdev, target_instance, 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 6c20c9f90a05..a3b71f03db75 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -632,7 +632,7 @@ cur_state_store(struct device *dev, struct device_attribute *attr,
>
>         result = cdev->ops->set_cur_state(cdev, state);
>         if (!result)
> -               thermal_cooling_device_stats_update(cdev, state);
> +               thermal_cooling_device_stats_update(cdev, NULL, state);
>
>         mutex_unlock(&cdev->lock);
>         return result ? result : count;
> @@ -661,6 +661,7 @@ static const struct attribute_group *cooling_device_attr_groups[] = {
>  };
>
>  #ifdef CONFIG_THERMAL_STATISTICS
> +/* thermal cooling device statistics handling */
>  struct cooling_dev_stats {
>         spinlock_t lock;
>         unsigned int total_trans;
> @@ -668,9 +669,29 @@ struct cooling_dev_stats {
>         ktime_t last_time;
>         ktime_t *time_in_state;
>         unsigned int *trans_table;
> +       struct thermal_instance *last_instance;
> +       struct thermal_instance *curr_instance;
>  };
>
> -static void update_time_in_state(struct cooling_dev_stats *stats)
> +static void update_time_in_instance(struct cooling_dev_stats *stats,
> +                                   struct thermal_instance *instance,
> +                                   ktime_t now, ktime_t delta)
> +{
> +       if (!instance)
> +               return;
> +
> +       stats->last_instance = stats->curr_instance;
> +       stats->curr_instance = instance;
> +
> +       if (!stats->last_instance)
> +               stats->last_instance = instance;
> +
> +       stats->last_instance->time_in =
> +                       ktime_add(stats->last_instance->time_in, delta);
> +}
> +
> +static void update_time_in_state(struct cooling_dev_stats *stats,
> +                                struct thermal_instance *instance)
>  {
>         ktime_t now = ktime_get(), delta;
>
> @@ -678,9 +699,12 @@ static void update_time_in_state(struct cooling_dev_stats *stats)
>         stats->time_in_state[stats->state] =
>                 ktime_add(stats->time_in_state[stats->state], delta);
>         stats->last_time = now;
> +
> +       update_time_in_instance(stats, instance, now, delta);
>  }
>
>  void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
> +                                        struct thermal_instance *instance,
>                                          unsigned long new_state)
>  {
>         struct cooling_dev_stats *stats = cdev->stats;
> @@ -695,7 +719,7 @@ void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
>         if (stats->state == new_state)
>                 goto unlock;
>
> -       update_time_in_state(stats);
> +       update_time_in_state(stats, instance);
>         stats->trans_table[stats->state * (cdev->max_state + 1) + new_state]++;
>         stats->state = new_state;
>         stats->total_trans++;
> @@ -744,7 +768,7 @@ time_in_state_ms_show(struct device *dev, struct device_attribute *attr,
>
>         spin_lock(&stats->lock);
>
> -       update_time_in_state(stats);
> +       update_time_in_state(stats, stats->curr_instance);
>
>         for (i = 0; i <= cdev->max_state; i++) {
>                 len += sprintf(buf + len, "state%u\t%llu\n", i,
> @@ -758,12 +782,98 @@ time_in_state_ms_show(struct device *dev, struct device_attribute *attr,
>         return len;
>  }
>
> +struct cdev_thermal_zone_residency {
> +       char thermal_zone[THERMAL_NAME_LENGTH];
> +       ktime_t time_in;
> +       unsigned long counter;
> +       struct list_head node; /* we build this as we go */
> +};

What is represented by this structure?

> +
> +static void
> +build_cdev_thermal_zone_residency(struct list_head *list,
> +                                 struct thermal_cooling_device *cdev)
> +{
> +       struct cdev_thermal_zone_residency *res, *update_res;
> +       struct thermal_instance *instance;
> +
> +       /*
> +        * Build an array of pairs <thermal zone, time> to represent
> +        * how this cooling device was driven by each thermal zone
> +        */
> +       list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
> +               update_res = NULL;
> +
> +               list_for_each_entry(res, list, node) {
> +                       if (strncmp(res->thermal_zone, instance->tz->type,
> +                                   THERMAL_NAME_LENGTH) == 0)
> +                               update_res = res;
> +               }
> +               if (!update_res) {
> +                       update_res = kzalloc(sizeof(*update_res), GFP_KERNEL);
> +                       strscpy(update_res->thermal_zone,
> +                               instance->tz->type, THERMAL_NAME_LENGTH);
> +                       list_add_tail(&update_res->node, list);
> +               }
> +       }
> +}
> +
> +static ssize_t
> +time_in_thermal_zone_ms_show(struct device *dev, struct device_attribute *attr,
> +                            char *buf)
> +{
> +       LIST_HEAD(cdev_thermal_zone_list);
> +       struct thermal_cooling_device *cdev = to_cooling_device(dev);
> +       struct cooling_dev_stats *stats = cdev->stats;
> +       struct cdev_thermal_zone_residency *res, *next;
> +       struct thermal_instance *instance;
> +       ssize_t len = 0, ret = 0;
> +
> +       mutex_lock(&cdev->lock);
> +
> +       spin_lock(&stats->lock);
> +       update_time_in_state(stats, stats->curr_instance);
> +       spin_unlock(&stats->lock);
> +
> +       build_cdev_thermal_zone_residency(&cdev_thermal_zone_list, cdev);
> +
> +       list_for_each_entry(instance, &cdev->thermal_instances, cdev_node)
> +               list_for_each_entry(res, &cdev_thermal_zone_list, node)
> +                       if (strncmp(res->thermal_zone, instance->tz->type,
> +                                   THERMAL_NAME_LENGTH) == 0)
> +                               res->time_in = ktime_add(res->time_in,
> +                                                        instance->time_in);
> +
> +       mutex_unlock(&cdev->lock);
> +
> +       list_for_each_entry_safe(res, next, &cdev_thermal_zone_list, node) {
> +               ret = snprintf(buf + len, PAGE_SIZE - len,
> +                              "thermal_zone: %s\t%llu\n",
> +                              res->thermal_zone, ktime_to_ms(res->time_in));
> +
> +               if (ret == 0)
> +                       ret = -EOVERFLOW;
> +
> +               if (ret < 0)
> +                       break;
> +
> +               len += ret;
> +       }

Why does the above loop need to use the _safe variant?

> +
> +       list_for_each_entry_safe(res, next, &cdev_thermal_zone_list, node) {
> +               list_del(&res->node);
> +               kfree(res);
> +       }
> +
> +       return ret < 0 ? ret : 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;
> +       struct thermal_instance *instance;
>         int i, states;
>
>         mutex_lock(&cdev->lock);
> @@ -774,6 +884,7 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
>
>         states = cdev->max_state + 1;
>
> +       mutex_lock(&cdev->lock);
>         spin_lock(&stats->lock);
>
>         stats->total_trans = 0;
> @@ -784,7 +895,14 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
>         for (i = 0; i < states; i++)
>                 stats->time_in_state[i] = ktime_set(0, 0);
>
> +       /* Make sure we reset all counters per instance */
> +       list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
> +               instance->time_in = ktime_set(0, 0);
> +       }
> +
>         spin_unlock(&stats->lock);
> +       mutex_unlock(&cdev->lock);
> +
>
>  unlock:
>         mutex_unlock(&cdev->lock);
> @@ -852,12 +970,14 @@ static ssize_t trans_table_show(struct device *dev,
>
>  static DEVICE_ATTR_RO(total_trans);
>  static DEVICE_ATTR_RO(time_in_state_ms);
> +static DEVICE_ATTR_RO(time_in_thermal_zone_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_time_in_thermal_zone_ms.attr,
>         &dev_attr_reset.attr,
>         &dev_attr_trans_table.attr,
>         NULL
> --

Re: [PATCH 2/7] thermal: stats: track number of change requests due to tz

[Rafael J. Wysocki]

On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
>
> From: Eduardo Valentin <eduval@amazon.com>
>
> This patch improves the current cooling device
> statistics by adding a new file under
> cdev/stats/requests_of_thermal_zone
>
> to represent the number of times each thermal zone
> requested the cooling device to effectively change.
> If the request associated was not serviced because
> another thermal zone asked for a higher cooling level,
> this counter does not increase.

What if the cdev is associated with two thermal zones asking for the
same state of it?

> The file format is:
> thermal_zone: <type> <count>
>
> Samples:
> $ cat cdev0/stats/requests_of_thermal_zone
> thermal_zone: amb0      2

The "one value per attribute" sysfs rule violation.

>
> In this example, it means the thermal zone 'amb0' has requested
> 2 times for cdev0 to change state.

Like in the previous patch, it would be good to explain the use case.

> Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
> Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
> Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
> Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> Cc: linux-pm@vger.kernel.org (open list:THERMAL)
> Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
> Cc: linux-kernel@vger.kernel.org (open list)
>
> Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> ---
>  .../driver-api/thermal/sysfs-api.rst          |  2 +
>  drivers/thermal/thermal_core.h                |  1 +
>  drivers/thermal/thermal_sysfs.c               | 52 +++++++++++++++++++
>  3 files changed, 55 insertions(+)
>
> diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
> index caa50d61a5bc..75309a51d9b3 100644
> --- a/Documentation/driver-api/thermal/sysfs-api.rst
> +++ b/Documentation/driver-api/thermal/sysfs-api.rst
> @@ -369,6 +369,8 @@ Thermal cooling device sys I/F, created once it's registered::
>      |---stats/trans_table:     Cooling state transition table
>      |---stats/time_in_thermal_zone_ms: Time that this cooling device was driven
>                                  each associated thermal zone.
> +    |---stats/requests_of_thermal_zone:        Total number of times this cooling device
> +                                changed due to each associated thermal zone.

The meaning of the above description is not clear to me.

>
>
>  Then next two dynamic attributes are created/removed in pairs. They represent
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 3cce60c6e065..ed6511c3b794 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -103,6 +103,7 @@ struct thermal_instance {
>         unsigned int weight; /* The weight of the cooling device */
>         bool upper_no_limit;
>  #if IS_ENABLED(CONFIG_THERMAL_STATISTICS)
> +       unsigned long total_requests;
>         ktime_t time_in; /* time spent in this instance */
>  #endif
>  };
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index a3b71f03db75..0bce1415f7e8 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -723,6 +723,7 @@ void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
>         stats->trans_table[stats->state * (cdev->max_state + 1) + new_state]++;
>         stats->state = new_state;
>         stats->total_trans++;
> +       stats->curr_instance->total_requests++;
>
>  unlock:
>         spin_unlock(&stats->lock);
> @@ -867,6 +868,54 @@ time_in_thermal_zone_ms_show(struct device *dev, struct device_attribute *attr,
>         return ret < 0 ? ret : len;
>  }
>
> +static ssize_t
> +requests_of_thermal_zone_show(struct device *dev, struct device_attribute *attr,
> +                             char *buf)
> +{
> +       LIST_HEAD(cdev_thermal_zone_list);
> +       struct thermal_cooling_device *cdev = to_cooling_device(dev);
> +       struct cooling_dev_stats *stats = cdev->stats;
> +       struct cdev_thermal_zone_residency *res, *next;
> +       struct thermal_instance *instance;
> +       ssize_t len = 0, ret = 0;
> +
> +       mutex_lock(&cdev->lock);
> +
> +       spin_lock(&stats->lock);
> +       update_time_in_state(stats, stats->curr_instance);
> +       spin_unlock(&stats->lock);
> +
> +       build_cdev_thermal_zone_residency(&cdev_thermal_zone_list, cdev);
> +
> +       list_for_each_entry(instance, &cdev->thermal_instances, cdev_node)
> +               list_for_each_entry(res, &cdev_thermal_zone_list, node)
> +                       if (strncmp(res->thermal_zone, instance->tz->type,
> +                                   THERMAL_NAME_LENGTH) == 0)
> +                               res->counter += instance->total_requests;
> +
> +       mutex_unlock(&cdev->lock);
> +
> +       list_for_each_entry_safe(res, next, &cdev_thermal_zone_list, node) {

Why is the _safe variant needed here?

> +               ret = sprintf(buf + len, "thermal_zone: %s\t%lu\n",
> +                             res->thermal_zone, res->counter);
> +
> +               if (ret == 0)
> +                       ret = -EOVERFLOW;
> +
> +               if (ret < 0)
> +                       break;
> +
> +               len += ret;
> +       }
> +
> +       list_for_each_entry_safe(res, next, &cdev_thermal_zone_list, node) {
> +               list_del(&res->node);
> +               kfree(res);
> +       }
> +
> +       return ret < 0 ? ret : len;

I would prefer

if (ret < 0)
        return ret;

return len;

> +}
> +
>  static ssize_t
>  reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
>             size_t count)
> @@ -897,6 +946,7 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
>
>         /* Make sure we reset all counters per instance */
>         list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
> +               instance->total_requests = 0;
>                 instance->time_in = ktime_set(0, 0);
>         }
>
> @@ -971,6 +1021,7 @@ static ssize_t trans_table_show(struct device *dev,
>  static DEVICE_ATTR_RO(total_trans);
>  static DEVICE_ATTR_RO(time_in_state_ms);
>  static DEVICE_ATTR_RO(time_in_thermal_zone_ms);
> +static DEVICE_ATTR_RO(requests_of_thermal_zone);
>  static DEVICE_ATTR_WO(reset);
>  static DEVICE_ATTR_RO(trans_table);
>
> @@ -978,6 +1029,7 @@ static struct attribute *cooling_device_stats_attrs[] = {
>         &dev_attr_total_trans.attr,
>         &dev_attr_time_in_state_ms.attr,
>         &dev_attr_time_in_thermal_zone_ms.attr,
> +       &dev_attr_requests_of_thermal_zone.attr,
>         &dev_attr_reset.attr,
>         &dev_attr_trans_table.attr,
>         NULL
> --

Re: [PATCH 4/7] thermal: stats: introduce thermal zone stats/min_gradient

[Rafael J. Wysocki]

On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
>
> From: Eduardo Valentin <eduval@amazon.com>
>
> The patch adds a statistic to track
> the minimum gradient (dT/dt) to the thermal zone
> stats/ folder.
>
> Samples:
>
> $ echo 1000 > emul_temp
> $ cat stats/min_gradient
> 0
> $ echo 2000 > emul_temp
> $ echo 1000 > emul_temp
> $ cat stats/min_gradient
> -3460
>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
> Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
> Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
> Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> Cc: linux-pm@vger.kernel.org (open list:THERMAL)
> Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
> Cc: linux-kernel@vger.kernel.org (open list)
>
> Signed-off-by: Eduardo Valentin <eduval@amazon.com>

This can be easily folded into the previous patch IMO.

> ---
>  .../driver-api/thermal/sysfs-api.rst          |  1 +
>  drivers/thermal/thermal_sysfs.c               | 23 +++++++++++++++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
> index 18140dbb1ce1..ed5e6ba4e0d7 100644
> --- a/Documentation/driver-api/thermal/sysfs-api.rst
> +++ b/Documentation/driver-api/thermal/sysfs-api.rst
> @@ -358,6 +358,7 @@ Thermal zone device sys I/F, created once it's registered::
>      |---stats:                 Directory containing thermal zone device's stats
>      |---stats/reset_tz_stats:  Writes to this file resets the statistics.
>      |---stats/max_gradient:    The maximum recorded dT/dt in uC/ms.
> +    |---stats/min_gradient:    The minimum recorded dT/dt in uC/ms.
>
>  Thermal cooling device sys I/F, created once it's registered::
>
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index aa28c1cae916..f89ec9a7e8c8 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -542,6 +542,7 @@ static void destroy_trip_attrs(struct thermal_zone_device *tz)
>  struct thermal_zone_device_stats {
>         spinlock_t lock; /* protects this struct */
>         s64 max_gradient;
> +       s64 min_gradient;
>         ktime_t last_time;
>  };
>
> @@ -569,6 +570,10 @@ static void temperature_stats_update(struct thermal_zone_device *tz)
>         /* update fastest temperature rise from our perspective */
>         if (cur_gradient > stats->max_gradient)
>                 stats->max_gradient = cur_gradient;
> +
> +       /* update fastest temperature decay from our perspective */
> +       if (cur_gradient < stats->min_gradient)
> +               stats->min_gradient = cur_gradient;
>  }
>
>  void thermal_zone_device_stats_update(struct thermal_zone_device *tz)
> @@ -595,6 +600,21 @@ static ssize_t max_gradient_show(struct device *dev,
>         return ret;
>  }
>
> +static ssize_t min_gradient_show(struct device *dev,
> +                                struct device_attribute *attr, char *buf)
> +{
> +       struct thermal_zone_device *tz = to_thermal_zone(dev);
> +       struct thermal_zone_device_stats *stats = tz->stats;
> +       int ret;
> +
> +       spin_lock(&stats->lock);
> +       temperature_stats_update(tz);
> +       ret = snprintf(buf, PAGE_SIZE, "%lld\n", stats->min_gradient);
> +       spin_unlock(&stats->lock);
> +
> +       return ret;
> +}
> +
>  static ssize_t
>  reset_tz_stats_store(struct device *dev, struct device_attribute *attr,
>                      const char *buf, size_t count)
> @@ -604,6 +624,7 @@ reset_tz_stats_store(struct device *dev, struct device_attribute *attr,
>
>         spin_lock(&stats->lock);
>
> +       stats->min_gradient = 0;
>         stats->max_gradient = 0;
>         stats->last_time = ktime_get();
>
> @@ -612,10 +633,12 @@ reset_tz_stats_store(struct device *dev, struct device_attribute *attr,
>         return count;
>  }
>
> +static DEVICE_ATTR_RO(min_gradient);
>  static DEVICE_ATTR_RO(max_gradient);
>  static DEVICE_ATTR_WO(reset_tz_stats);
>
>  static struct attribute *thermal_zone_device_stats_attrs[] = {
> +       &dev_attr_min_gradient.attr,
>         &dev_attr_max_gradient.attr,
>         &dev_attr_reset_tz_stats.attr,
>         NULL
> --
> 2.34.1
>

Re: [PATCH 5/7] thermal: stats: introduce tz time in trip

[Rafael J. Wysocki]

On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
>
> From: Eduardo Valentin <eduval@amazon.com>
>
> This patch adds a statistic to report how long
> the thermal zone spent on temperature intervals
> created by each trip point. The first interval
> is the range below the first trip point. All
> subsequent intervals are accounted when temperature
> is above the trip point temperature value.
>
> Samples:
> $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> trip-1  0       0

The above line is confusing.

> trip0   -10000  35188
> trip1   25000   0

And the format violates the "one value per attribute" sysfs rule.

> $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> trip-1  0       0
> trip0   -10000  36901
> trip1   25000   0
> $ echo 25001 > /sys//class/thermal/thermal_zone0/emul_temp
> $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> trip-1  0       0
> trip0   -10000  47810
> trip1   25000   2259
> $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> trip-1  0       0
> trip0   -10000  47810
> trip1   25000   3224
> $ echo 24001 > /sys//class/thermal/thermal_zone0/emul_temp
> $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> trip-1  0       0
> trip0   -10000  48960
> trip1   25000   10080
> $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> trip-1  0       0
> trip0   -10000  49844
> trip1   25000   10080
>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
> Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
> Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
> Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> Cc: linux-pm@vger.kernel.org (open list:THERMAL)
> Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
> Cc: linux-kernel@vger.kernel.org (open list)
>
> Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> ---
>  .../driver-api/thermal/sysfs-api.rst          |  2 +
>  drivers/thermal/thermal_sysfs.c               | 86 +++++++++++++++++++
>  2 files changed, 88 insertions(+)
>
> diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
> index ed5e6ba4e0d7..4a2b92a7488c 100644
> --- a/Documentation/driver-api/thermal/sysfs-api.rst
> +++ b/Documentation/driver-api/thermal/sysfs-api.rst
> @@ -359,6 +359,8 @@ Thermal zone device sys I/F, created once it's registered::
>      |---stats/reset_tz_stats:  Writes to this file resets the statistics.
>      |---stats/max_gradient:    The maximum recorded dT/dt in uC/ms.
>      |---stats/min_gradient:    The minimum recorded dT/dt in uC/ms.
> +    |---stats/time_in_trip_ms: Time spent on each temperature interval of
> +                               trip points.

I would write "in each temperature interval between consecutive trip points".

Doesn't this assume a specific temperature ordering of trip points?
And so what if they are not ordered?

Re: [PATCH 6/7] ythermal: core: report errors to governors

[Rafael J. Wysocki]

On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
>
> From: Eduardo Valentin <eduval@amazon.com>
>
> Currently the thermal governors are not allowed to
> react on temperature error events as the thermal core
> skips the handling and logs an error on kernel buffer.
> This patch adds the opportunity to report the errors
> when they happen to governors.
>
> Now, if a governor wants to react on temperature read
> errors, they can implement the .check_error() callback.

Explaining the use case for this would help a lot.

Re: [PATCH 7/7] thermal: stats: add error accounting to thermal zone

[Rafael J. Wysocki]

On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
>
> From: Eduardo Valentin <eduval@amazon.com>
>
> This patch adds an extra stat to report how many
> temperature update failures were detected.
> Error count is increase whenever the thermal
> driver returns an actual error or when the temperature
> is non positive.

Why can't the temperature be negative?

Re: [PATCH 0/7] thermal: enhancements on thermal stats

[Daniel Lezcano]

Hi Eduardo,

On 19/05/2023 05:27, Eduardo Valentin wrote:
> Hello Rafael and Daniel
> 
> After a long hiatus, I am returning to more frequent contributions
> to the thermal subsystems, as least until I drain some of the
> commits I have in my trees.
> 
> This is a first series of several that will come as improvements
> on the thermal subsystem that will enable using this subsystem
> in the Baseboard Management Controller (BMC) space, as part
> of the Nitro BMC project. To do so, there were a few improvements
> and new features wrote.
> 
> In this series in particular, I present a set of enhancements
> on how we are handling statistics. The cooling device stats
> are awesome, but I added a few new entries there. I also
> introduce stats per thermal zone here too.

 From my POV, that kind of information belongs to debugfs. sysfs is not 
suitable for that.

The cdev stats are a total mess because of the page size limitation of 
sysfs and the explosion of the combination when there are a large number 
of states (eg. display is 1024 cooling device states resulting in a 
matrix of 1024 x 1024, so more than 4MB of memory).

For the record, I'm working on such of statistics [1][2], and optimized 
this cooling device statistics in order to get ride of the existing 
sysfs cdev stats.

Actually, all the stats rely on the mitigation episodes. However, for 
that we need to correctly identify when they begin and when they end. We 
can have mitigation episode inside mitigation episode (eg. passive 
mitigation@trip0 and active mitigation@trip1).

This is not working today because the trip point detection is incorrect, 
thus the mitigation episodes are also incorrect, consequently the stats 
are de facto incorrect.

There is more details at [3] but the change assumes the trip points are 
ordered in the ascending order which is wrong, that is why it was not 
merged.

The mitigation works but the detection is fuzzy, so the math is 
inaccurate and as we are in the boundaries of a temperature limit, the 
resulting statistics do not show us the interesting information to 
optimize the governors when they are not totally inconsistent.

All the work around the generic trip points is to fix that.

There is a proposal at LPC to add statistic/debug information for 
thermal, may be you can participate so we join our efforts?

   -- Daniel

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/log/?h=thermal/trip-crossed%2bdebugfs

[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/log/?h=thermal/debugfs-v2

[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/commit/?h=thermal/trip-crossed%2bdebugfs&id=7d713a9128ad9a153de9c3f5b854c6f1acfb3064



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

[PATCH 0/7] thermal: enhancements on thermal stats

[Eduardo Valentin]

On Tue, Jun 20, 2023 at 09:05:07PM +0200, Daniel Lezcano wrote:
> 
> 
> 
> Hi Eduardo,
> 
> On 19/05/2023 05:27, Eduardo Valentin wrote:
> > Hello Rafael and Daniel
> > 
> > After a long hiatus, I am returning to more frequent contributions
> > to the thermal subsystems, as least until I drain some of the
> > commits I have in my trees.
> > 
> > This is a first series of several that will come as improvements
> > on the thermal subsystem that will enable using this subsystem
> > in the Baseboard Management Controller (BMC) space, as part
> > of the Nitro BMC project. To do so, there were a few improvements
> > and new features wrote.
> > 
> > In this series in particular, I present a set of enhancements
> > on how we are handling statistics. The cooling device stats
> > are awesome, but I added a few new entries there. I also
> > introduce stats per thermal zone here too.
> 
> From my POV, that kind of information belongs to debugfs. sysfs is not
> suitable for that.
> 
> The cdev stats are a total mess because of the page size limitation of
> sysfs and the explosion of the combination when there are a large number
> of states (eg. display is 1024 cooling device states resulting in a
> matrix of 1024 x 1024, so more than 4MB of memory).
> 
> For the record, I'm working on such of statistics [1][2], and optimized
> this cooling device statistics in order to get ride of the existing
> sysfs cdev stats.
> 
> Actually, all the stats rely on the mitigation episodes. However, for
> that we need to correctly identify when they begin and when they end. We
> can have mitigation episode inside mitigation episode (eg. passive
> mitigation@trip0 and active mitigation@trip1).
> 
> This is not working today because the trip point detection is incorrect,
> thus the mitigation episodes are also incorrect, consequently the stats
> are de facto incorrect.
> 
> There is more details at [3] but the change assumes the trip points are
> ordered in the ascending order which is wrong, that is why it was not
> merged.
> 
> The mitigation works but the detection is fuzzy, so the math is
> inaccurate and as we are in the boundaries of a temperature limit, the
> resulting statistics do not show us the interesting information to
> optimize the governors when they are not totally inconsistent.
> 
> All the work around the generic trip points is to fix that.
> 
> There is a proposal at LPC to add statistic/debug information for
> thermal, may be you can participate so we join our efforts?

I am not sure if I would be able to join but will look into this and get back to you soon. 

In fact, joining efforts will be awesome!

As for cdev statistics, I believe the transition table is an overkill. And for the cases I have been using,
with 20+ thermal zones with 10+ cdevs assgined to all of thermal zones, is way beyond the PAGE limit size.
Totally agree with that.

I agree this code deserves a cleanup. These patches build on top of what is currently in mainline.
I also would prefer to have this code potentially out of the -sysfs file and handled separately
from the actual sysfs node handling code.

As for the debugfs vs sysfs, the rule of thumb I use here is more if I need this into a production system
or not. The content of the cdev and thermal zone stats can in fact be interpreted in both ways:
(a) on a purely debug image for a developer to check governor behavior etc, which corroborates to your view,
but also (b) in an actual production system where statistics and residency are collected in the entire
population of devices running a particular governor/settings. In the later case, debugfs is not the best fit.


> 
>   -- Daniel
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/log/?h=thermal/trip-crossed%2bdebugfs
> 
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/log/?h=thermal/debugfs-v2
> 
> [3]
> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/commit/?h=thermal/trip-crossed%2bdebugfs&id=7d713a9128ad9a153de9c3f5b854c6f1acfb3064
> 

I will take a closer look on the above. Thanks for sharing.


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

-- 
All the best,
Eduardo Valentin

[PATCH 1/7] thermal: stats: track time each dev changes due to tz

[Eduardo Valentin]

On Tue, Jun 20, 2023 at 03:43:35PM +0200, Rafael J. Wysocki wrote:
> 
> 
> 
> On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
> >
> > From: Eduardo Valentin <eduval@amazon.com>
> >
> > This patch improves the current cooling device
> > statistics by adding a new file under
> > cdev/stats/time_in_thermal_zone_ms
> > to represent the time in milliseconds
> > that the cooling device was driven by each
> > associated thermal zone.
> 
> Can you please explain the use case addressed by this?

This is a case where one cooling device is assigned to multiple thermal zones and
a data scientist/mechanical engineer/system engineer wants to understand which
one of the thermal zones drives the cooling device in question for longer time.
Say within five minutes, thermal zone A may be the dominant zone for 1 min, while
thermal zone B would be dominant for 4 min. The cumulative time counters will tell
which zone has been dominant on that cooling device since the last time the counters
were reset.

> 
> >
> > The file format is:
> > thermal_zone: <type> <time_in_ms>
> 
> So there is the "one value per sysfs attribute" rule ...


Indeed. This patch series builds on top of the existing sysfs's. See the
cdev transition table. Like so, You can think of this one as a single array of tuples :-).

> 
> > Samples:
> > $ cat /sys/class/thermal/cooling_device0/stats/time_in_thermal_zone_ms
> > thermal_zone: amb0      117496
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
> > Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
> > Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
> > Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> > Cc: linux-pm@vger.kernel.org (open list:THERMAL)
> > Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
> > Cc: linux-kernel@vger.kernel.org (open list)
> >
> > Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> > ---
> >  .../driver-api/thermal/sysfs-api.rst          |   2 +
> >  drivers/thermal/thermal_core.c                |   2 +-
> >  drivers/thermal/thermal_core.h                |   5 +
> >  drivers/thermal/thermal_helpers.c             |  11 +-
> >  drivers/thermal/thermal_sysfs.c               | 128 +++++++++++++++++-
> >  5 files changed, 139 insertions(+), 9 deletions(-)
> >
> > diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
> > index 6c1175c6afba..caa50d61a5bc 100644
> > --- a/Documentation/driver-api/thermal/sysfs-api.rst
> > +++ b/Documentation/driver-api/thermal/sysfs-api.rst
> > @@ -367,6 +367,8 @@ Thermal cooling device sys I/F, created once it's registered::
> >      |---stats/time_in_state_ms:        Time (msec) spent in various cooling states
> >      |---stats/total_trans:     Total number of times cooling state is changed
> >      |---stats/trans_table:     Cooling state transition table
> > +    |---stats/time_in_thermal_zone_ms: Time that this cooling device was driven
> > +                                each associated thermal zone.
> 
> I think that "by" is missing from the above description, but in any
> case I'm not quite sure what exactly it means.
> 
> A cooling device may be shared by multiple thermal zones which is what
> instances are for IIUC, so is this going to measure how much time the
> given thermal zone was that caused the cdev to stay in the given
> state?  Like say there are two thermal zone sharing a cdev and one of
> them says "don't care" and the other says "turn on", so the second one
> causes the cdev to enter the "on" state?

yes! Except, for clarity, this particular stat will not take into consideration
the actual tharmal instances. It is a pure view of thermal zone vs cdev.
So, read it as "time that thermal zone was ON for this cdev since last reset"

> 
> But what if both thermal zones in this example say "turn on"?

That is not possible today. There is always a single thermal zone 
that is the current ON zone, or "driving thermal zone". The cdev state
update will always solve for the highest cooling state requested among
all the thermal zones. Sure there is the case of more than one being on top.
But that is a rare case and we always pick one of the instances requesting it
anyways.


> 
> >  Then next two dynamic attributes are created/removed in pairs. They represent
> > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> > index 842f678c1c3e..4bb77af6a6f4 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -1078,7 +1078,7 @@ void thermal_cooling_device_update(struct thermal_cooling_device *cdev)
> >         if (cdev->ops->get_cur_state(cdev, &state) || state > cdev->max_state)
> >                 goto unlock;
> >
> > -       thermal_cooling_device_stats_update(cdev, state);
> > +       thermal_cooling_device_stats_update(cdev, NULL, state);
> >
> >  unlock:
> >         mutex_unlock(&cdev->lock);
> > diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> > index 3d4a787c6b28..3cce60c6e065 100644
> > --- a/drivers/thermal/thermal_core.h
> > +++ b/drivers/thermal/thermal_core.h
> > @@ -102,6 +102,9 @@ struct thermal_instance {
> >         struct list_head cdev_node; /* node in cdev->thermal_instances */
> >         unsigned int weight; /* The weight of the cooling device */
> >         bool upper_no_limit;
> > +#if IS_ENABLED(CONFIG_THERMAL_STATISTICS)
> > +       ktime_t time_in; /* time spent in this instance */
> > +#endif
> >  };
> >
> >  #define to_thermal_zone(_dev) \
> > @@ -137,10 +140,12 @@ ssize_t weight_store(struct device *, struct device_attribute *, const char *,
> >
> >  #ifdef CONFIG_THERMAL_STATISTICS
> >  void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
> > +                                        struct thermal_instance *instance,
> >                                          unsigned long new_state);
> >  #else
> >  static inline void
> >  thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
> > +                                   struct thermal_instance *instance,
> >                                     unsigned long new_state) {}
> >  #endif /* CONFIG_THERMAL_STATISTICS */
> >
> > diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
> > index cfba0965a22d..ec8e86394977 100644
> > --- a/drivers/thermal/thermal_helpers.c
> > +++ b/drivers/thermal/thermal_helpers.c
> > @@ -149,18 +149,19 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
> >  EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
> >
> >  static void thermal_cdev_set_cur_state(struct thermal_cooling_device *cdev,
> > +                                      struct thermal_instance *target_instance,
> >                                        int target)
> >  {
> >         if (cdev->ops->set_cur_state(cdev, target))
> >                 return;
> >
> >         thermal_notify_cdev_state_update(cdev->id, target);
> > -       thermal_cooling_device_stats_update(cdev, target);
> > +       thermal_cooling_device_stats_update(cdev, target_instance, target);
> >  }
> >
> >  void __thermal_cdev_update(struct thermal_cooling_device *cdev)
> >  {
> > -       struct thermal_instance *instance;
> > +       struct thermal_instance *instance, *target_instance = NULL;
> >         unsigned long target = 0;
> >
> >         /* Make sure cdev enters the deepest cooling state */
> > @@ -169,11 +170,13 @@ void __thermal_cdev_update(struct thermal_cooling_device *cdev)
> >                         instance->tz->id, instance->target);
> >                 if (instance->target == THERMAL_NO_TARGET)
> >                         continue;
> > -               if (instance->target > target)
> > +               if (instance->target > target) {
> >                         target = instance->target;
> > +                       target_instance = instance;
> > +               }
> >         }
> >
> > -       thermal_cdev_set_cur_state(cdev, target);
> > +       thermal_cdev_set_cur_state(cdev, target_instance, 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 6c20c9f90a05..a3b71f03db75 100644
> > --- a/drivers/thermal/thermal_sysfs.c
> > +++ b/drivers/thermal/thermal_sysfs.c
> > @@ -632,7 +632,7 @@ cur_state_store(struct device *dev, struct device_attribute *attr,
> >
> >         result = cdev->ops->set_cur_state(cdev, state);
> >         if (!result)
> > -               thermal_cooling_device_stats_update(cdev, state);
> > +               thermal_cooling_device_stats_update(cdev, NULL, state);
> >
> >         mutex_unlock(&cdev->lock);
> >         return result ? result : count;
> > @@ -661,6 +661,7 @@ static const struct attribute_group *cooling_device_attr_groups[] = {
> >  };
> >
> >  #ifdef CONFIG_THERMAL_STATISTICS
> > +/* thermal cooling device statistics handling */
> >  struct cooling_dev_stats {
> >         spinlock_t lock;
> >         unsigned int total_trans;
> > @@ -668,9 +669,29 @@ struct cooling_dev_stats {
> >         ktime_t last_time;
> >         ktime_t *time_in_state;
> >         unsigned int *trans_table;
> > +       struct thermal_instance *last_instance;
> > +       struct thermal_instance *curr_instance;
> >  };
> >
> > -static void update_time_in_state(struct cooling_dev_stats *stats)
> > +static void update_time_in_instance(struct cooling_dev_stats *stats,
> > +                                   struct thermal_instance *instance,
> > +                                   ktime_t now, ktime_t delta)
> > +{
> > +       if (!instance)
> > +               return;
> > +
> > +       stats->last_instance = stats->curr_instance;
> > +       stats->curr_instance = instance;
> > +
> > +       if (!stats->last_instance)
> > +               stats->last_instance = instance;
> > +
> > +       stats->last_instance->time_in =
> > +                       ktime_add(stats->last_instance->time_in, delta);
> > +}
> > +
> > +static void update_time_in_state(struct cooling_dev_stats *stats,
> > +                                struct thermal_instance *instance)
> >  {
> >         ktime_t now = ktime_get(), delta;
> >
> > @@ -678,9 +699,12 @@ static void update_time_in_state(struct cooling_dev_stats *stats)
> >         stats->time_in_state[stats->state] =
> >                 ktime_add(stats->time_in_state[stats->state], delta);
> >         stats->last_time = now;
> > +
> > +       update_time_in_instance(stats, instance, now, delta);
> >  }
> >
> >  void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
> > +                                        struct thermal_instance *instance,
> >                                          unsigned long new_state)
> >  {
> >         struct cooling_dev_stats *stats = cdev->stats;
> > @@ -695,7 +719,7 @@ void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
> >         if (stats->state == new_state)
> >                 goto unlock;
> >
> > -       update_time_in_state(stats);
> > +       update_time_in_state(stats, instance);
> >         stats->trans_table[stats->state * (cdev->max_state + 1) + new_state]++;
> >         stats->state = new_state;
> >         stats->total_trans++;
> > @@ -744,7 +768,7 @@ time_in_state_ms_show(struct device *dev, struct device_attribute *attr,
> >
> >         spin_lock(&stats->lock);
> >
> > -       update_time_in_state(stats);
> > +       update_time_in_state(stats, stats->curr_instance);
> >
> >         for (i = 0; i <= cdev->max_state; i++) {
> >                 len += sprintf(buf + len, "state%u\t%llu\n", i,
> > @@ -758,12 +782,98 @@ time_in_state_ms_show(struct device *dev, struct device_attribute *attr,
> >         return len;
> >  }
> >
> > +struct cdev_thermal_zone_residency {
> > +       char thermal_zone[THERMAL_NAME_LENGTH];
> > +       ktime_t time_in;
> > +       unsigned long counter;
> > +       struct list_head node; /* we build this as we go */
> > +};
> 
> What is represented by this structure?

This is a struct with stats associate with each thermal zone that are assigned to the cdev.
Up to this patch, it holds two stats, number of times the zone was ON for this cdev
and duration (time_in) that the zone was ON. I can add a comment for this.

> 
> > +
> > +static void
> > +build_cdev_thermal_zone_residency(struct list_head *list,
> > +                                 struct thermal_cooling_device *cdev)
> > +{
> > +       struct cdev_thermal_zone_residency *res, *update_res;
> > +       struct thermal_instance *instance;
> > +
> > +       /*
> > +        * Build an array of pairs <thermal zone, time> to represent
> > +        * how this cooling device was driven by each thermal zone
> > +        */
> > +       list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
> > +               update_res = NULL;
> > +
> > +               list_for_each_entry(res, list, node) {
> > +                       if (strncmp(res->thermal_zone, instance->tz->type,
> > +                                   THERMAL_NAME_LENGTH) == 0)
> > +                               update_res = res;
> > +               }
> > +               if (!update_res) {
> > +                       update_res = kzalloc(sizeof(*update_res), GFP_KERNEL);
> > +                       strscpy(update_res->thermal_zone,
> > +                               instance->tz->type, THERMAL_NAME_LENGTH);
> > +                       list_add_tail(&update_res->node, list);
> > +               }
> > +       }
> > +}
> > +
> > +static ssize_t
> > +time_in_thermal_zone_ms_show(struct device *dev, struct device_attribute *attr,
> > +                            char *buf)
> > +{
> > +       LIST_HEAD(cdev_thermal_zone_list);
> > +       struct thermal_cooling_device *cdev = to_cooling_device(dev);
> > +       struct cooling_dev_stats *stats = cdev->stats;
> > +       struct cdev_thermal_zone_residency *res, *next;
> > +       struct thermal_instance *instance;
> > +       ssize_t len = 0, ret = 0;
> > +
> > +       mutex_lock(&cdev->lock);
> > +
> > +       spin_lock(&stats->lock);
> > +       update_time_in_state(stats, stats->curr_instance);
> > +       spin_unlock(&stats->lock);
> > +
> > +       build_cdev_thermal_zone_residency(&cdev_thermal_zone_list, cdev);
> > +
> > +       list_for_each_entry(instance, &cdev->thermal_instances, cdev_node)
> > +               list_for_each_entry(res, &cdev_thermal_zone_list, node)
> > +                       if (strncmp(res->thermal_zone, instance->tz->type,
> > +                                   THERMAL_NAME_LENGTH) == 0)
> > +                               res->time_in = ktime_add(res->time_in,
> > +                                                        instance->time_in);
> > +
> > +       mutex_unlock(&cdev->lock);
> > +
> > +       list_for_each_entry_safe(res, next, &cdev_thermal_zone_list, node) {
> > +               ret = snprintf(buf + len, PAGE_SIZE - len,
> > +                              "thermal_zone: %s\t%llu\n",
> > +                              res->thermal_zone, ktime_to_ms(res->time_in));
> > +
> > +               if (ret == 0)
> > +                       ret = -EOVERFLOW;
> > +
> > +               if (ret < 0)
> > +                       break;
> > +
> > +               len += ret;
> > +       }
> 
> Why does the above loop need to use the _safe variant?

It doesnt. Copy and Pasta.

> 
> > +
> > +       list_for_each_entry_safe(res, next, &cdev_thermal_zone_list, node) {
> > +               list_del(&res->node);
> > +               kfree(res);
> > +       }
> > +
> > +       return ret < 0 ? ret : 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;
> > +       struct thermal_instance *instance;
> >         int i, states;
> >
> >         mutex_lock(&cdev->lock);
> > @@ -774,6 +884,7 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
> >
> >         states = cdev->max_state + 1;
> >
> > +       mutex_lock(&cdev->lock);
> >         spin_lock(&stats->lock);
> >
> >         stats->total_trans = 0;
> > @@ -784,7 +895,14 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
> >         for (i = 0; i < states; i++)
> >                 stats->time_in_state[i] = ktime_set(0, 0);
> >
> > +       /* Make sure we reset all counters per instance */
> > +       list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
> > +               instance->time_in = ktime_set(0, 0);
> > +       }
> > +
> >         spin_unlock(&stats->lock);
> > +       mutex_unlock(&cdev->lock);
> > +
> >
> >  unlock:
> >         mutex_unlock(&cdev->lock);
> > @@ -852,12 +970,14 @@ static ssize_t trans_table_show(struct device *dev,
> >
> >  static DEVICE_ATTR_RO(total_trans);
> >  static DEVICE_ATTR_RO(time_in_state_ms);
> > +static DEVICE_ATTR_RO(time_in_thermal_zone_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_time_in_thermal_zone_ms.attr,
> >         &dev_attr_reset.attr,
> >         &dev_attr_trans_table.attr,
> >         NULL
> > --

-- 
All the best,
Eduardo Valentin

[PATCH 2/7] thermal: stats: track number of change requests due to tz

[Eduardo Valentin]

On Tue, Jun 20, 2023 at 07:12:52PM +0200, Rafael J. Wysocki wrote:
> 
> 
> 
> On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
> >
> > From: Eduardo Valentin <eduval@amazon.com>
> >
> > This patch improves the current cooling device
> > statistics by adding a new file under
> > cdev/stats/requests_of_thermal_zone
> >
> > to represent the number of times each thermal zone
> > requested the cooling device to effectively change.
> > If the request associated was not serviced because
> > another thermal zone asked for a higher cooling level,
> > this counter does not increase.
> 
> What if the cdev is associated with two thermal zones asking for the
> same state of it?

same as explained before, there will be always one thermal instance
that is picked. This patch considers that.

> 
> > The file format is:
> > thermal_zone: <type> <count>
> >
> > Samples:
> > $ cat cdev0/stats/requests_of_thermal_zone
> > thermal_zone: amb0      2
> 
> The "one value per attribute" sysfs rule violation.
> 
> >
> > In this example, it means the thermal zone 'amb0' has requested
> > 2 times for cdev0 to change state.
> 
> Like in the previous patch, it would be good to explain the use case.
> 
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
> > Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
> > Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
> > Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> > Cc: linux-pm@vger.kernel.org (open list:THERMAL)
> > Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
> > Cc: linux-kernel@vger.kernel.org (open list)
> >
> > Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> > ---
> >  .../driver-api/thermal/sysfs-api.rst          |  2 +
> >  drivers/thermal/thermal_core.h                |  1 +
> >  drivers/thermal/thermal_sysfs.c               | 52 +++++++++++++++++++
> >  3 files changed, 55 insertions(+)
> >
> > diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
> > index caa50d61a5bc..75309a51d9b3 100644
> > --- a/Documentation/driver-api/thermal/sysfs-api.rst
> > +++ b/Documentation/driver-api/thermal/sysfs-api.rst
> > @@ -369,6 +369,8 @@ Thermal cooling device sys I/F, created once it's registered::
> >      |---stats/trans_table:     Cooling state transition table
> >      |---stats/time_in_thermal_zone_ms: Time that this cooling device was driven
> >                                  each associated thermal zone.
> > +    |---stats/requests_of_thermal_zone:        Total number of times this cooling device
> > +                                changed due to each associated thermal zone.
> 
> The meaning of the above description is not clear to me.
> 
> >
> >
> >  Then next two dynamic attributes are created/removed in pairs. They represent
> > diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> > index 3cce60c6e065..ed6511c3b794 100644
> > --- a/drivers/thermal/thermal_core.h
> > +++ b/drivers/thermal/thermal_core.h
> > @@ -103,6 +103,7 @@ struct thermal_instance {
> >         unsigned int weight; /* The weight of the cooling device */
> >         bool upper_no_limit;
> >  #if IS_ENABLED(CONFIG_THERMAL_STATISTICS)
> > +       unsigned long total_requests;
> >         ktime_t time_in; /* time spent in this instance */
> >  #endif
> >  };
> > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> > index a3b71f03db75..0bce1415f7e8 100644
> > --- a/drivers/thermal/thermal_sysfs.c
> > +++ b/drivers/thermal/thermal_sysfs.c
> > @@ -723,6 +723,7 @@ void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
> >         stats->trans_table[stats->state * (cdev->max_state + 1) + new_state]++;
> >         stats->state = new_state;
> >         stats->total_trans++;
> > +       stats->curr_instance->total_requests++;
> >
> >  unlock:
> >         spin_unlock(&stats->lock);
> > @@ -867,6 +868,54 @@ time_in_thermal_zone_ms_show(struct device *dev, struct device_attribute *attr,
> >         return ret < 0 ? ret : len;
> >  }
> >
> > +static ssize_t
> > +requests_of_thermal_zone_show(struct device *dev, struct device_attribute *attr,
> > +                             char *buf)
> > +{
> > +       LIST_HEAD(cdev_thermal_zone_list);
> > +       struct thermal_cooling_device *cdev = to_cooling_device(dev);
> > +       struct cooling_dev_stats *stats = cdev->stats;
> > +       struct cdev_thermal_zone_residency *res, *next;
> > +       struct thermal_instance *instance;
> > +       ssize_t len = 0, ret = 0;
> > +
> > +       mutex_lock(&cdev->lock);
> > +
> > +       spin_lock(&stats->lock);
> > +       update_time_in_state(stats, stats->curr_instance);
> > +       spin_unlock(&stats->lock);
> > +
> > +       build_cdev_thermal_zone_residency(&cdev_thermal_zone_list, cdev);
> > +
> > +       list_for_each_entry(instance, &cdev->thermal_instances, cdev_node)
> > +               list_for_each_entry(res, &cdev_thermal_zone_list, node)
> > +                       if (strncmp(res->thermal_zone, instance->tz->type,
> > +                                   THERMAL_NAME_LENGTH) == 0)
> > +                               res->counter += instance->total_requests;
> > +
> > +       mutex_unlock(&cdev->lock);
> > +
> > +       list_for_each_entry_safe(res, next, &cdev_thermal_zone_list, node) {
> 
> Why is the _safe variant needed here?
> 
> > +               ret = sprintf(buf + len, "thermal_zone: %s\t%lu\n",
> > +                             res->thermal_zone, res->counter);
> > +
> > +               if (ret == 0)
> > +                       ret = -EOVERFLOW;
> > +
> > +               if (ret < 0)
> > +                       break;
> > +
> > +               len += ret;
> > +       }
> > +
> > +       list_for_each_entry_safe(res, next, &cdev_thermal_zone_list, node) {
> > +               list_del(&res->node);
> > +               kfree(res);
> > +       }
> > +
> > +       return ret < 0 ? ret : len;
> 
> I would prefer
> 
> if (ret < 0)
>         return ret;
> 
> return len;

OK. I will fix this and enhance the explanations.

And the same comments and replies of the previous patch applies here.

> 
> > +}
> > +
> >  static ssize_t
> >  reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
> >             size_t count)
> > @@ -897,6 +946,7 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
> >
> >         /* Make sure we reset all counters per instance */
> >         list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
> > +               instance->total_requests = 0;
> >                 instance->time_in = ktime_set(0, 0);
> >         }
> >
> > @@ -971,6 +1021,7 @@ static ssize_t trans_table_show(struct device *dev,
> >  static DEVICE_ATTR_RO(total_trans);
> >  static DEVICE_ATTR_RO(time_in_state_ms);
> >  static DEVICE_ATTR_RO(time_in_thermal_zone_ms);
> > +static DEVICE_ATTR_RO(requests_of_thermal_zone);
> >  static DEVICE_ATTR_WO(reset);
> >  static DEVICE_ATTR_RO(trans_table);
> >
> > @@ -978,6 +1029,7 @@ static struct attribute *cooling_device_stats_attrs[] = {
> >         &dev_attr_total_trans.attr,
> >         &dev_attr_time_in_state_ms.attr,
> >         &dev_attr_time_in_thermal_zone_ms.attr,
> > +       &dev_attr_requests_of_thermal_zone.attr,
> >         &dev_attr_reset.attr,
> >         &dev_attr_trans_table.attr,
> >         NULL
> > --

-- 
All the best,
Eduardo Valentin

[PATCH 5/7] thermal: stats: introduce tz time in trip

[Eduardo Valentin]

On Tue, Jun 20, 2023 at 07:27:57PM +0200, Rafael J. Wysocki wrote:
> 
> 
> 
> On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
> >
> > From: Eduardo Valentin <eduval@amazon.com>
> >
> > This patch adds a statistic to report how long
> > the thermal zone spent on temperature intervals
> > created by each trip point. The first interval
> > is the range below the first trip point. All
> > subsequent intervals are accounted when temperature
> > is above the trip point temperature value.
> >
> > Samples:
> > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > trip-1  0       0
> 
> The above line is confusing.
> 
> > trip0   -10000  35188
> > trip1   25000   0
> 
> And the format violates the "one value per attribute" sysfs rule.
> 
> > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > trip-1  0       0
> > trip0   -10000  36901
> > trip1   25000   0
> > $ echo 25001 > /sys//class/thermal/thermal_zone0/emul_temp
> > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > trip-1  0       0
> > trip0   -10000  47810
> > trip1   25000   2259
> > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > trip-1  0       0
> > trip0   -10000  47810
> > trip1   25000   3224
> > $ echo 24001 > /sys//class/thermal/thermal_zone0/emul_temp
> > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > trip-1  0       0
> > trip0   -10000  48960
> > trip1   25000   10080
> > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > trip-1  0       0
> > trip0   -10000  49844
> > trip1   25000   10080
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
> > Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
> > Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
> > Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> > Cc: linux-pm@vger.kernel.org (open list:THERMAL)
> > Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
> > Cc: linux-kernel@vger.kernel.org (open list)
> >
> > Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> > ---
> >  .../driver-api/thermal/sysfs-api.rst          |  2 +
> >  drivers/thermal/thermal_sysfs.c               | 86 +++++++++++++++++++
> >  2 files changed, 88 insertions(+)
> >
> > diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
> > index ed5e6ba4e0d7..4a2b92a7488c 100644
> > --- a/Documentation/driver-api/thermal/sysfs-api.rst
> > +++ b/Documentation/driver-api/thermal/sysfs-api.rst
> > @@ -359,6 +359,8 @@ Thermal zone device sys I/F, created once it's registered::
> >      |---stats/reset_tz_stats:  Writes to this file resets the statistics.
> >      |---stats/max_gradient:    The maximum recorded dT/dt in uC/ms.
> >      |---stats/min_gradient:    The minimum recorded dT/dt in uC/ms.
> > +    |---stats/time_in_trip_ms: Time spent on each temperature interval of
> > +                               trip points.
> 
> I would write "in each temperature interval between consecutive trip points".

Ok

> 
> Doesn't this assume a specific temperature ordering of trip points?
> And so what if they are not ordered?

It does. I believe other things will break if they are not ordered. Like the temperature update
against the governor throttle callback invocation in the thermal core.


-- 
All the best,
Eduardo Valentin

[PATCH 6/7] ythermal: core: report errors to governors

[Eduardo Valentin]

On Tue, Jun 20, 2023 at 07:29:57PM +0200, Rafael J. Wysocki wrote:
> 
> 
> 
> On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
> >
> > From: Eduardo Valentin <eduval@amazon.com>
> >
> > Currently the thermal governors are not allowed to
> > react on temperature error events as the thermal core
> > skips the handling and logs an error on kernel buffer.
> > This patch adds the opportunity to report the errors
> > when they happen to governors.
> >
> > Now, if a governor wants to react on temperature read
> > errors, they can implement the .check_error() callback.
> 
> Explaining the use case for this would help a lot.


Yeah I agree. I also did not send the full series and will also add
the governor changes for this in the next patch series.

The use case here is primarily when temperature reads can fail.
Common use case, not limited to though, is an I2C device temperature sensor.
While it can be, in many cases, reliable, it is not always guaranteed to
have a successful temperature read. In fact, it is common to see a sporadic
temperature read failure, followed by successful reads.

This patch series will enhance the core to allow temperature update
error communication to the governor so the governor can have the
opportunity to act upon sensor failure.

-- 
All the best,
Eduardo Valentin

[PATCH 7/7] thermal: stats: add error accounting to thermal zone

[Eduardo Valentin]

On Tue, Jun 20, 2023 at 07:32:45PM +0200, Rafael J. Wysocki wrote:
> 
> 
> 
> On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
> >
> > From: Eduardo Valentin <eduval@amazon.com>
> >
> > This patch adds an extra stat to report how many
> > temperature update failures were detected.
> > Error count is increase whenever the thermal
> > driver returns an actual error or when the temperature
> > is non positive.
> 
> Why can't the temperature be negative?

I sent a wrong version here. My bad. I also need
to cover for proper negative temperature. Will send an update
on V2.

-- 
All the best,
Eduardo Valentin

Re: [PATCH 5/7] thermal: stats: introduce tz time in trip

[Rafael J. Wysocki]

On Wed, Jun 21, 2023 at 6:45 AM Eduardo Valentin <evalenti@kernel.org> wrote:
>
> On Tue, Jun 20, 2023 at 07:27:57PM +0200, Rafael J. Wysocki wrote:
> >
> >
> >
> > On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
> > >
> > > From: Eduardo Valentin <eduval@amazon.com>
> > >
> > > This patch adds a statistic to report how long
> > > the thermal zone spent on temperature intervals
> > > created by each trip point. The first interval
> > > is the range below the first trip point. All
> > > subsequent intervals are accounted when temperature
> > > is above the trip point temperature value.
> > >
> > > Samples:
> > > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > > trip-1  0       0
> >
> > The above line is confusing.
> >
> > > trip0   -10000  35188
> > > trip1   25000   0
> >
> > And the format violates the "one value per attribute" sysfs rule.
> >
> > > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > > trip-1  0       0
> > > trip0   -10000  36901
> > > trip1   25000   0
> > > $ echo 25001 > /sys//class/thermal/thermal_zone0/emul_temp
> > > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > > trip-1  0       0
> > > trip0   -10000  47810
> > > trip1   25000   2259
> > > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > > trip-1  0       0
> > > trip0   -10000  47810
> > > trip1   25000   3224
> > > $ echo 24001 > /sys//class/thermal/thermal_zone0/emul_temp
> > > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > > trip-1  0       0
> > > trip0   -10000  48960
> > > trip1   25000   10080
> > > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > > trip-1  0       0
> > > trip0   -10000  49844
> > > trip1   25000   10080
> > >
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
> > > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
> > > Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
> > > Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
> > > Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> > > Cc: linux-pm@vger.kernel.org (open list:THERMAL)
> > > Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
> > > Cc: linux-kernel@vger.kernel.org (open list)
> > >
> > > Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> > > ---
> > >  .../driver-api/thermal/sysfs-api.rst          |  2 +
> > >  drivers/thermal/thermal_sysfs.c               | 86 +++++++++++++++++++
> > >  2 files changed, 88 insertions(+)
> > >
> > > diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
> > > index ed5e6ba4e0d7..4a2b92a7488c 100644
> > > --- a/Documentation/driver-api/thermal/sysfs-api.rst
> > > +++ b/Documentation/driver-api/thermal/sysfs-api.rst
> > > @@ -359,6 +359,8 @@ Thermal zone device sys I/F, created once it's registered::
> > >      |---stats/reset_tz_stats:  Writes to this file resets the statistics.
> > >      |---stats/max_gradient:    The maximum recorded dT/dt in uC/ms.
> > >      |---stats/min_gradient:    The minimum recorded dT/dt in uC/ms.
> > > +    |---stats/time_in_trip_ms: Time spent on each temperature interval of
> > > +                               trip points.
> >
> > I would write "in each temperature interval between consecutive trip points".
>
> Ok
>
> >
> > Doesn't this assume a specific temperature ordering of trip points?
> > And so what if they are not ordered?
>
> It does. I believe other things will break if they are not ordered.

But there's no guarantee that they will be ordered, so it looks like
those other things are already broken.

[PATCH 5/7] thermal: stats: introduce tz time in trip

[Eduardo Valentin]

On Fri, Jun 23, 2023 at 06:40:20PM +0200, Rafael J. Wysocki wrote:
> 
> 
> 
> On Wed, Jun 21, 2023 at 6:45 AM Eduardo Valentin <evalenti@kernel.org> wrote:
> >
> > On Tue, Jun 20, 2023 at 07:27:57PM +0200, Rafael J. Wysocki wrote:
> > >
> > >
> > >
> > > On Fri, May 19, 2023 at 5:27 AM Eduardo Valentin <evalenti@kernel.org> wrote:
> > > >
> > > > From: Eduardo Valentin <eduval@amazon.com>
> > > >
> > > > This patch adds a statistic to report how long
> > > > the thermal zone spent on temperature intervals
> > > > created by each trip point. The first interval
> > > > is the range below the first trip point. All
> > > > subsequent intervals are accounted when temperature
> > > > is above the trip point temperature value.
> > > >
> > > > Samples:
> > > > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > > > trip-1  0       0
> > >
> > > The above line is confusing.
> > >
> > > > trip0   -10000  35188
> > > > trip1   25000   0
> > >
> > > And the format violates the "one value per attribute" sysfs rule.
> > >
> > > > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > > > trip-1  0       0
> > > > trip0   -10000  36901
> > > > trip1   25000   0
> > > > $ echo 25001 > /sys//class/thermal/thermal_zone0/emul_temp
> > > > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > > > trip-1  0       0
> > > > trip0   -10000  47810
> > > > trip1   25000   2259
> > > > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > > > trip-1  0       0
> > > > trip0   -10000  47810
> > > > trip1   25000   3224
> > > > $ echo 24001 > /sys//class/thermal/thermal_zone0/emul_temp
> > > > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > > > trip-1  0       0
> > > > trip0   -10000  48960
> > > > trip1   25000   10080
> > > > $ cat /sys//class/thermal/thermal_zone0/stats/time_in_trip_ms
> > > > trip-1  0       0
> > > > trip0   -10000  49844
> > > > trip1   25000   10080
> > > >
> > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
> > > > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
> > > > Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
> > > > Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
> > > > Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> > > > Cc: linux-pm@vger.kernel.org (open list:THERMAL)
> > > > Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
> > > > Cc: linux-kernel@vger.kernel.org (open list)
> > > >
> > > > Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> > > > ---
> > > >  .../driver-api/thermal/sysfs-api.rst          |  2 +
> > > >  drivers/thermal/thermal_sysfs.c               | 86 +++++++++++++++++++
> > > >  2 files changed, 88 insertions(+)
> > > >
> > > > diff --git a/Documentation/driver-api/thermal/sysfs-api.rst b/Documentation/driver-api/thermal/sysfs-api.rst
> > > > index ed5e6ba4e0d7..4a2b92a7488c 100644
> > > > --- a/Documentation/driver-api/thermal/sysfs-api.rst
> > > > +++ b/Documentation/driver-api/thermal/sysfs-api.rst
> > > > @@ -359,6 +359,8 @@ Thermal zone device sys I/F, created once it's registered::
> > > >      |---stats/reset_tz_stats:  Writes to this file resets the statistics.
> > > >      |---stats/max_gradient:    The maximum recorded dT/dt in uC/ms.
> > > >      |---stats/min_gradient:    The minimum recorded dT/dt in uC/ms.
> > > > +    |---stats/time_in_trip_ms: Time spent on each temperature interval of
> > > > +                               trip points.
> > >
> > > I would write "in each temperature interval between consecutive trip points".
> >
> > Ok
> >
> > >
> > > Doesn't this assume a specific temperature ordering of trip points?
> > > And so what if they are not ordered?
> >
> > It does. I believe other things will break if they are not ordered.
> 
> But there's no guarantee that they will be ordered, so it looks like
> those other things are already broken.

Correct. (1) there is no guarantee, it works by construction, and (2) current
code does assume ascending order, so yes, if they come unsorted, the core
code will not properly work.

Ensuring the order is likely beyond the original intention of this patch, but
we do need to improve there, for sure.

-- 
All the best,
Eduardo Valentin