[U-Boot-Users] [PATCH 6/17] Reorganize and fix problems (returns) in the bootm command.

[U-Boot-Users] [PATCH 6/17] Reorganize and fix problems (returns) in the bootm command.

[Jerry Van Baren]

Do *NOT* return after the "point of no return" has been passed.
  If something goes wrong, the board must be reset after that point.
Move the "Transferring control to Linux" debug message back to where it
  belongs: just before transferring control to linux.

Signed-off-by: Gerald Van Baren <vanbaren@cideas.com>
---
 common/cmd_bootm.c |  130 +++++++++++++++++++++++++--------------------------
 1 files changed, 64 insertions(+), 66 deletions(-)

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index 25a55bd..25b9d74 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -45,8 +45,8 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
- /*cmd_boot.c*/
- extern int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]);
+/*cmd_boot.c*/
+extern int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]);
 
 #if (CONFIG_COMMANDS & CFG_CMD_DATE) || defined(CONFIG_TIMESTAMP)
 #include <rtc.h>
@@ -763,8 +763,8 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 
 			if ((ntohl(hdr->ih_load) <  ((unsigned long)hdr + ntohl(hdr->ih_size) + sizeof(hdr))) &&
 			   ((ntohl(hdr->ih_load) + ntohl(hdr->ih_size)) > (unsigned long)hdr)) {
-				printf ("ERROR: Load address overwrites Flat Device Tree uImage\n");
-				return;
+				puts ("ERROR: Load address overwrites Flat Device Tree uImage\nMust RESET board to recover\n");
+				do_reset (cmdtp, flag, argc, argv);
 			}
 
 			printf("   Verifying Checksum ... ");
@@ -773,34 +773,34 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 			header.ih_hcrc = 0;
 
 			if(checksum != crc32(0, (uchar *)&header, sizeof(image_header_t))) {
-				printf("ERROR: Flat Device Tree header checksum is invalid\n");
-				return;
+				puts ("ERROR: Flat Device Tree header checksum is invalid\nMust RESET board to recover\n");
+				do_reset (cmdtp, flag, argc, argv);
 			}
 
 			checksum = ntohl(hdr->ih_dcrc);
 			addr = (ulong)((uchar *)(hdr) + sizeof(image_header_t));
 
 			if(checksum != crc32(0, (uchar *)addr, ntohl(hdr->ih_size))) {
-				printf("ERROR: Flat Device Tree checksum is invalid\n");
-				return;
+				puts ("ERROR: Flat Device Tree checksum is invalid\nMust RESET board to recover\n");
+				do_reset (cmdtp, flag, argc, argv);
 			}
 			printf("OK\n");
 
 			if (ntohl(hdr->ih_type) != IH_TYPE_FLATDT) {
-				printf ("ERROR: uImage not Flat Device Tree type\n");
-				return;
+				puts ("ERROR: uImage not Flat Device Tree type\nMust RESET board to recover\n");
+				do_reset (cmdtp, flag, argc, argv);
 			}
 			if (ntohl(hdr->ih_comp) != IH_COMP_NONE) {
-				printf("ERROR: uImage is not uncompressed\n");
-				return;
+				puts ("ERROR: uImage is not uncompressed\nMust RESET board to recover\n");
+				do_reset (cmdtp, flag, argc, argv);
 			}
 #if defined(CONFIG_OF_LIBFDT)
 			if (fdt_check_header(of_flat_tree + sizeof(image_header_t)) != 0) {
 #else
 			if (*((ulong *)(of_flat_tree + sizeof(image_header_t))) != OF_DT_HEADER) {
 #endif
-				printf ("ERROR: uImage data is not a flat device tree\n");
-				return;
+				puts ("ERROR: uImage data is not a flat device tree\nMust RESET board to recover\n");
+				do_reset (cmdtp, flag, argc, argv);
 			}
 
 			memmove((void *)ntohl(hdr->ih_load),
@@ -808,8 +808,8 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 				ntohl(hdr->ih_size));
 			of_flat_tree = (char *)ntohl(hdr->ih_load);
 		} else {
-			printf ("Did not find a flat flat device tree at address %08lX\n", of_flat_tree);
-			return;
+			puts ("Did not find a flat flat device tree\nMust RESET board to recover\n");
+			do_reset (cmdtp, flag, argc, argv);
 		}
 		printf ("   Booting using flat device tree at 0x%x\n",
 				of_flat_tree);
