[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Tony Lindgren]

Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
(Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
into devel-stable) merged generic ioremap changes.

Commit 137d105d50f6e6c373c1aa759f59045e6239cf66
(ARM: OMAP4: Fix errata i688 with MPU interconnect barriers)
added a workaround for omap4.

In order for the errata to work, we now need the following
patch or else we'll get:

kernel BUG at mm/vmalloc.c:1134!

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -132,6 +132,7 @@ void omap3_map_io(void);
 void am33xx_map_io(void);
 void omap4_map_io(void);
 void ti81xx_map_io(void);
+int omap_barriers_init(void);
 
 /**
  * omap_test_timeout - busy-loop, testing a condition
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -306,7 +306,12 @@ void __init omapam33xx_map_common_io(void)
 #ifdef CONFIG_ARCH_OMAP4
 void __init omap44xx_map_common_io(void)
 {
+	int res;
+
 	iotable_init(omap44xx_io_desc, ARRAY_SIZE(omap44xx_io_desc));
+	res = omap_barriers_init();
+	if (res)
+		pr_err("Barriers broken\n");
 }
 #endif
 
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -51,7 +51,7 @@ void omap_bus_sync(void)
 	}
 }
 
-static int __init omap_barriers_init(void)
+int __init omap_barriers_init(void)
 {
 	struct map_desc dram_io_desc[1];
 	phys_addr_t paddr;
@@ -81,7 +81,11 @@ static int __init omap_barriers_init(void)
 
 	return 0;
 }
-core_initcall(omap_barriers_init);
+#else
+int __init omap_barriers_init(void)
+{
+	return 0;
+}
 #endif
 
 void __init gic_init_irq(void)

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Shilimkar, Santosh]

On Thu, Jan 12, 2012 at 7:42 PM, Tony Lindgren <tony@atomide.com> wrote:
> Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
> (Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
> into devel-stable) merged generic ioremap changes.
>
> Commit 137d105d50f6e6c373c1aa759f59045e6239cf66
> (ARM: OMAP4: Fix errata i688 with MPU interconnect barriers)
> added a workaround for omap4.
>
> In order for the errata to work, we now need the following
> patch or else we'll get:
>
> kernel BUG at mm/vmalloc.c:1134!
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
>
Thanks Tony.

Looks good to me.
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Nicolas Pitre]

On Thu, 12 Jan 2012, Tony Lindgren wrote:

> Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
> (Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
> into devel-stable) merged generic ioremap changes.
> 
> Commit 137d105d50f6e6c373c1aa759f59045e6239cf66
> (ARM: OMAP4: Fix errata i688 with MPU interconnect barriers)
> added a workaround for omap4.
> 
> In order for the errata to work, we now need the following
> patch or else we'll get:
> 
> kernel BUG at mm/vmalloc.c:1134!
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> --- a/arch/arm/mach-omap2/common.h
> +++ b/arch/arm/mach-omap2/common.h
> @@ -132,6 +132,7 @@ void omap3_map_io(void);
>  void am33xx_map_io(void);
>  void omap4_map_io(void);
>  void ti81xx_map_io(void);
> +int omap_barriers_init(void);
>  
>  /**
>   * omap_test_timeout - busy-loop, testing a condition
> --- a/arch/arm/mach-omap2/io.c
> +++ b/arch/arm/mach-omap2/io.c
> @@ -306,7 +306,12 @@ void __init omapam33xx_map_common_io(void)
>  #ifdef CONFIG_ARCH_OMAP4
>  void __init omap44xx_map_common_io(void)
>  {
> +	int res;
> +
>  	iotable_init(omap44xx_io_desc, ARRAY_SIZE(omap44xx_io_desc));
> +	res = omap_barriers_init();
> +	if (res)
> +		pr_err("Barriers broken\n");

Do you really need that return code?

It was initially hooked to core_initcall() which requires a return code.  
Now that you call it directly you could get rid of it.


Nicolas

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Nicolas Pitre]

On Thu, 12 Jan 2012, Nicolas Pitre wrote:

> On Thu, 12 Jan 2012, Tony Lindgren wrote:
> 
> > Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
> > (Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
> > into devel-stable) merged generic ioremap changes.
> > 
> > Commit 137d105d50f6e6c373c1aa759f59045e6239cf66
> > (ARM: OMAP4: Fix errata i688 with MPU interconnect barriers)
> > added a workaround for omap4.
> > 
> > In order for the errata to work, we now need the following
> > patch or else we'll get:
> > 
> > kernel BUG at mm/vmalloc.c:1134!
> > 
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > 
> > --- a/arch/arm/mach-omap2/common.h
> > +++ b/arch/arm/mach-omap2/common.h
> > @@ -132,6 +132,7 @@ void omap3_map_io(void);
> >  void am33xx_map_io(void);
> >  void omap4_map_io(void);
> >  void ti81xx_map_io(void);
> > +int omap_barriers_init(void);
> >  
> >  /**
> >   * omap_test_timeout - busy-loop, testing a condition
> > --- a/arch/arm/mach-omap2/io.c
> > +++ b/arch/arm/mach-omap2/io.c
> > @@ -306,7 +306,12 @@ void __init omapam33xx_map_common_io(void)
> >  #ifdef CONFIG_ARCH_OMAP4
> >  void __init omap44xx_map_common_io(void)
> >  {
> > +	int res;
> > +
> >  	iotable_init(omap44xx_io_desc, ARRAY_SIZE(omap44xx_io_desc));
> > +	res = omap_barriers_init();
> > +	if (res)
> > +		pr_err("Barriers broken\n");
> 
> Do you really need that return code?
> 
> It was initially hooked to core_initcall() which requires a return code.  
> Now that you call it directly you could get rid of it.

s/rid of it/rid of the return code/


Nicolas

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Tony Lindgren]

* Nicolas Pitre <nico@fluxnic.net> [120112 11:15]:
> On Thu, 12 Jan 2012, Nicolas Pitre wrote:
> > 
> > Do you really need that return code?
> > 
> > It was initially hooked to core_initcall() which requires a return code.  
> > Now that you call it directly you could get rid of it.
> 
> s/rid of it/rid of the return code/

Well memblock_alloc could fail there, but omap_barries_init
can be made void as there's already "failed to reserve 4Kbytes"
error there. So let's make it void. Updated patch below.

Tony


From: Tony Lindgren <tony@atomide.com>
Date: Thu, 12 Jan 2012 10:28:26 -0800
Subject: [PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
(Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
into devel-stable) merged generic ioremap changes.

Commit 137d105d50f6e6c373c1aa759f59045e6239cf66
(ARM: OMAP4: Fix errata i688 with MPU interconnect barriers)
added a workaround for omap4.

In order for the errata to work, we now need the following
patch or else we'll get:

kernel BUG at mm/vmalloc.c:1134!

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -132,6 +132,7 @@ void omap3_map_io(void);
 void am33xx_map_io(void);
 void omap4_map_io(void);
 void ti81xx_map_io(void);
+void omap_barriers_init(void);
 
 /**
  * omap_test_timeout - busy-loop, testing a condition
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -307,6 +307,7 @@ void __init omapam33xx_map_common_io(void)
 void __init omap44xx_map_common_io(void)
 {
 	iotable_init(omap44xx_io_desc, ARRAY_SIZE(omap44xx_io_desc));
+	omap_barriers_init();
 }
 #endif
 
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -51,20 +51,20 @@ void omap_bus_sync(void)
 	}
 }
 
-static int __init omap_barriers_init(void)
+void __init omap_barriers_init(void)
 {
 	struct map_desc dram_io_desc[1];
 	phys_addr_t paddr;
 	u32 size;
 
 	if (!cpu_is_omap44xx())
-		return -ENODEV;
+		return;
 
 	size = ALIGN(PAGE_SIZE, SZ_1M);
 	paddr = memblock_alloc(size, SZ_1M);
 	if (!paddr) {
 		pr_err("%s: failed to reserve 4 Kbytes\n", __func__);
-		return -ENOMEM;
+		return;
 	}
 	memblock_free(paddr, size);
 	memblock_remove(paddr, size);
@@ -78,10 +78,11 @@ static int __init omap_barriers_init(void)
 
 	pr_info("OMAP4: Map 0x%08llx to 0x%08lx for dram barrier\n",
 		(long long) paddr, dram_io_desc[0].virtual);
-
-	return 0;
 }
-core_initcall(omap_barriers_init);
+#else
+void __init omap_barriers_init(void)
+{
+}
 #endif
 
 void __init gic_init_irq(void)

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Russell King - ARM Linux]

On Thu, Jan 12, 2012 at 11:59:51AM -0800, Tony Lindgren wrote:
> * Nicolas Pitre <nico@fluxnic.net> [120112 11:15]:
> > On Thu, 12 Jan 2012, Nicolas Pitre wrote:
> > > 
> > > Do you really need that return code?
> > > 
> > > It was initially hooked to core_initcall() which requires a return code.  
> > > Now that you call it directly you could get rid of it.
> > 
> > s/rid of it/rid of the return code/
> 
> Well memblock_alloc could fail there

BTW, that's another thing that's wrong here - it can't fail in such a way
that it returns something to you:

phys_addr_t __init memblock_alloc_base(phys_addr_t size, phys_addr_t align, phys_addr_t max_addr)
{
        phys_addr_t alloc;

        alloc = __memblock_alloc_base(size, align, max_addr);

        if (alloc == 0)
                panic("ERROR: Failed to allocate 0x%llx bytes below 0x%llx.\n",
                      (unsigned long long) size, (unsigned long long) max_addr);
        return alloc;
}

phys_addr_t __init memblock_alloc(phys_addr_t size, phys_addr_t align)
{
        return memblock_alloc_base(size, align, MEMBLOCK_ALLOC_ACCESSIBLE);
}

So all these tests for zero addresses returned from memblock_alloc()
in the OMAP code need to be killed off.

Actually, I'll do it myself - especially as I'm considering adding
'arm_memblock_steal()' which will barf if it's misused.  This makes
the bug in the OMAP4 code glaringly obvious, to the point that OMAP4
will not boot until it's fixed.

8<===
From: Russell King <rmk+kernel@arm.linux.org.uk>
ARM: Add arm_memblock_steal() to allocate memory away from the kernel

Several platforms are now using the memblock_alloc+memblock_free+
memblock_remove trick to obtain memory which won't be mapped in the
kernel's page tables.  Most platforms do this (correctly) in the
->reserve callback.  However, OMAP has started to call these functions
outside of this callback, and this is extremely unsafe - memory will
not be unmapped, and could well be given out after memblock is no
longer responsible for its management.

So, provide arm_memblock_steal() to perform this function, and ensure
that it panic()s if it is used inappropriately.  Convert everyone
over, including OMAP.

As a result, OMAP will panic on boot with this change.  OMAP needs to
be fixed, or 137d105d50 (ARM: OMAP4: Fix errata i688 with MPU
interconnect barriers.) reverted until such time it can be fixed
correctly.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/memblock.h      |    2 ++
 arch/arm/mach-imx/mach-mx31_3ds.c    |    5 ++---
 arch/arm/mach-imx/mach-mx31moboard.c |    5 ++---
 arch/arm/mach-imx/mach-pcm037.c      |    5 ++---
 arch/arm/mach-omap2/omap-secure.c    |   13 ++-----------
 arch/arm/mach-omap2/omap4-common.c   |   10 +++-------
 arch/arm/mm/init.c                   |   17 +++++++++++++++++
 arch/arm/plat-omap/devices.c         |    5 ++---
 8 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/arch/arm/include/asm/memblock.h b/arch/arm/include/asm/memblock.h
index b8da2e4..00ca5f9 100644
--- a/arch/arm/include/asm/memblock.h
+++ b/arch/arm/include/asm/memblock.h
@@ -6,4 +6,6 @@ struct machine_desc;
 
 extern void arm_memblock_init(struct meminfo *, struct machine_desc *);
 
+phys_addr_t arm_memblock_steal(phys_addr_t size, phys_addr_t align);
+
 #endif
diff --git a/arch/arm/mach-imx/mach-mx31_3ds.c b/arch/arm/mach-imx/mach-mx31_3ds.c
index 89c3325..4d1aab1 100644
--- a/arch/arm/mach-imx/mach-mx31_3ds.c
+++ b/arch/arm/mach-imx/mach-mx31_3ds.c
@@ -36,6 +36,7 @@
 #include <asm/mach/time.h>
 #include <asm/memory.h>
 #include <asm/mach/map.h>
+#include <asm/memblock.h>
 #include <mach/common.h>
 #include <mach/iomux-mx3.h>
 #include <mach/3ds_debugboard.h>
@@ -754,10 +755,8 @@ static struct sys_timer mx31_3ds_timer = {
 static void __init mx31_3ds_reserve(void)
 {
 	/* reserve MX31_3DS_CAMERA_BUF_SIZE bytes for mx3-camera */
