From patchwork Wed Oct 21 15:50:57 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael Turquette X-Patchwork-Id: 7457891 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 1B3149F37F for ; Wed, 21 Oct 2015 15:53:07 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0690C208E5 for ; Wed, 21 Oct 2015 15:53:06 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CB781208DF for ; Wed, 21 Oct 2015 15:53:04 +0000 (UTC) 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 1Zovfk-0004TG-NH; Wed, 21 Oct 2015 15:51:28 +0000 Received: from mail-pa0-x22f.google.com ([2607:f8b0:400e:c03::22f]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1Zovfg-0004ML-5K for linux-arm-kernel@lists.infradead.org; Wed, 21 Oct 2015 15:51:25 +0000 Received: by pasz6 with SMTP id z6so58241487pas.2 for ; Wed, 21 Oct 2015 08:51:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre_com.20150623.gappssmtp.com; s=20150623; h=content-type:mime-version:content-transfer-encoding:to:from :in-reply-to:cc:references:message-id:user-agent:subject:date; bh=ZbbaJb80m216DKtYsRpv6ejWNJqKxU8geqEIllhkQ4k=; b=J7lQ1G6/RqjsNbyiP+Xs5cDqU8p2GIUpjhA8tmonV32sgqiejg62ToXyLWxYAWJRwM 1zRZBRBJ8Q5REHFOqNG71U0TWgiKeZcxNd1K8TCaygQfmHMclu2LLzvQI1cU0EO59wwT MNX7G+XXoGP0xgO44GKJM/5O9nSP6PoghJzbJBrIB7cOz2H8cTtp0ImwXZWcvMrJVf3B WI79F47gVVl/O2ajreM9W1fyxPynf3rtPjRbi+Z/VQozXuIdgvOF+GUOfAPiPOPwKson AhTbWPwR3or9wZafwzHKswvTNLQYuAoZhg8BkokzCtCvuZ798ki0UhrAAJu+el9MsFst +2hw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:content-type:mime-version :content-transfer-encoding:to:from:in-reply-to:cc:references :message-id:user-agent:subject:date; bh=ZbbaJb80m216DKtYsRpv6ejWNJqKxU8geqEIllhkQ4k=; b=dpNH5nvwe3xdozSpmioTyO3r/rbFVVkB0Tw0h3iR0kVNuYMAnGlh6J3EHd3WmugWn9 hb8xINANGLtCGfIfLhl2BU62Hqk6su5NVGclXI0Z1G3Z5fcTz3HTtnQ0ioM93RqHo8/Q 1uMym5LS8vrLbSi3tvbxYFSjVs1jMzpqmF+kDSgLkxKJmscssjEcmlCbglgYzkpuoeed MKXOEsY9vqLTclxuQrZjjlaZ0pp8NxQZsQbsNBcpFn+F9EGsog1yJCVlw6Il2f6uRVvp NLHdxL9f5vDg/kSGheX36oAI0g/CHfJxBdhVyIOERWbnLr6Xe/bQmz6V2sjxl8AldnN1 MJCA== X-Gm-Message-State: ALoCoQmlmFS6lhwY+0UCJdQmfcwFLNbfGWd3K2l2Mjtm+sYuP3xannC1MqvI1x49vdbRQOI31l65 X-Received: by 10.68.129.231 with SMTP id nz7mr11378472pbb.53.1445442662803; Wed, 21 Oct 2015 08:51:02 -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 bh4sm9836040pbb.62.2015.10.21.08.51.01 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 21 Oct 2015 08:51:02 -0700 (PDT) MIME-Version: 1.0 To: Russell King - ARM Linux , "Geert Uytterhoeven" From: Michael Turquette In-Reply-To: <20151021105932.GP32536@n2100.arm.linux.org.uk> References: <1438974570-20812-1-git-send-email-mturquette@baylibre.com> <1438974570-20812-3-git-send-email-mturquette@baylibre.com> <20151020124000.20687.60752@quantum> <20151021105932.GP32536@n2100.arm.linux.org.uk> Message-ID: <20151021155057.20687.14055@quantum> User-Agent: alot/0.3.6 Subject: Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk Date: Wed, 21 Oct 2015 08:50:57 -0700 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20151021_085124_294895_1B408EC5 X-CRM114-Status: GOOD ( 33.62 ) X-Spam-Score: -1.9 (-) 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: Kevin Hilman , Tony Lindgren , Sekhar Nori , Sascha Hauer , Linux-sh list , Stephen Boyd , "linux-kernel@vger.kernel.org" , Santosh Shilimkar , Linux PM list , Maxime Ripard , "linux-omap@vger.kernel.org" , Lee Jones , linux-clk , "linux-arm-kernel@lists.infradead.org" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable 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 Quoting Russell King - ARM Linux (2015-10-21 03:59:32) > On Wed, Oct 21, 2015 at 11:50:07AM +0200, Geert Uytterhoeven wrote: > > Hi Mike, Russell, > > > > On Tue, Oct 20, 2015 at 2:40 PM, Michael Turquette > > wrote: > > > Why not keep the reference to the struct clk after get'ing it the first > > > time? > > > > And store it where? > > Not my problem :) > > Users are supposed to hold on to the reference obtained via clk_get() > while they're making use of the clock: in some implementations, this > increments the module use count if the clock driver is a module, and > may have other effects too. > > Dropping that while you're still requiring the clock to be enabled is > unsafe: if it is provided by a module, then removing and reinserting > the module may very well change the enabled state of the clock, it > most certainly will disrupt the enable count. > > It's always been this way, right from the outset, and when I've seen > people doing this bollocks, I've always pointed out that it's wrong. > Generally, people will fix it once they become aware of it, so it's > really that people just don't like reading and conforming to published > API requirements. > > I think the root cause is that people just don't like reading what > other people write in terms of documentation, and they prefer to go > off and do their own thing, provided it works for them. Right, so in other words this problem must be solved by the caller of clk_get, as it should be. I have never much looked at the pm clk code in question, but I gave it a quick look and came up with some example code that does not compile, in an effort to be as helpful as possible. Basically I added a flex array to struct pm_clk_notifier_block, so that now there are two flex arrays which is stupid. But I am too lazy to replace the nbclk->clks thing with a malloc after walking all of the clk_id's to figure out the number of clocks. Or we could just add .num_clk to the struct, fix up all 4 users of it and drop the NULL sentinel used the .clk_id's... Hmm that would have been better. Anyways here is the ugly, non-compiling code. I'll take another look at it in one year if no one else beats me to it. Regards, Mike diff --git a/arch/arm/mach-davinci/pm_domain.c b/arch/arm/mach-davinci/pm_domain.c index 78eac2c..b46e5ce 100644 --- a/arch/arm/mach-davinci/pm_domain.c +++ b/arch/arm/mach-davinci/pm_domain.c @@ -24,6 +24,7 @@ static struct dev_pm_domain davinci_pm_domain = { static struct pm_clk_notifier_block platform_bus_notifier = { .pm_domain = &davinci_pm_domain, .con_ids = { "fck", "master", "slave", NULL }, + .clks = { ERR_PTR(-EAGAIN), ERR_PTR(-EAGAIN), ERR_PTR(-EAGAIN) }, }; static int __init davinci_pm_runtime_init(void) diff --git a/arch/arm/mach-keystone/pm_domain.c b/arch/arm/mach-keystone/pm_domain.c index e283939..d21c18b 100644 --- a/arch/arm/mach-keystone/pm_domain.c +++ b/arch/arm/mach-keystone/pm_domain.c @@ -27,6 +27,7 @@ static struct dev_pm_domain keystone_pm_domain = { static struct pm_clk_notifier_block platform_domain_notifier = { .pm_domain = &keystone_pm_domain, + .clks = { ERR_PTR(-EAGAIN) }, }; static const struct of_device_id of_keystone_table[] = { diff --git a/arch/arm/mach-omap1/pm_bus.c b/arch/arm/mach-omap1/pm_bus.c index 667c163..5506453 100644 --- a/arch/arm/mach-omap1/pm_bus.c +++ b/arch/arm/mach-omap1/pm_bus.c @@ -31,6 +31,7 @@ static struct dev_pm_domain default_pm_domain = { static struct pm_clk_notifier_block platform_bus_notifier = { .pm_domain = &default_pm_domain, .con_ids = { "ick", "fck", NULL, }, + .clks = { ERR_PTR(-EAGAIN), ERR_PTR(-EAGAIN) }, }; static int __init omap1_pm_runtime_init(void) diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c index 652b5a3..26f0dcf 100644 --- a/drivers/base/power/clock_ops.c +++ b/drivers/base/power/clock_ops.c @@ -407,40 +407,6 @@ int pm_clk_runtime_resume(struct device *dev) #else /* !CONFIG_PM */ /** - * enable_clock - Enable a device clock. - * @dev: Device whose clock is to be enabled. - * @con_id: Connection ID of the clock. - */ -static void enable_clock(struct device *dev, const char *con_id) -{ - struct clk *clk; - - clk = clk_get(dev, con_id); - if (!IS_ERR(clk)) { - clk_prepare_enable(clk); - clk_put(clk); - dev_info(dev, "Runtime PM disabled, clock forced on.\n"); - } -} - -/** - * disable_clock - Disable a device clock. - * @dev: Device whose clock is to be disabled. - * @con_id: Connection ID of the clock. - */ -static void disable_clock(struct device *dev, const char *con_id) -{ - struct clk *clk; - - clk = clk_get(dev, con_id); - if (!IS_ERR(clk)) { - clk_disable_unprepare(clk); - clk_put(clk); - dev_info(dev, "Runtime PM disabled, clock forced off.\n"); - } -} - -/** * pm_clk_notify - Notify routine for device addition and removal. * @nb: Notifier block object this function is a member of. * @action: Operation being carried out by the caller. @@ -465,18 +431,45 @@ static int pm_clk_notify(struct notifier_block *nb, switch (action) { case BUS_NOTIFY_BIND_DRIVER: if (clknb->con_ids[0]) { - for (con_id = clknb->con_ids; *con_id; con_id++) - enable_clock(dev, *con_id); + int i; + for (con_id = clknb->con_ids, i = 0; *con_id; + con_id++, i++) { + if (clknb->clks[i] == ERR_PTR(-EAGAIN)) + clks[i] = clk_get(dev, *con_id); + if (!IS_ERR(clknb->clks[i])) { + clk_prepare_enable(clk); + dev_info(dev, "Runtime PM disabled, clock forced on.\n"); + } } else { - enable_clock(dev, NULL); + if (clknb->clks[0] == ERR_PTR(-EAGAIN)) + clks[0] = clk_get(dev, NULL); + if (!IS_ERR(clknb->clks[0])) { + clk_prepare_enable(clk); + dev_info(dev, "Runtime PM disabled, clock forced on.\n"); + } } break; case BUS_NOTIFY_UNBOUND_DRIVER: + /* + * FIXME + * We never call clk_put. This should be done with + * pm_clk_remove_notifier, which doesn't exist but probably + * should + */ if (clknb->con_ids[0]) { - for (con_id = clknb->con_ids; *con_id; con_id++) - disable_clock(dev, *con_id); + int i; + for (con_id = clknb->con_ids, i = 0; *con_id; + con_id++, i++) { + if (!IS_ERR(clknb->clks[i])) { + clk_disable_unprepare(clknb->clks[i]); + dev_info(dev, "Runtime PM disabled, clock forced off.\n"); + } + } } else { - disable_clock(dev, NULL); + if (!IS_ERR(clknb->clks[0])) { + clk_disable_unprepare(clknb->clks[i]); + dev_info(dev, "Runtime PM disabled, clock forced off.\n"); + } } break; } diff --git a/drivers/sh/pm_runtime.c b/drivers/sh/pm_runtime.c index 25abd4e..08754a4 100644 --- a/drivers/sh/pm_runtime.c +++ b/drivers/sh/pm_runtime.c @@ -30,6 +30,7 @@ static struct dev_pm_domain default_pm_domain = { static struct pm_clk_notifier_block platform_bus_notifier = { .pm_domain = &default_pm_domain, .con_ids = { NULL, }, + .clks = { ERR_PTR(-EAGAIN) }, }; static int __init sh_pm_runtime_init(void) diff --git a/include/linux/pm_clock.h b/include/linux/pm_clock.h index 25266c6..45e58fe 100644 --- a/include/linux/pm_clock.h +++ b/include/linux/pm_clock.h @@ -16,6 +16,7 @@ struct pm_clk_notifier_block { struct notifier_block nb; struct dev_pm_domain *pm_domain; char *con_ids[]; + struct clk *clks[]; }; struct clk;