@@ -840,8 +840,8 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 #else
 		if (((struct boot_param_header *)of_data)->magic != OF_DT_HEADER) {
 #endif
-			printf ("ERROR: image is not a flat device tree\n");
-			return;
+			puts ("ERROR: image is not a flat device tree\nMust RESET board to recover\n");
+			do_reset (cmdtp, flag, argc, argv);
 		}
 
 #if defined(CONFIG_OF_LIBFDT)
@@ -849,8 +849,8 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 #else
 		if (((struct boot_param_header *)of_data)->totalsize != ntohl(len_ptr[2])) {
 #endif
-			printf ("ERROR: flat device tree size does not agree with image\n");
-			return;
+			puts ("ERROR: flat device tree size does not agree with image\nMust RESET board to recover\n");
+			do_reset (cmdtp, flag, argc, argv);
 		}
 	}
 #endif
@@ -923,15 +923,6 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 		initrd_end = 0;
 	}
 
-	debug ("## Transferring control to Linux (at address %08lx) ...\n",
-		(ulong)kernel);
-
-	SHOW_BOOT_PROGRESS (15);
-
-#if defined(CFG_INIT_RAM_LOCK) && !defined(CONFIG_E500)
-	unlock_ram_in_cache();
-#endif
-
 #if defined(CONFIG_OF_LIBFDT)
 	/* move of_flat_tree if needed */
 	if (of_data) {
@@ -960,19 +951,19 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 		 * if the user wants it (the logic is in the subroutines).
 		 */
 		if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0) {
-				printf("Failed creating the /chosen node (0x%08X), aborting.\n", of_flat_tree);
-				return;
+			puts ("ERROR: Failed creating the /chosen node, aborting.\nMust RESET board to recover\n");
+			do_reset (cmdtp, flag, argc, argv);
 		}
 #ifdef CONFIG_OF_HAS_UBOOT_ENV
 		if (fdt_env(of_flat_tree) < 0) {
-				printf("Failed creating the /u-boot-env node, aborting.\n");
-				return;
+			puts ("ERROR: Failed creating the /u-boot-env node, aborting.\nMust RESET board to recover\n");
+			do_reset (cmdtp, flag, argc, argv);
 		}
 #endif
 #ifdef CONFIG_OF_HAS_BD_T
 		if (fdt_bd_t(of_flat_tree) < 0) {
-				printf("Failed creating the /bd_t node, aborting.\n");
-				return;
+			puts ("ERROR: Failed creating the /bd_t node, aborting.\nMust RESET board to recover\n");
+			do_reset (cmdtp, flag, argc, argv);
 		}
 #endif
 	}
@@ -997,55 +988,62 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 		memmove ((void *)of_start, (void *)of_data, of_len);
 	}
 #endif
