[PATCH v2 0/5] thermal: core: Assorted improvements for v6.11

[PATCH v2 0/5] thermal: core: Assorted improvements for v6.11

[Rafael J. Wysocki]

Hi Everyone,

This is an update of

https://lore.kernel.org/linux-pm/13518388.uLZWGnKmhe@kreacher/

which mostly is a rebase on top of the recent fixes:

https://lore.kernel.org/linux-pm/12438941.O9o76ZdvQC@kreacher/

and the first patch from v1 is gone it turned out to be controversial (I will
be sending a new version of it separately).

The patches in the series address some shortcomings in the thermal
core code (including the Bang-Bang governor) and clean it up somewhat.

Please refer to the individual patch changelogs for details.

This series is independent of the thermal debugfs one posted earlier today:

https://lore.kernel.org/linux-pm/5794974.DvuYhMxLoT@kreacher/

At one point I'm going to put this series on a git branch for easier
access/testing.

Thanks!

[PATCH v2 1/5] thermal: trip: Rename __thermal_zone_set_trips() to thermal_zone_set_trips()

[Rafael J. Wysocki]

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

Drop the pointless double underline prefix from the function name as per
the subject.

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

v1 -> v2: Rebase.

---
 drivers/thermal/thermal_core.c |    2 +-
 drivers/thermal/thermal_core.h |    2 +-
 drivers/thermal/thermal_trip.c |    2 +-
 3 files changed, 3 insertions(+), 3 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,7 +513,7 @@ void __thermal_zone_device_update(struct
 	if (tz->temperature == THERMAL_TEMP_INVALID)
 		return;
 
-	__thermal_zone_set_trips(tz);
+	thermal_zone_set_trips(tz);
 
 	tz->notify_event = event;
 
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -242,7 +242,7 @@ void thermal_governor_update_tz(struct t
 
 const char *thermal_trip_type_name(enum thermal_trip_type trip_type);
 
-void __thermal_zone_set_trips(struct thermal_zone_device *tz);
+void thermal_zone_set_trips(struct thermal_zone_device *tz);
 int thermal_zone_trip_id(const struct thermal_zone_device *tz,
 			 const struct thermal_trip *trip);
 void thermal_zone_trip_updated(struct thermal_zone_device *tz,
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -76,7 +76,7 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_num_t
  *
  * It does not return a value
  */
-void __thermal_zone_set_trips(struct thermal_zone_device *tz)
+void thermal_zone_set_trips(struct thermal_zone_device *tz)
 {
 	const struct thermal_trip_desc *td;
 	int low = -INT_MAX, high = INT_MAX;

Re: [PATCH v2 1/5] thermal: trip: Rename __thermal_zone_set_trips() to thermal_zone_set_trips()

[Daniel Lezcano]

On 28/05/2024 18:50, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Drop the pointless double underline prefix from the function name as per
> the subject.
> 
> 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

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

[Rafael J. Wysocki]

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

[Daniel Lezcano]

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

[Rafael J. Wysocki]

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
>

[PATCH v2 3/5] thermal: trip: Use READ_ONCE() for lockless access to trip properties

[Rafael J. Wysocki]

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

When accessing trip temperature and hysteresis without locking, it is
better to use READ_ONCE() to prevent compiler optimizations possibly
affecting the read from being applied.

Of course, for the READ_ONCE() to be effective, WRITE_ONCE() needs to
be used when updating their values.

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

v1 -> v2: Rebase.

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

Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -139,7 +139,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].trip.temperature);
+	return sprintf(buf, "%d\n", READ_ONCE(tz->trips[trip_id].trip.temperature));
 }
 
 static ssize_t
@@ -163,7 +163,7 @@ trip_point_hyst_store(struct device *dev
 	trip = &tz->trips[trip_id].trip;
 
 	if (hyst != trip->hysteresis) {
-		trip->hysteresis = hyst;
+		WRITE_ONCE(trip->hysteresis, hyst);
 
 		thermal_zone_trip_updated(tz, trip);
 	}
@@ -183,7 +183,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].trip.hysteresis);
+	return sprintf(buf, "%d\n", READ_ONCE(tz->trips[trip_id].trip.hysteresis));
 }
 
 static ssize_t
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -161,7 +161,7 @@ void thermal_zone_set_trip_temp(struct t
 	if (trip->temperature == temp)
 		return;
 
-	trip->temperature = temp;
+	WRITE_ONCE(trip->temperature, temp);
 	thermal_notify_tz_trip_change(tz, trip);
 
 	if (temp == THERMAL_TEMP_INVALID) {

[PATCH v2 4/5] thermal: gov_bang_bang: Drop unnecessary cooling device target state checks

[Rafael J. Wysocki]

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

Some cooling device target state checks in bang_bang_control()
done before setting the new target state are not necessary after
recent changes, so drop them.

Also avoid updating the target state before checking it for
unexpected values.

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

v1 -> v2: Rebase.

---
 drivers/thermal/gov_bang_bang.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

Index: linux-pm/drivers/thermal/gov_bang_bang.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_bang_bang.c
+++ linux-pm/drivers/thermal/gov_bang_bang.c
@@ -57,24 +57,16 @@ static void bang_bang_control(struct the
 		if (instance->trip != trip)
 			continue;
 
-		if (instance->target == THERMAL_NO_TARGET)
-			instance->target = 0;
-
-		if (instance->target != 0 && instance->target != 1) {
+		if (instance->target != 0 && instance->target != 1 &&
+		    instance->target != THERMAL_NO_TARGET)
 			pr_debug("Unexpected state %ld of thermal instance %s in bang-bang\n",
 				 instance->target, instance->name);
 
-			instance->target = 1;
-		}
-
 		/*
 		 * Enable the fan when the trip is crossed on the way up and
 		 * disable it when the trip is crossed on the way down.
 		 */
-		if (instance->target == 0 && crossed_up)
-			instance->target = 1;
-		else if (instance->target == 1 && !crossed_up)
-			instance->target = 0;
+		instance->target = crossed_up;
 
 		dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target);
 

[PATCH v2 5/5] thermal: core: Avoid calling .trip_crossed() for critical and hot trips

[Rafael J. Wysocki]

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

Invoking the governor .trip_crossed() callback for critical and hot
trips is pointless because they are handled directly by the core,
so make thermal_governor_trip_crossed() avoid doing that.

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

v1 -> v2: Rebase.

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

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -463,6 +463,9 @@ static void thermal_governor_trip_crosse
 					  const struct thermal_trip *trip,
 					  bool crossed_up)
 {
+	if (trip->type == THERMAL_TRIP_HOT || trip->type == THERMAL_TRIP_CRITICAL)
+		return;
+
 	if (governor->trip_crossed)
 		governor->trip_crossed(tz, trip, crossed_up);
 }