[PATCH] cpufreq: CPPC: Return desired perf in ->get() if feedback counters are 0

[PATCH] cpufreq: CPPC: 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.  cppc_cpufreq_get_rate() returns 0 in this case, triggering two
problems:

1. 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.
2. 'cpuinfo_cur_freq' shows '<unknown>'

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>
---
 drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index bafa32dd375d..1c5eb12c1a5a 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -748,18 +748,25 @@ 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:
+	if (ret == -EFAULT)
+		return cppc_perf_to_khz(&cpu_data->perf_caps,
+					cpu_data->perf_ctrls.desired_perf);
+
+	return 0;
 }
 
 static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
-- 
2.33.0

Re: [PATCH] cpufreq: CPPC: Return desired perf in ->get() if feedback counters are 0

[Jie Zhan]

cc linux-arm-kernel

On 19/08/2024 11:51, 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.  cppc_cpufreq_get_rate() returns 0 in this case, triggering two
> problems:
>
> 1. 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.
> 2. 'cpuinfo_cur_freq' shows '<unknown>'
>
> 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>
> ---
>   drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index bafa32dd375d..1c5eb12c1a5a 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -748,18 +748,25 @@ 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:
> +	if (ret == -EFAULT)
> +		return cppc_perf_to_khz(&cpu_data->perf_caps,
> +					cpu_data->perf_ctrls.desired_perf);
> +
> +	return 0;
>   }
>   
>   static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)

Re: [PATCH] cpufreq: CPPC: Return desired perf in ->get() if feedback counters are 0

[Jie Zhan]

Can anyone take a look at this additional error handling of cppc_cpufreq?
Thanks!

Jie

On 21/08/2024 16:27, Jie Zhan wrote:
> cc linux-arm-kernel
>
> On 19/08/2024 11:51, 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.  cppc_cpufreq_get_rate() returns 0 in this case, triggering two
>> problems:
>>
>> 1. 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.
>> 2. 'cpuinfo_cur_freq' shows '<unknown>'
>>
>> 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>
>> ---
>>   drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c 
>> b/drivers/cpufreq/cppc_cpufreq.c
>> index bafa32dd375d..1c5eb12c1a5a 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -748,18 +748,25 @@ 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:
>> +    if (ret == -EFAULT)
>> +        return cppc_perf_to_khz(&cpu_data->perf_caps,
>> +                    cpu_data->perf_ctrls.desired_perf);
>> +
>> +    return 0;
>>   }
>>     static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, 
>> int state)
>

Re: [PATCH] cpufreq: CPPC: Return desired perf in ->get() if feedback counters are 0

[Viresh Kumar]

Cc'd few developers.

On 19-08-24, 11:51, 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.  cppc_cpufreq_get_rate() returns 0 in this case, triggering two
> problems:
> 
> 1. 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.
> 2. 'cpuinfo_cur_freq' shows '<unknown>'
> 
> 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>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index bafa32dd375d..1c5eb12c1a5a 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -748,18 +748,25 @@ 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:
> +	if (ret == -EFAULT)
> +		return cppc_perf_to_khz(&cpu_data->perf_caps,
> +					cpu_data->perf_ctrls.desired_perf);
> +
> +	return 0;
>  }
>  
>  static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
> -- 
> 2.33.0
> 

-- 
viresh

Re: [PATCH] cpufreq: CPPC: Return desired perf in ->get() if feedback counters are 0

[Ionela Voinescu]

Hi,

On Wednesday 28 Aug 2024 at 12:20:41 (+0530), Viresh Kumar wrote:
> Cc'd few developers.
> 
> On 19-08-24, 11:51, 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.  cppc_cpufreq_get_rate() returns 0 in this case, triggering two
> > problems:
> > 
> > 1. 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.
> > 2. 'cpuinfo_cur_freq' shows '<unknown>'

I suppose 2. is not necessarily a problem as the current (hardware)
frequency is indeed unknown.

But there's not clean way to fix 1. while keeping 2. as is, or at least
not one I could identify.

> > 
> > 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>
> > ---
> >  drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> > index bafa32dd375d..1c5eb12c1a5a 100644
> > --- a/drivers/cpufreq/cppc_cpufreq.c
> > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > @@ -748,18 +748,25 @@ 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:
> > +	if (ret == -EFAULT)
> > +		return cppc_perf_to_khz(&cpu_data->perf_caps,
> > +					cpu_data->perf_ctrls.desired_perf);
> > +

A better way might be to cppc_get_desired_perf(cpu, &desired_perf) first
and return the khz equivalent of the result, as currently done in
hisi_cppc_cpufreq_get_rate(). Even a merge of the two functions might be
suitable, but I'm not familiar with the specifics of the hisilicon platforms
involved. This might be better as some platforms can provide performance
feedback through the desired performance register so a read of it would
be better than using the cached desired_perf value.

Hope it helps,
Ionela.

> > +	return 0;
> >  }
> >  
> >  static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
> > -- 
> > 2.33.0
> > 
> 
> -- 
> viresh

Re: [PATCH] cpufreq: CPPC: Return desired perf in ->get() if feedback counters are 0

[Jie Zhan]

