[PATCH 0/6] cpufreq: Boost related cleanups / fixes

[PATCH 0/6] cpufreq: Boost related cleanups / fixes

[Viresh Kumar]

Hello,

This series tries to fix boost related issues found recently.

The first two patches (hopefully) fixes the boost related breakage
introduced recently. These should be applied for v6.15-rc4. Nicholas,
please give the first two patches a try.

The other four patches are general optimizations and fixes for boost
handling in general. These can be applied to -rc or next merge window.

--
Viresh

Viresh Kumar (6):
  cpufreq: acpi: Don't enable boost on policy exit
  cpufreq: acpi: Re-sync CPU boost state on system resume
  cpufreq: Don't unnecessarily call set_boost()
  cpufreq: Introduce policy_set_boost()
  cpufreq: Preserve policy's boost state after resume
  cpufreq: Force sync policy boost with global boost on sysfs update

 drivers/cpufreq/acpi-cpufreq.c | 38 ++++++++++------------
 drivers/cpufreq/cpufreq.c      | 58 ++++++++++++++++++++--------------
 2 files changed, 50 insertions(+), 46 deletions(-)

-- 
2.31.1.272.g89b43f80a514

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

[Viresh Kumar]

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(-)

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

[PATCH 2/6] cpufreq: acpi: Re-sync CPU boost state on system resume

[Viresh Kumar]

During suspend/resume cycles, platform firmware may alter the CPU boost
state.

If boost is disabled before suspend, it correctly remains off after
resume. However, if firmware re-enables boost during suspend, the system
may resume with boost frequencies enabled—even when the boost flag was
originally disabled. This violates expected behavior.

Ensure the boost state is re-synchronized with the kernel policy during
system resume to maintain consistency.

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>
---
 drivers/cpufreq/acpi-cpufreq.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 7002e8de8098..0ffabf740ff5 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -893,8 +893,19 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	if (perf->states[0].core_frequency * 1000 != freq_table[0].frequency)
 		pr_warn(FW_WARN "P-state 0 is not max freq\n");
 
-	if (acpi_cpufreq_driver.set_boost)
-		policy->boost_supported = true;
+	if (acpi_cpufreq_driver.set_boost) {
+		if (policy->boost_supported) {
+			/*
+			 * The firmware may have altered boost state while the
+			 * CPU was offline (for example during a suspend-resume
+			 * cycle).
+			 */
+			if (policy->boost_enabled != boost_state(cpu))
+				set_boost(policy, policy->boost_enabled);
+		} else {
+			policy->boost_supported = true;
+		}
+	}
 
 	return result;
 
-- 
2.31.1.272.g89b43f80a514

[PATCH 3/6] cpufreq: Don't unnecessarily call set_boost()

[Viresh Kumar]

The policy specific boost value may already be set correctly in
cpufreq_boost_trigger_state(), don't update it again unnecessarily.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 3841c9da6cac..e31891c7b500 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2861,7 +2861,7 @@ static int cpufreq_boost_trigger_state(int state)
 
 	cpus_read_lock();
 	for_each_active_policy(policy) {
-		if (!policy->boost_supported)
+		if (!policy->boost_supported || policy->boost_enabled == state)
 			continue;
 
 		policy->boost_enabled = state;
-- 
2.31.1.272.g89b43f80a514

[PATCH 4/6] cpufreq: Introduce policy_set_boost()

[Viresh Kumar]

Introduce policy_set_boost() to update boost state of a cpufreq policy.

No intentional function change.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 49 +++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e31891c7b500..24745088403b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -620,6 +620,22 @@ static ssize_t show_local_boost(struct cpufreq_policy *policy, char *buf)
 	return sysfs_emit(buf, "%d\n", policy->boost_enabled);
 }
 
