| Message ID | 1494856763-6543-3-git-send-email-aisheng.dong@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 05/15, 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. > > [ 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] [<c010c4bc>] (dump_backtrace) from [<c010c764>] (show_stack+0x18/0x1c) > [ 0.000000] r6:600000d3 r5:00000000 r4:c0e26358 r3:00000000 > [ 0.000000] [<c010c74c>] (show_stack) from [<c040599c>] (dump_stack+0xb4/0xe8) > [ 0.000000] [<c04058e8>] (dump_stack) from [<c0125c94>] (__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] [<c0125bbc>] (__warn) from [<c0125d74>] (warn_slowpath_null+0x28/0x30) > [ 0.000000] r9:00000000 r8:ef00bf80 r7:c165ac4c r6:ef00bf80 r5:ef00bf80 r4:ef00bf80 > [ 0.000000] [<c0125d4c>] (warn_slowpath_null) from [<c048aa78>] (clk_core_disable+0xb4/0xe0) > [ 0.000000] [<c048a9c4>] (clk_core_disable) from [<c048be88>] (clk_core_disable_lock+0x20/0x2c) > [ 0.000000] r4:000000d3 r3:c0e0af00 > [ 0.000000] [<c048be68>] (clk_core_disable_lock) from [<c048c224>] (clk_core_disable_unprepare+0x14/0x28) > [ 0.000000] r5:00000000 r4:ef00bf80 > [ 0.000000] [<c048c210>] (clk_core_disable_unprepare) from [<c048c270>] (__clk_set_parent_after+0x38/0x54) > [ 0.000000] r4:ef00bd80 r3:000010a0 > [ 0.000000] [<c048c238>] (__clk_set_parent_after) from [<c048daa8>] (clk_register+0x4d0/0x648) > [ 0.000000] r6:ef00d500 r5:ef00bf80 r4:ef00bd80 r3:ef00bfd4 > [ 0.000000] [<c048d5d8>] (clk_register) from [<c048dc30>] (clk_hw_register+0x10/0x1c) > [ 0.000000] r9:00000000 r8:00000003 r7:00000000 r6:00000824 r5:00000001 r4:ef00d500 > [ 0.000000] [<c048dc20>] (clk_hw_register) from [<c048e698>] (_register_divider+0xcc/0x120) > [ 0.000000] [<c048e5cc>] (_register_divider) from [<c048e730>] (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] [<c048e6ec>] (clk_register_divider) from [<c0d3ff58>] (imx7ulp_clocks_init+0x558/0xe98) > [ 0.000000] r7:c0e296f8 r6:c165c808 r5:00000000 r4:c165c808 > [ 0.000000] [<c0d3fa00>] (imx7ulp_clocks_init) from [<c0d24db0>] (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] [<c0d24c98>] (of_clk_init) from [<c0d04a50>] (time_init+0x2c/0x38) > [ 0.000000] r10:efffed40 r9:c0d61a48 r8:c0e78000 r7:c0e07900 r6:ffffffff r5:c0e78000 > [ 0.000000] r4:00000000 > [ 0.000000] [<c0d04a24>] (time_init) from [<c0d00b8c>] (start_kernel+0x218/0x394) > [ 0.000000] [<c0d00974>] (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 ]--- The warning is long and scary and spans many lines, but the scenario is described in one sentence. Please further describe the scenario that introduces this warning without requiring the reviewer to figure it out themselves! I certainly won't remember 2 years from now what happened here. > > Cc: Stephen Boyd <sboyd@codeaurora.org> > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Shawn Guo <shawnguo@kernel.org> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> This would want a fixes tag assuming it's fixing something.
On Mon, Jun 19, 2017 at 06:51:40PM -0700, Stephen Boyd wrote: > On 05/15, 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. > > > > [ 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] [<c010c4bc>] (dump_backtrace) from [<c010c764>] (show_stack+0x18/0x1c) > > [ 0.000000] r6:600000d3 r5:00000000 r4:c0e26358 r3:00000000 > > [ 0.000000] [<c010c74c>] (show_stack) from [<c040599c>] (dump_stack+0xb4/0xe8) > > [ 0.000000] [<c04058e8>] (dump_stack) from [<c0125c94>] (__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] [<c0125bbc>] (__warn) from [<c0125d74>] (warn_slowpath_null+0x28/0x30) > > [ 0.000000] r9:00000000 r8:ef00bf80 r7:c165ac4c r6:ef00bf80 r5:ef00bf80 r4:ef00bf80 > > [ 0.000000] [<c0125d4c>] (warn_slowpath_null) from [<c048aa78>] (clk_core_disable+0xb4/0xe0) > > [ 0.000000] [<c048a9c4>] (clk_core_disable) from [<c048be88>] (clk_core_disable_lock+0x20/0x2c) > > [ 0.000000] r4:000000d3 r3:c0e0af00 > > [ 0.000000] [<c048be68>] (clk_core_disable_lock) from [<c048c224>] (clk_core_disable_unprepare+0x14/0x28) > > [ 0.000000] r5:00000000 r4:ef00bf80 > > [ 0.000000] [<c048c210>] (clk_core_disable_unprepare) from [<c048c270>] (__clk_set_parent_after+0x38/0x54) > > [ 0.000000] r4:ef00bd80 r3:000010a0 > > [ 0.000000] [<c048c238>] (__clk_set_parent_after) from [<c048daa8>] (clk_register+0x4d0/0x648) > > [ 0.000000] r6:ef00d500 r5:ef00bf80 r4:ef00bd80 r3:ef00bfd4 > > [ 0.000000] [<c048d5d8>] (clk_register) from [<c048dc30>] (clk_hw_register+0x10/0x1c) > > [ 0.000000] r9:00000000 r8:00000003 r7:00000000 r6:00000824 r5:00000001 r4:ef00d500 > > [ 0.000000] [<c048dc20>] (clk_hw_register) from [<c048e698>] (_register_divider+0xcc/0x120) > > [ 0.000000] [<c048e5cc>] (_register_divider) from [<c048e730>] (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] [<c048e6ec>] (clk_register_divider) from [<c0d3ff58>] (imx7ulp_clocks_init+0x558/0xe98) > > [ 0.000000] r7:c0e296f8 r6:c165c808 r5:00000000 r4:c165c808 > > [ 0.000000] [<c0d3fa00>] (imx7ulp_clocks_init) from [<c0d24db0>] (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] [<c0d24c98>] (of_clk_init) from [<c0d04a50>] (time_init+0x2c/0x38) > > [ 0.000000] r10:efffed40 r9:c0d61a48 r8:c0e78000 r7:c0e07900 r6:ffffffff r5:c0e78000 > > [ 0.000000] r4:00000000 > > [ 0.000000] [<c0d04a24>] (time_init) from [<c0d00b8c>] (start_kernel+0x218/0x394) > > [ 0.000000] [<c0d00974>] (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 ]--- > > The warning is long and scary and spans many lines, but the > scenario is described in one sentence. Please further describe > the scenario that introduces this warning without requiring the > reviewer to figure it out themselves! I certainly won't remember > 2 years from now what happened here. > Yes, my fault. I will add more descriptions in next version. Below is the draft explaination. This issue actually is caused by that in current code, orphan clocks re-parent operation is before the critical clock enable operation. Before the critical clock is enabled, it may has a chance to be disabled already in orphan re-parent operation. Assuming we have two clocks: A and B while B is A's parent. 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 find B parent and do reparent, then the parent B may be disabled in __clk_set_parent_after() due to CLK_OPS_PARENT_ENABLE flag. 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); /* * 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); __clk_recalc_accuracies(orphan); __clk_recalc_rates(orphan, 0); } } .... if (core->flags & CLK_IS_CRITICAL) { unsigned long flags; clk_core_prepare(core); flags = clk_enable_lock(); clk_core_enable(core); clk_enable_unlock(flags); } ... } > > > > Cc: Stephen Boyd <sboyd@codeaurora.org> > > Cc: Michael Turquette <mturquette@baylibre.com> > > Cc: Shawn Guo <shawnguo@kernel.org> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > This would want a fixes tag assuming it's fixing something. > Yes, then it should be commit fc8726a2c021 clk: core: support clocks which requires parents enable (part 2) Did not consider that situation. Will add later. Regards Dong Aisheng > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > -- > To unsubscribe from this list: send the line "unsubscribe linux-clk" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index fc58c52..e2955b1 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2471,25 +2471,6 @@ static int __clk_core_init(struct clk_core *core) core->rate = core->req_rate = rate; /* - * walk the list of orphan clocks and reparent any that newly finds a - * parent. - */ - hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) { - struct clk_core *parent = __clk_init_parent(orphan); - - /* - * 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); - __clk_recalc_accuracies(orphan); - __clk_recalc_rates(orphan, 0); - } - } - - /* * optional platform-specific magic * * The .init callback is not used by any of the basic clock types, but @@ -2511,6 +2492,26 @@ static int __clk_core_init(struct clk_core *core) } kref_init(&core->ref); + + /* + * walk the list of orphan clocks and reparent any that newly finds a + * parent. + */ + hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) { + struct clk_core *parent = __clk_init_parent(orphan); + + /* + * 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); + __clk_recalc_accuracies(orphan); + __clk_recalc_rates(orphan, 0); + } + } + out: clk_prepare_unlock();
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. [ 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] [<c010c4bc>] (dump_backtrace) from [<c010c764>] (show_stack+0x18/0x1c) [ 0.000000] r6:600000d3 r5:00000000 r4:c0e26358 r3:00000000 [ 0.000000] [<c010c74c>] (show_stack) from [<c040599c>] (dump_stack+0xb4/0xe8) [ 0.000000] [<c04058e8>] (dump_stack) from [<c0125c94>] (__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] [<c0125bbc>] (__warn) from [<c0125d74>] (warn_slowpath_null+0x28/0x30) [ 0.000000] r9:00000000 r8:ef00bf80 r7:c165ac4c r6:ef00bf80 r5:ef00bf80 r4:ef00bf80 [ 0.000000] [<c0125d4c>] (warn_slowpath_null) from [<c048aa78>] (clk_core_disable+0xb4/0xe0) [ 0.000000] [<c048a9c4>] (clk_core_disable) from [<c048be88>] (clk_core_disable_lock+0x20/0x2c) [ 0.000000] r4:000000d3 r3:c0e0af00 [ 0.000000] [<c048be68>] (clk_core_disable_lock) from [<c048c224>] (clk_core_disable_unprepare+0x14/0x28) [ 0.000000] r5:00000000 r4:ef00bf80 [ 0.000000] [<c048c210>] (clk_core_disable_unprepare) from [<c048c270>] (__clk_set_parent_after+0x38/0x54) [ 0.000000] r4:ef00bd80 r3:000010a0 [ 0.000000] [<c048c238>] (__clk_set_parent_after) from [<c048daa8>] (clk_register+0x4d0/0x648) [ 0.000000] r6:ef00d500 r5:ef00bf80 r4:ef00bd80 r3:ef00bfd4 [ 0.000000] [<c048d5d8>] (clk_register) from [<c048dc30>] (clk_hw_register+0x10/0x1c) [ 0.000000] r9:00000000 r8:00000003 r7:00000000 r6:00000824 r5:00000001 r4:ef00d500 [ 0.000000] [<c048dc20>] (clk_hw_register) from [<c048e698>] (_register_divider+0xcc/0x120) [ 0.000000] [<c048e5cc>] (_register_divider) from [<c048e730>] (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] [<c048e6ec>] (clk_register_divider) from [<c0d3ff58>] (imx7ulp_clocks_init+0x558/0xe98) [ 0.000000] r7:c0e296f8 r6:c165c808 r5:00000000 r4:c165c808 [ 0.000000] [<c0d3fa00>] (imx7ulp_clocks_init) from [<c0d24db0>] (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] [<c0d24c98>] (of_clk_init) from [<c0d04a50>] (time_init+0x2c/0x38) [ 0.000000] r10:efffed40 r9:c0d61a48 r8:c0e78000 r7:c0e07900 r6:ffffffff r5:c0e78000 [ 0.000000] r4:00000000 [ 0.000000] [<c0d04a24>] (time_init) from [<c0d00b8c>] (start_kernel+0x218/0x394) [ 0.000000] [<c0d00974>] (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 ]--- Cc: Stephen Boyd <sboyd@codeaurora.org> Cc: Michael Turquette <mturquette@baylibre.com> Cc: Shawn Guo <shawnguo@kernel.org> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- drivers/clk/clk.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-)