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: 7457851 X-Patchwork-Delegate: geert@linux-m68k.org Return-Path: X-Original-To: patchwork-linux-sh@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 6A5549F37F for ; Wed, 21 Oct 2015 15:51:10 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 28A1F208C0 for ; Wed, 21 Oct 2015 15:51:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C8CA4208C1 for ; Wed, 21 Oct 2015 15:51:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753531AbbJUPvG (ORCPT ); Wed, 21 Oct 2015 11:51:06 -0400 Received: from mail-pa0-f49.google.com ([209.85.220.49]:34707 "EHLO mail-pa0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751236AbbJUPvE convert rfc822-to-8bit (ORCPT ); Wed, 21 Oct 2015 11:51:04 -0400 Received: by padhk11 with SMTP id hk11so58328613pad.1 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=dyMiC026jRmYtGR/N3EcgWAFT0vE+6MmtW5Dk89f65kT4JRFZXANQYVkyMMbM7dQbT L46Mb0Zism5Rlrhftc0bVJJOb6nn8dq9eZ2EaAzDXp89hnbsYfpJOu/L4s/EMLU3wznr VKv60s5tXcmlipu8Mdc0XRRtUvv4aRmQJJ3ShUCeOtDF6Xx+SW+OrSr8TWjuXmbDyUYW fDwCu7egZIjKTZmGLWQ92O7qMwMpNMrjYN5HSnIMQtQf8mZxindBhs6W5g4JDeogsbeP eeKZGScqHsuaBnyajQ72TvOZbUjGKRm7TYhkBjWwyTUau1ioIzFGjZOe/hqejXepO6XD RdQA== X-Gm-Message-State: ALoCoQmiXop9jt9ZXkPIKr+cwTC95r2Vrgn61N4ONYi6pBBXLI+4dl72KSGrjw6exHUCkASixpi0 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> Cc: "linux-kernel@vger.kernel.org" , "linux-clk" , "Stephen Boyd" , "Lee Jones" , "Maxime Ripard" , "Sascha Hauer" , "Sekhar Nori" , "Kevin Hilman" , "Santosh Shilimkar" , "Tony Lindgren" , "linux-omap@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "Linux-sh list" , "Linux PM list" 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 Sender: linux-sh-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-sh@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=ham 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 --- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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;