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

["Gautham R. Shenoy" <gautham.shenoy@xxxxxxx>]

On Thu, Jan 16, 2025 at 09:26:37AM +0800, zhenglifeng (A) wrote:
> On 2025/1/15 22:51, Gautham R. Shenoy wrote:
> 
> > Hello Lifeng,
> > 
> > 
> > On Mon, Jan 13, 2025 at 08:21:04PM +0800, Lifeng Zheng wrote:
> >> Add sysfs interfaces for CPPC autonomous selection in the cppc_cpufreq
> >> driver.
> >>
> > 
> > [..snip..]
> > 
> >> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> >> index bd8f75accfa0..ea6c6a5bbd8c 100644
> >> --- a/drivers/cpufreq/cppc_cpufreq.c
> >> +++ b/drivers/cpufreq/cppc_cpufreq.c
> >> @@ -814,10 +814,119 @@ static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
> >>  
> >>  	return cpufreq_show_cpus(cpu_data->shared_cpu_map, buf);
> >>  }
> >> +
> >> +static ssize_t show_auto_select(struct cpufreq_policy *policy, char *buf)
> >> +{
> >> +	bool val;
> >> +	int ret;
> >> +
> >> +	ret = cppc_get_auto_sel(policy->cpu, &val);
> >> +
> >> +	/* show "<unsupported>" when this register is not supported by cpc */
> >> +	if (ret == -EOPNOTSUPP)
> >> +		return sysfs_emit(buf, "%s\n", "<unsupported>");
> >> +
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	return sysfs_emit(buf, "%d\n", val);
> >> +}
> >> +
> >> +static ssize_t store_auto_select(struct cpufreq_policy *policy,
> >> +				 const char *buf, size_t count)
> >> +{
> >> +	bool val;
> >> +	int ret;
> >> +
> >> +	ret = kstrtobool(buf, &val);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = cppc_set_auto_sel(policy->cpu, val);
> > 
> > When the auto_select register is not supported, since
> > cppc_set_reg_val() doesn't have the !CPC_SUPPORTED(reg) check, that
> > function won't return an error, and thus this store function won't
> > return an error either. Should there be a !CPC_SUPPORTED(reg) check in
> > cppc_set_reg_val() as well? Or should the store function call
> > cppc_get_auto_sel() to figure out if the register is supported or not?
> 
> In patch 2, I have this check in cppc_set_reg_val():
> 
> +	/* if a register is writeable, it must be a buffer */
> +	if ((reg->type != ACPI_TYPE_BUFFER) ||
> +	    (IS_OPTIONAL_CPC_REG(reg_idx) && IS_NULL_REG(&reg->cpc_entry.reg))) {
> +		pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
> +		return -EOPNOTSUPP;
> +	}
> 
> If a register is not a cpc supported one, it must be either an integer type
> or a null one. So it won't pass this check I think.

Ah, I see. In that case, you can remove the cppc_get_auto_sel() in
shmem_init_perf() function in amd_pstate.c (in Patch 5/6) from the
following snippet. The auto_sel value is nowhere used in the rest of
the code.

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


--
Thanks and Regards
gautham.