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!
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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);
}
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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 */
From: Rafael J. Wysocki <[email protected]>
Drop the pointless double underline prefix from the function name as per
the subject.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
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;
On 28/05/2024 18:50, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Drop the pointless double underline prefix from the function name as per
> the subject.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
Acked-by: Daniel Lezcano <[email protected]>
--
<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
On 28/05/2024 18:51, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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 <[email protected]>
> + 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
On Mon, Jun 10, 2024 at 8:01 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 28/05/2024 18:51, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> >
> > 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 <[email protected]>
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
>