1. Patchew
  2. linux
  3. [PATCH v2 0/5] thermal: core: Assorted improvements for v6.11
  4. View patch

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

Rafael J. Wysocki posted 5 patches 1 year, 6 months ago
Only 0 patches received!
[PATCH v2 2/5] thermal: trip: Make thermal_zone_set_trips() use trip thresholds
Posted by Rafael J. Wysocki 1 year, 6 months ago 
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);
 
+	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 */
Re: [PATCH v2 2/5] thermal: trip: Make thermal_zone_set_trips() use trip thresholds
Posted by Daniel Lezcano 1 year, 6 months ago 
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 */
> 
> 
> 

-- 
<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 v2 2/5] thermal: trip: Make thermal_zone_set_trips() use trip thresholds
Posted by Rafael J. Wysocki 1 year, 6 months ago 
On Mon, Jun 10, 2024 at 8:01 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> 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);
> }

So you essentially mean moving the for_each_trip_desc() loop from
thermal_zone_set_trips() to __thermal_zone_device_update() IIUC.

The caveat is that it is not necessary to run this loop at all if
tz->ops.set_trips is NULL.

I was thinking about folding the entire thermal_zone_set_trips() into
the caller, but that would be a different patch.

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

I do.

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

Thank you!

>
> > +     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 */
> >
> >
> >
>
> --
> <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
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.