From patchwork Fri Jul 29 08:47:43 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jean Pihet X-Patchwork-Id: 1019272 Received: from smtp1.linux-foundation.org (smtp1.linux-foundation.org [140.211.169.13]) by demeter1.kernel.org (8.14.4/8.14.4) with ESMTP id p6T8nDso002303 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL) for ; Fri, 29 Jul 2011 08:49:33 GMT Received: from daredevil.linux-foundation.org (localhost [127.0.0.1]) by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with ESMTP id p6T8lmvv029873; Fri, 29 Jul 2011 01:47:48 -0700 Received: from mail-qw0-f47.google.com (mail-qw0-f47.google.com [209.85.216.47]) by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with ESMTP id p6T8liWP029860 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=FAIL) for ; Fri, 29 Jul 2011 01:47:45 -0700 Received: by qwh5 with SMTP id 5so1873785qwh.6 for ; Fri, 29 Jul 2011 01:47:43 -0700 (PDT) MIME-Version: 1.0 Received: by 10.229.118.72 with SMTP id u8mr775647qcq.1.1311929263569; Fri, 29 Jul 2011 01:47:43 -0700 (PDT) Received: by 10.229.84.19 with HTTP; Fri, 29 Jul 2011 01:47:43 -0700 (PDT) X-Originating-IP: [87.66.66.156] In-Reply-To: <20110729075942.GA26959@google.com> References: <1311841821-10252-1-git-send-email-j-pihet@ti.com> <1311841821-10252-9-git-send-email-j-pihet@ti.com> <20110729075942.GA26959@google.com> Date: Fri, 29 Jul 2011 10:47:43 +0200 Message-ID: From: Jean Pihet To: Todd Poynor Received-SPF: pass (localhost is always allowed.) X-Spam-Status: No, hits=-1.912 required=5 tests=AWL, BAYES_00, FRT_TODAY2, OSDL_HEADER_SUBJECT_BRACKETED X-Spam-Checker-Version: SpamAssassin 3.2.4-osdl_revision__1.47__ X-MIMEDefang-Filter: lf$Revision: 1.188 $ X-Scanned-By: MIMEDefang 2.63 on 140.211.169.21 Cc: markgross@thegnar.org, broonie@opensource.wolfsonmicro.com, Linux PM mailing list , linux-omap@vger.kernel.org, Jean Pihet Subject: Re: [linux-pm] [PATCH 08/13] OMAP2+: powerdomain: control power domains next state X-BeenThere: linux-pm@lists.linux-foundation.org X-Mailman-Version: 2.1.9 Precedence: list List-Id: Linux power management List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-pm-bounces@lists.linux-foundation.org Errors-To: linux-pm-bounces@lists.linux-foundation.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Fri, 29 Jul 2011 08:49:33 +0000 (UTC) X-MIME-Autoconverted: from quoted-printable to 8bit by demeter1.kernel.org id p6T8nDso002303 Todd, On Fri, Jul 29, 2011 at 9:59 AM, Todd Poynor wrote: > On Thu, Jul 28, 2011 at 10:30:15AM +0200, jean.pihet@newoldbits.com wrote: > ... >> +int pwrdm_set_wkup_lat_constraint(struct powerdomain *pwrdm, void *cookie, >> +                               long min_latency) >> +{ >> +     struct pwrdm_wkup_constraints_entry *user = NULL; >> +     struct pwrdm_wkup_constraints_entry *tmp_user, *new_user; >> +     int ret = 0, free_new_user = 0, free_node = 0; >> +     long value = 0; >> +     unsigned long flags; >> + >> +     pr_debug("powerdomain: %s: pwrdm %s, cookie=0x%p, min_latency=%ld\n", >> +              __func__, pwrdm->name, cookie, min_latency); >> + >> +     new_user = kzalloc(sizeof(struct pwrdm_wkup_constraints_entry), >> +                        GFP_KERNEL); >> +     if (!new_user) { >> +             pr_err("%s: FATAL ERROR: kzalloc failed\n", __func__); >> +             return -ENOMEM; >> +     } >> + >> +     spin_lock_irqsave(&pwrdm->wkup_lat_plist_lock, flags); >> + >> +     /* Check if there already is a constraint for cookie */ >> +     plist_for_each_entry(tmp_user, &pwrdm->wkup_lat_plist_head, node) { >> +             if (tmp_user->cookie == cookie) { >> +                     user = tmp_user; >> +                     free_new_user = 1; >> +                     break; >> +             } >> +     } >> + >> +     if (min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE) { >> +             /* If nothing to update, job done */ >> +             if (user && (user->node.prio == min_latency)) >> +                     goto exit_ok; >> + >> +             if (!user) { >> +                     /* Add new entry to the list */ >> +                     user = new_user; >> +                     user->cookie = cookie; >> +             } else { >> +                     /* Update existing entry */ >> +                     plist_del(&user->node, &pwrdm->wkup_lat_plist_head); >> +             } >> + >> +             plist_node_init(&user->node, min_latency); >> +             plist_add(&user->node, &pwrdm->wkup_lat_plist_head); >> +     } else { >> +             /* Remove the constraint from the list */ >> +             if (!user) { >> +                     pr_err("%s: Error: no prior constraint to release\n", >> +                            __func__); >> +                     ret = -EINVAL; >> +                     goto exit_error; >> +             } >> + >> +             plist_del(&user->node, &pwrdm->wkup_lat_plist_head); >> +             free_node = 1; > > All min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE paths need > free_new_user = 1. free_new_user = 1 is only needed if no existing constraint has been found, i.e. user stays at NULL. This is implemented in the check for an existing constraint (plist_for_each_entry(...)). >(Or maybe change the logic to check user != > new_user and free new_user if so.) That could be done as well. > >> +     } >> + >> +exit_ok: >> +     /* Find the strongest constraint from the list */ >> +     if (!plist_head_empty(&pwrdm->wkup_lat_plist_head)) >> +             value = plist_first(&pwrdm->wkup_lat_plist_head)->prio; >> + >> +     spin_unlock_irqrestore(&pwrdm->wkup_lat_plist_lock, flags); >> + >> +     if (free_node) >> +             kfree(user); >> + >> +     if (free_new_user) >> +             kfree(new_user); >> + >> +     /* Apply the constraint to the pwrdm */ >> +     pr_debug("powerdomain: %s: pwrdm %s, value=%ld\n", >> +              __func__, pwrdm->name, value); >> +     pwrdm_wakeuplat_update_pwrst(pwrdm, value); >> + >> +     return 0; >> + >> +exit_error: >> +     spin_unlock_irqrestore(&pwrdm->wkup_lat_plist_lock, flags); > > Need: >        kfree(new_user); Correct! BTW I have a new version (patch here below) that improves the cases with latency = PM_QOS_DEV_LAT_DEFAULT_VALUE. This will come in v4 after testing. > >> +     return ret; >> +} > > > Todd > Thanks for reviewing! Regards, Jean --- Patch for cases with latency = PM_QOS_DEV_LAT_DEFAULT_VALUE: * Programs a new target state if it is different from current power state. @@ -232,7 +232,7 @@ static int pwrdm_wakeuplat_update_pwrst(struct powerdomain *pwrdm, /* Find power state with wakeup latency < minimum constraint */ for (new_state = 0x0; new_state < PWRDM_MAX_PWRSTS; new_state++) { - if (min_latency == 0 || + if (min_latency == PM_QOS_DEV_LAT_DEFAULT_VALUE || pwrdm->wakeup_lat[new_state] <= min_latency) break; } @@ -1018,8 +1018,8 @@ int pwrdm_post_transition(void) * @pwrdm: struct powerdomain * which the constraint applies to * @cookie: constraint identifier, used for tracking. * @min_latency: minimum wakeup latency constraint (in microseconds) for - * the given pwrdm. The value of PM_QOS_DEV_WAKEUP_LAT_DEFAULT_VALUE - * removes the constraint. + * the given pwrdm. The value of PM_QOS_DEV_LAT_DEFAULT_VALUE removes + * the constraint. * * Tracks the constraints by @cookie. * Constraint set/update: Adds a new entry to powerdomain's wake-up latency @@ -1042,7 +1042,7 @@ int pwrdm_set_wkup_lat_constraint(struct powerdomain *pwrdm, void *cookie, struct pwrdm_wkup_constraints_entry *user = NULL; struct pwrdm_wkup_constraints_entry *tmp_user, *new_user; int ret = 0, free_new_user = 0, free_node = 0; - long value = 0; + long value = PM_QOS_DEV_LAT_DEFAULT_VALUE; unsigned long flags; pr_debug("powerdomain: %s: pwrdm %s, cookie=0x%p, min_latency=%ld\n", @@ -1083,16 +1083,17 @@ int pwrdm_set_wkup_lat_constraint(struct powerdomain *pwrdm, void *cookie, plist_node_init(&user->node, min_latency); plist_add(&user->node, &pwrdm->wkup_lat_plist_head); } else { - /* Remove the constraint from the list */ - if (!user) { - pr_err("%s: Error: no prior constraint to release\n", - __func__); + if (user) { + /* Remove the constraint from the list */ + plist_del(&user->node, &pwrdm->wkup_lat_plist_head); + free_node = 1; + } else { + /* Constraint not existing or list empty, do nothing */ + free_new_user = 1; ret = -EINVAL; goto exit_error; } - plist_del(&user->node, &pwrdm->wkup_lat_plist_head); - free_node = 1; } exit_ok: @@ -1117,6 +1118,10 @@ exit_ok: exit_error: spin_unlock_irqrestore(&pwrdm->wkup_lat_plist_lock, flags); + + if (free_new_user) + kfree(new_user); + return ret; } diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c index c0f48ab..2e9379b 100644 --- a/arch/arm/mach-omap2/powerdomain.c +++ b/arch/arm/mach-omap2/powerdomain.c @@ -206,7 +206,7 @@ static int _pwrdm_post_transition_cb(struct powerdomain *pwrdm, void *unused) * pwrdm_wakeuplat_update_pwrst - Update power domain power state if needed * @pwrdm: struct powerdomain * to which requesting device belongs to. * @min_latency: the allowed wake-up latency for the given power domain. A - * value of 0 means 'no constraint' on the pwrdm. + * value of PM_QOS_DEV_LAT_DEFAULT_VALUE means 'no constraint' on the pwrdm. * * Finds the power domain next power state that fulfills the constraint.