| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5c259029-23cc-4bdd-9012-a6ff8c3583c4@nvidia.com>
Date: Mon, 13 Oct 2025 18:21:09 +0530
From: Sumit Gupta <sumitg@...dia.com>
To: Jie Zhan <zhanjie9@...ilicon.com>, rafael@...nel.org,
viresh.kumar@...aro.org, lenb@...nel.org, robert.moore@...el.com,
corbet@....net, pierre.gondois@....com, zhenglifeng1@...wei.com,
rdunlap@...radead.org, ray.huang@....com, gautham.shenoy@....com,
mario.limonciello@....com, perry.yuan@....com, linux-pm@...r.kernel.org,
linux-acpi@...r.kernel.org, linux-doc@...r.kernel.org,
acpica-devel@...ts.linux.dev, linux-kernel@...r.kernel.org
Cc: linux-tegra@...r.kernel.org, treding@...dia.com, jonathanh@...dia.com,
vsethi@...dia.com, ksitaraman@...dia.com, sanjayc@...dia.com,
bbasu@...dia.com, sumitg@...dia.com
Subject: Re: [PATCH v3 1/8] cpufreq: CPPC: Add generic helpers for sysfs
show/store
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@...dia.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);
Powered by blists - more mailing lists