Hello,
On Sat, Sep 30, 2023 at 08:08:05PM +0900, Masahiro Yamada wrote:
> On Sat, Sep 30, 2023 at 2:41 AM Uwe Kleine-König
> <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> >
> > Hello,
> >
> > On Fri, Sep 29, 2023 at 10:15:40AM +0200, Uwe Kleine-König wrote:
> > > by manual inspection I found a section mismatch in
> > > drivers/hwtracing/coresight/coresight-etm4x-core.c where
> > > etm4_platform_driver (which lives in ".data") contains a reference to
> > > etm4_remove_platform_dev() (which lives in ".exit.text").
> > >
> > > However building with CONFIG_DEBUG_SECTION_MISMATCH=y +
> > > CONFIG_CORESIGHT_SOURCE_ETM4X=y doesn't warn about that one.
> >
> > Arnd had the right hint in irc: If I do
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 34a5386d444a..070e53be1ea2 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1017,7 +1017,7 @@ static int secref_whitelist(const char *fromsec, const char *fromsym,
> >
> > /* symbols in data sections that may refer to meminit/exit sections */
> > if (match(fromsec, PATTERNS(DATA_SECTIONS)) &&
> > - match(tosec, PATTERNS(ALL_XXXINIT_SECTIONS, ALL_EXIT_SECTIONS)) &&
> > + match(tosec, PATTERNS(ALL_XXXINIT_SECTIONS, ALL_XXXEXIT_SECTIONS)) &&
> > match(fromsym, PATTERNS("*driver")))
> > return 0;
> >
> > I get a mismatch warning:
> >
> > WARNING: modpost: vmlinux: section mismatch in reference: etm4x_amba_driver+0x98 (section: .data) -> etm4_remove_amba (section: .exit.text)
> > WARNING: modpost: vmlinux: section mismatch in reference: etm4_platform_driver+0x8 (section: .data) -> etm4_remove_platform_dev (section: .exit.text)
> > ERROR: modpost: Section mismatches detected.
> >
> > I remember that back in the times of CONFIG_HOTPLUG references to
> > ".devinit.text" and ".devexit.text" were ok. Is a reference to
> > .exit.text ever ok?
> >
> > I started an allyesconfig build on a few archs with the above patch
> > applied. This will take some time, when it's done I will report what it
> > found.
>
>
>
> allmodconfig on x86 detects several issues.
>
> [...]
> WARNING: modpost: sound/soc/codecs/snd-soc-tlv320adc3xxx: section
> mismatch in reference: adc3xxx_i2c_driver+0x10 (section: .data) ->
> adc3xxx_i2c_remove (section: .exit.text)
I checked just one of those, this is also a valid warning:
static void __exit adc3xxx_i2c_remove(struct i2c_client *client)
{
...
}
static struct i2c_driver adc3xxx_i2c_driver = {
...
.remove = __exit_p(adc3xxx_i2c_remove),
...
};
so if adc3xxx_i2c_remove is discarded (happens with
CONFIG_SND_SOC_TLV320ADC3XXX=y) no warning happens, but .remove is just
NULL and so if the driver is unbound no code runs and so it leaks
resources. My thought was that allyesconfig would catch all problems,
didn't consider driver authors to be that creative :-)
$(git grep -E '\.remove\s*=\s*__exit') suggests there are quite a few of
these ...
> Linus requires zero warning, so we cannot enable the modpost warning
> unless we fix all the sources of the warnings.
> That will need one or two dev cycles.
>
> If you want to turn on the warning now, you can surround the check
> with 'if (extra_warn) '.
OK, extra_warn is a nice compromise. Just keeping the build silent about
these real problems to allow a warning-free build sounds a bit like an
instance of Goodhart's law[1].
> Since commit 20ff36856fe00879f82de71fe6f1482ca1b72334
> it is possible to enable a modpost warning only under W=1 builds.
OK, I will rework the patch to only trigger on W=1.
Best regards
Uwe
[1] https://en.wikipedia.org/wiki/Goodhart%27s_law
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
