Re: [PATCH v3] cpufreq: CPPC: Update FIE arch_freq_scale in ticks for non-PCC regs

[Beata Michalska <beata.michalska@arm.com>]

Hi Jie,
On Tue, Nov 04, 2025 at 02:50:39PM +0800, Jie Zhan wrote:
> Currently, the CPPC Frequency Invariance Engine (FIE) is invoked from the
> scheduler tick but defers the update of arch_freq_scale to a separate
> thread because cppc_get_perf_ctrs() would sleep if the CPC regs are in PCC.
> 
> However, this deferred update mechanism is unnecessary and introduces extra
> overhead for non-PCC register spaces (e.g. System Memory or FFH), where
> accessing the regs won't sleep and can be safely performed from the tick
> context.
> 
> Furthermore, with the CPPC FIE registered, it throws repeated warnings of
> "cppc_scale_freq_workfn: failed to read perf counters" on our platform with
> the CPC regs in System Memory and a power-down idle state enabled.  That's
> because the remote CPU can be in a power-down idle state, and reading its
> perf counters returns 0.  Moving the FIE handling back to the scheduler
> tick process makes the CPU handle its own perf counters, so it won't be
> idle and the issue would be inherently solved.
> 
> To address the above issues, update arch_freq_scale directly in ticks for
> non-PCC regs and keep the deferred update mechanism for PCC regs.
Something about it just didn’t sit right with me, and apparently, it needed some
time to settle down - thus the delay.

It all looks sensible though it might be worth to considered applying
the change on a per-CPU basis, as, in theory at least, different address
spaces are allowed for different registers (at least according to the ACPI
spec, if I read it right).
So I was thinking about smth along the lines of:

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 6c684e54fe01..07f4e59f2f0a 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1431,38 +1431,47 @@ EXPORT_SYMBOL_GPL(cppc_get_perf_caps);
  *
  * Return: true if any of the counters are in PCC regions, false otherwise
  */
