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

["zhenglifeng (A)" <zhenglifeng1@huawei.com>]

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(&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.