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

login
register
mail settings
Submitter Tom Rini
Date June 28, 2013, 8:25 p.m.
Message ID <1372451133-30779-1-git-send-email-trini@ti.com>
Download mbox | patch
Permalink /patch/255664/
State Accepted
Delegated to: Tom Rini
Headers show

Comments

Tom Rini - June 28, 2013, 8:25 p.m.
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@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(-)
Simon Glass - June 28, 2013, 9:41 p.m.
Hi Tom,

On Fri, Jun 28, 2013 at 1:25 PM, Tom Rini <trini@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@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
Tom Rini - June 28, 2013, 10:07 p.m.
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@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@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!
Simon Glass - June 28, 2013, 11:59 p.m.
Hi Tom,

On Fri, Jun 28, 2013 at 3:07 PM, Tom Rini <trini@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@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@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
Andreas Bießmann - July 2, 2013, 10:49 a.m.
Hi all,

On 06/28/2013 11:41 PM, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, Jun 28, 2013 at 1:25 PM, Tom Rini <trini@ti.com> wrote:

<snip>

>> @@ -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...

as pointed out by Simon the error message 'subcommand not supported'
disappears with this patch. On avr32 this means currently:

---8<---
## Booting kernel from Legacy Image at 10400000 ...
   Image Name:   Linux-3.6.2
   Image Type:   AVR32 Linux Kernel Image (gzip compressed)
   Data Size:    1846216 Bytes = 1.8 MiB
   Load Address: 10000000
   Entry Point:  90000000
   Verifying Checksum ... OK
   Uncompressing Kernel Image ... OK
U-Boot>
--->8---

Before it was at least:
---8<---
## Booting kernel from Legacy Image at 10400000 ...
   Image Name:   Linux-3.6.2
   Image Type:   AVR32 Linux Kernel Image (gzip compressed)
   Data Size:    1846216 Bytes = 1.8 MiB
   Load Address: 10000000
   Entry Point:  90000000
   Verifying Checksum ... OK
   Uncompressing Kernel Image ... OK
subcommand not supported
U-Boot>
--->8---

Who will fix that? I'll have a look for the avr32 specific part.

Regards

Andreas Bießmann
Tom Rini - July 2, 2013, 12:35 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/02/2013 06:49 AM, Andreas Bießmann wrote:
> Hi all,
> 
> On 06/28/2013 11:41 PM, Simon Glass wrote:
>> Hi Tom,
>> 
>> On Fri, Jun 28, 2013 at 1:25 PM, Tom Rini <trini@ti.com> wrote:
> 
> <snip>
> 
>>> @@ -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...
> 
> as pointed out by Simon the error message 'subcommand not
> supported' disappears with this patch. On avr32 this means
> currently:
> 
> ---8<--- ## Booting kernel from Legacy Image at 10400000 ... Image
> Name:   Linux-3.6.2 Image Type:   AVR32 Linux Kernel Image (gzip
> compressed) Data Size:    1846216 Bytes = 1.8 MiB Load Address:
> 10000000 Entry Point:  90000000 Verifying Checksum ... OK 
> Uncompressing Kernel Image ... OK U-Boot> --->8---
> 
> Before it was at least: ---8<--- ## Booting kernel from Legacy
> Image at 10400000 ... Image Name:   Linux-3.6.2 Image Type:   AVR32
> Linux Kernel Image (gzip compressed) Data Size:    1846216 Bytes =
> 1.8 MiB Load Address: 10000000 Entry Point:  90000000 Verifying
> Checksum ... OK Uncompressing Kernel Image ... OK subcommand not
> supported U-Boot> --->8---
> 
> Who will fix that? I'll have a look for the avr32 specific part.

