From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754045AbdKFRlf (ORCPT ); Mon, 6 Nov 2017 12:41:35 -0500 Received: from foss.arm.com ([217.140.101.70]:38268 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753993AbdKFRld (ORCPT ); Mon, 6 Nov 2017 12:41:33 -0500 Date: Mon, 6 Nov 2017 17:41:29 +0000 From: Dave Martin To: Arnd Bergmann Cc: Catalin Marinas , Will Deacon , "Luis R. Rodriguez" , Kees Cook , Alex =?iso-8859-1?Q?Benn=E9e?= , "Eric W. Biederman" , Andrew Morton , Thomas Gleixner , "linux-kernel@vger.kernel.org" , "linux-fsdevel@vger.kernel.org" Subject: Re: [PATCH] sysctl: add register_sysctl() dummy helper Message-ID: <20171106174128.GW19485@e103592.cambridge.arm.com> References: <20171106133700.558647-1-arnd@arndb.de> <20171106165021.GV19485@e103592.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 Mon, Nov 06, 2017 at 05:12:03PM +0000, Arnd Bergmann wrote: > On Mon, Nov 6, 2017 at 5:50 PM, Dave Martin wrote: > > On Mon, Nov 06, 2017 at 01:36:40PM +0000, Arnd Bergmann wrote: > >> The register_sysctl() function has been around for five years with commit > >> fea478d4101a ("sysctl: Add register_sysctl for normal sysctl users") > >> but now that arm64 started using it, I ran into a compile error: > >> > >> arch/arm64/kernel/armv8_deprecated.c: In function 'register_insn_emulation_sysctl': > >> arch/arm64/kernel/armv8_deprecated.c:257:2: error: implicit declaration of function 'register_sysctl' > > > > Hmmm, looks like I missed this combination in testing. > > > > I wonder whether ARMV8_DEPRECATED without SYSCTL is really a good idea > > though: in that config, we build a lot of dead code and leak some > > memory today. The default emulation is still potentially useful, but > > all the support for runtime twiddling of the emulation modes becomes > > useless. > > > > For parallel reasons, the SVE sysctl stuff is protected by #ifdef > > CONFIG_SYSCTL, which is why I didn't get a similar splat there. > > > > > > So, although this patch is superficially sensible, it may tend to hide > > bugs: code that calls register_sysctl() when CONFIG_SYSCTL=n is > > suspicious and probably needs review... no? > > I think your analysis for this code is correct, we waste a lot of memory > if we do it like this. However in the general case where we register a > statically allocated 'struct ctl_table', it wouldn't be an issue, because > gcc could then eliminate all the dead code. > > Adding a CONFIG_SYSCTL #ifdef or Kconfig dependency would > probably be reasonable for armv8_deprecated.c, for the rest of > the kernel, having that wrapper is probably better. > > I don't really care how this gets fixed, as long as some solution gets > merged. Well, since your patch makes things more consistent, I'm happy to Reviewed-by: Dave Martin The armv8_deprecated code looks suspicious in any case, so I'll propose a fix for that separately -- adding a Kconfig dependency seems simplest. Cheers ---Dave