-	mx3_camera_base = memblock_alloc(MX31_3DS_CAMERA_BUF_SIZE,
+	mx3_camera_base = arm_memblock_steal(MX31_3DS_CAMERA_BUF_SIZE,
 					 MX31_3DS_CAMERA_BUF_SIZE);
-	memblock_free(mx3_camera_base, MX31_3DS_CAMERA_BUF_SIZE);
-	memblock_remove(mx3_camera_base, MX31_3DS_CAMERA_BUF_SIZE);
 }
 
 MACHINE_START(MX31_3DS, "Freescale MX31PDK (3DS)")
diff --git a/arch/arm/mach-imx/mach-mx31moboard.c b/arch/arm/mach-imx/mach-mx31moboard.c
index b95981d..f225262 100644
--- a/arch/arm/mach-imx/mach-mx31moboard.c
+++ b/arch/arm/mach-imx/mach-mx31moboard.c
@@ -41,6 +41,7 @@
 #include <asm/mach/arch.h>
 #include <asm/mach/time.h>
 #include <asm/mach/map.h>
+#include <asm/memblock.h>
 #include <mach/board-mx31moboard.h>
 #include <mach/common.h>
 #include <mach/hardware.h>
@@ -584,10 +585,8 @@ struct sys_timer mx31moboard_timer = {
 static void __init mx31moboard_reserve(void)
 {
 	/* reserve 4 MiB for mx3-camera */
-	mx3_camera_base = memblock_alloc(MX3_CAMERA_BUF_SIZE,
+	mx3_camera_base = arm_memblock_steal(MX3_CAMERA_BUF_SIZE,
 			MX3_CAMERA_BUF_SIZE);
-	memblock_free(mx3_camera_base, MX3_CAMERA_BUF_SIZE);
-	memblock_remove(mx3_camera_base, MX3_CAMERA_BUF_SIZE);
 }
 
 MACHINE_START(MX31MOBOARD, "EPFL Mobots mx31moboard")
diff --git a/arch/arm/mach-imx/mach-pcm037.c b/arch/arm/mach-imx/mach-pcm037.c
index d7e1516..e48854b 100644
--- a/arch/arm/mach-imx/mach-pcm037.c
+++ b/arch/arm/mach-imx/mach-pcm037.c
@@ -39,6 +39,7 @@
 #include <asm/mach/arch.h>
 #include <asm/mach/time.h>
 #include <asm/mach/map.h>
+#include <asm/memblock.h>
 #include <mach/common.h>
 #include <mach/hardware.h>
 #include <mach/iomux-mx3.h>
@@ -680,10 +681,8 @@ struct sys_timer pcm037_timer = {
 static void __init pcm037_reserve(void)
 {
 	/* reserve 4 MiB for mx3-camera */
-	mx3_camera_base = memblock_alloc(MX3_CAMERA_BUF_SIZE,
+	mx3_camera_base = arm_memblock_steal(MX3_CAMERA_BUF_SIZE,
 			MX3_CAMERA_BUF_SIZE);
-	memblock_free(mx3_camera_base, MX3_CAMERA_BUF_SIZE);
-	memblock_remove(mx3_camera_base, MX3_CAMERA_BUF_SIZE);
 }
 
 MACHINE_START(PCM037, "Phytec Phycore pcm037")
diff --git a/arch/arm/mach-omap2/omap-secure.c b/arch/arm/mach-omap2/omap-secure.c
index 69f3c72..d8f8ef4 100644
--- a/arch/arm/mach-omap2/omap-secure.c
+++ b/arch/arm/mach-omap2/omap-secure.c
@@ -16,6 +16,7 @@
 #include <linux/memblock.h>
 
 #include <asm/cacheflush.h>
+#include <asm/memblock.h>
 
 #include <mach/omap-secure.h>
 
@@ -57,20 +58,10 @@ u32 omap_secure_dispatcher(u32 idx, u32 flag, u32 nargs, u32 arg1, u32 arg2,
 /* Allocate the memory to save secure ram */
 int __init omap_secure_ram_reserve_memblock(void)
 {
-	phys_addr_t paddr;
 	u32 size = OMAP_SECURE_RAM_STORAGE;
 
 	size = ALIGN(size, SZ_1M);
-	paddr = memblock_alloc(size, SZ_1M);
-	if (!paddr) {
-		pr_err("%s: failed to reserve %x bytes\n",
-				__func__, size);
-		return -ENOMEM;
-	}
-	memblock_free(paddr, size);
-	memblock_remove(paddr, size);
-
-	omap_secure_memblock_base = paddr;
+	omap_secure_memblock_base = arm_memblock_steal(size, SZ_1M);
 
 	return 0;
 }
diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
index bc16c81..40a8fbc 100644
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -20,6 +20,7 @@
 #include <asm/hardware/gic.h>
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/mach/map.h>
+#include <asm/memblock.h>
 
 #include <plat/irqs.h>
 #include <plat/sram.h>
@@ -61,13 +62,8 @@ static int __init omap_barriers_init(void)
 		return -ENODEV;
 
 	size = ALIGN(PAGE_SIZE, SZ_1M);
-	paddr = memblock_alloc(size, SZ_1M);
-	if (!paddr) {
-		pr_err("%s: failed to reserve 4 Kbytes\n", __func__);
-		return -ENOMEM;
-	}
-	memblock_free(paddr, size);
-	memblock_remove(paddr, size);
+	paddr = arm_memblock_steal(size, SZ_1M);
+
 	dram_io_desc[0].virtual = OMAP4_DRAM_BARRIER_VA;
 	dram_io_desc[0].pfn = __phys_to_pfn(paddr);
 	dram_io_desc[0].length = size;
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index e34ea8a..6ec1226 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -22,6 +22,7 @@
 #include <linux/memblock.h>
 
 #include <asm/mach-types.h>
+#include <asm/memblock.h>
 #include <asm/prom.h>
 #include <asm/sections.h>
 #include <asm/setup.h>
@@ -307,6 +308,22 @@ static void arm_memory_present(void)
 }
 #endif
 
+static bool arm_memblock_steal_permitted = true;
+
+phys_addr_t arm_memblock_steal(phys_addr_t size, phys_addr_t align)
+{
+	phys_addr_t phys;
+
+	if (!arm_memblock_steal_permitted)
+		panic("arm_memblock_steal used in an unsafe manner\n");
+
+	phys = memblock_alloc(size, align);
+	memblock_free(phys, size);
+	memblock_remove(phys, size);
+
+	return phys;
+}
+
 void __init arm_memblock_init(struct meminfo *mi, struct machine_desc *mdesc)
 {
 	int i;
@@ -349,6 +366,7 @@ void __init arm_memblock_init(struct meminfo *mi, struct machine_desc *mdesc)
 	if (mdesc->reserve)
 		mdesc->reserve();
 
+	arm_memblock_steal_permitted = false;
 	memblock_allow_resize();
 	memblock_dump_all();
 }
diff --git a/arch/arm/plat-omap/devices.c b/arch/arm/plat-omap/devices.c
index 1971932..60278f4 100644
--- a/arch/arm/plat-omap/devices.c
+++ b/arch/arm/plat-omap/devices.c
@@ -20,6 +20,7 @@
 #include <mach/hardware.h>
 #include <asm/mach-types.h>
 #include <asm/mach/map.h>
+#include <asm/memblock.h>
 
 #include <plat/tc.h>
 #include <plat/board.h>
@@ -164,14 +165,12 @@ void __init omap_dsp_reserve_sdram_memblock(void)
 	if (!size)
 		return;
 
-	paddr = memblock_alloc(size, SZ_1M);
+	paddr = arm_memblock_steal(size, SZ_1M);
 	if (!paddr) {
 		pr_err("%s: failed to reserve %x bytes\n",
 				__func__, size);
 		return;
 	}
-	memblock_free(paddr, size);
-	memblock_remove(paddr, size);
 
 	omap_dsp_phys_mempool_base = paddr;
 }

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Russell King - ARM Linux]

On Fri, Jan 13, 2012 at 02:05:20PM +0000, Russell King - ARM Linux wrote:
> From: Russell King <rmk+kernel@arm.linux.org.uk>
> ARM: Add arm_memblock_steal() to allocate memory away from the kernel
> 
> Several platforms are now using the memblock_alloc+memblock_free+
> memblock_remove trick to obtain memory which won't be mapped in the
> kernel's page tables.  Most platforms do this (correctly) in the
> ->reserve callback.  However, OMAP has started to call these functions
> outside of this callback, and this is extremely unsafe - memory will
> not be unmapped, and could well be given out after memblock is no
> longer responsible for its management.
> 
> So, provide arm_memblock_steal() to perform this function, and ensure
> that it panic()s if it is used inappropriately.  Convert everyone
> over, including OMAP.
> 
> As a result, OMAP will panic on boot with this change.  OMAP needs to
> be fixed, or 137d105d50 (ARM: OMAP4: Fix errata i688 with MPU
> interconnect barriers.) reverted until such time it can be fixed
> correctly.

Santosh points out that this is only used if the errata i688 option is
enabled, so I've added to this patch to make this config option depend
on BROKEN, marked it as such, and commited the result to my fixes branch.

I'll be planning to push this to Linus sometime on Monday.

> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  arch/arm/include/asm/memblock.h      |    2 ++
>  arch/arm/mach-imx/mach-mx31_3ds.c    |    5 ++---
>  arch/arm/mach-imx/mach-mx31moboard.c |    5 ++---
>  arch/arm/mach-imx/mach-pcm037.c      |    5 ++---
>  arch/arm/mach-omap2/omap-secure.c    |   13 ++-----------
>  arch/arm/mach-omap2/omap4-common.c   |   10 +++-------
>  arch/arm/mm/init.c                   |   17 +++++++++++++++++
>  arch/arm/plat-omap/devices.c         |    5 ++---
>  8 files changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/arm/include/asm/memblock.h b/arch/arm/include/asm/memblock.h
> index b8da2e4..00ca5f9 100644
> --- a/arch/arm/include/asm/memblock.h
> +++ b/arch/arm/include/asm/memblock.h
> @@ -6,4 +6,6 @@ struct machine_desc;
>  
>  extern void arm_memblock_init(struct meminfo *, struct machine_desc *);
>  
> +phys_addr_t arm_memblock_steal(phys_addr_t size, phys_addr_t align);
> +
>  #endif
> diff --git a/arch/arm/mach-imx/mach-mx31_3ds.c b/arch/arm/mach-imx/mach-mx31_3ds.c
> index 89c3325..4d1aab1 100644
> --- a/arch/arm/mach-imx/mach-mx31_3ds.c
> +++ b/arch/arm/mach-imx/mach-mx31_3ds.c
> @@ -36,6 +36,7 @@
>  #include <asm/mach/time.h>
>  #include <asm/memory.h>
>  #include <asm/mach/map.h>
> +#include <asm/memblock.h>
>  #include <mach/common.h>
>  #include <mach/iomux-mx3.h>
>  #include <mach/3ds_debugboard.h>
> @@ -754,10 +755,8 @@ static struct sys_timer mx31_3ds_timer = {
>  static void __init mx31_3ds_reserve(void)
>  {
>  	/* reserve MX31_3DS_CAMERA_BUF_SIZE bytes for mx3-camera */
> -	mx3_camera_base = memblock_alloc(MX31_3DS_CAMERA_BUF_SIZE,
> +	mx3_camera_base = arm_memblock_steal(MX31_3DS_CAMERA_BUF_SIZE,
>  					 MX31_3DS_CAMERA_BUF_SIZE);
> -	memblock_free(mx3_camera_base, MX31_3DS_CAMERA_BUF_SIZE);
> -	memblock_remove(mx3_camera_base, MX31_3DS_CAMERA_BUF_SIZE);
>  }
>  
>  MACHINE_START(MX31_3DS, "Freescale MX31PDK (3DS)")
> diff --git a/arch/arm/mach-imx/mach-mx31moboard.c b/arch/arm/mach-imx/mach-mx31moboard.c
> index b95981d..f225262 100644
> --- a/arch/arm/mach-imx/mach-mx31moboard.c
> +++ b/arch/arm/mach-imx/mach-mx31moboard.c
> @@ -41,6 +41,7 @@
>  #include <asm/mach/arch.h>
>  #include <asm/mach/time.h>
>  #include <asm/mach/map.h>
> +#include <asm/memblock.h>
>  #include <mach/board-mx31moboard.h>
>  #include <mach/common.h>
>  #include <mach/hardware.h>
> @@ -584,10 +585,8 @@ struct sys_timer mx31moboard_timer = {
>  static void __init mx31moboard_reserve(void)
>  {
>  	/* reserve 4 MiB for mx3-camera */
> -	mx3_camera_base = memblock_alloc(MX3_CAMERA_BUF_SIZE,
> +	mx3_camera_base = arm_memblock_steal(MX3_CAMERA_BUF_SIZE,
>  			MX3_CAMERA_BUF_SIZE);
> -	memblock_free(mx3_camera_base, MX3_CAMERA_BUF_SIZE);
> -	memblock_remove(mx3_camera_base, MX3_CAMERA_BUF_SIZE);
>  }
>  
>  MACHINE_START(MX31MOBOARD, "EPFL Mobots mx31moboard")
> diff --git a/arch/arm/mach-imx/mach-pcm037.c b/arch/arm/mach-imx/mach-pcm037.c
> index d7e1516..e48854b 100644
> --- a/arch/arm/mach-imx/mach-pcm037.c
> +++ b/arch/arm/mach-imx/mach-pcm037.c
> @@ -39,6 +39,7 @@
>  #include <asm/mach/arch.h>
>  #include <asm/mach/time.h>
>  #include <asm/mach/map.h>
> +#include <asm/memblock.h>
>  #include <mach/common.h>
>  #include <mach/hardware.h>
>  #include <mach/iomux-mx3.h>
> @@ -680,10 +681,8 @@ struct sys_timer pcm037_timer = {
>  static void __init pcm037_reserve(void)
>  {
>  	/* reserve 4 MiB for mx3-camera */
> -	mx3_camera_base = memblock_alloc(MX3_CAMERA_BUF_SIZE,
> +	mx3_camera_base = arm_memblock_steal(MX3_CAMERA_BUF_SIZE,
>  			MX3_CAMERA_BUF_SIZE);
> -	memblock_free(mx3_camera_base, MX3_CAMERA_BUF_SIZE);
> -	memblock_remove(mx3_camera_base, MX3_CAMERA_BUF_SIZE);
>  }
>  
>  MACHINE_START(PCM037, "Phytec Phycore pcm037")
> diff --git a/arch/arm/mach-omap2/omap-secure.c b/arch/arm/mach-omap2/omap-secure.c
> index 69f3c72..d8f8ef4 100644
> --- a/arch/arm/mach-omap2/omap-secure.c
> +++ b/arch/arm/mach-omap2/omap-secure.c
> @@ -16,6 +16,7 @@
>  #include <linux/memblock.h>
>  
>  #include <asm/cacheflush.h>
> +#include <asm/memblock.h>
>  
>  #include <mach/omap-secure.h>
>  
> @@ -57,20 +58,10 @@ u32 omap_secure_dispatcher(u32 idx, u32 flag, u32 nargs, u32 arg1, u32 arg2,
>  /* Allocate the memory to save secure ram */
>  int __init omap_secure_ram_reserve_memblock(void)
>  {
> -	phys_addr_t paddr;
>  	u32 size = OMAP_SECURE_RAM_STORAGE;
>  
>  	size = ALIGN(size, SZ_1M);
> -	paddr = memblock_alloc(size, SZ_1M);
> -	if (!paddr) {
> -		pr_err("%s: failed to reserve %x bytes\n",
> -				__func__, size);
> -		return -ENOMEM;
> -	}
> -	memblock_free(paddr, size);
> -	memblock_remove(paddr, size);
> -
> -	omap_secure_memblock_base = paddr;
> +	omap_secure_memblock_base = arm_memblock_steal(size, SZ_1M);
>  
>  	return 0;
>  }
> diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
> index bc16c81..40a8fbc 100644
> --- a/arch/arm/mach-omap2/omap4-common.c
> +++ b/arch/arm/mach-omap2/omap4-common.c
> @@ -20,6 +20,7 @@
>  #include <asm/hardware/gic.h>
>  #include <asm/hardware/cache-l2x0.h>
>  #include <asm/mach/map.h>
> +#include <asm/memblock.h>
>  
>  #include <plat/irqs.h>
>  #include <plat/sram.h>
> @@ -61,13 +62,8 @@ static int __init omap_barriers_init(void)
>  		return -ENODEV;
>  
>  	size = ALIGN(PAGE_SIZE, SZ_1M);
> -	paddr = memblock_alloc(size, SZ_1M);
> -	if (!paddr) {
> -		pr_err("%s: failed to reserve 4 Kbytes\n", __func__);
> -		return -ENOMEM;
> -	}
> -	memblock_free(paddr, size);
> -	memblock_remove(paddr, size);
> +	paddr = arm_memblock_steal(size, SZ_1M);
> +
>  	dram_io_desc[0].virtual = OMAP4_DRAM_BARRIER_VA;
>  	dram_io_desc[0].pfn = __phys_to_pfn(paddr);
>  	dram_io_desc[0].length = size;
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index e34ea8a..6ec1226 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -22,6 +22,7 @@
>  #include <linux/memblock.h>
>  
>  #include <asm/mach-types.h>
> +#include <asm/memblock.h>
>  #include <asm/prom.h>
>  #include <asm/sections.h>
>  #include <asm/setup.h>
> @@ -307,6 +308,22 @@ static void arm_memory_present(void)
>  }
>  #endif
>  
> +static bool arm_memblock_steal_permitted = true;
> +
> +phys_addr_t arm_memblock_steal(phys_addr_t size, phys_addr_t align)
> +{
> +	phys_addr_t phys;
> +
> +	if (!arm_memblock_steal_permitted)
> +		panic("arm_memblock_steal used in an unsafe manner\n");
> +
> +	phys = memblock_alloc(size, align);
> +	memblock_free(phys, size);
> +	memblock_remove(phys, size);
> +
> +	return phys;
> +}
> +
>  void __init arm_memblock_init(struct meminfo *mi, struct machine_desc *mdesc)
>  {
>  	int i;
> @@ -349,6 +366,7 @@ void __init arm_memblock_init(struct meminfo *mi, struct machine_desc *mdesc)
>  	if (mdesc->reserve)
>  		mdesc->reserve();
>  
> +	arm_memblock_steal_permitted = false;
>  	memblock_allow_resize();
>  	memblock_dump_all();
>  }
> diff --git a/arch/arm/plat-omap/devices.c b/arch/arm/plat-omap/devices.c
> index 1971932..60278f4 100644
> --- a/arch/arm/plat-omap/devices.c
> +++ b/arch/arm/plat-omap/devices.c
> @@ -20,6 +20,7 @@
>  #include <mach/hardware.h>
>  #include <asm/mach-types.h>
>  #include <asm/mach/map.h>
> +#include <asm/memblock.h>
>  
>  #include <plat/tc.h>
>  #include <plat/board.h>
> @@ -164,14 +165,12 @@ void __init omap_dsp_reserve_sdram_memblock(void)
>  	if (!size)
>  		return;
>  
> -	paddr = memblock_alloc(size, SZ_1M);
> +	paddr = arm_memblock_steal(size, SZ_1M);
>  	if (!paddr) {
>  		pr_err("%s: failed to reserve %x bytes\n",
>  				__func__, size);
>  		return;
>  	}
> -	memblock_free(paddr, size);
> -	memblock_remove(paddr, size);
>  
>  	omap_dsp_phys_mempool_base = paddr;
>  }
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [120113 06:31]:
> On Fri, Jan 13, 2012 at 02:05:20PM +0000, Russell King - ARM Linux wrote:
> > From: Russell King <rmk+kernel@arm.linux.org.uk>
> > ARM: Add arm_memblock_steal() to allocate memory away from the kernel
> > 
> > Several platforms are now using the memblock_alloc+memblock_free+
> > memblock_remove trick to obtain memory which won't be mapped in the
> > kernel's page tables.  Most platforms do this (correctly) in the
> > ->reserve callback.  However, OMAP has started to call these functions
> > outside of this callback, and this is extremely unsafe - memory will
> > not be unmapped, and could well be given out after memblock is no
> > longer responsible for its management.
> > 
> > So, provide arm_memblock_steal() to perform this function, and ensure
> > that it panic()s if it is used inappropriately.  Convert everyone
> > over, including OMAP.
> > 
> > As a result, OMAP will panic on boot with this change.  OMAP needs to
> > be fixed, or 137d105d50 (ARM: OMAP4: Fix errata i688 with MPU
> > interconnect barriers.) reverted until such time it can be fixed
> > correctly.
> 
> Santosh points out that this is only used if the errata i688 option is
> enabled, so I've added to this patch to make this config option depend
> on BROKEN, marked it as such, and commited the result to my fixes branch.
> 
> I'll be planning to push this to Linus sometime on Monday.

Sounds good to me.

Thanks,

Tony

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Felipe Contreras]

On Fri, Jan 13, 2012 at 4:05 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> +phys_addr_t arm_memblock_steal(phys_addr_t size, phys_addr_t align)
> +{
> + ? ? ? phys_addr_t phys;
> +
> + ? ? ? if (!arm_memblock_steal_permitted)
> + ? ? ? ? ? ? ? panic("arm_memblock_steal used in an unsafe manner\n");
> +
> + ? ? ? phys = memblock_alloc(size, align);
> + ? ? ? memblock_free(phys, size);
> + ? ? ? memblock_remove(phys, size);
> +
> + ? ? ? return phys;
> +}

Excellent! I think I suggested a function like that at some point.

But I wonder about the 'align' argument. I think most people just
copy-pasted SZ_1M, although I think originally you suggested SZ_2M,
maybe it would make sense to align 'align' to SZ_1M, or set SZ_1M if
it's 0 for convenience.

Also, what about people that need memblock_alloc_base? AFAIK OMAP 3
secure ram needs to be below certain area (I wonder why the current
code is not handling that).

Cheers.

-- 
Felipe Contreras

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Russell King - ARM Linux]

On Thu, Jan 12, 2012 at 10:42:57AM -0800, Tony Lindgren wrote:
> Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
> (Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
> into devel-stable) merged generic ioremap changes.
> 
> Commit 137d105d50f6e6c373c1aa759f59045e6239cf66
> (ARM: OMAP4: Fix errata i688 with MPU interconnect barriers)
> added a workaround for omap4.
> 
> In order for the errata to work, we now need the following
> patch or else we'll get:
> 
> kernel BUG at mm/vmalloc.c:1134!

Oh my, I've just read this, and I'm extremely annoyed that this even hit
mainline in the first place.  It's utter crap.

It's trying to use memblock to allocate memory _AFTER_ that memory has
been passed on from memblock's control to other allocators.  Calling
memblock_alloc() at *any* initcall is Bad News (it _may_ appear to work
but there's no way for memblock_alloc to tell anything else that the
memory is being re-used.)

Calling it and then trying to reserve it at ->map_io time is also Bad
News - the memory at that point has already been mapped, and if you're
expecting to be able to remap it with different attributes, you're going
to double-map it with differing attributes.  You lose.

Not only that, but it's an abuse of the various callback functions into
machine code.  Don't do it.

By all means, allocate the memory via memblock, but do it in the ->reserve
callback.  It's *exactly* what that callback is there for.  The map it in
the ->map_io callback.

Don't try to be clever and abuse these callbacks.  They aren't named just
for fun and my delectation.  They have *specific* purposes.  Stick to
those purposes in them and don't try to be clever, or you'll be moaned at.

So, NAK.  NAK for the original patch too.  Do it properly.

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Russell King - ARM Linux]

On Thu, Jan 12, 2012 at 08:04:43PM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 12, 2012 at 10:42:57AM -0800, Tony Lindgren wrote:
> > Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
> > (Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
> > into devel-stable) merged generic ioremap changes.
> > 
> > Commit 137d105d50f6e6c373c1aa759f59045e6239cf66
> > (ARM: OMAP4: Fix errata i688 with MPU interconnect barriers)
> > added a workaround for omap4.
> > 
> > In order for the errata to work, we now need the following
> > patch or else we'll get:
> > 
> > kernel BUG at mm/vmalloc.c:1134!
> 
> Oh my, I've just read this, and I'm extremely annoyed that this even hit
> mainline in the first place.  It's utter crap.
> 
> It's trying to use memblock to allocate memory _AFTER_ that memory has
> been passed on from memblock's control to other allocators.  Calling
> memblock_alloc() at *any* initcall is Bad News (it _may_ appear to work
> but there's no way for memblock_alloc to tell anything else that the
> memory is being re-used.)
> 
> Calling it and then trying to reserve it at ->map_io time is also Bad
> News - the memory at that point has already been mapped, and if you're
> expecting to be able to remap it with different attributes, you're going
> to double-map it with differing attributes.  You lose.
> 
> Not only that, but it's an abuse of the various callback functions into
> machine code.  Don't do it.
> 
> By all means, allocate the memory via memblock, but do it in the ->reserve
> callback.  It's *exactly* what that callback is there for.  The map it in
> the ->map_io callback.
> 
> Don't try to be clever and abuse these callbacks.  They aren't named just
> for fun and my delectation.  They have *specific* purposes.  Stick to
> those purposes in them and don't try to be clever, or you'll be moaned at.
> 
> So, NAK.  NAK for the original patch too.  Do it properly.

It seems I missed this detail when I quickly read through the original
patch last September, which is rather unfortunate.

That doesn't stop this being completely the wrong approach though - and
being very very broken as it currently stands.

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Shilimkar, Santosh]

On Thu, Jan 12, 2012 at 9:11 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jan 12, 2012 at 08:04:43PM +0000, Russell King - ARM Linux wrote:
>> On Thu, Jan 12, 2012 at 10:42:57AM -0800, Tony Lindgren wrote:
>> > Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
>> > (Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
>> > into devel-stable) merged generic ioremap changes.
>> >
>> > Commit 137d105d50f6e6c373c1aa759f59045e6239cf66
>> > (ARM: OMAP4: Fix errata i688 with MPU interconnect barriers)
>> > added a workaround for omap4.
>> >
>> > In order for the errata to work, we now need the following
>> > patch or else we'll get:
>> >
>> > kernel BUG at mm/vmalloc.c:1134!
>>
>> Oh my, I've just read this, and I'm extremely annoyed that this even hit
>> mainline in the first place. ?It's utter crap.
>>
>> It's trying to use memblock to allocate memory _AFTER_ that memory has
>> been passed on from memblock's control to other allocators. ?Calling
>> memblock_alloc() at *any* initcall is Bad News (it _may_ appear to work
>> but there's no way for memblock_alloc to tell anything else that the
>> memory is being re-used.)
>>
>> Calling it and then trying to reserve it at ->map_io time is also Bad
>> News - the memory at that point has already been mapped, and if you're
>> expecting to be able to remap it with different attributes, you're going
>> to double-map it with differing attributes. ?You lose.
>>
>> Not only that, but it's an abuse of the various callback functions into
>> machine code. ?Don't do it.
>>
>> By all means, allocate the memory via memblock, but do it in the ->reserve
>> callback. ?It's *exactly* what that callback is there for. ?The map it in
>> the ->map_io callback.
>>
>> Don't try to be clever and abuse these callbacks. ?They aren't named just
>> for fun and my delectation. ?They have *specific* purposes. ?Stick to
>> those purposes in them and don't try to be clever, or you'll be moaned at.
>>
>> So, NAK. ?NAK for the original patch too. ?Do it properly.
>
> It seems I missed this detail when I quickly read through the original
> patch last September, which is rather unfortunate.
>
> That doesn't stop this being completely the wrong approach though - and
> being very very broken as it currently stands.

May be I have missed you point but I thought below
should remove the initial mapping.

memblock_free(paddr, size);
memblock_remove(paddr, size);

This patch actually got under various versions. Indeed the
first version did implement the ->reserve callback method
but then it kept changing and you might have lost track of it.

Regards
Santosh

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Russell King - ARM Linux]

On Thu, Jan 12, 2012 at 09:20:38PM +0100, Shilimkar, Santosh wrote:
> On Thu, Jan 12, 2012 at 9:11 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Thu, Jan 12, 2012 at 08:04:43PM +0000, Russell King - ARM Linux wrote:
> >> On Thu, Jan 12, 2012 at 10:42:57AM -0800, Tony Lindgren wrote:
> >> > Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
> >> > (Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
> >> > into devel-stable) merged generic ioremap changes.
> >> >
> >> > Commit 137d105d50f6e6c373c1aa759f59045e6239cf66
> >> > (ARM: OMAP4: Fix errata i688 with MPU interconnect barriers)
> >> > added a workaround for omap4.
> >> >
> >> > In order for the errata to work, we now need the following
> >> > patch or else we'll get:
> >> >
> >> > kernel BUG at mm/vmalloc.c:1134!
> >>
> >> Oh my, I've just read this, and I'm extremely annoyed that this even hit
> >> mainline in the first place. ?It's utter crap.
> >>
> >> It's trying to use memblock to allocate memory _AFTER_ that memory has
> >> been passed on from memblock's control to other allocators. ?Calling
> >> memblock_alloc() at *any* initcall is Bad News (it _may_ appear to work
> >> but there's no way for memblock_alloc to tell anything else that the
> >> memory is being re-used.)
> >>
> >> Calling it and then trying to reserve it at ->map_io time is also Bad
> >> News - the memory at that point has already been mapped, and if you're
> >> expecting to be able to remap it with different attributes, you're going
> >> to double-map it with differing attributes. ?You lose.
> >>
> >> Not only that, but it's an abuse of the various callback functions into
> >> machine code. ?Don't do it.
> >>
> >> By all means, allocate the memory via memblock, but do it in the ->reserve
> >> callback. ?It's *exactly* what that callback is there for. ?The map it in
> >> the ->map_io callback.
> >>
> >> Don't try to be clever and abuse these callbacks. ?They aren't named just
> >> for fun and my delectation. ?They have *specific* purposes. ?Stick to
> >> those purposes in them and don't try to be clever, or you'll be moaned at.
> >>
> >> So, NAK. ?NAK for the original patch too. ?Do it properly.
> >
> > It seems I missed this detail when I quickly read through the original
> > patch last September, which is rather unfortunate.
> >
> > That doesn't stop this being completely the wrong approach though - and
> > being very very broken as it currently stands.
> 
> May be I have missed you point but I thought below
> should remove the initial mapping.
> 
> memblock_free(paddr, size);
> memblock_remove(paddr, size);

Yes - but _only_ in the ->reserve callback, which was specifically added
to allow this to happen.  It is _only_ possible in _that_ callback and
nowhere else.

And, as I've said, memblock_alloc() elsewhere[*] is _potentially_ _dangerous_
because although it succeeds, the memory has _already_ been handed off to
other kernel allocators, and memblock no longer has control over how the
memory it holds will be used.

> This patch actually got under various versions. Indeed the
> first version did implement the ->reserve callback method
> but then it kept changing and you might have lost track of it.

Which "it" kept changing ?  The ->reserve callback hasn't, neither has
the above condition - and neither will it change.

As I've said, it's the whole point why the ->reserve callback as added: to
allow platforms to mark various regions of RAM as reserved, and remove
regions of RAM from the kernel's control _before_ they get mapped by the
kernel.



* - actually, the latest memblock_alloc() can be called is the map_io
callback, but at that point a call to memblock_free() would be buggy.
So lets not dilute the message.  ->reserve ONLY.

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Shilimkar, Santosh]

On Thu, Jan 12, 2012 at 9:27 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jan 12, 2012 at 09:20:38PM +0100, Shilimkar, Santosh wrote:
>> On Thu, Jan 12, 2012 at 9:11 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Thu, Jan 12, 2012 at 08:04:43PM +0000, Russell King - ARM Linux wrote:
>> >> On Thu, Jan 12, 2012 at 10:42:57AM -0800, Tony Lindgren wrote:
>> >> > Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
>> >> > (Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
>> >> > into devel-stable) merged generic ioremap changes.
>> >> >
>> >> > Commit 137d105d50f6e6c373c1aa759f59045e6239cf66
>> >> > (ARM: OMAP4: Fix errata i688 with MPU interconnect barriers)
>> >> > added a workaround for omap4.
>> >> >
>> >> > In order for the errata to work, we now need the following
>> >> > patch or else we'll get:
>> >> >
>> >> > kernel BUG at mm/vmalloc.c:1134!
>> >>
>> >> Oh my, I've just read this, and I'm extremely annoyed that this even hit
>> >> mainline in the first place. ?It's utter crap.
>> >>
>> >> It's trying to use memblock to allocate memory _AFTER_ that memory has
>> >> been passed on from memblock's control to other allocators. ?Calling
>> >> memblock_alloc() at *any* initcall is Bad News (it _may_ appear to work
>> >> but there's no way for memblock_alloc to tell anything else that the
>> >> memory is being re-used.)
>> >>
>> >> Calling it and then trying to reserve it at ->map_io time is also Bad
>> >> News - the memory at that point has already been mapped, and if you're
>> >> expecting to be able to remap it with different attributes, you're going
>> >> to double-map it with differing attributes. ?You lose.
>> >>
>> >> Not only that, but it's an abuse of the various callback functions into
>> >> machine code. ?Don't do it.
>> >>
>> >> By all means, allocate the memory via memblock, but do it in the ->reserve
>> >> callback. ?It's *exactly* what that callback is there for. ?The map it in
>> >> the ->map_io callback.
>> >>
>> >> Don't try to be clever and abuse these callbacks. ?They aren't named just
>> >> for fun and my delectation. ?They have *specific* purposes. ?Stick to
>> >> those purposes in them and don't try to be clever, or you'll be moaned at.
>> >>
>> >> So, NAK. ?NAK for the original patch too. ?Do it properly.
>> >
>> > It seems I missed this detail when I quickly read through the original
>> > patch last September, which is rather unfortunate.
>> >
>> > That doesn't stop this being completely the wrong approach though - and
>> > being very very broken as it currently stands.
>>
>> May be I have missed you point but I thought below
>> should remove the initial mapping.
>>
>> memblock_free(paddr, size);
>> memblock_remove(paddr, size);
>
> Yes - but _only_ in the ->reserve callback, which was specifically added
> to allow this to happen. ?It is _only_ possible in _that_ callback and
> nowhere else.
>
> And, as I've said, memblock_alloc() elsewhere[*] is _potentially_ _dangerous_
> because although it succeeds, the memory has _already_ been handed off to
> other kernel allocators, and memblock no longer has control over how the
> memory it holds will be used.
>
>> This patch actually got under various versions. Indeed the
>> first version did implement the ->reserve callback method
>> but then it kept changing and you might have lost track of it.
>
> Which "it" kept changing ? ?The ->reserve callback hasn't, neither has
> the above condition - and neither will it change.
>
I mean my patch and not the ->reserve callback.

> As I've said, it's the whole point why the ->reserve callback as added: to
> allow platforms to mark various regions of RAM as reserved, and remove
> regions of RAM from the kernel's control _before_ they get mapped by the
> kernel.
>
>
>
> * - actually, the latest memblock_alloc() can be called is the map_io
> callback, but at that point a call to memblock_free() would be buggy.
> So lets not dilute the message. ?->reserve ONLY.

OK. Point taken.

Regards
Santosh

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Russell King - ARM Linux]

On Thu, Jan 12, 2012 at 09:32:40PM +0100, Shilimkar, Santosh wrote:
> OK. Point taken.

Can you also explain this in the code:

	size = ALIGN(PAGE_SIZE, SZ_1M);

Isn't that just SZ_1M written in a rather complex way?  A comment may be
a better way of explaining it.

[PATCH] arm: omap4: Fix omap_barriers_init for generic ioremap changes

[Shilimkar, Santosh]

On Thu, Jan 12, 2012 at 10:00 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jan 12, 2012 at 09:32:40PM +0100, Shilimkar, Santosh wrote:
>> OK. Point taken.
>
> Can you also explain this in the code:
>
> ? ? ? ?size = ALIGN(PAGE_SIZE, SZ_1M);
>
> Isn't that just SZ_1M written in a rather complex way? ?A comment may be
> a better way of explaining it.

memblock alloc was failing without the alignment IIRC. Will add comments
about the same.

Regards
Santosh