| Message ID | 1319134031-28503-1-git-send-email-Kyle.D.Moffett@boeing.com |
|---|---|
| State | Rejected |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
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
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/
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,
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)
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(-)