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

["lihuisong (C)" <lihuisong@huawei.com>]

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