diff mbox series

[v2,2/5] thermal: trip: Make thermal_zone_set_trips() use trip thresholds

Message ID 2340783.ElGaqSPkdT@kreacher
State New
Headers show
Series thermal: core: Assorted improvements for v6.11 | expand

Commit Message

Rafael J. Wysocki May 28, 2024, 4:51 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify thermal_zone_set_trips() to use trip thresholds instead of
computing the low temperature for each trip to avoid deriving both
the high and low temperature levels from the same trip (which may
happen if the zone temperature falls into the hysteresis range of
one trip).

Accordingly, make __thermal_zone_device_update() call
thermal_zone_set_trips() later, when threshold values have been
updated for all trips.

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

v1 -> v2: Rebase.

---
 drivers/thermal/thermal_core.c |    4 ++--
 drivers/thermal/thermal_trip.c |   14 ++++----------
 2 files changed, 6 insertions(+), 12 deletions(-)

Comments

Daniel Lezcano June 10, 2024, 6:01 p.m. UTC | #1 
On 28/05/2024 18:51, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Modify thermal_zone_set_trips() to use trip thresholds instead of
> computing the low temperature for each trip to avoid deriving both
> the high and low temperature levels from the same trip (which may
> happen if the zone temperature falls into the hysteresis range of
> one trip).
> 
> Accordingly, make __thermal_zone_device_update() call
> thermal_zone_set_trips() later, when threshold values have been
> updated for all trips.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v1 -> v2: Rebase.
> 
> ---
>   drivers/thermal/thermal_core.c |    4 ++--
>   drivers/thermal/thermal_trip.c |   14 ++++----------
>   2 files changed, 6 insertions(+), 12 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -513,13 +513,13 @@ void __thermal_zone_device_update(struct
>   	if (tz->temperature == THERMAL_TEMP_INVALID)
>   		return;
>   
> -	thermal_zone_set_trips(tz);
> -
>   	tz->notify_event = event;
>   
>   	for_each_trip_desc(tz, td)
>   		handle_thermal_trip(tz, td, &way_up_list, &way_down_list);

Would it make sense to use the for_each_trip_desc() loop here and update
low and high on the fly in this loop ?

If a trip point is crossed the way up or down, then 
handle_thermal_trip() returns a value which in turn results in updating 
low and high. If low and high are changed then the we call 
thermal_zone_set_trips() after the loop.

The results for the thermal_zone_set_trips() will be the loop, the low, 
high, prev_low_trip and prev_high_trip variables going away.

The resulting function should be:

void thermal_zone_set_trips(struct thermal_zone_device *tz, int low, int 
high)
{
         int ret;

         lockdep_assert_held(&tz->lock);

         if (!tz->ops.set_trips)
                 return;

         /* 
 

          * Set a temperature window. When this window is left the 
driver 

          * must inform the thermal core via thermal_zone_device_update. 
 

          */
         ret = tz->ops.set_trips(tz, low, high);
         if (ret)
                 dev_err(&tz->device, "Failed to set trips: %d\n", ret);
}

But if you consider that is an additional change, then:

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


> +	thermal_zone_set_trips(tz);
> +
>   	list_sort(&way_up_list, &way_up_list, thermal_trip_notify_cmp);
>   	list_for_each_entry(td, &way_up_list, notify_list_node)
>   		thermal_trip_crossed(tz, &td->trip, governor, true);
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -88,17 +88,11 @@ void thermal_zone_set_trips(struct therm
>   		return;
>   
>   	for_each_trip_desc(tz, td) {
> -		const struct thermal_trip *trip = &td->trip;
> -		int trip_low;
> +		if (td->threshold < tz->temperature && td->threshold > low)
> +			low = td->threshold;
>   
> -		trip_low = trip->temperature - trip->hysteresis;
> -
> -		if (trip_low < tz->temperature && trip_low > low)
> -			low = trip_low;
> -
> -		if (trip->temperature > tz->temperature &&
> -		    trip->temperature < high)
> -			high = trip->temperature;
> +		if (td->threshold > tz->temperature && td->threshold < high)
> +			high = td->threshold;
>   	}
>   
>   	/* No need to change trip points */
> 
> 
>
diff mbox series

Patch

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -513,13 +513,13 @@  void __thermal_zone_device_update(struct
 	if (tz->temperature == THERMAL_TEMP_INVALID)
 		return;
 
-	thermal_zone_set_trips(tz);
-
 	tz->notify_event = event;
 
 	for_each_trip_desc(tz, td)
 		handle_thermal_trip(tz, td, &way_up_list, &way_down_list);
 
+	thermal_zone_set_trips(tz);
+
 	list_sort(&way_up_list, &way_up_list, thermal_trip_notify_cmp);
 	list_for_each_entry(td, &way_up_list, notify_list_node)
 		thermal_trip_crossed(tz, &td->trip, governor, true);
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -88,17 +88,11 @@  void thermal_zone_set_trips(struct therm
 		return;
 
 	for_each_trip_desc(tz, td) {
-		const struct thermal_trip *trip = &td->trip;
-		int trip_low;
+		if (td->threshold < tz->temperature && td->threshold > low)
+			low = td->threshold;
 
-		trip_low = trip->temperature - trip->hysteresis;
-
-		if (trip_low < tz->temperature && trip_low > low)
-			low = trip_low;
-
-		if (trip->temperature > tz->temperature &&
-		    trip->temperature < high)
-			high = trip->temperature;
+		if (td->threshold > tz->temperature && td->threshold < high)
+			high = td->threshold;
 	}
 
 	/* No need to change trip points */