From patchwork Thu Jul 30 23:35:30 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mike Turquette X-Patchwork-Id: 51730 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-la0-f69.google.com (mail-la0-f69.google.com [209.85.215.69]) by patches.linaro.org (Postfix) with ESMTPS id 42E1122D9C for ; Thu, 30 Jul 2015 23:37:25 +0000 (UTC) Received: by labkf3 with SMTP id kf3sf528991lab.0 for ; Thu, 30 Jul 2015 16:37:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mime-version:to:from:in-reply-to :references:message-id:user-agent:subject:date:precedence:list-id :list-unsubscribe:list-archive:list-post:list-help:list-subscribe:cc :content-type:content-transfer-encoding:sender:errors-to :x-original-sender:x-original-authentication-results:mailing-list; bh=l2tXsIvTnn7uXn5lVPzn80RzvswS/32eR6K7yn0pq4c=; b=MrJY1M5df+CgnN5PfiHpcGHe9/WZO15T0NnOuvLpV2dP9W2jrPEqPa96jhzB3RCZGS W3Znd5xOJEVt8jekWXWVphevBo7vpaXXxYBlNJynEA/+YGlMvZ+g22asifP1GBDUhXN4 CnawDhKYAg4j3QRsOizVOF6rIYeiZz7KnrnjM1VMQXt/vh+vTIzCXUN/O6pygsRGhnSp edJjBFKPtAqpDuyNx+qn4vyRgTexe+GEeXSdqmrputkp+pcCbaIElq1eE3gCM5Z2VCHL RLDMttzT4LyNb3jhx/UggJ8cK3xnetvu8LaxFvpRYBObU7RwEcmv7v07hWF9lxsGz1bR vj4w== X-Gm-Message-State: ALoCoQnymlT2yo6WWU0X64E9YiPMoVjGNZ/BZf3UqEAv8KcDts4YNW7XUSa1BwdW4Y6C/f9j6fMs X-Received: by 10.112.215.67 with SMTP id og3mr18417615lbc.8.1438299443792; Thu, 30 Jul 2015 16:37:23 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.179.171 with SMTP id dh11ls204147lac.47.gmail; Thu, 30 Jul 2015 16:37:23 -0700 (PDT) X-Received: by 10.112.185.100 with SMTP id fb4mr47147822lbc.79.1438299443513; Thu, 30 Jul 2015 16:37:23 -0700 (PDT) Received: from mail-lb0-f171.google.com (mail-lb0-f171.google.com. [209.85.217.171]) by mx.google.com with ESMTPS id le8si2021260lbb.78.2015.07.30.16.37.23 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 30 Jul 2015 16:37:23 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.171 as permitted sender) client-ip=209.85.217.171; Received: by lbqc9 with SMTP id c9so11215599lbq.1 for ; Thu, 30 Jul 2015 16:37:23 -0700 (PDT) X-Received: by 10.152.5.228 with SMTP id v4mr46559081lav.36.1438299442909; Thu, 30 Jul 2015 16:37:22 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.7.198 with SMTP id l6csp32220lba; Thu, 30 Jul 2015 16:37:21 -0700 (PDT) X-Received: by 10.66.217.138 with SMTP id oy10mr582216pac.83.1438299440752; Thu, 30 Jul 2015 16:37:20 -0700 (PDT) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id m3si5720697pdg.13.2015.07.30.16.37.19 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 30 Jul 2015 16:37:20 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 2001:1868:205::9 as permitted sender) client-ip=2001:1868:205::9; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZKxMp-0003Lm-4b; Thu, 30 Jul 2015 23:36:03 +0000 Received: from mail-pa0-f53.google.com ([209.85.220.53]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZKxMm-0003JF-Ie for linux-arm-kernel@lists.infradead.org; Thu, 30 Jul 2015 23:36:01 +0000 Received: by padck2 with SMTP id ck2so30598477pad.0 for ; Thu, 30 Jul 2015 16:35:39 -0700 (PDT) X-Received: by 10.66.119.136 with SMTP id ku8mr109874922pab.26.1438299339428; Thu, 30 Jul 2015 16:35:39 -0700 (PDT) Received: from localhost (cpe-172-248-200-249.socal.res.rr.com. [172.248.200.249]) by smtp.gmail.com with ESMTPSA id nh3sm4046580pdb.72.2015.07.30.16.35.37 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 30 Jul 2015 16:35:38 -0700 (PDT) MIME-Version: 1.0 To: Lee Jones , From: Michael Turquette In-Reply-To: <20150730111747.GF14642@x1> References: <1437570255-21049-1-git-send-email-lee.jones@linaro.org> <1437570255-21049-4-git-send-email-lee.jones@linaro.org> <20150730010213.642.10831@quantum> <20150730111747.GF14642@x1> Message-ID: <20150730233530.23791.17746@quantum> User-Agent: alot/0.3.5 Subject: Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework Date: Thu, 30 Jul 2015 16:35:30 -0700 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150730_163600_676380_A62BF214 X-CRM114-Status: GOOD ( 36.65 ) X-Spam-Score: -2.6 (--) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-2.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.220.53 listed in list.dnswl.org] -0.0 RCVD_IN_MSPIKE_H2 RBL: Average reputation (+2) [209.85.220.53 listed in wl.mailspike.net] -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: , List-Help: , List-Subscribe: , Cc: devicetree@vger.kernel.org, kernel@stlinux.com, s.hauer@pengutronix.de, sboyd@codeaurora.org, linux-kernel@vger.kernel.org, geert@linux-m68k.org, maxime.ripard@free-electrons.com, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: mturquette@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.171 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 Quoting Lee Jones (2015-07-30 04:17:47) > On Wed, 29 Jul 2015, Michael Turquette wrote: > > > Hi Lee, > > > > + linux-clk ml > > > > Quoting Lee Jones (2015-07-22 06:04:13) > > > These new API calls will firstly provide a mechanisms to tag a clock as > > > critical and secondly allow any knowledgeable driver to (un)gate clocks, > > > even if they are marked as critical. > > > > > > Suggested-by: Maxime Ripard > > > Signed-off-by: Lee Jones > > > --- > > > drivers/clk/clk.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ > > > include/linux/clk-provider.h | 2 ++ > > > include/linux/clk.h | 30 +++++++++++++++++++++++++++++ > > > 3 files changed, 77 insertions(+) > > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > index 61c3fc5..486b1da 100644 > > > --- a/drivers/clk/clk.c > > > +++ b/drivers/clk/clk.c > > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name); > > > > > > /*** private data structures ***/ > > > > > > +/** > > > + * struct critical - Provides 'play' over critical clocks. A clock can be > > > + * marked as critical, meaning that it should not be > > > + * disabled. However, if a driver which is aware of the > > > + * critical behaviour wants to control it, it can do so > > > + * using clk_enable_critical() and clk_disable_critical(). > > > + * > > > + * @enabled Is clock critical? Once set, doesn't change > > > + * @leave_on Self explanatory. Can be disabled by knowledgeable drivers > > > > Not self explanatory. I need this explained to me. What does leave_on > > do? Better yet, what would happen if leave_on did not exist? > > > > > + */ > > > +struct critical { > > > + bool enabled; > > > + bool leave_on; > > > +}; > > > + > > > struct clk_core { > > > const char *name; > > > const struct clk_ops *ops; > > > @@ -75,6 +90,7 @@ struct clk_core { > > > struct dentry *dentry; > > > #endif > > > struct kref ref; > > > + struct critical critical; > > > }; > > > > > > struct clk { > > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk) > > > if (WARN_ON(clk->enable_count == 0)) > > > return; > > > > > > + /* Refuse to turn off a critical clock */ > > > + if (clk->enable_count == 1 && clk->critical.leave_on) > > > + return; > > > > How do we get to this point? clk_enable_critical actually calls > > clk_enable, thus incrementing the enable_count. The only time that we > > could hit the above case is if, > > > > a) there is an imbalance in clk_enable and clk_disable calls. If this is > > the case then the drivers need to be fixed. Or better yet some > > infrastructure to catch that, now that we have per-user struct clk > > cookies. > > > > b) a driver knowingly calls clk_enable_critical(foo) and then regular, > > old clk_disable(foo). But why would a driver do that? > > > > It might be that I am missing the point here, so please feel free to > > clue me in. > > This check behaves in a very similar to the WARN() above. It's more > of a fail-safe. If all drivers are behaving properly, then it > shouldn't ever be true. If they're not, it prevents an incorrectly > written driver from irrecoverably crippling the system. Then this check should be replaced with a generic approach that refuses to honor imbalances anyways. Below are two patches that probably resolve the issue of badly behaving drivers that cause enable imbalances. > > As I said in the other mail. We can do without these 3 new wrappers. > We _could_ just write a driver which only calls clk_enable() _after_ > it calls clk_disable(), a kind of intentional unbalance and it would > do that same thing. This naive approach will not work with per-user imbalance tracking. > However, what we're trying to do here is provide > a proper API, so we can see at first glance what the 'knowledgeable' > driver is trying to do and not have someone attempt to submit a 'fix' > which calls clk_enable() or something. We'll need some type of api for sure for the handoff. Regards, Mike >From 3599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001 From: Michael Turquette Date: Wed, 29 Jul 2015 18:22:45 -0700 Subject: [PATCH 1/2] clk: per-user clk prepare & enable ref counts This patch adds prepare and enable reference counts for the per-user handles that clock consumers have for a clock node. This patch warns if an imbalance occurs while trying to disable or unprepare a clock and aborts, leaving the hardware unaffected. Signed-off-by: Michael Turquette --- drivers/clk/clk.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 898052e..72feee9 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -84,6 +84,8 @@ struct clk { unsigned long min_rate; unsigned long max_rate; struct hlist_node clks_node; + unsigned int enable_count; + unsigned int prepare_count; }; /*** locking ***/ @@ -600,6 +602,9 @@ void clk_unprepare(struct clk *clk) return; clk_prepare_lock(); + if (WARN_ON(clk->prepare_count == 0)) + return; + clk->prepare_count--; clk_core_unprepare(clk->core); clk_prepare_unlock(); } @@ -657,6 +662,7 @@ int clk_prepare(struct clk *clk) return 0; clk_prepare_lock(); + clk->prepare_count++; ret = clk_core_prepare(clk->core); clk_prepare_unlock(); @@ -707,6 +713,9 @@ void clk_disable(struct clk *clk) return; flags = clk_enable_lock(); + if (WARN_ON(clk->enable_count == 0)) + return; + clk->enable_count--; clk_core_disable(clk->core); clk_enable_unlock(flags); } @@ -769,6 +778,7 @@ int clk_enable(struct clk *clk) return 0; flags = clk_enable_lock(); + clk->enable_count++; ret = clk_core_enable(clk->core); clk_enable_unlock(flags); -- 1.9.1 >From ace76f6ed634a69c499f8440a98d4b5a54d78368 Mon Sep 17 00:00:00 2001 From: Michael Turquette Date: Thu, 30 Jul 2015 12:52:26 -0700 Subject: [PATCH 2/2] clk: clk_put WARNs if user has not disabled clk >From the clk_put kerneldoc in include/linux/clk.h: """ Note: drivers must ensure that all clk_enable calls made on this clock source are balanced by clk_disable calls prior to calling this function. """ The common clock framework implementation of the clk.h api has per-user reference counts for calls to clk_prepare and clk_disable. As such it can enforce the requirement to properly call clk_disable and clk_unprepare before calling clk_put. Because this requirement is probably violated in many places, this patch starts with a simple warning. Once offending code has been fixed this check could additionally release the reference counts automatically. Signed-off-by: Michael Turquette --- drivers/clk/clk.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 72feee9..6ec0f77 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2764,6 +2764,14 @@ void __clk_put(struct clk *clk) clk->max_rate < clk->core->req_rate) clk_core_set_rate_nolock(clk->core, clk->core->req_rate); + /* + * before calling clk_put, all calls to clk_prepare and clk_enable from + * a given user must be balanced with calls to clk_disable and + * clk_unprepare by that same user + */ + WARN_ON(clk->prepare_count); + WARN_ON(clk->enable_count); + owner = clk->core->owner; kref_put(&clk->core->ref, __clk_release);