cpufreq: Boost related cleanups / fixes
Viresh Kumar
maintainercover cpufreq: Boost related cleanups / fixes
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(-)
Signature · 1 line
2.31.1.272.g89b43f80a514
Viresh Kumar
maintainerPATCH 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.
---
This was sent separately earlier. No changes from that.
drivers/cpufreq/acpi-cpufreq.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-)
drivers/cpufreq/acpi-cpufreq.c +3 -20
@@ -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);
Signature · 1 line
2.31.1.272.g89b43f80a514
Re: [PATCH 1/6] cpufreq: acpi: Don't enable boost on policy exit
Quoted 86 lines · 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: Removeset_boostinacpi_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 <[email protected]>Signed-off-by Viresh Kumar <[email protected]>---
This was sent separately earlier. No changes from that.drivers/cpufreq/acpi-cpufreq.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-)drivers/cpufreq/acpi-cpufreq.c +3
-19@@ -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 *
drivers/cpufreq/acpi-cpufreq.c static void acpi_cpufreq_cpu_exit(struct cpufreq_policy *policy) · line 920
Quoted 7 lines
drivers/cpufreq/acpi-cpufreq.c +0
-1@@ -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.
Rafael J. Wysocki
maintainerRe: [PATCH 1/6] cpufreq: acpi: Don't enable boost on policy exit
In reply to Viresh KumarQuoted 13 lines · 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: Removeset_boostinacpi_cpufreq_cpu_init()") changed that behavior—correctly—by avoiding unnecessary updates.
Quoted 7 lines
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?
Quoted 76 lines
Fixes 2b16c631832d ("cpufreq: ACPI: Remove set_boost in acpi_cpufreq_cpu_init()")Reported-by Nicholas Chin <[email protected]>Signed-off-by Viresh Kumar <[email protected]>---
This was sent separately earlier. No changes from that.drivers/cpufreq/acpi-cpufreq.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-)drivers/cpufreq/acpi-cpufreq.c +3
-21@@ -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
Viresh Kumar
maintainerRe: [PATCH 1/6] cpufreq: acpi: Don't enable boost on policy exit
Quoted 5 lines · Rafael J. Wysocki
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
Quoted 4 lines
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_enabledduring 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.
Signature · 1 line
viresh
Rafael J. Wysocki
maintainerRe: [PATCH 1/6] cpufreq: acpi: Don't enable boost on policy exit
Quoted 11 lines · Viresh Kumar
Quoted 5 lines · Rafael J. Wysocki
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).
Quoted 4 lines
Also, IIUC this and the boost-enabling at
init()only happens for one CPU in a policy, asinit()andexit()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.
Quoted 9 lines
And yes, this patch isn't mandatory at all for the
Quoted 4 lines
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_enabledduring 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
Quoted 13 lines · Rafael J. Wysocki
Quoted 11 lines · Viresh Kumar
Quoted 5 lines · Rafael J. Wysocki
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 :(
Quoted 7 lines
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.😥
Quoted 29 lines
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).
Quoted 4 lines
Also, IIUC this and the boost-enabling at
init()only happens for one CPU in a policy, asinit()andexit()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.
Quoted 9 lines
And yes, this patch isn't mandatory at all for the
Quoted 4 lines
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_enabledduring 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.
Viresh Kumar
maintainerRe: [PATCH 1/6] cpufreq: acpi: Don't enable boost on policy exit
In reply to Rafael J. WysockiQuoted 7 lines · Rafael J. Wysocki
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.
Quoted 4 lines
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..
Quoted 6 lines
Quoted 4 lines
Also, IIUC this and the boost-enabling at
init()only happens for one CPU in a policy, asinit()andexit()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.
Quoted 5 lines
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.
Signature · 1 line
viresh
Viresh Kumar
maintainerPATCH 2/6 cpufreq: acpi: Re-sync CPU boost state on system resume
In reply to Viresh KumarDuring 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.
--- drivers/cpufreq/acpi-cpufreq.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
drivers/cpufreq/acpi-cpufreq.c +13 -2
@@ -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;
Signature · 1 line
2.31.1.272.g89b43f80a514
Re: [PATCH 2/6] cpufreq: acpi: Re-sync CPU boost state on system resume
Quoted 35 lines · 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 <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/acpi-cpufreq.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)drivers/cpufreq/acpi-cpufreq.c +6
-2@@ -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).
drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) · line 911
Quoted 10 lines
drivers/cpufreq/acpi-cpufreq.c +7
-0+*/ +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.
Rafael J. Wysocki
maintainerRe: [PATCH 2/6] cpufreq: acpi: Re-sync CPU boost state on system resume
In reply to Viresh KumarQuoted 31 lines · 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 <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/acpi-cpufreq.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)drivers/cpufreq/acpi-cpufreq.c +1
-2@@ -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) {
drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) · line 906
Quoted 10 lines
drivers/cpufreq/acpi-cpufreq.c +10
-0+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.
Quoted 6 lines
drivers/cpufreq/acpi-cpufreq.c +2
-1+ } + }return result; --
Rafael J. Wysocki
maintainerRe: [PATCH 2/6] cpufreq: acpi: Re-sync CPU boost state on system resume
Quoted 39 lines · Rafael J. Wysocki
Quoted 37 lines · 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 <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/acpi-cpufreq.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)drivers/cpufreq/acpi-cpufreq.c +7
-2@@ -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). +*/
Quoted 8 lines
drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) · line 906
Quoted 4 lines
drivers/cpufreq/acpi-cpufreq.c +4
-0+if (policy->boost_enabled !=boost_state (cpu)) +set_boost (policy, policy->boost_enabled); + }else { + policy->boost_supported = true;IIUC
policy->boost_enabledis false at this point, so say thatboost_state(cpu)returns true and saycpufreq_boost_enabled()returns false.
This cannot happen for CPU 0 because of acpi_cpufreq_boost_init() ->
Quoted 3 lines
cpufreq_online()will seepolicy->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.
Rafael J. Wysocki
maintainerRe: [PATCH 2/6] cpufreq: acpi: Re-sync CPU boost state on system resume
Quoted 56 lines · Rafael J. Wysocki
Quoted 39 lines · Rafael J. Wysocki
Quoted 37 lines · 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 <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/acpi-cpufreq.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)drivers/cpufreq/acpi-cpufreq.c +7
-2@@ -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). +*/ Quoted 8 lines
drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) · line 906
Quoted 4 lines
drivers/cpufreq/acpi-cpufreq.c +4
-0+if (policy->boost_enabled !=boost_state (cpu)) +set_boost (policy, policy->boost_enabled); + }else { + policy->boost_supported = true;IIUC
policy->boost_enabledis false at this point, so say thatboost_state(cpu)returns true and saycpufreq_boost_enabled()returns false.This cannot happen for CPU 0 because of
acpi_cpufreq_boost_init()->Quoted 3 lines
cpufreq_online()will seepolicy->boost_enabled==cpufreq_boost_enabled(), so it won't do anything regarding boost, and say that this happens for all online CPUs.
Quoted 7 lines
-> so if
boost_state(0)returns true,policy->boost_enabledwill be set for all policies to start with due to the code incpufreq_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.
Viresh Kumar
maintainerRe: [PATCH 2/6] cpufreq: acpi: Re-sync CPU boost state on system resume
Quoted 6 lines · Rafael J. Wysocki
Quoted 5 lines · Rafael J. Wysocki
Quoted 3 lines
IIUC
policy->boost_enabledis false at this point, so say thatboost_state(cpu)returns true and saycpufreq_boost_enabled()returns false.This cannot happen for CPU 0 because of
acpi_cpufreq_boost_init()->
Right.
Quoted 11 lines
Quoted 11 lines
Quoted 3 lines
cpufreq_online()will seepolicy->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_enabledwill be set for all policies to start with due to the code incpufreq_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.
Quoted 3 lines
I also think that
acpi_cpufreq_resume()may be a better place for re-syncing the boost state withpolicy->boost_enabledbecause 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.
Signature · 1 line
viresh
Rafael J. Wysocki
maintainerRe: [PATCH 2/6] cpufreq: acpi: Re-sync CPU boost state on system resume
Quoted 25 lines · Viresh Kumar
Quoted 6 lines · Rafael J. Wysocki
Quoted 5 lines · Rafael J. Wysocki
Quoted 3 lines
IIUC
policy->boost_enabledis false at this point, so say thatboost_state(cpu)returns true and saycpufreq_boost_enabled()returns false.This cannot happen for CPU 0 because of
acpi_cpufreq_boost_init()->Right.
Quoted 11 lines
Quoted 11 lines
Quoted 3 lines
cpufreq_online()will seepolicy->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_enabledwill be set for all policies to start with due to the code incpufreq_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.
Quoted 8 lines
Quoted 3 lines
I also think that
acpi_cpufreq_resume()may be a better place for re-syncing the boost state withpolicy->boost_enabledbecause 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.
Viresh Kumar
maintainerPATCH 3/6 cpufreq: Don't unnecessarily call set_boost()
In reply to Viresh KumarThe policy specific boost value may already be set correctly in cpufreq_boost_trigger_state(), don't update it again unnecessarily.
--- drivers/cpufreq/cpufreq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/cpufreq/cpufreq.c +1 -1
@@ -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;
Signature · 1 line
2.31.1.272.g89b43f80a514
Viresh Kumar
maintainerPATCH 4/6 cpufreq: Introduce policy_set_boost()
In reply to Viresh KumarIntroduce policy_set_boost() to update boost state of a cpufreq policy.
No intentional function change.
--- drivers/cpufreq/cpufreq.c | 49 +++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 23 deletions(-)
drivers/cpufreq/cpufreq.c +26 -23
@@ -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 ();
Signature · 1 line
2.31.1.272.g89b43f80a514
Viresh Kumar
maintainerPATCH 5/6 cpufreq: Preserve policy's boost state after resume
In reply to Viresh KumarIf 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.
--- drivers/cpufreq/cpufreq.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/cpufreq/cpufreq.c +7 -2
@@ -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 */
Signature · 1 line
2.31.1.272.g89b43f80a514
Viresh Kumar
maintainerPATCH 6/6 cpufreq: Force sync policy boost with global boost on sysfs update
In reply to Viresh KumarIf 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.
--- drivers/cpufreq/cpufreq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/cpufreq/cpufreq.c +4 -2
@@ -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;
Signature · 1 line
2.31.1.272.g89b43f80a514
Quoted 22 lines · 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.
--
VireshViresh 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 callset_boost()
cpufreq: Introducepolicy_set_boost()
cpufreq: Preserve policy's boost state after resume
cpufreq: Force sync policy boost with global boost on sysfs update
Quoted 4 lines
drivers/cpufreq/acpi-cpufreq.c | 38 ++++++++++------------ drivers/cpufreq/cpufreq.c | 58 ++++++++++++++++++++-------------- 2 files changed, 50 insertions(+), 46 deletions(-)