Code using IS_ENABLED(CONFIG_MODULES) as a C expression may need access to the module structure definitions to compile. Make sure these structure definitions are always visible.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de --- Thomas Weißschuh (3): module: move 'struct module_use' to internal.h module: make structure definitions always visible kunit: test: Drop CONFIG_MODULE ifdeffery
include/linux/module.h | 30 ++++++++++++------------------ kernel/module/internal.h | 7 +++++++ lib/kunit/test.c | 8 -------- 3 files changed, 19 insertions(+), 26 deletions(-) --- base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494 change-id: 20250611-kunit-ifdef-modules-0fefd13ae153
Best regards,
The struct was moved to the public header file in commit c8e21ced08b3 ("module: fix kdb's illicit use of struct module_use."). Back then the structure was used outside of the module core. Nowadays this is not true anymore, so the structure can be made internal.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de --- include/linux/module.h | 7 ------- kernel/module/internal.h | 7 +++++++ 2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h index 92e1420fccdffc9de9f49da9061546cc1e0c89d1..52f7b0487a2733c56e2531a434887e56e1bf45b2 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -313,13 +313,6 @@ void *__symbol_get_gpl(const char *symbol); __used __section(".no_trim_symbol") = __stringify(x); \ (typeof(&x))(__symbol_get(__stringify(x))); })
-/* modules using other modules: kdb wants to see this. */ -struct module_use { - struct list_head source_list; - struct list_head target_list; - struct module *source, *target; -}; - enum module_state { MODULE_STATE_LIVE, /* Normal state. */ MODULE_STATE_COMING, /* Full formed, running module_init. */ diff --git a/kernel/module/internal.h b/kernel/module/internal.h index 8d74b0a21c82b5360977a29736eca78ee6b6be3e..1c2e0b0dc52e72d5ecd2f1b310ce535364b3f33b 100644 --- a/kernel/module/internal.h +++ b/kernel/module/internal.h @@ -109,6 +109,13 @@ struct find_symbol_arg { enum mod_license license; };
+/* modules using other modules */ +struct module_use { + struct list_head source_list; + struct list_head target_list; + struct module *source, *target; +}; + int mod_verify_sig(const void *mod, struct load_info *info); int try_to_force_load(struct module *mod, const char *reason); bool find_symbol(struct find_symbol_arg *fsa);
On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
The struct was moved to the public header file in commit c8e21ced08b3 ("module: fix kdb's illicit use of struct module_use."). Back then the structure was used outside of the module core. Nowadays this is not true anymore, so the structure can be made internal.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de
Reviewed-by: Petr Pavlu petr.pavlu@suse.com
On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
The struct was moved to the public header file in commit c8e21ced08b3 ("module: fix kdb's illicit use of struct module_use."). Back then the structure was used outside of the module core. Nowadays this is not true anymore, so the structure can be made internal.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de
Reviewed-by: Daniel Gomez da.gomez@samsung.com
To write code that works with both CONFIG_MODULES=y and CONFIG_MODULES=n it is convenient to use "if (IS_ENABLED(CONFIG_MODULES))" over raw #ifdef. The code will still fully typechecked but the unreachable parts are discarded by the compiler. This prevents accidental breakage when a certain kconfig combination was not specifically tested by the developer. This pattern is already supported to some extend by module.h defining empty stub functions if CONFIG_MODULES=n. However some users of module.h work on the structured defined by module.h.
Therefore these structure definitions need to be visible, too.
Many structure members are still gated by specific configuration settings. The assumption for those is that the code using them will be gated behind the same configuration setting anyways.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de --- include/linux/module.h | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h index 52f7b0487a2733c56e2531a434887e56e1bf45b2..7f783e71636542b99db3dd869a9387d14992df45 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -302,17 +302,6 @@ static typeof(name) __mod_device_table__##type##__##name \
struct notifier_block;
-#ifdef CONFIG_MODULES - -extern int modules_disabled; /* for sysctl */ -/* Get/put a kernel symbol (calls must be symmetric) */ -void *__symbol_get(const char *symbol); -void *__symbol_get_gpl(const char *symbol); -#define symbol_get(x) ({ \ - static const char __notrim[] \ - __used __section(".no_trim_symbol") = __stringify(x); \ - (typeof(&x))(__symbol_get(__stringify(x))); }) - enum module_state { MODULE_STATE_LIVE, /* Normal state. */ MODULE_STATE_COMING, /* Full formed, running module_init. */ @@ -598,6 +587,18 @@ struct module { struct _ddebug_info dyndbg_info; #endif } ____cacheline_aligned __randomize_layout; + +#ifdef CONFIG_MODULES + +extern int modules_disabled; /* for sysctl */ +/* Get/put a kernel symbol (calls must be symmetric) */ +void *__symbol_get(const char *symbol); +void *__symbol_get_gpl(const char *symbol); +#define symbol_get(x) ({ \ + static const char __notrim[] \ + __used __section(".no_trim_symbol") = __stringify(x); \ + (typeof(&x))(__symbol_get(__stringify(x))); }) + #ifndef MODULE_ARCH_INIT #define MODULE_ARCH_INIT {} #endif
The function stubs exposed by module.h allow the code to compile properly without the ifdeffery. The generated object code stays the same, as the compiler can optimize away all the dead code. As the code is still typechecked developer errors can be detected faster.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de --- lib/kunit/test.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 146d1b48a0965e8aaddb6162928f408bbb542645..019b2ac9c8469021542b610278f8842e100d57ad 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -759,7 +759,6 @@ void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites) } EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);
-#ifdef CONFIG_MODULES static void kunit_module_init(struct module *mod) { struct kunit_suite_set suite_set, filtered_set; @@ -847,7 +846,6 @@ static struct notifier_block kunit_mod_nb = { .notifier_call = kunit_module_notify, .priority = 0, }; -#endif
KUNIT_DEFINE_ACTION_WRAPPER(kfree_action_wrapper, kfree, const void *)
@@ -938,20 +936,14 @@ static int __init kunit_init(void) kunit_debugfs_init();
kunit_bus_init(); -#ifdef CONFIG_MODULES return register_module_notifier(&kunit_mod_nb); -#else - return 0; -#endif } late_initcall(kunit_init);
static void __exit kunit_exit(void) { memset(&kunit_hooks, 0, sizeof(kunit_hooks)); -#ifdef CONFIG_MODULES unregister_module_notifier(&kunit_mod_nb); -#endif
kunit_bus_shutdown();
On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
The function stubs exposed by module.h allow the code to compile properly without the ifdeffery. The generated object code stays the same, as the compiler can optimize away all the dead code. As the code is still typechecked developer errors can be detected faster.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de
I'm worried that patches #2 and #3 make the code harder to understand because they hide what is compiled and when.
Normally, '#ifdef CONFIG_XYZ' or IS_ENABLED(CONFIG_XYZ) directly surrounds functionality that should be conditional. This makes it clear what is used and when.
The patches obscure whether, for instance, kunit_module_notify() in lib/kunit/test.c is actually used and present in the resulting binary behind several steps. Understanding its usage requires tracing the code from kunit_module_notify() to the definition of kunit_mod_nb, then to the register_module_notifier() call, and finally depends on an ifdef in another file, include/linux/module.h.
Is this really better? Are there places where this pattern is already used? Does it actually help in practice, considering that CONFIG_MODULES is enabled in most cases?
On Tue, Jun 17, 2025 at 09:44:49AM +0200, Petr Pavlu wrote:
On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
The function stubs exposed by module.h allow the code to compile properly without the ifdeffery. The generated object code stays the same, as the compiler can optimize away all the dead code. As the code is still typechecked developer errors can be detected faster.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de
I'm worried that patches #2 and #3 make the code harder to understand because they hide what is compiled and when.
Normally, '#ifdef CONFIG_XYZ' or IS_ENABLED(CONFIG_XYZ) directly surrounds functionality that should be conditional. This makes it clear what is used and when.
#ifdef is discouraged in C files and IS_ENABLED(CONFIG_MODULES) does not work (here) without patch #2.
The patches obscure whether, for instance, kunit_module_notify() in lib/kunit/test.c is actually used and present in the resulting binary behind several steps. Understanding its usage requires tracing the code from kunit_module_notify() to the definition of kunit_mod_nb, then to the register_module_notifier() call, and finally depends on an ifdef in another file, include/linux/module.h.
I agree that it is not completely clear what will end up in the binary. On the other hand we can program the happy path and the compiler will take care of all the corner cases. We could add an "if (IS_ENABLED(CONFIG_MODULES))" which does not really change anything but would be clearer to read.
Is this really better? Are there places where this pattern is already used? Does it actually help in practice, considering that CONFIG_MODULES is enabled in most cases?
This came up for me when refactoring some KUnit internal code. I used "kunit.py run" (which uses CONFIG_MODULES=n) to test my changes. But some callers of changed functions were not updated and this wasn't reported.
The stub functions are a standard pattern and already implemented by module.h. I have not found a header which hides structure definitions.
Documentation/process/coding-style.rst:
21) Conditional Compilation ---------------------------
Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c files; doing so makes code harder to read and logic harder to follow. Instead, use such conditionals in a header file defining functions for use in those .c files, providing no-op stub versions in the #else case, and then call those functions unconditionally from .c files. The compiler will avoid generating any code for the stub calls, producing identical results, but the logic will remain easy to follow.
I should add the documentation reference to patch #2.
On 6/17/25 10:39 AM, Thomas Weißschuh wrote:
On Tue, Jun 17, 2025 at 09:44:49AM +0200, Petr Pavlu wrote:
On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
The function stubs exposed by module.h allow the code to compile properly without the ifdeffery. The generated object code stays the same, as the compiler can optimize away all the dead code. As the code is still typechecked developer errors can be detected faster.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de
I'm worried that patches #2 and #3 make the code harder to understand because they hide what is compiled and when.
Normally, '#ifdef CONFIG_XYZ' or IS_ENABLED(CONFIG_XYZ) directly surrounds functionality that should be conditional. This makes it clear what is used and when.
#ifdef is discouraged in C files and IS_ENABLED(CONFIG_MODULES) does not work (here) without patch #2.
The patches obscure whether, for instance, kunit_module_notify() in lib/kunit/test.c is actually used and present in the resulting binary behind several steps. Understanding its usage requires tracing the code from kunit_module_notify() to the definition of kunit_mod_nb, then to the register_module_notifier() call, and finally depends on an ifdef in another file, include/linux/module.h.
I agree that it is not completely clear what will end up in the binary. On the other hand we can program the happy path and the compiler will take care of all the corner cases. We could add an "if (IS_ENABLED(CONFIG_MODULES))" which does not really change anything but would be clearer to read.
Is this really better? Are there places where this pattern is already used? Does it actually help in practice, considering that CONFIG_MODULES is enabled in most cases?
This came up for me when refactoring some KUnit internal code. I used "kunit.py run" (which uses CONFIG_MODULES=n) to test my changes. But some callers of changed functions were not updated and this wasn't reported.
I see.
The stub functions are a standard pattern and already implemented by module.h.
Stub functions are ok, I have no concerns about that pattern.
I have not found a header which hides structure definitions.
It seems you're right. I think that makes patch #2 acceptable, it is consistent with other kernel code.
Documentation/process/coding-style.rst:
- Conditional Compilation
Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c files; doing so makes code harder to read and logic harder to follow. Instead, use such conditionals in a header file defining functions for use in those .c files, providing no-op stub versions in the #else case, and then call those functions unconditionally from .c files. The compiler will avoid generating any code for the stub calls, producing identical results, but the logic will remain easy to follow.
I should add the documentation reference to patch #2.
This part discusses stub functions. I feel patch #3 stretches the intention of the coding style described here. As discussed, the patch somewhat hides when the functions are actually used, which might not be desirable, but I'll leave it to the kunit folks to decide what they prefer.
On Thu, 12 Jun 2025 at 22:54, Thomas Weißschuh thomas.weissschuh@linutronix.de wrote:
The function stubs exposed by module.h allow the code to compile properly without the ifdeffery. The generated object code stays the same, as the compiler can optimize away all the dead code. As the code is still typechecked developer errors can be detected faster.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de
Acked-by: David Gow davidgow@google.com
Cheers, -- David
lib/kunit/test.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 146d1b48a0965e8aaddb6162928f408bbb542645..019b2ac9c8469021542b610278f8842e100d57ad 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -759,7 +759,6 @@ void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites) } EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);
-#ifdef CONFIG_MODULES static void kunit_module_init(struct module *mod) { struct kunit_suite_set suite_set, filtered_set; @@ -847,7 +846,6 @@ static struct notifier_block kunit_mod_nb = { .notifier_call = kunit_module_notify, .priority = 0, }; -#endif
KUNIT_DEFINE_ACTION_WRAPPER(kfree_action_wrapper, kfree, const void *)
@@ -938,20 +936,14 @@ static int __init kunit_init(void) kunit_debugfs_init();
kunit_bus_init();-#ifdef CONFIG_MODULES return register_module_notifier(&kunit_mod_nb); -#else
return 0;-#endif } late_initcall(kunit_init);
static void __exit kunit_exit(void) { memset(&kunit_hooks, 0, sizeof(kunit_hooks)); -#ifdef CONFIG_MODULES unregister_module_notifier(&kunit_mod_nb); -#endif
kunit_bus_shutdown();-- 2.49.0
linux-kselftest-mirror@lists.linaro.org
-
Daniel Gomez -
David Gow -
Petr Pavlu -
Thomas Weißschuh