cmd_bootm.c: Correct BOOTM_ERR_OVERLAP handling

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 <[email protected]>

---
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(-)

common/cmd_bootm.c +3 -1

index ba0bcd4..3009ece 100644
@@ -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;

common/cmd_bootm.c +11 -1

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

common/cmd_bootm.c +0 -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)

common/cmd_bootm.c +7 -3

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

common/cmd_bootm.c +0 -0

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

common/cmd_bootm.c +7 -23

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

Hi Tom,

Quoted 109 lines · Tom Rini

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 <[email protected]>

---
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(-)

common/cmd_bootm.c +3 -1

index ba0bcd4..3009ece 100644
@@ -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;

common/cmd_bootm.c +11 -1

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

common/cmd_bootm.c +0 -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)

common/cmd_bootm.c +7 -3

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

common/cmd_bootm.c +0 -0

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

common/cmd_bootm.c static int do_bootm_states(cmd_tbl_t *cmdtp, int · line 692

Quoted 10 lines

common/cmd_bootm.c +6 -1

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

Quoted 6 lines · Simon Glass

Hi Tom,

Quoted 2 lines · Tom Rini

With 35fc84fa1 [Refactor the bootm command to reduce code duplication] we stopped checking the return value of bootm_load_os

(unintentionally!)

Quoted 1 line

Quoted 1 line

and simply returned if we had a non-zero return value from the

function.

Quoted 11 lines

Quoted 11 lines

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

Quoted 15 lines

Quoted 15 lines

(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 <[email protected]>

---
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(-)

Quoted 12 lines

common/cmd_bootm.c static int bootm_find_other(cmd_tbl_t *cmdtp, int · line 350

Quoted 12 lines

common/cmd_bootm.c +1 -1

index ba0bcd4..3009ece 100644
@@ -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,

Quoted 9 lines

common/cmd_bootm.c static int bootm_load_os(image_info_t os, ulong · line 467

Quoted 9 lines

common/cmd_bootm.c +2 -0

+               int boot_progress)
 {
+       image_info_t os = images->os;
        uint8_t comp = os.comp;

        ulong load = os.load;
        ulong blob_start = os.start;

common/cmd_bootm.c +0 -0

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

Quoted 12 lines

common/cmd_bootm.c static int bootm_load_os(image_info_t os, ulong *load_end, int boot_progress)

Quoted 12 lines

common/cmd_bootm.c +8 -1

                        *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

Quoted 6 lines

Quoted 6 lines

common/cmd_bootm.c +3 -0

 RESET the board to recover\n");
+                       bootstage_error(BOOTSTAGE_ID_OVERWRITTEN);
+                       return BOOTM_ERR_RESET;
+               }

        }

Quoted 16 lines

Quoted 8 lines

        return 0;

common/cmd_bootm.c +0 -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

Quoted 8 lines

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)

Quoted 38 lines

Quoted 25 lines

common/cmd_bootm.c +7 -3

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

common/cmd_bootm.c +0 -0

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

common/cmd_bootm.c static int do_bootm_states(cmd_tbl_t *cmdtp, int · line 692

Quoted 10 lines

common/cmd_bootm.c +6 -1

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

Quoted 2 lines

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!

Hi Tom,

On Fri, Jun 28, 2013 at 1:25 PM, Tom Rini <[email protected]> wrote:

Quoted 5 lines

common/cmd_bootm.c +0 -0

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

common/cmd_bootm.c static int do_bootm_states(cmd_tbl_t *cmdtp, int · line 692

Quoted 10 lines

common/cmd_bootm.c +6 -1

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

Hi all,

Quoted 3 lines · Simon Glass

Hi Tom,

On Fri, Jun 28, 2013 at 1:25 PM, Tom Rini <[email protected]> wrote:

<snip>

Quoted 12 lines

Quoted 9 lines

(quoted diff) +0 -0

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

Quoted 3 lines

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

Quoted 27 lines · Andreas Bie?mann

Hi all,

Quoted 3 lines · Simon Glass

Hi Tom,

On Fri, Jun 28, 2013 at 1:25 PM, Tom Rini <[email protected]> wrote:

<snip>

Quoted 12 lines

Quoted 9 lines

(quoted diff) +0 -0

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

Quoted 3 lines

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.

Quoted 1 line

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

The patch I posted yesterday should fix that.

Quoted 29 lines · Tom Rini

Quoted 28 lines · Andreas Bie?mann

Hi all,

Quoted 4 lines · Simon Glass

Hi Tom,

On Fri, Jun 28, 2013 at 1:25 PM, Tom Rini <[email protected]> wrote:

<snip>

Quoted 12 lines

Quoted 9 lines

(quoted diff) +0 -0

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

Quoted 3 lines

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>

Quoted 4 lines

Quoted 2 lines

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?