Hello Ágatha,
On Tue, Jul 02, 2024 at 02:44:31AM -0300, Ágatha Isabelle Chris Moreira Guedes wrote:
> ACKNOWLEDGEMENTS
> Thanks for Jookia, heat and ukleinek for the important comments &
> suggestions on this patch prior to submission.
FTR: That happend in the #kernelnewbies irc channel.
> drivers/staging/Makefile | 2 ++
> include/linux/init.h | 6 ++++++
> include/linux/module.h | 33 +++++++++++++++++++++++++++++++++
> init/main.c | 20 ++++++++++++++++++++
> kernel/module/main.c | 4 +---
> 5 files changed, 62 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index 5390879b5d1b..7cea13436426 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -1,6 +1,8 @@
> # SPDX-License-Identifier: GPL-2.0
> # Makefile for staging directory
>
> +subdir-ccflags-y += -DSTAGING_CODE
> +
> obj-y += media/
> obj-$(CONFIG_FB_OLPC_DCON) += olpc_dcon/
> obj-$(CONFIG_RTL8192E) += rtl8192e/
> diff --git a/include/linux/init.h b/include/linux/init.h
> index 58cef4c2e59a..68c37600958f 100644
> --- a/include/linux/init.h
> +++ b/include/linux/init.h
> @@ -397,4 +397,10 @@ void __init parse_early_options(char *cmdline);
> #define __exit_p(x) NULL
> #endif
>
> +#ifdef CONFIG_STAGING
> +#ifndef __ASSEMBLY__
> +extern void staging_taint(const char *code_id, bool module);
> +#endif /* __ASSEMBLY__ */
> +#endif /* CONFIG_STAGING */
You could drop the #ifdef for CONFIG_STAGING here. The obvious downside
of this suggestion is that then you have a declaration of a function
that isn't available depending on configuration. However the compiler
doesn't care and the upside of not having CONFIG_STAGING in
<linux/init.h> is that compile units that have nothing to do with
CONFIG_STAGING but include <linux/init.h> won't get recompiled if you
switch the setting.
(OK, maybe a minor issue given that most drivers also include
<linux/module.h> where the #ifdef cannot be dropped.)
> #endif /* _LINUX_INIT_H */
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 330ffb59efe5..ffe58f5d143b 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -76,6 +76,39 @@ extern struct module_attribute module_uevent;
> extern int init_module(void);
> extern void cleanup_module(void);
>
> +#ifdef CONFIG_STAGING
> +
> +#define __lower_define_initcall(fn, id) ___define_initcall(fn, id, .initcall##id)
> +
> +/**
> + * __staging_define_initcall(fn,id) - staging initialization entry point
> + * @fn: the function to run at kernel boot time
> + * @id: the initcall level
> + *
> + * __staging_define_initcall() will ensure the drive's init function is always
> + * called during initcalls for staging code by producing a wrapper function.
> + * It applies if a module from the drivers/staging subtree is builtin to the
> + * kernel. It reproduces the behavior in load_module() by tainting the kernel
> + * and logging a warning about the code quality.
> + */
> +
> +#define __staging_define_initcall(fn, id) \
> + static int __init __staging_wrapped_##fn(void) \
> + { \
> + staging_taint(__FILE__, false); \
> + return fn(); \
> + } \
> +__lower_define_initcall(__staging_wrapped_##fn, id)
> +
> +#ifdef STAGING_CODE
> +
> +#undef __define_initcall
> +#define __define_initcall(fn, id) __staging_define_initcall(fn, id)
undefining macros makes the implemented logic hard to understand. While
it allows to keep the touched code compact, in the long run it's more
important that it's understandable.
So I suggest to modify the original definition of __define_initcall().
> +#endif /* STAGING_CODE */
> +
> +#endif /* CONFIG_STAGING */
> +
> #ifndef MODULE
> /**
> * module_init() - driver initialization entry point
I like the idea and think the missing taint for built-in code is an
oversight that went unnoticed for an astonishingly long time (since 2008
if my quick research is right).
Maybe add
Fixes: 061b1bd394ca ("Staging: add TAINT_CRAP for all drivers/staging code")
to the commit log?!
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
