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

[Sumit Gupta <sumitg@nvidia.com>]

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.

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.

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.
  - 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.

Thanks,
Sumit

[1] https://lore.kernel.org/lkml/20260623095403.3407436-1-sumitg@nvidia.com/
[2] 
https://lore.kernel.org/lkml/15def9ce-abbd-4ef5-bb8f-0c55b42a0181@arm.com/

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@arm.com>
>> Link: https://lore.kernel.org/all/86780f97-29ee-4a72-b311-38c89434b707@arm.com/
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>> ---
>>   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
>