Hi Tom,
On Fri, Jun 28, 2013 at 1:25 PM, Tom Rini <tr...@ti.com> wrote:
> With 35fc84fa1 [Refactor the bootm command to reduce code duplication]
> we stopped checking the return value of bootm_load_os (unintentionally!)
> and simply returned if we had a non-zero return value from the function.
> This broke the valid case of a legacy image file of a single kernel
> loaded into an overlapping memory area (the default way of booting
> nearly all TI platforms).
>
> The best way to fix this problem in the new code is to make
> bootm_load_os be the one to see if we have a problem with this, and if
> it's fatal return BOOTM_ERR_RESET and if it's not BOOTM_ERR_OVERLAP, so
> that we can avoid calling lmb_reserve() but continue with booting. We
> however still need to handle the other BOOTM_ERR values so re-work
> do_bootm_states so that we have an error handler at the bottom we can
> goto for problems from bootm_load_os, or problems from the other callers
> (as the code was before). Add a comment to do_bootm_states noting the
> existing restriction on negative return values.
>
> Signed-off-by: Tom Rini <tr...@ti.com>
>
> ---
> Changes in v2:
> - Rework so that only bootm_load_os and boot_selected_os head down into
> the err case code, and other errors simply return back to the caller.
> Fixes 'spl export'.
> ---
> common/cmd_bootm.c | 65
> ++++++++++++++++++++++++++++++----------------------
> 1 file changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
> index ba0bcd4..3009ece 100644
> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> @@ -345,8 +345,10 @@ static int bootm_find_other(cmd_tbl_t *cmdtp, int
> flag, int argc,
> #define BOOTM_ERR_RESET -1
> #define BOOTM_ERR_OVERLAP -2
> #define BOOTM_ERR_UNIMPLEMENTED -3
> -static int bootm_load_os(image_info_t os, ulong *load_end, int
> boot_progress)
> +static int bootm_load_os(bootm_headers_t *images, unsigned long *load_end,
> + int boot_progress)
> {
> + image_info_t os = images->os;
> uint8_t comp = os.comp;
> ulong load = os.load;
> ulong blob_start = os.start;
> @@ -464,7 +466,17 @@ static int bootm_load_os(image_info_t os, ulong
> *load_end, int boot_progress)
> debug("images.os.load = 0x%lx, load_end = 0x%lx\n", load,
> *load_end);
>
> - return BOOTM_ERR_OVERLAP;
> + /* Check what type of image this is. */
> + if (images->legacy_hdr_valid) {
> + if (image_get_type(&images->legacy_hdr_os_copy)
> + == IH_TYPE_MULTI)
> + puts("WARNING: legacy format multi
> component image overwritten\n");
> + return BOOTM_ERR_OVERLAP;
> + } else {
> + puts("ERROR: new format image overwritten - must
> RESET the board to recover\n");
> + bootstage_error(BOOTSTAGE_ID_OVERWRITTEN);
> + return BOOTM_ERR_RESET;
> + }
> }
>
> return 0;
> @@ -558,6 +570,10 @@ static int boot_selected_os(int argc, char * const
> argv[], int state,
> * Note that if states contains more than one flag it MUST contain
> * BOOTM_STATE_START, since this handles and consumes the command line
> args.
> *
> + * Also note that aside from boot_os_fn functions and bootm_load_os no
> other
> + * functions we store the return value of in 'ret' may use a negative
> return
> + * value, without special handling.
> + *
> * @param cmdtp Pointer to bootm command table entry
> * @param flag Command flags (CMD_FLAG_...)
> * @param argc Number of subcommand arguments (0 = no arguments)
> @@ -599,11 +615,15 @@ static int do_bootm_states(cmd_tbl_t *cmdtp, int
> flag, int argc,
> if (!ret && (states & BOOTM_STATE_LOADOS)) {
> ulong load_end;
>
> - ret = bootm_load_os(images->os, &load_end, 0);
> - if (!ret) {
> + ret = bootm_load_os(images, &load_end, 0);
> + if (ret && ret != BOOTM_ERR_OVERLAP)
> + goto err;
> +
> + if (ret == 0)
> lmb_reserve(&images->lmb, images->os.load,
> (load_end - images->os.load));
> - }
> + else if (ret == BOOTM_ERR_OVERLAP)
> + ret = 0;
> }
>
> /* Relocate the ramdisk */
> @@ -660,34 +680,25 @@ static int do_bootm_states(cmd_tbl_t *cmdtp, int
> flag, int argc,
> }
> #endif
> /* Now run the OS! We hope this doesn't return */
> - if (!ret && (states & BOOTM_STATE_OS_GO))
> + if (!ret && (states & BOOTM_STATE_OS_GO)) {
> ret = boot_selected_os(argc, argv, BOOTM_STATE_OS_GO,
> images, boot_fn, &iflag);
> + if (ret)
> + goto err;
> + }
> +
> + return ret;
>
Thanks for getting to the bottom of this.
Just a question here - should this fall through to display the error with
the code below? For example if the subcommand is not supported...
>
> /* Deal with any fallout */
> - if (ret < 0) {
> - if (ret == BOOTM_ERR_UNIMPLEMENTED) {
> - if (iflag)
> - enable_interrupts();
> - bootstage_error(BOOTSTAGE_ID_DECOMP_UNIMPL);
> - return 1;
> - } else if (ret == BOOTM_ERR_OVERLAP) {
> - if (images->legacy_hdr_valid) {
> - if
> (image_get_type(&images->legacy_hdr_os_copy)
> - == IH_TYPE_MULTI)
> - puts("WARNING: legacy format multi
> component image overwritten\n");
> - } else {
> - puts("ERROR: new format image overwritten
> - must RESET the board to recover\n");
> - bootstage_error(BOOTSTAGE_ID_OVERWRITTEN);
> - ret = BOOTM_ERR_RESET;
> - }
> - }
> - if (ret == BOOTM_ERR_RESET)
> - do_reset(cmdtp, flag, argc, argv);
> - }
> +err:
> if (iflag)
> enable_interrupts();
> - if (ret)
> +
> + if (ret == BOOTM_ERR_UNIMPLEMENTED)
> + bootstage_error(BOOTSTAGE_ID_DECOMP_UNIMPL);
> + else if (ret == BOOTM_ERR_RESET)
> + do_reset(cmdtp, flag, argc, argv);
> + else
> puts("subcommand not supported\n");
>
> return ret;
> --
> 1.7.9.5
>
>
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
