Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753668AbdGCMBO (ORCPT ); Mon, 3 Jul 2017 08:01:14 -0400 Received: from smtp6-v.fe.bosch.de ([139.15.237.11]:32241 "EHLO smtp6-v.fe.bosch.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752538AbdGCMBK (ORCPT ); Mon, 3 Jul 2017 08:01:10 -0400 Subject: Re: [PATCH 2/3] clk: WARN_ON about to disable a critical clock To: Lee Jones CC: Michael Turquette , , , , , , , , References: <1453127331-20616-1-git-send-email-lee.jones@linaro.org> <1453127331-20616-3-git-send-email-lee.jones@linaro.org> <20160211004327.26445.27416@quark.deferred.io> <20170703115324.5re2d32bd3slutcb@dell> From: Dirk Behme Organization: Robert Bosch Car Multimedia GmbH Message-ID: Date: Mon, 3 Jul 2017 14:01:07 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170703115324.5re2d32bd3slutcb@dell> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.34.211.14] X-TM-AS-MML: disable X-TM-AS-Product-Ver: IMSS-7.1.0.1679-8.1.0.1062-23172.006 X-TMASE-MatchedRID: y/2oPz6gbvgOwH4pD14DsPHkpkyUphL9ll6sbwG1OHJVbahn2vxMVvlY oV6p/cSxO86I5zECmW2UUmypFGwGyOETiJzZYgN7utvHF25zoU+usS9CiBzL8U8vg1FXaj1o/VQ G3oVfx34sr7IG4BZVskXg8rEbZT9yLwN0Ppqp4vemCb3pcbqeI34rryovYbmmymP/1piI/6Fh4r /wCPzhZEQI4wgVndbV2esB5Fs1I+nF2obiMfx35TvfsoCuAcP+9Ry+UpVqOf7rnLh7BL7kxWeMZ iYJGeMBjp6XhEN+L9kRA4hwIn2MDTcpdZ3fQiLdOX/V8P8ail1ZDL1gLmoa/PoA9r2LThYYKrau Xd3MZDW2yXVx6y9ff6q9fuM31aPtuKmQ/cxZ3jdXc+VN8dwxd0rfo8xesqwc Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3074 Lines: 98 On 03.07.2017 13:53, Lee Jones wrote: > On Tue, 27 Jun 2017, Dirk Behme wrote: > >> On 11.02.2016 01:43, Michael Turquette wrote: >>> Quoting Lee Jones (2016-01-18 06:28:50) >>>> Signed-off-by: Lee Jones >>> >>> Looks good to me. >>> >>> Regards, >>> Mike >>> >>>> --- >>>> drivers/clk/clk.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >>>> index 835cb85..178b364 100644 >>>> --- a/drivers/clk/clk.c >>>> +++ b/drivers/clk/clk.c >>>> @@ -575,6 +575,9 @@ static void clk_core_unprepare(struct clk_core *core) >>>> if (WARN_ON(core->prepare_count == 0)) >>>> return; >>>> + if (WARN_ON(core->prepare_count == 1 && core->flags & CLK_IS_CRITICAL)) >>>> + return; >>>> + >>>> if (--core->prepare_count > 0) >>>> return; >>>> @@ -680,6 +683,9 @@ static void clk_core_disable(struct clk_core *core) >>>> if (WARN_ON(core->enable_count == 0)) >>>> return; >>>> + if (WARN_ON(core->enable_count == 1 && core->flags & CLK_IS_CRITICAL)) >>>> + return; >>>> + >>>> if (--core->enable_count > 0) >>>> return; >> >> >> I have a question regarding this patch, which is mainline meanwhile [1]: >> >> Having the following clock configuration: >> >> |--> child clk '1' (crit) >> clk source --> parent clk 'A' (crit) -->| >> |--> child clk '2' >> >> >> Clock '2' might be used, or not. It might be disabled or not. It doesn't >> matter. Clock '1' is not allowed to be disabled. Therefore its marked as >> critical. >> >> Parent clock 'A' is marked as critical because its not allowed to be >> disabled, even if the enable_count of all child clocks is 0. To avoid that >> by disabling parent clock 'A' the child clock '1' is disabled, too, whats >> not allowed as its marked as critical. >> >> >> Now, child clock '2' is used and enabled & disabled continuously by a (SPI) >> driver. What is ok. But: >> >> Disabling child clock '2' results in the attempt to disable parent clock >> 'A', too, which has correct enable_count 1 (from enabling the child '2'). >> What results >> >> a) in the WARN_ON output >> >> and >> >> b) enable_count of 'A' never decreases to 0. Being off by one after the >> WARN_ON >> >> >> It sounds like both is wrong for a configuration like above. > > Clock A still has one user, Clock 1. > > Why is that wrong? Because clock 1 is not a (Linux kernel clock framework) used clock. Its enable count is 0. So from Linux kernel (clock framework) point of view clock 1 is unused. The increase/decrease of enable count of parent clock A is only driven by the Linux kernel usage of clock 2. Best regards Dirk >> Opinions or proposal how to fix/change this? >> >> >> Best regards >> >> Dirk >> >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/clk.c?id=2e20fbf592621b2c2aeddd82e0fa3dad053cce03