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

[Rafael J. Wysocki]

On Wed, Mar 11, 2026 at 2:52 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Wed, Feb 25, 2026 at 9:49 AM Pierre Gondois <pierre.gondois@xxxxxxx> 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@xxxxxxx>
> > ---
> >  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.
>
> > +                       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.