[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Timur Tabi]

The do_bootm_linux() function was using the same variable ('len') to calculate
the the dtu length and the initrd length, which meant that the initrd length
was incorrect.  This patch creates renames 'len' and 'data' to 'initrd_len'
and 'initrd_data', thereby preventing any future confusion.  It also deletes
'len' and 'data' because the dtu calculations don't actually need them.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 common/cmd_bootm.c |   57 +++++++++++++++++++++++----------------------------
 1 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index 7aae8a6..0c092c7 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -518,11 +518,10 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl
 		int	verify)
 {
 	ulong	sp;
-	ulong	len, checksum;
-	ulong	initrd_start, initrd_end;
+	ulong	checksum;
+	ulong	initrd_start, initrd_end, initrd_data, initrd_len;
 	ulong	cmd_start, cmd_end;
 	ulong	initrd_high;
-	ulong	data;
 	int	initrd_copy_to_ram = 1;
 	char    *cmdline;
 	char	*s;
@@ -626,7 +625,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl
 	/* Look for a '-' which indicates to ignore the ramdisk argument */
 	if (argc >= 3 && strcmp(argv[2], "-") ==  0) {
 			debug ("Skipping initrd\n");
-			len = data = 0;
+			initrd_len = initrd_data = 0;
 		}
 	else
 #endif
@@ -647,13 +646,10 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl
 			do_reset (cmdtp, flag, argc, argv);
 		}
 
-		data = (ulong)&header;
-		len  = sizeof(image_header_t);
-
 		checksum = ntohl(hdr->ih_hcrc);
 		hdr->ih_hcrc = 0;
 
