[patch 0/6] cleanup setup.c (take #2)

[patch 0/6] cleanup setup.c (take #2)

[Franck Bui-Huu]

Here's a patchset that clean up arch/mips/kernel/setup.c file.

This second take doesn't use "initrd=xxx@yyy" semantic anymore for
passing initrd memory area through the command line. It sticks
with the old semantic for bootloader compatibilities.

		Franck

Overall diffstats:

 arch/mips/kernel/setup.c |  442 ++++++++++++++++++++--------------------------
 1 files changed, 189 insertions(+), 253 deletions(-)

[PATCH 1/6] setup.c: cleanup bootmem_init()

[Franck Bui-Huu]

This function although doing simple thing is hard to follow. It's
mainly due to:

    - a lot of #ifdef
    - bad local names
    - redundant tests

So this patch try to address these issues. It also do not use
max_pfn global which is marked as an unused exported symbol.

As a bonus side, it's now really easy to see what part of the
code is for no-numa system.

There's also no point to make this function inline.

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/setup.c |  135 +++++++++++++++++++---------------------------
 1 files changed, 57 insertions(+), 78 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 8c2b596..e331e63 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -96,6 +96,12 @@ void __init add_memory_region(phys_t sta
 	int x = boot_mem_map.nr_map;
 	struct boot_mem_map_entry *prev = boot_mem_map.map + x - 1;
 
+	/* Sanity check */
+	if (start + size < start) {
+		printk("Trying to add an invalid memory region, skipped\n");
+		return;
+	}
+
 	/*
 	 * Try to merge with previous entry if any.  This is far less than
 	 * perfect but is sufficient for most real world cases.
@@ -257,15 +263,16 @@ #endif
 	return 0;
 }
 
-#define MAXMEM		HIGHMEM_START
-#define MAXMEM_PFN	PFN_DOWN(MAXMEM)
-
-static inline void bootmem_init(void)
+/*
+ * Initialize the bootmem allocator. It also setup initrd related data
+ * if needed.
+ */
+static void __init bootmem_init(void)
 {
-	unsigned long start_pfn;
 	unsigned long reserved_end = (unsigned long)&_end;
 #ifndef CONFIG_SGI_IP27
-	unsigned long first_usable_pfn;
+	unsigned long highest = 0;
+	unsigned long mapstart = -1UL;
 	unsigned long bootmap_size;
 	int i;
 #endif
@@ -281,7 +288,7 @@ #ifdef CONFIG_BLK_DEV_INITRD
 		unsigned long tmp;
 		u32 *initrd_header;
 
-		tmp = ((reserved_end + PAGE_SIZE-1) & PAGE_MASK) - sizeof(u32) * 2;
+		tmp = PAGE_ALIGN(reserved_end) - sizeof(u32) * 2;
 		if (tmp < reserved_end)
 			tmp += PAGE_SIZE;
 		initrd_header = (u32 *)tmp;
@@ -294,16 +301,15 @@ #ifdef CONFIG_BLK_DEV_INITRD
 	}
 #endif	/* CONFIG_BLK_DEV_INITRD */
 
+#ifndef CONFIG_SGI_IP27
 	/*
-	 * Partially used pages are not usable - thus
-	 * we are rounding upwards.
+	 * reserved_end is now a pfn
 	 */
-	start_pfn = PFN_UP(CPHYSADDR(reserved_end));
+	reserved_end = PFN_UP(CPHYSADDR(reserved_end));
 
-#ifndef CONFIG_SGI_IP27
-	/* Find the highest page frame number we have available.  */
-	max_pfn = 0;
-	first_usable_pfn = -1UL;
+	/*
+	 * Find the highest page frame number we have available.
+	 */
 	for (i = 0; i < boot_mem_map.nr_map; i++) {
 		unsigned long start, end;
 
@@ -312,56 +318,38 @@ #ifndef CONFIG_SGI_IP27
 
 		start = PFN_UP(boot_mem_map.map[i].addr);
 		end = PFN_DOWN(boot_mem_map.map[i].addr
-		      + boot_mem_map.map[i].size);
+				+ boot_mem_map.map[i].size);
 
-		if (start >= end)
+		if (end > highest)
+			highest = end;
+		if (end <= reserved_end)
 			continue;
-		if (end > max_pfn)
-			max_pfn = end;
-		if (start < first_usable_pfn) {
-			if (start > start_pfn) {
-				first_usable_pfn = start;
-			} else if (end > start_pfn) {
-				first_usable_pfn = start_pfn;
-			}
-		}
+		if (start >= mapstart)
+			continue;
+		mapstart = max(reserved_end, start);
 	}
 
 	/*
 	 * Determine low and high memory ranges
 	 */
-	max_low_pfn = max_pfn;
-	if (max_low_pfn > MAXMEM_PFN) {
-		max_low_pfn = MAXMEM_PFN;
-#ifndef CONFIG_HIGHMEM
-		/* Maximum memory usable is what is directly addressable */
-		printk(KERN_WARNING "Warning only %ldMB will be used.\n",
-		       MAXMEM >> 20);
-		printk(KERN_WARNING "Use a HIGHMEM enabled kernel.\n");
+	if (highest > PFN_DOWN(HIGHMEM_START)) {
+#ifdef CONFIG_HIGHMEM
+		highstart_pfn = PFN_DOWN(HIGHMEM_START);
+		highend_pfn = highest;
 #endif
+		highest = PFN_DOWN(HIGHMEM_START);
 	}
 
-#ifdef CONFIG_HIGHMEM
 	/*
-	 * Crude, we really should make a better attempt at detecting
-	 * highstart_pfn
+	 * Initialize the boot-time allocator with low memory only.
 	 */
-	highstart_pfn = highend_pfn = max_pfn;
-	if (max_pfn > MAXMEM_PFN) {
-		highstart_pfn = MAXMEM_PFN;
-		printk(KERN_NOTICE "%ldMB HIGHMEM available.\n",
-		       (highend_pfn - highstart_pfn) >> (20 - PAGE_SHIFT));
-	}
-#endif
-
-	/* Initialize the boot-time allocator with low memory only.  */
-	bootmap_size = init_bootmem(first_usable_pfn, max_low_pfn);
+	bootmap_size = init_bootmem(mapstart, highest);
 
 	/*
 	 * Register fully available low RAM pages with the bootmem allocator.
 	 */
 	for (i = 0; i < boot_mem_map.nr_map; i++) {
-		unsigned long curr_pfn, last_pfn, size;
+		unsigned long start, end, size;
 
 		/*
 		 * Reserve usable memory.
@@ -369,49 +357,37 @@ #endif
 		if (boot_mem_map.map[i].type != BOOT_MEM_RAM)
 			continue;
 
-		/*
-		 * We are rounding up the start address of usable memory:
-		 */
-		curr_pfn = PFN_UP(boot_mem_map.map[i].addr);
-		if (curr_pfn >= max_low_pfn)
-			continue;
-		if (curr_pfn < start_pfn)
-			curr_pfn = start_pfn;
-
-		/*
-		 * ... and at the end of the usable range downwards:
-		 */
-		last_pfn = PFN_DOWN(boot_mem_map.map[i].addr
+		start = PFN_UP(boot_mem_map.map[i].addr);
+		end   = PFN_DOWN(boot_mem_map.map[i].addr
 				    + boot_mem_map.map[i].size);
-
-		if (last_pfn > max_low_pfn)
-			last_pfn = max_low_pfn;
-
 		/*
-		 * Only register lowmem part of lowmem segment with bootmem.
+		 * We are rounding up the start address of usable memory 
+		 * and at the end of the usable range downwards.
 		 */
-		size = last_pfn - curr_pfn;
-		if (curr_pfn > PFN_DOWN(HIGHMEM_START))
-			continue;
-		if (curr_pfn + size - 1 > PFN_DOWN(HIGHMEM_START))
-			size = PFN_DOWN(HIGHMEM_START) - curr_pfn;
-		if (!size)
+		if (start >= max_low_pfn)
 			continue;
+		if (start < reserved_end)
+			start = reserved_end;
+		if (end > max_low_pfn)
+			end = max_low_pfn;
 
 		/*
-		 * ... finally, did all the rounding and playing
-		 * around just make the area go away?
+		 * ... finally, is the area going away?
 		 */
-		if (last_pfn <= curr_pfn)
+		if (end <= start)
 			continue;
+		size = end - start;
 
 		/* Register lowmem ranges */
-		free_bootmem(PFN_PHYS(curr_pfn), PFN_PHYS(size));
-		memory_present(0, curr_pfn, curr_pfn + size - 1);
+		free_bootmem(PFN_PHYS(start), size << PAGE_SHIFT);
+		memory_present(0, start, end);
 	}
 
-	/* Reserve the bootmap memory.  */
-	reserve_bootmem(PFN_PHYS(first_usable_pfn), bootmap_size);
+	/*
+	 * Reserve the bootmap memory.
+	 */
+	reserve_bootmem(PFN_PHYS(mapstart), bootmap_size);
+
 #endif /* CONFIG_SGI_IP27 */
 
 #ifdef CONFIG_BLK_DEV_INITRD
@@ -483,6 +459,9 @@ static void __init arch_mem_init(char **
 	paging_init();
 }
 
+#define MAXMEM		HIGHMEM_START
+#define MAXMEM_PFN	PFN_DOWN(MAXMEM)
+
 static inline void resource_init(void)
 {
 	int i;
-- 
1.4.2.rc2

[PATCH 2/6] setup.c: move initrd code inside dedicated functions

[Franck Bui-Huu]

NUMA specific code could rely on them too.

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/setup.c |  145 ++++++++++++++++++++++++++++------------------
 1 files changed, 87 insertions(+), 58 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index e331e63..8f357c2 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -200,7 +200,12 @@ static inline void parse_cmdline_early(v
 	}
 }
 
-static inline int parse_rd_cmdline(unsigned long* rd_start, unsigned long* rd_end)
+/*
+ * Manage initrd
+ */
+#ifdef CONFIG_BLK_DEV_INITRD
+
+static int __init parse_rd_cmdline(unsigned long *rd_start, unsigned long *rd_end)
 {
 	/*
 	 * "rd_start=0xNNNNNNNN" defines the memory address of an initrd
@@ -263,49 +268,94 @@ #endif
 	return 0;
 }
 
+static unsigned long __init init_initrd(void)
+{
+	unsigned long tmp, end;
+	u32 *initrd_header;
+
+	ROOT_DEV = Root_RAM0;
+
+	if (parse_rd_cmdline(&initrd_start, &initrd_end))
+		return initrd_end;
+	/* 
+	 * Board specific code should have set up initrd_start 
+	 * and initrd_end...
+	 */
+	end = (unsigned long)&_end;
+	tmp = PAGE_ALIGN(end) - sizeof(u32) * 2;
+	if (tmp < end)
+		tmp += PAGE_SIZE;
+	
+	initrd_header = (u32 *)tmp;
+	if (initrd_header[0] == 0x494E5244) {
+		initrd_start = (unsigned long)&initrd_header[2];
+		initrd_end = initrd_start + initrd_header[1];
+	}
+	return initrd_end;
+}
+
+static void __init finalize_initrd(void)
+{
+	unsigned long initrd_size = 
+		(unsigned long)initrd_end - (unsigned long)initrd_start;
+
+	if (initrd_size == 0) {
+		printk(KERN_INFO "Initrd not found or empty");
+		goto check_ko;
+	}
+	if (CPHYSADDR(initrd_end) > PFN_PHYS(max_low_pfn)) {
+		printk("Initrd extends beyond end of memory");
+		goto check_ko;
+	}
+
+	reserve_bootmem(CPHYSADDR(initrd_start), initrd_size);
+	initrd_below_start_ok = 1;
+
+	printk(KERN_INFO "Initial ramdisk at: 0x%p (%lu bytes)\n",
+	       (void *)initrd_start, initrd_size);
+	return;
+check_ko:
+	printk(" - disabling initrd\n");
+	initrd_start = 0;
+	initrd_end = 0;
+}
+
+#else  /* !CONFIG_BLK_DEV_INITRD */
+
+#define init_initrd()		0
+#define finalize_initrd()	do {} while (0)
+
+#endif
+
 /*
  * Initialize the bootmem allocator. It also setup initrd related data
  * if needed.
  */
+#ifdef CONFIG_SGI_IP27
+
 static void __init bootmem_init(void)
 {
-	unsigned long reserved_end = (unsigned long)&_end;
-#ifndef CONFIG_SGI_IP27
+	init_initrd();
+	finalize_initrd();
+}
+
+#else  /* !CONFIG_SGI_IP27 */
+
+static void __init bootmem_init(void)
+{
+	unsigned long reserved_end;
 	unsigned long highest = 0;
 	unsigned long mapstart = -1UL;
 	unsigned long bootmap_size;
 	int i;
-#endif
-#ifdef CONFIG_BLK_DEV_INITRD
-	int initrd_reserve_bootmem = 0;
-
-	/* Board specific code should have set up initrd_start and initrd_end */
- 	ROOT_DEV = Root_RAM0;
-	if (parse_rd_cmdline(&initrd_start, &initrd_end)) {
-		reserved_end = max(reserved_end, initrd_end);
-		initrd_reserve_bootmem = 1;
-	} else {
-		unsigned long tmp;
-		u32 *initrd_header;
-
-		tmp = PAGE_ALIGN(reserved_end) - sizeof(u32) * 2;
-		if (tmp < reserved_end)
-			tmp += PAGE_SIZE;
-		initrd_header = (u32 *)tmp;
-		if (initrd_header[0] == 0x494E5244) {
-			initrd_start = (unsigned long)&initrd_header[2];
-			initrd_end = initrd_start + initrd_header[1];
-			reserved_end = max(reserved_end, initrd_end);
-			initrd_reserve_bootmem = 1;
-		}
-	}
-#endif	/* CONFIG_BLK_DEV_INITRD */
 
-#ifndef CONFIG_SGI_IP27
 	/*
-	 * reserved_end is now a pfn
+	 * Init any data related to initrd. It's a nop if INITRD is
+	 * not selected. Once that done we can determine the low bound
+	 * of usable memory.
 	 */
-	reserved_end = PFN_UP(CPHYSADDR(reserved_end));
+	reserved_end = init_initrd();
+	reserved_end = PFN_UP(CPHYSADDR(max(reserved_end, (unsigned long)&_end)));
 
 	/*
 	 * Find the highest page frame number we have available.
@@ -388,35 +438,14 @@ #endif
 	 */
 	reserve_bootmem(PFN_PHYS(mapstart), bootmap_size);
 
-#endif /* CONFIG_SGI_IP27 */
-
-#ifdef CONFIG_BLK_DEV_INITRD
-	initrd_below_start_ok = 1;
-	if (initrd_start) {
-		unsigned long initrd_size = ((unsigned char *)initrd_end) -
-			((unsigned char *)initrd_start);
-		const int width = sizeof(long) * 2;
-
-		printk("Initial ramdisk at: 0x%p (%lu bytes)\n",
-		       (void *)initrd_start, initrd_size);
-
-		if (CPHYSADDR(initrd_end) > PFN_PHYS(max_low_pfn)) {
-			printk("initrd extends beyond end of memory "
-			       "(0x%0*Lx > 0x%0*Lx)\ndisabling initrd\n",
-			       width,
-			       (unsigned long long) CPHYSADDR(initrd_end),
-			       width,
-			       (unsigned long long) PFN_PHYS(max_low_pfn));
-			initrd_start = initrd_end = 0;
-			initrd_reserve_bootmem = 0;
-		}
-
-		if (initrd_reserve_bootmem)
-			reserve_bootmem(CPHYSADDR(initrd_start), initrd_size);
-	}
-#endif /* CONFIG_BLK_DEV_INITRD  */
+	/*
+	 * Reserve initrd memory if needed.
+	 */
+	finalize_initrd();
 }
 
+#endif	/* CONFIG_SGI_IP27 */
+
 /*
  * arch_mem_init - initialize memory managment subsystem
  *
-- 
1.4.2.rc2

Re: [PATCH 2/6] setup.c: move initrd code inside dedicated functions

[Atsushi Nemoto]

On Wed,  9 Aug 2006 16:52:34 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> +	unsigned long initrd_size = 
> +		(unsigned long)initrd_end - (unsigned long)initrd_start;

While initrd_end and initrd_start are unsigned long, these casts are
redundant.

> +	printk(KERN_INFO "Initial ramdisk at: 0x%p (%lu bytes)\n",
> +	       (void *)initrd_start, initrd_size);

You can use "0x%lx" for initrd_start and remove the cast.  I know this
fragment are copied from corrent code as is, but it would be a good
chance to clean it up.

---
Atsushi Nemoto

Re: [PATCH 2/6] setup.c: move initrd code inside dedicated functions

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Wed,  9 Aug 2006 16:52:34 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>> +	unsigned long initrd_size = 
>> +		(unsigned long)initrd_end - (unsigned long)initrd_start;
> 
> While initrd_end and initrd_start are unsigned long, these casts are
> redundant.
> 

Absolutely. I don't remember why I added these useless casts...

>> +	printk(KERN_INFO "Initial ramdisk at: 0x%p (%lu bytes)\n",
>> +	       (void *)initrd_start, initrd_size);
> 
> You can use "0x%lx" for initrd_start and remove the cast.  I know this
> fragment are copied from corrent code as is, but it would be a good
> chance to clean it up.
> 

You're right.

I'll change that and make a take #3.

Thanks for your comments.

		Franck

[PATCH 3/6] setup.c: remove useless includes.

[Franck Bui-Huu]

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/setup.c |   14 --------------
 1 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 8f357c2..c73bd80 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -10,29 +10,15 @@
  * Copyright (C) 1999 Silicon Graphics, Inc.
  * Copyright (C) 2000 2001, 2002  Maciej W. Rozycki
  */
-#include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/ioport.h>
-#include <linux/sched.h>
-#include <linux/kernel.h>
-#include <linux/mm.h>
 #include <linux/module.h>
-#include <linux/stddef.h>
-#include <linux/string.h>
-#include <linux/unistd.h>
-#include <linux/slab.h>
-#include <linux/user.h>
-#include <linux/utsname.h>
-#include <linux/a.out.h>
 #include <linux/screen_info.h>
 #include <linux/bootmem.h>
 #include <linux/initrd.h>
-#include <linux/major.h>
-#include <linux/kdev_t.h>
 #include <linux/root_dev.h>
 #include <linux/highmem.h>
 #include <linux/console.h>
-#include <linux/mmzone.h>
 #include <linux/pfn.h>
 
 #include <asm/addrspace.h>
-- 
1.4.2.rc2

[PATCH 4/6] setup.c: do not inline functions

[Franck Bui-Huu]

There's no point to inline any functions in setup.c. Let's GCC
doing its job, it's good enough for that now.

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/setup.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index c73bd80..019b564 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -135,7 +135,7 @@ static void __init print_memory_map(void
 	}
 }
 
-static inline void parse_cmdline_early(void)
+static void __init parse_cmdline_early(void)
 {
 	char c = ' ', *to = command_line, *from = saved_command_line;
 	unsigned long start_at, mem_size;
@@ -477,7 +477,7 @@ static void __init arch_mem_init(char **
 #define MAXMEM		HIGHMEM_START
 #define MAXMEM_PFN	PFN_DOWN(MAXMEM)
 
-static inline void resource_init(void)
+static void __init resource_init(void)
 {
 	int i;
 
-- 
1.4.2.rc2

[PATCH 5/6] setup.c: remove MAXMEM macro

[Franck Bui-Huu]

It doens't improve readability.

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/setup.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 019b564..895a357 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -474,9 +474,6 @@ static void __init arch_mem_init(char **
 	paging_init();
 }
 
-#define MAXMEM		HIGHMEM_START
-#define MAXMEM_PFN	PFN_DOWN(MAXMEM)
-
 static void __init resource_init(void)
 {
 	int i;
@@ -498,10 +495,10 @@ static void __init resource_init(void)
 
 		start = boot_mem_map.map[i].addr;
 		end = boot_mem_map.map[i].addr + boot_mem_map.map[i].size - 1;
-		if (start >= MAXMEM)
+		if (start >= HIGHMEM_START)
 			continue;
-		if (end >= MAXMEM)
-			end = MAXMEM - 1;
+		if (end >= HIGHMEM_START)
+			end = HIGHMEM_START - 1;
 
 		res = alloc_bootmem(sizeof(struct resource));
 		switch (boot_mem_map.map[i].type) {
@@ -530,9 +527,6 @@ static void __init resource_init(void)
 	}
 }
 
-#undef MAXMEM
-#undef MAXMEM_PFN
-
 void __init setup_arch(char **cmdline_p)
 {
 	cpu_probe();
-- 
1.4.2.rc2

[PATCH 6/6] setup.c: use early_param() for early command line parsing

[Franck Bui-Huu]

There's no point to rewrite some logic to parse command line
to pass initrd parameters or to declare a user memory area.
We could use instead parse_early_param() that does the same
thing.

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/setup.c |  186 +++++++++++++++++-----------------------------
 1 files changed, 67 insertions(+), 119 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 895a357..7dc643c 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -135,138 +135,54 @@ static void __init print_memory_map(void
 	}
 }
 
-static void __init parse_cmdline_early(void)
-{
-	char c = ' ', *to = command_line, *from = saved_command_line;
-	unsigned long start_at, mem_size;
-	int len = 0;
-	int usermem = 0;
-
-	printk("Determined physical RAM map:\n");
-	print_memory_map();
-
-	for (;;) {
-		/*
-		 * "mem=XXX[kKmM]" defines a memory region from
-		 * 0 to <XXX>, overriding the determined size.
-		 * "mem=XXX[KkmM]@YYY[KkmM]" defines a memory region from
-		 * <YYY> to <YYY>+<XXX>, overriding the determined size.
-		 */
-		if (c == ' ' && !memcmp(from, "mem=", 4)) {
-			if (to != command_line)
-				to--;
-			/*
-			 * If a user specifies memory size, we
-			 * blow away any automatically generated
-			 * size.
-			 */
-			if (usermem == 0) {
-				boot_mem_map.nr_map = 0;
-				usermem = 1;
-			}
-			mem_size = memparse(from + 4, &from);
-			if (*from == '@')
-				start_at = memparse(from + 1, &from);
-			else
-				start_at = 0;
-			add_memory_region(start_at, mem_size, BOOT_MEM_RAM);
-		}
-		c = *(from++);
-		if (!c)
-			break;
-		if (CL_SIZE <= ++len)
-			break;
-		*(to++) = c;
-	}
-	*to = '\0';
-
-	if (usermem) {
-		printk("User-defined physical RAM map:\n");
-		print_memory_map();
-	}
-}
-
 /*
  * Manage initrd
  */
 #ifdef CONFIG_BLK_DEV_INITRD
 
-static int __init parse_rd_cmdline(unsigned long *rd_start, unsigned long *rd_end)
+static int __init rd_start_early(char *p)
 {
-	/*
-	 * "rd_start=0xNNNNNNNN" defines the memory address of an initrd
-	 * "rd_size=0xNN" it's size
-	 */
-	unsigned long start = 0;
-	unsigned long size = 0;
-	unsigned long end;
-	char cmd_line[CL_SIZE];
-	char *start_str;
-	char *size_str;
-	char *tmp;
-
-	strcpy(cmd_line, command_line);
-	*command_line = 0;
-	tmp = cmd_line;
-	/* Ignore "rd_start=" strings in other parameters. */
-	start_str = strstr(cmd_line, "rd_start=");
-	if (start_str && start_str != cmd_line && *(start_str - 1) != ' ')
-		start_str = strstr(start_str, " rd_start=");
-	while (start_str) {
-		if (start_str != cmd_line)
-			strncat(command_line, tmp, start_str - tmp);
-		start = memparse(start_str + 9, &start_str);
-		tmp = start_str + 1;
-		start_str = strstr(start_str, " rd_start=");
-	}
-	if (*tmp)
-		strcat(command_line, tmp);
-
-	strcpy(cmd_line, command_line);
-	*command_line = 0;
-	tmp = cmd_line;
-	/* Ignore "rd_size" strings in other parameters. */
-	size_str = strstr(cmd_line, "rd_size=");
-	if (size_str && size_str != cmd_line && *(size_str - 1) != ' ')
-		size_str = strstr(size_str, " rd_size=");
-	while (size_str) {
-		if (size_str != cmd_line)
-			strncat(command_line, tmp, size_str - tmp);
-		size = memparse(size_str + 8, &size_str);
-		tmp = size_str + 1;
-		size_str = strstr(size_str, " rd_size=");
-	}
-	if (*tmp)
-		strcat(command_line, tmp);
+	unsigned long start = memparse(p, &p);
 
 #ifdef CONFIG_64BIT
 	/* HACK: Guess if the sign extension was forgotten */
 	if (start > 0x0000000080000000 && start < 0x00000000ffffffff)
 		start |= 0xffffffff00000000UL;
 #endif
+	initrd_start = start;
+	initrd_end += start;
+	return 0;
+}
+early_param("rd_start", rd_start_early);
 
-	end = start + size;
-	if (start && end) {
-		*rd_start = start;
-		*rd_end = end;
-		return 1;
-	}
+
+static int __init rd_size_early(char *p)
+{
+	initrd_end += memparse(p, &p);
 	return 0;
 }
+early_param("rd_size", rd_size_early);
+
 
 static unsigned long __init init_initrd(void)
 {
-	unsigned long tmp, end;
+	unsigned long tmp, end, size;
 	u32 *initrd_header;
 
 	ROOT_DEV = Root_RAM0;
 
-	if (parse_rd_cmdline(&initrd_start, &initrd_end))
-		return initrd_end;
 	/* 
-	 * Board specific code should have set up initrd_start 
-	 * and initrd_end...
+	 * Board specific code or command line parser should have
+	 * already set up initrd_start and initrd_end. In these cases
+	 * perfom sanity checks and use them if all looks good.
 	 */
+	size = (unsigned long)initrd_end - (unsigned long)initrd_start;
+	if (initrd_end == 0 || size == 0) {
+		initrd_start = 0;
+		initrd_end = 0;
+	} else
+		return initrd_end;
+
 	end = (unsigned long)&_end;
 	tmp = PAGE_ALIGN(end) - sizeof(u32) * 2;
 	if (tmp < end)
@@ -282,25 +198,25 @@ static unsigned long __init init_initrd(
 
 static void __init finalize_initrd(void)
 {
-	unsigned long initrd_size = 
+	unsigned long size =
 		(unsigned long)initrd_end - (unsigned long)initrd_start;
 
-	if (initrd_size == 0) {
+	if (size == 0) {
 		printk(KERN_INFO "Initrd not found or empty");
-		goto check_ko;
+		goto disable;
 	}
 	if (CPHYSADDR(initrd_end) > PFN_PHYS(max_low_pfn)) {
 		printk("Initrd extends beyond end of memory");
-		goto check_ko;
+		goto disable;
 	}
 
-	reserve_bootmem(CPHYSADDR(initrd_start), initrd_size);
+	reserve_bootmem(CPHYSADDR(initrd_start), size);
 	initrd_below_start_ok = 1;
 
 	printk(KERN_INFO "Initial ramdisk at: 0x%p (%lu bytes)\n",
-	       (void *)initrd_start, initrd_size);
+	       (void *)initrd_start, size);
 	return;
-check_ko:
+disable:
 	printk(" - disabling initrd\n");
 	initrd_start = 0;
 	initrd_end = 0;
@@ -437,8 +353,6 @@ #endif	/* CONFIG_SGI_IP27 */
  *
  *  o plat_mem_setup() detects the memory configuration and will record detected
  *    memory areas using add_memory_region.
- *  o parse_cmdline_early() parses the command line for mem= options which,
- *    iff detected, will override the results of the automatic detection.
  *
  * At this stage the memory configuration of the system is known to the
  * kernel but generic memory managment system is still entirely uninitialized.
@@ -456,19 +370,53 @@ #endif	/* CONFIG_SGI_IP27 */
  * initialization hook for anything else was introduced.
  */
 
-extern void plat_mem_setup(void);
+static int usermem __initdata = 0;
+
+static int __init early_parse_mem(char *p)
+{
+	unsigned long start, size;
+
+	/*
+	 * If a user specifies memory size, we
+	 * blow away any automatically generated
+	 * size.
+	 */
+	if (usermem == 0) {
+		boot_mem_map.nr_map = 0;
+		usermem = 1;
+ 	}
+	start = 0;
+	size = memparse(p, &p);
+	if (*p == '@')
+		start = memparse(p + 1, &p);
+
+	add_memory_region(start, size, BOOT_MEM_RAM);
+	return 0;
+}
+early_param("mem", early_parse_mem);
 
 static void __init arch_mem_init(char **cmdline_p)
 {
+	extern void plat_mem_setup(void);
+
 	/* call board setup routine */
 	plat_mem_setup();
 
+	printk("Determined physical RAM map:\n");
+	print_memory_map();
+
 	strlcpy(command_line, arcs_cmdline, sizeof(command_line));
 	strlcpy(saved_command_line, command_line, COMMAND_LINE_SIZE);
 
 	*cmdline_p = command_line;
 
-	parse_cmdline_early();
+	parse_early_param();
+
+	if (usermem) {
+		printk("User-defined physical RAM map:\n");
+		print_memory_map();
+	}
+
 	bootmem_init();
 	sparse_init();
 	paging_init();
-- 
1.4.2.rc2

Re: [PATCH 6/6] setup.c: use early_param() for early command line parsing

[Franck Bui-Huu]

Franck Bui-Huu wrote:
> There's no point to rewrite some logic to parse command line
> to pass initrd parameters or to declare a user memory area.
> We could use instead parse_early_param() that does the same
> thing.
> 

Funny, Rusty Russel has recently posted a patch to make all
arch use early_param(). See

http://marc.theaimsgroup.com/?l=linux-kernel&m=115528095722115&w=2

So this patch seems come at the right time.

		Franck

cleanup setup.c (take #3)

[Franck Bui-Huu]

Here's a patchset that clean up arch/mips/kernel/setup.c file.

Changes from take #2:
---------------------
- Include Atsushi Nemoto's comments.

Changes from take #1:
---------------------
- don't use "initrd=xxx@yyy" semantic anymore for passing initrd
  memory area through the command line. It sticks with the old 
  semantic for bootloader compatibilities.

		Franck

Overall diffstat:

 arch/mips/kernel/setup.c |  441 ++++++++++++++++++++--------------------------
 1 files changed, 188 insertions(+), 253 deletions(-)

[PATCH 6/6] setup.c: use early_param() for early command line parsing

[Franck Bui-Huu]

There's no point to rewrite some logic to parse command line
to pass initrd parameters or to declare a user memory area.
We could use instead parse_early_param() that does the same
thing.

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/setup.c |  172 ++++++++++++++++------------------------------
 1 files changed, 60 insertions(+), 112 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index fad28a6..3819635 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -135,138 +135,54 @@ static void __init print_memory_map(void
 	}
 }
 
-static void __init parse_cmdline_early(void)
-{
-	char c = ' ', *to = command_line, *from = saved_command_line;
-	unsigned long start_at, mem_size;
-	int len = 0;
-	int usermem = 0;
-
-	printk("Determined physical RAM map:\n");
-	print_memory_map();
-
-	for (;;) {
-		/*
-		 * "mem=XXX[kKmM]" defines a memory region from
-		 * 0 to <XXX>, overriding the determined size.
-		 * "mem=XXX[KkmM]@YYY[KkmM]" defines a memory region from
-		 * <YYY> to <YYY>+<XXX>, overriding the determined size.
-		 */
-		if (c == ' ' && !memcmp(from, "mem=", 4)) {
-			if (to != command_line)
-				to--;
-			/*
-			 * If a user specifies memory size, we
-			 * blow away any automatically generated
-			 * size.
-			 */
-			if (usermem == 0) {
-				boot_mem_map.nr_map = 0;
-				usermem = 1;
-			}
-			mem_size = memparse(from + 4, &from);
-			if (*from == '@')
-				start_at = memparse(from + 1, &from);
-			else
-				start_at = 0;
-			add_memory_region(start_at, mem_size, BOOT_MEM_RAM);
-		}
-		c = *(from++);
-		if (!c)
-			break;
-		if (CL_SIZE <= ++len)
-			break;
-		*(to++) = c;
-	}
-	*to = '\0';
-
-	if (usermem) {
-		printk("User-defined physical RAM map:\n");
-		print_memory_map();
-	}
-}
-
 /*
  * Manage initrd
  */
 #ifdef CONFIG_BLK_DEV_INITRD
 
-static int __init parse_rd_cmdline(unsigned long *rd_start, unsigned long *rd_end)
+static int __init rd_start_early(char *p)
 {
-	/*
-	 * "rd_start=0xNNNNNNNN" defines the memory address of an initrd
-	 * "rd_size=0xNN" it's size
-	 */
-	unsigned long start = 0;
-	unsigned long size = 0;
-	unsigned long end;
-	char cmd_line[CL_SIZE];
-	char *start_str;
-	char *size_str;
-	char *tmp;
-
-	strcpy(cmd_line, command_line);
-	*command_line = 0;
-	tmp = cmd_line;
-	/* Ignore "rd_start=" strings in other parameters. */
-	start_str = strstr(cmd_line, "rd_start=");
-	if (start_str && start_str != cmd_line && *(start_str - 1) != ' ')
-		start_str = strstr(start_str, " rd_start=");
-	while (start_str) {
-		if (start_str != cmd_line)
-			strncat(command_line, tmp, start_str - tmp);
-		start = memparse(start_str + 9, &start_str);
-		tmp = start_str + 1;
-		start_str = strstr(start_str, " rd_start=");
-	}
-	if (*tmp)
-		strcat(command_line, tmp);
-
-	strcpy(cmd_line, command_line);
-	*command_line = 0;
-	tmp = cmd_line;
-	/* Ignore "rd_size" strings in other parameters. */
-	size_str = strstr(cmd_line, "rd_size=");
-	if (size_str && size_str != cmd_line && *(size_str - 1) != ' ')
-		size_str = strstr(size_str, " rd_size=");
-	while (size_str) {
-		if (size_str != cmd_line)
-			strncat(command_line, tmp, size_str - tmp);
-		size = memparse(size_str + 8, &size_str);
-		tmp = size_str + 1;
-		size_str = strstr(size_str, " rd_size=");
-	}
-	if (*tmp)
-		strcat(command_line, tmp);
+	unsigned long start = memparse(p, &p);
 
 #ifdef CONFIG_64BIT
 	/* HACK: Guess if the sign extension was forgotten */
 	if (start > 0x0000000080000000 && start < 0x00000000ffffffff)
 		start |= 0xffffffff00000000UL;
 #endif
+	initrd_start = start;
+	initrd_end += start;
+	return 0;
+}
+early_param("rd_start", rd_start_early);
 
-	end = start + size;
-	if (start && end) {
-		*rd_start = start;
-		*rd_end = end;
-		return 1;
-	}
+
+static int __init rd_size_early(char *p)
+{
+	initrd_end += memparse(p, &p);
 	return 0;
 }
+early_param("rd_size", rd_size_early);
+
 
 static unsigned long __init init_initrd(void)
 {
-	unsigned long tmp, end;
+	unsigned long tmp, end, size;
 	u32 *initrd_header;
 
 	ROOT_DEV = Root_RAM0;
 
-	if (parse_rd_cmdline(&initrd_start, &initrd_end))
-		return initrd_end;
 	/* 
-	 * Board specific code should have set up initrd_start 
-	 * and initrd_end...
+	 * Board specific code or command line parser should have
+	 * already set up initrd_start and initrd_end. In these cases
+	 * perfom sanity checks and use them if all looks good.
 	 */
+	size = initrd_end - initrd_start;
+	if (initrd_end == 0 || size == 0) {
+		initrd_start = 0;
+		initrd_end = 0;
+	} else
+		return initrd_end;
+
 	end = (unsigned long)&_end;
 	tmp = PAGE_ALIGN(end) - sizeof(u32) * 2;
 	if (tmp < end)
@@ -436,8 +352,6 @@ #endif	/* CONFIG_SGI_IP27 */
  *
  *  o plat_mem_setup() detects the memory configuration and will record detected
  *    memory areas using add_memory_region.
- *  o parse_cmdline_early() parses the command line for mem= options which,
- *    iff detected, will override the results of the automatic detection.
  *
  * At this stage the memory configuration of the system is known to the
  * kernel but generic memory managment system is still entirely uninitialized.
@@ -455,19 +369,53 @@ #endif	/* CONFIG_SGI_IP27 */
  * initialization hook for anything else was introduced.
  */
 
-extern void plat_mem_setup(void);
+static int usermem __initdata = 0;
+
+static int __init early_parse_mem(char *p)
+{
+	unsigned long start, size;
+
+	/*
+	 * If a user specifies memory size, we
+	 * blow away any automatically generated
+	 * size.
+	 */
+	if (usermem == 0) {
+		boot_mem_map.nr_map = 0;
+		usermem = 1;
+ 	}
+	start = 0;
+	size = memparse(p, &p);
+	if (*p == '@')
+		start = memparse(p + 1, &p);
+
+	add_memory_region(start, size, BOOT_MEM_RAM);
+	return 0;
+}
+early_param("mem", early_parse_mem);
 
 static void __init arch_mem_init(char **cmdline_p)
 {
+	extern void plat_mem_setup(void);
+
 	/* call board setup routine */
 	plat_mem_setup();
 
+	printk("Determined physical RAM map:\n");
+	print_memory_map();
+
 	strlcpy(command_line, arcs_cmdline, sizeof(command_line));
 	strlcpy(saved_command_line, command_line, COMMAND_LINE_SIZE);
 
 	*cmdline_p = command_line;
 
-	parse_cmdline_early();
+	parse_early_param();
+
+	if (usermem) {
+		printk("User-defined physical RAM map:\n");
+		print_memory_map();
+	}
+
 	bootmem_init();
 	sparse_init();
 	paging_init();
-- 
1.4.2.rc4

[patch 0/6] cleanup setup.c

[Franck Bui-Huu]

Here's a patchset that clean up arch/mips/kernel/setup.c file.

NOTE !  The semantic for initrd command line argument has been
changed mainly because it is now a lot simpler to parse.
Before we were using "rd_start=xxx rd_size=yyy", now the patch uses
"initrd=yyy@xxx". It seems that no default config files use the old
semantic but I'm not sure for bootloader users for example...

Am I allowed to do that ?

		Franck

Overall diffstats:

 arch/mips/kernel/setup.c |  429 +++++++++++++++++++---------------------------
 1 files changed, 177 insertions(+), 252 deletions(-)

[PATCH 6/6] setup.c: use early_param() for early command line parsing

[Franck Bui-Huu]

There's no point to rewrite some logic to parse command line
to pass initrd parameters or to declare a user memory area.
We could use instead parse_early_param() that does the same
thing.

NOTE ! This patch also changes the initrd semantic. Old code
was expecting "rd_start=xxx rd_size=xxx" which uses two
parameters. Now the code expects "initrd=xxx@yyy" which is
really simpler to parse and to use. No default config files
use these parameters anyways but not sure for bootloader's
users...

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 arch/mips/kernel/setup.c |  173 +++++++++++++++-------------------------------
 1 files changed, 55 insertions(+), 118 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index e835737..ec459a1 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -135,124 +135,29 @@ static void __init print_memory_map(void
 	}
 }
 
-static void __init parse_cmdline_early(void)
-{
-	char c = ' ', *to = command_line, *from = saved_command_line;
-	unsigned long start_at, mem_size;
-	int len = 0;
-	int usermem = 0;
-
-	printk("Determined physical RAM map:\n");
-	print_memory_map();
-
-	for (;;) {
-		/*
-		 * "mem=XXX[kKmM]" defines a memory region from
-		 * 0 to <XXX>, overriding the determined size.
-		 * "mem=XXX[KkmM]@YYY[KkmM]" defines a memory region from
-		 * <YYY> to <YYY>+<XXX>, overriding the determined size.
-		 */
-		if (c == ' ' && !memcmp(from, "mem=", 4)) {
-			if (to != command_line)
-				to--;
-			/*
-			 * If a user specifies memory size, we
-			 * blow away any automatically generated
-			 * size.
-			 */
-			if (usermem == 0) {
-				boot_mem_map.nr_map = 0;
-				usermem = 1;
-			}
-			mem_size = memparse(from + 4, &from);
-			if (*from == '@')
-				start_at = memparse(from + 1, &from);
-			else
-				start_at = 0;
-			add_memory_region(start_at, mem_size, BOOT_MEM_RAM);
-		}
-		c = *(from++);
-		if (!c)
-			break;
-		if (CL_SIZE <= ++len)
-			break;
-		*(to++) = c;
-	}
-	*to = '\0';
-
-	if (usermem) {
-		printk("User-defined physical RAM map:\n");
-		print_memory_map();
-	}
-}
-
 /*
  * Manage initrd
  */
 #ifdef CONFIG_BLK_DEV_INITRD
 
-static int __init parse_rd_cmdline(unsigned long *rd_start, unsigned long *rd_end)
+static int __init initrd_early(char *p)
 {
-	/*
-	 * "rd_start=0xNNNNNNNN" defines the memory address of an initrd
-	 * "rd_size=0xNN" it's size
-	 */
-	unsigned long start = 0;
-	unsigned long size = 0;
-	unsigned long end;
-	char cmd_line[CL_SIZE];
-	char *start_str;
-	char *size_str;
-	char *tmp;
-
-	strcpy(cmd_line, command_line);
-	*command_line = 0;
-	tmp = cmd_line;
-	/* Ignore "rd_start=" strings in other parameters. */
-	start_str = strstr(cmd_line, "rd_start=");
-	if (start_str && start_str != cmd_line && *(start_str - 1) != ' ')
-		start_str = strstr(start_str, " rd_start=");
-	while (start_str) {
-		if (start_str != cmd_line)
-			strncat(command_line, tmp, start_str - tmp);
-		start = memparse(start_str + 9, &start_str);
-		tmp = start_str + 1;
-		start_str = strstr(start_str, " rd_start=");
-	}
-	if (*tmp)
-		strcat(command_line, tmp);
-
-	strcpy(cmd_line, command_line);
-	*command_line = 0;
-	tmp = cmd_line;
-	/* Ignore "rd_size" strings in other parameters. */
-	size_str = strstr(cmd_line, "rd_size=");
-	if (size_str && size_str != cmd_line && *(size_str - 1) != ' ')
-		size_str = strstr(size_str, " rd_size=");
-	while (size_str) {
-		if (size_str != cmd_line)
-			strncat(command_line, tmp, size_str - tmp);
-		size = memparse(size_str + 8, &size_str);
-		tmp = size_str + 1;
-		size_str = strstr(size_str, " rd_size=");
-	}
-	if (*tmp)
-		strcat(command_line, tmp);
+	unsigned long start, size;
 
+	size = memparse(p, &p);
+	if (size && *p == '@') {
+		start = memparse(p + 1, &p);
 #ifdef CONFIG_64BIT
-	/* HACK: Guess if the sign extension was forgotten */
-	if (start > 0x0000000080000000 && start < 0x00000000ffffffff)
-		start |= 0xffffffff00000000UL;
+		/* HACK: Guess if the sign extension was forgotten */
+		if (start > 0x0000000080000000 && start < 0x00000000ffffffff)
+			start |= 0xffffffff00000000UL;
 #endif
-
-	end = start + size;
-	if (start && end) {
-		*rd_start = start;
-		*rd_end = end;
-		return 1;
+		initrd_start = start;
+		initrd_end = start + size;
 	}
 	return 0;
 }
+early_param("initrd", initrd_early);
 
 static unsigned long __init init_initrd(void)
 {
@@ -260,7 +165,7 @@ static unsigned long __init init_initrd(
 	u32 *initrd_header;
 
 	ROOT_DEV = Root_RAM0;
-	if (parse_rd_cmdline(&initrd_start, &initrd_end))
+	if (initrd_end)
 		return initrd_end;
 
 	/* 
@@ -282,25 +187,25 @@ static unsigned long __init init_initrd(
 
 static void __init finalize_initrd(void)
 {
-	unsigned long initrd_size = 
+	unsigned long size =
 		(unsigned long)initrd_end - (unsigned long)initrd_start;
 
-	if (initrd_size == 0) {
+	if (size == 0) {
 		printk(KERN_INFO "Initrd not found or empty");
-		goto check_ko;
+		goto disable;
 	}
 	if (CPHYSADDR(initrd_end) > PFN_PHYS(max_low_pfn)) {
 		printk("Initrd extends beyond end of memory");
-		goto check_ko;
+		goto disable;
 	}
 
-	reserve_bootmem(CPHYSADDR(initrd_start), initrd_size);
+	reserve_bootmem(CPHYSADDR(initrd_start), size);
 	initrd_below_start_ok = 1;
 
 	printk(KERN_INFO "Initial ramdisk at: 0x%p (%lu bytes)\n",
-	       (void *)initrd_start, initrd_size);
+	       (void *)initrd_start, size);
 	return;
-check_ko:
+disable:
 	printk(" - disabling initrd\n");
 	initrd_start = 0;
 	initrd_end = 0;
@@ -437,8 +342,6 @@ #endif	/* CONFIG_SGI_IP27 */
  *
  *  o plat_mem_setup() detects the memory configuration and will record detected
  *    memory areas using add_memory_region.
- *  o parse_cmdline_early() parses the command line for mem= options which,
- *    iff detected, will override the results of the automatic detection.
  *
  * At this stage the memory configuration of the system is known to the
  * kernel but generic memory managment system is still entirely uninitialized.
@@ -456,19 +359,53 @@ #endif	/* CONFIG_SGI_IP27 */
  * initialization hook for anything else was introduced.
  */
 
-extern void plat_mem_setup(void);
+static int usermem __initdata = 0;
+
+static int __init early_parse_mem(char *p)
+{
+	unsigned long start, size;
+
+	/*
+	 * If a user specifies memory size, we
+	 * blow away any automatically generated
+	 * size.
+	 */
+	if (usermem == 0) {
+		boot_mem_map.nr_map = 0;
+		usermem = 1;
+ 	}
+	start = 0;
+	size = memparse(p, &p);
+	if (*p == '@')
+		start = memparse(p + 1, &p);
+
+	add_memory_region(start, size, BOOT_MEM_RAM);
+	return 0;
+}
+early_param("mem", early_parse_mem);
 
 static void __init arch_mem_init(char **cmdline_p)
 {
+	extern void plat_mem_setup(void);
+
 	/* call board setup routine */
 	plat_mem_setup();
 
+	printk("Determined physical RAM map:\n");
+	print_memory_map();
+
 	strlcpy(command_line, arcs_cmdline, sizeof(command_line));
 	strlcpy(saved_command_line, command_line, COMMAND_LINE_SIZE);
 
 	*cmdline_p = command_line;
 
-	parse_cmdline_early();
+	parse_early_param();
+
+	if (usermem) {
+		printk("User-defined physical RAM map:\n");
+		print_memory_map();
+	}
+
 	bootmem_init();
 	sparse_init();
 	paging_init();
-- 
1.4.2.rc2

Re: [PATCH 6/6] setup.c: use early_param() for early command line parsing

[Thiemo Seufer]

Franck Bui-Huu wrote:
> There's no point to rewrite some logic to parse command line
> to pass initrd parameters or to declare a user memory area.
> We could use instead parse_early_param() that does the same
> thing.
> 
> NOTE ! This patch also changes the initrd semantic. Old code
> was expecting "rd_start=xxx rd_size=xxx" which uses two
> parameters. Now the code expects "initrd=xxx@yyy" which is
> really simpler to parse and to use. No default config files
> use these parameters anyways but not sure for bootloader's
> users...

This code is there precisely because most mips bootloaders use
rd_start/rd_size. It also is IMHO a bad idea to overload the
semantics of initrd= with both file names and memory locations.


Thiemo

Re: [PATCH 6/6] setup.c: use early_param() for early command line parsing

[Franck Bui-Huu]

Thiemo Seufer wrote:
> Franck Bui-Huu wrote:
>> There's no point to rewrite some logic to parse command line
>> to pass initrd parameters or to declare a user memory area.
>> We could use instead parse_early_param() that does the same
>> thing.
>>
>> NOTE ! This patch also changes the initrd semantic. Old code
>> was expecting "rd_start=xxx rd_size=xxx" which uses two
>> parameters. Now the code expects "initrd=xxx@yyy" which is
>> really simpler to parse and to use. No default config files
>> use these parameters anyways but not sure for bootloader's
>> users...
> 
> This code is there precisely because most mips bootloaders use
> rd_start/rd_size.

OK, I guess we have to stick with this weird semantic...

> It also is IMHO a bad idea to overload the
> semantics of initrd= with both file names and memory locations.
> 

I wasn't aware of any file name usages. Can you give a pointer ?

Thanks

		Franck

Re: [PATCH 6/6] setup.c: use early_param() for early command line parsing

[Thiemo Seufer]

Franck Bui-Huu wrote:
> Thiemo Seufer wrote:
> > Franck Bui-Huu wrote:
> >> There's no point to rewrite some logic to parse command line
> >> to pass initrd parameters or to declare a user memory area.
> >> We could use instead parse_early_param() that does the same
> >> thing.
> >>
> >> NOTE ! This patch also changes the initrd semantic. Old code
> >> was expecting "rd_start=xxx rd_size=xxx" which uses two
> >> parameters. Now the code expects "initrd=xxx@yyy" which is
> >> really simpler to parse and to use. No default config files
> >> use these parameters anyways but not sure for bootloader's
> >> users...
> > 
> > This code is there precisely because most mips bootloaders use
> > rd_start/rd_size.
> 
> OK, I guess we have to stick with this weird semantic...
> 
> > It also is IMHO a bad idea to overload the
> > semantics of initrd= with both file names and memory locations.
> 
> I wasn't aware of any file name usages. Can you give a pointer ?

Documentation/initrd.txt
Documentation/filesystems/ramfs-rootfs-initramfs.txt


Thiemo

Re: [PATCH 6/6] setup.c: use early_param() for early command line parsing

[Franck Bui-Huu]

Thiemo Seufer wrote:
> Franck Bui-Huu wrote:
>> Thiemo Seufer wrote:
>>> Franck Bui-Huu wrote:
>>>> There's no point to rewrite some logic to parse command line
>>>> to pass initrd parameters or to declare a user memory area.
>>>> We could use instead parse_early_param() that does the same
>>>> thing.
>>>>
>>>> NOTE ! This patch also changes the initrd semantic. Old code
>>>> was expecting "rd_start=xxx rd_size=xxx" which uses two
>>>> parameters. Now the code expects "initrd=xxx@yyy" which is
>>>> really simpler to parse and to use. No default config files
>>>> use these parameters anyways but not sure for bootloader's
>>>> users...
>>> This code is there precisely because most mips bootloaders use
>>> rd_start/rd_size.
>> OK, I guess we have to stick with this weird semantic...
>>
>>> It also is IMHO a bad idea to overload the
>>> semantics of initrd= with both file names and memory locations.
>> I wasn't aware of any file name usages. Can you give a pointer ?
> 
> Documentation/initrd.txt
> Documentation/filesystems/ramfs-rootfs-initramfs.txt
> 

I was asking for pointers on MIPS bootloaders which use
initrd=/path/to/initrd...

Anyways, you're talking about specific bootloader's parameters,
aren't you ? I don't know any MIPS bootloaders, but I wouldn't 
expect them to pass their own parameters to the kernel, that 
would be surprising...

What are you suggesting ? kernel_initrd ?

BTW, what do you think about rd_start/rd_size names ?

thanks

		Franck

Re: [PATCH 6/6] setup.c: use early_param() for early command line parsing

[Thiemo Seufer]

Franck Bui-Huu wrote:
[snip]
> >>> It also is IMHO a bad idea to overload the
> >>> semantics of initrd= with both file names and memory locations.
> >> I wasn't aware of any file name usages. Can you give a pointer ?
> > 
> > Documentation/initrd.txt
> > Documentation/filesystems/ramfs-rootfs-initramfs.txt
> > 
> 
> I was asking for pointers on MIPS bootloaders which use
> initrd=/path/to/initrd...

AFAIR arcboot does.

> Anyways, you're talking about specific bootloader's parameters,
> aren't you ? I don't know any MIPS bootloaders, but I wouldn't 
> expect them to pass their own parameters to the kernel, that 
> would be surprising...
>
> What are you suggesting ? kernel_initrd ?
> 
> BTW, what do you think about rd_start/rd_size names ?

Is there a good reason to change it?


Thiemo

Re: [PATCH 6/6] setup.c: use early_param() for early command line parsing

[Atsushi Nemoto]

On Tue, 08 Aug 2006 16:00:30 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> >> NOTE ! This patch also changes the initrd semantic. Old code
> >> was expecting "rd_start=xxx rd_size=xxx" which uses two
> >> parameters. Now the code expects "initrd=xxx@yyy" which is
> >> really simpler to parse and to use. No default config files
> >> use these parameters anyways but not sure for bootloader's
> >> users...
> > 
> > This code is there precisely because most mips bootloaders use
> > rd_start/rd_size.
> 
> OK, I guess we have to stick with this weird semantic...

Maybe you can add something like "initrdmem=xxx@yyy", keeping
"rd_start" and "rd_size" for the backward compatibility.  Just a
thought.

---
Atsushi Nemoto

Re: [PATCH 6/6] setup.c: use early_param() for early command line parsing

[Franck Bui-Huu]

Hi Atsushi,

Atsushi Nemoto wrote:
> On Tue, 08 Aug 2006 16:00:30 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>>>> NOTE ! This patch also changes the initrd semantic. Old code
>>>> was expecting "rd_start=xxx rd_size=xxx" which uses two
>>>> parameters. Now the code expects "initrd=xxx@yyy" which is
>>>> really simpler to parse and to use. No default config files
>>>> use these parameters anyways but not sure for bootloader's
>>>> users...
>>> This code is there precisely because most mips bootloaders use
>>> rd_start/rd_size.
>> OK, I guess we have to stick with this weird semantic...
> 
> Maybe you can add something like "initrdmem=xxx@yyy", keeping
> "rd_start" and "rd_size" for the backward compatibility.  Just a
> thought.
> 

Well that what I was planning when writing this patch but I didn't.
I think that we will end up with two different semantics and the
old one never replaced by the new one... Except if we mark them as
deprecated by showing a warning at boot. What do you think ?

		Franck

Re: [PATCH 6/6] setup.c: use early_param() for early command line parsing

[Atsushi Nemoto]

On Wed, 09 Aug 2006 10:21:22 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> > Maybe you can add something like "initrdmem=xxx@yyy", keeping
> > "rd_start" and "rd_size" for the backward compatibility.  Just a
> > thought.
> 
> Well that what I was planning when writing this patch but I didn't.
> I think that we will end up with two different semantics and the
> old one never replaced by the new one... Except if we mark them as
> deprecated by showing a warning at boot. What do you think ?

While the kernel command line is very limited resource (only 256
chars), I prefer a single short option to specify initrd range, if
available.

But nothing wrong with rd_start and rd_size, and it seems there are
some boot loader expected them already, so removing them would not be
good (especially without some grace period).

I don't care if there were two way to specify initrd range.  It would
be somewhat redundant, but that is usual on "Backword compatibility"
issue, isn't it?  ;-)

---
Atsushi Nemoto

Re: [PATCH 6/6] setup.c: use early_param() for early command line parsing

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Wed, 09 Aug 2006 10:21:22 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>>> Maybe you can add something like "initrdmem=xxx@yyy", keeping
>>> "rd_start" and "rd_size" for the backward compatibility.  Just a
>>> thought.
>> Well that what I was planning when writing this patch but I didn't.
>> I think that we will end up with two different semantics and the
>> old one never replaced by the new one... Except if we mark them as
>> deprecated by showing a warning at boot. What do you think ?
> 
> While the kernel command line is very limited resource (only 256
> chars), I prefer a single short option to specify initrd range, if
> available.
> 
> But nothing wrong with rd_start and rd_size, and it seems there are
> some boot loader expected them already, so removing them would not be
> good (especially without some grace period).
> 
> I don't care if there were two way to specify initrd range.  It would
> be somewhat redundant, but that is usual on "Backword compatibility"
> issue, isn't it?  ;-)
> 

Well, I resent a new version (take #2) of the patchset that uses _only_
"rd_xxx" semantic. I prefer not add some code which isn't going to be
used. Mainly because only bootloaders use this parameter and I guess
they never change the way they pass initrd address. And there won't be
a lot of new bootloaders anyways.

Thanks
		Franck

Re: [PATCH 6/6] setup.c: use early_param() for early command line parsing

[Atsushi Nemoto]

On Fri, 11 Aug 2006 09:36:42 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> Well, I resent a new version (take #2) of the patchset that uses _only_
> "rd_xxx" semantic. I prefer not add some code which isn't going to be
> used. Mainly because only bootloaders use this parameter and I guess
> they never change the way they pass initrd address. And there won't be
> a lot of new bootloaders anyways.

I see.  OK for me.
---
Atsushi Nemoto