From mboxrd@z Thu Jan 1 00:00:00 1970 Received: with ECARTIS (v1.0.0; list linux-mips); Thu, 29 Jan 2015 20:13:45 +0100 (CET) Received: from smtp.codeaurora.org ([198.145.11.231]:40208 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S27012117AbbA2TNnWgyla (ORCPT ); Thu, 29 Jan 2015 20:13:43 +0100 Received: from smtp.codeaurora.org (localhost [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id C68CE141707; Thu, 29 Jan 2015 19:13:40 +0000 (UTC) Received: by smtp.codeaurora.org (Postfix, from userid 486) id A5D6614170A; Thu, 29 Jan 2015 19:13:40 +0000 (UTC) Received: from [10.46.167.8] (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 D024E141707; Thu, 29 Jan 2015 19:13:38 +0000 (UTC) Message-ID: <54CA8662.7040008@codeaurora.org> Date: Thu, 29 Jan 2015 11:13:38 -0800 From: Stephen Boyd User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Geert Uytterhoeven , Tomeu Vizoso , Mike Turquette CC: "linux-kernel@vger.kernel.org" , Javier Martinez Canillas , Jonathan Corbet , Tony Lindgren , Russell King , Ralf Baechle , Boris Brezillon , =?UTF-8?B?RW1pbGlvIEzDs3Bleg==?= , Maxime Ripard , Tero Kristo , Manuel Lauss , Alex Elder , Matt Porter , Haojian Zhuang , Zhangfei Gao , Bintian Wang , Chao Xie , "linux-doc@vger.kernel.org" , "linux-omap@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Linux MIPS Mailing List , Linux-sh list Subject: Re: [PATCH v13 4/6] clk: Add rate constraints to clocks References: <1422011024-32283-1-git-send-email-tomeu.vizoso@collabora.com> <1422011024-32283-5-git-send-email-tomeu.vizoso@collabora.com> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Virus-Scanned: ClamAV using ClamSMTP Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s linux-mips"> (uid 0) X-Orcpt: rfc822;linux-mips@linux-mips.org Original-Recipient: rfc822;linux-mips@linux-mips.org X-archive-position: 45549 X-ecartis-version: Ecartis v1.0.0 Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org X-original-sender: sboyd@codeaurora.org Precedence: bulk List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: linux-mips X-List-ID: linux-mips List-subscribe: List-owner: List-post: List-archive: X-list: linux-mips On 01/29/15 05:31, Geert Uytterhoeven wrote: > Hi Tomeu, Mike, > > On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso > wrote: >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk) >> return 1; >> } >> >> -static void clk_core_put(struct clk_core *core) >> +void __clk_put(struct clk *clk) >> { >> struct module *owner; >> >> - owner = core->owner; >> + if (!clk || WARN_ON_ONCE(IS_ERR(clk))) >> + return; >> >> clk_prepare_lock(); >> - kref_put(&core->ref, __clk_release); >> + >> + hlist_del(&clk->child_node); >> + clk_core_set_rate_nolock(clk->core, clk->core->req_rate); > At this point, clk->core->req_rate is still zero, causing > cpg_div6_clock_round_rate() to be called with a zero "rate" parameter, > e.g. on r8a7791: Hmm.. I wonder if we should assign core->req_rate to be the same as core->rate during __clk_init()? That would make this call to clk_core_set_rate_nolock() a nop in this case. > > cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock mmc0 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 > > and cpg_div6_clock_calc_div() is called to calculate > > div = DIV_ROUND_CLOSEST(parent_rate, rate); > > Why was this call to clk_core_set_rate_nolock() in __clk_put() added? > Before, there was no rate setting done at this point, and > cpg_div6_clock_round_rate() was not called. We need to call clk_core_set_rate_nolock() here to drop any min/max rate request that this consumer has. > > Have the semantics changed? Should .round_rate() be ready to > accept a "zero" rate, and use e.g. the current rate instead? It seems like we've also exposed a bug in cpg_div6_clock_calc_div(). Technically any driver could have called clk_round_rate() with a zero rate before this change and that would have caused the same division by zero. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [PATCH v13 4/6] clk: Add rate constraints to clocks Date: Thu, 29 Jan 2015 11:13:38 -0800 Message-ID: <54CA8662.7040008@codeaurora.org> References: <1422011024-32283-1-git-send-email-tomeu.vizoso@collabora.com> <1422011024-32283-5-git-send-email-tomeu.vizoso@collabora.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-doc-owner@vger.kernel.org To: Geert Uytterhoeven , Tomeu Vizoso , Mike Turquette Cc: "linux-kernel@vger.kernel.org" , Javier Martinez Canillas , Jonathan Corbet , Tony Lindgren , Russell King , Ralf Baechle , Boris Brezillon , =?UTF-8?B?RW1pbGlvIEzDs3Bleg==?= , Maxime Ripard , Tero Kristo , Manuel Lauss , Alex Elder , Matt Porter , Haojian Zhuang , Zhangfei Gao , Bintian Wang , Chao Xie , "linux-doc@vger.kernel.org" , "linux-omap@vger.kernel.org" List-Id: linux-omap@vger.kernel.org On 01/29/15 05:31, Geert Uytterhoeven wrote: > Hi Tomeu, Mike, > > On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso > wrote: >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk) >> return 1; >> } >> >> -static void clk_core_put(struct clk_core *core) >> +void __clk_put(struct clk *clk) >> { >> struct module *owner; >> >> - owner = core->owner; >> + if (!clk || WARN_ON_ONCE(IS_ERR(clk))) >> + return; >> >> clk_prepare_lock(); >> - kref_put(&core->ref, __clk_release); >> + >> + hlist_del(&clk->child_node); >> + clk_core_set_rate_nolock(clk->core, clk->core->req_rate); > At this point, clk->core->req_rate is still zero, causing > cpg_div6_clock_round_rate() to be called with a zero "rate" parameter, > e.g. on r8a7791: Hmm.. I wonder if we should assign core->req_rate to be the same as core->rate during __clk_init()? That would make this call to clk_core_set_rate_nolock() a nop in this case. > > cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock mmc0 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 > > and cpg_div6_clock_calc_div() is called to calculate > > div = DIV_ROUND_CLOSEST(parent_rate, rate); > > Why was this call to clk_core_set_rate_nolock() in __clk_put() added? > Before, there was no rate setting done at this point, and > cpg_div6_clock_round_rate() was not called. We need to call clk_core_set_rate_nolock() here to drop any min/max rate request that this consumer has. > > Have the semantics changed? Should .round_rate() be ready to > accept a "zero" rate, and use e.g. the current rate instead? It seems like we've also exposed a bug in cpg_div6_clock_calc_div(). Technically any driver could have called clk_round_rate() with a zero rate before this change and that would have caused the same division by zero. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Date: Thu, 29 Jan 2015 19:13:38 +0000 Subject: Re: [PATCH v13 4/6] clk: Add rate constraints to clocks Message-Id: <54CA8662.7040008@codeaurora.org> List-Id: References: <1422011024-32283-1-git-send-email-tomeu.vizoso@collabora.com> <1422011024-32283-5-git-send-email-tomeu.vizoso@collabora.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-arm-kernel@lists.infradead.org On 01/29/15 05:31, Geert Uytterhoeven wrote: > Hi Tomeu, Mike, > > On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso > wrote: >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk) >> return 1; >> } >> >> -static void clk_core_put(struct clk_core *core) >> +void __clk_put(struct clk *clk) >> { >> struct module *owner; >> >> - owner = core->owner; >> + if (!clk || WARN_ON_ONCE(IS_ERR(clk))) >> + return; >> >> clk_prepare_lock(); >> - kref_put(&core->ref, __clk_release); >> + >> + hlist_del(&clk->child_node); >> + clk_core_set_rate_nolock(clk->core, clk->core->req_rate); > At this point, clk->core->req_rate is still zero, causing > cpg_div6_clock_round_rate() to be called with a zero "rate" parameter, > e.g. on r8a7791: Hmm.. I wonder if we should assign core->req_rate to be the same as core->rate during __clk_init()? That would make this call to clk_core_set_rate_nolock() a nop in this case. > > cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock mmc0 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 > > and cpg_div6_clock_calc_div() is called to calculate > > div = DIV_ROUND_CLOSEST(parent_rate, rate); > > Why was this call to clk_core_set_rate_nolock() in __clk_put() added? > Before, there was no rate setting done at this point, and > cpg_div6_clock_round_rate() was not called. We need to call clk_core_set_rate_nolock() here to drop any min/max rate request that this consumer has. > > Have the semantics changed? Should .round_rate() be ready to > accept a "zero" rate, and use e.g. the current rate instead? It seems like we've also exposed a bug in cpg_div6_clock_calc_div(). Technically any driver could have called clk_round_rate() with a zero rate before this change and that would have caused the same division by zero. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project From mboxrd@z Thu Jan 1 00:00:00 1970 From: sboyd@codeaurora.org (Stephen Boyd) Date: Thu, 29 Jan 2015 11:13:38 -0800 Subject: [PATCH v13 4/6] clk: Add rate constraints to clocks In-Reply-To: References: <1422011024-32283-1-git-send-email-tomeu.vizoso@collabora.com> <1422011024-32283-5-git-send-email-tomeu.vizoso@collabora.com> Message-ID: <54CA8662.7040008@codeaurora.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 01/29/15 05:31, Geert Uytterhoeven wrote: > Hi Tomeu, Mike, > > On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso > wrote: >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk) >> return 1; >> } >> >> -static void clk_core_put(struct clk_core *core) >> +void __clk_put(struct clk *clk) >> { >> struct module *owner; >> >> - owner = core->owner; >> + if (!clk || WARN_ON_ONCE(IS_ERR(clk))) >> + return; >> >> clk_prepare_lock(); >> - kref_put(&core->ref, __clk_release); >> + >> + hlist_del(&clk->child_node); >> + clk_core_set_rate_nolock(clk->core, clk->core->req_rate); > At this point, clk->core->req_rate is still zero, causing > cpg_div6_clock_round_rate() to be called with a zero "rate" parameter, > e.g. on r8a7791: Hmm.. I wonder if we should assign core->req_rate to be the same as core->rate during __clk_init()? That would make this call to clk_core_set_rate_nolock() a nop in this case. > > cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock mmc0 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 > cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 > > and cpg_div6_clock_calc_div() is called to calculate > > div = DIV_ROUND_CLOSEST(parent_rate, rate); > > Why was this call to clk_core_set_rate_nolock() in __clk_put() added? > Before, there was no rate setting done at this point, and > cpg_div6_clock_round_rate() was not called. We need to call clk_core_set_rate_nolock() here to drop any min/max rate request that this consumer has. > > Have the semantics changed? Should .round_rate() be ready to > accept a "zero" rate, and use e.g. the current rate instead? It seems like we've also exposed a bug in cpg_div6_clock_calc_div(). Technically any driver could have called clk_round_rate() with a zero rate before this change and that would have caused the same division by zero. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project