On 2025/1/16 14:13, Gautham R. Shenoy wrote:
> 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(®->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.
>
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?
>
> --
> Thanks and Regards
> gautham.
