Re: [PATCH 11/14] init: deps: annotate various initcalls

[Linus Torvalds]

On Sat, Oct 17, 2015 at 10:14 AM, Alexander Holler <holler@xxxxxxxxxxxxx> 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@xxxxxxxxxx>");
  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/