[U-Boot] [PATCH 1/2] spl: Fix redundant image of uboot

[U-Boot] [PATCH 1/2] spl: Fix redundant image of uboot

[Michael Trimarchi]

We need to address the redundat image case and undestand if the
image is corrupted or not and fallback to the copy. The function
used before was always return 0 without any evaluation of the
error. We try to make it work properly

Change-Id: Id6fc221c5cc08934b7324dd5d319b93c56e2e678
Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
 common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
index 9a52500..b96fce2 100644
--- a/common/spl/spl_nand.c
+++ b/common/spl/spl_nand.c
@@ -44,9 +44,7 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
 {
 	int err;
 
-	err = nand_spl_load_image(offset, sizeof(*header), (void *)header);
-	if (err)
-		return err;
+	nand_spl_load_image(offset, sizeof(*header), (void *)header);
 
 	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
 	    image_get_magic(header) == FDT_MAGIC) {
@@ -59,13 +57,31 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
 		load.bl_len = 1;
 		load.read = spl_nand_fit_read;
 		return spl_load_simple_fit(spl_image, &load, offset, header);
-	} else {
-		err = spl_parse_image_header(spl_image, header);
-		if (err)
-			return err;
-		return nand_spl_load_image(offset, spl_image->size,
-					   (void *)(ulong)spl_image->load_addr);
 	}
