From patchwork Thu Nov 2 07:36:09 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Boyd X-Patchwork-Id: 10038079 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id C15CF6032D for ; Thu, 2 Nov 2017 07:36:50 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B1C9C28E26 for ; Thu, 2 Nov 2017 07:36:50 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A654D28E2B; Thu, 2 Nov 2017 07:36:50 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id B890A28E26 for ; Thu, 2 Nov 2017 07:36:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=6eRwFu/rXzGiX9lcM18vePncTJ25gr36366ITDWgNaI=; b=QSKIV1JGOILJVN /y/CkGGUa0FRy9BBxW2JI6LlO9jmFwuUkFRzHaWpfhs7eCOsGN4xj8yxSlAlKz4B3Ncluj1bi7CRd ODTS9VY9HE5N9n8NFkQ8JwK763mseODaXFrS/Ej4A9ulFm+TeOt4p3Jlhw5Ih010LQnLHdcQSF0pV oc+o8fmGjATiFJe4JPywYhugGlG7cnWjRk++IKJI2tiQ/Cm82yh/LJqZkTegNcd+BO/boL6ewWDsg nLy4NYGt0sEAGjwbosdLX1f7badxFckQaoiGoctSD7IdDzAIP6/O/jOG9b4SQ/ysMfx1IUKJ3J5Nu zkB+r5jLFYcNUpKdsJcw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1eAA3U-0001jD-Pt; Thu, 02 Nov 2017 07:36:48 +0000 Received: from smtp.codeaurora.org ([198.145.29.96]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1eAA3S-0001dX-0R for linux-arm-kernel@lists.infradead.org; Thu, 02 Nov 2017 07:36:47 +0000 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id 9D8256079E; Thu, 2 Nov 2017 07:36:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1509608185; bh=m9LRiZK67zhMqnMTaqKRQXxToub1TluHx8fDOlaRE38=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ABNdLB7GLIzX/f9VLfe/S+XXKDdTk45QXpF+BgXrMDLHjsEfAfM+sTohyKgJ9G8Vi CGYKMn716dGR892Kgz6ucvViqSeH01XoZM1DuUf/L6GzU7j2cjjY8p0zHA/PeB7FKw x/0e4vg9jeYeNMF5bMTWz6EaNHEAkdGquoSAAGrQ= Received: from localhost (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 7AE496079E; Thu, 2 Nov 2017 07:36:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1509608184; bh=m9LRiZK67zhMqnMTaqKRQXxToub1TluHx8fDOlaRE38=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=LyoiHYLnYWa7wwGMwKirJTJM63/HmuS6f6Jwrwd4vKIfVUwtxIWadEJxy0WFiNay5 uW3oOfj+VYe7a9CTQGuIdWx+rgyfOUsLVw1IEyTW1IDWdhpXxWN97DKIM6rYvz6Oc6 RGfZE3lZk1Vln5XHw1ZdJ+7kLpf4UpbwdBRmmT/Y= DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 7AE496079E Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=sboyd@codeaurora.org Date: Thu, 2 Nov 2017 00:36:09 -0700 From: Stephen Boyd To: Dong Aisheng Subject: Re: [PATCH V2 02/10] clk: reparent orphans after critical clocks enabled Message-ID: <20171102073609.GT30645@codeaurora.org> References: <1499946435-7177-1-git-send-email-aisheng.dong@nxp.com> <1499946435-7177-3-git-send-email-aisheng.dong@nxp.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1499946435-7177-3-git-send-email-aisheng.dong@nxp.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20171102_003646_127184_54278682 X-CRM114-Status: GOOD ( 19.82 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: dongas86@gmail.com, Anson.Huang@nxp.com, mturquette@baylibre.com, linux-kernel@vger.kernel.org, shawnguo@kernel.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, ping.bai@nxp.com Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On 07/13, Dong Aisheng wrote: > The orphan clocks reparent operation should be moved after the critical > clocks enabled, otherwise it may get a chance to disable a newly > registered critical clock which triggers the following warning. > > Assuming we have two clocks: A and B, B is the parent of A. > Clock A has flag: CLK_OPS_PARENT_ENABLE > Clock B has flag: CLK_IS_CRITICAL > > Step 1: > Clock A is registered, then it becomes orphan. > > Step 2: > Clock B is registered. Before clock B reach the critical clock enable > operation, orphan A will find the newly registered parent B and do > reparent operation, then parent B will be finally disabled in > __clk_set_parent_after() due to CLK_OPS_PARENT_ENABLE flag as there's > still no users of B which will then trigger the following warning. > > [ 0.000000] WARNING: CPU: 0 PID: 0 at drivers/clk/clk.c:597 clk_core_disable+0xb4/0xe0 > [ 0.000000] Modules linked in: > [ 0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.11.0-rc1-00056-gdff1f66-dirty #1373 > [ 0.000000] Hardware name: Generic DT based system > [ 0.000000] Backtrace: > [ 0.000000] [] (dump_backtrace) from [] (show_stack+0x18/0x1c) > [ 0.000000] r6:600000d3 r5:00000000 r4:c0e26358 r3:00000000 > [ 0.000000] [] (show_stack) from [] (dump_stack+0xb4/0xe8) > [ 0.000000] [] (dump_stack) from [] (__warn+0xd8/0x104) > [ 0.000000] r10:c0c21cd0 r9:c048aa78 r8:00000255 r7:00000009 r6:c0c1cd90 r5:00000000 > [ 0.000000] r4:00000000 r3:c0e01d34 > [ 0.000000] [] (__warn) from [] (warn_slowpath_null+0x28/0x30) > [ 0.000000] r9:00000000 r8:ef00bf80 r7:c165ac4c r6:ef00bf80 r5:ef00bf80 r4:ef00bf80 > [ 0.000000] [] (warn_slowpath_null) from [] (clk_core_disable+0xb4/0xe0) > [ 0.000000] [] (clk_core_disable) from [] (clk_core_disable_lock+0x20/0x2c) > [ 0.000000] r4:000000d3 r3:c0e0af00 > [ 0.000000] [] (clk_core_disable_lock) from [] (clk_core_disable_unprepare+0x14/0x28) > [ 0.000000] r5:00000000 r4:ef00bf80 > [ 0.000000] [] (clk_core_disable_unprepare) from [] (__clk_set_parent_after+0x38/0x54) > [ 0.000000] r4:ef00bd80 r3:000010a0 > [ 0.000000] [] (__clk_set_parent_after) from [] (clk_register+0x4d0/0x648) > [ 0.000000] r6:ef00d500 r5:ef00bf80 r4:ef00bd80 r3:ef00bfd4 > [ 0.000000] [] (clk_register) from [] (clk_hw_register+0x10/0x1c) > [ 0.000000] r9:00000000 r8:00000003 r7:00000000 r6:00000824 r5:00000001 r4:ef00d500 > [ 0.000000] [] (clk_hw_register) from [] (_register_divider+0xcc/0x120) > [ 0.000000] [] (_register_divider) from [] (clk_register_divider+0x44/0x54) > [ 0.000000] r10:00000004 r9:00000003 r8:00000001 r7:00000000 r6:00000003 r5:00000001 > [ 0.000000] r4:f0810030 > [ 0.000000] [] (clk_register_divider) from [] (imx7ulp_clocks_init+0x558/0xe98) > [ 0.000000] r7:c0e296f8 r6:c165c808 r5:00000000 r4:c165c808 > [ 0.000000] [] (imx7ulp_clocks_init) from [] (of_clk_init+0x118/0x1e0) > [ 0.000000] r10:00000001 r9:c0e01f68 r8:00000000 r7:c0e01f60 r6:ef7f8974 r5:ef0035c0 > [ 0.000000] r4:00000006 > [ 0.000000] [] (of_clk_init) from [] (time_init+0x2c/0x38) > [ 0.000000] r10:efffed40 r9:c0d61a48 r8:c0e78000 r7:c0e07900 r6:ffffffff r5:c0e78000 > [ 0.000000] r4:00000000 > [ 0.000000] [] (time_init) from [] (start_kernel+0x218/0x394) > [ 0.000000] [] (start_kernel) from [<6000807c>] (0x6000807c) > [ 0.000000] r10:00000000 r9:410fc075 r8:6000406a r7:c0e0c930 r6:c0d61a44 r5:c0e07918 > [ 0.000000] r4:c0e78294 > [ 0.000000] ---[ end trace 0000000000000000 ]--- Please remove timestamps from logs unless they're important. > > Fixes: fc8726a2c021 ("clk: core: support clocks which requires parents enable (part 2)") > Cc: Stephen Boyd > Cc: Michael Turquette > Cc: Shawn Guo > Signed-off-by: Dong Aisheng > > --- > ChangeLog: > v1->v2: > * add more detailed commit messages Thanks for that. We shouldn't be touching the hardware during clk registration though, so something is wrong there. It seems that adding the flag to enable clks when touching their registers has exposed that we should just be doing the toggle of the bookeeping stuff underneath the enable lock here. We know that the clk isn't enabled with any sort of prepare_count here so we don't need to enable anything to prevent a race. And we're holding the prepare mutex so set_rate/set_parent can't race here either. Can you try this patch instead? ---8<---- diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index c8d83acda006..416d44cc772c 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2476,14 +2476,17 @@ static int __clk_core_init(struct clk_core *core) */ hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) { struct clk_core *parent = __clk_init_parent(orphan); + unsigned long flags; /* * we could call __clk_set_parent, but that would result in a * redundant call to the .set_rate op, if it exists */ if (parent) { - __clk_set_parent_before(orphan, parent); - __clk_set_parent_after(orphan, parent, NULL); + /* update the clk tree topology */ + flags = clk_enable_lock(); + clk_reparent(orphan, parent); + clk_enable_unlock(flags); __clk_recalc_accuracies(orphan); __clk_recalc_rates(orphan, 0); }