Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Jason Gunthorpe]

Hello Santosh,

Testing v3.13-rc1 I see build breakage from this patch:

commit ca5a45c06cd4764fb8510740f7fc550d9a0208d4
Author: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date:   Wed Jul 31 12:44:41 2013 -0400

    ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions
    
    Fix remainder types used when converting back and forth between
    physical and virtual addresses.
    
    Cc: Russell King <linux@arm.linux.org.uk>
    
    Acked-by: Nicolas Pitre <nico@linaro.org>
    Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>


In file included from /scratchl/jgg/3.13/linux/arch/arm/include/asm/page.h:163:0,
                 from /scratchl/jgg/3.13/linux/include/linux/mm_types.h:16,
                 from /scratchl/jgg/3.13/linux/include/linux/sched.h:24,
                 from /scratchl/jgg/3.13/linux/arch/arm/kernel/asm-offsets.c:13:
/scratchl/jgg/3.13/linux/arch/arm/include/asm/memory.h: In function '__virt_to_phys':
/scratchl/jgg/3.13/linux/arch/arm/include/asm/memory.h:244:40: error: 'PHYS_OFFSET' undeclared (first use in this function)
/scratchl/jgg/3.13/linux/arch/arm/include/asm/memory.h:244:40: note: each undeclared identifier is reported only once for each function it appears in
/scratchl/jgg/3.13/linux/arch/arm/include/asm/memory.h: In function '__phys_to_virt':
/scratchl/jgg/3.13/linux/arch/arm/include/asm/memory.h:249:13: error: 'PHYS_OFFSET' undeclared (first use in this function)

My config doesn't set CONFIG_ARM_PATCH_PHYS_VIRT, which means
PHYS_OFFSET is not defined at this point:

static inline phys_addr_t __virt_to_phys(unsigned long x)
{
        return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
}

The definition my config uses follows this path:

#ifndef PHYS_OFFSET
#ifdef PLAT_PHYS_OFFSET
#define PHYS_OFFSET     PLAT_PHYS_OFFSET
#else
#define PHYS_OFFSET     UL(CONFIG_PHYS_OFFSET)
#endif
#endif

Which is after your new inlines..

An elegant fix wasn't obvious to me :)

Regards,
Jason

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Russell King - ARM Linux]

On Mon, Nov 25, 2013 at 03:36:36PM -0700, Jason Gunthorpe wrote:
> Which is after your new inlines..
> 
> An elegant fix wasn't obvious to me :)

Same here... it's all rather complicated because of all those ifdefs.
Nothing easy comes to mind about how to fix this one.  I'll look into
it at some point.

If not, it's going to have to be a revert - or we bite the bullet and
start killing off some of these Kconfig options such as making
ARM_PATCH_PHYS_VIRT mandatory.

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Jason Gunthorpe]

On Mon, Nov 25, 2013 at 11:20:03PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 25, 2013 at 03:36:36PM -0700, Jason Gunthorpe wrote:
> > Which is after your new inlines..
> > 
> > An elegant fix wasn't obvious to me :)
> 
> Same here... it's all rather complicated because of all those ifdefs.
> Nothing easy comes to mind about how to fix this one.  I'll look into
> it at some point.
> 
> If not, it's going to have to be a revert - or we bite the bullet and
> start killing off some of these Kconfig options such as making
> ARM_PATCH_PHYS_VIRT mandatory.

This is enough to fix the build:

diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index eaf428c..b886eba 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -239,15 +239,8 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
 
 #else
 
-static inline phys_addr_t __virt_to_phys(unsigned long x)
-{
-       return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
-}
-
-static inline unsigned long __phys_to_virt(phys_addr_t x)
-{
-       return x - PHYS_OFFSET + PAGE_OFFSET;
-}
+#define __virt_to_phys(x)      ((phys_addr_t)(x) - PAGE_OFFSET + PHYS_OFFSET)
+#define __phys_to_virt(x)      ((unsigned long)((phys_addr_t)(x) - PHYS_OFFSET + PAGE_OFFSET))

 #endif
 #endif

I don't seen an obvious major downside to loosing the inline, static
checking will have to be done with the ARM_PATCH_PHYS_VIRT branch..

I'm working on runtime testing this, let me know if you want a proper
patch.

Regards,
Jason

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Russell King - ARM Linux]

On Mon, Nov 25, 2013 at 11:20:03PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 25, 2013 at 03:36:36PM -0700, Jason Gunthorpe wrote:
> > Which is after your new inlines..
> > 
> > An elegant fix wasn't obvious to me :)
> 
> Same here... it's all rather complicated because of all those ifdefs.
> Nothing easy comes to mind about how to fix this one.  I'll look into
> it at some point.
> 
> If not, it's going to have to be a revert - or we bite the bullet and
> start killing off some of these Kconfig options such as making
> ARM_PATCH_PHYS_VIRT mandatory.

