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

[Sumit Gupta <sumitg@nvidia.com>]

On 01/07/26 21:54, Pierre Gondois wrote:
> External email: Use caution opening links or attachments
>
>
> Hello Sumit,
>
> On 6/23/26 11:54, Sumit Gupta 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.
>>
>> The firmware value is captured on a CPU's first activation rather than
>> once at module load, so CPUs offline at boot or hot-added later are
>> covered.
>>
>> Reapply is only needed on a full policy teardown and bring-up, which 
>> goes
>> through ->init(). In a SHARED_TYPE_ANY policy, offlining a single CPU
>> leaves the shared register untouched, so nothing is lost there.
>>
>> Cover the OSPM Nominal Performance, Autonomous Selection
>> (auto_sel) and Energy Performance Preference (EPP) registers. For
>> auto_sel it replaces the previous unconditional
>> cppc_set_auto_sel(cpu, false) on unload with a restore of the firmware
>> value captured at the CPU's first init.
>>
>> Suggested-by: Pierre Gondois<pierre.gondois@arm.com>
>> Signed-off-by: Sumit Gupta<sumitg@nvidia.com>
>> ---
>>
>> This applies on top of (not yet merged):
>>   [1] ACPI: CPPC: Add ospm_nominal_perf support
>> https://lore.kernel.org/lkml/20260615185934.2383514-1-sumitg@nvidia.com/
>>   [2] cpufreq: CPPC: add autonomous mode boot parameter support
>> https://lore.kernel.org/lkml/20260623080652.3353386-1-sumitg@nvidia.com/
>>
>>   drivers/cpufreq/cppc_cpufreq.c | 194 +++++++++++++++++++++++++++++++--
>>   1 file changed, 186 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c 
>> b/drivers/cpufreq/cppc_cpufreq.c
>> index a3fabfb07fbe..d6ea2cbde187 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -28,6 +28,174 @@
>>
>>   static struct cpufreq_driver cppc_cpufreq_driver;
>>
>> +/*
>> + * OSPM-set CPPC registers tracked for save/restore. A value set via 
>> sysfs or
>> + * the autonomous boot parameter is reapplied across CPU hotplug, 
>> and the
>> + * firmware value is restored on driver unload.
>> + */
>> +enum cppc_saved_reg_id {
>> +     CPPC_SAVED_OSPM_NOMINAL_PERF,
>> +     CPPC_SAVED_AUTO_SEL,
>> +     CPPC_SAVED_EPP,
>> +     CPPC_NR_SAVED_REGS,
>> +};
>> +
>> +struct cppc_saved_reg {
>> +     int (*get)(int cpu, u64 *val);
>> +     int (*set)(int cpu, u64 val);
>> +};
>> +
>> +/* u64 wrappers so the bool auto_sel register fits the table 
>> signatures. */
>> +static int cppc_get_auto_sel_u64(int cpu, u64 *val)
>
> Should these functions be moved to cppc_acpi.c ?
> IIUC, the issue is that cppc_set_auto_sel()/cppc_get_auto_sel()
> don't have the right prototype for the above function pointers.
> IMO there should still be moved to cppc_acpi.c
>

Agreed, I will move cppc_get/set_auto_sel_u64() into cppc_acpi.c in
a separate patch before this.


>
>> +{
>> +     bool enable;
>> +     int ret;
>> +
>> +     ret = cppc_get_auto_sel(cpu, &enable);
>> +     if (ret)
>> +             return ret;
>> +
>> +     *val = enable;
>> +     return 0;
>> +}
>> +
>> +static int cppc_set_auto_sel_u64(int cpu, u64 val)
>> +{
>> +     return cppc_set_auto_sel(cpu, !!val);
>> +}
>> +
>> +static const struct cppc_saved_reg 
>> cppc_saved_regs[CPPC_NR_SAVED_REGS] = {
> Ideally it would be nice
>> +     [CPPC_SAVED_OSPM_NOMINAL_PERF] = {
>> +             cppc_get_ospm_nominal_perf, cppc_set_ospm_nominal_perf,
>> +     },
>> +     [CPPC_SAVED_AUTO_SEL] = {
>> +             cppc_get_auto_sel_u64, cppc_set_auto_sel_u64,
>> +     },
>> +     [CPPC_SAVED_EPP] = {
>> +             cppc_get_epp_perf, cppc_set_epp,
>> +     },
>> +};
>> +
>> +/*
>> + * Per-CPU saved state for each register in cppc_saved_regs[]:
>> + *   firmware_val      - register value before the driver touched 
>> it, restored
>> + *                       on unload
>> + *   requested_val     - last value the driver set (sysfs or boot 
>> parameter),
>> + *                       reapplied on policy reactivation
>> + *   firmware_captured - whether firmware_val has been read, so a 
>> not-yet-seen
>> + *                       CPU isn't mistaken for one whose firmware 
>> value is 0
>> + */
>> +struct cppc_saved_state {
>> +     u64 firmware_val;
>> +     u64 requested_val;
>> +     bool firmware_captured;
>> +};
>> +
>> +/*
>> + * Per-CPU and not tied to a policy, so the saved values survive policy
>> + * teardown/bring-up across CPU hotplug. cpu_data->perf_ctrls is 
>> per-policy
>> + * and freed on policy ->exit.
>> + */
>> +static DEFINE_PER_CPU(struct cppc_saved_state[CPPC_NR_SAVED_REGS], 
>> cppc_saved_state);
>> +
>> +static void cppc_cache_perf_ctrls(struct cppc_cpudata *cpu_data,
>> +                               enum cppc_saved_reg_id reg, u64 val)
> Is it necessary to keep the cpu_data->perf_ctrl.[auto_sel|energy_perf]
> values synced ?
>
> AUTO_SEL_ENABLE can be set using: - cppc_set_epp_perf(): AUTO_SEL_ENABLE
> + ENERGY_PERF - cppc_set_auto_sel(): AUTO_SEL_ENABLE ENERGY_PERF: -
> cppc_set_epp_perf(): AUTO_SEL_ENABLE + ENERGY_PERF - cppc_set_epp():
> ENERGY_PERF
>
> cppc_set_epp_perf() is not used in cppc_cpufreq.c, so
> auto_sel/energy_perf should not be overwritten inadvertently right ?
>

