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