-
-	/*
-	 * Linux Kernel Parameters (passing board info data):
-	 *   r3: ptr to board info data
-	 *   r4: initrd_start or 0 if no initrd
-	 *   r5: initrd_end - unused if r4 is 0
-	 *   r6: Start of command line string
-	 *   r7: End   of command line string
-	 */
-#if defined(CONFIG_OF_FLAT_TREE) || defined(CONFIG_OF_LIBFDT)
-	if (!of_flat_tree)	/* no device tree; boot old style */
-#endif
-		(*kernel) (kbd, initrd_start, initrd_end, cmd_start, cmd_end);
-		/* does not return */
-
-#if defined(CONFIG_OF_FLAT_TREE) || defined(CONFIG_OF_LIBFDT)
-	/*
-	 * Linux Kernel Parameters (passing device tree):
-	 *   r3: ptr to OF flat tree, followed by the board info data
-	 *   r4: physical pointer to the kernel itself
-	 *   r5: NULL
-	 *   r6: NULL
-	 *   r7: NULL
-	 */
 #if defined(CONFIG_OF_FLAT_TREE)
 	ft_setup(of_flat_tree, kbd, initrd_start, initrd_end);
 	/* ft_dump_blob(of_flat_tree); */
 #endif
 #if defined(CONFIG_OF_LIBFDT)
 	if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0) {
-		printf("Failed creating the /chosen node (0x%08X), aborting.\n", of_flat_tree);
-		return;
+		puts ("ERROR: Failed to create the /chosen node, aborting.\nMust RESET board to recover\n");
+		do_reset (cmdtp, flag, argc, argv);
 	}
 #ifdef CONFIG_OF_HAS_UBOOT_ENV
 	if (fdt_env(of_flat_tree) < 0) {
-		printf("Failed creating the /u-boot-env node, aborting.\n");
-		return;
+		puts ("ERROR: Failed to create the /u-boot-env node, aborting.\nMust RESET board to recover\n");
+		do_reset (cmdtp, flag, argc, argv);
 	}
 #endif
 #ifdef CONFIG_OF_HAS_BD_T
 	if (fdt_bd_t(of_flat_tree) < 0) {
-		printf("Failed creating the /bd_t node, aborting.\n");
-		return;
+		puts ("ERROR: Failed to create the /bd_t node, aborting.\nMust RESET board to recover\n");
+		do_reset (cmdtp, flag, argc, argv);
 	}
 #endif
 #endif /* if defined(CONFIG_OF_LIBFDT) */
 
-	(*kernel) ((bd_t *)of_flat_tree, (ulong)kernel, 0, 0, 0);
+	debug ("## Transferring control to Linux (at address %08lx) ...\n",
+		(ulong)kernel);
+
+	SHOW_BOOT_PROGRESS (15);
+
+#if defined(CFG_INIT_RAM_LOCK) && !defined(CONFIG_E500)
+	unlock_ram_in_cache();
 #endif
+
+#if defined(CONFIG_OF_FLAT_TREE) || defined(CONFIG_OF_LIBFDT)
+	if (of_flat_tree) {	/* device tree; boot new style */
+		/*
+		 * Linux Kernel Parameters (passing device tree):
+		 *   r3: ptr to flattened device tree, followed by the board info data
+		 *   r4: physical pointer to the kernel itself
+		 *   r5: NULL
+		 *   r6: NULL
+		 *   r7: NULL
+		 */
+		(*kernel) ((bd_t *)of_flat_tree, (ulong)kernel, 0, 0, 0);
+		/* does not return */
+	}
+#endif
+	/*
+	 * Linux Kernel Parameters (passing board info data):
+	 *   r3: ptr to board info data
+	 *   r4: initrd_start or 0 if no initrd
+	 *   r5: initrd_end - unused if r4 is 0
+	 *   r6: Start of command line string
+	 *   r7: End   of command line string
+	 */
+	(*kernel) (kbd, initrd_start, initrd_end, cmd_start, cmd_end);
+	/* does not return */
 }
 #endif /* CONFIG_PPC */
 
-- 
1.4.4.4

[U-Boot-Users] [PATCH 6/17] Reorganize and fix problems (returns) in the bootm command.

[Grant Likely]

On 7/4/07, Jerry Van Baren <gvb.uboot@gmail.com> wrote:
> Do *NOT* return after the "point of no return" has been passed.
>   If something goes wrong, the board must be reset after that point.
> Move the "Transferring control to Linux" debug message back to where it
>   belongs: just before transferring control to linux.

Why is it necessary to reset the board at this point?  The failure
paths here seem to be things like invalid checksums and the like.  Why
are these tests after a point of no return?

I would think that if any of these failures are hit, you would *not*
want to reset the board so you can figure out what was going on.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195

[U-Boot-Users] [PATCH 6/17] Reorganize and fix problems (returns) in the bootm command.

[Jerry Van Baren]

Grant Likely wrote:
> On 7/4/07, Jerry Van Baren <gvb.uboot@gmail.com> wrote:
>> Do *NOT* return after the "point of no return" has been passed.
>>   If something goes wrong, the board must be reset after that point.
>> Move the "Transferring control to Linux" debug message back to where it
>>   belongs: just before transferring control to linux.
> 
> Why is it necessary to reset the board at this point?  The failure
> paths here seem to be things like invalid checksums and the like.  Why
> are these tests after a point of no return?
> 
> I would think that if any of these failures are hit, you would *not*
> want to reset the board so you can figure out what was going on.
> 
> Cheers,
> g.

We've smashed our underpinnings by uncompressing linux over our 
interrupt vectors before the checksum catches the problem.  There is no 
way back other than a reset.  The error I fixed (in a couple of places) 
was returns.  FWIIW, my experience with the erroneous returns is that it 
sort of works, but not right and then funky stuff happens. :-/

I presume the comment was a general one.  If you have any specific reset 
vs. return questions, hollar and I'll justify them or eat crow.

Thanks,
gvb

[U-Boot-Users] [PATCH 6/17] Reorganize and fix problems (returns) in the bootm command.

[Grant Likely]

On 7/5/07, Jerry Van Baren <gvb.uboot@gmail.com> wrote:
> Grant Likely wrote:
> > On 7/4/07, Jerry Van Baren <gvb.uboot@gmail.com> wrote:
> >> Do *NOT* return after the "point of no return" has been passed.
> >>   If something goes wrong, the board must be reset after that point.
> >> Move the "Transferring control to Linux" debug message back to where it
> >>   belongs: just before transferring control to linux.
> >
> > Why is it necessary to reset the board at this point?  The failure
> > paths here seem to be things like invalid checksums and the like.  Why
> > are these tests after a point of no return?
> >
> > I would think that if any of these failures are hit, you would *not*
> > want to reset the board so you can figure out what was going on.
> >
> > Cheers,
> > g.
>
> We've smashed our underpinnings by uncompressing linux over our
> interrupt vectors before the checksum catches the problem.  There is no
> way back other than a reset.  The error I fixed (in a couple of places)
> was returns.  FWIIW, my experience with the erroneous returns is that it
> sort of works, but not right and then funky stuff happens. :-/
>
> I presume the comment was a general one.  If you have any specific reset
> vs. return questions, hollar and I'll justify them or eat crow.

Ah, okay that makes sense.  So the obvious next question is: can any
of those tests be performed before uncompressing Linux?  If so, then
they probably should be moved.  If not...

Acked-by: Grant Likely <grant.likely@secretlab.ca>
>
> Thanks,
> gvb
>


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195

[U-Boot-Users] [PATCH 6/17] Reorganize and fix problems (returns) in the bootm command.

[Jerry Van Baren]

Grant Likely wrote:
> On 7/5/07, Jerry Van Baren <gvb.uboot@gmail.com> wrote:
>> Grant Likely wrote:
>>> On 7/4/07, Jerry Van Baren <gvb.uboot@gmail.com> wrote:
>>>> Do *NOT* return after the "point of no return" has been passed.
>>>>   If something goes wrong, the board must be reset after that point.
>>>> Move the "Transferring control to Linux" debug message back to where it
>>>>   belongs: just before transferring control to linux.
>>> Why is it necessary to reset the board at this point?  The failure
>>> paths here seem to be things like invalid checksums and the like.  Why
>>> are these tests after a point of no return?
>>>
>>> I would think that if any of these failures are hit, you would *not*
>>> want to reset the board so you can figure out what was going on.
>>>
>>> Cheers,
>>> g.
>> We've smashed our underpinnings by uncompressing linux over our
>> interrupt vectors before the checksum catches the problem.  There is no
>> way back other than a reset.  The error I fixed (in a couple of places)
>> was returns.  FWIIW, my experience with the erroneous returns is that it
>> sort of works, but not right and then funky stuff happens. :-/
>>
>> I presume the comment was a general one.  If you have any specific reset
>> vs. return questions, hollar and I'll justify them or eat crow.
> 
> Ah, okay that makes sense.  So the obvious next question is: can any
> of those tests be performed before uncompressing Linux?  If so, then
> they probably should be moved.  If not...
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>

Sorry, no.  The CRC is over the uncompressed data, and for good reason - 
it will detect if Something Bad[tm] happened during the decompression. 
The good news is, the reset-to-recover path /should/ never be taken.

gvb

[U-Boot-Users] [PATCH 6/17] Reorganize and fix problems (returns) in the bootm command.

[Timur Tabi]

Jerry Van Baren wrote:
 > We've smashed our underpinnings by uncompressing linux over our
 > interrupt vectors before the checksum catches the problem.  There is no
 > way back other than a reset.

Can you include this explanation as a comment in the code at the exact point this occurs?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH 6/17] Reorganize and fix problems (returns) in the bootm command.

[Jerry Van Baren]

Timur Tabi wrote:
> Jerry Van Baren wrote:
>  > We've smashed our underpinnings by uncompressing linux over our
>  > interrupt vectors before the checksum catches the problem.  There is no
>  > way back other than a reset.
> 
> Can you include this explanation as a comment in the code at the
> exact point this occurs?

It is already in the original code.
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=common/cmd_bootm.c;h=a6499e8dd9bc3fc853606567c847d0a064f02e55;hb=HEAD#l306>

gvb

[U-Boot-Users] [PATCH 6/17] Reorganize and fix problems (returns) in the bootm command.

[Wolfgang Denk]

In message <468D008D.9030901@smiths-aerospace.com> you wrote:
>
> Sorry, no.  The CRC is over the uncompressed data, and for good reason - 

Umm... no, it is over the data blob, which usually is compressed.


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
Every program has at least one bug and can be shortened by  at  least
one instruction - from which, by induction, one can deduce that every
program can be reduced to one instruction which doesn't work.

[U-Boot-Users] [PATCH 6/17] Reorganize and fix problems (returns) in the bootm command.

[Wolfgang Denk]

In message <468D0E3D.3000308@freescale.com> you wrote:
> Jerry Van Baren wrote:
>  > We've smashed our underpinnings by uncompressing linux over our
>  > interrupt vectors before the checksum catches the problem.  There is no
>  > way back other than a reset.
> 
> Can you include this explanation as a comment in the code at the exact point this occurs?

Ummm... what exactly is not clear enough with this comment in
"common/cmd_bootm.c":

        /*      
         * We have reached the point of no return: we are going to
         * overwrite all exception vector code, so we cannot easily
         * recover from any failures any more... 
         */      

??? Do you need additional explanations? What should be added?

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
"Logic and practical information do not seem to apply here."
"You admit that?"
"To deny the facts would be illogical, Doctor"
	-- Spock and McCoy, "A Piece of the Action", stardate unknown

[U-Boot-Users] [PATCH 6/17] Reorganize and fix problems (returns) in the bootm command.

[Jerry Van Baren]

Wolfgang Denk wrote:
> In message <468D0E3D.3000308@freescale.com> you wrote:
>> Jerry Van Baren wrote:
>>  > We've smashed our underpinnings by uncompressing linux over our
>>  > interrupt vectors before the checksum catches the problem.  There is no
>>  > way back other than a reset.
>>
>> Can you include this explanation as a comment in the code at the exact point this occurs?
> 
> Ummm... what exactly is not clear enough with this comment in
> "common/cmd_bootm.c":
> 
>         /*      
>          * We have reached the point of no return: we are going to
>          * overwrite all exception vector code, so we cannot easily
>          * recover from any failures any more... 
>          */      
> 
> ??? Do you need additional explanations? What should be added?
> 
> Best regards,
> 
> Wolfgang Denk

Maybe if we enhanced the comment so it catches the eye of your average 
geek.  Perhaps one of these:
   <http://www.chris.com/ASCII/art/html/nakedladies.html>
Of course, we would have to do #ifdef MALE around it...

;-)

gvb (who missed the comment himself, the first time around)

[U-Boot-Users] [PATCH 6/17] Reorganize and fix problems (returns) in the bootm command.

[Timur Tabi]

Wolfgang Denk wrote:

>         /*      
>          * We have reached the point of no return: we are going to
>          * overwrite all exception vector code, so we cannot easily
>          * recover from any failures any more... 
>          */      
> 
> ??? Do you need additional explanations? What should be added?

I read only the patch, not the code in cmd_bootm.c.  And since someone else asked why the patch was needed, I assumed that there was no comment or any other documentation explaining the need for the patch.