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().