[U-Boot] [PATCH 1/2] spl: Fix redundant image of uboot
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,
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();
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!
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
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 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
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@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 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,
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,
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 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
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.
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
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!
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
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!
participants (4)
-
Lothar Waßmann -
Michael Nazzareno Trimarchi -
Michael Trimarchi
-
Tom Rini