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

[Sumit Gupta <sumitg@nvidia.com>]

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@arm.com> 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@nvidia.com> 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@nvidia.com> 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