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

[Rafael J. Wysocki]

On Tue, Mar 26, 2024 at 12:29 AM Nícolas F. R. A. Prado
<nfraprado@xxxxxxxxxxxxx> 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=95°C, hyst=5°C (passive)
> >  trip point 2: temp=115°C, hyst=0°C (critical)
> >  governor: step_wise
> >
> > 1. The temperature crosses the way up the trip point 1 at 95°C
> >
> >    - trend=raising
> >    - throttle=1, target=1
> >    - passive=1
> >    - set_trips: low=90°C, high=115°C
> >
> > 2. The temperature decreases but stays in the hysteresis region at
> >    93°C
> >
> >    - trend=dropping
> >    - throttle=0, target=0
> >    - passive=1
> >
> >    Before cf3986f8c01d3
> >    - set_trips: low=90°C, high=95°C
> >
> >    After cf3986f8c01d3
> >    - set_trips: low=90°C, high=115°C
> >
> > 3. The temperature increases a bit but stays in the hysteresis region
> >    at 94°C (so below the trip point 1 temp 95°C)
> >
> >    - trend=raising
> >    - throttle=0, target=0
> >    - passive=1
> >
> >    Before cf3986f8c01d3
> >    - set_trips: low=90°C, high=95°C
> >
> >    After cf3986f8c01d3
> >    - set_trips: low=90°C, high=115°C
> >
> > 4. The temperature decreases but stays in the hysteresis region at
> >    93°C
> >
> >    - trend=dropping
> >    - throttle=0, target=THERMAL_NO_TARGET
> >    - passive=0
> >
> >    Before cf3986f8c01d3
> >    - set_trips: low=90°C, high=95°C
> >
> >    After cf3986f8c01d3
> >    - set_trips: low=90°C, high=115°C
> >
> > 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°C. 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°C trip point and
> > reboot.
> >
> > Before the optimization, the high boundary would have been 95°C, thus
> > 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 the hysteresis range")
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > Reported-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@xxxxxxxxxxx>
> > Cc: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
>
> As mentioned in the commit, the issue is elsewhere, but given the original
> commit was an optimization to prevent unnecessary trip point updates, and that
> it seems to have caused a regression, sounds reasonable to revert at least while
> a proper fix isn't found.
>
> Acked-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
>
> 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!