Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753085AbdLEX2u (ORCPT ); Tue, 5 Dec 2017 18:28:50 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:54440 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753038AbdLEX2q (ORCPT ); Tue, 5 Dec 2017 18:28:46 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org A77956034D 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: Tue, 5 Dec 2017 15:28:44 -0800 From: Stephen Boyd To: Chunyan Zhang Cc: Chunyan Zhang , Michael Turquette , linux-clk , "linux-kernel@vger.kernel.org" , Cai Li , Orson Zhai Subject: Re: [PATCH] clk: fix a panic error caused by accessing NULL pointer Message-ID: <20171205232844.GB4283@codeaurora.org> References: <20171120033816.28414-1-chunyan.zhang@spreadtrum.com> <20171120191219.GD18379@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2198 Lines: 56 On 11/21, Chunyan Zhang wrote: > On 21 November 2017 at 16:57, Chunyan Zhang wrote: > > On 21 November 2017 at 03:12, Stephen Boyd wrote: > >> On 11/20, Chunyan Zhang wrote: > >>> From: Cai Li > >>> > >>> 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 > >>> Signed-off-by: Chunyan Zhang > >> > >> 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