| Message ID | 20200324164005.8259-1-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: mark some functions as weak to avoid conflict with Octeon ones | expand |
On Tue, Mar 24, 2020 at 9:40 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > MIPS provides multiple definitions for the following functions: > > fw_init_cmdline > __delay > __udelay > __ndelay > memmove > __rmemcpy > memcpy > __copy_user > > The generic ones are defined in lib-y objects, which are overridden by > the Octeon ones when CONFIG_CAVIUM_OCTEON_SOC is enabled. > > The use of EXPORT_SYMBOL in static libraries potentially causes a > problem for the llvm linker [1]. So, I want to forcibly link lib-y > objects to vmlinux when CONFIG_MODULES=y. > > As a groundwork, we must fix multiple definitions that have been > hidden by lib-y. > > In this case, the generic implementations in arch/mips/lib/ are > weaker than the ones in arch/mips/cavium-octen/, so annotating __weak > is a straight-forward solution. > > I also removed EXPORT_SYMBOL from the Octeon files to avoid the > 'exported twice' warnings from modpost. > > [1]: https://github.com/ClangBuiltLinux/linux/issues/515 > > Reported-by: kbuild test robot <lkp@intel.com> > Suggested-by: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > --- > > arch/mips/cavium-octeon/csrc-octeon.c | 4 ---- > arch/mips/cavium-octeon/octeon-memcpy.S | 3 --- > arch/mips/fw/lib/cmdline.c | 2 +- > arch/mips/lib/delay.c | 6 +++--- > arch/mips/lib/memcpy.S | 5 +++++ > 5 files changed, 9 insertions(+), 11 deletions(-) > > diff --git a/arch/mips/cavium-octeon/csrc-octeon.c b/arch/mips/cavium-octeon/csrc-octeon.c > index 124817609ce0..fdc28fb5eda4 100644 > --- a/arch/mips/cavium-octeon/csrc-octeon.c > +++ b/arch/mips/cavium-octeon/csrc-octeon.c > @@ -153,7 +153,6 @@ void __udelay(unsigned long us) > while (end > cur) > cur = read_c0_cvmcount(); > } > -EXPORT_SYMBOL(__udelay); > > void __ndelay(unsigned long ns) > { > @@ -167,7 +166,6 @@ void __ndelay(unsigned long ns) > while (end > cur) > cur = read_c0_cvmcount(); > } > -EXPORT_SYMBOL(__ndelay); > > void __delay(unsigned long loops) > { > @@ -179,8 +177,6 @@ void __delay(unsigned long loops) > while (end > cur) > cur = read_c0_cvmcount(); > } > -EXPORT_SYMBOL(__delay); > - > > /** > * octeon_io_clk_delay - wait for a given number of io clock cycles to pass. > diff --git a/arch/mips/cavium-octeon/octeon-memcpy.S b/arch/mips/cavium-octeon/octeon-memcpy.S > index 0a7c9834b81c..3eb8d1a72d7f 100644 > --- a/arch/mips/cavium-octeon/octeon-memcpy.S > +++ b/arch/mips/cavium-octeon/octeon-memcpy.S > @@ -147,11 +147,9 @@ > */ > .align 5 > LEAF(memcpy) /* a0=dst a1=src a2=len */ > -EXPORT_SYMBOL(memcpy) > move v0, dst /* return value */ > __memcpy: > FEXPORT(__copy_user) > -EXPORT_SYMBOL(__copy_user) > /* > * Note: dst & src may be unaligned, len may be 0 > * Temps > @@ -438,7 +436,6 @@ s_exc: > > .align 5 > LEAF(memmove) > -EXPORT_SYMBOL(memmove) > ADD t0, a0, a2 > ADD t1, a1, a2 > sltu t0, a1, t0 # dst + len <= src -> memcpy > diff --git a/arch/mips/fw/lib/cmdline.c b/arch/mips/fw/lib/cmdline.c > index 6ecda64ad184..e1f9a0c23005 100644 > --- a/arch/mips/fw/lib/cmdline.c > +++ b/arch/mips/fw/lib/cmdline.c > @@ -16,7 +16,7 @@ int fw_argc; > int *_fw_argv; > int *_fw_envp; > > -void __init fw_init_cmdline(void) > +void __init __weak fw_init_cmdline(void) > { > int i; > > diff --git a/arch/mips/lib/delay.c b/arch/mips/lib/delay.c > index 68c495ed71e3..ba0ae7da5ced 100644 > --- a/arch/mips/lib/delay.c > +++ b/arch/mips/lib/delay.c > @@ -24,7 +24,7 @@ > #define GCC_DADDI_IMM_ASM() "r" > #endif > > -void __delay(unsigned long loops) > +void __weak __delay(unsigned long loops) > { > __asm__ __volatile__ ( > " .set noreorder \n" > @@ -48,7 +48,7 @@ EXPORT_SYMBOL(__delay); > * a constant) > */ > > -void __udelay(unsigned long us) > +void __weak __udelay(unsigned long us) > { > unsigned int lpj = raw_current_cpu_data.udelay_val; > > @@ -56,7 +56,7 @@ void __udelay(unsigned long us) > } > EXPORT_SYMBOL(__udelay); > > -void __ndelay(unsigned long ns) > +void __weak __ndelay(unsigned long ns) > { > unsigned int lpj = raw_current_cpu_data.udelay_val; > > diff --git a/arch/mips/lib/memcpy.S b/arch/mips/lib/memcpy.S > index f7994d936505..f2f58326b927 100644 > --- a/arch/mips/lib/memcpy.S > +++ b/arch/mips/lib/memcpy.S > @@ -598,6 +598,9 @@ SEXC(1) > nop > .endm > > + .weak memmove > + .weak __rmemcpy > + > .align 5 > LEAF(memmove) > EXPORT_SYMBOL(memmove) > @@ -655,6 +658,8 @@ LEAF(__rmemcpy) /* a0=dst a1=src a2=len */ > * the number of uncopied bytes. > * memcpy sets v0 to dst. > */ > + .weak memcpy > + .weak __copy_user I think it would be better to use SYM_FUNC_START_WEAK from include/linux/linkage.h rather than LEAF, but it looks like LEAF uses a different value for ALIGN, and sets up call frame information CFI. So in that case, no complaints. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Thanks for the patch! > .align 5 > LEAF(memcpy) /* a0=dst a1=src a2=len */ > EXPORT_SYMBOL(memcpy) > -- > 2.17.1 > > -- -- Thanks, ~Nick Desaulniers
On Wed, 25 Mar 2020, Masahiro Yamada wrote: > MIPS provides multiple definitions for the following functions: > > fw_init_cmdline > __delay > __udelay > __ndelay > memmove > __rmemcpy > memcpy > __copy_user > > The generic ones are defined in lib-y objects, which are overridden by > the Octeon ones when CONFIG_CAVIUM_OCTEON_SOC is enabled. > > The use of EXPORT_SYMBOL in static libraries potentially causes a > problem for the llvm linker [1]. So, I want to forcibly link lib-y > objects to vmlinux when CONFIG_MODULES=y. > > As a groundwork, we must fix multiple definitions that have been > hidden by lib-y. IIUC that causes known dead code to be included in the kernel image. Wouldn't it be possible to actually omit replaced functions from output by keying the build of the sources providing generic code with appropriate CONFIG_* settings (such as CONFIG_GENERIC_DELAY, CONFIG_GENERIC_MEMCPY, etc. or suchlike)? Maciej
Hi Maciej, On Wed, Mar 25, 2020 at 11:46 AM Maciej W. Rozycki <macro@linux-mips.org> wrote: > > On Wed, 25 Mar 2020, Masahiro Yamada wrote: > > > MIPS provides multiple definitions for the following functions: > > > > fw_init_cmdline > > __delay > > __udelay > > __ndelay > > memmove > > __rmemcpy > > memcpy > > __copy_user > > > > The generic ones are defined in lib-y objects, which are overridden by > > the Octeon ones when CONFIG_CAVIUM_OCTEON_SOC is enabled. > > > > The use of EXPORT_SYMBOL in static libraries potentially causes a > > problem for the llvm linker [1]. So, I want to forcibly link lib-y > > objects to vmlinux when CONFIG_MODULES=y. > > > > As a groundwork, we must fix multiple definitions that have been > > hidden by lib-y. > > IIUC that causes known dead code to be included in the kernel image. > Wouldn't it be possible to actually omit replaced functions from output by > keying the build of the sources providing generic code with appropriate > CONFIG_* settings (such as CONFIG_GENERIC_DELAY, CONFIG_GENERIC_MEMCPY, > etc. or suchlike)? > > Maciej You are right. __weak cannot trim the dead code. I can work on the CONFIG_ approach, but I'd rather to use inverted CONFIG_HAVE_PLAT_* because it is easier to make CAVIUM_OCTEON_SOC select them.
diff --git a/arch/mips/cavium-octeon/csrc-octeon.c b/arch/mips/cavium-octeon/csrc-octeon.c index 124817609ce0..fdc28fb5eda4 100644 --- a/arch/mips/cavium-octeon/csrc-octeon.c +++ b/arch/mips/cavium-octeon/csrc-octeon.c @@ -153,7 +153,6 @@ void __udelay(unsigned long us) while (end > cur) cur = read_c0_cvmcount(); } -EXPORT_SYMBOL(__udelay); void __ndelay(unsigned long ns) { @@ -167,7 +166,6 @@ void __ndelay(unsigned long ns) while (end > cur) cur = read_c0_cvmcount(); } -EXPORT_SYMBOL(__ndelay); void __delay(unsigned long loops) { @@ -179,8 +177,6 @@ void __delay(unsigned long loops) while (end > cur) cur = read_c0_cvmcount(); } -EXPORT_SYMBOL(__delay); - /** * octeon_io_clk_delay - wait for a given number of io clock cycles to pass. diff --git a/arch/mips/cavium-octeon/octeon-memcpy.S b/arch/mips/cavium-octeon/octeon-memcpy.S index 0a7c9834b81c..3eb8d1a72d7f 100644 --- a/arch/mips/cavium-octeon/octeon-memcpy.S +++ b/arch/mips/cavium-octeon/octeon-memcpy.S @@ -147,11 +147,9 @@ */ .align 5 LEAF(memcpy) /* a0=dst a1=src a2=len */ -EXPORT_SYMBOL(memcpy) move v0, dst /* return value */ __memcpy: FEXPORT(__copy_user) -EXPORT_SYMBOL(__copy_user) /* * Note: dst & src may be unaligned, len may be 0 * Temps @@ -438,7 +436,6 @@ s_exc: .align 5 LEAF(memmove) -EXPORT_SYMBOL(memmove) ADD t0, a0, a2 ADD t1, a1, a2 sltu t0, a1, t0 # dst + len <= src -> memcpy diff --git a/arch/mips/fw/lib/cmdline.c b/arch/mips/fw/lib/cmdline.c index 6ecda64ad184..e1f9a0c23005 100644 --- a/arch/mips/fw/lib/cmdline.c +++ b/arch/mips/fw/lib/cmdline.c @@ -16,7 +16,7 @@ int fw_argc; int *_fw_argv; int *_fw_envp; -void __init fw_init_cmdline(void) +void __init __weak fw_init_cmdline(void) { int i; diff --git a/arch/mips/lib/delay.c b/arch/mips/lib/delay.c index 68c495ed71e3..ba0ae7da5ced 100644 --- a/arch/mips/lib/delay.c +++ b/arch/mips/lib/delay.c @@ -24,7 +24,7 @@ #define GCC_DADDI_IMM_ASM() "r" #endif -void __delay(unsigned long loops) +void __weak __delay(unsigned long loops) { __asm__ __volatile__ ( " .set noreorder \n" @@ -48,7 +48,7 @@ EXPORT_SYMBOL(__delay); * a constant) */ -void __udelay(unsigned long us) +void __weak __udelay(unsigned long us) { unsigned int lpj = raw_current_cpu_data.udelay_val; @@ -56,7 +56,7 @@ void __udelay(unsigned long us) } EXPORT_SYMBOL(__udelay); -void __ndelay(unsigned long ns) +void __weak __ndelay(unsigned long ns) { unsigned int lpj = raw_current_cpu_data.udelay_val; diff --git a/arch/mips/lib/memcpy.S b/arch/mips/lib/memcpy.S index f7994d936505..f2f58326b927 100644 --- a/arch/mips/lib/memcpy.S +++ b/arch/mips/lib/memcpy.S @@ -598,6 +598,9 @@ SEXC(1) nop .endm + .weak memmove + .weak __rmemcpy + .align 5 LEAF(memmove) EXPORT_SYMBOL(memmove) @@ -655,6 +658,8 @@ LEAF(__rmemcpy) /* a0=dst a1=src a2=len */ * the number of uncopied bytes. * memcpy sets v0 to dst. */ + .weak memcpy + .weak __copy_user .align 5 LEAF(memcpy) /* a0=dst a1=src a2=len */ EXPORT_SYMBOL(memcpy)
MIPS provides multiple definitions for the following functions: fw_init_cmdline __delay __udelay __ndelay memmove __rmemcpy memcpy __copy_user The generic ones are defined in lib-y objects, which are overridden by the Octeon ones when CONFIG_CAVIUM_OCTEON_SOC is enabled. The use of EXPORT_SYMBOL in static libraries potentially causes a problem for the llvm linker [1]. So, I want to forcibly link lib-y objects to vmlinux when CONFIG_MODULES=y. As a groundwork, we must fix multiple definitions that have been hidden by lib-y. In this case, the generic implementations in arch/mips/lib/ are weaker than the ones in arch/mips/cavium-octen/, so annotating __weak is a straight-forward solution. I also removed EXPORT_SYMBOL from the Octeon files to avoid the 'exported twice' warnings from modpost. [1]: https://github.com/ClangBuiltLinux/linux/issues/515 Reported-by: kbuild test robot <lkp@intel.com> Suggested-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> --- arch/mips/cavium-octeon/csrc-octeon.c | 4 ---- arch/mips/cavium-octeon/octeon-memcpy.S | 3 --- arch/mips/fw/lib/cmdline.c | 2 +- arch/mips/lib/delay.c | 6 +++--- arch/mips/lib/memcpy.S | 5 +++++ 5 files changed, 9 insertions(+), 11 deletions(-)