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

[Rafael J. Wysocki]

On Fri, Mar 14, 2025 at 10:25 AM zhenglifeng (A)
<zhenglifeng1@xxxxxxxxxx> wrote:
>
> On 2025/3/13 3:54, Rafael J. Wysocki wrote:
>
> > On Thu, Feb 6, 2025 at 2:14 PM Lifeng Zheng <zhenglifeng1@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >>  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.

> >
> > 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.

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.