| 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: <CAJZ5v0iNzNROkPD4+b=Au8DwdF9unajKivdRQMBFfwzjFxHLcg@mail.gmail.com>
Date: Wed, 12 Mar 2025 20:54:36 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Lifeng Zheng <zhenglifeng1@...wei.com>
Cc: rafael@...nel.org, lenb@...nel.org, robert.moore@...el.com,
viresh.kumar@...aro.org, mario.limonciello@....com, gautham.shenoy@....com,
ray.huang@....com, pierre.gondois@....com, acpica-devel@...ts.linux.dev,
linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org, linuxarm@...wei.com, yumpusamongus@...il.com,
srinivas.pandruvada@...ux.intel.com, jonathan.cameron@...wei.com,
zhanjie9@...ilicon.com, lihuisong@...wei.com, hepeng68@...wei.com,
fanghao11@...wei.com
Subject: Re: [PATCH v5 3/8] ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()
On Thu, Feb 6, 2025 at 2:14 PM Lifeng Zheng <zhenglifeng1@...wei.com> wrote:
>
> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
> cppc registers. And extract the operations if register is in pcc out as
> cppc_get_reg_val_in_pcc(). Without functional change.
This should be split into two patches IMV.
> Signed-off-by: Lifeng Zheng <zhenglifeng1@...wei.com>
> ---
> drivers/acpi/cppc_acpi.c | 66 +++++++++++++++++++++-------------------
> 1 file changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index db22f8f107db..3c9c4ce2a0b0 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1189,48 +1189,52 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
> return ret_val;
> }
>
> -static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
> +static int cppc_get_reg_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 *val)
> {
> - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> - struct cpc_register_resource *reg;
> + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> + struct cppc_pcc_data *pcc_ss_data = NULL;
> + int ret;
>
> - if (!cpc_desc) {
> - pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> + if (pcc_ss_id < 0) {
> + pr_debug("Invalid pcc_ss_id\n");
> return -ENODEV;
> }
>
> - reg = &cpc_desc->cpc_regs[reg_idx];
> + pcc_ss_data = pcc_data[pcc_ss_id];
>
> - if (IS_OPTIONAL_CPC_REG(reg_idx) && !CPC_SUPPORTED(reg)) {
> - pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
> - return -EOPNOTSUPP;
> - }
I'm not a big fan of the IS_OPTIONAL_CPC_REG() macro. I'm not
convinced at all that it adds any value above (and in the next patch
for that matter) and the message printing the register index is just
plain unuseful to anyone who doesn't know how to decode it.
If CPC_SUPPORTED(reg) is not true, the register cannot be used AFAICS
regardless of what IS_OPTIONAL_CPC_REG() has to say about it.
> + down_write(&pcc_ss_data->pcc_lock);
>
> - if (CPC_IN_PCC(reg)) {
> - int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> - struct cppc_pcc_data *pcc_ss_data = NULL;
> - int ret;
> + if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
> + ret = cpc_read(cpu, reg, val);
> + else
> + ret = -EIO;
>
> - if (pcc_ss_id < 0) {
> - pr_debug("Invalid pcc_ss_id\n");
> - return -ENODEV;
> - }
> + up_write(&pcc_ss_data->pcc_lock);
>
> - pcc_ss_data = pcc_data[pcc_ss_id];
> + return ret;
> +}
>
> - down_write(&pcc_ss_data->pcc_lock);
> +static int cppc_get_reg_val(int cpu, enum cppc_regs reg_idx, u64 *val)
> +{
> + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> + struct cpc_register_resource *reg;
>
> - if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
> - ret = cpc_read(cpunum, reg, perf);
> - else
> - ret = -EIO;
> + if (!cpc_desc) {
> + pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> + return -ENODEV;
> + }
>
> - up_write(&pcc_ss_data->pcc_lock);
> + reg = &cpc_desc->cpc_regs[reg_idx];
>
> - return ret;
> + if (IS_OPTIONAL_CPC_REG(reg_idx) && !CPC_SUPPORTED(reg)) {
> + pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
> + return -EOPNOTSUPP;
> }
>
> - return cpc_read(cpunum, reg, perf);
> + if (CPC_IN_PCC(reg))
> + return cppc_get_reg_val_in_pcc(cpu, reg, val);
> +
> + return cpc_read(cpu, reg, val);
> }
>
> /**
> @@ -1242,7 +1246,7 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
> */
> int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
> {
> - return cppc_get_perf(cpunum, DESIRED_PERF, desired_perf);
> + return cppc_get_reg_val(cpunum, DESIRED_PERF, desired_perf);
> }
> EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>
> @@ -1255,7 +1259,7 @@ EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
> */
> int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
> {
> - return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf);
> + return cppc_get_reg_val(cpunum, NOMINAL_PERF, nominal_perf);
> }
>
> /**
> @@ -1267,7 +1271,7 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
> */
> int cppc_get_highest_perf(int cpunum, u64 *highest_perf)
> {
> - return cppc_get_perf(cpunum, HIGHEST_PERF, highest_perf);
> + return cppc_get_reg_val(cpunum, HIGHEST_PERF, highest_perf);
> }
> EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>
> @@ -1280,7 +1284,7 @@ EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
> */
> int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
> {
> - return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf);
> + return cppc_get_reg_val(cpunum, ENERGY_PERF, epp_perf);
> }
> EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
>
> --
> 2.33.0
>
>
Powered by blists - more mailing lists