-		if (crc32 (0, (uchar *)data, len) != checksum) {
+		if (crc32 (0, (uchar *) &header, sizeof(image_header_t)) != checksum) {
 			puts ("Bad Header Checksum\n");
 			SHOW_BOOT_PROGRESS (-11);
 			do_reset (cmdtp, flag, argc, argv);
@@ -663,13 +659,13 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl
 
 		print_image_hdr (hdr);
 
-		data = addr + sizeof(image_header_t);
-		len  = ntohl(hdr->ih_size);
+		initrd_data = addr + sizeof(image_header_t);
+		initrd_len  = ntohl(hdr->ih_size);
 
 		if (verify) {
 			ulong csum = 0;
 #if defined(CONFIG_HW_WATCHDOG) || defined(CONFIG_WATCHDOG)
-			ulong cdata = data, edata = cdata + len;
+			ulong cdata = initrd_data, edata = cdata + initrd_len;
 #endif	/* CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG */
 
 			puts ("   Verifying Checksum ... ");
@@ -687,7 +683,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl
 				WATCHDOG_RESET();
 			}
 #else	/* !(CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG) */
-			csum = crc32 (0, (uchar *)data, len);
+			csum = crc32 (0, (uchar *) initrd_data, initrd_len);
 #endif	/* CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG */
 
 			if (csum != ntohl(hdr->ih_dcrc)) {
@@ -718,17 +714,17 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl
 		SHOW_BOOT_PROGRESS (13);
 
 		/* skip kernel length and terminator */
-		data = (ulong)(&len_ptr[2]);
+		initrd_data = (ulong)(&len_ptr[2]);
 		/* skip any additional image length fields */
 		for (i=1; len_ptr[i]; ++i)
-			data += 4;
+			initrd_data += 4;
 		/* add kernel length, and align */
-		data += ntohl(len_ptr[0]);
+		initrd_data += ntohl(len_ptr[0]);
 		if (tail) {
-			data += 4 - tail;
+			initrd_data += 4 - tail;
 		}
 
-		len   = ntohl(len_ptr[1]);
+		initrd_len   = ntohl(len_ptr[1]);
 
 	} else {
 		/*
@@ -736,7 +732,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl
 		 */
 		SHOW_BOOT_PROGRESS (14);
 
-		len = data = 0;
+		initrd_len = initrd_data = 0;
 	}
 
 #ifdef CONFIG_OF_FLAT_TREE
@@ -771,9 +767,8 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl
 
 			checksum = ntohl(hdr->ih_dcrc);
 			addr = (ulong)((uchar *)(hdr) + sizeof(image_header_t));
-			len = ntohl(hdr->ih_size);
 
-			if(checksum != crc32(0, (uchar *)addr, len)) {
+			if(checksum != crc32(0, (uchar *)addr, ntohl(hdr->ih_size))) {
 				printf("ERROR: Flat Device Tree checksum is invalid\n");
 				return;
 			}
@@ -835,16 +830,16 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl
 		}
 	}
 #endif
-	if (!data) {
+	if (!initrd_data) {
 		debug ("No initrd\n");
 	}
 
-	if (data) {
+	if (initrd_data) {
 	    if (!initrd_copy_to_ram) {	/* zero-copy ramdisk support */
-		initrd_start = data;
-		initrd_end = initrd_start + len;
+		initrd_start = initrd_data;
+		initrd_end = initrd_start + initrd_len;
 	    } else {
-		initrd_start  = (ulong)kbd - len;
+		initrd_start  = (ulong)kbd - initrd_len;
 		initrd_start &= ~(4096 - 1);	/* align on page */
 
 		if (initrd_high) {
@@ -865,7 +860,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl
 			nsp &= ~0xF;
 			if (nsp > initrd_high)	/* limit as specified */
 				nsp = initrd_high;
-			nsp -= len;
+			nsp -= initrd_len;
 			nsp &= ~(4096 - 1);	/* align on page */
 			if (nsp >= sp)
 				initrd_start = nsp;
@@ -874,16 +869,16 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl
 		SHOW_BOOT_PROGRESS (12);
 
 		debug ("## initrd at 0x%08lX ... 0x%08lX (len=%ld=0x%lX)\n",
-			data, data + len - 1, len, len);
+			initrd_data, initrd_data + initrd_len - 1, initrd_len, initrd_len);
 
-		initrd_end    = initrd_start + len;
+		initrd_end    = initrd_start + initrd_len;
 		printf ("   Loading Ramdisk to %08lx, end %08lx ... ",
 			initrd_start, initrd_end);
 #if defined(CONFIG_HW_WATCHDOG) || defined(CONFIG_WATCHDOG)
 		{
-			size_t l = len;
+			size_t l = initrd_len;
 			void *to = (void *)initrd_start;
-			void *from = (void *)data;
+			void *from = (void *)initrd_data;
 
 			while (l > 0) {
 				size_t tail = (l > CHUNKSZ) ? CHUNKSZ : l;
@@ -895,7 +890,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl
 			}
 		}
 #else	/* !(CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG) */
-		memmove ((void *)initrd_start, (void *)data, len);
+		memmove ((void *)initrd_start, (void *)initrd_data, initrd_len);
 #endif	/* CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG */
 		puts ("OK\n");
 	    }
-- 
1.4.4

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Wolfgang Denk]

In message <11707043793601-git-send-email-timur@freescale.com> you wrote:
> The do_bootm_linux() function was using the same variable ('len') to calculate
> the the dtu length and the initrd length, which meant that the initrd length
> was incorrect.  This patch creates renames 'len' and 'data' to 'initrd_len'
> and 'initrd_data', thereby preventing any future confusion.  It also deletes
> 'len' and 'data' because the dtu calculations don't actually need them.

I can't parse this. 'len' was used twice, but actually it wasn't used
in the second case, but you renamed the only remaining use it had?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Vor allem kein Gedanke! Nichts ist kompromittierender als ein  Gedan-
ke!            - Friedrich Wilhelm Nietzsche _Der Fall Wagner_ (1888)

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Timur Tabi]

Wolfgang Denk wrote:
> In message <11707043793601-git-send-email-timur@freescale.com> you wrote:
>> The do_bootm_linux() function was using the same variable ('len') to calculate
>> the the dtu length and the initrd length, which meant that the initrd length
>> was incorrect.  This patch creates renames 'len' and 'data' to 'initrd_len'
>> and 'initrd_data', thereby preventing any future confusion.  It also deletes
>> 'len' and 'data' because the dtu calculations don't actually need them.
> 
> I can't parse this. 'len' was used twice, but actually it wasn't used
> in the second case, but you renamed the only remaining use it had?

Sorry, it looks like I have a few typos in my description.  Here's a new one:

"The do_bootm_linux() function was using the same variable ('len') to calculate 
the dtu length and the initrd length.  This meant that the initrd length was 
incorrect when it came time to book the kernel.  This resulted in the inability 
to boot with an initrd on an OF-based kernel.  This patch renames the local 
variables 'len' and 'data' to 'initrd_len' and 'initrd_data', respectively, and 
it uses initrd_len and initrd_data only for initrd calculations.

In addition, there were a few places where 'len' and 'data' where being used as 
temporary variables.  The code was modified to eliminate the need for temporary 
variables."

Does this explain everything?  Do you want me to resubmit the patch with the new 
changelog text?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Kumar Gala]

On Feb 6, 2007, at 9:11 AM, Timur Tabi wrote:

> Wolfgang Denk wrote:
>> In message <11707043793601-git-send-email-timur@freescale.com> you  
>> wrote:
>>> The do_bootm_linux() function was using the same variable ('len')  
>>> to calculate
>>> the the dtu length and the initrd length, which meant that the  
>>> initrd length
>>> was incorrect.  This patch creates renames 'len' and 'data' to  
>>> 'initrd_len'
>>> and 'initrd_data', thereby preventing any future confusion.  It  
>>> also deletes
>>> 'len' and 'data' because the dtu calculations don't actually need  
>>> them.
>>
>> I can't parse this. 'len' was used twice, but actually it wasn't used
>> in the second case, but you renamed the only remaining use it had?
>
> Sorry, it looks like I have a few typos in my description.  Here's  
> a new one:
>
> "The do_bootm_linux() function was using the same variable ('len')  
> to calculate
> the dtu length and the initrd length.  This meant that the initrd  
> length was
> incorrect when it came time to book the kernel.  This resulted in  
> the inability

book -> boot

> to boot with an initrd on an OF-based kernel.  This patch renames  
> the local
> variables 'len' and 'data' to 'initrd_len' and 'initrd_data',  
> respectively, and
> it uses initrd_len and initrd_data only for initrd calculations.
>
> In addition, there were a few places where 'len' and 'data' where  
> being used as
> temporary variables.  The code was modified to eliminate the need  
> for temporary
> variables."
>
> Does this explain everything?  Do you want me to resubmit the patch  
> with the new
> changelog text?

Did you test this with a multi image as well? (kernel + initrd + dtb)

- k

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Timur Tabi]

