[U-Boot-Users] [PATCH] [new uImage] rework error handling so common functions don't reset

[U-Boot-Users] [PATCH] [new uImage] rework error handling so common functions don't reset

[Kumar Gala]

Changed image_get_ramdisk() to just return NULL on error and have
get_ramdisk() propogate that error to the caller.  It's left to the
caller to call do_reset() if it wants to.

Also moved calling do_reset() in get_fdt() on ppc to a common location.
In the future we will change get_fdt() to return success/failure and
not call do_reset() at all.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---

This is against the new-image branch of u-boot-testing

 common/image.c   |   25 ++++++++++++++++---------
 include/image.h  |    2 +-
 lib_m68k/bootm.c |   15 +++++++++++++--
 lib_ppc/bootm.c  |   46 +++++++++++++++++++++++++++++-----------------
 4 files changed, 59 insertions(+), 29 deletions(-)

diff --git a/common/image.c b/common/image.c
index 46cecef..0a74083 100644
--- a/common/image.c
+++ b/common/image.c
@@ -41,8 +41,6 @@
 #include <logbuff.h>
 #endif

-extern int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]);
-
 #ifdef CONFIG_CMD_BDI
 extern int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]);
 #endif
@@ -324,7 +322,7 @@ const char* image_get_comp_name (uint8_t comp)
  *
  * returns:
  *     pointer to a ramdisk image header, if image was found and valid
