[PATCH v4 1/4] thermal/core: Encapsulate the trip point crossed function

[PATCH v4 1/4] thermal/core: Encapsulate the trip point crossed function

[Daniel Lezcano]

The routine where the trip point crossed is detected is a strategic
place where different processing will happen. Encapsulate the code in
a function, so all specific actions related with a trip point crossed
can be grouped.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Reviewed by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 0d9e9b175f93..fa5767c6d5f4 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -358,6 +358,25 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
 		tz->ops->critical(tz);
 }
 
+static void handle_thermal_trip_crossed(struct thermal_zone_device *tz, int trip,
+					int trip_temp, int trip_hyst, enum thermal_trip_type trip_type)
+{
+	if (tz->last_temperature == THERMAL_TEMP_INVALID)
+		return;
+
+	if (tz->last_temperature < trip_temp &&
+	    tz->temperature >= trip_temp) {
+		thermal_notify_tz_trip_up(tz->id, trip,
+					  tz->temperature);
+	}
+
+	if (tz->last_temperature >= trip_temp &&
+	    tz->temperature < (trip_temp - trip_hyst)) {
+		thermal_notify_tz_trip_down(tz->id, trip,
+					    tz->temperature);
+	}
+}
+
 static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 {
 	enum thermal_trip_type type;
@@ -372,16 +391,7 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 	if (tz->ops->get_trip_hyst)
 		tz->ops->get_trip_hyst(tz, trip, &hyst);
 
-	if (tz->last_temperature != THERMAL_TEMP_INVALID) {
-		if (tz->last_temperature < trip_temp &&
-		    tz->temperature >= trip_temp)
-			thermal_notify_tz_trip_up(tz->id, trip,
-						  tz->temperature);
-		if (tz->last_temperature >= trip_temp &&
-		    tz->temperature < (trip_temp - hyst))
-			thermal_notify_tz_trip_down(tz->id, trip,
-						    tz->temperature);
-	}
+	handle_thermal_trip_crossed(tz, trip, trip_temp, hyst, type);
 
 	if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
 		handle_critical_trips(tz, trip, type);
-- 
2.25.1

[PATCH v4 2/4] thermal/core: Avoid calling ->get_trip_temp() unnecessarily

[Daniel Lezcano]

As the trip temperature is already available when calling the function
handle_critical_trips(), pass it as a parameter instead of having this
function calling the ops again to retrieve the same data.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Reviewed-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 v3:
   - Massaged the patch title and the description
---
 drivers/thermal/thermal_core.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index fa5767c6d5f4..fc6ccc5edbfb 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -340,12 +340,8 @@ void thermal_zone_device_critical(struct thermal_zone_device *tz)
 EXPORT_SYMBOL(thermal_zone_device_critical);
 
 static void handle_critical_trips(struct thermal_zone_device *tz,
-				  int trip, enum thermal_trip_type trip_type)
+				  int trip, int trip_temp, enum thermal_trip_type trip_type)
 {
-	int trip_temp;
-
-	tz->ops->get_trip_temp(tz, trip, &trip_temp);
-
 	/* If we have not crossed the trip_temp, we do not care. */
 	if (trip_temp <= 0 || tz->temperature < trip_temp)
 		return;
@@ -394,7 +390,7 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 	handle_thermal_trip_crossed(tz, trip, trip_temp, hyst, type);
 
 	if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
-		handle_critical_trips(tz, trip, type);
+		handle_critical_trips(tz, trip, trip_temp, type);
 	else
 		handle_non_critical_trips(tz, trip);
 	/*
-- 
2.25.1

[PATCH v4 3/4] thermal/core: Build ascending ordered indexes for the trip points

[Daniel Lezcano]

By convention the trips points are declared in the ascending
temperature order. However, no specification for the device tree, ACPI
or documentation tells the trip points must be ordered this way.

In the other hand, we need those to be ordered to browse them at the
thermal events. But if we assume they are ordered and change the code
based on this assumption, any platform with shuffled trip points
description will be broken (if they exist).

Instead of taking the risk of breaking the existing platforms, use an
array of temperature ordered trip identifiers and make it available
for the code needing to browse the trip points in an ordered way.

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

V4:
  - Fix conflicts due to tz->trips renamed to tz->num_trips
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 63 +++++++++++++++++++++++++++-------
 include/linux/thermal.h        |  2 ++
 2 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index fc6ccc5edbfb..f274dc7d9c48 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -355,7 +355,8 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
 }
 
 static void handle_thermal_trip_crossed(struct thermal_zone_device *tz, int trip,
-					int trip_temp, int trip_hyst, enum thermal_trip_type trip_type)
+					int trip_temp, int trip_hyst,
+					enum thermal_trip_type trip_type)
 {
 	if (tz->last_temperature == THERMAL_TEMP_INVALID)
 		return;
@@ -1171,6 +1172,47 @@ static void thermal_set_delay_jiffies(unsigned long *delay_jiffies, int delay_ms
 	if (delay_ms > 1000)
 		*delay_jiffies = round_jiffies(*delay_jiffies);
 }
+	
+static void sort_trips_indexes(struct thermal_zone_device *tz)
+{
+	int i, j;
+
+	for (i = 0; i < tz->num_trips; i++)
+		tz->trips_indexes[i] = i;
+ 
+	for (i = 0; i < tz->num_trips; i++) {
+
+		for (j = i + 1; j < tz->num_trips; j++) {
+			int t1, t2;
+
+			tz->ops->get_trip_temp(tz, tz->trips_indexes[i], &t1);
+			tz->ops->get_trip_temp(tz, tz->trips_indexes[j], &t2);
+
+			if (t1 > t2)
+				swap(tz->trips_indexes[i], tz->trips_indexes[j]);
+		}
+ 	}
+}
+
+static int thermal_zone_device_trip_init(struct thermal_zone_device *tz)
+{
+	enum thermal_trip_type trip_type;
+	int trip_temp, i;
+	
+	tz->trips_indexes = kzalloc(tz->num_trips * sizeof(int), GFP_KERNEL);
+	if (!tz->trips_indexes)
+		return -ENOMEM;
+
+	for (i = 0; i < tz->num_trips; i++) {
+		if (tz->ops->get_trip_type(tz, i, &trip_type) ||
+		    tz->ops->get_trip_temp(tz, i, &trip_temp) || !trip_temp)
+			set_bit(i, &tz->trips_disabled);
+	}
+
+	sort_trips_indexes(tz);
+
+	return 0;
+}
 
 /**
  * thermal_zone_device_register_with_trips() - register a new thermal zone device
@@ -1204,11 +1246,8 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 					int polling_delay)
 {
 	struct thermal_zone_device *tz;
-	enum thermal_trip_type trip_type;
-	int trip_temp;
 	int id;
 	int result;
-	int count;
 	struct thermal_governor *governor;
 
 	if (!type || strlen(type) == 0) {
@@ -1281,12 +1320,9 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 	if (result)
 		goto release_device;
 
-	for (count = 0; count < num_trips; count++) {
-		if (tz->ops->get_trip_type(tz, count, &trip_type) ||
-		    tz->ops->get_trip_temp(tz, count, &trip_temp) ||
-		    !trip_temp)
-			set_bit(count, &tz->trips_disabled);
-	}
+	result = thermal_zone_device_trip_init(tz);
+	if (result)
+		goto unregister;
 
 	/* Update 'this' zone's governor information */
 	mutex_lock(&thermal_governor_lock);
@@ -1299,7 +1335,7 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 	result = thermal_set_governor(tz, governor);
 	if (result) {
 		mutex_unlock(&thermal_governor_lock);
-		goto unregister;
+		goto kfree_indexes;
 	}
 
 	mutex_unlock(&thermal_governor_lock);
@@ -1307,7 +1343,7 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 	if (!tz->tzp || !tz->tzp->no_hwmon) {
 		result = thermal_add_hwmon_sysfs(tz);
 		if (result)
-			goto unregister;
+			goto kfree_indexes;
 	}
 
 	mutex_lock(&thermal_list_lock);
@@ -1328,6 +1364,8 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 
 	return tz;
 
+kfree_indexes:
+	kfree(tz->trips_indexes);
 unregister:
 	device_del(&tz->device);
 release_device:
@@ -1406,6 +1444,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 	thermal_remove_hwmon_sysfs(tz);
 	ida_simple_remove(&thermal_tz_ida, tz->id);
 	ida_destroy(&tz->ida);
+	kfree(tz->trips_indexes);
 	mutex_destroy(&tz->lock);
 	device_unregister(&tz->device);
 
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 1386c713885d..4e576184df49 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -125,6 +125,7 @@ struct thermal_cooling_device {
  * @devdata:	private pointer for device private data
  * @trips:	an array of struct thermal_trip
  * @num_trips:	number of trip points the thermal zone supports
+ * @trips_indexes:	an array of sorted trip points indexes
  * @trips_disabled;	bitmap for disabled trips
  * @passive_delay_jiffies: number of jiffies to wait between polls when
  *			performing passive cooling.
@@ -166,6 +167,7 @@ struct thermal_zone_device {
 	void *devdata;
 	struct thermal_trip *trips;
 	int num_trips;
+	int *trips_indexes;
 	unsigned long trips_disabled;	/* bitmap for disabled trips */
 	unsigned long passive_delay_jiffies;
 	unsigned long polling_delay_jiffies;
-- 
2.25.1

[PATCH v4 4/4] thermal/core: Fix thermal trip cross point

[Daniel Lezcano]

The routine doing trip point crossing the way up or down is actually
wrong.

A trip point is composed with a trip temperature and a hysteresis.

The trip temperature is used to detect when the trip point is crossed
the way up.

The trip temperature minus the hysteresis is used to detect when the
trip point is crossed the way down.

|-----------low--------high------------|
             |<--------->|
             |    hyst   |
             |           |
             |          -|--> crossed the way up
             |
         <---|-- crossed the way down

For that, there is a two point comparison: the current temperature and
the previous temperature.

The actual code assumes if the current temperature is greater than the
trip temperature and the previous temperature was lesser, then the
trip point is crossed the way up. That is true only if we crossed the
way down the low temperature boundary from the previous temperature or
if the hysteresis is zero. The temperature can decrease between the
low and high, so the trip point is not crossed the way down and then
increase again and cross the high temperature raising a new trip point
crossed detection which is incorrect. The same scenario happens when
crossing the way down.

The trip point crossing the way up and down must act as parenthesis, a
trip point down must close a trip point up. Today we have multiple
trip point up without the corresponding trip point down.

In order to fix that, we store the previous trip point which gives the
information about the previous trip and we change the trip point
browsing order depending on the temperature trend: in the ascending
order when the temperature trend is raising, otherwise in the
descending order.

As a sidenote, the thermal_zone_device structure has already the
prev_trip_low and prev_trip_high information which are used by the
thermal_zone_set_trips() function. This one can be changed to be
triggered by the trip temperature crossing function, which makes more
sense, and the two fields will disappear.

Tested on a rk3399-rock960 with thermal stress and 4 trip points. Also
tested with temperature emulation to create a temperature jump
directly to the second trip point.

Reviewed-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
V4:

  - Fix conflict due to tz->trips renamed to tz->num_trips

V3:

  - Use the ordered indexes introduced in the previous patch as the
    trip could be not ordered

V2:
  - As spotted by Zhang Rui, the trip cross notification does not
  work if the temperature drops and crosses two trip points in the
  same update interval. In order to fix that, we browse the trip point
  in the ascending order when the temperature trend is raising,
  otherwise in the descending order.
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 54 ++++++++++++++++++++++++----------
 include/linux/thermal.h        |  2 ++
 2 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index f274dc7d9c48..c43d078b7656 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -354,30 +354,48 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
 		tz->ops->critical(tz);
 }
 
-static void handle_thermal_trip_crossed(struct thermal_zone_device *tz, int trip,
+static void handle_thermal_trip_crossed(struct thermal_zone_device *tz, int index,
 					int trip_temp, int trip_hyst,
 					enum thermal_trip_type trip_type)
 {
+	int trip_low_temp = trip_temp - trip_hyst;
+	int trip = tz->trips_indexes[index];
+	
 	if (tz->last_temperature == THERMAL_TEMP_INVALID)
 		return;
 
-	if (tz->last_temperature < trip_temp &&
-	    tz->temperature >= trip_temp) {
-		thermal_notify_tz_trip_up(tz->id, trip,
-					  tz->temperature);
-	}
-
-	if (tz->last_temperature >= trip_temp &&
-	    tz->temperature < (trip_temp - trip_hyst)) {
-		thermal_notify_tz_trip_down(tz->id, trip,
-					    tz->temperature);
+	/*
+	 * Due to the hysteresis, a third information is needed to
+	 * detect when the temperature is wavering between the
+	 * trip_low_temp and the trip_temp. A trip point is crossed
+	 * the way up only if the temperature is above it while the
+	 * previous temperature was below *and* we crossed the
+	 * trip_temp_low before. The previous trip point give us the
+	 * previous trip point transition. The similar problem exists
+	 * when crossing the way down.
+	 *
+	 * Note the mechanism works only if the caller of the function
+	 * invoke the function with the trip point ascending or
+	 * descending regarding the temperature trend. A temperature
+	 * drop trend will browse the trip point in the descending
+	 * order
+	 */
+	if (tz->last_temperature < trip_temp && tz->temperature >= trip_temp &&
+	    index != tz->prev_index) {
+		thermal_notify_tz_trip_up(tz->id, trip, tz->temperature);
+		tz->prev_index = index;
+	} else if (tz->last_temperature >= trip_low_temp && tz->temperature < trip_low_temp &&
+		   index == tz->prev_index) {
+		thermal_notify_tz_trip_down(tz->id, trip, tz->temperature);
+		tz->prev_index--;
 	}
 }
 
-static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
+static void handle_thermal_trip(struct thermal_zone_device *tz, int index)
 {
 	enum thermal_trip_type type;
 	int trip_temp, hyst = 0;
+	int trip = tz->trips_indexes[index];
 
 	/* Ignore disabled trip points */
 	if (test_bit(trip, &tz->trips_disabled))
@@ -388,7 +406,7 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 	if (tz->ops->get_trip_hyst)
 		tz->ops->get_trip_hyst(tz, trip, &hyst);
 
-	handle_thermal_trip_crossed(tz, trip, trip_temp, hyst, type);
+	handle_thermal_trip_crossed(tz, index, trip_temp, hyst, type);
 
 	if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
 		handle_critical_trips(tz, trip, trip_temp, type);
@@ -428,6 +446,7 @@ static void thermal_zone_device_init(struct thermal_zone_device *tz)
 {
 	struct thermal_instance *pos;
 	tz->temperature = THERMAL_TEMP_INVALID;
+	tz->prev_index = -1;
 	tz->prev_low_trip = -INT_MAX;
 	tz->prev_high_trip = INT_MAX;
 	list_for_each_entry(pos, &tz->thermal_instances, tz_node)
@@ -512,8 +531,13 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
 
 	tz->notify_event = event;
 
-	for (count = 0; count < tz->num_trips; count++)
-		handle_thermal_trip(tz, count);
+	if (tz->last_temperature <=  tz->temperature) {
+		for (count = 0; count < tz->num_trips; count++)
+			handle_thermal_trip(tz, count);
+	} else {
+		for (count = tz->num_trips; count >= 0; count--)
+			handle_thermal_trip(tz, count);
+	}
 }
 EXPORT_SYMBOL_GPL(thermal_zone_device_update);
 
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 4e576184df49..65ba65ec95b0 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -138,6 +138,7 @@ struct thermal_cooling_device {
  * @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_index:		previous index pointing to the trip point the thermal zone was
  * @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
@@ -175,6 +176,7 @@ struct thermal_zone_device {
 	int last_temperature;
 	int emul_temperature;
 	int passive;
+	int prev_index;
 	int prev_low_trip;
 	int prev_high_trip;
 	atomic_t need_update;
-- 
2.25.1

Re: [PATCH v4 3/4] thermal/core: Build ascending ordered indexes for the trip points

[Rafael J. Wysocki]

On Mon, Jul 18, 2022 at 4:50 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> By convention the trips points are declared in the ascending
> temperature order. However, no specification for the device tree, ACPI
> or documentation tells the trip points must be ordered this way.
>
> In the other hand, we need those to be ordered to browse them at the

s/In/On/

> thermal events.

What if they are all inspected every time?

> But if we assume they are ordered and change the code
> based on this assumption, any platform with shuffled trip points
> description will be broken (if they exist).
>
> Instead of taking the risk of breaking the existing platforms, use an
> array of temperature ordered trip identifiers and make it available
> for the code needing to browse the trip points in an ordered way.

Well, having ops->get_trip_temp() suggests that the trip temperatures
can be dynamic.  Is the ordering guaranteed to be preserved in that
case?

Anyway, if they need to be sorted, why don't we just sort them
properly instead of adding this extra array?

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>
> V4:
>   - Fix conflicts due to tz->trips renamed to tz->num_trips
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 63 +++++++++++++++++++++++++++-------
>  include/linux/thermal.h        |  2 ++
>  2 files changed, 53 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index fc6ccc5edbfb..f274dc7d9c48 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -355,7 +355,8 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
>  }
>
>  static void handle_thermal_trip_crossed(struct thermal_zone_device *tz, int trip,
> -                                       int trip_temp, int trip_hyst, enum thermal_trip_type trip_type)
> +                                       int trip_temp, int trip_hyst,
> +                                       enum thermal_trip_type trip_type)
>  {
>         if (tz->last_temperature == THERMAL_TEMP_INVALID)
>                 return;
> @@ -1171,6 +1172,47 @@ static void thermal_set_delay_jiffies(unsigned long *delay_jiffies, int delay_ms
>         if (delay_ms > 1000)
>                 *delay_jiffies = round_jiffies(*delay_jiffies);
>  }
> +
> +static void sort_trips_indexes(struct thermal_zone_device *tz)
> +{
> +       int i, j;
> +
> +       for (i = 0; i < tz->num_trips; i++)
> +               tz->trips_indexes[i] = i;
> +
> +       for (i = 0; i < tz->num_trips; i++) {
> +
> +               for (j = i + 1; j < tz->num_trips; j++) {
> +                       int t1, t2;
> +
> +                       tz->ops->get_trip_temp(tz, tz->trips_indexes[i], &t1);
> +                       tz->ops->get_trip_temp(tz, tz->trips_indexes[j], &t2);
> +
> +                       if (t1 > t2)
> +                               swap(tz->trips_indexes[i], tz->trips_indexes[j]);
> +               }
> +       }
> +}
> +
> +static int thermal_zone_device_trip_init(struct thermal_zone_device *tz)
> +{
> +       enum thermal_trip_type trip_type;
> +       int trip_temp, i;
> +
> +       tz->trips_indexes = kzalloc(tz->num_trips * sizeof(int), GFP_KERNEL);
> +       if (!tz->trips_indexes)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < tz->num_trips; i++) {
> +               if (tz->ops->get_trip_type(tz, i, &trip_type) ||
> +                   tz->ops->get_trip_temp(tz, i, &trip_temp) || !trip_temp)
> +                       set_bit(i, &tz->trips_disabled);
> +       }
> +
> +       sort_trips_indexes(tz);
> +
> +       return 0;
> +}
>
>  /**
>   * thermal_zone_device_register_with_trips() - register a new thermal zone device
> @@ -1204,11 +1246,8 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
>                                         int polling_delay)
>  {
>         struct thermal_zone_device *tz;
> -       enum thermal_trip_type trip_type;
> -       int trip_temp;
>         int id;
>         int result;
> -       int count;
>         struct thermal_governor *governor;
>
>         if (!type || strlen(type) == 0) {
> @@ -1281,12 +1320,9 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
>         if (result)
>                 goto release_device;
>
> -       for (count = 0; count < num_trips; count++) {
> -               if (tz->ops->get_trip_type(tz, count, &trip_type) ||
> -                   tz->ops->get_trip_temp(tz, count, &trip_temp) ||
> -                   !trip_temp)
> -                       set_bit(count, &tz->trips_disabled);
> -       }
> +       result = thermal_zone_device_trip_init(tz);
> +       if (result)
> +               goto unregister;
>
>         /* Update 'this' zone's governor information */
>         mutex_lock(&thermal_governor_lock);
> @@ -1299,7 +1335,7 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
>         result = thermal_set_governor(tz, governor);
>         if (result) {
>                 mutex_unlock(&thermal_governor_lock);
> -               goto unregister;
> +               goto kfree_indexes;
>         }
>
>         mutex_unlock(&thermal_governor_lock);
> @@ -1307,7 +1343,7 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
>         if (!tz->tzp || !tz->tzp->no_hwmon) {
>                 result = thermal_add_hwmon_sysfs(tz);
>                 if (result)
> -                       goto unregister;
> +                       goto kfree_indexes;
>         }
>
>         mutex_lock(&thermal_list_lock);
> @@ -1328,6 +1364,8 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
>
>         return tz;
>
> +kfree_indexes:
> +       kfree(tz->trips_indexes);
>  unregister:
>         device_del(&tz->device);
>  release_device:
> @@ -1406,6 +1444,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>         thermal_remove_hwmon_sysfs(tz);
>         ida_simple_remove(&thermal_tz_ida, tz->id);
>         ida_destroy(&tz->ida);
> +       kfree(tz->trips_indexes);
>         mutex_destroy(&tz->lock);
>         device_unregister(&tz->device);
>
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 1386c713885d..4e576184df49 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -125,6 +125,7 @@ struct thermal_cooling_device {
>   * @devdata:   private pointer for device private data
>   * @trips:     an array of struct thermal_trip
>   * @num_trips: number of trip points the thermal zone supports
> + * @trips_indexes:     an array of sorted trip points indexes
>   * @trips_disabled;    bitmap for disabled trips
>   * @passive_delay_jiffies: number of jiffies to wait between polls when
>   *                     performing passive cooling.
> @@ -166,6 +167,7 @@ struct thermal_zone_device {
>         void *devdata;
>         struct thermal_trip *trips;
>         int num_trips;
> +       int *trips_indexes;
>         unsigned long trips_disabled;   /* bitmap for disabled trips */
>         unsigned long passive_delay_jiffies;
>         unsigned long polling_delay_jiffies;
> --
> 2.25.1
>

Re: [PATCH v4 3/4] thermal/core: Build ascending ordered indexes for the trip points

[Daniel Lezcano]

On 19/07/2022 20:56, Rafael J. Wysocki wrote:
> On Mon, Jul 18, 2022 at 4:50 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> By convention the trips points are declared in the ascending
>> temperature order. However, no specification for the device tree, ACPI
>> or documentation tells the trip points must be ordered this way.
>>
>> In the other hand, we need those to be ordered to browse them at the
> 
> s/In/On/
> 
>> thermal events.
> 
> What if they are all inspected every time?

My bad, my sentence is confusing. The trip point are browsed every time 
and we need to have them ordered to detect correctly the thermal events.

>> But if we assume they are ordered and change the code
>> based on this assumption, any platform with shuffled trip points
>> description will be broken (if they exist).
>>
>> Instead of taking the risk of breaking the existing platforms, use an
>> array of temperature ordered trip identifiers and make it available
>> for the code needing to browse the trip points in an ordered way.
> 
> Well, having ops->get_trip_temp() suggests that the trip temperatures
> can be dynamic.  Is the ordering guaranteed to be preserved in that
> case?

The number of trips can not be changed. It is fixed when the thermal 
zone is created AFAICT. The get_trip_temp() is just a way to let the 
different driver declare their own trip structure which is actually 
something I'm trying to fix by moving the structure thermal_trip inside 
the thermal zone. But that is a longer and separate work.

> Anyway, if they need to be sorted, why don't we just sort them
> properly instead of adding this extra array?

We can not because ATM the trip points array is private to the different 
sensors.

[ ... ]


-- 
<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 v4 3/4] thermal/core: Build ascending ordered indexes for the trip points

[Rafael J. Wysocki]

On Thu, Jul 21, 2022 at 12:59 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 19/07/2022 20:56, Rafael J. Wysocki wrote:
> > On Mon, Jul 18, 2022 at 4:50 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> By convention the trips points are declared in the ascending
> >> temperature order. However, no specification for the device tree, ACPI
> >> or documentation tells the trip points must be ordered this way.
> >>
> >> In the other hand, we need those to be ordered to browse them at the
> >
> > s/In/On/
> >
> >> thermal events.
> >
> > What if they are all inspected every time?
>
> My bad, my sentence is confusing. The trip point are browsed every time
> and we need to have them ordered to detect correctly the thermal events.

I see.

So this mostly is a preparation for patch 4, isn't it?

> >> But if we assume they are ordered and change the code
> >> based on this assumption, any platform with shuffled trip points
> >> description will be broken (if they exist).
> >>
> >> Instead of taking the risk of breaking the existing platforms, use an
> >> array of temperature ordered trip identifiers and make it available
> >> for the code needing to browse the trip points in an ordered way.
> >
> > Well, having ops->get_trip_temp() suggests that the trip temperatures
> > can be dynamic.  Is the ordering guaranteed to be preserved in that
> > case?
>
> The number of trips can not be changed. It is fixed when the thermal
> zone is created AFAICT.

The current code appears to assume that and I think that this is a
reasonable expectation.

> The get_trip_temp() is just a way to let the
> different driver declare their own trip structure which is actually
> something I'm trying to fix by moving the structure thermal_trip inside
> the thermal zone. But that is a longer and separate work.

Well, I'm not sure.

Trip point temperatures can be set via trip_point_temp_store() at
least in principle.  How is it guaranteed that this won't affect the
ordering?

> > Anyway, if they need to be sorted, why don't we just sort them
> > properly instead of adding this extra array?
>
> We can not because ATM the trip points array is private to the different
> sensors.

Well, the core could create an array or list of trip points for the
thermal zone during registration and populate it from the
driver-provided data.  Then, it could be sorted at the creation time.

However, the above question needs to be addressed first.

Re: [PATCH v4 3/4] thermal/core: Build ascending ordered indexes for the trip points

[Daniel Lezcano]

On 21/07/2022 13:25, Rafael J. Wysocki wrote:
> On Thu, Jul 21, 2022 at 12:59 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 19/07/2022 20:56, Rafael J. Wysocki wrote:
>>> On Mon, Jul 18, 2022 at 4:50 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> By convention the trips points are declared in the ascending
>>>> temperature order. However, no specification for the device tree, ACPI
>>>> or documentation tells the trip points must be ordered this way.
>>>>
>>>> In the other hand, we need those to be ordered to browse them at the
>>>
>>> s/In/On/
>>>
>>>> thermal events.
>>>
>>> What if they are all inspected every time?
>>
>> My bad, my sentence is confusing. The trip point are browsed every time
>> and we need to have them ordered to detect correctly the thermal events.
> 
> I see.
> 
> So this mostly is a preparation for patch 4, isn't it?

Yes, it is correct

>>>> But if we assume they are ordered and change the code
>>>> based on this assumption, any platform with shuffled trip points
>>>> description will be broken (if they exist).
>>>>
>>>> Instead of taking the risk of breaking the existing platforms, use an
>>>> array of temperature ordered trip identifiers and make it available
>>>> for the code needing to browse the trip points in an ordered way.
>>>
>>> Well, having ops->get_trip_temp() suggests that the trip temperatures
>>> can be dynamic.  Is the ordering guaranteed to be preserved in that
>>> case?
>>
>> The number of trips can not be changed. It is fixed when the thermal
>> zone is created AFAICT.
> 
> The current code appears to assume that and I think that this is a
> reasonable expectation.
> 
>> The get_trip_temp() is just a way to let the
>> different driver declare their own trip structure which is actually
>> something I'm trying to fix by moving the structure thermal_trip inside
>> the thermal zone. But that is a longer and separate work.
> 
> Well, I'm not sure.
> 
> Trip point temperatures can be set via trip_point_temp_store() at
> least in principle.  How is it guaranteed that this won't affect the
> ordering?

Right, that is a good point. I don't see a logical use case where a trip 
point will be set below or above the previous or the next one, so the 
order should be kept. However, strictly speaking, nothing prevents that 
so I guess we need to reorder the trips when one is changed. It should 
be a one line call.


>>> Anyway, if they need to be sorted, why don't we just sort them
>>> properly instead of adding this extra array?
>>
>> We can not because ATM the trip points array is private to the different
>> sensors.
> 
> Well, the core could create an array or list of trip points for the
> thermal zone during registration and populate it from the
> driver-provided data.  Then, it could be sorted at the creation time.


There won't be any benefit ATM. The get_trip_temp/type/hyst ops are 
called all over the place. If we create a second sorted trip point array 
and use it in the core code, then all those ops should be replaced to 
use the sorted array instead of addressing the private trip structure. A 
big deal in terms of changes.

If we don't do the ops changes, then it is simpler to have an array of 
index->trip id and the impact is small.

But I agree we should have a sorted trip array per thermal zone and stop 
using the ops->get_trip_temp|type|hyst. That is part of the work I'm 
doing in parallel to cleanup the thermal-of and I've the plan to migrate 
all the sensors to use the struct thermal_trip instead of private data. 
 From there we will be able to get rid of the get_trip[*] and the sorted 
trip indexes array.

All these changes are not feasible in the short term. I would like to 
keep the indexes trip array approach to fix the trip cross events which 
is broken right now and then go forward with the struct thermal_trip 
changes and the thermal-of cleanups I've sent last week.

Does it sound reasonable ?


> However, the above question needs to be addressed first.


-- 
<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 v4 3/4] thermal/core: Build ascending ordered indexes for the trip points

[Rafael J. Wysocki]

On Thu, Jul 21, 2022 at 11:15 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 21/07/2022 13:25, Rafael J. Wysocki wrote:
> > On Thu, Jul 21, 2022 at 12:59 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 19/07/2022 20:56, Rafael J. Wysocki wrote:
> >>> On Mon, Jul 18, 2022 at 4:50 PM Daniel Lezcano
> >>> <daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>> By convention the trips points are declared in the ascending
> >>>> temperature order. However, no specification for the device tree, ACPI
> >>>> or documentation tells the trip points must be ordered this way.
> >>>>
> >>>> In the other hand, we need those to be ordered to browse them at the
> >>>
> >>> s/In/On/
> >>>
> >>>> thermal events.
> >>>
> >>> What if they are all inspected every time?
> >>
> >> My bad, my sentence is confusing. The trip point are browsed every time
> >> and we need to have them ordered to detect correctly the thermal events.
> >
> > I see.
> >
> > So this mostly is a preparation for patch 4, isn't it?
>
> Yes, it is correct
>
> >>>> But if we assume they are ordered and change the code
> >>>> based on this assumption, any platform with shuffled trip points
> >>>> description will be broken (if they exist).
> >>>>
> >>>> Instead of taking the risk of breaking the existing platforms, use an
> >>>> array of temperature ordered trip identifiers and make it available
> >>>> for the code needing to browse the trip points in an ordered way.
> >>>
> >>> Well, having ops->get_trip_temp() suggests that the trip temperatures
> >>> can be dynamic.  Is the ordering guaranteed to be preserved in that
> >>> case?
> >>
> >> The number of trips can not be changed. It is fixed when the thermal
> >> zone is created AFAICT.
> >
> > The current code appears to assume that and I think that this is a
> > reasonable expectation.
> >
> >> The get_trip_temp() is just a way to let the
> >> different driver declare their own trip structure which is actually
> >> something I'm trying to fix by moving the structure thermal_trip inside
> >> the thermal zone. But that is a longer and separate work.
> >
> > Well, I'm not sure.
> >
> > Trip point temperatures can be set via trip_point_temp_store() at
> > least in principle.  How is it guaranteed that this won't affect the
> > ordering?
>
> Right, that is a good point. I don't see a logical use case where a trip
> point will be set below or above the previous or the next one, so the
> order should be kept.

Good.

> However, strictly speaking, nothing prevents that
> so I guess we need to reorder the trips when one is changed. It should
> be a one line call.

Or modify trip_point_temp_store() to preserve the ordering of trip
points (which would be way simpler if trip point temperatures were
stored as numbers in a sorted array - see below).

>
> >>> Anyway, if they need to be sorted, why don't we just sort them
> >>> properly instead of adding this extra array?
> >>
> >> We can not because ATM the trip points array is private to the different
> >> sensors.
> >
> > Well, the core could create an array or list of trip points for the
> > thermal zone during registration and populate it from the
> > driver-provided data.  Then, it could be sorted at the creation time.
>
>
> There won't be any benefit ATM.

What about less overhead?  And the code being easier to understand?
And no hidden assumptions?  Hmm??

> The get_trip_temp/type/hyst ops are
> called all over the place. If we create a second sorted trip point array
> and use it in the core code, then all those ops should be replaced to
> use the sorted array instead of addressing the private trip structure.

Right.

> A big deal in terms of changes.

It is not overwhelming in my view.

> If we don't do the ops changes, then it is simpler to have an array of
> index->trip id and the impact is small.
>
> But I agree we should have a sorted trip array per thermal zone and stop
> using the ops->get_trip_temp|type|hyst. That is part of the work I'm
> doing in parallel to cleanup the thermal-of and I've the plan to migrate
> all the sensors to use the struct thermal_trip instead of private data.
>  From there we will be able to get rid of the get_trip[*] and the sorted
> trip indexes array.

Well, what about making the core use struct thermal_trip internally in
the first place?

Except that I don't like the np member of that one.

> All these changes are not feasible in the short term.

They can be made in the opposite direction, starting at the core
level.  Then, it would be clear where you were going.

> I would like to
> keep the indexes trip array approach to fix the trip cross events which
> is broken right now and then go forward with the struct thermal_trip
> changes and the thermal-of cleanups I've sent last week.
>
> Does it sound reasonable ?

I'm not convinced about the need to make the code more complicated if
the overall direction is to simplify it.

I understand that you want to avoid regressing things, but you want to
make these changes eventually anyway, so why to you think that the
risk of regressing things would be smaller in the future, after making
the code more complicated than it is now?  Sounds counter-intuitive to
me.

Re: [PATCH v4 3/4] thermal/core: Build ascending ordered indexes for the trip points

[Daniel Lezcano]

Hi Rafael,

On 22/07/2022 19:40, Rafael J. Wysocki wrote:

[ ... ]

> They can be made in the opposite direction, starting at the core
> level.  Then, it would be clear where you were going.
> 
>> I would like to
>> keep the indexes trip array approach to fix the trip cross events which
>> is broken right now and then go forward with the struct thermal_trip
>> changes and the thermal-of cleanups I've sent last week.
>>
>> Does it sound reasonable ?
> 
> I'm not convinced about the need to make the code more complicated if
> the overall direction is to simplify it.
> 
> I understand that you want to avoid regressing things, but you want to
> make these changes eventually anyway, so why to you think that the
> risk of regressing things would be smaller in the future, after making
> the code more complicated than it is now?  Sounds counter-intuitive to
> me.

Ok, I'll rework the core code for that.

Having the series [1] and the new version of [2] will have the trip 
point partly reworked.

Thanks
   -- Daniel

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

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

Re: [PATCH v4 2/4] thermal/core: Avoid calling ->get_trip_temp() unnecessarily

[Daniel Lezcano]

On 18/07/2022 16:50, Daniel Lezcano wrote:
> As the trip temperature is already available when calling the function
> handle_critical_trips(), pass it as a parameter instead of having this
> function calling the ops again to retrieve the same data.
> 
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
> Reviewed-by: Zhang Rui <rui.zhang@intel.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>   v3:
>     - Massaged the patch title and the description
> ---
>   drivers/thermal/thermal_core.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)

I'll apply this patch individually

Thanks
   -- D.


-- 
<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 v4 3/4] thermal/core: Build ascending ordered indexes for the trip points

[Daniel Lezcano]

On 25/07/2022 18:29, Daniel Lezcano wrote:
> 
> Hi Rafael,
> 
> On 22/07/2022 19:40, Rafael J. Wysocki wrote:
> 
> [ ... ]
> 
>> They can be made in the opposite direction, starting at the core
>> level.  Then, it would be clear where you were going.
>>
>>> I would like to
>>> keep the indexes trip array approach to fix the trip cross events which
>>> is broken right now and then go forward with the struct thermal_trip
>>> changes and the thermal-of cleanups I've sent last week.
>>>
>>> Does it sound reasonable ?
>>
>> I'm not convinced about the need to make the code more complicated if
>> the overall direction is to simplify it.
>>
>> I understand that you want to avoid regressing things, but you want to
>> make these changes eventually anyway, so why to you think that the
>> risk of regressing things would be smaller in the future, after making
>> the code more complicated than it is now?  Sounds counter-intuitive to
>> me.
> 
> Ok, I'll rework the core code for that.
> 
> Having the series [1] and the new version of [2] will have the trip 
> point partly reworked.
> 
> Thanks
>    -- Daniel

Adding missing pointers:

[1] 
https://lore.kernel.org/lkml/20220722200007.1839356-7-daniel.lezcano@linexp.org/T/

[2] 
https://lore.kernel.org/lkml/20220710212423.681301-1-daniel.lezcano@linexp.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