Message-ID: <49bd9db5-b05d-4bed-8f9d-10ec087323b5@huawei.com>
Date: Fri, 10 Jan 2025 10:23:00 +0800
From: "zhenglifeng (A)" <zhenglifeng1@...wei.com>
To: Pierre Gondois <pierre.gondois@....com>, <rafael@...nel.org>,
	<lenb@...nel.org>, <robert.moore@...el.com>, <viresh.kumar@...aro.org>
CC: <acpica-devel@...ts.linux.dev>, <linux-acpi@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-pm@...r.kernel.org>,
	<linuxarm@...wei.com>, <ionela.voinescu@....com>,
	<jonathan.cameron@...wei.com>, <zhanjie9@...ilicon.com>,
	<lihuisong@...wei.com>, <hepeng68@...wei.com>, <fanghao11@...wei.com>
Subject: Re: [PATCH v3 1/4] ACPI: CPPC: Add cppc_get_reg_val and
 cppc_set_reg_val function

Hello Pierre,

On 2025/1/8 0:54, Pierre Gondois wrote:
> 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@...wei.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

As you suggest in another patch, the logic should be placed in
cppc_get_auto_act_window() and some other functions. I'm afraid these
functions couldn't be implemented with the macros you suggest.

> - 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 with this parameter, we should put all the cppc_get_reg_val() and
cppc_set_reg_val() in the macro. This wouldn't look really nice. I
prefer to use a macro to judge mandatory / optional. I'll show you in
v4.

> - if the field is writeable

I think we can define a READ macro, a WRITE macro and a RW macro. For
the registers which are not writeable, only use the READ macro to
implement getting function.

> - if the field is implemented as an integer, register, or can be both

I don't think this parameter is necessary. The field type can be got
from cpc_desc->cpc_regs[reg_idx].type.

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

So from what I see the input parameters required are name of the field
and reg_idx. Thanks for your advice!

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

Yeah, I think it's better to have this check, too.

Regards,
Lifeng

> 
> Regards,
> Pierre
> 

Powered by blists - more mailing lists