[PATCH v2] cpufreq: intel_pstate: Restore P-state request on resume without HWP

[Zak Brighton Knight]

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>
---
Changes in v2:
- Only restore the request in the passive mode, from a new ->resume()
  callback of intel_cpufreq, and do it there unconditionally, as
  suggested by Rafael.  v1 did it in intel_pstate_resume() for both
  modes, where it could race with the active-mode util update hook, as
  reported by Sashiko.
- Hold the policy rwsem around the write.  The governor of the policy
  is stopped at that point, but cpufreq_resume() has already cleared
  cpufreq_suspended, so a policy->update work item (handle_update() ->
  refresh_frequency_limits() -> cpufreq_set_policy() ->
  cpufreq_governor_limits() -> ->limits()) can still get to
  intel_cpufreq_target() for the same CPU, for example after a limit
  change during resume, and its write could then be overwritten with
  the stale request.  This is the second case in Sashiko's report;
  please drop the lock if that cannot happen there.
- Describe the suspend/resume flow in the changelog, based on Rafael's
  explanation.  When the nonboot CPUs come back online during resume,
  cpufreq_init_governor() and cpufreq_start_governor() return early
  because cpufreq_suspended is still set, so the request is only
  dropped later, by the target_freq == policy->cur check in
  __cpufreq_driver_target() (see the debug messages below).
- Fixes: 4adcf2e5829f, after which the nonboot CPUs no longer go
  through ->init on online.  Before it, intel_cpufreq_cpu_init() reset
  PERF_CTL and policy->cur to the minimum when they came back, so the
  request after resume was written.

v1: https://lore.kernel.org/linux-pm/20261027143725.3763827-1-zakbrightonknight@xxxxxxxxx/

Tested on the T520 (i7-3840QM, no HWP, coreboot 24.08 with SeaBIOS)
with v7.3 plus this patch and a temporary module parameter, not part
of this patch, to turn the new write off, built with PROVE_LOCKING.
intel_pstate.c and the cpufreq core functions involved are the same
in v7.3 and in the base commit below.  Passive mode, performance
governor, one suspend to RAM and resume per row (rtcwake -m mem
-s 25), PERF_CTL read on all eight CPUs:

  fix  before resume   5 and 17 s after resume   single-thread clock
  off  38 on all       28 on all                 2.8 GHz
  on   28 on all       38 on all                 3.8 GHz
  on   38 on all       38 on all                 3.8 GHz

The second row started from the state the first one left: the restore
writes the cached request, which was still 38.  No lockdep reports,
and debug_locks was still 1 after the three cycles.

With dynamic debug enabled for cpufreq.c, cpufreq_performance.c and
intel_pstate.c, the online part of resume logged "governor switch" and
"governor change" for CPUs 1-7, but no cpufreq_init_governor() or
cpufreq_start_governor() messages and no frequency requests.  After
"Resuming Governors", every CPU logged cpufreq_start_governor(),
"setting to 3800000 kHz" and "CPU n: cur 3800000 kHz -> target
3800000 kHz", and there were no transition notifications at all, so
intel_cpufreq_target() was not called during resume.

In v1's testing, the active mode was back at 38 on all CPUs after
resume on this machine even without a restore.  Doug Smythies saw the
same on an i5-10600K with HWP disabled (passive mode affected, active
mode fine) and tested v1 there; v2 has not been tested on that
machine yet.  sparse was not run.
The tests were scripted and run by the same AI assistant.

 drivers/cpufreq/intel_pstate.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 17158cb..67a3015 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -3408,6 +3408,27 @@ static int intel_cpufreq_suspend(struct cpufreq_policy *policy)
 	return 0;
 }
 
+static int intel_cpufreq_resume(struct cpufreq_policy *policy)
+{
+	struct cpudata *cpu = all_cpu_data[policy->cpu];
+
+	/*
+	 * Without HWP, PERF_CTL is only written when the requested P-state
+	 * changes, but the platform firmware may have changed it during
+	 * resume, so write the last request back.  The governor is not
+	 * running yet, and holding the policy rwsem serializes this with
+	 * updates of the policy limits.
+	 */
+	if (!hwp_active && cpu->suspended) {
+		guard(cpufreq_policy_write)(policy);
+
+		intel_cpufreq_perf_ctl_update(cpu, cpu->pstate.current_pstate,
+					      false);
+	}
+
+	return intel_pstate_resume(policy);
+}
+
 static struct cpufreq_driver intel_cpufreq = {
 	.flags		= CPUFREQ_CONST_LOOPS,
 	.verify		= intel_cpufreq_verify_policy,
@@ -3418,7 +3439,7 @@ static struct cpufreq_driver intel_cpufreq = {
 	.offline	= intel_cpufreq_cpu_offline,
 	.online		= intel_pstate_cpu_online,
 	.suspend	= intel_cpufreq_suspend,
-	.resume		= intel_pstate_resume,
+	.resume		= intel_cpufreq_resume,
 	.update_limits	= intel_pstate_update_limits,
 	.scale_freq_ref = intel_pstate_scale_freq_ref,
 	.name		= "intel_cpufreq",

base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
-- 
2.56.0