- *     otherwise, board is reset
+ *     otherwise, return NULL
  */
 image_header_t* image_get_ramdisk (cmd_tbl_t *cmdtp, int flag,
 		int argc, char *argv[],
@@ -348,13 +346,13 @@ image_header_t* image_get_ramdisk (cmd_tbl_t *cmdtp, int flag,
 	if (!image_check_magic (rd_hdr)) {
 		puts ("Bad Magic Number\n");
 		show_boot_progress (-10);
-		do_reset (cmdtp, flag, argc, argv);
+		return NULL;
 	}

 	if (!image_check_hcrc (rd_hdr)) {
 		puts ("Bad Header Checksum\n");
 		show_boot_progress (-11);
-		do_reset (cmdtp, flag, argc, argv);
+		return NULL;
 	}

 	show_boot_progress (10);
@@ -377,7 +375,7 @@ image_header_t* image_get_ramdisk (cmd_tbl_t *cmdtp, int flag,
 		if (!image_check_dcrc_wd (rd_hdr, CHUNKSZ)) {
 			puts ("Bad Data CRC\n");
 			show_boot_progress (-12);
-			do_reset (cmdtp, flag, argc, argv);
+			return NULL;
 		}
 		puts("OK\n");
 	}
@@ -390,7 +388,7 @@ image_header_t* image_get_ramdisk (cmd_tbl_t *cmdtp, int flag,
 		printf ("No Linux %s Ramdisk Image\n",
 				image_get_arch_name(arch));
 		show_boot_progress (-13);
-		do_reset (cmdtp, flag, argc, argv);
+		return NULL;
 	}

 	return rd_hdr;
@@ -417,9 +415,10 @@ image_header_t* image_get_ramdisk (cmd_tbl_t *cmdtp, int flag,
  *     rd_start and rd_end are set to ramdisk start/end addresses if
  *     ramdisk image is found and valid
  *     rd_start and rd_end are set to 0 if no ramdisk exists
- *     board is reset if ramdisk image is found but corrupted
+ *     return 1 if ramdisk image is found but corrupted
+ *
  */
-void get_ramdisk (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[],
+int get_ramdisk (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[],
 		image_header_t *hdr, int verify, uint8_t arch,
 		ulong *rd_start, ulong *rd_end)
 {
@@ -447,6 +446,12 @@ void get_ramdisk (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[],
 			rd_hdr = image_get_ramdisk (cmdtp, flag, argc, argv,
 						rd_addr, arch, verify);

+			if (rd_hdr == NULL) {
+				*rd_start = 0;
+				*rd_end = 0;
+				return 1;
+			}
+
 			rd_data = image_get_data (rd_hdr);
 			rd_len = image_get_data_size (rd_hdr);

@@ -488,6 +493,8 @@ void get_ramdisk (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[],
 	}
 	debug ("   ramdisk start = 0x%08lx, ramdisk end = 0x%08lx\n",
 			*rd_start, *rd_end);
+
+	return 0;
 }

 #if defined(CONFIG_PPC) || defined(CONFIG_M68K)
diff --git a/include/image.h b/include/image.h
index a67489e..d35b476 100644
--- a/include/image.h
+++ b/include/image.h
@@ -338,7 +338,7 @@ image_header_t* image_get_ramdisk (cmd_tbl_t *cmdtp, int flag,
 		int argc, char *argv[],
 		ulong rd_addr, uint8_t arch, int verify);

-void get_ramdisk (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[],
+int get_ramdisk (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[],
 		image_header_t *hdr, int verify, uint8_t arch,
 		ulong *rd_start, ulong *rd_end);

diff --git a/lib_m68k/bootm.c b/lib_m68k/bootm.c
index da76844..9a98b83 100644
--- a/lib_m68k/bootm.c
+++ b/lib_m68k/bootm.c
@@ -35,6 +35,8 @@

 DECLARE_GLOBAL_DATA_PTR;

+extern int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]);
+
 #define PHYSADDR(x) x

 #define LINUX_MAX_ENVS		256
@@ -51,6 +53,7 @@ void do_bootm_linux(cmd_tbl_t * cmdtp, int flag,

 	ulong rd_data_start, rd_data_end, rd_len;
 	ulong initrd_start, initrd_end;
+	int ret;

 	ulong cmd_start, cmd_end;
 	bd_t *kbd;
@@ -83,8 +86,11 @@ void do_bootm_linux(cmd_tbl_t * cmdtp, int flag,
 	    (void (*)(bd_t *, ulong, ulong, ulong, ulong))image_get_ep (hdr);

 	/* find ramdisk */
-	get_ramdisk (cmdtp, flag, argc, argv, hdr, verify,
-			IH_ARCH_M68K, &rd_data_start, &rd_data_end);
+	ret = get_ramdisk (cmdtp, flag, argc, argv, hdr, verify,
+				IH_ARCH_M68K, &rd_data_start, &rd_data_end);
+
+	if (ret)
+		goto error;

 	rd_len = rd_data_end - rd_data_start;
 	alloc_current = ramdisk_high (alloc_current, rd_data_start, rd_len,
@@ -105,6 +111,11 @@ void do_bootm_linux(cmd_tbl_t * cmdtp, int flag,
 	 */
 	(*kernel) (kbd, initrd_start, initrd_end, cmd_start, cmd_end);
 	/* does not return */
+	return ;
+
+error:
+	do_reset (cmdtp, flag, argc, argv);
+	return ;
 }

 static ulong get_sp (void)
diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c
index 99d32eb..ec98e1b 100644
--- a/lib_ppc/bootm.c
+++ b/lib_ppc/bootm.c
@@ -75,7 +75,7 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag,
 	int	has_of = 0;

 #if defined(CONFIG_OF_LIBFDT)
-	char	*of_flat_tree;
+	char	*of_flat_tree = NULL;

 	/* determine if we are booting w/of */
 	if (argc > 3)
@@ -117,8 +117,11 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag,
 	kernel = (void (*)(bd_t *, ulong, ulong, ulong, ulong))image_get_ep (hdr);

 	/* find ramdisk */
-	get_ramdisk (cmdtp, flag, argc, argv, hdr, verify,
-			IH_ARCH_PPC, &rd_data_start, &rd_data_end);
+	ret = get_ramdisk (cmdtp, flag, argc, argv, hdr, verify,
+				IH_ARCH_PPC, &rd_data_start, &rd_data_end);
+
+	if (ret)
+		goto error;

 	rd_len = rd_data_end - rd_data_start;

@@ -135,18 +138,18 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag,
 		/* pass in dummy initrd info, we'll fix up later */
 		if (fdt_chosen(of_flat_tree, rd_data_start, rd_data_end, 0) < 0) {
 			fdt_error ("/chosen node create failed");
-			do_reset (cmdtp, flag, argc, argv);
+			goto error;
 		}
 #ifdef CONFIG_OF_HAS_UBOOT_ENV
 		if (fdt_env(of_flat_tree) < 0) {
 			fdt_error ("/u-boot-env node create failed");
-			do_reset (cmdtp, flag, argc, argv);
+			goto error;
 		}
 #endif
 #ifdef CONFIG_OF_HAS_BD_T
 		if (fdt_bd_t(of_flat_tree) < 0) {
 			fdt_error ("/bd_t node create failed");
-			do_reset (cmdtp, flag, argc, argv);
+			goto error;
 		}
 #endif
 #ifdef CONFIG_OF_BOARD_SETUP
@@ -179,7 +182,7 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag,
 					initrd_end - initrd_start + 1);
 		if (ret < 0) {
 			printf("fdt_chosen: %s\n", fdt_strerror(ret));
-			do_reset (cmdtp, flag, argc, argv);
+			goto error;
 		}

 		do_fixup_by_path_u32(of_flat_tree, "/chosen",
@@ -221,6 +224,11 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag,
 	 */
 	(*kernel) (kbd, initrd_start, initrd_end, cmd_start, cmd_end);
 	/* does not return */
+	return ;
+
+error:
+	do_reset (cmdtp, flag, argc, argv);
+	return ;
 }

 static ulong get_sp (void)
@@ -304,32 +312,32 @@ static ulong get_fdt (ulong alloc_current,

 			if ((load_start < image_end) && (load_end > image_start)) {
 				fdt_error ("fdt overwritten");
-				do_reset (cmdtp, flag, argc, argv);
+				goto error;
 			}

 			puts ("   Verifying Checksum ... ");
 			if (!image_check_hcrc (fdt_hdr)) {
 				fdt_error ("fdt header checksum invalid");
-				do_reset (cmdtp, flag, argc, argv);
+				goto error;
 			}

 			if (!image_check_dcrc (fdt_hdr)) {
 				fdt_error ("fdt checksum invalid");
-				do_reset (cmdtp, flag, argc, argv);
+				goto error;
 			}
 			puts ("OK\n");

 			if (!image_check_type (fdt_hdr, IH_TYPE_FLATDT)) {
 				fdt_error ("uImage is not a fdt");
-				do_reset (cmdtp, flag, argc, argv);
+				goto error;
 			}
 			if (image_get_comp (fdt_hdr) != IH_COMP_NONE) {
 				fdt_error ("uImage is compressed");
-				do_reset (cmdtp, flag, argc, argv);
+				goto error;
 			}
 			if (fdt_check_header ((char *)image_get_data (fdt_hdr)) != 0) {
 				fdt_error ("uImage data is not a fdt");
-				do_reset (cmdtp, flag, argc, argv);
+				goto error;
 			}

 			memmove ((void *)image_get_load (fdt_hdr),
@@ -339,7 +347,7 @@ static ulong get_fdt (ulong alloc_current,
 			fdt = (char *)image_get_load (fdt_hdr);
 		} else {
 			fdt_error ("Did not find a Flattened Device Tree");
-			do_reset (cmdtp, flag, argc, argv);
+			goto error;
 		}
 		printf ("   Booting using the fdt at 0x%x\n",
 				fdt);
@@ -363,12 +371,12 @@ static ulong get_fdt (ulong alloc_current,

 			if (fdt_check_header (fdt) != 0) {
 				fdt_error ("image is not a fdt");
-				do_reset (cmdtp, flag, argc, argv);
+				goto error;
 			}

 			if (be32_to_cpu (fdt_totalsize (fdt)) != fdt_len) {
 				fdt_error ("fdt size != image size");
-				do_reset (cmdtp, flag, argc, argv);
+				goto error;
 			}
 		} else {
 			debug ("   Did not find a Flattened Device Tree");
@@ -405,7 +413,7 @@ static ulong get_fdt (ulong alloc_current,
 		err = fdt_open_into (fdt, (void *)of_start, of_len);
 		if (err != 0) {
 			fdt_error ("fdt move failed");
-			do_reset (cmdtp, flag, argc, argv);
+			goto error;
 		}
 		puts ("OK\n");

@@ -417,5 +425,9 @@ static ulong get_fdt (ulong alloc_current,
 	}

 	return new_alloc_current;
+
+error:
+	do_reset (cmdtp, flag, argc, argv);
+	return 1;
 }
 #endif
-- 
1.5.3.8

[U-Boot-Users] [PATCH] [new uImage] rework error handling so common functions don't reset

[Wolfgang Denk]

Dear Kumar,

in message <Pine.LNX.4.64.0802181306060.21964@blarg.am.freescale.net> you wrote:
> Changed image_get_ramdisk() to just return NULL on error and have
> get_ramdisk() propogate that error to the caller.  It's left to the
> caller to call do_reset() if it wants to.
> 
> Also moved calling do_reset() in get_fdt() on ppc to a common location.
> In the future we will change get_fdt() to return success/failure and
> not call do_reset() at all.
> 
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>

Just to make my position clear: I expect that you negotiate new-image
related patches more or less directly with Bartek, who I consider  as
kind of "new-image" custodian.

That means that I will not track the state of these patches - if  you
want me to apply something directly, please ring a bell.

Bartek: I would also appreciate if you could merge Kumar's patches in
a local repo and then send me just a pull request when something is
ready to go into u-boot-testing (or even into mainline).

Thanks!

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Q:  What's a light-year?
A:  One-third less calories than a regular year.

[U-Boot-Users] [PATCH] [new uImage] rework error handling so common functions don't reset

[Bartlomiej Sieka]

Wolfgang Denk wrote:
> Dear Kumar,
> 
> in message <Pine.LNX.4.64.0802181306060.21964@blarg.am.freescale.net> you wrote:
>> Changed image_get_ramdisk() to just return NULL on error and have
>> get_ramdisk() propogate that error to the caller.  It's left to the
>> caller to call do_reset() if it wants to.
>>
>> Also moved calling do_reset() in get_fdt() on ppc to a common location.
>> In the future we will change get_fdt() to return success/failure and
>> not call do_reset() at all.
>>
>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> 
> Just to make my position clear: I expect that you negotiate new-image
> related patches more or less directly with Bartek, who I consider  as
> kind of "new-image" custodian.
> 
> That means that I will not track the state of these patches - if  you
> want me to apply something directly, please ring a bell.
> 
> Bartek: I would also appreciate if you could merge Kumar's patches in
> a local repo and then send me just a pull request when something is
> ready to go into u-boot-testing (or even into mainline).

OK, will coordinate new uImage stuff that gets posted to the list, and 
request pulling as needed.

Regards,
Bartlomiej

[U-Boot-Users] [PATCH] [new uImage] rework error handling so common functions don't reset

[Kumar Gala]

On Feb 22, 2008, at 8:36 AM, Bartlomiej Sieka wrote:

> Wolfgang Denk wrote:
>> Dear Kumar,
>> in message <Pine.LNX. 
>> 4.64.0802181306060.21964 at blarg.am.freescale.net> you wrote:
>>> Changed image_get_ramdisk() to just return NULL on error and have
>>> get_ramdisk() propogate that error to the caller.  It's left to the
>>> caller to call do_reset() if it wants to.
>>>
>>> Also moved calling do_reset() in get_fdt() on ppc to a common  
>>> location.
>>> In the future we will change get_fdt() to return success/failure and
>>> not call do_reset() at all.
>>>
>>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>> Just to make my position clear: I expect that you negotiate new-image
>> related patches more or less directly with Bartek, who I consider  as
>> kind of "new-image" custodian.
>> That means that I will not track the state of these patches - if  you
>> want me to apply something directly, please ring a bell.
>> Bartek: I would also appreciate if you could merge Kumar's patches in
>> a local repo and then send me just a pull request when something is
>> ready to go into u-boot-testing (or even into mainline).
>
> OK, will coordinate new uImage stuff that gets posted to the list,  
> and request pulling as needed.

Works for me.  I'll look at updating my patches based on the new code  
in the branch.

- k