LinuxLists.cc - [GIT PULL] kbuild changes for v4.9-rc1

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Sat, 15 Oct 2016 17:22:05 -0700
Omar Sandoval <[email protected]> wrote:

> On Fri, Oct 14, 2016 at 10:12:46PM +0200, Michal Marek wrote:
> > Hi Linus,
> >
> > please pull these kbuild changes for v4.9-rc1:
> >
> > - EXPORT_SYMBOL for asm source by Al Viro. This does bring a regression,
> > because genksyms no longer generates checksums for these symbols
> > (CONFIG_MODVERSIONS). Nick Piggin is working on a patch to fix this.
> > Plus, we are talking about functions like strcpy(), which rarely
> > change prototypes.
>
> So this has broken all module loading for me. I get the following dmesg
> spew:
>
> ...
> [ 4.586914] scsi_mod: no symbol version for memset
> [ 4.587920] scsi_mod: Unknown symbol memset (err -22)
> [ 4.588443] scsi_mod: no symbol version for ___preempt_schedule
> [ 4.589026] scsi_mod: Unknown symbol ___preempt_schedule (err -22)
> ...
>
> Reverting 784d5699eddc ("x86: move exports to actual definitions") fixes
> it for me. This is with GCC 6.2.1, binutils 2.27, attached config.
>

Thanks for the report. Could you try this patch and see if it helps?

Allow architectures to create asm/asm-prototypes.h file that
provides C prototypes for exported asm functions, which enables
proper CRC versions to be generated for them.

It's done by creating a trivial C program that includes that file
and the EXPORT_SYMBOL()s from the .S file, and preprocesses that
then sends the result to genksyms.

Signed-off-by: Nicholas Piggin <[email protected]>
---
scripts/Makefile.build | 71 +++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 65 insertions(+), 6 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index de46ab0..edcf876 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -159,7 +159,8 @@ cmd_cpp_i_c = $(CPP) $(c_flags) -o $@ $<
$(obj)/%.i: $(src)/%.c FORCE
$(call if_changed_dep,cpp_i_c)

-cmd_gensymtypes = \
+# These mirror gensymtypes_S and co below, keep them in synch.
+cmd_gensymtypes_c = \
$(CPP) -D__GENKSYMS__ $(c_flags) $< | \
$(GENKSYMS) $(if $(1), -T $(2)) \
$(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX)) \
@@ -169,7 +170,7 @@ cmd_gensymtypes = \
quiet_cmd_cc_symtypes_c = SYM $(quiet_modtag) $@
cmd_cc_symtypes_c = \
set -e; \
- $(call cmd_gensymtypes,true,$@) >/dev/null; \
+ $(call cmd_gensymtypes_c,true,$@) >/dev/null; \
test -s $@ || rm -f $@

$(obj)/%.symtypes : $(src)/%.c FORCE
@@ -198,9 +199,10 @@ else
# the actual value of the checksum generated by genksyms

cmd_cc_o_c = $(CC) $(c_flags) -c -o $(@D)/.tmp_$(@F) $<
-cmd_modversions = \
+
+cmd_modversions_c = \
if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then \
- $(call cmd_gensymtypes,$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
+ $(call cmd_gensymtypes_c,$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
> $(@D)/.tmp_$(@F:.o=.ver); \
\
$(LD) $(LDFLAGS) -r -o $@ $(@D)/.tmp_$(@F) \
@@ -268,13 +270,14 @@ endif # CONFIG_STACK_VALIDATION
define rule_cc_o_c
$(call echo-cmd,checksrc) $(cmd_checksrc) \
$(call cmd_and_fixdep,cc_o_c) \
- $(cmd_modversions) \
+ $(cmd_modversions_c) \
$(cmd_objtool) \
$(call echo-cmd,record_mcount) $(cmd_record_mcount)
endef

define rule_as_o_S
$(call cmd_and_fixdep,as_o_S) \
+ $(cmd_modversions_S) \
$(cmd_objtool)
endef

@@ -314,6 +317,32 @@ modkern_aflags := $(KBUILD_AFLAGS_KERNEL) $(AFLAGS_KERNEL)
$(real-objs-m) : modkern_aflags := $(KBUILD_AFLAGS_MODULE) $(AFLAGS_MODULE)
$(real-objs-m:.o=.s): modkern_aflags := $(KBUILD_AFLAGS_MODULE) $(AFLAGS_MODULE)

+# .S file exports must have their C prototypes defined in asm/asm-prototypes.h
+# or a file that it includes, in order to get versioned symbols. We build a
+# dummy C file that includes asm-prototypes and the EXPORT_SYMBOL lines from
+# the .S file (with trailing ';'), and run genksyms on that, to extract vers.
+#
+# These mirror gensymtypes_c and co above, keep them in synch.
+cmd_gensymtypes_S = \
+ (echo "\#include <linux/kernel.h>" ; \
+ echo "\#include <asm/asm-prototypes.h>" ; \
+ grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) | \
+ $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | \
+ $(GENKSYMS) $(if $(1), -T $(2)) \
+ $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX)) \
+ $(if $(KBUILD_PRESERVE),-p) \
+ -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))
+
+quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@
+cmd_cc_symtypes_S = \
+ set -e; \
+ $(call cmd_gensymtypes_S,true,$@) >/dev/null; \
+ test -s $@ || rm -f $@
+
+$(obj)/%.symtypes : $(src)/%.S FORCE
+ $(call cmd,cc_symtypes_S)
+
+
quiet_cmd_cpp_s_S = CPP $(quiet_modtag) $@
cmd_cpp_s_S = $(CPP) $(a_flags) -o $@ $<

@@ -321,7 +350,37 @@ $(obj)/%.s: $(src)/%.S FORCE
$(call if_changed_dep,cpp_s_S)

quiet_cmd_as_o_S = AS $(quiet_modtag) $@
-cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
+
+ifndef CONFIG_MODVERSIONS
+cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
+
+else
+
+ASM_PROTOTYPES := $(wildcard $(srctree)/arch/$(SRCARCH)/include/asm/asm-prototypes.h)
+
+ifeq ($(ASM_PROTOTYPES),)
+cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
+
+else
+
+# versioning matches the C process described above, with difference that
+# we parse asm-prototypes.h C header to get function definitions.
+
+cmd_as_o_S = $(CC) $(a_flags) -c -o $(@D)/.tmp_$(@F) $<
+
+cmd_modversions_S = \
+ if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then \
+ $(call cmd_gensymtypes_S,$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
+ > $(@D)/.tmp_$(@F:.o=.ver); \
+ \
+ $(LD) $(LDFLAGS) -r -o $@ $(@D)/.tmp_$(@F) \
+ -T $(@D)/.tmp_$(@F:.o=.ver); \
+ rm -f $(@D)/.tmp_$(@F) $(@D)/.tmp_$(@F:.o=.ver); \
+ else \
+ mv -f $(@D)/.tmp_$(@F) $@; \
+ fi;
+endif
+endif

$(obj)/%.o: $(src)/%.S $(objtool_obj) FORCE
$(call if_changed_rule,as_o_S)
--
2.9.3

2016-10-17 06:59:30

by Mathieu Othacehe

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

Hi,

>> Reverting 784d5699eddc ("x86: move exports to actual definitions") fixes
>> it for me. This is with GCC 6.2.1, binutils 2.27, attached config.

I've had the same problem. Reverting your binutils from 2.27 to 2.26,
will also fix it. It seems ld 2.27 is not merging weak symbols from .o
files to vmlinux when linking.

Anyhow all crc for symbols exported from .S will be 0, so Nicholas
patch make sense.

Mathieu

2016-10-17 07:00:05

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Mon, 17 Oct 2016 08:51:31 +0200
Adam Borowski <[email protected]> wrote:

> On Mon, Oct 17, 2016 at 02:57:09PM +1100, Nicholas Piggin wrote:
> > On Sat, 15 Oct 2016 17:22:05 -0700 Omar Sandoval <[email protected]> wrote:
> > > On Fri, Oct 14, 2016 at 10:12:46PM +0200, Michal Marek wrote:
> > > > please pull these kbuild changes for v4.9-rc1:
> > > >
> > > > - EXPORT_SYMBOL for asm source by Al Viro. This does bring a regression,
> > > > because genksyms no longer generates checksums for these symbols
> > > > (CONFIG_MODVERSIONS). Nick Piggin is working on a patch to fix this.
> > > > Plus, we are talking about functions like strcpy(), which rarely
> > > > change prototypes.
> > >
> > > So this has broken all module loading for me. I get the following dmesg
> > > spew:
> > > ...
> > > [ 4.586914] scsi_mod: no symbol version for memset
> > > [ 4.587920] scsi_mod: Unknown symbol memset (err -22)
> > > [ 4.588443] scsi_mod: no symbol version for ___preempt_schedule
> > > [ 4.589026] scsi_mod: Unknown symbol ___preempt_schedule (err -22)
> > > ...
> > >
> > > Reverting 784d5699eddc ("x86: move exports to actual definitions") fixes
> > > it for me. This is with GCC 6.2.1, binutils 2.27, attached config.
> >
> > Thanks for the report. Could you try this patch and see if it helps?
> [patch snipped]
>
> Omar probably won't wake up in quite a while, so I've tested the patch.
> Alas, doesn't help. Similar spew (for the few modules I don't have =y),
> while reverting 784d5699eddc fixes it for me too.
>
> Debian sid toolchain: gcc 6.2.0-6, binutils 2.27-8, config at
> https://angband.pl/tmp/config-4.9.0-rc1-debug+.gz

Forgot to engage my brain before posting.

Architectures will need to have an include/asm/asm-prototypes.h that
defines or #include<>s C-style prototypes for exported asm functions.
We can do an asm-generic version for the common ones like memset so
there's not a lot of pointless duplication there.

Care to do a patch for x86?

Thanks,
Nick

2016-10-17 07:09:28

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Mon, Oct 17, 2016 at 02:57:09PM +1100, Nicholas Piggin wrote:
> On Sat, 15 Oct 2016 17:22:05 -0700 Omar Sandoval <[email protected]> wrote:
> > On Fri, Oct 14, 2016 at 10:12:46PM +0200, Michal Marek wrote:
> > > please pull these kbuild changes for v4.9-rc1:
> > >
> > > - EXPORT_SYMBOL for asm source by Al Viro. This does bring a regression,
> > > because genksyms no longer generates checksums for these symbols
> > > (CONFIG_MODVERSIONS). Nick Piggin is working on a patch to fix this.
> > > Plus, we are talking about functions like strcpy(), which rarely
> > > change prototypes.
> >
> > So this has broken all module loading for me. I get the following dmesg
> > spew:
> > ...
> > [ 4.586914] scsi_mod: no symbol version for memset
> > [ 4.587920] scsi_mod: Unknown symbol memset (err -22)
> > [ 4.588443] scsi_mod: no symbol version for ___preempt_schedule
> > [ 4.589026] scsi_mod: Unknown symbol ___preempt_schedule (err -22)
> > ...
> >
> > Reverting 784d5699eddc ("x86: move exports to actual definitions") fixes
> > it for me. This is with GCC 6.2.1, binutils 2.27, attached config.
>
> Thanks for the report. Could you try this patch and see if it helps?
[patch snipped]

Omar probably won't wake up in quite a while, so I've tested the patch.
Alas, doesn't help. Similar spew (for the few modules I don't have =y),
while reverting 784d5699eddc fixes it for me too.

Debian sid toolchain: gcc 6.2.0-6, binutils 2.27-8, config at
https://angband.pl/tmp/config-4.9.0-rc1-debug+.gz

