Received-SPF: pass (google.com: domain of linux-kernel+bounces-118959-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240325222424.4179635-1-daniel.lezcano@linaro.org> <fbef883e-23f8-41b9-852b-c52d18816559@notapiano>
In-Reply-To: <fbef883e-23f8-41b9-852b-c52d18816559@notapiano>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Tue, 26 Mar 2024 13:21:14 +0100
Message-ID: <CAJZ5v0g5wZvKgh7HGa0NSrVQ=E-WeC1v0Av0h08-mQC9t7HQ3Q@mail.gmail.com>
Subject: Re: [PATCH] Revert "thermal: core: Don't update trip points inside
 the hysteresis range"
To: =?UTF-8?B?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= <nfraprado@collabora.com>, 
	Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>, Zhang Rui <rui.zhang@intel.com>, 
	Lukasz Luba <lukasz.luba@arm.com>, "open list:THERMAL" <linux-pm@vger.kernel.org>, 
	open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 26, 2024 at 12:29=E2=80=AFAM N=C3=ADcolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> On Mon, Mar 25, 2024 at 11:24:24PM +0100, Daniel Lezcano wrote:
> > It has been reported the commit cf3986f8c01d3 introduced a regression
> > when the temperature is wavering in the hysteresis region. The
> > mitigation stops leading to an uncontrolled temperature increase until
> > reaching the critical trip point.
> >
> > Here what happens:
> >
> >  * 'throttle' is when the current temperature is greater than the trip
> >    point temperature
> >  * 'target' is the mitigation level
> >  * 'passive' is positive when there is a mitigation, zero otherwise
> >  * these values are computed in the step_wise governor
> >
> > Configuration:
> >
> >  trip point 1: temp=3D95=C2=B0C, hyst=3D5=C2=B0C (passive)
> >  trip point 2: temp=3D115=C2=B0C, hyst=3D0=C2=B0C (critical)
> >  governor: step_wise
> >
> > 1. The temperature crosses the way up the trip point 1 at 95=C2=B0C
> >
> >    - trend=3Draising
> >    - throttle=3D1, target=3D1
> >    - passive=3D1
> >    - set_trips: low=3D90=C2=B0C, high=3D115=C2=B0C
> >
> > 2. The temperature decreases but stays in the hysteresis region at
> >    93=C2=B0C
> >
> >    - trend=3Ddropping
> >    - throttle=3D0, target=3D0
> >    - passive=3D1
> >
> >    Before cf3986f8c01d3
> >    - set_trips: low=3D90=C2=B0C, high=3D95=C2=B0C
> >
> >    After cf3986f8c01d3
> >    - set_trips: low=3D90=C2=B0C, high=3D115=C2=B0C
> >
> > 3. The temperature increases a bit but stays in the hysteresis region
> >    at 94=C2=B0C (so below the trip point 1 temp 95=C2=B0C)
> >
> >    - trend=3Draising
> >    - throttle=3D0, target=3D0
> >    - passive=3D1
> >
> >    Before cf3986f8c01d3
> >    - set_trips: low=3D90=C2=B0C, high=3D95=C2=B0C
> >
> >    After cf3986f8c01d3
> >    - set_trips: low=3D90=C2=B0C, high=3D115=C2=B0C
> >
> > 4. The temperature decreases but stays in the hysteresis region at
> >    93=C2=B0C
> >
> >    - trend=3Ddropping
> >    - throttle=3D0, target=3DTHERMAL_NO_TARGET
> >    - passive=3D0
> >
> >    Before cf3986f8c01d3
> >    - set_trips: low=3D90=C2=B0C, high=3D95=C2=B0C
> >
> >    After cf3986f8c01d3
> >    - set_trips: low=3D90=C2=B0C, high=3D115=C2=B0C
> >
> > At this point, the 'passive' value is zero, there is no mitigation,
> > the temperature is in the hysteresis region, the next trip point is
> > 115=C2=B0C. As 'passive' is zero, the timer to monitor the thermal zone=
 is
> > disabled. Consequently if the temperature continues to increase, no
> > mitigation will happen and it will reach the 115=C2=B0C trip point and
> > reboot.
> >
> > Before the optimization, the high boundary would have been 95=C2=B0C, t=
hus
> > triggering the mitigation again and rearming the polling timer.
> >
> > The optimization make sense but given the current implementation of
> > the step_wise governor collaborating via this 'passive' flag with the
> > core framework it can not work.
> >
> > From a higher perspective it seems like there is a problem between the
> > governor which sets a variable to be used by the core framework. That
> > sounds akward and it would make much more sense if the core framework
> > controls the governor and not the opposite. But as the devil hides in
> > the details, there are some subtilities to be addressed before.
> >
> > Elaborating those would be out of the scope this changelog. So let's
> > stay simple and revert the change first to fixup all broken mobile
> > platforms.
> >
> > This reverts commit cf3986f8c01d355490d0ac6024391b989a9d1e9d.
> >
> > This revert applies on top of v6.9-rc1.
> >
> > Fixes: cf3986f8c01d3 ("thermal: core: Don't update trip points inside t=
he hysteresis range")
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Reported-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
> > Cc: N=C3=ADcolas F. R. A. Prado <nfraprado@collabora.com>
>
> As mentioned in the commit, the issue is elsewhere, but given the origina=
l
> commit was an optimization to prevent unnecessary trip point updates, and=
 that
> it seems to have caused a regression, sounds reasonable to revert at leas=
t while
> a proper fix isn't found.
>
> Acked-by: N=C3=ADcolas F. R. A. Prado <nfraprado@collabora.com>
>
> Only thing is you might want to add a cc: stable tag to guarantee it is
> backported (AFAIR Fixes: doesn't guarantee backport), even more so given =
there
> are conflicts.

Applied as 6.9-rc material, thanks!