From patchwork Wed Sep 16 00:39:31 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Stephen Boyd X-Patchwork-Id: 7190031 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 76D7BBEEC1 for ; Wed, 16 Sep 2015 00:39:55 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 62D6E20821 for ; Wed, 16 Sep 2015 00:39:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C287D20846 for ; Wed, 16 Sep 2015 00:39:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753226AbbIPAjv (ORCPT ); Tue, 15 Sep 2015 20:39:51 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:44391 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752639AbbIPAje (ORCPT ); Tue, 15 Sep 2015 20:39:34 -0400 Received: from smtp.codeaurora.org (localhost [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id 336E913FEF5; Wed, 16 Sep 2015 00:39:34 +0000 (UTC) Received: by smtp.codeaurora.org (Postfix, from userid 486) id 2299813FEF7; Wed, 16 Sep 2015 00:39:34 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Level: 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 Received: from localhost (i-global254.qualcomm.com [199.106.103.254]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) (Authenticated sender: sboyd@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id C318413FEF5; Wed, 16 Sep 2015 00:39:32 +0000 (UTC) Date: Tue, 15 Sep 2015 17:39:31 -0700 From: Stephen Boyd To: Heiko =?iso-8859-1?Q?St=FCbner?= Cc: mturquette@baylibre.com, linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, "Rafael J. Wysocki" , Pavel Machek , linux-pm@vger.kernel.org Subject: Re: [PATCH] clk: readd refcounting for struct clk instances Message-ID: <20150916003931.GK23081@codeaurora.org> References: <1878691.zhVxsBBBGg@diego> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1878691.zhVxsBBBGg@diego> User-Agent: Mutt/1.5.21 (2010-09-15) X-Virus-Scanned: ClamAV using ClamSMTP Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 09/15, Heiko Stübner wrote: > With the split into struct clk and struct clk_core, clocks lost the > ability for nested __clk_get clkdev calls. While it stays possible to > call __clk_get, the first call to (__)clk_put will clear the struct clk, > making subsequent clk_put calls run into a NULL pointer dereference. > > One prime example of this sits in the generic power domain code, where it > is possible to add the clocks both by name and by passing in a struct clk > via pm_clk_add_clk(). __pm_clk_add() in turn then calls __clk_get to > increase the refcount, so that the original code can put the clock again. > > A possible call-path looks like > clk = of_clk_get(); > pm_clk_add_clk(dev, clk); > clk_put(clk); > > with pm_clk_add_clk() => __pm_clk_add() then calling __clk_get on the clk > and later clk_put when the pm clock list gets destroyed, thus creating > a NULL pointer deref, as the struct clk doesn't exist anymore. > > So add a separate refcounting for struct clk instances and only clean up > once the refcount reaches zero. > > Signed-off-by: Heiko Stuebner > --- > I stumbled upon this while applying the new Rockchip power-domain driver, > but I guess the underlying issue is universal and probably present since > the original clk/clk_core split, so this probably counts as fix. Ok. The fix makes sense, but I wonder why we do this. Perhaps we should stop exporting __clk_get() and __clk_put() to everything that uses clkdev in the kernel. They're called per-user clks for a reason. There's one user. Now we have two users of the same struct clk instance, so we have to refcount it too? I hope the shared clk instance isn't being used to set rates in two pieces of code. And this only affects pm_clk_add_clk() callers right? So the only caller in the kernel (drivers/clk/shmobile/clk-mstp.c) doesn't seem to have this problem right now because it never calls clk_put() on the pointer it passes to pm_clk_add_clk(). So what if we did the patch below? This would rely on callers not calling clk_put() before calling pm_clk_remove() or pm_clk_destroy(), and the life cycle would be clear, but the sharing is still there. Or we could say that after a clk is given to pm_clk_add_clk() that the caller shouldn't touch it anymore, like shmobile is doing right now. Then there's nothing to do besides remove the extra __clk_get() call in the pm layer. > > @@ -2606,6 +2607,18 @@ static void __clk_release(struct kref *ref) > kfree(core); > } > > +static void __clk_release(struct kref *ref) > +{ > + struct clk *clk = container_of(ref, struct clk, ref); > + > + hlist_del(&clk->clks_node); > + if ((clk->min_rate > clk->core->req_rate || > + clk->max_rate < clk->core->req_rate)) Why did we grow a pair of parenthesis? > + clk_core_set_rate_nolock(clk->core, clk->core->req_rate); > + > + kfree(clk); > +} > + > /* > * Empty clk_ops for unregistered clocks. These are used temporarily > * after clk_unregister() was called on a clock and until last clock diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c index 652b5a367c1f..ef62bb3d7b26 100644 --- a/drivers/base/power/clock_ops.c +++ b/drivers/base/power/clock_ops.c @@ -31,6 +31,7 @@ struct pm_clock_entry { char *con_id; struct clk *clk; enum pce_status status; + bool needs_clk_put; }; /** @@ -59,8 +60,10 @@ static inline void __pm_clk_enable(struct device *dev, struct pm_clock_entry *ce */ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce) { - if (!ce->clk) + if (!ce->clk) { ce->clk = clk_get(dev, ce->con_id); + ce->needs_clk_put = true; + } if (IS_ERR(ce->clk)) { ce->status = PCE_STATUS_ERROR; } else { @@ -93,7 +96,7 @@ static int __pm_clk_add(struct device *dev, const char *con_id, return -ENOMEM; } } else { - if (IS_ERR(clk) || !__clk_get(clk)) { + if (IS_ERR(clk)) { kfree(ce); return -ENOENT; } @@ -149,7 +152,8 @@ static void __pm_clk_remove(struct pm_clock_entry *ce) if (ce->status >= PCE_STATUS_ACQUIRED) { clk_unprepare(ce->clk); - clk_put(ce->clk); + if (ce->needs_clk_put) + clk_put(ce->clk); } }