Meow!
--
A MAP07 (Dead Simple) raspberry tincture recipe: 0.5l 95% alcohol, 1kg
raspberries, 0.4kg sugar; put into a big jar for 1 month. Filter out and
throw away the fruits (can dump them into a cake, etc), let the drink age
at least 3-6 months.

2016-10-17 10:01:54

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Mon, Oct 17, 2016 at 05:59:51PM +1100, Nicholas Piggin wrote:
> On Mon, 17 Oct 2016 08:51:31 +0200
> Adam Borowski <[email protected]> wrote:
> > On Mon, Oct 17, 2016 at 02:57:09PM +1100, Nicholas Piggin wrote:
> > > On Sat, 15 Oct 2016 17:22:05 -0700 Omar Sandoval <[email protected]> wrote:
> > > > So this has broken all module loading for me. I get the following dmesg
> > > > spew:
> > > > ...
> > > > [ 4.586914] scsi_mod: no symbol version for memset
> > > > [ 4.587920] scsi_mod: Unknown symbol memset (err -22)
> > > > [ 4.588443] scsi_mod: no symbol version for ___preempt_schedule
> > > > [ 4.589026] scsi_mod: Unknown symbol ___preempt_schedule (err -22)
> > > > ...
> > > >
> > > > Reverting 784d5699eddc ("x86: move exports to actual definitions") fixes
> > > > it for me. This is with GCC 6.2.1, binutils 2.27, attached config.
> > >
> > > Thanks for the report. Could you try this patch and see if it helps?
> > [patch snipped]
> >
> > Omar probably won't wake up in quite a while, so I've tested the patch.
> > Alas, doesn't help. Similar spew (for the few modules I don't have =y),
> > while reverting 784d5699eddc fixes it for me too.
>
> Forgot to engage my brain before posting.
>
> Architectures will need to have an include/asm/asm-prototypes.h that
> defines or #include<>s C-style prototypes for exported asm functions.
> We can do an asm-generic version for the common ones like memset so
> there's not a lot of pointless duplication there.
>
> Care to do a patch for x86?

Sure, did so. With the prototypes added, your patch works!
Tested on a handful of modules, and one out-of-tree (virtualbox).
I didn't try a 32-bit build.

Note that powerpc already has an include/asm/asm-prototypes.h which might or
might not be what you want. It doesn't have memset and co, for example.

Anyway, here's my stab at x86:

>From db746df65b920591606398b4b244f5b6dc9eea04 Mon Sep 17 00:00:00 2001
From: Adam Borowski <[email protected]>
Date: Mon, 17 Oct 2016 11:42:35 +0200
Subject: [PATCH] kbuild: provide include/asm/asm-prototypes.h for x86

Nicholas Piggin wrote:
> Architectures will need to have an include/asm/asm-prototypes.h that
> defines or #include<>s C-style prototypes for exported asm functions.
> We can do an asm-generic version for the common ones like memset so
> there's not a lot of pointless duplication there.

Signed-off-by: Adam Borowski <[email protected]>
---
arch/x86/include/asm/asm-prototypes.h | 13 +++++++++++++
include/asm-generic/asm-prototypes.h | 7 +++++++
2 files changed, 20 insertions(+)
create mode 100644 arch/x86/include/asm/asm-prototypes.h
create mode 100644 include/asm-generic/asm-prototypes.h

diff --git a/arch/x86/include/asm/asm-prototypes.h b/arch/x86/include/asm/asm-prototypes.h
new file mode 100644
index 0000000..072c97c
--- /dev/null
+++ b/arch/x86/include/asm/asm-prototypes.h
@@ -0,0 +1,13 @@
+#include <asm/ftrace.h>
+#include <asm/uaccess.h>
+#include <asm/uaccess.h>
+#include <asm/string.h>
+#include <asm/page.h>
+#include <asm/checksum.h>
+
+#include <asm-generic/asm-prototypes.h>
+
+#include <asm/page.h>
+#include <asm/pgtable.h>
+#include <asm/special_insns.h>
+#include <asm/preempt.h>
diff --git a/include/asm-generic/asm-prototypes.h b/include/asm-generic/asm-prototypes.h
new file mode 100644
index 0000000..df13637
--- /dev/null
+++ b/include/asm-generic/asm-prototypes.h
@@ -0,0 +1,7 @@
+#include <linux/bitops.h>
+extern void *__memset(void *, int, __kernel_size_t);
+extern void *__memcpy(void *, const void *, __kernel_size_t);
+extern void *__memmove(void *, const void *, __kernel_size_t);
+extern void *memset(void *, int, __kernel_size_t);
+extern void *memcpy(void *, const void *, __kernel_size_t);
+extern void *memmove(void *, const void *, __kernel_size_t);
--
2.9.3

2016-10-17 11:12:34

by Alexey Dobriyan

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Mon, Oct 17, 2016 at 1:01 PM, Adam Borowski <[email protected]> wrote:
> On Mon, Oct 17, 2016 at 05:59:51PM +1100, Nicholas Piggin wrote:
>> On Mon, 17 Oct 2016 08:51:31 +0200
>> Adam Borowski <[email protected]> wrote:

> --- /dev/null
> +++ b/include/asm-generic/asm-prototypes.h
> @@ -0,0 +1,7 @@
> +#include <linux/bitops.h>
> +extern void *__memset(void *, int, __kernel_size_t);
> +extern void *__memcpy(void *, const void *, __kernel_size_t);
> +extern void *__memmove(void *, const void *, __kernel_size_t);
> +extern void *memset(void *, int, __kernel_size_t);
> +extern void *memcpy(void *, const void *, __kernel_size_t);
> +extern void *memmove(void *, const void *, __kernel_size_t);

Before too late, those extern keywords aren't needed and
only slowdown compilation.

2016-10-17 11:18:14

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

Hi Alexey,

On Mon, Oct 17, 2016 at 1:12 PM, Alexey Dobriyan <[email protected]> wrote:
> On Mon, Oct 17, 2016 at 1:01 PM, Adam Borowski <[email protected]> wrote:
>> On Mon, Oct 17, 2016 at 05:59:51PM +1100, Nicholas Piggin wrote:
>>> On Mon, 17 Oct 2016 08:51:31 +0200
>>> Adam Borowski <[email protected]> wrote:
>
>> --- /dev/null
>> +++ b/include/asm-generic/asm-prototypes.h
>> @@ -0,0 +1,7 @@
>> +#include <linux/bitops.h>
>> +extern void *__memset(void *, int, __kernel_size_t);
>> +extern void *__memcpy(void *, const void *, __kernel_size_t);
>> +extern void *__memmove(void *, const void *, __kernel_size_t);
>> +extern void *memset(void *, int, __kernel_size_t);
>> +extern void *memcpy(void *, const void *, __kernel_size_t);
>> +extern void *memmove(void *, const void *, __kernel_size_t);
>
> Before too late, those extern keywords aren't needed and
> only slowdown compilation.

Do you have any profiling data backing this?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2016-10-17 11:33:05

by Alexey Dobriyan

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Mon, Oct 17, 2016 at 2:17 PM, Geert Uytterhoeven
<[email protected]> wrote:
> Hi Alexey,
>
> On Mon, Oct 17, 2016 at 1:12 PM, Alexey Dobriyan <[email protected]> wrote:
>> On Mon, Oct 17, 2016 at 1:01 PM, Adam Borowski <[email protected]> wrote:
>>> On Mon, Oct 17, 2016 at 05:59:51PM +1100, Nicholas Piggin wrote:
>>>> On Mon, 17 Oct 2016 08:51:31 +0200
>>>> Adam Borowski <[email protected]> wrote:
>>
>>> --- /dev/null
>>> +++ b/include/asm-generic/asm-prototypes.h
>>> @@ -0,0 +1,7 @@
>>> +#include <linux/bitops.h>
>>> +extern void *__memset(void *, int, __kernel_size_t);
>>> +extern void *__memcpy(void *, const void *, __kernel_size_t);
>>> +extern void *__memmove(void *, const void *, __kernel_size_t);
>>> +extern void *memset(void *, int, __kernel_size_t);
>>> +extern void *memcpy(void *, const void *, __kernel_size_t);
>>> +extern void *memmove(void *, const void *, __kernel_size_t);
>>
>> Before too late, those extern keywords aren't needed and
>> only slowdown compilation.
>
> Do you have any profiling data backing this?

It is easy to obtain. Here is top 10 with runtime bigger than 0.50%:

_int_malloc
ht_lookup_with_hash
_cpp_lex_direct
cpp_get_token_1
ggc_internal_alloc_statm
_int_free
malloc_consolidate
lex_identifier
enter_macro_context
c_lex_with_flags
bitmap_set_bit
malloc
grokdeclarator
htab_find_slot_with_hash
c_lex_one_token
_cpp_lex_token
pop_scopev
c_parser_declspecs

Imagine you're a compiler. Those extern's are tokens. They need to be parsed
(byte-by-byte), allocated, and, in case of "extern" with prototype, thrown away.
They make line longer forcing people to dance around with splitting
per 80 characters
and generally making everything somewhat slower. Might as well don't new ones.

bitops.h is wrong header as well.
Why do you need bitops for bunch of function prototypes?

2016-10-17 12:24:51

by Mathieu Othacehe

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

> +#include <asm/uaccess.h>
> +#include <asm/uaccess.h>

Included twice.

> +#include <asm/string.h>
> +#include <asm/page.h>
> +#include <asm/checksum.h>
> +
> +#include <asm-generic/asm-prototypes.h>
> +
> +#include <asm/page.h>
> +#include <asm/pgtable.h>
> +#include <asm/special_insns.h>
> +#include <asm/preempt.h>

No <asm/arch_hweight.h> for __sw_hweight32 and __sw_hweight64 ?

2016-10-17 12:27:44

[permalink] [raw]

Subject: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

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 'extern' declarations out of memset_io/memcpy_io
to make them visible to the symbol version generator
- 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.

Signed-off-by: Arnd Bergmann <[email protected]>

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/include/asm/io.h b/arch/arm/include/asm/io.h
index 51458d8273ad..fbc3695293cf 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -316,11 +316,13 @@ extern void _memset_io(volatile void __iomem *, int, size_t);
#define writesw(p,d,l) __raw_writesw(p,d,l)
#define writesl(p,d,l) __raw_writesl(p,d,l)

+extern void mmioset(void *, unsigned int, size_t);
+extern void mmiocpy(void *, const void *, size_t);
+
#ifndef __ARMBE__
static inline void memset_io(volatile void __iomem *dst, unsigned c,
size_t count)
{
- extern void mmioset(void *, unsigned int, size_t);
mmioset((void __force *)dst, c, count);
}
#define memset_io(dst,c,count) memset_io(dst,c,count)
@@ -328,7 +330,6 @@ static inline void memset_io(volatile void __iomem *dst, unsigned c,
static inline void memcpy_fromio(void *to, const volatile void __iomem *from,
size_t count)
{
- extern void mmiocpy(void *, const void *, size_t);
mmiocpy(to, (const void __force *)from, count);
}
#define memcpy_fromio(to,from,count) memcpy_fromio(to,from,count)
@@ -336,7 +337,6 @@ static inline void memcpy_fromio(void *to, const volatile void __iomem *from,
static inline void memcpy_toio(volatile void __iomem *to, const void *from,
size_t count)
{
- extern void mmiocpy(void *, const void *, size_t);
mmiocpy((void __force *)to, from, count);
}
#define memcpy_toio(to,from,count) memcpy_toio(to,from,count)
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)

2016-10-18 00:17:12

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Mon, Oct 17, 2016 at 02:22:34PM +0200, Mathieu OTHACEHE wrote:
> > +#include <asm/uaccess.h>
> > +#include <asm/uaccess.h>
>
> Included twice.

D'oh!

> > +#include <asm/string.h>
> > +#include <asm/page.h>
> > +#include <asm/checksum.h>
> > +
> > +#include <asm-generic/asm-prototypes.h>
> > +
> > +#include <asm/page.h>
> > +#include <asm/pgtable.h>
> > +#include <asm/special_insns.h>
> > +#include <asm/preempt.h>
>
> No <asm/arch_hweight.h> for __sw_hweight32 and __sw_hweight64 ?

diff --git a/include/asm-generic/asm-prototypes.h b/include/asm-generic/asm-prototypes.h
new file mode 100644
index 0000000..df13637
--- /dev/null
+++ b/include/asm-generic/asm-prototypes.h
@@ -0,0 +1,7 @@
+#include <linux/bitops.h>

... which has these.

Alexey Dobriyan <[email protected]> wrote:
} bitops.h is wrong header as well.
} Why do you need bitops for bunch of function prototypes?

