linux-kernel - Re: [PATCH 1/6] cpufreq: acpi: Don't enable boost on policy exit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0iCrQeKs=4S-x83Fgf-W4u=2JYLA5VmgKPaLCvYAkNpig@mail.gmail.com>
Date: Wed, 23 Apr 2025 16:14:56 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Lifeng Zheng <zhenglifeng1@...wei.com>, linux-pm@...r.kernel.org, 
	Vincent Guittot <vincent.guittot@...aro.org>, Nicholas Chin <nic.c3.14@...il.com>, 
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] cpufreq: acpi: Don't enable boost on policy exit

On Tue, Apr 22, 2025 at 11:54 AM Viresh Kumar <viresh.kumar@...aro.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@...il.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220013
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.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
>