-bool cppc_perf_ctrs_in_pcc(void)
+bool cppc_perf_ctrs_in_pcc(unsigned int cpu)
 {
-	int cpu;
+	struct cpc_register_resource *ref_perf_reg;
+	struct cpc_desc *cpc_desc;
 
-	for_each_present_cpu(cpu) {
-		struct cpc_register_resource *ref_perf_reg;
-		struct cpc_desc *cpc_desc;
+	cpc_desc = per_cpu(cpc_desc_ptr, cpu);
 
-		cpc_desc = per_cpu(cpc_desc_ptr, cpu);
+	if (CPC_IN_PCC(&cpc_desc->cpc_regs[DELIVERED_CTR]) ||
+	    CPC_IN_PCC(&cpc_desc->cpc_regs[REFERENCE_CTR]) ||
+	    CPC_IN_PCC(&cpc_desc->cpc_regs[CTR_WRAP_TIME]))
+		return true;
 
-		if (CPC_IN_PCC(&cpc_desc->cpc_regs[DELIVERED_CTR]) ||
-		    CPC_IN_PCC(&cpc_desc->cpc_regs[REFERENCE_CTR]) ||
-		    CPC_IN_PCC(&cpc_desc->cpc_regs[CTR_WRAP_TIME]))
-			return true;
 
+	ref_perf_reg = &cpc_desc->cpc_regs[REFERENCE_PERF];
 
-		ref_perf_reg = &cpc_desc->cpc_regs[REFERENCE_PERF];
+	/*
+	 * If reference perf register is not supported then we should
+	 * use the nominal perf value
+	 */
+	if (!CPC_SUPPORTED(ref_perf_reg))
+		ref_perf_reg = &cpc_desc->cpc_regs[NOMINAL_PERF];
 
-		/*
-		 * If reference perf register is not supported then we should
-		 * use the nominal perf value
-		 */
-		if (!CPC_SUPPORTED(ref_perf_reg))
-			ref_perf_reg = &cpc_desc->cpc_regs[NOMINAL_PERF];
+	if (CPC_IN_PCC(ref_perf_reg))
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(cppc_perf_ctrs_in_pcc);
 
-		if (CPC_IN_PCC(ref_perf_reg))
+bool cppc_any_perf_ctrs_in_pcc(void)
+{
+	int cpu;
+
+	for_each_present_cpu(cpu) {
+		if (cppc_perf_ctrs_in_pcc(cpu))
 			return true;
 	}
 
 	return false;
 }
-EXPORT_SYMBOL_GPL(cppc_perf_ctrs_in_pcc);
+EXPORT_SYMBOL_GPL(cppc_any_perf_ctrs_in_pcc);
 
 /**
  * cppc_get_perf_ctrs - Read a CPU's performance feedback counters.
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 4fcaec7e2034..fdf5a49c04ed 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -48,7 +48,6 @@ struct cppc_freq_invariance {
 };
 
 static DEFINE_PER_CPU(struct cppc_freq_invariance, cppc_freq_inv);
-static bool perf_ctrs_in_pcc;
 static struct kthread_worker *kworker_fie;
 
 static int cppc_perf_from_fbctrs(struct cppc_perf_fb_ctrs *fb_ctrs_t0,
@@ -132,7 +131,12 @@ static void cppc_scale_freq_tick_pcc(void)
 
 static void cppc_scale_freq_tick(void)
 {
-	__cppc_scale_freq_tick(&per_cpu(cppc_freq_inv, smp_processor_id()));
+	unsigned int cpu = smp_processor_id();
+
+	cppc_perf_ctrs_in_pcc(cpu) ? cppc_scale_freq_tick_pcc()
+				   : __cppc_scale_freq_tick(
+				   			&per_cpu(cppc_freq_inv,
+				   				 cpu));
 }
 
 static struct scale_freq_data cppc_sftd = {
@@ -152,7 +156,7 @@ static void cppc_cpufreq_cpu_fie_init(struct cpufreq_policy *policy)
 		cppc_fi = &per_cpu(cppc_freq_inv, cpu);
 		cppc_fi->cpu = cpu;
 		cppc_fi->cpu_data = policy->driver_data;
-		if (perf_ctrs_in_pcc) {
+		if (cppc_perf_ctrs_in_pcc(cpu)) {
 			kthread_init_work(&cppc_fi->work, cppc_scale_freq_workfn);
 			init_irq_work(&cppc_fi->irq_work, cppc_irq_work);
 		}
@@ -193,10 +197,9 @@ static void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
 	/* policy->cpus will be empty here, use related_cpus instead */
 	topology_clear_scale_freq_source(SCALE_FREQ_SOURCE_CPPC, policy->related_cpus);
 
-	if (!perf_ctrs_in_pcc)
-		return;
-
 	for_each_cpu(cpu, policy->related_cpus) {
+		if (!cppc_perf_ctrs_in_pcc(cpu))
+			continue;
 		cppc_fi = &per_cpu(cppc_freq_inv, cpu);
 		irq_work_sync(&cppc_fi->irq_work);
 		kthread_cancel_work_sync(&cppc_fi->work);
@@ -218,14 +221,11 @@ static void __init cppc_freq_invariance_init(void)
 		.sched_deadline = 10 * NSEC_PER_MSEC,
 		.sched_period	= 10 * NSEC_PER_MSEC,
 	};
+	bool perf_ctrs_in_pcc = cppc_any_perf_ctrs_in_pcc();
 	int ret;
 
-	perf_ctrs_in_pcc = cppc_perf_ctrs_in_pcc();
-
 	if (fie_disabled != FIE_ENABLED && fie_disabled != FIE_DISABLED) {
-		if (!perf_ctrs_in_pcc) {
-			fie_disabled = FIE_ENABLED;
-		} else {
+		if (perf_ctrs_in_pcc) {
 			pr_info("FIE not enabled on systems with registers in PCC\n");
 			fie_disabled = FIE_DISABLED;
 		}
@@ -234,12 +234,12 @@ static void __init cppc_freq_invariance_init(void)
 	if (fie_disabled || !perf_ctrs_in_pcc)
 		return;
 
-	cppc_sftd.set_freq_scale = cppc_scale_freq_tick_pcc;
 
 	kworker_fie = kthread_run_worker(0, "cppc_fie");
 	if (IS_ERR(kworker_fie)) {
 		pr_warn("%s: failed to create kworker_fie: %ld\n", __func__,
 			PTR_ERR(kworker_fie));
+		kworker_fie = NULL;
 		fie_disabled = FIE_DISABLED;
 		return;
 	}
@@ -255,10 +255,8 @@ static void __init cppc_freq_invariance_init(void)
 
 static void cppc_freq_invariance_exit(void)
 {
-	if (fie_disabled || !perf_ctrs_in_pcc)
-		return;
-
-	kthread_destroy_worker(kworker_fie);
+	if (kworker_fie)
+		kthread_destroy_worker(kworker_fie);
 }
 
 #else
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 13fa81504844..3af503b12f60 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -154,7 +154,8 @@ extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
 extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
 extern int cppc_set_enable(int cpu, bool enable);
 extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
-extern bool cppc_perf_ctrs_in_pcc(void);
+extern bool cppc_perf_ctrs_in_pcc(unsigned int cpu);
+extern bool cppc_any_perf_ctrs_in_pcc(void);
 extern unsigned int cppc_perf_to_khz(struct cppc_perf_caps *caps, unsigned int perf);
 extern unsigned int cppc_khz_to_perf(struct cppc_perf_caps *caps, unsigned int freq);
 extern bool acpi_cpc_valid(void);
@@ -204,7 +205,11 @@ static inline int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps)
 {
 	return -EOPNOTSUPP;
 }
-static inline bool cppc_perf_ctrs_in_pcc(void)
+static inline bool cppc_perf_ctrs_in_pcc(unsigned int cpu)
+{
+	return false;
+}
+static inline bool cppc_any_perf_ctrs_in_pcc(void)
 {
 	return false;
 }


Additionally, it might be worth to get rid of (at least) some messages printed
on the path of reading the counters in case it is being done in tick context.

Also , I do not have access to any machine using PCC, and it would be good to
double check that as well.

---
BR
Beata
> 
> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
> ---
> We have tested this on Kunpeng SoCs with the CPC regs both in System Memory
> and FFH.  More tests on other platforms are welcome.
> 
> Changelog:
> 
> v3:
> - Stash the state of 'cppc_perf_ctrs_in_pcc' so it won't have to check the CPC
>   regs of all CPUs everywhere (Thanks to the suggestion from Beata Michalska).
> - Update the commit log, explaining more on the warning issue caused by
>   accessing perf counters on remote CPUs.
> - Drop Patch 1 that has been accepted, and rebase Patch 2 on that.
> 
> v2:
> https://lore.kernel.org/linux-pm/20250828110212.2108653-1-zhanjie9@hisilicon.com/
> - Update the cover letter and the commit log based on v1 discussion
> - Update FIE arch_freq_scale in ticks for non-PCC regs
> 
> v1:
> https://lore.kernel.org/linux-pm/20250730032312.167062-1-yubowen8@huawei.com/
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 60 ++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 9eac77c4f294..4fcaec7e2034 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -48,37 +48,31 @@ struct cppc_freq_invariance {
>  };
>  
>  static DEFINE_PER_CPU(struct cppc_freq_invariance, cppc_freq_inv);
> +static bool perf_ctrs_in_pcc;
>  static struct kthread_worker *kworker_fie;
>  
>  static int cppc_perf_from_fbctrs(struct cppc_perf_fb_ctrs *fb_ctrs_t0,
>  				 struct cppc_perf_fb_ctrs *fb_ctrs_t1);
>  
>  /**
> - * cppc_scale_freq_workfn - CPPC arch_freq_scale updater for frequency invariance
> - * @work: The work item.
> + * __cppc_scale_freq_tick - CPPC arch_freq_scale updater for frequency invariance
> + * @cppc_fi: per-cpu CPPC FIE data.
>   *
> - * The CPPC driver register itself with the topology core to provide its own
> + * The CPPC driver registers itself with the topology core to provide its own
>   * implementation (cppc_scale_freq_tick()) of topology_scale_freq_tick() which
>   * gets called by the scheduler on every tick.
>   *
>   * Note that the arch specific counters have higher priority than CPPC counters,
>   * if available, though the CPPC driver doesn't need to have any special
>   * handling for that.
> - *
> - * On an invocation of cppc_scale_freq_tick(), we schedule an irq work (since we
> - * reach here from hard-irq context), which then schedules a normal work item
> - * and cppc_scale_freq_workfn() updates the per_cpu arch_freq_scale variable
> - * based on the counter updates since the last tick.
>   */
> -static void cppc_scale_freq_workfn(struct kthread_work *work)
> +static void __cppc_scale_freq_tick(struct cppc_freq_invariance *cppc_fi)
>  {
> -	struct cppc_freq_invariance *cppc_fi;
>  	struct cppc_perf_fb_ctrs fb_ctrs = {0};
>  	struct cppc_cpudata *cpu_data;
>  	unsigned long local_freq_scale;
>  	u64 perf;
>  
> -	cppc_fi = container_of(work, struct cppc_freq_invariance, work);
>  	cpu_data = cppc_fi->cpu_data;
>  
>  	if (cppc_get_perf_ctrs(cppc_fi->cpu, &fb_ctrs)) {
> @@ -102,6 +96,14 @@ static void cppc_scale_freq_workfn(struct kthread_work *work)
>  	per_cpu(arch_freq_scale, cppc_fi->cpu) = local_freq_scale;
>  }
>  
> +static void cppc_scale_freq_workfn(struct kthread_work *work)
> +{
> +	struct cppc_freq_invariance *cppc_fi;
> +
> +	cppc_fi = container_of(work, struct cppc_freq_invariance, work);
> +	__cppc_scale_freq_tick(cppc_fi);
> +}
> +
>  static void cppc_irq_work(struct irq_work *irq_work)
>  {
>  	struct cppc_freq_invariance *cppc_fi;
> @@ -110,7 +112,14 @@ static void cppc_irq_work(struct irq_work *irq_work)
>  	kthread_queue_work(kworker_fie, &cppc_fi->work);
>  }
>  
> -static void cppc_scale_freq_tick(void)
> +/*
> + * Reading perf counters may sleep if the CPC regs are in PCC.  Thus, we
> + * schedule an irq work in scale_freq_tick (since we reach here from hard-irq
> + * context), which then schedules a normal work item cppc_scale_freq_workfn()
> + * that updates the per_cpu arch_freq_scale variable based on the counter
> + * updates since the last tick.
> + */
> +static void cppc_scale_freq_tick_pcc(void)
>  {
>  	struct cppc_freq_invariance *cppc_fi = &per_cpu(cppc_freq_inv, smp_processor_id());
>  
> @@ -121,6 +130,11 @@ static void cppc_scale_freq_tick(void)
>  	irq_work_queue(&cppc_fi->irq_work);
>  }
>  
> +static void cppc_scale_freq_tick(void)
> +{
> +	__cppc_scale_freq_tick(&per_cpu(cppc_freq_inv, smp_processor_id()));
> +}
> +
>  static struct scale_freq_data cppc_sftd = {
>  	.source = SCALE_FREQ_SOURCE_CPPC,
>  	.set_freq_scale = cppc_scale_freq_tick,
> @@ -138,8 +152,10 @@ static void cppc_cpufreq_cpu_fie_init(struct cpufreq_policy *policy)
>  		cppc_fi = &per_cpu(cppc_freq_inv, cpu);
>  		cppc_fi->cpu = cpu;
>  		cppc_fi->cpu_data = policy->driver_data;
> -		kthread_init_work(&cppc_fi->work, cppc_scale_freq_workfn);
> -		init_irq_work(&cppc_fi->irq_work, cppc_irq_work);
> +		if (perf_ctrs_in_pcc) {
> +			kthread_init_work(&cppc_fi->work, cppc_scale_freq_workfn);
> +			init_irq_work(&cppc_fi->irq_work, cppc_irq_work);
> +		}
>  
>  		ret = cppc_get_perf_ctrs(cpu, &cppc_fi->prev_perf_fb_ctrs);
>  
> @@ -177,6 +193,9 @@ static void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
>  	/* policy->cpus will be empty here, use related_cpus instead */
>  	topology_clear_scale_freq_source(SCALE_FREQ_SOURCE_CPPC, policy->related_cpus);
>  
> +	if (!perf_ctrs_in_pcc)
> +		return;
> +
>  	for_each_cpu(cpu, policy->related_cpus) {
>  		cppc_fi = &per_cpu(cppc_freq_inv, cpu);
>  		irq_work_sync(&cppc_fi->irq_work);
> @@ -201,17 +220,22 @@ static void __init cppc_freq_invariance_init(void)
>  	};
>  	int ret;
>  
> +	perf_ctrs_in_pcc = cppc_perf_ctrs_in_pcc();
> +
>  	if (fie_disabled != FIE_ENABLED && fie_disabled != FIE_DISABLED) {
> -		fie_disabled = FIE_ENABLED;
> -		if (cppc_perf_ctrs_in_pcc()) {
> +		if (!perf_ctrs_in_pcc) {
> +			fie_disabled = FIE_ENABLED;
> +		} else {
>  			pr_info("FIE not enabled on systems with registers in PCC\n");
>  			fie_disabled = FIE_DISABLED;
>  		}
>  	}
>  
> -	if (fie_disabled)
> +	if (fie_disabled || !perf_ctrs_in_pcc)
>  		return;
>  
> +	cppc_sftd.set_freq_scale = cppc_scale_freq_tick_pcc;
> +
>  	kworker_fie = kthread_run_worker(0, "cppc_fie");
>  	if (IS_ERR(kworker_fie)) {
>  		pr_warn("%s: failed to create kworker_fie: %ld\n", __func__,
> @@ -231,7 +255,7 @@ static void __init cppc_freq_invariance_init(void)
>  
>  static void cppc_freq_invariance_exit(void)
>  {
> -	if (fie_disabled)
> +	if (fie_disabled || !perf_ctrs_in_pcc)
>  		return;
>  
>  	kthread_destroy_worker(kworker_fie);
> -- 
> 2.33.0
>