Re: [PATCH v2 1/3] ACPI: CPPC: Refactor register get and set ABIs

[zhenglifeng (A)]

On 2024/12/11 2:14, Rafael J. Wysocki wrote:
> On Fri, Nov 22, 2024 at 7:21 AM Lifeng Zheng <zhenglifeng1@xxxxxxxxxx> wrote:
>>
>> Refactor register get and set ABIs using cppc_get_reg() and cppc_set_reg().
> 
> I don't quite like the cppc_get_reg() name.  I think that
> cppc_get_reg_val() would be better.

Indeed, it is better. Will change. Thanks.

> 
>> Rename cppc_get_perf() to cppc_get_reg() as a generic function to read cppc
>> registers, with two changes:
>>
>> 1. Change the error kind to "no such device" when pcc_ss_id < 0, which
>> means that this cpu cannot get a valid pcc_ss_id.
>>
>> 2. Add a check to verify if the register is a cpc supported one before
>> using it.
> 
> So it's not just a rename, but also a change in behavior.  Can this
> change in behavior become user-visible?

The register value get ABIs in this file returned different error numbers
when pcc_ss_id < 0, but should be the same one. So I chose a most suitable
one I thought to be returned here when doing refactoring. This change is
not user-visible as I know.

It is necessary to do the CPC_SUPPORTED() check before using the register.
If it is not a cpc supported one, the rest of the operation is pointless
and may be dangerous. This change might be user-visible but is still
necessary.

> 
>> Add cppc_set_reg() as a generic function for setting cppc registers.
> 
> Again, I would prefer cppc_set_reg_val().
> 
>> Unlike other set reg ABIs, this function checks CPC_SUPPORTED right after getting
>> the register, because the rest of the operations are meaningless if this
>> register is not a cpc supported one.
> 
> And the new function is used to reduce some existing code duplication,
> isn't it?  Which would be good to mention here.

Yes, Will mention it in next version. Thanks.

