Received-SPF: pass (google.com: domain of linux-kernel+bounces-22233-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240110115526.30776-1-di.shen@unisoc.com> <0cbc1708-bc50-459c-ad57-0cf283921f2e@arm.com>
In-Reply-To: <0cbc1708-bc50-459c-ad57-0cf283921f2e@arm.com>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Wed, 10 Jan 2024 14:30:13 +0100
Message-ID: <CAJZ5v0iTff4Uvmd7KT-SJ253xvHo8Jxrqjy5XQ6TTAomiVagbg@mail.gmail.com>
Subject: Re: [PATCH V7] thermal/core/power_allocator: avoid thermal cdev can
 not be reset
To: Lukasz Luba <lukasz.luba@arm.com>
Cc: Di Shen <di.shen@unisoc.com>, linux-pm@vger.kernel.org, rui.zhang@intel.com, 
	daniel.lezcano@linaro.org, rafael@kernel.org, linux-kernel@vger.kernel.org, 
	wvw@google.com, tkjos@google.com, xuewen.yan@unisoc.com, zhanglyra@gmail.com, 
	orsonzhai@gmail.com, cindygm567@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 10, 2024 at 2:03=E2=80=AFPM Lukasz Luba <lukasz.luba@arm.com> w=
rote:
>
>
>
> On 1/10/24 11:55, Di Shen wrote:
> > Commit 0952177f2a1f ("thermal/core/power_allocator: Update once
> > cooling devices when temp is low") adds an update flag to avoid
> > the thermal event is triggered when there is no need, and
> > thermal cdev would be updated once when temperature is low.
> >
> > But when the trips are writable, and switch_on_temp is set
> > to be a higher value, the cooling device state may not be
> > reset to 0, because last_temperature is smaller than the
> > switch_on_temp.
> >
> > For example:
> > First:
> > switch_on_temp=3D70 control_temp=3D85;
> > Then userspace change the trip_temp:
> > switch_on_temp=3D45 control_temp=3D55 cur_temp=3D54
> >
> > Then userspace reset the trip_temp:
> > switch_on_temp=3D70 control_temp=3D85 cur_temp=3D57 last_temp=3D54
> >
> > At this time, the cooling device state should be reset to 0.
> > However, because cur_temp(57) < switch_on_temp(70)
> > last_temp(54) < switch_on_temp(70)  ---->  update =3D false,
> > update is false, the cooling device state can not be reset.
> >
> > Considering tz->passive can also be represented the temperature
> > status, this patch modifies the update flag with tz->passive.
> >
> > When the first time the temperature drops below switch_on, the
> > states of cooling devices can be reset once, and the tz->passive
> > is updated to 0. In the next round, because tz->passive is 0,
> > the cdev->state would not be updated.
> >
> > By using the tz->passive as the "update" flag, the issue above
> > can be solved, and the cooling devices can be update only once
> > when the temperature is low.
> >
> > Fixes: 0952177f2a1f ("thermal/core/power_allocator: Update once cooling=
 devices when temp is low")
> > Cc: <stable@vger.kernel.org> # v5.13+
> > Suggested-by: Wei Wang <wvw@google.com>
> > Signed-off-by: Di Shen <di.shen@unisoc.com>
> >
> > ---
> > V7:
> > - Some formatting changes.
> > - Add Suggested-by tag.
> >
> > V6: [6]
> > Compared to the previous version:
> > - Not change the thermal core.
> > - Not add new variables and function.
> > - Use tz->passive as "update" flag to indicate whether the cooling
> >    devices should be reset.
> >
> > V5: [5]
> > - Simplify the reset ops, make it no return value and no specific
> >    trip ID as argument.
> > - Extend the commit message.
> >
> > V4: [4]
> > - Compared to V3, handle it in thermal core instead of in governor.
> > - Add an ops to the governor structure, and call it when a trip
> >    point is changed.
> > - Define reset ops for power allocator.
> >
> > V3: [3]
> > - Add fix tag.
> >
> > V2: [2]
> > - Compared to v1, do not revert.
> > - Add a variable(last_switch_on_temp) in power_allocator_params
> >    to record the last switch_on_temp value.
> > - Adds a function to renew the update flag and update the
> >    last_switch_on_temp when thermal trips are writable.
> >
> > V1: [1]
> > - Revert commit 0952177f2a1f.
> >
> > [1] https://lore.kernel.org/all/20230309135515.1232-1-di.shen@unisoc.co=
m/
> > [2] https://lore.kernel.org/all/20230315093008.17489-1-di.shen@unisoc.c=
om/
> > [3] https://lore.kernel.org/all/20230320095620.7480-1-di.shen@unisoc.co=
m/
> > [4] https://lore.kernel.org/all/20230619063534.12831-1-di.shen@unisoc.c=
om/
> > [5] https://lore.kernel.org/all/20230710033234.28641-1-di.shen@unisoc.c=
om/
> > [6] https://lore.kernel.org/all/20240109112736.32566-1-di.shen@unisoc.c=
om/
> > ---
> > ---
> >   drivers/thermal/gov_power_allocator.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/go=
v_power_allocator.c
> > index 7b6aa265ff6a..81e061f183ad 100644
> > --- a/drivers/thermal/gov_power_allocator.c
> > +++ b/drivers/thermal/gov_power_allocator.c
> > @@ -762,7 +762,7 @@ static int power_allocator_throttle(struct thermal_=
zone_device *tz,
> >
> >       trip =3D params->trip_switch_on;
> >       if (trip && tz->temperature < trip->temperature) {
> > -             update =3D tz->last_temperature >=3D trip->temperature;
> > +             update =3D tz->passive;
> >               tz->passive =3D 0;
> >               reset_pid_controller(params);
> >               allow_maximum_power(tz, update);
>
> Thanks for the patch, LGTM.
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Applied as 6.8-rc1 material with modified subject and edited changelog.

Thanks!