+	err = spl_parse_image_header(spl_image, header);
+	if (err)
+		return err;
+
+	nand_spl_load_image(offset, spl_image->size,
+			   (void *)(ulong)spl_image->load_addr);
+
+	/*
+	 * Logic of the error is inverted for image_check* functions.
+	 * We want to verify that header is correct and the data are correct
+	 * for LEGACY image type
+	 */
+	err = image_check_hcrc((const image_header_t *)spl_image->load_addr);
+	if (!err) {
+		debug("Header checksum failed\n");
+		return -EINVAL;
+	}
+	err = image_check_dcrc((const image_header_t *)spl_image->load_addr);
+	if (!err) {
+		debug("Image checksum failed\n");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static int spl_nand_load_image(struct spl_image_info *spl_image,
-- 
2.7.4

[U-Boot] [PATCH 2/2] spl: Make the spl_nand_load_image static

[Michael Trimarchi]

Change-Id: I1c2f71e75fc052c54fc94b13d0942cb9e75ff1c6
Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
 common/spl/spl_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
index b96fce2..527cfc0 100644
--- a/common/spl/spl_nand.c
+++ b/common/spl/spl_nand.c
@@ -12,7 +12,7 @@
 #include <fdt.h>
 
 #if defined(CONFIG_SPL_NAND_RAW_ONLY)
-int spl_nand_load_image(struct spl_image_info *spl_image,
+static int spl_nand_load_image(struct spl_image_info *spl_image,
 			struct spl_boot_device *bootdev)
 {
 	nand_init();
-- 
2.7.4

[U-Boot] [U-Boot,2/2] spl: Make the spl_nand_load_image static

[Tom Rini]

On Wed, Jul 04, 2018 at 03:53:37PM +0200, Michael Trimarchi wrote:

> Change-Id: I1c2f71e75fc052c54fc94b13d0942cb9e75ff1c6
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180720/d9499837/attachment.sig>

[U-Boot] [PATCH 1/2] spl: Fix redundant image of uboot

[Lothar Waßmann]

Hi,

On Wed,  4 Jul 2018 15:53:36 +0200 Michael Trimarchi wrote:
> We need to address the redundat image case and undestand if the
> image is corrupted or not and fallback to the copy. The function
> used before was always return 0 without any evaluation of the
> error. We try to make it work properly
> 
> Change-Id: Id6fc221c5cc08934b7324dd5d319b93c56e2e678
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
>  common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
> index 9a52500..b96fce2 100644
> --- a/common/spl/spl_nand.c
> +++ b/common/spl/spl_nand.c
> @@ -44,9 +44,7 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
>  {
>  	int err;
>  
> -	err = nand_spl_load_image(offset, sizeof(*header), (void *)header);
> -	if (err)
> -		return err;
> +	nand_spl_load_image(offset, sizeof(*header), (void *)header);
>  
>  	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
>  	    image_get_magic(header) == FDT_MAGIC) {
> @@ -59,13 +57,31 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
>  		load.bl_len = 1;
>  		load.read = spl_nand_fit_read;
>  		return spl_load_simple_fit(spl_image, &load, offset, header);
> -	} else {
> -		err = spl_parse_image_header(spl_image, header);
> -		if (err)
> -			return err;
> -		return nand_spl_load_image(offset, spl_image->size,
> -					   (void *)(ulong)spl_image->load_addr);
>  	}
> +	err = spl_parse_image_header(spl_image, header);
> +	if (err)
> +		return err;
> +
> +	nand_spl_load_image(offset, spl_image->size,
> +			   (void *)(ulong)spl_image->load_addr);
> +
> +	/*
> +	 * Logic of the error is inverted for image_check* functions.
> +	 * We want to verify that header is correct and the data are correct
> +	 * for LEGACY image type
> +	 */
> +	err = image_check_hcrc((const image_header_t *)spl_image->load_addr);
> +	if (!err) {
>
Why not simply:
	if (!image_check_hcrc(...) {
> +		debug("Header checksum failed\n");
> +		return -EINVAL;
> +	}
> +	err = image_check_dcrc((const image_header_t *)spl_image->load_addr);
> +	if (!err) {
>
dto.

> +		debug("Image checksum failed\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }
>  
>  static int spl_nand_load_image(struct spl_image_info *spl_image,


Lothar Waßmann

[U-Boot] [PATCH 1/2] spl: Fix redundant image of uboot

[Michael Nazzareno Trimarchi]

Hi

On Wed, Jul 4, 2018 at 4:19 PM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> Hi,
>
> On Wed,  4 Jul 2018 15:53:36 +0200 Michael Trimarchi wrote:
>> We need to address the redundat image case and undestand if the
>> image is corrupted or not and fallback to the copy. The function
>> used before was always return 0 without any evaluation of the
>> error. We try to make it work properly
>>
>> Change-Id: Id6fc221c5cc08934b7324dd5d319b93c56e2e678
>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>> ---
>>  common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
>>  1 file changed, 25 insertions(+), 9 deletions(-)
>>
>> diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
>> index 9a52500..b96fce2 100644
>> --- a/common/spl/spl_nand.c
>> +++ b/common/spl/spl_nand.c
>> @@ -44,9 +44,7 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
>>  {
>>       int err;
>>
>> -     err = nand_spl_load_image(offset, sizeof(*header), (void *)header);
>> -     if (err)
>> -             return err;
>> +     nand_spl_load_image(offset, sizeof(*header), (void *)header);
>>
>>       if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
>>           image_get_magic(header) == FDT_MAGIC) {
>> @@ -59,13 +57,31 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
>>               load.bl_len = 1;
>>               load.read = spl_nand_fit_read;
>>               return spl_load_simple_fit(spl_image, &load, offset, header);
>> -     } else {
>> -             err = spl_parse_image_header(spl_image, header);
>> -             if (err)
>> -                     return err;
>> -             return nand_spl_load_image(offset, spl_image->size,
>> -                                        (void *)(ulong)spl_image->load_addr);
>>       }
>> +     err = spl_parse_image_header(spl_image, header);
>> +     if (err)
>> +             return err;
>> +
>> +     nand_spl_load_image(offset, spl_image->size,
>> +                        (void *)(ulong)spl_image->load_addr);
>> +
>> +     /*
>> +      * Logic of the error is inverted for image_check* functions.
>> +      * We want to verify that header is correct and the data are correct
>> +      * for LEGACY image type
>> +      */
>> +     err = image_check_hcrc((const image_header_t *)spl_image->load_addr);
>> +     if (!err) {
>>
> Why not simply:
>         if (!image_check_hcrc(...) {
>> +             debug("Header checksum failed\n");
>> +             return -EINVAL;
>> +     }
>> +     err = image_check_dcrc((const image_header_t *)spl_image->load_addr);
>> +     if (!err) {
>>
> dto.

Just because I want to be in the style of this file ;)

Michael

>
>> +             debug("Image checksum failed\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     return 0;
>>  }
>>
>>  static int spl_nand_load_image(struct spl_image_info *spl_image,
>
>
> Lothar Waßmann



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[U-Boot] [PATCH 1/2] spl: Fix redundant image of uboot

[Lothar Waßmann]

Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:

> Hi
> 
> On Wed, Jul 4, 2018 at 4:19 PM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> > Hi,
> >
> > On Wed,  4 Jul 2018 15:53:36 +0200 Michael Trimarchi wrote:
> >> We need to address the redundat image case and undestand if the
> >> image is corrupted or not and fallback to the copy. The function
> >> used before was always return 0 without any evaluation of the
> >> error. We try to make it work properly
> >>
> >> Change-Id: Id6fc221c5cc08934b7324dd5d319b93c56e2e678
> >> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> >> ---
> >>  common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
> >>  1 file changed, 25 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
> >> index 9a52500..b96fce2 100644
> >> --- a/common/spl/spl_nand.c
> >> +++ b/common/spl/spl_nand.c
> >> @@ -44,9 +44,7 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
> >>  {
> >>       int err;
> >>
> >> -     err = nand_spl_load_image(offset, sizeof(*header), (void *)header);
> >> -     if (err)
> >> -             return err;
> >> +     nand_spl_load_image(offset, sizeof(*header), (void *)header);
> >>
> >>       if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
> >>           image_get_magic(header) == FDT_MAGIC) {
> >> @@ -59,13 +57,31 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
> >>               load.bl_len = 1;
> >>               load.read = spl_nand_fit_read;
> >>               return spl_load_simple_fit(spl_image, &load, offset, header);
> >> -     } else {
> >> -             err = spl_parse_image_header(spl_image, header);
> >> -             if (err)
> >> -                     return err;
> >> -             return nand_spl_load_image(offset, spl_image->size,
> >> -                                        (void *)(ulong)spl_image->load_addr);
> >>       }
> >> +     err = spl_parse_image_header(spl_image, header);
> >> +     if (err)
> >> +             return err;
> >> +
> >> +     nand_spl_load_image(offset, spl_image->size,
> >> +                        (void *)(ulong)spl_image->load_addr);
> >> +
> >> +     /*
> >> +      * Logic of the error is inverted for image_check* functions.
> >> +      * We want to verify that header is correct and the data are correct
> >> +      * for LEGACY image type
> >> +      */
> >> +     err = image_check_hcrc((const image_header_t *)spl_image->load_addr);
> >> +     if (!err) {
> >>
> > Why not simply:
> >         if (!image_check_hcrc(...) {
> >> +             debug("Header checksum failed\n");
> >> +             return -EINVAL;
> >> +     }
> >> +     err = image_check_dcrc((const image_header_t *)spl_image->load_addr);
> >> +     if (!err) {
> >>
> > dto.
> 
> Just because I want to be in the style of this file ;)
> 
In other places the 'err' variable is used as return value.
If you don't need it for that, it's vain to assign a value to it.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[U-Boot] [PATCH 1/2] spl: Fix redundant image of uboot

[Michael Nazzareno Trimarchi]

Hi

On Wed, Jul 4, 2018 at 5:10 PM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
>
>> Hi
>>
>> On Wed, Jul 4, 2018 at 4:19 PM, Lothar Waßmann <LW@karo-electronics.de> wrote:
>> > Hi,
>> >
>> > On Wed,  4 Jul 2018 15:53:36 +0200 Michael Trimarchi wrote:
>> >> We need to address the redundat image case and undestand if the
>> >> image is corrupted or not and fallback to the copy. The function
>> >> used before was always return 0 without any evaluation of the
>> >> error. We try to make it work properly
>> >>
>> >> Change-Id: Id6fc221c5cc08934b7324dd5d319b93c56e2e678
>> >> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>> >> ---
>> >>  common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
>> >>  1 file changed, 25 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
>> >> index 9a52500..b96fce2 100644
>> >> --- a/common/spl/spl_nand.c
>> >> +++ b/common/spl/spl_nand.c
>> >> @@ -44,9 +44,7 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
>> >>  {
>> >>       int err;
>> >>
>> >> -     err = nand_spl_load_image(offset, sizeof(*header), (void *)header);
>> >> -     if (err)
>> >> -             return err;
>> >> +     nand_spl_load_image(offset, sizeof(*header), (void *)header);
>> >>
>> >>       if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
>> >>           image_get_magic(header) == FDT_MAGIC) {
>> >> @@ -59,13 +57,31 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
>> >>               load.bl_len = 1;
>> >>               load.read = spl_nand_fit_read;
>> >>               return spl_load_simple_fit(spl_image, &load, offset, header);
>> >> -     } else {
>> >> -             err = spl_parse_image_header(spl_image, header);
>> >> -             if (err)
>> >> -                     return err;
>> >> -             return nand_spl_load_image(offset, spl_image->size,
>> >> -                                        (void *)(ulong)spl_image->load_addr);
>> >>       }
>> >> +     err = spl_parse_image_header(spl_image, header);
>> >> +     if (err)
>> >> +             return err;
>> >> +
>> >> +     nand_spl_load_image(offset, spl_image->size,
>> >> +                        (void *)(ulong)spl_image->load_addr);
>> >> +
>> >> +     /*
>> >> +      * Logic of the error is inverted for image_check* functions.
>> >> +      * We want to verify that header is correct and the data are correct
>> >> +      * for LEGACY image type
>> >> +      */
>> >> +     err = image_check_hcrc((const image_header_t *)spl_image->load_addr);
>> >> +     if (!err) {
>> >>
>> > Why not simply:
>> >         if (!image_check_hcrc(...) {
>> >> +             debug("Header checksum failed\n");
>> >> +             return -EINVAL;
>> >> +     }
>> >> +     err = image_check_dcrc((const image_header_t *)spl_image->load_addr);
>> >> +     if (!err) {
>> >>
>> > dto.
>>
>> Just because I want to be in the style of this file ;)
>>
> In other places the 'err' variable is used as return value.
> If you don't need it for that, it's vain to assign a value to it.
>

Let me collect all the comments and I will follow your suggestion on next post

Michael

>
> Lothar Waßmann
> --
> ___________________________________________________________
>
> Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
> Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
> Geschäftsführer: Matthias Kaussen
> Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
>
> www.karo-electronics.de | info at karo-electronics.de
> ___________________________________________________________



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[U-Boot] [PATCH V2 1/2] spl: Fix redundant image of uboot

[Michael Trimarchi]

We need to address the redundat image case and undestand if the
image is corrupted or not. In error case we need to try the fallback copy.
The function used before was always return 0 without any evaluation of the
error. We try to make it work properly

Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
Changes V1->V2:
	Address the comments on using the err variable
---
 common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
index 9a52500..109dd6e 100644
--- a/common/spl/spl_nand.c
+++ b/common/spl/spl_nand.c
@@ -44,9 +44,7 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
 {
 	int err;
 
-	err = nand_spl_load_image(offset, sizeof(*header), (void *)header);
-	if (err)
-		return err;
+	nand_spl_load_image(offset, sizeof(*header), (void *)header);
 
 	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
 	    image_get_magic(header) == FDT_MAGIC) {
@@ -59,13 +57,31 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
 		load.bl_len = 1;
 		load.read = spl_nand_fit_read;
 		return spl_load_simple_fit(spl_image, &load, offset, header);
-	} else {
-		err = spl_parse_image_header(spl_image, header);
-		if (err)
-			return err;
-		return nand_spl_load_image(offset, spl_image->size,
-					   (void *)(ulong)spl_image->load_addr);
 	}
+	err = spl_parse_image_header(spl_image, header);
+	if (err)
+		return err;
+
+	nand_spl_load_image(offset, spl_image->size,
+			   (void *)(ulong)spl_image->load_addr);
+
+	/*
+	 * Verify the crc of the header
+	 */
+	if (!((const image_header_t *)spl_image->load_addr)) {
+		debug("Header checksum failed\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Verify the crc of the data
+	 */
+	if (!image_check_dcrc((const image_header_t *)spl_image->load_addr)) {
+		debug("Image checksum failed\n");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static int spl_nand_load_image(struct spl_image_info *spl_image,
-- 
2.7.4

-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[U-Boot] [PATCH V3 1/2] spl: Fix redundant image of uboot

[Michael Trimarchi]

We need to address the redundat image case and undestand if the
image is corrupted or not. In error case we need to try the fallback copy.
The function used before was always return 0 without any evaluation of the
error. We try to make it work properly

Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
Changes V2->V3:
        Fix patch mistake due the a wrong edit of it
Changes V1->V2:
        Address the comments on using the err variable
---
 common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
index 9a52500..f00246d 100644
--- a/common/spl/spl_nand.c
+++ b/common/spl/spl_nand.c
@@ -44,9 +44,7 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
 {
 	int err;
 
-	err = nand_spl_load_image(offset, sizeof(*header), (void *)header);
-	if (err)
-		return err;
+	nand_spl_load_image(offset, sizeof(*header), (void *)header);
 
 	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
 	    image_get_magic(header) == FDT_MAGIC) {
@@ -59,13 +57,31 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
 		load.bl_len = 1;
 		load.read = spl_nand_fit_read;
 		return spl_load_simple_fit(spl_image, &load, offset, header);
-	} else {
-		err = spl_parse_image_header(spl_image, header);
-		if (err)
-			return err;
-		return nand_spl_load_image(offset, spl_image->size,
-					   (void *)(ulong)spl_image->load_addr);
 	}
+	err = spl_parse_image_header(spl_image, header);
+	if (err)
+		return err;
+
+	nand_spl_load_image(offset, spl_image->size,
+			   (void *)(ulong)spl_image->load_addr);
+
+	/*
+	 * Verify the crc of the header
+	 */
+	if (!image_check_hcrc((const image_header_t *)spl_image->load_addr)) {
+		debug("Header checksum failed\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Verify the crc of the data
+	 */
+	if (!image_check_dcrc((const image_header_t *)spl_image->load_addr)) {
+		debug("Image checksum failed\n");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static int spl_nand_load_image(struct spl_image_info *spl_image,
-- 
2.7.4

-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[U-Boot] [PATCH V3 1/2] spl: Fix redundant image of uboot

[Michael Nazzareno Trimarchi]

Hi Lothar

On Fri, Jul 6, 2018 at 5:09 PM, Michael Trimarchi
<michael@amarulasolutions.com> wrote:
> We need to address the redundat image case and undestand if the
> image is corrupted or not. In error case we need to try the fallback copy.
> The function used before was always return 0 without any evaluation of the
> error. We try to make it work properly
>
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>

Any more comment?

Michael

> ---
> Changes V2->V3:
>         Fix patch mistake due the a wrong edit of it
> Changes V1->V2:
>         Address the comments on using the err variable
> ---
>  common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
> index 9a52500..f00246d 100644
> --- a/common/spl/spl_nand.c
> +++ b/common/spl/spl_nand.c
> @@ -44,9 +44,7 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
>  {
>         int err;
>
> -       err = nand_spl_load_image(offset, sizeof(*header), (void *)header);
> -       if (err)
> -               return err;
> +       nand_spl_load_image(offset, sizeof(*header), (void *)header);
>
>         if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
>             image_get_magic(header) == FDT_MAGIC) {
> @@ -59,13 +57,31 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
>                 load.bl_len = 1;
>                 load.read = spl_nand_fit_read;
>                 return spl_load_simple_fit(spl_image, &load, offset, header);
> -       } else {
> -               err = spl_parse_image_header(spl_image, header);
> -               if (err)
> -                       return err;
> -               return nand_spl_load_image(offset, spl_image->size,
> -                                          (void *)(ulong)spl_image->load_addr);
>         }
> +       err = spl_parse_image_header(spl_image, header);
> +       if (err)
> +               return err;
> +
> +       nand_spl_load_image(offset, spl_image->size,
> +                          (void *)(ulong)spl_image->load_addr);
> +
> +       /*
> +        * Verify the crc of the header
> +        */
> +       if (!image_check_hcrc((const image_header_t *)spl_image->load_addr)) {
> +               debug("Header checksum failed\n");
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * Verify the crc of the data
> +        */
> +       if (!image_check_dcrc((const image_header_t *)spl_image->load_addr)) {
> +               debug("Image checksum failed\n");
> +               return -EINVAL;
> +       }
> +
> +       return 0;
>  }
>
>  static int spl_nand_load_image(struct spl_image_info *spl_image,
> --
> 2.7.4
>
> --
> | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> | COO  -  Founder                                      Cruquiuskade 47 |
> | +31(0)851119172                                 Amsterdam 1018 AM NL |
> |                  [`as] http://www.amarulasolutions.com               |



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[U-Boot] [PATCH V3 1/2] spl: Fix redundant image of uboot

[Lothar Waßmann]

Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:

> Hi Lothar
> 
> On Fri, Jul 6, 2018 at 5:09 PM, Michael Trimarchi
> <michael@amarulasolutions.com> wrote:
> > We need to address the redundat image case and undestand if the
> > image is corrupted or not. In error case we need to try the fallback copy.
> > The function used before was always return 0 without any evaluation of the
> > error. We try to make it work properly
> >
> > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> 
> Any more comment?
> 
Apart from the typos in your first sentence of your cover
letter (redundat ... undestand), no.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[U-Boot] [U-Boot,V3,1/2] spl: Fix redundant image of uboot

[Tom Rini]

On Fri, Jul 06, 2018 at 05:09:22PM +0200, Michael Trimarchi wrote:

> We need to address the redundat image case and undestand if the
> image is corrupted or not. In error case we need to try the fallback copy.
> The function used before was always return 0 without any evaluation of the
> error. We try to make it work properly
> 
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
> Changes V2->V3:
>         Fix patch mistake due the a wrong edit of it
> Changes V1->V2:
>         Address the comments on using the err variable
> ---
>  common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)

I see two problems here.  First, this is a generic issue (any
legacy-style U-Boot image that we load should be verified).  Second, we
need to make this behavior configurable as as-is this overflows one
board (omapl138_lcdk) and I expect would be problematic for many more
boards when we make it done more commonly.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180720/c81bbd92/attachment.sig>

[U-Boot] [U-Boot,V3,1/2] spl: Fix redundant image of uboot

[Michael Nazzareno Trimarchi]

Hi Tom

On Fri, Jul 20, 2018 at 9:54 PM, Tom Rini <trini@konsulko.com> wrote:
> On Fri, Jul 06, 2018 at 05:09:22PM +0200, Michael Trimarchi wrote:
>
>> We need to address the redundat image case and undestand if the
>> image is corrupted or not. In error case we need to try the fallback copy.
>> The function used before was always return 0 without any evaluation of the
>> error. We try to make it work properly
>>
>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>> ---
>> Changes V2->V3:
>>         Fix patch mistake due the a wrong edit of it
>> Changes V1->V2:
>>         Address the comments on using the err variable
>> ---
>>  common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
>>  1 file changed, 25 insertions(+), 9 deletions(-)
>
> I see two problems here.  First, this is a generic issue (any
> legacy-style U-Boot image that we load should be verified).  Second, we
> need to make this behavior configurable as as-is this overflows one
> board (omapl138_lcdk) and I expect would be problematic for many more
> boards when we make it done more commonly.
>

This patch fix a no-working uboot feature and this was the address problem on
the specific case. We can call ->verify image every ->load, anyway can you
explain better why you need a configurable behavior?

Michael

> --
> Tom



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[U-Boot] [U-Boot,V3,1/2] spl: Fix redundant image of uboot

[Tom Rini]

On Fri, Jul 20, 2018 at 10:09:00PM +0200, Michael Nazzareno Trimarchi wrote:
> Hi Tom
> 
> On Fri, Jul 20, 2018 at 9:54 PM, Tom Rini <trini@konsulko.com> wrote:
> > On Fri, Jul 06, 2018 at 05:09:22PM +0200, Michael Trimarchi wrote:
> >
> >> We need to address the redundat image case and undestand if the
> >> image is corrupted or not. In error case we need to try the fallback copy.
> >> The function used before was always return 0 without any evaluation of the
> >> error. We try to make it work properly
> >>
> >> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> >> ---
> >> Changes V2->V3:
> >>         Fix patch mistake due the a wrong edit of it
> >> Changes V1->V2:
> >>         Address the comments on using the err variable
> >> ---
> >>  common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
> >>  1 file changed, 25 insertions(+), 9 deletions(-)
> >
> > I see two problems here.  First, this is a generic issue (any
> > legacy-style U-Boot image that we load should be verified).  Second, we
> > need to make this behavior configurable as as-is this overflows one
> > board (omapl138_lcdk) and I expect would be problematic for many more
> > boards when we make it done more commonly.
> 
> This patch fix a no-working uboot feature and this was the address problem on
> the specific case. We can call ->verify image every ->load, anyway can you
> explain better why you need a configurable behavior?

It fixes the legitimate case of having read in bad data and the
controller didn't return up an error to us, and it wasn't in the header,
yes.  And it needs to be configurable as adding in these checks
increases the binary size and various targets will fail to build, such
as omapl138_lcdk does with your patch as-is.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180720/23086c62/attachment.sig>

[U-Boot] [U-Boot,V3,1/2] spl: Fix redundant image of uboot

[Michael Nazzareno Trimarchi]

Hi

On Fri, Jul 20, 2018 at 10:11 PM, Tom Rini <trini@konsulko.com> wrote:
> On Fri, Jul 20, 2018 at 10:09:00PM +0200, Michael Nazzareno Trimarchi wrote:
>> Hi Tom
>>
>> On Fri, Jul 20, 2018 at 9:54 PM, Tom Rini <trini@konsulko.com> wrote:
>> > On Fri, Jul 06, 2018 at 05:09:22PM +0200, Michael Trimarchi wrote:
>> >
>> >> We need to address the redundat image case and undestand if the
>> >> image is corrupted or not. In error case we need to try the fallback copy.
>> >> The function used before was always return 0 without any evaluation of the
>> >> error. We try to make it work properly
>> >>
>> >> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>> >> ---
>> >> Changes V2->V3:
>> >>         Fix patch mistake due the a wrong edit of it
>> >> Changes V1->V2:
>> >>         Address the comments on using the err variable
>> >> ---
>> >>  common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
>> >>  1 file changed, 25 insertions(+), 9 deletions(-)
>> >
>> > I see two problems here.  First, this is a generic issue (any
>> > legacy-style U-Boot image that we load should be verified).  Second, we
>> > need to make this behavior configurable as as-is this overflows one
>> > board (omapl138_lcdk) and I expect would be problematic for many more
>> > boards when we make it done more commonly.
>>
>> This patch fix a no-working uboot feature and this was the address problem on
>> the specific case. We can call ->verify image every ->load, anyway can you
>> explain better why you need a configurable behavior?
>
> It fixes the legitimate case of having read in bad data and the
> controller didn't return up an error to us, and it wasn't in the header,
> yes.  And it needs to be configurable as adding in these checks
> increases the binary size and various targets will fail to build, such
> as omapl138_lcdk does with your patch as-is.  Thanks!
>

+#if defined(CONFIG_VERIFY_IMAGE)
+int verify_image(spl_image)
+{
+...
+}
+#else
+int verify_image(spl_image) { return 0; }
+#endif
+
 static int spl_load_image(struct spl_image_info *spl_image,
                          struct spl_image_loader *loader)
 {
        struct spl_boot_device bootdev;
+       int ret;

        bootdev.boot_device = loader->boot_device;
        bootdev.boot_device_name = NULL;

-       return loader->load_image(spl_image, &bootdev);
+       ret = loader->load_image(spl_image, &bootdev);
+       if (ret)
+               return -EINVAL;
+
+       return verify_image(spl_image);
 }

Somenthing like this?

or return loader->verify_image()

Michael

> --
> Tom



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[U-Boot] [U-Boot,V3,1/2] spl: Fix redundant image of uboot

[Tom Rini]

On Fri, Jul 20, 2018 at 10:27:26PM +0200, Michael Nazzareno Trimarchi wrote:

> Hi
> 
> On Fri, Jul 20, 2018 at 10:11 PM, Tom Rini <trini@konsulko.com> wrote:
> > On Fri, Jul 20, 2018 at 10:09:00PM +0200, Michael Nazzareno Trimarchi wrote:
> >> Hi Tom
> >>
> >> On Fri, Jul 20, 2018 at 9:54 PM, Tom Rini <trini@konsulko.com> wrote:
> >> > On Fri, Jul 06, 2018 at 05:09:22PM +0200, Michael Trimarchi wrote:
> >> >
> >> >> We need to address the redundat image case and undestand if the
> >> >> image is corrupted or not. In error case we need to try the fallback copy.
> >> >> The function used before was always return 0 without any evaluation of the
> >> >> error. We try to make it work properly
> >> >>
> >> >> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> >> >> ---
> >> >> Changes V2->V3:
> >> >>         Fix patch mistake due the a wrong edit of it
> >> >> Changes V1->V2:
> >> >>         Address the comments on using the err variable
> >> >> ---
> >> >>  common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
> >> >>  1 file changed, 25 insertions(+), 9 deletions(-)
> >> >
> >> > I see two problems here.  First, this is a generic issue (any
> >> > legacy-style U-Boot image that we load should be verified).  Second, we
> >> > need to make this behavior configurable as as-is this overflows one
> >> > board (omapl138_lcdk) and I expect would be problematic for many more
> >> > boards when we make it done more commonly.
> >>
> >> This patch fix a no-working uboot feature and this was the address problem on
> >> the specific case. We can call ->verify image every ->load, anyway can you
> >> explain better why you need a configurable behavior?
> >
> > It fixes the legitimate case of having read in bad data and the
> > controller didn't return up an error to us, and it wasn't in the header,
> > yes.  And it needs to be configurable as adding in these checks
> > increases the binary size and various targets will fail to build, such
> > as omapl138_lcdk does with your patch as-is.  Thanks!
> >
> 
> +#if defined(CONFIG_VERIFY_IMAGE)

CONFIG_SPL_VERIFY_IMAGE, for namespace.

> +int verify_image(spl_image)
> +{
> +...
> +}
> +#else
> +int verify_image(spl_image) { return 0; }
> +#endif
> +
>  static int spl_load_image(struct spl_image_info *spl_image,
>                           struct spl_image_loader *loader)
>  {
>         struct spl_boot_device bootdev;
> +       int ret;
> 
>         bootdev.boot_device = loader->boot_device;
>         bootdev.boot_device_name = NULL;
> 
> -       return loader->load_image(spl_image, &bootdev);
> +       ret = loader->load_image(spl_image, &bootdev);
> +       if (ret)
> +               return -EINVAL;
> +
> +       return verify_image(spl_image);
>  }
> 
> Somenthing like this?
> 
> or return loader->verify_image()

Yes, something like that.  And please pass it through travis-ci for the
world build and seeing if anything fails to fit into size constraints,
thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180720/083718d0/attachment.sig>