From patchwork Fri Mar 20 04:41:48 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 6054331 Return-Path: X-Original-To: patchwork-linux-pm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 67F38BF90F for ; Fri, 20 Mar 2015 04:41:52 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 792F0204D1 for ; Fri, 20 Mar 2015 04:41:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E04812026F for ; Fri, 20 Mar 2015 04:41:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751199AbbCTElt (ORCPT ); Fri, 20 Mar 2015 00:41:49 -0400 Received: from mail-ob0-f169.google.com ([209.85.214.169]:34628 "EHLO mail-ob0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750889AbbCTEls (ORCPT ); Fri, 20 Mar 2015 00:41:48 -0400 Received: by obbgg8 with SMTP id gg8so70328551obb.1 for ; Thu, 19 Mar 2015 21:41:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=6haNOVloBDViVqOjEwJjd9Jzw0RBL99jqKVHnkSVtYc=; b=S1Yxaxvnw7YLbsqK+/ShmNhiJvwK8ZFov5kOVHxeCVzt5EZqa2JQ9dFLmxNgXb+S42 /k0xLAMcoX+fEJb1HlPWZ+rCOsFsg7ETzzGN3V3vPrAABIynafF9GydMLGfYABdArk07 Zn54uWQkvyTJX6uoxM3NP5NXJ6hC/Sa5ADkKug0lq/ckWpby2O42sifQs9risObEq1FP 8mVa1wrywd0TGQ4LrpnGuCPntCuziV3pJtzVjL1Ied7OgbaZZvMR7611D7ZJa3OGrCAz 9aWGnlFeP6Ihqf+rI7xSp3DUkZvK3E1aZgbSzP1MEiuMoSit3TWmXDtQur6Eh7rU32zM H2jw== X-Gm-Message-State: ALoCoQkK9Am0Kb1lAwaeAYaBRqEmGCLlEtbrF86xcjfEq/kvqbxlsmZ+VAwa9C/pCZhgQndKrM+5 MIME-Version: 1.0 X-Received: by 10.60.134.17 with SMTP id pg17mr56163075oeb.12.1426826508325; Thu, 19 Mar 2015 21:41:48 -0700 (PDT) Received: by 10.182.250.51 with HTTP; Thu, 19 Mar 2015 21:41:48 -0700 (PDT) In-Reply-To: <550B7159.7090601@codeaurora.org> References: <3335dcc924b60249617dfad711c602927fb1f2b7.1424345053.git.viresh.kumar@linaro.org> <550B7159.7090601@codeaurora.org> Date: Fri, 20 Mar 2015 10:11:48 +0530 Message-ID: Subject: Re: [PATCH V2 06/20] cpufreq: Create for_each_{in}active_policy() From: Viresh Kumar To: Saravana Kannan Cc: Rafael Wysocki , Linaro Kernel Mailman List , "linux-pm@vger.kernel.org" , Stephen Boyd , Prarit Bhargava Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 20 March 2015 at 06:31, Saravana Kannan 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 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);