Re: [PATCH v6 4/5] cpufreq: CPPC: Preserve OSPM-set registers across hotplug and unload
From: Christian Loehle
Date: Thu Oct 22 2026 - 09:25:22 EST
On 10/22/26 11:10, Sumit Gupta wrote:
> Hi Christian,
>
>
> On 20/10/26 21:41, Christian Loehle wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 10/20/26 13:18, Sumit Gupta wrote:
>>> Values written to OSPM-set CPPC registers via sysfs can be lost in two
>>> ways:
>>>
>>> - Across CPU hotplug: the platform may reset a CPU's registers while it
>>> is offline.
>>> - 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:
>>>
>>> - On init(), capture each register's firmware value before the
>>> driver programs anything.
>>> - On offline(), read back each register's current value (whatever was
>>> last set via sysfs) so it can be reapplied, then restore the firmware
>>> value.
>>
>> Sorry for the late suggestion, but all the ospm-set registers I think we can
>> save ourselves some headache by just saving these at sysfs_store (and not deal
>> with potential read-errors during offline/suspend (which actually may happen!).
>>
>
> v1 [1] saved the values at store time. In v2 I switched to reading
> them in offline(), following the flow discussed in [2]. One read there
> covers every store handler and the pending boot parameter series.
Ah right!
>
> But a read error in offline() or suspend() drops the request. A failed
> reapply in online() can also leave the register at its firmware or reset
> value, which the next offline() or suspend() then saves over the
> request. Saving at store time keeps the request in both cases.
Yup, that's more reasonable IMO
>
> We could combine the two. Keep the policy across hotplug and the
> firmware restore in offline() from v2, and save the requested values
> at store time as in v1:
> - init() saves the firmware values and uses them as the initial
> requested values.
Yes, I guess if we cannot read the initial values at init() (or first
autosel enable?) there's no point in letting the driver initialize
(or allowing autosel=1 respectively)?
> - Each sysfs store updates the requested value after it succeeds.> The boot parameter series will do the same.
> - offline() and suspend() restore the firmware values without
> reading the registers.
> - online() and resume() reapply the requested values. If that fails,
> the request is kept for a later online().
>
> Does this sound fine and keep the intent of the earlier discussion?
> I will respin accordingly.
To me it does, I have an updated version for my 1/5 I'll post as soon
as I verify it works on an AMD platform too!
Thanks!
>
> Thanks,
> Sumit
>
> [1] https://lore.kernel.org/lkml/20260623095403.3407436-1-sumitg@xxxxxxxxxx/
> [2] https://lore.kernel.org/lkml/15def9ce-abbd-4ef5-bb8f-0c55b42a0181@xxxxxxx/
>
> Thanks,
> Sumit
>
>
>>> - On online(), reapply the value captured at offline() after
>>> reprogramming the performance controls. A failed write to the controls
>>> does not skip the reapply, as no other path restores these registers.
>>> - On exit(), nothing is needed, as the core calls offline() first, which
>>> already restored the firmware values.
>>>
>>> Cover the Autonomous Selection (auto_sel), Energy Performance Preference
>>> (EPP) and Autonomous Activity Window (auto_act_window) registers. Writes
>>> to EPP and auto_act_window only have meaning while auto_sel is enabled,
>>> so write auto_sel before them when enabling it and after them when
>>> disabling it. While autonomous selection stays disabled, the platform may
>>> ignore those writes.
>>>
>>> Suggested-by: Pierre Gondois <pierre.gondois@xxxxxxx>
>>> Link: https://lore.kernel.org/all/86780f97-29ee-4a72-b311-38c89434b707@xxxxxxx/
>>> Signed-off-by: Sumit Gupta <sumitg@xxxxxxxxxx>
>>> ---
>>> drivers/cpufreq/cppc_cpufreq.c | 176 ++++++++++++++++++++++++++++++++-
>>> 1 file changed, 173 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>> index 9e1214e1cb0d..1b733f15d641 100644
>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>> @@ -29,6 +29,166 @@
>>>
>>> static struct cpufreq_driver cppc_cpufreq_driver;
>>>
>>> +/*
>>> + * OSPM-set CPPC registers tracked for save/restore. A value the OS wrote is
>>> + * reapplied from online() across CPU hotplug, and the firmware value is
>>> + * restored from offline().
>>> + *
>>> + * Autonomous Selection (auto_sel) is kept first, as the registers after it
>>> + * only have meaning while it is enabled.
>>> + */
>>> +enum cppc_saved_reg_id {
>>> + CPPC_SAVED_AUTO_SEL,
>>> + CPPC_SAVED_EPP,
>>> + CPPC_SAVED_AUTO_ACT_WINDOW,
>>> + CPPC_NR_SAVED_REGS,
>>> +};
>>> +
>>> +struct cppc_saved_reg {
>>> + const char *name;
>>> + int (*get)(int cpu, u64 *val);
>>> + int (*set)(int cpu, u64 val);
>>> +};
>>> +
>>> +static const struct cppc_saved_reg cppc_saved_regs[CPPC_NR_SAVED_REGS] = {
>>> + [CPPC_SAVED_AUTO_SEL] = {
>>> + .name = "auto_sel",
>>> + .get = cppc_get_auto_sel,
>>> + .set = cppc_set_auto_sel,
>>> + },
>>> + [CPPC_SAVED_EPP] = {
>>> + .name = "epp",
>>> + .get = cppc_get_epp_perf,
>>> + .set = cppc_set_epp,
>>> + },
>>> + [CPPC_SAVED_AUTO_ACT_WINDOW] = {
>>> + .name = "auto_act_window",
>>> + .get = cppc_get_auto_act_window,
>>> + .set = cppc_set_auto_act_window,
>>> + },
>>> +};
>>> +
>>> +enum cppc_saved_type {
>>> + CPPC_SAVED_FIRMWARE,
>>> + CPPC_SAVED_REQUESTED,
>>> + CPPC_NR_SAVED_TYPES,
>>> +};
>>> +
>>> +/*
>>> + * Per-policy values saved for each register in cppc_saved_regs[], U64_MAX
>>> + * if there is none:
>>> + * CPPC_SAVED_FIRMWARE - value before the driver touched it, captured at
>>> + * init() and written back when the policy goes
>>> + * offline
>>> + * CPPC_SAVED_REQUESTED - value in effect when the policy last went offline,
>>> + * reapplied at online()
>>> + */
>>> +struct cppc_policy_state {
>>> + u64 regs[CPPC_NR_SAVED_REGS][CPPC_NR_SAVED_TYPES];
>>> +};
>>> +
>>> +static DEFINE_PER_CPU(struct cppc_policy_state, cppc_policy_state);
>>> +
>>> +/*
>>> + * Per-policy state is kept in the per-CPU variable of the first CPU the policy
>>> + * manages. related_cpus (the policy's full set of CPUs) never changes while the
>>> + * policy exists, so this CPU (unlike policy->cpu) stays the same across CPU
>>> + * hotplug, and every callback reaches the same copy.
>>> + */
>>> +static struct cppc_policy_state *
>>> +cppc_cpufreq_policy_state(struct cpufreq_policy *policy)
>>> +{
>>> + const struct cpumask *policy_cpus = policy->related_cpus;
>>> +
>>> + /*
>>> + * related_cpus is empty until the core fills it in after init(), so
>>> + * fall back to policy->cpus, which has the same first CPU.
>>> + */
>>> + if (cpumask_empty(policy_cpus))
>>> + policy_cpus = policy->cpus;
>>> +
>>> + return &per_cpu(cppc_policy_state, cpumask_first(policy_cpus));
>>> +}
>>> +
>>> +/*
>>> + * Save each register's current value, either as the firmware value, captured
>>> + * before the driver programs anything, or as the requested value, to reapply
>>> + * at online().
>>> + */
>>> +static void cppc_cpufreq_save_regs(struct cpufreq_policy *policy,
>>> + enum cppc_saved_type saved_type)
>>> +{
>>> + struct cppc_policy_state *st = cppc_cpufreq_policy_state(policy);
>>> + unsigned int cpu = policy->cpu;
>>> + u64 val;
>>> + int i;
>>> +
>>> + for (i = 0; i < CPPC_NR_SAVED_REGS; i++) {
>>> + if (cppc_saved_regs[i].get(cpu, &val))
>>> + val = U64_MAX;
>>> +
>>> + st->regs[i][saved_type] = val;
>>> + if (saved_type == CPPC_SAVED_FIRMWARE)
>>> + st->regs[i][CPPC_SAVED_REQUESTED] = U64_MAX;
>>> + }
>>> +}
>>> +
>>> +/*
>>> + * Write one tracked register, skipping it when there is no saved value.
>>> + * A register the platform does not allow writing is not an error.
>>> + */
>>> +static void cppc_cpufreq_write_saved_reg(unsigned int cpu,
>>> + enum cppc_saved_reg_id reg, u64 val,
>>> + enum cppc_saved_type saved_type)
>>> +{
>>> + const char *op = (saved_type == CPPC_SAVED_FIRMWARE) ?
>>> + "restore firmware" : "reapply saved";
>>> + int ret;
>>> +
>>> + if (val == U64_MAX)
>>> + return;
>>> +
>>> + ret = cppc_saved_regs[reg].set(cpu, val);
>>> + if (ret == -EOPNOTSUPP)
>>> + return;
>>> + if (ret)
>>> + pr_debug("Failed to %s %s=%llu on CPU%u (%d)\n", op,
>>> + cppc_saved_regs[reg].name, val, cpu, ret);
>>> +}
>>> +
>>> +/*
>>> + * Apply the saved firmware or requested value to each tracked register.
>>> + *
>>> + * Write auto_sel first when the value being applied enables autonomous
>>> + * selection and last when it disables it, so the writes to the dependent
>>> + * registers can still take effect. While autonomous selection stays disabled,
>>> + * the platform may ignore those writes. Do not enable it temporarily to force
>>> + * them through.
>>> + */
>>> +static void cppc_cpufreq_apply_saved_regs(struct cpufreq_policy *policy,
>>> + enum cppc_saved_type saved_type)
>>> +{
>>> + const struct cppc_policy_state *st = cppc_cpufreq_policy_state(policy);
>>> + unsigned int cpu = policy->cpu;
>>> + u64 auto_sel, val;
>>> + int i;
>>> +
>>> + auto_sel = st->regs[CPPC_SAVED_AUTO_SEL][saved_type];
>>> +
>>> + if (auto_sel)
>>> + cppc_cpufreq_write_saved_reg(cpu, CPPC_SAVED_AUTO_SEL, auto_sel,
>>> + saved_type);
>>> +
>>> + for (i = CPPC_SAVED_AUTO_SEL + 1; i < CPPC_NR_SAVED_REGS; i++) {
>>> + val = st->regs[i][saved_type];
>>> + cppc_cpufreq_write_saved_reg(cpu, i, val, saved_type);
>>> + }
>>> +
>>> + if (!auto_sel)
>>> + cppc_cpufreq_write_saved_reg(cpu, CPPC_SAVED_AUTO_SEL, auto_sel,
>>> + saved_type);
>>> +}
>>> +
>>> #ifdef CONFIG_ACPI_CPPC_CPUFREQ_FIE
>>> static enum {
>>> FIE_UNSET = -1,
>>> @@ -800,6 +960,8 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>> policy->cur = cppc_perf_to_khz(caps, desired_perf);
>>> cpu_data->perf_ctrls.desired_perf = desired_perf;
>>>
>>> + cppc_cpufreq_save_regs(policy, CPPC_SAVED_FIRMWARE);
>>> +
>>> ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
>>> if (ret) {
>>> pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
>>> @@ -843,12 +1005,14 @@ static void cppc_cpufreq_cpu_exit(struct cpufreq_policy *policy)
>>> *
>>> * The platform may have disabled CPPC and reset the performance controls
>>> * (desired, min and max performance) while the CPU was offline, so re-enable
>>> - * CPPC and reprogram them.
>>> + * CPPC and reprogram them. Also reapply the OSPM-set registers that offline()
>>> + * reset to firmware values.
>>> *
>>> * Report failures without returning them, or the core would free the policy and
>>> * leave the CPU without cpufreq. A failed write to the performance controls is
>>> - * not fatal, as the governor's next request programs them again. A failed CPPC
>>> - * enable stops the restore, as the writes that follow may not reach the
>>> + * not fatal, as the governor's next request programs them again. The OSPM-set
>>> + * registers are reapplied even then, as no other path restores them. A failed
>>> + * CPPC enable skips both restores, as the writes that follow may not reach the
>>> * platform.
>>> */
>>> static int cppc_cpufreq_cpu_online(struct cpufreq_policy *policy)
>>> @@ -896,6 +1060,8 @@ static int cppc_cpufreq_cpu_online(struct cpufreq_policy *policy)
>>> WRITE_ONCE(cpu_data->perf_limits_valid, true);
>>> }
>>>
>>> + cppc_cpufreq_apply_saved_regs(policy, CPPC_SAVED_REQUESTED);
>>> +
>>> out_fie:
>>> /* Restart what offline() stopped, with a new counter snapshot. */
>>> cppc_cpufreq_cpu_fie_init(policy);
>>> @@ -915,6 +1081,10 @@ static int cppc_cpufreq_cpu_offline(struct cpufreq_policy *policy)
>>> unsigned int cpu = policy->cpu;
>>> int ret;
>>>
>>> + /* Save what the OS set, and leave the platform in its pre-driver state. */
>>> + cppc_cpufreq_save_regs(policy, CPPC_SAVED_REQUESTED);
>>> + cppc_cpufreq_apply_saved_regs(policy, CPPC_SAVED_FIRMWARE);
>>> +
>>> /*
>>> * Stop the frequency invariance updates and cancel the pending work, so
>>> * that no sample spans the offline window. online() restarts them with
>>
>