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

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

[Sumit Gupta]

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)
+{
+	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] = {
+	[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)
+{
+	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);
 		}
 
 		/* 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)
+		cppc_save_requested(cpu_data, sib, CPPC_SAVED_OSPM_NOMINAL_PERF, perf);
+
 	return count;
 
 rollback:
@@ -1258,13 +1438,11 @@ static int __init cppc_cpufreq_init(void)
 
 static void __exit cppc_cpufreq_exit(void)
 {
-	unsigned int cpu;
-
-	for_each_present_cpu(cpu)
-		cppc_set_auto_sel(cpu, false);
-
 	cpufreq_unregister_driver(&cppc_cpufreq_driver);
 	cppc_freq_invariance_exit();
+
+	/* Restore auto_sel and the other saved registers to their firmware value. */
+	cppc_cpufreq_restore_firmware_regs();
 }
 
 module_param_cb(auto_sel_mode, &auto_sel_mode_ops, &auto_sel_mode, 0444);
-- 
2.34.1

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

[Rafael J. Wysocki]

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.

> 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)
> +{
> +       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] = {
> +       [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)
> +{
> +       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);
>                 }
>
>                 /* 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)
> +               cppc_save_requested(cpu_data, sib, CPPC_SAVED_OSPM_NOMINAL_PERF, perf);
> +
>         return count;
>
>  rollback:
> @@ -1258,13 +1438,11 @@ static int __init cppc_cpufreq_init(void)
>
>  static void __exit cppc_cpufreq_exit(void)
>  {
> -       unsigned int cpu;
> -
> -       for_each_present_cpu(cpu)
> -               cppc_set_auto_sel(cpu, false);
> -
>         cpufreq_unregister_driver(&cppc_cpufreq_driver);
>         cppc_freq_invariance_exit();
> +
> +       /* Restore auto_sel and the other saved registers to their firmware value. */
> +       cppc_cpufreq_restore_firmware_regs();
>  }
>
>  module_param_cb(auto_sel_mode, &auto_sel_mode_ops, &auto_sel_mode, 0444);
> --
> 2.34.1
>
>

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

[Sumit Gupta]

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?

Thanks,
Sumit

>> 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)
>> +{
>> +       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] = {
>> +       [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)
>> +{
>> +       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);
>>                  }
>>
>>                  /* 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)
>> +               cppc_save_requested(cpu_data, sib, CPPC_SAVED_OSPM_NOMINAL_PERF, perf);
>> +
>>          return count;
>>
>>   rollback:
>> @@ -1258,13 +1438,11 @@ static int __init cppc_cpufreq_init(void)
>>
>>   static void __exit cppc_cpufreq_exit(void)
>>   {
>> -       unsigned int cpu;
>> -
>> -       for_each_present_cpu(cpu)
>> -               cppc_set_auto_sel(cpu, false);
>> -
>>          cpufreq_unregister_driver(&cppc_cpufreq_driver);
>>          cppc_freq_invariance_exit();
>> +
>> +       /* Restore auto_sel and the other saved registers to their firmware value. */
>> +       cppc_cpufreq_restore_firmware_regs();
>>   }
>>
>>   module_param_cb(auto_sel_mode, &auto_sel_mode_ops, &auto_sel_mode, 0444);
>> --
>> 2.34.1
>>
>>

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

[Rafael J. Wysocki]

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.

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

[Pierre Gondois]

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


> +{
> +	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 ?

> +{
> +	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() ?


>   		}
>   
>   		/* 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.


> +		cppc_save_requested(cpu_data, sib, CPPC_SAVED_OSPM_NOMINAL_PERF, perf);
> +
>   	return count;
>   
>   rollback:
> @@ -1258,13 +1438,11 @@ static int __init cppc_cpufreq_init(void)
>   
>   static void __exit cppc_cpufreq_exit(void)
>   {
> -	unsigned int cpu;
> -
> -	for_each_present_cpu(cpu)
> -		cppc_set_auto_sel(cpu, false);
> -
>   	cpufreq_unregister_driver(&cppc_cpufreq_driver);
>   	cppc_freq_invariance_exit();
> +
> +	/* Restore auto_sel and the other saved registers to their firmware value. */
> +	cppc_cpufreq_restore_firmware_regs();
>   }
>   
>   module_param_cb(auto_sel_mode, &auto_sel_mode_ops, &auto_sel_mode, 0444);
>

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

[Pierre Gondois]

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

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

[Rafael J. Wysocki (Intel)]

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.

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

[Sumit Gupta]

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

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

[Sumit Gupta]

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