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

[Viresh Kumar]

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.

> > 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.

> > @@ -2818,16 +2822,10 @@ int cpufreq_boost_set_sw(struct cpufreq_policy *policy, int state)
> >                 return -ENXIO;
> >
> >         ret = cpufreq_frequency_table_cpuinfo(policy);
> > -       if (ret) {
> > +       if (ret)
> >                 pr_err("%s: Policy frequency update failed\n", __func__);
> > -               return ret;
> > -       }
> >
> > -       ret = freq_qos_update_request(policy->max_freq_req, policy->max);
> > -       if (ret < 0)
> > -               return ret;
> > -
> > -       return 0;
> > +       return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(cpufreq_boost_set_sw);
> 
> This also looks like it belongs to the second patch in the series.
> 
> > diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> > index 7f251daf03ce3..9b37f37c36389 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;

why are policy->min/max assignments removed here ?

-- 
viresh