[U-Boot] [PATCH] bootm: Use "panic()" in non-recoverable error conditions

[U-Boot] [PATCH] bootm: Use "panic()" in non-recoverable error conditions

[Kyle Moffett]

All of these errors are various kinds of fatal memory overwrite
conditions and so should be handled by panic().  This fixes a bug in
which the error message might not get all the way out to the serial
console before the system reboots; panic() has a built-in delay after
doing a printf() before calling do_reset().

This will result in a change in behavior for the 27 board configuration
files which set CONFIG_PANIC_HANG (less than 5% of the total).  They
will now hang in those fatal error conditions instead of trying to
reboot.

Given that CONFIG_PANIC_HANG is intended to prevent the system from
rebooting after it has encountered an unrecoverable error, this seems to
be the desired behavior for those 27 board configurations.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Andy Fleming <afleming@gmail.com>
Cc: Kumar Gala <kumar.gala@freescale.com>
---

This patch has been test-booted on my board and is checkpatch-clean.

---
 common/cmd_bootm.c |   43 +++++++++++++------------------------------
 1 files changed, 13 insertions(+), 30 deletions(-)

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index c2e8038..dea9093 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -300,7 +300,6 @@ static int bootm_start(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]
 	return 0;
 }
 
-#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)
@@ -335,11 +334,9 @@ static int bootm_load_os(image_info_t os, ulong *load_end, int boot_progress)
 		printf ("   Uncompressing %s ... ", type_name);
 		if (gunzip ((void *)load, unc_len,
 					(uchar *)image_start, &image_len) != 0) {
-			puts ("GUNZIP: uncompress, out-of-mem or overwrite error "
-				"- must RESET board to recover\n");
 			if (boot_progress)
-				show_boot_progress (-6);
-			return BOOTM_ERR_RESET;
+				show_boot_progress(-6);
+			panic("GUNZIP: uncompress or overwrite error");
 		}
 
 		*load_end = load + image_len;
@@ -357,11 +354,9 @@ static int bootm_load_os(image_info_t os, ulong *load_end, int boot_progress)
 					&unc_len, (char *)image_start, image_len,
 					CONFIG_SYS_MALLOC_LEN < (4096 * 1024), 0);
 		if (i != BZ_OK) {
-			printf ("BUNZIP2: uncompress or overwrite error %d "
-				"- must RESET board to recover\n", i);
 			if (boot_progress)
-				show_boot_progress (-6);
-			return BOOTM_ERR_RESET;
+				show_boot_progress(-6);
+			panic("BUNZIP2: uncompress or overwrite error");
 		}
 
 		*load_end = load + unc_len;
@@ -377,10 +372,9 @@ static int bootm_load_os(image_info_t os, ulong *load_end, int boot_progress)
 			(unsigned char *)image_start, image_len);
 		unc_len = lzma_len;
 		if (ret != SZ_OK) {
-			printf ("LZMA: uncompress or overwrite error %d "
-				"- must RESET board to recover\n", ret);
-			show_boot_progress (-6);
-			return BOOTM_ERR_RESET;
+			if (boot_progress)
+				show_boot_progress(-6);
+			panic("LZMA: uncompress or overwrite error");
 		}
 		*load_end = load + unc_len;
 		break;
@@ -394,11 +388,9 @@ static int bootm_load_os(image_info_t os, ulong *load_end, int boot_progress)
 					  image_len, (unsigned char *)load,
 					  &unc_len);
 		if (ret != LZO_E_OK) {
-			printf ("LZO: uncompress or overwrite error %d "
-			      "- must RESET board to recover\n", ret);
 			if (boot_progress)
-				show_boot_progress (-6);
-			return BOOTM_ERR_RESET;
+				show_boot_progress(-6);
+			panic("LZO: uncompress or overwrite error");
 		}
 
 		*load_end = load + unc_len;