+static int policy_set_boost(struct cpufreq_policy *policy, bool enable)
+{
+	int ret;
+
+	if (policy->boost_enabled == enable)
+		return 0;
+
+	policy->boost_enabled = enable;
+
+	ret = cpufreq_driver->set_boost(policy, enable);
+	if (ret)
+		policy->boost_enabled = !policy->boost_enabled;
+
+	return ret;
+}
+
 static ssize_t store_local_boost(struct cpufreq_policy *policy,
 				 const char *buf, size_t count)
 {
@@ -635,21 +651,14 @@ static ssize_t store_local_boost(struct cpufreq_policy *policy,
 	if (!policy->boost_supported)
 		return -EINVAL;
 
-	if (policy->boost_enabled == enable)
-		return count;
-
-	policy->boost_enabled = enable;
-
 	cpus_read_lock();
-	ret = cpufreq_driver->set_boost(policy, enable);
+	ret = policy_set_boost(policy, enable);
 	cpus_read_unlock();
 
-	if (ret) {
-		policy->boost_enabled = !policy->boost_enabled;
-		return ret;
-	}
+	if (!ret)
+		return count;
 
-	return count;
+	return ret;
 }
 
 static struct freq_attr local_boost = __ATTR(boost, 0644, show_local_boost, store_local_boost);
@@ -1618,15 +1627,12 @@ static int cpufreq_online(unsigned int cpu)
 		policy->cdev = of_cpufreq_cooling_register(policy);
 
 	/* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
-	if (cpufreq_driver->set_boost && policy->boost_supported &&
-	    policy->boost_enabled != cpufreq_boost_enabled()) {
-		policy->boost_enabled = cpufreq_boost_enabled();
-		ret = cpufreq_driver->set_boost(policy, policy->boost_enabled);
+	if (cpufreq_driver->set_boost && policy->boost_supported) {
+		ret = policy_set_boost(policy, cpufreq_boost_enabled());
 		if (ret) {
 			/* If the set_boost fails, the online operation is not affected */
 			pr_info("%s: CPU%d: Cannot %s BOOST\n", __func__, policy->cpu,
-				str_enable_disable(policy->boost_enabled));
-			policy->boost_enabled = !policy->boost_enabled;
+				str_enable_disable(cpufreq_boost_enabled()));
 		}
 	}
 
@@ -2861,15 +2867,12 @@ static int cpufreq_boost_trigger_state(int state)
 
 	cpus_read_lock();
 	for_each_active_policy(policy) {
-		if (!policy->boost_supported || policy->boost_enabled == state)
+		if (!policy->boost_supported)
 			continue;
 
-		policy->boost_enabled = state;
-		ret = cpufreq_driver->set_boost(policy, state);
-		if (ret) {
-			policy->boost_enabled = !policy->boost_enabled;
+		ret = policy_set_boost(policy, state);
+		if (ret)
 			goto err_reset_state;
-		}
 	}
 	cpus_read_unlock();
 
-- 
2.31.1.272.g89b43f80a514

[PATCH 5/6] cpufreq: Preserve policy's boost state after resume

[Viresh Kumar]

If the global boost flag was enabled and policy boost flag was disabled
before a suspend resume cycle, cpufreq_online() will enable the policy
boost flag on resume.

While it is important for the policy boost flag to mirror the global
boost flag when a policy is first created, it should be avoided when the
policy is reinitialized (for example after a suspend resume cycle).

Though, if the global boost flag is disabled at this point of time, we
want to make sure policy boost flag is disabled too.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 24745088403b..0ad459bc8f84 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1626,8 +1626,13 @@ static int cpufreq_online(unsigned int cpu)
 	if (new_policy && cpufreq_thermal_control_enabled(cpufreq_driver))
 		policy->cdev = of_cpufreq_cooling_register(policy);
 
-	/* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
-	if (cpufreq_driver->set_boost && policy->boost_supported) {
+	/*
+	 * Let the per-policy boost flag mirror the cpufreq_driver boost during
+	 * initialization for a new policy. For an existing policy, maintain the
+	 * previous boost value unless global boost is disabled.
+	 */
+	if (cpufreq_driver->set_boost && policy->boost_supported &&
+	    (new_policy || !cpufreq_boost_enabled())) {
 		ret = policy_set_boost(policy, cpufreq_boost_enabled());
 		if (ret) {
 			/* If the set_boost fails, the online operation is not affected */
-- 
2.31.1.272.g89b43f80a514

[PATCH 6/6] cpufreq: Force sync policy boost with global boost on sysfs update

[Viresh Kumar]

If the global boost flag is enabled and policy boost flag is disabled, a
call to `cpufreq_boost_trigger_state(true)` must enable the policy's
boost state.

The current code misses that because of an optimization. Fix it.

Suggested-by: Lifeng Zheng <zhenglifeng1@huawei.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 0ad459bc8f84..4ac5d4fcfdd4 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2863,8 +2863,10 @@ static int cpufreq_boost_trigger_state(int state)
 	unsigned long flags;
 	int ret = 0;
 
-	if (cpufreq_driver->boost_enabled == state)
-		return 0;
+	/*
+	 * Don't compare 'cpufreq_driver->boost_enabled' with 'state' here to
+	 * make sure all policies are in sync with global boost flag.
+	 */
 
 	write_lock_irqsave(&cpufreq_driver_lock, flags);
 	cpufreq_driver->boost_enabled = state;
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH 0/6] cpufreq: Boost related cleanups / fixes

[zhenglifeng (A)]

On 2025/4/22 17:53, Viresh Kumar wrote:

> Hello,
> 
> This series tries to fix boost related issues found recently.
> 
> The first two patches (hopefully) fixes the boost related breakage
> introduced recently. These should be applied for v6.15-rc4. Nicholas,
> please give the first two patches a try.
> 
> The other four patches are general optimizations and fixes for boost
> handling in general. These can be applied to -rc or next merge window.
> 
> --
> Viresh
> 
> Viresh Kumar (6):
>   cpufreq: acpi: Don't enable boost on policy exit
>   cpufreq: acpi: Re-sync CPU boost state on system resume
>   cpufreq: Don't unnecessarily call set_boost()
>   cpufreq: Introduce policy_set_boost()
>   cpufreq: Preserve policy's boost state after resume
>   cpufreq: Force sync policy boost with global boost on sysfs update
> 
>  drivers/cpufreq/acpi-cpufreq.c | 38 ++++++++++------------
>  drivers/cpufreq/cpufreq.c      | 58 ++++++++++++++++++++--------------
>  2 files changed, 50 insertions(+), 46 deletions(-)
> 

Reviewed-by: Lifeng Zheng <zhenglifeng1@huawei.com>

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

[Nicholas Chin]

On 2025-04-22 03:53, Viresh Kumar 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.
> 
> 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);

