[1/6] cpufreq: acpi: Don't enable boost on policy exit

The boost-related code in cpufreq has undergone several changes over the
years, but this particular piece remained unchanged and is now outdated.

The cpufreq core currently manages boost settings during initialization,
and only when necessary. As such, there's no longer a need to enable
boost explicitly when entering system suspend.

Previously, this wasn’t causing issues because boost settings were
force-updated during policy initialization. However, commit 2b16c631832d
("cpufreq: ACPI: Remove set_boost in acpi_cpufreq_cpu_init()") changed
that behavior—correctly—by avoiding unnecessary updates.

As a result of this change, if boost was disabled prior to suspend, it
remains disabled on resume as expected. But due to the current code
forcibly enabling boost at suspend time, the system ends up with boost
frequencies enabled after resume, even if the global boost flag was
disabled. This contradicts the intended behavior.

Don't enable boost on policy exit.

Fixes: 2b16c631832d ("cpufreq: ACPI: Remove set_boost in acpi_cpufreq_cpu_init()")
Reported-by: Nicholas Chin <nic.c3.14@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220013
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
This was sent separately earlier. No changes from that.

 drivers/cpufreq/acpi-cpufreq.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

On Tue, Apr 22, 2025 at 11:54 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> The boost-related code in cpufreq has undergone several changes over the
> years, but this particular piece remained unchanged and is now outdated.
>
> The cpufreq core currently manages boost settings during initialization,
> and only when necessary. As such, there's no longer a need to enable
> boost explicitly when entering system suspend.
>
> Previously, this wasn’t causing issues because boost settings were
> force-updated during policy initialization. However, commit 2b16c631832d
> ("cpufreq: ACPI: Remove set_boost in acpi_cpufreq_cpu_init()") changed
> that behavior—correctly—by avoiding unnecessary updates.
>
> As a result of this change, if boost was disabled prior to suspend, it
> remains disabled on resume as expected. But due to the current code
> forcibly enabling boost at suspend time, the system ends up with boost
> frequencies enabled after resume, even if the global boost flag was
> disabled. This contradicts the intended behavior.
>
> Don't enable boost on policy exit.

Even after commit 2b16c631832d, the code removed by this patch does a
useful thing.  Namely, it clears the boost-disable bit in the MSR so
that the offline CPU doesn't prevent online CPUs from getting the
boost (in case the boost settings change after it has been taken
offline).  It doesn't actually touch policy->boost_enabled etc, just
that particular bit in the MSR.

I'm not sure how this useful thing will be done after the $subject patch.

Moreover, without the $subject patch, the change made by the next one
will cause the boost setting in the MSR to get back in sync with
policy->boost_enabled during online AFAICS, so why exactly is the
$subject patch needed?

