* [PATCH] clk: fix a panic error caused by accessing NULL pointer @ 2017-11-20 3:38 Chunyan Zhang 2017-11-20 19:12 ` Stephen Boyd 0 siblings, 1 reply; 6+ messages in thread From: Chunyan Zhang @ 2017-11-20 3:38 UTC (permalink / raw) To: Stephen Boyd, Michael Turquette Cc: linux-clk, linux-kernel, Cai Li, Orson Zhai, Chunyan Zhang From: Cai Li <cai.li@spreadtrum.com> In some cases the clock parent would be set NULL when doing re-parent, it will cause a NULL pointer accessing if clk_set trace event is enabled, since the trace event function would not check the input parameter. Signed-off-by: Cai Li <cai.li@spreadtrum.com> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com> --- drivers/clk/clk.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index c8d83ac..64efaf0 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1242,13 +1242,12 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent, old_parent = __clk_set_parent_before(core, parent); - trace_clk_set_parent(core, parent); - /* change clock input source */ - if (parent && core->ops->set_parent) + if (parent && core->ops->set_parent) { + trace_clk_set_parent(core, parent); ret = core->ops->set_parent(core->hw, p_index); - - trace_clk_set_parent_complete(core, parent); + trace_clk_set_parent_complete(core, parent); + } if (ret) { flags = clk_enable_lock(); -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] clk: fix a panic error caused by accessing NULL pointer 2017-11-20 3:38 [PATCH] clk: fix a panic error caused by accessing NULL pointer Chunyan Zhang @ 2017-11-20 19:12 ` Stephen Boyd 2017-11-21 8:57 ` Chunyan Zhang 0 siblings, 1 reply; 6+ messages in thread From: Stephen Boyd @ 2017-11-20 19:12 UTC (permalink / raw) To: Chunyan Zhang Cc: Michael Turquette, linux-clk, linux-kernel, Cai Li, Orson Zhai, Chunyan Zhang On 11/20, Chunyan Zhang wrote: > From: Cai Li <cai.li@spreadtrum.com> > > In some cases the clock parent would be set NULL when doing re-parent, > it will cause a NULL pointer accessing if clk_set trace event is enabled, > since the trace event function would not check the input parameter. > > Signed-off-by: Cai Li <cai.li@spreadtrum.com> > Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com> Fixes: tag? > --- > drivers/clk/clk.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index c8d83ac..64efaf0 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1242,13 +1242,12 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent, > > old_parent = __clk_set_parent_before(core, parent); > > - trace_clk_set_parent(core, parent); > - > /* change clock input source */ > - if (parent && core->ops->set_parent) > + if (parent && core->ops->set_parent) { > + trace_clk_set_parent(core, parent); > ret = core->ops->set_parent(core->hw, p_index); > - > - trace_clk_set_parent_complete(core, parent); > + trace_clk_set_parent_complete(core, parent); > + } Is the problem that parent may be NULL and the tracepoint dereferences it? Perhaps we need to update the tracepoint code instead so that we always see that the tracepoint is called even if we don't actually touch the hardware. Something like the patch below instead. ---8<---- diff --git a/include/trace/events/clk.h b/include/trace/events/clk.h index 758607226bfd..5a85ea2090c4 100644 --- a/include/trace/events/clk.h +++ b/include/trace/events/clk.h @@ -139,7 +139,7 @@ DECLARE_EVENT_CLASS(clk_parent, TP_fast_assign( __assign_str(name, core->name); - __assign_str(pname, parent->name); + __assign_str(pname, parent ? parent->name : NULL); ), TP_printk("%s %s", __get_str(name), __get_str(pname)) -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] clk: fix a panic error caused by accessing NULL pointer 2017-11-20 19:12 ` Stephen Boyd @ 2017-11-21 8:57 ` Chunyan Zhang 2017-11-21 9:21 ` Chunyan Zhang 0 siblings, 1 reply; 6+ messages in thread From: Chunyan Zhang @ 2017-11-21 8:57 UTC (permalink / raw) To: Stephen Boyd Cc: Chunyan Zhang, Michael Turquette, linux-clk, linux-kernel@vger.kernel.org, Cai Li, Orson Zhai On 21 November 2017 at 03:12, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 11/20, Chunyan Zhang wrote: >> From: Cai Li <cai.li@spreadtrum.com> >> >> In some cases the clock parent would be set NULL when doing re-parent, >> it will cause a NULL pointer accessing if clk_set trace event is enabled, >> since the trace event function would not check the input parameter. >> >> Signed-off-by: Cai Li <cai.li@spreadtrum.com> >> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com> > > Fixes: tag? > >> --- >> drivers/clk/clk.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index c8d83ac..64efaf0 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -1242,13 +1242,12 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent, >> >> old_parent = __clk_set_parent_before(core, parent); >> >> - trace_clk_set_parent(core, parent); >> - >> /* change clock input source */ >> - if (parent && core->ops->set_parent) >> + if (parent && core->ops->set_parent) { >> + trace_clk_set_parent(core, parent); >> ret = core->ops->set_parent(core->hw, p_index); >> - >> - trace_clk_set_parent_complete(core, parent); >> + trace_clk_set_parent_complete(core, parent); >> + } > > Is the problem that parent may be NULL and the tracepoint > dereferences it? Yes, I think that probably is uncommon usage though, it revealed that the tracepoint could be stronger :) > Perhaps we need to update the tracepoint code > instead so that we always see that the tracepoint is called even > if we don't actually touch the hardware. Something like the patch > below instead. Ok, we will cook a new patch according to your comments. Thanks, Chunyan > > ---8<---- > diff --git a/include/trace/events/clk.h b/include/trace/events/clk.h > index 758607226bfd..5a85ea2090c4 100644 > --- a/include/trace/events/clk.h > +++ b/include/trace/events/clk.h > @@ -139,7 +139,7 @@ DECLARE_EVENT_CLASS(clk_parent, > > TP_fast_assign( > __assign_str(name, core->name); > - __assign_str(pname, parent->name); > + __assign_str(pname, parent ? parent->name : NULL); > ), > > TP_printk("%s %s", __get_str(name), __get_str(pname)) > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] clk: fix a panic error caused by accessing NULL pointer 2017-11-21 8:57 ` Chunyan Zhang @ 2017-11-21 9:21 ` Chunyan Zhang 2017-12-05 23:28 ` Stephen Boyd 0 siblings, 1 reply; 6+ messages in thread From: Chunyan Zhang @ 2017-11-21 9:21 UTC (permalink / raw) To: Stephen Boyd Cc: Chunyan Zhang, Michael Turquette, linux-clk, linux-kernel@vger.kernel.org, Cai Li, Orson Zhai On 21 November 2017 at 16:57, Chunyan Zhang <zhang.lyra@gmail.com> wrote: > On 21 November 2017 at 03:12, Stephen Boyd <sboyd@codeaurora.org> wrote: >> On 11/20, Chunyan Zhang wrote: >>> From: Cai Li <cai.li@spreadtrum.com> >>> >>> In some cases the clock parent would be set NULL when doing re-parent, >>> it will cause a NULL pointer accessing if clk_set trace event is enabled, >>> since the trace event function would not check the input parameter. >>> >>> Signed-off-by: Cai Li <cai.li@spreadtrum.com> >>> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com> >> >> Fixes: tag? >> >>> --- >>> drivers/clk/clk.c | 9 ++++----- >>> 1 file changed, 4 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >>> index c8d83ac..64efaf0 100644 >>> --- a/drivers/clk/clk.c >>> +++ b/drivers/clk/clk.c >>> @@ -1242,13 +1242,12 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent, >>> >>> old_parent = __clk_set_parent_before(core, parent); >>> >>> - trace_clk_set_parent(core, parent); >>> - >>> /* change clock input source */ >>> - if (parent && core->ops->set_parent) >>> + if (parent && core->ops->set_parent) { >>> + trace_clk_set_parent(core, parent); >>> ret = core->ops->set_parent(core->hw, p_index); >>> - >>> - trace_clk_set_parent_complete(core, parent); >>> + trace_clk_set_parent_complete(core, parent); >>> + } >> >> Is the problem that parent may be NULL and the tracepoint >> dereferences it? > > Yes, I think that probably is uncommon usage though, it revealed that > the tracepoint could be stronger :) The reason we need to set the parent as NULL is to disable the clk for the purpose of saving power. > >> Perhaps we need to update the tracepoint code >> instead so that we always see that the tracepoint is called even >> if we don't actually touch the hardware. Something like the patch >> below instead. > > Ok, we will cook a new patch according to your comments. > > Thanks, > Chunyan > >> >> ---8<---- >> diff --git a/include/trace/events/clk.h b/include/trace/events/clk.h >> index 758607226bfd..5a85ea2090c4 100644 >> --- a/include/trace/events/clk.h >> +++ b/include/trace/events/clk.h >> @@ -139,7 +139,7 @@ DECLARE_EVENT_CLASS(clk_parent, >> >> TP_fast_assign( >> __assign_str(name, core->name); >> - __assign_str(pname, parent->name); >> + __assign_str(pname, parent ? parent->name : NULL); >> ), >> >> TP_printk("%s %s", __get_str(name), __get_str(pname)) >> -- >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, >> a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] clk: fix a panic error caused by accessing NULL pointer 2017-11-21 9:21 ` Chunyan Zhang @ 2017-12-05 23:28 ` Stephen Boyd 2017-12-06 7:38 ` Chunyan Zhang 0 siblings, 1 reply; 6+ messages in thread From: Stephen Boyd @ 2017-12-05 23:28 UTC (permalink / raw) To: Chunyan Zhang Cc: Chunyan Zhang, Michael Turquette, linux-clk, linux-kernel@vger.kernel.org, Cai Li, Orson Zhai On 11/21, Chunyan Zhang wrote: > On 21 November 2017 at 16:57, Chunyan Zhang <zhang.lyra@gmail.com> wrote: > > On 21 November 2017 at 03:12, Stephen Boyd <sboyd@codeaurora.org> wrote: > >> On 11/20, Chunyan Zhang wrote: > >>> From: Cai Li <cai.li@spreadtrum.com> > >>> > >>> In some cases the clock parent would be set NULL when doing re-parent, > >>> it will cause a NULL pointer accessing if clk_set trace event is enabled, > >>> since the trace event function would not check the input parameter. > >>> > >>> Signed-off-by: Cai Li <cai.li@spreadtrum.com> > >>> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com> > >> > >> Fixes: tag? > >> > >>> --- > >>> drivers/clk/clk.c | 9 ++++----- > >>> 1 file changed, 4 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > >>> index c8d83ac..64efaf0 100644 > >>> --- a/drivers/clk/clk.c > >>> +++ b/drivers/clk/clk.c > >>> @@ -1242,13 +1242,12 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent, > >>> > >>> old_parent = __clk_set_parent_before(core, parent); > >>> > >>> - trace_clk_set_parent(core, parent); > >>> - > >>> /* change clock input source */ > >>> - if (parent && core->ops->set_parent) > >>> + if (parent && core->ops->set_parent) { > >>> + trace_clk_set_parent(core, parent); > >>> ret = core->ops->set_parent(core->hw, p_index); > >>> - > >>> - trace_clk_set_parent_complete(core, parent); > >>> + trace_clk_set_parent_complete(core, parent); > >>> + } > >> > >> Is the problem that parent may be NULL and the tracepoint > >> dereferences it? > > > > Yes, I think that probably is uncommon usage though, it revealed that > > the tracepoint could be stronger :) > > The reason we need to set the parent as NULL is to disable the clk for > the purpose of saving power. Do you have drivers calling set_parent with NULL to save power? Seems sort of odd. Why not do something when disabling the clk in clk_disable() path instead? Either way, I'll apply the patch to clk-fixes. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] clk: fix a panic error caused by accessing NULL pointer 2017-12-05 23:28 ` Stephen Boyd @ 2017-12-06 7:38 ` Chunyan Zhang 0 siblings, 0 replies; 6+ messages in thread From: Chunyan Zhang @ 2017-12-06 7:38 UTC (permalink / raw) To: Stephen Boyd Cc: Chunyan Zhang, Michael Turquette, linux-clk, linux-kernel@vger.kernel.org, Cai Li, Orson Zhai, Ben Li, Xiaolong Zhang On 6 December 2017 at 07:28, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 11/21, Chunyan Zhang wrote: >> On 21 November 2017 at 16:57, Chunyan Zhang <zhang.lyra@gmail.com> wrote: >> > On 21 November 2017 at 03:12, Stephen Boyd <sboyd@codeaurora.org> wrote: >> >> On 11/20, Chunyan Zhang wrote: >> >>> From: Cai Li <cai.li@spreadtrum.com> >> >>> >> >>> In some cases the clock parent would be set NULL when doing re-parent, >> >>> it will cause a NULL pointer accessing if clk_set trace event is enabled, >> >>> since the trace event function would not check the input parameter. >> >>> >> >>> Signed-off-by: Cai Li <cai.li@spreadtrum.com> >> >>> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com> >> >> >> >> Fixes: tag? >> >> >> >>> --- >> >>> drivers/clk/clk.c | 9 ++++----- >> >>> 1 file changed, 4 insertions(+), 5 deletions(-) >> >>> >> >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> >>> index c8d83ac..64efaf0 100644 >> >>> --- a/drivers/clk/clk.c >> >>> +++ b/drivers/clk/clk.c >> >>> @@ -1242,13 +1242,12 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent, >> >>> >> >>> old_parent = __clk_set_parent_before(core, parent); >> >>> >> >>> - trace_clk_set_parent(core, parent); >> >>> - >> >>> /* change clock input source */ >> >>> - if (parent && core->ops->set_parent) >> >>> + if (parent && core->ops->set_parent) { >> >>> + trace_clk_set_parent(core, parent); >> >>> ret = core->ops->set_parent(core->hw, p_index); >> >>> - >> >>> - trace_clk_set_parent_complete(core, parent); >> >>> + trace_clk_set_parent_complete(core, parent); >> >>> + } >> >> >> >> Is the problem that parent may be NULL and the tracepoint >> >> dereferences it? >> > >> > Yes, I think that probably is uncommon usage though, it revealed that >> > the tracepoint could be stronger :) >> >> The reason we need to set the parent as NULL is to disable the clk for >> the purpose of saving power. > > Do you have drivers calling set_parent with NULL to save power? > Seems sort of odd. Why not do something when disabling the clk in It's a mux clk in the same address area with a device, when powerring off the device module, the registers of this mux are reset with default value by hardware. When the device resumed afterward, the mux clock needs to recover its previous clk parent, but the clk core would find it is setting a same clk parent, the function clk_core_set_parent() [1] will return directly without writing the exact register to set the parent again. So setting the clk parent with NULL is just a workaround, we will appreciate if you have other better solution for us. Thanks, Chunyan [1] http://elixir.free-electrons.com/linux/v4.15-rc1/source/drivers/clk/clk.c#L1886 > clk_disable() path instead? Either way, I'll apply the patch to > clk-fixes. > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-06 7:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-11-20 3:38 [PATCH] clk: fix a panic error caused by accessing NULL pointer Chunyan Zhang 2017-11-20 19:12 ` Stephen Boyd 2017-11-21 8:57 ` Chunyan Zhang 2017-11-21 9:21 ` Chunyan Zhang 2017-12-05 23:28 ` Stephen Boyd 2017-12-06 7:38 ` Chunyan Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox