Message-ID: <74be38cf-8e18-44fc-995c-a5b734d9df29@huawei.com>
Date: Fri, 20 Dec 2024 16:30: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

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?

> 
>>   -        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? And what about a mandatory +
integer one. Should we directly write the int_value?

Looking forward to your opinion.

> 
>> +
>> +    if (CPC_IN_PCC(reg))
>> +        return cppc_set_reg_val_in_pcc(cpu, reg, val);
>> +
>> +    return cpc_write(cpu, reg, val);
>>   }
>>     /**
>> @@ -1223,11 +1280,11 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>>    * @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_val(cpunum, DESIRED_PERF, desired_perf);
>>   }
>>   EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>>   @@ -1236,11 +1293,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.
>>    */
>>   int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
>>   {
>> -    return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf);
>> +    return cppc_get_reg_val(cpunum, NOMINAL_PERF, nominal_perf);
>>   }
>>     /**
>> @@ -1248,11 +1305,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_val(cpunum, HIGHEST_PERF, highest_perf);
>>   }
>>   EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>>   @@ -1261,11 +1318,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.
>>    */
>>   int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
>>   {
>> -    return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf);
>> +    return cppc_get_reg_val(cpunum, ENERGY_PERF, epp_perf);
>>   }
>>   EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
>>   
> 

Powered by blists - more mailing lists