| 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: <4fc77a58-8c77-463c-a50d-06ad19685bfb@huawei.com>
Date: Fri, 14 Mar 2025 17:24:59 +0800
From: "zhenglifeng (A)" <zhenglifeng1@...wei.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
CC: <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 2025/3/13 3:54, Rafael J. Wysocki wrote:
> 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.
Yes. That makes sense. Thanks.
>
>> 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.
With this index, it is easier to locate problems. This is what a "pr_debug"
for, isn't 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.
The name "CPC_SUPPORTED" may be a little confused. Actually, in ACPI 6.5,
only optional _CPC package fields that are not supported by the platform
should be encoded as 0 intergers or NULL registers. A mandatory field as a
0 interger is valid. So If I wanted to make this function as a generic one
to read cppc registers, it would have been more reasonable to do this
IS_OPTIONAL_CPC_REG() check before CPC_SUPPORTED().
>
>> + 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