| Message ID | 14534456.e6dv16VA3R@wuerfel (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 24 Oct 2016 17:05:26 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > This adds an asm/asm-prototypes.h header for ARM to fix the > broken symbol versioning for symbols exported from assembler > files. > > In addition to the header, we have to do these other small > changes: > > - move the exports from bitops.h to {change,clear,set,...}bit.S > - move the exports from csumpartialgeneric.S into the files > including it > > I couldn't find the correct prototypes for the compiler builtins, > so I went with the fake 'void f(void)' prototypes that we had > before. > > This leaves the mmioset/mmiocpy function for now, as it's not > obvious how to best handle them. This looks nicer. I like variant B because it keeps the GENKSYMS cruft to a single location, but either one isn't too bad. I'd like to get moving on this, so let's at least get the generic kbuild change merged. In the end, the kbuild code does not prevent a maintainer from putting their EXPORT_SYMBOL in whatever location they like, so there is no reason not to merge it (certainly there will be archs that do use it). Michal, what's your thoughts? If you merge my patch 2/2 and skip 1/2, it should not give any new build warnings or errors, so then arch patches can go via arch trees. 1/2 could go in after everyone is up to date. Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 25, 2016 at 07:32:00PM +1100, Nicholas Piggin wrote: > On Mon, 24 Oct 2016 17:05:26 +0200 > Arnd Bergmann <arnd@arndb.de> wrote: > > > This adds an asm/asm-prototypes.h header for ARM to fix the > > broken symbol versioning for symbols exported from assembler > > files. > > > > In addition to the header, we have to do these other small > > changes: > > > > - move the exports from bitops.h to {change,clear,set,...}bit.S > > - move the exports from csumpartialgeneric.S into the files > > including it > > > > I couldn't find the correct prototypes for the compiler builtins, > > so I went with the fake 'void f(void)' prototypes that we had > > before. > > > > This leaves the mmioset/mmiocpy function for now, as it's not > > obvious how to best handle them. > > > This looks nicer. I like variant B because it keeps the GENKSYMS cruft to > a single location, but either one isn't too bad. > > I'd like to get moving on this, so let's at least get the generic kbuild > change merged. In the end, the kbuild code does not prevent a maintainer > from putting their EXPORT_SYMBOL in whatever location they like, so there > is no reason not to merge it (certainly there will be archs that do use > it). > > Michal, what's your thoughts? If you merge my patch 2/2 and skip 1/2, it > should not give any new build warnings or errors, so then arch patches can > go via arch trees. 1/2 could go in after everyone is up to date. So what's the conclusion on this? I've just had a failure due to CONFIG_TRIM_UNUSED_KSYMS reported on ARM, and it looks like (at least some of) patch 1 could resolve it. Do we need to split patch 1? Has any of these patches been committed yet?
On Sun, Nov 20, 2016 at 5:21 AM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Tue, Oct 25, 2016 at 07:32:00PM +1100, Nicholas Piggin wrote: >> >> Michal, what's your thoughts? If you merge my patch 2/2 and skip 1/2, it >> should not give any new build warnings or errors, so then arch patches can >> go via arch trees. 1/2 could go in after everyone is up to date. > > So what's the conclusion on this? I've just had a failure due to > CONFIG_TRIM_UNUSED_KSYMS reported on ARM, and it looks like (at > least some of) patch 1 could resolve it. Hmm. I've got cc6acc11cad1 kbuild: be more careful about matching preprocessed asm ___EXPORT_SYMBOL 4efca4ed05cb kbuild: modversions for EXPORT_SYMBOL() for asm in my tree. Is that sufficient, or do we still have issues? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Nov 20, 2016 at 10:32:50AM -0800, Linus Torvalds wrote: > On Sun, Nov 20, 2016 at 5:21 AM, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: > > On Tue, Oct 25, 2016 at 07:32:00PM +1100, Nicholas Piggin wrote: > >> > >> Michal, what's your thoughts? If you merge my patch 2/2 and skip 1/2, it > >> should not give any new build warnings or errors, so then arch patches can > >> go via arch trees. 1/2 could go in after everyone is up to date. > > > > So what's the conclusion on this? I've just had a failure due to > > CONFIG_TRIM_UNUSED_KSYMS reported on ARM, and it looks like (at > > least some of) patch 1 could resolve it. > > Hmm. I've got > > cc6acc11cad1 kbuild: be more careful about matching preprocessed asm > ___EXPORT_SYMBOL > 4efca4ed05cb kbuild: modversions for EXPORT_SYMBOL() for asm > > in my tree. Is that sufficient, or do we still have issues? Hmm, those seem to have gone in during the last week, so I haven't tested it yet (build running, but it'll take a while). However, I don't think they'll solve _this_ problem. Some of the issue here is that we use a mixture of assembly macros and preprocessor for the ARM bitops - the ARM bitops are created with an assembly macro which contains some pre-processor expanded macros (eg, EXPORT_SYMBOL()). This means that the actual symbol being exported is not known to the preprocessor, so doing the "__is_defined(__KSYM_##sym)" inside "EXPORT_SYMBOL(\name)" becomes "__is_defined(__KSYM_\name)" to the preprocessor. As "__KSYM_\name" is never defined, it always comes out as zero, hence we always use __cond_export_sym_0, which omits the symbol export from the assembly macro definition: .macro bitop, name, instr .globl \name ; .align 0 ; \name: ... .type \name, %function; .size \name, .-\name .endm In other words, using preprocessor macros inside an assembly macro may not work as expected, and now leads to config-specific failures.
On Sun, 20 Nov 2016 19:12:57 +0000 Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Sun, Nov 20, 2016 at 10:32:50AM -0800, Linus Torvalds wrote: > > On Sun, Nov 20, 2016 at 5:21 AM, Russell King - ARM Linux > > <linux@armlinux.org.uk> wrote: > > > On Tue, Oct 25, 2016 at 07:32:00PM +1100, Nicholas Piggin wrote: > > >> > > >> Michal, what's your thoughts? If you merge my patch 2/2 and skip 1/2, it > > >> should not give any new build warnings or errors, so then arch patches can > > >> go via arch trees. 1/2 could go in after everyone is up to date. > > > > > > So what's the conclusion on this? I've just had a failure due to > > > CONFIG_TRIM_UNUSED_KSYMS reported on ARM, and it looks like (at > > > least some of) patch 1 could resolve it. > > > > Hmm. I've got > > > > cc6acc11cad1 kbuild: be more careful about matching preprocessed asm > > ___EXPORT_SYMBOL > > 4efca4ed05cb kbuild: modversions for EXPORT_SYMBOL() for asm > > > > in my tree. Is that sufficient, or do we still have issues? > > Hmm, those seem to have gone in during the last week, so I haven't > tested it yet (build running, but it'll take a while). However, I > don't think they'll solve _this_ problem. > > Some of the issue here is that we use a mixture of assembly macros > and preprocessor for the ARM bitops - the ARM bitops are created > with an assembly macro which contains some pre-processor expanded > macros (eg, EXPORT_SYMBOL()). > > This means that the actual symbol being exported is not known to > the preprocessor, so doing the "__is_defined(__KSYM_##sym)" inside > "EXPORT_SYMBOL(\name)" becomes "__is_defined(__KSYM_\name)" to the > preprocessor. As "__KSYM_\name" is never defined, it always comes > out as zero, hence we always use __cond_export_sym_0, which omits > the symbol export from the assembly macro definition: > > .macro bitop, name, instr > .globl \name ; .align 0 ; \name: > > ... > > .type \name, %function; .size \name, .-\name > > .endm > > In other words, using preprocessor macros inside an assembly macro > may not work as expected, and now leads to config-specific failures. > Yes, that's a limitation. cpp expansion we can handle, but not gas macros. You will need Arnd's patches for ARM. http://marc.info/?l=linux-kbuild&m=147732160529499&w=2 If that doesn't fix it for you, send me your .config offline and I'll set up a cross compile to work on it. Again, any arch always has the option of going back to doing asm exports in the old style of putting them into a .c file, but hopefully you'll find Arnd's reworked patches to be something you're willing to merge. Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Mon, Oct 24, 2016 at 05:05:26PM +0200, Arnd Bergmann wrote: > This adds an asm/asm-prototypes.h header for ARM to fix the > broken symbol versioning for symbols exported from assembler > files. > > In addition to the header, we have to do these other small > changes: > > - move the exports from bitops.h to {change,clear,set,...}bit.S > - move the exports from csumpartialgeneric.S into the files > including it > > I couldn't find the correct prototypes for the compiler builtins, > so I went with the fake 'void f(void)' prototypes that we had > before. > > This leaves the mmioset/mmiocpy function for now, as it's not > obvious how to best handle them. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> In my test builds of 4.9-rc5 plus 4efca4ed05cb ("kbuild: modversions for EXPORT_SYMBOL() for asm") cc6acc11cad1 ("kbuild: be more careful about matching preprocessed asm ___EXPORT_SYMBOL") (which are in -rc6) I got many warnings à la: WARNING: "memset" [drivers/media/usb/airspy/airspy.ko] has no CRC! and booting the resulting kernel failed with messages of the type: [ 3.024126] usbcore: no symbol version for __memzero [ 3.029107] usbcore: Unknown symbol __memzero (err -22) so hardly any module could be loaded. modprobe -f works however, but that's not what my initramfs does. With this patch and https://patchwork.kernel.org/patch/9392291/ ("ARM: move mmiocpy/mmioset exports to io.c") I could compile a kernel without CRC warnings and it boots fine. So it would be great to get these two patches into 4.9. Thanks Uwe
On Mon, Nov 21, 2016 at 07:46:44PM +0100, Uwe Kleine-König wrote: > Hello, > > On Mon, Oct 24, 2016 at 05:05:26PM +0200, Arnd Bergmann wrote: > > This adds an asm/asm-prototypes.h header for ARM to fix the > > broken symbol versioning for symbols exported from assembler > > files. > > > > In addition to the header, we have to do these other small > > changes: > > > > - move the exports from bitops.h to {change,clear,set,...}bit.S > > - move the exports from csumpartialgeneric.S into the files > > including it > > > > I couldn't find the correct prototypes for the compiler builtins, > > so I went with the fake 'void f(void)' prototypes that we had > > before. > > > > This leaves the mmioset/mmiocpy function for now, as it's not > > obvious how to best handle them. > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > In my test builds of 4.9-rc5 plus > > 4efca4ed05cb ("kbuild: modversions for EXPORT_SYMBOL() for asm") > cc6acc11cad1 ("kbuild: be more careful about matching preprocessed asm ___EXPORT_SYMBOL") > > (which are in -rc6) I got many warnings à la: > > WARNING: "memset" [drivers/media/usb/airspy/airspy.ko] has no CRC! > > and booting the resulting kernel failed with messages of the type: > > [ 3.024126] usbcore: no symbol version for __memzero > [ 3.029107] usbcore: Unknown symbol __memzero (err -22) > > so hardly any module could be loaded. modprobe -f works however, but > that's not what my initramfs does. > > With this patch and https://patchwork.kernel.org/patch/9392291/ ("ARM: > move mmiocpy/mmioset exports to io.c") I could compile a kernel without > CRC warnings and it boots fine. So it would be great to get these two > patches into 4.9. Yea, many things would be nice, but I've been unable to track the issues here - it really didn't help _not_ being copied on the original set of patches which introduced this mess. I've merged Nicolas' patch, so now we need to work out what to do with the remaining bits - which I guess are the asm-prototypes.h and the mmio* bits. I'm not aware of what's happening with the patches that they depend on (which is why I recently asked the question - again, I seem to be completely out of the loop due to lack of Cc's). So I'm just throwing my hands up and saying "I don't know what to do" at this stage.
On Mon, 21 Nov 2016 19:13:55 +0000 Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Mon, Nov 21, 2016 at 07:46:44PM +0100, Uwe Kleine-König wrote: > > Hello, > > > > On Mon, Oct 24, 2016 at 05:05:26PM +0200, Arnd Bergmann wrote: > > > This adds an asm/asm-prototypes.h header for ARM to fix the > > > broken symbol versioning for symbols exported from assembler > > > files. > > > > > > In addition to the header, we have to do these other small > > > changes: > > > > > > - move the exports from bitops.h to {change,clear,set,...}bit.S > > > - move the exports from csumpartialgeneric.S into the files > > > including it > > > > > > I couldn't find the correct prototypes for the compiler builtins, > > > so I went with the fake 'void f(void)' prototypes that we had > > > before. > > > > > > This leaves the mmioset/mmiocpy function for now, as it's not > > > obvious how to best handle them. > > > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > > In my test builds of 4.9-rc5 plus > > > > 4efca4ed05cb ("kbuild: modversions for EXPORT_SYMBOL() for asm") > > cc6acc11cad1 ("kbuild: be more careful about matching preprocessed asm ___EXPORT_SYMBOL") > > > > (which are in -rc6) I got many warnings à la: > > > > WARNING: "memset" [drivers/media/usb/airspy/airspy.ko] has no CRC! > > > > and booting the resulting kernel failed with messages of the type: > > > > [ 3.024126] usbcore: no symbol version for __memzero > > [ 3.029107] usbcore: Unknown symbol __memzero (err -22) > > > > so hardly any module could be loaded. modprobe -f works however, but > > that's not what my initramfs does. > > > > With this patch and https://patchwork.kernel.org/patch/9392291/ ("ARM: > > move mmiocpy/mmioset exports to io.c") I could compile a kernel without > > CRC warnings and it boots fine. So it would be great to get these two > > patches into 4.9. > > Yea, many things would be nice, but I've been unable to track the > issues here - it really didn't help _not_ being copied on the > original set of patches which introduced this mess. Quick overview: - asm exports changes allow EXPORT_SYMBOL to be moved into .S files, but they would not get modversion CRCs generated. - The core kbuild patches to add modversions support for asm exports is now merged in Linus's tree from the recent kbuild tree pull. asm/asm-prototypes.h must contain C style declarations of the symbol for this to work. - Architectures can now add their asm/asm-prototypes.h and things *should* start working. - Dependency is not a hard one. If you add asm-prototypes.h before merging the core patches then it should not introduce any problems. > I've merged Nicolas' patch, so now we need to work out what to do > with the remaining bits - which I guess are the asm-prototypes.h > and the mmio* bits. I'm not aware of what's happening with the > patches that they depend on (which is why I recently asked the > question - again, I seem to be completely out of the loop due to > lack of Cc's). > > So I'm just throwing my hands up and saying "I don't know what to > do" at this stage. > I don't think you have missed much since last it came up, it's just taken a bit of time to get the details right and get it merged. Not sure what your tree looks like, but if you merge this patch 1/2 plus 2a/2 or 2b/2 into upstream, then ARM should be working. Thanks, Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/include/asm/asm-prototypes.h b/arch/arm/include/asm/asm-prototypes.h new file mode 100644 index 000000000000..04e5616a7b15 --- /dev/null +++ b/arch/arm/include/asm/asm-prototypes.h @@ -0,0 +1,34 @@ +#include <linux/arm-smccc.h> +#include <linux/bitops.h> +#include <linux/ftrace.h> +#include <linux/io.h> +#include <linux/platform_data/asoc-imx-ssi.h> +#include <linux/string.h> +#include <linux/uaccess.h> + +#include <asm/checksum.h> +#include <asm/div64.h> +#include <asm/memory.h> + +extern void __aeabi_idivmod(void); +extern void __aeabi_idiv(void); +extern void __aeabi_lasr(void); +extern void __aeabi_llsl(void); +extern void __aeabi_llsr(void); +extern void __aeabi_lmul(void); +extern void __aeabi_uidivmod(void); +extern void __aeabi_uidiv(void); +extern void __aeabi_ulcmp(void); + +extern void __ashldi3(void); +extern void __ashrdi3(void); +extern void __bswapdi2(void); +extern void __bswapsi2(void); +extern void __divsi3(void); +extern void __do_div64(void); +extern void __lshrdi3(void); +extern void __modsi3(void); +extern void __muldi3(void); +extern void __ucmpdi2(void); +extern void __udivsi3(void); +extern void __umodsi3(void); diff --git a/arch/arm/lib/bitops.h b/arch/arm/lib/bitops.h index df06638b327c..afaef2a7faec 100644 --- a/arch/arm/lib/bitops.h +++ b/arch/arm/lib/bitops.h @@ -26,7 +26,6 @@ UNWIND( .fnstart ) bx lr UNWIND( .fnend ) ENDPROC(\name ) -EXPORT_SYMBOL(\name ) .endm .macro testop, name, instr, store @@ -57,7 +56,6 @@ UNWIND( .fnstart ) 2: bx lr UNWIND( .fnend ) ENDPROC(\name ) -EXPORT_SYMBOL(\name ) .endm #else .macro bitop, name, instr @@ -77,7 +75,6 @@ UNWIND( .fnstart ) ret lr UNWIND( .fnend ) ENDPROC(\name ) -EXPORT_SYMBOL(\name ) .endm /** @@ -106,6 +103,5 @@ UNWIND( .fnstart ) ret lr UNWIND( .fnend ) ENDPROC(\name ) -EXPORT_SYMBOL(\name ) .endm #endif diff --git a/arch/arm/lib/changebit.S b/arch/arm/lib/changebit.S index f4027862172f..005fdd18c509 100644 --- a/arch/arm/lib/changebit.S +++ b/arch/arm/lib/changebit.S @@ -13,3 +13,4 @@ .text bitop _change_bit, eor +EXPORT_SYMBOL(_change_bit) diff --git a/arch/arm/lib/clearbit.S b/arch/arm/lib/clearbit.S index f6b75fb64d30..501eff09968d 100644 --- a/arch/arm/lib/clearbit.S +++ b/arch/arm/lib/clearbit.S @@ -13,3 +13,4 @@ .text bitop _clear_bit, bic +EXPORT_SYMBOL(_clear_bit) diff --git a/arch/arm/lib/csumpartialcopy.S b/arch/arm/lib/csumpartialcopy.S index 9c3383fed129..bdcc2eea4e5c 100644 --- a/arch/arm/lib/csumpartialcopy.S +++ b/arch/arm/lib/csumpartialcopy.S @@ -49,6 +49,7 @@ #define FN_ENTRY ENTRY(csum_partial_copy_nocheck) #define FN_EXIT ENDPROC(csum_partial_copy_nocheck) -#define FN_EXPORT EXPORT_SYMBOL(csum_partial_copy_nocheck) #include "csumpartialcopygeneric.S" + +EXPORT_SYMBOL(csum_partial_copy_nocheck) diff --git a/arch/arm/lib/csumpartialcopygeneric.S b/arch/arm/lib/csumpartialcopygeneric.S index 8b94d20e51d1..06825566c0f7 100644 --- a/arch/arm/lib/csumpartialcopygeneric.S +++ b/arch/arm/lib/csumpartialcopygeneric.S @@ -332,4 +332,3 @@ FN_ENTRY mov r5, r4, get_byte_1 b .Lexit FN_EXIT -FN_EXPORT diff --git a/arch/arm/lib/csumpartialcopyuser.S b/arch/arm/lib/csumpartialcopyuser.S index 5d495edf3d83..d5522c94f58c 100644 --- a/arch/arm/lib/csumpartialcopyuser.S +++ b/arch/arm/lib/csumpartialcopyuser.S @@ -73,9 +73,9 @@ #define FN_ENTRY ENTRY(csum_partial_copy_from_user) #define FN_EXIT ENDPROC(csum_partial_copy_from_user) -#define FN_EXPORT EXPORT_SYMBOL(csum_partial_copy_from_user) #include "csumpartialcopygeneric.S" +EXPORT_SYMBOL(csum_partial_copy_from_user) /* * FIXME: minor buglet here diff --git a/arch/arm/lib/setbit.S b/arch/arm/lib/setbit.S index 618fedae4b37..d748b8d1326f 100644 --- a/arch/arm/lib/setbit.S +++ b/arch/arm/lib/setbit.S @@ -13,3 +13,4 @@ .text bitop _set_bit, orr +EXPORT_SYMBOL(_set_bit) diff --git a/arch/arm/lib/testchangebit.S b/arch/arm/lib/testchangebit.S index 4becdc3a59cb..4d2dafa9b787 100644 --- a/arch/arm/lib/testchangebit.S +++ b/arch/arm/lib/testchangebit.S @@ -13,3 +13,4 @@ .text testop _test_and_change_bit, eor, str +EXPORT_SYMBOL(_test_and_change_bit) diff --git a/arch/arm/lib/testclearbit.S b/arch/arm/lib/testclearbit.S index 918841dcce7a..fe5cae2e480a 100644 --- a/arch/arm/lib/testclearbit.S +++ b/arch/arm/lib/testclearbit.S @@ -13,3 +13,4 @@ .text testop _test_and_clear_bit, bicne, strne +EXPORT_SYMBOL(_test_and_clear_bit) diff --git a/arch/arm/lib/testsetbit.S b/arch/arm/lib/testsetbit.S index 8d1b2fe9e487..25fed837edb3 100644 --- a/arch/arm/lib/testsetbit.S +++ b/arch/arm/lib/testsetbit.S @@ -13,3 +13,4 @@ .text testop _test_and_set_bit, orreq, streq +EXPORT_SYMBOL(_test_and_set_bit)
This adds an asm/asm-prototypes.h header for ARM to fix the broken symbol versioning for symbols exported from assembler files. In addition to the header, we have to do these other small changes: - move the exports from bitops.h to {change,clear,set,...}bit.S - move the exports from csumpartialgeneric.S into the files including it I couldn't find the correct prototypes for the compiler builtins, so I went with the fake 'void f(void)' prototypes that we had before. This leaves the mmioset/mmiocpy function for now, as it's not obvious how to best handle them. Signed-off-by: Arnd Bergmann <arnd@arndb.de> -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html