Yes. I will drop the perf_ctrls syncing (cppc_cache_perf_ctrls()).

>> +{
>> +     switch (reg) {
>> +     case CPPC_SAVED_AUTO_SEL:
>> +             cpu_data->perf_ctrls.auto_sel = val;
>> +             break;
>> +     case CPPC_SAVED_EPP:
>> +             cpu_data->perf_ctrls.energy_perf = val;
>> +             break;
>> +     default:
>> +             break;
>> +     }
>> +}
>> +
>> +/*
>> + * Save the requested value for the given register and CPU, to be 
>> reapplied when
>> + * the policy is reactivated after CPU hotplug. Also update the 
>> per-policy
>> + * perf_ctrls copy so the saved and current values stay in sync.
>> + */
>> +static void cppc_save_requested(struct cppc_cpudata *cpu_data, 
>> unsigned int cpu,
>> +                             enum cppc_saved_reg_id reg, u64 val)
>> +{
>> +     per_cpu(cppc_saved_state, cpu)[reg].requested_val = val;
>> +     cppc_cache_perf_ctrls(cpu_data, reg, val);
>> +}
>> +
>> +/*
>> + * Reapply each register's last requested value from ->init(), so a 
>> value set
>> + * via sysfs or the boot parameter survives a policy teardown and 
>> bring-up
>> + * across CPU hotplug. Also keep perf_ctrls in sync with it.
>> + */
>> +static void cppc_cpufreq_reapply_requested_regs(struct 
>> cpufreq_policy *policy)
>> +{
>> +     struct cppc_cpudata *cpu_data = policy->driver_data;
>> +     unsigned int cpu, i;
>> +     u64 val;
>> +
>> +     for_each_cpu(cpu, policy->cpus) {
>> +             for (i = 0; i < CPPC_NR_SAVED_REGS; i++) {
>> +                     val = per_cpu(cppc_saved_state, 
>> cpu)[i].requested_val;
>> +                     if (val == U64_MAX)
>> +                             continue;
>> +
>> +                     cppc_saved_regs[i].set(cpu, val);
>> +
>> +                     /* Keep perf_ctrls in sync via the policy's 
>> CPU. */
>> +                     if (cpu == policy->cpu)
>> +                             cppc_cache_perf_ctrls(cpu_data, i, val);
>> +             }
>> +     }
>> +}
>> +
>> +/*
>> + * On a CPU's first ->init(), capture each register's firmware value 
>> to be
>> + * restored on driver unload. Later calls for the same CPU are a 
>> no-op. Capturing
>> + * from ->init() rather than module load covers CPUs that appear 
>> later. Also seed
>> + * requested_val to U64_MAX so its zeroed default is not taken as a 
>> request for 0.
>> + */
>> +static void cppc_cpufreq_save_firmware_regs(struct cpufreq_policy 
>> *policy)
>> +{
>> +     unsigned int cpu, i;
>> +     u64 val;
>> +
>> +     for_each_cpu(cpu, policy->cpus) {
>> +             for (i = 0; i < CPPC_NR_SAVED_REGS; i++) {
>> +                     struct cppc_saved_state *s =
>> +                             &per_cpu(cppc_saved_state, cpu)[i];
>> +
>> +                     /* Capture once per CPU; skip if already 
>> recorded. */
>> +                     if (s->firmware_captured)
>> +                             continue;
>> +
>> +                     if (cppc_saved_regs[i].get(cpu, &val))
>> +                             val = U64_MAX;
>> +                     s->firmware_val = val;
>> +                     s->requested_val = U64_MAX;
>> +                     s->firmware_captured = true;
>> +             }
>> +     }
>> +}
>> +
>> +/* On driver unload, restore each captured CPU's firmware value. */
>> +static void cppc_cpufreq_restore_firmware_regs(void)
>> +{
>> +     unsigned int cpu, i;
>> +
>> +     for_each_present_cpu(cpu) {
>> +             for (i = 0; i < CPPC_NR_SAVED_REGS; i++) {
>> +                     struct cppc_saved_state *s =
>> +                             &per_cpu(cppc_saved_state, cpu)[i];
>> +
>> +                     if (s->firmware_captured && s->firmware_val != 
>> U64_MAX)
>> +                             cppc_saved_regs[i].set(cpu, 
>> s->firmware_val);
>> +             }
>> +     }
>> +}
>> +
>>   /* Autonomous Selection boot parameter modes */
>>   enum {
>>       AUTO_SEL_DISABLED = 0,
>> @@ -766,6 +934,9 @@ static int cppc_cpufreq_cpu_init(struct 
>> cpufreq_policy *policy)
>>       policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
>>       cpu_data->perf_ctrls.desired_perf = caps->highest_perf;
>>
>> +     /* Capture a CPU's firmware values on its first init, before 
>> any driver write. */
>> +     cppc_cpufreq_save_firmware_regs(policy);
>> +
>>       /*
>>        * Enable autonomous mode on first init if boot param is set.
>>        * Check last_governor to detect first init and skip if auto_sel
>> @@ -812,7 +983,7 @@ static int cppc_cpufreq_cpu_init(struct 
>> cpufreq_policy *policy)
>>                       if (ret && ret != -EOPNOTSUPP)
>>                               pr_warn("Failed to set EPP for CPU%d 
>> (%d)\n", cpu, ret);
>>                       else if (!ret)
>> - cpu_data->perf_ctrls.energy_perf = epp;
>> +                             cppc_save_requested(cpu_data, cpu, 
>> CPPC_SAVED_EPP, epp);
>
> It would be nice to handle this in a more generic way.
>
> ------
>
> Theoretically, we should save all the writable CPPC registers,
> not only ospm_nominal_perf/auto_sel/epp. Maybe saving the
> value could be done in cppc_cpufreq_sysfs_store_u64() ?
>

Agreed on handling it generically.
But we can't use cppc_cpufreq_sysfs_store_u64() for this, as it's only
used by auto_act_window and perf_limited. The others (auto_sel, EPP,
ospm_nominal) have their own store handlers, and some values also come
from the boot parameter. None of those pass through that helper, so
saving there would cover only a subset of the registers.

Instead, as you suggested in the other thread, re-reading each tracked
register in offline() and reapplying it in online() works better here.
It captures the value regardless of how it was set.

And we can limit this to the OSPM set values rather than every writable
register:
  - perf_limited is write-to-clear.
  - min/max/desired are governor-managed. cppc_cpufreq_set_target()
    reprograms them from policy->min/max on every transition.
    So preserving them here would conflict with the governor.


>
>>               }
>>
>>               /* Program min/max/desired into CPPC regs (non-fatal on 
>> failure). */
>> @@ -826,7 +997,7 @@ static int cppc_cpufreq_cpu_init(struct 
>> cpufreq_policy *policy)
>>                       pr_warn("auto_sel CPU%d failed (%d); using OS 
>> mode\n",
>>                               cpu, ret);
>>               else if (!ret)
>> -                     cpu_data->perf_ctrls.auto_sel = true;
>> +                     cppc_save_requested(cpu_data, cpu, 
>> CPPC_SAVED_AUTO_SEL, true);
>>       }
>>
>>       if (cpu_data->perf_ctrls.auto_sel) {
>> @@ -850,6 +1021,10 @@ static int cppc_cpufreq_cpu_init(struct 
>> cpufreq_policy *policy)
>>       }
>>
>>       cppc_cpufreq_cpu_fie_init(policy);
>> +
>> +     /* Reapply any saved values lost across a full policy teardown. */
>> +     cppc_cpufreq_reapply_requested_regs(policy);
>> +
>>       return 0;
>>
>>   out:
>> @@ -1039,6 +1214,8 @@ static ssize_t store_auto_select(struct 
>> cpufreq_policy *policy,
>>               }
>>       }
>>
>> +     cppc_save_requested(cpu_data, policy->cpu, CPPC_SAVED_AUTO_SEL, 
>> val);
>> +
>>       return count;
>>   }
>>
>> @@ -1111,7 +1288,7 @@ store_energy_performance_preference_val(struct 
>> cpufreq_policy *policy,
>>       if (ret)
>>               return ret;
>>
>> -     cpu_data->perf_ctrls.energy_perf = val;
>> +     cppc_save_requested(cpu_data, policy->cpu, CPPC_SAVED_EPP, val);
>>
>>       return count;
>>   }
>> @@ -1193,6 +1370,9 @@ static ssize_t store_ospm_nominal_freq(struct 
>> cpufreq_policy *policy,
>>               }
>>       }
>>
>> +     for_each_cpu(sib, policy->cpus)
> Cf. comment on the other patch, maybe we should not
> actually set/get the ospm_nominal_perf value for all the CPUs
> of the policy. We might just assume all the CPUs have the same
> register.
>

Yes, I will change that patch to operate on policy->cpu considering
a shared register. Will drop the per-sibling loop/rollback, consistent
with auto_sel/EPP.

Thanks,
Sumit