Unless you guys prefer using low-level headers only, that is.

Meow!
--
A MAP07 (Dead Simple) raspberry tincture recipe: 0.5l 95% alcohol, 1kg
raspberries, 0.4kg sugar; put into a big jar for 1 month. Filter out and
throw away the fruits (can dump them into a cake, etc), let the drink age
at least 3-6 months.

2016-10-18 01:34:30

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

Hi Adam,

Thanks, this is looking good. powerpc will be able to use the generic
header.

On Tue, 18 Oct 2016 02:16:26 +0200
Adam Borowski <[email protected]> wrote:

> On Mon, Oct 17, 2016 at 02:22:34PM +0200, Mathieu OTHACEHE wrote:
> > > +#include <asm/uaccess.h>
> > > +#include <asm/uaccess.h>
> >
> > Included twice.
>
> D'oh!
>
> > > +#include <asm/string.h>
> > > +#include <asm/page.h>
> > > +#include <asm/checksum.h>
> > > +
> > > +#include <asm-generic/asm-prototypes.h>
> > > +
> > > +#include <asm/page.h>
> > > +#include <asm/pgtable.h>
> > > +#include <asm/special_insns.h>
> > > +#include <asm/preempt.h>
> >
> > No <asm/arch_hweight.h> for __sw_hweight32 and __sw_hweight64 ?
>
> diff --git a/include/asm-generic/asm-prototypes.h b/include/asm-generic/asm-prototypes.h
> new file mode 100644
> index 0000000..df13637
> --- /dev/null
> +++ b/include/asm-generic/asm-prototypes.h
> @@ -0,0 +1,7 @@
> +#include <linux/bitops.h>
>
> ... which has these.
>
> Alexey Dobriyan <[email protected]> wrote:
> } bitops.h is wrong header as well.
> } Why do you need bitops for bunch of function prototypes?
>
> Unless you guys prefer using low-level headers only, that is.

Well you can't use asm/arch_hweight.h in a generic header of course.
I would suggest just including linux/ variants where practical for
the asm-generic/asm-prototypes.h header.

We should probably just bring all these arch patches through the
kbuild tree.

I'm sorry for the breakage: I didn't realize it broke the build with
some configs, otherwise I would have given Michal a heads up before
his pull request, and worked to get this stuff in first.

Thanks,
Nick

2016-10-19 14:38:28

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

Dne 18.10.2016 v 03:34 Nicholas Piggin napsal(a):
> We should probably just bring all these arch patches through the
> kbuild tree.
>
> I'm sorry for the breakage: I didn't realize it broke the build with
> some configs, otherwise I would have given Michal a heads up before
> his pull request, and worked to get this stuff in first.

It breaks with some binutils versions only (and only with
CONFIG_MODVERSIONS=y, of course).

Michal

2016-10-19 14:52:15

[permalink] [raw]

Subject: Re: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

Dne 17.10.2016 v 14:26 Arnd Bergmann napsal(a):
> 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 'extern' declarations out of memset_io/memcpy_io
> to make them visible to the symbol version generator
> - 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.
>
> Signed-off-by: Arnd Bergmann <[email protected]>

Hi Arnd,

just to make sure I'm looking at the right code - is this based on the
patch by Nick here: https://patchwork.kernel.org/patch/9377783/?

Thanks,
Michal

2016-10-19 15:03:19

[permalink] [raw]

Subject: Re: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

On Wednesday, October 19, 2016 4:52:06 PM CEST Michal Marek wrote:
> Dne 17.10.2016 v 14:26 Arnd Bergmann napsal(a):
> > 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 'extern' declarations out of memset_io/memcpy_io
> > to make them visible to the symbol version generator
> > - 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.
> >
> > Signed-off-by: Arnd Bergmann <[email protected]>
>
> Hi Arnd,
>
> just to make sure I'm looking at the right code - is this based on the
> patch by Nick here: https://patchwork.kernel.org/patch/9377783/?
>