On 28/08/2024 16:17, Ionela Voinescu wrote:
> Hi,
>
> On Wednesday 28 Aug 2024 at 12:20:41 (+0530), Viresh Kumar wrote:
>> Cc'd few developers.
>>
>> On 19-08-24, 11:51, 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.  cppc_cpufreq_get_rate() returns 0 in this case, triggering two
>>> problems:
>>>
>>> 1. 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.
>>> 2. 'cpuinfo_cur_freq' shows '<unknown>'
Hi Ionela,
> I suppose 2. is not necessarily a problem as the current (hardware)
> frequency is indeed unknown.
>
> But there's not clean way to fix 1. while keeping 2. as is, or at least
> not one I could identify.
Yeah. 1 is the main thing to deal with.
>>> 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>
>>> ---
>>>   drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++--
>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>> index bafa32dd375d..1c5eb12c1a5a 100644
>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>> @@ -748,18 +748,25 @@ 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:
>>> +	if (ret == -EFAULT)
>>> +		return cppc_perf_to_khz(&cpu_data->perf_caps,
>>> +					cpu_data->perf_ctrls.desired_perf);
>>> +
> A better way might be to cppc_get_desired_perf(cpu, &desired_perf) first
> and return the khz equivalent of the result, as currently done in
> hisi_cppc_cpufreq_get_rate(). Even a merge of the two functions might be
> suitable, but I'm not familiar with the specifics of the hisilicon platforms
> involved. This might be better as some platforms can provide performance
> feedback through the desired performance register so a read of it would
> be better than using the cached desired_perf value.
>
> Hope it helps,
> Ionela.
Sure, understood.
Getting the latest desired perf would be more compatible across platforms.

Merging hisi_cppc_cpufreq_get_rate() can be risky but worth a try. The
workaround also disables the FIE. I'll figure out whether it's feasible 
to do.

I'll send a V2 if no objection to the error handling.

Thanks,
Jie
>
>>> +	return 0;
>>>   }
>>>   
>>>   static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
>>> -- 
>>> 2.33.0
>>>
>> -- 
>> viresh

Re: [PATCH] cpufreq: CPPC: Return desired perf in ->get() if feedback counters are 0

[Ionela Voinescu]

On Wednesday 28 Aug 2024 at 17:45:09 (+0800), Jie Zhan wrote:
> 
> 
> On 28/08/2024 16:17, Ionela Voinescu wrote:
> > Hi,
> > 
> > On Wednesday 28 Aug 2024 at 12:20:41 (+0530), Viresh Kumar wrote:
> > > Cc'd few developers.
> > > 
> > > On 19-08-24, 11:51, 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.  cppc_cpufreq_get_rate() returns 0 in this case, triggering two
> > > > problems:
> > > > 
> > > > 1. 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.
> > > > 2. 'cpuinfo_cur_freq' shows '<unknown>'
> Hi Ionela,
> > I suppose 2. is not necessarily a problem as the current (hardware)
> > frequency is indeed unknown.
> > 
> > But there's not clean way to fix 1. while keeping 2. as is, or at least
> > not one I could identify.
> Yeah. 1 is the main thing to deal with.
> > > > 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>
> > > > ---
> > > >   drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++--
> > > >   1 file changed, 9 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> > > > index bafa32dd375d..1c5eb12c1a5a 100644
> > > > --- a/drivers/cpufreq/cppc_cpufreq.c
> > > > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > > > @@ -748,18 +748,25 @@ 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:
> > > > +	if (ret == -EFAULT)
> > > > +		return cppc_perf_to_khz(&cpu_data->perf_caps,
> > > > +					cpu_data->perf_ctrls.desired_perf);
> > > > +
> > A better way might be to cppc_get_desired_perf(cpu, &desired_perf) first
> > and return the khz equivalent of the result, as currently done in
> > hisi_cppc_cpufreq_get_rate(). Even a merge of the two functions might be
> > suitable, but I'm not familiar with the specifics of the hisilicon platforms
> > involved. This might be better as some platforms can provide performance
> > feedback through the desired performance register so a read of it would
> > be better than using the cached desired_perf value.
> > 
> > Hope it helps,
> > Ionela.
> Sure, understood.
> Getting the latest desired perf would be more compatible across platforms.
> 
> Merging hisi_cppc_cpufreq_get_rate() can be risky but worth a try. The
> workaround also disables the FIE. I'll figure out whether it's feasible to
> do.

Thanks! What I was thinking was that possibly after your changes the
current cppc_cpufreq_get_rate() would be suitable for what is now the
hisilicon workaround, so there wouldn't be a need to overwrite the .get
callback with a custom one. In depends on whether on that particular
platform the unsupported counter registers read as 0 and result in the
same -EFAUT error.

As for disabling FIE, the current cppc_check_hisi_workaround() can be
called from cppc_freq_invariance_init() as an added check to the existing
ones that result in disabling FIE.

Thanks,
Ionela.

> 
> I'll send a V2 if no objection to the error handling.
> 
> Thanks,
> Jie
> > 
> > > > +	return 0;
> > > >   }
> > > >   static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
> > > > -- 
> > > > 2.33.0
> > > > 
> > > -- 
> > > viresh
>