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