Re: [PATCH v3] cpufreq/cppc: Remove the desired_perf compare when set target

[Ionela Voinescu]

Hey,

On Thursday 06 Jun 2024 at 14:37:37 (+0530), Viresh Kumar wrote:
> Ionela, Beata,
> 
> On 30-05-24, 19:08, Riwen Lu wrote:
> > From: Riwen Lu <luriwen@xxxxxxxxxx>
> > 
> > There is a case that desired_perf is exactly the same with the old perf,
> > but the actual current freq is not.
> > 
> > This happened in S3 while the cpufreq governor is set to powersave.
> > During cpufreq resume process, the booting CPU's new_freq obtained via
> > .get() is the highest frequency, while the policy->cur and
> > cpu->perf_ctrls.desired_perf are in the lowest level(powersave
> > governor). Causing the warning: "CPU frequency out of sync:", and set
> > policy->cur to new_freq. Then the governor->limits() calls
> > cppc_cpufreq_set_target() to configures the CPU frequency and returns
> > directly because the desired_perf converted from target_freq is the
> > same with cpu->perf_ctrls.desired_perf and both are the lowest_perf.
> > Since target_freq and policy->cur have been compared in
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	  [note] below

> > __cpufreq_driver_target(), there's no need to compare desired_perf
> > and cpu->perf_ctrls.desired_perf again in cppc_cpufreq_set_target()
> > to ensure that the CPU frequency is properly configured.
> > 
> > Signed-off-by: Riwen Lu <luriwen@xxxxxxxxxx>
> > 
> > ---
> > v1 -> v2:
> >  - Update commit message and email.
> > v2 -> v3:
> >  - Update patch subject and commit message.
> >  - Remove the desired_perf compare logic.
> > ---
> >  drivers/cpufreq/cppc_cpufreq.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> > index 15f1d41920a3..337cece61ab5 100644
> > --- a/drivers/cpufreq/cppc_cpufreq.c
> > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > @@ -295,9 +295,6 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
> >  	int ret = 0;
> >  
> >  	desired_perf = cppc_khz_to_perf(&cpu_data->perf_caps, target_freq);
> > -	/* Return if it is exactly the same perf */
> > -	if (desired_perf == cpu_data->perf_ctrls.desired_perf)
> > -		return ret;
> >  
> >  	cpu_data->perf_ctrls.desired_perf = desired_perf;
> >  	freqs.old = policy->cur;
> 
> Any objections to this change ?

It's alright with me.

Some "nits":
 - the "desired_perf" local variable could be removed in this case.

 - [note] while this change helps, we'd still need policy->cur to always
   have the latest request value (see details at [1]) for this check to
   be made obsolete by the comparison between target_freq and policy->cur,
   as mentioned in the commit message. But this is/can be a separate
   matter.

   [1] https://lore.kernel.org/lkml/ZmB1qKucR5fXk100@xxxxxxx/

Thanks,
Ionela.

> 
> -- 
> viresh