Actually, I think Nicolas' commit:

1b9f95f8ade9efc2bd49f0e7b9dc61a038ac3eef

was wrong to remove PLAT_PHYS_OFFSET.  So, partially undoing Nicolas'
patch, and reordering things a little, we end up with this, which if
it weren't for the comment would be 5 lines shorter!

 arch/arm/include/asm/memory.h |   23 ++++++++++++-----------
 arch/arm/kernel/head-nommu.S  |    4 ++--
 arch/arm/kernel/head.S        |    2 +-
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index 9ecccc865046..c7e0d708e911 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -157,6 +157,16 @@
 #endif
 #define ARCH_PGD_MASK		((1 << ARCH_PGD_SHIFT) - 1)
 
+/*
+ * PLAT_PHYS_OFFSET is the offset (from zero) of the start of physical
+ * memory.  This is used for XIP and NoMMU kernels, or by kernels which
+ * have their own mach/memory.h.  Assembly code must always use
+ * PLAT_PHYS_OFFSET and not PHYS_OFFSET.
+ */
+#ifndef PLAT_PHYS_OFFSET
+#define PLAT_PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
+#endif
+
 #ifndef __ASSEMBLY__
 
 /*
@@ -239,6 +249,8 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
 
 #else
 
+#define PHYS_OFFSET	PLAT_PHYS_OFFSET
+
 static inline phys_addr_t __virt_to_phys(unsigned long x)
 {
 	return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
@@ -251,17 +263,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
 
 #endif
 #endif
-#endif /* __ASSEMBLY__ */
-
-#ifndef PHYS_OFFSET
-#ifdef PLAT_PHYS_OFFSET
-#define PHYS_OFFSET	PLAT_PHYS_OFFSET
-#else
-#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
-#endif
-#endif
-
-#ifndef __ASSEMBLY__
 
 /*
  * PFNs are used to describe any physical page; this means
diff --git a/arch/arm/kernel/head-nommu.S b/arch/arm/kernel/head-nommu.S
index 14235ba64a90..716249cc2ee1 100644
--- a/arch/arm/kernel/head-nommu.S
+++ b/arch/arm/kernel/head-nommu.S
@@ -68,7 +68,7 @@ ENTRY(stext)
 
 #ifdef CONFIG_ARM_MPU
 	/* Calculate the size of a region covering just the kernel */
-	ldr	r5, =PHYS_OFFSET		@ Region start: PHYS_OFFSET
+	ldr	r5, =PLAT_PHYS_OFFSET		@ Region start: PHYS_OFFSET
 	ldr     r6, =(_end)			@ Cover whole kernel
 	sub	r6, r6, r5			@ Minimum size of region to map
 	clz	r6, r6				@ Region size must be 2^N...
@@ -213,7 +213,7 @@ ENTRY(__setup_mpu)
 	set_region_nr r0, #MPU_RAM_REGION
 	isb
 	/* Full access from PL0, PL1, shared for CONFIG_SMP, cacheable */
-	ldr	r0, =PHYS_OFFSET		@ RAM starts at PHYS_OFFSET
+	ldr	r0, =PLAT_PHYS_OFFSET		@ RAM starts@PHYS_OFFSET
 	ldr	r5,=(MPU_AP_PL1RW_PL0RW | MPU_RGN_NORMAL)
 
 	setup_region r0, r5, r6, MPU_DATA_SIDE	@ PHYS_OFFSET, shared, enabled
diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index 11d59b32fb8d..32f317e5828a 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -110,7 +110,7 @@ ENTRY(stext)
 	sub	r4, r3, r4			@ (PHYS_OFFSET - PAGE_OFFSET)
 	add	r8, r8, r4			@ PHYS_OFFSET
 #else
