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

[Saravana Kannan <skannan@codeaurora.org>]

On 03/19/2015 09:41 PM, Viresh Kumar wrote:
> 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;
>   }

Yes please!! The other uses like inside a thread seem more reasonable to me.

>
> 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..

Ah, right. Makes sense.

>
>>> +
>>> +               /* 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 :)

I thought the last valid entry is what would point to the list head. Not 
the actual list head. I'll check again.

-Saravana


-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project