* Re: [PATCH v4 1/4] kernel: param: initialize module_kset before do_initcalls()
From: Shashank Balaji @ 2026-04-28 13:07 UTC (permalink / raw)
To: Gary Guo, Thierry Reding, Jonathan Hunter
Cc: Suzuki K Poulose, James Clark, Alexander Shishkin,
Maxime Coquelin, Alexandre Torgue, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Miguel Ojeda, Boqun Feng,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Richard Cochran, Jonathan Corbet, Shuah Khan,
Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
Aaron Tomlin, Mike Leach, Leo Yan, Rahul Bukte, linux-kernel,
coresight, linux-arm-kernel, driver-core, rust-for-linux,
linux-doc, Daniel Palmer, Tim Bird, linux-modules, linux-tegra
In-Reply-To: <DI4QQA6EGIA1.N8WRFWVKG91S@garyguo.net>
Adding Tegra maintainers.
On Tue, Apr 28, 2026 at 12:10:50PM +0100, Gary Guo wrote:
> On Tue Apr 28, 2026 at 1:37 AM BST, Shashank Balaji wrote:
> > Hi Gary,
> >
> > On Mon, Apr 27, 2026 at 02:29:55PM +0100, Gary Guo wrote:
> >> On Mon Apr 27, 2026 at 3:41 AM BST, Shashank Balaji wrote:
> >> > module_kset is initialized in param_sysfs_init(), a subsys_initcall. A number
> >> > of platform drivers register themselves prior to subsys_initcalls
> >> > (tegra194_cbb_driver registers in a pure_initcall, for example). With an
> >> > upcoming patch ("driver core: platform: set mod_name in driver registration")
> >> > that sets their mod_name in struct device_driver, lookup_or_create_module_kobject()
> >> > will be called for those drivers, which calls kset_find_obj(module_kset, mod_name).
> >> > This causes a null deref because module_kset isn't alive yet.
> >> >
> >> > Fix this by initializing module_kset in do_basic_setup() before do_initcalls().
> >> > Modernize the pr_warn while we're at it.
> >> >
> >> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> > Suggested-by: Gary Guo <gary@garyguo.net>
> >>
> >> I didn't suggest this change :)
> >>
> >> I suggested `pure_initcall`, which is just a one line change.
> >
> > Oops, sorry about the misattribution.
> >
> >> diff --git a/kernel/params.c b/kernel/params.c
> >> index 74d620bc2521..ac088d4b09a9 100644
> >> --- a/kernel/params.c
> >> +++ b/kernel/params.c
> >> @@ -957,7 +957,7 @@ static int __init param_sysfs_init(void)
> >>
> >> return 0;
> >> }
> >> -subsys_initcall(param_sysfs_init);
> >> +pure_initcall(param_sysfs_init);
> >>
> >> /*
> >> * param_sysfs_builtin_init - add sysfs version and parameter
> >>
> >> pure_initcall is level 0 so it happens before all other init calls. Does it not
> >> work?
> >
> > tegra194_cbb_driver registers itself in a pure_initcall too. We wouldn't
> > want the ordering of its registration and module_kset init to be link order
> > dependent.
>
> It's the only device driver that does this. And I don't think it's supposed to.
>
> >From documentation:
>
> > A "pure" initcall has no dependencies on anything else, and purely
> > initializes variables that couldn't be statically initialized.
>
> I understand that given large amount of drivers registering themselves during
> core/arch_initcall that there might be regressions if all of them are moved, but
> surely we can demote these two specific tegra driver to core/postcore_initcall?
> This will still be called earlier than init_machine call which happens during
> arch_initcall.
>
> Looks like the tegra CBB driver is just doing error logging anyway.
That's a good point, Gary. Thanks!
Hi Thierry and Jonathan,
You can find the context for this email in this patch:
https://lore.kernel.org/all/20260427-acpi_mod_name-v4-1-22b42240c9bf@sony.com/
TL;DR: tegra194_cbb_driver and tegra234_cbb_driver are the only drivers
registering themselves as early as in a pure_initcall. This is a problem
on two fronts:
1. Philosophical: As Gary pointed out, pure_initcalls are intended to purely
initialize variables that couldn't be statically initialized. But these
are doing driver registrations.
2. module_kset not initialized at pure_initcall stage: This is needed to
set the module sysfs symlink. Since module_kset is not alive yet during
pure_initcalls, registering these drivers panics the kernel.
We would like to do the tegra cbb driver registration in a core_initcall
(or some later initcall works too), and move module_kset initialization
to a pure_initcall. Like this:
diff --git a/drivers/soc/tegra/cbb/tegra194-cbb.c b/drivers/soc/tegra/cbb/tegra194-cbb.c
index ab75d50cc85c..2f69e104c838 100644
--- a/drivers/soc/tegra/cbb/tegra194-cbb.c
+++ b/drivers/soc/tegra/cbb/tegra194-cbb.c
@@ -2342,7 +2342,7 @@ static int __init tegra194_cbb_init(void)
{
return platform_driver_register(&tegra194_cbb_driver);
}
-pure_initcall(tegra194_cbb_init);
+core_initcall(tegra194_cbb_init);
static void __exit tegra194_cbb_exit(void)
{
diff --git a/drivers/soc/tegra/cbb/tegra234-cbb.c b/drivers/soc/tegra/cbb/tegra234-cbb.c
index fb26f085f691..785072fa4e85 100644
--- a/drivers/soc/tegra/cbb/tegra234-cbb.c
+++ b/drivers/soc/tegra/cbb/tegra234-cbb.c
@@ -1774,7 +1774,7 @@ static int __init tegra234_cbb_init(void)
{
return platform_driver_register(&tegra234_cbb_driver);
}
-pure_initcall(tegra234_cbb_init);
+core_initcall(tegra234_cbb_init);
static void __exit tegra234_cbb_exit(void)
{
Would this work?
Thanks,
Shashank
^ permalink raw reply related
* Re: [PATCH v4 1/4] kernel: param: initialize module_kset before do_initcalls()
From: Gary Guo @ 2026-04-28 11:10 UTC (permalink / raw)
To: Shashank Balaji, Gary Guo
Cc: Suzuki K Poulose, James Clark, Alexander Shishkin,
Maxime Coquelin, Alexandre Torgue, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Miguel Ojeda, Boqun Feng,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Richard Cochran, Jonathan Corbet, Shuah Khan,
Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
Aaron Tomlin, Mike Leach, Leo Yan, Rahul Bukte, linux-kernel,
coresight, linux-arm-kernel, driver-core, rust-for-linux,
linux-doc, Daniel Palmer, Tim Bird, linux-modules
In-Reply-To: <afABOMT_s9DvF6NY@JPC00244420>
On Tue Apr 28, 2026 at 1:37 AM BST, Shashank Balaji wrote:
> Hi Gary,
>
> On Mon, Apr 27, 2026 at 02:29:55PM +0100, Gary Guo wrote:
>> On Mon Apr 27, 2026 at 3:41 AM BST, Shashank Balaji wrote:
>> > module_kset is initialized in param_sysfs_init(), a subsys_initcall. A number
>> > of platform drivers register themselves prior to subsys_initcalls
>> > (tegra194_cbb_driver registers in a pure_initcall, for example). With an
>> > upcoming patch ("driver core: platform: set mod_name in driver registration")
>> > that sets their mod_name in struct device_driver, lookup_or_create_module_kobject()
>> > will be called for those drivers, which calls kset_find_obj(module_kset, mod_name).
>> > This causes a null deref because module_kset isn't alive yet.
>> >
>> > Fix this by initializing module_kset in do_basic_setup() before do_initcalls().
>> > Modernize the pr_warn while we're at it.
>> >
>> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > Suggested-by: Gary Guo <gary@garyguo.net>
>>
>> I didn't suggest this change :)
>>
>> I suggested `pure_initcall`, which is just a one line change.
>
> Oops, sorry about the misattribution.
>
>> diff --git a/kernel/params.c b/kernel/params.c
>> index 74d620bc2521..ac088d4b09a9 100644
>> --- a/kernel/params.c
>> +++ b/kernel/params.c
>> @@ -957,7 +957,7 @@ static int __init param_sysfs_init(void)
>>
>> return 0;
>> }
>> -subsys_initcall(param_sysfs_init);
>> +pure_initcall(param_sysfs_init);
>>
>> /*
>> * param_sysfs_builtin_init - add sysfs version and parameter
>>
>> pure_initcall is level 0 so it happens before all other init calls. Does it not
>> work?
>
> tegra194_cbb_driver registers itself in a pure_initcall too. We wouldn't
> want the ordering of its registration and module_kset init to be link order
> dependent.
It's the only device driver that does this. And I don't think it's supposed to.
From documentation:
> A "pure" initcall has no dependencies on anything else, and purely
> initializes variables that couldn't be statically initialized.
I understand that given large amount of drivers registering themselves during
core/arch_initcall that there might be regressions if all of them are moved, but
surely we can demote these two specific tegra driver to core/postcore_initcall?
This will still be called earlier than init_machine call which happens during
arch_initcall.
Looks like the tegra CBB driver is just doing error logging anyway.
Best,
Gary
^ permalink raw reply
* Re: [PATCH v2 2/2] module/kallsyms: sort function symbols and use binary search
From: Stanislaw Gruszka @ 2026-04-28 8:23 UTC (permalink / raw)
To: Petr Pavlu
Cc: linux-modules, Sami Tolvanen, Luis Chamberlain, linux-kernel,
linux-trace-kernel, live-patching, Daniel Gomez, Aaron Tomlin,
Steven Rostedt, Masami Hiramatsu, Jordan Rome, Viktor Malik
In-Reply-To: <88ae41dc-e5e0-442a-9b95-5125adf31e75@suse.com>
On Mon, Apr 27, 2026 at 03:51:31PM +0200, Petr Pavlu wrote:
> On 4/24/26 11:13 AM, Stanislaw Gruszka wrote:
> > On Thu, Apr 23, 2026 at 04:00:04PM +0200, Petr Pavlu wrote:
> >> On 3/27/26 12:00 PM, Stanislaw Gruszka wrote:
> [...]
> >>> diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
> >>> index f23126d804b2..d69e99e67707 100644
> >>> --- a/kernel/module/kallsyms.c
> >>> +++ b/kernel/module/kallsyms.c
> >>> @@ -10,6 +10,7 @@
> >>> #include <linux/kallsyms.h>
> >>> #include <linux/buildid.h>
> >>> #include <linux/bsearch.h>
> >>> +#include <linux/sort.h>
> >>> #include "internal.h"
> >>>
> >>> /* Lookup exported symbol in given range of kernel_symbols */
> >>> @@ -103,6 +104,95 @@ static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs,
> >>> return true;
> >>> }
> >>>
> >>> +static inline bool is_func_symbol(const Elf_Sym *sym)
> >>> +{
> >>> + return sym->st_shndx != SHN_UNDEF && sym->st_size != 0 &&
> >>> + ELF_ST_TYPE(sym->st_info) == STT_FUNC;
> >>> +}
> >>> +
> >>> +static unsigned int bsearch_func_symbol(struct mod_kallsyms *kallsyms,
> >>> + unsigned long addr,
> >>> + unsigned long *bestval,
> >>> + unsigned long *nextval)
> >>> +
> >>> +{
> >>> + unsigned int mid, low = 1, high = kallsyms->num_func_syms + 1;
> >>> + unsigned int best = 0;
> >>> + unsigned long thisval;
> >>> +
> >>> + while (low < high) {
> >>> + mid = low + (high - low) / 2;
> >>> + thisval = kallsyms_symbol_value(&kallsyms->symtab[mid]);
> >>> +
> >>> + if (thisval <= addr) {
> >>> + *bestval = thisval;
> >>> + best = mid;
> >>> + low = mid + 1;
> >>
> >> If thisval == addr, the search moves to the right and finds the last
> >> symbol with the same address. I believe it should do the opposite and
> >> return the first symbol to match the behavior of
> >> search_kallsyms_symbol().
> >
> > In the case of multiple symbols sharing the same address, we have
> > to pick one and ignore the others. I don’t think it matters much which
> > one is chosen in practice. Also, I expect function symbol addresses
> > to be unique, so this shouldn’t be a real issue.
>
> I think that the code should consistently pick the same answer. If
> someone uses aliases for their functions, the logic shouldn't
> arbitrarily return one of them, but preferably the first one, which
> should normally be the actual implementation.
>
> >
> >>> + } else {
> >>> + *nextval = thisval;
> >>> + high = mid;
> >>> + }
> >>> + }
> >>> +
> >>> + return best;
> >>> +}
> >>> +
> >>> +static const char *kallsyms_symbol_name(struct mod_kallsyms *kallsyms,
> >>> + unsigned int symnum)
> >>> +{
> >>> + return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
> >>> +}
> >>> +
> >>> +static unsigned int search_kallsyms_symbol(struct mod_kallsyms *kallsyms,
> >>> + unsigned long addr,
> >>> + unsigned long *bestval,
> >>> + unsigned long *nextval)
> >>> +{
> >>> + unsigned int i, best = 0;
> >>> +
> >>> + /*
> >>> + * Scan for closest preceding symbol and next symbol. (ELF starts
> >>> + * real symbols at 1). Skip the initial function symbols range
> >>> + * if num_func_syms is non-zero, those are handled separately for
> >>> + * the core TEXT segment lookup.
> >>> + */
> >>> + for (i = 1 + kallsyms->num_func_syms; i < kallsyms->num_symtab; i++) {
> >>> + const Elf_Sym *sym = &kallsyms->symtab[i];
> >>> + unsigned long thisval = kallsyms_symbol_value(sym);
> >>> +
> >>> + if (sym->st_shndx == SHN_UNDEF)
> >>> + continue;
> >>> +
> >>> + /*
> >>> + * We ignore unnamed symbols: they're uninformative
> >>> + * and inserted at a whim.
> >>> + */
> >>> + if (*kallsyms_symbol_name(kallsyms, i) == '\0' ||
> >>> + is_mapping_symbol(kallsyms_symbol_name(kallsyms, i)))
> >>> + continue;
> >>> +
> >>> + if (thisval <= addr && thisval > *bestval) {
> >>> + best = i;
> >>> + *bestval = thisval;
> >>> + }
> >>> + if (thisval > addr && thisval < *nextval)
> >>> + *nextval = thisval;
> >>> + }
> >>> +
> >>> + return best;
> >>> +}
> >>> +
> >>> +static int elf_sym_cmp(const void *a, const void *b)
> >>> +{
> >>> + unsigned long val_a = kallsyms_symbol_value((const Elf_Sym *)a);
> >>> + unsigned long val_b = kallsyms_symbol_value((const Elf_Sym *)b);
> >>> +
> >>> + if (val_a < val_b)
> >>> + return -1;
> >>> +
> >>> + return val_a > val_b;
> >>
> >> Does this comparison function and the sort() call result in stable
> >> sorting? If val_a and val_b are the same, the sorting should preserve
> >> the original order.
> >
> > The kernel’s sort() implementation is not stable.
>
> Ok, I see it is a heapsort. It would require additional data to keep
> information about the original indexes for elf_sym_cmp() to use as
> a tiebreaker.
>
> >
> >>> +}
> >>> +
> >>> /*
> >>> * We only allocate and copy the strings needed by the parts of symtab
> >>> * we keep. This is simple, but has the effect of making multiple
> >>> @@ -115,9 +205,10 @@ void layout_symtab(struct module *mod, struct load_info *info)
> >>> Elf_Shdr *symsect = info->sechdrs + info->index.sym;
> >>> Elf_Shdr *strsect = info->sechdrs + info->index.str;
> >>> const Elf_Sym *src;
> >>> - unsigned int i, nsrc, ndst, strtab_size = 0;
> >>> + unsigned int i, nsrc, ndst, nfunc, strtab_size = 0;
> >>> struct module_memory *mod_mem_data = &mod->mem[MOD_DATA];
> >>> struct module_memory *mod_mem_init_data = &mod->mem[MOD_INIT_DATA];
> >>> + bool is_lp_mod = is_livepatch_module(mod);
> >>>
> >>> /* Put symbol section at end of init part of module. */
> >>> symsect->sh_flags |= SHF_ALLOC;
> >>> @@ -129,12 +220,14 @@ void layout_symtab(struct module *mod, struct load_info *info)
> >>> nsrc = symsect->sh_size / sizeof(*src);
> >>>
> >>> /* Compute total space required for the core symbols' strtab. */
> >>> - for (ndst = i = 0; i < nsrc; i++) {
> >>> - if (i == 0 || is_livepatch_module(mod) ||
> >>> + for (ndst = nfunc = i = 0; i < nsrc; i++) {
> >>> + if (i == 0 || is_lp_mod ||
> >>> is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
> >>> info->index.pcpu)) {
> >>> strtab_size += strlen(&info->strtab[src[i].st_name]) + 1;
> >>> ndst++;
> >>> + if (!is_lp_mod && is_func_symbol(src + i))
> >>> + nfunc++;
> >>> }
> >>> }
> >>>
> >>> @@ -156,6 +249,7 @@ void layout_symtab(struct module *mod, struct load_info *info)
> >>> mod_mem_init_data->size = ALIGN(mod_mem_init_data->size,
> >>> __alignof__(struct mod_kallsyms));
> >>> info->mod_kallsyms_init_off = mod_mem_init_data->size;
> >>> + info->num_func_syms = nfunc;
> >>>
> >>> mod_mem_init_data->size += sizeof(struct mod_kallsyms);
> >>> info->init_typeoffs = mod_mem_init_data->size;
> >>> @@ -169,7 +263,7 @@ void layout_symtab(struct module *mod, struct load_info *info)
> >>> */
> >>> void add_kallsyms(struct module *mod, const struct load_info *info)
> >>> {
> >>> - unsigned int i, ndst;
> >>> + unsigned int i, di, nfunc, ndst;
> >>> const Elf_Sym *src;
> >>> Elf_Sym *dst;
> >>> char *s;
> >>> @@ -178,6 +272,7 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
> >>> void *data_base = mod->mem[MOD_DATA].base;
> >>> void *init_data_base = mod->mem[MOD_INIT_DATA].base;
> >>> struct mod_kallsyms *kallsyms;
> >>> + bool is_lp_mod = is_livepatch_module(mod);
> >>>
> >>> kallsyms = init_data_base + info->mod_kallsyms_init_off;
> >>
> >> This code is followed by the initialization of kallsyms:
> >>
> >> kallsyms->symtab = (void *)symsec->sh_addr;
> >> kallsyms->num_symtab = symsec->sh_size / sizeof(Elf_Sym);
> >> /* Make sure we get permanent strtab: don't use info->strtab. */
> >> kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
> >> kallsyms->typetab = init_data_base + info->init_typeoffs;
> >>
> >> I suggest adding 'kallsyms->num_func_syms = 0;' after the initialization
> >> of kallsyms->num_symtab.
> >
> > I relied on zeroed memory initialization, but I can add this explicitly
> > for clarity.
> >
> >>> @@ -194,19 +289,28 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
> >>> mod->core_kallsyms.symtab = dst = data_base + info->symoffs;
> >>> mod->core_kallsyms.strtab = s = data_base + info->stroffs;
> >>> mod->core_kallsyms.typetab = data_base + info->core_typeoffs;
> >>> +
> >>> strtab_size = info->core_typeoffs - info->stroffs;
> >>> src = kallsyms->symtab;
> >>> - for (ndst = i = 0; i < kallsyms->num_symtab; i++) {
> >>> + ndst = info->num_func_syms + 1;
> >>> +
> >>> + for (nfunc = i = 0; i < kallsyms->num_symtab; i++) {
> >>> kallsyms->typetab[i] = elf_type(src + i, info);
> >>> - if (i == 0 || is_livepatch_module(mod) ||
> >>> + if (i == 0 || is_lp_mod ||
> >>> is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
> >>> info->index.pcpu)) {
> >>> ssize_t ret;
> >>>
> >>> - mod->core_kallsyms.typetab[ndst] =
> >>> - kallsyms->typetab[i];
> >>> - dst[ndst] = src[i];
> >>> - dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
> >>> + if (i == 0)
> >>> + di = 0;
> >>> + else if (!is_lp_mod && is_func_symbol(src + i))
> >>> + di = 1 + nfunc++;
> >>> + else
> >>> + di = ndst++;
> >>> +
> >>> + mod->core_kallsyms.typetab[di] = kallsyms->typetab[i];
> >>> + dst[di] = src[i];
> >>> + dst[di].st_name = s - mod->core_kallsyms.strtab;
> >>> ret = strscpy(s, &kallsyms->strtab[src[i].st_name],
> >>> strtab_size);
> >>> if (ret < 0)
> >>> @@ -216,9 +320,13 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
> >>> }
> >>> }
> >>>
> >>> + WARN_ON_ONCE(nfunc != info->num_func_syms);
> >>> + sort(dst + 1, nfunc, sizeof(Elf_Sym), elf_sym_cmp, NULL);
> >>> +
> >>
> >> The code sorts mod->core_kallsyms.symtab but mod->core_kallsyms.typetab
> >> is not reordered accordingly.
> >
> > Right, but for function symbols the typetab entries are all 't',
> > so swapping them does not change the type value. The 'T' vs 't'
> > distinction is handled later when printing (based on export status).
> > But the comment explaining skiping adjusting of
> > mod->core_kallsyms.typetab is needed.
>
> Modules can also contain weak functions with elf_type() = 'w'.
Good point.
> >>> /* Set up to point into init section. */
> >>> rcu_assign_pointer(mod->kallsyms, kallsyms);
> >>> mod->core_kallsyms.num_symtab = ndst;
> >>> + mod->core_kallsyms.num_func_syms = nfunc;
> >>> }
> >>>
> >>> #if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
> >>> @@ -241,11 +349,6 @@ void init_build_id(struct module *mod, const struct load_info *info)
> >>> }
> >>> #endif
> >>>
> >>> -static const char *kallsyms_symbol_name(struct mod_kallsyms *kallsyms, unsigned int symnum)
> >>> -{
> >>> - return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
> >>> -}
> >>> -
> >>> /*
> >>> * Given a module and address, find the corresponding symbol and return its name
> >>> * while providing its size and offset if needed.
> >>> @@ -255,7 +358,10 @@ static const char *find_kallsyms_symbol(struct module *mod,
> >>> unsigned long *size,
> >>> unsigned long *offset)
> >>> {
> >>> - unsigned int i, best = 0;
> >>> + unsigned int (*search)(struct mod_kallsyms *kallsyms,
> >>> + unsigned long addr, unsigned long *bestval,
> >>> + unsigned long *nextval);
> >>> + unsigned int best;
> >>> unsigned long nextval, bestval;
> >>> struct mod_kallsyms *kallsyms = rcu_dereference(mod->kallsyms);
> >>> struct module_memory *mod_mem = NULL;
> >>> @@ -266,6 +372,11 @@ static const char *find_kallsyms_symbol(struct module *mod,
> >>> continue;
> >>> #endif
> >>> if (within_module_mem_type(addr, mod, type)) {
> >>> + if (type == MOD_TEXT && kallsyms->num_func_syms > 0)
> >>> + search = bsearch_func_symbol;
> >>
> >> I'm not sure if it is ok to limit the search only to function symbols
> >> when the address lies in MOD_TEXT. The text can theoretically contain
> >> non-function symbols.
> >
> > Yes, the patch assumes that the only valid symbols in the MOD_TEXT
> > are functions. If there are defined OBJECT symbols in .text, the patch
> > would break lookup for those.
> >
> > While it’s theoretically possible (e.g. hand-written assembly placing
> > data in .text ?), I’m not sure this is a practical concern. In general,
> > having data in executable segments is discouraged for security reasons.
> >
> >> Could this optimization be adjusted to sort all
> >> MOD_TEXT symbols (excluding anonymous and mapping symbols) and move them
> >> to the front of the symbol table?
> >
> > That’s possible. We could track .text sections indices in
> > __layout_sections() and include all valid symbols from those sections,
> > and also reorder typetab accordingly.
> >
> > However, this adds complexity. I would prefer to first confirm whether
> > OBJECT symbols in MOD_TEXT is a real issue before going in that direction.
>
> I'm not aware of specific OBJECT symbols that end up in MOD_TEXT.
> Nonetheless, it is a valid case and it is preferable that an
> optimization doesn't break their lookup by address.
>
> In general, I'm worried about the several edge cases and inconsistencies
> that this optimization introduces. This also includes the fact that it
> doesn't work for livepatch modules.
>
> An alternative could be to keep the symbol table untouched and have
> a separate array with symbol indexes that is sorted by their addresses,
> but it requires evaluation if the additional memory usage is worth it.
I personally prefer not to over-engineer for cases that may never occur.
That said, your concerns about edge cases, consistency, and livepatch
handling are valid.
Let me take some time to think this through and experiment with possible
solutions that handle these cases properly. I can’t work on this
immediately, but I’ll get back to it.
Regards
Stanislaw
^ permalink raw reply
* Re: [PATCH v4 1/4] kernel: param: initialize module_kset before do_initcalls()
From: Shashank Balaji @ 2026-04-28 0:37 UTC (permalink / raw)
To: Gary Guo
Cc: Suzuki K Poulose, James Clark, Alexander Shishkin,
Maxime Coquelin, Alexandre Torgue, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Miguel Ojeda, Boqun Feng,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Richard Cochran, Jonathan Corbet, Shuah Khan,
Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
Aaron Tomlin, Mike Leach, Leo Yan, Rahul Bukte, linux-kernel,
coresight, linux-arm-kernel, driver-core, rust-for-linux,
linux-doc, Daniel Palmer, Tim Bird, linux-modules
In-Reply-To: <DI3Z28IZZOT9.349TTWNN9VDMB@garyguo.net>
Hi Gary,
On Mon, Apr 27, 2026 at 02:29:55PM +0100, Gary Guo wrote:
> On Mon Apr 27, 2026 at 3:41 AM BST, Shashank Balaji wrote:
> > module_kset is initialized in param_sysfs_init(), a subsys_initcall. A number
> > of platform drivers register themselves prior to subsys_initcalls
> > (tegra194_cbb_driver registers in a pure_initcall, for example). With an
> > upcoming patch ("driver core: platform: set mod_name in driver registration")
> > that sets their mod_name in struct device_driver, lookup_or_create_module_kobject()
> > will be called for those drivers, which calls kset_find_obj(module_kset, mod_name).
> > This causes a null deref because module_kset isn't alive yet.
> >
> > Fix this by initializing module_kset in do_basic_setup() before do_initcalls().
> > Modernize the pr_warn while we're at it.
> >
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Suggested-by: Gary Guo <gary@garyguo.net>
>
> I didn't suggest this change :)
>
> I suggested `pure_initcall`, which is just a one line change.
Oops, sorry about the misattribution.
> diff --git a/kernel/params.c b/kernel/params.c
> index 74d620bc2521..ac088d4b09a9 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -957,7 +957,7 @@ static int __init param_sysfs_init(void)
>
> return 0;
> }
> -subsys_initcall(param_sysfs_init);
> +pure_initcall(param_sysfs_init);
>
> /*
> * param_sysfs_builtin_init - add sysfs version and parameter
>
> pure_initcall is level 0 so it happens before all other init calls. Does it not
> work?
tegra194_cbb_driver registers itself in a pure_initcall too. We wouldn't
want the ordering of its registration and module_kset init to be link order
dependent.
Thanks,
Shashank
^ permalink raw reply
* Re: [PATCH v2 2/2] module/kallsyms: sort function symbols and use binary search
From: Petr Pavlu @ 2026-04-27 13:51 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: linux-modules, Sami Tolvanen, Luis Chamberlain, linux-kernel,
linux-trace-kernel, live-patching, Daniel Gomez, Aaron Tomlin,
Steven Rostedt, Masami Hiramatsu, Jordan Rome, Viktor Malik
In-Reply-To: <20260424091330.GA31168@wp.pl>
On 4/24/26 11:13 AM, Stanislaw Gruszka wrote:
> On Thu, Apr 23, 2026 at 04:00:04PM +0200, Petr Pavlu wrote:
>> On 3/27/26 12:00 PM, Stanislaw Gruszka wrote:
[...]
>>> diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
>>> index f23126d804b2..d69e99e67707 100644
>>> --- a/kernel/module/kallsyms.c
>>> +++ b/kernel/module/kallsyms.c
>>> @@ -10,6 +10,7 @@
>>> #include <linux/kallsyms.h>
>>> #include <linux/buildid.h>
>>> #include <linux/bsearch.h>
>>> +#include <linux/sort.h>
>>> #include "internal.h"
>>>
>>> /* Lookup exported symbol in given range of kernel_symbols */
>>> @@ -103,6 +104,95 @@ static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs,
>>> return true;
>>> }
>>>
>>> +static inline bool is_func_symbol(const Elf_Sym *sym)
>>> +{
>>> + return sym->st_shndx != SHN_UNDEF && sym->st_size != 0 &&
>>> + ELF_ST_TYPE(sym->st_info) == STT_FUNC;
>>> +}
>>> +
>>> +static unsigned int bsearch_func_symbol(struct mod_kallsyms *kallsyms,
>>> + unsigned long addr,
>>> + unsigned long *bestval,
>>> + unsigned long *nextval)
>>> +
>>> +{
>>> + unsigned int mid, low = 1, high = kallsyms->num_func_syms + 1;
>>> + unsigned int best = 0;
>>> + unsigned long thisval;
>>> +
>>> + while (low < high) {
>>> + mid = low + (high - low) / 2;
>>> + thisval = kallsyms_symbol_value(&kallsyms->symtab[mid]);
>>> +
>>> + if (thisval <= addr) {
>>> + *bestval = thisval;
>>> + best = mid;
>>> + low = mid + 1;
>>
>> If thisval == addr, the search moves to the right and finds the last
>> symbol with the same address. I believe it should do the opposite and
>> return the first symbol to match the behavior of
>> search_kallsyms_symbol().
>
> In the case of multiple symbols sharing the same address, we have
> to pick one and ignore the others. I don’t think it matters much which
> one is chosen in practice. Also, I expect function symbol addresses
> to be unique, so this shouldn’t be a real issue.
I think that the code should consistently pick the same answer. If
someone uses aliases for their functions, the logic shouldn't
arbitrarily return one of them, but preferably the first one, which
should normally be the actual implementation.
>
>>> + } else {
>>> + *nextval = thisval;
>>> + high = mid;
>>> + }
>>> + }
>>> +
>>> + return best;
>>> +}
>>> +
>>> +static const char *kallsyms_symbol_name(struct mod_kallsyms *kallsyms,
>>> + unsigned int symnum)
>>> +{
>>> + return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
>>> +}
>>> +
>>> +static unsigned int search_kallsyms_symbol(struct mod_kallsyms *kallsyms,
>>> + unsigned long addr,
>>> + unsigned long *bestval,
>>> + unsigned long *nextval)
>>> +{
>>> + unsigned int i, best = 0;
>>> +
>>> + /*
>>> + * Scan for closest preceding symbol and next symbol. (ELF starts
>>> + * real symbols at 1). Skip the initial function symbols range
>>> + * if num_func_syms is non-zero, those are handled separately for
>>> + * the core TEXT segment lookup.
>>> + */
>>> + for (i = 1 + kallsyms->num_func_syms; i < kallsyms->num_symtab; i++) {
>>> + const Elf_Sym *sym = &kallsyms->symtab[i];
>>> + unsigned long thisval = kallsyms_symbol_value(sym);
>>> +
>>> + if (sym->st_shndx == SHN_UNDEF)
>>> + continue;
>>> +
>>> + /*
>>> + * We ignore unnamed symbols: they're uninformative
>>> + * and inserted at a whim.
>>> + */
>>> + if (*kallsyms_symbol_name(kallsyms, i) == '\0' ||
>>> + is_mapping_symbol(kallsyms_symbol_name(kallsyms, i)))
>>> + continue;
>>> +
>>> + if (thisval <= addr && thisval > *bestval) {
>>> + best = i;
>>> + *bestval = thisval;
>>> + }
>>> + if (thisval > addr && thisval < *nextval)
>>> + *nextval = thisval;
>>> + }
>>> +
>>> + return best;
>>> +}
>>> +
>>> +static int elf_sym_cmp(const void *a, const void *b)
>>> +{
>>> + unsigned long val_a = kallsyms_symbol_value((const Elf_Sym *)a);
>>> + unsigned long val_b = kallsyms_symbol_value((const Elf_Sym *)b);
>>> +
>>> + if (val_a < val_b)
>>> + return -1;
>>> +
>>> + return val_a > val_b;
>>
>> Does this comparison function and the sort() call result in stable
>> sorting? If val_a and val_b are the same, the sorting should preserve
>> the original order.
>
> The kernel’s sort() implementation is not stable.
Ok, I see it is a heapsort. It would require additional data to keep
information about the original indexes for elf_sym_cmp() to use as
a tiebreaker.
>
>>> +}
>>> +
>>> /*
>>> * We only allocate and copy the strings needed by the parts of symtab
>>> * we keep. This is simple, but has the effect of making multiple
>>> @@ -115,9 +205,10 @@ void layout_symtab(struct module *mod, struct load_info *info)
>>> Elf_Shdr *symsect = info->sechdrs + info->index.sym;
>>> Elf_Shdr *strsect = info->sechdrs + info->index.str;
>>> const Elf_Sym *src;
>>> - unsigned int i, nsrc, ndst, strtab_size = 0;
>>> + unsigned int i, nsrc, ndst, nfunc, strtab_size = 0;
>>> struct module_memory *mod_mem_data = &mod->mem[MOD_DATA];
>>> struct module_memory *mod_mem_init_data = &mod->mem[MOD_INIT_DATA];
>>> + bool is_lp_mod = is_livepatch_module(mod);
>>>
>>> /* Put symbol section at end of init part of module. */
>>> symsect->sh_flags |= SHF_ALLOC;
>>> @@ -129,12 +220,14 @@ void layout_symtab(struct module *mod, struct load_info *info)
>>> nsrc = symsect->sh_size / sizeof(*src);
>>>
>>> /* Compute total space required for the core symbols' strtab. */
>>> - for (ndst = i = 0; i < nsrc; i++) {
>>> - if (i == 0 || is_livepatch_module(mod) ||
>>> + for (ndst = nfunc = i = 0; i < nsrc; i++) {
>>> + if (i == 0 || is_lp_mod ||
>>> is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
>>> info->index.pcpu)) {
>>> strtab_size += strlen(&info->strtab[src[i].st_name]) + 1;
>>> ndst++;
>>> + if (!is_lp_mod && is_func_symbol(src + i))
>>> + nfunc++;
>>> }
>>> }
>>>
>>> @@ -156,6 +249,7 @@ void layout_symtab(struct module *mod, struct load_info *info)
>>> mod_mem_init_data->size = ALIGN(mod_mem_init_data->size,
>>> __alignof__(struct mod_kallsyms));
>>> info->mod_kallsyms_init_off = mod_mem_init_data->size;
>>> + info->num_func_syms = nfunc;
>>>
>>> mod_mem_init_data->size += sizeof(struct mod_kallsyms);
>>> info->init_typeoffs = mod_mem_init_data->size;
>>> @@ -169,7 +263,7 @@ void layout_symtab(struct module *mod, struct load_info *info)
>>> */
>>> void add_kallsyms(struct module *mod, const struct load_info *info)
>>> {
>>> - unsigned int i, ndst;
>>> + unsigned int i, di, nfunc, ndst;
>>> const Elf_Sym *src;
>>> Elf_Sym *dst;
>>> char *s;
>>> @@ -178,6 +272,7 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
>>> void *data_base = mod->mem[MOD_DATA].base;
>>> void *init_data_base = mod->mem[MOD_INIT_DATA].base;
>>> struct mod_kallsyms *kallsyms;
>>> + bool is_lp_mod = is_livepatch_module(mod);
>>>
>>> kallsyms = init_data_base + info->mod_kallsyms_init_off;
>>
>> This code is followed by the initialization of kallsyms:
>>
>> kallsyms->symtab = (void *)symsec->sh_addr;
>> kallsyms->num_symtab = symsec->sh_size / sizeof(Elf_Sym);
>> /* Make sure we get permanent strtab: don't use info->strtab. */
>> kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
>> kallsyms->typetab = init_data_base + info->init_typeoffs;
>>
>> I suggest adding 'kallsyms->num_func_syms = 0;' after the initialization
>> of kallsyms->num_symtab.
>
> I relied on zeroed memory initialization, but I can add this explicitly
> for clarity.
>
>>> @@ -194,19 +289,28 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
>>> mod->core_kallsyms.symtab = dst = data_base + info->symoffs;
>>> mod->core_kallsyms.strtab = s = data_base + info->stroffs;
>>> mod->core_kallsyms.typetab = data_base + info->core_typeoffs;
>>> +
>>> strtab_size = info->core_typeoffs - info->stroffs;
>>> src = kallsyms->symtab;
>>> - for (ndst = i = 0; i < kallsyms->num_symtab; i++) {
>>> + ndst = info->num_func_syms + 1;
>>> +
>>> + for (nfunc = i = 0; i < kallsyms->num_symtab; i++) {
>>> kallsyms->typetab[i] = elf_type(src + i, info);
>>> - if (i == 0 || is_livepatch_module(mod) ||
>>> + if (i == 0 || is_lp_mod ||
>>> is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
>>> info->index.pcpu)) {
>>> ssize_t ret;
>>>
>>> - mod->core_kallsyms.typetab[ndst] =
>>> - kallsyms->typetab[i];
>>> - dst[ndst] = src[i];
>>> - dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
>>> + if (i == 0)
>>> + di = 0;
>>> + else if (!is_lp_mod && is_func_symbol(src + i))
>>> + di = 1 + nfunc++;
>>> + else
>>> + di = ndst++;
>>> +
>>> + mod->core_kallsyms.typetab[di] = kallsyms->typetab[i];
>>> + dst[di] = src[i];
>>> + dst[di].st_name = s - mod->core_kallsyms.strtab;
>>> ret = strscpy(s, &kallsyms->strtab[src[i].st_name],
>>> strtab_size);
>>> if (ret < 0)
>>> @@ -216,9 +320,13 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
>>> }
>>> }
>>>
>>> + WARN_ON_ONCE(nfunc != info->num_func_syms);
>>> + sort(dst + 1, nfunc, sizeof(Elf_Sym), elf_sym_cmp, NULL);
>>> +
>>
>> The code sorts mod->core_kallsyms.symtab but mod->core_kallsyms.typetab
>> is not reordered accordingly.
>
> Right, but for function symbols the typetab entries are all 't',
> so swapping them does not change the type value. The 'T' vs 't'
> distinction is handled later when printing (based on export status).
> But the comment explaining skiping adjusting of
> mod->core_kallsyms.typetab is needed.
Modules can also contain weak functions with elf_type() = 'w'.
>
>>> /* Set up to point into init section. */
>>> rcu_assign_pointer(mod->kallsyms, kallsyms);
>>> mod->core_kallsyms.num_symtab = ndst;
>>> + mod->core_kallsyms.num_func_syms = nfunc;
>>> }
>>>
>>> #if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
>>> @@ -241,11 +349,6 @@ void init_build_id(struct module *mod, const struct load_info *info)
>>> }
>>> #endif
>>>
>>> -static const char *kallsyms_symbol_name(struct mod_kallsyms *kallsyms, unsigned int symnum)
>>> -{
>>> - return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
>>> -}
>>> -
>>> /*
>>> * Given a module and address, find the corresponding symbol and return its name
>>> * while providing its size and offset if needed.
>>> @@ -255,7 +358,10 @@ static const char *find_kallsyms_symbol(struct module *mod,
>>> unsigned long *size,
>>> unsigned long *offset)
>>> {
>>> - unsigned int i, best = 0;
>>> + unsigned int (*search)(struct mod_kallsyms *kallsyms,
>>> + unsigned long addr, unsigned long *bestval,
>>> + unsigned long *nextval);
>>> + unsigned int best;
>>> unsigned long nextval, bestval;
>>> struct mod_kallsyms *kallsyms = rcu_dereference(mod->kallsyms);
>>> struct module_memory *mod_mem = NULL;
>>> @@ -266,6 +372,11 @@ static const char *find_kallsyms_symbol(struct module *mod,
>>> continue;
>>> #endif
>>> if (within_module_mem_type(addr, mod, type)) {
>>> + if (type == MOD_TEXT && kallsyms->num_func_syms > 0)
>>> + search = bsearch_func_symbol;
>>
>> I'm not sure if it is ok to limit the search only to function symbols
>> when the address lies in MOD_TEXT. The text can theoretically contain
>> non-function symbols.
>
> Yes, the patch assumes that the only valid symbols in the MOD_TEXT
> are functions. If there are defined OBJECT symbols in .text, the patch
> would break lookup for those.
>
> While it’s theoretically possible (e.g. hand-written assembly placing
> data in .text ?), I’m not sure this is a practical concern. In general,
> having data in executable segments is discouraged for security reasons.
>
>> Could this optimization be adjusted to sort all
>> MOD_TEXT symbols (excluding anonymous and mapping symbols) and move them
>> to the front of the symbol table?
>
> That’s possible. We could track .text sections indices in
> __layout_sections() and include all valid symbols from those sections,
> and also reorder typetab accordingly.
>
> However, this adds complexity. I would prefer to first confirm whether
> OBJECT symbols in MOD_TEXT is a real issue before going in that direction.
I'm not aware of specific OBJECT symbols that end up in MOD_TEXT.
Nonetheless, it is a valid case and it is preferable that an
optimization doesn't break their lookup by address.
In general, I'm worried about the several edge cases and inconsistencies
that this optimization introduces. This also includes the fact that it
doesn't work for livepatch modules.
An alternative could be to keep the symbol table untouched and have
a separate array with symbol indexes that is sorted by their addresses,
but it requires evaluation if the additional memory usage is worth it.
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH] rust: module_param: use `pr_warn_once!` for null pointer warning
From: Gary Guo @ 2026-04-27 13:36 UTC (permalink / raw)
To: Andreas Hindborg, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen, Aaron Tomlin, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
Danilo Krummrich
Cc: linux-modules, linux-kernel, rust-for-linux
In-Reply-To: <20260427-params-pr-once-v1-1-3a8c64704cc4@kernel.org>
On Mon Apr 27, 2026 at 9:11 AM BST, Andreas Hindborg wrote:
> Replace `pr_warn!` and the accompanying TODO with `pr_warn_once!`, now that
> the macro is available.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
Reviewed-by: Gary Guo <gary@garyguo.net>
> ---
> rust/kernel/module_param.rs | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
^ permalink raw reply
* Re: [PATCH v4 1/4] kernel: param: initialize module_kset before do_initcalls()
From: Gary Guo @ 2026-04-27 13:29 UTC (permalink / raw)
To: Shashank Balaji, Suzuki K Poulose, James Clark,
Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
Greg Kroah-Hartman, Rafael J. Wysocki, Danilo Krummrich,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Richard Cochran, Jonathan Corbet, Shuah Khan, Luis Chamberlain,
Petr Pavlu, Daniel Gomez, Sami Tolvanen, Aaron Tomlin, Mike Leach,
Leo Yan
Cc: Rahul Bukte, linux-kernel, coresight, linux-arm-kernel,
driver-core, rust-for-linux, linux-doc, Daniel Palmer, Tim Bird,
linux-modules
In-Reply-To: <20260427-acpi_mod_name-v4-1-22b42240c9bf@sony.com>
On Mon Apr 27, 2026 at 3:41 AM BST, Shashank Balaji wrote:
> module_kset is initialized in param_sysfs_init(), a subsys_initcall. A number
> of platform drivers register themselves prior to subsys_initcalls
> (tegra194_cbb_driver registers in a pure_initcall, for example). With an
> upcoming patch ("driver core: platform: set mod_name in driver registration")
> that sets their mod_name in struct device_driver, lookup_or_create_module_kobject()
> will be called for those drivers, which calls kset_find_obj(module_kset, mod_name).
> This causes a null deref because module_kset isn't alive yet.
>
> Fix this by initializing module_kset in do_basic_setup() before do_initcalls().
> Modernize the pr_warn while we're at it.
>
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Suggested-by: Gary Guo <gary@garyguo.net>
I didn't suggest this change :)
I suggested `pure_initcall`, which is just a one line change.
diff --git a/kernel/params.c b/kernel/params.c
index 74d620bc2521..ac088d4b09a9 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -957,7 +957,7 @@ static int __init param_sysfs_init(void)
return 0;
}
-subsys_initcall(param_sysfs_init);
+pure_initcall(param_sysfs_init);
/*
* param_sysfs_builtin_init - add sysfs version and parameter
pure_initcall is level 0 so it happens before all other init calls. Does it not
work?
Best,
Gary
> Co-developed-by: Rahul Bukte <rahul.bukte@sony.com>
> Signed-off-by: Rahul Bukte <rahul.bukte@sony.com>
> Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
> ---
> include/linux/module.h | 4 ++++
> init/main.c | 1 +
> kernel/params.c | 21 +++++++++------------
> 3 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 7566815fabbe..6478596e8f9f 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -886,6 +886,10 @@ static inline void module_for_each_mod(int(*func)(struct module *mod, void *data
> #ifdef CONFIG_SYSFS
> extern struct kset *module_kset;
> extern const struct kobj_type module_ktype;
> +
> +void param_sysfs_init(void);
> +#else
> +static inline void param_sysfs_init(void) {}
> #endif /* CONFIG_SYSFS */
>
> #define symbol_request(x) try_then_request_module(symbol_get(x), "symbol:" #x)
> diff --git a/init/main.c b/init/main.c
> index 96f93bb06c49..01552c6b62ff 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1486,6 +1486,7 @@ static void __init do_basic_setup(void)
> ksysfs_init();
> driver_init();
> init_irq_proc();
> + param_sysfs_init();
> do_ctors();
> do_initcalls();
> }
> diff --git a/kernel/params.c b/kernel/params.c
> index 74d620bc2521..d1e3934fb3a7 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -942,22 +942,19 @@ const struct kobj_type module_ktype = {
> /*
> * param_sysfs_init - create "module" kset
> *
> - * This must be done before the initramfs is unpacked and
> - * request_module() thus becomes possible, because otherwise the
> - * module load would fail in mod_sysfs_init.
> + * Must run before:
> + * - do_initcalls(): some drivers register during initcalls and rely on
> + * module_kset existing for their sysfs module symlink.
> + * - rootfs_initcall (initramfs unpack): request_module() becomes possible.
> + * But if module_kset is null, module load would fail in mod_sysfs_init(),
> + * causing request_module() to fail.
> */
> -static int __init param_sysfs_init(void)
> +void __init param_sysfs_init(void)
> {
> module_kset = kset_create_and_add("module", &module_uevent_ops, NULL);
> - if (!module_kset) {
> - printk(KERN_WARNING "%s (%d): error creating kset\n",
> - __FILE__, __LINE__);
> - return -ENOMEM;
> - }
> -
> - return 0;
> + if (!module_kset)
> + pr_warn("Error creating module kset\n");
> }
> -subsys_initcall(param_sysfs_init);
>
> /*
> * param_sysfs_builtin_init - add sysfs version and parameter
^ permalink raw reply related
* Re: [PATCH] rust: module_param: use `pr_warn_once!` for null pointer warning
From: Daniel Gomez @ 2026-04-27 11:28 UTC (permalink / raw)
To: Andreas Hindborg, Luis Chamberlain, Petr Pavlu, Sami Tolvanen,
Aaron Tomlin, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
Danilo Krummrich
Cc: linux-modules, linux-kernel, rust-for-linux
In-Reply-To: <20260427-params-pr-once-v1-1-3a8c64704cc4@kernel.org>
On 27/04/2026 10.11, Andreas Hindborg wrote:
> Replace `pr_warn!` and the accompanying TODO with `pr_warn_once!`, now that
> the macro is available.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
Available since v7.0-rc1. LGTM,
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply
* Re: [PATCH v4 2/4] coresight: pass THIS_MODULE implicitly through a macro
From: Leo Yan @ 2026-04-27 10:49 UTC (permalink / raw)
To: Shashank Balaji
Cc: Suzuki K Poulose, James Clark, Alexander Shishkin,
Maxime Coquelin, Alexandre Torgue, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Richard Cochran, Jonathan Corbet,
Shuah Khan, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen, Aaron Tomlin, Mike Leach, Rahul Bukte,
linux-kernel, coresight, linux-arm-kernel, driver-core,
rust-for-linux, linux-doc, Daniel Palmer, Tim Bird, linux-modules
In-Reply-To: <20260427-acpi_mod_name-v4-2-22b42240c9bf@sony.com>
Hi Shashank,
On Mon, Apr 27, 2026 at 11:41:22AM +0900, Shashank Balaji wrote:
[...]
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1694,7 +1694,7 @@ static void __exit coresight_exit(void)
> module_init(coresight_init);
> module_exit(coresight_exit);
>
> -int coresight_init_driver(const char *drv, struct amba_driver *amba_drv,
> +int __coresight_init_driver(const char *drv, struct amba_driver *amba_drv,
> struct platform_driver *pdev_drv, struct module *owner)
> {
> int ret;
> @@ -1713,7 +1713,7 @@ int coresight_init_driver(const char *drv, struct amba_driver *amba_drv,
> amba_driver_unregister(amba_drv);
> return ret;
> }
> -EXPORT_SYMBOL_GPL(coresight_init_driver);
> +EXPORT_SYMBOL_GPL(__coresight_init_driver);
For consistency, we usually use prefix "__" for internal functions.
Could you rename this function as:
int coresight_init_driver_with_owner(..., struct module *owner);
> +#define coresight_init_driver(drv, amba_drv, pdev_drv) \
> + __coresight_init_driver(drv, amba_drv, pdev_drv, THIS_MODULE)
> +int __coresight_init_driver(const char *drv, struct amba_driver *amba_drv,
> struct platform_driver *pdev_drv, struct module *owner);
Please first function declaration, then followed by the macro to
define coresight_init_driver().
With above changes:
Reviewed-by: Leo Yan <leo.yan@arm.com>
^ permalink raw reply
* [PATCH] rust: module_param: use `pr_warn_once!` for null pointer warning
From: Andreas Hindborg @ 2026-04-27 8:11 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
Aaron Tomlin, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
Danilo Krummrich
Cc: linux-modules, linux-kernel, rust-for-linux, Andreas Hindborg
Replace `pr_warn!` and the accompanying TODO with `pr_warn_once!`, now that
the macro is available.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/module_param.rs | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
index 6a8a7a875643..dd6d663a0a3c 100644
--- a/rust/kernel/module_param.rs
+++ b/rust/kernel/module_param.rs
@@ -62,8 +62,7 @@ pub trait ModuleParam: Sized + Copy {
// NOTE: If we start supporting arguments without values, val _is_ allowed
// to be null here.
if val.is_null() {
- // TODO: Use pr_warn_once available.
- crate::pr_warn!("Null pointer passed to `module_param::set_param`");
+ crate::pr_warn_once!("Null pointer passed to `module_param::set_param`");
return EINVAL.to_errno();
}
---
base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
change-id: 20260427-params-pr-once-481c03df3e2a
Best regards,
--
Andreas Hindborg <a.hindborg@kernel.org>
^ permalink raw reply related
* Re: [PATCH 1/1] rust: module_param: support bool parameters
From: Andreas Hindborg @ 2026-04-27 8:04 UTC (permalink / raw)
To: Wenzhao Liao, mcgrof, petr.pavlu, da.gomez, samitolvanen, ojeda,
linux-modules, rust-for-linux
Cc: atomlin, boqun, gary, bjorn3_gh, lossin, aliceryhl, tmgross, dakr,
linux-kernel
In-Reply-To: <20260411130254.3510128-2-wenzhaoliao@ruc.edu.cn>
Wenzhao Liao <wenzhaoliao@ruc.edu.cn> writes:
> Add support for parsing boolean module parameters in the Rust
> module! macro.
>
> Currently, only integer types are supported by the `module_param!`
> macros. This patch implements the `ModuleParam` trait for `bool`
> by delegating the string parsing to the existing C implementation
> via `kstrtobool_bytes()`. It also wires up `PARAM_OPS_BOOL` so that
> the Rust parameter system correctly links to the C `param_ops_bool`
> structure.
>
> For demonstration and verification, a boolean parameter is added
> to `samples/rust/rust_minimal.rs`.
>
> Assisted-by: Codex:GPT-5
> Signed-off-by: Wenzhao Liao <wenzhaoliao@ruc.edu.cn>
Looks good to me!
Tested-by: Andreas Hindborg <a.hindborg@kernel.org>
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Best regards,
Andreas Hindborg
^ permalink raw reply
* [PATCH v4 4/4] docs: driver-api: add mod_name argument to __platform_register_drivers()
From: Shashank Balaji @ 2026-04-27 2:41 UTC (permalink / raw)
To: Suzuki K Poulose, James Clark, Alexander Shishkin,
Maxime Coquelin, Alexandre Torgue, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Richard Cochran, Jonathan Corbet,
Shuah Khan, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen, Aaron Tomlin, Mike Leach, Leo Yan, Mike Leach
Cc: Rahul Bukte, Shashank Balaji, linux-kernel, coresight,
linux-arm-kernel, driver-core, rust-for-linux, linux-doc,
Daniel Palmer, Tim Bird, linux-modules
In-Reply-To: <20260427-acpi_mod_name-v4-0-22b42240c9bf@sony.com>
Sync the function signature of __platform_register_driver() between the code and
the doc.
Co-developed-by: Rahul Bukte <rahul.bukte@sony.com>
Signed-off-by: Rahul Bukte <rahul.bukte@sony.com>
Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
---
Documentation/driver-api/driver-model/platform.rst | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/driver-api/driver-model/platform.rst b/Documentation/driver-api/driver-model/platform.rst
index cf5ff48d3115..9673470bded2 100644
--- a/Documentation/driver-api/driver-model/platform.rst
+++ b/Documentation/driver-api/driver-model/platform.rst
@@ -70,7 +70,8 @@ Kernel modules can be composed of several platform drivers. The platform core
provides helpers to register and unregister an array of drivers::
int __platform_register_drivers(struct platform_driver * const *drivers,
- unsigned int count, struct module *owner);
+ unsigned int count, struct module *owner,
+ const char *mod_name);
void platform_unregister_drivers(struct platform_driver * const *drivers,
unsigned int count);
--
2.43.0
^ permalink raw reply related
* [PATCH v4 3/4] driver core: platform: set mod_name in driver registration
From: Shashank Balaji @ 2026-04-27 2:41 UTC (permalink / raw)
To: Suzuki K Poulose, James Clark, Alexander Shishkin,
Maxime Coquelin, Alexandre Torgue, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Richard Cochran, Jonathan Corbet,
Shuah Khan, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen, Aaron Tomlin, Mike Leach, Leo Yan, Mike Leach
Cc: Rahul Bukte, Shashank Balaji, linux-kernel, coresight,
linux-arm-kernel, driver-core, rust-for-linux, linux-doc,
Daniel Palmer, Tim Bird, linux-modules
In-Reply-To: <20260427-acpi_mod_name-v4-0-22b42240c9bf@sony.com>
Pass KBUILD_MODNAME through the driver registration macro so that
the driver core can create the module symlink in sysfs for built-in
drivers, and fixup all callers.
The Rust platform adapter is updated to pass the module name through to the new
parameter.
Tested on qemu with:
- x86 defconfig + CONFIG_RUST
- arm64 defconfig + CONFIG_RUST + CONFIG_CORESIGHT stuff
Examples after this patch:
/sys/bus/platform/drivers/...
coresight-itnoc/module -> coresight_tnoc
coresight-static-tpdm/module -> coresight_tpdm
coresight-catu-platform/module -> coresight_catu
serial8250/module -> 8250
acpi-ged/module -> acpi
vmclock/module -> ptp_vmclock
Co-developed-by: Rahul Bukte <rahul.bukte@sony.com>
Signed-off-by: Rahul Bukte <rahul.bukte@sony.com>
Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
---
This patch depends on patches 1 and 2.
---
drivers/base/platform.c | 21 ++++++++++++++-------
drivers/hwtracing/coresight/coresight-core.c | 5 +++--
include/linux/coresight.h | 5 +++--
include/linux/platform_device.h | 17 +++++++++--------
rust/kernel/platform.rs | 4 +++-
5 files changed, 32 insertions(+), 20 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 75b4698d0e58..2b0cc0889386 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -901,11 +901,14 @@ EXPORT_SYMBOL_GPL(platform_device_register_full);
* __platform_driver_register - register a driver for platform-level devices
* @drv: platform driver structure
* @owner: owning module/driver
+ * @mod_name: module name string
*/
-int __platform_driver_register(struct platform_driver *drv, struct module *owner)
+int __platform_driver_register(struct platform_driver *drv, struct module *owner,
+ const char *mod_name)
{
drv->driver.owner = owner;
drv->driver.bus = &platform_bus_type;
+ drv->driver.mod_name = mod_name;
return driver_register(&drv->driver);
}
@@ -938,6 +941,7 @@ static int is_bound_to_driver(struct device *dev, void *driver)
* @drv: platform driver structure
* @probe: the driver probe routine, probably from an __init section
* @module: module which will be the owner of the driver
+ * @mod_name: module name string
*
* Use this instead of platform_driver_register() when you know the device
* is not hotpluggable and has already been registered, and you want to
@@ -955,7 +959,8 @@ static int is_bound_to_driver(struct device *dev, void *driver)
*/
int __init_or_module __platform_driver_probe(struct platform_driver *drv,
int (*probe)(struct platform_device *),
- struct module *module)
+ struct module *module,
+ const char *mod_name)
{
int retval;
@@ -983,7 +988,7 @@ int __init_or_module __platform_driver_probe(struct platform_driver *drv,
/* temporary section violation during probe() */
drv->probe = probe;
- retval = __platform_driver_register(drv, module);
+ retval = __platform_driver_register(drv, module, mod_name);
if (retval)
return retval;
@@ -1011,6 +1016,7 @@ EXPORT_SYMBOL_GPL(__platform_driver_probe);
* @data: platform specific data for this platform device
* @size: size of platform specific data
* @module: module which will be the owner of the driver
+ * @mod_name: module name string
*
* Use this in legacy-style modules that probe hardware directly and
* register a single platform device and corresponding platform driver.
@@ -1021,7 +1027,7 @@ struct platform_device * __init_or_module
__platform_create_bundle(struct platform_driver *driver,
int (*probe)(struct platform_device *),
struct resource *res, unsigned int n_res,
- const void *data, size_t size, struct module *module)
+ const void *data, size_t size, struct module *module, const char *mod_name)
{
struct platform_device *pdev;
int error;
@@ -1044,7 +1050,7 @@ __platform_create_bundle(struct platform_driver *driver,
if (error)
goto err_pdev_put;
- error = __platform_driver_probe(driver, probe, module);
+ error = __platform_driver_probe(driver, probe, module, mod_name);
if (error)
goto err_pdev_del;
@@ -1064,6 +1070,7 @@ EXPORT_SYMBOL_GPL(__platform_create_bundle);
* @drivers: an array of drivers to register
* @count: the number of drivers to register
* @owner: module owning the drivers
+ * @mod_name: module name string
*
* Registers platform drivers specified by an array. On failure to register a
* driver, all previously registered drivers will be unregistered. Callers of
@@ -1073,7 +1080,7 @@ EXPORT_SYMBOL_GPL(__platform_create_bundle);
* Returns: 0 on success or a negative error code on failure.
*/
int __platform_register_drivers(struct platform_driver * const *drivers,
- unsigned int count, struct module *owner)
+ unsigned int count, struct module *owner, const char *mod_name)
{
unsigned int i;
int err;
@@ -1081,7 +1088,7 @@ int __platform_register_drivers(struct platform_driver * const *drivers,
for (i = 0; i < count; i++) {
pr_debug("registering platform driver %ps\n", drivers[i]);
- err = __platform_driver_register(drivers[i], owner);
+ err = __platform_driver_register(drivers[i], owner, mod_name);
if (err < 0) {
pr_err("failed to register platform driver %ps: %d\n",
drivers[i], err);
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 4ebcbd95b7fb..9c4fdef76833 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1695,7 +1695,8 @@ module_init(coresight_init);
module_exit(coresight_exit);
int __coresight_init_driver(const char *drv, struct amba_driver *amba_drv,
- struct platform_driver *pdev_drv, struct module *owner)
+ struct platform_driver *pdev_drv, struct module *owner,
+ const char *mod_name)
{
int ret;
@@ -1705,7 +1706,7 @@ int __coresight_init_driver(const char *drv, struct amba_driver *amba_drv,
return ret;
}
- ret = __platform_driver_register(pdev_drv, owner);
+ ret = __platform_driver_register(pdev_drv, owner, mod_name);
if (!ret)
return 0;
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index b9ec5f195907..d7ae0527d039 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -692,9 +692,10 @@ coresight_find_output_type(struct coresight_platform_data *pdata,
union coresight_dev_subtype subtype);
#define coresight_init_driver(drv, amba_drv, pdev_drv) \
- __coresight_init_driver(drv, amba_drv, pdev_drv, THIS_MODULE)
+ __coresight_init_driver(drv, amba_drv, pdev_drv, THIS_MODULE, KBUILD_MODNAME)
int __coresight_init_driver(const char *drv, struct amba_driver *amba_drv,
- struct platform_driver *pdev_drv, struct module *owner);
+ struct platform_driver *pdev_drv, struct module *owner,
+ const char *mod_name);
void coresight_remove_driver(struct amba_driver *amba_drv,
struct platform_driver *pdev_drv);
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 975400a472e3..26e6a43358e2 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -293,18 +293,19 @@ struct platform_driver {
* use a macro to avoid include chaining to get THIS_MODULE
*/
#define platform_driver_register(drv) \
- __platform_driver_register(drv, THIS_MODULE)
+ __platform_driver_register(drv, THIS_MODULE, KBUILD_MODNAME)
extern int __platform_driver_register(struct platform_driver *,
- struct module *);
+ struct module *, const char *mod_name);
extern void platform_driver_unregister(struct platform_driver *);
/* non-hotpluggable platform devices may use this so that probe() and
* its support may live in __init sections, conserving runtime memory.
*/
#define platform_driver_probe(drv, probe) \
- __platform_driver_probe(drv, probe, THIS_MODULE)
+ __platform_driver_probe(drv, probe, THIS_MODULE, KBUILD_MODNAME)
extern int __platform_driver_probe(struct platform_driver *driver,
- int (*probe)(struct platform_device *), struct module *module);
+ int (*probe)(struct platform_device *), struct module *module,
+ const char *mod_name);
static inline void *platform_get_drvdata(const struct platform_device *pdev)
{
@@ -368,19 +369,19 @@ static int __init __platform_driver##_init(void) \
device_initcall(__platform_driver##_init); \
#define platform_create_bundle(driver, probe, res, n_res, data, size) \
- __platform_create_bundle(driver, probe, res, n_res, data, size, THIS_MODULE)
+ __platform_create_bundle(driver, probe, res, n_res, data, size, THIS_MODULE, KBUILD_MODNAME)
extern struct platform_device *__platform_create_bundle(
struct platform_driver *driver, int (*probe)(struct platform_device *),
struct resource *res, unsigned int n_res,
- const void *data, size_t size, struct module *module);
+ const void *data, size_t size, struct module *module, const char *mod_name);
int __platform_register_drivers(struct platform_driver * const *drivers,
- unsigned int count, struct module *owner);
+ unsigned int count, struct module *owner, const char *mod_name);
void platform_unregister_drivers(struct platform_driver * const *drivers,
unsigned int count);
#define platform_register_drivers(drivers, count) \
- __platform_register_drivers(drivers, count, THIS_MODULE)
+ __platform_register_drivers(drivers, count, THIS_MODULE, KBUILD_MODNAME)
#ifdef CONFIG_SUSPEND
extern int platform_pm_suspend(struct device *dev);
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 8917d4ee499f..2d626eecc450 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -82,7 +82,9 @@ unsafe fn register(
}
// SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
- to_result(unsafe { bindings::__platform_driver_register(pdrv.get(), module.0) })
+ to_result(unsafe {
+ bindings::__platform_driver_register(pdrv.get(), module.0, name.as_char_ptr())
+ })
}
unsafe fn unregister(pdrv: &Opaque<Self::DriverType>) {
--
2.43.0
^ permalink raw reply related
* [PATCH v4 2/4] coresight: pass THIS_MODULE implicitly through a macro
From: Shashank Balaji @ 2026-04-27 2:41 UTC (permalink / raw)
To: Suzuki K Poulose, James Clark, Alexander Shishkin,
Maxime Coquelin, Alexandre Torgue, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Richard Cochran, Jonathan Corbet,
Shuah Khan, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen, Aaron Tomlin, Mike Leach, Leo Yan, Mike Leach
Cc: Rahul Bukte, Shashank Balaji, linux-kernel, coresight,
linux-arm-kernel, driver-core, rust-for-linux, linux-doc,
Daniel Palmer, Tim Bird, linux-modules
In-Reply-To: <20260427-acpi_mod_name-v4-0-22b42240c9bf@sony.com>
Rename coresight_init_driver() to __coresight_init_driver() and replace
it with a macro wrapper that passes THIS_MODULE implicitly. This is in line with
what other buses do.
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Co-developed-by: Rahul Bukte <rahul.bukte@sony.com>
Signed-off-by: Rahul Bukte <rahul.bukte@sony.com>
Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
---
drivers/hwtracing/coresight/coresight-catu.c | 2 +-
drivers/hwtracing/coresight/coresight-core.c | 4 ++--
drivers/hwtracing/coresight/coresight-cpu-debug.c | 3 +--
drivers/hwtracing/coresight/coresight-funnel.c | 3 +--
drivers/hwtracing/coresight/coresight-replicator.c | 3 +--
drivers/hwtracing/coresight/coresight-stm.c | 2 +-
drivers/hwtracing/coresight/coresight-tmc-core.c | 2 +-
drivers/hwtracing/coresight/coresight-tnoc.c | 2 +-
drivers/hwtracing/coresight/coresight-tpdm.c | 3 +--
drivers/hwtracing/coresight/coresight-tpiu.c | 2 +-
include/linux/coresight.h | 4 +++-
11 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
index ce71dcddfca2..0c8698c8fc5e 100644
--- a/drivers/hwtracing/coresight/coresight-catu.c
+++ b/drivers/hwtracing/coresight/coresight-catu.c
@@ -706,7 +706,7 @@ static int __init catu_init(void)
{
int ret;
- ret = coresight_init_driver("catu", &catu_driver, &catu_platform_driver, THIS_MODULE);
+ ret = coresight_init_driver("catu", &catu_driver, &catu_platform_driver);
tmc_etr_set_catu_ops(&etr_catu_buf_ops);
return ret;
}
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 46f247f73cf6..4ebcbd95b7fb 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1694,7 +1694,7 @@ static void __exit coresight_exit(void)
module_init(coresight_init);
module_exit(coresight_exit);
-int coresight_init_driver(const char *drv, struct amba_driver *amba_drv,
+int __coresight_init_driver(const char *drv, struct amba_driver *amba_drv,
struct platform_driver *pdev_drv, struct module *owner)
{
int ret;
@@ -1713,7 +1713,7 @@ int coresight_init_driver(const char *drv, struct amba_driver *amba_drv,
amba_driver_unregister(amba_drv);
return ret;
}
-EXPORT_SYMBOL_GPL(coresight_init_driver);
+EXPORT_SYMBOL_GPL(__coresight_init_driver);
void coresight_remove_driver(struct amba_driver *amba_drv,
struct platform_driver *pdev_drv)
diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
index 629614278e46..3a806c1d50ea 100644
--- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
+++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
@@ -757,8 +757,7 @@ static struct platform_driver debug_platform_driver = {
static int __init debug_init(void)
{
- return coresight_init_driver("debug", &debug_driver, &debug_platform_driver,
- THIS_MODULE);
+ return coresight_init_driver("debug", &debug_driver, &debug_platform_driver);
}
static void __exit debug_exit(void)
diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
index 3f56ceccd8c9..0abc11f0690c 100644
--- a/drivers/hwtracing/coresight/coresight-funnel.c
+++ b/drivers/hwtracing/coresight/coresight-funnel.c
@@ -412,8 +412,7 @@ static struct amba_driver dynamic_funnel_driver = {
static int __init funnel_init(void)
{
- return coresight_init_driver("funnel", &dynamic_funnel_driver, &funnel_driver,
- THIS_MODULE);
+ return coresight_init_driver("funnel", &dynamic_funnel_driver, &funnel_driver);
}
static void __exit funnel_exit(void)
diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
index 07fc04f53b88..2f382de357ee 100644
--- a/drivers/hwtracing/coresight/coresight-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-replicator.c
@@ -418,8 +418,7 @@ static struct amba_driver dynamic_replicator_driver = {
static int __init replicator_init(void)
{
- return coresight_init_driver("replicator", &dynamic_replicator_driver, &replicator_driver,
- THIS_MODULE);
+ return coresight_init_driver("replicator", &dynamic_replicator_driver, &replicator_driver);
}
static void __exit replicator_exit(void)
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index aca6cec7885a..4e860519a73f 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -1050,7 +1050,7 @@ static struct platform_driver stm_platform_driver = {
static int __init stm_init(void)
{
- return coresight_init_driver("stm", &stm_driver, &stm_platform_driver, THIS_MODULE);
+ return coresight_init_driver("stm", &stm_driver, &stm_platform_driver);
}
static void __exit stm_exit(void)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
index c89fe996af23..bc5a133ada3e 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-core.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
@@ -1046,7 +1046,7 @@ static struct platform_driver tmc_platform_driver = {
static int __init tmc_init(void)
{
- return coresight_init_driver("tmc", &tmc_driver, &tmc_platform_driver, THIS_MODULE);
+ return coresight_init_driver("tmc", &tmc_driver, &tmc_platform_driver);
}
static void __exit tmc_exit(void)
diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
index 96a25877b824..9e8de4323d28 100644
--- a/drivers/hwtracing/coresight/coresight-tnoc.c
+++ b/drivers/hwtracing/coresight/coresight-tnoc.c
@@ -344,7 +344,7 @@ static struct platform_driver itnoc_driver = {
static int __init tnoc_init(void)
{
- return coresight_init_driver("tnoc", &trace_noc_driver, &itnoc_driver, THIS_MODULE);
+ return coresight_init_driver("tnoc", &trace_noc_driver, &itnoc_driver);
}
static void __exit tnoc_exit(void)
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index eaf7210af648..8464edbba2d4 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -1563,8 +1563,7 @@ static struct platform_driver static_tpdm_driver = {
static int __init tpdm_init(void)
{
- return coresight_init_driver("tpdm", &dynamic_tpdm_driver, &static_tpdm_driver,
- THIS_MODULE);
+ return coresight_init_driver("tpdm", &dynamic_tpdm_driver, &static_tpdm_driver);
}
static void __exit tpdm_exit(void)
diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c
index b8560b140e0f..7b029d2eb389 100644
--- a/drivers/hwtracing/coresight/coresight-tpiu.c
+++ b/drivers/hwtracing/coresight/coresight-tpiu.c
@@ -310,7 +310,7 @@ static struct platform_driver tpiu_platform_driver = {
static int __init tpiu_init(void)
{
- return coresight_init_driver("tpiu", &tpiu_driver, &tpiu_platform_driver, THIS_MODULE);
+ return coresight_init_driver("tpiu", &tpiu_driver, &tpiu_platform_driver);
}
static void __exit tpiu_exit(void)
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 2131febebee9..b9ec5f195907 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -691,7 +691,9 @@ coresight_find_output_type(struct coresight_platform_data *pdata,
enum coresight_dev_type type,
union coresight_dev_subtype subtype);
-int coresight_init_driver(const char *drv, struct amba_driver *amba_drv,
+#define coresight_init_driver(drv, amba_drv, pdev_drv) \
+ __coresight_init_driver(drv, amba_drv, pdev_drv, THIS_MODULE)
+int __coresight_init_driver(const char *drv, struct amba_driver *amba_drv,
struct platform_driver *pdev_drv, struct module *owner);
void coresight_remove_driver(struct amba_driver *amba_drv,
--
2.43.0
^ permalink raw reply related
* [PATCH v4 1/4] kernel: param: initialize module_kset before do_initcalls()
From: Shashank Balaji @ 2026-04-27 2:41 UTC (permalink / raw)
To: Suzuki K Poulose, James Clark, Alexander Shishkin,
Maxime Coquelin, Alexandre Torgue, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Richard Cochran, Jonathan Corbet,
Shuah Khan, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen, Aaron Tomlin, Mike Leach, Leo Yan, Mike Leach
Cc: Rahul Bukte, Shashank Balaji, linux-kernel, coresight,
linux-arm-kernel, driver-core, rust-for-linux, linux-doc,
Daniel Palmer, Tim Bird, linux-modules
In-Reply-To: <20260427-acpi_mod_name-v4-0-22b42240c9bf@sony.com>
module_kset is initialized in param_sysfs_init(), a subsys_initcall. A number
of platform drivers register themselves prior to subsys_initcalls
(tegra194_cbb_driver registers in a pure_initcall, for example). With an
upcoming patch ("driver core: platform: set mod_name in driver registration")
that sets their mod_name in struct device_driver, lookup_or_create_module_kobject()
will be called for those drivers, which calls kset_find_obj(module_kset, mod_name).
This causes a null deref because module_kset isn't alive yet.
Fix this by initializing module_kset in do_basic_setup() before do_initcalls().
Modernize the pr_warn while we're at it.
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Suggested-by: Gary Guo <gary@garyguo.net>
Co-developed-by: Rahul Bukte <rahul.bukte@sony.com>
Signed-off-by: Rahul Bukte <rahul.bukte@sony.com>
Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
---
include/linux/module.h | 4 ++++
init/main.c | 1 +
kernel/params.c | 21 +++++++++------------
3 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 7566815fabbe..6478596e8f9f 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -886,6 +886,10 @@ static inline void module_for_each_mod(int(*func)(struct module *mod, void *data
#ifdef CONFIG_SYSFS
extern struct kset *module_kset;
extern const struct kobj_type module_ktype;
+
+void param_sysfs_init(void);
+#else
+static inline void param_sysfs_init(void) {}
#endif /* CONFIG_SYSFS */
#define symbol_request(x) try_then_request_module(symbol_get(x), "symbol:" #x)
diff --git a/init/main.c b/init/main.c
index 96f93bb06c49..01552c6b62ff 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1486,6 +1486,7 @@ static void __init do_basic_setup(void)
ksysfs_init();
driver_init();
init_irq_proc();
+ param_sysfs_init();
do_ctors();
do_initcalls();
}
diff --git a/kernel/params.c b/kernel/params.c
index 74d620bc2521..d1e3934fb3a7 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -942,22 +942,19 @@ const struct kobj_type module_ktype = {
/*
* param_sysfs_init - create "module" kset
*
- * This must be done before the initramfs is unpacked and
- * request_module() thus becomes possible, because otherwise the
- * module load would fail in mod_sysfs_init.
+ * Must run before:
+ * - do_initcalls(): some drivers register during initcalls and rely on
+ * module_kset existing for their sysfs module symlink.
+ * - rootfs_initcall (initramfs unpack): request_module() becomes possible.
+ * But if module_kset is null, module load would fail in mod_sysfs_init(),
+ * causing request_module() to fail.
*/
-static int __init param_sysfs_init(void)
+void __init param_sysfs_init(void)
{
module_kset = kset_create_and_add("module", &module_uevent_ops, NULL);
- if (!module_kset) {
- printk(KERN_WARNING "%s (%d): error creating kset\n",
- __FILE__, __LINE__);
- return -ENOMEM;
- }
-
- return 0;
+ if (!module_kset)
+ pr_warn("Error creating module kset\n");
}
-subsys_initcall(param_sysfs_init);
/*
* param_sysfs_builtin_init - add sysfs version and parameter
--
2.43.0
^ permalink raw reply related
* [PATCH v4 0/4] Enable sysfs module symlink for more built-in drivers
From: Shashank Balaji @ 2026-04-27 2:41 UTC (permalink / raw)
To: Suzuki K Poulose, James Clark, Alexander Shishkin,
Maxime Coquelin, Alexandre Torgue, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Richard Cochran, Jonathan Corbet,
Shuah Khan, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen, Aaron Tomlin, Mike Leach, Leo Yan, Mike Leach
Cc: Rahul Bukte, Shashank Balaji, linux-kernel, coresight,
linux-arm-kernel, driver-core, rust-for-linux, linux-doc,
Daniel Palmer, Tim Bird, linux-modules
In-Reply-To: <20260422-acpi_mod_name-v3-0-a184eff9ff6f@sony.com>
struct device_driver's mod_name is not set by a number of bus' driver registration
functions. Without that, built-in drivers don't have the module symlink in sysfs.
We want this to go from unbound driver name -> module name -> kernel config name.
This is useful on embedded platforms to minimize kernel config, reduce kernel size,
and reduce boot time.
In order to achieve this, mod_name has to be set to KBUILD_MODNAME, and this has
to be done for all buses which don't yet do this.
Here are some treewide stats:
- 110 registration functions across all bus types
- 20 of them set mod_name
- Remaining 90 do not set mod_name:
1. 36 functions under pattern 1:
They have a __register function + register macro. KBUILD_MODNAME needs to
be passed and the function needs to take mod_name as input.
2. 42 functions under pattern 2:
These have no macro wrapper. They need a double-underscore rename + macro
wrapper to make them similar to pattern 1.
3. Remaining 12 do not have such a clean registration interface. More analysis
is required.
We plan to start with pattern 1, since it's the easiest category of changes.
Within that, for now we're only sending the platform patch. If we get the go-ahead
on that, we'll send the remaining ones.
Patch 3 depends on patches 1 and 2.
Co-developed-by: Rahul Bukte <rahul.bukte@sony.com>
Signed-off-by: Rahul Bukte <rahul.bukte@sony.com>
Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
---
Changes in v4:
- Initialize module_kset in do_basic_setup() before do_initcalls() (Gary)
- Add commit body to the documentation patch (Greg)
- Link to v3: https://patch.msgid.link/20260422-acpi_mod_name-v3-0-a184eff9ff6f@sony.com
Changes in v3:
- Initialize module_kset on-demand (Greg)
- Make coresight driver registration happen through a macro (Greg)
- Split up the patch adding mod_name to platform driver registrations (Greg)
- Link to v2: https://patch.msgid.link/20260421-acpi_mod_name-v2-0-e73f9310dad3@sony.com
Changes in v2:
- Drop acpi patch, send platform instead (Rafael)
- Link to v1: https://patch.msgid.link/20260416-acpi_mod_name-v1-0-1a4d96fd86c9@sony.com
To: Suzuki K Poulose <suzuki.poulose@arm.com>
To: Mike Leach <mike.leach@linaro.org>
To: James Clark <james.clark@linaro.org>
To: Alexander Shishkin <alexander.shishkin@linux.intel.com>
To: Maxime Coquelin <mcoquelin.stm32@gmail.com>
To: Alexandre Torgue <alexandre.torgue@foss.st.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: "Rafael J. Wysocki" <rafael@kernel.org>
To: Danilo Krummrich <dakr@kernel.org>
To: Miguel Ojeda <ojeda@kernel.org>
To: Boqun Feng <boqun@kernel.org>
To: Gary Guo <gary@garyguo.net>
To: Björn Roy Baron <bjorn3_gh@protonmail.com>
To: Benno Lossin <lossin@kernel.org>
To: Andreas Hindborg <a.hindborg@kernel.org>
To: Alice Ryhl <aliceryhl@google.com>
To: Trevor Gross <tmgross@umich.edu>
To: Richard Cochran <richardcochran@gmail.com>
To: Jonathan Corbet <corbet@lwn.net>
To: Shuah Khan <skhan@linuxfoundation.org>
To: Luis Chamberlain <mcgrof@kernel.org>
To: Petr Pavlu <petr.pavlu@suse.com>
To: Daniel Gomez <da.gomez@kernel.org>
To: Sami Tolvanen <samitolvanen@google.com>
To: Aaron Tomlin <atomlin@atomlin.com>
To: Mike Leach <mike.leach@arm.com>
To: Leo Yan <leo.yan@arm.com>
Cc: linux-kernel@vger.kernel.org
Cc: coresight@lists.linaro.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: driver-core@lists.linux.dev
Cc: rust-for-linux@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: Shashank Balaji <shashank.mahadasyam@sony.com>
Cc: Rahul Bukte <rahul.bukte@sony.com>
Cc: Daniel Palmer <daniel.palmer@sony.com>
Cc: Tim Bird <tim.bird@sony.com>
Cc: linux-modules@vger.kernel.org
---
Shashank Balaji (4):
kernel: param: initialize module_kset before do_initcalls()
coresight: pass THIS_MODULE implicitly through a macro
driver core: platform: set mod_name in driver registration
docs: driver-api: add mod_name argument to __platform_register_drivers()
Documentation/driver-api/driver-model/platform.rst | 3 ++-
drivers/base/platform.c | 21 ++++++++++++++-------
drivers/hwtracing/coresight/coresight-catu.c | 2 +-
drivers/hwtracing/coresight/coresight-core.c | 9 +++++----
drivers/hwtracing/coresight/coresight-cpu-debug.c | 3 +--
drivers/hwtracing/coresight/coresight-funnel.c | 3 +--
drivers/hwtracing/coresight/coresight-replicator.c | 3 +--
drivers/hwtracing/coresight/coresight-stm.c | 2 +-
drivers/hwtracing/coresight/coresight-tmc-core.c | 2 +-
drivers/hwtracing/coresight/coresight-tnoc.c | 2 +-
drivers/hwtracing/coresight/coresight-tpdm.c | 3 +--
drivers/hwtracing/coresight/coresight-tpiu.c | 2 +-
include/linux/coresight.h | 7 +++++--
include/linux/module.h | 4 ++++
include/linux/platform_device.h | 17 +++++++++--------
init/main.c | 1 +
kernel/params.c | 21 +++++++++------------
rust/kernel/platform.rs | 4 +++-
18 files changed, 61 insertions(+), 48 deletions(-)
---
base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
change-id: 20260416-acpi_mod_name-f645a76e337b
Best regards,
--
Shashank Balaji <shashank.mahadasyam@sony.com>
^ permalink raw reply
* Re: [RFC PATCH 2/2] kernel/module: Decouple klp and ftrace from load_module
From: Song Chen @ 2026-04-26 14:26 UTC (permalink / raw)
To: Masami Hiramatsu (Google), Petr Mladek
Cc: Petr Pavlu, rafael, lenb, mturquette, sboyd, viresh.kumar, agk,
snitzer, mpatocka, bmarzins, song, yukuai, linan122, jason.wessel,
danielt, dianders, horms, davem, edumazet, kuba, pabeni, paulmck,
frederic, mcgrof, da.gomez, samitolvanen, atomlin, jpoimboe,
jikos, mbenes, joe.lawrence, rostedt, mark.rutland,
mathieu.desnoyers, linux-modules, linux-kernel,
linux-trace-kernel, linux-acpi, linux-clk, linux-pm,
live-patching, dm-devel, linux-raid, kgdb-bugreport, netdev
In-Reply-To: <20260420112707.aa3627ca9f975eeaf7d8ea0e@kernel.org>
Hi,
On 4/20/26 10:27, Masami Hiramatsu (Google) wrote:
> On Thu, 16 Apr 2026 16:49:32 +0200
> Petr Mladek <pmladek@suse.com> wrote:
>
>> On Thu 2026-04-16 13:18:30, Petr Pavlu wrote:
>>> On 4/15/26 8:43 AM, Song Chen wrote:
>>>> On 4/14/26 22:33, Petr Pavlu wrote:
>>>>> On 4/13/26 10:07 AM, chensong_2000@189.cn wrote:
>>>>>> diff --git a/include/linux/module.h b/include/linux/module.h
>>>>>> index 14f391b186c6..0bdd56f9defd 100644
>>>>>> --- a/include/linux/module.h
>>>>>> +++ b/include/linux/module.h
>>>>>> @@ -308,6 +308,14 @@ enum module_state {
>>>>>> MODULE_STATE_COMING, /* Full formed, running module_init. */
>>>>>> MODULE_STATE_GOING, /* Going away. */
>>>>>> MODULE_STATE_UNFORMED, /* Still setting it up. */
>>>>>> + MODULE_STATE_FORMED,
>>>>>
>>>>> I don't see a reason to add a new module state. Why is it necessary and
>>>>> how does it fit with the existing states?
>>>>>
>>>> because once notifier fails in state MODULE_STATE_UNFORMED (now only ftrace has someting to do in this state), notifier chain will roll back by calling blocking_notifier_call_chain_robust, i'm afraid MODULE_STATE_GOING is going to jeopardise the notifers which don't handle it appropriately, like:
>>>>
>>>> case MODULE_STATE_COMING:
>>>> kmalloc();
>>>> case MODULE_STATE_GOING:
>>>> kfree();
>>>
>>> My understanding is that the current module "state machine" operates as
>>> follows. Transitions marked with an asterisk (*) are announced via the
>>> module notifier.
>>>
>>> ---> UNFORMED --*> COMING --*> LIVE --*> GOING -.
>>> ^ | ^ |
>>> | '---------------------* |
>>> '---------------------------------------'
>>>
>>> The new code aims to replace the current ftrace_module_init() call in
>>> load_module(). To achieve this, it adds a notification for the UNFORMED
>>> state (only when loading a module) and introduces a new FORMED state for
>>> rollback. FORMED is purely a fake state because it never appears in
>>> module::state. The new structure is as follows:
>>>
>>> ,--*> (FORMED)
>>> |
>>> --*> UNFORMED --*> COMING --*> LIVE --*> GOING -.
>>> ^ | ^ |
>>> | '---------------------* |
>>> '---------------------------------------'
>>>
>>> I'm afraid this is quite complex and inconsistent. Unless it can be kept
>>> simple, we would be just replacing one special handling with a different
>>> complexity, which is not worth it.
>>
>>>>>
>>>>>> + if (err)
>>>>>> + goto ddebug_cleanup;
>>>>>> /* Finally it's fully formed, ready to start executing. */
>>>>>> err = complete_formation(mod, info);
>>>>>> - if (err)
>>>>>> + if (err) {
>>>>>> + blocking_notifier_call_chain_reverse(&module_notify_list,
>>>>>> + MODULE_STATE_FORMED, mod);
>>>>>> goto ddebug_cleanup;
>>>>>> + }
>>>>>> - err = prepare_coming_module(mod);
>>>>>> + err = prepare_module_state_transaction(mod,
>>>>>> + MODULE_STATE_COMING, MODULE_STATE_GOING);
>>>>>> if (err)
>>>>>> goto bug_cleanup;
>>>>>> @@ -3522,7 +3519,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
>>>>>> destroy_params(mod->kp, mod->num_kp);
>>>>>> blocking_notifier_call_chain(&module_notify_list,
>>>>>> MODULE_STATE_GOING, mod);
>>>>>
>>>>> My understanding is that all notifier chains for MODULE_STATE_GOING
>>>>> should be reversed.
>>>> yes, all, from lowest priority notifier to highest.
>>>> I will resend patch 1 which was failed due to my proxy setting.
>>>
>>> What I meant here is that the call:
>>>
>>> blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod);
>>>
>>> should be replaced with:
>>>
>>> blocking_notifier_call_chain_reverse(&module_notify_list, MODULE_STATE_GOING, mod);
>>>
>>>>
>>>>>
>>>>>> - klp_module_going(mod);
>>>>>> bug_cleanup:
>>>>>> mod->state = MODULE_STATE_GOING;
>>>>>> /* module_bug_cleanup needs module_mutex protection */
>>>>>
>>>>> The patch removes the klp_module_going() cleanup call in load_module().
>>>>> Similarly, the ftrace_release_mod() call under the ddebug_cleanup label
>>>>> should be removed and appropriately replaced with a cleanup via
>>>>> a notifier.
>>>>>
>>>> err = prepare_module_state_transaction(mod,
>>>> MODULE_STATE_UNFORMED, MODULE_STATE_FORMED);
>>>> if (err)
>>>> goto ddebug_cleanup;
>>>>
>>>> ftrace will be cleanup in blocking_notifier_call_chain_robust rolling back.
>>>>
>>>> err = prepare_module_state_transaction(mod,
>>>> MODULE_STATE_COMING, MODULE_STATE_GOING);
>>>>
>>>> each notifier including ftrace and klp will be cleanup in blocking_notifier_call_chain_robust rolling back.
>>>>
>>>> if all notifiers are successful in MODULE_STATE_COMING, they all will be clean up in
>>>> coming_cleanup:
>>>> mod->state = MODULE_STATE_GOING;
>>>> destroy_params(mod->kp, mod->num_kp);
>>>> blocking_notifier_call_chain(&module_notify_list,
>>>> MODULE_STATE_GOING, mod);
>>>>
>>>> if something wrong underneath.
>>>
>>> My point is that the patch leaves a call to ftrace_release_mod() in
>>> load_module(), which I expected to be handled via a notifier.
>>
>> I think that I have got it. The ftrace code needs two notifiers when
>> the module is being loaded and two when it is going.
>>
>> This is why Sond added the new state. But I think that we would
>> need two new states to call:
>>
>> + ftrace_module_init() in MODULE_STATE_UNFORMED
>> + ftrace_module_enable() in MODULE_STATE_FORMED
>>
>> and
>>
>> + ftrace_free_mem() in MODULE_STATE_PRE_GOING
>> + ftrace_free_mem() in MODULE_STATE_GOING
>>
>>
>> By using the ascii art:
>>
>> -*> UNFORMED -*> FORMED -> COMING -*> LIVE -*> PRE_GOING -*> GOING -.
>> | | | ^ ^ ^
>> | | '----------------' | |
>> | '--------------------------------------' |
>> '------------------------------------------------------'
>>
>>
>> But I think that this is not worth it.
>
> Agree.
>
> If this needs to be ordered so strictly, why we will use a "single"
> module notifier chain for this complex situation?
>
> I think the notifier call chain is just for notice a single signal,
> instead of sending several different signals, especially if there is
> any dependency among the callbacks.
>
> If notification callbacks need to be ordered, they are currently
> sorted by representing priority numerically, but this is quite
> fragile for updating. It has to look up other registered priorities
> and adjust the order among dependencies each time. For this reason,
> this mechanism is not suitable for global ordering. (It's like line
> numbers in BASIC.)
> It is probably only useful for representing dependencies between
> two components maintained by the same maintainer.
>
> I'm against a general-purpose system that makes everything modular.
> It unnecessarily complicates things. If there are processes that
> require strict ordering, especially processes that must be performed
> before each stage as part of the framework, they should be called
> directly from the framework, not via notification callbacks.
>
> This makes it simpler and more robust to maintain.
>
> Only the framework's end users should utilize notification callbacks.
>
> Thank you,
>
>
my motivation is to decouple ftrace and klp from module loader and make
blocking_notifier_chain more generic, but it doesn't become generic
completely. I understand your and Petr's comments and agree.
Thanks
Best regards
Song
>>
>> Best Regards,
>> Petr
>>
>
>
^ permalink raw reply
* Re: [RFC PATCH 1/2] kernel/notifier: replace single-linked list with double-linked list for reverse traversal
From: Song Chen @ 2026-04-26 14:14 UTC (permalink / raw)
To: Masami Hiramatsu (Google)
Cc: rafael, lenb, mturquette, sboyd, viresh.kumar, agk, snitzer,
mpatocka, bmarzins, song, yukuai, linan122, jason.wessel, danielt,
dianders, horms, davem, edumazet, kuba, pabeni, paulmck, frederic,
mcgrof, petr.pavlu, da.gomez, samitolvanen, atomlin, jpoimboe,
jikos, mbenes, pmladek, joe.lawrence, rostedt, mark.rutland,
mathieu.desnoyers, linux-modules, linux-kernel,
linux-trace-kernel, linux-acpi, linux-clk, linux-pm,
live-patching, dm-devel, linux-raid, kgdb-bugreport, netdev
In-Reply-To: <20260420144429.57b45f2beece690bceea96ec@kernel.org>
Hi Hiramatsu san,
On 4/20/26 13:44, Masami Hiramatsu (Google) wrote:
> Hi Song,
>
> On Wed, 15 Apr 2026 15:01:37 +0800
> chensong_2000@189.cn wrote:
>
>> From: Song Chen <chensong_2000@189.cn>
>>
>> The current notifier chain implementation uses a single-linked list
>> (struct notifier_block *next), which only supports forward traversal
>> in priority order. This makes it difficult to handle cleanup/teardown
>> scenarios that require notifiers to be called in reverse priority order.
>
> What about introducing a new notification callback API that allows you
> to describe dependencies between callback functions?
>
> For example, when registering a callback, you could register a string
> as an ID and specify whether to call it before or after that ID,
> or you could register a comparison function that is called when adding
> to a list. (I prefer @name and @depends fields so that it can be easily
> maintained.)
>
> This would allow for better dependency building when adding to the list.
>
Is the new notification callback API going to replace
blocking_notifier_chain in module loader? or an expansion inside
blocking_notifier_chain but introducing less complexity?
>>
>> A concrete example is the ordering dependency between ftrace and
>> livepatch during module load/unload. see the detail here [1].
>
> If this only concerns notification callback issues with the ftrace
> and livepatch modules, it's far more robust to simply call the
> necessary processing directly when the modules load and unload,
> rather than registering notification callbacks externally.
>
> There are fprobe, kprobe and its trace-events, all of them are using
> ftrace as its fundation layer. In this case, I always needs to
> consider callback order when a module is unloaded.
>
> If ftrace is working as a part of module callbacks, it will conflict
> with fprobe/kprobe module callback. Of course we can reorder it with
> modifying its priority. But this is ugly, because when we introduce
> a new other feature which depends on another layer, we need to
> reorder the callback's priority number on the list.
>
> Based on the above, I don't think this can be resolved simply by
> changing the list of notification callbacks to a bidirectional list.
>
> Thank you,
>
understood, many thanks for your proposal, i will think about it.
best regards,
Song
^ permalink raw reply
* Re: [RFC PATCH 1/2] kernel/notifier: replace single-linked list with double-linked list for reverse traversal
From: Song Chen @ 2026-04-26 13:56 UTC (permalink / raw)
To: Petr Mladek, Masami Hiramatsu
Cc: chensong_2000, rafael, lenb, mturquette, sboyd, viresh.kumar, agk,
snitzer, mpatocka, bmarzins, song, yukuai, linan122, jason.wessel,
danielt, dianders, horms, davem, edumazet, kuba, pabeni, paulmck,
frederic, mcgrof, petr.pavlu, da.gomez, samitolvanen, atomlin,
jpoimboe, jikos, mbenes, joe.lawrence, rostedt, mark.rutland,
mathieu.desnoyers, linux-modules, linux-kernel,
linux-trace-kernel, linux-acpi, linux-clk, linux-pm,
live-patching, dm-devel, linux-raid, kgdb-bugreport, netdev
In-Reply-To: <aec90caYZDHDAHgw@pathway.suse.cz>
Hi,
On 4/21/26 17:05, Petr Mladek wrote:
> On Mon 2026-04-20 14:44:29, Masami Hiramatsu wrote:
>> Hi Song,
>>
>> On Wed, 15 Apr 2026 15:01:37 +0800
>> chensong_2000@189.cn wrote:
>>
>>> From: Song Chen <chensong_2000@189.cn>
>>>
>>> The current notifier chain implementation uses a single-linked list
>>> (struct notifier_block *next), which only supports forward traversal
>>> in priority order. This makes it difficult to handle cleanup/teardown
>>> scenarios that require notifiers to be called in reverse priority order.
>>
>> What about introducing a new notification callback API that allows you
>> to describe dependencies between callback functions?
>>
>> For example, when registering a callback, you could register a string
>> as an ID and specify whether to call it before or after that ID,
>> or you could register a comparison function that is called when adding
>> to a list. (I prefer @name and @depends fields so that it can be easily
>> maintained.)
>
> This looks too complex. It would make sense only
> when this API has more users.
>
> Also this won't be enough for the ftrace/livepatch callbacks.
> They need to be ordered against against each other. But they
> also need to be called before/after all other callbacks.
> For example, when the module is loaded:
>
> + 1st frace
> + 2nd livepatch
> + then other notifiers
>
> See the commit c1bf08ac26e92122 ("ftrace: Be first to run code
> modification on modules").
>
>> This would allow for better dependency building when adding to the list.
>
>>>
>>> A concrete example is the ordering dependency between ftrace and
>>> livepatch during module load/unload. see the detail here [1].
>>
>> If this only concerns notification callback issues with the ftrace
>> and livepatch modules, it's far more robust to simply call the
>> necessary processing directly when the modules load and unload,
>> rather than registering notification callbacks externally.
>>
>> There are fprobe, kprobe and its trace-events, all of them are using
>> ftrace as its fundation layer. In this case, I always needs to
>> consider callback order when a module is unloaded.
>>
>> If ftrace is working as a part of module callbacks, it will conflict
>> with fprobe/kprobe module callback. Of course we can reorder it with
>> modifying its priority. But this is ugly, because when we introduce
>> a new other feature which depends on another layer, we need to
>> reorder the callback's priority number on the list.
>>
>> Based on the above, I don't think this can be resolved simply by
>> changing the list of notification callbacks to a bidirectional list.
>
> I agree. I would keep it as is (hardcoded).
>
> Best Regards,
> Petr
>
Thanks for the feedback, the necessity doesn't convincing enough. I will
try the proposal from Masami Hiramatsu.
Best regards,
Song
^ permalink raw reply
* Re: [PATCH v2 2/2] module/kallsyms: sort function symbols and use binary search
From: Stanislaw Gruszka @ 2026-04-24 9:13 UTC (permalink / raw)
To: Petr Pavlu
Cc: linux-modules, Sami Tolvanen, Luis Chamberlain, linux-kernel,
linux-trace-kernel, live-patching, Daniel Gomez, Aaron Tomlin,
Steven Rostedt, Masami Hiramatsu, Jordan Rome, Viktor Malik
In-Reply-To: <11c8e139-f9f3-4b22-863a-4e021a3947e7@suse.com>
Hi Petr,
thanks for the review.
On Thu, Apr 23, 2026 at 04:00:04PM +0200, Petr Pavlu wrote:
> On 3/27/26 12:00 PM, Stanislaw Gruszka wrote:
> > Module symbol lookup via find_kallsyms_symbol() performs a linear scan
> > over the entire symtab when resolving an address. The number of symbols
> > in module symtabs has grown over the years, largely due to additional
> > metadata in non-standard sections, making this lookup very slow.
> >
> > Improve this by separating function symbols during module load, placing
> > them at the beginning of the symtab, sorting them by address, and using
> > binary search when resolving addresses in module text.
> >
> > This also should improve times for linear symbol name lookups, as valid
> > function symbols are now located at the beginning of the symtab.
> >
> > The cost of sorting is small relative to module load time. In repeated
> > module load tests [1], depending on .config options, this change
> > increases load time between 2% and 4%. With cold caches, the difference
> > is not measurable, as memory access latency dominates.
> >
> > The sorting theoretically could be done in compile time, but much more
> > complicated as we would have to simulate kernel addresses resolution
> > for symbols, and then correct relocation entries. That would be risky
> > if get out of sync.
> >
> > The improvement can be observed when listing ftrace filter functions.
> >
> > Before:
> >
> > root@nano:~# time cat /sys/kernel/tracing/available_filter_functions | wc -l
> > 74908
> >
> > real 0m1.315s
> > user 0m0.000s
> > sys 0m1.312s
> >
> > After:
> >
> > root@nano:~# time cat /sys/kernel/tracing/available_filter_functions | wc -l
> > 74911
> >
> > real 0m0.167s
> > user 0m0.004s
> > sys 0m0.175s
> >
> > (there are three more symbols introduced by the patch)
> >
> > For livepatch modules, the symtab layout is preserved and the existing
> > linear search is used. For this case, it should be possible to keep
> > the original ELF symtab instead of copying it 1:1, but that is outside
> > the scope of this patch.
> >
> > Link: https://gist.github.com/sgruszka/09f3fb1dad53a97b1aad96e1927ab117 [1]
> > Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>
>
> Sorry for the delay reviewing this patch.
No problem.
> > ---
> > v1 -> v2:
> > - fix searching data symbols for CONFIG_KALLSYMS_ALL
> > - use kallsyms_symbol_value() in elf_sym_cmp()
> >
> > include/linux/module.h | 1 +
> > kernel/module/internal.h | 1 +
> > kernel/module/kallsyms.c | 171 +++++++++++++++++++++++++++++----------
> > 3 files changed, 130 insertions(+), 43 deletions(-)
> >
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index ac254525014c..67c053afa882 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -379,6 +379,7 @@ struct module_memory {
> > struct mod_kallsyms {
> > Elf_Sym *symtab;
> > unsigned int num_symtab;
> > + unsigned int num_func_syms;
> > char *strtab;
> > char *typetab;
> > };
> > diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> > index 618202578b42..6a4d498619b1 100644
> > --- a/kernel/module/internal.h
> > +++ b/kernel/module/internal.h
> > @@ -73,6 +73,7 @@ struct load_info {
> > bool sig_ok;
> > #ifdef CONFIG_KALLSYMS
> > unsigned long mod_kallsyms_init_off;
> > + unsigned long num_func_syms;
> > #endif
> > #ifdef CONFIG_MODULE_DECOMPRESS
> > #ifdef CONFIG_MODULE_STATS
> > diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
> > index f23126d804b2..d69e99e67707 100644
> > --- a/kernel/module/kallsyms.c
> > +++ b/kernel/module/kallsyms.c
> > @@ -10,6 +10,7 @@
> > #include <linux/kallsyms.h>
> > #include <linux/buildid.h>
> > #include <linux/bsearch.h>
> > +#include <linux/sort.h>
> > #include "internal.h"
> >
> > /* Lookup exported symbol in given range of kernel_symbols */
> > @@ -103,6 +104,95 @@ static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs,
> > return true;
> > }
> >
> > +static inline bool is_func_symbol(const Elf_Sym *sym)
> > +{
> > + return sym->st_shndx != SHN_UNDEF && sym->st_size != 0 &&
> > + ELF_ST_TYPE(sym->st_info) == STT_FUNC;
> > +}
> > +
> > +static unsigned int bsearch_func_symbol(struct mod_kallsyms *kallsyms,
> > + unsigned long addr,
> > + unsigned long *bestval,
> > + unsigned long *nextval)
> > +
> > +{
> > + unsigned int mid, low = 1, high = kallsyms->num_func_syms + 1;
> > + unsigned int best = 0;
> > + unsigned long thisval;
> > +
> > + while (low < high) {
> > + mid = low + (high - low) / 2;
> > + thisval = kallsyms_symbol_value(&kallsyms->symtab[mid]);
> > +
> > + if (thisval <= addr) {
> > + *bestval = thisval;
> > + best = mid;
> > + low = mid + 1;
>
> If thisval == addr, the search moves to the right and finds the last
> symbol with the same address. I believe it should do the opposite and
> return the first symbol to match the behavior of
> search_kallsyms_symbol().
In the case of multiple symbols sharing the same address, we have
to pick one and ignore the others. I don’t think it matters much which
one is chosen in practice. Also, I expect function symbol addresses
to be unique, so this shouldn’t be a real issue.
> > + } else {
> > + *nextval = thisval;
> > + high = mid;
> > + }
> > + }
> > +
> > + return best;
> > +}
> > +
> > +static const char *kallsyms_symbol_name(struct mod_kallsyms *kallsyms,
> > + unsigned int symnum)
> > +{
> > + return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
> > +}
> > +
> > +static unsigned int search_kallsyms_symbol(struct mod_kallsyms *kallsyms,
> > + unsigned long addr,
> > + unsigned long *bestval,
> > + unsigned long *nextval)
> > +{
> > + unsigned int i, best = 0;
> > +
> > + /*
> > + * Scan for closest preceding symbol and next symbol. (ELF starts
> > + * real symbols at 1). Skip the initial function symbols range
> > + * if num_func_syms is non-zero, those are handled separately for
> > + * the core TEXT segment lookup.
> > + */
> > + for (i = 1 + kallsyms->num_func_syms; i < kallsyms->num_symtab; i++) {
> > + const Elf_Sym *sym = &kallsyms->symtab[i];
> > + unsigned long thisval = kallsyms_symbol_value(sym);
> > +
> > + if (sym->st_shndx == SHN_UNDEF)
> > + continue;
> > +
> > + /*
> > + * We ignore unnamed symbols: they're uninformative
> > + * and inserted at a whim.
> > + */
> > + if (*kallsyms_symbol_name(kallsyms, i) == '\0' ||
> > + is_mapping_symbol(kallsyms_symbol_name(kallsyms, i)))
> > + continue;
> > +
> > + if (thisval <= addr && thisval > *bestval) {
> > + best = i;
> > + *bestval = thisval;
> > + }
> > + if (thisval > addr && thisval < *nextval)
> > + *nextval = thisval;
> > + }
> > +
> > + return best;
> > +}
> > +
> > +static int elf_sym_cmp(const void *a, const void *b)
> > +{
> > + unsigned long val_a = kallsyms_symbol_value((const Elf_Sym *)a);
> > + unsigned long val_b = kallsyms_symbol_value((const Elf_Sym *)b);
> > +
> > + if (val_a < val_b)
> > + return -1;
> > +
> > + return val_a > val_b;
>
> Does this comparison function and the sort() call result in stable
> sorting? If val_a and val_b are the same, the sorting should preserve
> the original order.
The kernel’s sort() implementation is not stable.
> > +}
> > +
> > /*
> > * We only allocate and copy the strings needed by the parts of symtab
> > * we keep. This is simple, but has the effect of making multiple
> > @@ -115,9 +205,10 @@ void layout_symtab(struct module *mod, struct load_info *info)
> > Elf_Shdr *symsect = info->sechdrs + info->index.sym;
> > Elf_Shdr *strsect = info->sechdrs + info->index.str;
> > const Elf_Sym *src;
> > - unsigned int i, nsrc, ndst, strtab_size = 0;
> > + unsigned int i, nsrc, ndst, nfunc, strtab_size = 0;
> > struct module_memory *mod_mem_data = &mod->mem[MOD_DATA];
> > struct module_memory *mod_mem_init_data = &mod->mem[MOD_INIT_DATA];
> > + bool is_lp_mod = is_livepatch_module(mod);
> >
> > /* Put symbol section at end of init part of module. */
> > symsect->sh_flags |= SHF_ALLOC;
> > @@ -129,12 +220,14 @@ void layout_symtab(struct module *mod, struct load_info *info)
> > nsrc = symsect->sh_size / sizeof(*src);
> >
> > /* Compute total space required for the core symbols' strtab. */
> > - for (ndst = i = 0; i < nsrc; i++) {
> > - if (i == 0 || is_livepatch_module(mod) ||
> > + for (ndst = nfunc = i = 0; i < nsrc; i++) {
> > + if (i == 0 || is_lp_mod ||
> > is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
> > info->index.pcpu)) {
> > strtab_size += strlen(&info->strtab[src[i].st_name]) + 1;
> > ndst++;
> > + if (!is_lp_mod && is_func_symbol(src + i))
> > + nfunc++;
> > }
> > }
> >
> > @@ -156,6 +249,7 @@ void layout_symtab(struct module *mod, struct load_info *info)
> > mod_mem_init_data->size = ALIGN(mod_mem_init_data->size,
> > __alignof__(struct mod_kallsyms));
> > info->mod_kallsyms_init_off = mod_mem_init_data->size;
> > + info->num_func_syms = nfunc;
> >
> > mod_mem_init_data->size += sizeof(struct mod_kallsyms);
> > info->init_typeoffs = mod_mem_init_data->size;
> > @@ -169,7 +263,7 @@ void layout_symtab(struct module *mod, struct load_info *info)
> > */
> > void add_kallsyms(struct module *mod, const struct load_info *info)
> > {
> > - unsigned int i, ndst;
> > + unsigned int i, di, nfunc, ndst;
> > const Elf_Sym *src;
> > Elf_Sym *dst;
> > char *s;
> > @@ -178,6 +272,7 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
> > void *data_base = mod->mem[MOD_DATA].base;
> > void *init_data_base = mod->mem[MOD_INIT_DATA].base;
> > struct mod_kallsyms *kallsyms;
> > + bool is_lp_mod = is_livepatch_module(mod);
> >
> > kallsyms = init_data_base + info->mod_kallsyms_init_off;
>
> This code is followed by the initialization of kallsyms:
>
> kallsyms->symtab = (void *)symsec->sh_addr;
> kallsyms->num_symtab = symsec->sh_size / sizeof(Elf_Sym);
> /* Make sure we get permanent strtab: don't use info->strtab. */
> kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
> kallsyms->typetab = init_data_base + info->init_typeoffs;
>
> I suggest adding 'kallsyms->num_func_syms = 0;' after the initialization
> of kallsyms->num_symtab.
I relied on zeroed memory initialization, but I can add this explicitly
for clarity.
> > @@ -194,19 +289,28 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
> > mod->core_kallsyms.symtab = dst = data_base + info->symoffs;
> > mod->core_kallsyms.strtab = s = data_base + info->stroffs;
> > mod->core_kallsyms.typetab = data_base + info->core_typeoffs;
> > +
> > strtab_size = info->core_typeoffs - info->stroffs;
> > src = kallsyms->symtab;
> > - for (ndst = i = 0; i < kallsyms->num_symtab; i++) {
> > + ndst = info->num_func_syms + 1;
> > +
> > + for (nfunc = i = 0; i < kallsyms->num_symtab; i++) {
> > kallsyms->typetab[i] = elf_type(src + i, info);
> > - if (i == 0 || is_livepatch_module(mod) ||
> > + if (i == 0 || is_lp_mod ||
> > is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
> > info->index.pcpu)) {
> > ssize_t ret;
> >
> > - mod->core_kallsyms.typetab[ndst] =
> > - kallsyms->typetab[i];
> > - dst[ndst] = src[i];
> > - dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
> > + if (i == 0)
> > + di = 0;
> > + else if (!is_lp_mod && is_func_symbol(src + i))
> > + di = 1 + nfunc++;
> > + else
> > + di = ndst++;
> > +
> > + mod->core_kallsyms.typetab[di] = kallsyms->typetab[i];
> > + dst[di] = src[i];
> > + dst[di].st_name = s - mod->core_kallsyms.strtab;
> > ret = strscpy(s, &kallsyms->strtab[src[i].st_name],
> > strtab_size);
> > if (ret < 0)
> > @@ -216,9 +320,13 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
> > }
> > }
> >
> > + WARN_ON_ONCE(nfunc != info->num_func_syms);
> > + sort(dst + 1, nfunc, sizeof(Elf_Sym), elf_sym_cmp, NULL);
> > +
>
> The code sorts mod->core_kallsyms.symtab but mod->core_kallsyms.typetab
> is not reordered accordingly.
Right, but for function symbols the typetab entries are all 't',
so swapping them does not change the type value. The 'T' vs 't'
distinction is handled later when printing (based on export status).
But the comment explaining skiping adjusting of
mod->core_kallsyms.typetab is needed.
> > /* Set up to point into init section. */
> > rcu_assign_pointer(mod->kallsyms, kallsyms);
> > mod->core_kallsyms.num_symtab = ndst;
> > + mod->core_kallsyms.num_func_syms = nfunc;
> > }
> >
> > #if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
> > @@ -241,11 +349,6 @@ void init_build_id(struct module *mod, const struct load_info *info)
> > }
> > #endif
> >
> > -static const char *kallsyms_symbol_name(struct mod_kallsyms *kallsyms, unsigned int symnum)
> > -{
> > - return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
> > -}
> > -
> > /*
> > * Given a module and address, find the corresponding symbol and return its name
> > * while providing its size and offset if needed.
> > @@ -255,7 +358,10 @@ static const char *find_kallsyms_symbol(struct module *mod,
> > unsigned long *size,
> > unsigned long *offset)
> > {
> > - unsigned int i, best = 0;
> > + unsigned int (*search)(struct mod_kallsyms *kallsyms,
> > + unsigned long addr, unsigned long *bestval,
> > + unsigned long *nextval);
> > + unsigned int best;
> > unsigned long nextval, bestval;
> > struct mod_kallsyms *kallsyms = rcu_dereference(mod->kallsyms);
> > struct module_memory *mod_mem = NULL;
> > @@ -266,6 +372,11 @@ static const char *find_kallsyms_symbol(struct module *mod,
> > continue;
> > #endif
> > if (within_module_mem_type(addr, mod, type)) {
> > + if (type == MOD_TEXT && kallsyms->num_func_syms > 0)
> > + search = bsearch_func_symbol;
>
> I'm not sure if it is ok to limit the search only to function symbols
> when the address lies in MOD_TEXT. The text can theoretically contain
> non-function symbols.
Yes, the patch assumes that the only valid symbols in the MOD_TEXT
are functions. If there are defined OBJECT symbols in .text, the patch
would break lookup for those.
While it’s theoretically possible (e.g. hand-written assembly placing
data in .text ?), I’m not sure this is a practical concern. In general,
having data in executable segments is discouraged for security reasons.
> Could this optimization be adjusted to sort all
> MOD_TEXT symbols (excluding anonymous and mapping symbols) and move them
> to the front of the symbol table?
That’s possible. We could track .text sections indices in
__layout_sections() and include all valid symbols from those sections,
and also reorder typetab accordingly.
However, this adds complexity. I would prefer to first confirm whether
OBJECT symbols in MOD_TEXT is a real issue before going in that direction.
Regards
Stanislaw
> > + else
> > + search = search_kallsyms_symbol;
> > +
> > mod_mem = &mod->mem[type];
> > break;
> > }
> > @@ -278,33 +389,7 @@ static const char *find_kallsyms_symbol(struct module *mod,
> > nextval = (unsigned long)mod_mem->base + mod_mem->size;
> > bestval = (unsigned long)mod_mem->base - 1;
> >
> > - /*
> > - * Scan for closest preceding symbol, and next symbol. (ELF
> > - * starts real symbols at 1).
> > - */
> > - for (i = 1; i < kallsyms->num_symtab; i++) {
> > - const Elf_Sym *sym = &kallsyms->symtab[i];
> > - unsigned long thisval = kallsyms_symbol_value(sym);
> > -
> > - if (sym->st_shndx == SHN_UNDEF)
> > - continue;
> > -
> > - /*
> > - * We ignore unnamed symbols: they're uninformative
> > - * and inserted at a whim.
> > - */
> > - if (*kallsyms_symbol_name(kallsyms, i) == '\0' ||
> > - is_mapping_symbol(kallsyms_symbol_name(kallsyms, i)))
> > - continue;
> > -
> > - if (thisval <= addr && thisval > bestval) {
> > - best = i;
> > - bestval = thisval;
> > - }
> > - if (thisval > addr && thisval < nextval)
> > - nextval = thisval;
> > - }
> > -
> > + best = search(kallsyms, addr, &bestval, &nextval);
> > if (!best)
> > return NULL;
> >
>
> --
> Thanks,
> Petr
^ permalink raw reply
* [PATCH v14 19/92] dyndbg,module: make proper substructs in _ddebug_info
From: Jim Cromie @ 2026-04-23 20:54 UTC (permalink / raw)
To: Arnd Bergmann, Jason Baron, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, Aaron Tomlin, Andrew Morton,
Jonathan Corbet, Shuah Khan, Shuah Khan, Greg Kroah-Hartman,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, Alex Deucher, Christian König, David Airlie,
Gerd Hoffmann, Dmitry Osipenko, Gurchetan Singh, Chia-I Wu,
Matthew Brost, Thomas Hellström, Lyude Paul,
Danilo Krummrich, Patrik Jakobsson, Zack Rusin,
Broadcom internal kernel review list, Louis Chauvet,
Haneen Mohammed, Melissa Wen, Sean Paul, Jocelyn Falempe,
Ruben Wauters, Dave Stevenson, Maíra Canal,
Raspberry Pi Kernel Maintenance, Rob Clark, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, Xinliang Liu,
Tian Tao, Xinwei Kong, Sumit Semwal, Yongqin Liu, John Stultz,
Philipp Zabel, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Chun-Kuang Hu, Matthias Brugger,
AngeloGioacchino Del Regno, Sandy Huang, Heiko Stübner,
Andy Yan, Alain Volmat, Raphael Gallais-Pou, Yannick Fertre,
Raphael Gallais-Pou, Philippe Cornu, Maxime Coquelin,
Alexandre Torgue, Oded Gabbay, Maciej Falkowski, Karol Wachowski,
Rob Herring (Arm), Tomeu Vizoso, Liviu Dudau, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Liu Ying, Laurentiu Palcu, Lucas Stach,
Paul Kocialkowski, Jianmin Lv, Qianhai Wu, Huacai Chen,
Mingcong Bai, Xi Ruoyao, Icenowy Zheng, Laurent Pinchart,
Tomi Valkeinen, Kieran Bingham, Geert Uytterhoeven, Magnus Damm,
Javier Martinez Canillas, Huang Rui, Matthew Auld, Jani Nikula,
Luca Coelho, Russell King, Christian Gmeiner
Cc: linux-arch, linux-kernel, linux-modules, linux-doc,
linux-kselftest, dri-devel, intel-gfx, amd-gfx, virtualization,
intel-xe, nouveau, spice-devel, linux-arm-msm, freedreno, imx,
linux-arm-kernel, linux-mediatek, linux-rockchip, linux-stm32,
linux-renesas-soc, etnaviv, Jim Cromie
In-Reply-To: <20260423-submit-dyndbg-classmap-foundation-v14-0-2b809a8019d0@gmail.com>
recompose struct _ddebug_info, inserting proper sub-structs.
The struct _ddebug_info has 2 pairs of _vec, num_##_vec fields, for
descs and classes respectively. for_subvec() makes walking these
vectors less cumbersome, now lets move those field pairs into their
own "vec" structs: _ddebug_descs & _ddebug_class_maps, and re-compose
struct _ddebug_info to contain them cleanly. This also lets us get
rid of for_subvec()'s num_##_vec paste-up.
Also recompose struct ddebug_table to contain a _ddebug_info. This
reinforces its use as a cursor into relevant data for a builtin
module, and access to the full _ddebug state for modules.
NOTES:
Fixup names:
Normalize all struct names to "struct _ddebug_*" eliminating the
minor/stupid variations created in classmaps-v1.
Modify __section names: __dyndbg to __dyndbg_descriptors, and
__dyndbg_classes to __dyndbg_class_maps. This better matches the new
struct names, and makes room for forthcoming _ddebug_class_user(s)
structs and section.
Invariant: These vectors ref a contiguous subrange of __section memory
in builtin/DATA or in loadable modules via mod->dyndbg_info; with
guaranteed life-time for us.
struct module contains a _ddebug_info field and module/main.c sets it
up, so that gets adjusted rather obviously.
Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
include/asm-generic/dyndbg.lds.h | 18 +++---
include/linux/dynamic_debug.h | 40 ++++++++-----
kernel/module/main.c | 12 ++--
lib/dynamic_debug.c | 120 +++++++++++++++++++--------------------
lib/test_dynamic_debug.c | 2 +-
5 files changed, 103 insertions(+), 89 deletions(-)
diff --git a/include/asm-generic/dyndbg.lds.h b/include/asm-generic/dyndbg.lds.h
index f95683aa16b6..8345ac6c52b7 100644
--- a/include/asm-generic/dyndbg.lds.h
+++ b/include/asm-generic/dyndbg.lds.h
@@ -3,17 +3,19 @@
#define __ASM_GENERIC_DYNDBG_LDS_H
#include <asm-generic/bounded_sections.lds.h>
-#define DYNDBG_SECTIONS() \
- . = ALIGN(8); \
- BOUNDED_SECTION_BY(__dyndbg, ___dyndbg) \
- BOUNDED_SECTION_BY(__dyndbg_classes, ___dyndbg_classes)
+#define DYNDBG_SECTIONS() \
+ . = ALIGN(8); \
+ BOUNDED_SECTION_BY(__dyndbg_descriptors, ___dyndbg_descs) \
+ BOUNDED_SECTION_BY(__dyndbg_class_maps, ___dyndbg_class_maps)
#define MOD_DYNDBG_SECTIONS() \
- __dyndbg : { \
- BOUNDED_SECTION_BY(__dyndbg, ___dyndbg) \
+ __dyndbg_descriptors : { \
+ BOUNDED_SECTION_BY(__dyndbg_descriptors, \
+ ___dyndbg_descs) \
} \
- __dyndbg_classes : { \
- BOUNDED_SECTION_BY(__dyndbg_classes, ___dyndbg_classes) \
+ __dyndbg_class_maps : { \
+ BOUNDED_SECTION_BY(__dyndbg_class_maps, \
+ ___dyndbg_class_maps) \
}
#endif /* __ASM_GENERIC_DYNDBG_LDS_H */
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 9fd36339db52..5429315ada8e 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -83,8 +83,8 @@ enum class_map_type {
*/
};
-struct ddebug_class_map {
- struct module *mod;
+struct _ddebug_class_map {
+ struct module *mod; /* NULL for builtins */
const char *mod_name; /* needed for builtins */
const char **class_names;
const int length;
@@ -92,21 +92,33 @@ struct ddebug_class_map {
enum class_map_type map_type;
};
-/* encapsulate linker provided built-in (or module) dyndbg data */
+/*
+ * @_ddebug_info: gathers module/builtin dyndbg_* __sections together.
+ * For builtins, it is used as a cursor, with the inner structs
+ * marking sub-vectors of the builtin __sections in DATA.
+ */
+struct _ddebug_descs {
+ struct _ddebug *start;
+ int len;
+};
+
+struct _ddebug_class_maps {
+ struct _ddebug_class_map *start;
+ int len;
+};
+
struct _ddebug_info {
- struct _ddebug *descs;
- struct ddebug_class_map *classes;
- unsigned int num_descs;
- unsigned int num_classes;
+ struct _ddebug_descs descs;
+ struct _ddebug_class_maps maps;
};
-struct ddebug_class_param {
+struct _ddebug_class_param {
union {
unsigned long *bits;
unsigned long *lvl;
};
char flags[8];
- const struct ddebug_class_map *map;
+ const struct _ddebug_class_map *map;
};
/*
@@ -125,8 +137,8 @@ struct ddebug_class_param {
*/
#define DECLARE_DYNDBG_CLASSMAP(_var, _maptype, _base, ...) \
static const char *_var##_classnames[] = { __VA_ARGS__ }; \
- static struct ddebug_class_map __aligned(8) __used \
- __section("__dyndbg_classes") _var = { \
+ static struct _ddebug_class_map __aligned(8) __used \
+ __section("__dyndbg_class_maps") _var = { \
.mod = THIS_MODULE, \
.mod_name = KBUILD_MODNAME, \
.base = _base, \
@@ -166,7 +178,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
#define DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, cls, fmt) \
static struct _ddebug __aligned(8) \
- __section("__dyndbg") name = { \
+ __section("__dyndbg_descriptors") name = { \
.modname = KBUILD_MODNAME, \
.function = __func__, \
.filename = __FILE__, \
@@ -253,7 +265,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
* macro.
*/
#define _dynamic_func_call_cls(cls, fmt, func, ...) \
- __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
+ __dynamic_func_call_cls(__UNIQUE_ID(_ddebug), cls, fmt, func, ##__VA_ARGS__)
#define _dynamic_func_call(fmt, func, ...) \
_dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
@@ -263,7 +275,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
* with precisely the macro's varargs.
*/
#define _dynamic_func_call_cls_no_desc(cls, fmt, func, ...) \
- __dynamic_func_call_cls_no_desc(__UNIQUE_ID(ddebug), cls, fmt, \
+ __dynamic_func_call_cls_no_desc(__UNIQUE_ID(_ddebug), cls, fmt, \
func, ##__VA_ARGS__)
#define _dynamic_func_call_no_desc(fmt, func, ...) \
_dynamic_func_call_cls_no_desc(_DPRINTK_CLASS_DFLT, fmt, \
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 46dd8d25a605..c2b6e70f2e77 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2774,12 +2774,12 @@ static int find_module_sections(struct module *mod, struct load_info *info)
pr_warn("%s: Ignoring obsolete parameters\n", mod->name);
#ifdef CONFIG_DYNAMIC_DEBUG_CORE
- mod->dyndbg_info.descs = section_objs(info, "__dyndbg",
- sizeof(*mod->dyndbg_info.descs),
- &mod->dyndbg_info.num_descs);
- mod->dyndbg_info.classes = section_objs(info, "__dyndbg_classes",
- sizeof(*mod->dyndbg_info.classes),
- &mod->dyndbg_info.num_classes);
+ mod->dyndbg_info.descs.start = section_objs(info, "__dyndbg_descriptors",
+ sizeof(*mod->dyndbg_info.descs.start),
+ &mod->dyndbg_info.descs.len);
+ mod->dyndbg_info.maps.start = section_objs(info, "__dyndbg_class_maps",
+ sizeof(*mod->dyndbg_info.maps.start),
+ &mod->dyndbg_info.maps.len);
#endif
return 0;
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 8f614eba8ace..f47fdb769d7a 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -39,17 +39,15 @@
#include <rdma/ib_verbs.h>
-extern struct _ddebug __start___dyndbg[];
-extern struct _ddebug __stop___dyndbg[];
-extern struct ddebug_class_map __start___dyndbg_classes[];
-extern struct ddebug_class_map __stop___dyndbg_classes[];
+extern struct _ddebug __start___dyndbg_descs[];
+extern struct _ddebug __stop___dyndbg_descs[];
+extern struct _ddebug_class_map __start___dyndbg_class_maps[];
+extern struct _ddebug_class_map __stop___dyndbg_class_maps[];
struct ddebug_table {
struct list_head link;
const char *mod_name;
- struct _ddebug *ddebugs;
- struct ddebug_class_map *classes;
- unsigned int num_ddebugs, num_classes;
+ struct _ddebug_info info;
};
struct ddebug_query {
@@ -136,19 +134,19 @@ do { \
* @_i: caller provided counter.
* @_sp: cursor into _vec, to examine each item.
* @_box: ptr to a struct containing @_vec member
- * @_vec: name of a member in @_box
+ * @_vec: name of a vector member in @_box
*/
#define __ASSERT_IS_LVALUE(x) ((void)sizeof((void)0, &(x)))
#define __ASSERT_HAS_VEC_MEMBER(_box, _vec) \
- (void)sizeof((_box)->_vec + (_box)->num_##_vec)
+ ((void)sizeof((_box)->_vec.start + (_box)->_vec.len))
#define for_subvec(_i, _sp, _box, _vec) \
for (__ASSERT_IS_LVALUE(_i), \
__ASSERT_IS_LVALUE(_sp), \
__ASSERT_HAS_VEC_MEMBER(_box, _vec), \
(_i) = 0, \
- (_sp) = (_box)->_vec; \
- (_i) < (_box)->num_##_vec; \
+ (_sp) = (_box)->_vec.start; \
+ (_i) < (_box)->_vec.len; \
(_i)++, (_sp)++) /* { block } */
static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
@@ -171,14 +169,14 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
query->first_lineno, query->last_lineno, query->class_string);
}
-static struct ddebug_class_map *ddebug_find_valid_class(struct ddebug_table const *dt,
+static struct _ddebug_class_map *ddebug_find_valid_class(struct ddebug_table const *dt,
const char *class_string,
int *class_id)
{
- struct ddebug_class_map *map;
+ struct _ddebug_class_map *map;
int i, idx;
- for_subvec(i, map, dt, classes) {
+ for_subvec(i, map, &dt->info, maps) {
idx = match_string(map->class_names, map->length, class_string);
if (idx >= 0) {
*class_id = idx + map->base;
@@ -249,7 +247,7 @@ static int ddebug_change(const struct ddebug_query *query,
unsigned int newflags;
unsigned int nfound = 0;
struct flagsbuf fbuf, nbuf;
- struct ddebug_class_map *map = NULL;
+ struct _ddebug_class_map *map = NULL;
int valid_class;
/* search for matching ddebugs */
@@ -270,8 +268,8 @@ static int ddebug_change(const struct ddebug_query *query,
valid_class = _DPRINTK_CLASS_DFLT;
}
- for (i = 0; i < dt->num_ddebugs; i++) {
- struct _ddebug *dp = &dt->ddebugs[i];
+ for (i = 0; i < dt->info.descs.len; i++) {
+ struct _ddebug *dp = &dt->info.descs.start[i];
if (!ddebug_match_desc(query, dp, valid_class))
continue;
@@ -629,14 +627,14 @@ static int ddebug_exec_queries(char *query, const char *modname)
}
/* apply a new class-param setting */
-static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
+static int ddebug_apply_class_bitmap(const struct _ddebug_class_param *dcp,
const unsigned long *new_bits,
const unsigned long old_bits,
const char *query_modname)
{
#define QUERY_SIZE 128
char query[QUERY_SIZE];
- const struct ddebug_class_map *map = dcp->map;
+ const struct _ddebug_class_map *map = dcp->map;
int matches = 0;
int bi, ct;
@@ -672,8 +670,8 @@ static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
/* accept comma-separated-list of [+-] classnames */
static int param_set_dyndbg_classnames(const char *instr, const struct kernel_param *kp)
{
- const struct ddebug_class_param *dcp = kp->arg;
- const struct ddebug_class_map *map = dcp->map;
+ const struct _ddebug_class_param *dcp = kp->arg;
+ const struct _ddebug_class_map *map = dcp->map;
unsigned long curr_bits, old_bits;
char *cl_str, *p, *tmp;
int cls_id, totct = 0;
@@ -743,8 +741,8 @@ static int param_set_dyndbg_module_classes(const char *instr,
const struct kernel_param *kp,
const char *mod_name)
{
- const struct ddebug_class_param *dcp = kp->arg;
- const struct ddebug_class_map *map = dcp->map;
+ const struct _ddebug_class_param *dcp = kp->arg;
+ const struct _ddebug_class_map *map = dcp->map;
unsigned long inrep, new_bits, old_bits;
int rc, totct = 0;
@@ -831,8 +829,8 @@ EXPORT_SYMBOL(param_set_dyndbg_classes);
*/
int param_get_dyndbg_classes(char *buffer, const struct kernel_param *kp)
{
- const struct ddebug_class_param *dcp = kp->arg;
- const struct ddebug_class_map *map = dcp->map;
+ const struct _ddebug_class_param *dcp = kp->arg;
+ const struct _ddebug_class_map *map = dcp->map;
switch (map->map_type) {
@@ -1083,8 +1081,8 @@ static struct _ddebug *ddebug_iter_first(struct ddebug_iter *iter)
}
iter->table = list_entry(ddebug_tables.next,
struct ddebug_table, link);
- iter->idx = iter->table->num_ddebugs;
- return &iter->table->ddebugs[--iter->idx];
+ iter->idx = iter->table->info.descs.len;
+ return &iter->table->info.descs.start[--iter->idx];
}
/*
@@ -1105,10 +1103,10 @@ static struct _ddebug *ddebug_iter_next(struct ddebug_iter *iter)
}
iter->table = list_entry(iter->table->link.next,
struct ddebug_table, link);
- iter->idx = iter->table->num_ddebugs;
+ iter->idx = iter->table->info.descs.len;
--iter->idx;
}
- return &iter->table->ddebugs[iter->idx];
+ return &iter->table->info.descs.start[iter->idx];
}
/*
@@ -1152,16 +1150,19 @@ static void *ddebug_proc_next(struct seq_file *m, void *p, loff_t *pos)
return dp;
}
-#define class_in_range(class_id, map) \
- (class_id >= map->base && class_id < map->base + map->length)
+static bool ddebug_class_in_range(const int class_id, const struct _ddebug_class_map *map)
+{
+ return (class_id >= map->base &&
+ class_id < map->base + map->length);
+}
-static const char *ddebug_class_name(struct ddebug_iter *iter, struct _ddebug *dp)
+static const char *ddebug_class_name(struct ddebug_table *dt, struct _ddebug *dp)
{
- struct ddebug_class_map *map = iter->table->classes;
- int i, nc = iter->table->num_classes;
+ struct _ddebug_class_map *map;
+ int i;
- for (i = 0; i < nc; i++, map++)
- if (class_in_range(dp->class_id, map))
+ for_subvec(i, map, &dt->info, maps)
+ if (ddebug_class_in_range(dp->class_id, map))
return map->class_names[dp->class_id - map->base];
return NULL;
@@ -1194,7 +1195,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
seq_putc(m, '"');
if (dp->class_id != _DPRINTK_CLASS_DFLT) {
- class = ddebug_class_name(iter, dp);
+ class = ddebug_class_name(iter->table, dp);
if (class)
seq_printf(m, " class:%s", class);
else
@@ -1246,7 +1247,7 @@ static const struct proc_ops proc_fops = {
static void ddebug_attach_module_classes(struct ddebug_table *dt, struct _ddebug_info *di)
{
- struct ddebug_class_map *cm;
+ struct _ddebug_class_map *cm;
int i, nc = 0;
/*
@@ -1254,18 +1255,18 @@ static void ddebug_attach_module_classes(struct ddebug_table *dt, struct _ddebug
* the builtin/modular classmap vector/section. Save the start
* and length of the subrange at its edges.
*/
- for_subvec(i, cm, di, classes) {
+ for_subvec(i, cm, di, maps) {
if (!strcmp(cm->mod_name, dt->mod_name)) {
if (!nc) {
v2pr_info("start subrange, class[%d]: module:%s base:%d len:%d ty:%d\n",
i, cm->mod_name, cm->base, cm->length, cm->map_type);
- dt->classes = cm;
+ dt->info.maps.start = cm;
}
nc++;
}
}
if (nc) {
- dt->num_classes = nc;
+ dt->info.maps.len = nc;
vpr_info("module:%s attached %d classes\n", dt->mod_name, nc);
}
}
@@ -1278,10 +1279,10 @@ static int ddebug_add_module(struct _ddebug_info *di, const char *modname)
{
struct ddebug_table *dt;
- if (!di->num_descs)
+ if (!di->descs.len)
return 0;
- v3pr_info("add-module: %s %d sites\n", modname, di->num_descs);
+ v3pr_info("add-module: %s %d sites\n", modname, di->descs.len);
dt = kzalloc_obj(*dt);
if (dt == NULL) {
@@ -1295,19 +1296,18 @@ static int ddebug_add_module(struct _ddebug_info *di, const char *modname)
* this struct ddebug_table.
*/
dt->mod_name = modname;
- dt->ddebugs = di->descs;
- dt->num_ddebugs = di->num_descs;
+ dt->info = *di;
INIT_LIST_HEAD(&dt->link);
- if (di->classes && di->num_classes)
+ if (di->maps.len)
ddebug_attach_module_classes(dt, di);
mutex_lock(&ddebug_lock);
list_add_tail(&dt->link, &ddebug_tables);
mutex_unlock(&ddebug_lock);
- vpr_info("%3u debug prints in module %s\n", di->num_descs, modname);
+ vpr_info("%3u debug prints in module %s\n", di->descs.len, modname);
return 0;
}
@@ -1454,10 +1454,10 @@ static int __init dynamic_debug_init(void)
char *cmdline;
struct _ddebug_info di = {
- .descs = __start___dyndbg,
- .classes = __start___dyndbg_classes,
- .num_descs = __stop___dyndbg - __start___dyndbg,
- .num_classes = __stop___dyndbg_classes - __start___dyndbg_classes,
+ .descs.start = __start___dyndbg_descs,
+ .maps.start = __start___dyndbg_class_maps,
+ .descs.len = __stop___dyndbg_descs - __start___dyndbg_descs,
+ .maps.len = __stop___dyndbg_class_maps - __start___dyndbg_class_maps,
};
#ifdef CONFIG_MODULES
@@ -1468,7 +1468,7 @@ static int __init dynamic_debug_init(void)
}
#endif /* CONFIG_MODULES */
- if (&__start___dyndbg == &__stop___dyndbg) {
+ if (&__start___dyndbg_descs == &__stop___dyndbg_descs) {
if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
pr_warn("_ddebug table is empty in a CONFIG_DYNAMIC_DEBUG build\n");
return 1;
@@ -1478,16 +1478,16 @@ static int __init dynamic_debug_init(void)
return 0;
}
- iter = iter_mod_start = __start___dyndbg;
+ iter = iter_mod_start = __start___dyndbg_descs;
modname = iter->modname;
i = mod_sites = mod_ct = 0;
- for (; iter < __stop___dyndbg; iter++, i++, mod_sites++) {
+ for (; iter < __stop___dyndbg_descs; iter++, i++, mod_sites++) {
if (strcmp(modname, iter->modname)) {
mod_ct++;
- di.num_descs = mod_sites;
- di.descs = iter_mod_start;
+ di.descs.len = mod_sites;
+ di.descs.start = iter_mod_start;
ret = ddebug_add_module(&di, modname);
if (ret)
goto out_err;
@@ -1497,8 +1497,8 @@ static int __init dynamic_debug_init(void)
iter_mod_start = iter;
}
}
- di.num_descs = mod_sites;
- di.descs = iter_mod_start;
+ di.descs.len = mod_sites;
+ di.descs.start = iter_mod_start;
ret = ddebug_add_module(&di, modname);
if (ret)
goto out_err;
@@ -1508,8 +1508,8 @@ static int __init dynamic_debug_init(void)
i, mod_ct, (int)((mod_ct * sizeof(struct ddebug_table)) >> 10),
(int)((i * sizeof(struct _ddebug)) >> 10));
- if (di.num_classes)
- v2pr_info(" %d builtin ddebug class-maps\n", di.num_classes);
+ if (di.maps.len)
+ v2pr_info(" %d builtin ddebug class-maps\n", di.maps.len);
/* now that ddebug tables are loaded, process all boot args
* again to find and activate queries given in dyndbg params.
diff --git a/lib/test_dynamic_debug.c b/lib/test_dynamic_debug.c
index 396144cf351b..8434f70b51bb 100644
--- a/lib/test_dynamic_debug.c
+++ b/lib/test_dynamic_debug.c
@@ -41,7 +41,7 @@ module_param_cb(do_prints, ¶m_ops_do_prints, NULL, 0600);
*/
#define DD_SYS_WRAP(_model, _flags) \
static unsigned long bits_##_model; \
- static struct ddebug_class_param _flags##_model = { \
+ static struct _ddebug_class_param _flags##_model = { \
.bits = &bits_##_model, \
.flags = #_flags, \
.map = &map_##_model, \
--
2.53.0
^ permalink raw reply related
* [PATCH v14 18/92] dyndbg: macrofy a 2-index for-loop pattern
From: Jim Cromie @ 2026-04-23 20:53 UTC (permalink / raw)
To: Arnd Bergmann, Jason Baron, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, Aaron Tomlin, Andrew Morton,
Jonathan Corbet, Shuah Khan, Shuah Khan, Greg Kroah-Hartman,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, Alex Deucher, Christian König, David Airlie,
Gerd Hoffmann, Dmitry Osipenko, Gurchetan Singh, Chia-I Wu,
Matthew Brost, Thomas Hellström, Lyude Paul,
Danilo Krummrich, Patrik Jakobsson, Zack Rusin,
Broadcom internal kernel review list, Louis Chauvet,
Haneen Mohammed, Melissa Wen, Sean Paul, Jocelyn Falempe,
Ruben Wauters, Dave Stevenson, Maíra Canal,
Raspberry Pi Kernel Maintenance, Rob Clark, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, Xinliang Liu,
Tian Tao, Xinwei Kong, Sumit Semwal, Yongqin Liu, John Stultz,
Philipp Zabel, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Chun-Kuang Hu, Matthias Brugger,
AngeloGioacchino Del Regno, Sandy Huang, Heiko Stübner,
Andy Yan, Alain Volmat, Raphael Gallais-Pou, Yannick Fertre,
Raphael Gallais-Pou, Philippe Cornu, Maxime Coquelin,
Alexandre Torgue, Oded Gabbay, Maciej Falkowski, Karol Wachowski,
Rob Herring (Arm), Tomeu Vizoso, Liviu Dudau, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Liu Ying, Laurentiu Palcu, Lucas Stach,
Paul Kocialkowski, Jianmin Lv, Qianhai Wu, Huacai Chen,
Mingcong Bai, Xi Ruoyao, Icenowy Zheng, Laurent Pinchart,
Tomi Valkeinen, Kieran Bingham, Geert Uytterhoeven, Magnus Damm,
Javier Martinez Canillas, Huang Rui, Matthew Auld, Jani Nikula,
Luca Coelho, Russell King, Christian Gmeiner
Cc: linux-arch, linux-kernel, linux-modules, linux-doc,
linux-kselftest, dri-devel, intel-gfx, amd-gfx, virtualization,
intel-xe, nouveau, spice-devel, linux-arm-msm, freedreno, imx,
linux-arm-kernel, linux-mediatek, linux-rockchip, linux-stm32,
linux-renesas-soc, etnaviv, Jim Cromie
In-Reply-To: <20260423-submit-dyndbg-classmap-foundation-v14-0-2b809a8019d0@gmail.com>
dynamic-debug currently has 2 __sections (__dyndbg, __dyndb_classes),
struct _ddebug_info keeps track of them both, with 2 members each:
_vec and _vec#_len.
We need to loop over these sections, with index and record pointer,
making ref to both _vec and _vec_len. This is already fiddly and
error-prone, and will get worse as we add a 3rd section.
Lets instead embed/abstract the fiddly-ness in the `for_subvec()`
macro, and avoid repeating it going forward.
This is a for-loop macro expander, so it syntactically expects to
precede either a single statement or a { block } of them, and the
usual typeof or do-while-0 tricks are unavailable to fix the
multiple-expansion warning.
The macro needs a lot from its caller: it needs 2 local vars, 1 of
which is a ref to a contained struct with named members. To support
these requirements, add:
1. __ASSERT_IS_LVALUE(_X):
ie: ((void)sizeof((void)0, &(x)))
2. __ASSERT_HAS_VEC_MEMBERS(_X, _Y):
compile-time check that the _Y "vector" exists
ie: _X->_Y and _X->num_##_Y are lvalues.
The for_subvec() macro then invokes these in the initialization of the
for-loop; they disappear at runtime.
Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
lib/dynamic_debug.c | 27 ++++++++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 94a66c8537ab..8f614eba8ace 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -129,6 +129,28 @@ do { \
#define v3pr_info(fmt, ...) vnpr_info(3, fmt, ##__VA_ARGS__)
#define v4pr_info(fmt, ...) vnpr_info(4, fmt, ##__VA_ARGS__)
+/*
+ * simplify a repeated for-loop pattern walking N steps in a T _vec
+ * member inside a struct _box. It expects int i and T *_sp to be
+ * declared in the caller.
+ * @_i: caller provided counter.
+ * @_sp: cursor into _vec, to examine each item.
+ * @_box: ptr to a struct containing @_vec member
+ * @_vec: name of a member in @_box
+ */
+#define __ASSERT_IS_LVALUE(x) ((void)sizeof((void)0, &(x)))
+#define __ASSERT_HAS_VEC_MEMBER(_box, _vec) \
+ (void)sizeof((_box)->_vec + (_box)->num_##_vec)
+
+#define for_subvec(_i, _sp, _box, _vec) \
+ for (__ASSERT_IS_LVALUE(_i), \
+ __ASSERT_IS_LVALUE(_sp), \
+ __ASSERT_HAS_VEC_MEMBER(_box, _vec), \
+ (_i) = 0, \
+ (_sp) = (_box)->_vec; \
+ (_i) < (_box)->num_##_vec; \
+ (_i)++, (_sp)++) /* { block } */
+
static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
{
/* trim any trailing newlines */
@@ -156,7 +178,7 @@ static struct ddebug_class_map *ddebug_find_valid_class(struct ddebug_table cons
struct ddebug_class_map *map;
int i, idx;
- for (map = dt->classes, i = 0; i < dt->num_classes; i++, map++) {
+ for_subvec(i, map, dt, classes) {
idx = match_string(map->class_names, map->length, class_string);
if (idx >= 0) {
*class_id = idx + map->base;
@@ -1232,8 +1254,7 @@ static void ddebug_attach_module_classes(struct ddebug_table *dt, struct _ddebug
* the builtin/modular classmap vector/section. Save the start
* and length of the subrange at its edges.
*/
- for (cm = di->classes, i = 0; i < di->num_classes; i++, cm++) {
-
+ for_subvec(i, cm, di, classes) {
if (!strcmp(cm->mod_name, dt->mod_name)) {
if (!nc) {
v2pr_info("start subrange, class[%d]: module:%s base:%d len:%d ty:%d\n",
--
2.53.0
^ permalink raw reply related
* [PATCH v14 17/92] dyndbg: replace classmap list with a vector
From: Jim Cromie @ 2026-04-23 20:53 UTC (permalink / raw)
To: Arnd Bergmann, Jason Baron, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, Aaron Tomlin, Andrew Morton,
Jonathan Corbet, Shuah Khan, Shuah Khan, Greg Kroah-Hartman,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, Alex Deucher, Christian König, David Airlie,
Gerd Hoffmann, Dmitry Osipenko, Gurchetan Singh, Chia-I Wu,
Matthew Brost, Thomas Hellström, Lyude Paul,
Danilo Krummrich, Patrik Jakobsson, Zack Rusin,
Broadcom internal kernel review list, Louis Chauvet,
Haneen Mohammed, Melissa Wen, Sean Paul, Jocelyn Falempe,
Ruben Wauters, Dave Stevenson, Maíra Canal,
Raspberry Pi Kernel Maintenance, Rob Clark, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, Xinliang Liu,
Tian Tao, Xinwei Kong, Sumit Semwal, Yongqin Liu, John Stultz,
Philipp Zabel, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Chun-Kuang Hu, Matthias Brugger,
AngeloGioacchino Del Regno, Sandy Huang, Heiko Stübner,
Andy Yan, Alain Volmat, Raphael Gallais-Pou, Yannick Fertre,
Raphael Gallais-Pou, Philippe Cornu, Maxime Coquelin,
Alexandre Torgue, Oded Gabbay, Maciej Falkowski, Karol Wachowski,
Rob Herring (Arm), Tomeu Vizoso, Liviu Dudau, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Liu Ying, Laurentiu Palcu, Lucas Stach,
Paul Kocialkowski, Jianmin Lv, Qianhai Wu, Huacai Chen,
Mingcong Bai, Xi Ruoyao, Icenowy Zheng, Laurent Pinchart,
Tomi Valkeinen, Kieran Bingham, Geert Uytterhoeven, Magnus Damm,
Javier Martinez Canillas, Huang Rui, Matthew Auld, Jani Nikula,
Luca Coelho, Russell King, Christian Gmeiner
Cc: linux-arch, linux-kernel, linux-modules, linux-doc,
linux-kselftest, dri-devel, intel-gfx, amd-gfx, virtualization,
intel-xe, nouveau, spice-devel, linux-arm-msm, freedreno, imx,
linux-arm-kernel, linux-mediatek, linux-rockchip, linux-stm32,
linux-renesas-soc, etnaviv, Jim Cromie
In-Reply-To: <20260423-submit-dyndbg-classmap-foundation-v14-0-2b809a8019d0@gmail.com>
Classmaps are stored in an elf section/array, but currently are
individually list-linked onto dyndbg's per-module ddebug_table for
operation. This is unnecessary.
Just like dyndbg's descriptors, classes are packed in compile order;
so even with many builtin modules employing multiple classmaps, each
modules' maps are packed contiguously, and can be treated as a
array-start-address & array-length.
So this drops the whole list building operation done in
ddebug_attach_module_classes(), and removes the list-head members.
The "select-by-modname" condition is reused to find the start,end of
the subrange.
NOTE: This "filter-by-modname" on classmaps should really be done in
ddebug_add_module(1); ie at least one step closer to ddebug_init(2),
which already splits up pr-debug descriptors into subranges by
modname, then calls (1) on each. (2) knows nothing of classmaps
currently, and doesn't need to. For now, just add comment.
Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
include/linux/dynamic_debug.h | 1 -
lib/dynamic_debug.c | 62 ++++++++++++++++++++++---------------------
2 files changed, 32 insertions(+), 31 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 92627a03b4d1..9fd36339db52 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -84,7 +84,6 @@ enum class_map_type {
};
struct ddebug_class_map {
- struct list_head link;
struct module *mod;
const char *mod_name; /* needed for builtins */
const char **class_names;
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index a18f4bc63473..94a66c8537ab 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -45,10 +45,11 @@ extern struct ddebug_class_map __start___dyndbg_classes[];
extern struct ddebug_class_map __stop___dyndbg_classes[];
struct ddebug_table {
- struct list_head link, maps;
+ struct list_head link;
const char *mod_name;
- unsigned int num_ddebugs;
struct _ddebug *ddebugs;
+ struct ddebug_class_map *classes;
+ unsigned int num_ddebugs, num_classes;
};
struct ddebug_query {
@@ -149,12 +150,13 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
}
static struct ddebug_class_map *ddebug_find_valid_class(struct ddebug_table const *dt,
- const char *class_string, int *class_id)
+ const char *class_string,
+ int *class_id)
{
struct ddebug_class_map *map;
- int idx;
+ int i, idx;
- list_for_each_entry(map, &dt->maps, link) {
+ for (map = dt->classes, i = 0; i < dt->num_classes; i++, map++) {
idx = match_string(map->class_names, map->length, class_string);
if (idx >= 0) {
*class_id = idx + map->base;
@@ -165,7 +167,6 @@ static struct ddebug_class_map *ddebug_find_valid_class(struct ddebug_table cons
return NULL;
}
-#define __outvar /* filled by callee */
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -227,7 +228,7 @@ static int ddebug_change(const struct ddebug_query *query,
unsigned int nfound = 0;
struct flagsbuf fbuf, nbuf;
struct ddebug_class_map *map = NULL;
- int __outvar valid_class;
+ int valid_class;
/* search for matching ddebugs */
mutex_lock(&ddebug_lock);
@@ -1134,9 +1135,10 @@ static void *ddebug_proc_next(struct seq_file *m, void *p, loff_t *pos)
static const char *ddebug_class_name(struct ddebug_iter *iter, struct _ddebug *dp)
{
- struct ddebug_class_map *map;
+ struct ddebug_class_map *map = iter->table->classes;
+ int i, nc = iter->table->num_classes;
- list_for_each_entry(map, &iter->table->maps, link)
+ for (i = 0; i < nc; i++, map++)
if (class_in_range(dp->class_id, map))
return map->class_names[dp->class_id - map->base];
@@ -1220,30 +1222,31 @@ static const struct proc_ops proc_fops = {
.proc_write = ddebug_proc_write
};
-static void ddebug_attach_module_classes(struct ddebug_table *dt,
- struct ddebug_class_map *classes,
- int num_classes)
+static void ddebug_attach_module_classes(struct ddebug_table *dt, struct _ddebug_info *di)
{
struct ddebug_class_map *cm;
- int i, j, ct = 0;
+ int i, nc = 0;
- for (cm = classes, i = 0; i < num_classes; i++, cm++) {
+ /*
+ * Find this module's classmaps in a subrange/wholerange of
+ * the builtin/modular classmap vector/section. Save the start
+ * and length of the subrange at its edges.
+ */
+ for (cm = di->classes, i = 0; i < di->num_classes; i++, cm++) {
if (!strcmp(cm->mod_name, dt->mod_name)) {
-
- v2pr_info("class[%d]: module:%s base:%d len:%d ty:%d\n", i,
- cm->mod_name, cm->base, cm->length, cm->map_type);
-
- for (j = 0; j < cm->length; j++)
- v3pr_info(" %d: %d %s\n", j + cm->base, j,
- cm->class_names[j]);
-
- list_add(&cm->link, &dt->maps);
- ct++;
+ if (!nc) {
+ v2pr_info("start subrange, class[%d]: module:%s base:%d len:%d ty:%d\n",
+ i, cm->mod_name, cm->base, cm->length, cm->map_type);
+ dt->classes = cm;
+ }
+ nc++;
}
}
- if (ct)
- vpr_info("module:%s attached %d classes\n", dt->mod_name, ct);
+ if (nc) {
+ dt->num_classes = nc;
+ vpr_info("module:%s attached %d classes\n", dt->mod_name, nc);
+ }
}
/*
@@ -1275,10 +1278,9 @@ static int ddebug_add_module(struct _ddebug_info *di, const char *modname)
dt->num_ddebugs = di->num_descs;
INIT_LIST_HEAD(&dt->link);
- INIT_LIST_HEAD(&dt->maps);
if (di->classes && di->num_classes)
- ddebug_attach_module_classes(dt, di->classes, di->num_classes);
+ ddebug_attach_module_classes(dt, di);
mutex_lock(&ddebug_lock);
list_add_tail(&dt->link, &ddebug_tables);
@@ -1391,8 +1393,8 @@ static void ddebug_remove_all_tables(void)
mutex_lock(&ddebug_lock);
while (!list_empty(&ddebug_tables)) {
struct ddebug_table *dt = list_entry(ddebug_tables.next,
- struct ddebug_table,
- link);
+ struct ddebug_table,
+ link);
ddebug_table_free(dt);
}
mutex_unlock(&ddebug_lock);
--
2.53.0
^ permalink raw reply related
* [PATCH v14 16/92] dyndbg: tighten fn-sig of ddebug_apply_class_bitmap
From: Jim Cromie @ 2026-04-23 20:53 UTC (permalink / raw)
To: Arnd Bergmann, Jason Baron, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, Aaron Tomlin, Andrew Morton,
Jonathan Corbet, Shuah Khan, Shuah Khan, Greg Kroah-Hartman,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, Alex Deucher, Christian König, David Airlie,
Gerd Hoffmann, Dmitry Osipenko, Gurchetan Singh, Chia-I Wu,
Matthew Brost, Thomas Hellström, Lyude Paul,
Danilo Krummrich, Patrik Jakobsson, Zack Rusin,
Broadcom internal kernel review list, Louis Chauvet,
Haneen Mohammed, Melissa Wen, Sean Paul, Jocelyn Falempe,
Ruben Wauters, Dave Stevenson, Maíra Canal,
Raspberry Pi Kernel Maintenance, Rob Clark, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, Xinliang Liu,
Tian Tao, Xinwei Kong, Sumit Semwal, Yongqin Liu, John Stultz,
Philipp Zabel, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Chun-Kuang Hu, Matthias Brugger,
AngeloGioacchino Del Regno, Sandy Huang, Heiko Stübner,
Andy Yan, Alain Volmat, Raphael Gallais-Pou, Yannick Fertre,
Raphael Gallais-Pou, Philippe Cornu, Maxime Coquelin,
Alexandre Torgue, Oded Gabbay, Maciej Falkowski, Karol Wachowski,
Rob Herring (Arm), Tomeu Vizoso, Liviu Dudau, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Liu Ying, Laurentiu Palcu, Lucas Stach,
Paul Kocialkowski, Jianmin Lv, Qianhai Wu, Huacai Chen,
Mingcong Bai, Xi Ruoyao, Icenowy Zheng, Laurent Pinchart,
Tomi Valkeinen, Kieran Bingham, Geert Uytterhoeven, Magnus Damm,
Javier Martinez Canillas, Huang Rui, Matthew Auld, Jani Nikula,
Luca Coelho, Russell King, Christian Gmeiner
Cc: linux-arch, linux-kernel, linux-modules, linux-doc,
linux-kselftest, dri-devel, intel-gfx, amd-gfx, virtualization,
intel-xe, nouveau, spice-devel, linux-arm-msm, freedreno, imx,
linux-arm-kernel, linux-mediatek, linux-rockchip, linux-stm32,
linux-renesas-soc, etnaviv, Jim Cromie
In-Reply-To: <20260423-submit-dyndbg-classmap-foundation-v14-0-2b809a8019d0@gmail.com>
old_bits arg is currently a pointer to the input bits, but this could
allow inadvertent changes to the input by the fn. Disallow this.
And constify new_bits while here.
Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
lib/dynamic_debug.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 4313c8803007..a18f4bc63473 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -607,7 +607,8 @@ static int ddebug_exec_queries(char *query, const char *modname)
/* apply a new class-param setting */
static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
- unsigned long *new_bits, unsigned long *old_bits,
+ const unsigned long *new_bits,
+ const unsigned long old_bits,
const char *query_modname)
{
#define QUERY_SIZE 128
@@ -616,12 +617,12 @@ static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
int matches = 0;
int bi, ct;
- if (*new_bits != *old_bits)
+ if (*new_bits != old_bits)
v2pr_info("apply bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
- *old_bits, query_modname ?: "'*'");
+ old_bits, query_modname ?: "'*'");
for (bi = 0; bi < map->length; bi++) {
- if (test_bit(bi, new_bits) == test_bit(bi, old_bits))
+ if (test_bit(bi, new_bits) == test_bit(bi, &old_bits))
continue;
snprintf(query, QUERY_SIZE, "class %s %c%s", map->class_names[bi],
@@ -633,9 +634,9 @@ static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
v2pr_info("bit_%d: %d matches on class: %s -> 0x%lx\n", bi,
ct, map->class_names[bi], *new_bits);
}
- if (*new_bits != *old_bits)
+ if (*new_bits != old_bits)
v2pr_info("applied bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
- *old_bits, query_modname ?: "'*'");
+ old_bits, query_modname ?: "'*'");
return matches;
}
@@ -691,7 +692,7 @@ static int param_set_dyndbg_classnames(const char *instr, const struct kernel_pa
continue;
}
curr_bits ^= BIT(cls_id);
- totct += ddebug_apply_class_bitmap(dcp, &curr_bits, dcp->bits, NULL);
+ totct += ddebug_apply_class_bitmap(dcp, &curr_bits, *dcp->bits, NULL);
*dcp->bits = curr_bits;
v2pr_info("%s: changed bit %d:%s\n", KP_NAME(kp), cls_id,
map->class_names[cls_id]);
@@ -701,7 +702,7 @@ static int param_set_dyndbg_classnames(const char *instr, const struct kernel_pa
old_bits = CLASSMAP_BITMASK(*dcp->lvl);
curr_bits = CLASSMAP_BITMASK(cls_id + (wanted ? 1 : 0 ));
- totct += ddebug_apply_class_bitmap(dcp, &curr_bits, &old_bits, NULL);
+ totct += ddebug_apply_class_bitmap(dcp, &curr_bits, old_bits, NULL);
*dcp->lvl = (cls_id + (wanted ? 1 : 0));
v2pr_info("%s: changed bit-%d: \"%s\" %lx->%lx\n", KP_NAME(kp), cls_id,
map->class_names[cls_id], old_bits, curr_bits);
@@ -755,7 +756,7 @@ static int param_set_dyndbg_module_classes(const char *instr,
inrep &= CLASSMAP_BITMASK(map->length);
}
v2pr_info("bits:0x%lx > %s.%s\n", inrep, mod_name ?: "*", KP_NAME(kp));
- totct += ddebug_apply_class_bitmap(dcp, &inrep, dcp->bits, mod_name);
+ totct += ddebug_apply_class_bitmap(dcp, &inrep, *dcp->bits, mod_name);
*dcp->bits = inrep;
break;
case DD_CLASS_TYPE_LEVEL_NUM:
@@ -768,7 +769,7 @@ static int param_set_dyndbg_module_classes(const char *instr,
old_bits = CLASSMAP_BITMASK(*dcp->lvl);
new_bits = CLASSMAP_BITMASK(inrep);
v2pr_info("lvl:%ld bits:0x%lx > %s\n", inrep, new_bits, KP_NAME(kp));
- totct += ddebug_apply_class_bitmap(dcp, &new_bits, &old_bits, mod_name);
+ totct += ddebug_apply_class_bitmap(dcp, &new_bits, old_bits, mod_name);
*dcp->lvl = inrep;
break;
default:
--
2.53.0
^ permalink raw reply related
* [PATCH v14 15/92] dyndbg: refactor param_set_dyndbg_classes and below
From: Jim Cromie @ 2026-04-23 20:53 UTC (permalink / raw)
To: Arnd Bergmann, Jason Baron, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, Aaron Tomlin, Andrew Morton,
Jonathan Corbet, Shuah Khan, Shuah Khan, Greg Kroah-Hartman,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, Alex Deucher, Christian König, David Airlie,
Gerd Hoffmann, Dmitry Osipenko, Gurchetan Singh, Chia-I Wu,
Matthew Brost, Thomas Hellström, Lyude Paul,
Danilo Krummrich, Patrik Jakobsson, Zack Rusin,
Broadcom internal kernel review list, Louis Chauvet,
Haneen Mohammed, Melissa Wen, Sean Paul, Jocelyn Falempe,
Ruben Wauters, Dave Stevenson, Maíra Canal,
Raspberry Pi Kernel Maintenance, Rob Clark, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, Xinliang Liu,
Tian Tao, Xinwei Kong, Sumit Semwal, Yongqin Liu, John Stultz,
Philipp Zabel, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Chun-Kuang Hu, Matthias Brugger,
AngeloGioacchino Del Regno, Sandy Huang, Heiko Stübner,
Andy Yan, Alain Volmat, Raphael Gallais-Pou, Yannick Fertre,
Raphael Gallais-Pou, Philippe Cornu, Maxime Coquelin,
Alexandre Torgue, Oded Gabbay, Maciej Falkowski, Karol Wachowski,
Rob Herring (Arm), Tomeu Vizoso, Liviu Dudau, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Liu Ying, Laurentiu Palcu, Lucas Stach,
Paul Kocialkowski, Jianmin Lv, Qianhai Wu, Huacai Chen,
Mingcong Bai, Xi Ruoyao, Icenowy Zheng, Laurent Pinchart,
Tomi Valkeinen, Kieran Bingham, Geert Uytterhoeven, Magnus Damm,
Javier Martinez Canillas, Huang Rui, Matthew Auld, Jani Nikula,
Luca Coelho, Russell King, Christian Gmeiner
Cc: linux-arch, linux-kernel, linux-modules, linux-doc,
linux-kselftest, dri-devel, intel-gfx, amd-gfx, virtualization,
intel-xe, nouveau, spice-devel, linux-arm-msm, freedreno, imx,
linux-arm-kernel, linux-mediatek, linux-rockchip, linux-stm32,
linux-renesas-soc, etnaviv, Jim Cromie
In-Reply-To: <20260423-submit-dyndbg-classmap-foundation-v14-0-2b809a8019d0@gmail.com>
Refactor callchain below param_set_dyndbg_classes(1) to allow mod-name
specific settings. Split (1) into upper/lower fns, adding modname
param to lower, and passing NULL in from upper. Below that, add the
same param to ddebug_apply_class_bitmap(), and pass it thru to
_ddebug_queries(), replacing NULL with the param.
This allows the callchain to update the classmap in just one module,
vs just all as currently done. While the sysfs param is unlikely to
ever update just one module, the callchain will be used for modprobe
handling, which should update only that just-probed module.
In ddebug_apply_class_bitmap(), also check for actual changes to the
bits before announcing them, to declutter logs.
No functional change.
Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
lib/dynamic_debug.c | 65 ++++++++++++++++++++++++++++++++---------------------
1 file changed, 40 insertions(+), 25 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 3ae9ecabdad1..4313c8803007 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -605,9 +605,10 @@ static int ddebug_exec_queries(char *query, const char *modname)
return nfound;
}
-/* apply a new bitmap to the sys-knob's current bit-state */
+/* apply a new class-param setting */
static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
- unsigned long *new_bits, unsigned long *old_bits)
+ unsigned long *new_bits, unsigned long *old_bits,
+ const char *query_modname)
{
#define QUERY_SIZE 128
char query[QUERY_SIZE];
@@ -615,7 +616,9 @@ static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
int matches = 0;
int bi, ct;
- v2pr_info("apply: 0x%lx to: 0x%lx\n", *new_bits, *old_bits);
+ if (*new_bits != *old_bits)
+ v2pr_info("apply bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
+ *old_bits, query_modname ?: "'*'");
for (bi = 0; bi < map->length; bi++) {
if (test_bit(bi, new_bits) == test_bit(bi, old_bits))
@@ -624,12 +627,16 @@ static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
snprintf(query, QUERY_SIZE, "class %s %c%s", map->class_names[bi],
test_bit(bi, new_bits) ? '+' : '-', dcp->flags);
- ct = ddebug_exec_queries(query, NULL);
+ ct = ddebug_exec_queries(query, query_modname);
matches += ct;
v2pr_info("bit_%d: %d matches on class: %s -> 0x%lx\n", bi,
ct, map->class_names[bi], *new_bits);
}
+ if (*new_bits != *old_bits)
+ v2pr_info("applied bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
+ *old_bits, query_modname ?: "'*'");
+
return matches;
}
@@ -684,7 +691,7 @@ static int param_set_dyndbg_classnames(const char *instr, const struct kernel_pa
continue;
}
curr_bits ^= BIT(cls_id);
- totct += ddebug_apply_class_bitmap(dcp, &curr_bits, dcp->bits);
+ totct += ddebug_apply_class_bitmap(dcp, &curr_bits, dcp->bits, NULL);
*dcp->bits = curr_bits;
v2pr_info("%s: changed bit %d:%s\n", KP_NAME(kp), cls_id,
map->class_names[cls_id]);
@@ -694,7 +701,7 @@ static int param_set_dyndbg_classnames(const char *instr, const struct kernel_pa
old_bits = CLASSMAP_BITMASK(*dcp->lvl);
curr_bits = CLASSMAP_BITMASK(cls_id + (wanted ? 1 : 0 ));
- totct += ddebug_apply_class_bitmap(dcp, &curr_bits, &old_bits);
+ totct += ddebug_apply_class_bitmap(dcp, &curr_bits, &old_bits, NULL);
*dcp->lvl = (cls_id + (wanted ? 1 : 0));
v2pr_info("%s: changed bit-%d: \"%s\" %lx->%lx\n", KP_NAME(kp), cls_id,
map->class_names[cls_id], old_bits, curr_bits);
@@ -708,18 +715,9 @@ static int param_set_dyndbg_classnames(const char *instr, const struct kernel_pa
return 0;
}
-/**
- * param_set_dyndbg_classes - class FOO >control
- * @instr: string echo>d to sysfs, input depends on map_type
- * @kp: kp->arg has state: bits/lvl, map, map_type
- *
- * Enable/disable prdbgs by their class, as given in the arguments to
- * DECLARE_DYNDBG_CLASSMAP. For LEVEL map-types, enforce relative
- * levels by bitpos.
- *
- * Returns: 0 or <0 if error.
- */
-int param_set_dyndbg_classes(const char *instr, const struct kernel_param *kp)
+static int param_set_dyndbg_module_classes(const char *instr,
+ const struct kernel_param *kp,
+ const char *mod_name)
{
const struct ddebug_class_param *dcp = kp->arg;
const struct ddebug_class_map *map = dcp->map;
@@ -756,8 +754,8 @@ int param_set_dyndbg_classes(const char *instr, const struct kernel_param *kp)
KP_NAME(kp), inrep, CLASSMAP_BITMASK(map->length));
inrep &= CLASSMAP_BITMASK(map->length);
}
- v2pr_info("bits:%lx > %s\n", inrep, KP_NAME(kp));
- totct += ddebug_apply_class_bitmap(dcp, &inrep, dcp->bits);
+ v2pr_info("bits:0x%lx > %s.%s\n", inrep, mod_name ?: "*", KP_NAME(kp));
+ totct += ddebug_apply_class_bitmap(dcp, &inrep, dcp->bits, mod_name);
*dcp->bits = inrep;
break;
case DD_CLASS_TYPE_LEVEL_NUM:
@@ -770,7 +768,7 @@ int param_set_dyndbg_classes(const char *instr, const struct kernel_param *kp)
old_bits = CLASSMAP_BITMASK(*dcp->lvl);
new_bits = CLASSMAP_BITMASK(inrep);
v2pr_info("lvl:%ld bits:0x%lx > %s\n", inrep, new_bits, KP_NAME(kp));
- totct += ddebug_apply_class_bitmap(dcp, &new_bits, &old_bits);
+ totct += ddebug_apply_class_bitmap(dcp, &new_bits, &old_bits, mod_name);
*dcp->lvl = inrep;
break;
default:
@@ -779,16 +777,33 @@ int param_set_dyndbg_classes(const char *instr, const struct kernel_param *kp)
vpr_info("%s: total matches: %d\n", KP_NAME(kp), totct);
return 0;
}
+
+/**
+ * param_set_dyndbg_classes - classmap kparam setter
+ * @instr: string echo>d to sysfs, input depends on map_type
+ * @kp: kp->arg has state: bits/lvl, map, map_type
+ *
+ * enable/disable all class'd pr_debugs in the classmap. For LEVEL
+ * map-types, enforce * relative levels by bitpos.
+ *
+ * Returns: 0 or <0 if error.
+ */
+int param_set_dyndbg_classes(const char *instr, const struct kernel_param *kp)
+{
+ return param_set_dyndbg_module_classes(instr, kp, NULL);
+}
EXPORT_SYMBOL(param_set_dyndbg_classes);
/**
- * param_get_dyndbg_classes - classes reader
+ * param_get_dyndbg_classes - classmap kparam getter
* @buffer: string description of controlled bits -> classes
* @kp: kp->arg has state: bits, map
*
- * Reads last written state, underlying prdbg state may have been
- * altered by direct >control. Displays 0x for DISJOINT, 0-N for
- * LEVEL Returns: #chars written or <0 on error
+ * Reads last written state, underlying pr_debug states may have been
+ * altered by direct >control. Displays 0x for DISJOINT classmap
+ * types, 0-N for LEVEL types.
+ *
+ * Returns: ct of chars written or <0 on error
*/
int param_get_dyndbg_classes(char *buffer, const struct kernel_param *kp)
{
--
2.53.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox