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

[Simon Glass]

Hi Tom,

On Fri, Jun 28, 2013 at 3:07 PM, Tom Rini <trini at ti.com> wrote:

> On Fri, Jun 28, 2013 at 02:41:49PM -0700, Simon Glass wrote:
> > 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...
>
> Good point.  But we also aren't able to say what the subcommand is
> anymore, so it's not as helpful, but we do need to say something.
>
> I also don't want to leave things non-functional over the weekend, so
> I'm pushing this version now, and we'll need another clean-up early next
> week.  Thanks for spotting!
>

Yes agreed, thanks again for looking at this.

Regards,
Simon