From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter De Schrijver Subject: Re: [PATCH] clk: Fix race conditions between clk_set_parent() and clk_enable() Date: Tue, 22 May 2012 16:58:13 +0300 Message-ID: <20120522135813.GL8730@tbergstrom-lnx.Nvidia.com> References: <1336798797-8724-1-git-send-email-skannan@codeaurora.org> <4FB29E7C.7010606@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <4FB29E7C.7010606@codeaurora.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Saravana Kannan Cc: Andrew Lunn , Grant Likely , Jamie Iles , Jeremy Kerr , Mike Turquette , Magnus Damm , Deepak Saxena , "linux-arm-kernel@lists.infradead.org" , Arnd Bergman , Stephen Warren , "linux-arm-msm@vger.kernel.org" , Sascha Hauer , Rob Herring , Russell King , Thomas Gleixner , Richard Zhao , Shawn Guo , Paul Walmsley , Linus Walleij , Mark Brown , Stephen Boyd List-Id: linux-arm-msm@vger.kernel.org On Tue, May 15, 2012 at 08:20:44PM +0200, Saravana Kannan wrote: > On 05/11/2012 09:59 PM, Saravana Kannan wrote: > > Without this patch, the following race conditions are possible. > > > > Race condition 1: > > * clk-A has two parents - clk-X and clk-Y. > > * All three are disabled and clk-X is current parent. > > * Thread A: clk_set_parent(clk-A, clk-Y). > > * Thread A: > > * Thread A: Grabs enable lock. > > * Thread A: Sees enable count of clk-A is 0, so doesn't enable clk-Y. > > * Thread A: Releases enable lock. > > * Thread B: Calls clk_enable(clk-A), which in turn enables clk-X. > > * Thread A: Switches clk-A's parent to clk-Y in hardware. > > > > clk-A is now enabled in software, but not clocking in hardware. > > > > Race condition 2: > > * clk-A has two parents - clk-X and clk-Y. > > * All three are disabled and clk-X is current parent. > > * Thread A: clk_set_parent(clk-A, clk-Y). > > * Thread A: > > * Thread A: Switches parent in hardware to clk-Y. > > * Thread A: Grabs enable lock. > > * Thread A: Sees enable count of clk-A is 0, so doesn't disable clk-X. > > * Thread A: Releases enable lock. > > * Thread B: Calls clk_enable(clk-A) > > * Thread B: Software state still says parent is clk-X. > > * Thread B: So, enables clk-X and then itself. > > * Thread A: Updates parent in software state to clk-Y. > > This looks correct to me. Is there any usecase where enabling/disabling a clock would require sleeping but changing the parent would not? Cheers, Peter. From mboxrd@z Thu Jan 1 00:00:00 1970 From: pdeschrijver@nvidia.com (Peter De Schrijver) Date: Tue, 22 May 2012 16:58:13 +0300 Subject: [PATCH] clk: Fix race conditions between clk_set_parent() and clk_enable() In-Reply-To: <4FB29E7C.7010606@codeaurora.org> References: <1336798797-8724-1-git-send-email-skannan@codeaurora.org> <4FB29E7C.7010606@codeaurora.org> Message-ID: <20120522135813.GL8730@tbergstrom-lnx.Nvidia.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, May 15, 2012 at 08:20:44PM +0200, Saravana Kannan wrote: > On 05/11/2012 09:59 PM, Saravana Kannan wrote: > > Without this patch, the following race conditions are possible. > > > > Race condition 1: > > * clk-A has two parents - clk-X and clk-Y. > > * All three are disabled and clk-X is current parent. > > * Thread A: clk_set_parent(clk-A, clk-Y). > > * Thread A: > > * Thread A: Grabs enable lock. > > * Thread A: Sees enable count of clk-A is 0, so doesn't enable clk-Y. > > * Thread A: Releases enable lock. > > * Thread B: Calls clk_enable(clk-A), which in turn enables clk-X. > > * Thread A: Switches clk-A's parent to clk-Y in hardware. > > > > clk-A is now enabled in software, but not clocking in hardware. > > > > Race condition 2: > > * clk-A has two parents - clk-X and clk-Y. > > * All three are disabled and clk-X is current parent. > > * Thread A: clk_set_parent(clk-A, clk-Y). > > * Thread A: > > * Thread A: Switches parent in hardware to clk-Y. > > * Thread A: Grabs enable lock. > > * Thread A: Sees enable count of clk-A is 0, so doesn't disable clk-X. > > * Thread A: Releases enable lock. > > * Thread B: Calls clk_enable(clk-A) > > * Thread B: Software state still says parent is clk-X. > > * Thread B: So, enables clk-X and then itself. > > * Thread A: Updates parent in software state to clk-Y. > > This looks correct to me. Is there any usecase where enabling/disabling a clock would require sleeping but changing the parent would not? Cheers, Peter. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756546Ab2EVOAQ (ORCPT ); Tue, 22 May 2012 10:00:16 -0400 Received: from hqemgate04.nvidia.com ([216.228.121.35]:18892 "EHLO hqemgate04.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753047Ab2EVOAO (ORCPT ); Tue, 22 May 2012 10:00:14 -0400 X-PGP-Universal: processed; by hqnvupgp06.nvidia.com on Tue, 22 May 2012 06:58:45 -0700 Date: Tue, 22 May 2012 16:58:13 +0300 From: Peter De Schrijver To: Saravana Kannan CC: Mike Turquette , Arnd Bergman , "linux-arm-kernel@lists.infradead.org" , Paul Walmsley , Sascha Hauer , Mark Brown , Shawn Guo , Stephen Warren , Andrew Lunn , Russell King , Linus Walleij , Stephen Boyd , "linux-arm-msm@vger.kernel.org" , Magnus Damm , "linux-kernel@vger.kernel.org" , Rob Herring , Richard Zhao , Grant Likely , Deepak Saxena , Amit Kucheria , Jamie Iles , Jeremy Kerr , Thomas Gleixner Subject: Re: [PATCH] clk: Fix race conditions between clk_set_parent() and clk_enable() Message-ID: <20120522135813.GL8730@tbergstrom-lnx.Nvidia.com> References: <1336798797-8724-1-git-send-email-skannan@codeaurora.org> <4FB29E7C.7010606@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <4FB29E7C.7010606@codeaurora.org> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 15, 2012 at 08:20:44PM +0200, Saravana Kannan wrote: > On 05/11/2012 09:59 PM, Saravana Kannan wrote: > > Without this patch, the following race conditions are possible. > > > > Race condition 1: > > * clk-A has two parents - clk-X and clk-Y. > > * All three are disabled and clk-X is current parent. > > * Thread A: clk_set_parent(clk-A, clk-Y). > > * Thread A: > > * Thread A: Grabs enable lock. > > * Thread A: Sees enable count of clk-A is 0, so doesn't enable clk-Y. > > * Thread A: Releases enable lock. > > * Thread B: Calls clk_enable(clk-A), which in turn enables clk-X. > > * Thread A: Switches clk-A's parent to clk-Y in hardware. > > > > clk-A is now enabled in software, but not clocking in hardware. > > > > Race condition 2: > > * clk-A has two parents - clk-X and clk-Y. > > * All three are disabled and clk-X is current parent. > > * Thread A: clk_set_parent(clk-A, clk-Y). > > * Thread A: > > * Thread A: Switches parent in hardware to clk-Y. > > * Thread A: Grabs enable lock. > > * Thread A: Sees enable count of clk-A is 0, so doesn't disable clk-X. > > * Thread A: Releases enable lock. > > * Thread B: Calls clk_enable(clk-A) > > * Thread B: Software state still says parent is clk-X. > > * Thread B: So, enables clk-X and then itself. > > * Thread A: Updates parent in software state to clk-Y. > > This looks correct to me. Is there any usecase where enabling/disabling a clock would require sleeping but changing the parent would not? Cheers, Peter.