Re: [PATCH v1 10/10] cpufreq: Pass policy pointer to ->update_limits()

[srinivas pandruvada]

On Mon, 2025-04-07 at 20:48 +0200, Rafael J. Wysocki wrote:
> On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> wrote:
> > 
> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > 
> > Since cpufreq_update_limits() obtains a cpufreq policy pointer for
> > the
> > given CPU and reference counts the corresponding policy object, it
> > may
> > as well pass the policy pointer to the cpufreq driver's -
> > >update_limits()
> > callback which allows that callback to avoid invoking
> > cpufreq_cpu_get()
> > for the same CPU.
> > 
> > Accordingly, redefine ->update_limits() to take a policy pointer
> > instead
> > of a CPU number and update both drivers implementing it,
> > intel_pstate
> > and amd-pstate, as needed.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> 
Hi Rafael,

> Hi Srinivas,
> 
> If you have any concerns regarding this patch, please let me know
> (note that it is based on the [05/10]).
> 
Changes looks fine, but wants to test out some update limits from
interrupt path.
Checked your branches at linux-pm, not able to locate in any branch to
apply.
Please point me to a branch.

Thanks,
Srinivas

> > ---
> >  drivers/cpufreq/amd-pstate.c   |    7 ++-----
> >  drivers/cpufreq/cpufreq.c      |    2 +-
> >  drivers/cpufreq/intel_pstate.c |   29 ++++++++++++++++++----------
> > -
> >  include/linux/cpufreq.h        |    2 +-
> >  4 files changed, 22 insertions(+), 18 deletions(-)
> > 
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -821,19 +821,16 @@
> >         schedule_work(&sched_prefcore_work);
> >  }
> > 
> > -static void amd_pstate_update_limits(unsigned int cpu)
> > +static void amd_pstate_update_limits(struct cpufreq_policy
> > *policy)
> >  {
> > -       struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> > cpufreq_cpu_get(cpu);
> >         struct amd_cpudata *cpudata;
> >         u32 prev_high = 0, cur_high = 0;
> >         bool highest_perf_changed = false;
> > +       unsigned int cpu = policy->cpu;
> > 
> >         if (!amd_pstate_prefcore)
> >                 return;
> > 
> > -       if (!policy)
> > -               return;
> > -
> >         if (amd_get_highest_perf(cpu, &cur_high))
> >                 return;
> > 
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2741,7 +2741,7 @@
> >                 return;
> > 
> >         if (cpufreq_driver->update_limits)
> > -               cpufreq_driver->update_limits(cpu);
> > +               cpufreq_driver->update_limits(policy);
> >         else
> >                 cpufreq_policy_refresh(policy);
> >  }
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -1353,14 +1353,9 @@
> >                 cpufreq_update_policy(cpu);
> >  }
> > 
> > -static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
> > +static void __intel_pstate_update_max_freq(struct cpufreq_policy
> > *policy,
> > +                                          struct cpudata *cpudata)
> >  {
> > -       struct cpufreq_policy *policy __free(put_cpufreq_policy);
> > -
> > -       policy = cpufreq_cpu_get(cpudata->cpu);
> > -       if (!policy)
> > -               return false;
> > -
> >         guard(cpufreq_policy_write)(policy);
> > 
> >         if (hwp_active)
> > @@ -1370,16 +1365,28 @@
> >                         cpudata->pstate.max_freq : cpudata-
> > >pstate.turbo_freq;
> > 
> >         refresh_frequency_limits(policy);
> > +}
> > +
> > +static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
> > +{
> > +       struct cpufreq_policy *policy __free(put_cpufreq_policy);
> > +
> > +       policy = cpufreq_cpu_get(cpudata->cpu);
> > +       if (!policy)
> > +               return false;
> > +
> > +       __intel_pstate_update_max_freq(policy, cpudata);
> > 
> >         return true;
> >  }
> > 
> > -static void intel_pstate_update_limits(unsigned int cpu)
> > +static void intel_pstate_update_limits(struct cpufreq_policy
> > *policy)
> >  {
> > -       struct cpudata *cpudata = all_cpu_data[cpu];
> > +       struct cpudata *cpudata = all_cpu_data[policy->cpu];
> > +
> > +       __intel_pstate_update_max_freq(policy, cpudata);
> > 
> > -       if (intel_pstate_update_max_freq(cpudata))
> > -               hybrid_update_capacity(cpudata);
> > +       hybrid_update_capacity(cpudata);
> >  }
> > 
> >  static void intel_pstate_update_limits_for_all(void)
> > --- a/include/linux/cpufreq.h
> > +++ b/include/linux/cpufreq.h
> > @@ -399,7 +399,7 @@
> >         unsigned int    (*get)(unsigned int cpu);
> > 
> >         /* Called to update policy limits on firmware
> > notifications. */
> > -       void            (*update_limits)(unsigned int cpu);
> > +       void            (*update_limits)(struct cpufreq_policy
> > *policy);
> > 
> >         /* optional */
> >         int             (*bios_limit)(int cpu, unsigned int
> > *limit);
> > 
> > 
> > 
> > 
>