From patchwork Mon Sep 28 18:46:40 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Stephen Boyd X-Patchwork-Id: 7279241 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 5DC74BEEA4 for ; Mon, 28 Sep 2015 18:46:46 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 76F4D20718 for ; Mon, 28 Sep 2015 18:46:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 77AEC206D8 for ; Mon, 28 Sep 2015 18:46:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750925AbbI1Sqn (ORCPT ); Mon, 28 Sep 2015 14:46:43 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:56315 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750877AbbI1Sqm (ORCPT ); Mon, 28 Sep 2015 14:46:42 -0400 Received: from smtp.codeaurora.org (localhost [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id B5D18140DA4; Mon, 28 Sep 2015 18:46:41 +0000 (UTC) Received: by smtp.codeaurora.org (Postfix, from userid 486) id A60B2140E0E; Mon, 28 Sep 2015 18:46:41 +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=unavailable 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 2AD64140DA4; Mon, 28 Sep 2015 18:46:41 +0000 (UTC) Date: Mon, 28 Sep 2015 11:46:40 -0700 From: Stephen Boyd To: Heiko =?iso-8859-1?Q?St=FCbner?= Cc: mturquette@baylibre.com, Kevin Hilman , "Rafael J. Wysocki" , Pavel Machek , Caesar Wang , linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org Subject: Re: [PATCH v2] clk: readd refcounting for struct clk instances Message-ID: <20150928184640.GM23081@codeaurora.org> References: <6257096.h7pub3XnDQ@diego> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <6257096.h7pub3XnDQ@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/23, Heiko Stübner wrote: > --- > > While it may be nice to do the actual handling of the clock references > only in the calling code, in this current use case it would create > a big additional overhead. > > It looks like this so called synchronous reset on power-domain state- > changes, requiring device clocks to be turned on, is not that uncommon > or rockchip-specific. > For this Kevin requested that we read the clocks from the actual consumer > devices and not double-list them in the power-domain node as well. > > So when expecting pm_clk_add_clk() to work, the current powerdomain code > can simply do when adding a device to a domain in rockchip_pd_attach_dev(): > while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) { > dev_dbg(dev, "adding clock '%pC' to list of PM clocks\n", clk); > error = pm_clk_add_clk(dev, clk); > clk_put(clk); > } > > The clock gets handed off to the generic pm clock handling and thus > clk_put in there. > > > On the other hand when only the rockchip power-domain code is expected > to get and put the clock, we would require a lot of new overhead, as now > the code would also need to track which devices got added to what > domain and also all clock-references until the device gets detached > again. So this would essentially duplicate a big part of what the > genpd-code does (per-domain device-list and per-device clock-list). > > As this seems to be not uncommon, future powerdomain drivers > might need that too and would also need to duplicate that handling. > > When allowing multiple __clk_get and __clk_put calls on the other > hand, the overhead for the regular case comes down to one atomic_inc, > atomic_sub_and_test and the function call to the new separate release > function ;-) . > Why are we doing of_clk_get(), pm_clk_add_clk(), and then clk_put()? Just drop that clk_put() in the caller and remove the __clk_get() inside pm_clk_add_clk() and everything works the same. This patch does most of that, except it doesn't handle the error path where we would need to throw a clk_put(). Really, that snippet of code that loops over a device's clocks and adds them to a pm domain is an example of duplicate code that should go into some common layer like the PM clock stuff. Then we don't have any kind of situation where we're passing struct clk pointers off to other layers of code and this "problem" doesn't exist. ----8<---- diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c index acef9f9f759a..529a03e8282c 100644 --- a/drivers/base/power/clock_ops.c +++ b/drivers/base/power/clock_ops.c @@ -95,7 +95,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; } @@ -129,7 +129,7 @@ int pm_clk_add(struct device *dev, const char *con_id) * @clk: Clock pointer * * Add the clock to the list of clocks used for the power management of @dev. - * It will increment refcount on clock pointer, use clk_put() on it when done. + * Callers should not call clk_put() on @clk after calling this function. */ int pm_clk_add_clk(struct device *dev, struct clk *clk) {