[U-Boot] [PATCH v2] cmd_bootm.c: Correct BOOTM_ERR_OVERLAP handling

Simon Glass sjg at chromium.org
Fri Jun 28 23:41:49 CEST 2013


Hi Tom,

On Fri, Jun 28, 2013 at 1:25 PM, Tom Rini <trini at 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 <trini at 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


More information about the U-Boot mailing list