On Fri, Oct 16, 2026 at 02:14:16PM +0200, Petr Pavlu wrote:
> This version looks mostly ok to me, from a modules perspective. Below
> are a few nits plus one more useful comment about handling
> MODULE_ARCH_INIT.
Ack thanks :)
>
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 0fd43c8a89ea..9d96459e492c 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
...
> > +/* The equivalent of KSYMTAB_FUNC()/KSYMTAB_DATA(). */
> > +static void add_asm_ksymtab(struct buffer *buf, const struct symbol *sym)
> > +{
> > + const char *name = sym->name;
> > +
> > + add_asm_kstrtab(buf, sym);
> > +
> > + buf_printf(buf, "\t.section \"___ksymtab+%s\", \"a\"\n", name);
> > + buf_printf(buf, "\t.balign %d\n", KSYM_ALIGN);
> > + buf_printf(buf, "__ksymtab_%s:\n", name);
> > + add_asm_ksym_ref(buf, sym->is_func ? MOD_FUNC_PREFIX : "", name);
> > + add_asm_ksym_ref(buf, "__kstrtab_", name);
> > + add_asm_ksym_ref(buf, "__kstrtabns_", name);
> > + buf_printf(buf, "\t.previous\n");
> > +
> > + add_asm_kflagstab(buf, sym);
> > +}
>
> add_asm_kstrtab() and add_asm_kflagstab() are only called from
> add_asm_ksymtab() and their logic is fairly short. I suggest merging
> these functions.
Ack, done for v5.
>
> > +
> > +/* The equivalent of SYMBOL_CRC(). */
> > +static void add_asm_crc(struct buffer *buf, const struct symbol *sym)
> > +{
> > + buf_printf(buf, "\t.section \"___kcrctab+%s\",\"a\"\n", sym->name);
> > + buf_printf(buf, "\t.balign 4\n");
> > + buf_printf(buf, "__crc_%s:\n", sym->name);
> > + buf_printf(buf, "\t.long 0x%08x\n", sym->crc);
> > + buf_printf(buf, "\t.previous\n");
> > +}
> > +
> > +static bool export_is_kept(const struct symbol *sym)
> > +{
> > + return !trim_unused_exports || sym->used;
> > +}
>
> export_is_kept() is only called once from add_asm_exported_symbols().
> The condition can be inlined directly there.
Ack done.
> > +/*
> > + * The equivalent of:
> > + *
> > + * __visible struct module __this_module
> > __section(".gnu.linkonce.this_module")
> > + * = { .name = KBUILD_MODNAME, .init = init_module, .exit =
> > cleanup_module,
> > + * .arch = MODULE_ARCH_INIT };
> > + *
> > + * Everything not listed is zero, MODULE_ARCH_INIT included, except on
> > m68k.
> > + */
> > +static void add_asm_this_module(struct buffer *buf, const struct module
> > *mod,
> > + const char *modname)
> > +{
> > + struct this_module_field fields[THIS_MODULE_MAX_FIELDS];
> > + const unsigned int nr_fields = get_this_module_fields(mod, modname,
> > fields);
>
> The const qualifier can be removed from the nr_fields definition to
> match the usual kernel style.
Ack done.
> > +/*
> > + * The equivalent of:
> > + * static const u32 ____version_ext_crcs[] __section("__version_ext_crcs")
> > = { crc, ... };
> > + * static const char ____version_ext_names[]
> > __section("__version_ext_names") = "name\0" ...;
> > + *
> > + * for unresolved symbols.
> > + */
> > +static void add_asm_extended_versions(struct buffer *buf, struct module
> > *mod)
> > +{
> > + struct symbol *sym;
> > +
> > + if (!extended_modversions)
> > + return;
> > +
> > + buf_printf(buf, "\n\t.section __version_ext_crcs,\"a\",%%progbits\n");
> > + buf_printf(buf, "\t.balign 4\n");
> > + list_for_each_entry(sym, &mod->unresolved_symbols, list) {
> > + if (skip_unversioned(mod, sym))
> > + continue;
> > +
> > + buf_printf(buf, "\t.long 0x%08x\n", sym->crc);
> > + }
> > +
> > + /*
> > + * A symbol without a CRC was warned about above, and is skipped here
> > + * too so that the names line up with the CRCs.
> > + */
> > + buf_printf(buf, "\t.section __version_ext_names,\"a\",%%progbits\n");
> > + list_for_each_entry(sym, &mod->unresolved_symbols, list) {
> > + if (!sym->module || !sym->crc_valid)
> > + continue;
>
> This condition must match the logic in skip_unversioned(). It would be
> clearer to keep it only in one place by calling skip_unversioned() here
> and adding a "silent" parameter to the function.
Ack done.
>
> > diff --git a/scripts/mod/module-offsets.c b/scripts/mod/module-offsets.c
> > new file mode 100644
> > index 000000000000..a336dd47aa33
> > --- /dev/null
> > +++ b/scripts/mod/module-offsets.c
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Layout of the structures modpost emits into *.mod.S, extracted from the
> > + * target headers as devicetable-offsets.c does for the device tables.
> > + */
>
> The second part of the comment, after the comma, is superfluous. It is
> expected that the various parts of modpost use consistent mechanisms
> where appropriate.
Ack done.
> > +int main(void)
> > +{
> > + DEFINE(MOD_SIZEOF_LONG, sizeof(long));
> > + DEFINE(MOD_PREL32_RELOCATIONS,
> > IS_ENABLED(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS));
> > + DEFINE(MOD_FUNC_PLABEL, IS_ENABLED(CONFIG_PARISC) &&
> > IS_ENABLED(CONFIG_64BIT));
> > +
> > + DEFINE(MOD_SIZEOF_struct_module, sizeof(struct module));
> > + DEFINE(MOD_ALIGNOF_struct_module, __alignof__(struct module));
> > + OFFSET(MOD_OFF_module_name, module, name);
> > + OFFSET(MOD_OFF_module_init, module, init);
> > +#ifdef CONFIG_MODULE_UNLOAD
> > + OFFSET(MOD_OFF_module_exit, module, exit);
> > +#endif
> > +#if defined(CONFIG_M68K) && defined(CONFIG_MMU)
> > + /* MODULE_ARCH_INIT: the only architecture where it is not all zeroes.
> > */
> > + OFFSET(MOD_OFF_module_arch_fixup_start, module, arch.fixup_start);
> > + OFFSET(MOD_OFF_module_arch_fixup_end, module, arch.fixup_end);
> > +#endif
>
> I feel it is a bit of a layering violation for modpost to know how
> individual architectures need to initialize module::arch.
Yeah that is kinda gross :)
>
> For now, this is probably ok. I would just remove MODULE_ARCH_INIT from
> include/linux/module.h and arch/m68k/include/asm/module.h, along with
> the comment referring to it in scripts/mod/modpost.c.
ack done.
>
> In the future, this could be avoided by having the m68k module support
> look up the .m68k_fixup section at runtime, as other architectures
> typically do for similar sections.
Yes, arch stuff should live with the arch-specific stuff.
--
Cheers, Lorenzo
