Re: [PATCH v2 1/3] cppc_cpufreq: Return desired perf in ->get() if feedback counters are 0

[Ionela Voinescu <ionela.voinescu@arm.com>]

Hi,

On Thursday 12 Sep 2024 at 15:22:29 (+0800), Jie Zhan wrote:
> The CPPC performance feedback counters could return 0 when the target cpu
> is in a deep idle state, e.g. powered off, and those counters are not
> powered.  In this case, cppc_cpufreq_get_rate() returns 0, and hence,
> cpufreq_online() gets a false error and doesn't generate a cpufreq policy,
> which happens in cpufreq_add_dev() when a new cpu device is added.
> 
> Don't take it as an error and return the frequency corresponding to the
> desired perf when the feedback counters are 0.
> 
> Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.")
> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
> Reviewed-by: Zeng Heng <zengheng4@huawei.com>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index bafa32dd375d..6aa3af56924b 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -748,18 +748,33 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>  
>  	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
>  	if (ret)
> -		return 0;
> +		goto out_err;
>  
>  	udelay(2); /* 2usec delay between sampling */
>  
>  	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
>  	if (ret)
> -		return 0;
> +		goto out_err;
>  
>  	delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>  					       &fb_ctrs_t1);
>  
>  	return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
> +
> +out_err:
> +	/*
> +	 * Feedback counters could be 0 when cores are powered down.
> +	 * Take desired perf for reflecting frequency in this case.
> +	 */
> +	if (ret == -EFAULT) {
> +		ret = cppc_get_desired_perf(cpu, &delivered_perf);
> +		if (ret)
> +			return 0;
> +
> +		return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
> +	}
> +
> +	return 0;
>  }

A possible (slimmer) alternative implementation for you to consider
(this merges patches 1 & 2):

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index bafa32dd375d..c16be9651a6f 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work)

        perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs,
                                     &fb_ctrs);
+       if (!perf)
+               perf = cpu_data->perf_ctrls.desired_perf;
+
        cppc_fi->prev_perf_fb_ctrs = fb_ctrs;

        perf <<= SCHED_CAPACITY_SHIFT;
@@ -726,7 +729,7 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,

        /* Check to avoid divide-by zero and invalid delivered_perf */
        if (!delta_reference || !delta_delivered)
-               return cpu_data->perf_ctrls.desired_perf;
+               return 0;

        return (reference_perf * delta_delivered) / delta_reference;
 }
@@ -736,7 +739,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
        struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
        struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
        struct cppc_cpudata *cpu_data;
-       u64 delivered_perf;
+       u64 delivered_perf = 0;
        int ret;

        if (!policy)
@@ -747,19 +750,22 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
        cpufreq_cpu_put(policy);

        ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
-       if (ret)
-               return 0;
-
-       udelay(2); /* 2usec delay between sampling */
-
-       ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
-       if (ret)
-               return 0;
+       if (!ret) {
+               udelay(2); /* 2usec delay between sampling */
+               ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
+       }
+       if (!ret)
+               delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
+                                                      &fb_ctrs_t1);
+       if ((ret == -EFAULT) || !delivered_perf) {
+               if (cppc_get_desired_perf(cpu, &delivered_perf))
+                       delivered_perf = cpu_data->perf_ctrls.desired_perf;
+       }

-       delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
-                                              &fb_ctrs_t1);
+       if (delivered_perf)
+               return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);

-       return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
+       return 0;
 }

disclaimer: not fully checked so likely not "production ready" code :)

Hope it helps,
Ionela.

>  
>  static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
> -- 
> 2.33.0
>