Hi,

On Friday 13 Sep 2024 at 20:05:50 (+0800), Jie Zhan wrote:

Hi Ionela,

On 12/09/2024 17:43, Ionela Voinescu wrote:

...

quoted

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

I think it's better to just return here.
If feedback counters are successfully read but unchanged, the following
calculation and update in cppc_scale_freq_workfn() is meaningless because it
won't change anything.

Agreed!

quoted

        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;

This makes sense to me.
Here is probably why Patch 2 looks bulky.

quoted

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

TBH, 'if (!ret)' style looks very strange to me.
We haven't done so anywhere in cppc_cpufreq, so let's keep consistency and make
it easier for people to read and maintain?

I agree it's a bit of a difficult read, that's why I only sent my code
as a suggestion. I did like the benefit of not having to have two
different calls to cppc_perf_to_khz() and making the code below common
for the error and non-error paths. But it's up to you. 

You need a check here for !delivered_perf (when at least one of the
deltas is 0) in which case it would be good to take the same error path
below. Something like:

            if(delivered_perf)
	            return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
	    else
		ret = -EFAULT;

That's why I did the tricky if/else dance above as we need to take the
error path below for multiple cases.

Thanks,
Ionela.

          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) {
+               if (cppc_get_desired_perf(cpu, &delivered_perf))
+                       delivered_perf = cpu_data->perf_ctrls.desired_perf;
+
+               return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
+       }
+
+       return 0;
   }

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

Thanks indeed!
Jie