| Message ID | CAKohpokanT+6scDTYotp90GNhyatURQNyjKpkfDKxgU9vhbWqQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
On Friday, March 20, 2015 12:18:49 PM Saravana Kannan wrote: > 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. Viresh, have you ever sent an update of this one?
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 --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);