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

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

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,
so whatever seems is better to both of you.


>>> +                       if (!policy->boost_freq_req) {
>>> +                               ret = -ENOMEM;
>>> +                               goto out_destroy_policy;
>>> +                       }
>>> +
>>> +                       ret = freq_qos_add_request(&policy->constraints,
>>> +                                                  policy->boost_freq_req,
>>> +                                                  FREQ_QOS_MAX,
>>> +                                                  FREQ_QOS_MAX_DEFAULT_VALUE);
>>> +                       if (ret < 0) {
>>> +                               /*
>>> +                                * So we don't call freq_qos_remove_request() for an
>>> +                                * uninitialized request.
>>> +                                */
>>> +                               kfree(policy->boost_freq_req);
>>> +                               policy->boost_freq_req = NULL;
>>> +                               goto out_destroy_policy;
>>> +                       }
>>> +               }
>> Then you can put this block before the policy->min_freq_req addition
>> and the corresponding update of cpufreq_policy_free() will be more
>> straightforward.
>>
>> BTW, if I'm not mistaken, there is a bug in the current
>> cpufreq_policy_free() because it may attempt to remove a NULL freq QoS
>> request for policy->min_freq_req.  I'll send a patch for that later if
>> I can confirm it.
> Fortunately, freq_qos_remove_request() checks its argument against
> NULL, never mind.
>
> Of course, this means that
> freq_qos_remove_request(policy->max_freq_req) need not be there under
> the if (policy->max_freq_req) in cpufreq_policy_free() which you have
> noticed.