linux-next: Tree for Feb 9

Message ID 20160209143554.GA9332@leverpostej
State New
Headers show

Commit Message

Mark Rutland Feb. 9, 2016, 2:35 p.m.
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?

Mark.

---->8----
From e53a0fa34689cca13846475fb5a7cb4c7da87384 Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>

Date: Tue, 9 Feb 2016 14:27:36 +0000
Subject: [PATCH] asm-generic: Fix build when __set_fixmap is absent

Commit ac4c0ac73485 ("asm-generic: make __set_fixmap_offset a static
inline") relied on the architecture code to define a __set_fixmap
function, even if unused. This broke the build for architectures which
do not implement __set_fixmap:

../include/asm-generic/fixmap.h:Infunction'__set_fixmap_offset':
../include/asm-generic/fixmap.h:77:2:error:implicitdeclarationof
function'__set_fixmap'[-Werror=implicit-function-declaration]

As we only require the prototype to be present, add this to
asm-generic/fixmap.h, preventing build failures on these architectures.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>

Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Jeremy Linton <jeremy.linton@arm.com>
Cc: Laura Abbott <labbott@fedoraproject.org>
---
 include/asm-generic/fixmap.h | 2 ++
 1 file changed, 2 insertions(+)

-- 
1.9.1

Comments

Catalin Marinas Feb. 9, 2016, 2:38 p.m. | #1
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
Arnd Bergmann Feb. 9, 2016, 3:08 p.m. | #2
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
Mark Rutland Feb. 9, 2016, 4:01 p.m. | #3
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.
Arnd Bergmann Feb. 9, 2016, 4:08 p.m. | #4
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>
kernel test robot Feb. 9, 2016, 4:13 p.m. | #5
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
Mark Rutland Feb. 9, 2016, 4:33 p.m. | #6
> >> 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.
Arnd Bergmann Feb. 9, 2016, 4:45 p.m. | #7
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
Catalin Marinas Feb. 9, 2016, 4:52 p.m. | #8
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
Mark Rutland Feb. 9, 2016, 5:21 p.m. | #9
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)

Patch

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,