Re: [PATCH] thermal/core: Don't update trip points inside the hysteresis range

[Rafael J. Wysocki]

On Mon, Jul 3, 2023 at 7:15 PM Nícolas F. R. A. Prado
<nfraprado@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>
> ---
>
>  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;
>
> --