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

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

[Jie Zhan]

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;
 }
 
 static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
-- 
2.33.0

[PATCH v2 0/3] cppc_cpufreq: Rework ->get() error handling when cores are idle

[Jie Zhan]

CPPC feedback counters can be unchanged or 0 when cores are idle, e.g.
clock-gated or power-gated.  In such case, get the latest desired perf for
calculating frequency.  Also, the HiSilicon CPPC workaround is now handled
by the common code and can be removed.

Jie Zhan (3):
  cppc_cpufreq: Return desired perf in ->get() if feedback counters are
    0
  cppc_cpufreq: Return latest desired perf if feedback counters don't
    change
  cppc_cpufreq: Remove HiSilicon CPPC workaround

 drivers/cpufreq/cppc_cpufreq.c | 103 +++++++++------------------------
 1 file changed, 27 insertions(+), 76 deletions(-)

-- 
2.33.0

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

[Jie Zhan]

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;
 }
 
 static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
-- 
2.33.0

[PATCH v2 2/3] cppc_cpufreq: Return latest desired perf if feedback counters don't change

[Jie Zhan]

The existing cppc_perf_from_fbctrs() returns a cached desired perf if the
delta of feedback counters is 0.  Some platforms may update the real
frequency back to the desired perf reg.  Try getting the latest desired
perf first; if failed, return the cached desired perf.

Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
Reviewed-by: Zeng Heng <zengheng4@huawei.com>
---
 drivers/cpufreq/cppc_cpufreq.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 6aa3af56924b..c8fe0f1fc22b 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -715,7 +715,8 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
 				 struct cppc_perf_fb_ctrs *fb_ctrs_t1)
 {
 	u64 delta_reference, delta_delivered;
-	u64 reference_perf;
+	u64 reference_perf, desired_perf;
+	int cpu, ret;
 
 	reference_perf = fb_ctrs_t0->reference_perf;
 
@@ -725,8 +726,14 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
 				    fb_ctrs_t0->delivered);
 
 	/* Check to avoid divide-by zero and invalid delivered_perf */
-	if (!delta_reference || !delta_delivered)
-		return cpu_data->perf_ctrls.desired_perf;
+	if (!delta_reference || !delta_delivered) {
+		cpu = cpumask_first(cpu_data->shared_cpu_map);
+		ret = cppc_get_desired_perf(cpu, &desired_perf);
+		if (ret)
+			return cpu_data->perf_ctrls.desired_perf;
+
+		return desired_perf;
+	}
 
 	return (reference_perf * delta_delivered) / delta_reference;
 }
-- 
2.33.0

[PATCH v2 3/3] cppc_cpufreq: Remove HiSilicon CPPC workaround

[Jie Zhan]

Since 6c8d750f9784 ("cpufreq / cppc: Work around for Hisilicon CPPC cpufreq"),
we introduce a workround for HiSilicon platforms that do not support
performance feedback counters to get the actual frequency from the desired
performance register.  Later on, FIE is disabled in that workaround as
well.

Now the workround can be handled by the common code.  Desired perf would be
read and converted to frequency if feedback counters don't change.  FIE
would be disabled if the CPPC regs are in PCC region.

Hence, the workaround is no longer needed and can be safely removed, in an
effort to consolidate the cppc_cpufreq driver procedure.

Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
---
 drivers/cpufreq/cppc_cpufreq.c | 71 ----------------------------------
 1 file changed, 71 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index c8fe0f1fc22b..f48fc2a21fa8 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -36,24 +36,6 @@ static LIST_HEAD(cpu_data_list);
 
 static bool boost_supported;
 
-struct cppc_workaround_oem_info {
-	char oem_id[ACPI_OEM_ID_SIZE + 1];
-	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
-	u32 oem_revision;
-};
-
-static struct cppc_workaround_oem_info wa_info[] = {
-	{
-		.oem_id		= "HISI  ",
-		.oem_table_id	= "HIP07   ",
-		.oem_revision	= 0,
-	}, {
-		.oem_id		= "HISI  ",
-		.oem_table_id	= "HIP08   ",
-		.oem_revision	= 0,
-	}
-};
-
 static struct cpufreq_driver cppc_cpufreq_driver;
 
 static enum {
@@ -78,7 +60,6 @@ struct cppc_freq_invariance {
 static DEFINE_PER_CPU(struct cppc_freq_invariance, cppc_freq_inv);
 static struct kthread_worker *kworker_fie;
 
-static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu);
 static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
 				 struct cppc_perf_fb_ctrs *fb_ctrs_t0,
 				 struct cppc_perf_fb_ctrs *fb_ctrs_t1);
