diff mbox series

[v3,1/2] cppc_cpufreq: Use desired perf if feedback ctrs are 0 or unchanged

Message ID 20240919084552.3591400-2-zhanjie9@hisilicon.com
State New
Headers show
Series cppc_cpufreq: Rework ->get() error handling when cores are idle | expand

Commit Message

Jie Zhan Sept. 19, 2024, 8:45 a.m. UTC
The CPPC performance feedback counters could be 0 or unchanged when the
target cpu is in a low-power idle state, e.g. power-gated or clock-gated.

When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes
cpufreq_online() get a false error and fail to generate a cpufreq policy.

When the counters are unchanged, the existing cppc_perf_from_fbctrs()
returns a cached desired perf, but some platforms may update the real
frequency back to the desired perf reg.

For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf
to reflect the frequency; if failed, return the cached desired perf.

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>
Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
---
 drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 10 deletions(-)

Comments

lihuisong (C) Sept. 25, 2024, 9:28 a.m. UTC | #1
Hi Jie,

LGTM except for some trivial,
Reviewed-by: Huisong Li <lihuisong@huawei.com>


在 2024/9/19 16:45, Jie Zhan 写道:
> The CPPC performance feedback counters could be 0 or unchanged when the
> target cpu is in a low-power idle state, e.g. power-gated or clock-gated.
>
> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes
> cpufreq_online() get a false error and fail to generate a cpufreq policy.
>
> When the counters are unchanged, the existing cppc_perf_from_fbctrs()
> returns a cached desired perf, but some platforms may update the real
> frequency back to the desired perf reg.
>
> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf
> to reflect the frequency; if failed, return the cached desired perf.
>
> 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>
> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
> ---
>   drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++-------
>   1 file changed, 39 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index bafa32dd375d..e55192303a9f 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,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>   
>   	/* Check to avoid divide-by zero and invalid delivered_perf */
Now this comment can be removed, right?
>   	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 */
> +
> +	return cppc_get_perf_ctrs(cpu, fb_ctrs_t1);
> +}
> +
>   static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>   {
>   	struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
> @@ -746,18 +764,29 @@ 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;
suggest that add some comments for ret == -EFAULT case.
Because this error code depands on the implementation of cppc_get_perf_ctrs.
If add a new exception case which also return -EFAULT, then this switch 
is unreasonable.
> +		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);
>   }
Jie Zhan Sept. 26, 2024, 2:57 a.m. UTC | #2
On 25/09/2024 17:28, lihuisong (C) wrote:
> Hi Jie,
> 
> LGTM except for some trivial,
> Reviewed-by: Huisong Li <lihuisong@huawei.com>

Thanks.

> 
> 
> 在 2024/9/19 16:45, Jie Zhan 写道:
>> The CPPC performance feedback counters could be 0 or unchanged when the
>> target cpu is in a low-power idle state, e.g. power-gated or clock-gated.
>>
>> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes
>> cpufreq_online() get a false error and fail to generate a cpufreq policy.
>>
>> When the counters are unchanged, the existing cppc_perf_from_fbctrs()
>> returns a cached desired perf, but some platforms may update the real
>> frequency back to the desired perf reg.
>>
>> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf
>> to reflect the frequency; if failed, return the cached desired perf.
>>
>> 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>
>> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
>> ---
>>   drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++-------
>>   1 file changed, 39 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index bafa32dd375d..e55192303a9f 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,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>         /* Check to avoid divide-by zero and invalid delivered_perf */
> Now this comment can be removed, right?

Didn't notice this comment, but, having a check, I think it still fits.
'!delta_reference' avoids divide-by zero, and '!delta_delivered' checks
invalid delivered_perf.

So I think we just leave it unchanged.

