linux-kernel - Re: [PATCH] thermal: gov_power_allocator: Allow binding without cooling devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1bfcb1cf-fc08-404b-be36-b5e1863f7c59@arm.com>
Date: Thu, 28 Mar 2024 09:12:10 +0000
From: Lukasz Luba <lukasz.luba@....com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Daniel Lezcano <daniel.lezcano@...aro.org>, nikita@...n.ru,
 Zhang Rui <rui.zhang@...el.com>,
 "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>, linux-pm@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] thermal: gov_power_allocator: Allow binding without
 cooling devices

Hi Rafael,

On 3/27/24 15:25, Rafael J. Wysocki wrote:
> On Thu, Mar 21, 2024 at 3:44 PM Nikita Travkin <nikita@...n.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.
>>
>> 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")
>> Signed-off-by: Nikita Travkin <nikita@...n.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");
>> -               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) {
>> +       if (ret && ret != -EINVAL) {
>>                  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)
>> +               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@...n.ru>
>>