On 25/09/2024 17:28, lihuisong (C) wrote:
> Hi Jie,
>
> LGTM except for some trivial,
> Reviewed-by: Huisong Li <lihuisong@xxxxxxxxxx>
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@xxxxxxxxxxxxx>
>> Reviewed-by: Zeng Heng <zengheng4@xxxxxxxxxx>
>> Reviewed-by: Ionela Voinescu <ionela.voinescu@xxxxxxx>
>> ---
>> 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
