Re: [PATCH v5 4/6] cpufreq: Update .set_boost() callbacks to rely on boost_freq_req

[Rafael J. Wysocki]

On Fri, Mar 13, 2026 at 10:38 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> On 11-03-26, 17:19, Rafael J. Wysocki wrote:
> > On Wed, Feb 25, 2026 at 9:50 AM Pierre Gondois <pierre.gondois@xxxxxxx> wrote:
> > >
> > > In the existing .set_boost() callbacks:
> > > - Don't update policy->max as this is done through the qos notifier
> > >   cpufreq_notifier_max() which calls cpufreq_set_policy().
> > > - Remove freq_qos_update_request() calls as the qos request is now
> > >   done in policy_set_boost() and updates the new boost_freq_req
> > >
> > > Note:
> > > cpufreq_frequency_table_cpuinfo() is also called through:
> > > cpufreq_policy_online()
> > > \-cpufreq_table_validate_and_sort()
> > >   \-cpufreq_frequency_table_cpuinfo()
> > > which relies on cpufreq_frequency_table_cpuinfo() to set
> > > policy->min and max initizalization at driver init. This
> > > regression is solved in the next patch.
> > >
> > > Note2:
> > > acpi-cpufreq.c seems to be the only cpufreq driver not
> > > setting cpuinfo.max_freq. Populate it the nominal frequency at
> > > driver init.
> > >
> > > Signed-off-by: Pierre Gondois <pierre.gondois@xxxxxxx>
> > > ---
> > >  drivers/cpufreq/acpi-cpufreq.c |  1 +
> > >  drivers/cpufreq/amd-pstate.c   |  2 --
> > >  drivers/cpufreq/cppc_cpufreq.c | 10 ++--------
> > >  drivers/cpufreq/cpufreq.c      | 16 +++++++---------
> > >  drivers/cpufreq/freq_table.c   |  7 +++----
> > >  5 files changed, 13 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> > > index e73a66785d69d..6a6e26e1be14a 100644
> > > --- a/drivers/cpufreq/acpi-cpufreq.c
> > > +++ b/drivers/cpufreq/acpi-cpufreq.c
> > > @@ -857,6 +857,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
> > >                  * governor from selecting inadequate CPU frequencies.
> > >                  */
> > >                 arch_set_max_freq_ratio(true);
> > > +               policy->cpuinfo.max_freq = nominal_freq;
> > >         }
> > >
> > >         policy->freq_table = freq_table;
> >
> > This looks like a fix for acpi_cpufreq that should be sent separately.
>
> cpufreq_frequency_table_cpuinfo() (called before this) should always end up
> setting cpuinfo.max_freq properly and so this never broke earlier.

So is this change needed?

> > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > > index c45bc98721d24..310d5938cbdf6 100644
> > > --- a/drivers/cpufreq/amd-pstate.c
> > > +++ b/drivers/cpufreq/amd-pstate.c
> > > @@ -756,8 +756,6 @@ static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
> > >         else if (policy->cpuinfo.max_freq > nominal_freq)
> > >                 policy->cpuinfo.max_freq = nominal_freq;
> > >
> > > -       policy->max = policy->cpuinfo.max_freq;
> > > -
> > >         if (cppc_state == AMD_PSTATE_PASSIVE) {
> > >                 ret = freq_qos_update_request(&cpudata->req[1], policy->cpuinfo.max_freq);
> > >                 if (ret < 0)
> > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> > > index 9eac77c4f2944..4c46c7ea318eb 100644
> > > --- a/drivers/cpufreq/cppc_cpufreq.c
> > > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > > @@ -775,17 +775,11 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
> > >  {
> > >         struct cppc_cpudata *cpu_data = policy->driver_data;
> > >         struct cppc_perf_caps *caps = &cpu_data->perf_caps;
> > > -       int ret;
> > >
> > >         if (state)
> > > -               policy->max = cppc_perf_to_khz(caps, caps->highest_perf);
> > > +               policy->cpuinfo.max_freq = cppc_perf_to_khz(caps, caps->highest_perf);
> > >         else
> > > -               policy->max = cppc_perf_to_khz(caps, caps->nominal_perf);
> > > -       policy->cpuinfo.max_freq = policy->max;
> > > -
> > > -       ret = freq_qos_update_request(policy->max_freq_req, policy->max);
> > > -       if (ret < 0)
> > > -               return ret;
> > > +               policy->cpuinfo.max_freq = cppc_perf_to_khz(caps, caps->nominal_perf);
> > >
> > >         return 0;
> > >  }
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index 42de32488f422..20266fb42d18d 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -1500,10 +1500,14 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
> > >                                 goto out_destroy_policy;
> > >                         }
> > >
> > > +                       /*
> > > +                        * If boost is supported,
> > > +                        * init the constraint with cpuinfo.max_freq.
> > > +                        */
> > >                         ret = freq_qos_add_request(&policy->constraints,
> > >                                                    policy->boost_freq_req,
> > >                                                    FREQ_QOS_MAX,
> > > -                                                  FREQ_QOS_MAX_DEFAULT_VALUE);
> > > +                                                  policy->cpuinfo.max_freq);
> > >                         if (ret < 0) {
> > >                                 /*
> > >                                  * So we don't call freq_qos_remove_request() for an
> >
> > Why do you need to update stuff introduced in the previous patch?
> >
> > Is that because policy->cpuinfo.max_freq is not set consistently
> > before the changes made here?  In which case, wouldn't it be better to
> > make all drivers set cpuinfo.max_freq consistently before making the
> > changes in the second patch and fold the chunk above into the latter?
>
> Since cpuinfo.max_freq is set correctly earlier, I think this can be done
> earlier only.

I'm not sure what you mean.

To clarify my point, the question really is what prevents patch [2/6]
from passing policy->cpuinfo.max_freq to freq_qos_add_request() as the
last argument right away.