Re: [PATCH v5 3/8] ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()

["zhenglifeng (A)" <zhenglifeng1@huawei.com>]

On 2025/3/14 18:32, Rafael J. Wysocki wrote:

> On Fri, Mar 14, 2025 at 10:25 AM zhenglifeng (A)
> <zhenglifeng1@huawei.com> wrote:
>>
>> On 2025/3/13 3:54, Rafael J. Wysocki wrote:
>>
>>> On Thu, Feb 6, 2025 at 2:14 PM Lifeng Zheng <zhenglifeng1@huawei.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@huawei.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?
> 
> For those who know how to decode it, yes.  For others, not really.

At least it means something. But if you think this index is confusing for
those who don't know how to decode it, I'll remove it in the next version.

> 
>>>
>>> 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().
> 
> I see, so you need to explain this in the changelog.

OK.

> 
> And IMV the code logic should be:
> 
> (1) If this is a NULL register, don't use it.
> (2) If it is integer 0, check if it is optional.
>     (a) If it is optional, don't use it.
>     (b) Otherwise, use 0 as the value.
> 
> Of course, there is a problem for platforms that may want to pass 0 as
> an optional field value, but this is a spec issue.

I'll change the logic in next version. Thanks!