Refactor register get and set ABIs using cppc_get_reg() and cppc_set_reg().
Rename cppc_get_perf() to cppc_get_reg() as a generic function to read cppc
registers, with two 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.
Add cppc_set_reg() as a generic function for setting cppc registers. Unlike
other set reg ABIs, this function 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.
Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
drivers/acpi/cppc_acpi.c | 191 +++++++++++++++------------------------
1 file changed, 72 insertions(+), 119 deletions(-)
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index c1f3568d0c50..9aab22d8136a 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1179,10 +1179,13 @@ 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(int cpunum, enum cppc_regs reg_idx, u64 *val)
{
struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
+ struct cppc_pcc_data *pcc_ss_data = NULL;
struct cpc_register_resource *reg;
+ int pcc_ss_id;
+ int ret = 0;
if (!cpc_desc) {
pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
@@ -1191,20 +1194,23 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
reg = &cpc_desc->cpc_regs[reg_idx];
+ if (!CPC_SUPPORTED(reg)) {
+ pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
+ return -EOPNOTSUPP;
+ }
+
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;
+ pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
if (pcc_ss_id < 0)
- return -EIO;
+ 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)
- cpc_read(cpunum, reg, perf);
+ cpc_read(cpunum, reg, val);
else
ret = -EIO;
@@ -1213,21 +1219,65 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
return ret;
}
- cpc_read(cpunum, reg, perf);
+ cpc_read(cpunum, reg, val);
return 0;
}
+static int cppc_set_reg(int cpu, enum cppc_regs reg_idx, u64 val)
+{
+ struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
+ struct cppc_pcc_data *pcc_ss_data = NULL;
+ struct cpc_register_resource *reg;
+ int pcc_ss_id;
+ int ret;
+
+ if (!cpc_desc) {
+ pr_debug("No CPC descriptor for CPU:%d\n", cpu);
+ return -ENODEV;
+ }
+
+ reg = &cpc_desc->cpc_regs[reg_idx];
+
+ if (!CPC_SUPPORTED(reg)) {
+ pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
+ return -EOPNOTSUPP;
+ }
+
+ if (CPC_IN_PCC(reg)) {
+ pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+
+ 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;
+ }
+
+ return cpc_write(cpu, reg, val);
+}
+
/**
* cppc_get_desired_perf - Get the desired performance register value.
* @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(cpunum, DESIRED_PERF, desired_perf);
}
EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
@@ -1236,11 +1286,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(cpunum, NOMINAL_PERF, nominal_perf);
}
/**
@@ -1248,11 +1298,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(cpunum, HIGHEST_PERF, highest_perf);
}
EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
@@ -1261,11 +1311,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(cpunum, ENERGY_PERF, epp_perf);
}
EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
@@ -1545,44 +1595,14 @@ EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
*/
int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
{
- struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
- struct cpc_register_resource *auto_sel_reg;
- u64 auto_sel;
-
- if (!cpc_desc) {
- pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
- return -ENODEV;
- }
-
- auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
-
- if (!CPC_SUPPORTED(auto_sel_reg))
- pr_warn_once("Autonomous mode is not unsupported!\n");
-
- if (CPC_IN_PCC(auto_sel_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 -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) {
- cpc_read(cpunum, auto_sel_reg, &auto_sel);
- perf_caps->auto_sel = (bool)auto_sel;
- } else {
- ret = -EIO;
- }
-
- up_write(&pcc_ss_data->pcc_lock);
+ u64 auto_sel;
+ int ret;
+ ret = cppc_get_reg(cpunum, AUTO_SEL_ENABLE, &auto_sel);
+ if (ret)
return ret;
- }
+ perf_caps->auto_sel = (bool)auto_sel;
return 0;
}
EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
@@ -1594,43 +1614,7 @@ EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
*/
int cppc_set_auto_sel(int cpu, bool enable)
{
- int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
- struct cpc_register_resource *auto_sel_reg;
- struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
- struct cppc_pcc_data *pcc_ss_data = NULL;
- int ret = -EINVAL;
-
- if (!cpc_desc) {
- pr_debug("No CPC descriptor for CPU:%d\n", cpu);
- return -ENODEV;
- }
-
- auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
-
- if (CPC_IN_PCC(auto_sel_reg)) {
- if (pcc_ss_id < 0) {
- pr_debug("Invalid pcc_ss_id\n");
- return -ENODEV;
- }
-
- if (CPC_SUPPORTED(auto_sel_reg)) {
- ret = cpc_write(cpu, auto_sel_reg, enable);
- 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);
- } else {
- ret = -ENOTSUPP;
- pr_debug("_CPC in PCC is not supported\n");
- }
-
- return ret;
+ return cppc_set_reg(cpu, AUTO_SEL_ENABLE, enable);
}
EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
@@ -1644,38 +1628,7 @@ EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
*/
int cppc_set_enable(int cpu, bool enable)
{
- int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
- struct cpc_register_resource *enable_reg;
- struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
- struct cppc_pcc_data *pcc_ss_data = NULL;
- int ret = -EINVAL;
-
- if (!cpc_desc) {
- pr_debug("No CPC descriptor for CPU:%d\n", cpu);
- return -EINVAL;
- }
-
- enable_reg = &cpc_desc->cpc_regs[ENABLE];
-
- if (CPC_IN_PCC(enable_reg)) {
-
- if (pcc_ss_id < 0)
- return -EIO;
-
- ret = cpc_write(cpu, enable_reg, enable);
- 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 platfrom */
- ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
- up_write(&pcc_ss_data->pcc_lock);
- return ret;
- }
-
- return cpc_write(cpu, enable_reg, enable);
+ return cppc_set_reg(cpu, ENABLE, enable);
}
EXPORT_SYMBOL_GPL(cppc_set_enable);
--
2.33.0On Fri, Nov 22, 2024 at 7:21 AM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> Refactor register get and set ABIs using cppc_get_reg() and cppc_set_reg().
I don't quite like the cppc_get_reg() name. I think that
cppc_get_reg_val() would be better.
> Rename cppc_get_perf() to cppc_get_reg() as a generic function to read cppc
> registers, with two 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.
So it's not just a rename, but also a change in behavior. Can this
change in behavior become user-visible?
> Add cppc_set_reg() as a generic function for setting cppc registers.
Again, I would prefer cppc_set_reg_val().
> Unlike other set reg ABIs, this function 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.
And the new function is used to reduce some existing code duplication,
isn't it? Which would be good to mention here.
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
> drivers/acpi/cppc_acpi.c | 191 +++++++++++++++------------------------
> 1 file changed, 72 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index c1f3568d0c50..9aab22d8136a 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1179,10 +1179,13 @@ 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(int cpunum, enum cppc_regs reg_idx, u64 *val)
> {
> struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> + struct cppc_pcc_data *pcc_ss_data = NULL;
Why are you moving this here? This change is not related to the rest
of the patch, is it?
> struct cpc_register_resource *reg;
> + int pcc_ss_id;
> + int ret = 0;
And here?
>
> if (!cpc_desc) {
> pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> @@ -1191,20 +1194,23 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>
> reg = &cpc_desc->cpc_regs[reg_idx];
>
> + if (!CPC_SUPPORTED(reg)) {
> + pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
> + return -EOPNOTSUPP;
> + }
> +
> 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;
> + pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
>
> if (pcc_ss_id < 0)
> - return -EIO;
> + 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)
> - cpc_read(cpunum, reg, perf);
> + cpc_read(cpunum, reg, val);
> else
> ret = -EIO;
>
> @@ -1213,21 +1219,65 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
> return ret;
> }
>
> - cpc_read(cpunum, reg, perf);
> + cpc_read(cpunum, reg, val);
>
> return 0;
> }
>
> +static int cppc_set_reg(int cpu, enum cppc_regs reg_idx, u64 val)
> +{
> + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> + struct cppc_pcc_data *pcc_ss_data = NULL;
> + struct cpc_register_resource *reg;
> + int pcc_ss_id;
> + int ret;
> +
> + if (!cpc_desc) {
> + pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> + return -ENODEV;
> + }
> +
> + reg = &cpc_desc->cpc_regs[reg_idx];
> +
> + if (!CPC_SUPPORTED(reg)) {
> + pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
> + return -EOPNOTSUPP;
> + }
> +
> + if (CPC_IN_PCC(reg)) {
> + pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
Please declare the variables that are only needed in the PCC case here.
Also, I think it would be better to define a new function, say
cppc_set_reg_val_in_pcc() for this code and then have
if (CPC_IN_PCC(reg))
return cppc_set_reg_val_in_pcc(reg, val);
> +
> + 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;
> + }
> +
> + return cpc_write(cpu, reg, val);
> +}
> +
> /**
> * cppc_get_desired_perf - Get the desired performance register value.
> * @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(cpunum, DESIRED_PERF, desired_perf);
> }
> EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>
> @@ -1236,11 +1286,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.
What do you mean by ERRNO?
> */
> int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
> {
> - return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf);
> + return cppc_get_reg(cpunum, NOMINAL_PERF, nominal_perf);
> }
>
> /**
> @@ -1248,11 +1298,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(cpunum, HIGHEST_PERF, highest_perf);
> }
> EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>
> @@ -1261,11 +1311,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.
Same here?
> */
> int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
> {
> - return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf);
> + return cppc_get_reg(cpunum, ENERGY_PERF, epp_perf);
> }
> EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
It would be cleaner to do the changes below in a separate patch IMV.
> @@ -1545,44 +1595,14 @@ EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
> */
> int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
> {
> - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> - struct cpc_register_resource *auto_sel_reg;
> - u64 auto_sel;
> -
> - if (!cpc_desc) {
> - pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> - return -ENODEV;
> - }
> -
> - auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
> -
> - if (!CPC_SUPPORTED(auto_sel_reg))
> - pr_warn_once("Autonomous mode is not unsupported!\n");
> -
> - if (CPC_IN_PCC(auto_sel_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 -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) {
> - cpc_read(cpunum, auto_sel_reg, &auto_sel);
> - perf_caps->auto_sel = (bool)auto_sel;
> - } else {
> - ret = -EIO;
> - }
> -
> - up_write(&pcc_ss_data->pcc_lock);
> + u64 auto_sel;
> + int ret;
>
> + ret = cppc_get_reg(cpunum, AUTO_SEL_ENABLE, &auto_sel);
> + if (ret)
> return ret;
> - }
>
> + perf_caps->auto_sel = (bool)auto_sel;
> return 0;
> }
> EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
> @@ -1594,43 +1614,7 @@ EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
> */
> int cppc_set_auto_sel(int cpu, bool enable)
> {
> - int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> - struct cpc_register_resource *auto_sel_reg;
> - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> - struct cppc_pcc_data *pcc_ss_data = NULL;
> - int ret = -EINVAL;
> -
> - if (!cpc_desc) {
> - pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> - return -ENODEV;
> - }
> -
> - auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
> -
> - if (CPC_IN_PCC(auto_sel_reg)) {
> - if (pcc_ss_id < 0) {
> - pr_debug("Invalid pcc_ss_id\n");
> - return -ENODEV;
> - }
> -
> - if (CPC_SUPPORTED(auto_sel_reg)) {
> - ret = cpc_write(cpu, auto_sel_reg, enable);
> - 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);
> - } else {
> - ret = -ENOTSUPP;
> - pr_debug("_CPC in PCC is not supported\n");
> - }
> -
> - return ret;
> + return cppc_set_reg(cpu, AUTO_SEL_ENABLE, enable);
> }
> EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
>
> @@ -1644,38 +1628,7 @@ EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
> */
> int cppc_set_enable(int cpu, bool enable)
> {
> - int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> - struct cpc_register_resource *enable_reg;
> - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> - struct cppc_pcc_data *pcc_ss_data = NULL;
> - int ret = -EINVAL;
> -
> - if (!cpc_desc) {
> - pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> - return -EINVAL;
> - }
> -
> - enable_reg = &cpc_desc->cpc_regs[ENABLE];
> -
> - if (CPC_IN_PCC(enable_reg)) {
> -
> - if (pcc_ss_id < 0)
> - return -EIO;
> -
> - ret = cpc_write(cpu, enable_reg, enable);
> - 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 platfrom */
> - ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> - up_write(&pcc_ss_data->pcc_lock);
> - return ret;
> - }
> -
> - return cpc_write(cpu, enable_reg, enable);
> + return cppc_set_reg(cpu, ENABLE, enable);
> }
> EXPORT_SYMBOL_GPL(cppc_set_enable);
>
> --
On 2024/12/11 2:14, Rafael J. Wysocki wrote:
> On Fri, Nov 22, 2024 at 7:21 AM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>
>> Refactor register get and set ABIs using cppc_get_reg() and cppc_set_reg().
>
> I don't quite like the cppc_get_reg() name. I think that
> cppc_get_reg_val() would be better.
Indeed, it is better. Will change. Thanks.
>
>> Rename cppc_get_perf() to cppc_get_reg() as a generic function to read cppc
>> registers, with two 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.
>
> So it's not just a rename, but also a change in behavior. Can this
> change in behavior become user-visible?
The register value get ABIs in this file returned different error numbers
when pcc_ss_id < 0, but should be the same one. So I chose a most suitable
one I thought to be returned here when doing refactoring. This change is
not user-visible as I know.
It is necessary to do the CPC_SUPPORTED() check before using the register.
If it is not a cpc supported one, the rest of the operation is pointless
and may be dangerous. This change might be user-visible but is still
necessary.
>
>> Add cppc_set_reg() as a generic function for setting cppc registers.
>
> Again, I would prefer cppc_set_reg_val().
>
>> Unlike other set reg ABIs, this function 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.
>
> And the new function is used to reduce some existing code duplication,
> isn't it? Which would be good to mention here.
Yes, Will mention it in next version. Thanks.
>
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>> drivers/acpi/cppc_acpi.c | 191 +++++++++++++++------------------------
>> 1 file changed, 72 insertions(+), 119 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index c1f3568d0c50..9aab22d8136a 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1179,10 +1179,13 @@ 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(int cpunum, enum cppc_regs reg_idx, u64 *val)
>> {
>> struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>> + struct cppc_pcc_data *pcc_ss_data = NULL;
>
> Why are you moving this here? This change is not related to the rest
> of the patch, is it?
>
>> struct cpc_register_resource *reg;
>> + int pcc_ss_id;
>> + int ret = 0;
>
> And here?
Moving these because I'm used to declare variables at the beginning of a
function. It's really unnecessary. After defining new functions as
cppc_get_reg_val_in_pcc() and cppc_set_reg_val_in_pcc() as you suggest
below, these variables will be moved to the new functions.
>
>>
>> if (!cpc_desc) {
>> pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>> @@ -1191,20 +1194,23 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>>
>> reg = &cpc_desc->cpc_regs[reg_idx];
>>
>> + if (!CPC_SUPPORTED(reg)) {
>> + pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>> + return -EOPNOTSUPP;
>> + }
>> +
>> 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;
>> + pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
>>
>> if (pcc_ss_id < 0)
>> - return -EIO;
>> + 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)
>> - cpc_read(cpunum, reg, perf);
>> + cpc_read(cpunum, reg, val);
>> else
>> ret = -EIO;
>>
>> @@ -1213,21 +1219,65 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>> return ret;
>> }
>>
>> - cpc_read(cpunum, reg, perf);
>> + cpc_read(cpunum, reg, val);
>>
>> return 0;
>> }
>>
>> +static int cppc_set_reg(int cpu, enum cppc_regs reg_idx, u64 val)
>> +{
>> + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> + struct cppc_pcc_data *pcc_ss_data = NULL;
>> + struct cpc_register_resource *reg;
>> + int pcc_ss_id;
>> + int ret;
>> +
>> + if (!cpc_desc) {
>> + pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> + return -ENODEV;
>> + }
>> +
>> + reg = &cpc_desc->cpc_regs[reg_idx];
>> +
>> + if (!CPC_SUPPORTED(reg)) {
>> + pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + if (CPC_IN_PCC(reg)) {
>> + pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>
> Please declare the variables that are only needed in the PCC case here.
>
> Also, I think it would be better to define a new function, say
> cppc_set_reg_val_in_pcc() for this code and then have
>
> if (CPC_IN_PCC(reg))
> return cppc_set_reg_val_in_pcc(reg, val);
Will define new functions as cppc_get_reg_val_in_pcc() and
cppc_set_reg_val_in_pcc(). Thanks.
>
>> +
>> + 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;
>> + }
>> +
>> + return cpc_write(cpu, reg, val);
>> +}
>> +
>> /**
>> * cppc_get_desired_perf - Get the desired performance register value.
>> * @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(cpunum, DESIRED_PERF, desired_perf);
>> }
>> EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>>
>> @@ -1236,11 +1286,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.
>
> What do you mean by ERRNO?
Error number. I see this expression elsewhere in this file so I use it too.
>
>> */
>> int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
>> {
>> - return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf);
>> + return cppc_get_reg(cpunum, NOMINAL_PERF, nominal_perf);
>> }
>>
>> /**
>> @@ -1248,11 +1298,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(cpunum, HIGHEST_PERF, highest_perf);
>> }
>> EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>>
>> @@ -1261,11 +1311,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.
>
> Same here?
>
>> */
>> int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
>> {
>> - return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf);
>> + return cppc_get_reg(cpunum, ENERGY_PERF, epp_perf);
>> }
>> EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
>
> It would be cleaner to do the changes below in a separate patch IMV.
Will separate it. Thanks.
>
>> @@ -1545,44 +1595,14 @@ EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>> */
>> int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
>> {
>> - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>> - struct cpc_register_resource *auto_sel_reg;
>> - u64 auto_sel;
>> -
>> - if (!cpc_desc) {
>> - pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>> - return -ENODEV;
>> - }
>> -
>> - auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
>> -
>> - if (!CPC_SUPPORTED(auto_sel_reg))
>> - pr_warn_once("Autonomous mode is not unsupported!\n");
>> -
>> - if (CPC_IN_PCC(auto_sel_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 -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) {
>> - cpc_read(cpunum, auto_sel_reg, &auto_sel);
>> - perf_caps->auto_sel = (bool)auto_sel;
>> - } else {
>> - ret = -EIO;
>> - }
>> -
>> - up_write(&pcc_ss_data->pcc_lock);
>> + u64 auto_sel;
>> + int ret;
>>
>> + ret = cppc_get_reg(cpunum, AUTO_SEL_ENABLE, &auto_sel);
>> + if (ret)
>> return ret;
>> - }
>>
>> + perf_caps->auto_sel = (bool)auto_sel;
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
>> @@ -1594,43 +1614,7 @@ EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
>> */
>> int cppc_set_auto_sel(int cpu, bool enable)
>> {
>> - int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>> - struct cpc_register_resource *auto_sel_reg;
>> - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> - struct cppc_pcc_data *pcc_ss_data = NULL;
>> - int ret = -EINVAL;
>> -
>> - if (!cpc_desc) {
>> - pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> - return -ENODEV;
>> - }
>> -
>> - auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
>> -
>> - if (CPC_IN_PCC(auto_sel_reg)) {
>> - if (pcc_ss_id < 0) {
>> - pr_debug("Invalid pcc_ss_id\n");
>> - return -ENODEV;
>> - }
>> -
>> - if (CPC_SUPPORTED(auto_sel_reg)) {
>> - ret = cpc_write(cpu, auto_sel_reg, enable);
>> - 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);
>> - } else {
>> - ret = -ENOTSUPP;
>> - pr_debug("_CPC in PCC is not supported\n");
>> - }
>> -
>> - return ret;
>> + return cppc_set_reg(cpu, AUTO_SEL_ENABLE, enable);
>> }
>> EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
>>
>> @@ -1644,38 +1628,7 @@ EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
>> */
>> int cppc_set_enable(int cpu, bool enable)
>> {
>> - int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>> - struct cpc_register_resource *enable_reg;
>> - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> - struct cppc_pcc_data *pcc_ss_data = NULL;
>> - int ret = -EINVAL;
>> -
>> - if (!cpc_desc) {
>> - pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> - return -EINVAL;
>> - }
>> -
>> - enable_reg = &cpc_desc->cpc_regs[ENABLE];
>> -
>> - if (CPC_IN_PCC(enable_reg)) {
>> -
>> - if (pcc_ss_id < 0)
>> - return -EIO;
>> -
>> - ret = cpc_write(cpu, enable_reg, enable);
>> - 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 platfrom */
>> - ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>> - up_write(&pcc_ss_data->pcc_lock);
>> - return ret;
>> - }
>> -
>> - return cpc_write(cpu, enable_reg, enable);
>> + return cppc_set_reg(cpu, ENABLE, enable);
>> }
>> EXPORT_SYMBOL_GPL(cppc_set_enable);
>>
>> --
>
© 2016 - 2026 Red Hat, Inc.