Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[Gautham R. Shenoy]

On Thu, Jan 16, 2025 at 04:01:08PM +0800, zhenglifeng (A) wrote:

> > @@ -399,6 +399,7 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
> >  {
> >  	struct cppc_perf_caps cppc_perf;
> >  	u64 numerator;
> > +	bool auto_sel; <--- Not needed.
> >  
> >  	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> >  	if (ret)
> > @@ -420,7 +421,7 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
> >  	if (cppc_state == AMD_PSTATE_ACTIVE)
> >  		return 0;
> >  
> > -	ret = cppc_get_auto_sel_caps(cpudata->cpu, &cppc_perf);   <--- Not needed.
> > +	ret = cppc_get_auto_sel(cpudata->cpu, &auto_sel);         <--- Not needed.
> >  	if (ret) {                                                <--- Not needed.
> >  		pr_warn("failed to get auto_sel, ret: %d\n", ret); <--- Not needed.
> > 
> 
> If auto_sel is not supported, this function will return 0 after getting
> fail. But after removing cppc_get_auto_sel(), this function will return
> -EOPNOTSUPP by setting. Is this alright?

This is not ok. The shmem_init_perf() function shouldn't error out if
the auto-selection register is not supported.

Also, looking at the function, there may be a few additional cleanups
required in that one. For now, let us just retain the
cppc_get_auto_sel() and cppc_set_auto_sel() functions as you have done
in this patchset.

--
Thanks and Regards
gautham.