(adding Russell to Cc, I missed him during my earlier mail, which
is now archived at https://lkml.org/lkml/2016/10/17/356)

Correct. I had imported Nick's patch into my randconfig tree and
this is what I needed to build all configurations cleanly with it.

Arnd

2016-10-19 15:32:22

[permalink] [raw]

Subject: Re: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

On Wed, Oct 19, 2016 at 05:02:55PM +0200, Arnd Bergmann wrote:
> On Wednesday, October 19, 2016 4:52:06 PM CEST Michal Marek wrote:
> > Dne 17.10.2016 v 14:26 Arnd Bergmann napsal(a):
> > > 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 'extern' declarations out of memset_io/memcpy_io
> > > to make them visible to the symbol version generator
> > > - 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.
> > >
> > > Signed-off-by: Arnd Bergmann <[email protected]>
> >
> > Hi Arnd,
> >
> > just to make sure I'm looking at the right code - is this based on the
> > patch by Nick here: https://patchwork.kernel.org/patch/9377783/?
> >
>
> (adding Russell to Cc, I missed him during my earlier mail, which
> is now archived at https://lkml.org/lkml/2016/10/17/356)

I'm not in favour of this.

+extern void mmioset(void *, unsigned int, size_t);
+extern void mmiocpy(void *, const void *, size_t);
+
#ifndef __ARMBE__
static inline void memset_io(volatile void __iomem *dst, unsigned c,
size_t count)
{
- extern void mmioset(void *, unsigned int, size_t);
mmioset((void __force *)dst, c, count);
}

The reason they're declared _within_ memset_io() is to prevent people
from using them by hiding their declaration. Moving them outside is
an open invitation to stupid people starting to use them as an "oh it
must be an official API".

We know this happens, there's been a long history of this kind of stupid
in the ARM community, not only with cache flushing APIs, but also the
DMA APIs.

The way the existing code is written is a completely valid way to hide
declarations from outside the intended caller's scope.

We've been here many times, we've had many people doing this crap, so
I'm now at the point of NAKing changes which result in an increased
visibility to the rest of the kernel of symbols that should not be
used by stupid driver authors.

Now, why do we have these extra functions when they're just aliased to
memset()/memcpy() - to avoid GCC optimising them because it thinks that
they're standard memset()/memcpy().

So overall this gets a NAK from me.

Now, it would have _ALSO_ been nice to have been at least COPIED on the
original set of changes that caused the need for this change. I wasn't.
So I want to see the original set of changes reverted, because they're
clearly causing breakage. Let's revert them and then go through the
proper process of maintainer review, rather than bypassing maintainers
and screwing up architectures in the process. There really is no
excuse for this crap.

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

2016-10-20 03:52:14

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Wed, 19 Oct 2016 16:38:14 +0200
Michal Marek <[email protected]> wrote:

> Dne 18.10.2016 v 03:34 Nicholas Piggin napsal(a):
> > We should probably just bring all these arch patches through the
> > kbuild tree.
> >
> > I'm sorry for the breakage: I didn't realize it broke the build with
> > some configs, otherwise I would have given Michal a heads up before
> > his pull request, and worked to get this stuff in first.
>
> It breaks with some binutils versions only (and only with
> CONFIG_MODVERSIONS=y, of course).

Yeah this seems to be the issue, it apparently slipped past all the
automated builds. It seems like the existing CRC warnings in the tree
only trigger in rare circumstances too, so something could be a bit
fragile there.

Thanks,
Nick

2016-10-20 04:08:25

[permalink] [raw]

Subject: Re: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

On Wed, 19 Oct 2016 16:32:00 +0100
Russell King - ARM Linux <[email protected]> wrote:

> On Wed, Oct 19, 2016 at 05:02:55PM +0200, Arnd Bergmann wrote:
> > On Wednesday, October 19, 2016 4:52:06 PM CEST Michal Marek wrote:
> > > Dne 17.10.2016 v 14:26 Arnd Bergmann napsal(a):
> > > > 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 'extern' declarations out of memset_io/memcpy_io
> > > > to make them visible to the symbol version generator
> > > > - 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.
> > > >
> > > > Signed-off-by: Arnd Bergmann <[email protected]>
> > >
> > > Hi Arnd,
> > >
> > > just to make sure I'm looking at the right code - is this based on the
> > > patch by Nick here: https://patchwork.kernel.org/patch/9377783/?
> > >
> >
> > (adding Russell to Cc, I missed him during my earlier mail, which
> > is now archived at https://lkml.org/lkml/2016/10/17/356)
>
> I'm not in favour of this.
>
> +extern void mmioset(void *, unsigned int, size_t);
> +extern void mmiocpy(void *, const void *, size_t);
> +
> #ifndef __ARMBE__
> static inline void memset_io(volatile void __iomem *dst, unsigned c,
> size_t count)
> {
> - extern void mmioset(void *, unsigned int, size_t);
> mmioset((void __force *)dst, c, count);
> }
>
> The reason they're declared _within_ memset_io() is to prevent people
> from using them by hiding their declaration. Moving them outside is
> an open invitation to stupid people starting to use them as an "oh it
> must be an official API".
>
> We know this happens, there's been a long history of this kind of stupid
> in the ARM community, not only with cache flushing APIs, but also the
> DMA APIs.
>
> The way the existing code is written is a completely valid way to hide
> declarations from outside the intended caller's scope.
>
> We've been here many times, we've had many people doing this crap, so
> I'm now at the point of NAKing changes which result in an increased
> visibility to the rest of the kernel of symbols that should not be
> used by stupid driver authors.
>
> Now, why do we have these extra functions when they're just aliased to
> memset()/memcpy() - to avoid GCC optimising them because it thinks that
> they're standard memset()/memcpy().
>
> So overall this gets a NAK from me.

Fair point, what about leaving those as they are, and also adding
them to asm-prototypes.h protected with GENKSYMS ifdef? It's not
beautiful, but still better than armksyms.c before Al's patches (or
at least no worse).

> Now, it would have _ALSO_ been nice to have been at least COPIED on the
> original set of changes that caused the need for this change. I wasn't.
> So I want to see the original set of changes reverted, because they're
> clearly causing breakage. Let's revert them and then go through the
> proper process of maintainer review, rather than bypassing maintainers
> and screwing up architectures in the process. There really is no
> excuse for this crap.

You may have a point about improvement of the process. I wasn't
involved in the original patches, but we did cc linux-arch when the
.S CRC issue became known.

However let's work on the assumption that they won't be reverted at this
stage, and try to come up with something to fix it that you're happy with.

Thanks,
Nick

2016-10-20 07:37:34

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

On Wed, Oct 19, 2016 at 5:32 PM, Russell King - ARM Linux
<[email protected]> wrote:
> I'm not in favour of this.
>
> +extern void mmioset(void *, unsigned int, size_t);
> +extern void mmiocpy(void *, const void *, size_t);
> +
> #ifndef __ARMBE__
> static inline void memset_io(volatile void __iomem *dst, unsigned c,
> size_t count)
> {
> - extern void mmioset(void *, unsigned int, size_t);
> mmioset((void __force *)dst, c, count);
> }
>
> The reason they're declared _within_ memset_io() is to prevent people
> from using them by hiding their declaration. Moving them outside is
> an open invitation to stupid people starting to use them as an "oh it
> must be an official API".

If they're not intended for public use, they should (also) be prefixed
with "__" or even "____" to make this clear.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2016-10-20 08:20:36

[permalink] [raw]

Subject: Re: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

On Thu, Oct 20, 2016 at 09:37:30AM +0200, Geert Uytterhoeven wrote:
> On Wed, Oct 19, 2016 at 5:32 PM, Russell King - ARM Linux
> <[email protected]> wrote:
> > I'm not in favour of this.
> >
> > +extern void mmioset(void *, unsigned int, size_t);
> > +extern void mmiocpy(void *, const void *, size_t);
> > +
> > #ifndef __ARMBE__
> > static inline void memset_io(volatile void __iomem *dst, unsigned c,
> > size_t count)
> > {
> > - extern void mmioset(void *, unsigned int, size_t);
> > mmioset((void __force *)dst, c, count);
> > }
> >
> > The reason they're declared _within_ memset_io() is to prevent people
> > from using them by hiding their declaration. Moving them outside is
> > an open invitation to stupid people starting to use them as an "oh it
> > must be an official API".
>
> If they're not intended for public use, they should (also) be prefixed
> with "__" or even "____" to make this clear.

Tried that with the __cpuc_* cache flushing interfaces. It doesn't
have any effect what so ever.

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

2016-10-20 08:24:02

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

Hi Russell,

On Thu, Oct 20, 2016 at 10:20 AM, Russell King - ARM Linux
<[email protected]> wrote:
> On Thu, Oct 20, 2016 at 09:37:30AM +0200, Geert Uytterhoeven wrote:
>> On Wed, Oct 19, 2016 at 5:32 PM, Russell King - ARM Linux
>> <[email protected]> wrote:
>> > I'm not in favour of this.
>> >
>> > +extern void mmioset(void *, unsigned int, size_t);
>> > +extern void mmiocpy(void *, const void *, size_t);
>> > +
>> > #ifndef __ARMBE__
>> > static inline void memset_io(volatile void __iomem *dst, unsigned c,
>> > size_t count)
>> > {
>> > - extern void mmioset(void *, unsigned int, size_t);
>> > mmioset((void __force *)dst, c, count);
>> > }
>> >
>> > The reason they're declared _within_ memset_io() is to prevent people
>> > from using them by hiding their declaration. Moving them outside is
>> > an open invitation to stupid people starting to use them as an "oh it
>> > must be an official API".
>>
>> If they're not intended for public use, they should (also) be prefixed
>> with "__" or even "____" to make this clear.
>
> Tried that with the __cpuc_* cache flushing interfaces. It doesn't
> have any effect what so ever.

it may not stop the deliberate abuser, but it hints the casual reviewer.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2016-10-20 13:19:30

[permalink] [raw]

Subject: Re: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

On Thu, Oct 20, 2016 at 03:08:14PM +1100, Nicholas Piggin wrote:
> Fair point, what about leaving those as they are, and also adding
> them to asm-prototypes.h protected with GENKSYMS ifdef? It's not
> beautiful, but still better than armksyms.c before Al's patches (or
> at least no worse).

I disagree (also see below). The armksyms way was understandable.
The new way... I've no idea yet, because I wasn't even copied on
any of the patches. I've no idea how the exports are now handled.
I'm in a black hole with respect to that, and that's now a problem.

> > Now, it would have _ALSO_ been nice to have been at least COPIED on the
> > original set of changes that caused the need for this change. I wasn't.
> > So I want to see the original set of changes reverted, because they're
> > clearly causing breakage. Let's revert them and then go through the
> > proper process of maintainer review, rather than bypassing maintainers
> > and screwing up architectures in the process. There really is no
> > excuse for this crap.
>
> You may have a point about improvement of the process. I wasn't
> involved in the original patches, but we did cc linux-arch when the
> .S CRC issue became known.

Yes, but I'm not on linux-kernel-v2, and I've no desire to end up with
another list I've no hope of keeping up with to my mailbox - I'll just
ignore it. 99% of the messages on it at the time when vger kicked me
off the list was x86 related discussion, and not really cross-arch
issues. As I say, it just became another linux-kernel list.

> However let's work on the assumption that they won't be reverted at this
> stage, and try to come up with something to fix it that you're happy with.

Well, there's more problems with this new KSYMS approach than just the
CRCs. It forces a rebuild of the ksyms files every single time, which
then causes a relink of the kernel:

CHK include/config/kernel.release
GEN ./Makefile
CHK include/generated/uapi/linux/version.h
CHK include/generated/utsrelease.h
Using /home/rmk/git/linux-rmk as source for kernel
CHK include/generated/timeconst.h
CHK include/generated/bounds.h
CHK include/generated/asm-offsets.h
CALL /home/rmk/git/linux-rmk/scripts/checksyscalls.sh - due to target missing
CHK include/generated/compile.h
EXPORTS arch/arm/lib/lib-ksyms.o - due to lib-ksyms.o not in $(targets)
LD arch/arm/lib/built-in.o - due to: arch/arm/lib/lib-ksyms.o
EXPORTS lib/lib-ksyms.o - due to lib-ksyms.o not in $(targets)
LD lib/built-in.o - due to: lib/lib-ksyms.o
LD vmlinux.o
MODPOST vmlinux.o - due to vmlinux.o not in $(targets)
GEN .version
CHK include/generated/compile.h
UPD include/generated/compile.h
CC init/version.o - due to: include/generated/compile.h
LD init/built-in.o - due to: init/version.o
KSYM .tmp_kallsyms1.o
KSYM .tmp_kallsyms2.o
LD vmlinux
SORTEX vmlinux
SYSMAP System.map
OBJCOPY arch/arm/boot/Image - due to: vmlinux
Building modules, stage 2.
Kernel: arch/arm/boot/Image is ready
LZO arch/arm/boot/compressed/piggy_data - due to: arch/arm/boot/compressed/../Image
MODPOST 689 modules - due to target is PHONY
AS arch/arm/boot/compressed/piggy.o - due to: arch/arm/boot/compressed/piggy_data
LD arch/arm/boot/compressed/vmlinux - due to: arch/arm/boot/compressed/piggy.o
OBJCOPY arch/arm/boot/zImage - due to: arch/arm/boot/compressed/vmlinux
Kernel: arch/arm/boot/zImage is ready

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

2016-10-20 14:21:15

[permalink] [raw]

Subject: Re: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

On Thu, 20 Oct 2016 14:17:02 +0100
Russell King - ARM Linux <[email protected]> wrote:

> On Thu, Oct 20, 2016 at 03:08:14PM +1100, Nicholas Piggin wrote:
> > Fair point, what about leaving those as they are, and also adding
> > them to asm-prototypes.h protected with GENKSYMS ifdef? It's not
> > beautiful, but still better than armksyms.c before Al's patches (or
> > at least no worse).
>
> I disagree (also see below). The armksyms way was understandable.
> The new way... I've no idea yet, because I wasn't even copied on

New way is you put the EXPORT_SYMBOL in the .S file, and give it a C
style prototype in asm-prototypes.h, and that's it. The build system
will do the rest. Either way you require some file of C prototypes,
but after Al's patches, at least the EXPORT goes with its definition.

As far as rebuilding too often, that sounds like a bug, more below.

> any of the patches. I've no idea how the exports are now handled.
> I'm in a black hole with respect to that, and that's now a problem.
>
> > > Now, it would have _ALSO_ been nice to have been at least COPIED on the
> > > original set of changes that caused the need for this change. I wasn't.
> > > So I want to see the original set of changes reverted, because they're
> > > clearly causing breakage. Let's revert them and then go through the
> > > proper process of maintainer review, rather than bypassing maintainers
> > > and screwing up architectures in the process. There really is no
> > > excuse for this crap.
> >
> > You may have a point about improvement of the process. I wasn't
> > involved in the original patches, but we did cc linux-arch when the
> > .S CRC issue became known.
>
> Yes, but I'm not on linux-kernel-v2, and I've no desire to end up with
> another list I've no hope of keeping up with to my mailbox - I'll just
> ignore it. 99% of the messages on it at the time when vger kicked me
> off the list was x86 related discussion, and not really cross-arch
> issues. As I say, it just became another linux-kernel list.

For the patches that touched arm code, I'd agree you should have been cc'ed.

Not to dismiss that concern at all, but for issues of interest to arch code
but not specific to any (such as discovery that the asm exports change would
require this change to restore CRC generation), I was just saying linux-arch
is most appropriate for better or worse. Ccing 30 arch lists is not workable.

And again not to dismiss your concern, I'm just here trying to come up with
a workable fix for the non-revert scenario. Revert is no less valid an
option, it's just not one I'm in favour of myself.

> > However let's work on the assumption that they won't be reverted at this
> > stage, and try to come up with something to fix it that you're happy with.
>
> Well, there's more problems with this new KSYMS approach than just the
> CRCs. It forces a rebuild of the ksyms files every single time, which
> then causes a relink of the kernel:

Good catch, I'm surprised you're the first one who reported it. This patch
seems to do the trick for me:

From: Nicholas Piggin <[email protected]>
Date: Fri, 21 Oct 2016 01:13:33 +1100
Subject: [PATCH] kbuild: prevent lib-ksyms.o rebuilds

Signed-off-by: Nicholas Piggin <[email protected]>

---
scripts/Makefile.build | 3 +++
1 file changed, 3 insertions(+)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index de46ab0..e1f25d6 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -430,6 +430,9 @@ cmd_export_list = $(OBJDUMP) -h $< | \

$(obj)/lib-ksyms.o: $(lib-target) FORCE
$(call if_changed,export_list)
+
+targets += $(obj)/lib-ksyms.o
+
endif

#
--
2.9.3

2016-10-20 14:33:40

[permalink] [raw]

Subject: Re: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

On Fri, Oct 21, 2016 at 01:20:17AM +1100, Nicholas Piggin wrote:
> Good catch, I'm surprised you're the first one who reported it. This patch
> seems to do the trick for me:

And me, thanks, so...

>
> From: Nicholas Piggin <[email protected]>
> Date: Fri, 21 Oct 2016 01:13:33 +1100
> Subject: [PATCH] kbuild: prevent lib-ksyms.o rebuilds
>
> Signed-off-by: Nicholas Piggin <[email protected]>

Reported-by: Russell King <[email protected]>
Tested-by: Russell King <[email protected]>

>
> ---
> scripts/Makefile.build | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index de46ab0..e1f25d6 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -430,6 +430,9 @@ cmd_export_list = $(OBJDUMP) -h $< | \
>
> $(obj)/lib-ksyms.o: $(lib-target) FORCE
> $(call if_changed,export_list)
> +
> +targets += $(obj)/lib-ksyms.o
> +
> endif
>
> #

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

2016-10-20 14:52:06

[permalink] [raw]

Subject: Re: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

On Thu, 20 Oct 2016 15:33:27 +0100
Russell King - ARM Linux <[email protected]> wrote:

> On Fri, Oct 21, 2016 at 01:20:17AM +1100, Nicholas Piggin wrote:
> > Good catch, I'm surprised you're the first one who reported it. This patch
> > seems to do the trick for me:
>
> And me, thanks, so...
>
> >
> > From: Nicholas Piggin <[email protected]>
> > Date: Fri, 21 Oct 2016 01:13:33 +1100
> > Subject: [PATCH] kbuild: prevent lib-ksyms.o rebuilds
> >
> > Signed-off-by: Nicholas Piggin <[email protected]>
>
> Reported-by: Russell King <[email protected]>
> Tested-by: Russell King <[email protected]>

Thanks for testing it.

Hopefully if Arnd is able to respin the ARM patch to something a bit more
to your liking that doesn't expose prototypes, and no other problems arise,
we can avoid reverting. You had a poor first impression, but we may yet win
you over.

2016-10-22 19:51:25

[permalink] [raw]

Subject: Re: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

On Thu, Oct 20, 2016 at 03:33:27PM +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 21, 2016 at 01:20:17AM +1100, Nicholas Piggin wrote:
> > Good catch, I'm surprised you're the first one who reported it. This patch
> > seems to do the trick for me:
>
> And me, thanks, so...
>
> >
> > From: Nicholas Piggin <[email protected]>
> > Date: Fri, 21 Oct 2016 01:13:33 +1100
> > Subject: [PATCH] kbuild: prevent lib-ksyms.o rebuilds
> >
> > Signed-off-by: Nicholas Piggin <[email protected]>
>
> Reported-by: Russell King <[email protected]>
> Tested-by: Russell King <[email protected]>

Thanks. Added to kbuild.git#rc-fixes.

Michal

2016-10-24 15:05:53

[permalink] [raw]

Subject: Re: [PATCH] kbuild: provide include/asm/asm-prototypes.h for ARM

On Thursday, October 20, 2016 3:08:14 PM CEST Nicholas Piggin wrote:
> On Wed, 19 Oct 2016 16:32:00 +0100 Russell King - ARM Linux <[email protected]> wrote:
> > I'm not in favour of this.
> >
> > +extern void mmioset(void *, unsigned int, size_t);
> > +extern void mmiocpy(void *, const void *, size_t);
> > +
> > #ifndef __ARMBE__
> > static inline void memset_io(volatile void __iomem *dst, unsigned c,
> > size_t count)
> > {
> > - extern void mmioset(void *, unsigned int, size_t);
> > mmioset((void __force *)dst, c, count);
> > }
> >
> > The reason they're declared _within_ memset_io() is to prevent people
> > from using them by hiding their declaration. Moving them outside is
> > an open invitation to stupid people starting to use them as an "oh it
> > must be an official API".
> >

I've split out that change from the other ones now, and will follow
up with the patch to address all the other ones first.

> Fair point, what about leaving those as they are, and also adding
> them to asm-prototypes.h protected with GENKSYMS ifdef? It's not
> beautiful, but still better than armksyms.c before Al's patches (or
> at least no worse).

I'm trying this one, and an alternative patch that moves the
export into arch/arm/kernel/io.h. Let's see if we can agree
on one of these.

Arnd

2016-10-24 15:06:45

[permalink] [raw]

Subject: [PATCH 1/2] kbuild: provide include/asm/asm-prototypes.h for ARM

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 <[email protected]>

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)

2016-10-24 15:07:02

[permalink] [raw]

Subject: [PATCH 2/2, variant B] ARM: move mmiocpy/mmioset exports to io.c

The prototypes for mmioset/mmiocpy are intentionally hidden
inside of inline functions, which breaks the EXPORT_SYMBOL
statements when symbol versioning is enabled.

This moves the two exports from the files that implement the
code into the kernel/io.c file, adding another local declaration
there.

Signed-off-by: Arnd Bergmann <[email protected]>

diff --git a/arch/arm/kernel/io.c b/arch/arm/kernel/io.c
index eedefe050022..c74746997626 100644
--- a/arch/arm/kernel/io.c
+++ b/arch/arm/kernel/io.c
@@ -82,3 +82,10 @@ void _memset_io(volatile void __iomem *dst, int c, size_t count)
}
}
EXPORT_SYMBOL(_memset_io);
+
+/* can't export them from memcpy.S/memset.S because of hidden declaration */
+void mmioset(void __iomem *addr, unsigned int c, size_t n);
+EXPORT_SYMBOL(mmioset);
+
+void mmiocpy(void *dest, const void __iomem *src, size_t n);
+EXPORT_SYMBOL(mmiocpy);
diff --git a/arch/arm/lib/memcpy.S b/arch/arm/lib/memcpy.S
index 1be5b6ddf37c..1f822fc52400 100644
--- a/arch/arm/lib/memcpy.S
+++ b/arch/arm/lib/memcpy.S
@@ -70,4 +70,3 @@ ENTRY(memcpy)
ENDPROC(memcpy)
ENDPROC(mmiocpy)
EXPORT_SYMBOL(memcpy)
-EXPORT_SYMBOL(mmiocpy)
diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
index 7b72044cba62..6f075ca09abc 100644
--- a/arch/arm/lib/memset.S
+++ b/arch/arm/lib/memset.S
@@ -137,4 +137,3 @@ UNWIND( .fnend )
ENDPROC(memset)
ENDPROC(mmioset)
EXPORT_SYMBOL(memset)
-EXPORT_SYMBOL(mmioset)

2016-10-24 15:07:39

[permalink] [raw]

Subject: [PATCH 2/2, variant A] ARM: add hidden mmioset/mmiocpy prototypes

The prototypes for mmioset/mmiocpy are intentionally hidden
inside of inline functions, which breaks the EXPORT_SYMBOL
statements when symbol versioning is enabled.

This adds a prototype to asm/asm-prototypes.h but hides it
in an #ifdef so normal drivers don't see it and won't be
able to abuse the interface.

Suggested-by: Nicholas Piggin <[email protected]>
Signed-off-by: Arnd Bergmann <[email protected]>

diff --git a/arch/arm/include/asm/asm-prototypes.h b/arch/arm/include/asm/asm-prototypes.h
index 04e5616a7b15..e46b09536b14 100644
--- a/arch/arm/include/asm/asm-prototypes.h
+++ b/arch/arm/include/asm/asm-prototypes.h
@@ -32,3 +32,8 @@ extern void __muldi3(void);
extern void __ucmpdi2(void);
extern void __udivsi3(void);
extern void __umodsi3(void);
+
+#ifdef __GENKSYMS__
+extern void mmioset(void *, unsigned int, size_t);
+extern void mmiocpy(void *, const void *, size_t);
+#endif

2016-10-25 08:32:17

[permalink] [raw]

Subject: Re: [PATCH 1/2] kbuild: provide include/asm/asm-prototypes.h for ARM

On Mon, 24 Oct 2016 17:05:26 +0200
Arnd Bergmann <[email protected]> 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

2016-10-27 13:54:56

by Kalle Valo

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

Nicholas Piggin <[email protected]> writes:

> On Thu, 27 Oct 2016 11:10:03 +0300
> Kalle Valo <[email protected]> wrote:
>
>> (Adding Thorsten because of a serious regression and Steven because he
>> tried to fix something in the same commit)
>>
>> Nicholas Piggin <[email protected]> writes:
>>
>> > On Wed, 19 Oct 2016 16:38:14 +0200
>> > Michal Marek <[email protected]> wrote:
>> >
>> >> Dne 18.10.2016 v 03:34 Nicholas Piggin napsal(a):
>> >> > We should probably just bring all these arch patches through the
>> >> > kbuild tree.
>> >> >
>> >> > I'm sorry for the breakage: I didn't realize it broke the build with
>> >> > some configs, otherwise I would have given Michal a heads up before
>> >> > his pull request, and worked to get this stuff in first.
>> >>
>> >> It breaks with some binutils versions only (and only with
>> >> CONFIG_MODVERSIONS=y, of course).
>> >
>> > Yeah this seems to be the issue, it apparently slipped past all the
>> > automated builds. It seems like the existing CRC warnings in the tree
>> > only trigger in rare circumstances too, so something could be a bit
>> > fragile there.
>>
>> I upgraded from 4.8 to 4.9-rc2 and noticed that kernel modules fail to
>> load (log below). After investigating for some time I found this thread
>> and apparently this is not still fixed, at least not in Linus' tree.
>>
>> Reverting 784d5699eddc5 fixed the issue for me. As I don't see any fix
>> available (please correct me if I'm wrong) we should just revert that
>> commit until it's properly fixed.
>
> With these two patches together, does it work for you?
>
> http://marc.info/?l=linux-arch&m=147653546809512&w=2
> http://marc.info/?l=linux-kernel&m=147669851906489&w=2
>
> It would be helpful if you could test and let us know, because there seems
> to be a very tiny number of configs and toolchains that causes
> problems.

With these two patches (on top of ath-201610251249 from ath.git, in
practice 4.9-rc2 + latest wireless patches) module loading works again.
If you want you can add my Tested-by:

Tested-by: Kalle Valo <[email protected]>

Can we get these patches to Linus' tree soon? It's annoying to revert
784d5699eddc5 everytime I update my tree.

>> Also note that there's a related fix from Steven:
>>
>> [PATCH] x86: Fix export for mcount and __fentry__
>> https://marc.info/?l=linux-kernel&m=147733572502413
>>
>> For compiling the kernel I'm using Ubuntu 12.04:
>>
>> ii binutils 2.22-6ubuntu1.4 GNU assembler, linker and binary utilities
>> ii gcc 4:4.6.3-1ubuntu5 GNU C compiler
>>
>> The kernel is running on a separate machine with Ubuntu 14.04.
>>
>> [ 110.703414] bluetooth: disagrees about version of symbol __get_user_2
>> [ 110.703416] bluetooth: Unknown symbol __get_user_2 (err -22)
>> [ 110.703429] bluetooth: disagrees about version of symbol __put_user_2
>> [ 110.703430] bluetooth: Unknown symbol __put_user_2 (err -22)
>> [ 110.703579] bluetooth: disagrees about version of symbol __put_user_4
>> [ 110.703580] bluetooth: Unknown symbol __put_user_4 (err -22)
>> [ 110.703669] bluetooth: disagrees about version of symbol __put_user_1
>> [ 110.703670] bluetooth: Unknown symbol __put_user_1 (err -22)
>> [ 110.703688] bluetooth: disagrees about version of symbol mcount
>> [ 110.703689] bluetooth: Unknown symbol mcount (err -22)
>>
>
> I haven't seen that one before. Did you definitely make and install new
> modules?

I'm pretty sure modules are correctly installed as I have used the same
procedure for years: on my workstation I do 'make bindeb-pkg', copy the
.deb to the test laptop and install the deb there. Also once I revert
784d5699eddc5 it starts immeadiately working.

--
Kalle Valo

2016-10-27 14:09:42

by Kalle Valo

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

(Adding Thorsten because of a serious regression and Steven because he
tried to fix something in the same commit)

Nicholas Piggin <[email protected]> writes:

> On Wed, 19 Oct 2016 16:38:14 +0200
> Michal Marek <[email protected]> wrote:
>
>> Dne 18.10.2016 v 03:34 Nicholas Piggin napsal(a):
>> > We should probably just bring all these arch patches through the
>> > kbuild tree.
>> >
>> > I'm sorry for the breakage: I didn't realize it broke the build with
>> > some configs, otherwise I would have given Michal a heads up before
>> > his pull request, and worked to get this stuff in first.
>>
>> It breaks with some binutils versions only (and only with
>> CONFIG_MODVERSIONS=y, of course).
>
> Yeah this seems to be the issue, it apparently slipped past all the
> automated builds. It seems like the existing CRC warnings in the tree
> only trigger in rare circumstances too, so something could be a bit
> fragile there.

I upgraded from 4.8 to 4.9-rc2 and noticed that kernel modules fail to
load (log below). After investigating for some time I found this thread
and apparently this is not still fixed, at least not in Linus' tree.

Reverting 784d5699eddc5 fixed the issue for me. As I don't see any fix
available (please correct me if I'm wrong) we should just revert that
commit until it's properly fixed.

Also note that there's a related fix from Steven:

[PATCH] x86: Fix export for mcount and __fentry__
https://marc.info/?l=linux-kernel&m=147733572502413

For compiling the kernel I'm using Ubuntu 12.04:

ii binutils 2.22-6ubuntu1.4 GNU assembler, linker and binary utilities
ii gcc 4:4.6.3-1ubuntu5 GNU C compiler

The kernel is running on a separate machine with Ubuntu 14.04.

[ 110.703414] bluetooth: disagrees about version of symbol __get_user_2
[ 110.703416] bluetooth: Unknown symbol __get_user_2 (err -22)
[ 110.703429] bluetooth: disagrees about version of symbol __put_user_2
[ 110.703430] bluetooth: Unknown symbol __put_user_2 (err -22)
[ 110.703579] bluetooth: disagrees about version of symbol __put_user_4
[ 110.703580] bluetooth: Unknown symbol __put_user_4 (err -22)
[ 110.703669] bluetooth: disagrees about version of symbol __put_user_1
[ 110.703670] bluetooth: Unknown symbol __put_user_1 (err -22)
[ 110.703688] bluetooth: disagrees about version of symbol mcount
[ 110.703689] bluetooth: Unknown symbol mcount (err -22)

--
Kalle Valo

2016-10-27 14:35:15

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Thu, 27 Oct 2016 16:14:28 +0300
Kalle Valo <[email protected]> wrote:

> Nicholas Piggin <[email protected]> writes:
>
> > On Thu, 27 Oct 2016 11:10:03 +0300
> > Kalle Valo <[email protected]> wrote:
> >
> >> (Adding Thorsten because of a serious regression and Steven because he
> >> tried to fix something in the same commit)
> >>
> >> Nicholas Piggin <[email protected]> writes:
> >>
> >> > On Wed, 19 Oct 2016 16:38:14 +0200
> >> > Michal Marek <[email protected]> wrote:
> >> >
> >> >> Dne 18.10.2016 v 03:34 Nicholas Piggin napsal(a):
> >> >> > We should probably just bring all these arch patches through the
> >> >> > kbuild tree.
> >> >> >
> >> >> > I'm sorry for the breakage: I didn't realize it broke the build with
> >> >> > some configs, otherwise I would have given Michal a heads up before
> >> >> > his pull request, and worked to get this stuff in first.
> >> >>
> >> >> It breaks with some binutils versions only (and only with
> >> >> CONFIG_MODVERSIONS=y, of course).
> >> >
> >> > Yeah this seems to be the issue, it apparently slipped past all the
> >> > automated builds. It seems like the existing CRC warnings in the tree
> >> > only trigger in rare circumstances too, so something could be a bit
> >> > fragile there.
> >>
> >> I upgraded from 4.8 to 4.9-rc2 and noticed that kernel modules fail to
> >> load (log below). After investigating for some time I found this thread
> >> and apparently this is not still fixed, at least not in Linus' tree.
> >>
> >> Reverting 784d5699eddc5 fixed the issue for me. As I don't see any fix
> >> available (please correct me if I'm wrong) we should just revert that
> >> commit until it's properly fixed.
> >
> > With these two patches together, does it work for you?
> >
> > http://marc.info/?l=linux-arch&m=147653546809512&w=2
> > http://marc.info/?l=linux-kernel&m=147669851906489&w=2
> >
> > It would be helpful if you could test and let us know, because there seems
> > to be a very tiny number of configs and toolchains that causes
> > problems.
>
> With these two patches (on top of ath-201610251249 from ath.git, in
> practice 4.9-rc2 + latest wireless patches) module loading works again.
> If you want you can add my Tested-by:
>
> Tested-by: Kalle Valo <[email protected]>

Great, thanks for testing it.

> Can we get these patches to Linus' tree soon? It's annoying to revert
> 784d5699eddc5 everytime I update my tree.

Yes I think it's about ready to merge. Michal is returning from vacation
next week so we should get some progress soon.

> >> Also note that there's a related fix from Steven:
> >>
> >> [PATCH] x86: Fix export for mcount and __fentry__
> >> https://marc.info/?l=linux-kernel&m=147733572502413
> >>
> >> For compiling the kernel I'm using Ubuntu 12.04:
> >>
> >> ii binutils 2.22-6ubuntu1.4 GNU assembler, linker and binary utilities
> >> ii gcc 4:4.6.3-1ubuntu5 GNU C compiler
> >>
> >> The kernel is running on a separate machine with Ubuntu 14.04.
> >>
> >> [ 110.703414] bluetooth: disagrees about version of symbol __get_user_2
> >> [ 110.703416] bluetooth: Unknown symbol __get_user_2 (err -22)
> >> [ 110.703429] bluetooth: disagrees about version of symbol __put_user_2
> >> [ 110.703430] bluetooth: Unknown symbol __put_user_2 (err -22)
> >> [ 110.703579] bluetooth: disagrees about version of symbol __put_user_4
> >> [ 110.703580] bluetooth: Unknown symbol __put_user_4 (err -22)
> >> [ 110.703669] bluetooth: disagrees about version of symbol __put_user_1
> >> [ 110.703670] bluetooth: Unknown symbol __put_user_1 (err -22)
> >> [ 110.703688] bluetooth: disagrees about version of symbol mcount
> >> [ 110.703689] bluetooth: Unknown symbol mcount (err -22)
> >>
> >
> > I haven't seen that one before. Did you definitely make and install new
> > modules?
>
> I'm pretty sure modules are correctly installed as I have used the same
> procedure for years: on my workstation I do 'make bindeb-pkg', copy the
> .deb to the test laptop and install the deb there. Also once I revert
> 784d5699eddc5 it starts immeadiately working.
>

Sure, I was just checking because I've seen several types of failure but
not this one before. Thanks for reporting and testing.

Thanks,
Nick

2016-10-27 14:56:09

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Thu, 27 Oct 2016 11:10:03 +0300
Kalle Valo <[email protected]> wrote:

> (Adding Thorsten because of a serious regression and Steven because he
> tried to fix something in the same commit)
>
> Nicholas Piggin <[email protected]> writes:
>
> > On Wed, 19 Oct 2016 16:38:14 +0200
> > Michal Marek <[email protected]> wrote:
> >
> >> Dne 18.10.2016 v 03:34 Nicholas Piggin napsal(a):
> >> > We should probably just bring all these arch patches through the
> >> > kbuild tree.
> >> >
> >> > I'm sorry for the breakage: I didn't realize it broke the build with
> >> > some configs, otherwise I would have given Michal a heads up before
> >> > his pull request, and worked to get this stuff in first.
> >>
> >> It breaks with some binutils versions only (and only with
> >> CONFIG_MODVERSIONS=y, of course).
> >
> > Yeah this seems to be the issue, it apparently slipped past all the
> > automated builds. It seems like the existing CRC warnings in the tree
> > only trigger in rare circumstances too, so something could be a bit
> > fragile there.
>
> I upgraded from 4.8 to 4.9-rc2 and noticed that kernel modules fail to
> load (log below). After investigating for some time I found this thread
> and apparently this is not still fixed, at least not in Linus' tree.
>
> Reverting 784d5699eddc5 fixed the issue for me. As I don't see any fix
> available (please correct me if I'm wrong) we should just revert that
> commit until it's properly fixed.

With these two patches together, does it work for you?

http://marc.info/?l=linux-arch&m=147653546809512&w=2
http://marc.info/?l=linux-kernel&m=147669851906489&w=2

It would be helpful if you could test and let us know, because there seems
to be a very tiny number of configs and toolchains that causes problems.

>
> Also note that there's a related fix from Steven:
>
> [PATCH] x86: Fix export for mcount and __fentry__
> https://marc.info/?l=linux-kernel&m=147733572502413
>
> For compiling the kernel I'm using Ubuntu 12.04:
>
> ii binutils 2.22-6ubuntu1.4 GNU assembler, linker and binary utilities
> ii gcc 4:4.6.3-1ubuntu5 GNU C compiler
>
> The kernel is running on a separate machine with Ubuntu 14.04.
>
> [ 110.703414] bluetooth: disagrees about version of symbol __get_user_2
> [ 110.703416] bluetooth: Unknown symbol __get_user_2 (err -22)
> [ 110.703429] bluetooth: disagrees about version of symbol __put_user_2
> [ 110.703430] bluetooth: Unknown symbol __put_user_2 (err -22)
> [ 110.703579] bluetooth: disagrees about version of symbol __put_user_4
> [ 110.703580] bluetooth: Unknown symbol __put_user_4 (err -22)
> [ 110.703669] bluetooth: disagrees about version of symbol __put_user_1
> [ 110.703670] bluetooth: Unknown symbol __put_user_1 (err -22)
> [ 110.703688] bluetooth: disagrees about version of symbol mcount
> [ 110.703689] bluetooth: Unknown symbol mcount (err -22)
>

I haven't seen that one before. Did you definitely make and install new
modules?

Thanks,
Nick

2016-10-30 10:51:11

by Thorsten Leemhuis

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On 27.10.2016 10:10, Kalle Valo wrote:
> (Adding Thorsten because of a serious regression and Steven because he
> tried to fix something in the same commit)

Many thx.

I added this report to the list of regressions for Linux 4.9. I'll
watch this thread for further updates on this issue to document progress
in my weekly reports. Please let me know via [email protected]
in case the discussion moves to a different place (bugzilla or another
mail thread for example).

Current status (afaics): Fix available, waiting for Michal to get back
from vacation.

tia! Ciao, Thorsten

> Nicholas Piggin <[email protected]> writes:
>
>> On Wed, 19 Oct 2016 16:38:14 +0200
>> Michal Marek <[email protected]> wrote:
>>
>>> Dne 18.10.2016 v 03:34 Nicholas Piggin napsal(a):
>>>> We should probably just bring all these arch patches through the
>>>> kbuild tree.
>>>>
>>>> I'm sorry for the breakage: I didn't realize it broke the build with
>>>> some configs, otherwise I would have given Michal a heads up before
>>>> his pull request, and worked to get this stuff in first.
>>>
>>> It breaks with some binutils versions only (and only with
>>> CONFIG_MODVERSIONS=y, of course).
>>
>> Yeah this seems to be the issue, it apparently slipped past all the
>> automated builds. It seems like the existing CRC warnings in the tree
>> only trigger in rare circumstances too, so something could be a bit
>> fragile there.
>
> I upgraded from 4.8 to 4.9-rc2 and noticed that kernel modules fail to
> load (log below). After investigating for some time I found this thread
> and apparently this is not still fixed, at least not in Linus' tree.
>
> Reverting 784d5699eddc5 fixed the issue for me. As I don't see any fix
> available (please correct me if I'm wrong) we should just revert that
> commit until it's properly fixed.
>
> Also note that there's a related fix from Steven:
>
> [PATCH] x86: Fix export for mcount and __fentry__
> https://marc.info/?l=linux-kernel&m=147733572502413
>
> For compiling the kernel I'm using Ubuntu 12.04:
>
> ii binutils 2.22-6ubuntu1.4 GNU assembler, linker and binary utilities
> ii gcc 4:4.6.3-1ubuntu5 GNU C compiler
>
> The kernel is running on a separate machine with Ubuntu 14.04.
>
> [ 110.703414] bluetooth: disagrees about version of symbol __get_user_2
> [ 110.703416] bluetooth: Unknown symbol __get_user_2 (err -22)
> [ 110.703429] bluetooth: disagrees about version of symbol __put_user_2
> [ 110.703430] bluetooth: Unknown symbol __put_user_2 (err -22)
> [ 110.703579] bluetooth: disagrees about version of symbol __put_user_4
> [ 110.703580] bluetooth: Unknown symbol __put_user_4 (err -22)
> [ 110.703669] bluetooth: disagrees about version of symbol __put_user_1
> [ 110.703670] bluetooth: Unknown symbol __put_user_1 (err -22)
> [ 110.703688] bluetooth: disagrees about version of symbol mcount
> [ 110.703689] bluetooth: Unknown symbol mcount (err -22)
>

2016-11-01 15:48:56

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On 2016-10-18 03:34, Nicholas Piggin wrote:
> Hi Adam,
>
> Thanks, this is looking good. powerpc will be able to use the generic
> header.
>
> On Tue, 18 Oct 2016 02:16:26 +0200
> Adam Borowski <[email protected]> wrote:
>
>> On Mon, Oct 17, 2016 at 02:22:34PM +0200, Mathieu OTHACEHE wrote:
>>>> +#include <asm/uaccess.h>
>>>> +#include <asm/uaccess.h>
>>>
>>> Included twice.
>>
>> D'oh!
>>
>>>> +#include <asm/string.h>
>>>> +#include <asm/page.h>
>>>> +#include <asm/checksum.h>
>>>> +
>>>> +#include <asm-generic/asm-prototypes.h>
>>>> +
>>>> +#include <asm/page.h>
>>>> +#include <asm/pgtable.h>
>>>> +#include <asm/special_insns.h>
>>>> +#include <asm/preempt.h>
>>>
>>> No <asm/arch_hweight.h> for __sw_hweight32 and __sw_hweight64 ?
>>
>> diff --git a/include/asm-generic/asm-prototypes.h b/include/asm-generic/asm-prototypes.h
>> new file mode 100644
>> index 0000000..df13637
>> --- /dev/null
>> +++ b/include/asm-generic/asm-prototypes.h
>> @@ -0,0 +1,7 @@
>> +#include <linux/bitops.h>
>>
>> ... which has these.
>>
>> Alexey Dobriyan <[email protected]> wrote:
>> } bitops.h is wrong header as well.
>> } Why do you need bitops for bunch of function prototypes?
>>
>> Unless you guys prefer using low-level headers only, that is.
>
> Well you can't use asm/arch_hweight.h in a generic header of course.
> I would suggest just including linux/ variants where practical for
> the asm-generic/asm-prototypes.h header.
>
> We should probably just bring all these arch patches through the
> kbuild tree.

Adam,

are you submitting a new version of your x86 asm-prototypes.h patch?

Thanks,
Michal

2016-11-02 12:11:34

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Tue, Nov 01, 2016 at 04:48:48PM +0100, Michal Marek wrote:
> > Adam Borowski <[email protected]> wrote:
> >
> >> On Mon, Oct 17, 2016 at 02:22:34PM +0200, Mathieu OTHACEHE wrote:
> >>>> +#include <asm/uaccess.h>
> >>>> +#include <asm/uaccess.h>
> >>>
> >>> Included twice.
> >>
> >> D'oh!

This appears to be the only thing to fix, right?

> >>>> +#include <asm/string.h>
> >>>> +#include <asm/page.h>
> >>>> +#include <asm/checksum.h>
> >>>> +
> >>>> +#include <asm-generic/asm-prototypes.h>
> >>>> +
> >>>> +#include <asm/page.h>
> >>>> +#include <asm/pgtable.h>
> >>>> +#include <asm/special_insns.h>
> >>>> +#include <asm/preempt.h>
> >>>
> >>> No <asm/arch_hweight.h> for __sw_hweight32 and __sw_hweight64 ?
> >>
> >> diff --git a/include/asm-generic/asm-prototypes.h b/include/asm-generic/asm-prototypes.h
> >> new file mode 100644
> >> index 0000000..df13637
> >> --- /dev/null
> >> +++ b/include/asm-generic/asm-prototypes.h
> >> @@ -0,0 +1,7 @@
> >> +#include <linux/bitops.h>
> >>
> >> ... which has these.
> >>
> >> Alexey Dobriyan <[email protected]> wrote:
> >> } bitops.h is wrong header as well.
> >> } Why do you need bitops for bunch of function prototypes?
> >>
> >> Unless you guys prefer using low-level headers only, that is.
> >
> > Well you can't use asm/arch_hweight.h in a generic header of course.
> > I would suggest just including linux/ variants where practical for
> > the asm-generic/asm-prototypes.h header.
> >
> > We should probably just bring all these arch patches through the
> > kbuild tree.

I believe inclusion of <linux/bitops.h> is the right thing to do, but if
not, the patch would also need:

extern unsigned int __sw_hweight8(unsigned int w);
extern unsigned int __sw_hweight16(unsigned int w);
extern unsigned int __sw_hweight32(unsigned int w);
extern unsigned long __sw_hweight64(__u64 w);

There's also the issue of mcount/__fentry__, but that's apparently already
dealt with in 5de0a8c, already in mainline.

> are you submitting a new version of your x86 asm-prototypes.h patch?

The update is trivial, but yeah, I can resubmit. If I'm wrong about where
__sw_hweight{8,16,32,64} should come from, please say so.

Meow!
--
A MAP07 (Dead Simple) raspberry tincture recipe: 0.5l 95% alcohol, 1kg
raspberries, 0.4kg sugar; put into a big jar for 1 month. Filter out and
throw away the fruits (can dump them into a cake, etc), let the drink age
at least 3-6 months.

2016-11-02 12:15:10

[permalink] [raw]

Subject: [PATCH] kbuild: provide include/asm/asm-prototypes.h for x86

Nicholas Piggin wrote:
> Architectures will need to have an include/asm/asm-prototypes.h that
> defines or #include<>s C-style prototypes for exported asm functions.
> We can do an asm-generic version for the common ones like memset so
> there's not a lot of pointless duplication there.

Signed-off-by: Adam Borowski <[email protected]>
---
arch/x86/include/asm/asm-prototypes.h | 12 ++++++++++++
include/asm-generic/asm-prototypes.h | 7 +++++++
2 files changed, 19 insertions(+)
create mode 100644 arch/x86/include/asm/asm-prototypes.h
create mode 100644 include/asm-generic/asm-prototypes.h

diff --git a/arch/x86/include/asm/asm-prototypes.h b/arch/x86/include/asm/asm-prototypes.h
new file mode 100644
index 0000000..ae87224
--- /dev/null
+++ b/arch/x86/include/asm/asm-prototypes.h
@@ -0,0 +1,12 @@
+#include <asm/ftrace.h>
+#include <asm/uaccess.h>
+#include <asm/string.h>
+#include <asm/page.h>
+#include <asm/checksum.h>
+
+#include <asm-generic/asm-prototypes.h>
+
+#include <asm/page.h>
+#include <asm/pgtable.h>
+#include <asm/special_insns.h>
+#include <asm/preempt.h>
diff --git a/include/asm-generic/asm-prototypes.h b/include/asm-generic/asm-prototypes.h
new file mode 100644
index 0000000..df13637
--- /dev/null
+++ b/include/asm-generic/asm-prototypes.h
@@ -0,0 +1,7 @@
+#include <linux/bitops.h>
+extern void *__memset(void *, int, __kernel_size_t);
+extern void *__memcpy(void *, const void *, __kernel_size_t);
+extern void *__memmove(void *, const void *, __kernel_size_t);
+extern void *memset(void *, int, __kernel_size_t);
+extern void *memcpy(void *, const void *, __kernel_size_t);
+extern void *memmove(void *, const void *, __kernel_size_t);
--
2.10.2

2016-11-20 13:21:46

[permalink] [raw]

Subject: Re: [PATCH 1/2] kbuild: provide include/asm/asm-prototypes.h for ARM

On Tue, Oct 25, 2016 at 07:32:00PM +1100, Nicholas Piggin wrote:
> On Mon, 24 Oct 2016 17:05:26 +0200
> Arnd Bergmann <[email protected]> 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?

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

2016-11-20 18:32:56

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH 1/2] kbuild: provide include/asm/asm-prototypes.h for ARM