Kumar Gala wrote:

>> incorrect when it came time to book the kernel.  This resulted in the 
>> inability
> 
> book -> boot

Ugh.

> Did you test this with a multi image as well? (kernel + initrd + dtb)

I've never heard of multi image before.  How do I make one?  The README doesn't 
explain it.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Kumar Gala]

On Feb 6, 2007, at 11:11 AM, Timur Tabi wrote:

> Kumar Gala wrote:
>
>>> incorrect when it came time to book the kernel.  This resulted in  
>>> the inability
>> book -> boot
>
> Ugh.

;)

>> Did you test this with a multi image as well? (kernel + initrd + dtb)
>
> I've never heard of multi image before.  How do I make one?  The  
> README doesn't explain it.

mkimage  -A ppc -O Linux -T multi -C gzip -n 'Linux Multiboot-Image' - 
e 0 -a 0 -d vmlinux.gz:rootfs.ext2.gz:oftree.dtb  kern+fs+dtb.bin

- k

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Timur Tabi]

Kumar Gala wrote:

> mkimage  -A ppc -O Linux -T multi -C gzip -n 'Linux Multiboot-Image' -e 
> 0 -a 0 -d vmlinux.gz:rootfs.ext2.gz:oftree.dtb  kern+fs+dtb.bin

Ok, I'll test it out later this week.  I got something higher priority to finish 
first.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Timur Tabi]

Kumar Gala wrote:

>> I've never heard of multi image before.  How do I make one?  The 
>> README doesn't explain it.
> 
> mkimage  -A ppc -O Linux -T multi -C gzip -n 'Linux Multiboot-Image' -e 
> 0 -a 0 -d vmlinux.gz:rootfs.ext2.gz:oftree.dtb  kern+fs+dtb.bin

I need some more help getting the multi-image to work.  I have the following files:

vmlinux.bin.gz (built myself)
rootfs.ext2.gz.uboot (from the BSP, because I don't know how to build a root 
file system.  I don't know what the ".uboot" is for.)
mpc8349emitx.dtb (built myself)

I create the multi image per you instructions, tftp them to $loadaddr (200000), 
and do "bootm $loadaddr".  I get this output:

bootm $loadaddr
## Booting image at 00200000 ...
    Image Name:   Linux Multiboot-Image
    Created:      2007-02-19  23:46:03 UTC
    Image Type:   PowerPC Linux Multi-File Image (gzip compressed)
    Data Size:    20407814 Bytes = 19.5 MB
    Load Address: 00000000
    Entry Point:  00000000
    Contents:
    Image 0:  1416615 Bytes =  1.4 MB
    Image 1: 18987128 Bytes = 18.1 MB
    Image 2:     4054 Bytes =  4 kB
    Verifying Checksum ... OK
    Uncompressing Multi-File Image ... OK


U-Boot 1.2.0-g2e892316-dirty (Feb  9 2007 - 15:56:37) MPC83XX


So the image appears to be okay, but it doesn't boot at all.  Is there something 
obvious I'm doing wrong, or will I have to debug this?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Kumar Gala]

On Feb 19, 2007, at 5:51 PM, Timur Tabi wrote:

> Kumar Gala wrote:
>
>>> I've never heard of multi image before.  How do I make one?  The  
>>> README doesn't explain it.
>> mkimage  -A ppc -O Linux -T multi -C gzip -n 'Linux Multiboot- 
>> Image' -e 0 -a 0 -d vmlinux.gz:rootfs.ext2.gz:oftree.dtb  kern+fs 
>> +dtb.bin
>
> I need some more help getting the multi-image to work.  I have the  
> following files:
>
> vmlinux.bin.gz (built myself)
> rootfs.ext2.gz.uboot (from the BSP, because I don't know how to  
> build a root file system.  I don't know what the ".uboot" is for.)
> mpc8349emitx.dtb (built myself)
>
> I create the multi image per you instructions, tftp them to  
> $loadaddr (200000), and do "bootm $loadaddr".  I get this output:
>
> bootm $loadaddr
> ## Booting image at 00200000 ...
>    Image Name:   Linux Multiboot-Image
>    Created:      2007-02-19  23:46:03 UTC
>    Image Type:   PowerPC Linux Multi-File Image (gzip compressed)
>    Data Size:    20407814 Bytes = 19.5 MB
>    Load Address: 00000000
>    Entry Point:  00000000
>    Contents:
>    Image 0:  1416615 Bytes =  1.4 MB
>    Image 1: 18987128 Bytes = 18.1 MB
>    Image 2:     4054 Bytes =  4 kB
>    Verifying Checksum ... OK
>    Uncompressing Multi-File Image ... OK
>
>
> U-Boot 1.2.0-g2e892316-dirty (Feb  9 2007 - 15:56:37) MPC83XX
>
>
> So the image appears to be okay, but it doesn't boot at all.  Is  
> there something obvious I'm doing wrong, or will I have to debug this?

Sounds right, I'd double check that your rootfs.ext2.gz.uboot isn't  
already a uImage, but the actual ext2 image gziped.

Also, can you boot these three files if you keep them separate?  If  
so that makes me think a bug in the bootm changes.

- k

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Timur Tabi]

Kumar Gala wrote:

> Sounds right, I'd double check that your rootfs.ext2.gz.uboot isn't 
> already a uImage, but the actual ext2 image gziped.

Yes, it is a uImage.  I guess I need to find the original now.

Also, I was wondering if 200000 is too low of a load address.  Doesn't the 
kernel lock that memory or something?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Wolfgang Denk]

In message <45DB2370.6090804@freescale.com> you wrote:
> 
> Also, I was wondering if 200000 is too low of a load address.  Doesn't the 
> kernel lock that memory or something?

If you hve a big kernel, that's definitely too low.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"What the scientists have in their briefcases is terrifying."
- Nikita Khrushchev

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Timur Tabi]

Wolfgang Denk wrote:
> In message <45DB2370.6090804@freescale.com> you wrote:
>> Also, I was wondering if 200000 is too low of a load address.  Doesn't the 
>> kernel lock that memory or something?
> 
> If you hve a big kernel, that's definitely too low.

I moved it to 400000, and now the kernel starts to boot, but it panics.

bootm $loadaddr
## Booting image at 00400000 ...
    Image Name:   Linux Multiboot-Image
    Created:      2007-02-20  17:28:04 UTC
    Image Type:   PowerPC Linux Multi-File Image (gzip compressed)
    Data Size:    20407878 Bytes = 19.5 MB
    Load Address: 00000000
    Entry Point:  00000000
    Contents:
    Image 0:  1416615 Bytes =  1.4 MB
    Image 1: 18987192 Bytes = 18.1 MB
    Image 2:     4054 Bytes =  4 kB
    Verifying Checksum ... OK
    Uncompressing Multi-File Image ... OK
    Loading Ramdisk to 0ed59000, end 0ff748b8 ... OK
    Loading Device Tree to 0ed56000, end 0ed56fd5 ... Using MPC834x ITX machine 
description
Linux version 2.6.20-gc2944612 (b04825 at ld0169-tx32) (gcc version 4.0.2) #1 Mon 
Feb 19 17:22:35 CST 2007
Found initrd at 0xced59000:0xcff748b8