> Fixes: 2b16c631832d ("cpufreq: ACPI: Remove set_boost in acpi_cpufreq_cpu_init()")
> Reported-by: Nicholas Chin <nic.c3.14@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220013
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> This was sent separately earlier. No changes from that.
>
>  drivers/cpufreq/acpi-cpufreq.c | 23 +++--------------------
>  1 file changed, 3 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 924314cdeebc..7002e8de8098 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -89,8 +89,9 @@ static bool boost_state(unsigned int cpu)
>         return false;
>  }
>
> -static int boost_set_msr(bool enable)
> +static void boost_set_msr_each(void *p_en)
>  {
> +       bool enable = (bool)p_en;
>         u32 msr_addr;
>         u64 msr_mask, val;
>
> @@ -107,7 +108,7 @@ static int boost_set_msr(bool enable)
>                 msr_mask = MSR_K7_HWCR_CPB_DIS;
>                 break;
>         default:
> -               return -EINVAL;
> +               return;
>         }
>
>         rdmsrl(msr_addr, val);
> @@ -118,14 +119,6 @@ static int boost_set_msr(bool enable)
>                 val |= msr_mask;
>
>         wrmsrl(msr_addr, val);
> -       return 0;
> -}
> -
> -static void boost_set_msr_each(void *p_en)
> -{
> -       bool enable = (bool) p_en;
> -
> -       boost_set_msr(enable);
>  }
>
>  static int set_boost(struct cpufreq_policy *policy, int val)
> @@ -532,15 +525,6 @@ static void free_acpi_perf_data(void)
>         free_percpu(acpi_perf_data);
>  }
>
> -static int cpufreq_boost_down_prep(unsigned int cpu)
> -{
> -       /*
> -        * Clear the boost-disable bit on the CPU_DOWN path so that
> -        * this cpu cannot block the remaining ones from boosting.
> -        */
> -       return boost_set_msr(1);
> -}
> -
>  /*
>   * acpi_cpufreq_early_init - initialize ACPI P-States library
>   *
> @@ -931,7 +915,6 @@ static void acpi_cpufreq_cpu_exit(struct cpufreq_policy *policy)
>
>         pr_debug("%s\n", __func__);
>
> -       cpufreq_boost_down_prep(policy->cpu);
>         policy->fast_switch_possible = false;
>         policy->driver_data = NULL;
>         acpi_processor_unregister_performance(data->acpi_perf_cpu);
> --
> 2.31.1.272.g89b43f80a514
>

On 2025/4/24 19:26, Rafael J. Wysocki wrote:

> On Thu, Apr 24, 2025 at 9:15 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>
>> On 23-04-25, 16:14, Rafael J. Wysocki wrote:
>>> Even after commit 2b16c631832d, the code removed by this patch does a
>>> useful thing.  Namely, it clears the boost-disable bit in the MSR so
>>> that the offline CPU doesn't prevent online CPUs from getting the
>>> boost (in case the boost settings change after it has been taken
>>> offline).
>>
>> I didn't understand this part earlier (and even now). How does a CPU
>> with boost-disabled, prevents others from boosting ? I have tried
>> looking at git logs, and still don't understand it :(
> 
> At the HW level, this is certainly possible.
> 
> Say two (or more) cores are driven by the same VR.  Boost typically
> (always?) requires a separate OPP with a higher voltage and this
> applies to all cores sharing the VR, so if one of them says it doesn't
> want that (which is what the bit in the boost-disable MSR effectively
> means), they all won't get it.

IIUC, this means that if one sets unboost to policy A, another core in
policy B (but sharing the same VR with core in policy A) will not be able
to achieve boost freq too. Then if policy A goes exit, the core in policy B
will get back to boost freq (without patch 1). And then core in B will be
unboosted again after core in A goes online/resume (because of patch 2).
But in the entire process, the boost flag in policy B is always enabled.

Please tell me I misunderstood because it looks really weird.😥

> 
> They arguably should belong to the same cpufreq policy, but this
> information is often missing from the ACPI tables, sometimes on
> purpose (for instance, the firmware may want to be in charge of the
> frequency coordination between the cores).
> 
>> Also, IIUC this and the boost-enabling at init() only happens for one
>> CPU in a policy, as init() and exit() are only called for the first
>> and last CPU of a policy. So if a policy has multiple CPUs, we aren't
>> touching boost states of other CPUs at init/exit.
> 
> But there may be a policy per CPU.
> 
>> And yes, this patch isn't mandatory at all for the
>>
>>> Moreover, without the $subject patch, the change made by the next one
>>> will cause the boost setting in the MSR to get back in sync with
>>> policy->boost_enabled during online AFAICS, so why exactly is the
>>> $subject patch needed?
>>
>> Right, this is merely a cleanup patch and isn't really required for
>> the next patch to make it work.
> 
> So I'd rather not make this change.
> 
> Evidently, someone made the effort to put in a comment explaining the
> purpose of the code in question, so it looks like they had a reason
> for adding it.
>

On 24-04-25, 13:26, Rafael J. Wysocki wrote:
> At the HW level, this is certainly possible.
> 
> Say two (or more) cores are driven by the same VR.  Boost typically
> (always?) requires a separate OPP with a higher voltage and this
> applies to all cores sharing the VR, so if one of them says it doesn't
> want that (which is what the bit in the boost-disable MSR effectively
> means), they all won't get it.

Right.

> They arguably should belong to the same cpufreq policy, but this
> information is often missing from the ACPI tables, sometimes on
> purpose (for instance, the firmware may want to be in charge of the
> frequency coordination between the cores).

Ahh, I see..

> > Also, IIUC this and the boost-enabling at init() only happens for one
> > CPU in a policy, as init() and exit() are only called for the first
> > and last CPU of a policy. So if a policy has multiple CPUs, we aren't
> > touching boost states of other CPUs at init/exit.
> 
> But there may be a policy per CPU.

Right, and I thought they shouldn't interfere with boost of other
CPUs, but above example tells a different story.

> So I'd rather not make this change.
> 
> Evidently, someone made the effort to put in a comment explaining the
> purpose of the code in question, so it looks like they had a reason
> for adding it.

Fair enough.