arm: omap4: Fix omap_barriers_init for generic ioremap changes

PATCH New Tony Lindgren 5 participants 16 messages Jan 12, 2012 – Jan 13, 2012

Subsystems (4)

Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape OMAP2+ support

lore

Share & tools

lore

Tony Lindgren

author maintainer

2012-01-12 18:42 UTC #0 lore raw

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

arch/arm/mach-omap2/common.h +1 -0

@@ -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

arch/arm/mach-omap2/io.c +5 -0

@@ -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

arch/arm/mach-omap2/omap4-common.c +6 -2

@@ -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)

Shilimkar, Santosh

2012-01-12 19:26 UTC #1 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

Quoted 8 lines · 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.

Quoted 7 lines

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

Thanks Tony.

Looks good to me.

Acked-by Santosh Shilimkar <[email protected]>

Nicolas Pitre

2012-01-12 19:44 UTC #2 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

In reply to Tony Lindgren

Quoted 28 lines · 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 <[email protected]>
arch/arm/mach-omap2/common.h +1 -0
@@ -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

arch/arm/mach-omap2/io.c void __init omapam33xx_map_common_io(void) · line 314

Quoted 9 lines

arch/arm/mach-omap2/io.c +5 -0

@@ -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

Nicolas Pitre

2012-01-12 19:48 UTC #3 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

Quoted 42 lines · Nicolas Pitre

Quoted 28 lines · 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 <[email protected]>
arch/arm/mach-omap2/common.h +1 -0
@@ -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
arch/arm/mach-omap2/io.c void __init omapam33xx_map_common_io(void) · line 314
Quoted 9 lines
arch/arm/mach-omap2/io.c +5 -0
@@ -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?

Quoted 2 lines

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

Tony Lindgren

author maintainer

2012-01-12 19:59 UTC #4 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

Quoted 8 lines · Nicolas Pitre

Quoted 5 lines · Nicolas Pitre
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 <[email protected]>
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 <[email protected]>

arch/arm/mach-omap2/common.h +1 -0

@@ -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

arch/arm/mach-omap2/io.c +1 -0

@@ -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

arch/arm/mach-omap2/omap4-common.c +7 -6

@@ -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)

Russell King - ARM Linux

2012-01-13 14:05 UTC #5 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

Quoted 11 lines · Tony Lindgren

Quoted 8 lines · Nicolas Pitre
Quoted 5 lines · Nicolas Pitre
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 <[email protected]>
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 <[email protected]>

---
 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(-)

arch/arm/include/asm/memblock.h +2 -0

index b8da2e4..00ca5f9 100644
@@ -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

arch/arm/mach-imx/mach-mx31_3ds.c +2 -3

index 89c3325..4d1aab1 100644
@@ -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)")

arch/arm/mach-imx/mach-mx31moboard.c +2 -3

index b95981d..f225262 100644
@@ -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")

arch/arm/mach-imx/mach-pcm037.c +2 -3

index d7e1516..e48854b 100644
@@ -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")

arch/arm/mach-omap2/omap-secure.c +2 ~~-11~~

index 69f3c72..d8f8ef4 100644
@@ -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;
 }

arch/arm/mach-omap2/omap4-common.c +3 -7

index bc16c81..40a8fbc 100644
@@ -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;

arch/arm/mm/init.c +18 -0

index e34ea8a..6ec1226 100644
@@ -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();
 }

arch/arm/plat-omap/devices.c +2 -3

index 1971932..60278f4 100644
@@ -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;
 }

Russell King - ARM Linux

2012-01-13 15:04 UTC #6 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

Quoted 11 lines · Russell King - ARM Linux

From: Russell King <[email protected]>
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.

Quoted 8 lines

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.

Quoted 235 lines

Signed-off-by Russell King <[email protected]>

---
 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(-)

arch/arm/include/asm/memblock.h +2 -0

index b8da2e4..00ca5f9 100644
@@ -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

