Re: [PATCH v3 1/8] cpufreq: CPPC: Add generic helpers for sysfs show/store

[Sumit Gupta <sumitg@nvidia.com>]

On 10/10/25 08:54, Jie Zhan wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Sumit,
>
> On 10/1/2025 11:00 PM, Sumit Gupta wrote:
>> Add generic show/store helper functions for u64 sysfs attributes:
>> - cppc_cpufreq_sysfs_show_u64()
>> - cppc_cpufreq_sysfs_store_u64()
>>
>> Refactor auto_act_window and energy_performance_preference_val
>> attributes to use these helpers, eliminating code duplication.
>>
>> No functional changes.
>>
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
> Nice cleanup in general.  Some minor bits inline.
>> ---
>>   drivers/cpufreq/cppc_cpufreq.c | 87 ++++++++++++++--------------------
>>   1 file changed, 35 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index 12de0ac7bbaf..732f35096991 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -781,6 +781,36 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
>>        return 0;
>>   }
>>
>> +static ssize_t cppc_cpufreq_sysfs_show_u64(unsigned int cpu, int (*get_func)(int, u64 *), char *buf)
> Wrap a bit into 80 chars?

Wrapped line length to 100 as per the max limit.

  $ grep "max_line_length =" scripts/checkpatch.pl
  my $max_line_length = 100;


>
> BTW, trivial but I would prefer a symmetric param order, like:
> show(buf, get_func, cpu)
> store(buf, count, set_func, cpu)

Sure, will change the param order.


>> +{
>> +     u64 val;
>> +     int ret = get_func(cpu, &val);
>> +
>> +     if (ret == -EOPNOTSUPP)
>> +             return sysfs_emit(buf, "<unsupported>\n");
>> +     if (ret)
>> +             return ret;
>> +
>> +     return sysfs_emit(buf, "%llu\n", val);
>> +}
>> +
>> +static ssize_t cppc_cpufreq_sysfs_store_u64(const char *buf, size_t count,
>> +                                         int (*set_func)(int, u64), unsigned int cpu)
>> +{
>> +     u64 val;
>> +     int ret;
>> +
>> +     if (!buf || !set_func)
>> +             return -EINVAL;
> No need.

Ok.


>> +
>> +     ret = kstrtou64(buf, 0, &val);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = set_func((int)cpu, val);
>> +     return ret ? ret : count;
> I suppose it's preferred to avoid using ternary operators like this.

These are commonly use. e.g. within cpufreq:

  $ grep -inr "return ret ?" drivers/cpufreq/*
  drivers/cpufreq/amd-pstate.c:1184:      return ret ? ret : count;
  drivers/cpufreq/cpufreq.c:839:  return ret ? ret : count;
  drivers/cpufreq/intel_pstate.c:897:     return ret ?: count;

>> +}
>> +
> Would be nicer to move cppc_cpufreq_sysfs_show/store_u64() to just above
> where they are used, i.e. just before show_auto_act_window().

Sure.

Thank you,
Sumit

>>   static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
>>   {
>>        struct cppc_cpudata *cpu_data = policy->driver_data;
>> @@ -824,70 +854,23 @@ static ssize_t store_auto_select(struct cpufreq_policy *policy,
>>
>>   static ssize_t show_auto_act_window(struct cpufreq_policy *policy, char *buf)
>>   {
>> -     u64 val;
>> -     int ret;
>> -
>> -     ret = cppc_get_auto_act_window(policy->cpu, &val);
>> -
>> -     /* show "<unsupported>" when this register is not supported by cpc */
>> -     if (ret == -EOPNOTSUPP)
>> -             return sysfs_emit(buf, "<unsupported>\n");
>> -
>> -     if (ret)
>> -             return ret;
>> -
>> -     return sysfs_emit(buf, "%llu\n", val);
>> +     return cppc_cpufreq_sysfs_show_u64(policy->cpu, cppc_get_auto_act_window, buf);
>>   }
>>
>> -static ssize_t store_auto_act_window(struct cpufreq_policy *policy,
>> -                                  const char *buf, size_t count)
>> +static ssize_t store_auto_act_window(struct cpufreq_policy *policy, const char *buf, size_t count)
>>   {
>> -     u64 usec;
>> -     int ret;
>> -
>> -     ret = kstrtou64(buf, 0, &usec);
>> -     if (ret)
>> -             return ret;
>> -
>> -     ret = cppc_set_auto_act_window(policy->cpu, usec);
>> -     if (ret)
>> -             return ret;
>> -
>> -     return count;
>> +     return cppc_cpufreq_sysfs_store_u64(buf, count, cppc_set_auto_act_window, policy->cpu);
>>   }
>>
>>   static ssize_t show_energy_performance_preference_val(struct cpufreq_policy *policy, char *buf)
>>   {
>> -     u64 val;
>> -     int ret;
>> -
>> -     ret = cppc_get_epp_perf(policy->cpu, &val);
>> -
>> -     /* show "<unsupported>" when this register is not supported by cpc */
>> -     if (ret == -EOPNOTSUPP)
>> -             return sysfs_emit(buf, "<unsupported>\n");
>> -
>> -     if (ret)
>> -             return ret;
>> -
>> -     return sysfs_emit(buf, "%llu\n", val);
>> +     return cppc_cpufreq_sysfs_show_u64(policy->cpu, cppc_get_epp_perf, buf);
>>   }
>>
>>   static ssize_t store_energy_performance_preference_val(struct cpufreq_policy *policy,
>>                                                       const char *buf, size_t count)
>>   {
>> -     u64 val;
>> -     int ret;
>> -
>> -     ret = kstrtou64(buf, 0, &val);
>> -     if (ret)
>> -             return ret;
>> -
>> -     ret = cppc_set_epp(policy->cpu, val);
>> -     if (ret)
>> -             return ret;
>> -
>> -     return count;
>> +     return cppc_cpufreq_sysfs_store_u64(buf, count, cppc_set_epp, policy->cpu);
>>   }
>>
>>   cpufreq_freq_attr_ro(freqdomain_cpus);