Date:   Wed, 3 May 2023 13:19:31 +1000
From:   Dave Airlie <airlied@...il.com>
To:     Lucas De Marchi <lucas.demarchi@...el.com>
Cc:     Luis Chamberlain <mcgrof@...nel.org>,
        Dave Airlie <airlied@...hat.com>,
        dri-devel@...ts.freedesktop.org, linux-modules@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] modules/firmware: add a new option to denote a firmware
 group to choose one.

>
> >
> >> > the GROUP until after the FIRMWARE, so this can't work, as it already
> >> > will have included all the ones below, hence why I bracketed top and
> >> > bottom with a group.
> >>
> >> well... that is something that can be adapted easily by using a 2 pass
> >> approach, filtering out the list based on the groups.
> >>
> >> I agree that yours is simpler though.  If we can rely on the
> >> order produced by the compiler and we document the expectations of
> >> MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
> >> simpler approach.
> >>
> >> Luis, any thoughts here?
> >
> >I see the Dracut code indicates that the order says now that you should
> >put the preferred firmware last, and that seems to match most coding
> >conventions, ie, new firmwares likely get added last, so it's a nice
>
> not all. i915 for example keeps it newest first so when attempting
> multiple firmware versions it starts from the preferred version.  It
> will be harder to convert since it also uses a x-macro to make sure the
> MODULE_FIRMWARE() and the the platform mapping are actually using the same
> firmware.
>
> >coincidence. Will this always work? I don't know. But if you like to
>
> short answer: it depends if your compiler is gcc *and* -O2 is used
> Longer debug ahead. Simple test with the expansion of MODULE_FIRMWARE
> baked in:
>
>         $ cat /tmp/a.c
>         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char foo[] = "modinfo_manual_foo";
>         static const __attribute__((section("__modinfo_manual"), used, aligned(1))) char bar[] = "modinfo_manual_bar";
>         $ gcc -c -o /tmp/a.o /tmp/a.c
>         $ objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual
>         $ strings /tmp/modinfo_manual
>         modinfo_manual_foo
>         modinfo_manual_bar
>
> However that doesn't match when building modules. In kmod:
>
> diff --git a/testsuite/module-playground/mod-simple.c b/testsuite/module-playground/mod-simple.c
> index 503e4d8..6dd5771 100644
> --- a/testsuite/module-playground/mod-simple.c
> +++ b/testsuite/module-playground/mod-simple.c
> @@ -30,3 +30,9 @@ module_exit(test_module_exit);
>
>   MODULE_AUTHOR("Lucas De Marchi <lucas.demarchi@...el.com>");
>   MODULE_LICENSE("GPL");
> +
> +
> +static const char __UNIQUE_ID_firmware404[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_foo";
> +static const char __UNIQUE_ID_firmware405[] __attribute__((__used__)) __attribute__((__section__("__modinfo_cpp"))) __attribute__((__aligned__(1))) = "modinfo_cpp_bar";
>
>         $ make ....
>         $ objcopy -O binary --only-section=__modinfo_cpp testsuite/module-playground/mod-simple.ko /tmp/modinfo_cpp
>         $ strings /tmp/modinfo_cpp
>         modinfo_cpp_bar
>         modinfo_cpp_foo
>
> It doesn't seem to be ./scripts/Makefile.modfinal neither as it's also
> inverted in testsuite/module-playground/mod-simple.o
>
> After checking the options passed to gcc, here is the "culprit": -O2
>
>         $ gcc -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
>         modinfo_manual_foo
>         modinfo_manual_bar
>         $ gcc -O2 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
>         modinfo_manual_bar
>         modinfo_manual_foo
>
> It seems anything but -O0 inverts the section.
>
>         $ gcc --version
>         gcc (GCC) 12.2.1 20230201
>
> It doesn't match the behavior described in its man page though. Manually
> specifying all the options that -O1 turns on doesn't invert it.
>
>         $ gcc -fauto-inc-dec -fbranch-count-reg -fcombine-stack-adjustments \
>                 -fcompare-elim -fcprop-registers -fdce -fdefer-pop -fdelayed-branch
>                 -fdse -fforward-propagate -fguess-branch-probability -fif-conversion \
>                 -fif-conversion2 -finline-functions-called-once -fipa-modref \
>                 -fipa-profile -fipa-pure-const -fipa-reference -fipa-reference-addressable \
>                 -fmerge-constants -fmove-loop-stores -fomit-frame-pointer -freorder-blocks \
>                 -fshrink-wrap -fshrink-wrap-separate -fsplit-wide-types -fssa-backprop \
>                 -fssa-phiopt -ftree-bit-ccp -ftree-ccp -ftree-ch -ftree-coalesce-vars \
>                 -ftree-copy-prop -ftree-dce -ftree-dominator-opts -ftree-dse -ftree-forwprop \
>                 -ftree-fre -ftree-phiprop -ftree-pta -ftree-scev-cprop -ftree-sink -ftree-slsr \
>                 -ftree-sra -ftree-ter -funit-at-a-time -c -o /tmp/a.o /tmp/a.c \
>                 && objcopy -O binary --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings /tmp/modinfo_manual
>         cc1: warning: this target machine does not have delayed branches
>         modinfo_manual_foo
>         modinfo_manual_bar
>

Thanks Lucas,

-ftoplevel-reorder is the one that does it, now that does mean how
I've done it isn't going to be robust.

I will reconsider but in order to keep backwards compat, it might be
easier to add firmware groups as an explicit list, but also spit out
the individual names, but not sure how clean this will end up on
dracut side.

I'll take a look at the other options, but it does seem like reworking
dracut is going to be the harder end of this, esp if I still want to
keep compat with older ones.

Dave.

Powered by blists - more mailing lists