linux-kernel - Re: [PATCH v6 2/8] ACPI: CPPC: Optimize cppc_get

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4154812c-dfe0-4973-bb78-f7af9163933c@huawei.com>
Date: Thu, 10 Apr 2025 16:23:23 +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>,
	<perry.yuan@....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>, <jonathan.cameron@...wei.com>,
	<zhanjie9@...ilicon.com>, <lihuisong@...wei.com>,
	<cenxinghai@...artners.com>, <hepeng68@...wei.com>
Subject: Re: [PATCH v6 2/8] ACPI: CPPC: Optimize cppc_get_perf()

On 2025/4/10 1:10, Rafael J. Wysocki wrote:
> On Wed, Apr 9, 2025 at 8:57 AM Lifeng Zheng <zhenglifeng1@...wei.com> wrote:
>>
>> Optimize cppc_get_perf() with three changes:
>>
>> 1. Change the error kind to "no such device" when pcc_ss_id < 0, as other
>> register value getting functions.
>>
>> 2. Add a check to verify if the register is supported to be read before
>> using it. The logic is:
>>
>> (1) If the register is of the integer type, check whether the register is
>> optional and its value is 0. If yes, the register is not supported.
>>
>> (2) If the register is of other types, a null one is not supported.
>>
>> 3. Return the result of cpc_read() instead of 0.
>>
>> Reviewed-by: Pierre Gondois <pierre.gondois@....com>
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@...wei.com>
>> ---
>>  drivers/acpi/cppc_acpi.c | 21 ++++++++++++++-------
>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 39f019e265da..2f789d3b3cad 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1201,20 +1201,29 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>>
>>         reg = &cpc_desc->cpc_regs[reg_idx];
>>
>> +       if (reg->type == ACPI_TYPE_INTEGER ?
>> +           (IS_OPTIONAL_CPC_REG(reg_idx) && !reg->cpc_entry.int_value) :
>> +           IS_NULL_REG(&reg->cpc_entry.reg)) {
> 
> Please avoid using the ternary operator in any new kernel code.
> 
> Why not write it this way
> 
> if ((reg->type == ACPI_TYPE_INTEGER && IS_OPTIONAL_CPC_REG(reg_idx)
>     && !reg->cpc_entry.int_value) || (reg->type != ACPI_TYPE_INTEGER &&
>     IS_NULL_REG(&reg->cpc_entry.reg)) {

OK. Will replace it. Thanks!

> 
>> +               pr_debug("CPC register is not supported\n");
>> +               return -EOPNOTSUPP;
>> +       }
>> +
>>         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 = 0;
>> +               int ret;
>>
>> -               if (pcc_ss_id < 0)
>> -                       return -EIO;
>> +               if (pcc_ss_id < 0) {
>> +                       pr_debug("Invalid pcc_ss_id\n");
>> +                       return -ENODEV;
>> +               }
>>
>>                 pcc_ss_data = pcc_data[pcc_ss_id];
>>
>>                 down_write(&pcc_ss_data->pcc_lock);
>>
>>                 if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
>> -                       cpc_read(cpunum, reg, perf);
>> +                       ret = cpc_read(cpunum, reg, perf);
>>                 else
>>                         ret = -EIO;
>>
>> @@ -1223,9 +1232,7 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>>                 return ret;
>>         }
>>
>> -       cpc_read(cpunum, reg, perf);
>> -
>> -       return 0;
>> +       return cpc_read(cpunum, reg, perf);
>>  }
>>
>>  /**
>> --
> 
>