[PATCH] MIPS: mark some functions as weak to avoid conflict with Octeon ones

[PATCH] MIPS: mark some functions as weak to avoid conflict with Octeon ones

[Masahiro Yamada]

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
 	.align	5
 LEAF(memcpy)					/* a0=dst a1=src a2=len */
 EXPORT_SYMBOL(memcpy)
-- 
2.17.1

Re: [PATCH] MIPS: mark some functions as weak to avoid conflict with Octeon ones

[Nick Desaulniers]

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

Re: [PATCH] MIPS: mark some functions as weak to avoid conflict with Octeon ones

[Maciej W. Rozycki]

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

Re: [PATCH] MIPS: mark some functions as weak to avoid conflict with Octeon ones

[Masahiro Yamada]

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.



-- 
Best Regards
Masahiro Yamada