Message-ID: <56229AA8.8050609@ahsoftware.de>
Date:	Sat, 17 Oct 2015 20:59:52 +0200
From:	Alexander Holler <holler@...oftware.de>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Russell King <linux@....linux.org.uk>,
	Grant Likely <grant.likely@...aro.org>
Subject: Re: [PATCH 11/14] init: deps: annotate various initcalls

Am 17.10.2015 um 20:47 schrieb Linus Torvalds:
> On Sat, Oct 17, 2015 at 10:14 AM, Alexander Holler <holler@...oftware.de> 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 <ard.biesheuvel@...aro.org>");
>    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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists