Re: [PATCH 1/1] cpufreq: Set policy->min and max as real QoS constraints

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

Hello Jie,

On 4/27/26 05:08, Jie Zhan wrote:
> Hi Pierre,
>
> Thanks for updating this.
> A few questions inline.
>
> Regards,
> Jie
>
> On 4/23/2026 4:47 PM, Pierre Gondois wrote:
>> cpufreq_set_policy() will ultimately override the policy min/max
>> values written in the .init() callback through:
>> cpufreq_policy_online()
>> \-cpufreq_init_policy()
>>    \-cpufreq_set_policy()
>>      \-/* Set policy->min/max */
>> Thus the policy min/max values provided are only temporary.
>>
>> There is an exception if CPUFREQ_NEED_INITIAL_FREQ_CHECK is set and:
> Just a check, you mean this is the only place that policy->min/max set by
> driver->init() may ever be actually used, right?
I think I meant that this is the only place which might use

policy->min or max between:

- cpufreq_driver->init ();  ## which set policy->min/max

- cpufreq_set_policy(); ## which reset policy->min/max

so the min/max values set by the driver .init() callback
should only be used in the above call stack.

------

This patch sets policy->min/max values with:

/*
If the driver didn't set QoS constraints, policy->min/max still
need to be set as they are used to clamp frequency requests.
*/

So even if there are other users in the meantime, they
should have correct values.


>> cpufreq_policy_online()
>> \-cpufreq_init_policy()
>>    \-__cpufreq_driver_target()
>>      \-cpufreq_driver->target()
> cpufreq_init_policy() doesn't seem to be involved here?
> It's supposed to be:
> cpufreq_policy_online()
> \-__cpufreq_driver_target() /* CPUFREQ_NEED_INITIAL_FREQ_CHECK branch */
>    \-cpufreq_driver->target()
Yes right indeed, cpufreq_init_policy() should be removed.
>> is called. To avoid any regression, set policy->min/max in cpufreq.c
>> if the values were not initialized.
>>
>> In this patch:
>> - Setting policy->min or max value in driver .init() cb is
>>    interpreted as setting a QoS constraint.
>> - Remove policy->min/max initialization in drivers if the values
>>    are similar to policy->cpuinfo.min_freq/max_freq.
> Why is this necessary?
> Doing this will touch many drivers.
> Is this mainly for cleaning up? or is there any bugs if we directly take
> the existing policy->min/max initialized by drivers (mostly equal to
> cpufreq_min/max_freq) as QoS constraints?
I think I should also mention the following commit in the
message:

521223d8b3ec ("cpufreq: Fix initialization of min and max frequency QoS 
requests")

Prior to that commit, drivers were setting policy->min/max and
the value was used as a QoS constraint. After that, the value
was only temporarily used. Thus this commit helps coming
back to the first behaviour.

Pengjie Zhang recently wanted to be able to set a default min frequency
for the cppc driver at [1]. So this also helps for his case.

[1] 
https://lore.kernel.org/lkml/20260213100633.15413-1-zhangpengjie2@huawei.com/ 

>>    The only drivers where these values are different are:
>>    - gx-suspmod.c
>>    - cppc-cpufreq.c
>>    - longrun.c
>> - For the cppc-cpufreq driver, the lowest non-linear freq. is
>>    used as a min QoS constraint as suggested at:
>>    https://lore.kernel.org/lkml/20260213100633.15413-1-zhangpengjie2@huawei.com/
>>
>> Signed-off-by: Pierre Gondois<pierre.gondois@arm.com>
>> ---
>>   drivers/cpufreq/amd-pstate.c      | 16 ++++++++--------
>>   drivers/cpufreq/cppc_cpufreq.c    | 11 +++++++----
>>   drivers/cpufreq/cpufreq-nforce2.c |  4 ++--
>>   drivers/cpufreq/cpufreq.c         | 19 +++++++++++++++++--
>>   drivers/cpufreq/freq_table.c      |  7 +++----
>>   drivers/cpufreq/gx-suspmod.c      |  9 +++++----
>>   drivers/cpufreq/intel_pstate.c    |  3 ---
>>   drivers/cpufreq/pcc-cpufreq.c     |  8 ++++----
>>   drivers/cpufreq/pxa3xx-cpufreq.c  |  4 ++--
>>   drivers/cpufreq/sh-cpufreq.c      |  4 ++--
>>   drivers/cpufreq/virtual-cpufreq.c |  5 +----
>>   11 files changed, 51 insertions(+), 39 deletions(-)
>>
> [...]
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 44eb1b7e7fc1b..b30bfa3e27daa 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1453,6 +1453,14 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
>>   	cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
>>   
>>   	if (new_policy) {
>> +		unsigned int min, max;
>> +
>> +		/* Use policy->min/max set by the driver as QoS requests. */
>> +		min = max(FREQ_QOS_MIN_DEFAULT_VALUE, policy->min);
>> +		if (policy->max)
>> +			max = min(FREQ_QOS_MAX_DEFAULT_VALUE, policy->max);
>> +		else
>> +			max = FREQ_QOS_MAX_DEFAULT_VALUE;
> Is it practical (free of bugs) to set the default policy->min/max to
> FREQ_QOS_MIN/MAX_DEFAULT_VALUE in cpufreq_policy_alloc(), and keep drivers
> initializing policy->min/max?
>
> Such that we may only need to change the following two lines of
> FREQ_QOS_MIN/MAX_DEFAULT_VALUE to policy->min/max in this function, without
> adding the other two trunks.

One other goal of the patch is to distinguish drivers which:
1. actually requested a QoS min/max limit
2. have set min/max values by default, but don't actually meant it.

Cf. 521223d8b3ec ("cpufreq: Fix initialization of min and max frequency 
QoS requests")
Setting policy->max as a QoS constraint might result in performance
limitations, so we should try to avoid that.

FWIU, removing the default initialization of policy->min/max
must be done to handle case 2., but maybe I missed something.


>>   		for_each_cpu(j, policy->related_cpus) {
>>   			per_cpu(cpufreq_cpu_data, j) = policy;
>>   			add_cpu_dev_symlink(policy, j, get_cpu_device(j));
>> @@ -1469,18 +1477,25 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
>>   
>>   		ret = freq_qos_add_request(&policy->constraints,
>>   					   &policy->min_freq_req, FREQ_QOS_MIN,
>> -					   FREQ_QOS_MIN_DEFAULT_VALUE);
>> +					   min);
>>   		if (ret < 0)
>>   			goto out_destroy_policy;
>>   
>>   		ret = freq_qos_add_request(&policy->constraints,
>>   					   &policy->max_freq_req, FREQ_QOS_MAX,
>> -					   FREQ_QOS_MAX_DEFAULT_VALUE);
>> +					   max);
>>   		if (ret < 0)
>>   			goto out_destroy_policy;
>>   
>>   		blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
>>   				CPUFREQ_CREATE_POLICY, policy);
>> +
>> +		/*
>> +		 * If the driver didn't set QoS constraints, policy->min/max still
>> +		 * need to be set as they are used to clamp frequency requests.
>> +		 */
>> +		policy->min = policy->min ? policy->min : policy->cpuinfo.min_freq;
>> +		policy->max = policy->max ? policy->max : policy->cpuinfo.max_freq;
>>   	}
>>   
>>   	if (cpufreq_driver->get && has_target()) {
> [...]