| 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: <aRShdY+QNQZdRewN@arm.com>
Date: Wed, 12 Nov 2025 15:02:13 +0000
From: Ionela Voinescu <ionela.voinescu@....com>
To: Sumit Gupta <sumitg@...dia.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
Subject: Re: [PATCH v4 3/8] ACPI: CPPC: extend APIs to support auto_sel and
epp
Hi,
A small nit that applies to multiple places: let's keep the line length
under 80 characters - the lines seem easy to split.
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.
> + }
> } 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.
> +
> 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