Message ID | 1e029d3a67722c20d8b6194d3388efe5ca92f5bf.1481881501.git.michal.simek@xilinx.com |
---|---|
State | Deferred |
Delegated to: | Tom Rini |
Headers | show |
On 16-12-16 10:45, Michal Simek wrote: > Stop boot process if fpga programming fails. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > common/image.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/common/image.c b/common/image.c > index bd07e86701a4..e3486e46a459 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -1375,11 +1375,10 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, > img_len, BIT_PARTIAL); > } > > - printf(" Programming %s bitstream... ", name); > if (err) > - printf("failed\n"); > - else > - printf("OK\n"); > + return 1; Wouldn't "return err;" or "return -EIO;" make sense here instead of a magic "1"? > + > + printf(" Programming %s bitstream... OK", name); > break; > default: > printf("The given image format is not supported (corrupt?)\n"); > Kind regards, Mike Looijmans System Expert TOPIC Products Materiaalweg 4, NL-5681 RJ Best Postbus 440, NL-5680 AK Best Telefoon: +31 (0) 499 33 69 79 E-mail: mike.looijmans@topicproducts.com Website: www.topicproducts.com Please consider the environment before printing this e-mail
On 16.12.2016 10:50, Mike Looijmans wrote: > On 16-12-16 10:45, Michal Simek wrote: >> Stop boot process if fpga programming fails. >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> --- >> >> common/image.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/common/image.c b/common/image.c >> index bd07e86701a4..e3486e46a459 100644 >> --- a/common/image.c >> +++ b/common/image.c >> @@ -1375,11 +1375,10 @@ int boot_get_fpga(int argc, char * const >> argv[], bootm_headers_t *images, >> img_len, BIT_PARTIAL); >> } >> >> - printf(" Programming %s bitstream... ", name); >> if (err) >> - printf("failed\n"); >> - else >> - printf("OK\n"); >> + return 1; > > Wouldn't "return err;" or "return -EIO;" make sense here instead of a > magic "1"? Good idea. Will send v2 Thanks, Michal
Dear Michal... In message <1e029d3a67722c20d8b6194d3388efe5ca92f5bf.1481881501.git.michal.simek@xilinx.com> you wrote: > Stop boot process if fpga programming fails. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > common/image.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/common/image.c b/common/image.c > index bd07e86701a4..e3486e46a459 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -1375,11 +1375,10 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, > img_len, BIT_PARTIAL); > } > > - printf(" Programming %s bitstream... ", name); > if (err) > - printf("failed\n"); > - else > - printf("OK\n"); > + return 1; > + > + printf(" Programming %s bitstream... OK", name); Good old U-Boot style says we print a message when we start an operation, and then an OK / failed when done. When the system crashes in this command, you can see at last where it crashed, i. e. what the last running command was. Your change breaks this - in the error path nothing gets printed. This is even worse, as even though the system keeps running the user has no indication of what failed... I suggest to keep the printf() as before, and just fix the return code thing. Best regards, Wolfgang Denk
On 20.12.2016 18:30, Wolfgang Denk wrote: > Dear Michal... > > In message <1e029d3a67722c20d8b6194d3388efe5ca92f5bf.1481881501.git.michal.simek@xilinx.com> you wrote: >> Stop boot process if fpga programming fails. >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> --- >> >> common/image.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/common/image.c b/common/image.c >> index bd07e86701a4..e3486e46a459 100644 >> --- a/common/image.c >> +++ b/common/image.c >> @@ -1375,11 +1375,10 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, >> img_len, BIT_PARTIAL); >> } >> >> - printf(" Programming %s bitstream... ", name); >> if (err) >> - printf("failed\n"); >> - else >> - printf("OK\n"); >> + return 1; >> + >> + printf(" Programming %s bitstream... OK", name); > > Good old U-Boot style says we print a message when we start an > operation, and then an OK / failed when done. When the system crashes > in this command, you can see at last where it crashed, i. e. what the > last running command was. Based on this style the first part of this message should be called at the beginning of this function not in this possition. > > Your change breaks this - in the error path nothing gets printed. If you look at the code 253 ret = boot_get_fpga(argc, argv, &images, IH_ARCH_DEFAULT, 254 NULL, NULL); 255 if (ret) { 256 printf("FPGA image is corrupted or invalid\n"); 257 return 1; 258 } There is already printf for error. > > This is even worse, as even though the system keeps running the user > has no indication of what failed... Isn't it message above enough? Thanks, Michal
Dear Michal, In message <0e8b363f-f84f-cea2-7784-8cc10e1e751b@xilinx.com> you wrote: > > > Good old U-Boot style says we print a message when we start an > > operation, and then an OK / failed when done. When the system crashes > > in this command, you can see at last where it crashed, i. e. what the > > last running command was. > > Based on this style the first part of this message should be called at > the beginning of this function not in this possition. True,,, > 255 if (ret) { > 256 printf("FPGA image is corrupted or invalid\n"); > 257 return 1; > 258 } > > There is already printf for error. Hm... Sorry, I did not follow all brachnes thrugh the code this i snot exactly obvious from the patch. > > This is even worse, as even though the system keeps running the user > > has no indication of what failed... > > Isn't it message above enough? In the patch, I see only a "return 1;". Best regards, Wolfgang Denk
Dear Wolfgang, On 21.12.2016 00:19, Wolfgang Denk wrote: > Dear Michal, > > In message <0e8b363f-f84f-cea2-7784-8cc10e1e751b@xilinx.com> you wrote: >> >>> Good old U-Boot style says we print a message when we start an >>> operation, and then an OK / failed when done. When the system crashes >>> in this command, you can see at last where it crashed, i. e. what the >>> last running command was. >> >> Based on this style the first part of this message should be called at >> the beginning of this function not in this possition. > > True,,, > >> 255 if (ret) { >> 256 printf("FPGA image is corrupted or invalid\n"); >> 257 return 1; >> 258 } >> >> There is already printf for error. > > Hm... Sorry, I did not follow all brachnes thrugh the code this i snot > exactly obvious from the patch. ok. > >>> This is even worse, as even though the system keeps running the user >>> has no indication of what failed... >> >> Isn't it message above enough? > > In the patch, I see only a "return 1;". Like in every patch which error out in the middle of function and return 0 at the end. Please look at that function and you will see that at the end there is return 0. Thanks, Michal
diff --git a/common/image.c b/common/image.c index bd07e86701a4..e3486e46a459 100644 --- a/common/image.c +++ b/common/image.c @@ -1375,11 +1375,10 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, img_len, BIT_PARTIAL); } - printf(" Programming %s bitstream... ", name); if (err) - printf("failed\n"); - else - printf("OK\n"); + return 1; + + printf(" Programming %s bitstream... OK", name); break; default: printf("The given image format is not supported (corrupt?)\n");
Stop boot process if fpga programming fails. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- common/image.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)