| Message ID | 20161122110557.1533467-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
On Tue, 22 Nov 2016, 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. > > 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, restoring the state before they were moved. > > Originally I assumed that the problem was just a harmless warning > in unusual configurations, but as Uwe found, we actually need this > to load most modules when symbol versioning is enabled, as it is > in many distro kernels. > > Cc: Uwe Kleine-König <uwe@kleine-koenig.org> > Fixes: 4dd1837d7589 ("arm: move exports to definitions") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > Compared to the earlier version, I dropped the changes to the > csumpartial files, which now get handled correctly by Kbuild > even when the export comes from a macro, and I also dropped the > changes to the bitops files, which were already fixed in a > patch from Nico. > > The patch applies cleanly on top of the rmk/fixes tree but has > no effect there, as it also needs 4efca4ed05cb ("kbuild: modversions > for EXPORT_SYMBOL() for asm") and cc6acc11cad1 ("kbuild: be more > careful about matching preprocessed asm ___EXPORT_SYMBOL"). > > With the combination of rmk/fixes, torvalds/master and these two > patches, symbol versioning works again on ARM. As it is still > broken on almost all other architectures (powerpc is fixed, > x86 has a patch), I wonder if we should make CONFIG_MODVERSIONS > as broken for everything else. I'm not sure I like this at all. The goal for moving EXPORT_SYMBOL() to assembly code where symbols were defined is to make things close together and avoid those centralized list of symbols that you can easily miss when modifying the actual code. This series is therefore bringing back a centralized list of symbols in a slightly different form, nullifying the advantages from having moved EXPORT_SYMBOL() to asm code. To me this looks like a big step backward. Why not simply extending the original idea of keeping exports close to the actual code by _also_ having a macro that provides the function prototype alongside the EXPORT_SYMBOL() instance? That could even be expressed with some EXPORT_SYMBOL_PROTO(ret, sym, arg...) macro that does it all. Nicolas
On Tue, Nov 22, 2016 at 11:34:48AM -0500, Nicolas Pitre wrote: > On Tue, 22 Nov 2016, 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. > > > > 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, restoring the state before they were moved. > > > > Originally I assumed that the problem was just a harmless warning > > in unusual configurations, but as Uwe found, we actually need this > > to load most modules when symbol versioning is enabled, as it is > > in many distro kernels. > > > > Cc: Uwe Kleine-König <uwe@kleine-koenig.org> > > Fixes: 4dd1837d7589 ("arm: move exports to definitions") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > Compared to the earlier version, I dropped the changes to the > > csumpartial files, which now get handled correctly by Kbuild > > even when the export comes from a macro, and I also dropped the > > changes to the bitops files, which were already fixed in a > > patch from Nico. > > > > The patch applies cleanly on top of the rmk/fixes tree but has > > no effect there, as it also needs 4efca4ed05cb ("kbuild: modversions > > for EXPORT_SYMBOL() for asm") and cc6acc11cad1 ("kbuild: be more > > careful about matching preprocessed asm ___EXPORT_SYMBOL"). > > > > With the combination of rmk/fixes, torvalds/master and these two > > patches, symbol versioning works again on ARM. As it is still > > broken on almost all other architectures (powerpc is fixed, > > x86 has a patch), I wonder if we should make CONFIG_MODVERSIONS > > as broken for everything else. > > I'm not sure I like this at all. > > The goal for moving EXPORT_SYMBOL() to assembly code where symbols were > defined is to make things close together and avoid those centralized > list of symbols that you can easily miss when modifying the actual code. > > This series is therefore bringing back a centralized list of symbols in > a slightly different form, nullifying the advantages from having moved > EXPORT_SYMBOL() to asm code. To me this looks like a big step backward. > > Why not simply extending the original idea of keeping exports close to > the actual code by _also_ having a macro that provides the function > prototype alongside the EXPORT_SYMBOL() instance? That could even be > expressed with some EXPORT_SYMBOL_PROTO(ret, sym, arg...) macro that > does it all. What you're saying is that you don't like the solution that's taken weeks to get merged up to this point, so where do we go from here now? This crap has been broken since 4.9-rc1, and is a regression. I think at this point, we just declare that modversions are broken on ARM, and those who created this mess get to explain to people why the fsck they broke the kernel. 4.9 is the next LTS kernel? ROTFL! I agree with Nicolas - it seems that the whole EXPORT_SYMBOL() crap has just been a pointless exercise in churn, resulting only in something "different" because it looks "cool" to do it some other way. There's no real benefit here at all, only harm. Just revert the damned patches that created this breakage in the first place please. It's now way too late to be trying to fix it any other way.
On Tue, 22 Nov 2016 11:34:48 -0500 (EST) Nicolas Pitre <nicolas.pitre@linaro.org> wrote: > On Tue, 22 Nov 2016, 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. > > > > 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, restoring the state before they were moved. > > > > Originally I assumed that the problem was just a harmless warning > > in unusual configurations, but as Uwe found, we actually need this > > to load most modules when symbol versioning is enabled, as it is > > in many distro kernels. > > > > Cc: Uwe Kleine-König <uwe@kleine-koenig.org> > > Fixes: 4dd1837d7589 ("arm: move exports to definitions") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > Compared to the earlier version, I dropped the changes to the > > csumpartial files, which now get handled correctly by Kbuild > > even when the export comes from a macro, and I also dropped the > > changes to the bitops files, which were already fixed in a > > patch from Nico. > > > > The patch applies cleanly on top of the rmk/fixes tree but has > > no effect there, as it also needs 4efca4ed05cb ("kbuild: modversions > > for EXPORT_SYMBOL() for asm") and cc6acc11cad1 ("kbuild: be more > > careful about matching preprocessed asm ___EXPORT_SYMBOL"). > > > > With the combination of rmk/fixes, torvalds/master and these two > > patches, symbol versioning works again on ARM. As it is still > > broken on almost all other architectures (powerpc is fixed, > > x86 has a patch), I wonder if we should make CONFIG_MODVERSIONS > > as broken for everything else. > > I'm not sure I like this at all. > > The goal for moving EXPORT_SYMBOL() to assembly code where symbols were > defined is to make things close together and avoid those centralized > list of symbols that you can easily miss when modifying the actual code. Right. > > This series is therefore bringing back a centralized list of symbols in > a slightly different form, nullifying the advantages from having moved > EXPORT_SYMBOL() to asm code. To me this looks like a big step backward. Exported symbols have C declarations in headers already. For the most part, anyway -- these ones Arnd adds are for compiler runtime which is why some architectures haven't had the prototypes. > Why not simply extending the original idea of keeping exports close to > the actual code by _also_ having a macro that provides the function > prototype alongside the EXPORT_SYMBOL() instance? That could even be > expressed with some EXPORT_SYMBOL_PROTO(ret, sym, arg...) macro that > does it all. Well, the reason is to get 4.9 working, I never thought asm-prototypes.h was a beautiful solution or it should not be changed if we can find ways to improve it. EXPORT_SYMBOL_PROTO() for asm code seems possibly a good idea for 4.10. Of course your exported symbols will still have their prototypes in various headers -- that redundancy is inherent. Thanks, Nick
On Wed, 23 Nov 2016, Nicholas Piggin wrote: > On Tue, 22 Nov 2016 11:34:48 -0500 (EST) > Nicolas Pitre <nicolas.pitre@linaro.org> wrote: > > > On Tue, 22 Nov 2016, 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. > > > > > > 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, restoring the state before they were moved. > > > > > > Originally I assumed that the problem was just a harmless warning > > > in unusual configurations, but as Uwe found, we actually need this > > > to load most modules when symbol versioning is enabled, as it is > > > in many distro kernels. > > > > > > Cc: Uwe Kleine-König <uwe@kleine-koenig.org> > > > Fixes: 4dd1837d7589 ("arm: move exports to definitions") > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > --- > > > Compared to the earlier version, I dropped the changes to the > > > csumpartial files, which now get handled correctly by Kbuild > > > even when the export comes from a macro, and I also dropped the > > > changes to the bitops files, which were already fixed in a > > > patch from Nico. > > > > > > The patch applies cleanly on top of the rmk/fixes tree but has > > > no effect there, as it also needs 4efca4ed05cb ("kbuild: modversions > > > for EXPORT_SYMBOL() for asm") and cc6acc11cad1 ("kbuild: be more > > > careful about matching preprocessed asm ___EXPORT_SYMBOL"). > > > > > > With the combination of rmk/fixes, torvalds/master and these two > > > patches, symbol versioning works again on ARM. As it is still > > > broken on almost all other architectures (powerpc is fixed, > > > x86 has a patch), I wonder if we should make CONFIG_MODVERSIONS > > > as broken for everything else. > > > > I'm not sure I like this at all. > > > > The goal for moving EXPORT_SYMBOL() to assembly code where symbols were > > defined is to make things close together and avoid those centralized > > list of symbols that you can easily miss when modifying the actual code. > > Right. > > > > > This series is therefore bringing back a centralized list of symbols in > > a slightly different form, nullifying the advantages from having moved > > EXPORT_SYMBOL() to asm code. To me this looks like a big step backward. > > Exported symbols have C declarations in headers already. For the most > part, anyway -- these ones Arnd adds are for compiler runtime which is > why some architectures haven't had the prototypes. Hmmm. That's right. That makes it much more justifiable. My main objection is withdrawn. However there is a bunch of includes added to asm-prototypes.h: +#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> Are those necessary at all? Nicolas
On Wed, 23 Nov 2016 00:41:07 +0000 Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Tue, Nov 22, 2016 at 11:34:48AM -0500, Nicolas Pitre wrote: > > On Tue, 22 Nov 2016, 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. > > > > > > 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, restoring the state before they were moved. > > > > > > Originally I assumed that the problem was just a harmless warning > > > in unusual configurations, but as Uwe found, we actually need this > > > to load most modules when symbol versioning is enabled, as it is > > > in many distro kernels. > > > > > > Cc: Uwe Kleine-König <uwe@kleine-koenig.org> > > > Fixes: 4dd1837d7589 ("arm: move exports to definitions") > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > --- > > > Compared to the earlier version, I dropped the changes to the > > > csumpartial files, which now get handled correctly by Kbuild > > > even when the export comes from a macro, and I also dropped the > > > changes to the bitops files, which were already fixed in a > > > patch from Nico. > > > > > > The patch applies cleanly on top of the rmk/fixes tree but has > > > no effect there, as it also needs 4efca4ed05cb ("kbuild: modversions > > > for EXPORT_SYMBOL() for asm") and cc6acc11cad1 ("kbuild: be more > > > careful about matching preprocessed asm ___EXPORT_SYMBOL"). > > > > > > With the combination of rmk/fixes, torvalds/master and these two > > > patches, symbol versioning works again on ARM. As it is still > > > broken on almost all other architectures (powerpc is fixed, > > > x86 has a patch), I wonder if we should make CONFIG_MODVERSIONS > > > as broken for everything else. > > > > I'm not sure I like this at all. > > > > The goal for moving EXPORT_SYMBOL() to assembly code where symbols were > > defined is to make things close together and avoid those centralized > > list of symbols that you can easily miss when modifying the actual code. > > > > This series is therefore bringing back a centralized list of symbols in > > a slightly different form, nullifying the advantages from having moved > > EXPORT_SYMBOL() to asm code. To me this looks like a big step backward. > > > > Why not simply extending the original idea of keeping exports close to > > the actual code by _also_ having a macro that provides the function > > prototype alongside the EXPORT_SYMBOL() instance? That could even be > > expressed with some EXPORT_SYMBOL_PROTO(ret, sym, arg...) macro that > > does it all. > > What you're saying is that you don't like the solution that's taken > weeks to get merged up to this point, so where do we go from here > now? This crap has been broken since 4.9-rc1, and is a regression. > > I think at this point, we just declare that modversions are broken > on ARM, and those who created this mess get to explain to people > why the fsck they broke the kernel. Let's fix it instead :) Why not merge Arnd's 2 patches? I think he mostly addressed your concerns of them. Or... > > 4.9 is the next LTS kernel? ROTFL! > > I agree with Nicolas - it seems that the whole EXPORT_SYMBOL() crap > has just been a pointless exercise in churn, resulting only in > something "different" because it looks "cool" to do it some other > way. There's no real benefit here at all, only harm. > > Just revert the damned patches that created this breakage in the > first place please. It's now way too late to be trying to fix it > any other way. > 4dd1837d7589 diffstat is entirely in arch/arm. I think reverting that would fix it (I haven't tested it myself so I would advise testing before committing). So the ball is in your court. As for process concerns, you have made valid points. Sometimes a mistake is made or we make an incorrect assumption about how another person works or what mailing lists they have read, or do not anticipate the fallout from some change. Everyone does it sometimes, so it's never a bad time to reflect on how we work with others and try to do better. Thanks, Nick
On Tue, Nov 22, 2016 at 08:35:48PM -0500, Nicolas Pitre wrote: > On Wed, 23 Nov 2016, Nicholas Piggin wrote: > > > On Tue, 22 Nov 2016 11:34:48 -0500 (EST) > > Nicolas Pitre <nicolas.pitre@linaro.org> wrote: > > > > > On Tue, 22 Nov 2016, 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. > > > > > > > > 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, restoring the state before they were moved. > > > > > > > > Originally I assumed that the problem was just a harmless warning > > > > in unusual configurations, but as Uwe found, we actually need this > > > > to load most modules when symbol versioning is enabled, as it is > > > > in many distro kernels. > > > > > > > > Cc: Uwe Kleine-König <uwe@kleine-koenig.org> > > > > Fixes: 4dd1837d7589 ("arm: move exports to definitions") > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > > --- > > > > Compared to the earlier version, I dropped the changes to the > > > > csumpartial files, which now get handled correctly by Kbuild > > > > even when the export comes from a macro, and I also dropped the > > > > changes to the bitops files, which were already fixed in a > > > > patch from Nico. > > > > > > > > The patch applies cleanly on top of the rmk/fixes tree but has > > > > no effect there, as it also needs 4efca4ed05cb ("kbuild: modversions > > > > for EXPORT_SYMBOL() for asm") and cc6acc11cad1 ("kbuild: be more > > > > careful about matching preprocessed asm ___EXPORT_SYMBOL"). > > > > > > > > With the combination of rmk/fixes, torvalds/master and these two > > > > patches, symbol versioning works again on ARM. As it is still > > > > broken on almost all other architectures (powerpc is fixed, > > > > x86 has a patch), I wonder if we should make CONFIG_MODVERSIONS > > > > as broken for everything else. > > > > > > I'm not sure I like this at all. > > > > > > The goal for moving EXPORT_SYMBOL() to assembly code where symbols were > > > defined is to make things close together and avoid those centralized > > > list of symbols that you can easily miss when modifying the actual code. > > > > Right. > > > > > > > > This series is therefore bringing back a centralized list of symbols in > > > a slightly different form, nullifying the advantages from having moved > > > EXPORT_SYMBOL() to asm code. To me this looks like a big step backward. > > > > Exported symbols have C declarations in headers already. For the most > > part, anyway -- these ones Arnd adds are for compiler runtime which is > > why some architectures haven't had the prototypes. > > Hmmm. That's right. That makes it much more justifiable. > My main objection is withdrawn. I don't see it makes any difference - the armksyms.c originally had the same: -#include <linux/export.h> -#include <linux/sched.h> -#include <linux/string.h> -#include <linux/cryptohash.h> -#include <linux/delay.h> -#include <linux/in6.h> -#include <linux/syscalls.h> -#include <linux/uaccess.h> -#include <linux/io.h> -#include <linux/arm-smccc.h> - -#include <asm/checksum.h> -#include <asm/ftrace.h> followed by prototypes for the GCC internal functions, and: -extern void fpundefinstr(void); - -void mmioset(void *, unsigned int, size_t); -void mmiocpy(void *, const void *, size_t); So, the asm-prototypes.h approach is just the same, only that we now have a bunch of prototypes in a header file, and the EXPORT_SYMBOL()s in the assembly files. As the C prototypes are remote from the definitions, it means that the C prototypes are going to get forgotten about in exactly the same way that armksyms.c would've been forgotten about too. It _is_ worse than that though - with the armksyms.c approach, if the assembly code for it is removed, you get a build error reminding you to remove the export (and prototype). With this approach, you get no reminder to touch asm-prototypes.h. It's also error prone for another reason - adding a new assembly level export, if you forget to add it to asm-prototypes.h, we're back into the problem we have right now with MODVERSIONS breaking. So, I still think the whole approach is wrong - it's added extra fragility that wasn't there with the armksyms.c approach.
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);
This adds an asm/asm-prototypes.h header for ARM to fix the broken symbol versioning for symbols exported from assembler files. 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, restoring the state before they were moved. Originally I assumed that the problem was just a harmless warning in unusual configurations, but as Uwe found, we actually need this to load most modules when symbol versioning is enabled, as it is in many distro kernels. Cc: Uwe Kleine-König <uwe@kleine-koenig.org> Fixes: 4dd1837d7589 ("arm: move exports to definitions") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- Compared to the earlier version, I dropped the changes to the csumpartial files, which now get handled correctly by Kbuild even when the export comes from a macro, and I also dropped the changes to the bitops files, which were already fixed in a patch from Nico. The patch applies cleanly on top of the rmk/fixes tree but has no effect there, as it also needs 4efca4ed05cb ("kbuild: modversions for EXPORT_SYMBOL() for asm") and cc6acc11cad1 ("kbuild: be more careful about matching preprocessed asm ___EXPORT_SYMBOL"). With the combination of rmk/fixes, torvalds/master and these two patches, symbol versioning works again on ARM. As it is still broken on almost all other architectures (powerpc is fixed, x86 has a patch), I wonder if we should make CONFIG_MODVERSIONS as broken for everything else. --- arch/arm/include/asm/asm-prototypes.h | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 arch/arm/include/asm/asm-prototypes.h