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

From: Pierre Gondois

Date: Thu Apr 30 2026 - 09:47:45 EST

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@xxxxxxxxxx/
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@xxxxxxxxxx/

Signed-off-by: Pierre Gondois<pierre.gondois@xxxxxxx>
---
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()) {
[...]