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@xxxxxxxxxx> 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@xxxxxxxxx>
> Suggested-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> ---
>
>  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@xxxxxxxxxx>

Thanks for the patch!


>         .align  5
>  LEAF(memcpy)                                   /* a0=dst a1=src a2=len */
>  EXPORT_SYMBOL(memcpy)
> --
> 2.17.1
>
> --
--
Thanks,
~Nick Desaulniers