Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230703171502.44657-1-nfraprado@collabora.com>
In-Reply-To: <20230703171502.44657-1-nfraprado@collabora.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Mon, 21 Aug 2023 23:10:27 +0200
Message-ID: <CAJZ5v0hiiiReiJBPZRCMs16E247GL-nJGjnwkiMCNq5q4VjkyQ@mail.gmail.com>
Subject: Re: [PATCH] thermal/core: Don't update trip points inside the
 hysteresis range
To:     =?UTF-8?B?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= 
        <nfraprado@collabora.com>
Cc:     "Rafael J . Wysocki" <rafael@kernel.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>, kernel@collabora.com,
        Amit Kucheria <amitk@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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;
>
> --