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
On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
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.
We are missing here which structures are needed. + we are making more things visible than what we actually need.
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.
I think code and kconfig need to reflect the actual dependencies. For example, if CONFIG_LIVEPATCH depends on CONFIG_MODULES, we need to specify that in Kconfig with depends on, as well as keep the code gated by these 2 configs with ifdef/IS_ENABLED.
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))); })
The patch exposes data structures that are not needed. + breaks the config dependencies.
For example, before this patch:
#ifdef CONFIG_MODULES
{...}
struct mod_tree_node {
{...}
struct module_memory { void *base; bool is_rox; unsigned int size;
#ifdef CONFIG_MODULES_TREE_LOOKUP struct mod_tree_node mtn; #endif };
{...} #endif /* CONFIG_MODULES */
After the patch, mod_tree_node is not needed externally. And the mtn field in module_memory is exposed only under MODULES_TREE_LOOKUP and not MODULES + MODULES_TREE_LOOKUP.
I general, I see the issues I mentioned with LIVEPATCH, mod_tree_node, macros, and LOOKUP.
#define MODULE_ARCH_INIT {} #endif
On 7/7/25 9:11 PM, Daniel Gomez wrote:
On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
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.
We are missing here which structures are needed. + we are making more things visible than what we actually need.
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.
I think code and kconfig need to reflect the actual dependencies. For example, if CONFIG_LIVEPATCH depends on CONFIG_MODULES, we need to specify that in Kconfig with depends on, as well as keep the code gated by these 2 configs with ifdef/IS_ENABLED.
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))); })
The patch exposes data structures that are not needed. + breaks the config dependencies.
For example, before this patch:
#ifdef CONFIG_MODULES
{...}
struct mod_tree_node {
{...}
struct module_memory { void *base; bool is_rox; unsigned int size;
#ifdef CONFIG_MODULES_TREE_LOOKUP struct mod_tree_node mtn; #endif };
{...} #endif /* CONFIG_MODULES */
After the patch, mod_tree_node is not needed externally. And the mtn field in module_memory is exposed only under MODULES_TREE_LOOKUP and not MODULES
- MODULES_TREE_LOOKUP.
I general, I see the issues I mentioned with LIVEPATCH, mod_tree_node, macros, and LOOKUP.
I think the idea is that having unnecessary structures in header files isn't particularly harmful, as they won't affect the resulting binary. On the other hand, they can help with type checking of conditional code as shown by patch #3.
This is different compared to "extern int modules_disabled;" and "void *__symbol_get(const char *symbol);" which the patch correctly still protects by '#ifdef CONFIG_MODULES'. Not hiding them could result in successful compilation but an error only at link time.
On Mon, Jul 07, 2025 at 09:11:05PM +0200, Daniel Gomez wrote:
On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
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.
We are missing here which structures are needed. + we are making more things visible than what we actually need.
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.
I think code and kconfig need to reflect the actual dependencies. For example, if CONFIG_LIVEPATCH depends on CONFIG_MODULES, we need to specify that in Kconfig with depends on, as well as keep the code gated by these 2 configs with ifdef/IS_ENABLED.
If CONFIG_LIVEPATCH depends on CONFIG_MODULES in kconfig then IS_ENABLED(CONFIG_LIVEPATCH) will depend on CONFIG_MODULES automatically. There is no need for another explicit IS_ENABLED(CONFIG_MODULES).
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))); })
The patch exposes data structures that are not needed. + breaks the config dependencies.
If we want to expose 'struct module' to !CONFIG_MODULES code, all it's effective member types also need to be included. With my patch these member types are actually still implictly gated behind CONFIG_MODULES as they depend on it through kconfig.
For example, before this patch:
#ifdef CONFIG_MODULES
{...}
struct mod_tree_node {
{...}
struct module_memory { void *base; bool is_rox; unsigned int size;
#ifdef CONFIG_MODULES_TREE_LOOKUP struct mod_tree_node mtn; #endif };
{...} #endif /* CONFIG_MODULES */
After the patch, mod_tree_node is not needed externally.
Can you explain what you mean with "not needed externally"? 'struct mod_tree_node' is only ever used by core module code. It is only public because it is embedded in the public 'struct module'
And the mtn field in module_memory is exposed only under MODULES_TREE_LOOKUP and not MODULES
- MODULES_TREE_LOOKUP.
As mentioned above, MODULES_TREE_LOOKUP && !MODULES can never happen.
I general, I see the issues I mentioned with LIVEPATCH, mod_tree_node, macros, and LOOKUP.
#define MODULE_ARCH_INIT {} #endif
On 11/07/2025 08.29, Thomas WeiÃschuh wrote:
On Mon, Jul 07, 2025 at 09:11:05PM +0200, Daniel Gomez wrote:
On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
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.
We are missing here which structures are needed. + we are making more things visible than what we actually need.
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.
I think code and kconfig need to reflect the actual dependencies. For example, if CONFIG_LIVEPATCH depends on CONFIG_MODULES, we need to specify that in Kconfig with depends on, as well as keep the code gated by these 2 configs with ifdef/IS_ENABLED.
If CONFIG_LIVEPATCH depends on CONFIG_MODULES in kconfig then IS_ENABLED(CONFIG_LIVEPATCH) will depend on CONFIG_MODULES automatically. There is no need for another explicit IS_ENABLED(CONFIG_MODULES).
This makes sense to me. My assessment before to reflect in code what we have in kconfig does not scale. Thanks.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de
include/linux/module.h | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
{...}
After the patch, mod_tree_node is not needed externally.
Can you explain what you mean with "not needed externally"? 'struct mod_tree_node' is only ever used by core module code. It is only public because it is embedded in the public 'struct module'
But only when MODULES_TREE_LOOKUP is enabled. Now, all kernels (regardless of that config) will define mod_tree_node data structure.
However, Petr already stated that is harmless to do so. I was trying here to not be useless.
With that, changes look good to me:
Reviewed-by: Daniel Gomez da.gomez@samsung.com
On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
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
Nit: I suggest keeping MODULE_ARCH_INIT in its current position, immediately after the 'struct module' declaration, because the macro is directly tied to that structure.
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
On 12/06/2025 16.53, 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
Reviewed-by: Daniel Gomez da.gomez@samsung.com
linux-kselftest-mirror@lists.linaro.org
-
Daniel Gomez -
David Gow -
Petr Pavlu -
Thomas Weißschuh