From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [RFC PATCH 02/34] msm: clock: Always use an array to iterate over clocks Date: Wed, 02 Nov 2011 14:34:03 -0700 Message-ID: <4EB1B74B.8060006@codeaurora.org> References: <1320258991-22325-1-git-send-email-davidb@codeaurora.org> <1320258991-22325-3-git-send-email-davidb@codeaurora.org> <20111102194555.GE12913@n2100.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from wolverine01.qualcomm.com ([199.106.114.254]:25859 "EHLO wolverine01.qualcomm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751653Ab1KBVeE (ORCPT ); Wed, 2 Nov 2011 17:34:04 -0400 In-Reply-To: <20111102194555.GE12913@n2100.arm.linux.org.uk> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Russell King - ARM Linux Cc: David Brown , Daniel Walker , Bryan Huntsman , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org On 11/02/11 12:45, Russell King - ARM Linux wrote: > On Wed, Nov 02, 2011 at 11:35:59AM -0700, David Brown wrote: >> If the array of clk_lookups contains aliases for the same struct >> clk, msm_clock_init() will add the clock to the clocks list >> twice. This would cause list corruption so let's just remove the >> clocks list and any associated code and iterate over the array >> instead. > Hmm... > >> @@ -158,13 +152,13 @@ void __init msm_clock_init(struct clk_lookup *clock_tbl, unsigned num_clocks) >> */ >> static int __init clock_late_init(void) >> { >> + unsigned i, count = 0; >> unsigned long flags; >> - struct clk *clk; >> - unsigned count = 0; >> >> clock_debug_init(); >> - mutex_lock(&clocks_mutex); >> - list_for_each_entry(clk, &clocks, list) { >> + for (i = 0; i < msm_num_clocks; i++) { >> + struct clk *clk = msm_clocks[i].clk; >> + >> clock_debug_add(clk); > This means you'll end up calling clock_debug_add() twice for the same > struct clk - this sounds like a bad idea in itself. It looks like > there's no protection within that function against it being called > twice with the same struct clk. > > Are you sure this is safe? This hasn't proven to be a problem so far because debugfs returns an error when you create a directory with the same name twice. If we ever do something more in clock_debug_add() we would have a problem. -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. From mboxrd@z Thu Jan 1 00:00:00 1970 From: sboyd@codeaurora.org (Stephen Boyd) Date: Wed, 02 Nov 2011 14:34:03 -0700 Subject: [RFC PATCH 02/34] msm: clock: Always use an array to iterate over clocks In-Reply-To: <20111102194555.GE12913@n2100.arm.linux.org.uk> References: <1320258991-22325-1-git-send-email-davidb@codeaurora.org> <1320258991-22325-3-git-send-email-davidb@codeaurora.org> <20111102194555.GE12913@n2100.arm.linux.org.uk> Message-ID: <4EB1B74B.8060006@codeaurora.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 11/02/11 12:45, Russell King - ARM Linux wrote: > On Wed, Nov 02, 2011 at 11:35:59AM -0700, David Brown wrote: >> If the array of clk_lookups contains aliases for the same struct >> clk, msm_clock_init() will add the clock to the clocks list >> twice. This would cause list corruption so let's just remove the >> clocks list and any associated code and iterate over the array >> instead. > Hmm... > >> @@ -158,13 +152,13 @@ void __init msm_clock_init(struct clk_lookup *clock_tbl, unsigned num_clocks) >> */ >> static int __init clock_late_init(void) >> { >> + unsigned i, count = 0; >> unsigned long flags; >> - struct clk *clk; >> - unsigned count = 0; >> >> clock_debug_init(); >> - mutex_lock(&clocks_mutex); >> - list_for_each_entry(clk, &clocks, list) { >> + for (i = 0; i < msm_num_clocks; i++) { >> + struct clk *clk = msm_clocks[i].clk; >> + >> clock_debug_add(clk); > This means you'll end up calling clock_debug_add() twice for the same > struct clk - this sounds like a bad idea in itself. It looks like > there's no protection within that function against it being called > twice with the same struct clk. > > Are you sure this is safe? This hasn't proven to be a problem so far because debugfs returns an error when you create a directory with the same name twice. If we ever do something more in clock_debug_add() we would have a problem. -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.