spl: Fix redundant image of uboot

Change-Id I1c2f71e75fc052c54fc94b13d0942cb9e75ff1c6

Signed-off-by Michael Trimarchi <[email protected]>

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 <[email protected]>

---
 common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

common/spl/spl_nand.c +6 -9

index 9a52500..b96fce2 100644
@@ -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,

common/spl/spl_nand.c +5 -0

+		debug("Image checksum failed\n");
+		return -EINVAL;
+	}
+
+	return 0;
 }

static int spl_nand_load_image(struct spl_image_info *spl_image,

Hi,

Quoted 42 lines · 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 <[email protected]>

---
 common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

common/spl/spl_nand.c +6 -9

index 9a52500..b96fce2 100644
@@ -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,

common/spl/spl_nand.c static int spl_nand_load_element(struct spl_image_info *spl_image, · line 75

Quoted 10 lines

common/spl/spl_nand.c +9 -0

+                        (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(...) {

common/spl/spl_nand.c static int spl_nand_load_element(struct spl_image_info *spl_image, · line 86

Quoted 6 lines

common/spl/spl_nand.c +5 -0

+             debug("Header checksum failed\n");
+             return -EINVAL;
+     }
+     err = image_check_dcrc((const image_header_t *)spl_image->load_addr);
+     if (!err) {
 

dto.

Quoted 9 lines

common/spl/spl_nand.c +5 -0

+             debug("Image checksum failed\n");
+             return -EINVAL;
+     }
+
+     return 0;
 }

static int spl_nand_load_image(struct spl_image_info *spl_image,

Lothar Waßmann

Hi

Quoted 63 lines · Lothar Waßmann

Hi,

Quoted 42 lines · 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 <[email protected]>

---
 common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

common/spl/spl_nand.c +6 -9

index 9a52500..b96fce2 100644
@@ -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,

common/spl/spl_nand.c static int spl_nand_load_element(struct spl_image_info *spl_image, · line 75

Quoted 10 lines

common/spl/spl_nand.c +9 -0

+                        (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(...) {

Quoted 6 lines

common/spl/spl_nand.c +5 -0

+             debug("Header checksum failed\n");
+             return -EINVAL;
+     }
+     err = image_check_dcrc((const image_header_t *)spl_image->load_addr);
+     if (!err) {
 

Quoted 1 line

dto.

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

Quoted 66 lines · Michael Nazzareno Trimarchi

Hi

Quoted 63 lines · Lothar Waßmann

Hi,

Quoted 42 lines · 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 <[email protected]>

---
 common/spl/spl_nand.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

common/spl/spl_nand.c +6 -9

index 9a52500..b96fce2 100644
@@ -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,

common/spl/spl_nand.c static int spl_nand_load_element(struct spl_image_info *spl_image, · line 75

Quoted 10 lines

common/spl/spl_nand.c +9 -0

+                        (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(...) {

Quoted 6 lines

common/spl/spl_nand.c +5 -0

+             debug("Header checksum failed\n");
+             return -EINVAL;
+     }
+     err = image_check_dcrc((const image_header_t *)spl_image->load_addr);
+     if (!err) {
 

Quoted 4 lines

Quoted 1 line

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
___________________________________________________________

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 <[email protected]>

---
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(-)

common/spl/spl_nand.c +25 -10

index 9a52500..f00246d 100644
@@ -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               |

Hi Lothar

On Fri, Jul 6, 2018 at 5:09 PM, Michael Trimarchi
<[email protected]> wrote:

Quoted 6 lines

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 <[email protected]>

Any more comment?

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 <[email protected]>

---
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(-)

Quoted 6 lines · 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 <[email protected]>

Quoted 8 lines

---
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

Hi Tom

Quoted 17 lines · Tom Rini

Quoted 6 lines · 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 <[email protected]>

Quoted 8 lines

---
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(-)

Quoted 5 lines

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?

Quoted 26 lines · Michael Nazzareno Trimarchi

Hi Tom

Quoted 17 lines · Tom Rini

Quoted 6 lines · 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 <[email protected]>

Quoted 8 lines

---
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(-)

Quoted 5 lines

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.

Quoted 3 lines

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

Hi

Quoted 31 lines · Tom Rini

Quoted 29 lines · Michael Nazzareno Trimarchi

Hi Tom

Quoted 17 lines · Tom Rini

Quoted 6 lines · 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 <[email protected]>

Quoted 8 lines

---
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(-)

Quoted 5 lines

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?

Quoted 6 lines

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)

(quoted diff) +14 -1

+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()