-	ldr	r8, =PHYS_OFFSET		@ always constant in this case
+	ldr	r8, =PLAT_PHYS_OFFSET		@ always constant in this case
 #endif
 
 	/*

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Russell King - ARM Linux]

On Mon, Nov 25, 2013 at 04:34:58PM -0700, Jason Gunthorpe wrote:
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index eaf428c..b886eba 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -239,15 +239,8 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
>  
>  #else
>  
> -static inline phys_addr_t __virt_to_phys(unsigned long x)
> -{
> -       return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
> -}
> -
> -static inline unsigned long __phys_to_virt(phys_addr_t x)
> -{
> -       return x - PHYS_OFFSET + PAGE_OFFSET;
> -}
> +#define __virt_to_phys(x)      ((phys_addr_t)(x) - PAGE_OFFSET + PHYS_OFFSET)
> +#define __phys_to_virt(x)      ((unsigned long)((phys_addr_t)(x) - PHYS_OFFSET + PAGE_OFFSET))
> 
>  #endif
>  #endif
> 
> I don't seen an obvious major downside to loosing the inline, static
> checking will have to be done with the ARM_PATCH_PHYS_VIRT branch..

Well, at this point I'm seriously thinking that killing the __ versions
of this is the right thing to do: the original idea was that the __
versions were the underlying mathematical conversions, with all the
casting kept out of them - thereby making it easy for them to be
overridden without introducing subtle bugs.

That's been lost now with all the casting, so there's little point them
really existing.

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Santosh Shilimkar]

On Monday 25 November 2013 06:39 PM, Russell King - ARM Linux wrote:
> On Mon, Nov 25, 2013 at 04:34:58PM -0700, Jason Gunthorpe wrote:
>> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
>> index eaf428c..b886eba 100644
>> --- a/arch/arm/include/asm/memory.h
>> +++ b/arch/arm/include/asm/memory.h
>> @@ -239,15 +239,8 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
>>  
>>  #else
>>  
>> -static inline phys_addr_t __virt_to_phys(unsigned long x)
>> -{
>> -       return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
>> -}
>> -
>> -static inline unsigned long __phys_to_virt(phys_addr_t x)
>> -{
>> -       return x - PHYS_OFFSET + PAGE_OFFSET;
>> -}
>> +#define __virt_to_phys(x)      ((phys_addr_t)(x) - PAGE_OFFSET + PHYS_OFFSET)
>> +#define __phys_to_virt(x)      ((unsigned long)((phys_addr_t)(x) - PHYS_OFFSET + PAGE_OFFSET))
>>
>>  #endif
>>  #endif
>>
>> I don't seen an obvious major downside to loosing the inline, static
>> checking will have to be done with the ARM_PATCH_PHYS_VIRT branch..
> 
> Well, at this point I'm seriously thinking that killing the __ versions
> of this is the right thing to do: the original idea was that the __
> versions were the underlying mathematical conversions, with all the
> casting kept out of them - thereby making it easy for them to be
> overridden without introducing subtle bugs.
> 
> That's been lost now with all the casting, so there's little point them
> really existing.
> 
Just saw the thread. I was assuming that ARM_PATCH_PHYS_VIRT is always
enabled when the PLAT_PHYS_OFFSET was killed. Sorry for the build breakage.
NoMMU and machines with mach/memory.h still don't have ARM_PATCH_PHYS_VIRT
enabled so they would break.

Looks like you have proposed already a patch with PLAT_PHYS_OFFSET
which should help the case. Thanks.

Regards,
Santosh

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Nicolas Pitre]

On Mon, 25 Nov 2013, Russell King - ARM Linux wrote:

> On Mon, Nov 25, 2013 at 11:20:03PM +0000, Russell King - ARM Linux wrote:
> > On Mon, Nov 25, 2013 at 03:36:36PM -0700, Jason Gunthorpe wrote:
> > > Which is after your new inlines..
> > > 
> > > An elegant fix wasn't obvious to me :)
> > 
> > Same here... it's all rather complicated because of all those ifdefs.
> > Nothing easy comes to mind about how to fix this one.  I'll look into
> > it at some point.
> > 
> > If not, it's going to have to be a revert - or we bite the bullet and
> > start killing off some of these Kconfig options such as making
> > ARM_PATCH_PHYS_VIRT mandatory.
> 
> Actually, I think Nicolas' commit:
> 
> 1b9f95f8ade9efc2bd49f0e7b9dc61a038ac3eef
> 
> was wrong to remove PLAT_PHYS_OFFSET.  So, partially undoing Nicolas'
> patch, and reordering things a little, we end up with this, which if
> it weren't for the comment would be 5 lines shorter!
[...]

What about simply doing the following instead, which I'm sure used to 
work properly at some point:

diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index 9ecccc8650..2b8b8d3236 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
 
 #else
 
+#ifndef PHYS_OFFSET
+#ifdef PLAT_PHYS_OFFSET
+#define PHYS_OFFSET	PLAT_PHYS_OFFSET
+#else
+#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
+#endif
+#endif
+
 static inline phys_addr_t __virt_to_phys(unsigned long x)
 {
 	return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
@@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
 #endif
 #endif /* __ASSEMBLY__ */
 
