On 02/19/2015 03:32 AM, Viresh Kumar wrote:
policy->cpus is cleared unconditionally now on hotplug-out of a CPU and it can be checked to know if a policy is active or inactive. Create helper routines to iterate over all active/inactive policies, based on policy->cpus field.
Replace all instances of for_each_policy() with for_each_active_policy() to make them iterate only for active policies. (We haven't made changes yet to keep inactive policies in the same list, but that will be followed in a later patch).
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq.c | 81 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 72 insertions(+), 9 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 6ed87d02d293..d3f9ce3b94d3 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -32,11 +32,74 @@ #include <trace/events/power.h>
/* Macros to iterate over lists */ -/* Iterate over online CPUs policies */ static LIST_HEAD(cpufreq_policy_list); +static inline bool policy_is_inactive(struct cpufreq_policy *policy) +{
- return cpumask_empty(policy->cpus);
+}
+/*
- Finds Next Acive/Inactive policy
- policy: Previous policy.
- active: Looking for active policy (true) or Inactive policy (false).
- */
+static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
bool active)+{
- while (1) {
I don't like while(1) unless it's really meant to be an infinite loop. I think a do while would work here and also be more compact and readable.
if (likely(policy))policy = list_next_entry(policy, policy_list);elsepolicy = list_first_entry(&cpufreq_policy_list,typeof(*policy), policy_list);
Can't you just move this part into expr1? That would make it much clear/easier to understand
/* No more policies */if (&policy->policy_list == &cpufreq_policy_list)return policy;
I'm kinda confused by the fact that you return policy here unconditionally. I think it's a bug. No? Eg: Is there's only one policy in the system and you are looking for an inactive policy. Wouldn't this return the only policy -- the one that's active.
/** Table to explains logic behind following expression:** Active policy_is_inactive Result* (policy or next)** 0 0 next* 0 1 policy* 1 0 policy* 1 1 next*/if (active ^ policy_is_inactive(policy))return policy;- };
+}
+/*
- Iterate over online CPUs policies
- Explanation:
- expr1: marks __temp NULL and gets the first __active policy.
- expr2: checks if list is finished, if yes then it sets __policy to the last
__active policy and returns 0 to end the loop.
- expr3: preserves last __active policy and gets the next one.
- */
+#define __for_each_active_policy(__policy, __temp, __active) \
- for (__temp = NULL, __policy = next_policy(NULL, __active); \
&__policy->policy_list != &cpufreq_policy_list || \((__policy = __temp) && 0); \__temp = __policy, __policy = next_policy(__policy, __active))- #define for_each_policy(__policy) \ list_for_each_entry(__policy, &cpufreq_policy_list, policy_list)
+/*
- Routines to iterate over active or inactive policies
- __policy: next active/inactive policy will be returned in this.
- __temp: for internal purpose, not to be used by caller.
- */
+#define for_each_active_policy(__policy, __temp) __for_each_active_policy(__policy, __temp, true) +#define for_each_inactive_policy(__policy, __temp) __for_each_active_policy(__policy, __temp, false)
- /* Iterate over governors */ static LIST_HEAD(cpufreq_governor_list); #define for_each_governor(__governor) \
Stuff below this looks fine.
@@ -1142,7 +1205,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) { unsigned int j, cpu = dev->id; int ret = -ENOMEM;
- struct cpufreq_policy *policy;
- struct cpufreq_policy *policy, *tpolicy; unsigned long flags; bool recover_policy = cpufreq_suspended;
<snip>
-Saravana