| Message ID | 20230419-mips-check_bugs-init-attribute-v1-1-91e6eed55b89@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: Mark check_bugs{,_early}() as __init | expand |
On Wed, Apr 19, 2023 at 11:43 AM Nathan Chancellor <nathan@kernel.org> wrote: > > After commit ac7c3e4ff401 ("compiler: enable CONFIG_OPTIMIZE_INLINING > forcibly"), a compiler may choose not to inline a function marked with > just 'inline'. If check_bugs() is not inlined into start_kernel(), which > occurs when building with clang after commit 9ea7e6b62c2b ("init: Mark > [arch_call_]rest_init() __noreturn"), modpost complains with: > > WARNING: modpost: vmlinux.o: section mismatch in reference: check_bugs (section: .text) -> check_bugs32 (section: .init.text) > > check_bugs() is only called from start_kernel(), which itself is marked > __init, so mark check_bugs() as __init as well to clear up the warning > and make everything work properly. > > While there is currently no warning about check_bugs_early(), it could > have the same problem, as it is called from arch_setup() and calls > check_bugs64_early(), both marked __init. Mark it as __init for the same > reason as above. > > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > --- > NOTE: This is based on v6.3-rc7, as the issue shows up due to a patch in > the tip tree, but this appears to be an ancient issue that could have > showed up at any point (hence why no explicit Fixes tag), so I chose a > base that should allow either the MIPS or tip folks to apply this patch. Probably for the MIPS tree. > > Additionally, I was tempted to remove the explicit 'inline' since the > compiler is free to do whatever it wants anyways but this is a static > function in a header so we would need to add '__maybe_unused', which is > already added with 'inline' in a normal build so I just left it alone. > --- > arch/mips/include/asm/bugs.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/mips/include/asm/bugs.h b/arch/mips/include/asm/bugs.h > index d72dc6e1cf3c..9b9bf9bc7d24 100644 > --- a/arch/mips/include/asm/bugs.h > +++ b/arch/mips/include/asm/bugs.h > @@ -24,13 +24,13 @@ extern void check_bugs64_early(void); > extern void check_bugs32(void); > extern void check_bugs64(void); > > -static inline void check_bugs_early(void) > +static inline void __init check_bugs_early(void) > { > if (IS_ENABLED(CONFIG_CPU_R4X00_BUGS64)) > check_bugs64_early(); > } If the only call site is in arch/mips/kernel/setup.c, then perhaps we can move the definition of check_bugs_early there and mark it static __init and drop inline? Unless we foresee other callers potentially in the future? Patch otherwise LGTM. Thanks for the patch! > > -static inline void check_bugs(void) > +static inline void __init check_bugs(void) > { > unsigned int cpu = smp_processor_id(); > > > --- > base-commit: 6a8f57ae2eb07ab39a6f0ccad60c760743051026 > change-id: 20230419-mips-check_bugs-init-attribute-026103bdb255 > > Best regards, > -- > Nathan Chancellor <nathan@kernel.org> >
On Wed, Apr 19, 2023 at 03:03:31PM -0700, Nick Desaulniers wrote: > On Wed, Apr 19, 2023 at 11:43 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > After commit ac7c3e4ff401 ("compiler: enable CONFIG_OPTIMIZE_INLINING > > forcibly"), a compiler may choose not to inline a function marked with > > just 'inline'. If check_bugs() is not inlined into start_kernel(), which > > occurs when building with clang after commit 9ea7e6b62c2b ("init: Mark > > [arch_call_]rest_init() __noreturn"), modpost complains with: > > > > WARNING: modpost: vmlinux.o: section mismatch in reference: check_bugs (section: .text) -> check_bugs32 (section: .init.text) > > > > check_bugs() is only called from start_kernel(), which itself is marked > > __init, so mark check_bugs() as __init as well to clear up the warning > > and make everything work properly. > > > > While there is currently no warning about check_bugs_early(), it could > > have the same problem, as it is called from arch_setup() and calls > > check_bugs64_early(), both marked __init. Mark it as __init for the same > > reason as above. > > > > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > > --- > > NOTE: This is based on v6.3-rc7, as the issue shows up due to a patch in > > the tip tree, but this appears to be an ancient issue that could have > > showed up at any point (hence why no explicit Fixes tag), so I chose a > > base that should allow either the MIPS or tip folks to apply this patch. > > Probably for the MIPS tree. > > > > > Additionally, I was tempted to remove the explicit 'inline' since the > > compiler is free to do whatever it wants anyways but this is a static > > function in a header so we would need to add '__maybe_unused', which is > > already added with 'inline' in a normal build so I just left it alone. > > --- > > arch/mips/include/asm/bugs.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/mips/include/asm/bugs.h b/arch/mips/include/asm/bugs.h > > index d72dc6e1cf3c..9b9bf9bc7d24 100644 > > --- a/arch/mips/include/asm/bugs.h > > +++ b/arch/mips/include/asm/bugs.h > > @@ -24,13 +24,13 @@ extern void check_bugs64_early(void); > > extern void check_bugs32(void); > > extern void check_bugs64(void); > > > > -static inline void check_bugs_early(void) > > +static inline void __init check_bugs_early(void) > > { > > if (IS_ENABLED(CONFIG_CPU_R4X00_BUGS64)) > > check_bugs64_early(); > > } > > If the only call site is in arch/mips/kernel/setup.c, then perhaps we > can move the definition of check_bugs_early there and mark it static > __init and drop inline? Sure, we could even go a step further and just copy the body into the one call site ourselves, I see little reason for this to be a dedicated function. That is probably best done in a separate patch altogether in lieu of just adding __init. > Unless we foresee other callers potentially in the future? Patch > otherwise LGTM. Thanks for the patch! Seems unlikely, I did a super scientific survey of several different revisions (:P) and check_bugs_early() is called once in each one: v3.0:arch/mips/kernel/setup.c: check_bugs_early(); v3.5:arch/mips/kernel/setup.c: check_bugs_early(); v3.10:arch/mips/kernel/setup.c: check_bugs_early(); v4.0:arch/mips/kernel/setup.c: check_bugs_early(); v4.5:arch/mips/kernel/setup.c: check_bugs_early(); v5.0:arch/mips/kernel/setup.c: check_bugs_early(); v4.10:arch/mips/kernel/setup.c: check_bugs_early(); v5.5:arch/mips/kernel/setup.c: check_bugs_early(); v5.10:arch/mips/kernel/setup.c: check_bugs_early(); v6.0:arch/mips/kernel/setup.c: check_bugs_early(); I'll do whatever Thomas would prefer. > > > > -static inline void check_bugs(void) > > +static inline void __init check_bugs(void) > > { > > unsigned int cpu = smp_processor_id(); > > > > > > --- > > base-commit: 6a8f57ae2eb07ab39a6f0ccad60c760743051026 > > change-id: 20230419-mips-check_bugs-init-attribute-026103bdb255 > > > > Best regards, > > -- > > Nathan Chancellor <nathan@kernel.org> > > > > > -- > Thanks, > ~Nick Desaulniers
On Wed, Apr 19, 2023 at 04:37:10PM -0700, Nathan Chancellor wrote: > > > diff --git a/arch/mips/include/asm/bugs.h b/arch/mips/include/asm/bugs.h > > > index d72dc6e1cf3c..9b9bf9bc7d24 100644 > > > --- a/arch/mips/include/asm/bugs.h > > > +++ b/arch/mips/include/asm/bugs.h > > > @@ -24,13 +24,13 @@ extern void check_bugs64_early(void); > > > extern void check_bugs32(void); > > > extern void check_bugs64(void); > > > > > > -static inline void check_bugs_early(void) > > > +static inline void __init check_bugs_early(void) > > > { > > > if (IS_ENABLED(CONFIG_CPU_R4X00_BUGS64)) > > > check_bugs64_early(); > > > } > > > > If the only call site is in arch/mips/kernel/setup.c, then perhaps we > > can move the definition of check_bugs_early there and mark it static > > __init and drop inline? > > Sure, we could even go a step further and just copy the body into the > one call site ourselves, I see little reason for this to be a dedicated > function. That is probably best done in a separate patch altogether in > lieu of just adding __init. I don't expect any new checks added to check_bugs_early so let's move the whole function into the call site. Thomas.
diff --git a/arch/mips/include/asm/bugs.h b/arch/mips/include/asm/bugs.h index d72dc6e1cf3c..9b9bf9bc7d24 100644 --- a/arch/mips/include/asm/bugs.h +++ b/arch/mips/include/asm/bugs.h @@ -24,13 +24,13 @@ extern void check_bugs64_early(void); extern void check_bugs32(void); extern void check_bugs64(void); -static inline void check_bugs_early(void) +static inline void __init check_bugs_early(void) { if (IS_ENABLED(CONFIG_CPU_R4X00_BUGS64)) check_bugs64_early(); } -static inline void check_bugs(void) +static inline void __init check_bugs(void) { unsigned int cpu = smp_processor_id();
After commit ac7c3e4ff401 ("compiler: enable CONFIG_OPTIMIZE_INLINING forcibly"), a compiler may choose not to inline a function marked with just 'inline'. If check_bugs() is not inlined into start_kernel(), which occurs when building with clang after commit 9ea7e6b62c2b ("init: Mark [arch_call_]rest_init() __noreturn"), modpost complains with: WARNING: modpost: vmlinux.o: section mismatch in reference: check_bugs (section: .text) -> check_bugs32 (section: .init.text) check_bugs() is only called from start_kernel(), which itself is marked __init, so mark check_bugs() as __init as well to clear up the warning and make everything work properly. While there is currently no warning about check_bugs_early(), it could have the same problem, as it is called from arch_setup() and calls check_bugs64_early(), both marked __init. Mark it as __init for the same reason as above. Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- NOTE: This is based on v6.3-rc7, as the issue shows up due to a patch in the tip tree, but this appears to be an ancient issue that could have showed up at any point (hence why no explicit Fixes tag), so I chose a base that should allow either the MIPS or tip folks to apply this patch. Additionally, I was tempted to remove the explicit 'inline' since the compiler is free to do whatever it wants anyways but this is a static function in a header so we would need to add '__maybe_unused', which is already added with 'inline' in a normal build so I just left it alone. --- arch/mips/include/asm/bugs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- base-commit: 6a8f57ae2eb07ab39a6f0ccad60c760743051026 change-id: 20230419-mips-check_bugs-init-attribute-026103bdb255 Best regards,