Re: [PATCH v1 6/6] cpufreq: intel_pstate: Update the maximum CPU frequency consistently

[srinivas pandruvada]

On Mon, 2024-03-25 at 18:06 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> 
> There are 3 places at which the maximum CPU frequency may change,
> store_no_turbo(), intel_pstate_update_limits() (when called by the
> cpufreq core) and intel_pstate_notify_work() (when handling a HWP
> change notification).  Currently, cpuinfo.max_freq is only updated by
> store_no_turbo(), although it principle it may be necessary to update
> it at the other 2 places too.

It also works for intel_pstate_notify_work() path as is without this
change.

To start with:

$ sudo rdmsr 0x771
6080c14
$ cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
2000000
800000
0

Now trigger a max frequency change via SST. intel_pstate_notify_work()
called because guaranteed also changed. We didn't subscribe for max
change only (although we should).

Max changed from 2GHz to 1.9 GHz.

$ sudo rdmsr 0x771
6080e13
[labuser@gnr-bkc ~]$ cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
1900000
800000
0

Now trigger SST to change to max frequency to 2GHz.

sudo rdmsr 0x771
6080c14

cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
2000000
800000
0

May be you mean something else.

Thanks,
Srinivas


> 
> Make all of them mutually consistent.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
>  drivers/cpufreq/intel_pstate.c |   23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -1153,18 +1153,32 @@ static void intel_pstate_update_policies
>  static void __intel_pstate_update_max_freq(struct cpudata *cpudata,
>                                            struct cpufreq_policy
> *policy)
>  {
> +       intel_pstate_get_hwp_cap(cpudata);
> +
>         policy->cpuinfo.max_freq = READ_ONCE(global.no_turbo) ?
>                         cpudata->pstate.max_freq : cpudata-
> >pstate.turbo_freq;
> +
>         refresh_frequency_limits(policy);
>  }
>  
>  static void intel_pstate_update_limits(unsigned int cpu)
>  {
> -       mutex_lock(&intel_pstate_driver_lock);
> +       struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
>  
> -       cpufreq_update_policy(cpu);
> +       if (!policy)
> +               return;
>  
> -       mutex_unlock(&intel_pstate_driver_lock);
> +       __intel_pstate_update_max_freq(all_cpu_data[cpu], policy);
> +
> +       cpufreq_cpu_release(policy);
> +}
> +
> +static void intel_pstate_update_limits_for_all(void)
> +{
> +       int cpu;
> +
> +       for_each_possible_cpu(cpu)
> +               intel_pstate_update_limits(cpu);
>  }
>  
>  /************************** sysfs begin ************************/
> @@ -1311,7 +1325,7 @@ static ssize_t store_no_turbo(struct kob
>  
>         mutex_unlock(&intel_pstate_limits_lock);
>  
> -       intel_pstate_update_policies();
> +       intel_pstate_update_limits_for_all();
>         arch_set_max_freq_ratio(no_turbo);
>  
>  unlock_driver:
> @@ -1595,7 +1609,6 @@ static void intel_pstate_notify_work(str
>         struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpudata-
> >cpu);
>  
>         if (policy) {
> -               intel_pstate_get_hwp_cap(cpudata);
>                 __intel_pstate_update_max_freq(cpudata, policy);
>  
>                 cpufreq_cpu_release(policy);
> 
> 
>