-#ifndef PHYS_OFFSET
-#ifdef PLAT_PHYS_OFFSET
-#define PHYS_OFFSET	PLAT_PHYS_OFFSET
-#else
-#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
-#endif
-#endif
-
 #ifndef __ASSEMBLY__
 
 /*

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Russell King - ARM Linux]

On Mon, Nov 25, 2013 at 10:56:25PM -0500, Nicolas Pitre wrote:
> What about simply doing the following instead, which I'm sure used to 
> work properly at some point:
> 
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index 9ecccc8650..2b8b8d3236 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
>  
>  #else
>  
> +#ifndef PHYS_OFFSET
> +#ifdef PLAT_PHYS_OFFSET
> +#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> +#else
> +#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> +#endif
> +#endif
> +
>  static inline phys_addr_t __virt_to_phys(unsigned long x)
>  {
>  	return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
> @@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
>  #endif
>  #endif /* __ASSEMBLY__ */
>  
> -#ifndef PHYS_OFFSET
> -#ifdef PLAT_PHYS_OFFSET
> -#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> -#else
> -#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> -#endif
> -#endif

And that makes PHYS_OFFSET undefined to assembly code - and we have
references to it from said code.

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Nicolas Pitre]

On Tue, 26 Nov 2013, Russell King - ARM Linux wrote:

> On Mon, Nov 25, 2013 at 10:56:25PM -0500, Nicolas Pitre wrote:
> > What about simply doing the following instead, which I'm sure used to 
> > work properly at some point:
> > 
> > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > index 9ecccc8650..2b8b8d3236 100644
> > --- a/arch/arm/include/asm/memory.h
> > +++ b/arch/arm/include/asm/memory.h
> > @@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
> >  
> >  #else
> >  
> > +#ifndef PHYS_OFFSET
> > +#ifdef PLAT_PHYS_OFFSET
> > +#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> > +#else
> > +#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> > +#endif
> > +#endif
> > +
> >  static inline phys_addr_t __virt_to_phys(unsigned long x)
> >  {
> >  	return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
> > @@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
> >  #endif
> >  #endif /* __ASSEMBLY__ */
> >  
> > -#ifndef PHYS_OFFSET
> > -#ifdef PLAT_PHYS_OFFSET
> > -#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> > -#else
> > -#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> > -#endif
> > -#endif
> 
> And that makes PHYS_OFFSET undefined to assembly code - and we have
> references to it from said code.

Why does the kernel compile properly for me with this change then?


Nicolas

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Russell King - ARM Linux]

On Tue, Nov 26, 2013 at 08:35:43AM -0500, Nicolas Pitre wrote:
> On Tue, 26 Nov 2013, Russell King - ARM Linux wrote:
> 
> > On Mon, Nov 25, 2013 at 10:56:25PM -0500, Nicolas Pitre wrote:
> > > What about simply doing the following instead, which I'm sure used to 
> > > work properly at some point:
> > > 
> > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > > index 9ecccc8650..2b8b8d3236 100644
> > > --- a/arch/arm/include/asm/memory.h
> > > +++ b/arch/arm/include/asm/memory.h
> > > @@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
> > >  
> > >  #else
> > >  
> > > +#ifndef PHYS_OFFSET
> > > +#ifdef PLAT_PHYS_OFFSET
> > > +#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> > > +#else
> > > +#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> > > +#endif
> > > +#endif
> > > +
> > >  static inline phys_addr_t __virt_to_phys(unsigned long x)
> > >  {
> > >  	return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
> > > @@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
> > >  #endif
> > >  #endif /* __ASSEMBLY__ */
> > >  
> > > -#ifndef PHYS_OFFSET
> > > -#ifdef PLAT_PHYS_OFFSET
> > > -#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> > > -#else
> > > -#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> > > -#endif
> > > -#endif
> > 
> > And that makes PHYS_OFFSET undefined to assembly code - and we have
> > references to it from said code.
> 
> Why does the kernel compile properly for me with this change then?

Nicolas,

Try using grep rather than just typing emails for the hell of it.
Try looking at the patch I sent to fix this issue.  Either of those
will give you the appropriate clue necessary to answer your question.

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Nicolas Pitre]

On Tue, 26 Nov 2013, Russell King - ARM Linux wrote:

> On Tue, Nov 26, 2013 at 08:35:43AM -0500, Nicolas Pitre wrote:
> > On Tue, 26 Nov 2013, Russell King - ARM Linux wrote:
> > 
> > > On Mon, Nov 25, 2013 at 10:56:25PM -0500, Nicolas Pitre wrote:
> > > > What about simply doing the following instead, which I'm sure used to 
> > > > work properly at some point:
> > > > 
> > > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > > > index 9ecccc8650..2b8b8d3236 100644
> > > > --- a/arch/arm/include/asm/memory.h
> > > > +++ b/arch/arm/include/asm/memory.h
> > > > @@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
> > > >  
> > > >  #else
> > > >  
> > > > +#ifndef PHYS_OFFSET
> > > > +#ifdef PLAT_PHYS_OFFSET
> > > > +#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> > > > +#else
> > > > +#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> > > > +#endif
> > > > +#endif
> > > > +
> > > >  static inline phys_addr_t __virt_to_phys(unsigned long x)
> > > >  {
> > > >  	return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
> > > > @@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
> > > >  #endif
> > > >  #endif /* __ASSEMBLY__ */
> > > >  
> > > > -#ifndef PHYS_OFFSET
> > > > -#ifdef PLAT_PHYS_OFFSET
> > > > -#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> > > > -#else
> > > > -#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> > > > -#endif
> > > > -#endif
> > > 
> > > And that makes PHYS_OFFSET undefined to assembly code - and we have
> > > references to it from said code.
> > 
> > Why does the kernel compile properly for me with this change then?
> 
> Nicolas,
> 
> Try using grep rather than just typing emails for the hell of it.
> Try looking at the patch I sent to fix this issue.  Either of those
> will give you the appropriate clue necessary to answer your question.

Your patch does s/PHYS_OFFSET/PLAT_PHYS_OFFSET/ in assembly code and 
defines PLAT_PHYS_OFFSET with CONFIG_PHYS_OFFSET when needed.

Mine leaves PHYS_OFFSET as is in assembly code and defines PHYS_OFFSET 
with PLAT_PHYS_OFFSET or CONFIG_PHYS_OFFSET as appropriate, as it *used* 
to work before commit ca5a45c06c.

The reason this commit broke things is because it moved __virt_to_phys() 
and __phys_to_virt() from macros to static inline.  The macro version 
didn't need PHYS_OFFSET to be defined beforehand, which is not the case 
for static inline definitions.  Hence my fix which is to simply move 
(one of) the definition of PHYS_OFFSET above its use by the mentioned 
static inline code.

I also think it is a good thing _not_ to use PLAT_PHYS_OFFSET in 
assembly code.  This was in fact done on purpose.  The simple fact that 
PHYS_OFFSET, with some configuration, expands to something unappropriate 
for assembly code is a perfect mechanism to identify that something is 
wrong with that configuration.  The reason is that no assembly code 
should be using PHYS_OFFSET when CONFIG_PATCH_PHYS_VIRT is defined.  If 
it does then this is wrong.

By moving that assembly code to PLAT_PHYS_OFFSET you are basically 
removing that sanity check mechanism since it might be possible for a 
platform to still define PLAT_PHYS_OFFSET in its mach/memory.h even if 
CONFIG_PATCH_PHYS_VIRT is defined.

So, unless you identify a problem with the patch I sent, I still stand 
by my suggested fix.


Nicolas

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Russell King - ARM Linux]

On Tue, Nov 26, 2013 at 12:26:49PM -0500, Nicolas Pitre wrote:
> On Tue, 26 Nov 2013, Russell King - ARM Linux wrote:
> 
> > On Tue, Nov 26, 2013 at 08:35:43AM -0500, Nicolas Pitre wrote:
> > > On Tue, 26 Nov 2013, Russell King - ARM Linux wrote:
> > > 
> > > > On Mon, Nov 25, 2013 at 10:56:25PM -0500, Nicolas Pitre wrote:
> > > > > What about simply doing the following instead, which I'm sure used to 
> > > > > work properly at some point:
> > > > > 
> > > > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > > > > index 9ecccc8650..2b8b8d3236 100644
> > > > > --- a/arch/arm/include/asm/memory.h
> > > > > +++ b/arch/arm/include/asm/memory.h
> > > > > @@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
> > > > >  
> > > > >  #else
> > > > >  
> > > > > +#ifndef PHYS_OFFSET
> > > > > +#ifdef PLAT_PHYS_OFFSET
> > > > > +#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> > > > > +#else
> > > > > +#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> > > > > +#endif
> > > > > +#endif
> > > > > +
> > > > >  static inline phys_addr_t __virt_to_phys(unsigned long x)
> > > > >  {
> > > > >  	return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
> > > > > @@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
> > > > >  #endif
> > > > >  #endif /* __ASSEMBLY__ */
> > > > >  
> > > > > -#ifndef PHYS_OFFSET
> > > > > -#ifdef PLAT_PHYS_OFFSET
> > > > > -#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> > > > > -#else
> > > > > -#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> > > > > -#endif
> > > > > -#endif
> > > > 
> > > > And that makes PHYS_OFFSET undefined to assembly code - and we have
> > > > references to it from said code.
> > > 
> > > Why does the kernel compile properly for me with this change then?
> > 
> > Nicolas,
> > 
> > Try using grep rather than just typing emails for the hell of it.
> > Try looking at the patch I sent to fix this issue.  Either of those
> > will give you the appropriate clue necessary to answer your question.
> 
> Your patch does s/PHYS_OFFSET/PLAT_PHYS_OFFSET/ in assembly code and 
> defines PLAT_PHYS_OFFSET with CONFIG_PHYS_OFFSET when needed.
> 
> Mine leaves PHYS_OFFSET as is in assembly code and defines PHYS_OFFSET 
> with PLAT_PHYS_OFFSET or CONFIG_PHYS_OFFSET as appropriate, as it *used* 
> to work before commit ca5a45c06c.

If manual inspection fails you, try building a nommu or XIP kernel.

For crying out loud, you're moving the definition of PHYS_OFFSET to be
_inside_ a #ifndef __ASSEMBLY__ .. #endif section.  That's fscking
obvious if you bother to read your own patch, because you move the
definition to just _before_ some C code, and the assembler won't
parse C code.

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Nicolas Pitre]