[snip]

Kernel command line: root=/dev/ram rw console=ttyS0,115200

[snip]

md: Autodetecting RAID arrays.
md: autorun ...
md: ... autorun DONE.
RAMDISK: Couldn't find valid RAM disk image starting at 0.
No filesystem could mount root, tried:  ext3 ext2 msdos vfat
Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(1,0)

Now here's something interesting.  The output from "Using MPC834x ITX machine 
description" and "PID hash table entries: 1024 (order: 10, 4096 bytes)" is 
actually displayed twice.  It's as if the kernel spontaneously reboot after 
display the PID line, and then on the second boot it continues as normal.  Is 
this what really happens, or is it some kind of early printk() funkiness?

BTW, the above happens with or without my patch.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Timur Tabi]

Wolfgang Denk wrote:
> In message <45DB2370.6090804@freescale.com> you wrote:
>> Also, I was wondering if 200000 is too low of a load address.  Doesn't the 
>> kernel lock that memory or something?
> 
> If you hve a big kernel, that's definitely too low.

I had to set loadaddr to 800000 and ramdisk_size to 80000 to get it to work.

I can confirm that multi-images work both with and without my patch.  So if 
there are no objections, please apply this patch.  Thanks.


-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Wolfgang Denk]

In message <45DB7ED5.5000306@freescale.com> you wrote:
> 
> I can confirm that multi-images work both with and without my patch.  So if 
> there are no objections, please apply this patch.  Thanks.

Since it doesn't fix a problem I see  little  reason  to  change  the
code.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Vulcans never bluff.
	-- Spock, "The Doomsday Machine", stardate 4202.1

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Timur Tabi]

Wolfgang Denk wrote:
> In message <45DB7ED5.5000306@freescale.com> you wrote:
>> I can confirm that multi-images work both with and without my patch.  So if 
>> there are no objections, please apply this patch.  Thanks.
> 
> Since it doesn't fix a problem I see  little  reason  to  change  the
> code.

What are you talking about?  Without this patch, you cannot have an initrd with 
an OF-enable kernel!  It won't work!  There's a bug in do_bootm_linux(), and my 
patch fixes it.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Timur Tabi]

Wolfgang Denk wrote:
> In message <45DB7ED5.5000306@freescale.com> you wrote:
>> I can confirm that multi-images work both with and without my patch.  So if 
>> there are no objections, please apply this patch.  Thanks.
> 
> Since it doesn't fix a problem I see  little  reason  to  change  the
> code.

Wolfgang, I didn't get a response from you about my last email on this thread, 
so I just want to make sure that you understand that my patch, "Fix initrd 
length miscalculation in bootm command," does fix a real bug in do_bootm_linux().

Here's the changelog for the patch:

"The do_bootm_linux() function was using the same variable ('len') to calculate
the the dtu length and the initrd length, which meant that the initrd length
was incorrect.  This patch creates renames 'len' and 'data' to 'initrd_len'
and 'initrd_data', thereby preventing any future confusion.  It also deletes
'len' and 'data' because the dtu calculations don't actually need them."

The tracking number is DNX#2007020542000011.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Wolfgang Denk]

In message <45E460E4.2080004@freescale.com> you wrote:
> Wolfgang Denk wrote:
> > In message <45DB7ED5.5000306@freescale.com> you wrote:
> >> I can confirm that multi-images work both with and without my patch.  So if 
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> there are no objections, please apply this patch.  Thanks.
> > 
> > Since it doesn't fix a problem I see  little  reason  to  change  the
> > code.
> 
> Wolfgang, I didn't get a response from you about my last email on this thread, 

Been sick, trying to recover, even less time than normal :-(

> so I just want to make sure that you understand that my patch, "Fix initrd 
> length miscalculation in bootm command," does fix a real bug in do_bootm_linux().
> 
> Here's the changelog for the patch:

I remember the changelog, but your statement that "multi-images  work
both  with  and without" your patch. Confuses me. If it works without
your patch, then what's the problem?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The human race is faced with a cruel choice: work  or  daytime  tele-
vision.

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Timur Tabi]

Wolfgang Denk wrote:

> I remember the changelog, but your statement that "multi-images  work
> both  with  and without" your patch. Confuses me. If it works without
> your patch, then what's the problem?

