* [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling
@ 2024-10-16 11:18 Rafael J. Wysocki
2024-10-16 11:21 ` [PATCH v1 01/10] thermal: core: Build sorted lists instead of sorting them later Rafael J. Wysocki
` (10 more replies)
0 siblings, 11 replies; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-16 11:18 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
Hi Everyone,
This is a continuation of
https://lore.kernel.org/linux-pm/4985597.31r3eYUQgx@rjwysocki.net/
derived from patches [3-7/8] in the following patch series:
https://lore.kernel.org/linux-pm/4920970.GXAFRqVoOG@rjwysocki.net/
but mostly rewritten.
It is based on the observation that putting trip points on sorted lists
allows to reduce overhead related to the handling of them in some cases.
Namely, it avoids the need to walk all trips in a thermal zone every
time the zone temperature is updated (including invalid ones) and
generally leads to cleaner code.
Patches [01-08/10] are preliminary, patch [09/10] makes the key changes,
and patch [10/10] is a super-cosmetic cleanup on top of the rest.
Please refer to the individual patch changelogs for details.
Thanks!
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 01/10] thermal: core: Build sorted lists instead of sorting them later
2024-10-16 11:18 [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
@ 2024-10-16 11:21 ` Rafael J. Wysocki
2024-10-24 10:14 ` Lukasz Luba
2024-10-16 11:23 ` [PATCH v1 02/10] thermal: core: Rename trip list node in struct thermal_trip_desc Rafael J. Wysocki
` (9 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-16 11:21 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Since it is not expected that multiple trip points will be crossed
in one go very often (if this happens, there are too many trip points
in the given thermal zone or they are checked too rarely), quite likely
it is more efficient to build a sorted list of crossed trip points than
to put them on an unsorted list and sort it later.
Moreover, trip points are often sorted in ascending temperature order
during thermal zone registration. so building a sorted list out of
them is quite straightforward and relatively inexpensive.
Accordingly, make handle_thermal_trip() maintain list ordering when
adding trip points to the lists and get rid of separate list sorting
in __thermal_zone_device_update().
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/thermal/thermal_core.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -15,7 +15,6 @@
#include <linux/slab.h>
#include <linux/kdev_t.h>
#include <linux/idr.h>
-#include <linux/list_sort.h>
#include <linux/thermal.h>
#include <linux/reboot.h>
#include <linux/string.h>
@@ -409,6 +408,21 @@ static void handle_critical_trips(struct
tz->ops.hot(tz);
}
+static void add_trip_to_sorted_list(struct thermal_trip_desc *td,
+ struct list_head *list)
+{
+ struct thermal_trip_desc *entry;
+
+ /* Assume that the new entry is likely to be the last one. */
+ list_for_each_entry_reverse(entry, list, notify_list_node) {
+ if (entry->notify_temp <= td->notify_temp) {
+ list_add(&td->notify_list_node, &entry->notify_list_node);
+ return;
+ }
+ }
+ list_add(&td->notify_list_node, list);
+}
+
static void handle_thermal_trip(struct thermal_zone_device *tz,
struct thermal_trip_desc *td,
struct list_head *way_up_list,
@@ -438,8 +452,8 @@ static void handle_thermal_trip(struct t
* In that case, the trip temperature becomes the new threshold.
*/
if (tz->temperature < trip->temperature - trip->hysteresis) {
- list_add(&td->notify_list_node, way_down_list);
td->notify_temp = trip->temperature - trip->hysteresis;
+ add_trip_to_sorted_list(td, way_down_list);
if (trip->type == THERMAL_TRIP_PASSIVE) {
tz->passive--;
@@ -454,8 +468,9 @@ static void handle_thermal_trip(struct t
* if the zone temperature exceeds the trip one. The new
* threshold is then set to the low temperature of the trip.
*/
- list_add_tail(&td->notify_list_node, way_up_list);
td->notify_temp = trip->temperature;
+ add_trip_to_sorted_list(td, way_up_list);
+
td->threshold -= trip->hysteresis;
if (trip->type == THERMAL_TRIP_PASSIVE)
@@ -519,16 +534,6 @@ static void thermal_trip_crossed(struct
thermal_governor_trip_crossed(governor, tz, trip, crossed_up);
}
-static int thermal_trip_notify_cmp(void *not_used, const struct list_head *a,
- const struct list_head *b)
-{
- struct thermal_trip_desc *tda = container_of(a, struct thermal_trip_desc,
- notify_list_node);
- struct thermal_trip_desc *tdb = container_of(b, struct thermal_trip_desc,
- notify_list_node);
- return tda->notify_temp - tdb->notify_temp;
-}
-
void __thermal_zone_device_update(struct thermal_zone_device *tz,
enum thermal_notify_event event)
{
@@ -581,11 +586,9 @@ void __thermal_zone_device_update(struct
thermal_zone_set_trips(tz, low, high);
- list_sort(NULL, &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);
- list_sort(NULL, &way_down_list, thermal_trip_notify_cmp);
list_for_each_entry_reverse(td, &way_down_list, notify_list_node)
thermal_trip_crossed(tz, &td->trip, governor, false);
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 02/10] thermal: core: Rename trip list node in struct thermal_trip_desc
2024-10-16 11:18 [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
2024-10-16 11:21 ` [PATCH v1 01/10] thermal: core: Build sorted lists instead of sorting them later Rafael J. Wysocki
@ 2024-10-16 11:23 ` Rafael J. Wysocki
2024-10-24 10:16 ` Lukasz Luba
2024-10-16 11:24 ` [PATCH v1 03/10] thermal: core: Prepare for moving trips between sorted lists Rafael J. Wysocki
` (8 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-16 11:23 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Since the list node field in struct thermal_trip_desc is going to be
used for purposes other than trip crossing notification, rename it
to list_node.
No functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/thermal/thermal_core.c | 10 +++++-----
drivers/thermal/thermal_core.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -414,13 +414,13 @@ static void add_trip_to_sorted_list(stru
struct thermal_trip_desc *entry;
/* Assume that the new entry is likely to be the last one. */
- list_for_each_entry_reverse(entry, list, notify_list_node) {
+ list_for_each_entry_reverse(entry, list, list_node) {
if (entry->notify_temp <= td->notify_temp) {
- list_add(&td->notify_list_node, &entry->notify_list_node);
+ list_add(&td->list_node, &entry->list_node);
return;
}
}
- list_add(&td->notify_list_node, list);
+ list_add(&td->list_node, list);
}
static void handle_thermal_trip(struct thermal_zone_device *tz,
@@ -586,10 +586,10 @@ void __thermal_zone_device_update(struct
thermal_zone_set_trips(tz, low, high);
- list_for_each_entry(td, &way_up_list, notify_list_node)
+ list_for_each_entry(td, &way_up_list, list_node)
thermal_trip_crossed(tz, &td->trip, governor, true);
- list_for_each_entry_reverse(td, &way_down_list, notify_list_node)
+ list_for_each_entry_reverse(td, &way_down_list, list_node)
thermal_trip_crossed(tz, &td->trip, governor, false);
if (governor->manage)
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -31,7 +31,7 @@ struct thermal_trip_attrs {
struct thermal_trip_desc {
struct thermal_trip trip;
struct thermal_trip_attrs trip_attrs;
- struct list_head notify_list_node;
+ struct list_head list_node;
struct list_head thermal_instances;
int notify_temp;
int threshold;
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 03/10] thermal: core: Prepare for moving trips between sorted lists
2024-10-16 11:18 [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
2024-10-16 11:21 ` [PATCH v1 01/10] thermal: core: Build sorted lists instead of sorting them later Rafael J. Wysocki
2024-10-16 11:23 ` [PATCH v1 02/10] thermal: core: Rename trip list node in struct thermal_trip_desc Rafael J. Wysocki
@ 2024-10-16 11:24 ` Rafael J. Wysocki
2024-10-24 10:19 ` Lukasz Luba
2024-10-16 11:26 ` [PATCH v1 04/10] thermal: core: Rearrange __thermal_zone_device_update() Rafael J. Wysocki
` (7 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-16 11:24 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subsequently, trips will be moved between sorted lists in multiple
places, so replace add_trip_to_sorted_list() with an analogous
function, move_trip_to_sorted_list(), that will move a given trip
to a given sorted list.
To allow list_del() used in the new function to work, initialize the
list_node fields in trip descriptors where applicable so they are
always valid.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/thermal/thermal_core.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -408,11 +408,17 @@ static void handle_critical_trips(struct
tz->ops.hot(tz);
}
-static void add_trip_to_sorted_list(struct thermal_trip_desc *td,
- struct list_head *list)
+static void move_trip_to_sorted_list(struct thermal_trip_desc *td,
+ struct list_head *list)
{
struct thermal_trip_desc *entry;
+ /*
+ * Delete upfront and then add to make relocation within the same list
+ * work.
+ */
+ list_del(&td->list_node);
+
/* Assume that the new entry is likely to be the last one. */
list_for_each_entry_reverse(entry, list, list_node) {
if (entry->notify_temp <= td->notify_temp) {
@@ -453,7 +459,7 @@ static void handle_thermal_trip(struct t
*/
if (tz->temperature < trip->temperature - trip->hysteresis) {
td->notify_temp = trip->temperature - trip->hysteresis;
- add_trip_to_sorted_list(td, way_down_list);
+ move_trip_to_sorted_list(td, way_down_list);
if (trip->type == THERMAL_TRIP_PASSIVE) {
tz->passive--;
@@ -469,7 +475,7 @@ static void handle_thermal_trip(struct t
* threshold is then set to the low temperature of the trip.
*/
td->notify_temp = trip->temperature;
- add_trip_to_sorted_list(td, way_up_list);
+ move_trip_to_sorted_list(td, way_up_list);
td->threshold -= trip->hysteresis;
@@ -538,7 +544,7 @@ void __thermal_zone_device_update(struct
enum thermal_notify_event event)
{
struct thermal_governor *governor = thermal_get_tz_governor(tz);
- struct thermal_trip_desc *td;
+ struct thermal_trip_desc *td, *next;
LIST_HEAD(way_down_list);
LIST_HEAD(way_up_list);
int low = -INT_MAX, high = INT_MAX;
@@ -586,11 +592,15 @@ void __thermal_zone_device_update(struct
thermal_zone_set_trips(tz, low, high);
- list_for_each_entry(td, &way_up_list, list_node)
+ list_for_each_entry_safe(td, next, &way_up_list, list_node) {
thermal_trip_crossed(tz, &td->trip, governor, true);
+ list_del_init(&td->list_node);
+ }
- list_for_each_entry_reverse(td, &way_down_list, list_node)
+ list_for_each_entry_safe_reverse(td, next, &way_down_list, list_node) {
thermal_trip_crossed(tz, &td->trip, governor, false);
+ list_del_init(&td->list_node);
+ }
if (governor->manage)
governor->manage(tz);
@@ -1490,6 +1500,7 @@ thermal_zone_device_register_with_trips(
for_each_trip_desc(tz, td) {
td->trip = *trip++;
INIT_LIST_HEAD(&td->thermal_instances);
+ INIT_LIST_HEAD(&td->list_node);
/*
* Mark all thresholds as invalid to start with even though
* this only matters for the trips that start as invalid and
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 04/10] thermal: core: Rearrange __thermal_zone_device_update()
2024-10-16 11:18 [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
` (2 preceding siblings ...)
2024-10-16 11:24 ` [PATCH v1 03/10] thermal: core: Prepare for moving trips between sorted lists Rafael J. Wysocki
@ 2024-10-16 11:26 ` Rafael J. Wysocki
2024-10-24 10:20 ` Lukasz Luba
2024-10-16 11:27 ` [PATCH v1 05/10] thermal: core: Pass trip descriptor to thermal_trip_crossed() Rafael J. Wysocki
` (6 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-16 11:26 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
In preparation for subsequent changes, move the invocations of
thermal_thresholds_handle() and thermal_zone_set_trips() in
__thermal_zone_device_update() after the processing of the
temporary trip lists.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/thermal/thermal_core.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -588,10 +588,6 @@ void __thermal_zone_device_update(struct
high = td->threshold;
}
- thermal_thresholds_handle(tz, &low, &high);
-
- thermal_zone_set_trips(tz, low, high);
-
list_for_each_entry_safe(td, next, &way_up_list, list_node) {
thermal_trip_crossed(tz, &td->trip, governor, true);
list_del_init(&td->list_node);
@@ -602,6 +598,10 @@ void __thermal_zone_device_update(struct
list_del_init(&td->list_node);
}
+ thermal_thresholds_handle(tz, &low, &high);
+
+ thermal_zone_set_trips(tz, low, high);
+
if (governor->manage)
governor->manage(tz);
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 05/10] thermal: core: Pass trip descriptor to thermal_trip_crossed()
2024-10-16 11:18 [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
` (3 preceding siblings ...)
2024-10-16 11:26 ` [PATCH v1 04/10] thermal: core: Rearrange __thermal_zone_device_update() Rafael J. Wysocki
@ 2024-10-16 11:27 ` Rafael J. Wysocki
2024-10-24 10:22 ` Lukasz Luba
2024-10-16 11:29 ` [PATCH v1 06/10] thermal: core: Move some trip processing " Rafael J. Wysocki
` (5 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-16 11:27 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
In preparation for subsequent changes, modify thermal_trip_crossed()
to take a trip descriptor pointer instead of a pointer to struct
thermal_trip and propagate this change to thermal_zone_trip_down().
No functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/thermal/thermal_core.c | 12 +++++++-----
drivers/thermal/thermal_core.h | 2 +-
drivers/thermal/thermal_trip.c | 2 +-
3 files changed, 9 insertions(+), 7 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -526,10 +526,12 @@ static void thermal_governor_trip_crosse
}
static void thermal_trip_crossed(struct thermal_zone_device *tz,
- const struct thermal_trip *trip,
+ struct thermal_trip_desc *td,
struct thermal_governor *governor,
bool crossed_up)
{
+ const struct thermal_trip *trip = &td->trip;
+
if (crossed_up) {
thermal_notify_tz_trip_up(tz, trip);
thermal_debug_tz_trip_up(tz, trip);
@@ -589,12 +591,12 @@ void __thermal_zone_device_update(struct
}
list_for_each_entry_safe(td, next, &way_up_list, list_node) {
- thermal_trip_crossed(tz, &td->trip, governor, true);
+ thermal_trip_crossed(tz, td, governor, true);
list_del_init(&td->list_node);
}
list_for_each_entry_safe_reverse(td, next, &way_down_list, list_node) {
- thermal_trip_crossed(tz, &td->trip, governor, false);
+ thermal_trip_crossed(tz, td, governor, false);
list_del_init(&td->list_node);
}
@@ -664,9 +666,9 @@ void thermal_zone_device_update(struct t
EXPORT_SYMBOL_GPL(thermal_zone_device_update);
void thermal_zone_trip_down(struct thermal_zone_device *tz,
- const struct thermal_trip *trip)
+ struct thermal_trip_desc *td)
{
- thermal_trip_crossed(tz, trip, thermal_get_tz_governor(tz), false);
+ thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false);
}
int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -274,7 +274,7 @@ int thermal_zone_trip_id(const struct th
const struct thermal_trip *trip);
int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
void thermal_zone_trip_down(struct thermal_zone_device *tz,
- const struct thermal_trip *trip);
+ struct thermal_trip_desc *td);
void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz,
struct thermal_trip *trip, int hyst);
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -118,7 +118,7 @@ void thermal_zone_set_trip_temp(struct t
tz->passive--;
WARN_ON_ONCE(tz->passive < 0);
}
- thermal_zone_trip_down(tz, trip);
+ thermal_zone_trip_down(tz, td);
}
/*
* Invalidate the threshold to avoid triggering a spurious
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 06/10] thermal: core: Move some trip processing to thermal_trip_crossed()
2024-10-16 11:18 [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
` (4 preceding siblings ...)
2024-10-16 11:27 ` [PATCH v1 05/10] thermal: core: Pass trip descriptor to thermal_trip_crossed() Rafael J. Wysocki
@ 2024-10-16 11:29 ` Rafael J. Wysocki
2024-10-24 10:24 ` Lukasz Luba
2024-10-16 11:32 ` [PATCH v1 07/10] thermal: core: Relocate functions that update trip points Rafael J. Wysocki
` (4 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-16 11:29 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Notice that some processing related to trip point crossing carried out
in handle_thermal_trip() and thermal_zone_set_trip_temp() may as well
be done in thermal_trip_crossed(), which allows code duplication to be
reduced, so change the code accordingly.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/thermal/thermal_core.c | 21 ++++++++++-----------
drivers/thermal/thermal_trip.c | 17 ++++++-----------
2 files changed, 16 insertions(+), 22 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -460,11 +460,6 @@ static void handle_thermal_trip(struct t
if (tz->temperature < trip->temperature - trip->hysteresis) {
td->notify_temp = trip->temperature - trip->hysteresis;
move_trip_to_sorted_list(td, way_down_list);
-
- if (trip->type == THERMAL_TRIP_PASSIVE) {
- tz->passive--;
- WARN_ON(tz->passive < 0);
- }
} else {
td->threshold -= trip->hysteresis;
}
@@ -478,12 +473,6 @@ static void handle_thermal_trip(struct t
move_trip_to_sorted_list(td, way_up_list);
td->threshold -= trip->hysteresis;
-
- if (trip->type == THERMAL_TRIP_PASSIVE)
- tz->passive++;
- else if (trip->type == THERMAL_TRIP_CRITICAL ||
- trip->type == THERMAL_TRIP_HOT)
- handle_critical_trips(tz, trip);
}
}
@@ -533,9 +522,19 @@ static void thermal_trip_crossed(struct
const struct thermal_trip *trip = &td->trip;
if (crossed_up) {
+ if (trip->type == THERMAL_TRIP_PASSIVE)
+ tz->passive++;
+ else if (trip->type == THERMAL_TRIP_CRITICAL ||
+ trip->type == THERMAL_TRIP_HOT)
+ handle_critical_trips(tz, trip);
+
thermal_notify_tz_trip_up(tz, trip);
thermal_debug_tz_trip_up(tz, trip);
} else {
+ if (trip->type == THERMAL_TRIP_PASSIVE) {
+ tz->passive--;
+ WARN_ON(tz->passive < 0);
+ }
thermal_notify_tz_trip_down(tz, trip);
thermal_debug_tz_trip_down(tz, trip);
}
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -108,18 +108,13 @@ void thermal_zone_set_trip_temp(struct t
if (temp == THERMAL_TEMP_INVALID) {
struct thermal_trip_desc *td = trip_to_trip_desc(trip);
- if (tz->temperature >= td->threshold) {
- /*
- * The trip has been crossed on the way up, so some
- * adjustments are needed to compensate for the lack
- * of it going forward.
- */
- if (trip->type == THERMAL_TRIP_PASSIVE) {
- tz->passive--;
- WARN_ON_ONCE(tz->passive < 0);
- }
+ /*
+ * If the trip has been crossed on the way up, some adjustments
+ * are needed to compensate for the lack of it going forward.
+ */
+ if (tz->temperature >= td->threshold)
thermal_zone_trip_down(tz, td);
- }
+
/*
* Invalidate the threshold to avoid triggering a spurious
* trip crossing notification when the trip becomes valid.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 07/10] thermal: core: Relocate functions that update trip points
2024-10-16 11:18 [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
` (5 preceding siblings ...)
2024-10-16 11:29 ` [PATCH v1 06/10] thermal: core: Move some trip processing " Rafael J. Wysocki
@ 2024-10-16 11:32 ` Rafael J. Wysocki
2024-10-24 10:25 ` Lukasz Luba
2024-10-16 11:33 ` [PATCH v1 08/10] thermal: core: Eliminate thermal_zone_trip_down() Rafael J. Wysocki
` (3 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-16 11:32 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
In preparation for subsequent changes, move two functions used
for updating trip points, thermal_zone_set_trip_temp() and
thermal_zone_set_trip_hyst(), to thermal_core.c.
No functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/thermal/thermal_core.c | 35 +++++++++++++++++++++++++++++++++++
drivers/thermal/thermal_trip.c | 35 -----------------------------------
2 files changed, 35 insertions(+), 35 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -541,6 +541,41 @@ static void thermal_trip_crossed(struct
thermal_governor_trip_crossed(governor, tz, trip, crossed_up);
}
+void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz,
+ struct thermal_trip *trip, int hyst)
+{
+ WRITE_ONCE(trip->hysteresis, hyst);
+ thermal_notify_tz_trip_change(tz, trip);
+}
+
+void thermal_zone_set_trip_temp(struct thermal_zone_device *tz,
+ struct thermal_trip *trip, int temp)
+{
+ if (trip->temperature == temp)
+ return;
+
+ WRITE_ONCE(trip->temperature, temp);
+ thermal_notify_tz_trip_change(tz, trip);
+
+ if (temp == THERMAL_TEMP_INVALID) {
+ struct thermal_trip_desc *td = trip_to_trip_desc(trip);
+
+ /*
+ * If the trip has been crossed on the way up, some adjustments
+ * are needed to compensate for the lack of it going forward.
+ */
+ if (tz->temperature >= td->threshold)
+ thermal_zone_trip_down(tz, td);
+
+ /*
+ * Invalidate the threshold to avoid triggering a spurious
+ * trip crossing notification when the trip becomes valid.
+ */
+ td->threshold = INT_MAX;
+ }
+}
+EXPORT_SYMBOL_GPL(thermal_zone_set_trip_temp);
+
void __thermal_zone_device_update(struct thermal_zone_device *tz,
enum thermal_notify_event event)
{
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -88,38 +88,3 @@ int thermal_zone_trip_id(const struct th
*/
return trip_to_trip_desc(trip) - tz->trips;
}
-
-void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz,
- struct thermal_trip *trip, int hyst)
-{
- WRITE_ONCE(trip->hysteresis, hyst);
- thermal_notify_tz_trip_change(tz, trip);
-}
-
-void thermal_zone_set_trip_temp(struct thermal_zone_device *tz,
- struct thermal_trip *trip, int temp)
-{
- if (trip->temperature == temp)
- return;
-
- WRITE_ONCE(trip->temperature, temp);
- thermal_notify_tz_trip_change(tz, trip);
-
- if (temp == THERMAL_TEMP_INVALID) {
- struct thermal_trip_desc *td = trip_to_trip_desc(trip);
-
- /*
- * If the trip has been crossed on the way up, some adjustments
- * are needed to compensate for the lack of it going forward.
- */
- if (tz->temperature >= td->threshold)
- thermal_zone_trip_down(tz, td);
-
- /*
- * Invalidate the threshold to avoid triggering a spurious
- * trip crossing notification when the trip becomes valid.
- */
- td->threshold = INT_MAX;
- }
-}
-EXPORT_SYMBOL_GPL(thermal_zone_set_trip_temp);
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 08/10] thermal: core: Eliminate thermal_zone_trip_down()
2024-10-16 11:18 [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
` (6 preceding siblings ...)
2024-10-16 11:32 ` [PATCH v1 07/10] thermal: core: Relocate functions that update trip points Rafael J. Wysocki
@ 2024-10-16 11:33 ` Rafael J. Wysocki
2024-10-24 10:33 ` Lukasz Luba
2024-10-16 11:35 ` [PATCH v1 09/10] thermal: core: Use trip lists for trip crossing detection Rafael J. Wysocki
` (2 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-16 11:33 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Since thermal_zone_set_trip_temp() is not located in the same file
as thermal_trip_crossed(), it can invoke the latter directly without
using the thermal_zone_trip_down() wrapper that has no other users.
Update thermal_zone_set_trip_temp() accordingly and drop
thermal_zone_trip_down().
No functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/thermal/thermal_core.c | 8 +-------
drivers/thermal/thermal_core.h | 2 --
2 files changed, 1 insertion(+), 9 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -565,7 +565,7 @@ void thermal_zone_set_trip_temp(struct t
* are needed to compensate for the lack of it going forward.
*/
if (tz->temperature >= td->threshold)
- thermal_zone_trip_down(tz, td);
+ thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false);
/*
* Invalidate the threshold to avoid triggering a spurious
@@ -699,12 +699,6 @@ void thermal_zone_device_update(struct t
}
EXPORT_SYMBOL_GPL(thermal_zone_device_update);
-void thermal_zone_trip_down(struct thermal_zone_device *tz,
- struct thermal_trip_desc *td)
-{
- thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false);
-}
-
int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
void *data)
{
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -273,8 +273,6 @@ void thermal_zone_set_trips(struct therm
int thermal_zone_trip_id(const struct thermal_zone_device *tz,
const struct thermal_trip *trip);
int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
-void thermal_zone_trip_down(struct thermal_zone_device *tz,
- struct thermal_trip_desc *td);
void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz,
struct thermal_trip *trip, int hyst);
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 09/10] thermal: core: Use trip lists for trip crossing detection
2024-10-16 11:18 [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
` (7 preceding siblings ...)
2024-10-16 11:33 ` [PATCH v1 08/10] thermal: core: Eliminate thermal_zone_trip_down() Rafael J. Wysocki
@ 2024-10-16 11:35 ` Rafael J. Wysocki
2024-10-24 11:48 ` Lukasz Luba
2024-10-16 11:36 ` [PATCH v1 10/10] thermal: core: Relocate thermal zone initialization routine Rafael J. Wysocki
2024-10-21 11:16 ` [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
10 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-16 11:35 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Modify the thermal core to use three lists of trip points:
trips_high, containing trips with thresholds strictly above the current
thermal zone temperature,
trips_reached, containing trips with thresholds at or below the current
zone temperature,
trips_invalid, containing trips with temperature equal to
THERMAL_ZONE_INVALID,
where the first two lists are always sorted by the current trip
threshold.
For each trip in trips_high, there is no mitigation under way and
the trip threshold is equal to its temperature. In turn, for each
trip in trips_reached, there is mitigation under way and the trip
threshold is equal to its low temperature. The trips in trips_invalid,
of course, need not be taken into consideration.
The idea is to make __thermal_zone_device_update() walk trips_high and
trips_reached instead of walking the entire table of trip points in a
thermal zone. Usually, it will only need to walk a few entries in one
of the lists and check one entry in the other list, depending on the
direction of the zone temperature changes, because crossing many trips
by the zone temperature in one go between two consecutive temperature
checks should be unlikely (if it occurs often, the thermal zone
temperature should probably be checked more often either or there
are too many trips).
This also helps to eliminate one temporary trip list used for trip
crossing notification (only one temporary list is needed for this
purpose instead of two) and the remaining temporary list may be sorted
by the current trip threshold value, like the trips_reached list, so
the additional notify_temp field in struct thermal_trip_desc is not
necessary any more.
Moreover, since the trips_reached and trips_high lists are sorted,
the "low" and "high" values needed by thermal_zone_set_trips() can be
determined in a straightforward way by looking at one end of each list.
Of course, additional work is needed in some places in order to
maintain the ordering of the lists, but it is limited to situations
that should be rare, like updating a trip point temperature or
hysteresis, thermal zone initialization, or system resume.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/thermal/thermal_core.c | 211 +++++++++++++++++++++++++----------------
drivers/thermal/thermal_core.h | 7 +
2 files changed, 136 insertions(+), 82 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -33,7 +33,6 @@ struct thermal_trip_desc {
struct thermal_trip_attrs trip_attrs;
struct list_head list_node;
struct list_head thermal_instances;
- int notify_temp;
int threshold;
};
@@ -78,6 +77,9 @@ struct thermal_governor {
* @device: &struct device for this thermal zone
* @removal: removal completion
* @resume: resume completion
+ * @trips_high: trips above the current zone temperature
+ * @trips_reached: trips below or at the current zone temperature
+ * @trips_invalid: trips with invalid temperature
* @mode: current mode of this thermal zone
* @devdata: private pointer for device private data
* @num_trips: number of trip points the thermal zone supports
@@ -118,6 +120,9 @@ struct thermal_zone_device {
struct completion removal;
struct completion resume;
struct attribute_group trips_attribute_group;
+ struct list_head trips_high;
+ struct list_head trips_reached;
+ struct list_head trips_invalid;
enum thermal_device_mode mode;
void *devdata;
int num_trips;
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -421,7 +421,7 @@ static void move_trip_to_sorted_list(str
/* Assume that the new entry is likely to be the last one. */
list_for_each_entry_reverse(entry, list, list_node) {
- if (entry->notify_temp <= td->notify_temp) {
+ if (entry->threshold <= td->threshold) {
list_add(&td->list_node, &entry->list_node);
return;
}
@@ -429,53 +429,6 @@ static void move_trip_to_sorted_list(str
list_add(&td->list_node, list);
}
-static void handle_thermal_trip(struct thermal_zone_device *tz,
- struct thermal_trip_desc *td,
- struct list_head *way_up_list,
- struct list_head *way_down_list)
-{
- const struct thermal_trip *trip = &td->trip;
- int old_threshold;
-
- if (trip->temperature == THERMAL_TEMP_INVALID)
- return;
-
- /*
- * If the trip temperature or hysteresis has been updated recently,
- * the threshold needs to be computed again using the new values.
- * However, its initial value still reflects the old ones and that
- * is what needs to be compared with the previous zone temperature
- * to decide which action to take.
- */
- old_threshold = td->threshold;
- td->threshold = trip->temperature;
-
- if (tz->last_temperature >= old_threshold &&
- tz->last_temperature != THERMAL_TEMP_INIT) {
- /*
- * Mitigation is under way, so it needs to stop if the zone
- * temperature falls below the low temperature of the trip.
- * In that case, the trip temperature becomes the new threshold.
- */
- if (tz->temperature < trip->temperature - trip->hysteresis) {
- td->notify_temp = trip->temperature - trip->hysteresis;
- move_trip_to_sorted_list(td, way_down_list);
- } else {
- td->threshold -= trip->hysteresis;
- }
- } else if (tz->temperature >= trip->temperature) {
- /*
- * There is no mitigation under way, so it needs to be started
- * if the zone temperature exceeds the trip one. The new
- * threshold is then set to the low temperature of the trip.
- */
- td->notify_temp = trip->temperature;
- move_trip_to_sorted_list(td, way_up_list);
-
- td->threshold -= trip->hysteresis;
- }
-}
-
static void thermal_zone_device_check(struct work_struct *work)
{
struct thermal_zone_device *tz = container_of(work, struct
@@ -484,9 +437,30 @@ static void thermal_zone_device_check(st
thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
}
+static void move_to_trips_high(struct thermal_zone_device *tz,
+ struct thermal_trip_desc *td)
+{
+ td->threshold = td->trip.temperature;
+ move_trip_to_sorted_list(td, &tz->trips_high);
+}
+
+static void move_to_trips_reached(struct thermal_zone_device *tz,
+ struct thermal_trip_desc *td)
+{
+ td->threshold = td->trip.temperature - td->trip.hysteresis;
+ move_trip_to_sorted_list(td, &tz->trips_reached);
+}
+
+static void move_to_trips_invalid(struct thermal_zone_device *tz,
+ struct thermal_trip_desc *td)
+{
+ td->threshold = INT_MAX;
+ list_move(&td->list_node, &tz->trips_invalid);
+}
+
static void thermal_zone_device_init(struct thermal_zone_device *tz)
{
- struct thermal_trip_desc *td;
+ struct thermal_trip_desc *td, *next;
INIT_DELAYED_WORK(&tz->poll_queue, thermal_zone_device_check);
@@ -500,6 +474,21 @@ static void thermal_zone_device_init(str
list_for_each_entry(instance, &td->thermal_instances, trip_node)
instance->initialized = false;
}
+ /*
+ * At this point, all valid trips need to be moved to trips_high so that
+ * mitigation can be started if the zone temperature is above them.
+ */
+ list_for_each_entry_safe(td, next, &tz->trips_invalid, list_node) {
+ if (td->trip.temperature != THERMAL_TEMP_INVALID)
+ move_to_trips_high(tz, td);
+ }
+ /* The trips_reached list may not be empty during system resume. */
+ list_for_each_entry_safe(td, next, &tz->trips_reached, list_node) {
+ if (td->trip.temperature == THERMAL_TEMP_INVALID)
+ move_to_trips_invalid(tz, td);
+ else
+ move_to_trips_high(tz, td);
+ }
}
static void thermal_governor_trip_crossed(struct thermal_governor *governor,
@@ -544,45 +533,120 @@ static void thermal_trip_crossed(struct
void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz,
struct thermal_trip *trip, int hyst)
{
+ struct thermal_trip_desc *td = trip_to_trip_desc(trip);
+
WRITE_ONCE(trip->hysteresis, hyst);
thermal_notify_tz_trip_change(tz, trip);
+ /*
+ * If the zone temperature is above or at the trip tmperature, the trip
+ * is in the trips_reached list and its threshold is equal to its low
+ * temperature. It needs to stay in that list, but its threshold needs
+ * to be updated and the list ordering may need to be restored.
+ */
+ if (tz->temperature >= td->threshold)
+ move_to_trips_reached(tz, td);
}
void thermal_zone_set_trip_temp(struct thermal_zone_device *tz,
struct thermal_trip *trip, int temp)
{
- if (trip->temperature == temp)
+ struct thermal_trip_desc *td = trip_to_trip_desc(trip);
+ int old_temp = trip->temperature;
+
+ if (old_temp == temp)
return;
WRITE_ONCE(trip->temperature, temp);
thermal_notify_tz_trip_change(tz, trip);
- if (temp == THERMAL_TEMP_INVALID) {
- struct thermal_trip_desc *td = trip_to_trip_desc(trip);
+ if (old_temp == THERMAL_TEMP_INVALID) {
+ /*
+ * The trip was invalid before the change, so move it to the
+ * trips_high list regardless of the new temperature value
+ * because there is no mitigation under way for it. If a
+ * mitigation needs to be started, the trip will be moved to the
+ * trips_reached list later.
+ */
+ move_to_trips_high(tz, td);
+ return;
+ }
+ if (temp == THERMAL_TEMP_INVALID) {
/*
- * If the trip has been crossed on the way up, some adjustments
- * are needed to compensate for the lack of it going forward.
+ * If the trip is in the trips_reached list, mitigation is under
+ * way for it and it needs to be stopped because the trip is
+ * effectively going away.
*/
if (tz->temperature >= td->threshold)
thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false);
+ move_to_trips_invalid(tz, td);
+ return;
+ }
+
+ /*
+ * The trip stays on its current list, but its threshold needs to be
+ * updated due to the temperature change and the list ordering may need
+ * to be restored.
+ */
+ if (tz->temperature >= td->threshold)
+ move_to_trips_reached(tz, td);
+ else
+ move_to_trips_high(tz, td);
+}
+EXPORT_SYMBOL_GPL(thermal_zone_set_trip_temp);
+
+static void thermal_zone_handle_trips(struct thermal_zone_device *tz,
+ struct thermal_governor *governor,
+ int *low, int *high)
+{
+ struct thermal_trip_desc *td, *next;
+ LIST_HEAD(way_down_list);
+
+ /* Check the trips that were below or at the zone temperature. */
+ list_for_each_entry_safe_reverse(td, next, &tz->trips_reached, list_node) {
+ if (td->threshold <= tz->temperature)
+ break;
+
+ thermal_trip_crossed(tz, td, governor, false);
/*
- * Invalidate the threshold to avoid triggering a spurious
- * trip crossing notification when the trip becomes valid.
+ * The current trips_high list needs to be processed before
+ * adding new entries to it, so put them on a temporary list.
*/
- td->threshold = INT_MAX;
+ list_move(&td->list_node, &way_down_list);
+ }
+ /* Check the trips that were previously above the zone temperature. */
+ list_for_each_entry_safe(td, next, &tz->trips_high, list_node) {
+ if (td->threshold > tz->temperature)
+ break;
+
+ thermal_trip_crossed(tz, td, governor, true);
+ move_to_trips_reached(tz, td);
+ }
+ list_for_each_entry_safe(td, next, &way_down_list, list_node)
+ move_to_trips_high(tz, td);
+
+ if (!list_empty(&tz->trips_reached)) {
+ td = list_last_entry(&tz->trips_reached,
+ struct thermal_trip_desc, list_node);
+ /*
+ * Set the "low" value below the current trip threshold in case
+ * the zone temperature is at that threshold and stays there,
+ * which would trigger a new interrupt immediately in vain.
+ */
+ *low = td->threshold - 1;
+ }
+ if (!list_empty(&tz->trips_high)) {
+ td = list_first_entry(&tz->trips_high,
+ struct thermal_trip_desc, list_node);
+ *high = td->threshold;
}
}
-EXPORT_SYMBOL_GPL(thermal_zone_set_trip_temp);
void __thermal_zone_device_update(struct thermal_zone_device *tz,
enum thermal_notify_event event)
{
struct thermal_governor *governor = thermal_get_tz_governor(tz);
- struct thermal_trip_desc *td, *next;
- LIST_HEAD(way_down_list);
- LIST_HEAD(way_up_list);
int low = -INT_MAX, high = INT_MAX;
int temp, ret;
@@ -614,25 +678,7 @@ void __thermal_zone_device_update(struct
tz->notify_event = event;
- for_each_trip_desc(tz, td) {
- handle_thermal_trip(tz, td, &way_up_list, &way_down_list);
-
- if (td->threshold <= tz->temperature && td->threshold > low)
- low = td->threshold;
-
- if (td->threshold >= tz->temperature && td->threshold < high)
- high = td->threshold;
- }
-
- list_for_each_entry_safe(td, next, &way_up_list, list_node) {
- thermal_trip_crossed(tz, td, governor, true);
- list_del_init(&td->list_node);
- }
-
- list_for_each_entry_safe_reverse(td, next, &way_down_list, list_node) {
- thermal_trip_crossed(tz, td, governor, false);
- list_del_init(&td->list_node);
- }
+ thermal_zone_handle_trips(tz, governor, &low, &high);
thermal_thresholds_handle(tz, &low, &high);
@@ -1507,6 +1553,9 @@ thermal_zone_device_register_with_trips(
}
INIT_LIST_HEAD(&tz->node);
+ INIT_LIST_HEAD(&tz->trips_high);
+ INIT_LIST_HEAD(&tz->trips_reached);
+ INIT_LIST_HEAD(&tz->trips_invalid);
ida_init(&tz->ida);
mutex_init(&tz->lock);
init_completion(&tz->removal);
@@ -1536,7 +1585,7 @@ thermal_zone_device_register_with_trips(
* this only matters for the trips that start as invalid and
* become valid later.
*/
- td->threshold = INT_MAX;
+ move_to_trips_invalid(tz, td);
}
tz->polling_delay_jiffies = msecs_to_jiffies(polling_delay);
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 10/10] thermal: core: Relocate thermal zone initialization routine
2024-10-16 11:18 [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
` (8 preceding siblings ...)
2024-10-16 11:35 ` [PATCH v1 09/10] thermal: core: Use trip lists for trip crossing detection Rafael J. Wysocki
@ 2024-10-16 11:36 ` Rafael J. Wysocki
2024-10-24 10:35 ` Lukasz Luba
2024-10-21 11:16 ` [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
10 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-16 11:36 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Move thermal_zone_device_init() along with thermal_zone_device_check()
closer to the callers of the former, where they fit better together.
No functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/thermal/thermal_core.c | 82 ++++++++++++++++++++---------------------
1 file changed, 41 insertions(+), 41 deletions(-)
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -429,14 +429,6 @@ static void move_trip_to_sorted_list(str
list_add(&td->list_node, list);
}
-static void thermal_zone_device_check(struct work_struct *work)
-{
- struct thermal_zone_device *tz = container_of(work, struct
- thermal_zone_device,
- poll_queue.work);
- thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
-}
-
static void move_to_trips_high(struct thermal_zone_device *tz,
struct thermal_trip_desc *td)
{
@@ -458,39 +450,6 @@ static void move_to_trips_invalid(struct
list_move(&td->list_node, &tz->trips_invalid);
}
-static void thermal_zone_device_init(struct thermal_zone_device *tz)
-{
- struct thermal_trip_desc *td, *next;
-
- INIT_DELAYED_WORK(&tz->poll_queue, thermal_zone_device_check);
-
- tz->temperature = THERMAL_TEMP_INIT;
- tz->passive = 0;
- tz->prev_low_trip = -INT_MAX;
- tz->prev_high_trip = INT_MAX;
- for_each_trip_desc(tz, td) {
- struct thermal_instance *instance;
-
- list_for_each_entry(instance, &td->thermal_instances, trip_node)
- instance->initialized = false;
- }
- /*
- * At this point, all valid trips need to be moved to trips_high so that
- * mitigation can be started if the zone temperature is above them.
- */
- list_for_each_entry_safe(td, next, &tz->trips_invalid, list_node) {
- if (td->trip.temperature != THERMAL_TEMP_INVALID)
- move_to_trips_high(tz, td);
- }
- /* The trips_reached list may not be empty during system resume. */
- list_for_each_entry_safe(td, next, &tz->trips_reached, list_node) {
- if (td->trip.temperature == THERMAL_TEMP_INVALID)
- move_to_trips_invalid(tz, td);
- else
- move_to_trips_high(tz, td);
- }
-}
-
static void thermal_governor_trip_crossed(struct thermal_governor *governor,
struct thermal_zone_device *tz,
const struct thermal_trip *trip,
@@ -1425,6 +1384,47 @@ int thermal_zone_get_crit_temp(struct th
}
EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
+static void thermal_zone_device_check(struct work_struct *work)
+{
+ struct thermal_zone_device *tz = container_of(work, struct
+ thermal_zone_device,
+ poll_queue.work);
+ thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
+}
+
+static void thermal_zone_device_init(struct thermal_zone_device *tz)
+{
+ struct thermal_trip_desc *td, *next;
+
+ INIT_DELAYED_WORK(&tz->poll_queue, thermal_zone_device_check);
+
+ tz->temperature = THERMAL_TEMP_INIT;
+ tz->passive = 0;
+ tz->prev_low_trip = -INT_MAX;
+ tz->prev_high_trip = INT_MAX;
+ for_each_trip_desc(tz, td) {
+ struct thermal_instance *instance;
+
+ list_for_each_entry(instance, &td->thermal_instances, trip_node)
+ instance->initialized = false;
+ }
+ /*
+ * At this point, all valid trips need to be moved to trips_high so that
+ * mitigation can be started if the zone temperature is above them.
+ */
+ list_for_each_entry_safe(td, next, &tz->trips_invalid, list_node) {
+ if (td->trip.temperature != THERMAL_TEMP_INVALID)
+ move_to_trips_high(tz, td);
+ }
+ /* The trips_reached list may not be empty during system resume. */
+ list_for_each_entry_safe(td, next, &tz->trips_reached, list_node) {
+ if (td->trip.temperature == THERMAL_TEMP_INVALID)
+ move_to_trips_invalid(tz, td);
+ else
+ move_to_trips_high(tz, td);
+ }
+}
+
static int thermal_zone_init_governor(struct thermal_zone_device *tz)
{
struct thermal_governor *governor;
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling
2024-10-16 11:18 [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
` (9 preceding siblings ...)
2024-10-16 11:36 ` [PATCH v1 10/10] thermal: core: Relocate thermal zone initialization routine Rafael J. Wysocki
@ 2024-10-21 11:16 ` Rafael J. Wysocki
2024-10-22 22:52 ` Lukasz Luba
10 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-21 11:16 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Daniel Lezcano, Lukasz Luba, Zhang Rui, Srinivas Pandruvada
On Wed, Oct 16, 2024 at 1:37 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi Everyone,
>
> This is a continuation of
>
> https://lore.kernel.org/linux-pm/4985597.31r3eYUQgx@rjwysocki.net/
>
> derived from patches [3-7/8] in the following patch series:
>
> https://lore.kernel.org/linux-pm/4920970.GXAFRqVoOG@rjwysocki.net/
>
> but mostly rewritten.
>
> It is based on the observation that putting trip points on sorted lists
> allows to reduce overhead related to the handling of them in some cases.
> Namely, it avoids the need to walk all trips in a thermal zone every
> time the zone temperature is updated (including invalid ones) and
> generally leads to cleaner code.
>
> Patches [01-08/10] are preliminary, patch [09/10] makes the key changes,
> and patch [10/10] is a super-cosmetic cleanup on top of the rest.
>
> Please refer to the individual patch changelogs for details.
This material is on the thermal-core-experimental branch in
linux-pm.git along with
https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
and
https://lore.kernel.org/linux-pm/4985597.31r3eYUQgx@rjwysocki.net/
which are also present in the thermal-core-testing branch.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling
2024-10-21 11:16 ` [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
@ 2024-10-22 22:52 ` Lukasz Luba
2024-10-23 8:53 ` Rafael J. Wysocki
0 siblings, 1 reply; 27+ messages in thread
From: Lukasz Luba @ 2024-10-22 22:52 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
Hi Rafael,
On 10/21/24 12:16, Rafael J. Wysocki wrote:
> On Wed, Oct 16, 2024 at 1:37 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> Hi Everyone,
>>
>> This is a continuation of
>>
>> https://lore.kernel.org/linux-pm/4985597.31r3eYUQgx@rjwysocki.net/
>>
>> derived from patches [3-7/8] in the following patch series:
>>
>> https://lore.kernel.org/linux-pm/4920970.GXAFRqVoOG@rjwysocki.net/
>>
>> but mostly rewritten.
>>
>> It is based on the observation that putting trip points on sorted lists
>> allows to reduce overhead related to the handling of them in some cases.
>> Namely, it avoids the need to walk all trips in a thermal zone every
>> time the zone temperature is updated (including invalid ones) and
>> generally leads to cleaner code.
>>
>> Patches [01-08/10] are preliminary, patch [09/10] makes the key changes,
>> and patch [10/10] is a super-cosmetic cleanup on top of the rest.
>>
>> Please refer to the individual patch changelogs for details.
>
> This material is on the thermal-core-experimental branch in
> linux-pm.git along with
>
> https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
>
> and
>
> https://lore.kernel.org/linux-pm/4985597.31r3eYUQgx@rjwysocki.net/
>
> which are also present in the thermal-core-testing branch.
If it's not too late, I can review that stuff tomorrow for you.
Regards,
Lukasz
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling
2024-10-22 22:52 ` Lukasz Luba
@ 2024-10-23 8:53 ` Rafael J. Wysocki
0 siblings, 0 replies; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-23 8:53 UTC (permalink / raw)
To: Lukasz Luba
Cc: Rafael J. Wysocki, LKML, Linux PM, Daniel Lezcano, Zhang Rui,
Srinivas Pandruvada
Hi Lukasz,
On Wed, Oct 23, 2024 at 12:51 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Rafael,
>
> On 10/21/24 12:16, Rafael J. Wysocki wrote:
> > On Wed, Oct 16, 2024 at 1:37 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>
> >> Hi Everyone,
> >>
> >> This is a continuation of
> >>
> >> https://lore.kernel.org/linux-pm/4985597.31r3eYUQgx@rjwysocki.net/
> >>
> >> derived from patches [3-7/8] in the following patch series:
> >>
> >> https://lore.kernel.org/linux-pm/4920970.GXAFRqVoOG@rjwysocki.net/
> >>
> >> but mostly rewritten.
> >>
> >> It is based on the observation that putting trip points on sorted lists
> >> allows to reduce overhead related to the handling of them in some cases.
> >> Namely, it avoids the need to walk all trips in a thermal zone every
> >> time the zone temperature is updated (including invalid ones) and
> >> generally leads to cleaner code.
> >>
> >> Patches [01-08/10] are preliminary, patch [09/10] makes the key changes,
> >> and patch [10/10] is a super-cosmetic cleanup on top of the rest.
> >>
> >> Please refer to the individual patch changelogs for details.
> >
> > This material is on the thermal-core-experimental branch in
> > linux-pm.git along with
> >
> > https://lore.kernel.org/linux-pm/2215082.irdbgypaU6@rjwysocki.net/
> >
> > and
> >
> > https://lore.kernel.org/linux-pm/4985597.31r3eYUQgx@rjwysocki.net/
> >
> > which are also present in the thermal-core-testing branch.
>
>
> If it's not too late, I can review that stuff tomorrow for you.
No, it is not too late, please review it!
And thanks a lot for all of the reviews so far!
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 01/10] thermal: core: Build sorted lists instead of sorting them later
2024-10-16 11:21 ` [PATCH v1 01/10] thermal: core: Build sorted lists instead of sorting them later Rafael J. Wysocki
@ 2024-10-24 10:14 ` Lukasz Luba
0 siblings, 0 replies; 27+ messages in thread
From: Lukasz Luba @ 2024-10-24 10:14 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/16/24 12:21, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Since it is not expected that multiple trip points will be crossed
> in one go very often (if this happens, there are too many trip points
> in the given thermal zone or they are checked too rarely), quite likely
> it is more efficient to build a sorted list of crossed trip points than
> to put them on an unsorted list and sort it later.
>
> Moreover, trip points are often sorted in ascending temperature order
> during thermal zone registration. so building a sorted list out of
nit: small s
> them is quite straightforward and relatively inexpensive.
>
> Accordingly, make handle_thermal_trip() maintain list ordering when
> adding trip points to the lists and get rid of separate list sorting
> in __thermal_zone_device_update().
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/thermal_core.c | 33 ++++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -15,7 +15,6 @@
> #include <linux/slab.h>
> #include <linux/kdev_t.h>
> #include <linux/idr.h>
> -#include <linux/list_sort.h>
> #include <linux/thermal.h>
> #include <linux/reboot.h>
> #include <linux/string.h>
> @@ -409,6 +408,21 @@ static void handle_critical_trips(struct
> tz->ops.hot(tz);
> }
>
> +static void add_trip_to_sorted_list(struct thermal_trip_desc *td,
> + struct list_head *list)
> +{
> + struct thermal_trip_desc *entry;
> +
> + /* Assume that the new entry is likely to be the last one. */
> + list_for_each_entry_reverse(entry, list, notify_list_node) {
> + if (entry->notify_temp <= td->notify_temp) {
> + list_add(&td->notify_list_node, &entry->notify_list_node);
> + return;
> + }
> + }
> + list_add(&td->notify_list_node, list);
> +}
> +
> static void handle_thermal_trip(struct thermal_zone_device *tz,
> struct thermal_trip_desc *td,
> struct list_head *way_up_list,
> @@ -438,8 +452,8 @@ static void handle_thermal_trip(struct t
> * In that case, the trip temperature becomes the new threshold.
> */
> if (tz->temperature < trip->temperature - trip->hysteresis) {
> - list_add(&td->notify_list_node, way_down_list);
> td->notify_temp = trip->temperature - trip->hysteresis;
> + add_trip_to_sorted_list(td, way_down_list);
>
> if (trip->type == THERMAL_TRIP_PASSIVE) {
> tz->passive--;
> @@ -454,8 +468,9 @@ static void handle_thermal_trip(struct t
> * if the zone temperature exceeds the trip one. The new
> * threshold is then set to the low temperature of the trip.
> */
> - list_add_tail(&td->notify_list_node, way_up_list);
> td->notify_temp = trip->temperature;
> + add_trip_to_sorted_list(td, way_up_list);
> +
> td->threshold -= trip->hysteresis;
>
> if (trip->type == THERMAL_TRIP_PASSIVE)
> @@ -519,16 +534,6 @@ static void thermal_trip_crossed(struct
> thermal_governor_trip_crossed(governor, tz, trip, crossed_up);
> }
>
> -static int thermal_trip_notify_cmp(void *not_used, const struct list_head *a,
> - const struct list_head *b)
> -{
> - struct thermal_trip_desc *tda = container_of(a, struct thermal_trip_desc,
> - notify_list_node);
> - struct thermal_trip_desc *tdb = container_of(b, struct thermal_trip_desc,
> - notify_list_node);
> - return tda->notify_temp - tdb->notify_temp;
> -}
> -
> void __thermal_zone_device_update(struct thermal_zone_device *tz,
> enum thermal_notify_event event)
> {
> @@ -581,11 +586,9 @@ void __thermal_zone_device_update(struct
>
> thermal_zone_set_trips(tz, low, high);
>
> - list_sort(NULL, &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);
>
> - list_sort(NULL, &way_down_list, thermal_trip_notify_cmp);
> list_for_each_entry_reverse(td, &way_down_list, notify_list_node)
> thermal_trip_crossed(tz, &td->trip, governor, false);
>
>
>
>
Make sense, LGTM
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 02/10] thermal: core: Rename trip list node in struct thermal_trip_desc
2024-10-16 11:23 ` [PATCH v1 02/10] thermal: core: Rename trip list node in struct thermal_trip_desc Rafael J. Wysocki
@ 2024-10-24 10:16 ` Lukasz Luba
0 siblings, 0 replies; 27+ messages in thread
From: Lukasz Luba @ 2024-10-24 10:16 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/16/24 12:23, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Since the list node field in struct thermal_trip_desc is going to be
> used for purposes other than trip crossing notification, rename it
> to list_node.
>
> No functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/thermal_core.c | 10 +++++-----
> drivers/thermal/thermal_core.h | 2 +-
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -414,13 +414,13 @@ static void add_trip_to_sorted_list(stru
> struct thermal_trip_desc *entry;
>
> /* Assume that the new entry is likely to be the last one. */
> - list_for_each_entry_reverse(entry, list, notify_list_node) {
> + list_for_each_entry_reverse(entry, list, list_node) {
> if (entry->notify_temp <= td->notify_temp) {
> - list_add(&td->notify_list_node, &entry->notify_list_node);
> + list_add(&td->list_node, &entry->list_node);
> return;
> }
> }
> - list_add(&td->notify_list_node, list);
> + list_add(&td->list_node, list);
> }
>
> static void handle_thermal_trip(struct thermal_zone_device *tz,
> @@ -586,10 +586,10 @@ void __thermal_zone_device_update(struct
>
> thermal_zone_set_trips(tz, low, high);
>
> - list_for_each_entry(td, &way_up_list, notify_list_node)
> + list_for_each_entry(td, &way_up_list, list_node)
> thermal_trip_crossed(tz, &td->trip, governor, true);
>
> - list_for_each_entry_reverse(td, &way_down_list, notify_list_node)
> + list_for_each_entry_reverse(td, &way_down_list, list_node)
> thermal_trip_crossed(tz, &td->trip, governor, false);
>
> if (governor->manage)
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -31,7 +31,7 @@ struct thermal_trip_attrs {
> struct thermal_trip_desc {
> struct thermal_trip trip;
> struct thermal_trip_attrs trip_attrs;
> - struct list_head notify_list_node;
> + struct list_head list_node;
> struct list_head thermal_instances;
> int notify_temp;
> int threshold;
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 03/10] thermal: core: Prepare for moving trips between sorted lists
2024-10-16 11:24 ` [PATCH v1 03/10] thermal: core: Prepare for moving trips between sorted lists Rafael J. Wysocki
@ 2024-10-24 10:19 ` Lukasz Luba
0 siblings, 0 replies; 27+ messages in thread
From: Lukasz Luba @ 2024-10-24 10:19 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/16/24 12:24, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Subsequently, trips will be moved between sorted lists in multiple
> places, so replace add_trip_to_sorted_list() with an analogous
> function, move_trip_to_sorted_list(), that will move a given trip
> to a given sorted list.
>
> To allow list_del() used in the new function to work, initialize the
> list_node fields in trip descriptors where applicable so they are
> always valid.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/thermal_core.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -408,11 +408,17 @@ static void handle_critical_trips(struct
> tz->ops.hot(tz);
> }
>
> -static void add_trip_to_sorted_list(struct thermal_trip_desc *td,
> - struct list_head *list)
> +static void move_trip_to_sorted_list(struct thermal_trip_desc *td,
> + struct list_head *list)
> {
> struct thermal_trip_desc *entry;
>
> + /*
> + * Delete upfront and then add to make relocation within the same list
> + * work.
> + */
> + list_del(&td->list_node);
> +
> /* Assume that the new entry is likely to be the last one. */
> list_for_each_entry_reverse(entry, list, list_node) {
> if (entry->notify_temp <= td->notify_temp) {
> @@ -453,7 +459,7 @@ static void handle_thermal_trip(struct t
> */
> if (tz->temperature < trip->temperature - trip->hysteresis) {
> td->notify_temp = trip->temperature - trip->hysteresis;
> - add_trip_to_sorted_list(td, way_down_list);
> + move_trip_to_sorted_list(td, way_down_list);
>
> if (trip->type == THERMAL_TRIP_PASSIVE) {
> tz->passive--;
> @@ -469,7 +475,7 @@ static void handle_thermal_trip(struct t
> * threshold is then set to the low temperature of the trip.
> */
> td->notify_temp = trip->temperature;
> - add_trip_to_sorted_list(td, way_up_list);
> + move_trip_to_sorted_list(td, way_up_list);
>
> td->threshold -= trip->hysteresis;
>
> @@ -538,7 +544,7 @@ void __thermal_zone_device_update(struct
> enum thermal_notify_event event)
> {
> struct thermal_governor *governor = thermal_get_tz_governor(tz);
> - struct thermal_trip_desc *td;
> + struct thermal_trip_desc *td, *next;
> LIST_HEAD(way_down_list);
> LIST_HEAD(way_up_list);
> int low = -INT_MAX, high = INT_MAX;
> @@ -586,11 +592,15 @@ void __thermal_zone_device_update(struct
>
> thermal_zone_set_trips(tz, low, high);
>
> - list_for_each_entry(td, &way_up_list, list_node)
> + list_for_each_entry_safe(td, next, &way_up_list, list_node) {
> thermal_trip_crossed(tz, &td->trip, governor, true);
> + list_del_init(&td->list_node);
> + }
>
> - list_for_each_entry_reverse(td, &way_down_list, list_node)
> + list_for_each_entry_safe_reverse(td, next, &way_down_list, list_node) {
> thermal_trip_crossed(tz, &td->trip, governor, false);
> + list_del_init(&td->list_node);
> + }
>
> if (governor->manage)
> governor->manage(tz);
> @@ -1490,6 +1500,7 @@ thermal_zone_device_register_with_trips(
> for_each_trip_desc(tz, td) {
> td->trip = *trip++;
> INIT_LIST_HEAD(&td->thermal_instances);
> + INIT_LIST_HEAD(&td->list_node);
> /*
> * Mark all thresholds as invalid to start with even though
> * this only matters for the trips that start as invalid and
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 04/10] thermal: core: Rearrange __thermal_zone_device_update()
2024-10-16 11:26 ` [PATCH v1 04/10] thermal: core: Rearrange __thermal_zone_device_update() Rafael J. Wysocki
@ 2024-10-24 10:20 ` Lukasz Luba
0 siblings, 0 replies; 27+ messages in thread
From: Lukasz Luba @ 2024-10-24 10:20 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/16/24 12:26, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> In preparation for subsequent changes, move the invocations of
> thermal_thresholds_handle() and thermal_zone_set_trips() in
> __thermal_zone_device_update() after the processing of the
> temporary trip lists.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/thermal_core.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -588,10 +588,6 @@ void __thermal_zone_device_update(struct
> high = td->threshold;
> }
>
> - thermal_thresholds_handle(tz, &low, &high);
> -
> - thermal_zone_set_trips(tz, low, high);
> -
> list_for_each_entry_safe(td, next, &way_up_list, list_node) {
> thermal_trip_crossed(tz, &td->trip, governor, true);
> list_del_init(&td->list_node);
> @@ -602,6 +598,10 @@ void __thermal_zone_device_update(struct
> list_del_init(&td->list_node);
> }
>
> + thermal_thresholds_handle(tz, &low, &high);
> +
> + thermal_zone_set_trips(tz, low, high);
> +
> if (governor->manage)
> governor->manage(tz);
>
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 05/10] thermal: core: Pass trip descriptor to thermal_trip_crossed()
2024-10-16 11:27 ` [PATCH v1 05/10] thermal: core: Pass trip descriptor to thermal_trip_crossed() Rafael J. Wysocki
@ 2024-10-24 10:22 ` Lukasz Luba
0 siblings, 0 replies; 27+ messages in thread
From: Lukasz Luba @ 2024-10-24 10:22 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/16/24 12:27, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> In preparation for subsequent changes, modify thermal_trip_crossed()
> to take a trip descriptor pointer instead of a pointer to struct
> thermal_trip and propagate this change to thermal_zone_trip_down().
>
> No functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/thermal_core.c | 12 +++++++-----
> drivers/thermal/thermal_core.h | 2 +-
> drivers/thermal/thermal_trip.c | 2 +-
> 3 files changed, 9 insertions(+), 7 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -526,10 +526,12 @@ static void thermal_governor_trip_crosse
> }
>
> static void thermal_trip_crossed(struct thermal_zone_device *tz,
> - const struct thermal_trip *trip,
> + struct thermal_trip_desc *td,
> struct thermal_governor *governor,
> bool crossed_up)
> {
> + const struct thermal_trip *trip = &td->trip;
> +
> if (crossed_up) {
> thermal_notify_tz_trip_up(tz, trip);
> thermal_debug_tz_trip_up(tz, trip);
> @@ -589,12 +591,12 @@ void __thermal_zone_device_update(struct
> }
>
> list_for_each_entry_safe(td, next, &way_up_list, list_node) {
> - thermal_trip_crossed(tz, &td->trip, governor, true);
> + thermal_trip_crossed(tz, td, governor, true);
> list_del_init(&td->list_node);
> }
>
> list_for_each_entry_safe_reverse(td, next, &way_down_list, list_node) {
> - thermal_trip_crossed(tz, &td->trip, governor, false);
> + thermal_trip_crossed(tz, td, governor, false);
> list_del_init(&td->list_node);
> }
>
> @@ -664,9 +666,9 @@ void thermal_zone_device_update(struct t
> EXPORT_SYMBOL_GPL(thermal_zone_device_update);
>
> void thermal_zone_trip_down(struct thermal_zone_device *tz,
> - const struct thermal_trip *trip)
> + struct thermal_trip_desc *td)
> {
> - thermal_trip_crossed(tz, trip, thermal_get_tz_governor(tz), false);
> + thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false);
> }
>
> int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -274,7 +274,7 @@ int thermal_zone_trip_id(const struct th
> const struct thermal_trip *trip);
> int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
> void thermal_zone_trip_down(struct thermal_zone_device *tz,
> - const struct thermal_trip *trip);
> + struct thermal_trip_desc *td);
> void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz,
> struct thermal_trip *trip, int hyst);
>
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -118,7 +118,7 @@ void thermal_zone_set_trip_temp(struct t
> tz->passive--;
> WARN_ON_ONCE(tz->passive < 0);
> }
> - thermal_zone_trip_down(tz, trip);
> + thermal_zone_trip_down(tz, td);
> }
> /*
> * Invalidate the threshold to avoid triggering a spurious
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 06/10] thermal: core: Move some trip processing to thermal_trip_crossed()
2024-10-16 11:29 ` [PATCH v1 06/10] thermal: core: Move some trip processing " Rafael J. Wysocki
@ 2024-10-24 10:24 ` Lukasz Luba
0 siblings, 0 replies; 27+ messages in thread
From: Lukasz Luba @ 2024-10-24 10:24 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/16/24 12:29, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Notice that some processing related to trip point crossing carried out
> in handle_thermal_trip() and thermal_zone_set_trip_temp() may as well
> be done in thermal_trip_crossed(), which allows code duplication to be
> reduced, so change the code accordingly.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/thermal_core.c | 21 ++++++++++-----------
> drivers/thermal/thermal_trip.c | 17 ++++++-----------
> 2 files changed, 16 insertions(+), 22 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -460,11 +460,6 @@ static void handle_thermal_trip(struct t
> if (tz->temperature < trip->temperature - trip->hysteresis) {
> td->notify_temp = trip->temperature - trip->hysteresis;
> move_trip_to_sorted_list(td, way_down_list);
> -
> - if (trip->type == THERMAL_TRIP_PASSIVE) {
> - tz->passive--;
> - WARN_ON(tz->passive < 0);
> - }
> } else {
> td->threshold -= trip->hysteresis;
> }
> @@ -478,12 +473,6 @@ static void handle_thermal_trip(struct t
> move_trip_to_sorted_list(td, way_up_list);
>
> td->threshold -= trip->hysteresis;
> -
> - if (trip->type == THERMAL_TRIP_PASSIVE)
> - tz->passive++;
> - else if (trip->type == THERMAL_TRIP_CRITICAL ||
> - trip->type == THERMAL_TRIP_HOT)
> - handle_critical_trips(tz, trip);
> }
> }
>
> @@ -533,9 +522,19 @@ static void thermal_trip_crossed(struct
> const struct thermal_trip *trip = &td->trip;
>
> if (crossed_up) {
> + if (trip->type == THERMAL_TRIP_PASSIVE)
> + tz->passive++;
> + else if (trip->type == THERMAL_TRIP_CRITICAL ||
> + trip->type == THERMAL_TRIP_HOT)
> + handle_critical_trips(tz, trip);
> +
> thermal_notify_tz_trip_up(tz, trip);
> thermal_debug_tz_trip_up(tz, trip);
> } else {
> + if (trip->type == THERMAL_TRIP_PASSIVE) {
> + tz->passive--;
> + WARN_ON(tz->passive < 0);
> + }
> thermal_notify_tz_trip_down(tz, trip);
> thermal_debug_tz_trip_down(tz, trip);
> }
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -108,18 +108,13 @@ void thermal_zone_set_trip_temp(struct t
> if (temp == THERMAL_TEMP_INVALID) {
> struct thermal_trip_desc *td = trip_to_trip_desc(trip);
>
> - if (tz->temperature >= td->threshold) {
> - /*
> - * The trip has been crossed on the way up, so some
> - * adjustments are needed to compensate for the lack
> - * of it going forward.
> - */
> - if (trip->type == THERMAL_TRIP_PASSIVE) {
> - tz->passive--;
> - WARN_ON_ONCE(tz->passive < 0);
> - }
> + /*
> + * If the trip has been crossed on the way up, some adjustments
> + * are needed to compensate for the lack of it going forward.
> + */
> + if (tz->temperature >= td->threshold)
> thermal_zone_trip_down(tz, td);
> - }
> +
> /*
> * Invalidate the threshold to avoid triggering a spurious
> * trip crossing notification when the trip becomes valid.
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 07/10] thermal: core: Relocate functions that update trip points
2024-10-16 11:32 ` [PATCH v1 07/10] thermal: core: Relocate functions that update trip points Rafael J. Wysocki
@ 2024-10-24 10:25 ` Lukasz Luba
0 siblings, 0 replies; 27+ messages in thread
From: Lukasz Luba @ 2024-10-24 10:25 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/16/24 12:32, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> In preparation for subsequent changes, move two functions used
> for updating trip points, thermal_zone_set_trip_temp() and
> thermal_zone_set_trip_hyst(), to thermal_core.c.
>
> No functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/thermal_core.c | 35 +++++++++++++++++++++++++++++++++++
> drivers/thermal/thermal_trip.c | 35 -----------------------------------
> 2 files changed, 35 insertions(+), 35 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -541,6 +541,41 @@ static void thermal_trip_crossed(struct
> thermal_governor_trip_crossed(governor, tz, trip, crossed_up);
> }
>
> +void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz,
> + struct thermal_trip *trip, int hyst)
> +{
> + WRITE_ONCE(trip->hysteresis, hyst);
> + thermal_notify_tz_trip_change(tz, trip);
> +}
> +
> +void thermal_zone_set_trip_temp(struct thermal_zone_device *tz,
> + struct thermal_trip *trip, int temp)
> +{
> + if (trip->temperature == temp)
> + return;
> +
> + WRITE_ONCE(trip->temperature, temp);
> + thermal_notify_tz_trip_change(tz, trip);
> +
> + if (temp == THERMAL_TEMP_INVALID) {
> + struct thermal_trip_desc *td = trip_to_trip_desc(trip);
> +
> + /*
> + * If the trip has been crossed on the way up, some adjustments
> + * are needed to compensate for the lack of it going forward.
> + */
> + if (tz->temperature >= td->threshold)
> + thermal_zone_trip_down(tz, td);
> +
> + /*
> + * Invalidate the threshold to avoid triggering a spurious
> + * trip crossing notification when the trip becomes valid.
> + */
> + td->threshold = INT_MAX;
> + }
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_set_trip_temp);
> +
> void __thermal_zone_device_update(struct thermal_zone_device *tz,
> enum thermal_notify_event event)
> {
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -88,38 +88,3 @@ int thermal_zone_trip_id(const struct th
> */
> return trip_to_trip_desc(trip) - tz->trips;
> }
> -
> -void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz,
> - struct thermal_trip *trip, int hyst)
> -{
> - WRITE_ONCE(trip->hysteresis, hyst);
> - thermal_notify_tz_trip_change(tz, trip);
> -}
> -
> -void thermal_zone_set_trip_temp(struct thermal_zone_device *tz,
> - struct thermal_trip *trip, int temp)
> -{
> - if (trip->temperature == temp)
> - return;
> -
> - WRITE_ONCE(trip->temperature, temp);
> - thermal_notify_tz_trip_change(tz, trip);
> -
> - if (temp == THERMAL_TEMP_INVALID) {
> - struct thermal_trip_desc *td = trip_to_trip_desc(trip);
> -
> - /*
> - * If the trip has been crossed on the way up, some adjustments
> - * are needed to compensate for the lack of it going forward.
> - */
> - if (tz->temperature >= td->threshold)
> - thermal_zone_trip_down(tz, td);
> -
> - /*
> - * Invalidate the threshold to avoid triggering a spurious
> - * trip crossing notification when the trip becomes valid.
> - */
> - td->threshold = INT_MAX;
> - }
> -}
> -EXPORT_SYMBOL_GPL(thermal_zone_set_trip_temp);
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 08/10] thermal: core: Eliminate thermal_zone_trip_down()
2024-10-16 11:33 ` [PATCH v1 08/10] thermal: core: Eliminate thermal_zone_trip_down() Rafael J. Wysocki
@ 2024-10-24 10:33 ` Lukasz Luba
2024-10-24 12:33 ` Rafael J. Wysocki
0 siblings, 1 reply; 27+ messages in thread
From: Lukasz Luba @ 2024-10-24 10:33 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/16/24 12:33, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Since thermal_zone_set_trip_temp() is not located in the same file
nit: s/not/now
> as thermal_trip_crossed(), it can invoke the latter directly without
> using the thermal_zone_trip_down() wrapper that has no other users.
>
> Update thermal_zone_set_trip_temp() accordingly and drop
> thermal_zone_trip_down().
>
> No functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/thermal_core.c | 8 +-------
> drivers/thermal/thermal_core.h | 2 --
> 2 files changed, 1 insertion(+), 9 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -565,7 +565,7 @@ void thermal_zone_set_trip_temp(struct t
> * are needed to compensate for the lack of it going forward.
> */
> if (tz->temperature >= td->threshold)
> - thermal_zone_trip_down(tz, td);
> + thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false);
minor thing:
won't that be too long line?
IMHO we can add somewhere earlier:
struct thermal_governor *gov = thermal_get_tz_governor(tz);
and use it here
>
> /*
> * Invalidate the threshold to avoid triggering a spurious
> @@ -699,12 +699,6 @@ void thermal_zone_device_update(struct t
> }
> EXPORT_SYMBOL_GPL(thermal_zone_device_update);
>
> -void thermal_zone_trip_down(struct thermal_zone_device *tz,
> - struct thermal_trip_desc *td)
> -{
> - thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false);
> -}
> -
> int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
> void *data)
> {
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -273,8 +273,6 @@ void thermal_zone_set_trips(struct therm
> int thermal_zone_trip_id(const struct thermal_zone_device *tz,
> const struct thermal_trip *trip);
> int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
> -void thermal_zone_trip_down(struct thermal_zone_device *tz,
> - struct thermal_trip_desc *td);
> void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz,
> struct thermal_trip *trip, int hyst);
>
>
>
>
other than that, LGTM
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 10/10] thermal: core: Relocate thermal zone initialization routine
2024-10-16 11:36 ` [PATCH v1 10/10] thermal: core: Relocate thermal zone initialization routine Rafael J. Wysocki
@ 2024-10-24 10:35 ` Lukasz Luba
0 siblings, 0 replies; 27+ messages in thread
From: Lukasz Luba @ 2024-10-24 10:35 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/16/24 12:36, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Move thermal_zone_device_init() along with thermal_zone_device_check()
> closer to the callers of the former, where they fit better together.
>
> No functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/thermal_core.c | 82 ++++++++++++++++++++---------------------
> 1 file changed, 41 insertions(+), 41 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -429,14 +429,6 @@ static void move_trip_to_sorted_list(str
> list_add(&td->list_node, list);
> }
>
> -static void thermal_zone_device_check(struct work_struct *work)
> -{
> - struct thermal_zone_device *tz = container_of(work, struct
> - thermal_zone_device,
> - poll_queue.work);
> - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> -}
> -
> static void move_to_trips_high(struct thermal_zone_device *tz,
> struct thermal_trip_desc *td)
> {
> @@ -458,39 +450,6 @@ static void move_to_trips_invalid(struct
> list_move(&td->list_node, &tz->trips_invalid);
> }
>
> -static void thermal_zone_device_init(struct thermal_zone_device *tz)
> -{
> - struct thermal_trip_desc *td, *next;
> -
> - INIT_DELAYED_WORK(&tz->poll_queue, thermal_zone_device_check);
> -
> - tz->temperature = THERMAL_TEMP_INIT;
> - tz->passive = 0;
> - tz->prev_low_trip = -INT_MAX;
> - tz->prev_high_trip = INT_MAX;
> - for_each_trip_desc(tz, td) {
> - struct thermal_instance *instance;
> -
> - list_for_each_entry(instance, &td->thermal_instances, trip_node)
> - instance->initialized = false;
> - }
> - /*
> - * At this point, all valid trips need to be moved to trips_high so that
> - * mitigation can be started if the zone temperature is above them.
> - */
> - list_for_each_entry_safe(td, next, &tz->trips_invalid, list_node) {
> - if (td->trip.temperature != THERMAL_TEMP_INVALID)
> - move_to_trips_high(tz, td);
> - }
> - /* The trips_reached list may not be empty during system resume. */
> - list_for_each_entry_safe(td, next, &tz->trips_reached, list_node) {
> - if (td->trip.temperature == THERMAL_TEMP_INVALID)
> - move_to_trips_invalid(tz, td);
> - else
> - move_to_trips_high(tz, td);
> - }
> -}
> -
> static void thermal_governor_trip_crossed(struct thermal_governor *governor,
> struct thermal_zone_device *tz,
> const struct thermal_trip *trip,
> @@ -1425,6 +1384,47 @@ int thermal_zone_get_crit_temp(struct th
> }
> EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>
> +static void thermal_zone_device_check(struct work_struct *work)
> +{
> + struct thermal_zone_device *tz = container_of(work, struct
> + thermal_zone_device,
> + poll_queue.work);
> + thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> +}
> +
> +static void thermal_zone_device_init(struct thermal_zone_device *tz)
> +{
> + struct thermal_trip_desc *td, *next;
> +
> + INIT_DELAYED_WORK(&tz->poll_queue, thermal_zone_device_check);
> +
> + tz->temperature = THERMAL_TEMP_INIT;
> + tz->passive = 0;
> + tz->prev_low_trip = -INT_MAX;
> + tz->prev_high_trip = INT_MAX;
> + for_each_trip_desc(tz, td) {
> + struct thermal_instance *instance;
> +
> + list_for_each_entry(instance, &td->thermal_instances, trip_node)
> + instance->initialized = false;
> + }
> + /*
> + * At this point, all valid trips need to be moved to trips_high so that
> + * mitigation can be started if the zone temperature is above them.
> + */
> + list_for_each_entry_safe(td, next, &tz->trips_invalid, list_node) {
> + if (td->trip.temperature != THERMAL_TEMP_INVALID)
> + move_to_trips_high(tz, td);
> + }
> + /* The trips_reached list may not be empty during system resume. */
> + list_for_each_entry_safe(td, next, &tz->trips_reached, list_node) {
> + if (td->trip.temperature == THERMAL_TEMP_INVALID)
> + move_to_trips_invalid(tz, td);
> + else
> + move_to_trips_high(tz, td);
> + }
> +}
> +
> static int thermal_zone_init_governor(struct thermal_zone_device *tz)
> {
> struct thermal_governor *governor;
>
>
>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 09/10] thermal: core: Use trip lists for trip crossing detection
2024-10-16 11:35 ` [PATCH v1 09/10] thermal: core: Use trip lists for trip crossing detection Rafael J. Wysocki
@ 2024-10-24 11:48 ` Lukasz Luba
2024-10-24 12:22 ` Rafael J. Wysocki
0 siblings, 1 reply; 27+ messages in thread
From: Lukasz Luba @ 2024-10-24 11:48 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Linux PM, Daniel Lezcano, Zhang Rui, Srinivas Pandruvada
On 10/16/24 12:35, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Modify the thermal core to use three lists of trip points:
>
> trips_high, containing trips with thresholds strictly above the current
> thermal zone temperature,
>
> trips_reached, containing trips with thresholds at or below the current
> zone temperature,
>
> trips_invalid, containing trips with temperature equal to
> THERMAL_ZONE_INVALID,
>
> where the first two lists are always sorted by the current trip
> threshold.
>
> For each trip in trips_high, there is no mitigation under way and
> the trip threshold is equal to its temperature. In turn, for each
> trip in trips_reached, there is mitigation under way and the trip
> threshold is equal to its low temperature. The trips in trips_invalid,
> of course, need not be taken into consideration.
>
> The idea is to make __thermal_zone_device_update() walk trips_high and
> trips_reached instead of walking the entire table of trip points in a
> thermal zone. Usually, it will only need to walk a few entries in one
> of the lists and check one entry in the other list, depending on the
> direction of the zone temperature changes, because crossing many trips
> by the zone temperature in one go between two consecutive temperature
> checks should be unlikely (if it occurs often, the thermal zone
> temperature should probably be checked more often either or there
> are too many trips).
>
> This also helps to eliminate one temporary trip list used for trip
> crossing notification (only one temporary list is needed for this
> purpose instead of two) and the remaining temporary list may be sorted
> by the current trip threshold value, like the trips_reached list, so
> the additional notify_temp field in struct thermal_trip_desc is not
> necessary any more.
>
> Moreover, since the trips_reached and trips_high lists are sorted,
> the "low" and "high" values needed by thermal_zone_set_trips() can be
> determined in a straightforward way by looking at one end of each list.
>
> Of course, additional work is needed in some places in order to
> maintain the ordering of the lists, but it is limited to situations
> that should be rare, like updating a trip point temperature or
> hysteresis, thermal zone initialization, or system resume.
Good point I agree. Even the trips from userspace that Daniel
proposed shouldn't impact that design decision IMHO.
Any disagreement (or am I missing something)?
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/thermal_core.c | 211 +++++++++++++++++++++++++----------------
> drivers/thermal/thermal_core.h | 7 +
> 2 files changed, 136 insertions(+), 82 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -33,7 +33,6 @@ struct thermal_trip_desc {
> struct thermal_trip_attrs trip_attrs;
> struct list_head list_node;
> struct list_head thermal_instances;
> - int notify_temp;
> int threshold;
> };
>
> @@ -78,6 +77,9 @@ struct thermal_governor {
> * @device: &struct device for this thermal zone
> * @removal: removal completion
> * @resume: resume completion
> + * @trips_high: trips above the current zone temperature
> + * @trips_reached: trips below or at the current zone temperature
> + * @trips_invalid: trips with invalid temperature
> * @mode: current mode of this thermal zone
> * @devdata: private pointer for device private data
> * @num_trips: number of trip points the thermal zone supports
> @@ -118,6 +120,9 @@ struct thermal_zone_device {
> struct completion removal;
> struct completion resume;
> struct attribute_group trips_attribute_group;
> + struct list_head trips_high;
> + struct list_head trips_reached;
> + struct list_head trips_invalid;
> enum thermal_device_mode mode;
> void *devdata;
> int num_trips;
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -421,7 +421,7 @@ static void move_trip_to_sorted_list(str
>
> /* Assume that the new entry is likely to be the last one. */
> list_for_each_entry_reverse(entry, list, list_node) {
> - if (entry->notify_temp <= td->notify_temp) {
> + if (entry->threshold <= td->threshold) {
> list_add(&td->list_node, &entry->list_node);
> return;
> }
> @@ -429,53 +429,6 @@ static void move_trip_to_sorted_list(str
> list_add(&td->list_node, list);
> }
>
> -static void handle_thermal_trip(struct thermal_zone_device *tz,
> - struct thermal_trip_desc *td,
> - struct list_head *way_up_list,
> - struct list_head *way_down_list)
> -{
> - const struct thermal_trip *trip = &td->trip;
> - int old_threshold;
> -
> - if (trip->temperature == THERMAL_TEMP_INVALID)
> - return;
> -
> - /*
> - * If the trip temperature or hysteresis has been updated recently,
> - * the threshold needs to be computed again using the new values.
> - * However, its initial value still reflects the old ones and that
> - * is what needs to be compared with the previous zone temperature
> - * to decide which action to take.
> - */
> - old_threshold = td->threshold;
> - td->threshold = trip->temperature;
> -
> - if (tz->last_temperature >= old_threshold &&
> - tz->last_temperature != THERMAL_TEMP_INIT) {
> - /*
> - * Mitigation is under way, so it needs to stop if the zone
> - * temperature falls below the low temperature of the trip.
> - * In that case, the trip temperature becomes the new threshold.
> - */
> - if (tz->temperature < trip->temperature - trip->hysteresis) {
> - td->notify_temp = trip->temperature - trip->hysteresis;
> - move_trip_to_sorted_list(td, way_down_list);
> - } else {
> - td->threshold -= trip->hysteresis;
> - }
> - } else if (tz->temperature >= trip->temperature) {
> - /*
> - * There is no mitigation under way, so it needs to be started
> - * if the zone temperature exceeds the trip one. The new
> - * threshold is then set to the low temperature of the trip.
> - */
> - td->notify_temp = trip->temperature;
> - move_trip_to_sorted_list(td, way_up_list);
> -
> - td->threshold -= trip->hysteresis;
> - }
> -}
> -
> static void thermal_zone_device_check(struct work_struct *work)
> {
> struct thermal_zone_device *tz = container_of(work, struct
> @@ -484,9 +437,30 @@ static void thermal_zone_device_check(st
> thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> }
>
> +static void move_to_trips_high(struct thermal_zone_device *tz,
> + struct thermal_trip_desc *td)
> +{
> + td->threshold = td->trip.temperature;
> + move_trip_to_sorted_list(td, &tz->trips_high);
> +}
> +
> +static void move_to_trips_reached(struct thermal_zone_device *tz,
> + struct thermal_trip_desc *td)
> +{
> + td->threshold = td->trip.temperature - td->trip.hysteresis;
> + move_trip_to_sorted_list(td, &tz->trips_reached);
> +}
> +
> +static void move_to_trips_invalid(struct thermal_zone_device *tz,
> + struct thermal_trip_desc *td)
> +{
> + td->threshold = INT_MAX;
> + list_move(&td->list_node, &tz->trips_invalid);
> +}
> +
> static void thermal_zone_device_init(struct thermal_zone_device *tz)
> {
> - struct thermal_trip_desc *td;
> + struct thermal_trip_desc *td, *next;
>
> INIT_DELAYED_WORK(&tz->poll_queue, thermal_zone_device_check);
>
> @@ -500,6 +474,21 @@ static void thermal_zone_device_init(str
> list_for_each_entry(instance, &td->thermal_instances, trip_node)
> instance->initialized = false;
> }
> + /*
> + * At this point, all valid trips need to be moved to trips_high so that
> + * mitigation can be started if the zone temperature is above them.
> + */
> + list_for_each_entry_safe(td, next, &tz->trips_invalid, list_node) {
> + if (td->trip.temperature != THERMAL_TEMP_INVALID)
> + move_to_trips_high(tz, td);
> + }
> + /* The trips_reached list may not be empty during system resume. */
> + list_for_each_entry_safe(td, next, &tz->trips_reached, list_node) {
> + if (td->trip.temperature == THERMAL_TEMP_INVALID)
> + move_to_trips_invalid(tz, td);
> + else
> + move_to_trips_high(tz, td);
> + }
> }
>
> static void thermal_governor_trip_crossed(struct thermal_governor *governor,
> @@ -544,45 +533,120 @@ static void thermal_trip_crossed(struct
> void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz,
> struct thermal_trip *trip, int hyst)
> {
> + struct thermal_trip_desc *td = trip_to_trip_desc(trip);
> +
> WRITE_ONCE(trip->hysteresis, hyst);
> thermal_notify_tz_trip_change(tz, trip);
> + /*
> + * If the zone temperature is above or at the trip tmperature, the trip
> + * is in the trips_reached list and its threshold is equal to its low
> + * temperature. It needs to stay in that list, but its threshold needs
> + * to be updated and the list ordering may need to be restored.
> + */
> + if (tz->temperature >= td->threshold)
> + move_to_trips_reached(tz, td);
> }
>
> void thermal_zone_set_trip_temp(struct thermal_zone_device *tz,
> struct thermal_trip *trip, int temp)
> {
> - if (trip->temperature == temp)
> + struct thermal_trip_desc *td = trip_to_trip_desc(trip);
> + int old_temp = trip->temperature;
> +
> + if (old_temp == temp)
> return;
>
> WRITE_ONCE(trip->temperature, temp);
> thermal_notify_tz_trip_change(tz, trip);
>
> - if (temp == THERMAL_TEMP_INVALID) {
> - struct thermal_trip_desc *td = trip_to_trip_desc(trip);
> + if (old_temp == THERMAL_TEMP_INVALID) {
> + /*
> + * The trip was invalid before the change, so move it to the
> + * trips_high list regardless of the new temperature value
> + * because there is no mitigation under way for it. If a
> + * mitigation needs to be started, the trip will be moved to the
> + * trips_reached list later.
> + */
Good comment
> + move_to_trips_high(tz, td);
> + return;
> + }
>
> + if (temp == THERMAL_TEMP_INVALID) {
> /*
> - * If the trip has been crossed on the way up, some adjustments
> - * are needed to compensate for the lack of it going forward.
> + * If the trip is in the trips_reached list, mitigation is under
> + * way for it and it needs to be stopped because the trip is
> + * effectively going away.
> */
> if (tz->temperature >= td->threshold)
> thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false);
>
> + move_to_trips_invalid(tz, td);
> + return;
> + }
> +
> + /*
> + * The trip stays on its current list, but its threshold needs to be
> + * updated due to the temperature change and the list ordering may need
> + * to be restored.
> + */
> + if (tz->temperature >= td->threshold)
> + move_to_trips_reached(tz, td);
> + else
> + move_to_trips_high(tz, td);
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_set_trip_temp);
> +
> +static void thermal_zone_handle_trips(struct thermal_zone_device *tz,
> + struct thermal_governor *governor,
> + int *low, int *high)
> +{
> + struct thermal_trip_desc *td, *next;
> + LIST_HEAD(way_down_list);
> +
> + /* Check the trips that were below or at the zone temperature. */
> + list_for_each_entry_safe_reverse(td, next, &tz->trips_reached, list_node) {
> + if (td->threshold <= tz->temperature)
> + break;
> +
> + thermal_trip_crossed(tz, td, governor, false);
> /*
> - * Invalidate the threshold to avoid triggering a spurious
> - * trip crossing notification when the trip becomes valid.
> + * The current trips_high list needs to be processed before
> + * adding new entries to it, so put them on a temporary list.
> */
> - td->threshold = INT_MAX;
> + list_move(&td->list_node, &way_down_list);
> + }
> + /* Check the trips that were previously above the zone temperature. */
> + list_for_each_entry_safe(td, next, &tz->trips_high, list_node) {
> + if (td->threshold > tz->temperature)
> + break;
> +
> + thermal_trip_crossed(tz, td, governor, true);
> + move_to_trips_reached(tz, td);
> + }
nit:
I would add and extra empty line here, since this is
quite separate from the loop above and has important
meaning.
> + list_for_each_entry_safe(td, next, &way_down_list, list_node)
> + move_to_trips_high(tz, td);
> +
> + if (!list_empty(&tz->trips_reached)) {
> + td = list_last_entry(&tz->trips_reached,
> + struct thermal_trip_desc, list_node);
> + /*
> + * Set the "low" value below the current trip threshold in case
> + * the zone temperature is at that threshold and stays there,
> + * which would trigger a new interrupt immediately in vain.
> + */
> + *low = td->threshold - 1;
> + }
> + if (!list_empty(&tz->trips_high)) {
> + td = list_first_entry(&tz->trips_high,
> + struct thermal_trip_desc, list_node);
> + *high = td->threshold;
> }
> }
> -EXPORT_SYMBOL_GPL(thermal_zone_set_trip_temp);
>
> void __thermal_zone_device_update(struct thermal_zone_device *tz,
> enum thermal_notify_event event)
> {
> struct thermal_governor *governor = thermal_get_tz_governor(tz);
> - struct thermal_trip_desc *td, *next;
> - LIST_HEAD(way_down_list);
> - LIST_HEAD(way_up_list);
> int low = -INT_MAX, high = INT_MAX;
> int temp, ret;
>
> @@ -614,25 +678,7 @@ void __thermal_zone_device_update(struct
>
> tz->notify_event = event;
>
> - for_each_trip_desc(tz, td) {
> - handle_thermal_trip(tz, td, &way_up_list, &way_down_list);
> -
> - if (td->threshold <= tz->temperature && td->threshold > low)
> - low = td->threshold;
> -
> - if (td->threshold >= tz->temperature && td->threshold < high)
> - high = td->threshold;
> - }
> -
> - list_for_each_entry_safe(td, next, &way_up_list, list_node) {
> - thermal_trip_crossed(tz, td, governor, true);
> - list_del_init(&td->list_node);
> - }
> -
> - list_for_each_entry_safe_reverse(td, next, &way_down_list, list_node) {
> - thermal_trip_crossed(tz, td, governor, false);
> - list_del_init(&td->list_node);
> - }
> + thermal_zone_handle_trips(tz, governor, &low, &high);
>
> thermal_thresholds_handle(tz, &low, &high);
>
> @@ -1507,6 +1553,9 @@ thermal_zone_device_register_with_trips(
> }
>
> INIT_LIST_HEAD(&tz->node);
> + INIT_LIST_HEAD(&tz->trips_high);
> + INIT_LIST_HEAD(&tz->trips_reached);
> + INIT_LIST_HEAD(&tz->trips_invalid);
> ida_init(&tz->ida);
> mutex_init(&tz->lock);
> init_completion(&tz->removal);
> @@ -1536,7 +1585,7 @@ thermal_zone_device_register_with_trips(
> * this only matters for the trips that start as invalid and
> * become valid later.
> */
> - td->threshold = INT_MAX;
> + move_to_trips_invalid(tz, td);
> }
>
> tz->polling_delay_jiffies = msecs_to_jiffies(polling_delay);
>
>
>
It took me a while to get the new code flow and idea.
Anyway, not that much controversial changes, which might
look like, on the first glance, LGTM.
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 09/10] thermal: core: Use trip lists for trip crossing detection
2024-10-24 11:48 ` Lukasz Luba
@ 2024-10-24 12:22 ` Rafael J. Wysocki
0 siblings, 0 replies; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-24 12:22 UTC (permalink / raw)
To: Lukasz Luba
Cc: Rafael J. Wysocki, LKML, Linux PM, Daniel Lezcano, Zhang Rui,
Srinivas Pandruvada
On Thu, Oct 24, 2024 at 1:47 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 10/16/24 12:35, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Modify the thermal core to use three lists of trip points:
> >
> > trips_high, containing trips with thresholds strictly above the current
> > thermal zone temperature,
> >
> > trips_reached, containing trips with thresholds at or below the current
> > zone temperature,
> >
> > trips_invalid, containing trips with temperature equal to
> > THERMAL_ZONE_INVALID,
> >
> > where the first two lists are always sorted by the current trip
> > threshold.
> >
> > For each trip in trips_high, there is no mitigation under way and
> > the trip threshold is equal to its temperature. In turn, for each
> > trip in trips_reached, there is mitigation under way and the trip
> > threshold is equal to its low temperature. The trips in trips_invalid,
> > of course, need not be taken into consideration.
> >
> > The idea is to make __thermal_zone_device_update() walk trips_high and
> > trips_reached instead of walking the entire table of trip points in a
> > thermal zone. Usually, it will only need to walk a few entries in one
> > of the lists and check one entry in the other list, depending on the
> > direction of the zone temperature changes, because crossing many trips
> > by the zone temperature in one go between two consecutive temperature
> > checks should be unlikely (if it occurs often, the thermal zone
> > temperature should probably be checked more often either or there
> > are too many trips).
> >
> > This also helps to eliminate one temporary trip list used for trip
> > crossing notification (only one temporary list is needed for this
> > purpose instead of two) and the remaining temporary list may be sorted
> > by the current trip threshold value, like the trips_reached list, so
> > the additional notify_temp field in struct thermal_trip_desc is not
> > necessary any more.
> >
> > Moreover, since the trips_reached and trips_high lists are sorted,
> > the "low" and "high" values needed by thermal_zone_set_trips() can be
> > determined in a straightforward way by looking at one end of each list.
> >
> > Of course, additional work is needed in some places in order to
> > maintain the ordering of the lists, but it is limited to situations
> > that should be rare, like updating a trip point temperature or
> > hysteresis, thermal zone initialization, or system resume.
>
> Good point I agree. Even the trips from userspace that Daniel
> proposed shouldn't impact that design decision IMHO.
> Any disagreement (or am I missing something)?
No disagreement AFAIK.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> > drivers/thermal/thermal_core.c | 211 +++++++++++++++++++++++++----------------
> > drivers/thermal/thermal_core.h | 7 +
> > 2 files changed, 136 insertions(+), 82 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/thermal_core.h
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_core.h
> > +++ linux-pm/drivers/thermal/thermal_core.h
> > @@ -33,7 +33,6 @@ struct thermal_trip_desc {
> > struct thermal_trip_attrs trip_attrs;
> > struct list_head list_node;
> > struct list_head thermal_instances;
> > - int notify_temp;
> > int threshold;
> > };
> >
> > @@ -78,6 +77,9 @@ struct thermal_governor {
> > * @device: &struct device for this thermal zone
> > * @removal: removal completion
> > * @resume: resume completion
> > + * @trips_high: trips above the current zone temperature
> > + * @trips_reached: trips below or at the current zone temperature
> > + * @trips_invalid: trips with invalid temperature
> > * @mode: current mode of this thermal zone
> > * @devdata: private pointer for device private data
> > * @num_trips: number of trip points the thermal zone supports
> > @@ -118,6 +120,9 @@ struct thermal_zone_device {
> > struct completion removal;
> > struct completion resume;
> > struct attribute_group trips_attribute_group;
> > + struct list_head trips_high;
> > + struct list_head trips_reached;
> > + struct list_head trips_invalid;
> > enum thermal_device_mode mode;
> > void *devdata;
> > int num_trips;
> > Index: linux-pm/drivers/thermal/thermal_core.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_core.c
> > +++ linux-pm/drivers/thermal/thermal_core.c
> > @@ -421,7 +421,7 @@ static void move_trip_to_sorted_list(str
> >
> > /* Assume that the new entry is likely to be the last one. */
> > list_for_each_entry_reverse(entry, list, list_node) {
> > - if (entry->notify_temp <= td->notify_temp) {
> > + if (entry->threshold <= td->threshold) {
> > list_add(&td->list_node, &entry->list_node);
> > return;
> > }
> > @@ -429,53 +429,6 @@ static void move_trip_to_sorted_list(str
> > list_add(&td->list_node, list);
> > }
> >
> > -static void handle_thermal_trip(struct thermal_zone_device *tz,
> > - struct thermal_trip_desc *td,
> > - struct list_head *way_up_list,
> > - struct list_head *way_down_list)
> > -{
> > - const struct thermal_trip *trip = &td->trip;
> > - int old_threshold;
> > -
> > - if (trip->temperature == THERMAL_TEMP_INVALID)
> > - return;
> > -
> > - /*
> > - * If the trip temperature or hysteresis has been updated recently,
> > - * the threshold needs to be computed again using the new values.
> > - * However, its initial value still reflects the old ones and that
> > - * is what needs to be compared with the previous zone temperature
> > - * to decide which action to take.
> > - */
> > - old_threshold = td->threshold;
> > - td->threshold = trip->temperature;
> > -
> > - if (tz->last_temperature >= old_threshold &&
> > - tz->last_temperature != THERMAL_TEMP_INIT) {
> > - /*
> > - * Mitigation is under way, so it needs to stop if the zone
> > - * temperature falls below the low temperature of the trip.
> > - * In that case, the trip temperature becomes the new threshold.
> > - */
> > - if (tz->temperature < trip->temperature - trip->hysteresis) {
> > - td->notify_temp = trip->temperature - trip->hysteresis;
> > - move_trip_to_sorted_list(td, way_down_list);
> > - } else {
> > - td->threshold -= trip->hysteresis;
> > - }
> > - } else if (tz->temperature >= trip->temperature) {
> > - /*
> > - * There is no mitigation under way, so it needs to be started
> > - * if the zone temperature exceeds the trip one. The new
> > - * threshold is then set to the low temperature of the trip.
> > - */
> > - td->notify_temp = trip->temperature;
> > - move_trip_to_sorted_list(td, way_up_list);
> > -
> > - td->threshold -= trip->hysteresis;
> > - }
> > -}
> > -
> > static void thermal_zone_device_check(struct work_struct *work)
> > {
> > struct thermal_zone_device *tz = container_of(work, struct
> > @@ -484,9 +437,30 @@ static void thermal_zone_device_check(st
> > thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> > }
> >
> > +static void move_to_trips_high(struct thermal_zone_device *tz,
> > + struct thermal_trip_desc *td)
> > +{
> > + td->threshold = td->trip.temperature;
> > + move_trip_to_sorted_list(td, &tz->trips_high);
> > +}
> > +
> > +static void move_to_trips_reached(struct thermal_zone_device *tz,
> > + struct thermal_trip_desc *td)
> > +{
> > + td->threshold = td->trip.temperature - td->trip.hysteresis;
> > + move_trip_to_sorted_list(td, &tz->trips_reached);
> > +}
> > +
> > +static void move_to_trips_invalid(struct thermal_zone_device *tz,
> > + struct thermal_trip_desc *td)
> > +{
> > + td->threshold = INT_MAX;
> > + list_move(&td->list_node, &tz->trips_invalid);
> > +}
> > +
> > static void thermal_zone_device_init(struct thermal_zone_device *tz)
> > {
> > - struct thermal_trip_desc *td;
> > + struct thermal_trip_desc *td, *next;
> >
> > INIT_DELAYED_WORK(&tz->poll_queue, thermal_zone_device_check);
> >
> > @@ -500,6 +474,21 @@ static void thermal_zone_device_init(str
> > list_for_each_entry(instance, &td->thermal_instances, trip_node)
> > instance->initialized = false;
> > }
> > + /*
> > + * At this point, all valid trips need to be moved to trips_high so that
> > + * mitigation can be started if the zone temperature is above them.
> > + */
> > + list_for_each_entry_safe(td, next, &tz->trips_invalid, list_node) {
> > + if (td->trip.temperature != THERMAL_TEMP_INVALID)
> > + move_to_trips_high(tz, td);
> > + }
> > + /* The trips_reached list may not be empty during system resume. */
> > + list_for_each_entry_safe(td, next, &tz->trips_reached, list_node) {
> > + if (td->trip.temperature == THERMAL_TEMP_INVALID)
> > + move_to_trips_invalid(tz, td);
> > + else
> > + move_to_trips_high(tz, td);
> > + }
> > }
> >
> > static void thermal_governor_trip_crossed(struct thermal_governor *governor,
> > @@ -544,45 +533,120 @@ static void thermal_trip_crossed(struct
> > void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz,
> > struct thermal_trip *trip, int hyst)
> > {
> > + struct thermal_trip_desc *td = trip_to_trip_desc(trip);
> > +
> > WRITE_ONCE(trip->hysteresis, hyst);
> > thermal_notify_tz_trip_change(tz, trip);
> > + /*
> > + * If the zone temperature is above or at the trip tmperature, the trip
> > + * is in the trips_reached list and its threshold is equal to its low
> > + * temperature. It needs to stay in that list, but its threshold needs
> > + * to be updated and the list ordering may need to be restored.
> > + */
> > + if (tz->temperature >= td->threshold)
> > + move_to_trips_reached(tz, td);
> > }
> >
> > void thermal_zone_set_trip_temp(struct thermal_zone_device *tz,
> > struct thermal_trip *trip, int temp)
> > {
> > - if (trip->temperature == temp)
> > + struct thermal_trip_desc *td = trip_to_trip_desc(trip);
> > + int old_temp = trip->temperature;
> > +
> > + if (old_temp == temp)
> > return;
> >
> > WRITE_ONCE(trip->temperature, temp);
> > thermal_notify_tz_trip_change(tz, trip);
> >
> > - if (temp == THERMAL_TEMP_INVALID) {
> > - struct thermal_trip_desc *td = trip_to_trip_desc(trip);
> > + if (old_temp == THERMAL_TEMP_INVALID) {
> > + /*
> > + * The trip was invalid before the change, so move it to the
> > + * trips_high list regardless of the new temperature value
> > + * because there is no mitigation under way for it. If a
> > + * mitigation needs to be started, the trip will be moved to the
> > + * trips_reached list later.
> > + */
>
> Good comment
>
> > + move_to_trips_high(tz, td);
> > + return;
> > + }
> >
> > + if (temp == THERMAL_TEMP_INVALID) {
> > /*
> > - * If the trip has been crossed on the way up, some adjustments
> > - * are needed to compensate for the lack of it going forward.
> > + * If the trip is in the trips_reached list, mitigation is under
> > + * way for it and it needs to be stopped because the trip is
> > + * effectively going away.
> > */
> > if (tz->temperature >= td->threshold)
> > thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false);
> >
> > + move_to_trips_invalid(tz, td);
> > + return;
> > + }
> > +
> > + /*
> > + * The trip stays on its current list, but its threshold needs to be
> > + * updated due to the temperature change and the list ordering may need
> > + * to be restored.
> > + */
> > + if (tz->temperature >= td->threshold)
> > + move_to_trips_reached(tz, td);
> > + else
> > + move_to_trips_high(tz, td);
> > +}
> > +EXPORT_SYMBOL_GPL(thermal_zone_set_trip_temp);
> > +
> > +static void thermal_zone_handle_trips(struct thermal_zone_device *tz,
> > + struct thermal_governor *governor,
> > + int *low, int *high)
> > +{
> > + struct thermal_trip_desc *td, *next;
> > + LIST_HEAD(way_down_list);
> > +
> > + /* Check the trips that were below or at the zone temperature. */
> > + list_for_each_entry_safe_reverse(td, next, &tz->trips_reached, list_node) {
> > + if (td->threshold <= tz->temperature)
> > + break;
> > +
> > + thermal_trip_crossed(tz, td, governor, false);
> > /*
> > - * Invalidate the threshold to avoid triggering a spurious
> > - * trip crossing notification when the trip becomes valid.
> > + * The current trips_high list needs to be processed before
> > + * adding new entries to it, so put them on a temporary list.
> > */
> > - td->threshold = INT_MAX;
> > + list_move(&td->list_node, &way_down_list);
> > + }
> > + /* Check the trips that were previously above the zone temperature. */
> > + list_for_each_entry_safe(td, next, &tz->trips_high, list_node) {
> > + if (td->threshold > tz->temperature)
> > + break;
> > +
> > + thermal_trip_crossed(tz, td, governor, true);
> > + move_to_trips_reached(tz, td);
> > + }
>
> nit:
> I would add and extra empty line here, since this is
> quite separate from the loop above and has important
> meaning.
It is though closely related to the first loop.
I'll add a one-line comment here when applying the patch.
> > + list_for_each_entry_safe(td, next, &way_down_list, list_node)
> > + move_to_trips_high(tz, td);
> > +
> > + if (!list_empty(&tz->trips_reached)) {
> > + td = list_last_entry(&tz->trips_reached,
> > + struct thermal_trip_desc, list_node);
> > + /*
> > + * Set the "low" value below the current trip threshold in case
> > + * the zone temperature is at that threshold and stays there,
> > + * which would trigger a new interrupt immediately in vain.
> > + */
> > + *low = td->threshold - 1;
> > + }
> > + if (!list_empty(&tz->trips_high)) {
> > + td = list_first_entry(&tz->trips_high,
> > + struct thermal_trip_desc, list_node);
> > + *high = td->threshold;
> > }
> > }
> > -EXPORT_SYMBOL_GPL(thermal_zone_set_trip_temp);
> >
> > void __thermal_zone_device_update(struct thermal_zone_device *tz,
> > enum thermal_notify_event event)
> > {
> > struct thermal_governor *governor = thermal_get_tz_governor(tz);
> > - struct thermal_trip_desc *td, *next;
> > - LIST_HEAD(way_down_list);
> > - LIST_HEAD(way_up_list);
> > int low = -INT_MAX, high = INT_MAX;
> > int temp, ret;
> >
> > @@ -614,25 +678,7 @@ void __thermal_zone_device_update(struct
> >
> > tz->notify_event = event;
> >
> > - for_each_trip_desc(tz, td) {
> > - handle_thermal_trip(tz, td, &way_up_list, &way_down_list);
> > -
> > - if (td->threshold <= tz->temperature && td->threshold > low)
> > - low = td->threshold;
> > -
> > - if (td->threshold >= tz->temperature && td->threshold < high)
> > - high = td->threshold;
> > - }
> > -
> > - list_for_each_entry_safe(td, next, &way_up_list, list_node) {
> > - thermal_trip_crossed(tz, td, governor, true);
> > - list_del_init(&td->list_node);
> > - }
> > -
> > - list_for_each_entry_safe_reverse(td, next, &way_down_list, list_node) {
> > - thermal_trip_crossed(tz, td, governor, false);
> > - list_del_init(&td->list_node);
> > - }
> > + thermal_zone_handle_trips(tz, governor, &low, &high);
> >
> > thermal_thresholds_handle(tz, &low, &high);
> >
> > @@ -1507,6 +1553,9 @@ thermal_zone_device_register_with_trips(
> > }
> >
> > INIT_LIST_HEAD(&tz->node);
> > + INIT_LIST_HEAD(&tz->trips_high);
> > + INIT_LIST_HEAD(&tz->trips_reached);
> > + INIT_LIST_HEAD(&tz->trips_invalid);
> > ida_init(&tz->ida);
> > mutex_init(&tz->lock);
> > init_completion(&tz->removal);
> > @@ -1536,7 +1585,7 @@ thermal_zone_device_register_with_trips(
> > * this only matters for the trips that start as invalid and
> > * become valid later.
> > */
> > - td->threshold = INT_MAX;
> > + move_to_trips_invalid(tz, td);
> > }
> >
> > tz->polling_delay_jiffies = msecs_to_jiffies(polling_delay);
> >
> >
> >
>
> It took me a while to get the new code flow and idea.
>
> Anyway, not that much controversial changes, which might
> look like, on the first glance, LGTM.
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Thank you and thanks for all of the reviews!
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 08/10] thermal: core: Eliminate thermal_zone_trip_down()
2024-10-24 10:33 ` Lukasz Luba
@ 2024-10-24 12:33 ` Rafael J. Wysocki
2024-10-24 13:39 ` Lukasz Luba
0 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2024-10-24 12:33 UTC (permalink / raw)
To: Lukasz Luba
Cc: Rafael J. Wysocki, LKML, Linux PM, Daniel Lezcano, Zhang Rui,
Srinivas Pandruvada
On Thu, Oct 24, 2024 at 12:32 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 10/16/24 12:33, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Since thermal_zone_set_trip_temp() is not located in the same file
>
> nit: s/not/now
Thanks, will fix when applying the patch.
> > as thermal_trip_crossed(), it can invoke the latter directly without
> > using the thermal_zone_trip_down() wrapper that has no other users.
> >
> > Update thermal_zone_set_trip_temp() accordingly and drop
> > thermal_zone_trip_down().
> >
> > No functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> > drivers/thermal/thermal_core.c | 8 +-------
> > drivers/thermal/thermal_core.h | 2 --
> > 2 files changed, 1 insertion(+), 9 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/thermal_core.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_core.c
> > +++ linux-pm/drivers/thermal/thermal_core.c
> > @@ -565,7 +565,7 @@ void thermal_zone_set_trip_temp(struct t
> > * are needed to compensate for the lack of it going forward.
> > */
> > if (tz->temperature >= td->threshold)
> > - thermal_zone_trip_down(tz, td);
> > + thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false);
>
> minor thing:
> won't that be too long line?
It is longer than 80 characters, but this is not a hard boundary - see
"2) Breaking long lines and strings" in
Documentation/process/coding-style.rst).
Well, you can argue about the "hide information" part, but IMV this
line just looks cleaner the way it is than when it would be broken in
any way.
> IMHO we can add somewhere earlier:
> struct thermal_governor *gov = thermal_get_tz_governor(tz);
> and use it here
That would have been harder to follow than the current code IMO.
> >
> > /*
> > * Invalidate the threshold to avoid triggering a spurious
> > @@ -699,12 +699,6 @@ void thermal_zone_device_update(struct t
> > }
> > EXPORT_SYMBOL_GPL(thermal_zone_device_update);
> >
> > -void thermal_zone_trip_down(struct thermal_zone_device *tz,
> > - struct thermal_trip_desc *td)
> > -{
> > - thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false);
> > -}
> > -
> > int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
> > void *data)
> > {
> > Index: linux-pm/drivers/thermal/thermal_core.h
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_core.h
> > +++ linux-pm/drivers/thermal/thermal_core.h
> > @@ -273,8 +273,6 @@ void thermal_zone_set_trips(struct therm
> > int thermal_zone_trip_id(const struct thermal_zone_device *tz,
> > const struct thermal_trip *trip);
> > int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
> > -void thermal_zone_trip_down(struct thermal_zone_device *tz,
> > - struct thermal_trip_desc *td);
> > void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz,
> > struct thermal_trip *trip, int hyst);
> >
> >
> >
> >
>
> other than that, LGTM
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Thank you!
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 08/10] thermal: core: Eliminate thermal_zone_trip_down()
2024-10-24 12:33 ` Rafael J. Wysocki
@ 2024-10-24 13:39 ` Lukasz Luba
0 siblings, 0 replies; 27+ messages in thread
From: Lukasz Luba @ 2024-10-24 13:39 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, LKML, Linux PM, Daniel Lezcano, Zhang Rui,
Srinivas Pandruvada
On 10/24/24 13:33, Rafael J. Wysocki wrote:
> On Thu, Oct 24, 2024 at 12:32 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>
>>
>> On 10/16/24 12:33, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Since thermal_zone_set_trip_temp() is not located in the same file
>>
>> nit: s/not/now
>
> Thanks, will fix when applying the patch.
>
>>> as thermal_trip_crossed(), it can invoke the latter directly without
>>> using the thermal_zone_trip_down() wrapper that has no other users.
>>>
>>> Update thermal_zone_set_trip_temp() accordingly and drop
>>> thermal_zone_trip_down().
>>>
>>> No functional impact.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>> drivers/thermal/thermal_core.c | 8 +-------
>>> drivers/thermal/thermal_core.h | 2 --
>>> 2 files changed, 1 insertion(+), 9 deletions(-)
>>>
>>> Index: linux-pm/drivers/thermal/thermal_core.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/thermal/thermal_core.c
>>> +++ linux-pm/drivers/thermal/thermal_core.c
>>> @@ -565,7 +565,7 @@ void thermal_zone_set_trip_temp(struct t
>>> * are needed to compensate for the lack of it going forward.
>>> */
>>> if (tz->temperature >= td->threshold)
>>> - thermal_zone_trip_down(tz, td);
>>> + thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false);
>>
>> minor thing:
>> won't that be too long line?
>
> It is longer than 80 characters, but this is not a hard boundary - see
> "2) Breaking long lines and strings" in
> Documentation/process/coding-style.rst).
>
> Well, you can argue about the "hide information" part, but IMV this
> line just looks cleaner the way it is than when it would be broken in
> any way.
>
>> IMHO we can add somewhere earlier:
>> struct thermal_governor *gov = thermal_get_tz_governor(tz);
>> and use it here
>
> That would have been harder to follow than the current code IMO.
fair enough
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2024-10-24 13:38 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-16 11:18 [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
2024-10-16 11:21 ` [PATCH v1 01/10] thermal: core: Build sorted lists instead of sorting them later Rafael J. Wysocki
2024-10-24 10:14 ` Lukasz Luba
2024-10-16 11:23 ` [PATCH v1 02/10] thermal: core: Rename trip list node in struct thermal_trip_desc Rafael J. Wysocki
2024-10-24 10:16 ` Lukasz Luba
2024-10-16 11:24 ` [PATCH v1 03/10] thermal: core: Prepare for moving trips between sorted lists Rafael J. Wysocki
2024-10-24 10:19 ` Lukasz Luba
2024-10-16 11:26 ` [PATCH v1 04/10] thermal: core: Rearrange __thermal_zone_device_update() Rafael J. Wysocki
2024-10-24 10:20 ` Lukasz Luba
2024-10-16 11:27 ` [PATCH v1 05/10] thermal: core: Pass trip descriptor to thermal_trip_crossed() Rafael J. Wysocki
2024-10-24 10:22 ` Lukasz Luba
2024-10-16 11:29 ` [PATCH v1 06/10] thermal: core: Move some trip processing " Rafael J. Wysocki
2024-10-24 10:24 ` Lukasz Luba
2024-10-16 11:32 ` [PATCH v1 07/10] thermal: core: Relocate functions that update trip points Rafael J. Wysocki
2024-10-24 10:25 ` Lukasz Luba
2024-10-16 11:33 ` [PATCH v1 08/10] thermal: core: Eliminate thermal_zone_trip_down() Rafael J. Wysocki
2024-10-24 10:33 ` Lukasz Luba
2024-10-24 12:33 ` Rafael J. Wysocki
2024-10-24 13:39 ` Lukasz Luba
2024-10-16 11:35 ` [PATCH v1 09/10] thermal: core: Use trip lists for trip crossing detection Rafael J. Wysocki
2024-10-24 11:48 ` Lukasz Luba
2024-10-24 12:22 ` Rafael J. Wysocki
2024-10-16 11:36 ` [PATCH v1 10/10] thermal: core: Relocate thermal zone initialization routine Rafael J. Wysocki
2024-10-24 10:35 ` Lukasz Luba
2024-10-21 11:16 ` [PATCH v1 00/10] thermal: core: Use lists of trips for trip crossing detection and handling Rafael J. Wysocki
2024-10-22 22:52 ` Lukasz Luba
2024-10-23 8:53 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).