| Message ID | 1525942205-111855-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Shawn Lin (2018-05-10 01:50:05) > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 3487be7..7bbdb6f 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -519,6 +519,9 @@ > debug and development, but should not be needed on a > platform with proper driver support. For more > information, see Documentation/clk.txt. > + clk_panic_dump > + [CLK] > + Dump the whole clock tree-view upon panic. Typically when the system panics it's because something has gone wrong. When a clk has gone wrong, typically the system hangs or crashes hard and printing out panic info just doesn't happen. I'm not sure why clks are so special here that we need to dump the information about the clk tree at the point of a panic. Is this more of a debug patch to dump out clk tree info by calling panic() in certain places near where you get odd system hangs? > > clock= [BUGS=X86-32, HW] gettimeofday clocksource override. > [Deprecated] > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 9ae92aa..adb5537 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -2802,6 +2802,73 @@ static inline void clk_debug_unregister(struct clk_core *core) > } > #endif > > +static bool clk_pdump_enable; > +static int __init clk_pdump_setup(char *__unused) > +{ > + clk_pdump_enable = true; > + return 1; > +} > +__setup("clk_panic_dump", clk_pdump_setup); > + > +static void clk_pdump_show_one(struct clk_core *c, int level) > +{ > + if (!c) > + return; > + > + pr_err("%*s%-*s %11d %12d %11lu %10lu %-3d\n", > + level * 3 + 1, "", 30 - level * 3, c->name, > + c->enable_count, c->prepare_count, clk_core_get_rate(c), > + clk_core_get_accuracy(c), clk_core_get_phase(c)); > +} > + > +static void clk_pdump_show_subtree(struct clk_core *c, int level) > +{ > + struct clk_core *child; > + > + if (!c) > + return; > + > + clk_pdump_show_one(c, level); > + > + hlist_for_each_entry(child, &c->children, child_node) > + clk_pdump_show_subtree(child, level + 1); > +} > + > +static int clk_panic_dump(struct notifier_block *this, unsigned long ev, > + void *ptr) > +{ > + struct clk_core *c; > + > + if (!clk_pdump_enable) > + return 0; Why register the notifier in late init if the commandline doesn't have the parameter set? > + > + pr_err("clock panic dump:\n"); > + pr_err(" clock enable_cnt prepare_cnt rate accuracy phase\n"); > + pr_err("----------------------------------------------------------------------------------------\n"); > + > + clk_prepare_lock(); Why are we taking locks in the panic path? > + > + hlist_for_each_entry(c, &clk_root_list, child_node) > + clk_pdump_show_subtree(c, 0); > + hlist_for_each_entry(c, &clk_orphan_list, child_node) > + clk_pdump_show_subtree(c, 0); > + > + clk_prepare_unlock(); > + > + return 0; > +} > + > +static struct notifier_block clk_pdump_block = { > + .notifier_call = clk_panic_dump, > +}; > + > +static int clk_register_pdump(void) > +{ > + atomic_notifier_chain_register(&panic_notifier_list, &clk_pdump_block); > + return 0; > +} > +late_initcall_sync(clk_register_pdump); Why sync?
Hi Stephen, On 2018/5/16 16:09, Stephen Boyd wrote: > Quoting Shawn Lin (2018-05-10 01:50:05) >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >> index 3487be7..7bbdb6f 100644 >> --- a/Documentation/admin-guide/kernel-parameters.txt >> +++ b/Documentation/admin-guide/kernel-parameters.txt >> @@ -519,6 +519,9 @@ >> debug and development, but should not be needed on a >> platform with proper driver support. For more >> information, see Documentation/clk.txt. >> + clk_panic_dump >> + [CLK] >> + Dump the whole clock tree-view upon panic. > > Typically when the system panics it's because something has gone wrong. > When a clk has gone wrong, typically the system hangs or crashes hard > and printing out panic info just doesn't happen. > > I'm not sure why clks are so special here that we need to dump the > information about the clk tree at the point of a panic. Is this more of > a debug patch to dump out clk tree info by calling panic() in certain > places near where you get odd system hangs? Yes, it's a debug patch when system in a odd hangds/hard locked. If it crashes seriously, anything wrt. panic info is unusable, but if the system still has a chance to dump panic info, for instance stack trace, it would also be able to dump the clk tree. Btw, I should marked this patch as RFC but forgot it. So if this patch is acceptable from your point, if I address the your comment about the code? > >> >> clock= [BUGS=X86-32, HW] gettimeofday clocksource override. >> [Deprecated] >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index 9ae92aa..adb5537 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -2802,6 +2802,73 @@ static inline void clk_debug_unregister(struct clk_core *core) >> } >> #endif >> >> +static bool clk_pdump_enable; >> +static int __init clk_pdump_setup(char *__unused) >> +{ >> + clk_pdump_enable = true; >> + return 1; >> +} >> +__setup("clk_panic_dump", clk_pdump_setup); >> + >> +static void clk_pdump_show_one(struct clk_core *c, int level) >> +{ >> + if (!c) >> + return; >> + >> + pr_err("%*s%-*s %11d %12d %11lu %10lu %-3d\n", >> + level * 3 + 1, "", 30 - level * 3, c->name, >> + c->enable_count, c->prepare_count, clk_core_get_rate(c), >> + clk_core_get_accuracy(c), clk_core_get_phase(c)); >> +} >> + >> +static void clk_pdump_show_subtree(struct clk_core *c, int level) >> +{ >> + struct clk_core *child; >> + >> + if (!c) >> + return; >> + >> + clk_pdump_show_one(c, level); >> + >> + hlist_for_each_entry(child, &c->children, child_node) >> + clk_pdump_show_subtree(child, level + 1); >> +} >> + >> +static int clk_panic_dump(struct notifier_block *this, unsigned long ev, >> + void *ptr) >> +{ >> + struct clk_core *c; >> + >> + if (!clk_pdump_enable) >> + return 0; > > Why register the notifier in late init if the commandline doesn't have > the parameter set? > Ok, will fix. >> + >> + pr_err("clock panic dump:\n"); >> + pr_err(" clock enable_cnt prepare_cnt rate accuracy phase\n"); >> + pr_err("----------------------------------------------------------------------------------------\n"); >> + >> + clk_prepare_lock(); > > Why are we taking locks in the panic path? ok, it's unnecessary in the special path indeed. > >> + >> + hlist_for_each_entry(c, &clk_root_list, child_node) >> + clk_pdump_show_subtree(c, 0); >> + hlist_for_each_entry(c, &clk_orphan_list, child_node) >> + clk_pdump_show_subtree(c, 0); >> + >> + clk_prepare_unlock(); >> + >> + return 0; >> +} >> + >> +static struct notifier_block clk_pdump_block = { >> + .notifier_call = clk_panic_dump, >> +}; >> + >> +static int clk_register_pdump(void) >> +{ >> + atomic_notifier_chain_register(&panic_notifier_list, &clk_pdump_block); >> + return 0; >> +} >> +late_initcall_sync(clk_register_pdump); > > Why sync? Oh, sync is no needed. > > > >
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 3487be7..7bbdb6f 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -519,6 +519,9 @@ debug and development, but should not be needed on a platform with proper driver support. For more information, see Documentation/clk.txt. + clk_panic_dump + [CLK] + Dump the whole clock tree-view upon panic. clock= [BUGS=X86-32, HW] gettimeofday clocksource override. [Deprecated] diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 9ae92aa..adb5537 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2802,6 +2802,73 @@ static inline void clk_debug_unregister(struct clk_core *core) } #endif +static bool clk_pdump_enable; +static int __init clk_pdump_setup(char *__unused) +{ + clk_pdump_enable = true; + return 1; +} +__setup("clk_panic_dump", clk_pdump_setup); + +static void clk_pdump_show_one(struct clk_core *c, int level) +{ + if (!c) + return; + + pr_err("%*s%-*s %11d %12d %11lu %10lu %-3d\n", + level * 3 + 1, "", 30 - level * 3, c->name, + c->enable_count, c->prepare_count, clk_core_get_rate(c), + clk_core_get_accuracy(c), clk_core_get_phase(c)); +} + +static void clk_pdump_show_subtree(struct clk_core *c, int level) +{ + struct clk_core *child; + + if (!c) + return; + + clk_pdump_show_one(c, level); + + hlist_for_each_entry(child, &c->children, child_node) + clk_pdump_show_subtree(child, level + 1); +} + +static int clk_panic_dump(struct notifier_block *this, unsigned long ev, + void *ptr) +{ + struct clk_core *c; + + if (!clk_pdump_enable) + return 0; + + pr_err("clock panic dump:\n"); + pr_err(" clock enable_cnt prepare_cnt rate accuracy phase\n"); + pr_err("----------------------------------------------------------------------------------------\n"); + + clk_prepare_lock(); + + hlist_for_each_entry(c, &clk_root_list, child_node) + clk_pdump_show_subtree(c, 0); + hlist_for_each_entry(c, &clk_orphan_list, child_node) + clk_pdump_show_subtree(c, 0); + + clk_prepare_unlock(); + + return 0; +} + +static struct notifier_block clk_pdump_block = { + .notifier_call = clk_panic_dump, +}; + +static int clk_register_pdump(void) +{ + atomic_notifier_chain_register(&panic_notifier_list, &clk_pdump_block); + return 0; +} +late_initcall_sync(clk_register_pdump); + /** * __clk_core_init - initialize the data structures in a struct clk_core * @core: clk_core being initialized
Sometimes it's useful and for debugging to check the whole system clock configuration in tree-view upon panic. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- Documentation/admin-guide/kernel-parameters.txt | 3 ++ drivers/clk/clk.c | 67 +++++++++++++++++++++++++ 2 files changed, 70 insertions(+)