> 
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@xxxxxxxxxx>
>> ---
>>  drivers/acpi/cppc_acpi.c | 191 +++++++++++++++------------------------
>>  1 file changed, 72 insertions(+), 119 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index c1f3568d0c50..9aab22d8136a 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1179,10 +1179,13 @@ 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(int cpunum, enum cppc_regs reg_idx, u64 *val)
>>  {
>>         struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>> +       struct cppc_pcc_data *pcc_ss_data = NULL;
> 
> Why are you moving this here?  This change is not related to the rest
> of the patch, is it?
> 
>>         struct cpc_register_resource *reg;
>> +       int pcc_ss_id;
>> +       int ret = 0;
> 
> And here?

Moving these because I'm used to declare variables at the beginning of a
function. It's really unnecessary. After defining new functions as
cppc_get_reg_val_in_pcc() and cppc_set_reg_val_in_pcc() as you suggest
below, these variables will be moved to the new functions.

> 
>>
>>         if (!cpc_desc) {
>>                 pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>> @@ -1191,20 +1194,23 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>>
>>         reg = &cpc_desc->cpc_regs[reg_idx];
>>
>> +       if (!CPC_SUPPORTED(reg)) {
>> +               pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>> +               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;
>> +               pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
>>
>>                 if (pcc_ss_id < 0)
>> -                       return -EIO;
>> +                       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);
>> +                       cpc_read(cpunum, reg, val);
>>                 else
>>                         ret = -EIO;
>>
>> @@ -1213,21 +1219,65 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>>                 return ret;
>>         }
>>
>> -       cpc_read(cpunum, reg, perf);
>> +       cpc_read(cpunum, reg, val);
>>
>>         return 0;
>>  }
>>
>> +static int cppc_set_reg(int cpu, enum cppc_regs reg_idx, u64 val)
>> +{
>> +       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> +       struct cppc_pcc_data *pcc_ss_data = NULL;
>> +       struct cpc_register_resource *reg;
>> +       int pcc_ss_id;
>> +       int ret;
>> +
>> +       if (!cpc_desc) {
>> +               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> +               return -ENODEV;
>> +       }
>> +
>> +       reg = &cpc_desc->cpc_regs[reg_idx];
>> +
>> +       if (!CPC_SUPPORTED(reg)) {
>> +               pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>> +               return -EOPNOTSUPP;
>> +       }
>> +
>> +       if (CPC_IN_PCC(reg)) {
>> +               pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> 
> Please declare the variables that are only needed in the PCC case here.
> 
> Also, I think it would be better to define a new function, say
> cppc_set_reg_val_in_pcc() for this code and then have
> 
> if (CPC_IN_PCC(reg))
>         return cppc_set_reg_val_in_pcc(reg, val);

Will define new functions as cppc_get_reg_val_in_pcc() and
cppc_set_reg_val_in_pcc(). Thanks.

> 
>> +
>> +               if (pcc_ss_id < 0) {
>> +                       pr_debug("Invalid pcc_ss_id\n");
>> +                       return -ENODEV;
>> +               }
>> +
>> +               ret = cpc_write(cpu, reg, val);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               pcc_ss_data = pcc_data[pcc_ss_id];
>> +
>> +               down_write(&pcc_ss_data->pcc_lock);
>> +               /* 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);
>> +               return ret;
>> +       }
>> +
>> +       return cpc_write(cpu, reg, val);
>> +}
>> +
>>  /**
>>   * cppc_get_desired_perf - Get the desired performance register value.
>>   * @cpunum: CPU from which to get desired performance.
>>   * @desired_perf: Return address.
>>   *
>> - * Return: 0 for success, -EIO otherwise.
>> + * Return: 0 for success, -ERRNO otherwise.
>>   */
>>  int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
>>  {
>> -       return cppc_get_perf(cpunum, DESIRED_PERF, desired_perf);
>> +       return cppc_get_reg(cpunum, DESIRED_PERF, desired_perf);
>>  }
>>  EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>>
>> @@ -1236,11 +1286,11 @@ EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>>   * @cpunum: CPU from which to get nominal performance.
>>   * @nominal_perf: Return address.
>>   *
>> - * Return: 0 for success, -EIO otherwise.
>> + * Return: 0 for success, -ERRNO otherwise.
> 
> What do you mean by ERRNO?

Error number. I see this expression elsewhere in this file so I use it too.

> 
>>   */
>>  int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
>>  {
>> -       return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf);
>> +       return cppc_get_reg(cpunum, NOMINAL_PERF, nominal_perf);
>>  }
>>
>>  /**
>> @@ -1248,11 +1298,11 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
>>   * @cpunum: CPU from which to get highest performance.
>>   * @highest_perf: Return address.
>>   *
>> - * Return: 0 for success, -EIO otherwise.
>> + * Return: 0 for success, -ERRNO otherwise.
>>   */
>>  int cppc_get_highest_perf(int cpunum, u64 *highest_perf)
>>  {
>> -       return cppc_get_perf(cpunum, HIGHEST_PERF, highest_perf);
>> +       return cppc_get_reg(cpunum, HIGHEST_PERF, highest_perf);
>>  }
>>  EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>>
>> @@ -1261,11 +1311,11 @@ EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>>   * @cpunum: CPU from which to get epp preference value.
>>   * @epp_perf: Return address.
>>   *
>> - * Return: 0 for success, -EIO otherwise.
>> + * Return: 0 for success, -ERRNO otherwise.
> 
> Same here?
> 
>>   */
>>  int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
>>  {
>> -       return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf);
>> +       return cppc_get_reg(cpunum, ENERGY_PERF, epp_perf);
>>  }
>>  EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
> 
> It would be cleaner to do the changes below in a separate patch IMV.

Will separate it. Thanks.

