* [U-Boot] [PATCH v2 1/5] bootm: Handle errors consistently @ 2013-07-04 20:17 Simon Glass 2013-07-05 12:59 ` Tom Rini 2013-07-10 13:17 ` Tom Rini 0 siblings, 2 replies; 9+ messages in thread From: Simon Glass @ 2013-07-04 20:17 UTC (permalink / raw) To: u-boot A recent bootm fix left the error path incomplete. Reinstate this so that failures in bootm stages are handled properly. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v2: - Correct checking in the no-error case common/cmd_bootm.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 02a5013..652513a 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -684,12 +684,8 @@ static int do_bootm_states(cmd_tbl_t *cmdtp, int flag, int argc, if (!ret && (states & BOOTM_STATE_OS_GO)) { ret = boot_selected_os(argc, argv, BOOTM_STATE_OS_GO, images, boot_fn); - if (ret) - goto err; } - return ret; - /* Deal with any fallout */ err: if (iflag) @@ -699,7 +695,7 @@ err: bootstage_error(BOOTSTAGE_ID_DECOMP_UNIMPL); else if (ret == BOOTM_ERR_RESET) do_reset(cmdtp, flag, argc, argv); - else + else if (ret) puts("subcommand not supported\n"); return ret; -- 1.8.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2 1/5] bootm: Handle errors consistently 2013-07-04 20:17 [U-Boot] [PATCH v2 1/5] bootm: Handle errors consistently Simon Glass @ 2013-07-05 12:59 ` Tom Rini 2013-07-05 19:52 ` Simon Glass 2013-07-10 13:17 ` Tom Rini 1 sibling, 1 reply; 9+ messages in thread From: Tom Rini @ 2013-07-05 12:59 UTC (permalink / raw) To: u-boot On Thu, Jul 04, 2013 at 01:17:07PM -0700, Simon Glass wrote: > A recent bootm fix left the error path incomplete. Reinstate this so that > failures in bootm stages are handled properly. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > Changes in v2: > - Correct checking in the no-error case OK, this conflicts with the change I posted (and pushed later than I thought I had). Can you confirm the code is good in mainline now? Thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130705/b3309111/attachment.pgp> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2 1/5] bootm: Handle errors consistently 2013-07-05 12:59 ` Tom Rini @ 2013-07-05 19:52 ` Simon Glass 2013-07-05 20:15 ` Tom Rini 0 siblings, 1 reply; 9+ messages in thread From: Simon Glass @ 2013-07-05 19:52 UTC (permalink / raw) To: u-boot Hi Tom, On Fri, Jul 5, 2013 at 5:59 AM, Tom Rini <trini@ti.com> wrote: > On Thu, Jul 04, 2013 at 01:17:07PM -0700, Simon Glass wrote: > > > A recent bootm fix left the error path incomplete. Reinstate this so that > > failures in bootm stages are handled properly. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > Changes in v2: > > - Correct checking in the no-error case > > > OK, this conflicts with the change I posted (and pushed later than I > thought I had). Can you confirm the code is good in mainline now? > Thanks! > It's close, but I think it still needs this near the end of do_bootm_states(), something like: else if (ret == BOOTM_ERR_RESET) do_reset(cmdtp, flag, argc, argv); + else if (ret) + puts("subcommand not supported\n"); return ret; If you agree, I can prepare a patch as part of the bootz update. Regards, Simon ^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2 1/5] bootm: Handle errors consistently 2013-07-05 19:52 ` Simon Glass @ 2013-07-05 20:15 ` Tom Rini 2013-07-05 20:21 ` Simon Glass 0 siblings, 1 reply; 9+ messages in thread From: Tom Rini @ 2013-07-05 20:15 UTC (permalink / raw) To: u-boot On Fri, Jul 05, 2013 at 12:52:03PM -0700, Simon Glass wrote: > Hi Tom, > > On Fri, Jul 5, 2013 at 5:59 AM, Tom Rini <trini@ti.com> wrote: > > > On Thu, Jul 04, 2013 at 01:17:07PM -0700, Simon Glass wrote: > > > > > A recent bootm fix left the error path incomplete. Reinstate this so that > > > failures in bootm stages are handled properly. > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > --- > > > Changes in v2: > > > - Correct checking in the no-error case > > > > > > OK, this conflicts with the change I posted (and pushed later than I > > thought I had). Can you confirm the code is good in mainline now? > > Thanks! > > > > It's close, but I think it still needs this near the end > of do_bootm_states(), something like: > > else if (ret == BOOTM_ERR_RESET) do_reset(cmdtp, flag, argc, argv); + else > if (ret) + puts("subcommand not supported\n"); return ret; > > If you agree, I can prepare a patch as part of the bootz update. How do we get there in the code? When we do any subcalls is where we've got that puts already. Failures from that point on are either the OS bootm part failed (and return is > 0) or one of the BOOTM_ERR codes. Or did I miss a case still? -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130705/41610e89/attachment.pgp> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2 1/5] bootm: Handle errors consistently 2013-07-05 20:15 ` Tom Rini @ 2013-07-05 20:21 ` Simon Glass 2013-07-05 20:29 ` Tom Rini 0 siblings, 1 reply; 9+ messages in thread From: Simon Glass @ 2013-07-05 20:21 UTC (permalink / raw) To: u-boot Hi Tom, On Fri, Jul 5, 2013 at 1:15 PM, Tom Rini <trini@ti.com> wrote: > On Fri, Jul 05, 2013 at 12:52:03PM -0700, Simon Glass wrote: > > Hi Tom, > > > > On Fri, Jul 5, 2013 at 5:59 AM, Tom Rini <trini@ti.com> wrote: > > > > > On Thu, Jul 04, 2013 at 01:17:07PM -0700, Simon Glass wrote: > > > > > > > A recent bootm fix left the error path incomplete. Reinstate this so > that > > > > failures in bootm stages are handled properly. > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > --- > > > > Changes in v2: > > > > - Correct checking in the no-error case > > > > > > > > > OK, this conflicts with the change I posted (and pushed later than I > > > thought I had). Can you confirm the code is good in mainline now? > > > Thanks! > > > > > > > It's close, but I think it still needs this near the end > > of do_bootm_states(), something like: > > > > else if (ret == BOOTM_ERR_RESET) do_reset(cmdtp, flag, argc, argv); + > else > > if (ret) + puts("subcommand not supported\n"); return ret; > > > > If you agree, I can prepare a patch as part of the bootz update. > > How do we get there in the code? When we do any subcalls is where we've > got that puts already. Failures from that point on are either the OS > bootm part failed (and return is > 0) or one of the BOOTM_ERR codes. Or > did I miss a case still? > I think this is when the boot_os function returns an error. At least the old code had quite a lot of printf()s for that case. Regards, Simon ^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2 1/5] bootm: Handle errors consistently 2013-07-05 20:21 ` Simon Glass @ 2013-07-05 20:29 ` Tom Rini 2013-07-05 20:48 ` Simon Glass 0 siblings, 1 reply; 9+ messages in thread From: Tom Rini @ 2013-07-05 20:29 UTC (permalink / raw) To: u-boot On Fri, Jul 05, 2013 at 01:21:09PM -0700, Simon Glass wrote: > Hi Tom, > > On Fri, Jul 5, 2013 at 1:15 PM, Tom Rini <trini@ti.com> wrote: > > > On Fri, Jul 05, 2013 at 12:52:03PM -0700, Simon Glass wrote: > > > Hi Tom, > > > > > > On Fri, Jul 5, 2013 at 5:59 AM, Tom Rini <trini@ti.com> wrote: > > > > > > > On Thu, Jul 04, 2013 at 01:17:07PM -0700, Simon Glass wrote: > > > > > > > > > A recent bootm fix left the error path incomplete. Reinstate this so > > that > > > > > failures in bootm stages are handled properly. > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > --- > > > > > Changes in v2: > > > > > - Correct checking in the no-error case > > > > > > > > > > > > OK, this conflicts with the change I posted (and pushed later than I > > > > thought I had). Can you confirm the code is good in mainline now? > > > > Thanks! > > > > > > > > > > It's close, but I think it still needs this near the end > > > of do_bootm_states(), something like: > > > > > > else if (ret == BOOTM_ERR_RESET) do_reset(cmdtp, flag, argc, argv); + > > else > > > if (ret) + puts("subcommand not supported\n"); return ret; > > > > > > If you agree, I can prepare a patch as part of the bootz update. > > > > How do we get there in the code? When we do any subcalls is where we've > > got that puts already. Failures from that point on are either the OS > > bootm part failed (and return is > 0) or one of the BOOTM_ERR codes. Or > > did I miss a case still? > > > > I think this is when the boot_os function returns an error. At least the > old code had quite a lot of printf()s for that case. We had a printf per subcommand before, and just a single one now. We didn't say the 'go' subcommand failed however, just what the function happened to print out. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130705/6801750d/attachment.pgp> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2 1/5] bootm: Handle errors consistently 2013-07-05 20:29 ` Tom Rini @ 2013-07-05 20:48 ` Simon Glass 2013-07-08 13:24 ` Tom Rini 0 siblings, 1 reply; 9+ messages in thread From: Simon Glass @ 2013-07-05 20:48 UTC (permalink / raw) To: u-boot Hi Tom, On Fri, Jul 5, 2013 at 1:29 PM, Tom Rini <trini@ti.com> wrote: > On Fri, Jul 05, 2013 at 01:21:09PM -0700, Simon Glass wrote: > > Hi Tom, > > > > On Fri, Jul 5, 2013 at 1:15 PM, Tom Rini <trini@ti.com> wrote: > > > > > On Fri, Jul 05, 2013 at 12:52:03PM -0700, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Fri, Jul 5, 2013 at 5:59 AM, Tom Rini <trini@ti.com> wrote: > > > > > > > > > On Thu, Jul 04, 2013 at 01:17:07PM -0700, Simon Glass wrote: > > > > > > > > > > > A recent bootm fix left the error path incomplete. Reinstate > this so > > > that > > > > > > failures in bootm stages are handled properly. > > > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > > --- > > > > > > Changes in v2: > > > > > > - Correct checking in the no-error case > > > > > > > > > > > > > > > OK, this conflicts with the change I posted (and pushed later than > I > > > > > thought I had). Can you confirm the code is good in mainline now? > > > > > Thanks! > > > > > > > > > > > > > It's close, but I think it still needs this near the end > > > > of do_bootm_states(), something like: > > > > > > > > else if (ret == BOOTM_ERR_RESET) do_reset(cmdtp, flag, argc, argv); > + > > > else > > > > if (ret) + puts("subcommand not supported\n"); return ret; > > > > > > > > If you agree, I can prepare a patch as part of the bootz update. > > > > > > How do we get there in the code? When we do any subcalls is where > we've > > > got that puts already. Failures from that point on are either the OS > > > bootm part failed (and return is > 0) or one of the BOOTM_ERR codes. > Or > > > did I miss a case still? > > > > > > > I think this is when the boot_os function returns an error. At least the > > old code had quite a lot of printf()s for that case. > > We had a printf per subcommand before, and just a single one now. We > didn't say the 'go' subcommand failed however, just what the > function happened to print out. > Yes that looks right to me. But I believe that the GO command is not supposed to return, so it might be harmless to put the message there in all cases. If not, we can add a special case for GO. Regards, Simon ^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2 1/5] bootm: Handle errors consistently 2013-07-05 20:48 ` Simon Glass @ 2013-07-08 13:24 ` Tom Rini 0 siblings, 0 replies; 9+ messages in thread From: Tom Rini @ 2013-07-08 13:24 UTC (permalink / raw) To: u-boot On Fri, Jul 05, 2013 at 01:48:30PM -0700, Simon Glass wrote: > Hi Tom, > > On Fri, Jul 5, 2013 at 1:29 PM, Tom Rini <trini@ti.com> wrote: > > > On Fri, Jul 05, 2013 at 01:21:09PM -0700, Simon Glass wrote: > > > Hi Tom, > > > > > > On Fri, Jul 5, 2013 at 1:15 PM, Tom Rini <trini@ti.com> wrote: > > > > > > > On Fri, Jul 05, 2013 at 12:52:03PM -0700, Simon Glass wrote: > > > > > Hi Tom, > > > > > > > > > > On Fri, Jul 5, 2013 at 5:59 AM, Tom Rini <trini@ti.com> wrote: > > > > > > > > > > > On Thu, Jul 04, 2013 at 01:17:07PM -0700, Simon Glass wrote: > > > > > > > > > > > > > A recent bootm fix left the error path incomplete. Reinstate > > this so > > > > that > > > > > > > failures in bootm stages are handled properly. > > > > > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > > > --- > > > > > > > Changes in v2: > > > > > > > - Correct checking in the no-error case > > > > > > > > > > > > > > > > > > OK, this conflicts with the change I posted (and pushed later than > > I > > > > > > thought I had). Can you confirm the code is good in mainline now? > > > > > > Thanks! > > > > > > > > > > > > > > > > It's close, but I think it still needs this near the end > > > > > of do_bootm_states(), something like: > > > > > > > > > > else if (ret == BOOTM_ERR_RESET) do_reset(cmdtp, flag, argc, argv); > > + > > > > else > > > > > if (ret) + puts("subcommand not supported\n"); return ret; > > > > > > > > > > If you agree, I can prepare a patch as part of the bootz update. > > > > > > > > How do we get there in the code? When we do any subcalls is where > > we've > > > > got that puts already. Failures from that point on are either the OS > > > > bootm part failed (and return is > 0) or one of the BOOTM_ERR codes. > > Or > > > > did I miss a case still? > > > > > > > > > > I think this is when the boot_os function returns an error. At least the > > > old code had quite a lot of printf()s for that case. > > > > We had a printf per subcommand before, and just a single one now. We > > didn't say the 'go' subcommand failed however, just what the > > function happened to print out. > > Yes that looks right to me. But I believe that the GO command is not > supposed to return, so it might be harmless to put the message there in all > cases. If not, we can add a special case for GO. OK, I've been re-reading the before and after code, and at least wrt error messages and such, we're OK now. The 'GO' state is allowed to return 1 and usually does both for detectable errors (for example, FIT image for VxWorks, but a bad image) and "wth? we've been returned to from the OS". And I don't want to add a won't be reached string to the build given how badly we see compilers dealing with optimizing strings, along with the code bloat reason. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130708/5efee49e/attachment.pgp> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2 1/5] bootm: Handle errors consistently 2013-07-04 20:17 [U-Boot] [PATCH v2 1/5] bootm: Handle errors consistently Simon Glass 2013-07-05 12:59 ` Tom Rini @ 2013-07-10 13:17 ` Tom Rini 1 sibling, 0 replies; 9+ messages in thread From: Tom Rini @ 2013-07-10 13:17 UTC (permalink / raw) To: u-boot On Thu, Jul 04, 2013 at 01:17:07PM -0700, Simon Glass wrote: > A recent bootm fix left the error path incomplete. Reinstate this so that > failures in bootm stages are handled properly. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > Changes in v2: > - Correct checking in the no-error case > > common/cmd_bootm.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c > index 02a5013..652513a 100644 > --- a/common/cmd_bootm.c > +++ b/common/cmd_bootm.c > @@ -684,12 +684,8 @@ static int do_bootm_states(cmd_tbl_t *cmdtp, int flag, int argc, > if (!ret && (states & BOOTM_STATE_OS_GO)) { > ret = boot_selected_os(argc, argv, BOOTM_STATE_OS_GO, > images, boot_fn); > - if (ret) > - goto err; > } > > - return ret; > - > /* Deal with any fallout */ > err: > if (iflag) > @@ -699,7 +695,7 @@ err: > bootstage_error(BOOTSTAGE_ID_DECOMP_UNIMPL); > else if (ret == BOOTM_ERR_RESET) > do_reset(cmdtp, flag, argc, argv); > - else > + else if (ret) > puts("subcommand not supported\n"); > > return ret; Looking this part over again, BOOTM_STATE_OS_GO handles its own prints when returning 1, so we don't want to do the "subcommand not supported" part here (the other case is a BOOTM_ERR_RESET). But, trace does need to be covered and isn't by the "subcommand not supported" print. I'll make a v3 of this shortly and post. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130710/d75035b1/attachment.pgp> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-07-10 13:17 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-07-04 20:17 [U-Boot] [PATCH v2 1/5] bootm: Handle errors consistently Simon Glass 2013-07-05 12:59 ` Tom Rini 2013-07-05 19:52 ` Simon Glass 2013-07-05 20:15 ` Tom Rini 2013-07-05 20:21 ` Simon Glass 2013-07-05 20:29 ` Tom Rini 2013-07-05 20:48 ` Simon Glass 2013-07-08 13:24 ` Tom Rini 2013-07-10 13:17 ` Tom Rini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.