Re: [PATCH v1 5/7] modpost: Create modalias for builtin modules
On 4/29/25 17:15, Alexey Gladkov wrote: > On Tue, Apr 29, 2025 at 04:14:13PM +0200, Petr Pavlu wrote: >> On 4/29/25 14:49, Alexey Gladkov wrote: >>> On Tue, Apr 29, 2025 at 12:04:44PM +0200, Alexey Gladkov wrote: > I'm not sure it's best to overload this data in this way. I think mixing > actual files and "logical" modules in the modules list is somewhat > confusing. > > An alternative would be to keep a single module struct for vmlinux and > record the discovered aliases under it? It is possible to extend struct module_alias and add the module name. The problem is that alias is added by module_alias_printf() and we will have to add the module name to the arguments to each do_entry handler in addition to struct module where there is already a name (but in our case it is vmlinux). I can do that if you think it's a better way. >>> >>> If I don't add separate entries for each builtin module, the patch will >>> look like this: >>> [...] >> >> I see, that didn't turn out as well as I envisioned. One more approach >> would be to track builtin modules separately. A patch is below. I'm not >> sure if it's better. > > I'm not sure I get it. What do you mean when you say I need to track > builtin modules separately ? The patch that I sent in my reply introduces a new list called builtin_modules. This is what I meant by tracking builtin modules separately. This implementation has the advantage of not conceptually mixing the modules and allows the function write_vmlinux_export_c_file() to directly use the new list. On the other hand, keeping everything in one list and introducing a flag for builtin modules (or maybe replacing module.is_vmlinux with some module.type enum) allows functions that take only a module pointer to know which module they are dealing with, which I imagine could be useful. I don't have a clear preference, as long as we avoid misusing module.dump_file. -- Thanks, Petr
Re: [PATCH v1 5/7] modpost: Create modalias for builtin modules
On Tue, Apr 29, 2025 at 04:14:13PM +0200, Petr Pavlu wrote: > On 4/29/25 14:49, Alexey Gladkov wrote: > > On Tue, Apr 29, 2025 at 12:04:44PM +0200, Alexey Gladkov wrote: > >>> I'm not sure it's best to overload this data in this way. I think mixing > >>> actual files and "logical" modules in the modules list is somewhat > >>> confusing. > >>> > >>> An alternative would be to keep a single module struct for vmlinux and > >>> record the discovered aliases under it? > >> > >> It is possible to extend struct module_alias and add the module name. The > >> problem is that alias is added by module_alias_printf() and we will have > >> to add the module name to the arguments to each do_entry handler in > >> addition to struct module where there is already a name (but in our case > >> it is vmlinux). > >> > >> I can do that if you think it's a better way. > > > > If I don't add separate entries for each builtin module, the patch will > > look like this: > > [...] > > I see, that didn't turn out as well as I envisioned. One more approach > would be to track builtin modules separately. A patch is below. I'm not > sure if it's better. I'm not sure I get it. What do you mean when you say I need to track builtin modules separately ? -- Rgrds, legion
Re: [PATCH v1 5/7] modpost: Create modalias for builtin modules
On 4/29/25 14:49, Alexey Gladkov wrote:
> On Tue, Apr 29, 2025 at 12:04:44PM +0200, Alexey Gladkov wrote:
>>> I'm not sure it's best to overload this data in this way. I think mixing
>>> actual files and "logical" modules in the modules list is somewhat
>>> confusing.
>>>
>>> An alternative would be to keep a single module struct for vmlinux and
>>> record the discovered aliases under it?
>>
>> It is possible to extend struct module_alias and add the module name. The
>> problem is that alias is added by module_alias_printf() and we will have
>> to add the module name to the arguments to each do_entry handler in
>> addition to struct module where there is already a name (but in our case
>> it is vmlinux).
>>
>> I can do that if you think it's a better way.
>
> If I don't add separate entries for each builtin module, the patch will
> look like this:
> [...]
I see, that didn't turn out as well as I envisioned. One more approach
would be to track builtin modules separately. A patch is below. I'm not
sure if it's better.
diff --git a/include/linux/module.h b/include/linux/module.h
index 7250b4a527ec..6225793ddcd4 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -257,14 +257,10 @@ extern void cleanup_module(void);
__PASTE(type, \
__PASTE(__, name)))
-#ifdef MODULE
/* Creates an alias so file2alias.c can find device table. */
#define MODULE_DEVICE_TABLE(type, name)\
extern typeof(name) __mod_device_table(type, name) \
__attribute__ ((unused, alias(__stringify(name
-#else /* !MODULE */
-#define MODULE_DEVICE_TABLE(type, name)
-#endif
/* Version of form [:][-].
* Or for CVS/RCS ID version, everything but the number is stripped.
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index dff1799a4c79..28a4c045f66c 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1471,8 +1471,8 @@ static const struct devtable devtable[] = {
/* Create MODULE_ALIAS() statements.
* At this time, we cannot write the actual output C source yet,
* so we write into the mod->dev_table_buf buffer. */
-void handle_moddevtable(struct module *mod, struct elf_info *info,
- Elf_Sym *sym, const char *symname)
+void handle_moddevtable(struct module *mod, struct elf_info *info, Elf_Sym
*sym,
+ const char *symname)
{
void *symval;
char *zeros = NULL;
@@ -1509,6 +1509,10 @@ void handle_moddevtable(struct module *mod, struct
elf_info *info,
typelen = name - type;
name += strlen("__");
+ if (mod->is_vmlinux)
+ // XXX Check if the module doesn't already exist?
+ mod = new_module(modname, modnamelen, true);
+
/* Handle all-NULL symbols allocated into .bss */
if (info->sechdrs[get_secindex(info, sym)].sh_type & SHT_NOBITS) {
zeros = calloc(1, sym->st_size);
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index be89921d60b6..f39e3456e021 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -168,9 +168,12 @@ char *get_line(char **stringp)
return orig;
}
-/* A list of all modules we processed */
+/* A list of all modules (vmlinux or *.ko) we processed */
LIST_HEAD(modules);
+/* A list of all builtin modules we processed */
+LIST_HEAD(builtin_modules);
+
static struct module *find_module(const char *filename, const char *modname)
{
struct module *mod;
@@ -183,7 +186,7 @@ static struct module *find_module(const char *filename,
const char *modname)
return NULL;
}
-static struct module *new_module(const char *name, size_t namelen)
+struct module *new_module(const char *name, size_t namelen, bool is_builtin)
{
struct module *mod;
@@ -207,7 +210,10 @@ static struct module *new_module(const char *name, size_t
namelen)
*/
mod->is_gpl_compatible = true;
- list_add_tail(&mod->list, &modules);
+ if (is_builtin)
+ list_add_tail(&mod->list, &builtin_modules);
+ else
+ list_add_tail(&mod->list, &modules);
return mod;
}
@@ -1573,7 +1579,7 @@ static void read_symbols(const char *modname)
}
/* strip trailing .o */
- mod = new_module(modname, strlen(modname) - strlen(".o"));
+ mod = new_module(modname, strlen(modname) - strlen(".o"), false);
/* save .no_trim_symbol section for later use */
if (info.no_trim_symbol_len) {
@@ -2021,11 +2027,23 @@ static void write_if_changed(struct buffer *b, const
char *fname)
static void write_vmlinux_export_c_file(struct module *mod)
{
struct buffer buf = { };
+ struct module_alias *alias, *next;
buf_printf(&buf,
- "#include \n");
+ "#include \n"
+ "#include \n");
add_exported_symbols(&buf, mod);
+
+ list_for_each_entry(mod, &builtin_modules, list) {
+ list_
Re: [PATCH v1 5/7] modpost: Create modalias for builtin modules
On Tue, Apr 29, 2025 at 12:04:44PM +0200, Alexey Gladkov wrote:
> > I'm not sure it's best to overload this data in this way. I think mixing
> > actual files and "logical" modules in the modules list is somewhat
> > confusing.
> >
> > An alternative would be to keep a single module struct for vmlinux and
> > record the discovered aliases under it?
>
> It is possible to extend struct module_alias and add the module name. The
> problem is that alias is added by module_alias_printf() and we will have
> to add the module name to the arguments to each do_entry handler in
> addition to struct module where there is already a name (but in our case
> it is vmlinux).
>
> I can do that if you think it's a better way.
If I don't add separate entries for each builtin module, the patch will
look like this:
diff --git a/include/linux/module.h b/include/linux/module.h
index 7250b4a527ec..6225793ddcd4 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -257,14 +257,10 @@ extern void cleanup_module(void);
__PASTE(type, \
__PASTE(__, name)))
-#ifdef MODULE
/* Creates an alias so file2alias.c can find device table. */
#define MODULE_DEVICE_TABLE(type, name)\
extern typeof(name) __mod_device_table(type, name) \
__attribute__ ((unused, alias(__stringify(name
-#else /* !MODULE */
-#define MODULE_DEVICE_TABLE(type, name)
-#endif
/* Version of form [:][-].
* Or for CVS/RCS ID version, everything but the number is stripped.
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index dff1799a4c79..efb5e1e3fa1f 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -37,6 +37,21 @@ typedef Elf64_Addr kernel_ulong_t;
#include
#include
+struct module_info {
+ struct module *module;
+ const char *modname;
+ size_t modnamelen;
+};
+
+/* Does modnamelen bytes of modname exactly match the module name? */
+static bool modname_is(const char *modname, size_t modnamelen, const char
*name)
+{
+ if (modnamelen != strlen(name))
+ return false;
+
+ return memcmp(modname, name, modnamelen) == 0;
+}
+
/**
* module_alias_printf - add auto-generated MODULE_ALIAS()
*
@@ -45,10 +60,11 @@ typedef Elf64_Addr kernel_ulong_t;
* @fmt: printf(3)-like format
*/
static void __attribute__((format (printf, 3, 4)))
-module_alias_printf(struct module *mod, bool append_wildcard,
+module_alias_printf(struct module_info *modi, bool append_wildcard,
const char *fmt, ...)
{
struct module_alias *new, *als;
+ char *modname;
size_t len;
int n;
va_list ap;
@@ -68,7 +84,7 @@ module_alias_printf(struct module *mod, bool append_wildcard,
if (append_wildcard)
len++; /* extra byte for '*' */
- new = xmalloc(sizeof(*new) + len);
+ new = xmalloc(sizeof(*new) + len + modi->modnamelen + 1);
/* Now, really print it to the allocated buffer */
va_start(ap, fmt);
@@ -87,14 +103,31 @@ module_alias_printf(struct module *mod, bool
append_wildcard,
}
/* avoid duplication */
- list_for_each_entry(als, &mod->aliases, node) {
- if (!strcmp(als->str, new->str)) {
+ list_for_each_entry(als, &modi->module->aliases, node) {
+ if (modi->module->is_vmlinux &&
+ !modname_is(modi->modname, modi->modnamelen, als->modname))
+ continue;
+ if (!strcmp(als->alias, new->str)) {
free(new);
return;
}
}
- list_add_tail(&new->node, &mod->aliases);
+ modname = new->str + n + 1;
+ len = modi->modnamelen + 1; /* extra byte for '\0' */
+
+ n = snprintf(modname, len, "%s", modi->modname);
+
+ if (n < len) {
+ error("snprintf failed\n");
+ free(new);
+ return;
+ }
+
+ new->alias = new->str;
+ new->modname = modname;
+
+ list_add_tail(&new->node, &modi->module->aliases);
}
typedef uint32_t __u32;
@@ -125,7 +158,7 @@ typedef struct {
struct devtable {
const char *device_id;
unsigned long id_size;
- void (*do_entry)(struct module *mod, void *symval);
+ void (*do_entry)(struct module_info *mod, void *symval);
};
/* Define a variable f that holds the value of field f of struct devid
@@ -182,7 +215,7 @@ static inline void add_guid(char *str, guid_t guid)
static void do_usb_entry(void *symval,
unsigned int bcdDevice_initial, int
bcdDevice_initial_digits,
unsigned char range_lo, unsigned char range_hi,
-unsigned char max, struct module *mod)
+unsigned char max, struct module_info *mod)
{
char alias[500];
DEF_FIELD(symval, usb_device_id, match_flags);
@@ -284,7 +317,7 @@ static uns
Re: [PATCH v1 5/7] modpost: Create modalias for builtin modules
On Tue, Apr 29, 2025 at 11:25:47AM +0200, Petr Pavlu wrote:
> On 4/26/25 18:16, Alexey Gladkov wrote:
> > For some modules, modalias is generated using the modpost utility and
> > the section is added to the module file.
> >
> > When a module is added inside vmlinux, modpost does not generate
> > modalias for such modules and the information is lost.
> >
> > As a result kmod (which uses modules.builtin.modinfo in userspace)
> > cannot determine that modalias is handled by a builtin kernel module.
> >
> > $ cat /sys/devices/pci:00/:00:14.0/modalias
> > pci:v8086dA36Dsv1043sd8694bc0Csc03i30
> >
> > $ modinfo xhci_pci
> > name: xhci_pci
> > filename: (builtin)
> > license:GPL
> > file: drivers/usb/host/xhci-pci
> > description:xHCI PCI Host Controller Driver
> >
> > Missing modalias "pci:v*d*sv*sd*bc0Csc03i30*" which will be generated by
> > modpost if the module is built separately.
> >
> > To fix this it is necessary to generate the same modalias for vmlinux as
> > for the individual modules. Fortunately '.vmlinux.export.o' is already
> > generated from which '.modinfo' can be extracted in the same way as for
> > vmlinux.o.
> >
> > Signed-off-by: Alexey Gladkov
> > ---
> > include/linux/module.h | 4
> > scripts/mod/file2alias.c | 13 -
> > scripts/mod/modpost.c| 21 ++---
> > scripts/mod/modpost.h| 7 ++-
> > 4 files changed, 36 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 7250b4a527ec..6225793ddcd4 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -257,14 +257,10 @@ extern void cleanup_module(void);
> > __PASTE(type, \
> > __PASTE(__, name)))
> >
> > -#ifdef MODULE
> > /* Creates an alias so file2alias.c can find device table. */
> > #define MODULE_DEVICE_TABLE(type, name)\
> > extern typeof(name) __mod_device_table(type, name) \
> >__attribute__ ((unused, alias(__stringify(name
> > -#else /* !MODULE */
> > -#define MODULE_DEVICE_TABLE(type, name)
> > -#endif
> >
> > /* Version of form [:][-].
> > * Or for CVS/RCS ID version, everything but the number is stripped.
> > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> > index dff1799a4c79..0fa3f031b904 100644
> > --- a/scripts/mod/file2alias.c
> > +++ b/scripts/mod/file2alias.c
> > @@ -1471,7 +1471,8 @@ static const struct devtable devtable[] = {
> > /* Create MODULE_ALIAS() statements.
> > * At this time, we cannot write the actual output C source yet,
> > * so we write into the mod->dev_table_buf buffer. */
> > -void handle_moddevtable(struct module *mod, struct elf_info *info,
> > +void handle_moddevtable(struct list_head *modules,
> > + struct module *mod, struct elf_info *info,
> > Elf_Sym *sym, const char *symname)
> > {
> > void *symval;
>
> The new modules parameter is unused.
Indeed. It is no longer needed after the optimization. Thank you.
> > @@ -1509,6 +1510,16 @@ void handle_moddevtable(struct module *mod, struct
> > elf_info *info,
> > typelen = name - type;
> > name += strlen("__");
> >
> > + if (mod->is_vmlinux) {
> > + struct module *builtin_mod;
> > +
> > + builtin_mod = new_module(modname, modnamelen);
> > + builtin_mod->is_vmlinux = mod->is_vmlinux;
> > + builtin_mod->dump_file = MODULE_BUILTIN_FNAME;
>
> The module.dump_file member is described in scripts/mod/modpost.h as
> "path to the .symvers file if loaded from a file". However, that is not
> the case here.
>
> Similarly, the module struct in scripts/mod/modpost.h is commented as
> "represent a module (vmlinux or *.ko)", but this patch expands its scope
> to also include builtin modules.
Well, an alternative would be to add a separate flag. I used dump_file
because it allows you to exclude such "builtin" modules from processing in
write_dump(), write_namespace_deps_files(), etc.
But yes, I agree that it's an abuse.
> I'm not sure it's best to overload this data in this way. I think mixing
> actual files and "logical" modules in the modules list is somewhat
> confusing.
>
> An alternative would be to keep a single module struct for vmlinux and
> record the discovered aliases under it?
It is possible to extend struct module_alias and add the module name. The
problem is that alias is added by module_alias_printf() and we will have
to add the module name to the arguments to each do_entry handler in
addition to struct module where there is already a name (but in our case
it is vmlinux).
I can do that if you think it's a better way.
--
Rgrds, legion
Re: [PATCH v1 5/7] modpost: Create modalias for builtin modules
On 4/26/25 18:16, Alexey Gladkov wrote:
> For some modules, modalias is generated using the modpost utility and
> the section is added to the module file.
>
> When a module is added inside vmlinux, modpost does not generate
> modalias for such modules and the information is lost.
>
> As a result kmod (which uses modules.builtin.modinfo in userspace)
> cannot determine that modalias is handled by a builtin kernel module.
>
> $ cat /sys/devices/pci:00/:00:14.0/modalias
> pci:v8086dA36Dsv1043sd8694bc0Csc03i30
>
> $ modinfo xhci_pci
> name: xhci_pci
> filename: (builtin)
> license:GPL
> file: drivers/usb/host/xhci-pci
> description:xHCI PCI Host Controller Driver
>
> Missing modalias "pci:v*d*sv*sd*bc0Csc03i30*" which will be generated by
> modpost if the module is built separately.
>
> To fix this it is necessary to generate the same modalias for vmlinux as
> for the individual modules. Fortunately '.vmlinux.export.o' is already
> generated from which '.modinfo' can be extracted in the same way as for
> vmlinux.o.
>
> Signed-off-by: Alexey Gladkov
> ---
> include/linux/module.h | 4
> scripts/mod/file2alias.c | 13 -
> scripts/mod/modpost.c| 21 ++---
> scripts/mod/modpost.h| 7 ++-
> 4 files changed, 36 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 7250b4a527ec..6225793ddcd4 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -257,14 +257,10 @@ extern void cleanup_module(void);
> __PASTE(type, \
> __PASTE(__, name)))
>
> -#ifdef MODULE
> /* Creates an alias so file2alias.c can find device table. */
> #define MODULE_DEVICE_TABLE(type, name) \
> extern typeof(name) __mod_device_table(type, name) \
>__attribute__ ((unused, alias(__stringify(name
> -#else /* !MODULE */
> -#define MODULE_DEVICE_TABLE(type, name)
> -#endif
>
> /* Version of form [:][-].
> * Or for CVS/RCS ID version, everything but the number is stripped.
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index dff1799a4c79..0fa3f031b904 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1471,7 +1471,8 @@ static const struct devtable devtable[] = {
> /* Create MODULE_ALIAS() statements.
> * At this time, we cannot write the actual output C source yet,
> * so we write into the mod->dev_table_buf buffer. */
> -void handle_moddevtable(struct module *mod, struct elf_info *info,
> +void handle_moddevtable(struct list_head *modules,
> + struct module *mod, struct elf_info *info,
> Elf_Sym *sym, const char *symname)
> {
> void *symval;
The new modules parameter is unused.
> @@ -1509,6 +1510,16 @@ void handle_moddevtable(struct module *mod, struct
> elf_info *info,
> typelen = name - type;
> name += strlen("__");
>
> + if (mod->is_vmlinux) {
> + struct module *builtin_mod;
> +
> + builtin_mod = new_module(modname, modnamelen);
> + builtin_mod->is_vmlinux = mod->is_vmlinux;
> + builtin_mod->dump_file = MODULE_BUILTIN_FNAME;
The module.dump_file member is described in scripts/mod/modpost.h as
"path to the .symvers file if loaded from a file". However, that is not
the case here.
Similarly, the module struct in scripts/mod/modpost.h is commented as
"represent a module (vmlinux or *.ko)", but this patch expands its scope
to also include builtin modules.
I'm not sure it's best to overload this data in this way. I think mixing
actual files and "logical" modules in the modules list is somewhat
confusing.
An alternative would be to keep a single module struct for vmlinux and
record the discovered aliases under it?
--
Thanks,
Petr
