Re: [PATCH v3 3/4] cpufreq: Remove driver default policy->min/max init

[Rafael J. Wysocki]

On Thu, May 28, 2026 at 11:10 AM Pierre Gondois <pierre.gondois@xxxxxxx> wrote:
>
> Prior to [1], drivers were setting policy->min/max and
> the value was used as a QoS constraint. After that change,
> the values were only temporarily used: cpufreq_set_policy()
> ultimately overriding them through:
> cpufreq_policy_online()
> \-cpufreq_init_policy()
>   \-cpufreq_set_policy()
>     \-/* Set policy->min/max */
>
> This patch reinstate the initial behaviour. This will allow
> drivers to request min/max QoS frequencies if desired.
> For instance, the cppc driver advertises a lowest non-linear
> frequency, which should be used as a min QoS value.
>
> To avoid having drivers setting policy->min/max to default
> values which are considered as QoS values (i.e. the reason
> why [1] was introduced), remove the initialization of
> policy->min/max in .init() callbacks wherever the
> policy->min/max values are identical to the
> policy->cpuinfo.min/max_freq.
>
> Indeed, the previous patch ("cpufreq: Set default
> policy->min/max values for all drivers") makes this initialization
> redundant.
>
> The only drivers where these values are different are:
> - gx-suspmod.c (min)
> - cppc-cpufreq.c (min)
> - longrun.c
>
> [1]
> commit 521223d8b3ec ("cpufreq: Fix initialization of min and
> max frequency QoS requests")
>
> Signed-off-by: Pierre Gondois <pierre.gondois@xxxxxxx>
> Acked-by: Jie Zhan <zhanjie9@xxxxxxxxxxxxx>

sashiko.dev has some feedback on this patch and appears to have a point:

https://sashiko.dev/#/patchset/20260528090913.2759118-1-pierre.gondois%40arm.com

Can you have a look at it please?

> ---
>  drivers/cpufreq/amd-pstate.c      | 14 ++++++--------
>  drivers/cpufreq/cppc_cpufreq.c    |  5 ++---
>  drivers/cpufreq/cpufreq-nforce2.c |  4 ++--
>  drivers/cpufreq/freq_table.c      |  7 +++----
>  drivers/cpufreq/gx-suspmod.c      |  2 +-
>  drivers/cpufreq/intel_pstate.c    |  3 ---
>  drivers/cpufreq/pcc-cpufreq.c     | 10 ++++------
>  drivers/cpufreq/pxa3xx-cpufreq.c  |  5 ++---
>  drivers/cpufreq/sh-cpufreq.c      |  6 ++----
>  drivers/cpufreq/virtual-cpufreq.c |  5 +----
>  10 files changed, 23 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 62b5d995281d2..2f0bb732ea173 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1086,10 +1086,9 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>
>         perf = READ_ONCE(cpudata->perf);
>
> -       policy->cpuinfo.min_freq = policy->min = perf_to_freq(perf,
> -                                                             cpudata->nominal_freq,
> -                                                             perf.lowest_perf);
> -       policy->cpuinfo.max_freq = policy->max = cpudata->max_freq;
> +       policy->cpuinfo.min_freq = perf_to_freq(perf, cpudata->nominal_freq,
> +                                               perf.lowest_perf);
> +       policy->cpuinfo.max_freq = cpudata->max_freq;
>
>         policy->driver_data = cpudata;
>         ret = amd_pstate_cppc_enable(policy);
> @@ -1915,10 +1914,9 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>
>         perf = READ_ONCE(cpudata->perf);
>
> -       policy->cpuinfo.min_freq = policy->min = perf_to_freq(perf,
> -                                                             cpudata->nominal_freq,
> -                                                             perf.lowest_perf);
> -       policy->cpuinfo.max_freq = policy->max = cpudata->max_freq;
> +       policy->cpuinfo.min_freq = perf_to_freq(perf, cpudata->nominal_freq,
> +                                               perf.lowest_perf);
> +       policy->cpuinfo.max_freq = cpudata->max_freq;
>         policy->driver_data = cpudata;
>
>         ret = amd_pstate_cppc_enable(policy);
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 7e7f9dfb7a24c..5abac50df7508 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -660,8 +660,6 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>          * Section 8.4.7.1.1.5 of ACPI 6.1 spec)
>          */
>         policy->min = cppc_perf_to_khz(caps, caps->lowest_nonlinear_perf);
> -       policy->max = cppc_perf_to_khz(caps, policy->boost_enabled ?
> -                                               caps->highest_perf : caps->nominal_perf);
>
>         /*
>          * Set cpuinfo.min_freq to Lowest to make the full range of performance
> @@ -669,7 +667,8 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>          * nonlinear perf
>          */
>         policy->cpuinfo.min_freq = cppc_perf_to_khz(caps, caps->lowest_perf);
> -       policy->cpuinfo.max_freq = policy->max;
> +       policy->cpuinfo.max_freq = cppc_perf_to_khz(caps, policy->boost_enabled ?
> +                                                   caps->highest_perf : caps->nominal_perf);
>
>         policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu);
>         policy->shared_type = cpu_data->shared_type;
> diff --git a/drivers/cpufreq/cpufreq-nforce2.c b/drivers/cpufreq/cpufreq-nforce2.c
> index fbbbe501cf2dc..831102522ad64 100644
> --- a/drivers/cpufreq/cpufreq-nforce2.c
> +++ b/drivers/cpufreq/cpufreq-nforce2.c
> @@ -355,8 +355,8 @@ static int nforce2_cpu_init(struct cpufreq_policy *policy)
>                 min_fsb = NFORCE2_MIN_FSB;
>
>         /* cpuinfo and default policy values */
> -       policy->min = policy->cpuinfo.min_freq = min_fsb * fid * 100;
> -       policy->max = policy->cpuinfo.max_freq = max_fsb * fid * 100;
> +       policy->cpuinfo.min_freq = min_fsb * fid * 100;
> +       policy->cpuinfo.max_freq = max_fsb * fid * 100;
>
>         return 0;
>  }
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index 5b364d8da4f92..ea994647abc88 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -49,16 +49,15 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy)
>                         max_freq = freq;
>         }
>
> -       policy->min = policy->cpuinfo.min_freq = min_freq;
> -       policy->max = max_freq;
> +       policy->cpuinfo.min_freq = min_freq;
>         /*
>          * If the driver has set its own cpuinfo.max_freq above max_freq, leave
>          * it as is.
>          */
>         if (policy->cpuinfo.max_freq < max_freq)
> -               policy->max = policy->cpuinfo.max_freq = max_freq;
> +               policy->cpuinfo.max_freq = max_freq;
>
> -       if (policy->min == ~0)
> +       if (min_freq == ~0)
>                 return -EINVAL;
>         else
>                 return 0;
> diff --git a/drivers/cpufreq/gx-suspmod.c b/drivers/cpufreq/gx-suspmod.c
> index d269a4f26f98e..d40c9e0bbb740 100644
> --- a/drivers/cpufreq/gx-suspmod.c
> +++ b/drivers/cpufreq/gx-suspmod.c
> @@ -421,7 +421,7 @@ static int cpufreq_gx_cpu_init(struct cpufreq_policy *policy)
>                 policy->min = maxfreq / max_duration;
>         else
>                 policy->min = maxfreq / POLICY_MIN_DIV;
> -       policy->max = maxfreq;
> +
>         policy->cpuinfo.min_freq = maxfreq / max_duration;
>         policy->cpuinfo.max_freq = maxfreq;
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 1f093e3464301..dfc1e321f9e02 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -3049,9 +3049,6 @@ static int __intel_pstate_cpu_init(struct cpufreq_policy *policy)
>         policy->cpuinfo.max_freq = READ_ONCE(global.no_turbo) ?
>                         cpu->pstate.max_freq : cpu->pstate.turbo_freq;
>
> -       policy->min = policy->cpuinfo.min_freq;
> -       policy->max = policy->cpuinfo.max_freq;
> -
>         intel_pstate_init_acpi_perf_limits(policy);
>
>         policy->fast_switch_possible = true;
> diff --git a/drivers/cpufreq/pcc-cpufreq.c b/drivers/cpufreq/pcc-cpufreq.c
> index ac2e90a65f0c4..0f185a13577f8 100644
> --- a/drivers/cpufreq/pcc-cpufreq.c
> +++ b/drivers/cpufreq/pcc-cpufreq.c
> @@ -551,13 +551,11 @@ static int pcc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>                 goto out;
>         }
>
> -       policy->max = policy->cpuinfo.max_freq =
> -               ioread32(&pcch_hdr->nominal) * 1000;
> -       policy->min = policy->cpuinfo.min_freq =
> -               ioread32(&pcch_hdr->minimum_frequency) * 1000;
> +       policy->cpuinfo.max_freq = ioread32(&pcch_hdr->nominal) * 1000;
> +       policy->cpuinfo.min_freq = ioread32(&pcch_hdr->minimum_frequency) * 1000;
>
> -       pr_debug("init: policy->max is %d, policy->min is %d\n",
> -               policy->max, policy->min);
> +       pr_debug("init: max_freq is %d, min_freq is %d\n",
> +                policy->cpuinfo.max_freq, policy->cpuinfo.min_freq);
>  out:
>         return result;
>  }
> diff --git a/drivers/cpufreq/pxa3xx-cpufreq.c b/drivers/cpufreq/pxa3xx-cpufreq.c
> index 50ff3b6a69000..06b27cbc59d6a 100644
> --- a/drivers/cpufreq/pxa3xx-cpufreq.c
> +++ b/drivers/cpufreq/pxa3xx-cpufreq.c
> @@ -185,9 +185,8 @@ static int pxa3xx_cpufreq_init(struct cpufreq_policy *policy)
>         int ret = -EINVAL;
>
>         /* set default policy and cpuinfo */
> -       policy->min = policy->cpuinfo.min_freq = 104000;
> -       policy->max = policy->cpuinfo.max_freq =
> -               (cpu_is_pxa320()) ? 806000 : 624000;
> +       policy->cpuinfo.min_freq = 104000;
> +       policy->cpuinfo.max_freq = (cpu_is_pxa320()) ? 806000 : 624000;
>         policy->cpuinfo.transition_latency = 1000; /* FIXME: 1 ms, assumed */
>
>         if (cpu_is_pxa300() || cpu_is_pxa310())
> diff --git a/drivers/cpufreq/sh-cpufreq.c b/drivers/cpufreq/sh-cpufreq.c
> index 642ddb9ea217e..3c99d7009cbe2 100644
> --- a/drivers/cpufreq/sh-cpufreq.c
> +++ b/drivers/cpufreq/sh-cpufreq.c
> @@ -124,10 +124,8 @@ static int sh_cpufreq_cpu_init(struct cpufreq_policy *policy)
>                 dev_notice(dev, "no frequency table found, falling back "
>                            "to rate rounding.\n");
>
> -               policy->min = policy->cpuinfo.min_freq =
> -                       (clk_round_rate(cpuclk, 1) + 500) / 1000;
> -               policy->max = policy->cpuinfo.max_freq =
> -                       (clk_round_rate(cpuclk, ~0UL) + 500) / 1000;
> +               policy->cpuinfo.min_freq = (clk_round_rate(cpuclk, 1) + 500) / 1000;
> +               policy->cpuinfo.max_freq = (clk_round_rate(cpuclk, ~0UL) + 500) / 1000;
>         }
>
>         return 0;
> diff --git a/drivers/cpufreq/virtual-cpufreq.c b/drivers/cpufreq/virtual-cpufreq.c
> index 4159f31349b16..dc78b74409af4 100644
> --- a/drivers/cpufreq/virtual-cpufreq.c
> +++ b/drivers/cpufreq/virtual-cpufreq.c
> @@ -164,10 +164,7 @@ static int virt_cpufreq_get_freq_info(struct cpufreq_policy *policy)
>                 policy->cpuinfo.min_freq = 1;
>                 policy->cpuinfo.max_freq = virt_cpufreq_get_perftbl_entry(policy->cpu, 0);
>
> -               policy->min = policy->cpuinfo.min_freq;
> -               policy->max = policy->cpuinfo.max_freq;
> -
> -               policy->cur = policy->max;
> +               policy->cur = policy->cpuinfo.max_freq;
>                 return 0;
>         }
>
> --
> 2.43.0
>