[U-Boot] [PATCH v2 0/4] env: cleanups after adding multiple envs

[U-Boot] [PATCH v2 0/4] env: cleanups after adding multiple envs

[Simon Goldschmidt]

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

-- 
2.11.0

[U-Boot] [PATCH v2 1/4] env: make env_import(_redund) return 0 on success, not 1

[Simon Goldschmidt]

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

[U-Boot] [PATCH v2 1/4] env: make env_import(_redund) return 0 on success, not 1

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180131/e9128503/attachment.sig>

[U-Boot] [U-Boot, v2, 1/4] env: make env_import(_redund) return 0 on success, not 1

[Tom Rini]

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!

-- 
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/20180201/25ca419f/attachment.sig>

[U-Boot] [PATCH v2 2/4] env: move more common code to env_import_redund

[Simon Goldschmidt]

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
 
 /**
-- 
2.11.0

[U-Boot] [PATCH v2 2/4] env: move more common code to env_import_redund

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180131/27fc76c3/attachment.sig>

[U-Boot] [U-Boot, v2, 2/4] env: move more common code to env_import_redund

[Tom Rini]

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!

-- 
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/20180201/a29f0688/attachment.sig>

[U-Boot] [PATCH v2 3/4] env: make env drivers propagate env_import return value

[Simon Goldschmidt]

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 */
 
-- 
2.11.0

[U-Boot] [PATCH v2 3/4] env: make env drivers propagate env_import return value

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180131/31e87be6/attachment.sig>

[U-Boot] [U-Boot, v2, 3/4] env: make env drivers propagate env_import return value

[Tom Rini]

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!

-- 
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/20180201/ec7ab8be/attachment.sig>

[U-Boot] [PATCH v2 4/4] env: sf: use env_import_redund to simplify env_sf_load

[Simon Goldschmidt]

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

[U-Boot] [PATCH v2 4/4] env: sf: use env_import_redund to simplify env_sf_load

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180131/35468f86/attachment.sig>

[U-Boot] [U-Boot, v2, 4/4] env: sf: use env_import_redund to simplify env_sf_load

[Tom Rini]

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!

-- 
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/20180201/dcb67fd2/attachment.sig>