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

From: Nikita Travkin <nikitos.tr@gmail.com>
To: Lukasz Luba <lukasz.luba@arm.com>,
	"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
Subject: Re: [PATCH] thermal: gov_power_allocator: Allow binding without cooling devices
Date: Fri, 29 Mar 2024 18:54:33 +0500	[thread overview]
Message-ID: <1467af1e-cdeb-4fe0-ad42-407de96dcf01@gmail.com> (raw)
In-Reply-To: <3df783db-3243-4484-a429-4d3e64b9dbae@arm.com>

Hi Lukasz, Rafael!

First of all, it seems like outlook mail server that serves @arm.com refuses
to communicate with mine so I can't send anything to it and it refuses to
send anything to me either...

I'm sorry for any inconvenience this may cause and I will deal with it 
somehow...

28.03.2024 14:50, Lukasz Luba пишет:
>
>
> 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.

I'm sorry, I probably worded this poorly. I meant that there may be
a TZ that has no trip points because it's not meant to be used with
any cooling device.

>>>>
>>>> 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.

Hm you're right, I misread the commit I think and on a second thought
this patch combines two different issues. I will split it up into two.

>>>> +       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? 

As I understand it, cooling devices can attach to existing thermal zones
so it's possible that TZ is registered before any cooling devices has
attached to it. This is exactly what happens in my case: first the
IPA is probed for the TZ and after that the cooling device is attached.

check_power_actors() kerneldoc in IPA says:

> * If all of the cooling  devices currently attached to @tz implement the power
> * actor API, return the  number of them (which may be 0, because some cooling
> * devices may be  attached later). Otherwise, return -EINVAL.

and afaiu this is exactly how it's used by 
thermal_zone_device_register_with_trips():

     result = thermal_set_governor(tz, governor);
     if (result) {
mutex_unlock(&thermal_governor_lock);
         goto unregister;
     }
     (...)
     /* Bind cooling devices for this zone */
     bind_tz(tz);

>>>> 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.

Yes, I understand that IPA is useless without trip points and/or cooling
devices. However from the snippet above the TZ won't be registered if
the governor fails to probe. This means that if one has IPA set as the
default, thermal core will try to probe it for TZ without trip points and,
when it fails, will fail to probe the whole TZ.

get_governor_trips() kerneldoc in IPA says:

 > * on.  If there are no passive or active trip points, then the
 > * governor won't do anything.  In fact, its throttle function
 > * won't be called at all.

Which suggests to me that being bound to such TZ is expected.

Unless you have any thoughts on how to solve this better, I will split
this into two commits and resubmit in a few days:

  - Allow binding without cooling devices (allow -EINVAL after allocate)
  - Allow binding without trip points (Rest of this patch)

Nikita

>>>> + 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>