[U-Boot] [PATCH v2 0/4] env: cleanups after adding multiple envs
With the new code to support multiple environment drivers and select an environment at runtime, to correctly implement fallback when one environment fails to load (e.g. crc error), the return value of env_import has to be propagated by all env driver's load function.
While cleaning this up, made some other cleanups, mainly to reduce duplicated code.
Changes from v1: - fixed duplicated commit message for patch 3/4 - removed german company email footer
Simon Goldschmidt (4): env: make env_import(_redund) return 0 on success, not 1 env: move more common code to env_import_redund env: make env drivers propagate env_import return value env: sf: use env_import_redund to simplify env_sf_load
env/common.c | 29 ++++++++++++++++++---- env/eeprom.c | 4 +-- env/ext4.c | 3 +-- env/fat.c | 3 +-- env/flash.c | 4 +-- env/mmc.c | 26 +++---------------- env/nand.c | 24 +++--------------- env/nvram.c | 4 +-- env/onenand.c | 4 +-- env/remote.c | 2 +- env/sata.c | 4 +-- env/sf.c | 69 +++++++-------------------------------------------- env/ubi.c | 22 ++++++++-------- include/environment.h | 3 ++- 14 files changed, 61 insertions(+), 140 deletions(-)
env_import (and env_import_redund) currently return 1 on success and 0 on error. However, they are only used from functions returning 0 on success or a negative value on error.
Let's clean this up by making env_import and env_import_redund return 0 on success and -EIO on error (as was the case for all users before).
Users that cared for the return value are also updated. Funny enough, this only affects onenand.c and sf.c
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com --- env/common.c | 8 ++++---- env/onenand.c | 4 ++-- env/sf.c | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/env/common.c b/env/common.c index c633502d68..363ba6fead 100644 --- a/env/common.c +++ b/env/common.c @@ -118,21 +118,21 @@ int env_import(const char *buf, int check)
if (crc32(0, ep->data, ENV_SIZE) != crc) { set_default_env("!bad CRC"); - return 0; + return -EIO; } }
if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0, 0, NULL)) { gd->flags |= GD_FLG_ENV_READY; - return 1; + return 0; }
pr_err("Cannot import environment: errno = %d\n", errno);
set_default_env("!import failed");
- return 0; + return -EIO; }
#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT @@ -153,7 +153,7 @@ int env_import_redund(const char *buf1, const char *buf2)
if (!crc1_ok && !crc2_ok) { set_default_env("!bad CRC"); - return 0; + return -EIO; } else if (crc1_ok && !crc2_ok) { gd->env_valid = ENV_VALID; } else if (!crc1_ok && crc2_ok) { diff --git a/env/onenand.c b/env/onenand.c index 2e3045c5f5..10a8cccbe8 100644 --- a/env/onenand.c +++ b/env/onenand.c @@ -57,10 +57,10 @@ static int env_onenand_load(void) #endif /* !ENV_IS_EMBEDDED */
rc = env_import(buf, 1); - if (rc) + if (!rc) gd->env_valid = ENV_VALID;
- return rc ? 0 : -EIO; + return rc; }
static int env_onenand_save(void) diff --git a/env/sf.c b/env/sf.c index a2e4c93631..3dc54410df 100644 --- a/env/sf.c +++ b/env/sf.c @@ -236,7 +236,7 @@ static int env_sf_load(void) ep = tmp_env2;
ret = env_import((char *)ep, 0); - if (!ret) { + if (ret) { pr_err("Cannot import environment: errno = %d\n", errno); set_default_env("!env_import failed"); } @@ -336,7 +336,7 @@ static int env_sf_load(void) }
ret = env_import(buf, 1); - if (ret) + if (!ret) gd->env_valid = ENV_VALID;
err_read:
On Wed, Jan 31, 2018 at 02:47:10PM +0100, Simon Goldschmidt wrote:
env_import (and env_import_redund) currently return 1 on success and 0 on error. However, they are only used from functions returning 0 on success or a negative value on error.
Let's clean this up by making env_import and env_import_redund return 0 on success and -EIO on error (as was the case for all users before).
Users that cared for the return value are also updated. Funny enough, this only affects onenand.c and sf.c
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com
Acked-by: Maxime Ripard maxime.ripard@free-electrons.com
Thanks! Maxime
On Wed, Jan 31, 2018 at 02:47:10PM +0100, Simon Goldschmidt wrote:
env_import (and env_import_redund) currently return 1 on success and 0 on error. However, they are only used from functions returning 0 on success or a negative value on error.
Let's clean this up by making env_import and env_import_redund return 0 on success and -EIO on error (as was the case for all users before).
Users that cared for the return value are also updated. Funny enough, this only affects onenand.c and sf.c
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com Acked-by: Maxime Ripard maxime.ripard@free-electrons.com
Applied to u-boot/master, thanks!
There is more common code in mmc, nand and ubi env drivers that can be shared by moving to env_import_redund.
For this, a status/error value whether the buffers were loaded are passed as additional parameters to env_import_redund. Ideally, these are already returned to the env driver by the storage driver. This is the case for mmc, nand and ubi, so for this change, code deduplicated.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com --- env/common.c | 21 ++++++++++++++++++++- env/mmc.c | 23 ++--------------------- env/nand.c | 22 +++------------------- env/ubi.c | 18 +++++++++--------- include/environment.h | 3 ++- 5 files changed, 36 insertions(+), 51 deletions(-)
diff --git a/env/common.c b/env/common.c index 363ba6fead..f21ff70096 100644 --- a/env/common.c +++ b/env/common.c @@ -138,7 +138,8 @@ int env_import(const char *buf, int check) #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT static unsigned char env_flags;
-int env_import_redund(const char *buf1, const char *buf2) +int env_import_redund(const char *buf1, int buf1_read_fail, + const char *buf2, int buf2_read_fail) { int crc1_ok, crc2_ok; env_t *ep, *tmp_env1, *tmp_env2; @@ -146,6 +147,24 @@ int env_import_redund(const char *buf1, const char *buf2) tmp_env1 = (env_t *)buf1; tmp_env2 = (env_t *)buf2;
+ if (buf1_read_fail && buf2_read_fail) { + puts("*** Error - No Valid Environment Area found\n"); + } else if (buf1_read_fail || buf2_read_fail) { + puts("*** Warning - some problems detected "); + puts("reading environment; recovered successfully\n"); + } + + if (buf1_read_fail && buf2_read_fail) { + set_default_env("!bad env area"); + return -EIO; + } else if (!buf1_read_fail && buf2_read_fail) { + gd->env_valid = ENV_VALID; + return env_import((char *)tmp_env1, 1); + } else if (buf1_read_fail && !buf2_read_fail) { + gd->env_valid = ENV_REDUND; + return env_import((char *)tmp_env2, 1); + } + crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc; crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == diff --git a/env/mmc.c b/env/mmc.c index 528fbf9781..8847fdc7e2 100644 --- a/env/mmc.c +++ b/env/mmc.c @@ -290,27 +290,8 @@ static int env_mmc_load(void) read1_fail = read_env(mmc, CONFIG_ENV_SIZE, offset1, tmp_env1); read2_fail = read_env(mmc, CONFIG_ENV_SIZE, offset2, tmp_env2);
- if (read1_fail && read2_fail) - puts("*** Error - No Valid Environment Area found\n"); - else if (read1_fail || read2_fail) - puts("*** Warning - some problems detected " - "reading environment; recovered successfully\n"); - - if (read1_fail && read2_fail) { - errmsg = "!bad CRC"; - ret = -EIO; - goto fini; - } else if (!read1_fail && read2_fail) { - gd->env_valid = ENV_VALID; - env_import((char *)tmp_env1, 1); - } else if (read1_fail && !read2_fail) { - gd->env_valid = ENV_REDUND; - env_import((char *)tmp_env2, 1); - } else { - env_import_redund((char *)tmp_env1, (char *)tmp_env2); - } - - ret = 0; + ret = env_import_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2, + read2_fail);
fini: fini_mmc_for_env(mmc); diff --git a/env/nand.c b/env/nand.c index 8058b55c50..3e8df39c26 100644 --- a/env/nand.c +++ b/env/nand.c @@ -320,7 +320,7 @@ static int env_nand_load(void) #if defined(ENV_IS_EMBEDDED) return 0; #else - int read1_fail = 0, read2_fail = 0; + int read1_fail, read2_fail; env_t *tmp_env1, *tmp_env2; int ret = 0;
@@ -336,24 +336,8 @@ static int env_nand_load(void) read1_fail = readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1); read2_fail = readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *) tmp_env2);
- if (read1_fail && read2_fail) - puts("*** Error - No Valid Environment Area found\n"); - else if (read1_fail || read2_fail) - puts("*** Warning - some problems detected " - "reading environment; recovered successfully\n"); - - if (read1_fail && read2_fail) { - set_default_env("!bad env area"); - goto done; - } else if (!read1_fail && read2_fail) { - gd->env_valid = ENV_VALID; - env_import((char *)tmp_env1, 1); - } else if (read1_fail && !read2_fail) { - gd->env_valid = ENV_REDUND; - env_import((char *)tmp_env2, 1); - } else { - env_import_redund((char *)tmp_env1, (char *)tmp_env2); - } + ret = env_import_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2, + read2_fail);
done: free(tmp_env1); diff --git a/env/ubi.c b/env/ubi.c index 1c4653d4f6..c222ebc784 100644 --- a/env/ubi.c +++ b/env/ubi.c @@ -95,6 +95,7 @@ static int env_ubi_load(void) { ALLOC_CACHE_ALIGN_BUFFER(char, env1_buf, CONFIG_ENV_SIZE); ALLOC_CACHE_ALIGN_BUFFER(char, env2_buf, CONFIG_ENV_SIZE); + int read1_fail, read2_fail; env_t *tmp_env1, *tmp_env2;
/* @@ -118,21 +119,20 @@ static int env_ubi_load(void) return -EIO; }
- if (ubi_volume_read(CONFIG_ENV_UBI_VOLUME, (void *)tmp_env1, - CONFIG_ENV_SIZE)) { + read1_fail = ubi_volume_read(CONFIG_ENV_UBI_VOLUME, (void *)tmp_env1, + CONFIG_ENV_SIZE)); + if (read1_fail) printf("\n** Unable to read env from %s:%s **\n", CONFIG_ENV_UBI_PART, CONFIG_ENV_UBI_VOLUME); - }
- if (ubi_volume_read(CONFIG_ENV_UBI_VOLUME_REDUND, (void *)tmp_env2, - CONFIG_ENV_SIZE)) { + read2_fail = ubi_volume_read(CONFIG_ENV_UBI_VOLUME_REDUND, + (void *)tmp_env2, CONFIG_ENV_SIZE)); + if (read2_fail) printf("\n** Unable to read redundant env from %s:%s **\n", CONFIG_ENV_UBI_PART, CONFIG_ENV_UBI_VOLUME_REDUND); - }
- env_import_redund((char *)tmp_env1, (char *)tmp_env2); - - return 0; + return env_import_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2, + read2_fail); } #else /* ! CONFIG_SYS_REDUNDAND_ENVIRONMENT */ static int env_ubi_load(void) diff --git a/include/environment.h b/include/environment.h index a4060506fa..6044b9e1b4 100644 --- a/include/environment.h +++ b/include/environment.h @@ -297,7 +297,8 @@ int env_export(env_t *env_out);
#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT /* Select and import one of two redundant environments */ -int env_import_redund(const char *buf1, const char *buf2); +int env_import_redund(const char *buf1, int buf1_status, + const char *buf2, int buf2_status); #endif
/**
On Wed, Jan 31, 2018 at 02:47:11PM +0100, Simon Goldschmidt wrote:
There is more common code in mmc, nand and ubi env drivers that can be shared by moving to env_import_redund.
For this, a status/error value whether the buffers were loaded are passed as additional parameters to env_import_redund. Ideally, these are already returned to the env driver by the storage driver. This is the case for mmc, nand and ubi, so for this change, code deduplicated.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com
Acked-by: Maxime Ripard maxime.ripard@free-electrons.com
Thanks! Maxime
On Wed, Jan 31, 2018 at 02:47:11PM +0100, Simon Goldschmidt wrote:
There is more common code in mmc, nand and ubi env drivers that can be shared by moving to env_import_redund.
For this, a status/error value whether the buffers were loaded are passed as additional parameters to env_import_redund. Ideally, these are already returned to the env driver by the storage driver. This is the case for mmc, nand and ubi, so for this change, code deduplicated.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com Acked-by: Maxime Ripard maxime.ripard@free-electrons.com
Applied to u-boot/master, thanks!
For multiple env drivers to correctly implement fallback when one environment fails to load (e.g. crc error), the return value of env_import has to be propagated by all env driver's load function.
Without this change, the first driver that succeeds to load an environment with an invalid CRC return 0 (success) and no other drivers are checked.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com --- env/eeprom.c | 4 +--- env/ext4.c | 3 +-- env/fat.c | 3 +-- env/flash.c | 4 +--- env/mmc.c | 3 +-- env/nand.c | 2 +- env/nvram.c | 4 +--- env/remote.c | 2 +- env/sata.c | 4 +--- env/ubi.c | 4 +--- 10 files changed, 10 insertions(+), 23 deletions(-)
diff --git a/env/eeprom.c b/env/eeprom.c index 584379ebd2..55d19d9d99 100644 --- a/env/eeprom.c +++ b/env/eeprom.c @@ -181,9 +181,7 @@ static int env_eeprom_load(void) eeprom_bus_read(CONFIG_SYS_DEF_EEPROM_ADDR, off, (uchar *)buf_env, CONFIG_ENV_SIZE);
- env_import(buf_env, 1); - - return 0; + return env_import(buf_env, 1); }
static int env_eeprom_save(void) diff --git a/env/ext4.c b/env/ext4.c index 9cdf28e79f..3f3aac5737 100644 --- a/env/ext4.c +++ b/env/ext4.c @@ -114,8 +114,7 @@ static int env_ext4_load(void) goto err_env_relocate; }
- env_import(buf, 1); - return 0; + return env_import(buf, 1);
err_env_relocate: set_default_env(NULL); diff --git a/env/fat.c b/env/fat.c index 158a9a3435..35f7ab5c6d 100644 --- a/env/fat.c +++ b/env/fat.c @@ -117,8 +117,7 @@ static int env_fat_load(void) goto err_env_relocate; }
- env_import(buf, 1); - return 0; + return env_import(buf, 1);
err_env_relocate: set_default_env(NULL); diff --git a/env/flash.c b/env/flash.c index bac10ff985..ccade77ce3 100644 --- a/env/flash.c +++ b/env/flash.c @@ -351,9 +351,7 @@ static int env_flash_load(void) "reading environment; recovered successfully\n\n"); #endif /* CONFIG_ENV_ADDR_REDUND */
- env_import((char *)flash_addr, 1); - - return 0; + return env_import((char *)flash_addr, 1); } #endif /* LOADENV */
diff --git a/env/mmc.c b/env/mmc.c index 8847fdc7e2..1058b8c512 100644 --- a/env/mmc.c +++ b/env/mmc.c @@ -332,8 +332,7 @@ static int env_mmc_load(void) goto fini; }
- env_import(buf, 1); - ret = 0; + ret = env_import(buf, 1);
fini: fini_mmc_for_env(mmc); diff --git a/env/nand.c b/env/nand.c index 3e8df39c26..904f1c40d6 100644 --- a/env/nand.c +++ b/env/nand.c @@ -378,7 +378,7 @@ static int env_nand_load(void) return -EIO; }
- env_import(buf, 1); + return env_import(buf, 1); #endif /* ! ENV_IS_EMBEDDED */
return 0; diff --git a/env/nvram.c b/env/nvram.c index c8b34754ef..6f76fe4b8d 100644 --- a/env/nvram.c +++ b/env/nvram.c @@ -60,9 +60,7 @@ static int env_nvram_load(void) #else memcpy(buf, (void *)CONFIG_ENV_ADDR, CONFIG_ENV_SIZE); #endif - env_import(buf, 1); - - return 0; + return env_import(buf, 1); }
static int env_nvram_save(void) diff --git a/env/remote.c b/env/remote.c index c013fdd4b0..379d0eb1bb 100644 --- a/env/remote.c +++ b/env/remote.c @@ -49,7 +49,7 @@ static int env_remote_save(void) static int env_remote_load(void) { #ifndef ENV_IS_EMBEDDED - env_import((char *)env_ptr, 1); + return env_import((char *)env_ptr, 1); #endif
return 0; diff --git a/env/sata.c b/env/sata.c index a77029774e..4bfe0119df 100644 --- a/env/sata.c +++ b/env/sata.c @@ -113,9 +113,7 @@ static void env_sata_load(void) return -EIO; }
- env_import(buf, 1); - - return 0; + return env_import(buf, 1); }
U_BOOT_ENV_LOCATION(sata) = { diff --git a/env/ubi.c b/env/ubi.c index c222ebc784..d15d5b09fa 100644 --- a/env/ubi.c +++ b/env/ubi.c @@ -163,9 +163,7 @@ static int env_ubi_load(void) return -EIO; }
- env_import(buf, 1); - - return 0; + return env_import(buf, 1); } #endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */
On Wed, Jan 31, 2018 at 02:47:12PM +0100, Simon Goldschmidt wrote:
For multiple env drivers to correctly implement fallback when one environment fails to load (e.g. crc error), the return value of env_import has to be propagated by all env driver's load function.
Without this change, the first driver that succeeds to load an environment with an invalid CRC return 0 (success) and no other drivers are checked.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com
Acked-by: Maxime Ripard maxime.ripard@free-electrons.com
Thanks! maxime
On Wed, Jan 31, 2018 at 02:47:12PM +0100, Simon Goldschmidt wrote:
For multiple env drivers to correctly implement fallback when one environment fails to load (e.g. crc error), the return value of env_import has to be propagated by all env driver's load function.
Without this change, the first driver that succeeds to load an environment with an invalid CRC return 0 (success) and no other drivers are checked.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com Acked-by: Maxime Ripard maxime.ripard@free-electrons.com
Applied to u-boot/master, thanks!
For the redundant environment configuration, env_sf_load still contained duplicate code instead of using env_import_redund().
Simplify the code by only executing the load twice and delegating everything else to env_import_redund.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com --- env/sf.c | 67 ++++++++-------------------------------------------------------- 1 file changed, 8 insertions(+), 59 deletions(-)
diff --git a/env/sf.c b/env/sf.c index 3dc54410df..6326b37e46 100644 --- a/env/sf.c +++ b/env/sf.c @@ -166,10 +166,8 @@ static int env_sf_save(void) static int env_sf_load(void) { int ret; - int crc1_ok = 0, crc2_ok = 0; - env_t *tmp_env1 = NULL; - env_t *tmp_env2 = NULL; - env_t *ep = NULL; + int read1_fail, read2_fail; + env_t *tmp_env1, *tmp_env2;
tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN, CONFIG_ENV_SIZE); @@ -185,63 +183,14 @@ static int env_sf_load(void) if (ret) goto out;
- ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, - CONFIG_ENV_SIZE, tmp_env1); - if (ret) { - set_default_env("!spi_flash_read() failed"); - goto err_read; - } - - if (crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc) - crc1_ok = 1; + read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, + CONFIG_ENV_SIZE, tmp_env1); + read2_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET_REDUND, + CONFIG_ENV_SIZE, tmp_env2);
- ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET_REDUND, - CONFIG_ENV_SIZE, tmp_env2); - if (!ret) { - if (crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc) - crc2_ok = 1; - } + ret = env_import_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2, + read2_fail);
- if (!crc1_ok && !crc2_ok) { - set_default_env("!bad CRC"); - ret = -EIO; - goto err_read; - } else if (crc1_ok && !crc2_ok) { - gd->env_valid = ENV_VALID; - } else if (!crc1_ok && crc2_ok) { - gd->env_valid = ENV_REDUND; - } else if (tmp_env1->flags == ACTIVE_FLAG && - tmp_env2->flags == OBSOLETE_FLAG) { - gd->env_valid = ENV_VALID; - } else if (tmp_env1->flags == OBSOLETE_FLAG && - tmp_env2->flags == ACTIVE_FLAG) { - gd->env_valid = ENV_REDUND; - } else if (tmp_env1->flags == tmp_env2->flags) { - gd->env_valid = ENV_VALID; - } else if (tmp_env1->flags == 0xFF) { - gd->env_valid = ENV_VALID; - } else if (tmp_env2->flags == 0xFF) { - gd->env_valid = ENV_REDUND; - } else { - /* - * this differs from code in env_flash.c, but I think a sane - * default path is desirable. - */ - gd->env_valid = ENV_VALID; - } - - if (gd->env_valid == ENV_VALID) - ep = tmp_env1; - else - ep = tmp_env2; - - ret = env_import((char *)ep, 0); - if (ret) { - pr_err("Cannot import environment: errno = %d\n", errno); - set_default_env("!env_import failed"); - } - -err_read: spi_flash_free(env_flash); env_flash = NULL; out:
On Wed, Jan 31, 2018 at 02:47:13PM +0100, Simon Goldschmidt wrote:
For the redundant environment configuration, env_sf_load still contained duplicate code instead of using env_import_redund().
Simplify the code by only executing the load twice and delegating everything else to env_import_redund.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com
Acked-by: Maxime Ripard maxime.ripard@free-electrons.com
Thanks! Maxime
On Wed, Jan 31, 2018 at 02:47:13PM +0100, Simon Goldschmidt wrote:
For the redundant environment configuration, env_sf_load still contained duplicate code instead of using env_import_redund().
Simplify the code by only executing the load twice and delegating everything else to env_import_redund.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com Acked-by: Maxime Ripard maxime.ripard@free-electrons.com
Applied to u-boot/master, thanks!
participants (3)
-
Maxime Ripard -
Simon Goldschmidt -
Tom Rini