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

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

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.

> 
> --
> Thanks and Regards
> gautham.