Re: [PATCH v5 2/6] cpufreq: Add boost_freq_req QoS request

[Pierre Gondois <pierre.gondois@arm.com>]

On 3/13/26 16:31, Rafael J. Wysocki wrote:
> On Fri, Mar 13, 2026 at 3:30 PM Pierre Gondois <pierre.gondois@arm.com> wrote:
>>
>> On 3/11/26 14:57, Rafael J. Wysocki wrote:
>>> On Wed, Mar 11, 2026 at 2:52 PM Rafael J. Wysocki<rafael@kernel.org> wrote:
>>>> On Wed, Feb 25, 2026 at 9:49 AM Pierre Gondois<pierre.gondois@arm.com> wrote:
>>>>> The Power Management Quality of Service (PM QoS) allows to
>>>>> aggregate constraints from multiple entities. It is currently
>>>>> used to manage the min/max frequency of a given policy.
>>>>>
>>>>> Frequency constraints can come for instance from:
>>>>> - Thermal framework: acpi_thermal_cpufreq_init()
>>>>> - Firmware: _PPC objects: acpi_processor_ppc_init()
>>>>> - User: by setting policyX/scaling_[min|max]_freq
>>>>> The minimum of the max frequency constraints is used to compute
>>>>> the resulting maximum allowed frequency.
>>>>>
>>>>> When enabling boost frequencies, the same frequency request object
>>>>> (policy->max_freq_req) as to handle requests from users is used.
>>>>> As a result, when setting:
>>>>> - scaling_max_freq
>>>>> - boost
>>>>> The last sysfs file used overwrites the request from the other
>>>>> sysfs file.
>>>>>
>>>>> To avoid this, create a per-policy boost_freq_req to save the boost
>>>>> constraints instead of overwriting the last scaling_max_freq
>>>>> constraint.
>>>>>
>>>>> Signed-off-by: Pierre Gondois<pierre.gondois@arm.com>
>>>>> ---
>>>>>    drivers/cpufreq/cpufreq.c | 37 ++++++++++++++++++++++++++++++++-----
>>>>>    include/linux/cpufreq.h   |  1 +
>>>>>    2 files changed, 33 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>>>>> index db414c052658b..50467b938668a 100644
>>>>> --- a/drivers/cpufreq/cpufreq.c
>>>>> +++ b/drivers/cpufreq/cpufreq.c
>>>>> @@ -1359,17 +1359,21 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>>>>>           /* Cancel any pending policy->update work before freeing the policy. */
>>>>>           cancel_work_sync(&policy->update);
>>>>>
>>>>> -       if (policy->max_freq_req) {
>>>>> +       if ((policy->max_freq_req && !policy->boost_supported) ||
>>>>> +           policy->boost_freq_req) {
>>>> Are you sure?
>>>>
>>>>>                   /*
>>>>> -                * Remove max_freq_req after sending CPUFREQ_REMOVE_POLICY
>>>>> -                * notification, since CPUFREQ_CREATE_POLICY notification was
>>>>> -                * sent after adding max_freq_req earlier.
>>>>> +                * Remove max/boost _freq_req after sending CPUFREQ_REMOVE_POLICY
>>>>> +                * notification, since CPUFREQ_CREATE_POLICY notification was sent
>>>>> +                * after adding max/boost _freq_req earlier.
>>>>>                    */
>>>>>                   blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
>>>>>                                                CPUFREQ_REMOVE_POLICY, policy);
>>>>> -               freq_qos_remove_request(policy->max_freq_req);
>>>>>           }
>>>>>
>>>>> +       freq_qos_remove_request(policy->boost_freq_req);
>>>>> +       kfree(policy->boost_freq_req);
>>>>> +
>>>>> +       freq_qos_remove_request(policy->max_freq_req);
>>>>>           freq_qos_remove_request(policy->min_freq_req);
>>>>>           kfree(policy->min_freq_req);
>>>>>
>>>>> @@ -1479,6 +1483,29 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
>>>>>                           goto out_destroy_policy;
>>>>>                   }
>>>>>
>>>>> +               if (policy->boost_supported) {
>>>>> +                       policy->boost_freq_req = kzalloc(sizeof(*policy->boost_freq_req),
>>>>> +                                                        GFP_KERNEL);
>>>> Instead of doing this, why don't you add 1 to the policy->min_freq_req
>>>> allocation size when boost is supported?  You'll destroy the policy if
>>>> the allocation fails anyway.
>>
>> Yes right,
>> this would make the error handling much simpler to follow.
>>
>> On the other hand Viresh seems to have acknowledged the logic,
> But he has also said "+1" to this one. :-)

ah right indeed


>
>> so whatever seems is better to both of you.
> I think we agree.