[U-Boot-Users] [PATCH 6/17] Reorganize and fix problems (returns) in the bootm command.
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 */
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 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
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 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
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
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 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
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
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)
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.
participants (5)
-
Grant Likely -
Jerry Van Baren -
Jerry Van Baren -
Timur Tabi -
Wolfgang Denk