Skip to contentSkip to conversation

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

PATCH In review Masahiro Yamada 3 participants 4 messages Mar 24, 2020 – Mar 25, 2020 MIPS

lore
Share & tools
lore

Masahiro Yamada

author
#0 lore raw

PATCH 1/1 MIPS: mark some functions as weak to avoid conflict with Octeon ones

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 <[email protected]>
Suggested-by Nick Desaulniers <[email protected]>
Signed-off-by Masahiro Yamada <[email protected]>

---

 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(-)
arch/mips/cavium-octeon/csrc-octeon.c +0 -4
index 124817609ce0..fdc28fb5eda4 100644
@@ -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.
arch/mips/cavium-octeon/octeon-memcpy.S +0 -3
index 0a7c9834b81c..3eb8d1a72d7f 100644
@@ -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
arch/mips/fw/lib/cmdline.c +1 -1
index 6ecda64ad184..e1f9a0c23005 100644
@@ -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;
arch/mips/lib/delay.c +3 -3
index 68c495ed71e3..ba0ae7da5ced 100644
@@ -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;
arch/mips/lib/memcpy.S +5 -0
index f7994d936505..f2f58326b927 100644
@@ -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)
Signature · 1 line
2.17.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 155 lines · 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 <[email protected]>
Suggested-by Nick Desaulniers <[email protected]>
Signed-off-by Masahiro Yamada <[email protected]>

---

 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(-)
arch/mips/cavium-octeon/csrc-octeon.c +0 -4
index 124817609ce0..fdc28fb5eda4 100644
@@ -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.
arch/mips/cavium-octeon/octeon-memcpy.S +0 -3
index 0a7c9834b81c..3eb8d1a72d7f 100644
@@ -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
arch/mips/fw/lib/cmdline.c +1 -1
index 6ecda64ad184..e1f9a0c23005 100644
@@ -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;
arch/mips/lib/delay.c +3 -3
index 68c495ed71e3..ba0ae7da5ced 100644
@@ -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;
arch/mips/lib/memcpy.S +3 -0
index f7994d936505..f2f58326b927 100644
@@ -598,6 +598,9 @@ SEXC(1)
         nop
        .endm
 
+       .weak memmove
+       .weak __rmemcpy
+
        .align  5
 LEAF(memmove)
 EXPORT_SYMBOL(memmove)

arch/mips/lib/memcpy.S LEAF(__rmemcpy) /* a0=dst a1=src a2=len */ · line 662

Quoted 5 lines
arch/mips/lib/memcpy.S +2 -0
@@ -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 <[email protected]>

Thanks for the patch!

Quoted 7 lines
arch/mips/lib/memcpy.S +0 -1
        .align  5
 LEAF(memcpy)                                   /* a0=dst a1=src a2=len */
 EXPORT_SYMBOL(memcpy)
--

2.17.1

--

--
Thanks,
~Nick Desaulniers

#2 lore raw
In reply to Masahiro Yamada
Quoted 14 lines · 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.

Quoted 6 lines

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

author
#3 lore raw

Hi Maciej,

Quoted 24 lines · Maciej W. Rozycki
Quoted 14 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Signature · 2 lines
Best Regards
Masahiro Yamada