On Tue, 26 Nov 2013, Russell King - ARM Linux wrote:

> On Tue, Nov 26, 2013 at 12:26:49PM -0500, Nicolas Pitre wrote:
> > On Tue, 26 Nov 2013, Russell King - ARM Linux wrote:
> > 
> > > On Tue, Nov 26, 2013 at 08:35:43AM -0500, Nicolas Pitre wrote:
> > > > On Tue, 26 Nov 2013, Russell King - ARM Linux wrote:
> > > > 
> > > > > On Mon, Nov 25, 2013 at 10:56:25PM -0500, Nicolas Pitre wrote:
> > > > > > What about simply doing the following instead, which I'm sure used to 
> > > > > > work properly at some point:
> > > > > > 
> > > > > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > > > > > index 9ecccc8650..2b8b8d3236 100644
> > > > > > --- a/arch/arm/include/asm/memory.h
> > > > > > +++ b/arch/arm/include/asm/memory.h
> > > > > > @@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
> > > > > >  
> > > > > >  #else
> > > > > >  
> > > > > > +#ifndef PHYS_OFFSET
> > > > > > +#ifdef PLAT_PHYS_OFFSET
> > > > > > +#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> > > > > > +#else
> > > > > > +#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> > > > > > +#endif
> > > > > > +#endif
> > > > > > +
> > > > > >  static inline phys_addr_t __virt_to_phys(unsigned long x)
> > > > > >  {
> > > > > >  	return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
> > > > > > @@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
> > > > > >  #endif
> > > > > >  #endif /* __ASSEMBLY__ */
> > > > > >  
> > > > > > -#ifndef PHYS_OFFSET
> > > > > > -#ifdef PLAT_PHYS_OFFSET
> > > > > > -#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> > > > > > -#else
> > > > > > -#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> > > > > > -#endif
> > > > > > -#endif
> > > > > 
> > > > > And that makes PHYS_OFFSET undefined to assembly code - and we have
> > > > > references to it from said code.
> > > > 
> > > > Why does the kernel compile properly for me with this change then?
> > > 
> > > Nicolas,
> > > 
> > > Try using grep rather than just typing emails for the hell of it.
> > > Try looking at the patch I sent to fix this issue.  Either of those
> > > will give you the appropriate clue necessary to answer your question.
> > 
> > Your patch does s/PHYS_OFFSET/PLAT_PHYS_OFFSET/ in assembly code and 
> > defines PLAT_PHYS_OFFSET with CONFIG_PHYS_OFFSET when needed.
> > 
> > Mine leaves PHYS_OFFSET as is in assembly code and defines PHYS_OFFSET 
> > with PLAT_PHYS_OFFSET or CONFIG_PHYS_OFFSET as appropriate, as it *used* 
> > to work before commit ca5a45c06c.
> 
> If manual inspection fails you, try building a nommu or XIP kernel.

All right.  I did a build test of course.  What failed me is the 
difficulty nowdays to have the desired config symbols enabled (or 
left disabled for that matter) without other rules overriding manual 
choices.

I think your patch is therefore the best solution.  However, I'd suggest 
you include this as well to enforce configuration validity:

#ifdef CONFIG_ARM_PATCH_PHYS_VIRT
#undef PLAT_PHYS_OFFSET
#endif

The idea as I explained in my previous email is to prevent wrong usage.


Nicolas

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Russell King - ARM Linux]