@@ -834,57 +815,6 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
 	.name = "cppc_cpufreq",
 };
 
-/*
- * HISI platform does not support delivered performance counter and
- * reference performance counter. It can calculate the performance using the
- * platform specific mechanism. We reuse the desired performance register to
- * store the real performance calculated by the platform.
- */
-static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu)
-{
-	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
-	struct cppc_cpudata *cpu_data;
-	u64 desired_perf;
-	int ret;
-
-	if (!policy)
-		return -ENODEV;
-
-	cpu_data = policy->driver_data;
-
-	cpufreq_cpu_put(policy);
-
-	ret = cppc_get_desired_perf(cpu, &desired_perf);
-	if (ret < 0)
-		return -EIO;
-
-	return cppc_perf_to_khz(&cpu_data->perf_caps, desired_perf);
-}
-
-static void cppc_check_hisi_workaround(void)
-{
-	struct acpi_table_header *tbl;
-	acpi_status status = AE_OK;
-	int i;
-
-	status = acpi_get_table(ACPI_SIG_PCCT, 0, &tbl);
-	if (ACPI_FAILURE(status) || !tbl)
-		return;
-
-	for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
-		if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
-		    !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
-		    wa_info[i].oem_revision == tbl->oem_revision) {
-			/* Overwrite the get() callback */
-			cppc_cpufreq_driver.get = hisi_cppc_cpufreq_get_rate;
-			fie_disabled = FIE_DISABLED;
-			break;
-		}
-	}
-
-	acpi_put_table(tbl);
-}
-
 static int __init cppc_cpufreq_init(void)
 {
 	int ret;
@@ -892,7 +822,6 @@ static int __init cppc_cpufreq_init(void)
 	if (!acpi_cpc_valid())
 		return -ENODEV;
 
-	cppc_check_hisi_workaround();
 	cppc_freq_invariance_init();
 	populate_efficiency_class();
 
-- 
2.33.0

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

[Ionela Voinescu]

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
> 

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

[Jie Zhan]

Hi Ionela,

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

...

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

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

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

> +       if ((ret == -EFAULT) || !delivered_perf) {
> +               if (cppc_get_desired_perf(cpu, &delivered_perf))
> +                       delivered_perf = cpu_data->perf_ctrls.desired_perf;

will take this.

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

How about this? merged patch 1 & 2 as well.

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index bafa32dd375d..411303f2e8cb 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)
+               return;
+
          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;
   }
@@ -748,18 +751,32 @@ 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) {
+               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

Re: [PATCH v2 3/3] cppc_cpufreq: Remove HiSilicon CPPC workaround

[kernel test robot]

Hi Jie,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge linus/master v6.11-rc7 next-20240913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jie-Zhan/cppc_cpufreq-Return-desired-perf-in-get-if-feedback-counters-are-0/20240912-153059
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240912072231.439332-4-zhanjie9%40hisilicon.com
patch subject: [PATCH v2 3/3] cppc_cpufreq: Remove HiSilicon CPPC workaround
config: arm64-randconfig-001-20240913 (https://download.01.org/0day-ci/archive/20240914/202409141923.kDxHsCjn-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409141923.kDxHsCjn-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409141923.kDxHsCjn-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/cpufreq/cppc_cpufreq.c:45:3: warning: unused variable 'fie_disabled' [-Wunused-variable]
   } fie_disabled = FIE_UNSET;
     ^
   1 warning generated.


vim +/fie_disabled +45 drivers/cpufreq/cppc_cpufreq.c

a3f083e04a8766 Zheng Bin     2022-05-21  40  
ae2df912d1a557 Jeremy Linton 2022-09-12  41  static enum {
ae2df912d1a557 Jeremy Linton 2022-09-12  42  	FIE_UNSET = -1,
ae2df912d1a557 Jeremy Linton 2022-09-12  43  	FIE_ENABLED,
ae2df912d1a557 Jeremy Linton 2022-09-12  44  	FIE_DISABLED
ae2df912d1a557 Jeremy Linton 2022-09-12 @45  } fie_disabled = FIE_UNSET;
ae2df912d1a557 Jeremy Linton 2022-09-12  46  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Ionela Voinescu]

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