> 
>> @@ -1545,44 +1595,14 @@ EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>>   */
>>  int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
>>  {
>> -       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>> -       struct cpc_register_resource *auto_sel_reg;
>> -       u64  auto_sel;
>> -
>> -       if (!cpc_desc) {
>> -               pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>> -               return -ENODEV;
>> -       }
>> -
>> -       auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
>> -
>> -       if (!CPC_SUPPORTED(auto_sel_reg))
>> -               pr_warn_once("Autonomous mode is not unsupported!\n");
>> -
>> -       if (CPC_IN_PCC(auto_sel_reg)) {
>> -               int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
>> -               struct cppc_pcc_data *pcc_ss_data = NULL;
>> -               int ret = 0;
>> -
>> -               if (pcc_ss_id < 0)
>> -                       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, auto_sel_reg, &auto_sel);
>> -                       perf_caps->auto_sel = (bool)auto_sel;
>> -               } else {
>> -                       ret = -EIO;
>> -               }
>> -
>> -               up_write(&pcc_ss_data->pcc_lock);
>> +       u64 auto_sel;
>> +       int ret;
>>
>> +       ret = cppc_get_reg(cpunum, AUTO_SEL_ENABLE, &auto_sel);
>> +       if (ret)
>>                 return ret;
>> -       }
>>
>> +       perf_caps->auto_sel = (bool)auto_sel;
>>         return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
>> @@ -1594,43 +1614,7 @@ EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
>>   */
>>  int cppc_set_auto_sel(int cpu, bool enable)
>>  {
>> -       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>> -       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;
>> -       int ret = -EINVAL;
>> -
>> -       if (!cpc_desc) {
>> -               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> -               return -ENODEV;
>> -       }
>> -
>> -       auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
>> -
>> -       if (CPC_IN_PCC(auto_sel_reg)) {
>> -               if (pcc_ss_id < 0) {
>> -                       pr_debug("Invalid pcc_ss_id\n");
>> -                       return -ENODEV;
>> -               }
>> -
>> -               if (CPC_SUPPORTED(auto_sel_reg)) {
>> -                       ret = cpc_write(cpu, auto_sel_reg, enable);
>> -                       if (ret)
>> -                               return ret;
>> -               }
>> -
>> -               pcc_ss_data = pcc_data[pcc_ss_id];
>> -
>> -               down_write(&pcc_ss_data->pcc_lock);
>> -               /* 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 {
>> -               ret = -ENOTSUPP;
>> -               pr_debug("_CPC in PCC is not supported\n");
>> -       }
>> -
>> -       return ret;
>> +       return cppc_set_reg(cpu, AUTO_SEL_ENABLE, enable);
>>  }
>>  EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
>>
>> @@ -1644,38 +1628,7 @@ EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
>>   */
>>  int cppc_set_enable(int cpu, bool enable)
>>  {
>> -       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>> -       struct cpc_register_resource *enable_reg;
>> -       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> -       struct cppc_pcc_data *pcc_ss_data = NULL;
>> -       int ret = -EINVAL;
>> -
>> -       if (!cpc_desc) {
>> -               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> -               return -EINVAL;
>> -       }
>> -
>> -       enable_reg = &cpc_desc->cpc_regs[ENABLE];
>> -
>> -       if (CPC_IN_PCC(enable_reg)) {
>> -
>> -               if (pcc_ss_id < 0)
>> -                       return -EIO;
>> -
>> -               ret = cpc_write(cpu, enable_reg, enable);
>> -               if (ret)
>> -                       return ret;
>> -
>> -               pcc_ss_data = pcc_data[pcc_ss_id];
>> -
>> -               down_write(&pcc_ss_data->pcc_lock);
>> -               /* after writing CPC, transfer the ownership of PCC to platfrom */
>> -               ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>> -               up_write(&pcc_ss_data->pcc_lock);
>> -               return ret;
>> -       }
>> -
>> -       return cpc_write(cpu, enable_reg, enable);
>> +       return cppc_set_reg(cpu, ENABLE, enable);
>>  }
>>  EXPORT_SYMBOL_GPL(cppc_set_enable);
>>
>> --
>