On 18/09/2024 18:15, Ionela Voinescu wrote: > Hi, > > On Wednesday 18 Sep 2024 at 10:05:13 (+0800), Jie Zhan wrote: >> >> >> On 17/09/2024 18:36, Ionela Voinescu wrote: >> >> ... >> >>>>> @@ -747,19 +750,22 @@ 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; >>>>> + if (!ret) { >>>>> + udelay(2); /* 2usec delay between sampling */ >>>>> + ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1); >>>>> + } >>>>> + if (!ret) >>>>> + delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0, >>>>> + &fb_ctrs_t1); >>>> >>>> TBH, 'if (!ret)' style looks very strange to me. >>>> We haven't done so anywhere in cppc_cpufreq, so let's keep consistency and make >>>> it easier for people to read and maintain? >>> >>> I agree it's a bit of a difficult read, that's why I only sent my code >>> as a suggestion. I did like the benefit of not having to have two >>> different calls to cppc_perf_to_khz() and making the code below common >>> for the error and non-error paths. But it's up to you. >> >> Yeah understood. I did try minimizing duplicate code, but ended up with either >> duplicate 'get desired perf' stuff or duplicate cppc_perf_to_khz(). >> >> ... >>>> >>>> delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0, >>>> &fb_ctrs_t1); >>> >>> You need a check here for !delivered_perf (when at least one of the >>> deltas is 0) in which case it would be good to take the same error path >>> below. Something like: >>> >>> if(delivered_perf) >>> return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); >>> else >>> ret = -EFAULT; >>> >>> That's why I did the tricky if/else dance above as we need to take the >>> error path below for multiple cases. >>> >>> Thanks, >>> Ionela. >>> >> >> Sure, thanks for reminding this. >> >> ... >> >> How does this look? I think this should have the least duplicate code except for >> two cppc_perf_to_khz() calls, while keeping the logic easy to follow. >> >> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c >> index bafa32dd375d..6070444ed098 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,27 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data, >> >> /* Check to avoid divide-by zero and invalid delivered_perf */ >> 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 */ >> + >> + ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t1); > > nit: white line before return. > >> + return ret; >> +} >> + >> static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) >> { >> struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0}; >> @@ -746,20 +765,30 @@ 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; >> + 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; > > nit: same white line before return here :). > > Looks good, thanks for the changes. > > Reviewed-by: Ionela Voinescu <ionela.voinescu@xxxxxxx> > > Ionela. Sure, thanks. I'll send a V3 based on this. Jie > >> + return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); >> } >> >> static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state) >> >> >> Thanks! >> Jie >