> 
> >         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.
> 
> > 
> >         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. 
> 
> > +       if ((ret == -EFAULT) || !delivered_perf) {
> > +               if (cppc_get_desired_perf(cpu, &delivered_perf))
> > +                       delivered_perf = cpu_data->perf_ctrls.desired_perf;
> 
> will take this.
> 
> > +       }
> > 
> > -       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
> >>
> > 
> 
> How about this? merged patch 1 & 2 as well.
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index bafa32dd375d..411303f2e8cb 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)
> +               return;
> +
>           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;
>    }
> @@ -748,18 +751,32 @@ 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);

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

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

[Jie Zhan]

On 17/09/2024 18:36, Ionela Voinescu wrote:

...

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

Yeah understood. I did try minimizing duplicate code, but ended up with either
duplicate 'get desired perf' stuff or duplicate cppc_perf_to_khz().

...
>>
>>           delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>>                                                  &fb_ctrs_t1);
> 
> 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.
> 

Sure, thanks for reminding this.

...

How does this look? I think this should have the least duplicate code except for
two cppc_perf_to_khz() calls, while keeping the logic easy to follow.

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index bafa32dd375d..6070444ed098 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)
+               return;
+
        cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
 
        perf <<= SCHED_CAPACITY_SHIFT;
@@ -726,11 +729,27 @@ 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;
 }
 
+static int cppc_get_perf_ctrs_sample(int cpu,
+                                    struct cppc_perf_fb_ctrs *fb_ctrs_t0,
+                                    struct cppc_perf_fb_ctrs *fb_ctrs_t1)
+{
+       int ret;
+
+       ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t0);
+       if (ret)
+               return ret;
+
+       udelay(2); /* 2usec delay between sampling */
+
+       ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t1);
+       return ret;
+}
+
 static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
 {
        struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
@@ -746,20 +765,30 @@ 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;
+       ret = cppc_get_perf_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1);
+       if (ret) {
+               if (ret == -EFAULT)
+                       goto out_invalid_counters;
+               else
+                       return 0;
+       }
 
        delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
                                               &fb_ctrs_t1);
+       if (!delivered_perf)
+               goto out_invalid_counters;
 
        return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
+
+out_invalid_counters:
+       /*
+        * Feedback counters could be unchanged or 0 when a cpu enters a
+        * low-power idle state, e.g. clock-gated or power-gated.
+        * Get the lastest or cached desired perf for reflecting frequency.
+        */
+       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);
 }
 
 static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)


Thanks!
Jie

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

[Ionela Voinescu]

Hi,

On Wednesday 18 Sep 2024 at 10:05:13 (+0800), Jie Zhan wrote:
> 
> 
> On 17/09/2024 18:36, Ionela Voinescu wrote:
> 
> ...
> 
> >>> @@ -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. 
> 
> Yeah understood. I did try minimizing duplicate code, but ended up with either
> duplicate 'get desired perf' stuff or duplicate cppc_perf_to_khz().
> 
> ...
> >>
> >>           delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
> >>                                                  &fb_ctrs_t1);
> > 
> > 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.
> > 
> 
> Sure, thanks for reminding this.
> 
> ...
> 
> How does this look? I think this should have the least duplicate code except for
> two cppc_perf_to_khz() calls, while keeping the logic easy to follow.
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index bafa32dd375d..6070444ed098 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)
> +               return;
> +
>         cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
>  
>         perf <<= SCHED_CAPACITY_SHIFT;
> @@ -726,11 +729,27 @@ 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;
>  }
>  
> +static int cppc_get_perf_ctrs_sample(int cpu,
> +                                    struct cppc_perf_fb_ctrs *fb_ctrs_t0,
> +                                    struct cppc_perf_fb_ctrs *fb_ctrs_t1)
> +{
> +       int ret;
> +
> +       ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t0);
> +       if (ret)
> +               return ret;
> +
> +       udelay(2); /* 2usec delay between sampling */
> +
> +       ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t1);