The first two patches in this series appear to work as intended. The boost state (both enabled and disabled) persists across a resume from S3 suspend.

Tested-by: Nicholas Chin <nic.c3.14@gmail.com>

Re: [PATCH 2/6] cpufreq: acpi: Re-sync CPU boost state on system resume

[Nicholas Chin]

On 2025-04-22 03:53, Viresh Kumar wrote:
> During suspend/resume cycles, platform firmware may alter the CPU boost
> state.
> 
> If boost is disabled before suspend, it correctly remains off after
> resume. However, if firmware re-enables boost during suspend, the system
> may resume with boost frequencies enabled—even when the boost flag was
> originally disabled. This violates expected behavior.
> 
> Ensure the boost state is re-synchronized with the kernel policy during
> system resume to maintain consistency.
> 
> 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>
> ---
>  drivers/cpufreq/acpi-cpufreq.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 7002e8de8098..0ffabf740ff5 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -893,8 +893,19 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
>  	if (perf->states[0].core_frequency * 1000 != freq_table[0].frequency)
>  		pr_warn(FW_WARN "P-state 0 is not max freq\n");
>  
> -	if (acpi_cpufreq_driver.set_boost)
> -		policy->boost_supported = true;
> +	if (acpi_cpufreq_driver.set_boost) {
> +		if (policy->boost_supported) {
> +			/*
> +			 * The firmware may have altered boost state while the
> +			 * CPU was offline (for example during a suspend-resume
> +			 * cycle).
> +			 */
> +			if (policy->boost_enabled != boost_state(cpu))
> +				set_boost(policy, policy->boost_enabled);
> +		} else {
> +			policy->boost_supported = true;
> +		}
> +	}
>  
>  	return result;
>  

The first two patches in this series appear to work as intended. The boost state (both enabled and disabled) persists across a resume from S3 suspend.

Tested-by: Nicholas Chin <nic.c3.14@gmail.com>

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

[Rafael J. Wysocki]

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
>

Re: [PATCH 2/6] cpufreq: acpi: Re-sync CPU boost state on system resume

[Rafael J. Wysocki]

On Tue, Apr 22, 2025 at 11:54 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> During suspend/resume cycles, platform firmware may alter the CPU boost
> state.
>
> If boost is disabled before suspend, it correctly remains off after
> resume. However, if firmware re-enables boost during suspend, the system
> may resume with boost frequencies enabled—even when the boost flag was
> originally disabled. This violates expected behavior.
>
> Ensure the boost state is re-synchronized with the kernel policy during
> system resume to maintain consistency.
>
> 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>
> ---
>  drivers/cpufreq/acpi-cpufreq.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 7002e8de8098..0ffabf740ff5 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -893,8 +893,19 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
>         if (perf->states[0].core_frequency * 1000 != freq_table[0].frequency)
>                 pr_warn(FW_WARN "P-state 0 is not max freq\n");
>
> -       if (acpi_cpufreq_driver.set_boost)
> -               policy->boost_supported = true;
> +       if (acpi_cpufreq_driver.set_boost) {
> +               if (policy->boost_supported) {
> +                       /*
> +                        * The firmware may have altered boost state while the
> +                        * CPU was offline (for example during a suspend-resume
> +                        * cycle).
> +                        */
> +                       if (policy->boost_enabled != boost_state(cpu))
> +                               set_boost(policy, policy->boost_enabled);
> +               } else {
> +                       policy->boost_supported = true;

IIUC policy->boost_enabled is false at this point, so say that
boost_state(cpu) returns true and say cpufreq_boost_enabled() returns
false.

cpufreq_online() will see policy->boost_enabled ==
cpufreq_boost_enabled(), so it won't do anything regarding boost, and
say that this happens for all online CPUs.

cpufreq_boost_enabled() will be false, policy->boost_enabled will be
false for every policy, but boost will be effectively enabled AFAICS.

> +               }
> +       }
>
>         return result;
>
> --

