Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754799AbcDNOHG (ORCPT ); Thu, 14 Apr 2016 10:07:06 -0400 Received: from mx2.suse.de ([195.135.220.15]:38288 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750820AbcDNOHF (ORCPT ); Thu, 14 Apr 2016 10:07:05 -0400 Date: Thu, 14 Apr 2016 16:06:59 +0200 (CEST) From: Miroslav Benes To: Michael Ellerman cc: linuxppc-dev@ozlabs.org, bsingharora@gmail.com, duwe@lst.de, linux-kernel@vger.kernel.org, rostedt@goodmis.org, kamalesh@linux.vnet.ibm.com, pmladek@suse.com, jeyu@redhat.com, jikos@kernel.org, live-patching@vger.kernel.org Subject: Re: [PATCH 2/5] livepatch: Allow architectures to specify an alternate ftrace location In-Reply-To: <1460639178.21066.3.camel@ellerman.id.au> Message-ID: References: <1460552003-15409-1-git-send-email-mpe@ellerman.id.au> <1460552003-15409-3-git-send-email-mpe@ellerman.id.au> <1460639178.21066.3.camel@ellerman.id.au> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1385 Lines: 39 On Thu, 14 Apr 2016, Michael Ellerman wrote: > On Thu, 2016-04-14 at 14:01 +0200, Miroslav Benes wrote: > > On Wed, 13 Apr 2016, Michael Ellerman wrote: > > > > static void klp_disable_func(struct klp_func *func) > > > { > > > struct klp_ops *ops; > > > @@ -312,8 +325,14 @@ static void klp_disable_func(struct klp_func *func) > > > return; > > > > > > if (list_is_singular(&ops->func_stack)) { > > > + unsigned long ftrace_loc; > > > > This is a nit, but could you move the definition up to have them all in > > one place to be consistent with the rest of the code? The same applies to > > klp_enable_func() below. > > Hmm, actually I moved it in there because you pointed out we only needed it > inside the if: > > http://lkml.kernel.org/r/alpine.LNX.2.00.1603151113020.20252@pobox.suse.cz > > Thinking about it, we need ftrace_loc only in cases where we call > ftrace_set_filter_ip() right? So we can move klp_get_ftrace_location() > call to appropriate if branch both in klp_disable_func() and > klp_enable_func(). > > But I guess you meant the function call, not the variable declaration. Exactly. > Personally I think it's better this way, as the variable is in scope for the > shortest possible amount of time, but I can change it if you want me to. No, it is nothing I would insist on. Thanks, Miroslav