[U-Boot] [PATCH] common: Fix logic in fpga programming

[U-Boot] [PATCH] common: Fix logic in fpga programming

[Michal Simek]

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);
 		break;
 	default:
 		printf("The given image format is not supported (corrupt?)\n");
-- 
1.9.1

[U-Boot] [PATCH] common: Fix logic in fpga programming

[Mike Looijmans]

?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 at topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

[U-Boot] [PATCH] common: Fix logic in fpga programming

[Michal Simek]

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

[U-Boot] [PATCH] common: Fix logic in fpga programming

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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
Anarchy may not be the best form of government, but it's better  than
no government at all.

[U-Boot] [PATCH] common: Fix logic in fpga programming

[Michal Simek]

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

[U-Boot] [PATCH] common: Fix logic in fpga programming

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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
Contrary to popular belief, thinking does not cause brain damage.

[U-Boot] [PATCH] common: Fix logic in fpga programming

[Michal Simek]

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