From patchwork Thu Mar 28 10:13:54 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rajagopal Venkat X-Patchwork-Id: 2355921 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork2.kernel.org (Postfix) with ESMTP id 74695DF2A1 for ; Thu, 28 Mar 2013 10:16:47 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1UL9qS-0000lC-Ko; Thu, 28 Mar 2013 10:14:08 +0000 Received: from mail-ob0-x22a.google.com ([2607:f8b0:4003:c01::22a]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1UL9qG-0000jt-58 for linux-arm-kernel@lists.infradead.org; Thu, 28 Mar 2013 10:13:58 +0000 Received: by mail-ob0-f170.google.com with SMTP id wc20so9113704obb.15 for ; Thu, 28 Mar 2013 03:13:54 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type:x-gm-message-state; bh=BCjXSVYJxYBv0mjQ2y56up/lRDHRPjuMFoc9hjgCWdA=; b=ibmRpKi2+y7tzBOqRbvkTFdDbYE7i6BqY1EhhYF40Gf0L8kGIc46s1N09hubu64ave 5mcDqjoPffueW3kpialdEGS08akS2lG58yZWhLMvOWnW4bT5Jtii7vFqeCIq/1Trs77J UxOiiZivwTjxABwsTGjDt5JOXQ9cUMPI/2qKQsaYTPJlFOaAnW4Pv7crJSSGgjyTlfkL Wp6QTGGivELAXDBWETtZpkp42rb6mMgvpIin6rwBC8iiFpNLu5hAosredu/xfBhzy2RC TuvRIojGhtBrocIYNF/hOXD748ciFeI1MefsxrdLMvNPLfYGkNU7P4buEormF9BC5QaM PfNA== MIME-Version: 1.0 X-Received: by 10.60.124.196 with SMTP id mk4mr12617187oeb.118.1364465634683; Thu, 28 Mar 2013 03:13:54 -0700 (PDT) Received: by 10.76.10.41 with HTTP; Thu, 28 Mar 2013 03:13:54 -0700 (PDT) In-Reply-To: <1364377825-6171-3-git-send-email-ulf.hansson@stericsson.com> References: <1364377825-6171-1-git-send-email-ulf.hansson@stericsson.com> <1364377825-6171-3-git-send-email-ulf.hansson@stericsson.com> Date: Thu, 28 Mar 2013 15:43:54 +0530 Message-ID: Subject: Re: [PATCH V3 2/3] clk: Fixup errorhandling for clk_set_parent From: Rajagopal Venkat To: Ulf Hansson X-Gm-Message-State: ALoCoQm27fV0MvjZvJep3Kca7ghJ2vqtHt8EFhH7PsFqs9O+0j3+ixeB9gbHGLfVDBIZwNuunLLE X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130328_061356_355066_1A237DD4 X-CRM114-Status: GOOD ( 25.55 ) X-Spam-Score: -1.9 (-) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-1.9 points) pts rule name description ---- ---------------------- -------------------------------------------------- -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Linus Walleij , Mike Turquette , Ulf Hansson , linux-arm-kernel@lists.infradead.org, Par-Olof Hakansson X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org On 27 March 2013 15:20, Ulf Hansson wrote: > From: Ulf Hansson > > Fixup the broken feature of allowing reparent of a clk to the > orhpan list and vice verse. When operating on a single-parent > clk, the .set_parent callback for the clk hw is optional to > implement, but for a multi-parent clk it is mandatory. > > Moreover improve the errorhandling by verifying the prerequisites > before triggering clk notifiers. This will prevent unnecessary > rollback with ABORT_RATE_CHANGE. > > Signed-off-by: Ulf Hansson > Cc: Rajagopal Venkat > --- > drivers/clk/clk.c | 56 ++++++++++++++++++++++++++++++++++------------------- > 1 file changed, 36 insertions(+), 20 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 97d681b..6fa301b 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1269,15 +1269,10 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent) > __clk_recalc_rates(clk, POST_RATE_CHANGE); > } > > -static int __clk_set_parent(struct clk *clk, struct clk *parent) > +static u8 clk_fetch_parent_index(struct clk *clk, struct clk *parent) > { > - struct clk *old_parent; > - unsigned long flags; > - int ret = -EINVAL; > u8 i; > > - old_parent = clk->parent; > - > if (!clk->parents) > clk->parents = kzalloc((sizeof(struct clk*) * clk->num_parents), > GFP_KERNEL); > @@ -1297,11 +1292,14 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent) > } > } > > - if (i == clk->num_parents) { > - pr_debug("%s: clock %s is not a possible parent of clock %s\n", > - __func__, parent->name, clk->name); > - goto out; > - } > + return i; > +} > + > +static int __clk_set_parent(struct clk *clk, struct clk *parent, u8 p_index) > +{ > + unsigned long flags; > + int ret = 0; > + struct clk *old_parent = clk->parent; > > /* migrate prepare and enable */ > if (clk->prepare_count) > @@ -1314,7 +1312,8 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent) > spin_unlock_irqrestore(&enable_lock, flags); > > /* change clock input source */ > - ret = clk->ops->set_parent(clk->hw, i); > + if (parent && clk->ops->set_parent) > + ret = clk->ops->set_parent(clk->hw, p_index); What if clk->ops->set_parent() fails? should this clk undefined state be handled? > > /* clean up old prepare and enable */ > spin_lock_irqsave(&enable_lock, flags); > @@ -1325,7 +1324,6 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent) > if (clk->prepare_count) > __clk_unprepare(old_parent); > > -out: > return ret; > } > > @@ -1344,11 +1342,14 @@ out: > int clk_set_parent(struct clk *clk, struct clk *parent) > { > int ret = 0; > + u8 p_index = 0; > + unsigned long p_rate = 0; > > if (!clk || !clk->ops) > return -EINVAL; > > - if (!clk->ops->set_parent) > + /* verify ops for for multi-parent clks */ > + if ((clk->num_parents > 1) && (!clk->ops->set_parent)) > return -ENOSYS; Should this check be moved to clk registration path? clk ops validation is done in __clk_init(). > > /* prevent racing with updates to the clock topology */ > @@ -1357,19 +1358,34 @@ int clk_set_parent(struct clk *clk, struct clk *parent) > if (clk->parent == parent) > goto out; > > + /* check that we are allowed to re-parent if the clock is in use */ > + if ((clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count) { > + ret = -EBUSY; > + goto out; > + } > + > + /* try finding the new parent index */ > + if (parent) { > + p_index = clk_fetch_parent_index(clk, parent); > + p_rate = parent->rate; > + if (p_index == clk->num_parents) { > + pr_debug("%s: clk %s can not be parent of clk %s\n", > + __func__, parent->name, clk->name); > + ret = -EINVAL; > + goto out; > + } > + } > + > /* propagate PRE_RATE_CHANGE notifications */ > if (clk->notifier_count) > - ret = __clk_speculate_rates(clk, parent->rate); > + ret = __clk_speculate_rates(clk, p_rate); > > /* abort if a driver objects */ > if (ret == NOTIFY_STOP) > goto out; > > - /* only re-parent if the clock is not in use */ > - if ((clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count) > - ret = -EBUSY; > - else > - ret = __clk_set_parent(clk, parent); > + /* do the re-parent */ > + ret = __clk_set_parent(clk, parent, p_index); > > /* propagate ABORT_RATE_CHANGE if .set_parent failed */ > if (ret) { > -- > 1.7.10 > In addition, this fix is required to allow reparenting of clk to orphan list. --- drivers/clk/clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) hlist_del(&clk->child_node); diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index fb5d08a..ec436e0 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1221,7 +1221,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent) struct dentry *new_parent_d; #endif - if (!clk || !new_parent) + if (!clk) return;