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

[Christian Loehle <christian.loehle@arm.com>]

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

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