Re: [PATCH 2/6] cpufreq: acpi: Re-sync CPU boost state on system resume

[Rafael J. Wysocki]

On Wed, Apr 23, 2025 at 4:26 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Apr 22, 2025 at 11:54 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > During suspend/resume cycles, platform firmware may alter the CPU boost
> > state.
> >
> > If boost is disabled before suspend, it correctly remains off after
> > resume. However, if firmware re-enables boost during suspend, the system
> > may resume with boost frequencies enabled—even when the boost flag was
> > originally disabled. This violates expected behavior.
> >
> > Ensure the boost state is re-synchronized with the kernel policy during
> > system resume to maintain consistency.
> >
> > 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>
> > ---
> >  drivers/cpufreq/acpi-cpufreq.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> > index 7002e8de8098..0ffabf740ff5 100644
> > --- a/drivers/cpufreq/acpi-cpufreq.c
> > +++ b/drivers/cpufreq/acpi-cpufreq.c
> > @@ -893,8 +893,19 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
> >         if (perf->states[0].core_frequency * 1000 != freq_table[0].frequency)
> >                 pr_warn(FW_WARN "P-state 0 is not max freq\n");
> >
> > -       if (acpi_cpufreq_driver.set_boost)
> > -               policy->boost_supported = true;
> > +       if (acpi_cpufreq_driver.set_boost) {
> > +               if (policy->boost_supported) {
> > +                       /*
> > +                        * The firmware may have altered boost state while the
> > +                        * CPU was offline (for example during a suspend-resume
> > +                        * cycle).
> > +                        */
> > +                       if (policy->boost_enabled != boost_state(cpu))
> > +                               set_boost(policy, policy->boost_enabled);
> > +               } else {
> > +                       policy->boost_supported = true;
>
> IIUC policy->boost_enabled is false at this point, so say that
> boost_state(cpu) returns true and say cpufreq_boost_enabled() returns
> false.

This cannot happen for CPU 0 because of acpi_cpufreq_boost_init() ->

> cpufreq_online() will see policy->boost_enabled ==
> cpufreq_boost_enabled(), so it won't do anything regarding boost, and
> say that this happens for all online CPUs.

-> so if boost_state(0) returns true, policy->boost_enabled will be
set for all policies to start with due to the code in
cpufreq_online(), but this is far from obvious.

I would at least say in the changelog that set_boost() need not be
called directly at the policy initialization time because of the
above.

Re: [PATCH 2/6] cpufreq: acpi: Re-sync CPU boost state on system resume

[Rafael J. Wysocki]

On Wed, Apr 23, 2025 at 4:40 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Apr 23, 2025 at 4:26 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Apr 22, 2025 at 11:54 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > During suspend/resume cycles, platform firmware may alter the CPU boost
> > > state.
> > >
> > > If boost is disabled before suspend, it correctly remains off after
> > > resume. However, if firmware re-enables boost during suspend, the system
> > > may resume with boost frequencies enabled—even when the boost flag was
> > > originally disabled. This violates expected behavior.
> > >
> > > Ensure the boost state is re-synchronized with the kernel policy during
> > > system resume to maintain consistency.
> > >
> > > 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>
> > > ---
> > >  drivers/cpufreq/acpi-cpufreq.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> > > index 7002e8de8098..0ffabf740ff5 100644
> > > --- a/drivers/cpufreq/acpi-cpufreq.c
> > > +++ b/drivers/cpufreq/acpi-cpufreq.c
> > > @@ -893,8 +893,19 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
> > >         if (perf->states[0].core_frequency * 1000 != freq_table[0].frequency)
> > >                 pr_warn(FW_WARN "P-state 0 is not max freq\n");
> > >
> > > -       if (acpi_cpufreq_driver.set_boost)
> > > -               policy->boost_supported = true;
> > > +       if (acpi_cpufreq_driver.set_boost) {
> > > +               if (policy->boost_supported) {
> > > +                       /*
> > > +                        * The firmware may have altered boost state while the
> > > +                        * CPU was offline (for example during a suspend-resume
> > > +                        * cycle).
> > > +                        */
> > > +                       if (policy->boost_enabled != boost_state(cpu))
> > > +                               set_boost(policy, policy->boost_enabled);
> > > +               } else {
> > > +                       policy->boost_supported = true;
> >
> > IIUC policy->boost_enabled is false at this point, so say that
> > boost_state(cpu) returns true and say cpufreq_boost_enabled() returns
> > false.
>
> This cannot happen for CPU 0 because of acpi_cpufreq_boost_init() ->
>
> > cpufreq_online() will see policy->boost_enabled ==
> > cpufreq_boost_enabled(), so it won't do anything regarding boost, and
> > say that this happens for all online CPUs.
>
> -> so if boost_state(0) returns true, policy->boost_enabled will be
> set for all policies to start with due to the code in
> cpufreq_online(), but this is far from obvious.
>
> I would at least say in the changelog that set_boost() need not be
> called directly at the policy initialization time because of the
> above.

I also think that acpi_cpufreq_resume() may be a better place for
re-syncing the boost state with policy->boost_enabled because it may
do that for CPU 0 as well as for the non-boot CPUs.

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

[Viresh Kumar]

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 :(

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.

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.

-- 
viresh

Re: [PATCH 2/6] cpufreq: acpi: Re-sync CPU boost state on system resume

[Viresh Kumar]

On 23-04-25, 16:59, Rafael J. Wysocki wrote:
> On Wed, Apr 23, 2025 at 4:40 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > IIUC policy->boost_enabled is false at this point, so say that
> > > boost_state(cpu) returns true and say cpufreq_boost_enabled() returns
> > > false.
> >
> > This cannot happen for CPU 0 because of acpi_cpufreq_boost_init() ->

Right.

> > > cpufreq_online() will see policy->boost_enabled ==
> > > cpufreq_boost_enabled(), so it won't do anything regarding boost, and
> > > say that this happens for all online CPUs.
> >
> > -> so if boost_state(0) returns true, policy->boost_enabled will be
> > set for all policies to start with due to the code in
> > cpufreq_online(), but this is far from obvious.

> > I would at least say in the changelog that set_boost() need not be
> > called directly at the policy initialization time because of the
> > above.

Sure.

> I also think that acpi_cpufreq_resume() may be a better place for
> re-syncing the boost state with policy->boost_enabled because it may
> do that for CPU 0 as well as for the non-boot CPUs.

I thought about that but kept this in acpi_cpufreq_cpu_init() as there
are other corner cases too. A simple CPU hotplug (without
suspend/resume) for example. In that case exit/init will get called
but not resume.

-- 
viresh

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

[Rafael J. Wysocki]

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.

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.

Re: [PATCH 2/6] cpufreq: acpi: Re-sync CPU boost state on system resume

[Rafael J. Wysocki]

On Thu, Apr 24, 2025 at 9:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 23-04-25, 16:59, Rafael J. Wysocki wrote:
> > On Wed, Apr 23, 2025 at 4:40 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > IIUC policy->boost_enabled is false at this point, so say that
> > > > boost_state(cpu) returns true and say cpufreq_boost_enabled() returns
> > > > false.
> > >
> > > This cannot happen for CPU 0 because of acpi_cpufreq_boost_init() ->
>
> Right.
>
> > > > cpufreq_online() will see policy->boost_enabled ==
> > > > cpufreq_boost_enabled(), so it won't do anything regarding boost, and
> > > > say that this happens for all online CPUs.
> > >
> > > -> so if boost_state(0) returns true, policy->boost_enabled will be
> > > set for all policies to start with due to the code in
> > > cpufreq_online(), but this is far from obvious.
>
> > > I would at least say in the changelog that set_boost() need not be
> > > called directly at the policy initialization time because of the
> > > above.
>
> Sure.
>
> > I also think that acpi_cpufreq_resume() may be a better place for
> > re-syncing the boost state with policy->boost_enabled because it may
> > do that for CPU 0 as well as for the non-boot CPUs.
>
> I thought about that but kept this in acpi_cpufreq_cpu_init() as there
> are other corner cases too. A simple CPU hotplug (without
> suspend/resume) for example. In that case exit/init will get called
> but not resume.

Fair enough.

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

[zhenglifeng (A)]

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

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

[Viresh Kumar]

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.

-- 
viresh