| Message ID | 20240729150259.1089814-1-daniel.lezcano@linaro.org |
|---|---|
| Headers | show |
| Series | Add thermal thresholds support | expand |
On Monday, July 29, 2024 5:02:50 PM CEST Daniel Lezcano wrote: > In order to set the scene for the thresholds support which have to > manipulate the low and high temperature boundaries for the interrupt > support, we must pass the low and high value the incoming thresholds > routine. > > Instead of looping in the trip descriptors in > thermal_zone_device_update(), we move the loop in the > handle_thermal_trip() function and use it to set the low and high > values. > > As these variables can be set directly in the handle_thermal_trip(), > we can get rid of a descriptors loop found in the thermal_set_trips() > function as low and high are set in handle_thermal_trip(). > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/thermal/thermal_core.c | 104 +++++++++++++++++++-------------- > drivers/thermal/thermal_core.h | 2 +- > drivers/thermal/thermal_trip.c | 12 +--- > 3 files changed, 62 insertions(+), 56 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index 95c399f94744..5cfa2a706e96 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -425,59 +425,74 @@ static void handle_critical_trips(struct thermal_zone_device *tz, > } > > 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) > + struct list_head *way_down_list, > + int *low, int *high) > { > - const struct thermal_trip *trip = &td->trip; > + struct thermal_trip_desc *td; > int old_threshold; > > - if (trip->temperature == THERMAL_TEMP_INVALID) > - return; > + for_each_trip_desc(tz, td) { > > - /* > - * 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; > + const struct thermal_trip *trip = &td->trip; > + > + if (trip->temperature == THERMAL_TEMP_INVALID) > + continue; > + > + if (tz->last_temperature < old_threshold || > + tz->last_temperature == THERMAL_TEMP_INIT) > + continue; > > - 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 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. > */ > - if (tz->temperature < trip->temperature - trip->hysteresis) { > - list_add(&td->notify_list_node, way_down_list); > - td->notify_temp = trip->temperature - trip->hysteresis; > + old_threshold = td->threshold; > + td->threshold = trip->temperature; > > - if (trip->type == THERMAL_TRIP_PASSIVE) { > - tz->passive--; > - WARN_ON(tz->passive < 0); > + if (tz->last_temperature >= old_threshold && > + tz->last_temperature != THERMAL_TEMP_INVALID) { > + /* > + * 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) { > + list_add(&td->notify_list_node, way_down_list); > + td->notify_temp = trip->temperature - trip->hysteresis; > + > + if (trip->type == THERMAL_TRIP_PASSIVE) { > + tz->passive--; > + WARN_ON(tz->passive < 0); > + } > + } else { > + td->threshold -= trip->hysteresis; > } > - } else { > + } 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. > + */ > + list_add_tail(&td->notify_list_node, way_up_list); > + td->notify_temp = trip->temperature; > 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); > } > - } 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. > - */ > - list_add_tail(&td->notify_list_node, way_up_list); > - td->notify_temp = trip->temperature; > - 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); > + > + if (td->threshold < tz->temperature && td->threshold > *low) > + *low = td->threshold; > + > + if (td->threshold > tz->temperature && td->threshold < *high) > + *high = td->threshold; > } > } > > @@ -545,6 +560,8 @@ void __thermal_zone_device_update(struct thermal_zone_device *tz, > { > struct thermal_governor *governor = thermal_get_tz_governor(tz); > struct thermal_trip_desc *td; > + int low = -INT_MAX, high = INT_MAX; > + > LIST_HEAD(way_down_list); > LIST_HEAD(way_up_list); > int temp, ret; > @@ -580,10 +597,9 @@ void __thermal_zone_device_update(struct thermal_zone_device *tz, > > tz->notify_event = event; > > - for_each_trip_desc(tz, td) > - handle_thermal_trip(tz, td, &way_up_list, &way_down_list); > + handle_thermal_trip(tz, &way_up_list, &way_down_list, &low, &high); > > - thermal_zone_set_trips(tz); > + 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) > diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h > index 4cf2b7230d04..67a09f90eb95 100644 > --- a/drivers/thermal/thermal_core.h > +++ b/drivers/thermal/thermal_core.h > @@ -259,7 +259,7 @@ void thermal_governor_update_tz(struct thermal_zone_device *tz, > > const char *thermal_trip_type_name(enum thermal_trip_type trip_type); > > -void thermal_zone_set_trips(struct thermal_zone_device *tz); > +void thermal_zone_set_trips(struct thermal_zone_device *tz, int low, int high); > int thermal_zone_trip_id(const struct thermal_zone_device *tz, > const struct thermal_trip *trip); > void thermal_zone_trip_updated(struct thermal_zone_device *tz, > diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c > index c0b679b846b3..af0f9f5ae0de 100644 > --- a/drivers/thermal/thermal_trip.c > +++ b/drivers/thermal/thermal_trip.c > @@ -76,10 +76,8 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_num_trips); > * > * It does not return a value > */ > -void thermal_zone_set_trips(struct thermal_zone_device *tz) > +void thermal_zone_set_trips(struct thermal_zone_device *tz, int low, int high) > { > - const struct thermal_trip_desc *td; > - int low = -INT_MAX, high = INT_MAX; > int ret; > > lockdep_assert_held(&tz->lock); > @@ -87,14 +85,6 @@ void thermal_zone_set_trips(struct thermal_zone_device *tz) > if (!tz->ops.set_trips) > return; > > - for_each_trip_desc(tz, td) { > - if (td->threshold < tz->temperature && td->threshold > low) > - low = td->threshold; > - > - if (td->threshold > tz->temperature && td->threshold < high) > - high = td->threshold; > - } > - > /* No need to change trip points */ > if (tz->prev_low_trip == low && tz->prev_high_trip == high) > return; > Well, why not do the (untested) change below instead, which is way simpler? The thermal_zone_set_trips() kerneldoc needs to be either updated or dropped because it is not applicable any more after this and I think it's better to just drop it. --- drivers/thermal/thermal_core.c | 12 ++++++++++-- drivers/thermal/thermal_core.h | 2 +- drivers/thermal/thermal_trip.c | 27 +-------------------------- 3 files changed, 12 insertions(+), 29 deletions(-) Index: linux-pm/drivers/thermal/thermal_core.c =================================================================== --- linux-pm.orig/drivers/thermal/thermal_core.c +++ linux-pm/drivers/thermal/thermal_core.c @@ -551,6 +551,7 @@ void __thermal_zone_device_update(struct struct thermal_trip_desc *td; LIST_HEAD(way_down_list); LIST_HEAD(way_up_list); + int low = -INT_MAX, high = INT_MAX; int temp, ret; if (tz->suspended) @@ -584,10 +585,17 @@ void __thermal_zone_device_update(struct tz->notify_event = event; - for_each_trip_desc(tz, td) + for_each_trip_desc(tz, td) { handle_thermal_trip(tz, td, &way_up_list, &way_down_list); - thermal_zone_set_trips(tz); + if (td->threshold < tz->temperature && td->threshold > low) + low = td->threshold; + + if (td->threshold >= tz->temperature && td->threshold < high) + high = td->threshold; + } + + 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) Index: linux-pm/drivers/thermal/thermal_core.h =================================================================== --- linux-pm.orig/drivers/thermal/thermal_core.h +++ linux-pm/drivers/thermal/thermal_core.h @@ -254,7 +254,7 @@ void thermal_governor_update_tz(struct t const char *thermal_trip_type_name(enum thermal_trip_type trip_type); -void thermal_zone_set_trips(struct thermal_zone_device *tz); +void thermal_zone_set_trips(struct thermal_zone_device *tz, int low, int high); 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); Index: linux-pm/drivers/thermal/thermal_trip.c =================================================================== --- linux-pm.orig/drivers/thermal/thermal_trip.c +++ linux-pm/drivers/thermal/thermal_trip.c @@ -55,25 +55,8 @@ int thermal_zone_for_each_trip(struct th } EXPORT_SYMBOL_GPL(thermal_zone_for_each_trip); -/** - * thermal_zone_set_trips - Computes the next trip points for the driver - * @tz: a pointer to a thermal zone device structure - * - * The function computes the next temperature boundaries by browsing - * the trip points. The result is the closer low and high trip points - * to the current temperature. These values are passed to the backend - * driver to let it set its own notification mechanism (usually an - * interrupt). - * - * This function must be called with tz->lock held. Both tz and tz->ops - * must be valid pointers. - * - * It does not return a value - */ -void thermal_zone_set_trips(struct thermal_zone_device *tz) +void thermal_zone_set_trips(struct thermal_zone_device *tz, int low, int high) { - const struct thermal_trip_desc *td; - int low = -INT_MAX, high = INT_MAX; int ret; lockdep_assert_held(&tz->lock); @@ -81,14 +64,6 @@ void thermal_zone_set_trips(struct therm if (!tz->ops.set_trips) return; - for_each_trip_desc(tz, td) { - if (td->threshold < tz->temperature && td->threshold > low) - low = td->threshold; - - if (td->threshold > tz->temperature && td->threshold < high) - high = td->threshold; - } - /* No need to change trip points */ if (tz->prev_low_trip == low && tz->prev_high_trip == high) return;
Hi Rafael, On 29/07/2024 18:57, Rafael J. Wysocki wrote: > On Monday, July 29, 2024 5:02:50 PM CEST Daniel Lezcano wrote: >> In order to set the scene for the thresholds support which have to >> manipulate the low and high temperature boundaries for the interrupt >> support, we must pass the low and high value the incoming thresholds >> routine. >> >> Instead of looping in the trip descriptors in >> thermal_zone_device_update(), we move the loop in the >> handle_thermal_trip() function and use it to set the low and high >> values. >> >> As these variables can be set directly in the handle_thermal_trip(), >> we can get rid of a descriptors loop found in the thermal_set_trips() >> function as low and high are set in handle_thermal_trip(). >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- [ ... ] >> - for_each_trip_desc(tz, td) >> - handle_thermal_trip(tz, td, &way_up_list, &way_down_list); >> + handle_thermal_trip(tz, &way_up_list, &way_down_list, &low, &high); >> [ ... ] > Well, why not do the (untested) change below instead, which is way simpler? Right, I did your proposed changed initially. The patch looks very complicated but it is just the result of the difference between the change above and below. It is code reorg, without functional changes (except two loops => one loop). It looked to me more consistent to move the for_each_trip_desc() inside handle_thermal_trip() in order to: - encapsulate the trip code more and have one line call - be consistent with the thermal_threshold_handle() which is also one line call If you prefer, I can split the changes, but it is extra work for little benefit. I pushed the changes in the git tree, the resulting code from these changes are: https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/thermal/thermal_core.c?h=thermal/threshold-patchset#n427 and https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/thermal/thermal_core.c?h=thermal/threshold-patchset#n600 Let me know if you prefer to do a smaller change or go forward in the code encapsulation > The thermal_zone_set_trips() kerneldoc needs to be either updated or dropped > because it is not applicable any more after this and I think it's better to > just drop it. Sure > - for_each_trip_desc(tz, td) > + for_each_trip_desc(tz, td) { > handle_thermal_trip(tz, td, &way_up_list, &way_down_list); [ ... ]
Hi Daniel, On Tue, Jul 30, 2024 at 11:40 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > > Hi Rafael, > > On 29/07/2024 18:57, Rafael J. Wysocki wrote: > > On Monday, July 29, 2024 5:02:50 PM CEST Daniel Lezcano wrote: > >> In order to set the scene for the thresholds support which have to > >> manipulate the low and high temperature boundaries for the interrupt > >> support, we must pass the low and high value the incoming thresholds > >> routine. > >> > >> Instead of looping in the trip descriptors in > >> thermal_zone_device_update(), we move the loop in the > >> handle_thermal_trip() function and use it to set the low and high > >> values. > >> > >> As these variables can be set directly in the handle_thermal_trip(), > >> we can get rid of a descriptors loop found in the thermal_set_trips() > >> function as low and high are set in handle_thermal_trip(). > >> > >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >> --- > > [ ... ] > > >> - for_each_trip_desc(tz, td) > >> - handle_thermal_trip(tz, td, &way_up_list, &way_down_list); > >> + handle_thermal_trip(tz, &way_up_list, &way_down_list, &low, &high); > >> > > [ ... ] > > > Well, why not do the (untested) change below instead, which is way simpler? > > Right, I did your proposed changed initially. OK > The patch looks very > complicated but it is just the result of the difference between the > change above and below. It is code reorg, without functional changes > (except two loops => one loop). > > It looked to me more consistent to move the for_each_trip_desc() inside > handle_thermal_trip() in order to: > - encapsulate the trip code more and have one line call I don't agree with this. The purpose is questionable and the code becomes more complex overall. And I'm totally not a fan of passing values through pointers if that is avoidable. > - be consistent with the thermal_threshold_handle() which is also one > line call That is a somewhat different story. > If you prefer, I can split the changes, but it is extra work for little > benefit. I pushed the changes in the git tree, the resulting code from > these changes are: > > https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/thermal/thermal_core.c?h=thermal/threshold-patchset#n427 > > and > > https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/thermal/thermal_core.c?h=thermal/threshold-patchset#n600 > > Let me know if you prefer to do a smaller change or go forward in the > code encapsulation I'd prefer to make a smaller change (obviously). > > The thermal_zone_set_trips() kerneldoc needs to be either updated or dropped > > because it is not applicable any more after this and I think it's better to > > just drop it. > > Sure BTW, there is a problem in thermal_zone_set_trips() now if the zone temperature is exactly equal to one of the trip's thresholds because it will then skip that trip. Say there are three trips, A, B, C sorted in ascending temperature order with no hysteresis. Say the zone temperature is exactly equal to the temperature of B. thermal_zone_set_trips() will set the boundaries to A and C, so the crossing of B will not be caught. IMV, both comparisons with the zone temperature in thermal_zone_set_trips() need to be made not strict, that is if (td->threshold <= tz->temperature && td->threshold > low) low = td->threshold; if (td->threshold => tz->temperature && td->threshold < high) high = td->threshold; in order to catch all of the trip crossing events. Of course, in the example above, this will cause thermal_zone_set_trips() to set both the boundaries to B, but that's OK because it will trigger an interrupt when the zone temperature becomes different from the B threshold regardless of which way it goes and that will allow a new interval to be set (aither {A, B} if it goes down or {B, C} if it goes up). IMV this change needs to be made in -stable, so I'll send a patch for it to be applied before any other changes in thermal_zone_set_trips().
The trip points are a firmware description of the temperature limits of a specific thermal zone where we associate an action which is done by the kernel. The time resolution is low. The userspace has to deal with a more complex thermal management based on heuristics from different information coming from different places. The logic is much more complex but based on a bigger time resolution, usually one second based. The purpose of the userspace is to monitor the temperatures from different places and take actions. However, it can not be constantly reading the temperature to detect when a temperature threshold has been reached. This is especially bad for mobile or embedded system as that will lead to an unacceptable number of wakeup to check the temperature with nothing to do. On the other side, the sensors are now most of the time interrupt driven. That means the thermal framework will use the temperature trip points to program the sensor to trigger an interrupt when a temperature limit is crossed. Unfortunately, the userspace can not benefit this feature and current solutions found here and there, iow out-of-tree, are to add fake trip points in the firmware and enable the writable trip points. This is bad for different reasons, the trip points are for in-kernel actions, the semantic of their types is used by the thermal framework and by adding trip points in the device tree is a way to overcome the current limitation but tampering with how the thermal framework is supposed to work. The writable trip points is a way to adjust a temperature limit given a specific platform if the firmware is not accurate enough and TBH it is more a debug feature from my POV. The thresholds mechanism is a way to have the userspace to tell thermal framework to send a notification when a temperature limit is crossed. There is no id, no hysteresis, just the temperature and the direction of the limit crossing. That means we can be notified when a threshold is crossed the way up only, or the way down only or both ways. That allows to create hysteresis values if it is needed. A threshold can be added, deleted or flushed. The latter means all thresholds belonging to a thermal zone will be deleted. When a threshold is added: - if the same threshold (temperature and direction) exists, an error is returned - if a threshold is specified with the same temperature but a different direction, the specified direction is added - if there is no threshold with the same temperature then it is created When a threshold is deleted: - if the same threshold (temperature and direction) exists, it is deleted - if a threshold is specified with the same temperature but a different direction, the specified direction is removed - if there is no threshold with the same temperature, then an error is returned When the threshold are flushed: - All thresholds related to a thermal zone are deleted When a threshold is crossed: - the userspace does not need to know which threshold(s) have been crossed, it will be notified with the thermal zone identifier, the current temperature and the previous temperature - if multiple thresholds have been crossed between two updates only one notification will be send to the userspace, it is pointless to send a notification per thresholds crossed as the userspace can handle that easily when it has the temperature delta information All aforementioned actions and events lead to a notification to the userspace. A threshold change (add, delete and flush) is notified to the userspace with the process id responsible of the action. Along with the kernel changes, the thermal library has been extended to provide the different API to deal with the new threshold netlink events and commands. In addition, the thermal-engine skeleton uses these new API by flushing and adding thresholds as well as getting the notification about these actions. Overall the series has been tested with the thermal-engine skeleton and some selftests which are not part of this series. Daniel Lezcano (7): thermal/core: Encapsulate more handle_thermal_trip thermal/core: Add thresholds support thermal/core: Connect the threshold with the core thermal/netlink: Add the commands and the events for the thresholds tools/lib/thermal: Make more generic the command encoding function tools/lib/thermal: Add the threshold netlink ABI tools/thermal/thermal-engine: Take into account the thresholds API drivers/thermal/Kconfig | 15 ++ drivers/thermal/Makefile | 3 + drivers/thermal/thermal_core.c | 109 ++++---- drivers/thermal/thermal_core.h | 6 +- drivers/thermal/thermal_netlink.c | 239 ++++++++++++++++- drivers/thermal/thermal_netlink.h | 7 + drivers/thermal/thermal_thresholds.c | 246 ++++++++++++++++++ drivers/thermal/thermal_thresholds.h | 54 ++++ drivers/thermal/thermal_trip.c | 12 +- include/linux/thermal.h | 3 + include/uapi/linux/thermal.h | 30 ++- tools/lib/thermal/commands.c | 167 +++++++++++- tools/lib/thermal/events.c | 58 ++++- tools/lib/thermal/include/thermal.h | 40 +++ tools/lib/thermal/libthermal.map | 5 + tools/lib/thermal/thermal.c | 17 ++ tools/thermal/thermal-engine/thermal-engine.c | 109 +++++++- 17 files changed, 1016 insertions(+), 104 deletions(-) create mode 100644 drivers/thermal/thermal_thresholds.c create mode 100644 drivers/thermal/thermal_thresholds.h