From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752140AbbJQTAH (ORCPT ); Sat, 17 Oct 2015 15:00:07 -0400 Received: from h1446028.stratoserver.net ([85.214.92.142]:36599 "EHLO mail.ahsoftware.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751103AbbJQTAF (ORCPT ); Sat, 17 Oct 2015 15:00:05 -0400 Subject: Re: [PATCH 11/14] init: deps: annotate various initcalls To: Linus Torvalds References: <1445102067-11519-1-git-send-email-holler@ahsoftware.de> <1445102067-11519-12-git-send-email-holler@ahsoftware.de> Cc: Linux Kernel Mailing List , Andrew Morton , Greg Kroah-Hartman , Russell King , Grant Likely From: Alexander Holler Message-ID: <56229AA8.8050609@ahsoftware.de> Date: Sat, 17 Oct 2015 20:59:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Am 17.10.2015 um 20:47 schrieb Linus Torvalds: > On Sat, Oct 17, 2015 at 10:14 AM, Alexander Holler wrote: >> >> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c >> index 873dbfc..d5d2459 100644 >> --- a/arch/arm/common/edma.c >> +++ b/arch/arm/common/edma.c >> @@ -1872,5 +1872,4 @@ static int __init edma_init(void) >> { >> return platform_driver_probe(&edma_driver, edma_probe); >> } >> -arch_initcall(edma_init); >> - >> +annotated_initcall_drv(arch, edma_init, drvid_edma, NULL, edma_driver.driver); >> diff --git a/arch/arm/crypto/aes-ce-glue.c b/arch/arm/crypto/aes-ce-glue.c >> index b445a5d..d9bcb89 100644 >> --- a/arch/arm/crypto/aes-ce-glue.c >> +++ b/arch/arm/crypto/aes-ce-glue.c >> @@ -520,5 +520,10 @@ static void __exit aes_exit(void) >> crypto_unregister_algs(aes_algs, ARRAY_SIZE(aes_algs)); >> } >> >> -module_init(aes_init); >> +static const unsigned dependencies[] __initconst __maybe_unused = { >> + drvid_cryptomgr, >> + 0 >> +}; >> + >> +annotated_module_init(aes_init, drvid_aes_ce_arm, dependencies); >> module_exit(aes_exit); > > So I think this is kind of a sign of the same disease I mentioned > earlier: making dependencies "separate" from the init levels, now > means that you do the initialization of the dependencies *instead* of > the init level. And that smells bad and wrong, and causes this kind of > patch that is not only huge, but si unreadable and the end result > looks like crap too. > > We've actually been quite good at having the module attributes all be > *separate* things that work together. So the code had > > module_init(aes_init); > module_exit(aes_exit); > > but also things like > > MODULE_DESCRIPTION("AES-ECB/CBC/CTR/XTS using ARMv8 Crypto Extensions"); > MODULE_AUTHOR("Ard Biesheuvel "); > MODULE_LICENSE("GPL v2"); > > and that all helps improve readablity and keep things sane. > > In contrast, turds like these are just pure and utter crap: > > static const unsigned dependencies[] __initconst __maybe_unused = { > drvid_cryptomgr, > 0 > }; > annotated_module_init(aes_init, drvid_aes_ce_arm, dependencies); > > and yes, I know that we have things like this for the driver ID lists > etc, but that doesn't make it better. > > No, I think any dependency model should strive to make this really > really easy and separate, and do things like > > module_depends(cryptomgr); > > and then just use that to fill in a link section or something like > that. And no, there's no way we will ever maintain a "list of > dependency identifiers". This is stuff that should be all about > scripting, or - better yet - just make the link section contain > strings so that you don't *need* any C level identifiers. > > That would be trivial to do by just making the "module_depends()" > macro be something like > > #define _dependency(x,y) \ > static const struct module_dependency_attribute \ > __used __attribute__ ((__section__ ("__dependencies"))) \ > * __dependency_attr = { x,y } > > #define module_depends(x) \ > _dependency(#x, KBUILD_NAME) > > #define module_provides(x) \ > _dependency(KBUILD_NAME, #x) > > And if a module depends on multiple other things, then you just have > multiple of those "module_depends()" things. There's some gcc trick to > generating numbered (per compilation unit) C identifiers (so that you > can have multiple of those "__dependency_attr" variables in the same > file), but I forget it right now. > > And this is also where I think those "module_init()" vs > "subsys_init()" things come in. "module_init()" means that it's a > driver level thing, which would mean that module_init() implies > > module_depends(level7); > module_provides(level7_end); > > so that the module would automatically be sorted wrt the "driver" level. > > Another advantage (apart from legibility of the source, and > integrating with the *existing* level-based dependencies) is that > using something like "module_depends()" and "module_provides()" means > that it should be easy to parse even outside of a C compiler, so you > could - if you want to - make all the dependencies be done not as part > of compiling the source, but as a separate scripting thing. That could > be useful for things like statistics and visualization tools that > don't want to actually build the kernel, but want to just show the > dependencies between different modules. > > So no. I do *not* think big patches like this are acceptable. This > kind of patch - along with the patch that just adds the random > dependency identifier C enums - is exactly what we do *not* want. If > we do dependencies, they should all be small and local things, and > they should not *replace* the existing "module_init()" vs > "arch_init()" system, they should add on top of it. Thanks for the detailed answer and you are right. But I had to start somehow and, unfortunately, I don't have the resources to fulfill your requirements. Regards, Alexander Holler