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;
+	}
 
-		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;
+	}
+
+	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);
 
-- 
2.33.0

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.

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

> +
> +	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);
>

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?

> 
>>   -        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);
>>   
> 

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

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

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
> 

Hello Lifeng,

On 1/10/25 03:23, zhenglifeng (A) wrote:
> 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@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
> 
> 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.

If you're referring to the [get|set]_auto_act_window() functions, I guess
it should be ok to have the getter/setter functions implemented as a macros,
and then have a wrapper to do the conversion of the value.

> 
>> - 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 you prefer to have specific macro names to distinguish optional/mandatory
fields, it also seems a good solution.

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

Yes right, same comment as above.

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

Yes indeed.

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