MIME-Version: 1.0
References: <20230309135515.1232-1-di.shen@unisoc.com>
In-Reply-To: <20230309135515.1232-1-di.shen@unisoc.com>
From:   Di Shen <cindygm567@gmail.com>
Date:   Fri, 10 Mar 2023 15:31:28 +0800
Message-ID: <CAHYJL4obTjdYny4vGTfo89gmv7oKD8e=FzSdvDsL=Q+Z+cz_6g@mail.gmail.com>
Subject: Re: [PATCH] thermal/core/power_allocator: avoid cdev->state can not
 be reset
To:     Di Shen <di.shen@unisoc.com>
Cc:     lukasz.luba@arm.com, rafael@kernel.org, daniel.lezcano@linaro.org,
        amitk@kernel.org, rui.zhang@intel.com, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, xuewen.yan@unisoc.com,
        "qyousef@layalina.io" <qyousef@layalina.io>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi all,
any comments for this patch? Looking forward to your reply. Thank you.

--=20
di


On Thu, Mar 9, 2023 at 10:05=E2=80=AFPM Di Shen <di.shen@unisoc.com> wrote:
>
> Commit 0952177f2a1f (thermal/core/power_allocator: Update once cooling de=
vices when temp is low)
> add a update flag to update cooling device only once when temp is low.
> But when the switch_on_temp is set to be a higher value, the cooling devi=
ce state
> may not be reset to max, because the last_temp is smaller than the switch=
_on_temp.
>
> For example:
> First:
> swicth_on_temp=3D70 control_temp=3D85;
>
> Then userspace change the trip_temp:
> swicth_on_temp=3D45 control_temp=3D55 cur_temp=3D54
>
> Then userspace reset the trip_temp:
> swicth_on_temp=3D70 control_temp=3D85 cur_temp=3D57 last_temp=3D54
>
> At this time, the cooling device state should be reset to be max.
> However, because cur_temp(57) < switch_on_temp(70)
> last_temp(54) < swicth_on_temp(70) --> update =3D false
> When update is false, the cooling device state can not be reset.
>
> So delete the update condition, so that the cooling device state
> could be reset.
>
> Fixes: 0952177f2a1f (thermal/core/power_allocator: Update once cooling de=
vices when temp is low)
> Signed-off-by: Di Shen <di.shen@unisoc.com>
> ---
>  drivers/thermal/gov_power_allocator.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_=
power_allocator.c
> index 0eaf1527d3e3..153bf528b98c 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -560,7 +560,7 @@ static void reset_pid_controller(struct power_allocat=
or_params *params)
>         params->prev_err =3D 0;
>  }
>
> -static void allow_maximum_power(struct thermal_zone_device *tz, bool upd=
ate)
> +static void allow_maximum_power(struct thermal_zone_device *tz)
>  {
>         struct thermal_instance *instance;
>         struct power_allocator_params *params =3D tz->governor_data;
> @@ -582,8 +582,7 @@ static void allow_maximum_power(struct thermal_zone_d=
evice *tz, bool update)
>                  */
>                 cdev->ops->get_requested_power(cdev, &req_power);
>
> -               if (update)
> -                       __thermal_cdev_update(instance->cdev);
> +               __thermal_cdev_update(instance->cdev);
>
>                 mutex_unlock(&instance->cdev->lock);
>         }
> @@ -697,7 +696,6 @@ static int power_allocator_throttle(struct thermal_zo=
ne_device *tz, int trip_id)
>         struct power_allocator_params *params =3D tz->governor_data;
>         struct thermal_trip trip;
>         int ret;
> -       bool update;
>
>         lockdep_assert_held(&tz->lock);
>
> @@ -710,10 +708,9 @@ static int power_allocator_throttle(struct thermal_z=
one_device *tz, int trip_id)
>
>         ret =3D __thermal_zone_get_trip(tz, params->trip_switch_on, &trip=
);
>         if (!ret && (tz->temperature < trip.temperature)) {
> -               update =3D (tz->last_temperature >=3D trip.temperature);
>                 tz->passive =3D 0;
>                 reset_pid_controller(params);
> -               allow_maximum_power(tz, update);
> +               allow_maximum_power(tz);
>                 return 0;
>         }
>
> --
> 2.17.1
>