Now that I think about it, I guess that statement doesn't make any sense.  I was trying to 
say that I didn't *break* multi-images.  The patch fixes a problem with initrd + OF.  If 
you're not using an initrd, then my patch doesn't change anything, so I just wanted to 
make sure that things that worked before my patch still work after my patch.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Timur Tabi]

Timur Tabi wrote:

> Now that I think about it, I guess that statement doesn't make any sense.  I was trying to 
> say that I didn't *break* multi-images.  The patch fixes a problem with initrd + OF.  If 
> you're not using an initrd, then my patch doesn't change anything, so I just wanted to 
> make sure that things that worked before my patch still work after my patch.

Wolfgang,

I just wanted to make sure that you understood that this patch does indeed fix a real and 
severe problem with U-Boot, and therefore should be applied.  I submitted this patch over 
a month ago, and it's still not in your repository.  Without this patch, you cannot use an 
  initrd on any ARCH=powerpc kernel.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Johns Daniel]

On 3/16/07, Timur Tabi <timur@freescale.com> wrote:
> Timur Tabi wrote:
>
> > Now that I think about it, I guess that statement doesn't make any sense.  I was trying to
> > say that I didn't *break* multi-images.  The patch fixes a problem with initrd + OF.  If
> > you're not using an initrd, then my patch doesn't change anything, so I just wanted to
> > make sure that things that worked before my patch still work after my patch.
>
> Wolfgang,
>
> I just wanted to make sure that you understood that this patch does indeed fix a real and
> severe problem with U-Boot, and therefore should be applied.  I submitted this patch over
> a month ago, and it's still not in your repository.  Without this patch, you cannot use an
>   initrd on any ARCH=powerpc kernel.

Just want to confirm that the booting of "arch/powerpc" kernels with
an initrd is currently broken in u-boot 1.2.0. And, that Timur's patch
fixes the issue.

-- Johns

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Johns Daniel]

On 3/23/07, Johns Daniel <johns.daniel@gmail.com> wrote:
> On 3/16/07, Timur Tabi <timur@freescale.com> wrote:
> > Timur Tabi wrote:
> >
> > > Now that I think about it, I guess that statement doesn't make any sense.  I was trying to
> > > say that I didn't *break* multi-images.  The patch fixes a problem with initrd + OF.  If
> > > you're not using an initrd, then my patch doesn't change anything, so I just wanted to
> > > make sure that things that worked before my patch still work after my patch.
> >
> > Wolfgang,
> >
> > I just wanted to make sure that you understood that this patch does indeed fix a real and
> > severe problem with U-Boot, and therefore should be applied.  I submitted this patch over
> > a month ago, and it's still not in your repository.  Without this patch, you cannot use an
> >   initrd on any ARCH=powerpc kernel.
>
> Just want to confirm that the booting of "arch/powerpc" kernels with
> an initrd is currently broken in u-boot 1.2.0. And, that Timur's patch
> fixes the issue.

One more note to add to that. The only part of Timur's patch that is
really important is this bit:
@@ -771,9 +767,8 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl

                       checksum = ntohl(hdr->ih_dcrc);
                       addr = (ulong)((uchar *)(hdr) + sizeof(image_header_t));
-                       len = ntohl(hdr->ih_size);

-                       if(checksum != crc32(0, (uchar *)addr, len)) {
+                       if(checksum != crc32(0, (uchar *)addr,
ntohl(hdr->ih_size))) {
                               printf("ERROR: Flat Device Tree
checksum is invalid\n");
                               return;
                       }

The "len" variable should not be used for the DTB image.

-- Johns

[U-Boot-Users] [PATCH] Fix initrd length miscalculation in bootm command

[Timur Tabi]

Johns Daniel wrote:

> One more note to add to that. The only part of Timur's patch that is
> really important is this bit:
> @@ -771,9 +767,8 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int fl
> 
>                        checksum = ntohl(hdr->ih_dcrc);
>                        addr = (ulong)((uchar *)(hdr) + sizeof(image_header_t));
> -                       len = ntohl(hdr->ih_size);
> 
> -                       if(checksum != crc32(0, (uchar *)addr, len)) {
> +                       if(checksum != crc32(0, (uchar *)addr,
> ntohl(hdr->ih_size))) {
>                                printf("ERROR: Flat Device Tree
> checksum is invalid\n");
>                                return;
>                        }
> 
> The "len" variable should not be used for the DTB image.

One of the goals of my patch is to make it very clear what 'len' is supposed to be.  It is 
only supposed to be the initrd length.  Calling it 'len' is what confused the programmer 
who added the OF code.  He saw 'len' and thought it was a scratch variable.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale