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

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

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