Message-ID: <CAMwc25poqP-S2DSXLZuRA7F20fHsiq7uACbxUvYHCEkpY7BoHA@mail.gmail.com>
Date:   Wed, 24 May 2023 15:35:41 +1000
From:   David Airlie <airlied@...hat.com>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     Dave Airlie <airlied@...il.com>,
        Lucas De Marchi <lucas.demarchi@...el.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.

On Wed, May 24, 2023 at 3:26 PM Luis Chamberlain <mcgrof@...nel.org> wrote:
>
> On Wed, May 03, 2023 at 01:19:31PM +1000, Dave Airlie wrote:
> > >
> > > >
> > > >> > 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.
>
> Hey Dave, just curious if there was going to be another follow up patch
> for this or if it was already posted. I don't see it clearly so just
> wanted to double check.

I'm still considering the options here.

I could leave the kernel patch as-is and add explicit sorting in
dracut for anything in the groups, but then we have to name/version
the firmware in a certain way, another option might be to emit the
group bounds and two records, one old, one new per-fw file, then have
some sort of explicit versioning by the driver over what order to load
them.

Dave.

Powered by blists - more mailing lists