diff mbox

[V2,06/20] cpufreq: Create for_each_{in}active_policy()

Message ID CAKohpokanT+6scDTYotp90GNhyatURQNyjKpkfDKxgU9vhbWqQ@mail.gmail.com
State New
Headers show

Commit Message

Viresh Kumar March 20, 2015, 4:41 a.m. UTC 
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:

                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);
>> +               else
>> +                       policy = 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 :)
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Viresh Kumar May 8, 2015, 2:33 a.m. UTC | #1 
On 8 May 2015 at 03:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Viresh, have you ever sent an update of this one?

I haven't sent updates for any patches, was waiting for reviews
to finish. But will send the new copies now.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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