nit: white line before return.

> +       return ret;
> +}
> +
>  static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>  {
>         struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
> @@ -746,20 +765,30 @@ 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;
> +       ret = cppc_get_perf_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1);
> +       if (ret) {
> +               if (ret == -EFAULT)
> +                       goto out_invalid_counters;
> +               else
> +                       return 0;
> +       }
>  
>         delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>                                                &fb_ctrs_t1);
> +       if (!delivered_perf)
> +               goto out_invalid_counters;
>  
>         return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
> +
> +out_invalid_counters:
> +       /*
> +        * Feedback counters could be unchanged or 0 when a cpu enters a
> +        * low-power idle state, e.g. clock-gated or power-gated.
> +        * Get the lastest or cached desired perf for reflecting frequency.
> +        */
> +       if (cppc_get_desired_perf(cpu, &delivered_perf))
> +               delivered_perf = cpu_data->perf_ctrls.desired_perf;

nit: same white line before return here :).

Looks good, thanks for the changes.

Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>

Ionela.

> +       return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
>  }
>  
>  static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
> 
> 
> Thanks!
> Jie

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

[Jie Zhan]

On 18/09/2024 18:15, Ionela Voinescu wrote:
> Hi,
> 
> On Wednesday 18 Sep 2024 at 10:05:13 (+0800), Jie Zhan wrote:
>>
>>
>> On 17/09/2024 18:36, Ionela Voinescu wrote:
>>
>> ...
>>
>>>>> @@ -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. 
>>
>> Yeah understood. I did try minimizing duplicate code, but ended up with either
>> duplicate 'get desired perf' stuff or duplicate cppc_perf_to_khz().
>>
>> ...
>>>>
>>>>           delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>>>>                                                  &fb_ctrs_t1);
>>>
>>> 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.
>>>
>>
>> Sure, thanks for reminding this.
>>
>> ...
>>
>> How does this look? I think this should have the least duplicate code except for
>> two cppc_perf_to_khz() calls, while keeping the logic easy to follow.
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index bafa32dd375d..6070444ed098 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)
>> +               return;
>> +
>>         cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
>>  
>>         perf <<= SCHED_CAPACITY_SHIFT;
>> @@ -726,11 +729,27 @@ 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;
>>  }
>>  
>> +static int cppc_get_perf_ctrs_sample(int cpu,
>> +                                    struct cppc_perf_fb_ctrs *fb_ctrs_t0,
>> +                                    struct cppc_perf_fb_ctrs *fb_ctrs_t1)
>> +{
>> +       int ret;
>> +
>> +       ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t0);
>> +       if (ret)
>> +               return ret;
>> +
>> +       udelay(2); /* 2usec delay between sampling */
>> +
>> +       ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t1);
> 
> nit: white line before return.
> 
>> +       return ret;
>> +}
>> +
>>  static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>  {
>>         struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
>> @@ -746,20 +765,30 @@ 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;
>> +       ret = cppc_get_perf_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1);
>> +       if (ret) {
>> +               if (ret == -EFAULT)
>> +                       goto out_invalid_counters;
>> +               else
>> +                       return 0;
>> +       }
>>  
>>         delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>>                                                &fb_ctrs_t1);
>> +       if (!delivered_perf)
>> +               goto out_invalid_counters;
>>  
>>         return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
>> +
>> +out_invalid_counters:
>> +       /*
>> +        * Feedback counters could be unchanged or 0 when a cpu enters a
>> +        * low-power idle state, e.g. clock-gated or power-gated.
>> +        * Get the lastest or cached desired perf for reflecting frequency.
>> +        */
>> +       if (cppc_get_desired_perf(cpu, &delivered_perf))
>> +               delivered_perf = cpu_data->perf_ctrls.desired_perf;
> 
> nit: same white line before return here :).
> 
> Looks good, thanks for the changes.
> 
> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
> 
> Ionela.

Sure, thanks. I'll send a V3 based on this.

Jie

> 
>> +       return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
>>  }
>>  
>>  static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
>>
>>
>> Thanks!
>> Jie
>