@@ -624,18 +616,14 @@ int do_bootm (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	ret = bootm_load_os(images.os, &load_end, 1);
 
 	if (ret < 0) {
-		if (ret == BOOTM_ERR_RESET)
-			do_reset (cmdtp, flag, argc, argv);
 		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");
-				show_boot_progress (-113);
-				do_reset (cmdtp, flag, argc, argv);
+				show_boot_progress(-113);
+				panic("ERROR: new format image overwritten");
 			}
 		}
 		if (ret == BOOTM_ERR_UNIMPLEMENTED) {
@@ -678,13 +666,8 @@ int do_bootm (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	boot_fn(0, argc, argv, &images);
 
-	show_boot_progress (-9);
-#ifdef DEBUG
-	puts ("\n## Control returned to monitor - resetting...\n");
-#endif
-	do_reset (cmdtp, flag, argc, argv);
-
-	return 1;
+	show_boot_progress(-9);
+	panic("Control returned to monitor!");
 }
 
 int bootm_maybe_autostart(cmd_tbl_t *cmdtp, const char *cmd)
-- 
1.7.2.5

[U-Boot] [PATCH] bootm: Use "panic()" in non-recoverable error conditions

[Wolfgang Denk]

Dear Kyle Moffett,

In message <1319134031-28503-1-git-send-email-Kyle.D.Moffett@boeing.com> you wrote:
> All of these errors are various kinds of fatal memory overwrite
> conditions and so should be handled by panic().  This fixes a bug in
> which the error message might not get all the way out to the serial
> console before the system reboots; panic() has a built-in delay after
> doing a printf() before calling do_reset().
> 
> This will result in a change in behavior for the 27 board configuration
> files which set CONFIG_PANIC_HANG (less than 5% of the total).  They
> will now hang in those fatal error conditions instead of trying to
> reboot.
> 
> Given that CONFIG_PANIC_HANG is intended to prevent the system from
> rebooting after it has encountered an unrecoverable error, this seems to
> be the desired behavior for those 27 board configurations.

This is your interpretation, but the users and especially the
respective board maintainers may think different.  We should at least
try and get feedback from them first.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It's all Klatchian to me.
        - Terry Pratchett & Stephen Briggs, _The Discworld Companion_

[U-Boot] [PATCH] bootm: Use "panic()" in non-recoverable error conditions

[Moffett, Kyle D]

On Oct 20, 2011, at 15:31, Wolfgang Denk wrote:
> Dear Kyle Moffett,
> In message <1319134031-28503-1-git-send-email-Kyle.D.Moffett@boeing.com> you wrote:
>> All of these errors are various kinds of fatal memory overwrite
>> conditions and so should be handled by panic().  This fixes a bug in
>> which the error message might not get all the way out to the serial
>> console before the system reboots; panic() has a built-in delay after
>> doing a printf() before calling do_reset().
>> 
>> This will result in a change in behavior for the 27 board configuration
>> files which set CONFIG_PANIC_HANG (less than 5% of the total).  They
>> will now hang in those fatal error conditions instead of trying to
>> reboot.
>> 
>> Given that CONFIG_PANIC_HANG is intended to prevent the system from
>> rebooting after it has encountered an unrecoverable error, this seems to
>> be the desired behavior for those 27 board configurations.
> 
> This is your interpretation, but the users and especially the
> respective board maintainers may think different.  We should at least
> try and get feedback from them first.

Well, to be fair, the README says this about the config option:

  CONFIG_PANIC_HANG

  Define this variable to stop the system in case of a
  fatal error, so that you have to reset it manually.
  This is probably NOT a good idea for an embedded
  system where you want the system to reboot
  automatically as fast as possible, but it may be
  useful during development since you can try to debug
  the conditions that lead to the situation.

Cheers,
Kyle Moffett

--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/

[U-Boot] [PATCH] bootm: Use "panic()" in non-recoverable error conditions

[Albert ARIBAUD]

Hi Kyle,

Le 20/10/2011 22:06, Moffett, Kyle D a ?crit :
> On Oct 20, 2011, at 15:31, Wolfgang Denk wrote:
>> Dear Kyle Moffett,
>> In message<1319134031-28503-1-git-send-email-Kyle.D.Moffett@boeing.com>  you wrote:
>>> All of these errors are various kinds of fatal memory overwrite
>>> conditions and so should be handled by panic().  This fixes a bug in
>>> which the error message might not get all the way out to the serial
>>> console before the system reboots; panic() has a built-in delay after
>>> doing a printf() before calling do_reset().
>>>
>>> This will result in a change in behavior for the 27 board configuration
>>> files which set CONFIG_PANIC_HANG (less than 5% of the total).  They
>>> will now hang in those fatal error conditions instead of trying to
>>> reboot.
>>>
>>> Given that CONFIG_PANIC_HANG is intended to prevent the system from
>>> rebooting after it has encountered an unrecoverable error, this seems to
>>> be the desired behavior for those 27 board configurations.
>>
>> This is your interpretation, but the users and especially the
>> respective board maintainers may think different.  We should at least
>> try and get feedback from them first.
>
> Well, to be fair, the README says this about the config option:
>
>    CONFIG_PANIC_HANG
>
>    Define this variable to stop the system in case of a
>    fatal error, so that you have to reset it manually.
>    This is probably NOT a good idea for an embedded
>    system where you want the system to reboot
>    automatically as fast as possible, but it may be
>    useful during development since you can try to debug
>    the conditions that lead to the situation.
>
> Cheers,
> Kyle Moffett

This is the description for wanting panic() to hang rather than reset; 
but your patch puts panic()s where there were not, doesn't it? And in so 
doing, it changes the behavior of the code.

Amicalement,
-- 
Albert.