Re: [PATCH v3 1/4] ACPI: CPPC: Add cppc_get_reg_val and cppc_set_reg_val function

[Pierre Gondois <pierre.gondois@arm.com>]

Hello Lifeng,

On 12/20/24 09:30, zhenglifeng (A) wrote:
> On 2024/12/17 21:48, Pierre Gondois wrote:
>> Hello Lifeng,
>>
>> On 12/16/24 10:16, Lifeng Zheng wrote:
>>> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
>>> cppc registers, with four 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.
>>>
>>> 3. Extract the operations if register is in pcc out as
>>> cppc_get_reg_val_in_pcc().
>>>
>>> 4. Return the result of cpc_read() instead of 0.
>>>
>>> Add cppc_set_reg_val_in_pcc() and cppc_set_reg_val() as generic functions
>>> for setting cppc registers value. Unlike other set reg ABIs,
>>> cppc_set_reg_val() 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.
>>>
>>> These functions can be used to reduce some existing code duplication.
>>>
>>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>>> ---
>>>    drivers/acpi/cppc_acpi.c | 111 +++++++++++++++++++++++++++++----------
>>>    1 file changed, 84 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>> index c1f3568d0c50..bb5333a503a2 100644
>>> --- a/drivers/acpi/cppc_acpi.c
>>> +++ b/drivers/acpi/cppc_acpi.c
>>> @@ -1179,43 +1179,100 @@ 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);
>>> +    int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>>> +    struct cppc_pcc_data *pcc_ss_data = NULL;
>>> +    int ret;
>>> +
>>> +    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)
>>> +        ret = cpc_read(cpu, reg, val);
>>> +    else
>>> +        ret = -EIO;
>>> +
>>> +    up_write(&pcc_ss_data->pcc_lock);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int cppc_get_reg_val(int cpu, enum cppc_regs reg_idx, u64 *val)
>>> +{
>>> +    struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>>>        struct cpc_register_resource *reg;
>>>          if (!cpc_desc) {
>>> -        pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>>> +        pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>>>            return -ENODEV;
>>>        }
>>>          reg = &cpc_desc->cpc_regs[reg_idx];
>>>    -    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;
>>> -
>>> -        if (pcc_ss_id < 0)
>>> -            return -EIO;
>>> +    if (!CPC_SUPPORTED(reg)) {
>>> +        pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>>> +        return -EOPNOTSUPP;
>>> +    }
>>
>> I think this is only valid for optional fields. Meaning that:
>> - if the function is used one day for the mandatory 'Lowest Performance'
>> field, an integer value of 0 would be valid.
>> - if the function is used for a mandatory field containing a NULL Buffer,
>> it seems we would return -EFAULT currently, through cpc_read(). -EOPNOTSUPP
>> doesn't seem appropriate as the field would be mandatory.
>>
>> Maybe the function needs an additional 'bool optional' input parameter
>> to do these check conditionally.
> 
> Indeed, I should have judged the type before doing this check. But adding a
> input parameter is not a really nice way to me. How about adding a bool
> list of length MAX_CPC_REG_ENT in cppc_acpi.h to indicate wheter it is
> optional?

Actually all these functions:
- cppc_get_desired_perf
- cppc_get_highest_perf
- cppc_get_epp_perf
- cppc_set_epp
- cppc_get_auto_act_window
- cppc_set_auto_act_window
- cppc_get_auto_sel
- cppc_get_nominal_perf

and in general all the functions getting / setting one value at a time could
be implemented by macros similars to show_cppc_data(). From what I see the
input parameters required are:
- name of the field
- if the field is mandatory to have or not
- if the field is writeable
- if the field is implemented as an integer, register, or can be both

This would avoid having numerous function definitions doing approximately the
same thing.

> 
>>
>>>    -        pcc_ss_data = pcc_data[pcc_ss_id];
>>> +    if (CPC_IN_PCC(reg))
>>> +        return cppc_get_reg_val_in_pcc(cpu, reg, val);
>>>    -        down_write(&pcc_ss_data->pcc_lock);
>>> +    return cpc_read(cpu, reg, val);
>>> +}
>>>    -        if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
>>> -            cpc_read(cpunum, reg, perf);
>>> -        else
>>> -            ret = -EIO;
>>> +static int cppc_set_reg_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 val)
>>> +{
>>> +    int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>>> +    struct cppc_pcc_data *pcc_ss_data = NULL;
>>> +    int ret;
>>>    -        up_write(&pcc_ss_data->pcc_lock);
>>> +    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;
>>> +}
>>> +
>>> +static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val)
>>> +{
>>> +    struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>>> +    struct cpc_register_resource *reg;
>>> +
>>> +    if (!cpc_desc) {
>>> +        pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>>> +        return -ENODEV;
>>>        }
>>>    -    cpc_read(cpunum, reg, perf);
>>> +    reg = &cpc_desc->cpc_regs[reg_idx];
>>>    -    return 0;
>>> +    if (!CPC_SUPPORTED(reg)) {
>>> +        pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>>> +        return -EOPNOTSUPP;
>>> +    }
>>
>> Similarly to cppc_get_reg_val(), if a field is:
>> - mandatory + integer: currently doesn't exist. Not sure we should
>> try to detect that, but might be safer.
>> - mandatory + buffer: should not return -EOPNOTSUPP I think
>> - optional + integer: e.g.: 'Autonomous Selection Enable Register',
>> we should return -EOPNOTSUPP. It seems that currently, if the integer
>> value is 1, I get a 'write error: Bad address'
>> - optional + buffer:
>> should effectively return -EOPNOTSUPP if the buffer is NULL.
> 
> Actually, cpc_write() doesn't check field type and treats the field as a
> buffer. That's why you get 'Bad address' error when the integer value is 1.
> I think the existing code needs to be improved, otherwise there may be
> unexpected problems.
> 
> Do you mean we should return -EOPNOTSUPP no matter what to be written if
> this field is a optional + integer one?

Yes exact

  And what about a mandatory +
> integer one. Should we directly write the int_value?

I don't think it is possible to have this. Indeed, if a value is writeable,
it must be a register, so mandatory + integer should not exist. I suggested
a check in case someone made a mistake, but it is not sure the check is actually
necessary.

Regards,
Pierre