On 20 March 2015 at 06:31, Saravana Kannan skannan@codeaurora.org wrote:
On 02/19/2015 03:32 AM, Viresh Kumar wrote:
+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
I don't hate it that much, and neither does other parts of kernel :)
think a do while would work here and also be more compact and readable.
So you want this:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index d3f9ce3b94d3..ecbd8c2118c2 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -47,7 +47,7 @@ static inline bool policy_is_inactive(struct cpufreq_policy *policy) static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy, bool active) { - while (1) { + do { if (likely(policy)) policy = list_next_entry(policy, policy_list); else @@ -69,9 +69,9 @@ static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy, * 1 0 policy * 1 1 next */ - if (active ^ policy_is_inactive(policy)) - return policy; - }; + } while (!(active ^ policy_is_inactive(policy))); + + return policy; }
Not sure which one looked better :)
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, because we want that for-loop to iterate over active/inactive policies only, and we need to run this routine to find it..
For ex: - We want to iterate over active policies only - The first policy of the list is inactive - The change you are suggesting will break things here..
/* 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.
No. What we are returning here isn't a real policy actually but an container-of of the HEAD of the list, so it only has a valid ->policy_list value, others might give us a crash. See how list_next_entry() works :)