On Tue, Jul 02, 2024 at 09:50:49AM GMT, Uwe Kleine-König wrote:
> 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.
<3 <3 <3
> > 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.
Thanks a lot for the suggestion, it hasn't ocurred to me (despite the
intense task of recompiling it all over several times during
development LOL).
>
> (OK, maybe a minor issue given that most drivers also include
> <linux/module.h> where the #ifdef cannot be dropped.)
For drivers it would be true, but with a very minimal config I am using,
the core code that doesn't include it would get some relative build time
improvement. Definetely worth, considering the machine I am using for
kernel development here (a core2quad cpu LOL).
> > 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().
Yeah, I didn't like the idea of touching it at first, but it makes sense
at this time. Thanks for suggesting!
>
> > +#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?!
I'll do, thanks a lot so far Uwe!
---
Sincerely,
Ágatha Isabelle
she/her
https://agatha.dev
Attachment:
signature.asc
Description: PGP signature
