Messages in this thread | | | Date | Sat, 17 Oct 2015 11:47:30 -0700 | Subject | Re: [PATCH 11/14] init: deps: annotate various initcalls | From | Linus Torvalds <> |
| |
On Sat, Oct 17, 2015 at 10:14 AM, Alexander Holler <holler@ahsoftware.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@linaro.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.
Linus
| |