Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Mon, 21 Aug 2023 23:10:27 +0200 | Subject | Re: [PATCH] thermal/core: Don't update trip points inside the hysteresis range |
| |
On Mon, Jul 3, 2023 at 7:15 PM Nícolas F. R. A. Prado <nfraprado@collabora.com> wrote: > > When searching for the trip points that need to be set, the nearest trip > point's temperature is used for the high trip, while the nearest trip > point's temperature minus the hysteresis is used for the low trip. The > issue with this logic is that when the current temperature is inside a > trip point's hysteresis range, both high and low trips will come from > the same trip point. As a consequence instability can still occur like > this: > * the temperature rises slightly and enters the hysteresis range of a > trip point > * polling happens and updates the trip points to the hysteresis range > * the temperature falls slightly, exiting the hysteresis range, crossing > the trip point and triggering an IRQ, the trip points are updated > * repeat > > So even though the current hysteresis implementation prevents > instability from happening due to IRQs triggering on the same > temperature value, both ways, it doesn't prevent it from happening due > to an IRQ on one way and polling on the other. > > To properly implement a hysteresis behavior, when inside the hysteresis > range, don't update the trip points. This way, the previously set trip > points will stay in effect, which will in a way remember the previous > state (if the temperature signal came from above or below the range) and > therefore have the right trip point already set. The exception is if > there was no previous trip point set, in which case a previous state > doesn't exist, and so it's sensible to allow the hysteresis range as > trip points. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > drivers/thermal/thermal_trip.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c > index 907f3a4d7bc8..c386ac5d8bad 100644 > --- a/drivers/thermal/thermal_trip.c > +++ b/drivers/thermal/thermal_trip.c > @@ -57,6 +57,7 @@ void __thermal_zone_set_trips(struct thermal_zone_device *tz) > { > struct thermal_trip trip; > int low = -INT_MAX, high = INT_MAX; > + int low_trip_id = -1, high_trip_id = -2; > int i, ret; > > lockdep_assert_held(&tz->lock); > @@ -73,18 +74,34 @@ void __thermal_zone_set_trips(struct thermal_zone_device *tz) > > trip_low = trip.temperature - trip.hysteresis; > > - if (trip_low < tz->temperature && trip_low > low) > + if (trip_low < tz->temperature && trip_low > low) { > low = trip_low; > + low_trip_id = i; > + } >
I think I get the idea, but wouldn't a similar effect be achieved by adding an "else" here?
> if (trip.temperature > tz->temperature && > - trip.temperature < high) > + trip.temperature < high) { > high = trip.temperature; > + high_trip_id = i; > + } > } > > /* No need to change trip points */ > if (tz->prev_low_trip == low && tz->prev_high_trip == high) > return; > > + /* > + * If the current temperature is inside a trip point's hysteresis range, > + * don't update the trip points, rely on the previously set ones to > + * rememember the previous state. > + * > + * Unless no previous trip point was set, in which case there's no > + * previous state to remember. > + */ > + if ((tz->prev_low_trip > -INT_MAX || tz->prev_high_trip < INT_MAX) && > + low_trip_id == high_trip_id) > + return; > + > tz->prev_low_trip = low; > tz->prev_high_trip = high; > > --
| |