From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933570AbbEMJeb (ORCPT ); Wed, 13 May 2015 05:34:31 -0400 Received: from mail-bn1bon0131.outbound.protection.outlook.com ([157.56.111.131]:26170 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753889AbbEMJe0 (ORCPT ); Wed, 13 May 2015 05:34:26 -0400 Authentication-Results: spf=fail (sender IP is 192.88.168.50) smtp.mailfrom=freescale.com; freescale.mail.onmicrosoft.com; dkim=none (message not signed) header.d=none; Date: Wed, 13 May 2015 17:21:06 +0800 From: Dong Aisheng To: Stephen Boyd CC: Dong Aisheng , , , , , , , , , Subject: Re: [PATCH RFC v1 2/5] clk: add missing lock when call clk_core_enable in clk_set_parent Message-ID: <20150513092105.GB554@shlinux1.ap.freescale.net> References: <1429107999-24413-1-git-send-email-aisheng.dong@freescale.com> <1429107999-24413-3-git-send-email-aisheng.dong@freescale.com> <55427D83.3060900@codeaurora.org> <20150504083549.GC4082@shlinux1.ap.freescale.net> <20150507000154.GD21794@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20150507000154.GD21794@codeaurora.org> User-Agent: Mutt/1.5.20 (2009-06-14) X-EOPAttributedMessage: 0 X-Microsoft-Exchange-Diagnostics: 1;BY2FFO11FD050;1:Sp/BZU96R9K1+HfzNMIVp0WlEd4WnW0gLhaihtNW58h7Zb8111wGWPTIAZS7aeEYaytSGXyIgNmkTs4nqUVS0SPyRaYmK45qLp6+Tr1hZ/wdxFOYHhgM8ULcYgP8jFpgM/OszAgQ15wq37+a325Y9xM5p6XwGc+MPYJlsKQ3Ed6aX+PwfPzCwcXXRHdrF4Avijqri4OKK3n0H0SVVlIuwhKEWzazFnJmokM75qz8mskJmh+AOCtO/RftEyyT55onzvUJYdnUad3syh081ivQZ7McYD2Fm55zKt/Snz99rTuKwDh9EIbRfzZAqAN/OEZ/9poqsI744LLerwMCMsqRGw== X-Forefront-Antispam-Report: CIP:192.88.168.50;CTRY:US;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10019020)(6009001)(339900001)(24454002)(479174004)(51704005)(189002)(199003)(23726002)(85426001)(93886004)(92566002)(77096005)(77156002)(62966003)(47776003)(50466002)(6806004)(46406003)(2950100001)(106466001)(104016003)(46102003)(189998001)(87936001)(110136002)(5001960100002)(97756001)(107886002)(33656002)(4001350100001)(54356999)(50986999)(83506001)(76176999)(105606002)(19580395003)(19580405001)(217873001)(42262002)(4001430100001);DIR:OUT;SFP:1102;SCL:1;SRVR:BY1PR0301MB1205;H:tx30smr01.am.freescale.net;FPR:;SPF:Fail;MLV:sfv;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: 1;BY1PR0301MB1205;2:intJg/efNnvZIlUI5VFNANONVk/bFgojmeHz3S3wPBWuXAU3I6pwUz4Tg4oy1tbj;2:khmNvGJYMobnutIIt2HTBvyl4VJ5Q3pu2Mww8XJ8+WWsvq7EO40YHE56heK0Z408SHZtlaFih++trE9qoqpO+HFNHr4ahJQojKOF3qNTW1whjx9hr1npmEzqF3gKQCnaOTDyjqJw9ejSAKT754QYlrUMO8XV5pLwEB9zSPuFE7wXPP//7rxH9Isdt/hnTlhMzM++GkbTKHPcEJglFpCcZ6haosB2gYfY2t7L4FlZQPk=;6:GnDOViwAu1xsxEI1yl8s8VAS/3Gz+6nyPYuslOFDxz9k5b0ncva5uUTGEBAvEg6I0YcYHI2pf+Xd6w1O9KCSPbLpcUIq4UDnIHUaEDACLvsJCZBd7EUaRYg57fcqcITkSV1vjFisSdm5UwWCXRMrNf9OH0EwFBY73o2FKyDgJuoIV6dzZb5dtXaPVKnFJqBGCT5n1j2g9+E6WxAME6DSJ4Fx2o/5au2h/FvlgtuRVXxXihP/lcbsyEgSW31A3aI98n+/jYLKwFYhVkqzt26eJg2XS/3hrxCWOkV3sUrIX+r0990WHMi53LyGnkR5ZFk2wk+0Rd+7AR6ss6dcf5UGjQ==;3:P2m1q6VgQokb154+lYUytJl6rn8pViv3w1ZG4EJ1GxLjNtMD0bbnQRL1tZYk/K/oHW3/X/8uBMAo7UOgipPWJe0rm7q+DKWkoxTp+GaLdA6LgQr9Gbd4VHNOwViMEVt+esY+gGJDEL0mdZf1zGfvWIgqTltKMsqZu+GIpiyjMJsxl5ux4CL4C46OROk9ROz1V26lcwoHzRuEka7d7DebhRVuchwXP5iu7Sg2zGulplTxE6zZzyrqp7Ad8N9x7I1/qbxFIIIMM4fKWXl6c5Fouu1DrSOZun84Nn5jsoTZGsk= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BY1PR0301MB1205; X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(5005006)(3002001);SRVR:BY1PR0301MB1205;BCL:0;PCL:0;RULEID:;SRVR:BY1PR0301MB1205; X-Forefront-PRVS: 0575F81B58 X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;BY1PR0301MB1205;9:ayH6jsGNmQQx7gop/ncpgKBQg1Cj+iMIMMqi4vyF?= =?us-ascii?Q?vqEiCm43+CyDBA55ewVnmX4uSyy61Fhk7VHcumt0Eb4SZgemFonBs62Uawe7?= =?us-ascii?Q?nciS+wR5pzs2QvZ3IU96nfVoJXusWEvI4cJspTegoh5Fj0IjhjxKN+YuvXz9?= =?us-ascii?Q?JdbQcCXyoBCTVkLBYtiJFghueWi7apuzPEj4Lp606/YPpegun/4NGhW2YhPT?= =?us-ascii?Q?NadwM2CxSeXyRIfdU1+L7TTKTrhjPGZ66QzSLLD+6/vL6vRV1VLfrTg3dqjd?= =?us-ascii?Q?0JCifAbmc7BUy1IdTKJHCoThujCEmJmvcOo3UF8vQ7vTwuG+umVV92EaUDAn?= =?us-ascii?Q?BiyN/jzIAchMBrHPFz28XH4fJplu5GO17wAyqc+/ZNekKGcTp/lzYon1Ad3n?= =?us-ascii?Q?x3gp5oDu+Qs2E7PQi696CugdkBmel3MYzsxwmGzDxRSgQrhHmo2FBhOHN6If?= =?us-ascii?Q?vSxkzeDD1/DGv2R0hsrpEN+i/fXbaSQ+kky9JGIIhyu7dent2a5oeb4EvW9s?= =?us-ascii?Q?vWg0F3y1Hs1qksxtHt/+vy8VB3iY5kc2Oz+5TH0SREI7c4UCvCl6TJrNvZdY?= =?us-ascii?Q?fUh5oX8mMRyV1dMCeiTfLdH12OtgCihkW64HzUTOPBRKccFY9JjZP8Sc6gsw?= =?us-ascii?Q?m3Fsub5dSDbh4LhFeTEATKR6WXNaRLw+xEAMMV7CD46qajOrB+XLiwtUhyQ2?= =?us-ascii?Q?oWdGkwLxYhYq8byeUMSnD3jByI4doDVMX0zOypflV718jyER79+6FM9GArQF?= =?us-ascii?Q?gTjg9InxYoetICkgaTg8RJZufHRcAECwsLLumze1eZJNp0rf92uWDjQ0YGOb?= =?us-ascii?Q?erVCCjco17hG5NSMnv1qmgAnPxq3REIxe+RrGI31CzvHVvC1yLvHBWhU0MZ3?= =?us-ascii?Q?m93XunaBSLncFqCHEW/sih3CbkZZwmlFS4s0mWWUq3Y080SyFb+H4jQsDAEw?= =?us-ascii?Q?sqAKBtkU6f92NB4xIFKg5fdDRChGDCUfzcSDsf6OGEmlgnV/tazlo35QbRxe?= =?us-ascii?Q?JOac/4T82X8Auq5sTx1iomuSCVwEPkvO/pxZdvtk0qzqqyVL5+gD70jW//sa?= =?us-ascii?Q?UOubr48=3D?= X-Microsoft-Exchange-Diagnostics: 1;BY1PR0301MB1205;3:FJETTVmuo9PxusApAY+i26hyS6fs3Vcrtqn9zX28Cv7bP2bH8fR6NqlhjfknCj6biAWAYVV5pbA06NNmSNu62zbneLOtevTScnhGGeff5b+4VemZ3yTqasH0FgmvlRav4FljlZKvJEZLWmf09emtyw==;10:g9ZZAwbPTFMnb7HK+uOj/VxZgC82GKsWx5fOPFPsgogPafEIJb44FPHGZNQ1gfFlBcWpzNCi0nC/ct8KieGGT4bWKst8aw7pib5sj69kkt4=;6:pD0EFiNe+SZqWz4NPzZCvlj5D9s2OkudBCWwCjfpRrWIUdnJL4oWfTihl4aaD3+H X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 May 2015 09:34:20.7490 (UTC) X-MS-Exchange-CrossTenant-Id: 710a03f5-10f6-4d38-9ff4-a80b81da590d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=710a03f5-10f6-4d38-9ff4-a80b81da590d;Ip=[192.88.168.50];Helo=[tx30smr01.am.freescale.net] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR0301MB1205 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 06, 2015 at 05:01:54PM -0700, Stephen Boyd wrote: > On 05/04, Dong Aisheng wrote: > > On Thu, Apr 30, 2015 at 12:07:47PM -0700, Stephen Boyd wrote: > > > On 04/15/15 07:26, Dong Aisheng wrote: > > > > clk_core_enable is executed without &enable_clock in clk_set_parent function. > > > > Adding it to avoid potential race condition issue. > > > > > > > > Fixes: 035a61c314eb ("clk: Make clk API return per-user struct clk instances") > > > > Cc: Mike Turquette > > > > Cc: Stephen Boyd > > > > Signed-off-by: Dong Aisheng > > > > --- > > > > > > Can you please describe the race condition? From what I can tell there > > > is not a race condition here and we've gone around on this part of the > > > code before to fix any race conditions. > > > > > > > Do you mean we do not need to acquire enable lock when execute clk_core_enable > > in set_parent function? Can you help explain a bit more why? > > > > The clk doc looks to me says the enable lock should be held across calls to > > the .enable, .disable and .is_enabled operations. > > > > And before the commit > > 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), > > all the clk_enable/disable in set_parent() is executed with lock. > > > > A rough thinking of race condition is assuming Thread A calls > > clk_set_parent(x, y) while Thread B calls clk_enable(x), clock x is disabled > > but prepared initially, due to clk_core_enable in set_parent() is not > > executed with enable clock, the clk_core_enable may be reentrant during > > the locking time executed by B. > > Won't this be a race condition? > > > > Ah I see now. The commit text could say something like this: > > Before commit 035a61c314eb ("clk: Make clk API return per-user > struct clk instances") we acquired the enable_lock in > __clk_set_parent_{before,after}() by means of calling > clk_enable(). After commit 035a61c314eb we use clk_core_enable() > in place of the clk_enable(), and clk_core_enable() doesn't > acquire the enable_lock. This opens up a race condition between > clk_set_parent() and clk_enable(). > > I've replaced the commit text and applied it to clk-fixes. > Got it. Thanks for the change. Regards Dong Aisheng > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project