From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967153AbdAGQXO (ORCPT ); Sat, 7 Jan 2017 11:23:14 -0500 Received: from 92-243-34-74.adsl.nanet.at ([92.243.34.74]:46165 "EHLO mail.osadl.at" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1751114AbdAGQXC (ORCPT ); Sat, 7 Jan 2017 11:23:02 -0500 Date: Sat, 7 Jan 2017 16:22:32 +0000 From: Nicholas Mc Guire To: Dave Young Cc: Vivek Goyal , Nicholas Mc Guire , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, linux-kernel@vger.kernel.org, Baoquan He Subject: Re: [PATCH RFC V2] purgatory: fix up declarations Message-ID: <20170107162232.GA20320@osadl.at> References: <1482493387-30168-1-git-send-email-hofrat@osadl.org> <20170103153814.GC29807@redhat.com> <20170103163422.GB24542@osadl.at> <20170104061620.GA19466@dhcp-128-65.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170104061620.GA19466@dhcp-128-65.nay.redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 04, 2017 at 02:16:20PM +0800, Dave Young wrote: > Vivek, thanks for ccing me.. > > On 01/03/17 at 04:34pm, Nicholas Mc Guire wrote: > > On Tue, Jan 03, 2017 at 10:38:14AM -0500, Vivek Goyal wrote: > > > On Fri, Dec 23, 2016 at 12:43:07PM +0100, Nicholas Mc Guire wrote: > > > > Add the missing declarations of basic purgatory functions and variables > > > > used with kexec_purgatory_get_set_symbol() to allow a clean build. > > > > > > > > Fixes: commit 8fc5b4d4121c ("purgatory: core purgatory functionality") > > > > Signed-off-by: Nicholas Mc Guire > > > > --- > > > > > > > > V2: after kbuild test robot reported a build failure > > > > removed incorrect declaration of copy_backup_region which is static > > > > anyway. > > > > > > > > sparse complained about: > > > > CHECK arch/x86/purgatory/purgatory.c > > > > arch/x86/purgatory/purgatory.c:21:15: warning: symbol 'backup_dest' was not declared. Should it be static? > > > > arch/x86/purgatory/purgatory.c:22:15: warning: symbol 'backup_src' was not declared. Should it be static? > > > > arch/x86/purgatory/purgatory.c:23:15: warning: symbol 'backup_sz' was not declared. Should it be static? > > > > arch/x86/purgatory/purgatory.c:25:4: warning: symbol 'sha256_digest' was not declared. Should it be static? > > > > arch/x86/purgatory/purgatory.c:27:19: warning: symbol 'sha_regions' was not declared. Should it be static? > > > > arch/x86/purgatory/purgatory.c:42:5: warning: symbol 'verify_sha256_digest' was not declared. Should it be static? > > > > arch/x86/purgatory/purgatory.c:61:6: warning: symbol 'purgatory' was not declared. Should it be static? > > > > CC arch/x86/purgatory/purgatory.o > > > > > > > > Numerous sparse messages regarding functions not being declared, these > > > > functions are resolved via kexec_purgatory_get_set_symbol() and not > > > > directly called anywhere. > > > > > > Hi Nicholas, > > > > > > So purgatory() is a separate piece of small binary which does not link > > > against kernel. And we don't want these symbols static as kernel > > > obtains the values of these symbols and modifies binary in place on > > > the fly. I am assuming if we make them static, then we will lose this > > > capability to be able to read elf headers and be able to modify value > > > of these symbols. > > > > I donīt understand why this would be lost - the symbols are not being > > used by kernel code other than kexec code it self - in what way > > would declaring them extern change there handling ? > > kexec_purgatory_find_symbol is using the elf header to resolve the > > symbol location and declaring it extern should not change that in any > > way - am I overlooking something ? > > > > > > > > Now question is how to supress warnings from sparse. If just declaring > > > them extern in header file and including that header file in some other > > > .c file make the sparse warning go away, so be it. Atleast we need > > > to make explicit comment that this is being done just to take care > > > of sparse warning. > > > > > > I am not very happy with the solution though. In future it will make > > > people scratch their head that why are we including this header file > > > and why some symbols are being declared extern. So if there is another > > > way to tell sparse to not worry about it, would be even better. > > > > > > > The assumtion was that these changes would be side-effect free - if they are > > not then this is probably the wrong path to go - the intent is to remove > > the sparse warnings only. > > Another way is do not include the header file, but declare them in the c > file just for avoiding the sparse warnings with some comments to explain > it. > that would make sparse happy as you suggest but now checkpatch is fussing. ... WARNING: externs should be avoided in .c files #62: FILE: arch/x86/purgatory/purgatory.c:27: +extern unsigned long backup_src; WARNING: externs should be avoided in .c files #63: FILE: arch/x86/purgatory/purgatory.c:28: +extern unsigned long backup_sz; ... unfortunately it seems that both of these tools do not permit marking something as a false positive for this case (__force in sparse will not work here). At the same time I do think that would not be a very clean solution ither. So the alternative solution is to create arch/x86/purgatory.h and put it all into there - V3 containting that solution just sent out. thx! hofrat