| 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: <eaa37577-006f-42d0-b59c-01a77f30587c@nvidia.com>
Date: Tue, 18 Nov 2025 14:47:24 +0530
From: Sumit Gupta <sumitg@...dia.com>
To: Ionela Voinescu <ionela.voinescu@....com>
Cc: 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,
zhanjie9@...ilicon.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,
linux-tegra@...r.kernel.org, treding@...dia.com, jonathanh@...dia.com,
vsethi@...dia.com, ksitaraman@...dia.com, sanjayc@...dia.com,
nhartman@...dia.com, bbasu@...dia.com, sumitg@...dia.com
Subject: Re: [PATCH v4 3/8] ACPI: CPPC: extend APIs to support auto_sel and
epp
Hi Ionela,
On 12/11/25 20:32, Ionela Voinescu wrote:
> External email: Use caution opening links or attachments
>
>
> Hi,
>
> A small nit that applies to multiple places: let's keep the line length
> under 80 characters - the lines seem easy to split.
Wrapped line length to 100 as per the max limit.
Will change to 80 chars if that's preferred.
$ grep "max_line_length =" scripts/checkpatch.pl
my $max_line_length = 100;
>
> On Wednesday 05 Nov 2025 at 17:08:39 (+0530), Sumit Gupta wrote:
>> - Add auto_sel read support in cppc_get_perf_caps().
>> - Add write of both auto_sel and energy_perf in cppc_set_epp_perf().
>> - Remove redundant energy_perf field from 'struct cppc_perf_caps' as
>> the same is available in 'struct cppc_perf_ctrls' which is used.
>>
>> Signed-off-by: Sumit Gupta <sumitg@...dia.com>
>> ---
>> drivers/acpi/cppc_acpi.c | 42 ++++++++++++++++++++++++++++++++--------
>> include/acpi/cppc_acpi.h | 1 -
>> 2 files changed, 34 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 05672c30187c..757e8ce87e9b 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1344,8 +1344,8 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
>> struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>> struct cpc_register_resource *highest_reg, *lowest_reg,
>> *lowest_non_linear_reg, *nominal_reg, *guaranteed_reg,
>> - *low_freq_reg = NULL, *nom_freq_reg = NULL;
>> - u64 high, low, guaranteed, nom, min_nonlinear, low_f = 0, nom_f = 0;
>> + *low_freq_reg = NULL, *nom_freq_reg = NULL, *auto_sel_reg = NULL;
>> + u64 high, low, guaranteed, nom, min_nonlinear, low_f = 0, nom_f = 0, auto_sel = 0;
>> int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
>> struct cppc_pcc_data *pcc_ss_data = NULL;
>> int ret = 0, regs_in_pcc = 0;
>> @@ -1362,11 +1362,12 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
>> low_freq_reg = &cpc_desc->cpc_regs[LOWEST_FREQ];
>> nom_freq_reg = &cpc_desc->cpc_regs[NOMINAL_FREQ];
>> guaranteed_reg = &cpc_desc->cpc_regs[GUARANTEED_PERF];
>> + auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
>>
>> /* Are any of the regs PCC ?*/
>> if (CPC_IN_PCC(highest_reg) || CPC_IN_PCC(lowest_reg) ||
>> CPC_IN_PCC(lowest_non_linear_reg) || CPC_IN_PCC(nominal_reg) ||
>> - CPC_IN_PCC(low_freq_reg) || CPC_IN_PCC(nom_freq_reg)) {
>> + CPC_IN_PCC(low_freq_reg) || CPC_IN_PCC(nom_freq_reg) || CPC_IN_PCC(auto_sel_reg)) {
>> if (pcc_ss_id < 0) {
>> pr_debug("Invalid pcc_ss_id\n");
>> return -ENODEV;
>> @@ -1414,6 +1415,9 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
>> perf_caps->lowest_freq = low_f;
>> perf_caps->nominal_freq = nom_f;
>>
>> + if (CPC_SUPPORTED(auto_sel_reg))
>> + cpc_read(cpunum, auto_sel_reg, &auto_sel);
>> + perf_caps->auto_sel = (bool)auto_sel;
>>
>> out_err:
>> if (regs_in_pcc)
>> @@ -1555,6 +1559,8 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
>> struct cpc_register_resource *auto_sel_reg;
>> struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> struct cppc_pcc_data *pcc_ss_data = NULL;
>> + bool autosel_support_in_ffh_or_sysmem;
>> + bool epp_support_in_ffh_or_sysmem;
>> int ret;
>>
>> if (!cpc_desc) {
>> @@ -1565,6 +1571,11 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
>> auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
>> epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
>>
>> + epp_support_in_ffh_or_sysmem = CPC_SUPPORTED(epp_set_reg) &&
>> + (CPC_IN_FFH(epp_set_reg) || CPC_IN_SYSTEM_MEMORY(epp_set_reg));
>> + autosel_support_in_ffh_or_sysmem = CPC_SUPPORTED(auto_sel_reg) &&
>> + (CPC_IN_FFH(auto_sel_reg) || CPC_IN_SYSTEM_MEMORY(auto_sel_reg));
>> +
>> if (CPC_IN_PCC(epp_set_reg) || CPC_IN_PCC(auto_sel_reg)) {
>> if (pcc_ss_id < 0) {
>> pr_debug("Invalid pcc_ss_id for CPU:%d\n", cpu);
>> @@ -1589,14 +1600,29 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
>> /* after writing CPC, transfer the ownership of PCC to platform */
>> ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>> up_write(&pcc_ss_data->pcc_lock);
>> - } else if (osc_cpc_flexible_adr_space_confirmed &&
>> - CPC_SUPPORTED(epp_set_reg) && CPC_IN_FFH(epp_set_reg)) {
>> - ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
>> + } else if (osc_cpc_flexible_adr_space_confirmed) {
>> + if (!epp_support_in_ffh_or_sysmem && !autosel_support_in_ffh_or_sysmem) {
>> + ret = -EOPNOTSUPP;
>> + } else {
>> + if (autosel_support_in_ffh_or_sysmem) {
>> + ret = cpc_write(cpu, auto_sel_reg, enable);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + if (epp_support_in_ffh_or_sysmem) {
>> + ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
>> + if (ret)
>> + return ret;
>> + }
> Wouldn't it be more clear to have separate functions for setting auto-sel
> and EPP? I think this is functionally correct, but somewhat unclear, given
> the signature of the function. But I do acknowledge that the function was
> like this to begin with.
In v2, changed names of some functions for better clarity. One of those
was rename cppc_set_epp_perf() to cppc_set_epp_and_autosel() to reflect
that the API updates both. Reverted those changes after suggestion from
Rafael in [1] to avoid renaming.
There are existing separate API's cppc_set_auto_sel() & cppc_set_epp()
which set the register directly.
This change keeps the behavior of cppc_set_epp_perf() consistent to update
both together as that is already happening for PCC but not for other cases.
[1]
https://lore.kernel.org/lkml/CAJZ5v0ipt=zDDORjuHzuerfLryZO5NE1RpPfnADeWEqi6kyv6Q@mail.gmail.com/
>> + }
>> } else {
>> - ret = -ENOTSUPP;
>> - pr_debug("_CPC in PCC and _CPC in FFH are not supported\n");
>> + ret = -EOPNOTSUPP;
>> }
>>
>> + if (ret == -EOPNOTSUPP)
>> + pr_debug("_CPC in PCC and _CPC in FFH are not supported\n");
> This message needs updating.
>
> Thank you,
> Ionela.
Sure.
Thank you,
Sumit Gupta
>> +
>> return ret;
>> }
>> EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
>> index 7190afeead8b..42e37a84cac9 100644
>> --- a/include/acpi/cppc_acpi.h
>> +++ b/include/acpi/cppc_acpi.h
>> @@ -119,7 +119,6 @@ struct cppc_perf_caps {
>> u32 lowest_nonlinear_perf;
>> u32 lowest_freq;
>> u32 nominal_freq;
>> - u32 energy_perf;
>> bool auto_sel;
>> };
>>
>> --
>> 2.34.1
>>
Powered by blists - more mailing lists