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

Gautham R. Shenoy
2025-01-16T06:13:53Z

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.