arch/arm/mach-imx/mach-mx31_3ds.c +2 -3

index 89c3325..4d1aab1 100644
@@ -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)")

arch/arm/mach-imx/mach-mx31moboard.c +2 -3

index b95981d..f225262 100644
@@ -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")

arch/arm/mach-imx/mach-pcm037.c +2 -3

index d7e1516..e48854b 100644
@@ -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")

arch/arm/mach-omap2/omap-secure.c +2 ~~-11~~

index 69f3c72..d8f8ef4 100644
@@ -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;
 }

arch/arm/mach-omap2/omap4-common.c +3 -7

index bc16c81..40a8fbc 100644
@@ -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;

arch/arm/mm/init.c +18 -0

index e34ea8a..6ec1226 100644
@@ -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();
 }

arch/arm/plat-omap/devices.c +2 -3

index 1971932..60278f4 100644
@@ -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

Tony Lindgren

author maintainer

2012-01-13 16:44 UTC #7 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

Quoted 21 lines · Russell King - ARM Linux

Quoted 11 lines · Russell King - ARM Linux
From: Russell King <[email protected]>
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.
Quoted 8 lines
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.

Quoted 5 lines

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

Felipe Contreras

2012-01-13 17:12 UTC #8 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

In reply to Russell King - ARM Linux

Review

Quoted 13 lines · Russell King - ARM Linux

(quoted diff) +13 -0

+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.

Signature · 1 line

Felipe Contreras

Russell King - ARM Linux

2012-01-12 20:04 UTC #9 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

In reply to Tony Lindgren

Quoted 4 lines · Tony Lindgren

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

Quoted 8 lines

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.

Russell King - ARM Linux

2012-01-12 20:11 UTC #10 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

Quoted 35 lines · Russell King - ARM Linux

Quoted 4 lines · Tony Lindgren
Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
(Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
into devel-stable) merged generic ioremap changes.
Quoted 8 lines
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.

Quoted 5 lines

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.

Shilimkar, Santosh

2012-01-12 20:20 UTC #11 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

Quoted 40 lines · Russell King - ARM Linux

Quoted 39 lines · Russell King - ARM Linux
Quoted 4 lines · Tony Lindgren
Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
(Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
into devel-stable) merged generic ioremap changes.
Quoted 8 lines
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.

Quoted 7 lines

Quoted 1 line
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

Russell King - ARM Linux

2012-01-12 20:27 UTC #12 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

Quoted 47 lines · Santosh

Quoted 45 lines · Russell King - ARM Linux
Quoted 35 lines · Russell King - ARM Linux
Quoted 4 lines · Tony Lindgren
Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
(Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
into devel-stable) merged generic ioremap changes.
Quoted 8 lines
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.
Quoted 5 lines
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.

Quoted 8 lines

Quoted 2 lines
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.

Quoted 3 lines

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.

Shilimkar, Santosh

2012-01-12 20:32 UTC #13 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

Quoted 66 lines · Russell King - ARM Linux

Quoted 47 lines · Santosh
Quoted 45 lines · Russell King - ARM Linux
Quoted 35 lines · Russell King - ARM Linux
Quoted 4 lines · Tony Lindgren
Commit 73829af71fdb8655e7ba4b5a2a6612ad34a75a11
(Merge branch 'vmalloc' of git://git.linaro.org/people/nico/linux
into devel-stable) merged generic ioremap changes.
Quoted 8 lines
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.
Quoted 5 lines
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.
Quoted 8 lines
Quoted 2 lines
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.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 10 lines

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

Russell King - ARM Linux

2012-01-12 21:00 UTC #14 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

Quoted 1 line · Santosh

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.

Shilimkar, Santosh

2012-01-13 10:35 UTC #15 lore raw

PATCH 1/1 arm: omap4: Fix omap_barriers_init for generic ioremap changes

Quoted 9 lines · Russell King - ARM Linux

Quoted 1 line · Santosh
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