>>       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 */
>> +
>> +    return cppc_get_perf_ctrs(cpu, fb_ctrs_t1);
>> +}
>> +
>>   static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>   {
>>       struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
>> @@ -746,18 +764,29 @@ 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;
> suggest that add some comments for ret == -EFAULT case.
> Because this error code depands on the implementation of cppc_get_perf_ctrs.
> If add a new exception case which also return -EFAULT, then this switch is unreasonable.

Sure. What about adding the following comment:

/* -EFAULT indicates that any of the associated CPPC regs is 0. */

Thanks,
Jie
lihuisong (C) Sept. 26, 2024, 6:07 a.m. UTC | #3
在 2024/9/26 10:57, Jie Zhan 写道:
>
> On 25/09/2024 17:28, lihuisong (C) wrote:
>> Hi Jie,
>>
>> LGTM except for some trivial,
>> Reviewed-by: Huisong Li <lihuisong@huawei.com>
> Thanks.
>
>>
>> 在 2024/9/19 16:45, Jie Zhan 写道:
>>> The CPPC performance feedback counters could be 0 or unchanged when the
>>> target cpu is in a low-power idle state, e.g. power-gated or clock-gated.
>>>
>>> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes
>>> cpufreq_online() get a false error and fail to generate a cpufreq policy.
>>>
>>> When the counters are unchanged, the existing cppc_perf_from_fbctrs()
>>> returns a cached desired perf, but some platforms may update the real
>>> frequency back to the desired perf reg.
>>>
>>> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf
>>> to reflect the frequency; if failed, return the cached desired perf.
>>>
>>> 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>
>>> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
>>> ---
>>>    drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++-------
>>>    1 file changed, 39 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>> index bafa32dd375d..e55192303a9f 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,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>>          /* Check to avoid divide-by zero and invalid delivered_perf */
>> Now this comment can be removed, right?
> Didn't notice this comment, but, having a check, I think it still fits.
> '!delta_reference' avoids divide-by zero, and '!delta_delivered' checks
> invalid delivered_perf.
The comment  "avoid divide-by zero" is just for the below code: 
"(reference_perf * delta_delivered) / delta_reference".
So It is also useful, but I think It's obvious and it doesn't make much 
sense.

The comment "avoid invalid delivered_perf" is for the return value.
Now this func return zero which can't count as a valid delivered_perf, 
right?
>
> So I think we just leave it unchanged.
>
>>>        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 */
>>> +
>>> +    return cppc_get_perf_ctrs(cpu, fb_ctrs_t1);
>>> +}
>>> +
>>>    static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>>    {
>>>        struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
>>> @@ -746,18 +764,29 @@ 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;
>> suggest that add some comments for ret == -EFAULT case.
>> Because this error code depands on the implementation of cppc_get_perf_ctrs.
>> If add a new exception case which also return -EFAULT, then this switch is unreasonable.
> Sure. What about adding the following comment:
>
> /* -EFAULT indicates that any of the associated CPPC regs is 0. */
Ack
> .
Jie Zhan Sept. 26, 2024, 8:44 a.m. UTC | #4
On 26/09/2024 14:07, lihuisong (C) wrote:
> 
> 在 2024/9/26 10:57, Jie Zhan 写道:
>>
>> On 25/09/2024 17:28, lihuisong (C) wrote:
>>> Hi Jie,
>>>
>>> LGTM except for some trivial,
>>> Reviewed-by: Huisong Li <lihuisong@huawei.com>
>> Thanks.
>>
>>>
>>> 在 2024/9/19 16:45, Jie Zhan 写道:
>>>> The CPPC performance feedback counters could be 0 or unchanged when the
>>>> target cpu is in a low-power idle state, e.g. power-gated or clock-gated.
>>>>
>>>> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes
>>>> cpufreq_online() get a false error and fail to generate a cpufreq policy.
>>>>
>>>> When the counters are unchanged, the existing cppc_perf_from_fbctrs()
>>>> returns a cached desired perf, but some platforms may update the real
>>>> frequency back to the desired perf reg.
>>>>
>>>> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf
>>>> to reflect the frequency; if failed, return the cached desired perf.
>>>>
>>>> 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>
>>>> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
>>>> ---
>>>>    drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++-------
>>>>    1 file changed, 39 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>>> index bafa32dd375d..e55192303a9f 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,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>>>          /* Check to avoid divide-by zero and invalid delivered_perf */
>>> Now this comment can be removed, right?
>> Didn't notice this comment, but, having a check, I think it still fits.
>> '!delta_reference' avoids divide-by zero, and '!delta_delivered' checks
>> invalid delivered_perf.
> The comment  "avoid divide-by zero" is just for the below code: "(reference_perf * delta_delivered) / delta_reference".
> So It is also useful, but I think It's obvious and it doesn't make much sense.
> 
> The comment "avoid invalid delivered_perf" is for the return value.
> Now this func return zero which can't count as a valid delivered_perf, right?

so, what about this?

/*
 * Avoid divide-by zero and unchanged feedback counters.
 * Leave it for callers to handle.
 */

>>
>> So I think we just leave it unchanged.
>>

...
lihuisong (C) Sept. 26, 2024, 10:08 a.m. UTC | #5
在 2024/9/26 16:44, Jie Zhan 写道:
>
> On 26/09/2024 14:07, lihuisong (C) wrote:
>> 在 2024/9/26 10:57, Jie Zhan 写道:
>>> On 25/09/2024 17:28, lihuisong (C) wrote:
>>>> Hi Jie,
>>>>
>>>> LGTM except for some trivial,
>>>> Reviewed-by: Huisong Li <lihuisong@huawei.com>
>>> Thanks.
>>>
>>>> 在 2024/9/19 16:45, Jie Zhan 写道:
>>>>> The CPPC performance feedback counters could be 0 or unchanged when the
>>>>> target cpu is in a low-power idle state, e.g. power-gated or clock-gated.
>>>>>
>>>>> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes
>>>>> cpufreq_online() get a false error and fail to generate a cpufreq policy.
>>>>>
>>>>> When the counters are unchanged, the existing cppc_perf_from_fbctrs()
>>>>> returns a cached desired perf, but some platforms may update the real
>>>>> frequency back to the desired perf reg.
>>>>>
>>>>> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf
>>>>> to reflect the frequency; if failed, return the cached desired perf.
>>>>>
>>>>> 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>
>>>>> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
>>>>> ---
>>>>>     drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++-------
>>>>>     1 file changed, 39 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>>>> index bafa32dd375d..e55192303a9f 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,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>>>>           /* Check to avoid divide-by zero and invalid delivered_perf */
>>>> Now this comment can be removed, right?
>>> Didn't notice this comment, but, having a check, I think it still fits.
>>> '!delta_reference' avoids divide-by zero, and '!delta_delivered' checks
>>> invalid delivered_perf.
>> The comment  "avoid divide-by zero" is just for the below code: "(reference_perf * delta_delivered) / delta_reference".
>> So It is also useful, but I think It's obvious and it doesn't make much sense.
>>
>> The comment "avoid invalid delivered_perf" is for the return value.
>> Now this func return zero which can't count as a valid delivered_perf, right?
> so, what about this?
>
> /*
>   * Avoid divide-by zero and unchanged feedback counters.
>   * Leave it for callers to handle.
>   */
good.
>>> So I think we just leave it unchanged.
>>>
> ...
> .
diff mbox series

Patch

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index bafa32dd375d..e55192303a9f 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,26 @@  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 */
+
+	return cppc_get_perf_ctrs(cpu, fb_ctrs_t1);
+}
+
 static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
 {
 	struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
@@ -746,18 +764,29 @@  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);
 }