On Sun, Nov 20, 2016 at 5:21 AM, Russell King - ARM Linux
<[email protected]> 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

2016-11-20 19:13:16

[permalink] [raw]

Subject: Re: [PATCH 1/2] kbuild: provide include/asm/asm-prototypes.h for ARM

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
> <[email protected]> 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.

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

2016-11-21 06:11:18

[permalink] [raw]

Subject: Re: [PATCH 1/2] kbuild: provide include/asm/asm-prototypes.h for ARM

On Sun, 20 Nov 2016 19:12:57 +0000
Russell King - ARM Linux <[email protected]> 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
> > <[email protected]> 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

2016-11-21 18:54:59

by Uwe Kleine-König

[permalink] [raw]

Subject: Re: [1/2] kbuild: provide include/asm/asm-prototypes.h for ARM

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 <[email protected]>

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

Attachments:

(No filename) (1.53 kB)
signature.asc (455.00 B)
Download all attachments

2016-11-21 19:14:16

[permalink] [raw]

Subject: Re: [1/2] kbuild: provide include/asm/asm-prototypes.h for ARM

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 <[email protected]>
>
> 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.

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

2016-11-22 01:01:33

[permalink] [raw]

Subject: Re: [1/2] kbuild: provide include/asm/asm-prototypes.h for ARM

On Mon, 21 Nov 2016 19:13:55 +0000
Russell King - ARM Linux <[email protected]> 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 <[email protected]>
> >
> > 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

2016-12-16 19:55:23

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On 10/17/2016, 12:01 PM, Adam Borowski wrote:
> Anyway, here's my stab at x86:

Hi,

what happened to this? I had to apply this to fix 4.9-pae kernel here.

> From db746df65b920591606398b4b244f5b6dc9eea04 Mon Sep 17 00:00:00 2001
> From: Adam Borowski <[email protected]>
> Date: Mon, 17 Oct 2016 11:42:35 +0200
> Subject: [PATCH] kbuild: provide include/asm/asm-prototypes.h for x86
>
> Nicholas Piggin wrote:
>> Architectures will need to have an include/asm/asm-prototypes.h that
>> defines or #include<>s C-style prototypes for exported asm functions.
>> We can do an asm-generic version for the common ones like memset so
>> there's not a lot of pointless duplication there.
>
> Signed-off-by: Adam Borowski <[email protected]>
> ---
> arch/x86/include/asm/asm-prototypes.h | 13 +++++++++++++
> include/asm-generic/asm-prototypes.h | 7 +++++++
> 2 files changed, 20 insertions(+)
> create mode 100644 arch/x86/include/asm/asm-prototypes.h
> create mode 100644 include/asm-generic/asm-prototypes.h
>
> diff --git a/arch/x86/include/asm/asm-prototypes.h b/arch/x86/include/asm/asm-prototypes.h
> new file mode 100644
> index 0000000..072c97c
> --- /dev/null
> +++ b/arch/x86/include/asm/asm-prototypes.h
> @@ -0,0 +1,13 @@
> +#include <asm/ftrace.h>
> +#include <asm/uaccess.h>
> +#include <asm/uaccess.h>
> +#include <asm/string.h>
> +#include <asm/page.h>
> +#include <asm/checksum.h>
> +
> +#include <asm-generic/asm-prototypes.h>
> +
> +#include <asm/page.h>
> +#include <asm/pgtable.h>
> +#include <asm/special_insns.h>
> +#include <asm/preempt.h>
> diff --git a/include/asm-generic/asm-prototypes.h b/include/asm-generic/asm-prototypes.h
> new file mode 100644
> index 0000000..df13637
> --- /dev/null
> +++ b/include/asm-generic/asm-prototypes.h
> @@ -0,0 +1,7 @@
> +#include <linux/bitops.h>
> +extern void *__memset(void *, int, __kernel_size_t);
> +extern void *__memcpy(void *, const void *, __kernel_size_t);
> +extern void *__memmove(void *, const void *, __kernel_size_t);
> +extern void *memset(void *, int, __kernel_size_t);
> +extern void *memcpy(void *, const void *, __kernel_size_t);
> +extern void *memmove(void *, const void *, __kernel_size_t);
>