The patch I posted yesterday should fix that.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJR0skAAAoJENk4IS6UOR1WlV8P/iK99NqU9/FDg3CfLs+1m+99
MYJfFEAeDgOUkCzu6haT8xPumuwmhg/03xohXMUYYw+YdeZb8wGC4YwV1iFO+kxO
Nz9ru6lrYyNACq6NYg9Kiasvjp8Mn9DDPKTLyEt2QeIlP/1hpvnkag6jnHJlhNvl
Kyf4yHhT+ksIVbaIryWWohEf6VAghMOYvqU00tQdWvPYpVEqi2x0dHl6WsQPv7/V
xt7b+VF2pCFp8Bt4fRrGazZQuIM0T4Vw8AtBnyhpQsc+3VRfx5cbw8AZurq5BfPN
wQ6YGX/0tyPA6QSMkofP02/S8WVl//8ZJW+RDIq+d+klpIoujC6jggT3OQ4llSlW
Yl61lKFgkyk6+pkLOIRqlq2E+nkPCVtrmLUVYrYXKMHTyjNHhmsjYjJUpKJHHG1z
Kg/zGBBrpqzv1oNgKZ/rR1YIPHd/6W+hmMozYEeGTjhGnGM2eq57X4/WUNXvAdWh
lxuUrBttTn9IljsfxpxbN2fT9dVNmdyTxd8p6rOJTTu72Ooi7Wfsv5Laww1Y7jtn
KNywZTUtGaCoqs/v3JhktbV8iAjrp6euoTGF9A6I2bI+VF58Y8s9dwA7JPHgh27r
ngFg/tmAl9p1mv8C3Wt1K3jKPMDz+dzPZpmBcVqLTFBi6oEhkl0XtdmHs2YRXFEr
xRxCcxxOWgXe7EInP/sg
=3VVT
-----END PGP SIGNATURE-----
Andreas Bießmann - July 2, 2013, 12:58 p.m.
On 07/02/2013 02:35 PM, Tom Rini wrote:
> On 07/02/2013 06:49 AM, Andreas Bießmann wrote:
>> Hi all,
> 
>> On 06/28/2013 11:41 PM, Simon Glass wrote:
>>> Hi Tom,
>>>
>>> On Fri, Jun 28, 2013 at 1:25 PM, Tom Rini <trini@ti.com> wrote:
> 
>> <snip>
> 
>>>> @@ -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...
> 
>> as pointed out by Simon the error message 'subcommand not
>> supported' disappears with this patch. 

<snip>

>> Who will fix that? I'll have a look for the avr32 specific part.
> 
> The patch I posted yesterday should fix that.

Oups, overseen, you mean http://patchwork.ozlabs.org/patch/256125/ right?

Best regards

Andreas Bießmann
Tom Rini - July 2, 2013, 1:01 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/02/2013 08:58 AM, Andreas Bießmann wrote:
> On 07/02/2013 02:35 PM, Tom Rini wrote:
>> On 07/02/2013 06:49 AM, Andreas Bießmann wrote:
>>> Hi all,
>> 
>>> On 06/28/2013 11:41 PM, Simon Glass wrote:
>>>> Hi Tom,
>>>> 
>>>> On Fri, Jun 28, 2013 at 1:25 PM, Tom Rini <trini@ti.com>
>>>> wrote:
>> 
>>> <snip>
>> 
>>>>> @@ -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...
>> 
>>> as pointed out by Simon the error message 'subcommand not 
>>> supported' disappears with this patch.
> 
> <snip>
> 
>>> Who will fix that? I'll have a look for the avr32 specific
>>> part.
>> 
>> The patch I posted yesterday should fix that.
> 
> Oups, overseen, you mean http://patchwork.ozlabs.org/patch/256125/
> right?

Correct.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJR0s8xAAoJENk4IS6UOR1WuZAQAI2vkozEJyPKY6+6q7k3OOPP
P51Iut61IFYNREl9lYXXY/w6W08IC+K5SMD+4OvQZDhHJdYZBuOhXJjS9QlABQlg
9CE5nUvC3brhBJ90LIgno+6z02U1BmX/ackRe6fEGyOopBpibmsQGL7P2RhjX+Mo
g1oz/KisXJBsb+ZLQJptfuu5ZzQgjP2HT9z5yKmXXqJWWDayEOMHGcw2mlUIUgdE
xz/wm7mA45srwJfi/dRwavo4unswMOYFz8VzktCpQbVwIxgAkO7WmOYcxCkxFQ2t
3JZQVkrM6NL/b99ByZqrQy9FkmJotiWc/u4bQ3x1kn9KKxa+MSm4+NNS/UTc7dWh
z/wZ5DMNB9wYAzro0tVxLd7y6gZhOkhUCHsWUxV0ma0r8wd2sx3dLC85DwPpo2WA
JxWKAsJdY+NAs9phiVZ7h1iBMA/ekauRUtFJC9ahcCJf54AR89ppDWOY2Avxb+Y9
i6eX4jNUMJCjvJqzQL8M8gmJgnR1yXEAXylieIWgAzDU5ymIvxl6SvrzPCqxiTIX
YqZvjJkCxtDIDc1A5i4MkAnrogKIhoEH+2J6MzMpi+OZ1n+GyBaGuP2zcC9N5cD8
hJIXfsO8SepQJy+cAkE+9nOH5WvzdElMkUIPkZuhHYNzoMO83P5W6SE+mr/Qd+b6
948eA/6mlg1CopxPo8HU
=DxRB
-----END PGP SIGNATURE-----

Patch

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;
 
 	/* 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;