Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Fri, 13 Oct 2023 12:10:54 -0400
From:   =?utf-8?B?TsOtY29sYXMgRi4gUi4gQS4=?= Prado 
        <nfraprado@collabora.com>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        kernel@collabora.com, Amit Kucheria <amitk@kernel.org>,
        Caesar Wang <wxt@rock-chips.com>,
        Eduardo Valentin <edubezval@gmail.com>,
        Javi Merino <javi.merino@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Zhang Rui <rui.zhang@intel.com>, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org
Subject: Re: [PATCH v2] thermal/core: Don't update trip points inside the
 hysteresis range
Message-ID: <f33a5cb1-7ec3-429e-a162-fb5dcbcdd664@notapiano>
References: <20230922184425.290894-1-nfraprado@collabora.com>
 <6627b83b-bee7-a123-d845-cad8523ffb30@collabora.com>
 <CAJZ5v0hU9E10BeEjF-PZ8Yx9NZO1YimitockY3Hsobha7dARDQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAJZ5v0hU9E10BeEjF-PZ8Yx9NZO1YimitockY3Hsobha7dARDQ@mail.gmail.com>
Precedence: bulk

On Fri, Oct 13, 2023 at 05:27:08PM +0200, Rafael J. Wysocki wrote:
> On Mon, Sep 25, 2023 at 9:29 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
> >
> > Il 22/09/23 20:44, Nícolas F. R. A. Prado ha scritto:
> > > When searching for the trip points that need to be set, the nearest
> > > higher trip point's temperature is used for the high trip, while the
> > > nearest lower 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.
> > >
> > > The following logs show the current behavior when running on a real
> > > machine:
> > >
> > > [  202.524658] thermal thermal_zone0: new temperature boundaries: -2147483647 < x < 40000
> > >     203.562817: thermal_temperature: thermal_zone=vpu0-thermal id=0 temp_prev=36986 temp=37979
> > > [  203.562845] thermal thermal_zone0: new temperature boundaries: 37000 < x < 40000
> > >     204.176059: thermal_temperature: thermal_zone=vpu0-thermal id=0 temp_prev=37979 temp=40028
> > > [  204.176089] thermal thermal_zone0: new temperature boundaries: 37000 < x < 100000
> > >     205.226813: thermal_temperature: thermal_zone=vpu0-thermal id=0 temp_prev=40028 temp=38652
> > > [  205.226842] thermal thermal_zone0: new temperature boundaries: 37000 < x < 40000
> > >
> > > And with this patch applied:
> > >
> > > [  184.933415] thermal thermal_zone0: new temperature boundaries: -2147483647 < x < 40000
> > >     185.981182: thermal_temperature: thermal_zone=vpu0-thermal id=0 temp_prev=36986 temp=37872
> > >     186.744685: thermal_temperature: thermal_zone=vpu0-thermal id=0 temp_prev=37872 temp=40058
> > > [  186.744716] thermal thermal_zone0: new temperature boundaries: 37000 < x < 100000
> > >     187.773284: thermal_temperature: thermal_zone=vpu0-thermal id=0 temp_prev=40058 temp=38698
> > >
> > > Fixes: 060c034a9741 ("thermal: Add support for hardware-tracked trip points")
> > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> >
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >
> > >
> > > ---
> > >
> > > Changes in v2:
> > > - Changed logic as suggested by Rafael
> > > - Added log example to commit message
> > > - Added fixes tag
> > >
> > >   drivers/thermal/thermal_trip.c | 19 +++++++++++++++++--
> > >   1 file changed, 17 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> > > index 024e2e365a26..597ac4144e33 100644
> > > --- a/drivers/thermal/thermal_trip.c
> > > +++ b/drivers/thermal/thermal_trip.c
> > > @@ -55,6 +55,7 @@ void __thermal_zone_set_trips(struct thermal_zone_device *tz)
> > >   {
> > >       struct thermal_trip trip;
> > >       int low = -INT_MAX, high = INT_MAX;
> > > +     bool same_trip = false;
> > >       int i, ret;
> > >
> > >       lockdep_assert_held(&tz->lock);
> > > @@ -63,6 +64,7 @@ void __thermal_zone_set_trips(struct thermal_zone_device *tz)
> > >               return;
> > >
> > >       for (i = 0; i < tz->num_trips; i++) {
> > > +             bool low_set = false;
> > >               int trip_low;
> > >
> > >               ret = __thermal_zone_get_trip(tz, i , &trip);
> > > @@ -71,18 +73,31 @@ 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_set = true;
> > > +                     same_trip = false;
> > > +             }
> > >
> > >               if (trip.temperature > tz->temperature &&
> > > -                 trip.temperature < high)
> > > +                 trip.temperature < high) {
> > >                       high = trip.temperature;
> > > +                     same_trip = low_set;
> > > +             }
> > >       }
> > >
> > >       /* No need to change trip points */
> > >       if (tz->prev_low_trip == low && tz->prev_high_trip == high)
> > >               return;
> > >
> > > +     /*
> > > +      * If "high" and "low" are the same, skip the change unless this is the
> > > +      * first time.
> > > +      */
> > > +     if (same_trip && (tz->prev_low_trip != -INT_MAX ||
> > > +         tz->prev_high_trip != INT_MAX))
> > > +             return;
> > > +
> > >       tz->prev_low_trip = low;
> > >       tz->prev_high_trip = high;
> > >
> 
> Applied as 6.7 material, but I added a Co-developed-by tag for myself,
> because it has been based on my patch.

Sounds good, thanks! I'll add it myself in situations like this in the future.

Thanks,
Nícolas