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

[Rafael J. Wysocki (Intel)]

On Tue, Oct 27, 2026 at 5:36 PM Doug Smythies <dsmythies@xxxxxxxxx> wrote:
>
> On 2026.10.27 07:37 Zak Brighton Knight wrote:
>
> > From: Zak Brighton Knight <zakbrightonknight@xxxxxxxxx>
> >
> > 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 firmware sets each CPU's P-state request register,
> > MSR_IA32_PERF_CTL, to the base ratio (28), and the kernel never writes
> > the requested turbo ratio (38) again.

Well, not quite.

If this is suspend-to-RAM, all CPUs except for CPU0 are taken offline
during suspend and brought back online during resume.

So what happens is that cpufreq_suspend() runs first and calls
intel_pstate_suspend() for each CPU.  Without HWP, this just sets the
cpu->suspended flag.  In addition, in the passive mode,
cpufreq_suspend() stops the governor for all CPUs (which does nothing
for the performance governor).

When CPU offline takes place subsequently, intel_pstate_cpu_offline()
or intel_cpufreq_cpu_offline() is called for the CPU going offline.
The former clears the util update hook and invokes the latter.  The
latter sees the cpu->suspended flag set and bails out.

During resume, CPU online takes place first (except for CPU0) and
cpufreq_online() runs for each of the affected CPUs.  Each of the
involved policies is inactive at that time, so cpufreq_policy_online()
runs.  It invokes intel_pstate_cpu_online() which doesn't do anything
interesting without HWP, and then cpufreq_init_policy() is called and
it ends up calling cpufreq_set_policy().  That one calls either
intel_pstate_set_policy(), in the active mode, or the governor
->limits() callback that for the performance governor ends up calling
intel_cpufreq_target(), in the passive mode.

intel_pstate_set_policy() does the right thing and sets the P-state to
the max in the "performance" case, but intel_cpufreq_target() doesn't
update the PERF_CTL register if the new P-state value is equal to the
cached one.  That only happens for the performance governor though.

Subsequently, cpufreq_resume() runs.  It calls intel_pstate_resume()
for all CPUs (that doesn't do anything interesting without HWP) and
starts the governor for each of them in the passive mode, but that
does nothing for the performance governor.

This means a couple of things.

First of all, the fix is not needed in the active mode for CPUs other
than CPU0 because "online" takes care of them properly.  It may be
needed for CPU0 in that case because its PERF_CTL is never updated
throughout suspend-resume.  However, in the active mode, if the
performance governor is used, it is actually fine to access
pstate.current_pstate and call intel_pstate_set_pstate() without
synchronization because the update util hook is not set in that case.
If the powersave governor is used in the active mode, the update util
hook would need to be cleared at suspend time and restored at resume
time (modulo the possible offline and online in between).

Second, in the passive mode the fix is generally needed, but it is
safe to do in that case because the governor has been stopped during
suspend and will be re-started only after calling
intel_pstate_resume() (IOW, the needed synchronization is already
there).

> > Without hardware-managed
> > P-states (HWP), intel_pstate caches the last P-state it wrote and
> > skips writes that would not change it.  In passive mode the cpufreq
> > core also skips requests for the frequency it believes is already set
> > (policy->cur), because intel_cpufreq sets CPUFREQ_NEED_UPDATE_LIMITS
> > only with HWP.  Neither cached value is updated across suspend and
> > resume, so as long as the governor keeps requesting the same P-state,
> > as the performance governor always does, the register is never
> > rewritten.

Not entirely the case as you can see from the above.

> > Governors that change the request often, such as schedutil, mostly hide the problem.

Well, the question is whether or not it makes sense to restore a
pre-suspend value that's going to be overwritten immediately.  Maybe
not.

Since in the passive mode it is always safe to carry out that
restoration, it is kind of straightforward to do it always then.  In
the active mode though, it's probably better to just do it only in the
"performance" case and only on CPU0 (that doesn't undergo "offline"
and "online", but since it appears to work for you anyway, fixing the
passive mode may be sufficient for now.

I'll cut a patch for this unless someone beats me to it.

> >
> > Fix this by writing the cached P-state back to the register in
> > intel_pstate_resume() when HWP is not used, as is already done for
> > the HWP request when it is.
> >
> > 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: 001c76f05b01 ("cpufreq: intel_pstate: Generic governors support")

I'm not sure about this.  Maybe some other change related to "offline"
and "online" support.

> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Zak Brighton Knight <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 fix off.  intel_pstate.c is the same in
> > v7.3 and in the base commit below.
> >
> > Each row is one suspend to RAM and resume (rtcwake -m mem -s 25).
> > "Request" is the P-state read back from PERF_CTL on all eight CPUs
> > 5 and 17 seconds after resume; "clock" is the measured speed of a
> > single-threaded busy loop.  Before every suspend, all CPUs requested
> > 38 and the clock was about 3.8 GHz.
> >
> >  mode     governor     fix  request after resume  clock
> >  passive  performance  on   38 on all CPUs        3.8 GHz
> >  passive  performance  off  28 on all CPUs        2.8 GHz
> >  active   performance  off  38 on all CPUs        3.8 GHz

Had the changelog been accurate, this would have been 28.

> >  active   performance  on   38 on all CPUs        3.8 GHz
> >
> > The passive-mode failure was also reproduced twice on Fedora's 7.2.11
> > kernel.  In active mode the request was back at 38 after resume even
> > without the fix, so on this machine the fix only makes a difference in
> > passive mode, the default without HWP.
> >
> > Not tested: systems with HWP (their code path is unchanged, only
> > restructured), other machines and firmware, and suspend-to-idle.  The
> > patch adds no warnings with W=1; sparse was not run.  The tests were
> > scripted and run by the same AI assistant.
> >
> > drivers/cpufreq/intel_pstate.c | 16 +++++++++++++---
> > 1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > index 17158cb..53db91a 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -1371,6 +1371,7 @@ static void set_power_ctl_ee_state(bool input)
> >  }
> >
> >  static void intel_pstate_hwp_enable(struct cpudata *cpudata);
> > +static void intel_pstate_set_pstate(struct cpudata *cpu, int pstate);
> >
> >  static void intel_pstate_hwp_reenable(struct cpudata *cpu)
> >  {
> > @@ -1404,11 +1405,20 @@ static int intel_pstate_resume(struct cpufreq_policy *policy)
> >       else if (power_ctl_ee_state == POWER_CTL_EE_DISABLE)
> >               set_power_ctl_ee_state(false);
> >
> > -     if (cpu->suspended && hwp_active) {
> > +     if (cpu->suspended) {
> >               mutex_lock(&intel_pstate_limits_lock);
> >
> > -             /* Re-enable HWP, because "online" has not done that. */
> > -             intel_pstate_hwp_reenable(cpu);
> > +             if (hwp_active) {
> > +                     /* Re-enable HWP, because "online" has not done that. */
> > +                     intel_pstate_hwp_reenable(cpu);
> > +             } else {
> > +                     /*
> > +                      * PERF_CTL may have been changed by the platform
> > +                      * firmware, but it is only written when the requested
> > +                      * P-state changes, so restore it explicitly.
> > +                      */
> > +                     intel_pstate_set_pstate(cpu, cpu->pstate.current_pstate);
> > +             }
> >
> >               mutex_unlock(&intel_pstate_limits_lock);
> >       }
> >
> > base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
> > --
> > 2.56.0
>
> I confirm your findings and tested your proposed patch. It all worked fine.
> I had not seen Rafaels's reply about a possible race condition until now.
>
> Test computer: i5-10600K, 6 cores, 12 CPUs, HWP disabled.

Passive or active mode?