On Tue, Nov 26, 2013 at 01:41:08PM -0500, Nicolas Pitre wrote:
> On Tue, 26 Nov 2013, Russell King - ARM Linux wrote:
> > If manual inspection fails you, try building a nommu or XIP kernel.
> 
> All right.  I did a build test of course.  What failed me is the 
> difficulty nowdays to have the desired config symbols enabled (or 
> left disabled for that matter) without other rules overriding manual 
> choices.

This is precisely why reading and understanding the file (as I did) is
much more important than chucking out emails on a mailing list.  I'd
already taken the time to see that there were _three_ levels if ifdef
there and the problem case was buried in the depths of that.

> I think your patch is therefore the best solution.  However, I'd suggest 
> you include this as well to enforce configuration validity:
> 
> #ifdef CONFIG_ARM_PATCH_PHYS_VIRT
> #undef PLAT_PHYS_OFFSET
> #endif
> 
> The idea as I explained in my previous email is to prevent wrong usage.

I don't believe it's necessary.  Why?

PLAT_PHYS_OFFSET gets defined in one of two ways:

1. We have a mach/memory.h
2. We have CONFIG_PHYS_OFFSET defined

CONFIG_PHYS_OFFSET will only be defined if ARM_PATCH_PHYS_OFFSET has been
disabled.  So that leaves us with needing a mach/memory.h to define it.

Now, if you consider that the majority of builds today use multiplatform
which requires there that there be no mach/memory.h, and that requires
the phys offset patching, we've already got way more than enough build
coverage to find mis-uses, especially with the amount of building that
Olof and myself do on the ARM kernel.

Moreover, this is no different from what it is today.  It hasn't suddenly
become more visible.

Hence, no matter what, such a change should not be part of fixing up
the original breakage.

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Nicolas Pitre]

On Tue, 26 Nov 2013, Russell King - ARM Linux wrote:

> On Tue, Nov 26, 2013 at 01:41:08PM -0500, Nicolas Pitre wrote:
> > I think your patch is therefore the best solution.  However, I'd suggest 
> > you include this as well to enforce configuration validity:
> > 
> > #ifdef CONFIG_ARM_PATCH_PHYS_VIRT
> > #undef PLAT_PHYS_OFFSET
> > #endif
> > 
> > The idea as I explained in my previous email is to prevent wrong usage.
> 
> I don't believe it's necessary.  Why?
> 
> PLAT_PHYS_OFFSET gets defined in one of two ways:
> 
> 1. We have a mach/memory.h
> 2. We have CONFIG_PHYS_OFFSET defined
> 
> CONFIG_PHYS_OFFSET will only be defined if ARM_PATCH_PHYS_OFFSET has been
> disabled.  So that leaves us with needing a mach/memory.h to define it.
> 
> Now, if you consider that the majority of builds today use multiplatform
> which requires there that there be no mach/memory.h, and that requires
> the phys offset patching, we've already got way more than enough build
> coverage to find mis-uses, especially with the amount of building that
> Olof and myself do on the ARM kernel.

I don't worry about multiplatform.  It is more about those corner-case 
targets with a mach/memory.h where phys offset patching can be enabled.

> Moreover, this is no different from what it is today.  It hasn't suddenly
> become more visible.

Before commit ca5a45c06c a bad PHYS_OFFSET usage in assembly code was 
caught with a compilation error.  Today bad and good uses of PHYS_OFFSET 
are caught with a compilation error.  With your patch there is a risk 
for (the equivalent of) PHYS_OFFSET to be used wrongly without notice. 
I'm just arguing for bringing the same state of affair as it was before 
that commit which is the actual breakage origin.

But in the end that doesn't matter much.  As I said those are 
corner-case targets and very few people might still be using them.


Nicolas

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Jason Gunthorpe]

On Mon, Nov 25, 2013 at 11:36:54PM +0000, Russell King - ARM Linux wrote:
 
> Actually, I think Nicolas' commit:
> 
> 1b9f95f8ade9efc2bd49f0e7b9dc61a038ac3eef
> 
> was wrong to remove PLAT_PHYS_OFFSET.  So, partially undoing Nicolas'
> patch, and reordering things a little, we end up with this, which if
> it weren't for the comment would be 5 lines shorter!

Not sure where this thread ended up, but:

Tested-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

On ARM kirkwood with 3.13-rc3

Regards,
Jason
 
