[PATCH] env: increment redund flag on read fail

* [PATCH] env: increment redund flag on read fail
@ 2020-12-17 23:19 Brandon Maier
  2021-02-01 20:54 ` Tom Rini
  2021-04-18 12:45 ` Tom Rini
  0 siblings, 2 replies; 3+ messages in thread
From: Brandon Maier @ 2020-12-17 23:19 UTC (permalink / raw)
  To: u-boot

If one of the reads fails when importing redundant environments (a
single read failure), the env_flags wouldn't get initialized in
env_import_redund(). If a user then calls saveenv, the new environment
will have the wrong flags value. So on the next load the new environment
will be ignored.

While debugging this, I also noticed that env/sf.c was not correctly
handling a single read failure, as it would not check the crc before
assigning it to gd->env_addr.

Having a special error path for when there is a single read failure
seems unnecessary and may lead to future bugs. Instead collapse the
'single read failure' error to be the same as a 'single crc failure'.
That way env_check_redund() either passes or fails, and if it passes we
are guaranteed to have checked the CRC.

Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com>
CC: Joe Hershberger <joe.hershberger@ni.com>
CC: Wolfgang Denk <wd@denx.de>
CC: Heiko Schocher <hs@denx.de>
---
 env/common.c  | 27 ++++++++-------------------
 env/sf.c      |  2 +-
 include/env.h |  2 --
 3 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/env/common.c b/env/common.c
index 6c32a9b479..ca22885fa4 100644
--- a/env/common.c
+++ b/env/common.c
@@ -144,7 +144,7 @@ static unsigned char env_flags;
 int env_check_redund(const char *buf1, int buf1_read_fail,
 		     const char *buf2, int buf2_read_fail)
 {
-	int crc1_ok, crc2_ok;
+	int crc1_ok = 0, crc2_ok = 0;
 	env_t *tmp_env1, *tmp_env2;
 
 	tmp_env1 = (env_t *)buf1;
@@ -152,25 +152,18 @@ int env_check_redund(const char *buf1, int buf1_read_fail,
 
 	if (buf1_read_fail && buf2_read_fail) {
 		puts("*** Error - No Valid Environment Area found\n");
+		return -EIO;
 	} 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) {
-		return -EIO;
-	} else if (!buf1_read_fail && buf2_read_fail) {
-		gd->env_valid = ENV_VALID;
-		return -EINVAL;
-	} else if (buf1_read_fail && !buf2_read_fail) {
-		gd->env_valid = ENV_REDUND;
-		return -ENOENT;
-	}
-
-	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) ==
-			tmp_env1->crc;
-	crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) ==
-			tmp_env2->crc;
+	if (!buf1_read_fail)
+		crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) ==
+				tmp_env1->crc;
+	if (!buf2_read_fail)
+		crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) ==
+				tmp_env2->crc;
 
 	if (!crc1_ok && !crc2_ok) {
 		return -ENOMSG; /* needed for env_load() */
@@ -207,10 +200,6 @@ int env_import_redund(const char *buf1, int buf1_read_fail,
 	if (ret == -EIO) {
 		env_set_default("bad env area", 0);
 		return -EIO;
-	} else if (ret == -EINVAL) {
-		return env_import((char *)buf1, 1, flags);
-	} else if (ret == -ENOENT) {
-		return env_import((char *)buf2, 1, flags);
 	} else if (ret == -ENOMSG) {
 		env_set_default("bad CRC", 0);
 		return -ENOMSG;
diff --git a/env/sf.c b/env/sf.c
index 91ed2860ed..1102b42804 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -350,7 +350,7 @@ static int env_sf_init_early(void)
 		ret = env_check_redund((char *)tmp_env1, read1_fail,
 				       (char *)tmp_env2, read2_fail);
 
-		if (ret == -EIO || ret == -ENOMSG)
+		if (ret < 0)
 			goto err_read;
 
 		if (gd->env_valid == ENV_VALID)
diff --git a/include/env.h b/include/env.h
index c15339a93f..b5731e4b9a 100644
--- a/include/env.h
+++ b/include/env.h
@@ -328,8 +328,6 @@ int env_export(struct environment_s *env_out);
  * @buf2_read_fail: 0 if buf2 is valid, non-zero if invalid
  * @return 0 if OK,
  *	-EIO if no environment is valid,
- *	-EINVAL if read of second entry is good
- *	-ENOENT if read of first entry is good
  *	-ENOMSG if the CRC was bad
  */
 
-- 
2.29.1

^ permalink raw reply related	[flat|nested] 3+ messages in thread