[PATCH v3 0/6] thermal: More separation between the core and drivers

[PATCH v3 0/6] thermal: More separation between the core and drivers

[Rafael J. Wysocki]

Hi Everyone,

This is an update of

https://lore.kernel.org/linux-pm/4558384.LvFx2qVVIh@kreacher/

and

https://lore.kernel.org/linux-pm/2331888.ElGaqSPkdT@kreacher/

which rebases the first patch on top of 6.9-rc2, adds 3 patches and adjusts
the third patch from v2.

The original description of the first two patches still applies:

> Patch [1/2] is based on the observation that the threshold field in struct
> thermal_trip really should be core-internal and to make that happen it
> introduces a wrapper structure around struct thermal_trip for internal
> use in the core.
> 
> Patch [2/2] moves the definition of the new structure and the struct
> thermal_zone_device one to a local header file in the core to enforce
> more separation between the core and drivers.
> 
> The patches are not expected to introduce any observable differences in
> behavior, so please let me know if you see any of that.

Note that these patches were first sent before the merge window and have not
really changed since then (except for a minor rebase of the first patch in
this series).  Moreover, no comments regarding the merit of these patches
have been made shared, so if this continues, I will be considering them as
good to go by the end of this week.

Patch [3/6] is a rewrite of comments regarding trip crossing and threshold
computations.

Patch [4/6] updates the trip crossing detection code to consolidate the
threshold initialization with trip crossing on the way up.

Patch [5/6] ([3/3] in v2) adds a mechanism to sort notifications and debug
calls taking place during one invocation of __thermal_zone_device_update() so
they always go in temperature order.

Patch [6/6] relocates the critical and trip point handling to avoid a
redundant temperature check.

The series applies on top of 6.9-rc2 and I'm planning to create a test
branch containing it.

Thanks!

[PATCH v3 1/6] thermal: core: Move threshold out of struct thermal_trip

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The threshold field in struct thermal_trip is only used internally by
the thermal core and it is better to prevent drivers from misusing it.
It also takes some space unnecessarily in the trip tables passed by
drivers to the core during thermal zone registration.

For this reason, introduce struct thermal_trip_desc as a wrapper around
struct thermal_trip, move the threshold field directly into it and make
the thermal core store struct thermal_trip_desc objects in the internal
thermal zone trip tables.  Adjust all of the code using trip tables in
the thermal core accordingly.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v2 -> v3: Rebase on top of 6.9-rc2, minor changelog update.

v1 -> v2: No changes.

