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.
>
> --
> Thanks and Regards
> gautham.
