Re: [PATCH v2 1/3] cpufreq: Add boost_freq_req QoS request

[Pierre Gondois]

On 12/10/25 04:01, zhenglifeng (A) wrote:
> On 2025/12/8 18:59, Pierre Gondois 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 | 28 ++++++++++++++++++++++++++++
> >   include/linux/cpufreq.h   |  1 +
> >   2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 852e024facc3c..942416f2741b0 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1359,6 +1359,11 @@ 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->boost_freq_req) {
> > +		freq_qos_remove_request(policy->boost_freq_req);
> > +		kfree(policy->boost_freq_req);
> > +	}
> > +
> >   	if (policy->max_freq_req) {
> >   		/*
> >   		 * Remove max_freq_req after sending CPUFREQ_REMOVE_POLICY
> If adding boost_freq_req fails, CPUFREQ_CREATE_POLICY notification will
> never be sent but CPUFREQ_REMOVE_POLICY notification will be sent here. So
> maybe something like this is better:

Yes right indeed.
However cf. what you suggested in patch 3/3 I believe, it might be necessary
to always set boost_freq_req, even for drivers that don't actually 
support boost
frequencies.

This might simplify this patch.

> @@ -1365,17 +1365,28 @@ 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->boost_freq_req) {
>                  /*
> -                * Remove max_freq_req after sending CPUFREQ_REMOVE_POLICY
> +                * Remove boost_freq_req after sending CPUFREQ_REMOVE_POLICY
>                   * notification, since CPUFREQ_CREATE_POLICY notification was
> -                * sent after adding max_freq_req earlier.
> +                * sent after adding 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);
>          }
>   
> +       if (policy->max_freq_req && !policy->boost_supported) {
> +               /*
> +                * Send CPUFREQ_REMOVE_POLICY notification here if
> +                * boost_freq_req is not present.
> +                */
> +               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->min_freq_req);
>          kfree(policy->min_freq_req);
>
> ---
> It's a bit verbose, but I can't think of a better way.
>
> > @@ -1476,6 +1481,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);
> > +			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;
> > +			}
> > +		}
> > +
> >   		blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
> >   				CPUFREQ_CREATE_POLICY, policy);
> >   	} else {
> > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> > index 0465d1e6f72ac..c292a6a19e4f5 100644
> > --- a/include/linux/cpufreq.h
> > +++ b/include/linux/cpufreq.h
> > @@ -81,6 +81,7 @@ struct cpufreq_policy {
> >   	struct freq_constraints	constraints;
> >   	struct freq_qos_request	*min_freq_req;
> >   	struct freq_qos_request	*max_freq_req;
> > +	struct freq_qos_request *boost_freq_req;
> >   
> >   	struct cpufreq_frequency_table	*freq_table;
> >   	enum cpufreq_table_sorting freq_table_sorted;