>  arch/arm/include/asm/memory.h |   23 ++++++++++++-----------
>  arch/arm/kernel/head-nommu.S  |    4 ++--
>  arch/arm/kernel/head.S        |    2 +-
>  3 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index 9ecccc865046..c7e0d708e911 100644
> +++ b/arch/arm/include/asm/memory.h
> @@ -157,6 +157,16 @@
>  #endif
>  #define ARCH_PGD_MASK		((1 << ARCH_PGD_SHIFT) - 1)
>  
> +/*
> + * PLAT_PHYS_OFFSET is the offset (from zero) of the start of physical
> + * memory.  This is used for XIP and NoMMU kernels, or by kernels which
> + * have their own mach/memory.h.  Assembly code must always use
> + * PLAT_PHYS_OFFSET and not PHYS_OFFSET.
> + */
> +#ifndef PLAT_PHYS_OFFSET
> +#define PLAT_PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> +#endif
> +
>  #ifndef __ASSEMBLY__
>  
>  /*
> @@ -239,6 +249,8 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
>  
>  #else
>  
> +#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> +
>  static inline phys_addr_t __virt_to_phys(unsigned long x)
>  {
>  	return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
> @@ -251,17 +263,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
>  
>  #endif
>  #endif
> -#endif /* __ASSEMBLY__ */
> -
> -#ifndef PHYS_OFFSET
> -#ifdef PLAT_PHYS_OFFSET
> -#define PHYS_OFFSET	PLAT_PHYS_OFFSET
> -#else
> -#define PHYS_OFFSET	UL(CONFIG_PHYS_OFFSET)
> -#endif
> -#endif
> -
> -#ifndef __ASSEMBLY__
>  
>  /*
>   * PFNs are used to describe any physical page; this means
> diff --git a/arch/arm/kernel/head-nommu.S b/arch/arm/kernel/head-nommu.S
> index 14235ba64a90..716249cc2ee1 100644
> +++ b/arch/arm/kernel/head-nommu.S
> @@ -68,7 +68,7 @@ ENTRY(stext)
>  
>  #ifdef CONFIG_ARM_MPU
>  	/* Calculate the size of a region covering just the kernel */
> -	ldr	r5, =PHYS_OFFSET		@ Region start: PHYS_OFFSET
> +	ldr	r5, =PLAT_PHYS_OFFSET		@ Region start: PHYS_OFFSET
>  	ldr     r6, =(_end)			@ Cover whole kernel
>  	sub	r6, r6, r5			@ Minimum size of region to map
>  	clz	r6, r6				@ Region size must be 2^N...
> @@ -213,7 +213,7 @@ ENTRY(__setup_mpu)
>  	set_region_nr r0, #MPU_RAM_REGION
>  	isb
>  	/* Full access from PL0, PL1, shared for CONFIG_SMP, cacheable */
> -	ldr	r0, =PHYS_OFFSET		@ RAM starts at PHYS_OFFSET
> +	ldr	r0, =PLAT_PHYS_OFFSET		@ RAM starts at PHYS_OFFSET
>  	ldr	r5,=(MPU_AP_PL1RW_PL0RW | MPU_RGN_NORMAL)
>  
>  	setup_region r0, r5, r6, MPU_DATA_SIDE	@ PHYS_OFFSET, shared, enabled
> diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
> index 11d59b32fb8d..32f317e5828a 100644
> +++ b/arch/arm/kernel/head.S
> @@ -110,7 +110,7 @@ ENTRY(stext)
>  	sub	r4, r3, r4			@ (PHYS_OFFSET - PAGE_OFFSET)
>  	add	r8, r8, r4			@ PHYS_OFFSET
>  #else
> -	ldr	r8, =PHYS_OFFSET		@ always constant in this case
> +	ldr	r8, =PLAT_PHYS_OFFSET		@ always constant in this case
>  #endif
>  
>  	/*
>

Build breakage from 'ARM: mm: use phys_addr_t appropriately in p2v and v2p conversions'

[Russell King - ARM Linux]

On Tue, Dec 10, 2013 at 12:17:30PM -0700, Jason Gunthorpe wrote:
> On Mon, Nov 25, 2013 at 11:36:54PM +0000, Russell King - ARM Linux wrote:
>  
> > Actually, I think Nicolas' commit:
> > 
> > 1b9f95f8ade9efc2bd49f0e7b9dc61a038ac3eef
> > 
> > was wrong to remove PLAT_PHYS_OFFSET.  So, partially undoing Nicolas'
> > patch, and reordering things a little, we end up with this, which if
> > it weren't for the comment would be 5 lines shorter!
> 
> Not sure where this thread ended up, but:

Nowhere because there was no evidence of the patch being tested.

> Tested-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Now committed, thanks.