The current cpufreq_set_policy() takes three arguments, including
new_pol. However, new_pol is only meaningful when a driver provides
the ->setpolicy callback. Most cpufreq drivers (e.g. ACPI) do not
implement this callback, so the extra parameter is unused.

Passing new_pol in such cases is unnecessary and may mislead readers
into thinking it has a functional effect, while in reality it is only
relevant for a very limited set of drivers.

Simplify the interface by removing the redundant argument and letting
drivers that implement ->setpolicy rely directly on
policy->policy. This reduces parameter passing overhead and makes the
API clearer.

Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 drivers/cpufreq/cpufreq.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index a067b5447fe8..ea4a5d3f786c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -85,8 +85,7 @@ static int cpufreq_init_governor(struct cpufreq_policy *policy);
 static void cpufreq_exit_governor(struct cpufreq_policy *policy);
 static void cpufreq_governor_limits(struct cpufreq_policy *policy);
 static int cpufreq_set_policy(struct cpufreq_policy *policy,
-			      struct cpufreq_governor *new_gov,
-			      unsigned int new_pol);
+			      struct cpufreq_governor *new_gov);
 static bool cpufreq_boost_supported(void);
 static int cpufreq_boost_trigger_state(int state);
 
@@ -822,7 +821,8 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
 		if (!new_pol)
 			return -EINVAL;
 
-		ret = cpufreq_set_policy(policy, NULL, new_pol);
+		policy->policy = new_pol;
+		ret = cpufreq_set_policy(policy, NULL);
 	} else {
 		struct cpufreq_governor *new_gov;
 
@@ -830,8 +830,8 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
 		if (!new_gov)
 			return -EINVAL;
 
-		ret = cpufreq_set_policy(policy, new_gov,
-					 CPUFREQ_POLICY_UNKNOWN);
+		policy->policy = CPUFREQ_POLICY_UNKNOWN;
+		ret = cpufreq_set_policy(policy, new_gov);
 
 		module_put(new_gov->owner);
 	}
@@ -1154,7 +1154,8 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
 			return -ENODATA;
 	}
 
-	ret = cpufreq_set_policy(policy, gov, pol);
+	policy->policy = pol;
+	ret = cpufreq_set_policy(policy, gov);
 	if (gov)
 		module_put(gov->owner);
 
@@ -1190,7 +1191,7 @@ void refresh_frequency_limits(struct cpufreq_policy *policy)
 	if (!policy_is_inactive(policy)) {
 		pr_debug("updating policy for CPU %u\n", policy->cpu);
 
-		cpufreq_set_policy(policy, policy->governor, policy->policy);
+		cpufreq_set_policy(policy, policy->governor);
 	}
 }
 EXPORT_SYMBOL(refresh_frequency_limits);
@@ -2610,7 +2611,6 @@ static void cpufreq_update_pressure(struct cpufreq_policy *policy)
  * cpufreq_set_policy - Modify cpufreq policy parameters.
  * @policy: Policy object to modify.
  * @new_gov: Policy governor pointer.
- * @new_pol: Policy value (for drivers with built-in governors).
  *
  * Invoke the cpufreq driver's ->verify() callback to sanity-check the frequency
  * limits to be set for the policy, update @policy with the verified limits
@@ -2622,8 +2622,7 @@ static void cpufreq_update_pressure(struct cpufreq_policy *policy)
  * The cpuinfo part of @policy is not updated by this function.
  */
 static int cpufreq_set_policy(struct cpufreq_policy *policy,
-			      struct cpufreq_governor *new_gov,
-			      unsigned int new_pol)
+			      struct cpufreq_governor *new_gov)
 {
 	struct cpufreq_policy_data new_data;
 	struct cpufreq_governor *old_gov;
@@ -2676,7 +2675,6 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 		 policy->min, policy->max);
 
 	if (cpufreq_driver->setpolicy) {
-		policy->policy = new_pol;
 		pr_debug("setting range\n");
 		return cpufreq_driver->setpolicy(policy);
 	}
-- 
2.25.1

On 19-08-25, 18:39, Zihuan Zhang wrote:
>  static int cpufreq_set_policy(struct cpufreq_policy *policy,
> -			      struct cpufreq_governor *new_gov,
> -			      unsigned int new_pol);
> +			      struct cpufreq_governor *new_gov);

A driver will either support the policy or the governor. If we are
keeping `new_gov` around, I don't see why `new_pol` should be dropped.
And changing the policy for a `setpolicy` driver should happen from
within cpufreq_set_policy() instead of the caller. Also there is at
least one case (verify()) where we may end up returning early, before
changing the policy.

-- 
viresh

在 2025/8/19 18:59, Viresh Kumar 写道:
> On 19-08-25, 18:39, Zihuan Zhang wrote:
>>   static int cpufreq_set_policy(struct cpufreq_policy *policy,
>> -			      struct cpufreq_governor *new_gov,
>> -			      unsigned int new_pol);
>> +			      struct cpufreq_governor *new_gov);
> A driver will either support the policy or the governor. If we are
> keeping `new_gov` around, I don't see why `new_pol` should be dropped.

Thanks for the reminder.

If we remove new_pol, then new_gov should indeed be removed as well.

> And changing the policy for a `setpolicy` driver should happen from
> within cpufreq_set_policy() instead of the caller. Also there is at
> least one case (verify()) where we may end up returning early, before
> changing the policy.
>
You’re right, we didn’t really consider that case before.

The interface of cpufreq_set_policy() does look a bit odd:

- drivers using governors don’t really need the new_pol parameter

- while drivers using the setpolicy method don’t need the new_gov one.


I guess this might be due to some historical reasons.

The question is whether it’s worth modifying this function, or if we 
should just keep it as it is.

On 20-08-25, 13:42, Zihuan Zhang wrote:
> You’re right, we didn’t really consider that case before.
> 
> The interface of cpufreq_set_policy() does look a bit odd:
> 
> - drivers using governors don’t really need the new_pol parameter
> 
> - while drivers using the setpolicy method don’t need the new_gov one.

Right, one argument is _always_ unused. If we could have handled that via a
single argument, it would have been nice.

> I guess this might be due to some historical reasons.
> 
> The question is whether it’s worth modifying this function, or if we should
> just keep it as it is.

Unless there is a better way :)

-- 
viresh

在 2025/8/20 13:47, Viresh Kumar 写道:
> On 20-08-25, 13:42, Zihuan Zhang wrote:
>> You’re right, we didn’t really consider that case before.
>>
>> The interface of cpufreq_set_policy() does look a bit odd:
>>
>> - drivers using governors don’t really need the new_pol parameter
>>
>> - while drivers using the setpolicy method don’t need the new_gov one.
> Right, one argument is _always_ unused. If we could have handled that via a
> single argument, it would have been nice.
Sounds great.
>> I guess this might be due to some historical reasons.
>>
>> The question is whether it’s worth modifying this function, or if we should
>> just keep it as it is.
> Unless there is a better way :)

If we change cpufreq_set_policy() to only take one parameter, then we 
would need to add extra fields into struct cpufreq_policy to hold 
temporary values,

in order to avoid the issue where verify may return early and leave the 
policy in a partially updated state.


So maybe it is better not to modify it.
>