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

From	Alexander Holler <holler@ahsoftware.de>
Newsgroups	linux.kernel
Subject	Re: [PATCH 11/14] init: deps: annotate various initcalls
Date	2015-10-17 21:10 +0200
Message-ID	<qkF1E-6za-25@gated-at.bofh.it> (permalink)
References	<qkDjb-41n-7@gated-at.bofh.it> <qkDsT-4cT-51@gated-at.bofh.it> <qkEIh-5Va-11@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

Am 17.10.2015 um 20:47 schrieb 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.

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH 0/14] init: deps: dependency based (parallelized) init Alexander Holler <holler@ahsoftware.de> - 2015-10-17 19:20 +0200
  [PATCH 04/14] init: deps: order network interfaces by link order Alexander Holler <holler@ahsoftware.de> - 2015-10-17 19:20 +0200
    Re: [PATCH 04/14] init: deps: order network interfaces by link order Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-17 20:30 +0200
      Re: [PATCH 04/14] init: deps: order network interfaces by link order Alexander Holler <holler@ahsoftware.de> - 2015-10-17 20:40 +0200
        Re: [PATCH 04/14] init: deps: order network interfaces by link order Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-17 21:00 +0200
          Re: [PATCH 04/14] init: deps: order network interfaces by link order Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-17 21:10 +0200
            Re: [PATCH 04/14] init: deps: order network interfaces by link order Alexander Holler <holler@ahsoftware.de> - 2015-10-17 21:20 +0200
              Re: [PATCH 04/14] init: deps: order network interfaces by link order Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-17 21:40 +0200
              Re: [PATCH 04/14] init: deps: order network interfaces by link order Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-10-17 21:40 +0200
                Re: [PATCH 04/14] init: deps: order network interfaces by link order Alexander Holler <holler@ahsoftware.de> - 2015-10-17 22:00 +0200
                Re: [PATCH 04/14] init: deps: order network interfaces by link order Alexander Holler <holler@ahsoftware.de> - 2015-10-17 23:30 +0200
            Re: [PATCH 04/14] init: deps: order network interfaces by link order Alexander Holler <holler@ahsoftware.de> - 2015-10-17 23:40 +0200
          Re: [PATCH 04/14] init: deps: order network interfaces by link order Alexander Holler <holler@ahsoftware.de> - 2015-10-17 21:10 +0200
        Re: [PATCH 04/14] init: deps: order network interfaces by link order Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-10-17 21:00 +0200
        Re: [PATCH 04/14] init: deps: order network interfaces by link order Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-17 21:10 +0200
          Re: [PATCH 04/14] init: deps: order network interfaces by link order Alexander Holler <holler@ahsoftware.de> - 2015-10-17 21:10 +0200
  [PATCH 03/14] init: deps: dt: use (HW-specific) dependencies provided by the DT too Alexander Holler <holler@ahsoftware.de> - 2015-10-17 19:20 +0200
  [PATCH 06/14] dtc: deps: Automatically add new property 'dependencies' which contains a list of referenced phandles Alexander Holler <holler@ahsoftware.de> - 2015-10-17 19:20 +0200
  [PATCH 07/14] dtc: deps: introduce new (virtual) property no-dependencies Alexander Holler <holler@ahsoftware.de> - 2015-10-17 19:20 +0200
  [PATCH 09/14] dtc: deps: Add option to print dependency graph as dot (Graphviz) Alexander Holler <holler@ahsoftware.de> - 2015-10-17 19:20 +0200
  [PATCH 02/14] init: deps: use annotated initcalls for a dependency based (optionally parallelized) init Alexander Holler <holler@ahsoftware.de> - 2015-10-17 19:20 +0200
  [PATCH 01/14] init: deps: introduce annotated initcalls Alexander Holler <holler@ahsoftware.de> - 2015-10-17 19:20 +0200
  [PATCH 14/14] dt: dts: deps: omap: beagle: make some remote-endpoints non-dependencies Alexander Holler <holler@ahsoftware.de> - 2015-10-17 19:30 +0200
  [PATCH 13/14] dt: dts: deps: imx6q: make some remote-endpoints non-dependencies Alexander Holler <holler@ahsoftware.de> - 2015-10-17 19:30 +0200
  [PATCH 12/14] dt: dts: deps: kirkwood: dockstar: add dependency ehci -> usb power regulator Alexander Holler <holler@ahsoftware.de> - 2015-10-17 19:30 +0200
  Re: [PATCH 0/14] init: deps: dependency based (parallelized) init Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-10-17 19:50 +0200
    Re: [PATCH 0/14] init: deps: dependency based (parallelized) init Alexander Holler <holler@ahsoftware.de> - 2015-10-17 20:30 +0200
      Re: [PATCH 0/14] init: deps: dependency based (parallelized) init Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-10-17 20:40 +0200
        Re: [PATCH 0/14] init: deps: dependency based (parallelized) init Alexander Holler <holler@ahsoftware.de> - 2015-10-17 21:50 +0200
          Re: [PATCH 0/14] init: deps: dependency based (parallelized) init Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-10-17 22:30 +0200
            Re: [PATCH 0/14] init: deps: dependency based (parallelized) init Alexander Holler <holler@ahsoftware.de> - 2015-10-17 22:40 +0200
  Re: [PATCH 11/14] init: deps: annotate various initcalls Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-17 20:50 +0200
    Re: [PATCH 11/14] init: deps: annotate various initcalls Alexander Holler <holler@ahsoftware.de> - 2015-10-17 21:10 +0200

csiph-web