Re: [PATCH] cpufreq: CPPC: Preserve OSPM-set registers across hotplug and unload

[Sumit Gupta]

On 07/07/26 00:04, Rafael J. Wysocki (Intel) wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Jul 1, 2026 at 6:25 PM Pierre Gondois <pierre.gondois@xxxxxxx> wrote:
> > Hello Rafael, Sumit,
> >
> > On 6/24/26 15:20, Rafael J. Wysocki wrote:
> > > On Wed, Jun 24, 2026 at 2:56 PM Sumit Gupta<sumitg@xxxxxxxxxx> wrote:
> > > > Hi Rafael,
> > > >
> > > >
> > > > On 23/06/26 16:30, Rafael J. Wysocki wrote:
> > > > > External email: Use caution opening links or attachments
> > > > >
> > > > >
> > > > > On Tue, Jun 23, 2026 at 11:54 AM Sumit Gupta<sumitg@xxxxxxxxxx> wrote:
> > > > > > Values written to OSPM-set CPPC registers (via sysfs or the autonomous
> > > > > > boot parameter) can be lost in two ways:
> > > > > >
> > > > > >      - Across CPU hotplug: the platform may reset a CPU's registers when it
> > > > > >        is offlined.
> > > > > >      - On driver unload: the value the driver wrote is left in the register
> > > > > >        instead of returning to its pre-driver state.
> > > > > >
> > > > > > Add a small table-driven mechanism that handles both:
> > > > > >
> > > > > >      - Capture each register's firmware value when a CPU is first seen and
> > > > > >        restore it on driver unload.
> > > > > >      - Record the last value the driver set and reapply it from ->init()
> > > > > >        when the policy is reactivated after CPU hotplug.
> > > > > I'm not sure if this is a good idea TBH.
> > > > >
> > > > > The overall system state when the CPU goes online may be completely
> > > > > different from the system state when the CPU was online last time, so
> > > > > there is no reason to restore its settings from before offline, at
> > > > > least in principle.
> > > > These are values userspace deliberately set, more like a frequency QoS
> > > > request that persists until userspace changes it than transient state.
> > > > It's platform dependent though, as the platform I test on preserves them
> > > > across hotplug, but where the platform resets the register on offline
> > > > the value is silently lost. Should the kernel re-apply it on online,
> > > > or is that better left to userspace? If so, I will drop the hotplug reapply
> > > > and keep only the restore on driver unload.
> > > >
> > > > intel_pstate and amd-pstate already re-sync these on CPU online, as they
> > > > keep the policy across hotplug via ->online()/->offline(). So for them
> > > > it's re-syncing the hardware, not restoring state from before offline.
> > > >
> > > > cppc_cpufreq has no ->online()/->offline() today, so it fully tears the
> > > > policy down and rebuilds it, which is why this reads as "restoring
> > > > settings from before offline".
> > > > Would adding ->online()/->offline() be acceptable, with policy preserved
> > > > and ->online() just re-syncing the registers the platform reset?
> > > That would be a better approach IMV.
> > Just an example to be sure I understand correctly.
> >
> > 1. on a platform, auto_sel=0 by default
> > 2. cppc_cpufreq driver is loaded + we set auto_sel=1
> > 3. all the CPUs of a policy are unplugged
> > 4. cppc_cpufreq driver is unloaded
> >
> > To be sure we let the platform in the default state,
> > we should set auto_sel=0 in step A.3, i.e. in the
> > .offline() callback.
> > Doing it at A.4 (i.e. in the .exit()) would not be possible:
> > the CPU's registers might not be available anymore once offline.
> >
> > IoW, we should only need:
> > .init()
> > \-cppc_cpufreq_save_firmware_regs()
> > firmware_captured=0, save the firmware regs.
> >
> > .online()
> > \-cppc_cpufreq_save_firmware_regs()
> > firmware_captured=1, restore the user (or previous)
> > values that were saved in the previous .offline() call
> > (as .online() is not called for new policies).
> >
> > .offline()
> > \-cppc_cpufreq_restore_firmware_regs()
> > Save the user/current regs values
> > + Restore the firmware registers.
> >
> > And it should be ok to remove:
> > - cppc_cpufreq_reapply_requested_regs()
> > - the firmware_captured field.
> > If during .init(), we init requested_val to the firmware
> > values aswell, firmware_captured should be useless
> This sounds about right to me.

Thanks, will restructure accordingly and drop firmware_captured. Flow:

->init()
\- cppc_cpufreq_save_firmware_regs():
   Read regs and save to firmware_val.

->offline()
\- cppc_cpufreq_save_req_and_restore_firmware_regs():
   Re-read regs and save to requested_val.
   Then restore firmware_val to regs.

->online()
\- cppc_cpufreq_reapply_requested_regs():
   Reapply requested_val (saved in ->offline()) to regs.

I think it's better to keep the cppc_cpufreq_reapply_requested_regs()
helper and call it from online() rather than reuse save_firmware_regs().

init() and online() work in opposite directions:
 - init() / save_firmware_regs() reads the registers into firmware_val.
 - online() / reapply_requested_regs() writes requested_val back out to
   the registers.

A single shared function can't do both directions without the flag (i.e.
firmware_captured) even when init() seeds requested_val to firmware_val.
Reusing save_firmware_regs() for online() re-reads the register (which
holds the firmware value offline() just restored), resets requested_val
back to firmware, and writes it out, overwriting the user's value
instead of reapplying it.

So better to keep the three single-purpose helpers, which also read more
clearly, unless there is a clean flag-free way to merge them that I am
missing.

Thanks,
Sumit