| Message ID | 20160209143554.GA9332@leverpostej |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 09, 2016 at 02:35:54PM +0000, Mark Rutland wrote: > On Tue, Feb 09, 2016 at 02:48:26PM +0100, Geert Uytterhoeven wrote: > > On Tue, Feb 9, 2016 at 1:00 PM, Mark Rutland <mark.rutland@arm.com> wrote: > > > On Tue, Feb 09, 2016 at 01:04:28PM +0530, Sudip Mukherjee wrote: > > >> On Tue, Feb 09, 2016 at 04:41:04PM +1100, Stephen Rothwell wrote: > > >> > Changes since 20160208: > > >> > > >> tilepro, tilegx, mips defconfig build fails with the error: > > >> ../include/asm-generic/fixmap.h: In function '__set_fixmap_offset': > > >> ../include/asm-generic/fixmap.h:77:2: error: implicit declaration of > > >> function '__set_fixmap' [-Werror=implicit-function-declaration] > > >> > > >> caused by: > > >> commit ac4c0ac73485 ("asm-generic: make __set_fixmap_offset a static inline") > > >> > > >> Reverting the commit fixes the issue. > > > > > > Sorry about this. > > > > > > Is seems any arch without its own __set_fixmap may be adversely > > > affected. > > > > > > I can't easily stub __set_fixmap as it's not implemented as a macro. > > > > But you can add a forward declaration? > > Good point. That seems to work (tested on arm64, mips, tilegx). > > Arnd, Catalin, happy with the bloew fixup to the original patch? It looks fine to me. But I'll wait for Arnd's ack before pushing it. Thanks. -- Catalin
On Tuesday 09 February 2016 14:35:54 Mark Rutland wrote: > diff --git a/include/asm-generic/fixmap.h b/include/asm-generic/fixmap.h > index f9c27b6..e5255ff 100644 > --- a/include/asm-generic/fixmap.h > +++ b/include/asm-generic/fixmap.h > @@ -69,6 +69,8 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr) > __set_fixmap(idx, 0, FIXMAP_PAGE_CLEAR) > #endif > > +void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot); > + > /* Return a pointer with offset calculated */ > static inline unsigned long __set_fixmap_offset(enum fixed_addresses idx, > phys_addr_t phys, > I think there is a conflicting declaration in arch/x86/include/asm/paravirt.h: static inline void __set_fixmap(unsigned /* enum fixed_addresses */ idx, phys_addr_t phys, pgprot_t flags) { pv_mmu_ops.set_fixmap(idx, phys, flags); } Can you test on x86 with CONFIG_PARAVIRT set? Arnd
On Tue, Feb 09, 2016 at 04:08:03PM +0100, Arnd Bergmann wrote: > On Tuesday 09 February 2016 14:35:54 Mark Rutland wrote: > > diff --git a/include/asm-generic/fixmap.h b/include/asm-generic/fixmap.h > > index f9c27b6..e5255ff 100644 > > --- a/include/asm-generic/fixmap.h > > +++ b/include/asm-generic/fixmap.h > > @@ -69,6 +69,8 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr) > > __set_fixmap(idx, 0, FIXMAP_PAGE_CLEAR) > > #endif > > > > +void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot); > > + > > /* Return a pointer with offset calculated */ > > static inline unsigned long __set_fixmap_offset(enum fixed_addresses idx, > > phys_addr_t phys, > > > > > I think there is a conflicting declaration in arch/x86/include/asm/paravirt.h: > > static inline void __set_fixmap(unsigned /* enum fixed_addresses */ idx, > phys_addr_t phys, pgprot_t flags) > { > pv_mmu_ops.set_fixmap(idx, phys, flags); > } > > Can you test on x86 with CONFIG_PARAVIRT set? That builds fine for me atop of for-next/pgtable, both 64-bit and 32-bit. GCC seems to treat enum fixed_addresses the same as unsigned. Only if I change the type of idx in fixmap.h (e.g. to char) do I get a conflict against paravirt.h Mark.
On Tuesday 09 February 2016 16:01:18 Mark Rutland wrote: > That builds fine for me atop of for-next/pgtable, both 64-bit and > 32-bit. > > GCC seems to treat enum fixed_addresses the same as unsigned. Only if I > change the type of idx in fixmap.h (e.g. to char) do I get a conflict > against paravirt.h Interesting. The patch seems fine then: Acked-by: Arnd Bergmann <arnd@arndb.de>
Hi Mark,
[auto build test ERROR on next-20160209]
[also build test ERROR on v4.5-rc3]
[cannot apply to v4.5-rc3 v4.5-rc2 v4.5-rc1]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Mark-Rutland/asm-generic-Fix-build-when-__set_fixmap-is-absent/20160209-223916
config: um-alldefconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=um
All errors (new ones prefixed by >>):
In file included from arch/um/include/asm/fixmap.h:54:0,
from arch/um/include/asm/pgtable.h:11,
from include/linux/mm.h:67,
from include/linux/ring_buffer.h:5,
from include/linux/trace_events.h:5,
from include/trace/syscall.h:6,
from include/linux/syscalls.h:81,
from init/main.c:18:
>> include/asm-generic/fixmap.h:72:6: error: conflicting types for '__set_fixmap'
void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot);
^
In file included from arch/um/include/asm/pgtable.h:11:0,
from include/linux/mm.h:67,
from include/linux/ring_buffer.h:5,
from include/linux/trace_events.h:5,
from include/trace/syscall.h:6,
from include/linux/syscalls.h:81,
from init/main.c:18:
arch/um/include/asm/fixmap.h:39:13: note: previous declaration of '__set_fixmap' was here
extern void __set_fixmap (enum fixed_addresses idx,
^
vim +/__set_fixmap +72 include/asm-generic/fixmap.h
66
67 #ifndef clear_fixmap
68 #define clear_fixmap(idx) \
69 __set_fixmap(idx, 0, FIXMAP_PAGE_CLEAR)
70 #endif
71
> 72 void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot);
73
74 /* Return a pointer with offset calculated */
75 static inline unsigned long __set_fixmap_offset(enum fixed_addresses idx,
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
> >> include/asm-generic/fixmap.h:72:6: error: conflicting types for '__set_fixmap' > void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot); > ^ > In file included from arch/um/include/asm/pgtable.h:11:0, > from include/linux/mm.h:67, > from include/linux/ring_buffer.h:5, > from include/linux/trace_events.h:5, > from include/trace/syscall.h:6, > from include/linux/syscalls.h:81, > from init/main.c:18: > arch/um/include/asm/fixmap.h:39:13: note: previous declaration of '__set_fixmap' was here > extern void __set_fixmap (enum fixed_addresses idx, > ^ The conflict is the type of 'phys'. In arch/um that's an unsigned long rather than a phys_addr_t as it is elsewhere. If I convert that to a phys_addr_t the build goes along happily. Should we change set_fixmap_offset back to a macro function for now, or is it simple/correct to change arch/um to use phys_addr_t in __set_fixmap? Thanks, Mark.
On Tuesday 09 February 2016 16:33:34 Mark Rutland wrote: > > >> include/asm-generic/fixmap.h:72:6: error: conflicting types for '__set_fixmap' > > void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot); > > ^ > > In file included from arch/um/include/asm/pgtable.h:11:0, > > from include/linux/mm.h:67, > > from include/linux/ring_buffer.h:5, > > from include/linux/trace_events.h:5, > > from include/trace/syscall.h:6, > > from include/linux/syscalls.h:81, > > from init/main.c:18: > > arch/um/include/asm/fixmap.h:39:13: note: previous declaration of '__set_fixmap' was here > > extern void __set_fixmap (enum fixed_addresses idx, > > ^ > > The conflict is the type of 'phys'. In arch/um that's an unsigned long > rather than a phys_addr_t as it is elsewhere. > > If I convert that to a phys_addr_t the build goes along happily. > > Should we change set_fixmap_offset back to a macro function for now, or > is it simple/correct to change arch/um to use phys_addr_t in > __set_fixmap? > I'd vote for using phys_addr_t in arch/um. Arnd
On Tue, Feb 09, 2016 at 04:33:34PM +0000, Mark Rutland wrote: > > >> include/asm-generic/fixmap.h:72:6: error: conflicting types for '__set_fixmap' > > void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot); > > ^ > > In file included from arch/um/include/asm/pgtable.h:11:0, > > from include/linux/mm.h:67, > > from include/linux/ring_buffer.h:5, > > from include/linux/trace_events.h:5, > > from include/trace/syscall.h:6, > > from include/linux/syscalls.h:81, > > from init/main.c:18: > > arch/um/include/asm/fixmap.h:39:13: note: previous declaration of '__set_fixmap' was here > > extern void __set_fixmap (enum fixed_addresses idx, > > ^ > > The conflict is the type of 'phys'. In arch/um that's an unsigned long > rather than a phys_addr_t as it is elsewhere. At a quick grep, we also have: arch/sh/include/asm/fixmap.h arch/sh/mm/init.c arch/sh/mm/nommu.c > If I convert that to a phys_addr_t the build goes along happily. > > Should we change set_fixmap_offset back to a macro function for now, or > is it simple/correct to change arch/um to use phys_addr_t in > __set_fixmap? And sh. I prefer the static inline, though there is more effort needed to test and get acks ;) (I really don't mind either way). -- Catalin
On Tue, Feb 09, 2016 at 04:52:34PM +0000, Catalin Marinas wrote: > On Tue, Feb 09, 2016 at 04:33:34PM +0000, Mark Rutland wrote: > > > >> include/asm-generic/fixmap.h:72:6: error: conflicting types for '__set_fixmap' > > > void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot); > > > ^ > > > In file included from arch/um/include/asm/pgtable.h:11:0, > > > from include/linux/mm.h:67, > > > from include/linux/ring_buffer.h:5, > > > from include/linux/trace_events.h:5, > > > from include/trace/syscall.h:6, > > > from include/linux/syscalls.h:81, > > > from init/main.c:18: > > > arch/um/include/asm/fixmap.h:39:13: note: previous declaration of '__set_fixmap' was here > > > extern void __set_fixmap (enum fixed_addresses idx, > > > ^ > > > > The conflict is the type of 'phys'. In arch/um that's an unsigned long > > rather than a phys_addr_t as it is elsewhere. > > At a quick grep, we also have: > > arch/sh/include/asm/fixmap.h > arch/sh/mm/init.c > arch/sh/mm/nommu.c > > > If I convert that to a phys_addr_t the build goes along happily. > > > > Should we change set_fixmap_offset back to a macro function for now, or > > is it simple/correct to change arch/um to use phys_addr_t in > > __set_fixmap? > > And sh. I prefer the static inline, though there is more effort needed > to test and get acks ;) (I really don't mind either way). I would also prefer to make this a static inline, but it looks like we need to sort out some cross-architecture cleanup first. I'm happy to have a go at that. In the meantime, so as to allow linux-next to build, and to save us from merge hell, let's follow the usual idiom and hope that underscores will protect us. Hopefully this is the last time I ask this today: Arnd, Catalin, are you happy with the below patch? Mark. ---->8---- From 6d283603d18071690dc138e4a0591a445a1d1e30 Mon Sep 17 00:00:00 2001 From: Mark Rutland <mark.rutland@arm.com> Date: Tue, 9 Feb 2016 17:08:26 +0000 Subject: [PATCH] asm-generic: make __set_fixmap_offset a macro again Turning __set_fixmap_offset into a static inline breaks the build for several architectures. Fixing this properly requires updates to a number of architectures to make them agree on the prototype of __set_fixmap. For the timebeing, restore __set_fixmap_offset to its prior state as a macro function, reverting commit ac4c0ac73485867c ("asm-generic: make __set_fixmap_offset a static inline"). To avoid the original issue with namespace clashes, 'addr' is prefixed with a liberal sprinking of underscores. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Catalin Marinas <catalin.marinas@arm.com> --- include/asm-generic/fixmap.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) -- 1.9.1diff --git a/include/asm-generic/fixmap.h b/include/asm-generic/fixmap.h index f9c27b6..827e4d3 100644 --- a/include/asm-generic/fixmap.h +++ b/include/asm-generic/fixmap.h @@ -70,13 +70,13 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr) #endif /* Return a pointer with offset calculated */ -static inline unsigned long __set_fixmap_offset(enum fixed_addresses idx, - phys_addr_t phys, - pgprot_t flags) -{ - __set_fixmap(idx, phys, flags); - return fix_to_virt(idx) + (phys & (PAGE_SIZE - 1)); -} +#define __set_fixmap_offset(idx, phys, flags) \ +({ \ + unsigned long ________addr; \ + __set_fixmap(idx, phys, flags); \ + ________addr = fix_to_virt(idx) + ((phys) & (PAGE_SIZE - 1)); \ + ________addr; \ +}) #define set_fixmap_offset(idx, phys) \ __set_fixmap_offset(idx, phys, FIXMAP_PAGE_NORMAL)
diff --git a/include/asm-generic/fixmap.h b/include/asm-generic/fixmap.h index f9c27b6..e5255ff 100644 --- a/include/asm-generic/fixmap.h +++ b/include/asm-generic/fixmap.h @@ -69,6 +69,8 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr) __set_fixmap(idx, 0, FIXMAP_PAGE_CLEAR) #endif +void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot); + /* Return a pointer with offset calculated */ static inline unsigned long __set_fixmap_offset(enum fixed_addresses idx, phys_addr_t phys,