thanks,
--
js
suse labs

2016-12-16 19:57:14

by Linus Torvalds

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Fri, Dec 16, 2016 at 11:55 AM, Jiri Slaby <[email protected]> wrote:
>
> what happened to this? I had to apply this to fix 4.9-pae kernel here.

Did you actually have to do that?

Because a missing CRC shouldn't be fatal in 4.9.

What was the failure mode?

Linus

2016-12-17 08:57:57

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On 12/16/2016, 08:57 PM, Linus Torvalds wrote:
> On Fri, Dec 16, 2016 at 11:55 AM, Jiri Slaby <[email protected]> wrote:
>>
>> what happened to this? I had to apply this to fix 4.9-pae kernel here.
>
> Did you actually have to do that?

Yes, disk drivers won't load:
[ 2.141973] virtio_pci: disagrees about version of symbol mcount
[ 2.144415] virtio_pci: Unknown symbol mcount (err -22)
[ 2.164547] virtio_pci: disagrees about version of symbol mcount
[ 2.166309] virtio_pci: Unknown symbol mcount (err -22)
[ 2.180651] virtio_pci: disagrees about version of symbol mcount
[ 2.182823] virtio_pci: Unknown symbol mcount (err -22)
[ 2.210943] virtio_pci: disagrees about version of symbol mcount
[ 2.220097] virtio_pci: Unknown symbol mcount (err -22)
[ 2.220173] ata_piix: disagrees about version of symbol mcount
[ 2.220174] ata_piix: Unknown symbol mcount (err -22)

and whole machine gets stuck with systemd waiting for /dev/sd*.

> Because a missing CRC shouldn't be fatal in 4.9.
>
> What was the failure mode?

I am not sure what you mean? The kernel is rpm-ized 4.9 vanilla and this
is the config:
http://kernel.suse.com/cgit/kernel-source/tree/config/i386/default?h=stable

thanks,
--
js
suse labs

2016-12-17 09:39:41

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Sat, Dec 17, 2016 at 09:57:47AM +0100, Jiri Slaby wrote:
> On 12/16/2016, 08:57 PM, Linus Torvalds wrote:
> > On Fri, Dec 16, 2016 at 11:55 AM, Jiri Slaby <[email protected]> wrote:
> >>
> >> what happened to this? I had to apply this to fix 4.9-pae kernel here.
> >
> > Did you actually have to do that?
>
> Yes, disk drivers won't load:
> [ 2.141973] virtio_pci: disagrees about version of symbol mcount
> [ 2.144415] virtio_pci: Unknown symbol mcount (err -22)
> and whole machine gets stuck with systemd waiting for /dev/sd*.
>
> > Because a missing CRC shouldn't be fatal in 4.9.

Most of us get just a scary-looking warning, but whatever the problem is for
you, it's good to hear this patch works around it.

Whatever the long-term solution will be, for 4.10 an updated[1] version of
this fix is on kbuild/kbuild (and kbuild/for-next). I guess we'll bother
stable@ once it is merged.

Note that it handles only x86, there's a bunch of other architectures
affected, alpha m68k s390 sparc ia64 might still need fixing.

Meow!

[1]. Turns out there was a missing symbol on 486; people build-test those
but don't try to actually boot, and even when they do, they don't read
warnings.
--
Autotools hint: to do a zx-spectrum build on a pdp11 host, type:
./configure --host=zx-spectrum --build=pdp11

2016-12-17 23:59:40

by Linus Torvalds

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On Sat, Dec 17, 2016 at 12:57 AM, Jiri Slaby <[email protected]> wrote:
>
> Yes, disk drivers won't load:
> [ 2.141973] virtio_pci: disagrees about version of symbol mcount
> [ 2.144415] virtio_pci: Unknown symbol mcount (err -22)

This makes no sense.

mcount isn't even one of the symbols that the patch by Adam is touching.

There's something else screwed up here. Not to mention that others
don't have your issue.

Do you have some other hacks in this area? Are you testing actual
plain 4.9, or do you (for example) still carry Arnd's patch around
that turned out to not work (reverted by f27c2f69cc8e in my tree)?

Linus

2016-12-18 10:49:35

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On 12/18/2016, 12:59 AM, Linus Torvalds wrote:
> On Sat, Dec 17, 2016 at 12:57 AM, Jiri Slaby <[email protected]> wrote:
>>
>> Yes, disk drivers won't load:
>> [ 2.141973] virtio_pci: disagrees about version of symbol mcount
>> [ 2.144415] virtio_pci: Unknown symbol mcount (err -22)
>
> This makes no sense.
>
> mcount isn't even one of the symbols that the patch by Adam is touching.

asm-prototypes.h in his patch includes asm/ftrace.h, where the function
is declared. That should be enough IIUC scripts/Makefile.build.

> There's something else screwed up here. Not to mention that others
> don't have your issue.

I suppose people don't run i386 kernels or have different config.

> Do you have some other hacks in this area? Are you testing actual
> plain 4.9, or do you (for example) still carry Arnd's patch around
> that turned out to not work (reverted by f27c2f69cc8e in my tree)?

Not at all. This was plain 4.9 packaged by suse -- only rpm-related
fixes. I tried plain 4.9 without rpm right now with the same output:
# insmod soundcore.ko
[ 31.582326] soundcore: disagrees about version of symbol mcount
[ 31.586183] soundcore: Unknown symbol mcount (err -22)
insmod: ERROR: could not insert module soundcore.ko: Invalid parameters

$ git describe @
v4.9
$ git status
HEAD detached at v4.9

thanks,
--
js
suse labs

2016-12-18 11:03:40

by Arend van Spriel

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On 18-12-2016 11:49, Jiri Slaby wrote:
> On 12/18/2016, 12:59 AM, Linus Torvalds wrote:
>> On Sat, Dec 17, 2016 at 12:57 AM, Jiri Slaby <[email protected]> wrote:
>>>
>>> Yes, disk drivers won't load:
>>> [ 2.141973] virtio_pci: disagrees about version of symbol mcount
>>> [ 2.144415] virtio_pci: Unknown symbol mcount (err -22)
>>
>> This makes no sense.
>>
>> mcount isn't even one of the symbols that the patch by Adam is touching.
>
> asm-prototypes.h in his patch includes asm/ftrace.h, where the function
> is declared. That should be enough IIUC scripts/Makefile.build.
>
>> There's something else screwed up here. Not to mention that others
>> don't have your issue.
>
> I suppose people don't run i386 kernels or have different config.
>
>> Do you have some other hacks in this area? Are you testing actual
>> plain 4.9, or do you (for example) still carry Arnd's patch around
>> that turned out to not work (reverted by f27c2f69cc8e in my tree)?
>
> Not at all. This was plain 4.9 packaged by suse -- only rpm-related
> fixes. I tried plain 4.9 without rpm right now with the same output:
> # insmod soundcore.ko
> [ 31.582326] soundcore: disagrees about version of symbol mcount
> [ 31.586183] soundcore: Unknown symbol mcount (err -22)
> insmod: ERROR: could not insert module soundcore.ko: Invalid parameters

I hit an mcount issue a while back (years?) which was due to building a
driver with gcc v4.x while kernel was built using gcc v4.y. Not claiming
that is your issue though.

Regards,
Arend

2016-12-18 13:27:56

by Nikolay Borisov

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On 18.12.2016 13:03, Arend Van Spriel wrote:
> On 18-12-2016 11:49, Jiri Slaby wrote:
>> On 12/18/2016, 12:59 AM, Linus Torvalds wrote:
>>> On Sat, Dec 17, 2016 at 12:57 AM, Jiri Slaby <[email protected]> wrote:
>>>>
>>>> Yes, disk drivers won't load:
>>>> [ 2.141973] virtio_pci: disagrees about version of symbol mcount
>>>> [ 2.144415] virtio_pci: Unknown symbol mcount (err -22)
>>>
>>> This makes no sense.
>>>
>>> mcount isn't even one of the symbols that the patch by Adam is touching.
>>
>> asm-prototypes.h in his patch includes asm/ftrace.h, where the function
>> is declared. That should be enough IIUC scripts/Makefile.build.
>>
>>> There's something else screwed up here. Not to mention that others
>>> don't have your issue.
>>
>> I suppose people don't run i386 kernels or have different config.
>>
>>> Do you have some other hacks in this area? Are you testing actual
>>> plain 4.9, or do you (for example) still carry Arnd's patch around
>>> that turned out to not work (reverted by f27c2f69cc8e in my tree)?
>>
>> Not at all. This was plain 4.9 packaged by suse -- only rpm-related
>> fixes. I tried plain 4.9 without rpm right now with the same output:
>> # insmod soundcore.ko
>> [ 31.582326] soundcore: disagrees about version of symbol mcount
>> [ 31.586183] soundcore: Unknown symbol mcount (err -22)
>> insmod: ERROR: could not insert module soundcore.ko: Invalid parameters
>
> I hit an mcount issue a while back (years?) which was due to building a
> driver with gcc v4.x while kernel was built using gcc v4.y. Not claiming
> that is your issue though.

I've usually had the same thing happen to me if things were compiled
with different gcc versions . Essentially in newer gcc (starting with
4.6 I believe) CC_USING_FENTRY is defined, meaning that there is no
mcount() symbol but rather __fentry__. This is the likely problem here.

>
> Regards,
> Arend
>

2016-12-18 14:45:06

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On 12/18/2016, 02:27 PM, Nikolay Borisov wrote:
> This is the likely problem here.

No, it is not. How could a rpm be built with two compilers?

Moreover, with some modules, __put_user_1 and others are reported
instead of mcount.

--
js
suse labs

2016-12-18 14:54:15

by Nikolay Borisov

[permalink] [raw]

Subject: Re: [GIT PULL] kbuild changes for v4.9-rc1

On 18.12.2016 16:45, Jiri Slaby wrote:
> Moreover, with some modules, __put_user_1 and others are reported
> instead of mcount.

nm vmlinux | grep __fentry__
nm vmlinux | grep mcount

What do these report ? I bet you that in your vmlinux the first one
would return something like :

ffffffff822f1810 T __fentry__
ffffffff827fdc20 r __kcrctab___fentry__
ffffffff82809461 r __kstrtab___fentry__
ffffffff827e6c20 R __ksymtab___fentry__
and nothing for the second. Whereas doing nm on the module in question
would give nothing for __fentry__ and something like: U mcount

2016-12-18 15:08:17