Re: [PATCH v6 2/8] ACPI: CPPC: Optimize cppc_get_perf()

[Rafael J. Wysocki]

On Wed, Apr 9, 2025 at 8:57 AM Lifeng Zheng <zhenglifeng1@xxxxxxxxxx> 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@xxxxxxx>
> Signed-off-by: Lifeng Zheng <zhenglifeng1@xxxxxxxxxx>
> ---
>  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)) {

> +               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);
>  }
>
>  /**
> --