Received-SPF: pass (google.com: domain of linux-kernel+bounces-122720-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <3df783db-3243-4484-a429-4d3e64b9dbae@arm.com>
Date: Thu, 28 Mar 2024 09:50:14 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] thermal: gov_power_allocator: Allow binding without
 cooling devices
Content-Language: en-US
From: Lukasz Luba <lukasz.luba@arm.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>, nikita@trvn.ru
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>,
 Zhang Rui <rui.zhang@intel.com>,
 "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>, linux-pm@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240321-gpa-no-cooling-devs-v1-1-5c9e0ef2062e@trvn.ru>
 <CAJZ5v0jAq=iMKzYBz-Ni6Zqpbgxp3_0UYpPiAoSLcfGNJ8ruhQ@mail.gmail.com>
 <1bfcb1cf-fc08-404b-be36-b5e1863f7c59@arm.com>
In-Reply-To: <1bfcb1cf-fc08-404b-be36-b5e1863f7c59@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 3/28/24 09:12, Lukasz Luba wrote:
> Hi Rafael,
> 
> On 3/27/24 15:25, Rafael J. Wysocki wrote:
>> On Thu, Mar 21, 2024 at 3:44 PM Nikita Travkin <nikita@trvn.ru> wrote:
>>>
>>> Commit e83747c2f8e3 ("thermal: gov_power_allocator: Set up trip 
>>> points earlier")
>>> added a check that would fail binding the governer if there is no
>>> cooling devices bound to the thermal zone. Unfortunately this causes
>>> issues in cases when the TZ is bound to the governer before the cooling
>>> devices are attached to it. (I.e. when the tz is registered using
>>> thermal_zone_device_register_with_trips().)
>>>
>>> Additionally, the documentation across gov_power_allocator suggests it's
>>> intended to allow it to be bound to thermal zones without cooling
>>> devices (and thus without passive/active trip points), however the same
>>> change added a check for the trip point to be present, causing those TZ
>>> to fail probing.

This patch description is mixing trips and cooling devices and refers to
a commit which is only for guarding the trip points number to be
not less than 2. In IPA we require 2 trip points.

>>>
>>> Those changes cause all thermal zones to fail on some devices (such as
>>> sc7180-acer-aspire1) and prevent the kernel from controlling the cpu/gpu
>>> frequency based on the temperature, as well as losing all the other
>>> "informational" thermal zones if power_allocator is set as default.
>>>
>>> This commit partially reverts the referenced one by dropping the trip
>>> point check and by allowing the TZ to probe even if no actor buffer was
>>> allocated to allow those TZ to probe again.
>>>
>>> Fixes: e83747c2f8e3 ("thermal: gov_power_allocator: Set up trip 
>>> points earlier")

Not that commit.

>>> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
>>> ---
>>> I've noticed that all thermal zones fail probing with -EINVAL on my
>>> sc7180 based Acer Aspire 1 since 6.8. This commit allows me to bring
>>> them back.
>>
>> Łukasz, any comments?
> 
> Let me check this today.
> 
>>
>>> ---
>>>   drivers/thermal/gov_power_allocator.c | 14 +++++---------
>>>   1 file changed, 5 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/thermal/gov_power_allocator.c 
>>> b/drivers/thermal/gov_power_allocator.c
>>> index 1b17dc4c219c..4f2d7f3b7508 100644
>>> --- a/drivers/thermal/gov_power_allocator.c
>>> +++ b/drivers/thermal/gov_power_allocator.c
>>> @@ -679,11 +679,6 @@ static int power_allocator_bind(struct 
>>> thermal_zone_device *tz)
>>>                  return -ENOMEM;
>>>
>>>          get_governor_trips(tz, params);
>>> -       if (!params->trip_max) {
>>> -               dev_warn(&tz->device, "power_allocator: missing 
>>> trip_max\n");

This if() guards the binding of TZ with less than 2 trip points,
not the cooling devices.

>>> -               kfree(params);
>>> -               return -EINVAL;
>>> -       }
>>>
>>>          ret = check_power_actors(tz, params);
>>>          if (ret < 0) {
>>> @@ -693,7 +688,7 @@ static int power_allocator_bind(struct 
>>> thermal_zone_device *tz)
>>>          }
>>>
>>>          ret = allocate_actors_buffer(params, ret);
>>> -       if (ret) {

This if() is from different commit.

>>> +       if (ret && ret != -EINVAL) {

This is about 0 cooling devices in the thermal zone, but IPA won't work
so why to even fake and forward binding?

Rafael should we support binding with 0 cooling devices?

>>>                  dev_warn(&tz->device, "power_allocator: allocation 
>>> failed\n");
>>>                  kfree(params);
>>>                  return ret;
>>> @@ -714,9 +709,10 @@ static int power_allocator_bind(struct 
>>> thermal_zone_device *tz)
>>>          else
>>>                  params->sustainable_power = tz->tzp->sustainable_power;
>>>
>>> -       estimate_pid_constants(tz, tz->tzp->sustainable_power,
>>> -                              params->trip_switch_on,
>>> -                              params->trip_max->temperature);
>>> +       if (params->trip_max)

This is not supported, we need those 2 trip points.

>>> +               estimate_pid_constants(tz, tz->tzp->sustainable_power,
>>> +                                      params->trip_switch_on,
>>> +                                      params->trip_max->temperature);
>>>
>>>          reset_pid_controller(params);
>>>
>>>
>>> ---
>>> base-commit: e7528c088874326d3060a46f572252be43755a86
>>> change-id: 20240321-gpa-no-cooling-devs-c79ee3288325
>>>
>>> Best regards,
>>> -- 
>>> Nikita Travkin <nikita@trvn.ru>
>>>
>