---
 drivers/thermal/gov_fair_share.c      |    7 +++--
 drivers/thermal/gov_power_allocator.c |    6 ++--
 drivers/thermal/thermal_core.c        |   46 +++++++++++++++++++---------------
 drivers/thermal/thermal_core.h        |    7 +++--
 drivers/thermal/thermal_debugfs.c     |    6 ++--
 drivers/thermal/thermal_helpers.c     |    8 +++--
 drivers/thermal/thermal_netlink.c     |    6 ++--
 drivers/thermal/thermal_sysfs.c       |   20 +++++++-------
 drivers/thermal/thermal_trip.c        |   15 +++++------
 include/linux/thermal.h               |    9 ++++--
 10 files changed, 78 insertions(+), 52 deletions(-)

Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -61,7 +61,6 @@ enum thermal_notify_event {
  * struct thermal_trip - representation of a point in temperature domain
  * @temperature: temperature value in miliCelsius
  * @hysteresis: relative hysteresis in miliCelsius
- * @threshold: trip crossing notification threshold miliCelsius
  * @type: trip point type
  * @priv: pointer to driver data associated with this trip
  * @flags: flags representing binary properties of the trip
@@ -69,12 +68,16 @@ enum thermal_notify_event {
 struct thermal_trip {
 	int temperature;
 	int hysteresis;
-	int threshold;
 	enum thermal_trip_type type;
 	u8 flags;
 	void *priv;
 };
 
+struct thermal_trip_desc {
+	struct thermal_trip trip;
+	int threshold;
+};
+
 #define THERMAL_TRIP_FLAG_RW_TEMP	BIT(0)
 #define THERMAL_TRIP_FLAG_RW_HYST	BIT(1)
 
@@ -203,7 +206,7 @@ struct thermal_zone_device {
 #ifdef CONFIG_THERMAL_DEBUGFS
 	struct thermal_debugfs *debugfs;
 #endif
-	struct thermal_trip trips[] __counted_by(num_trips);
+	struct thermal_trip_desc trips[] __counted_by(num_trips);
 };
 
 /**
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -361,17 +361,19 @@ static void handle_critical_trips(struct
 }
 
 static void handle_thermal_trip(struct thermal_zone_device *tz,
-				struct thermal_trip *trip)
+				struct thermal_trip_desc *td)
 {
+	const struct thermal_trip *trip = &td->trip;
+
 	if (trip->temperature == THERMAL_TEMP_INVALID)
 		return;
 
 	if (tz->last_temperature == THERMAL_TEMP_INVALID) {
 		/* Initialization. */
-		trip->threshold = trip->temperature;
-		if (tz->temperature >= trip->threshold)
-			trip->threshold -= trip->hysteresis;
-	} else if (tz->last_temperature < trip->threshold) {
+		td->threshold = trip->temperature;
+		if (tz->temperature >= td->threshold)
+			td->threshold -= trip->hysteresis;
+	} else if (tz->last_temperature < td->threshold) {
 		/*
 		 * The trip threshold is equal to the trip temperature, unless
 		 * the latter has changed in the meantime.  In either case,
@@ -382,9 +384,9 @@ static void handle_thermal_trip(struct t
 		if (tz->temperature >= trip->temperature) {
 			thermal_notify_tz_trip_up(tz, trip);
 			thermal_debug_tz_trip_up(tz, trip);
-			trip->threshold = trip->temperature - trip->hysteresis;
+			td->threshold = trip->temperature - trip->hysteresis;
 		} else {
-			trip->threshold = trip->temperature;
+			td->threshold = trip->temperature;
 		}
 	} else {
 		/*
@@ -400,9 +402,9 @@ static void handle_thermal_trip(struct t
 		if (tz->temperature < trip->temperature - trip->hysteresis) {
 			thermal_notify_tz_trip_down(tz, trip);
 			thermal_debug_tz_trip_down(tz, trip);
-			trip->threshold = trip->temperature;
+			td->threshold = trip->temperature;
 		} else {
-			trip->threshold = trip->temperature - trip->hysteresis;
+			td->threshold = trip->temperature - trip->hysteresis;
 		}
 	}
 
@@ -458,7 +460,7 @@ static void thermal_zone_device_init(str
 void __thermal_zone_device_update(struct thermal_zone_device *tz,
 				  enum thermal_notify_event event)
 {
-	struct thermal_trip *trip;
+	struct thermal_trip_desc *td;
 
 	if (tz->suspended)
 		return;
@@ -472,8 +474,8 @@ void __thermal_zone_device_update(struct
 
 	tz->notify_event = event;
 
-	for_each_trip(tz, trip)
-		handle_thermal_trip(tz, trip);
+	for_each_trip_desc(tz, td)
+		handle_thermal_trip(tz, td);
 
 	monitor_thermal_zone(tz);
 }
@@ -766,7 +768,7 @@ int thermal_zone_bind_cooling_device(str
 	if (trip_index < 0 || trip_index >= tz->num_trips)
 		return -EINVAL;
 
-	return thermal_bind_cdev_to_trip(tz, &tz->trips[trip_index], cdev,
+	return thermal_bind_cdev_to_trip(tz, &tz->trips[trip_index].trip, cdev,
 					 upper, lower, weight);
 }
 EXPORT_SYMBOL_GPL(thermal_zone_bind_cooling_device);
@@ -825,7 +827,7 @@ int thermal_zone_unbind_cooling_device(s
 	if (trip_index < 0 || trip_index >= tz->num_trips)
 		return -EINVAL;
 
-	return thermal_unbind_cdev_from_trip(tz, &tz->trips[trip_index], cdev);
+	return thermal_unbind_cdev_from_trip(tz, &tz->trips[trip_index].trip, cdev);
 }
 EXPORT_SYMBOL_GPL(thermal_zone_unbind_cooling_device);
 
@@ -1221,16 +1223,19 @@ static void thermal_set_delay_jiffies(un
 
 int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp)
 {
-	int i, ret = -EINVAL;
+	const struct thermal_trip_desc *td;
+	int ret = -EINVAL;
 
 	if (tz->ops.get_crit_temp)
 		return tz->ops.get_crit_temp(tz, temp);
 
 	mutex_lock(&tz->lock);
 
-	for (i = 0; i < tz->num_trips; i++) {
-		if (tz->trips[i].type == THERMAL_TRIP_CRITICAL) {
-			*temp = tz->trips[i].temperature;
+	for_each_trip_desc(tz, td) {
+		const struct thermal_trip *trip = &td->trip;
+
+		if (trip->type == THERMAL_TRIP_CRITICAL) {
+			*temp = trip->temperature;
 			ret = 0;
 			break;
 		}
@@ -1274,7 +1279,9 @@ thermal_zone_device_register_with_trips(
 					const struct thermal_zone_params *tzp,
 					int passive_delay, int polling_delay)
 {
+	const struct thermal_trip *trip = trips;
 	struct thermal_zone_device *tz;
+	struct thermal_trip_desc *td;
 	int id;
 	int result;
 	struct thermal_governor *governor;
@@ -1339,7 +1346,8 @@ thermal_zone_device_register_with_trips(
 	tz->device.class = thermal_class;
 	tz->devdata = devdata;
 	tz->num_trips = num_trips;
-	memcpy(tz->trips, trips, num_trips * sizeof(*trips));
+	for_each_trip_desc(tz, td)
+		td->trip = *trip++;
 
 	thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
 	thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -120,8 +120,11 @@ void thermal_governor_update_tz(struct t
 				enum thermal_notify_event reason);
 
 /* Helpers */
-#define for_each_trip(__tz, __trip)	\
-	for (__trip = __tz->trips; __trip - __tz->trips < __tz->num_trips; __trip++)
+#define for_each_trip_desc(__tz, __td)	\
+	for (__td = __tz->trips; __td - __tz->trips < __tz->num_trips; __td++)
+
+#define trip_to_trip_desc(__trip)	\
+	container_of(__trip, struct thermal_trip_desc, trip)
 
 void __thermal_zone_set_trips(struct thermal_zone_device *tz);
 int thermal_zone_trip_id(const struct thermal_zone_device *tz,
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -88,7 +88,7 @@ trip_point_type_show(struct device *dev,
 	if (sscanf(attr->attr.name, "trip_point_%d_type", &trip_id) != 1)
 		return -EINVAL;
 
-	switch (tz->trips[trip_id].type) {
+	switch (tz->trips[trip_id].trip.type) {
 	case THERMAL_TRIP_CRITICAL:
 		return sprintf(buf, "critical\n");
 	case THERMAL_TRIP_HOT:
@@ -120,7 +120,7 @@ trip_point_temp_store(struct device *dev
 
 	mutex_lock(&tz->lock);
 
-	trip = &tz->trips[trip_id];
+	trip = &tz->trips[trip_id].trip;
 
 	if (temp != trip->temperature) {
 		if (tz->ops.set_trip_temp) {
@@ -150,7 +150,7 @@ trip_point_temp_show(struct device *dev,
 	if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip_id) != 1)
 		return -EINVAL;
 
-	return sprintf(buf, "%d\n", tz->trips[trip_id].temperature);
+	return sprintf(buf, "%d\n", tz->trips[trip_id].trip.temperature);
 }
 
 static ssize_t
@@ -171,7 +171,7 @@ trip_point_hyst_store(struct device *dev
 
 	mutex_lock(&tz->lock);
 
-	trip = &tz->trips[trip_id];
+	trip = &tz->trips[trip_id].trip;
 
 	if (hyst != trip->hysteresis) {
 		trip->hysteresis = hyst;
@@ -194,7 +194,7 @@ trip_point_hyst_show(struct device *dev,
 	if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip_id) != 1)
 		return -EINVAL;
 
-	return sprintf(buf, "%d\n", tz->trips[trip_id].hysteresis);
+	return sprintf(buf, "%d\n", tz->trips[trip_id].trip.hysteresis);
 }
 
 static ssize_t
@@ -393,7 +393,7 @@ static const struct attribute_group *the
  */
 static int create_trip_attrs(struct thermal_zone_device *tz)
 {
-	const struct thermal_trip *trip;
+	const struct thermal_trip_desc *td;
 	struct attribute **attrs;
 
 	/* This function works only for zones with at least one trip */
@@ -429,8 +429,8 @@ static int create_trip_attrs(struct ther
 		return -ENOMEM;
 	}
 
-	for_each_trip(tz, trip) {
-		int indx = thermal_zone_trip_id(tz, trip);
+	for_each_trip_desc(tz, td) {
+		int indx = thermal_zone_trip_id(tz, &td->trip);
 
 		/* create trip type attribute */
 		snprintf(tz->trip_type_attrs[indx].name, THERMAL_NAME_LENGTH,
@@ -452,7 +452,7 @@ static int create_trip_attrs(struct ther
 						tz->trip_temp_attrs[indx].name;
 		tz->trip_temp_attrs[indx].attr.attr.mode = S_IRUGO;
 		tz->trip_temp_attrs[indx].attr.show = trip_point_temp_show;
-		if (trip->flags & THERMAL_TRIP_FLAG_RW_TEMP) {
+		if (td->trip.flags & THERMAL_TRIP_FLAG_RW_TEMP) {
 			tz->trip_temp_attrs[indx].attr.attr.mode |= S_IWUSR;
 			tz->trip_temp_attrs[indx].attr.store =
 							trip_point_temp_store;
@@ -467,7 +467,7 @@ static int create_trip_attrs(struct ther
 					tz->trip_hyst_attrs[indx].name;
 		tz->trip_hyst_attrs[indx].attr.attr.mode = S_IRUGO;
 		tz->trip_hyst_attrs[indx].attr.show = trip_point_hyst_show;
-		if (trip->flags & THERMAL_TRIP_FLAG_RW_HYST) {
+		if (td->trip.flags & THERMAL_TRIP_FLAG_RW_HYST) {
 			tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR;
 			tz->trip_hyst_attrs[indx].attr.store =
 					trip_point_hyst_store;
Index: linux-pm/drivers/thermal/thermal_debugfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_debugfs.c
+++ linux-pm/drivers/thermal/thermal_debugfs.c
@@ -744,7 +744,7 @@ static void tze_seq_stop(struct seq_file
 static int tze_seq_show(struct seq_file *s, void *v)
 {
 	struct thermal_zone_device *tz = s->private;
-	struct thermal_trip *trip;
+	struct thermal_trip_desc *td;
 	struct tz_episode *tze;
 	const char *type;
 	int trip_id;
@@ -757,7 +757,9 @@ static int tze_seq_show(struct seq_file
 
 	seq_printf(s, "| trip |     type | temp(°mC) | hyst(°mC) |  duration  |  avg(°mC) |  min(°mC) |  max(°mC) |\n");
 
-	for_each_trip(tz, trip) {
+	for_each_trip_desc(tz, td) {
+		const struct thermal_trip *trip = &td->trip;
+
 		/*
 		 * There is no possible mitigation happening at the
 		 * critical trip point, so the stats will be always
Index: linux-pm/drivers/thermal/thermal_netlink.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_netlink.c
+++ linux-pm/drivers/thermal/thermal_netlink.c
@@ -442,7 +442,7 @@ out_cancel_nest:
 static int thermal_genl_cmd_tz_get_trip(struct param *p)
 {
 	struct sk_buff *msg = p->msg;
-	const struct thermal_trip *trip;
+	const struct thermal_trip_desc *td;
 	struct thermal_zone_device *tz;
 	struct nlattr *start_trip;
 	int id;
@@ -462,7 +462,9 @@ static int thermal_genl_cmd_tz_get_trip(
 
 	mutex_lock(&tz->lock);
 
-	for_each_trip(tz, trip) {
+	for_each_trip_desc(tz, td) {
+		const struct thermal_trip *trip = &td->trip;
+
 		if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_ID,
 				thermal_zone_trip_id(tz, trip)) ||
 		    nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_TYPE, trip->type) ||
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -13,11 +13,11 @@ int for_each_thermal_trip(struct thermal
 			  int (*cb)(struct thermal_trip *, void *),
 			  void *data)
 {
-	struct thermal_trip *trip;
+	struct thermal_trip_desc *td;
 	int ret;
 
-	for_each_trip(tz, trip) {
-		ret = cb(trip, data);
+	for_each_trip_desc(tz, td) {
+		ret = cb(&td->trip, data);
 		if (ret)
 			return ret;
 	}
@@ -63,7 +63,7 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_num_t
  */
 void __thermal_zone_set_trips(struct thermal_zone_device *tz)
 {
-	const struct thermal_trip *trip;
+	const struct thermal_trip_desc *td;
 	int low = -INT_MAX, high = INT_MAX;
 	int ret;
 
@@ -72,7 +72,8 @@ void __thermal_zone_set_trips(struct the
 	if (!tz->ops.set_trips)
 		return;
 
-	for_each_trip(tz, trip) {
+	for_each_trip_desc(tz, td) {
+		const struct thermal_trip *trip = &td->trip;
 		int trip_low;
 
 		trip_low = trip->temperature - trip->hysteresis;
@@ -110,7 +111,7 @@ int __thermal_zone_get_trip(struct therm
 	if (!tz || trip_id < 0 || trip_id >= tz->num_trips || !trip)
 		return -EINVAL;
 
-	*trip = tz->trips[trip_id];
+	*trip = tz->trips[trip_id].trip;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(__thermal_zone_get_trip);
@@ -135,7 +136,7 @@ int thermal_zone_trip_id(const struct th
 	 * Assume the trip to be located within the bounds of the thermal
 	 * zone's trips[] table.
 	 */
-	return trip - tz->trips;
+	return trip_to_trip_desc(trip) - tz->trips;
 }
 void thermal_zone_trip_updated(struct thermal_zone_device *tz,
 			       const struct thermal_trip *trip)
Index: linux-pm/drivers/thermal/gov_fair_share.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_fair_share.c
+++ linux-pm/drivers/thermal/gov_fair_share.c
@@ -17,10 +17,13 @@
 
 static int get_trip_level(struct thermal_zone_device *tz)
 {
-	const struct thermal_trip *trip, *level_trip = NULL;
+	const struct thermal_trip *level_trip = NULL;
+	const struct thermal_trip_desc *td;
 	int trip_level = -1;
 
-	for_each_trip(tz, trip) {
+	for_each_trip_desc(tz, td) {
+		const struct thermal_trip *trip = &td->trip;
+
 		if (trip->temperature >= tz->temperature)
 			continue;
 
Index: linux-pm/drivers/thermal/gov_power_allocator.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_power_allocator.c
+++ linux-pm/drivers/thermal/gov_power_allocator.c
@@ -496,9 +496,11 @@ static void get_governor_trips(struct th
 	const struct thermal_trip *first_passive = NULL;
 	const struct thermal_trip *last_passive = NULL;
 	const struct thermal_trip *last_active = NULL;
-	const struct thermal_trip *trip;
+	const struct thermal_trip_desc *td;
+
+	for_each_trip_desc(tz, td) {
+		const struct thermal_trip *trip = &td->trip;
 
-	for_each_trip(tz, trip) {
 		switch (trip->type) {
 		case THERMAL_TRIP_PASSIVE:
 			if (!first_passive) {
Index: linux-pm/drivers/thermal/thermal_helpers.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_helpers.c
+++ linux-pm/drivers/thermal/thermal_helpers.c
@@ -50,7 +50,7 @@ get_thermal_instance(struct thermal_zone
 	mutex_lock(&tz->lock);
 	mutex_lock(&cdev->lock);
 
-	trip = &tz->trips[trip_index];
+	trip = &tz->trips[trip_index].trip;
 
 	list_for_each_entry(pos, &tz->thermal_instances, tz_node) {
 		if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
@@ -82,7 +82,7 @@ EXPORT_SYMBOL(get_thermal_instance);
  */
 int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
 {
-	const struct thermal_trip *trip;
+	const struct thermal_trip_desc *td;
 	int crit_temp = INT_MAX;
 	int ret = -EINVAL;
 
@@ -91,7 +91,9 @@ int __thermal_zone_get_temp(struct therm
 	ret = tz->ops.get_temp(tz, temp);
 
 	if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) {
-		for_each_trip(tz, trip) {
+		for_each_trip_desc(tz, td) {
+			const struct thermal_trip *trip = &td->trip;
+
 			if (trip->type == THERMAL_TRIP_CRITICAL) {
 				crit_temp = trip->temperature;
 				break;

[PATCH v3 2/6] thermal: core: Make struct thermal_zone_device definition internal

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Move the definitions of struct thermal_trip_desc and struct
thermal_zone_device to an internal header file in the thermal core,
as they don't need to be accessible to any code other than the thermal
core and so they don't need to be present in a global header.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v2 -> v3: Minor changelog update

v1 -> v2: No changes

---
 drivers/thermal/thermal_core.h  |   85 +++++++++++++++++++++++++++++++++++++++
 drivers/thermal/thermal_trace.h |    2 
 include/linux/thermal.h         |   87 ----------------------------------------
 3 files changed, 89 insertions(+), 85 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -15,6 +15,91 @@
 #include "thermal_netlink.h"
 #include "thermal_debugfs.h"
 
+struct thermal_trip_desc {
+	struct thermal_trip trip;
+	int threshold;
+};
+
+/**
+ * struct thermal_zone_device - structure for a thermal zone
+ * @id:		unique id number for each thermal zone
+ * @type:	the thermal zone device type
+ * @device:	&struct device for this thermal zone
+ * @removal:	removal completion
+ * @trip_temp_attrs:	attributes for trip points for sysfs: trip temperature
+ * @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
+ * @devdata:	private pointer for device private data
+ * @num_trips:	number of trip points the thermal zone supports
+ * @passive_delay_jiffies: number of jiffies to wait between polls when
+ *			performing passive cooling.
+ * @polling_delay_jiffies: number of jiffies to wait between polls when
+ *			checking whether trip points have been crossed (0 for
+ *			interrupt driven systems)
+ * @temperature:	current temperature.  This is only for core code,
+ *			drivers should use thermal_zone_get_temp() to get the
+ *			current temperature
+ * @last_temperature:	previous temperature read
+ * @emul_temperature:	emulated temperature when using CONFIG_THERMAL_EMULATION
+ * @passive:		1 if you've crossed a passive trip point, 0 otherwise.
+ * @prev_low_trip:	the low current temperature if you've crossed a passive
+			trip point.
+ * @prev_high_trip:	the above current temperature if you've crossed a
+			passive trip point.
+ * @need_update:	if equals 1, thermal_zone_device_update needs to be invoked.
+ * @ops:	operations this &thermal_zone_device supports
+ * @tzp:	thermal zone parameters
+ * @governor:	pointer to the governor for this thermal zone
+ * @governor_data:	private pointer for governor data
+ * @thermal_instances:	list of &struct thermal_instance of this thermal zone
+ * @ida:	&struct ida to generate unique id for this zone's cooling
+ *		devices
+ * @lock:	lock to protect thermal_instances list
+ * @node:	node in thermal_tz_list (in thermal_core.c)
+ * @poll_queue:	delayed work for polling
+ * @notify_event: Last notification event
+ * @suspended: thermal zone suspend indicator
+ * @trips:	array of struct thermal_trip objects
+ */
+struct thermal_zone_device {
+	int id;
+	char type[THERMAL_NAME_LENGTH];
+	struct device device;
+	struct completion removal;
+	struct attribute_group trips_attribute_group;
+	struct thermal_attr *trip_temp_attrs;
+	struct thermal_attr *trip_type_attrs;
+	struct thermal_attr *trip_hyst_attrs;
+	enum thermal_device_mode mode;
+	void *devdata;
+	int num_trips;
+	unsigned long passive_delay_jiffies;
+	unsigned long polling_delay_jiffies;
+	int temperature;
+	int last_temperature;
+	int emul_temperature;
+	int passive;
+	int prev_low_trip;
+	int prev_high_trip;
+	atomic_t need_update;
+	struct thermal_zone_device_ops ops;
+	struct thermal_zone_params *tzp;
+	struct thermal_governor *governor;
+	void *governor_data;
+	struct list_head thermal_instances;
+	struct ida ida;
+	struct mutex lock;
+	struct list_head node;
+	struct delayed_work poll_queue;
+	enum thermal_notify_event notify_event;
+	bool suspended;
+#ifdef CONFIG_THERMAL_DEBUGFS
+	struct thermal_debugfs *debugfs;
+#endif
+	struct thermal_trip_desc trips[] __counted_by(num_trips);
+};
+
 /* Default Thermal Governor */
 #if defined(CONFIG_THERMAL_DEFAULT_GOV_STEP_WISE)
 #define DEFAULT_THERMAL_GOVERNOR       "step_wise"
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -73,17 +73,14 @@ struct thermal_trip {
 	void *priv;
 };
 
-struct thermal_trip_desc {
-	struct thermal_trip trip;
-	int threshold;
-};
-
 #define THERMAL_TRIP_FLAG_RW_TEMP	BIT(0)
 #define THERMAL_TRIP_FLAG_RW_HYST	BIT(1)
 
 #define THERMAL_TRIP_FLAG_RW	(THERMAL_TRIP_FLAG_RW_TEMP | \
 				 THERMAL_TRIP_FLAG_RW_HYST)
 
+struct thermal_zone_device;
+
 struct thermal_zone_device_ops {
 	int (*bind) (struct thermal_zone_device *,
 		     struct thermal_cooling_device *);
@@ -130,86 +127,6 @@ struct thermal_cooling_device {
 };
 
 /**
- * struct thermal_zone_device - structure for a thermal zone
- * @id:		unique id number for each thermal zone
- * @type:	the thermal zone device type
- * @device:	&struct device for this thermal zone
- * @removal:	removal completion
- * @trip_temp_attrs:	attributes for trip points for sysfs: trip temperature
- * @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
- * @devdata:	private pointer for device private data
- * @num_trips:	number of trip points the thermal zone supports
- * @passive_delay_jiffies: number of jiffies to wait between polls when
- *			performing passive cooling.
- * @polling_delay_jiffies: number of jiffies to wait between polls when
- *			checking whether trip points have been crossed (0 for
- *			interrupt driven systems)
- * @temperature:	current temperature.  This is only for core code,
- *			drivers should use thermal_zone_get_temp() to get the
- *			current temperature
- * @last_temperature:	previous temperature read
- * @emul_temperature:	emulated temperature when using CONFIG_THERMAL_EMULATION
- * @passive:		1 if you've crossed a passive trip point, 0 otherwise.
- * @prev_low_trip:	the low current temperature if you've crossed a passive
-			trip point.
- * @prev_high_trip:	the above current temperature if you've crossed a
-			passive trip point.
- * @need_update:	if equals 1, thermal_zone_device_update needs to be invoked.
- * @ops:	operations this &thermal_zone_device supports
- * @tzp:	thermal zone parameters
- * @governor:	pointer to the governor for this thermal zone
- * @governor_data:	private pointer for governor data
- * @thermal_instances:	list of &struct thermal_instance of this thermal zone
- * @ida:	&struct ida to generate unique id for this zone's cooling
- *		devices
- * @lock:	lock to protect thermal_instances list
- * @node:	node in thermal_tz_list (in thermal_core.c)
- * @poll_queue:	delayed work for polling
- * @notify_event: Last notification event
- * @suspended: thermal zone suspend indicator
- * @trips:	array of struct thermal_trip objects
- */
-struct thermal_zone_device {
-	int id;
-	char type[THERMAL_NAME_LENGTH];
-	struct device device;
-	struct completion removal;
-	struct attribute_group trips_attribute_group;
-	struct thermal_attr *trip_temp_attrs;
-	struct thermal_attr *trip_type_attrs;
-	struct thermal_attr *trip_hyst_attrs;
-	enum thermal_device_mode mode;
-	void *devdata;
-	int num_trips;
-	unsigned long passive_delay_jiffies;
-	unsigned long polling_delay_jiffies;
-	int temperature;
-	int last_temperature;
-	int emul_temperature;
-	int passive;
-	int prev_low_trip;
-	int prev_high_trip;
-	atomic_t need_update;
-	struct thermal_zone_device_ops ops;
-	struct thermal_zone_params *tzp;
-	struct thermal_governor *governor;
-	void *governor_data;
-	struct list_head thermal_instances;
-	struct ida ida;
-	struct mutex lock;
-	struct list_head node;
-	struct delayed_work poll_queue;
-	enum thermal_notify_event notify_event;
-	bool suspended;
-#ifdef CONFIG_THERMAL_DEBUGFS
-	struct thermal_debugfs *debugfs;
-#endif
-	struct thermal_trip_desc trips[] __counted_by(num_trips);
-};
-
-/**
  * struct thermal_governor - structure that holds thermal governor information
  * @name:	name of the governor
  * @bind_to_tz: callback called when binding to a thermal zone.  If it
Index: linux-pm/drivers/thermal/thermal_trace.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trace.h
+++ linux-pm/drivers/thermal/thermal_trace.h
@@ -9,6 +9,8 @@
 #include <linux/thermal.h>
 #include <linux/tracepoint.h>
 
+#include "thermal_core.h"
+
 TRACE_DEFINE_ENUM(THERMAL_TRIP_CRITICAL);
 TRACE_DEFINE_ENUM(THERMAL_TRIP_HOT);
 TRACE_DEFINE_ENUM(THERMAL_TRIP_PASSIVE);

[PATCH v3 3/6] thermal: core: Rewrite comments in handle_thermal_trip()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make the comments regarding trip crossing and threshold updates in
handle_thermal_trip() slightly more clear.

No functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v2 -> v3: New patch

---
 drivers/thermal/thermal_core.c |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -368,6 +368,13 @@ static void handle_thermal_trip(struct t
 	if (trip->temperature == THERMAL_TEMP_INVALID)
 		return;
 
+	/*
+	 * If the trip temperature or hysteresis has been updated recently,
+	 * the threshold needs to be computed again using the new values.
+	 * However, its initial value still reflects the old ones and that
+	 * is what needs to be compared with the previous zone temperature
+	 * to decide which action to take.
+	 */
 	if (tz->last_temperature == THERMAL_TEMP_INVALID) {
 		/* Initialization. */
 		td->threshold = trip->temperature;
@@ -375,11 +382,9 @@ static void handle_thermal_trip(struct t
 			td->threshold -= trip->hysteresis;
 	} else if (tz->last_temperature < td->threshold) {
 		/*
-		 * The trip threshold is equal to the trip temperature, unless
-		 * the latter has changed in the meantime.  In either case,
-		 * the trip is crossed if the current zone temperature is at
-		 * least equal to its temperature, but otherwise ensure that
-		 * the threshold and the trip temperature will be equal.
+		 * There is no mitigation under way, so it needs to be started
+		 * if the zone temperature exceeds the trip one.  The new
+		 * threshold is then set to the low temperature of the trip.
 		 */
 		if (tz->temperature >= trip->temperature) {
 			thermal_notify_tz_trip_up(tz, trip);
@@ -390,14 +395,9 @@ static void handle_thermal_trip(struct t
 		}
 	} else {
 		/*
-		 * The previous zone temperature was above or equal to the trip
-		 * threshold, which would be equal to the "low temperature" of
-		 * the trip (its temperature minus its hysteresis), unless the
-		 * trip temperature or hysteresis had changed.  In either case,
-		 * the trip is crossed if the current zone temperature is below
-		 * the low temperature of the trip, but otherwise ensure that
-		 * the trip threshold will be equal to the low temperature of
-		 * the trip.
+		 * Mitigation is under way, so it needs to stop if the zone
+		 * temperature falls below the low temperature of the trip.
+		 * In that case, the trip temperature becomes the new threshold.
 		 */
 		if (tz->temperature < trip->temperature - trip->hysteresis) {
 			thermal_notify_tz_trip_down(tz, trip);

[PATCH v3 4/6] thermal: core: Send trip crossing notifications at init time if needed

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If a trip point is already exceeded by the zone temperature at the
initialization time, no trip crossing notification is send regarding
this even though mitigation should be started then.

Address this by rearranging the code in handle_thermal_trip() to
send a trip crossing notification for trip points already exceeded
by the zone temperature initially which also allows to reduce its
size by using the observation that the initialization and regular
trip crossing on the way up become the same case then.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v2 -> v3: New patch

---
 drivers/thermal/thermal_core.c |   37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -364,6 +364,7 @@ static void handle_thermal_trip(struct t
 				struct thermal_trip_desc *td)
 {
 	const struct thermal_trip *trip = &td->trip;
+	int old_threshold;
 
 	if (trip->temperature == THERMAL_TEMP_INVALID)
 		return;
@@ -375,25 +376,11 @@ static void handle_thermal_trip(struct t
 	 * is what needs to be compared with the previous zone temperature
 	 * to decide which action to take.
 	 */
-	if (tz->last_temperature == THERMAL_TEMP_INVALID) {
-		/* Initialization. */
-		td->threshold = trip->temperature;
-		if (tz->temperature >= td->threshold)
-			td->threshold -= trip->hysteresis;
-	} else if (tz->last_temperature < td->threshold) {
-		/*
-		 * There is no mitigation under way, so it needs to be started
-		 * if the zone temperature exceeds the trip one.  The new
-		 * threshold is then set to the low temperature of the trip.
-		 */
-		if (tz->temperature >= trip->temperature) {
-			thermal_notify_tz_trip_up(tz, trip);
-			thermal_debug_tz_trip_up(tz, trip);
-			td->threshold = trip->temperature - trip->hysteresis;
-		} else {
-			td->threshold = trip->temperature;
-		}
-	} else {
+	old_threshold = td->threshold;
+	td->threshold = trip->temperature;
+
+	if (tz->last_temperature >= old_threshold &&
+	    tz->last_temperature != THERMAL_TEMP_INVALID) {
 		/*
 		 * Mitigation is under way, so it needs to stop if the zone
 		 * temperature falls below the low temperature of the trip.
@@ -402,10 +389,18 @@ static void handle_thermal_trip(struct t
 		if (tz->temperature < trip->temperature - trip->hysteresis) {
 			thermal_notify_tz_trip_down(tz, trip);
 			thermal_debug_tz_trip_down(tz, trip);
-			td->threshold = trip->temperature;
 		} else {
-			td->threshold = trip->temperature - trip->hysteresis;
+			td->threshold -= trip->hysteresis;
 		}
+	} else if (tz->temperature >= trip->temperature) {
+		/*
+		 * There is no mitigation under way, so it needs to be started
+		 * if the zone temperature exceeds the trip one.  The new
+		 * threshold is then set to the low temperature of the trip.
+		 */
+		thermal_notify_tz_trip_up(tz, trip);
+		thermal_debug_tz_trip_up(tz, trip);
+		td->threshold -= trip->hysteresis;
 	}
 
 	if (trip->type == THERMAL_TRIP_CRITICAL || trip->type == THERMAL_TRIP_HOT)

[PATCH v3 5/6] thermal: core: Sort trip point crossing notifications by temperature

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If multiple trip points are crossed in one go and the trips table in
the thermal zone device object is not sorted, the corresponding trip
point crossing notifications sent to user space will not be ordered
either.

Moreover, if the trips table is sorted by trip temperature in ascending
order, the trip crossing notifications on the way up will be sent in that
order too, but the trip crossing notifications on the way down will be
sent in the reverse order.

This is generally confusing and it is better to make the kernel send the
notifications in the order of growing (on the way up) or falling (on the
way down) trip temperature.

To achieve that, instead of sending a trip crossing notification and
recording a trip crossing event in the statistics right away from
handle_thermal_trip(), put the trip in question on a list that will be
sorted by __thermal_zone_device_update() after processing all of the trips
and before sending the notifications and recording trip crossing events.

Link: https://lore.kernel.org/linux-pm/20240306085428.88011-1-daniel.lezcano@linaro.org/
Reported-by: Daniel Lezcano <daniel.lezcano@linaro.org> 
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v2 -> v3: Rebase, update changelog, add notify_temp to struct thermal_trip_desc

v1 -> v2: New patch

---
 drivers/thermal/thermal_core.c |   41 +++++++++++++++++++++++++++++++++++------
 drivers/thermal/thermal_core.h |    2 ++
 2 files changed, 37 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/kdev_t.h>
 #include <linux/idr.h>
+#include <linux/list_sort.h>
 #include <linux/thermal.h>
 #include <linux/reboot.h>
 #include <linux/string.h>
@@ -361,7 +362,9 @@ static void handle_critical_trips(struct
 }
 
 static void handle_thermal_trip(struct thermal_zone_device *tz,
-				struct thermal_trip_desc *td)
+				struct thermal_trip_desc *td,
+				struct list_head *way_up_list,
+				struct list_head *way_down_list)
 {
 	const struct thermal_trip *trip = &td->trip;
 	int old_threshold;
@@ -387,8 +390,8 @@ static void handle_thermal_trip(struct t
 		 * In that case, the trip temperature becomes the new threshold.
 		 */
 		if (tz->temperature < trip->temperature - trip->hysteresis) {
-			thermal_notify_tz_trip_down(tz, trip);
-			thermal_debug_tz_trip_down(tz, trip);
+			list_add(&td->notify_list_node, way_down_list);
+			td->notify_temp = trip->temperature - trip->hysteresis;
 		} else {
 			td->threshold -= trip->hysteresis;
 		}
@@ -398,8 +401,8 @@ static void handle_thermal_trip(struct t
 		 * if the zone temperature exceeds the trip one.  The new
 		 * threshold is then set to the low temperature of the trip.
 		 */
-		thermal_notify_tz_trip_up(tz, trip);
-		thermal_debug_tz_trip_up(tz, trip);
+		list_add_tail(&td->notify_list_node, way_up_list);
+		td->notify_temp = trip->temperature;
 		td->threshold -= trip->hysteresis;
 	}
 
@@ -452,10 +455,24 @@ static void thermal_zone_device_init(str
 		pos->initialized = false;
 }
 
+static int thermal_trip_notify_cmp(void *ascending, const struct list_head *a,
+				   const struct list_head *b)
+{
+	struct thermal_trip_desc *tda = container_of(a, struct thermal_trip_desc,
+						     notify_list_node);
+	struct thermal_trip_desc *tdb = container_of(b, struct thermal_trip_desc,
+						     notify_list_node);
+	int ret = tdb->notify_temp - tda->notify_temp;
+
+	return ascending ? ret : -ret;
+}
+
 void __thermal_zone_device_update(struct thermal_zone_device *tz,
 				  enum thermal_notify_event event)
 {
 	struct thermal_trip_desc *td;
+	LIST_HEAD(way_down_list);
+	LIST_HEAD(way_up_list);
 
 	if (tz->suspended)
 		return;
@@ -470,7 +487,19 @@ void __thermal_zone_device_update(struct
 	tz->notify_event = event;
 
 	for_each_trip_desc(tz, td)
-		handle_thermal_trip(tz, td);
+		handle_thermal_trip(tz, td, &way_up_list, &way_down_list);
+
+	list_sort(&way_up_list, &way_up_list, thermal_trip_notify_cmp);
+	list_for_each_entry(td, &way_up_list, notify_list_node) {
+		thermal_notify_tz_trip_up(tz, &td->trip);
+		thermal_debug_tz_trip_up(tz, &td->trip);
+	}
+
+	list_sort(NULL, &way_down_list, thermal_trip_notify_cmp);
+	list_for_each_entry(td, &way_down_list, notify_list_node) {
+		thermal_notify_tz_trip_down(tz, &td->trip);
+		thermal_debug_tz_trip_down(tz, &td->trip);
+	}
 
 	monitor_thermal_zone(tz);
 }
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -17,6 +17,8 @@
 
 struct thermal_trip_desc {
 	struct thermal_trip trip;
+	struct list_head notify_list_node;
+	int notify_temp;
 	int threshold;
 };
 

[PATCH v3 6/6] thermal: core: Relocate critical and hot trip handling

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify handle_thermal_trip() to call handle_critical_trips() only after
finding that the trip temperature has been crossed on the way up and
remove the redundant temperature check from the latter.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_core.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -349,10 +349,6 @@ void thermal_zone_device_critical_reboot
 static void handle_critical_trips(struct thermal_zone_device *tz,
 				  const struct thermal_trip *trip)
 {
-	/* If we have not crossed the trip_temp, we do not care. */
-	if (trip->temperature <= 0 || tz->temperature < trip->temperature)
-		return;
-
 	trace_thermal_zone_trip(tz, thermal_zone_trip_id(tz, trip), trip->type);
 
 	if (trip->type == THERMAL_TRIP_CRITICAL)
@@ -404,12 +400,15 @@ static void handle_thermal_trip(struct t
 		list_add_tail(&td->notify_list_node, way_up_list);
 		td->notify_temp = trip->temperature;
 		td->threshold -= trip->hysteresis;
+
+		if (trip->type == THERMAL_TRIP_CRITICAL ||
+		    trip->type == THERMAL_TRIP_HOT) {
+			handle_critical_trips(tz, trip);
+			return;
+		}
 	}
 
-	if (trip->type == THERMAL_TRIP_CRITICAL || trip->type == THERMAL_TRIP_HOT)
-		handle_critical_trips(tz, trip);
-	else
-		handle_non_critical_trips(tz, trip);
+	handle_non_critical_trips(tz, trip);
 }
 
 static void update_temperature(struct thermal_zone_device *tz)

Re: [PATCH v3 0/6] thermal: More separation between the core and drivers

[Rafael J. Wysocki]

On Tue, Apr 2, 2024 at 9:04 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi Everyone,
>
> This is an update of
>
> https://lore.kernel.org/linux-pm/4558384.LvFx2qVVIh@kreacher/
>
> and
>
> https://lore.kernel.org/linux-pm/2331888.ElGaqSPkdT@kreacher/
>
> which rebases the first patch on top of 6.9-rc2, adds 3 patches and adjusts
> the third patch from v2.
>
> The original description of the first two patches still applies:
>
> > Patch [1/2] is based on the observation that the threshold field in struct
> > thermal_trip really should be core-internal and to make that happen it
> > introduces a wrapper structure around struct thermal_trip for internal
> > use in the core.
> >
> > Patch [2/2] moves the definition of the new structure and the struct
> > thermal_zone_device one to a local header file in the core to enforce
> > more separation between the core and drivers.
> >
> > The patches are not expected to introduce any observable differences in
> > behavior, so please let me know if you see any of that.
>
> Note that these patches were first sent before the merge window and have not
> really changed since then (except for a minor rebase of the first patch in
> this series).  Moreover, no comments regarding the merit of these patches
> have been made shared, so if this continues, I will be considering them as
> good to go by the end of this week.
>
> Patch [3/6] is a rewrite of comments regarding trip crossing and threshold
> computations.
>
> Patch [4/6] updates the trip crossing detection code to consolidate the
> threshold initialization with trip crossing on the way up.
>
> Patch [5/6] ([3/3] in v2) adds a mechanism to sort notifications and debug
> calls taking place during one invocation of __thermal_zone_device_update() so
> they always go in temperature order.
>
> Patch [6/6] relocates the critical and trip point handling to avoid a
> redundant temperature check.
>
> The series applies on top of 6.9-rc2 and I'm planning to create a test
> branch containing it.

As promised:

https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=thermal-core-testing

Re: [PATCH v3 6/6] thermal: core: Relocate critical and hot trip handling

[Rafael J. Wysocki]

On Tue, Apr 2, 2024 at 9:04 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Modify handle_thermal_trip() to call handle_critical_trips() only after
> finding that the trip temperature has been crossed on the way up and
> remove the redundant temperature check from the latter.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

This change is premature, as it will cause handle_non_critical_trips()
to be called for hot/critical trips which is questionable, so I'm
withdrawing it for now.

The rest of the series is still applicable, though.


> ---
>  drivers/thermal/thermal_core.c |   15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -349,10 +349,6 @@ void thermal_zone_device_critical_reboot
>  static void handle_critical_trips(struct thermal_zone_device *tz,
>                                   const struct thermal_trip *trip)
>  {
> -       /* If we have not crossed the trip_temp, we do not care. */
> -       if (trip->temperature <= 0 || tz->temperature < trip->temperature)
> -               return;
> -
>         trace_thermal_zone_trip(tz, thermal_zone_trip_id(tz, trip), trip->type);
>
>         if (trip->type == THERMAL_TRIP_CRITICAL)
> @@ -404,12 +400,15 @@ static void handle_thermal_trip(struct t
>                 list_add_tail(&td->notify_list_node, way_up_list);
>                 td->notify_temp = trip->temperature;
>                 td->threshold -= trip->hysteresis;
> +
> +               if (trip->type == THERMAL_TRIP_CRITICAL ||
> +                   trip->type == THERMAL_TRIP_HOT) {
> +                       handle_critical_trips(tz, trip);
> +                       return;
> +               }
>         }
>
> -       if (trip->type == THERMAL_TRIP_CRITICAL || trip->type == THERMAL_TRIP_HOT)
> -               handle_critical_trips(tz, trip);
> -       else
> -               handle_non_critical_trips(tz, trip);
> +       handle_non_critical_trips(tz, trip);
>  }
>
>  static void update_temperature(struct thermal_zone_device *tz)
>
>
>
>

Re: [PATCH v3 0/6] thermal: More separation between the core and drivers

[Lukasz Luba]

On 4/2/24 20:42, Rafael J. Wysocki wrote:
> On Tue, Apr 2, 2024 at 9:04 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> Hi Everyone,
>>
>> This is an update of
>>
>> https://lore.kernel.org/linux-pm/4558384.LvFx2qVVIh@kreacher/
>>
>> and
>>
>> https://lore.kernel.org/linux-pm/2331888.ElGaqSPkdT@kreacher/
>>
>> which rebases the first patch on top of 6.9-rc2, adds 3 patches and adjusts
>> the third patch from v2.
>>
>> The original description of the first two patches still applies:
>>
>>> Patch [1/2] is based on the observation that the threshold field in struct
>>> thermal_trip really should be core-internal and to make that happen it
>>> introduces a wrapper structure around struct thermal_trip for internal
>>> use in the core.
>>>
>>> Patch [2/2] moves the definition of the new structure and the struct
>>> thermal_zone_device one to a local header file in the core to enforce
>>> more separation between the core and drivers.
>>>
>>> The patches are not expected to introduce any observable differences in
>>> behavior, so please let me know if you see any of that.
>>
>> Note that these patches were first sent before the merge window and have not
>> really changed since then (except for a minor rebase of the first patch in
>> this series).  Moreover, no comments regarding the merit of these patches
>> have been made shared, so if this continues, I will be considering them as
>> good to go by the end of this week.
>>
>> Patch [3/6] is a rewrite of comments regarding trip crossing and threshold
>> computations.
>>
>> Patch [4/6] updates the trip crossing detection code to consolidate the
>> threshold initialization with trip crossing on the way up.
>>
>> Patch [5/6] ([3/3] in v2) adds a mechanism to sort notifications and debug
>> calls taking place during one invocation of __thermal_zone_device_update() so
>> they always go in temperature order.
>>
>> Patch [6/6] relocates the critical and trip point handling to avoid a
>> redundant temperature check.
>>
>> The series applies on top of 6.9-rc2 and I'm planning to create a test
>> branch containing it.
> 
> As promised:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=thermal-core-testing

Thanks Rafael for the handy branch. I'll play with it today.

Regards,
Lukasz

Re: [PATCH v3 1/6] thermal: core: Move threshold out of struct thermal_trip

[Lukasz Luba]

On 4/2/24 19:56, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The threshold field in struct thermal_trip is only used internally by
> the thermal core and it is better to prevent drivers from misusing it.
> It also takes some space unnecessarily in the trip tables passed by
> drivers to the core during thermal zone registration.
> 
> For this reason, introduce struct thermal_trip_desc as a wrapper around
> struct thermal_trip, move the threshold field directly into it and make
> the thermal core store struct thermal_trip_desc objects in the internal
> thermal zone trip tables.  Adjust all of the code using trip tables in
> the thermal core accordingly.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v2 -> v3: Rebase on top of 6.9-rc2, minor changelog update.
> 
> v1 -> v2: No changes.
> 
> ---
>   drivers/thermal/gov_fair_share.c      |    7 +++--
>   drivers/thermal/gov_power_allocator.c |    6 ++--
>   drivers/thermal/thermal_core.c        |   46 +++++++++++++++++++---------------
>   drivers/thermal/thermal_core.h        |    7 +++--
>   drivers/thermal/thermal_debugfs.c     |    6 ++--
>   drivers/thermal/thermal_helpers.c     |    8 +++--
>   drivers/thermal/thermal_netlink.c     |    6 ++--
>   drivers/thermal/thermal_sysfs.c       |   20 +++++++-------
>   drivers/thermal/thermal_trip.c        |   15 +++++------
>   include/linux/thermal.h               |    9 ++++--
>   10 files changed, 78 insertions(+), 52 deletions(-)
> 
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -61,7 +61,6 @@ enum thermal_notify_event {
>    * struct thermal_trip - representation of a point in temperature domain
>    * @temperature: temperature value in miliCelsius
>    * @hysteresis: relative hysteresis in miliCelsius
> - * @threshold: trip crossing notification threshold miliCelsius
>    * @type: trip point type
>    * @priv: pointer to driver data associated with this trip
>    * @flags: flags representing binary properties of the trip
> @@ -69,12 +68,16 @@ enum thermal_notify_event {
>   struct thermal_trip {
>   	int temperature;
>   	int hysteresis;
> -	int threshold;
>   	enum thermal_trip_type type;
>   	u8 flags;
>   	void *priv;
>   };
>   
> +struct thermal_trip_desc {
> +	struct thermal_trip trip;
> +	int threshold;
> +};
> +
>   #define THERMAL_TRIP_FLAG_RW_TEMP	BIT(0)
>   #define THERMAL_TRIP_FLAG_RW_HYST	BIT(1)
>   
> @@ -203,7 +206,7 @@ struct thermal_zone_device {
>   #ifdef CONFIG_THERMAL_DEBUGFS
>   	struct thermal_debugfs *debugfs;
>   #endif
> -	struct thermal_trip trips[] __counted_by(num_trips);
> +	struct thermal_trip_desc trips[] __counted_by(num_trips);
>   };
>   
>   /**
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -361,17 +361,19 @@ static void handle_critical_trips(struct
>   }
>   
>   static void handle_thermal_trip(struct thermal_zone_device *tz,
> -				struct thermal_trip *trip)
> +				struct thermal_trip_desc *td)
>   {
> +	const struct thermal_trip *trip = &td->trip;
> +
>   	if (trip->temperature == THERMAL_TEMP_INVALID)
>   		return;
>   
>   	if (tz->last_temperature == THERMAL_TEMP_INVALID) {
>   		/* Initialization. */
> -		trip->threshold = trip->temperature;
> -		if (tz->temperature >= trip->threshold)
> -			trip->threshold -= trip->hysteresis;
> -	} else if (tz->last_temperature < trip->threshold) {
> +		td->threshold = trip->temperature;
> +		if (tz->temperature >= td->threshold)
> +			td->threshold -= trip->hysteresis;
> +	} else if (tz->last_temperature < td->threshold) {
>   		/*
>   		 * The trip threshold is equal to the trip temperature, unless
>   		 * the latter has changed in the meantime.  In either case,
> @@ -382,9 +384,9 @@ static void handle_thermal_trip(struct t
>   		if (tz->temperature >= trip->temperature) {
>   			thermal_notify_tz_trip_up(tz, trip);
>   			thermal_debug_tz_trip_up(tz, trip);
> -			trip->threshold = trip->temperature - trip->hysteresis;
> +			td->threshold = trip->temperature - trip->hysteresis;
>   		} else {
> -			trip->threshold = trip->temperature;
> +			td->threshold = trip->temperature;
>   		}
>   	} else {
>   		/*
> @@ -400,9 +402,9 @@ static void handle_thermal_trip(struct t
>   		if (tz->temperature < trip->temperature - trip->hysteresis) {
>   			thermal_notify_tz_trip_down(tz, trip);
>   			thermal_debug_tz_trip_down(tz, trip);
> -			trip->threshold = trip->temperature;
> +			td->threshold = trip->temperature;
>   		} else {
> -			trip->threshold = trip->temperature - trip->hysteresis;
> +			td->threshold = trip->temperature - trip->hysteresis;
>   		}
>   	}
>   
> @@ -458,7 +460,7 @@ static void thermal_zone_device_init(str
>   void __thermal_zone_device_update(struct thermal_zone_device *tz,
>   				  enum thermal_notify_event event)
>   {
> -	struct thermal_trip *trip;
> +	struct thermal_trip_desc *td;
>   
>   	if (tz->suspended)
>   		return;
> @@ -472,8 +474,8 @@ void __thermal_zone_device_update(struct
>   
>   	tz->notify_event = event;
>   
> -	for_each_trip(tz, trip)
> -		handle_thermal_trip(tz, trip);
> +	for_each_trip_desc(tz, td)
> +		handle_thermal_trip(tz, td);
>   
>   	monitor_thermal_zone(tz);
>   }
> @@ -766,7 +768,7 @@ int thermal_zone_bind_cooling_device(str
>   	if (trip_index < 0 || trip_index >= tz->num_trips)
>   		return -EINVAL;
>   
> -	return thermal_bind_cdev_to_trip(tz, &tz->trips[trip_index], cdev,
> +	return thermal_bind_cdev_to_trip(tz, &tz->trips[trip_index].trip, cdev,
>   					 upper, lower, weight);
>   }
>   EXPORT_SYMBOL_GPL(thermal_zone_bind_cooling_device);
> @@ -825,7 +827,7 @@ int thermal_zone_unbind_cooling_device(s
>   	if (trip_index < 0 || trip_index >= tz->num_trips)
>   		return -EINVAL;
>   
> -	return thermal_unbind_cdev_from_trip(tz, &tz->trips[trip_index], cdev);
> +	return thermal_unbind_cdev_from_trip(tz, &tz->trips[trip_index].trip, cdev);
>   }
>   EXPORT_SYMBOL_GPL(thermal_zone_unbind_cooling_device);
>   
> @@ -1221,16 +1223,19 @@ static void thermal_set_delay_jiffies(un
>   
>   int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp)
>   {
> -	int i, ret = -EINVAL;
> +	const struct thermal_trip_desc *td;
> +	int ret = -EINVAL;
>   
>   	if (tz->ops.get_crit_temp)
>   		return tz->ops.get_crit_temp(tz, temp);
>   
>   	mutex_lock(&tz->lock);
>   
> -	for (i = 0; i < tz->num_trips; i++) {
> -		if (tz->trips[i].type == THERMAL_TRIP_CRITICAL) {
> -			*temp = tz->trips[i].temperature;
> +	for_each_trip_desc(tz, td) {
> +		const struct thermal_trip *trip = &td->trip;
> +
> +		if (trip->type == THERMAL_TRIP_CRITICAL) {
> +			*temp = trip->temperature;
>   			ret = 0;
>   			break;
>   		}
> @@ -1274,7 +1279,9 @@ thermal_zone_device_register_with_trips(
>   					const struct thermal_zone_params *tzp,
>   					int passive_delay, int polling_delay)
>   {
> +	const struct thermal_trip *trip = trips;
>   	struct thermal_zone_device *tz;
> +	struct thermal_trip_desc *td;
>   	int id;
>   	int result;
>   	struct thermal_governor *governor;
> @@ -1339,7 +1346,8 @@ thermal_zone_device_register_with_trips(
>   	tz->device.class = thermal_class;
>   	tz->devdata = devdata;
>   	tz->num_trips = num_trips;
> -	memcpy(tz->trips, trips, num_trips * sizeof(*trips));
> +	for_each_trip_desc(tz, td)
> +		td->trip = *trip++;
>   
>   	thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
>   	thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -120,8 +120,11 @@ void thermal_governor_update_tz(struct t
>   				enum thermal_notify_event reason);
>   
>   /* Helpers */
> -#define for_each_trip(__tz, __trip)	\
> -	for (__trip = __tz->trips; __trip - __tz->trips < __tz->num_trips; __trip++)
> +#define for_each_trip_desc(__tz, __td)	\
> +	for (__td = __tz->trips; __td - __tz->trips < __tz->num_trips; __td++)
> +
> +#define trip_to_trip_desc(__trip)	\
> +	container_of(__trip, struct thermal_trip_desc, trip)
>   
>   void __thermal_zone_set_trips(struct thermal_zone_device *tz);
>   int thermal_zone_trip_id(const struct thermal_zone_device *tz,
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -88,7 +88,7 @@ trip_point_type_show(struct device *dev,
>   	if (sscanf(attr->attr.name, "trip_point_%d_type", &trip_id) != 1)
>   		return -EINVAL;
>   
> -	switch (tz->trips[trip_id].type) {
> +	switch (tz->trips[trip_id].trip.type) {

This could be a bit shorter, with some helper variable, but that's
minor cosmetic.

>   	case THERMAL_TRIP_CRITICAL:
>   		return sprintf(buf, "critical\n");
>   	case THERMAL_TRIP_HOT:
> @@ -120,7 +120,7 @@ trip_point_temp_store(struct device *dev
>   
>   	mutex_lock(&tz->lock);
>   
> -	trip = &tz->trips[trip_id];
> +	trip = &tz->trips[trip_id].trip;
>   
>   	if (temp != trip->temperature) {
>   		if (tz->ops.set_trip_temp) {
> @@ -150,7 +150,7 @@ trip_point_temp_show(struct device *dev,
>   	if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip_id) != 1)
>   		return -EINVAL;
>   
> -	return sprintf(buf, "%d\n", tz->trips[trip_id].temperature);
> +	return sprintf(buf, "%d\n", tz->trips[trip_id].trip.temperature);
>   }
>   
>   static ssize_t
> @@ -171,7 +171,7 @@ trip_point_hyst_store(struct device *dev
>   
>   	mutex_lock(&tz->lock);
>   
> -	trip = &tz->trips[trip_id];
> +	trip = &tz->trips[trip_id].trip;
>   
>   	if (hyst != trip->hysteresis) {
>   		trip->hysteresis = hyst;
> @@ -194,7 +194,7 @@ trip_point_hyst_show(struct device *dev,
>   	if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip_id) != 1)
>   		return -EINVAL;
>   
> -	return sprintf(buf, "%d\n", tz->trips[trip_id].hysteresis);
> +	return sprintf(buf, "%d\n", tz->trips[trip_id].trip.hysteresis);
>   }
>   
>   static ssize_t
> @@ -393,7 +393,7 @@ static const struct attribute_group *the
>    */
>   static int create_trip_attrs(struct thermal_zone_device *tz)
>   {
> -	const struct thermal_trip *trip;
> +	const struct thermal_trip_desc *td;
>   	struct attribute **attrs;
>   
>   	/* This function works only for zones with at least one trip */
> @@ -429,8 +429,8 @@ static int create_trip_attrs(struct ther
>   		return -ENOMEM;
>   	}
>   
> -	for_each_trip(tz, trip) {
> -		int indx = thermal_zone_trip_id(tz, trip);
> +	for_each_trip_desc(tz, td) {
> +		int indx = thermal_zone_trip_id(tz, &td->trip);
>   
>   		/* create trip type attribute */
>   		snprintf(tz->trip_type_attrs[indx].name, THERMAL_NAME_LENGTH,
> @@ -452,7 +452,7 @@ static int create_trip_attrs(struct ther
>   						tz->trip_temp_attrs[indx].name;
>   		tz->trip_temp_attrs[indx].attr.attr.mode = S_IRUGO;
>   		tz->trip_temp_attrs[indx].attr.show = trip_point_temp_show;
> -		if (trip->flags & THERMAL_TRIP_FLAG_RW_TEMP) {
> +		if (td->trip.flags & THERMAL_TRIP_FLAG_RW_TEMP) {
>   			tz->trip_temp_attrs[indx].attr.attr.mode |= S_IWUSR;
>   			tz->trip_temp_attrs[indx].attr.store =
>   							trip_point_temp_store;
> @@ -467,7 +467,7 @@ static int create_trip_attrs(struct ther
>   					tz->trip_hyst_attrs[indx].name;
>   		tz->trip_hyst_attrs[indx].attr.attr.mode = S_IRUGO;
>   		tz->trip_hyst_attrs[indx].attr.show = trip_point_hyst_show;
> -		if (trip->flags & THERMAL_TRIP_FLAG_RW_HYST) {
> +		if (td->trip.flags & THERMAL_TRIP_FLAG_RW_HYST) {
>   			tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR;
>   			tz->trip_hyst_attrs[indx].attr.store =
>   					trip_point_hyst_store;
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -744,7 +744,7 @@ static void tze_seq_stop(struct seq_file
>   static int tze_seq_show(struct seq_file *s, void *v)
>   {
>   	struct thermal_zone_device *tz = s->private;
> -	struct thermal_trip *trip;
> +	struct thermal_trip_desc *td;
>   	struct tz_episode *tze;
>   	const char *type;
>   	int trip_id;
> @@ -757,7 +757,9 @@ static int tze_seq_show(struct seq_file
>   
>   	seq_printf(s, "| trip |     type | temp(°mC) | hyst(°mC) |  duration  |  avg(°mC) |  min(°mC) |  max(°mC) |\n");
>   
> -	for_each_trip(tz, trip) {
> +	for_each_trip_desc(tz, td) {
> +		const struct thermal_trip *trip = &td->trip;
> +
>   		/*
>   		 * There is no possible mitigation happening at the
>   		 * critical trip point, so the stats will be always
> Index: linux-pm/drivers/thermal/thermal_netlink.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_netlink.c
> +++ linux-pm/drivers/thermal/thermal_netlink.c
> @@ -442,7 +442,7 @@ out_cancel_nest:
>   static int thermal_genl_cmd_tz_get_trip(struct param *p)
>   {
>   	struct sk_buff *msg = p->msg;
> -	const struct thermal_trip *trip;
> +	const struct thermal_trip_desc *td;
>   	struct thermal_zone_device *tz;
>   	struct nlattr *start_trip;
>   	int id;
> @@ -462,7 +462,9 @@ static int thermal_genl_cmd_tz_get_trip(
>   
>   	mutex_lock(&tz->lock);
>   
> -	for_each_trip(tz, trip) {
> +	for_each_trip_desc(tz, td) {
> +		const struct thermal_trip *trip = &td->trip;
> +
>   		if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_ID,
>   				thermal_zone_trip_id(tz, trip)) ||
>   		    nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_TYPE, trip->type) ||
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -13,11 +13,11 @@ int for_each_thermal_trip(struct thermal
>   			  int (*cb)(struct thermal_trip *, void *),
>   			  void *data)
>   {
> -	struct thermal_trip *trip;
> +	struct thermal_trip_desc *td;
>   	int ret;
>   
> -	for_each_trip(tz, trip) {
> -		ret = cb(trip, data);
> +	for_each_trip_desc(tz, td) {
> +		ret = cb(&td->trip, data);
>   		if (ret)
>   			return ret;
>   	}
> @@ -63,7 +63,7 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_num_t
>    */
>   void __thermal_zone_set_trips(struct thermal_zone_device *tz)
>   {
> -	const struct thermal_trip *trip;
> +	const struct thermal_trip_desc *td;
>   	int low = -INT_MAX, high = INT_MAX;
>   	int ret;
>   
> @@ -72,7 +72,8 @@ void __thermal_zone_set_trips(struct the
>   	if (!tz->ops.set_trips)
>   		return;
>   
> -	for_each_trip(tz, trip) {
> +	for_each_trip_desc(tz, td) {
> +		const struct thermal_trip *trip = &td->trip;
>   		int trip_low;
>   
>   		trip_low = trip->temperature - trip->hysteresis;
> @@ -110,7 +111,7 @@ int __thermal_zone_get_trip(struct therm
>   	if (!tz || trip_id < 0 || trip_id >= tz->num_trips || !trip)
>   		return -EINVAL;
>   
> -	*trip = tz->trips[trip_id];
> +	*trip = tz->trips[trip_id].trip;
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(__thermal_zone_get_trip);
> @@ -135,7 +136,7 @@ int thermal_zone_trip_id(const struct th
>   	 * Assume the trip to be located within the bounds of the thermal
>   	 * zone's trips[] table.
>   	 */
> -	return trip - tz->trips;
> +	return trip_to_trip_desc(trip) - tz->trips;
>   }
>   void thermal_zone_trip_updated(struct thermal_zone_device *tz,
>   			       const struct thermal_trip *trip)
> Index: linux-pm/drivers/thermal/gov_fair_share.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_fair_share.c
> +++ linux-pm/drivers/thermal/gov_fair_share.c
> @@ -17,10 +17,13 @@
>   
>   static int get_trip_level(struct thermal_zone_device *tz)
>   {
> -	const struct thermal_trip *trip, *level_trip = NULL;
> +	const struct thermal_trip *level_trip = NULL;
> +	const struct thermal_trip_desc *td;
>   	int trip_level = -1;
>   
> -	for_each_trip(tz, trip) {
> +	for_each_trip_desc(tz, td) {
> +		const struct thermal_trip *trip = &td->trip;
> +
>   		if (trip->temperature >= tz->temperature)
>   			continue;
>   
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -496,9 +496,11 @@ static void get_governor_trips(struct th
>   	const struct thermal_trip *first_passive = NULL;
>   	const struct thermal_trip *last_passive = NULL;
>   	const struct thermal_trip *last_active = NULL;
> -	const struct thermal_trip *trip;
> +	const struct thermal_trip_desc *td;
> +
> +	for_each_trip_desc(tz, td) {
> +		const struct thermal_trip *trip = &td->trip;
>   
> -	for_each_trip(tz, trip) {
>   		switch (trip->type) {
>   		case THERMAL_TRIP_PASSIVE:
>   			if (!first_passive) {
> Index: linux-pm/drivers/thermal/thermal_helpers.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_helpers.c
> +++ linux-pm/drivers/thermal/thermal_helpers.c
> @@ -50,7 +50,7 @@ get_thermal_instance(struct thermal_zone
>   	mutex_lock(&tz->lock);
>   	mutex_lock(&cdev->lock);
>   
> -	trip = &tz->trips[trip_index];
> +	trip = &tz->trips[trip_index].trip;
>   
>   	list_for_each_entry(pos, &tz->thermal_instances, tz_node) {
>   		if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
> @@ -82,7 +82,7 @@ EXPORT_SYMBOL(get_thermal_instance);
>    */
>   int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
>   {
> -	const struct thermal_trip *trip;
> +	const struct thermal_trip_desc *td;
>   	int crit_temp = INT_MAX;
>   	int ret = -EINVAL;
>   
> @@ -91,7 +91,9 @@ int __thermal_zone_get_temp(struct therm
>   	ret = tz->ops.get_temp(tz, temp);
>   
>   	if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) {
> -		for_each_trip(tz, trip) {
> +		for_each_trip_desc(tz, td) {
> +			const struct thermal_trip *trip = &td->trip;
> +
>   			if (trip->type == THERMAL_TRIP_CRITICAL) {
>   				crit_temp = trip->temperature;
>   				break;
> 
> 
> 

LGTM,

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v3 2/6] thermal: core: Make struct thermal_zone_device definition internal

[Lukasz Luba]

On 4/2/24 19:57, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Move the definitions of struct thermal_trip_desc and struct
> thermal_zone_device to an internal header file in the thermal core,
> as they don't need to be accessible to any code other than the thermal
> core and so they don't need to be present in a global header.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v2 -> v3: Minor changelog update
> 
> v1 -> v2: No changes
> 
> ---
>   drivers/thermal/thermal_core.h  |   85 +++++++++++++++++++++++++++++++++++++++
>   drivers/thermal/thermal_trace.h |    2
>   include/linux/thermal.h         |   87 ----------------------------------------
>   3 files changed, 89 insertions(+), 85 deletions(-)
> 

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v3 3/6] thermal: core: Rewrite comments in handle_thermal_trip()

[Lukasz Luba]

On 4/2/24 19:59, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make the comments regarding trip crossing and threshold updates in
> handle_thermal_trip() slightly more clear.
> 
> No functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v2 -> v3: New patch
> 
> ---
>   drivers/thermal/thermal_core.c |   26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -368,6 +368,13 @@ static void handle_thermal_trip(struct t
>   	if (trip->temperature == THERMAL_TEMP_INVALID)
>   		return;
>   
> +	/*
> +	 * If the trip temperature or hysteresis has been updated recently,
> +	 * the threshold needs to be computed again using the new values.
> +	 * However, its initial value still reflects the old ones and that
> +	 * is what needs to be compared with the previous zone temperature
> +	 * to decide which action to take.
> +	 */
>   	if (tz->last_temperature == THERMAL_TEMP_INVALID) {
>   		/* Initialization. */
>   		td->threshold = trip->temperature;
> @@ -375,11 +382,9 @@ static void handle_thermal_trip(struct t
>   			td->threshold -= trip->hysteresis;
>   	} else if (tz->last_temperature < td->threshold) {
>   		/*
> -		 * The trip threshold is equal to the trip temperature, unless
> -		 * the latter has changed in the meantime.  In either case,
> -		 * the trip is crossed if the current zone temperature is at
> -		 * least equal to its temperature, but otherwise ensure that
> -		 * the threshold and the trip temperature will be equal.
> +		 * There is no mitigation under way, so it needs to be started
> +		 * if the zone temperature exceeds the trip one.  The new
> +		 * threshold is then set to the low temperature of the trip.
>   		 */
>   		if (tz->temperature >= trip->temperature) {
>   			thermal_notify_tz_trip_up(tz, trip);
> @@ -390,14 +395,9 @@ static void handle_thermal_trip(struct t
>   		}
>   	} else {
>   		/*
> -		 * The previous zone temperature was above or equal to the trip
> -		 * threshold, which would be equal to the "low temperature" of
> -		 * the trip (its temperature minus its hysteresis), unless the
> -		 * trip temperature or hysteresis had changed.  In either case,
> -		 * the trip is crossed if the current zone temperature is below
> -		 * the low temperature of the trip, but otherwise ensure that
> -		 * the trip threshold will be equal to the low temperature of
> -		 * the trip.
> +		 * Mitigation is under way, so it needs to stop if the zone
> +		 * temperature falls below the low temperature of the trip.
> +		 * In that case, the trip temperature becomes the new threshold.
>   		 */
>   		if (tz->temperature < trip->temperature - trip->hysteresis) {
>   			thermal_notify_tz_trip_down(tz, trip);
> 
> 
> 

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v3 5/6] thermal: core: Sort trip point crossing notifications by temperature

[Lukasz Luba]

On 4/2/24 20:03, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If multiple trip points are crossed in one go and the trips table in
> the thermal zone device object is not sorted, the corresponding trip
> point crossing notifications sent to user space will not be ordered
> either.
> 
> Moreover, if the trips table is sorted by trip temperature in ascending
> order, the trip crossing notifications on the way up will be sent in that
> order too, but the trip crossing notifications on the way down will be
> sent in the reverse order.
> 
> This is generally confusing and it is better to make the kernel send the
> notifications in the order of growing (on the way up) or falling (on the
> way down) trip temperature.
> 
> To achieve that, instead of sending a trip crossing notification and
> recording a trip crossing event in the statistics right away from
> handle_thermal_trip(), put the trip in question on a list that will be
> sorted by __thermal_zone_device_update() after processing all of the trips
> and before sending the notifications and recording trip crossing events.
> 
> Link: https://lore.kernel.org/linux-pm/20240306085428.88011-1-daniel.lezcano@linaro.org/
> Reported-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v2 -> v3: Rebase, update changelog, add notify_temp to struct thermal_trip_desc
> 
> v1 -> v2: New patch
> 
> ---
>   drivers/thermal/thermal_core.c |   41 +++++++++++++++++++++++++++++++++++------
>   drivers/thermal/thermal_core.h |    2 ++
>   2 files changed, 37 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -15,6 +15,7 @@
>   #include <linux/slab.h>
>   #include <linux/kdev_t.h>
>   #include <linux/idr.h>
> +#include <linux/list_sort.h>
>   #include <linux/thermal.h>
>   #include <linux/reboot.h>
>   #include <linux/string.h>
> @@ -361,7 +362,9 @@ static void handle_critical_trips(struct
>   }
>   
>   static void handle_thermal_trip(struct thermal_zone_device *tz,
> -				struct thermal_trip_desc *td)
> +				struct thermal_trip_desc *td,
> +				struct list_head *way_up_list,
> +				struct list_head *way_down_list)
>   {
>   	const struct thermal_trip *trip = &td->trip;
>   	int old_threshold;
> @@ -387,8 +390,8 @@ static void handle_thermal_trip(struct t
>   		 * In that case, the trip temperature becomes the new threshold.
>   		 */
>   		if (tz->temperature < trip->temperature - trip->hysteresis) {
> -			thermal_notify_tz_trip_down(tz, trip);
> -			thermal_debug_tz_trip_down(tz, trip);
> +			list_add(&td->notify_list_node, way_down_list);
> +			td->notify_temp = trip->temperature - trip->hysteresis;
>   		} else {
>   			td->threshold -= trip->hysteresis;
>   		}
> @@ -398,8 +401,8 @@ static void handle_thermal_trip(struct t
>   		 * if the zone temperature exceeds the trip one.  The new
>   		 * threshold is then set to the low temperature of the trip.
>   		 */
> -		thermal_notify_tz_trip_up(tz, trip);
> -		thermal_debug_tz_trip_up(tz, trip);
> +		list_add_tail(&td->notify_list_node, way_up_list);
> +		td->notify_temp = trip->temperature;
>   		td->threshold -= trip->hysteresis;
>   	}
>   
> @@ -452,10 +455,24 @@ static void thermal_zone_device_init(str
>   		pos->initialized = false;
>   }
>   
> +static int thermal_trip_notify_cmp(void *ascending, const struct list_head *a,
> +				   const struct list_head *b)
> +{
> +	struct thermal_trip_desc *tda = container_of(a, struct thermal_trip_desc,
> +						     notify_list_node);
> +	struct thermal_trip_desc *tdb = container_of(b, struct thermal_trip_desc,
> +						     notify_list_node);
> +	int ret = tdb->notify_temp - tda->notify_temp;
> +
> +	return ascending ? ret : -ret;
> +}
> +
>   void __thermal_zone_device_update(struct thermal_zone_device *tz,
>   				  enum thermal_notify_event event)
>   {
>   	struct thermal_trip_desc *td;
> +	LIST_HEAD(way_down_list);
> +	LIST_HEAD(way_up_list);
>   
>   	if (tz->suspended)
>   		return;
> @@ -470,7 +487,19 @@ void __thermal_zone_device_update(struct
>   	tz->notify_event = event;
>   
>   	for_each_trip_desc(tz, td)
> -		handle_thermal_trip(tz, td);
> +		handle_thermal_trip(tz, td, &way_up_list, &way_down_list);
> +
> +	list_sort(&way_up_list, &way_up_list, thermal_trip_notify_cmp);
> +	list_for_each_entry(td, &way_up_list, notify_list_node) {
> +		thermal_notify_tz_trip_up(tz, &td->trip);
> +		thermal_debug_tz_trip_up(tz, &td->trip);
> +	}
> +
> +	list_sort(NULL, &way_down_list, thermal_trip_notify_cmp);
> +	list_for_each_entry(td, &way_down_list, notify_list_node) {
> +		thermal_notify_tz_trip_down(tz, &td->trip);
> +		thermal_debug_tz_trip_down(tz, &td->trip);
> +	}
>   
>   	monitor_thermal_zone(tz);
>   }
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -17,6 +17,8 @@
>   
>   struct thermal_trip_desc {
>   	struct thermal_trip trip;
> +	struct list_head notify_list_node;
> +	int notify_temp;
>   	int threshold;
>   };
>   
> 
> 
> 


Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v3 4/6] thermal: core: Send trip crossing notifications at init time if needed

[Lukasz Luba]

On 4/2/24 20:02, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If a trip point is already exceeded by the zone temperature at the
> initialization time, no trip crossing notification is send regarding
> this even though mitigation should be started then.
> 
> Address this by rearranging the code in handle_thermal_trip() to
> send a trip crossing notification for trip points already exceeded
> by the zone temperature initially which also allows to reduce its
> size by using the observation that the initialization and regular
> trip crossing on the way up become the same case then.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v2 -> v3: New patch
> 
> ---
>   drivers/thermal/thermal_core.c |   37 ++++++++++++++++---------------------
>   1 file changed, 16 insertions(+), 21 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -364,6 +364,7 @@ static void handle_thermal_trip(struct t
>   				struct thermal_trip_desc *td)
>   {
>   	const struct thermal_trip *trip = &td->trip;
> +	int old_threshold;
>   
>   	if (trip->temperature == THERMAL_TEMP_INVALID)
>   		return;
> @@ -375,25 +376,11 @@ static void handle_thermal_trip(struct t
>   	 * is what needs to be compared with the previous zone temperature
>   	 * to decide which action to take.
>   	 */
> -	if (tz->last_temperature == THERMAL_TEMP_INVALID) {
> -		/* Initialization. */
> -		td->threshold = trip->temperature;
> -		if (tz->temperature >= td->threshold)
> -			td->threshold -= trip->hysteresis;
> -	} else if (tz->last_temperature < td->threshold) {
> -		/*
> -		 * There is no mitigation under way, so it needs to be started
> -		 * if the zone temperature exceeds the trip one.  The new
> -		 * threshold is then set to the low temperature of the trip.
> -		 */
> -		if (tz->temperature >= trip->temperature) {
> -			thermal_notify_tz_trip_up(tz, trip);
> -			thermal_debug_tz_trip_up(tz, trip);
> -			td->threshold = trip->temperature - trip->hysteresis;
> -		} else {
> -			td->threshold = trip->temperature;
> -		}
> -	} else {
> +	old_threshold = td->threshold;
> +	td->threshold = trip->temperature;
> +
> +	if (tz->last_temperature >= old_threshold &&
> +	    tz->last_temperature != THERMAL_TEMP_INVALID) {
>   		/*
>   		 * Mitigation is under way, so it needs to stop if the zone
>   		 * temperature falls below the low temperature of the trip.
> @@ -402,10 +389,18 @@ static void handle_thermal_trip(struct t
>   		if (tz->temperature < trip->temperature - trip->hysteresis) {
>   			thermal_notify_tz_trip_down(tz, trip);
>   			thermal_debug_tz_trip_down(tz, trip);
> -			td->threshold = trip->temperature;
>   		} else {
> -			td->threshold = trip->temperature - trip->hysteresis;
> +			td->threshold -= trip->hysteresis;
>   		}
> +	} else if (tz->temperature >= trip->temperature) {
> +		/*
> +		 * There is no mitigation under way, so it needs to be started
> +		 * if the zone temperature exceeds the trip one.  The new
> +		 * threshold is then set to the low temperature of the trip.
> +		 */
> +		thermal_notify_tz_trip_up(tz, trip);
> +		thermal_debug_tz_trip_up(tz, trip);
> +		td->threshold -= trip->hysteresis;
>   	}
>   
>   	if (trip->type == THERMAL_TRIP_CRITICAL || trip->type == THERMAL_TRIP_HOT)
> 
> 
> 


Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v3 6/6] thermal: core: Relocate critical and hot trip handling

[Lukasz Luba]

Hi Rafael,

On 4/4/24 10:03, Rafael J. Wysocki wrote:
> On Tue, Apr 2, 2024 at 9:04 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Modify handle_thermal_trip() to call handle_critical_trips() only after
>> finding that the trip temperature has been crossed on the way up and
>> remove the redundant temperature check from the latter.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> This change is premature, as it will cause handle_non_critical_trips()
> to be called for hot/critical trips which is questionable, so I'm
> withdrawing it for now.
> 
> The rest of the series is still applicable, though.
> 
> 

Could you explain your concerns about this, please?
Is about the extra execution time for the non-critical trip,
while we are in section of handling critical ASAP?
(also it would require that extra sorting there IMO)

Regards,
Lukasz

Re: [PATCH v3 2/6] thermal: core: Make struct thermal_zone_device definition internal

[Daniel Lezcano]

On 02/04/2024 20:57, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Move the definitions of struct thermal_trip_desc and struct
> thermal_zone_device to an internal header file in the thermal core,
> as they don't need to be accessible to any code other than the thermal
> core and so they don't need to be present in a global header.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

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

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

Re: [PATCH v3 3/6] thermal: core: Rewrite comments in handle_thermal_trip()

[Daniel Lezcano]

On 02/04/2024 20:59, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make the comments regarding trip crossing and threshold updates in
> handle_thermal_trip() slightly more clear.
> 
> No functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

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

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

Re: [PATCH v3 6/6] thermal: core: Relocate critical and hot trip handling

[Rafael J. Wysocki]

Hi Lukasz,

On Fri, Apr 5, 2024 at 9:35 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Rafael,
>
> On 4/4/24 10:03, Rafael J. Wysocki wrote:
> > On Tue, Apr 2, 2024 at 9:04 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> Modify handle_thermal_trip() to call handle_critical_trips() only after
> >> finding that the trip temperature has been crossed on the way up and
> >> remove the redundant temperature check from the latter.
> >>
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > This change is premature, as it will cause handle_non_critical_trips()
> > to be called for hot/critical trips which is questionable, so I'm
> > withdrawing it for now.
> >
> > The rest of the series is still applicable, though.
> >
> >
>
> Could you explain your concerns about this, please?
> Is about the extra execution time for the non-critical trip,
> while we are in section of handling critical ASAP?
> (also it would require that extra sorting there IMO)

No, it is mostly about exposing the critical and hot trips to the
governor code that may not be ready for seeing them and get somewhat
surprised.  In particular, this would cause the User Space governor to
send uevents regarding critical and hot trip points which it has not
been doing so far and so user space may get confused.

Re: [PATCH v3 6/6] thermal: core: Relocate critical and hot trip handling

[Lukasz Luba]

On 4/10/24 16:56, Rafael J. Wysocki wrote:
> Hi Lukasz,
> 
> On Fri, Apr 5, 2024 at 9:35 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Rafael,
>>
>> On 4/4/24 10:03, Rafael J. Wysocki wrote:
>>> On Tue, Apr 2, 2024 at 9:04 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>>>> Modify handle_thermal_trip() to call handle_critical_trips() only after
>>>> finding that the trip temperature has been crossed on the way up and
>>>> remove the redundant temperature check from the latter.
>>>>
>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> This change is premature, as it will cause handle_non_critical_trips()
>>> to be called for hot/critical trips which is questionable, so I'm
>>> withdrawing it for now.
>>>
>>> The rest of the series is still applicable, though.
>>>
>>>
>>
>> Could you explain your concerns about this, please?
>> Is about the extra execution time for the non-critical trip,
>> while we are in section of handling critical ASAP?
>> (also it would require that extra sorting there IMO)
> 
> No, it is mostly about exposing the critical and hot trips to the
> governor code that may not be ready for seeing them and get somewhat
> surprised.  In particular, this would cause the User Space governor to
> send uevents regarding critical and hot trip points which it has not
> been doing so far and so user space may get confused.

Got it, thanks!