Re: [PATCH v2] cpufreq: intel_pstate: Restore P-state request on resume without HWP
From: Rafael J. Wysocki (Intel)
Date: Wed Oct 28 2026 - 11:59:47 EST
On Wed, Oct 28, 2026 at 10:48 AM Zak Brighton Knight
<zakbrightonknight@xxxxxxxxx> wrote:
>
> On a ThinkPad T520 with an i7-3840QM (Ivy Bridge) running coreboot,
> the CPUs stop using turbo after every suspend to RAM when intel_pstate
> is in passive mode with the performance governor: they stay at the
> 2.8 GHz base frequency instead of boosting up to 3.8 GHz, until the
> policy limits change or the system is rebooted.
>
> During resume, the platform firmware sets each CPU's P-state request
> register, MSR_IA32_PERF_CTL, to the base ratio (28). Without
> hardware-managed P-states (HWP), nothing in the passive mode writes it
> again afterwards:
>
> - When the nonboot CPUs go offline during suspend,
> intel_cpufreq_cpu_offline() bails out because cpu->suspended is
> set, so the cached P-state (cpu->pstate.current_pstate) and
> policy->cur keep the values from before suspend.
>
> - When they come back online, cpufreq_set_policy() does not start the
> governor, because cpufreq_suspended is still set at that point.
>
> - cpufreq_resume() then calls intel_pstate_resume(), which does not
> touch the register without HWP, and starts the governor. The
> performance governor asks for policy->max, which is equal to
> policy->cur, so __cpufreq_driver_target() returns without calling
> the driver. Even if it did, intel_cpufreq_update_pstate() would
> skip the write, because the requested P-state matches the cached
> one.
>
> Governors that change the request often, like schedutil, write the
> register again shortly after resume, so they mostly hide the problem.
> In the active mode, intel_pstate_set_policy() writes the register for
> the nonboot CPUs when they come back online.
>
> Fix this by adding a ->resume() callback to intel_cpufreq that writes
> the cached P-state back to the register when HWP is not used, before
> calling intel_pstate_resume(). The governor of the policy is not
> running at that point, and the write is done under the policy rwsem,
> so it cannot race with updates of the policy limits either.
>
> The problem was found and root-caused, and this patch was drafted, with
> the help of an AI coding assistant (Claude, by Anthropic) during a
> performance review of the machine described above.
>
> Fixes: 4adcf2e5829f ("cpufreq: intel_pstate: Add ->offline and ->online callbacks")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Zak Brighton Knight <zakbrightonknight@xxxxxxxxx>
This patch would correct the observed behavior with intel_pstate, but
it has drawbacks.
First, it papers over issues in the cpufreq core that may affect more
drivers in principle.
Second, it adds IPI overhead to the resume path that is entirely
avoidable AFAICS.
The first core issue related to it that should be addressed in general
is suspend/resume synchronization in it that is insufficient because
the cpufreq_suspended global variable is accessed locklessly, so its
updates are subject to reordering with respect to other operations and
may be missed by concurrent threads running on other CPUs.
Fortunately, this can be done in a straightforward way by replacing
cpufreq_suspended with a per-policy "suspended" flag updated under the
policy rwsem.
Another related core issue is that policy->cur is retained after
stopping the governor during suspend, even though it may be stale
during the subsequent resume, which generally doesn't affect drivers
implementing the .get() callback because of the
cpufreq_verify_current_freq() call in cpufreq_start_governor().
intel_pstate doesn't implement that callback though and so it allows
the stale policy->cur to be used past the governor restart during
resume and there may be other drivers that don't implement .get() in
principle.
After addressing the previous issue, fixing this one is a matter of
clearing policy->cur at suspend time.
The intel_pstate-specific issue, which is that
cpu->pstate.current_pstate is retained across suspend-resume, even
though it may be stale during resume, can also be addressed by
clearing that field at suspend time. Doing so doesn't require any
special synchronization after fixing the first core issue above
because the core will prevent the frequency from being updated and so
there will be no other cpu->pstate.current_pstate updates till resume.
So please test the attached patch (that needs to be split before
submitting etc) and let me know if it works for you.
Thanks!
---
drivers/cpufreq/cpufreq.c | 71 +++++++++++++++++++++--------------------
drivers/cpufreq/intel_pstate.c | 14 +++++++-
include/linux/cpufreq.h | 3 +
3 files changed, 54 insertions(+), 34 deletions(-)
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -66,9 +66,6 @@ bool cpufreq_supports_freq_invariance(vo
return static_branch_likely(&cpufreq_freq_invariance);
}
-/* Flag to suspend/resume CPUFreq governors */
-static bool cpufreq_suspended;
-
static inline bool has_target(void)
{
return cpufreq_driver->target_index || cpufreq_driver->target;
@@ -1956,6 +1953,19 @@ int cpufreq_generic_suspend(struct cpufr
}
EXPORT_SYMBOL(cpufreq_generic_suspend);
+static void cpufreq_suspend_policy(struct cpufreq_policy *policy)
+{
+ guard(cpufreq_policy_write)(policy);
+
+ cpufreq_stop_governor(policy);
+ policy->suspended = true;
+ /*
+ * Invalidate the cached current frequency that may be stale when the
+ * governor is restarted during resume.
+ */
+ policy->cur = 0;
+}
+
/**
* cpufreq_suspend() - Suspend CPUFreq governors.
*
@@ -1971,27 +1981,34 @@ void cpufreq_suspend(void)
if (!cpufreq_driver)
return;
- cpus_read_lock();
if (!has_target() && !cpufreq_driver->suspend)
- goto suspend;
+ return;
pr_debug("%s: Suspending Governors\n", __func__);
+ guard(cpus_read_lock)();
+
for_each_active_policy(policy) {
- if (has_target()) {
- scoped_guard(cpufreq_policy_write, policy) {
- cpufreq_stop_governor(policy);
- }
- }
+ if (has_target())
+ cpufreq_suspend_policy(policy);
if (cpufreq_driver->suspend && cpufreq_driver->suspend(policy))
pr_err("%s: Failed to suspend driver: %s\n", __func__,
cpufreq_driver->name);
}
+}
+
+static void cpufreq_resume_policy(struct cpufreq_policy *policy)
+{
+ guard(cpufreq_policy_write)(policy);
+
+ if (!policy->suspended)
+ return;
-suspend:
- cpufreq_suspended = true;
- cpus_read_unlock();
+ policy->suspended = false;
+ if (cpufreq_start_governor(policy))
+ pr_err("%s: Failed to start governor for CPU%u's policy\n",
+ __func__, policy->cpu);
}
/**
@@ -2003,16 +2020,10 @@ suspend:
void cpufreq_resume(void)
{
struct cpufreq_policy *policy;
- int ret;
if (!cpufreq_driver)
return;
- if (unlikely(!cpufreq_suspended))
- return;
-
- cpufreq_suspended = false;
-
if (!has_target() && !cpufreq_driver->resume)
return;
@@ -2023,13 +2034,7 @@ void cpufreq_resume(void)
pr_err("%s: Failed to resume driver: %s\n", __func__,
cpufreq_driver->name);
} else if (has_target()) {
- scoped_guard(cpufreq_policy_write, policy) {
- ret = cpufreq_start_governor(policy);
- }
-
- if (ret)
- pr_err("%s: Failed to start governor for CPU%u's policy\n",
- __func__, policy->cpu);
+ cpufreq_resume_policy(policy);
}
}
}
@@ -2416,8 +2421,8 @@ static int cpufreq_init_governor(struct
{
int ret;
- /* Don't start any governor operations if we are entering suspend */
- if (cpufreq_suspended)
+ /* Don't start any governor operations for suspended policies. */
+ if (policy->suspended)
return 0;
/*
* Governor might not be initiated here if ACPI _PPC changed
@@ -2460,7 +2465,7 @@ static int cpufreq_init_governor(struct
static void cpufreq_exit_governor(struct cpufreq_policy *policy)
{
- if (cpufreq_suspended || !policy->governor)
+ if (policy->suspended || !policy->governor)
return;
pr_debug("%s: for CPU %u\n", __func__, policy->cpu);
@@ -2475,7 +2480,7 @@ int cpufreq_start_governor(struct cpufre
{
int ret;
- if (cpufreq_suspended)
+ if (policy->suspended)
return 0;
if (!policy->governor)
@@ -2499,7 +2504,7 @@ int cpufreq_start_governor(struct cpufre
void cpufreq_stop_governor(struct cpufreq_policy *policy)
{
- if (cpufreq_suspended || !policy->governor)
+ if (policy->suspended || !policy->governor)
return;
pr_debug("%s: for CPU %u\n", __func__, policy->cpu);
@@ -2510,7 +2515,7 @@ void cpufreq_stop_governor(struct cpufre
static void cpufreq_governor_limits(struct cpufreq_policy *policy)
{
- if (cpufreq_suspended || !policy->governor)
+ if (policy->suspended || !policy->governor)
return;
pr_debug("%s: for CPU %u\n", __func__, policy->cpu);
@@ -2747,7 +2752,7 @@ static void cpufreq_policy_refresh(struc
* -> ask driver for current freq and notify governors about a change
*/
if (cpufreq_driver->get && has_target() &&
- (cpufreq_suspended || WARN_ON(!cpufreq_verify_current_freq(policy, false))))
+ (policy->suspended || WARN_ON(!cpufreq_verify_current_freq(policy, false))))
return;
refresh_frequency_limits(policy);
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -3389,10 +3389,12 @@ static void intel_cpufreq_cpu_exit(struc
static int intel_cpufreq_suspend(struct cpufreq_policy *policy)
{
+ struct cpudata *cpu;
+
intel_pstate_suspend(policy);
+ cpu = all_cpu_data[policy->cpu];
if (hwp_active) {
- struct cpudata *cpu = all_cpu_data[policy->cpu];
u64 value = READ_ONCE(cpu->hwp_req_cached);
/*
@@ -3403,6 +3405,16 @@ static int intel_cpufreq_suspend(struct
value &= ~HWP_DESIRED_PERF(~0L);
wrmsrq_on_cpu(cpu->cpu, MSR_HWP_REQUEST, value);
WRITE_ONCE(cpu->hwp_req_cached, value);
+ } else {
+ /*
+ * Invalidate the cached P-state value that may be stale when
+ * the governor is started again during resume.
+ *
+ * This does not require synchronization because the core
+ * prevents frequency updates for suspended policies from
+ * occurring.
+ */
+ cpu->pstate.current_pstate = 0;
}
return 0;
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -149,6 +149,9 @@ struct cpufreq_policy {
/* Pending policy->min/max update for the driver */
bool update_limits;
+ /* The policy has been suspended. */
+ bool suspended;
+
/* Cached frequency lookup from cpufreq_driver_resolve_freq. */
unsigned int cached_target_freq;
unsigned int cached_resolved_idx;