[U-Boot] [PATCH 00/11] Introduce variables whitelisting in environment

[U-Boot] [PATCH 00/11] Introduce variables whitelisting in environment

[Quentin Schulz]

This patch series is based on this[1] patch series from Maxime.

This is an RFC. It's been only tested in a specific use case on a custom
i.MX6 board. It's known to break compilation on a few boards.

I have a use case where we want some variables from a first environment to
be overriden by variables from a second environment. For example, we want
to load variables from the default env (ENV_IS_NOWHERE) and then load only
a handful of other variables from, e.g., NAND.

In our use case, we basically can be sure that the default env in the
U-Boot binary is secure but we want only a few variables to be modified,
thus keeping control over the overall behaviour of U-Boot in secure mode.

It works in that way:
  - from highest to lowest priority, the first environment that can be
  loaded (that has successfully init and whose load function has returned
  no errors) will be the main environment,
  - then, all the following environment that could be successfully loaded
  (same conditions as the main environment) are secondary environment. The
  env variables that are defined both in CONFIG_ENV_VAR_WHITELIST_LIST and
  in the secondary environments override the ones in the main environment,
  - for saving, we save the whole environment to all environments
  available, be they main or secondary (it does not matter to save the
  whole environment on secondary environments as only the whitelisted
  variables will be overriden in the loading process,

I have also a few questions that could help me to get the whole thing to
work.

1) I can't really get my head around the use of gd->env_addr, what is it used
for? It is set in a bunch of different places but only once is it
explicitly used (basically to alternate the env_addr between the one
associated to main and redundant environment (in NAND for example)).

2) Why do we consider ENV_IS_NOWHERE an invalid environment? The only place I
found a use for it was to just say that if the environment is invalid, we
should set to default environment (in env_relocate in env/common.c). With
my patch series I guess that we could remove this fallback and force
ENV_IS_NOWHERE to be always there.

3) There are a few (20) boards that set gd->env_addr and gd->env_valid in
their board file. What is the reason to do such a thing? Isn't those
overriden anyway by the environment driver?

I'm looking forward to getting your feedback on this patch series.

Thanks,
Quentin

[1] https://patchwork.ozlabs.org/cover/842057/

Quentin Schulz (11):
  env: fix ret not being set and fails when no env could have been init
  lib: hashtable: support whitelisting env variables
  env: add support for whitelisting variables from secondary environments
  env: make nowhere an env medium like the others
  cmd: saveenv: enable the saveenv command when ENV_IS_NOWHERE is defined but another env medium is enabled too
  env: add env_driver to env_driver functions' arguments
  env: gd flags without ENV_READY is enough to discriminate in env_get_default
  env: add env_driver parameter to env_import_redund
  env: make env_locations a global variable
  env: introducing env_info struct for storing info per env
  env: store information about each environment in gd

 board/sunxi/board.c               |   2 +-
 cmd/nvedit.c                      |  16 ++-
 common/board_r.c                  |   8 +-
 env/Kconfig                       |  29 +++---
 env/common.c                      |  45 ++++++----
 env/eeprom.c                      |  40 ++++-----
 env/env.c                         | 142 +++++++++++++++++++++++++------
 env/ext4.c                        |   4 +-
 env/fat.c                         |   4 +-
 env/flash.c                       |  58 ++++++-------
 env/mmc.c                         |  14 +--
 env/nand.c                        |  46 +++++-----
 env/nowhere.c                     |  12 ++-
 env/nvram.c                       |  18 ++--
 env/onenand.c                     |   6 +-
 env/remote.c                      |  10 +-
 env/sata.c                        |   4 +-
 env/sf.c                          |  34 +++----
 env/ubi.c                         |  14 +--
 include/asm-generic/global_data.h |   5 +-
 include/environment.h             |  59 ++++++++-----
 include/search.h                  |   2 +-
 lib/hashtable.c                   |  17 +++-
 23 files changed, 379 insertions(+), 210 deletions(-)

base-commit: 5d41e28058e7b378c9fa5c61ecc074a682ba2db4
-- 
git-series 0.9.1

[U-Boot] [PATCH 01/11] env: fix ret not being set and fails when no env could have been init

[Quentin Schulz]

This patch is fixing things that most likely will be done by Maxime in
the v2 of his patch series for multiple environments support, one way or
the other.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 env/env.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/env/env.c b/env/env.c
index 9b8b38c..5e70ddf 100644
--- a/env/env.c
+++ b/env/env.c
@@ -182,24 +182,25 @@ int env_init(void)
 	struct env_driver *drv;
 	int ret = -ENOENT;
 	int prio;
+	bool init = false;
 
 	for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) {
-		if (!drv->init || !drv->init())
+		if (drv->init)
+			ret = drv->init(drv);
+		else
+			ret = 0;
+
+		if (!ret) {
 			gd->env_has_init |= BIT(drv->location);
+			init = true;
+		}
 
 		debug("%s: Environment %s init done (ret=%d)\n", __func__,
 		      drv->name, ret);
 	}
 
-	if (!prio)
+	if (!prio || !init)
 		return -ENODEV;
 
-	if (ret == -ENOENT) {
-		gd->env_addr = (ulong)&default_environment[0];
-		gd->env_valid = ENV_VALID;
-
-		return 0;
-	}
-
-	return ret;
+	return 0;
 }
-- 
git-series 0.9.1

[U-Boot] [PATCH 02/11] lib: hashtable: support whitelisting env variables

[Quentin Schulz]

When passing the vars array to himport_r, variables in the array that
are not in the loaded environment are removed from the current
environment.

Of course, this isn't suitable for whitelisting some variables. Let's
introduce a whitelisting boolean that will not remove array variables
that are not in the loaded environment from the current environment.

The remaining of the code will rightfully override any variable that
already exists.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 board/sunxi/board.c |  2 +-
 cmd/nvedit.c        |  2 +-
 env/common.c        |  6 +++---
 include/search.h    |  2 +-
 lib/hashtable.c     | 17 ++++++++++++++++-
 5 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 8891961..aac4904 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -647,7 +647,7 @@ static void parse_spl_header(const uint32_t spl_addr)
 		 * import -t" the string(s) at fel_script_address right away.
 		 */
 		himport_r(&env_htab, (char *)(uintptr_t)spl->fel_script_address,
-			  spl->fel_uEnv_length, '\n', H_NOCLEAR, 0, 0, NULL);
+			  spl->fel_uEnv_length, '\n', H_NOCLEAR, 0, 0, NULL, 0);
 		return;
 	}
 	/* otherwise assume .scr format (mkimage-type script) */
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index a690d74..c00e1da 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -1079,7 +1079,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 	}
 
 	if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,
-			crlf_is_lf, 0, NULL) == 0) {
+			crlf_is_lf, 0, NULL, false) == 0) {
 		pr_err("Environment import failed: errno = %d\n", errno);
 		return 1;
 	}
diff --git a/env/common.c b/env/common.c
index fb66432..9d97541 100644
--- a/env/common.c
+++ b/env/common.c
@@ -83,7 +83,7 @@ void set_default_env(const char *s)
 
 	if (himport_r(&env_htab, (char *)default_environment,
 			sizeof(default_environment), '\0', flags, 0,
-			0, NULL) == 0)
+			0, NULL, false) == 0)
 		pr_err("Environment import failed: errno = %d\n", errno);
 
 	gd->flags |= GD_FLG_ENV_READY;
@@ -100,7 +100,7 @@ int set_default_vars(int nvars, char * const vars[])
 	 */
 	return himport_r(&env_htab, (const char *)default_environment,
 				sizeof(default_environment), '\0',
-				H_NOCLEAR | H_INTERACTIVE, 0, nvars, vars);
+				H_NOCLEAR | H_INTERACTIVE, 0, nvars, vars, 0);
 }
 
 #ifdef CONFIG_ENV_AES
@@ -178,7 +178,7 @@ int env_import(const char *buf, int check)
 	}
 
 	if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0,
-			0, NULL)) {
+			0, NULL, false)) {
 		gd->flags |= GD_FLG_ENV_READY;
 		return 1;
 	}
diff --git a/include/search.h b/include/search.h
index df5d61c..ed2d9bf 100644
--- a/include/search.h
+++ b/include/search.h
@@ -101,7 +101,7 @@ extern ssize_t hexport_r(struct hsearch_data *__htab,
 extern int himport_r(struct hsearch_data *__htab,
 		     const char *__env, size_t __size, const char __sep,
 		     int __flag, int __crlf_is_lf, int nvars,
-		     char * const vars[]);
+		     char * const vars[], bool whitelisting);
 
 /* Walk the whole table calling the callback on each element */
 extern int hwalk_r(struct hsearch_data *__htab, int (*callback)(ENTRY *));
diff --git a/lib/hashtable.c b/lib/hashtable.c
index f088477..432fe14 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -772,11 +772,17 @@ static int drop_var_from_set(const char *name, int nvars, char * vars[])
  *
  * In theory, arbitrary separator characters can be used, but only
  * '\0' and '\n' have really been tested.
+ *
+ * "whitelisting" makes the function to not remove variables from the
+ * env that were not found in "vars".
+ * if "whitelisting" is false, the function remove variables from env
+ * that were not found in "vars".
  */
 
 int himport_r(struct hsearch_data *htab,
 		const char *env, size_t size, const char sep, int flag,
-		int crlf_is_lf, int nvars, char * const vars[])
+		int crlf_is_lf, int nvars, char * const vars[],
+		bool whitelisting)
 {
 	char *data, *sp, *dp, *name, *value;
 	char *localvars[nvars];
@@ -935,6 +941,14 @@ int himport_r(struct hsearch_data *htab,
 	free(data);
 
 	/* process variables which were not considered */
+	/*
+	 * If we are importing variables from a second env and checking they're
+	 * whitelisted, we don't want to delete the variables in current env
+	 * because it was not in the whitelist.
+	 */
+	if (whitelisting)
+		goto out;
+
 	for (i = 0; i < nvars; i++) {
 		if (localvars[i] == NULL)
 			continue;
@@ -952,6 +966,7 @@ int himport_r(struct hsearch_data *htab,
 			printf("WARNING: '%s' not in imported env, deleting it!\n", localvars[i]);
 	}
 
+out:
 	debug("INSERT: done\n");
 	return 1;		/* everything OK */
 }
-- 
git-series 0.9.1

[U-Boot] [PATCH 03/11] env: add support for whitelisting variables from secondary environments

[Quentin Schulz]

This patch makes it possible to have a first environment that will be
the base environment, secondary environment that will be pre-loaded and
have its variables passing through the whitelist "mask" to override
variables in the base environment.

The base environment will be the environment with the highest priority
(0 is highest) that can load and the next loadable environment will be
the secondary environment (with a lowest priority than the base
environment of course).

The whitelist "mask" is built with the content of space-separated list
of environment variables in ENV_VAR_WHITELIST_LIST in Kconfig.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 env/Kconfig           |  17 +++++++-
 env/common.c          |   7 +++-
 env/env.c             | 100 +++++++++++++++++++++++++++++++++++++++----
 include/environment.h |   6 +++-
 4 files changed, 123 insertions(+), 7 deletions(-)

diff --git a/env/Kconfig b/env/Kconfig
index 1952463..d57594d 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -438,4 +438,21 @@ config ENV_UBI_VOLUME
 
 endif
 
+config ENV_VAR_WHITELIST
+	bool "Enable overriding some variables from secondary environments"
+	help
+	  This allows overriding some variables from secondary environments.
+	  After the first valid environment is loaded, a secondary environment
+	  is pre-loaded and its variables that are present in the whitelist will
+	  be added to the current environment or will override existing
+	  variables.
+
+config ENV_VAR_WHITELIST_LIST
+	depends on ENV_VAR_WHITELIST
+	string "Whitelist of variables from secondary environments"
+	help
+	  This defines the variables that are allowed to be overriden by
+	  ones from secondary environments.
+	  This is a list of environment variables that are space-separated.
+
 endmenu
diff --git a/env/common.c b/env/common.c
index 9d97541..00c454d 100644
--- a/env/common.c
+++ b/env/common.c
@@ -177,8 +177,15 @@ int env_import(const char *buf, int check)
 		return ret;
 	}
 
+#ifdef CONFIG_ENV_VAR_WHITELIST
+	if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0',
+		      whitelisting ? H_NOCLEAR : 0, 0,
+		      whitelisting ? whitelist_nvars : 0,
+		      whitelisting ? whitelist : NULL, whitelisting)) {
+#else
 	if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0,
 			0, NULL, false)) {
+#endif
 		gd->flags |= GD_FLG_ENV_READY;
 		return 1;
 	}
diff --git a/env/env.c b/env/env.c
index 5e70ddf..43a62b8 100644
--- a/env/env.c
+++ b/env/env.c
@@ -7,9 +7,18 @@
 
 #include <common.h>
 #include <environment.h>
+#ifdef CONFIG_ENV_VAR_WHITELIST
+# include <malloc.h>
+#endif
 
 DECLARE_GLOBAL_DATA_PTR;
 
+#ifdef CONFIG_ENV_VAR_WHITELIST
+char *const *whitelist;
+bool whitelisting;
+unsigned int whitelist_nvars;
+#endif
+
 static struct env_driver *_env_driver_lookup(enum env_location loc)
 {
 	struct env_driver *drv;
@@ -62,6 +71,31 @@ static enum env_location env_locations[] = {
 
 static enum env_location env_load_location;
 
+#ifdef CONFIG_ENV_VAR_WHITELIST
+void env_create_whitelist(void)
+{
+	char **wl_vars;
+	char *wl = strchr(CONFIG_ENV_VAR_WHITELIST_LIST, ' ');
+	unsigned int nvars = 0;
+
+	while(wl) {
+		nvars++;
+		wl = strchr(wl + 1, ' ');
+	}
+
+	whitelist_nvars = nvars + 1;
+
+	wl_vars = malloc(sizeof(char *) * (nvars + 1));
+
+	wl_vars[nvars] = strtok(CONFIG_ENV_VAR_WHITELIST_LIST, " ");
+	while (nvars) {
+		wl_vars[--nvars] = strtok(NULL, " ");
+	}
+
+	whitelist = wl_vars;
+}
+#endif
+
 __weak enum env_location env_get_location(enum env_operation op, int prio)
 {
 	switch (op) {
@@ -74,7 +108,11 @@ __weak enum env_location env_get_location(enum env_operation op, int prio)
 		return env_load_location = env_locations[prio];
 
 	case ENVO_SAVE:
+#ifdef CONFIG_ENV_VAR_WHITELIST
+		return env_locations[prio];
+#else
 		return env_load_location;
+#endif
 	}
 
 	return ENVL_UNKNOWN;
@@ -130,10 +168,12 @@ int env_load(void)
 {
 	struct env_driver *drv;
 	int prio;
+	int ret = 1;
+#ifdef CONFIG_ENV_VAR_WHITELIST
+	bool found = false;
+#endif
 
 	for (prio = 0; (drv = env_driver_lookup(ENVO_LOAD, prio)); prio++) {
-		int ret;
-
 		if (!drv->load)
 			continue;
 
@@ -144,16 +184,52 @@ int env_load(void)
 		ret = drv->load();
 		printf("%s\n", ret ? "Failed" : "OK");
 		if (!ret)
-			return 0;
+			break;
 	}
 
-	return -ENODEV;
+	if (ret)
+		return -ENODEV;
+
+#ifdef CONFIG_ENV_VAR_WHITELIST
+	env_create_whitelist();
+
+	whitelisting = true;
+	/* When whitelisting, env from prio x+n will override env from prio x */
+	for (prio++; prio < ARRAY_SIZE(env_locations); prio++) {
+		drv = env_driver_lookup(ENVO_LOAD, prio);
+		if (!drv)
+			continue;
+
+		if (!drv->load)
+			continue;
+
+		printf("Overriding env variables with ones from %s env...",
+		      __func__, drv->name);
+		ret = drv->load();
+		printf("%s\n", ret ? "Failed" : "OK");
+		if (!ret) {
+			found = true;
+			continue;
+		}
+	}
+
+out:
+	if (!found)
+		debug("%s: Couldn't find other valid env for whitelisting\n",
+		      __func__);
+
+	whitelisting = false;
+#endif
+	return 0;
 }
 
 int env_save(void)
 {
 	struct env_driver *drv;
 	int prio;
+#ifdef CONFIG_ENV_VAR_WHITELIST
+	bool saved = false;
+#endif
 
 	for (prio = 0; (drv = env_driver_lookup(ENVO_SAVE, prio)); prio++) {
 		int ret;
@@ -167,13 +243,23 @@ int env_save(void)
 		printf("Saving Environment to %s... ", drv->name);
 		ret = drv->save();
 		printf("%s\n", ret ? "Failed" : "OK");
+#ifdef CONFIG_ENV_VAR_WHITELIST
+		/* When whitelisting, we want to save to all media available */
+		if (!ret) {
+			saved = true;
+			continue;
+		}
+#else
 		if (!ret)
 			return 0;
-
-		debug("%s: Environment %s failed to save (err=%d)\n", __func__,
-		      drv->name, ret);
+#endif
 	}
 
+#ifdef CONFIG_ENV_VAR_WHITELIST
+	if (saved)
+		return 0;
+#endif
+
 	return -ENODEV;
 }
 
diff --git a/include/environment.h b/include/environment.h
index 7fa8b98..33e47ba 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -267,6 +267,12 @@ struct env_driver {
 	int (*init)(void);
 };
 
+#ifdef CONFIG_ENV_VAR_WHITELIST
+extern char *const *whitelist;
+extern unsigned int whitelist_nvars;
+extern bool whitelisting;
+#endif
+
 /* Declare a new environment location driver */
 #define U_BOOT_ENV_LOCATION(__name)					\
 	ll_entry_declare(struct env_driver, __name, env_driver)
-- 
git-series 0.9.1

[U-Boot] [PATCH 04/11] env: make nowhere an env medium like the others

[Quentin Schulz]

Since we now allow the loading of different environment media by
priority, we can mimic the current fallback system with nowhere medium
by adding a load function to the nowhere driver (and make it the medium
with the lowest priority).

Nowhere then becomes a valid environment medium.

This also makes it possible to either enforce the value of some
variables by using nowhere medium when using it as secondary environment
or to completely enforce all the environment variables except a few from
a secondary environment when using it as base environment.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 env/Kconfig   | 12 ++----------
 env/common.c  |  2 +-
 env/nowhere.c |  8 +++++++-
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/env/Kconfig b/env/Kconfig
index d57594d..f99a701 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -2,16 +2,6 @@ menu "Environment"
 
 config ENV_IS_NOWHERE
 	bool "Environment is not stored"
-	depends on !ENV_IS_IN_EEPROM
-	depends on !ENV_IS_IN_FAT
-	depends on !ENV_IS_IN_FLASH
-	depends on !ENV_IS_IN_MMC
-	depends on !ENV_IS_IN_NAND
-	depends on !ENV_IS_IN_NVRAM
-	depends on !ENV_IS_IN_ONENAND
-	depends on !ENV_IS_IN_REMOTE
-	depends on !ENV_IS_IN_SPI_FLASH
-	depends on !ENV_IS_IN_UBI
 	default y
 	help
 	  Define this if you don't want to or can't have an environment stored
@@ -19,6 +9,8 @@ config ENV_IS_NOWHERE
 	  while U-Boot is running, but once U-Boot exits it will not be
 	  stored. U-Boot will therefore always start up with a default
 	  environment.
+	  When whitelisting is enabled, define this to be able to use the
+	  default environment as either base or secondary environment.
 
 config ENV_IS_IN_EEPROM
 	bool "Environment in EEPROM"
diff --git a/env/common.c b/env/common.c
index 00c454d..c8d8993 100644
--- a/env/common.c
+++ b/env/common.c
@@ -279,7 +279,7 @@ void env_relocate(void)
 	env_htab.change_ok += gd->reloc_off;
 #endif
 	if (gd->env_valid == ENV_INVALID) {
-#if defined(CONFIG_ENV_IS_NOWHERE) || defined(CONFIG_SPL_BUILD)
+#if defined(CONFIG_SPL_BUILD)
 		/* Environment not changable */
 		set_default_env(NULL);
 #else
diff --git a/env/nowhere.c b/env/nowhere.c
index f654883..7a37909 100644
--- a/env/nowhere.c
+++ b/env/nowhere.c
@@ -15,6 +15,11 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+static int env_nowhere_load(void)
+{
+	return !env_import((char *)default_environment, 0);
+}
+
 /*
  * Because we only ever have the default environment available we must mark
  * it as invalid.
@@ -22,7 +27,7 @@ DECLARE_GLOBAL_DATA_PTR;
 static int env_nowhere_init(void)
 {
 	gd->env_addr	= (ulong)&default_environment[0];
-	gd->env_valid	= ENV_INVALID;
+	gd->env_valid	= ENV_VALID;
 
 	return 0;
 }
@@ -31,4 +36,5 @@ U_BOOT_ENV_LOCATION(nowhere) = {
 	.location	= ENVL_NOWHERE,
 	.init		= env_nowhere_init,
 	ENV_NAME("nowhere")
+	.load		= env_nowhere_load,
 };
-- 
git-series 0.9.1

[U-Boot] [PATCH 05/11] cmd: saveenv: enable the saveenv command when ENV_IS_NOWHERE is defined

[Quentin Schulz]

but another env medium is enabled too

Now that it is possible to have multiple environments at the same time,
nowhere included, enable the saveenv command when nowhere medium is
enabled but accompanied by another medium on which the saveenv command
is possible.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 cmd/nvedit.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index c00e1da..84a7004 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -704,7 +704,19 @@ ulong env_get_ulong(const char *name, int base, ulong default_val)
 }
 
 #ifndef CONFIG_SPL_BUILD
-#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
+#if defined(CONFIG_CMD_SAVEENV) && (!defined(CONFIG_ENV_IS_NOWHERE) || (\
+	defined(CONFIG_ENV_IS_IN_EEPROM)	|| \
+	defined(CONFIG_ENV_IS_IN_FLASH)		|| \
+	defined(CONFIG_ENV_IS_IN_MMC)		|| \
+	defined(CONFIG_ENV_IS_IN_FAT)		|| \
+	defined(CONFIG_ENV_IS_IN_EXT4)		|| \
+	defined(CONFIG_ENV_IS_IN_NAND)		|| \
+	defined(CONFIG_ENV_IS_IN_NVRAM)		|| \
+	defined(CONFIG_ENV_IS_IN_ONENAND)	|| \
+	defined(CONFIG_ENV_IS_IN_SATA)		|| \
+	defined(CONFIG_ENV_IS_IN_SPI_FLASH)	|| \
+	defined(CONFIG_ENV_IS_IN_REMOTE)	|| \
+	defined(CONFIG_ENV_IS_IN_UBI)))
 static int do_env_save(cmd_tbl_t *cmdtp, int flag, int argc,
 		       char * const argv[])
 {
-- 
git-series 0.9.1

[U-Boot] [PATCH 06/11] env: add env_driver to env_driver functions' arguments

[Quentin Schulz]

Might be interesting to get some infos about the driver (e.g. its
location) when inside one of its functions.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 env/eeprom.c          |  6 +++---
 env/env.c             |  8 ++++----
 env/ext4.c            |  4 ++--
 env/fat.c             |  4 ++--
 env/flash.c           | 10 +++++-----
 env/mmc.c             |  6 +++---
 env/nand.c            |  8 ++++----
 env/nowhere.c         |  4 ++--
 env/nvram.c           |  8 ++++----
 env/onenand.c         |  4 ++--
 env/remote.c          |  6 +++---
 env/sata.c            |  4 ++--
 env/sf.c              |  8 ++++----
 env/ubi.c             |  8 ++++----
 include/environment.h | 14 ++++++++++----
 15 files changed, 54 insertions(+), 48 deletions(-)

diff --git a/env/eeprom.c b/env/eeprom.c
index 584379e..b0ffce4 100644
--- a/env/eeprom.c
+++ b/env/eeprom.c
@@ -61,7 +61,7 @@ static int eeprom_bus_write(unsigned dev_addr, unsigned offset,
 	return rcode;
 }
 
-static int env_eeprom_get_char(int index)
+static int env_eeprom_get_char(struct env_driver *drv, int index)
 {
 	uchar c;
 	unsigned int off = CONFIG_ENV_OFFSET;
@@ -76,7 +76,7 @@ static int env_eeprom_get_char(int index)
 	return c;
 }
 
-static int env_eeprom_load(void)
+static int env_eeprom_load(struct env_driver *drv)
 {
 	char buf_env[CONFIG_ENV_SIZE];
 	unsigned int off = CONFIG_ENV_OFFSET;
@@ -186,7 +186,7 @@ static int env_eeprom_load(void)
 	return 0;
 }
 
-static int env_eeprom_save(void)
+static int env_eeprom_save(struct env_driver *drv)
 {
 	env_t	env_new;
 	int	rc;
diff --git a/env/env.c b/env/env.c
index 43a62b8..7c98083 100644
--- a/env/env.c
+++ b/env/env.c
@@ -153,7 +153,7 @@ int env_get_char(int index)
 		if (!(gd->env_has_init & BIT(drv->location)))
 			continue;
 
-		ret = drv->get_char(index);
+		ret = drv->get_char(drv, index);
 		if (!ret)
 			return 0;
 
@@ -181,7 +181,7 @@ int env_load(void)
 			continue;
 
 		printf("Loading Environment from %s... ", drv->name);
-		ret = drv->load();
+		ret = drv->load(drv);
 		printf("%s\n", ret ? "Failed" : "OK");
 		if (!ret)
 			break;
@@ -205,7 +205,7 @@ int env_load(void)
 
 		printf("Overriding env variables with ones from %s env...",
 		      __func__, drv->name);
-		ret = drv->load();
+		ret = drv->load(drv);
 		printf("%s\n", ret ? "Failed" : "OK");
 		if (!ret) {
 			found = true;
@@ -241,7 +241,7 @@ int env_save(void)
 			continue;
 
 		printf("Saving Environment to %s... ", drv->name);
-		ret = drv->save();
+		ret = drv->save(drv);
 		printf("%s\n", ret ? "Failed" : "OK");
 #ifdef CONFIG_ENV_VAR_WHITELIST
 		/* When whitelisting, we want to save to all media available */
diff --git a/env/ext4.c b/env/ext4.c
index 6520221..77c0389 100644
--- a/env/ext4.c
+++ b/env/ext4.c
@@ -34,7 +34,7 @@
 DECLARE_GLOBAL_DATA_PTR;
 
 #ifdef CONFIG_CMD_SAVEENV
-static int env_ext4_save(void)
+static int env_ext4_save(struct env_driver *drv)
 {
 	env_t	env_new;
 	struct blk_desc *dev_desc = NULL;
@@ -75,7 +75,7 @@ static int env_ext4_save(void)
 }
 #endif /* CONFIG_CMD_SAVEENV */
 
-static int env_ext4_load(void)
+static int env_ext4_load(struct env_driver *drv)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
 	struct blk_desc *dev_desc = NULL;
diff --git a/env/fat.c b/env/fat.c
index 51c4ced..05b5971 100644
--- a/env/fat.c
+++ b/env/fat.c
@@ -34,7 +34,7 @@
 DECLARE_GLOBAL_DATA_PTR;
 
 #ifdef CMD_SAVEENV
-static int env_fat_save(void)
+static int env_fat_save(struct env_driver *drv)
 {
 	env_t	env_new;
 	struct blk_desc *dev_desc = NULL;
@@ -73,7 +73,7 @@ static int env_fat_save(void)
 #endif /* CMD_SAVEENV */
 
 #ifdef LOADENV
-static int env_fat_load(void)
+static int env_fat_load(struct env_driver *drv)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
 	struct blk_desc *dev_desc = NULL;
diff --git a/env/flash.c b/env/flash.c
index bac10ff..45e58b4 100644
--- a/env/flash.c
+++ b/env/flash.c
@@ -71,7 +71,7 @@ static ulong __maybe_unused end_addr_new =
 
 #ifdef CONFIG_ENV_ADDR_REDUND
 #ifdef INITENV
-static int env_flash_init(void)
+static int env_flash_init(struct env_driver *drv)
 {
 	int crc1_ok = 0, crc2_ok = 0;
 
@@ -117,7 +117,7 @@ static int env_flash_init(void)
 #endif
 
 #ifdef CMD_SAVEENV
-static int env_flash_save(void)
+static int env_flash_save(struct env_driver *drv)
 {
 	env_t	env_new;
 	char	*saved_data = NULL;
@@ -222,7 +222,7 @@ done:
 #else /* ! CONFIG_ENV_ADDR_REDUND */
 
 #ifdef INITENV
-static int env_flash_init(void)
+static int env_flash_init(struct env_driver *drv)
 {
 	if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
 		gd->env_addr	= (ulong)&(env_ptr->data);
@@ -237,7 +237,7 @@ static int env_flash_init(void)
 #endif
 
 #ifdef CMD_SAVEENV
-static int env_flash_save(void)
+static int env_flash_save(struct env_driver *drv)
 {
 	env_t	env_new;
 	int	rc = 1;
@@ -308,7 +308,7 @@ done:
 #endif /* CONFIG_ENV_ADDR_REDUND */
 
 #ifdef LOADENV
-static int env_flash_load(void)
+static int env_flash_load(struct env_driver *drv)
 {
 #ifdef CONFIG_ENV_ADDR_REDUND
 	if (gd->env_addr != (ulong)&(flash_addr->data)) {
diff --git a/env/mmc.c b/env/mmc.c
index 885e000..64d1404 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -143,7 +143,7 @@ static inline int write_env(struct mmc *mmc, unsigned long size,
 	return (n == blk_cnt) ? 0 : -1;
 }
 
-static int env_mmc_save(void)
+static int env_mmc_save(struct env_driver *drv)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
 	int dev = mmc_get_env_dev();
@@ -206,7 +206,7 @@ static inline int read_env(struct mmc *mmc, unsigned long size,
 }
 
 #ifdef CONFIG_ENV_OFFSET_REDUND
-static int env_mmc_load(void)
+static int env_mmc_load(struct env_driver *drv)
 {
 #if !defined(ENV_IS_EMBEDDED)
 	struct mmc *mmc;
@@ -268,7 +268,7 @@ err:
 	return ret;
 }
 #else /* ! CONFIG_ENV_OFFSET_REDUND */
-static int env_mmc_load(void)
+static int env_mmc_load(struct env_driver *drv)
 {
 #if !defined(ENV_IS_EMBEDDED)
 	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
diff --git a/env/nand.c b/env/nand.c
index 8058b55..6ed3c26 100644
--- a/env/nand.c
+++ b/env/nand.c
@@ -62,7 +62,7 @@ DECLARE_GLOBAL_DATA_PTR;
  * This way the SPL loads not only the U-Boot image from NAND but
  * also the environment.
  */
-static int env_nand_init(void)
+static int env_nand_init(struct env_driver *drv)
 {
 #if defined(ENV_IS_EMBEDDED) || defined(CONFIG_NAND_ENV_DST)
 	int crc1_ok = 0, crc2_ok = 0;
@@ -183,7 +183,7 @@ static int erase_and_write_env(const struct nand_env_location *location,
 	return ret;
 }
 
-static int env_nand_save(void)
+static int env_nand_save(struct env_driver *drv)
 {
 	int	ret = 0;
 	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
@@ -315,7 +315,7 @@ int get_nand_env_oob(struct mtd_info *mtd, unsigned long *result)
 #endif
 
 #ifdef CONFIG_ENV_OFFSET_REDUND
-static int env_nand_load(void)
+static int env_nand_load(struct env_driver *drv)
 {
 #if defined(ENV_IS_EMBEDDED)
 	return 0;
@@ -368,7 +368,7 @@ done:
  * device i.e., nand_dev_desc + 0. This is also the behaviour using
  * the new NAND code.
  */
-static int env_nand_load(void)
+static int env_nand_load(struct env_driver *drv)
 {
 #if !defined(ENV_IS_EMBEDDED)
 	int ret;
diff --git a/env/nowhere.c b/env/nowhere.c
index 7a37909..fb6ea9e 100644
--- a/env/nowhere.c
+++ b/env/nowhere.c
@@ -15,7 +15,7 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-static int env_nowhere_load(void)
+static int env_nowhere_load(struct env_driver *drv)
 {
 	return !env_import((char *)default_environment, 0);
 }
@@ -24,7 +24,7 @@ static int env_nowhere_load(void)
  * Because we only ever have the default environment available we must mark
  * it as invalid.
  */
-static int env_nowhere_init(void)
+static int env_nowhere_init(struct env_driver *drv)
 {
 	gd->env_addr	= (ulong)&default_environment[0];
 	gd->env_valid	= ENV_VALID;
diff --git a/env/nvram.c b/env/nvram.c
index c8b3475..aad341d 100644
--- a/env/nvram.c
+++ b/env/nvram.c
@@ -41,7 +41,7 @@ env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR;
 #endif
 
 #ifdef CONFIG_SYS_NVRAM_ACCESS_ROUTINE
-static int env_nvram_get_char(int index)
+static int env_nvram_get_char(struct env_driver *drv, int index)
 {
 	uchar c;
 
@@ -51,7 +51,7 @@ static int env_nvram_get_char(int index)
 }
 #endif
 
-static int env_nvram_load(void)
+static int env_nvram_load(struct env_driver *drv)
 {
 	char buf[CONFIG_ENV_SIZE];
 
@@ -65,7 +65,7 @@ static int env_nvram_load(void)
 	return 0;
 }
 
-static int env_nvram_save(void)
+static int env_nvram_save(struct env_driver *drv)
 {
 	env_t	env_new;
 	int	rcode = 0;
@@ -88,7 +88,7 @@ static int env_nvram_save(void)
  *
  * We are still running from ROM, so data use is limited
  */
-static int env_nvram_init(void)
+static int env_nvram_init(struct env_driver *drv)
 {
 #if defined(CONFIG_SYS_NVRAM_ACCESS_ROUTINE)
 	ulong crc;
diff --git a/env/onenand.c b/env/onenand.c
index 2e3045c..e633ebe 100644
--- a/env/onenand.c
+++ b/env/onenand.c
@@ -26,7 +26,7 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-static int env_onenand_load(void)
+static int env_onenand_load(struct env_driver *drv)
 {
 	struct mtd_info *mtd = &onenand_mtd;
 #ifdef CONFIG_ENV_ADDR_FLEX
@@ -63,7 +63,7 @@ static int env_onenand_load(void)
 	return rc ? 0 : -EIO;
 }
 
-static int env_onenand_save(void)
+static int env_onenand_save(struct env_driver *drv)
 {
 	env_t	env_new;
 	int ret;
diff --git a/env/remote.c b/env/remote.c
index c013fdd..b004964 100644
--- a/env/remote.c
+++ b/env/remote.c
@@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_ENV_OFFSET 0
 #endif
 
-static int env_remote_init(void)
+static int env_remote_init(struct env_driver *drv)
 {
 	if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
 		gd->env_addr = (ulong)&(env_ptr->data);
@@ -35,7 +35,7 @@ static int env_remote_init(void)
 }
 
 #ifdef CONFIG_CMD_SAVEENV
-static int env_remote_save(void)
+static int env_remote_save(struct env_driver *drv)
 {
 #ifdef CONFIG_SRIO_PCIE_BOOT_SLAVE
 	printf("Can not support the 'saveenv' when boot from SRIO or PCIE!\n");
@@ -46,7 +46,7 @@ static int env_remote_save(void)
 }
 #endif /* CONFIG_CMD_SAVEENV */
 
-static int env_remote_load(void)
+static int env_remote_load(struct env_driver *drv)
 {
 #ifndef ENV_IS_EMBEDDED
 	env_import((char *)env_ptr, 1);
diff --git a/env/sata.c b/env/sata.c
index a770297..93289be 100644
--- a/env/sata.c
+++ b/env/sata.c
@@ -45,7 +45,7 @@ static inline int write_env(struct blk_desc *sata, unsigned long size,
 	return (n == blk_cnt) ? 0 : -1;
 }
 
-static int env_sata_save(void)
+static int env_sata_save(struct env_driver *drv)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
 	struct blk_desc *sata = NULL;
@@ -91,7 +91,7 @@ static inline int read_env(struct blk_desc *sata, unsigned long size,
 	return (n == blk_cnt) ? 0 : -1;
 }
 
-static void env_sata_load(void)
+static void env_sata_load(struct env_driver *drv)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
 	struct blk_desc *sata = NULL;
diff --git a/env/sf.c b/env/sf.c
index e51b1ae..e811cde 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -82,7 +82,7 @@ static int setup_flash_device(void)
 
 #if defined(CONFIG_ENV_OFFSET_REDUND)
 #ifdef CMD_SAVEENV
-static int env_sf_save(void)
+static int env_sf_save(struct env_driver *drv)
 {
 	env_t	env_new;
 	char	*saved_buffer = NULL, flag = OBSOLETE_FLAG;
@@ -162,7 +162,7 @@ static int env_sf_save(void)
 }
 #endif /* CMD_SAVEENV */
 
-static int env_sf_load(void)
+static int env_sf_load(struct env_driver *drv)
 {
 	int ret;
 	int crc1_ok = 0, crc2_ok = 0;
@@ -251,7 +251,7 @@ out:
 }
 #else
 #ifdef CMD_SAVEENV
-static int env_sf_save(void)
+static int env_sf_save(struct env_driver *drv)
 {
 	u32	saved_size, saved_offset, sector;
 	char	*saved_buffer = NULL;
@@ -312,7 +312,7 @@ static int env_sf_save(void)
 }
 #endif /* CMD_SAVEENV */
 
-static int env_sf_load(void)
+static int env_sf_load(struct env_driver *drv)
 {
 	int ret;
 	char *buf = NULL;
diff --git a/env/ubi.c b/env/ubi.c
index 1c4653d..a649999 100644
--- a/env/ubi.c
+++ b/env/ubi.c
@@ -20,7 +20,7 @@ DECLARE_GLOBAL_DATA_PTR;
 
 #ifdef CONFIG_CMD_SAVEENV
 #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
-static int env_ubi_save(void)
+static int env_ubi_save(struct env_driver *drv)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
 	int ret;
@@ -62,7 +62,7 @@ static int env_ubi_save(void)
 	return 0;
 }
 #else /* ! CONFIG_SYS_REDUNDAND_ENVIRONMENT */
-static int env_ubi_save(void)
+static int env_ubi_save(struct env_driver *drv)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
 	int ret;
@@ -91,7 +91,7 @@ static int env_ubi_save(void)
 #endif /* CONFIG_CMD_SAVEENV */
 
 #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
-static int env_ubi_load(void)
+static int env_ubi_load(struct env_driver *drv)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(char, env1_buf, CONFIG_ENV_SIZE);
 	ALLOC_CACHE_ALIGN_BUFFER(char, env2_buf, CONFIG_ENV_SIZE);
@@ -135,7 +135,7 @@ static int env_ubi_load(void)
 	return 0;
 }
 #else /* ! CONFIG_SYS_REDUNDAND_ENVIRONMENT */
-static int env_ubi_load(void)
+static int env_ubi_load(struct env_driver *drv)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
 
diff --git a/include/environment.h b/include/environment.h
index 33e47ba..34a0d5e 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -222,6 +222,8 @@ enum env_operation {
 	ENVO_SAVE,
 };
 
+struct env_driver;
+
 struct env_driver {
 	const char *name;
 	enum env_location location;
@@ -232,10 +234,11 @@ struct env_driver {
 	 * This method is optional. If not provided, a default implementation
 	 * will read from gd->env_addr.
 	 *
+	 * @drv: the driver whose get_char function is about to be called
 	 * @index: Index of character to read (0=first)
 	 * @return character read, or -ve on error
 	 */
-	int (*get_char)(int index);
+	int (*get_char)(struct env_driver *drv, int index);
 
 	/**
 	 * load() - Load the environment from storage
@@ -243,28 +246,31 @@ struct env_driver {
 	 * This method is optional. If not provided, no environment will be
 	 * loaded.
 	 *
+	 * @drv: the driver whose load function is about to be called
 	 * @return 0 if OK, -ve on error
 	 */
-	int (*load)(void);
+	int (*load)(struct env_driver *drv);
 
 	/**
 	 * save() - Save the environment to storage
 	 *
 	 * This method is required for 'saveenv' to work.
 	 *
+	 * @drv: the driver whose save function is about to be called
 	 * @return 0 if OK, -ve on error
 	 */
-	int (*save)(void);
+	int (*save)(struct env_driver *drv);
 
 	/**
 	 * init() - Set up the initial pre-relocation environment
 	 *
 	 * This method is optional.
 	 *
+	 * @drv: the driver whose init function is about to be called
 	 * @return 0 if OK, -ENOENT if no initial environment could be found,
 	 * other -ve on error
 	 */
-	int (*init)(void);
+	int (*init)(struct env_driver *drv);
 };
 
 #ifdef CONFIG_ENV_VAR_WHITELIST
-- 
git-series 0.9.1

[U-Boot] [PATCH 07/11] env: gd flags without ENV_READY is enough to discriminate in

[Quentin Schulz]

env_get_default

To prepare the removal of env_valid in an upcoming patch, and since the
ENV_READY flag in gd is enough to discriminate in env_get_char, let's
update env_get_char to return the variable in the default environment
when ENV_READY flag is not set and remove env_valid setting and getting
from env_get_default.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 env/common.c | 3 ---
 env/env.c    | 2 +-
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/env/common.c b/env/common.c
index c8d8993..75b8334 100644
--- a/env/common.c
+++ b/env/common.c
@@ -47,14 +47,11 @@ int env_get_yesno(const char *var)
 char *env_get_default(const char *name)
 {
 	char *ret_val;
-	unsigned long really_valid = gd->env_valid;
 	unsigned long real_gd_flags = gd->flags;
 
 	/* Pretend that the image is bad. */
 	gd->flags &= ~GD_FLG_ENV_READY;
-	gd->env_valid = ENV_INVALID;
 	ret_val = env_get(name);
-	gd->env_valid = really_valid;
 	gd->flags = real_gd_flags;
 	return ret_val;
 }
diff --git a/env/env.c b/env/env.c
index 7c98083..2565e7a 100644
--- a/env/env.c
+++ b/env/env.c
@@ -141,7 +141,7 @@ int env_get_char(int index)
 	struct env_driver *drv;
 	int prio;
 
-	if (gd->env_valid == ENV_INVALID)
+	if (!(gd->flags & GD_FLG_ENV_READY))
 		return default_environment[index];
 
 	for (prio = 0; (drv = env_driver_lookup(ENVO_GET_CHAR, prio)); prio++) {
-- 
git-series 0.9.1

[U-Boot] [PATCH 08/11] env: add env_driver parameter to env_import_redund

[Quentin Schulz]

To prepare for an upcoming patch that uses the location of the driver
used when doing an env_import_redund, add env_driver as an argument of
env_import_redund so that it can be used within.

Update all calls to this function and the prototype.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 env/common.c          | 2 +-
 env/nand.c            | 2 +-
 env/ubi.c             | 2 +-
 include/environment.h | 3 ++-
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/env/common.c b/env/common.c
index 75b8334..24cf41b 100644
--- a/env/common.c
+++ b/env/common.c
@@ -197,7 +197,7 @@ 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(struct env_driver *drv, const char *buf1, const char *buf2)
 {
 	int crc1_ok, crc2_ok;
 	env_t *ep, *tmp_env1, *tmp_env2;
diff --git a/env/nand.c b/env/nand.c
index 6ed3c26..49e506e 100644
--- a/env/nand.c
+++ b/env/nand.c
@@ -352,7 +352,7 @@ static int env_nand_load(struct env_driver *drv)
 		gd->env_valid = ENV_REDUND;
 		env_import((char *)tmp_env2, 1);
 	} else {
-		env_import_redund((char *)tmp_env1, (char *)tmp_env2);
+		env_import_redund(drv, (char *)tmp_env1, (char *)tmp_env2);
 	}
 
 done:
diff --git a/env/ubi.c b/env/ubi.c
index a649999..51b61eb 100644
--- a/env/ubi.c
+++ b/env/ubi.c
@@ -130,7 +130,7 @@ static int env_ubi_load(struct env_driver *drv)
 		       CONFIG_ENV_UBI_PART, CONFIG_ENV_UBI_VOLUME_REDUND);
 	}
 
-	env_import_redund((char *)tmp_env1, (char *)tmp_env2);
+	env_import_redund(drv, (char *)tmp_env1, (char *)tmp_env2);
 
 	return 0;
 }
diff --git a/include/environment.h b/include/environment.h
index 34a0d5e..1e9254f 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -318,7 +318,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(struct env_driver *drv, const char *buf1,
+		      const char *buf2);
 #endif
 
 /**
-- 
git-series 0.9.1

[U-Boot] [PATCH 09/11] env: make env_locations a global variable

[Quentin Schulz]

Make env_locations a global variable so that it can be used in env
drivers.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 env/env.c             | 2 +-
 include/environment.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/env/env.c b/env/env.c
index 2565e7a..ccddc2a 100644
--- a/env/env.c
+++ b/env/env.c
@@ -35,7 +35,7 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
 	return NULL;
 }
 
-static enum env_location env_locations[] = {
+enum env_location env_locations[] = {
 #ifdef CONFIG_ENV_IS_IN_EEPROM
 	ENVL_EEPROM,
 #endif
diff --git a/include/environment.h b/include/environment.h
index 1e9254f..dd6450c 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -215,6 +215,8 @@ enum env_location {
 	ENVL_UNKNOWN,
 };
 
+extern enum env_location env_locations[];
+
 enum env_operation {
 	ENVO_GET_CHAR,
 	ENVO_INIT,
-- 
git-series 0.9.1

[U-Boot] [PATCH 10/11] env: introducing env_info struct for storing info per env

[Quentin Schulz]

To introduce the ability to use multiple environments at the same time,
it is required to store a few information about each of the environment.

Each environment has a different env_addr, "validity" (redund or main)
and status of init (failed or succeeded).

This structure is meant to be used in include/asm-generic/global_data.h
and thus needs to be put out of the #ifndef DO_DEPS_ONLY.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 include/environment.h | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/include/environment.h b/include/environment.h
index dd6450c..14df43f 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -183,20 +183,6 @@ extern uint mmc_get_env_part(struct mmc *mmc);
 # endif
 #endif
 
-#ifndef DO_DEPS_ONLY
-
-#include <env_attr.h>
-#include <env_callback.h>
-#include <env_flags.h>
-#include <search.h>
-
-/* Value for environment validity */
-enum env_valid {
-	ENV_INVALID,	/* No valid environment */
-	ENV_VALID,	/* First or only environment is valid */
-	ENV_REDUND,	/* Redundant environment is valid */
-};
-
 enum env_location {
 	ENVL_EEPROM,
 	ENVL_EXT4,
@@ -215,6 +201,26 @@ enum env_location {
 	ENVL_UNKNOWN,
 };
 
+/* Value for environment validity */
+enum env_valid {
+	ENV_INVALID,	/* No valid environment */
+	ENV_VALID,	/* First or only environment is valid */
+	ENV_REDUND,	/* Redundant environment is valid */
+};
+
+struct env_info {
+	unsigned long	env_addr;
+	enum env_valid	env_valid;
+	bool		has_init;
+};
+
+#ifndef DO_DEPS_ONLY
+
+#include <env_attr.h>
+#include <env_callback.h>
+#include <env_flags.h>
+#include <search.h>
+
 extern enum env_location env_locations[];
 
 enum env_operation {
-- 
git-series 0.9.1

[U-Boot] [PATCH 11/11] env: store information about each environment in gd

[Quentin Schulz]

To store the "validity" (redund or main), the env_addr (different when
redund or main env is used) and the init status of the environment for
each environment, an array of env_info structure is created in the
global data pointer.

Move all calls to env_addr, env_valid and env_has_init to the new model
in the different env drivers.

Note that env_addr and env_valid defined in board file are intentionally
left untouched and thus, will break compilation for a few boards.
The question is: why do we need to set env_addr and env_valid in board
files? In addition, env_addr is never used (only when using redundant
env and it's set before being used).

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 common/board_r.c                  |  8 ++++-
 env/common.c                      | 25 ++++++++++-------
 env/eeprom.c                      | 34 +++++++++++------------
 env/env.c                         | 13 +++++----
 env/flash.c                       | 48 ++++++++++++++++----------------
 env/mmc.c                         |  8 ++---
 env/nand.c                        | 36 ++++++++++++------------
 env/nowhere.c                     |  4 +--
 env/nvram.c                       | 10 +++----
 env/onenand.c                     |  2 +-
 env/remote.c                      |  4 +--
 env/sf.c                          | 26 ++++++++---------
 env/ubi.c                         |  4 +--
 include/asm-generic/global_data.h |  5 +--
 14 files changed, 121 insertions(+), 106 deletions(-)

diff --git a/common/board_r.c b/common/board_r.c
index a3b9bfb..512e109 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -125,6 +125,10 @@ __weak int fixup_cpu(void)
 
 static int initr_reloc_global_data(void)
 {
+#ifdef CONFIG_SYS_EXTRA_ENV_RELOC
+	int i;
+#endif
+
 #ifdef __ARM__
 	monitor_flash_len = _end - __image_copy_start;
 #elif defined(CONFIG_NDS32)
@@ -154,7 +158,9 @@ static int initr_reloc_global_data(void)
 	 * in SRAM mode and initialize that cache from SRAM mode back to being
 	 * a cache in cpu_init_r.
 	 */
-	gd->env_addr += gd->relocaddr - CONFIG_SYS_MONITOR_BASE;
+	for (i = 0; i < ENVL_COUNT; i++)
+		gd->env_infos[i].env_addr += gd->relocaddr -
+			CONFIG_SYS_MONITOR_BASE;
 #endif
 #ifdef CONFIG_OF_EMBED
 	/*
diff --git a/env/common.c b/env/common.c
index 24cf41b..72359fe 100644
--- a/env/common.c
+++ b/env/common.c
@@ -214,24 +214,24 @@ int env_import_redund(struct env_driver *drv, const char *buf1, const char *buf2
 		set_default_env("!bad CRC");
 		return 0;
 	} else if (crc1_ok && !crc2_ok) {
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 	} else if (!crc1_ok && crc2_ok) {
-		gd->env_valid = ENV_REDUND;
+		gd->env_infos[drv->location].env_valid = ENV_REDUND;
 	} else {
 		/* both ok - check serial */
 		if (tmp_env1->flags == 255 && tmp_env2->flags == 0)
-			gd->env_valid = ENV_REDUND;
+			gd->env_infos[drv->location].env_valid = ENV_REDUND;
 		else if (tmp_env2->flags == 255 && tmp_env1->flags == 0)
-			gd->env_valid = ENV_VALID;
+			gd->env_infos[drv->location].env_valid = ENV_VALID;
 		else if (tmp_env1->flags > tmp_env2->flags)
-			gd->env_valid = ENV_VALID;
+			gd->env_infos[drv->location].env_valid = ENV_VALID;
 		else if (tmp_env2->flags > tmp_env1->flags)
-			gd->env_valid = ENV_REDUND;
+			gd->env_infos[drv->location].env_valid = ENV_REDUND;
 		else /* flags are equal - almost impossible */
-			gd->env_valid = ENV_VALID;
+			gd->env_infos[drv->location].env_valid = ENV_VALID;
 	}
 
-	if (gd->env_valid == ENV_VALID)
+	if (gd->env_infos[drv->location].env_valid == ENV_VALID)
 		ep = tmp_env1;
 	else
 		ep = tmp_env2;
@@ -271,11 +271,18 @@ int env_export(env_t *env_out)
 
 void env_relocate(void)
 {
+	int i;
+
 #if defined(CONFIG_NEEDS_MANUAL_RELOC)
 	env_reloc();
 	env_htab.change_ok += gd->reloc_off;
+
 #endif
-	if (gd->env_valid == ENV_INVALID) {
+	for(i = 0; i < ENVL_COUNT; i++)
+		if (gd->env_infos[i].has_init)
+			break;
+
+	if (i == ENVL_COUNT) {
 #if defined(CONFIG_SPL_BUILD)
 		/* Environment not changable */
 		set_default_env(NULL);
diff --git a/env/eeprom.c b/env/eeprom.c
index b0ffce4..b48aa97 100644
--- a/env/eeprom.c
+++ b/env/eeprom.c
@@ -67,7 +67,7 @@ static int env_eeprom_get_char(struct env_driver *drv, int index)
 	unsigned int off = CONFIG_ENV_OFFSET;
 
 #ifdef CONFIG_ENV_OFFSET_REDUND
-	if (gd->env_valid == ENV_REDUND)
+	if (gd->env_infos[drv->location].env_valid == ENV_REDUND)
 		off = CONFIG_ENV_OFFSET_REDUND;
 #endif
 	eeprom_bus_read(CONFIG_SYS_DEF_EEPROM_ADDR,
@@ -121,24 +121,24 @@ static int env_eeprom_load(struct env_driver *drv)
 	}
 
 	if (!crc_ok[0] && !crc_ok[1]) {
-		gd->env_addr	= 0;
-		gd->env_valid = ENV_INVALID;
+		gd->env_infos[drv->location].env_addr	= 0;
+		gd->env_infos[drv->location].env_valid = ENV_INVALID;
 	} else if (crc_ok[0] && !crc_ok[1]) {
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 	} else if (!crc_ok[0] && crc_ok[1]) {
-		gd->env_valid = ENV_REDUND;
+		gd->env_infos[drv->location].env_valid = ENV_REDUND;
 	} else {
 		/* both ok - check serial */
 		if (flags[0] == ACTIVE_FLAG && flags[1] == OBSOLETE_FLAG)
-			gd->env_valid = ENV_VALID;
+			gd->env_infos[drv->location].env_valid = ENV_VALID;
 		else if (flags[0] == OBSOLETE_FLAG && flags[1] == ACTIVE_FLAG)
-			gd->env_valid = ENV_REDUND;
+			gd->env_infos[drv->location].env_valid = ENV_REDUND;
 		else if (flags[0] == 0xFF && flags[1] == 0)
-			gd->env_valid = ENV_REDUND;
+			gd->env_infos[drv->location].env_valid = ENV_REDUND;
 		else if (flags[1] == 0xFF && flags[0] == 0)
-			gd->env_valid = ENV_VALID;
+			gd->env_infos[drv->location].env_valid = ENV_VALID;
 		else /* flags are equal - almost impossible */
-			gd->env_valid = ENV_VALID;
+			gd->env_infos[drv->location].env_valid = ENV_VALID;
 	}
 
 #else /* CONFIG_ENV_OFFSET_REDUND */
@@ -166,15 +166,15 @@ static int env_eeprom_load(struct env_driver *drv)
 	}
 
 	if (crc == new) {
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 	} else {
-		gd->env_valid = ENV_INVALID;
+		gd->env_infos[drv->location].env_valid = ENV_INVALID;
 	}
 #endif /* CONFIG_ENV_OFFSET_REDUND */
 
 	off = CONFIG_ENV_OFFSET;
 #ifdef CONFIG_ENV_OFFSET_REDUND
-	if (gd->env_valid == ENV_REDUND)
+	if (gd->env_infos[drv->location].env_valid == ENV_REDUND)
 		off = CONFIG_ENV_OFFSET_REDUND;
 #endif
 
@@ -201,7 +201,7 @@ static int env_eeprom_save(struct env_driver *drv)
 		return rc;
 
 #ifdef CONFIG_ENV_OFFSET_REDUND
-	if (gd->env_valid == ENV_VALID) {
+	if (gd->env_infos[drv->location].env_valid == ENV_VALID) {
 		off	= CONFIG_ENV_OFFSET_REDUND;
 		off_red	= CONFIG_ENV_OFFSET;
 	}
@@ -218,10 +218,10 @@ static int env_eeprom_save(struct env_driver *drv)
 				 off_red + offsetof(env_t, flags),
 				 (uchar *)&flag_obsolete, 1);
 
-		if (gd->env_valid == ENV_VALID)
-			gd->env_valid = ENV_REDUND;
+		if (gd->env_infos[drv->location].env_valid == ENV_VALID)
+			gd->env_infos[drv->location].env_valid = ENV_REDUND;
 		else
-			gd->env_valid = ENV_VALID;
+			gd->env_infos[drv->location].env_valid = ENV_VALID;
 	}
 #endif
 	return rc;
diff --git a/env/env.c b/env/env.c
index ccddc2a..01233ce 100644
--- a/env/env.c
+++ b/env/env.c
@@ -150,7 +150,7 @@ int env_get_char(int index)
 		if (!drv->get_char)
 			continue;
 
-		if (!(gd->env_has_init & BIT(drv->location)))
+		if (!gd->env_infos[drv->location].has_init)
 			continue;
 
 		ret = drv->get_char(drv, index);
@@ -177,7 +177,7 @@ int env_load(void)
 		if (!drv->load)
 			continue;
 
-		if (!(gd->env_has_init & BIT(drv->location)))
+		if (!gd->env_infos[drv->location].has_init)
 			continue;
 
 		printf("Loading Environment from %s... ", drv->name);
@@ -203,7 +203,10 @@ int env_load(void)
 		if (!drv->load)
 			continue;
 
-		printf("Overriding env variables with ones from %s env...",
+		if (!gd->env_infos[drv->location].has_init)
+			continue;
+
+		printf("%s: Overriding env variables with ones from %s env...",
 		      __func__, drv->name);
 		ret = drv->load(drv);
 		printf("%s\n", ret ? "Failed" : "OK");
@@ -237,7 +240,7 @@ int env_save(void)
 		if (!drv->save)
 			continue;
 
-		if (!(gd->env_has_init & BIT(drv->location)))
+		if (!gd->env_infos[drv->location].has_init)
 			continue;
 
 		printf("Saving Environment to %s... ", drv->name);
@@ -277,7 +280,7 @@ int env_init(void)
 			ret = 0;
 
 		if (!ret) {
-			gd->env_has_init |= BIT(drv->location);
+			gd->env_infos[drv->location].has_init = true;
 			init = true;
 		}
 
diff --git a/env/flash.c b/env/flash.c
index 45e58b4..1c7666a 100644
--- a/env/flash.c
+++ b/env/flash.c
@@ -87,29 +87,29 @@ static int env_flash_init(struct env_driver *drv)
 		crc32(0, flash_addr_new->data, ENV_SIZE) == flash_addr_new->crc;
 
 	if (crc1_ok && !crc2_ok) {
-		gd->env_addr	= addr1;
-		gd->env_valid	= ENV_VALID;
+		gd->env_infos[drv->location].env_addr	= addr1;
+		gd->env_infos[drv->location].env_valid	= ENV_VALID;
 	} else if (!crc1_ok && crc2_ok) {
-		gd->env_addr	= addr2;
-		gd->env_valid	= ENV_VALID;
+		gd->env_infos[drv->location].env_addr	= addr2;
+		gd->env_infos[drv->location].env_valid	= ENV_VALID;
 	} else if (!crc1_ok && !crc2_ok) {
-		gd->env_addr	= addr_default;
-		gd->env_valid	= ENV_INVALID;
+		gd->env_infos[drv->location].env_addr	= addr_default;
+		gd->env_infos[drv->location].env_valid	= ENV_INVALID;
 	} else if (flag1 == ACTIVE_FLAG && flag2 == OBSOLETE_FLAG) {
-		gd->env_addr	= addr1;
-		gd->env_valid	= ENV_VALID;
+		gd->env_infos[drv->location].env_addr	= addr1;
+		gd->env_infos[drv->location].env_valid	= ENV_VALID;
 	} else if (flag1 == OBSOLETE_FLAG && flag2 == ACTIVE_FLAG) {
-		gd->env_addr	= addr2;
-		gd->env_valid	= ENV_VALID;
+		gd->env_infos[drv->location].env_addr	= addr2;
+		gd->env_infos[drv->location].env_valid	= ENV_VALID;
 	} else if (flag1 == flag2) {
-		gd->env_addr	= addr1;
-		gd->env_valid	= ENV_REDUND;
+		gd->env_infos[drv->location].env_addr	= addr1;
+		gd->env_infos[drv->location].env_valid	= ENV_REDUND;
 	} else if (flag1 == 0xFF) {
-		gd->env_addr	= addr1;
-		gd->env_valid	= ENV_REDUND;
+		gd->env_infos[drv->location].env_addr	= addr1;
+		gd->env_infos[drv->location].env_valid	= ENV_REDUND;
 	} else if (flag2 == 0xFF) {
-		gd->env_addr	= addr2;
-		gd->env_valid	= ENV_REDUND;
+		gd->env_infos[drv->location].env_addr	= addr2;
+		gd->env_infos[drv->location].env_valid	= ENV_REDUND;
 	}
 
 	return 0;
@@ -225,13 +225,13 @@ done:
 static int env_flash_init(struct env_driver *drv)
 {
 	if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
-		gd->env_addr	= (ulong)&(env_ptr->data);
-		gd->env_valid	= ENV_VALID;
+		gd->env_infos[drv->location].env_addr	= (ulong)&(env_ptr->data);
+		gd->env_infos[drv->location].env_valid	= ENV_VALID;
 		return 0;
 	}
 
-	gd->env_addr	= (ulong)&default_environment[0];
-	gd->env_valid	= ENV_INVALID;
+	gd->env_infos[drv->location].env_addr	= (ulong)&default_environment[0];
+	gd->env_infos[drv->location].env_valid	= ENV_INVALID;
 	return 0;
 }
 #endif
@@ -311,7 +311,7 @@ done:
 static int env_flash_load(struct env_driver *drv)
 {
 #ifdef CONFIG_ENV_ADDR_REDUND
-	if (gd->env_addr != (ulong)&(flash_addr->data)) {
+	if (gd->env_infos[drv->location].env_addr != (ulong)&(flash_addr->data)) {
 		env_t *etmp = flash_addr;
 		ulong ltmp = end_addr;
 
@@ -326,7 +326,7 @@ static int env_flash_load(struct env_driver *drv)
 	    crc32(0, flash_addr_new->data, ENV_SIZE) == flash_addr_new->crc) {
 		char flag = OBSOLETE_FLAG;
 
-		gd->env_valid = ENV_REDUND;
+		gd->env_infos[drv->location].env_valid = ENV_REDUND;
 		flash_sect_protect(0, (ulong)flash_addr_new, end_addr_new);
 		flash_write(&flag,
 			    (ulong)&(flash_addr_new->flags),
@@ -338,7 +338,7 @@ static int env_flash_load(struct env_driver *drv)
 	    (flash_addr->flags & ACTIVE_FLAG) == ACTIVE_FLAG) {
 		char flag = ACTIVE_FLAG;
 
-		gd->env_valid = ENV_REDUND;
+		gd->env_infos[drv->location].env_valid = ENV_REDUND;
 		flash_sect_protect(0, (ulong)flash_addr, end_addr);
 		flash_write(&flag,
 			    (ulong)&(flash_addr->flags),
@@ -346,7 +346,7 @@ static int env_flash_load(struct env_driver *drv)
 		flash_sect_protect(1, (ulong)flash_addr, end_addr);
 	}
 
-	if (gd->env_valid == ENV_REDUND)
+	if (gd->env_infos[drv->location].env_valid == ENV_REDUND)
 		puts("*** Warning - some problems detected "
 		     "reading environment; recovered successfully\n\n");
 #endif /* CONFIG_ENV_ADDR_REDUND */
diff --git a/env/mmc.c b/env/mmc.c
index 64d1404..250108f 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -163,7 +163,7 @@ static int env_mmc_save(struct env_driver *drv)
 		goto fini;
 
 #ifdef CONFIG_ENV_OFFSET_REDUND
-	if (gd->env_valid == ENV_VALID)
+	if (gd->env_infos[drv->location].env_valid == ENV_VALID)
 		copy = 1;
 #endif
 
@@ -182,7 +182,7 @@ static int env_mmc_save(struct env_driver *drv)
 	ret = 0;
 
 #ifdef CONFIG_ENV_OFFSET_REDUND
-	gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
+	gd->env_infos[drv->location].env_valid = gd->env_infos[drv->location].env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
 #endif
 
 fini:
@@ -247,10 +247,10 @@ static int env_mmc_load(struct env_driver *drv)
 		ret = -EIO;
 		goto fini;
 	} else if (!read1_fail && read2_fail) {
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 		env_import((char *)tmp_env1, 1);
 	} else if (read1_fail && !read2_fail) {
-		gd->env_valid = ENV_REDUND;
+		gd->env_infos[drv->location].env_valid = ENV_REDUND;
 		env_import((char *)tmp_env2, 1);
 	} else {
 		env_import_redund((char *)tmp_env1, (char *)tmp_env2);
diff --git a/env/nand.c b/env/nand.c
index 49e506e..77b9ca1 100644
--- a/env/nand.c
+++ b/env/nand.c
@@ -78,42 +78,42 @@ static int env_nand_init(struct env_driver *drv)
 	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc;
 
 	if (!crc1_ok && !crc2_ok) {
-		gd->env_addr	= 0;
-		gd->env_valid	= ENV_INVALID;
+		gd->env_infos[drv->location].env_addr	= 0;
+		gd->env_infos[drv->location].env_valid	= ENV_INVALID;
 
 		return 0;
 	} else if (crc1_ok && !crc2_ok) {
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 	}
 #ifdef CONFIG_ENV_OFFSET_REDUND
 	else if (!crc1_ok && crc2_ok) {
-		gd->env_valid = ENV_REDUND;
+		gd->env_infos[drv->location].env_valid = ENV_REDUND;
 	} else {
 		/* both ok - check serial */
 		if (tmp_env1->flags == 255 && tmp_env2->flags == 0)
-			gd->env_valid = ENV_REDUND;
+			gd->env_infos[drv->location].env_valid = ENV_REDUND;
 		else if (tmp_env2->flags == 255 && tmp_env1->flags == 0)
-			gd->env_valid = ENV_VALID;
+			gd->env_infos[drv->location].env_valid = ENV_VALID;
 		else if (tmp_env1->flags > tmp_env2->flags)
-			gd->env_valid = ENV_VALID;
+			gd->env_infos[drv->location].env_valid = ENV_VALID;
 		else if (tmp_env2->flags > tmp_env1->flags)
-			gd->env_valid = ENV_REDUND;
+			gd->env_infos[drv->location].env_valid = ENV_REDUND;
 		else /* flags are equal - almost impossible */
-			gd->env_valid = ENV_VALID;
+			gd->env_infos[drv->location].env_valid = ENV_VALID;
 	}
 
-	if (gd->env_valid == ENV_REDUND)
+	if (gd->env_infos[drv->location].env_valid == ENV_REDUND)
 		env_ptr = tmp_env2;
 	else
 #endif
-	if (gd->env_valid == ENV_VALID)
+	if (gd->env_infos[drv->location].env_valid == ENV_VALID)
 		env_ptr = tmp_env1;
 
-	gd->env_addr = (ulong)env_ptr->data;
+	gd->env_infos[drv->location].env_addr = (ulong)env_ptr->data;
 
 #else /* ENV_IS_EMBEDDED || CONFIG_NAND_ENV_DST */
-	gd->env_addr	= (ulong)&default_environment[0];
-	gd->env_valid	= ENV_VALID;
+	gd->env_infos[drv->location].env_addr	= (ulong)&default_environment[0];
+	gd->env_infos[drv->location].env_valid	= ENV_VALID;
 #endif /* ENV_IS_EMBEDDED || CONFIG_NAND_ENV_DST */
 
 	return 0;
@@ -216,14 +216,14 @@ static int env_nand_save(struct env_driver *drv)
 		return ret;
 
 #ifdef CONFIG_ENV_OFFSET_REDUND
-	env_idx = (gd->env_valid == ENV_VALID);
+	env_idx = (gd->env_infos[drv->location].env_valid == ENV_VALID);
 #endif
 
 	ret = erase_and_write_env(&location[env_idx], (u_char *)env_new);
 #ifdef CONFIG_ENV_OFFSET_REDUND
 	if (!ret) {
 		/* preset other copy for next write */
-		gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID :
+		gd->env_infos[drv->location].env_valid = gd->env_infos[drv->location].env_valid == ENV_REDUND ? ENV_VALID :
 				ENV_REDUND;
 		return ret;
 	}
@@ -346,10 +346,10 @@ static int env_nand_load(struct env_driver *drv)
 		set_default_env("!bad env area");
 		goto done;
 	} else if (!read1_fail && read2_fail) {
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 		env_import((char *)tmp_env1, 1);
 	} else if (read1_fail && !read2_fail) {
-		gd->env_valid = ENV_REDUND;
+		gd->env_infos[drv->location].env_valid = ENV_REDUND;
 		env_import((char *)tmp_env2, 1);
 	} else {
 		env_import_redund(drv, (char *)tmp_env1, (char *)tmp_env2);
diff --git a/env/nowhere.c b/env/nowhere.c
index fb6ea9e..7d81a57 100644
--- a/env/nowhere.c
+++ b/env/nowhere.c
@@ -26,8 +26,8 @@ static int env_nowhere_load(struct env_driver *drv)
  */
 static int env_nowhere_init(struct env_driver *drv)
 {
-	gd->env_addr	= (ulong)&default_environment[0];
-	gd->env_valid	= ENV_VALID;
+	gd->env_infos[drv->location].env_addr	= (ulong)&default_environment[0];
+	gd->env_infos[drv->location].env_valid	= ENV_VALID;
 
 	return 0;
 }
diff --git a/env/nvram.c b/env/nvram.c
index aad341d..c36c1af 100644
--- a/env/nvram.c
+++ b/env/nvram.c
@@ -98,15 +98,15 @@ static int env_nvram_init(struct env_driver *drv)
 	nvram_read(data, CONFIG_ENV_ADDR + sizeof(ulong), ENV_SIZE);
 
 	if (crc32(0, data, ENV_SIZE) == crc) {
-		gd->env_addr	= (ulong)CONFIG_ENV_ADDR + sizeof(long);
+		gd->env_infos[drv->location].env_addr	= (ulong)CONFIG_ENV_ADDR + sizeof(long);
 #else
 	if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
-		gd->env_addr	= (ulong)&env_ptr->data;
+		gd->env_infos[drv->location].env_addr	= (ulong)&env_ptr->data;
 #endif
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 	} else {
-		gd->env_addr	= (ulong)&default_environment[0];
-		gd->env_valid	= ENV_INVALID;
+		gd->env_infos[drv->location].env_addr	= (ulong)&default_environment[0];
+		gd->env_infos[drv->location].env_valid	= ENV_INVALID;
 	}
 
 	return 0;
diff --git a/env/onenand.c b/env/onenand.c
index e633ebe..68790f1 100644
--- a/env/onenand.c
+++ b/env/onenand.c
@@ -58,7 +58,7 @@ static int env_onenand_load(struct env_driver *drv)
 
 	rc = env_import(buf, 1);
 	if (rc)
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 
 	return rc ? 0 : -EIO;
 }
diff --git a/env/remote.c b/env/remote.c
index b004964..895759e 100644
--- a/env/remote.c
+++ b/env/remote.c
@@ -26,8 +26,8 @@ DECLARE_GLOBAL_DATA_PTR;
 static int env_remote_init(struct env_driver *drv)
 {
 	if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
-		gd->env_addr = (ulong)&(env_ptr->data);
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_addr = (ulong)&(env_ptr->data);
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 		return 0;
 	}
 
diff --git a/env/sf.c b/env/sf.c
index e811cde..40e2671 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -98,7 +98,7 @@ static int env_sf_save(struct env_driver *drv)
 		return -EIO;
 	env_new.flags	= ACTIVE_FLAG;
 
-	if (gd->env_valid == ENV_VALID) {
+	if (gd->env_infos[drv->location].env_valid == ENV_VALID) {
 		env_new_offset = CONFIG_ENV_OFFSET_REDUND;
 		env_offset = CONFIG_ENV_OFFSET;
 	} else {
@@ -150,9 +150,9 @@ static int env_sf_save(struct env_driver *drv)
 
 	puts("done\n");
 
-	gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
+	gd->env_infos[drv->location].env_valid = gd->env_infos[drv->location].env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
 
-	printf("Valid environment: %d\n", (int)gd->env_valid);
+	printf("Valid environment: %d\n", (int)gd->env_infos[drv->location].env_valid);
 
  done:
 	if (saved_buffer)
@@ -206,30 +206,30 @@ static int env_sf_load(struct env_driver *drv)
 		ret = -EIO;
 		goto err_read;
 	} else if (crc1_ok && !crc2_ok) {
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 	} else if (!crc1_ok && crc2_ok) {
-		gd->env_valid = ENV_REDUND;
+		gd->env_infos[drv->location].env_valid = ENV_REDUND;
 	} else if (tmp_env1->flags == ACTIVE_FLAG &&
 		   tmp_env2->flags == OBSOLETE_FLAG) {
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 	} else if (tmp_env1->flags == OBSOLETE_FLAG &&
 		   tmp_env2->flags == ACTIVE_FLAG) {
-		gd->env_valid = ENV_REDUND;
+		gd->env_infos[drv->location].env_valid = ENV_REDUND;
 	} else if (tmp_env1->flags == tmp_env2->flags) {
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 	} else if (tmp_env1->flags == 0xFF) {
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 	} else if (tmp_env2->flags == 0xFF) {
-		gd->env_valid = ENV_REDUND;
+		gd->env_infos[drv->location].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;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 	}
 
-	if (gd->env_valid == ENV_VALID)
+	if (gd->env_infos[drv->location].env_valid == ENV_VALID)
 		ep = tmp_env1;
 	else
 		ep = tmp_env2;
@@ -336,7 +336,7 @@ static int env_sf_load(struct env_driver *drv)
 
 	ret = env_import(buf, 1);
 	if (ret)
-		gd->env_valid = ENV_VALID;
+		gd->env_infos[drv->location].env_valid = ENV_VALID;
 
 err_read:
 	spi_flash_free(env_flash);
diff --git a/env/ubi.c b/env/ubi.c
index 51b61eb..bd137e9 100644
--- a/env/ubi.c
+++ b/env/ubi.c
@@ -35,7 +35,7 @@ static int env_ubi_save(struct env_driver *drv)
 		return 1;
 	}
 
-	if (gd->env_valid == ENV_VALID) {
+	if (gd->env_infos[drv->location].env_valid == ENV_VALID) {
 		puts("Writing to redundant UBI... ");
 		if (ubi_volume_write(CONFIG_ENV_UBI_VOLUME_REDUND,
 				     (void *)env_new, CONFIG_ENV_SIZE)) {
@@ -57,7 +57,7 @@ static int env_ubi_save(struct env_driver *drv)
 
 	puts("done\n");
 
-	gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
+	gd->env_infos[drv->location].env_valid = gd->env_infos[drv->location].env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
 
 	return 0;
 }
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index 1d0611f..17cf1b7 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -23,6 +23,7 @@
 #ifndef __ASSEMBLY__
 #include <membuff.h>
 #include <linux/list.h>
+#include <environment.h>
 
 typedef struct global_data {
 	bd_t *bd;
@@ -48,9 +49,7 @@ typedef struct global_data {
 #if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER)
 	unsigned long precon_buf_idx;	/* Pre-Console buffer index */
 #endif
-	unsigned long env_addr;		/* Address  of Environment struct */
-	unsigned long env_valid;	/* Environment valid? enum env_valid */
-	unsigned long env_has_init;	/* Bitmask of boolean of struct env_location offsets */
+	struct env_info env_infos[ENVL_COUNT];
 
 	unsigned long ram_top;		/* Top address of RAM used by U-Boot */
 	unsigned long relocaddr;	/* Start address of U-Boot in RAM */
-- 
git-series 0.9.1

[U-Boot] [PATCH 03/11] env: add support for whitelisting variables from secondary environments

[Lukasz Majewski]

Hi Quentin,

> +config ENV_VAR_WHITELIST
> +	bool "Enable overriding some variables from secondary
> environments"
> +	help
> +	  This allows overriding some variables from secondary
> environments.
> +	  After the first valid environment is loaded, a secondary
> environment
> +	  is pre-loaded and its variables that are present in the
> whitelist will
> +	  be added to the current environment or will override
> existing
> +	  variables.

This description seems to do what is already done in the "env import"
command.

It also allows for setting env variables from HUSH prompt.

Maybe it would be possible to re-use or extend it to suits your needs?


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171226/96eb08db/attachment.sig>

[U-Boot] [PATCH 00/11] Introduce variables whitelisting in environment

[Lukasz Majewski]

Hi Quentin,

> 2) Why do we consider ENV_IS_NOWHERE an invalid environment? The only
> place I found a use for it was to just say that if the environment is
> invalid, we should set to default environment (in env_relocate in
> env/common.c). With my patch series I guess that we could remove this
> fallback and force ENV_IS_NOWHERE to be always there.

It is perfectly valid to use ENV_IS_NOWHERE to get envs from hardcoded
board file (for example for production u-boot).

However, some time ago we had extension in the envs to get the support
for envs from different mediums (with set priorities).

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171226/1b8894d9/attachment.sig>

[U-Boot] [PATCH 00/11] Introduce variables whitelisting in environment

[Simon Glass]

Hi Quentin,

On 22 December 2017 at 14:13, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> This patch series is based on this[1] patch series from Maxime.
>
> This is an RFC. It's been only tested in a specific use case on a custom
> i.MX6 board. It's known to break compilation on a few boards.
>
> I have a use case where we want some variables from a first environment to
> be overriden by variables from a second environment. For example, we want
> to load variables from the default env (ENV_IS_NOWHERE) and then load only
> a handful of other variables from, e.g., NAND.
>
> In our use case, we basically can be sure that the default env in the
> U-Boot binary is secure but we want only a few variables to be modified,
> thus keeping control over the overall behaviour of U-Boot in secure mode.
>
> It works in that way:
>   - from highest to lowest priority, the first environment that can be
>   loaded (that has successfully init and whose load function has returned
>   no errors) will be the main environment,
>   - then, all the following environment that could be successfully loaded
>   (same conditions as the main environment) are secondary environment. The
>   env variables that are defined both in CONFIG_ENV_VAR_WHITELIST_LIST and
>   in the secondary environments override the ones in the main environment,
>   - for saving, we save the whole environment to all environments
>   available, be they main or secondary (it does not matter to save the
>   whole environment on secondary environments as only the whitelisted
>   variables will be overriden in the loading process,
>
> I have also a few questions that could help me to get the whole thing to
> work.
>
> 1) I can't really get my head around the use of gd->env_addr, what is it used
> for? It is set in a bunch of different places but only once is it
> explicitly used (basically to alternate the env_addr between the one
> associated to main and redundant environment (in NAND for example)).
>
> 2) Why do we consider ENV_IS_NOWHERE an invalid environment? The only place I
> found a use for it was to just say that if the environment is invalid, we
> should set to default environment (in env_relocate in env/common.c). With
> my patch series I guess that we could remove this fallback and force
> ENV_IS_NOWHERE to be always there.
>
> 3) There are a few (20) boards that set gd->env_addr and gd->env_valid in
> their board file. What is the reason to do such a thing? Isn't those
> overriden anyway by the environment driver?
>
> I'm looking forward to getting your feedback on this patch series.
>

I certainly understand the goal and it seems generally useful.

But I wonder whether this is the best way to implement it.

We could have a UCLASS_ENV uclass, with driver-model drivers which
load the environment (i.e. have load() and save() methods). Config for
the drivers would come from the device tree. Useful drivers would be:

- one that loads the env from a single location
- one that loads multiple envs from different locations in priority order
- one that does what you want

Then people could set their own policy by adding a driver.

I worry that what we have here is quite heavyweight, and will be
imposed on all users, e.g. the size increase of gd, the new logic.
Where does it end? I think splitting things up into different use
cases makes sense.

When I did the env refactor I envisaged using driver-model for the
post-reloc env load/save at some point in the future. It solves the
problem of getting the config (can use device tree) and different
boards wanting to do different things (boards can provide an env
driver).

Regards,
Simon

> Thanks,
> Quentin
>
> [1] https://patchwork.ozlabs.org/cover/842057/
>
> Quentin Schulz (11):
>   env: fix ret not being set and fails when no env could have been init
>   lib: hashtable: support whitelisting env variables
>   env: add support for whitelisting variables from secondary environments
>   env: make nowhere an env medium like the others
>   cmd: saveenv: enable the saveenv command when ENV_IS_NOWHERE is defined but another env medium is enabled too
>   env: add env_driver to env_driver functions' arguments
>   env: gd flags without ENV_READY is enough to discriminate in env_get_default
>   env: add env_driver parameter to env_import_redund
>   env: make env_locations a global variable
>   env: introducing env_info struct for storing info per env
>   env: store information about each environment in gd
>
>  board/sunxi/board.c               |   2 +-
>  cmd/nvedit.c                      |  16 ++-
>  common/board_r.c                  |   8 +-
>  env/Kconfig                       |  29 +++---
>  env/common.c                      |  45 ++++++----
>  env/eeprom.c                      |  40 ++++-----
>  env/env.c                         | 142 +++++++++++++++++++++++++------
>  env/ext4.c                        |   4 +-
>  env/fat.c                         |   4 +-
>  env/flash.c                       |  58 ++++++-------
>  env/mmc.c                         |  14 +--
>  env/nand.c                        |  46 +++++-----
>  env/nowhere.c                     |  12 ++-
>  env/nvram.c                       |  18 ++--
>  env/onenand.c                     |   6 +-
>  env/remote.c                      |  10 +-
>  env/sata.c                        |   4 +-
>  env/sf.c                          |  34 +++----
>  env/ubi.c                         |  14 +--
>  include/asm-generic/global_data.h |   5 +-
>  include/environment.h             |  59 ++++++++-----
>  include/search.h                  |   2 +-
>  lib/hashtable.c                   |  17 +++-
>  23 files changed, 379 insertions(+), 210 deletions(-)
>
> base-commit: 5d41e28058e7b378c9fa5c61ecc074a682ba2db4
> --
> git-series 0.9.1

[U-Boot] [PATCH 00/11] Introduce variables whitelisting in environment

[Quentin Schulz]

Hi Simon,

On 29/12/2017 04:13, Simon Glass wrote:
> Hi Quentin,
> 
> On 22 December 2017 at 14:13, Quentin Schulz
> <quentin.schulz@free-electrons.com> wrote:
>> This patch series is based on this[1] patch series from Maxime.
>>
>> This is an RFC. It's been only tested in a specific use case on a custom
>> i.MX6 board. It's known to break compilation on a few boards.
>>
>> I have a use case where we want some variables from a first environment to
>> be overriden by variables from a second environment. For example, we want
>> to load variables from the default env (ENV_IS_NOWHERE) and then load only
>> a handful of other variables from, e.g., NAND.
>>
>> In our use case, we basically can be sure that the default env in the
>> U-Boot binary is secure but we want only a few variables to be modified,
>> thus keeping control over the overall behaviour of U-Boot in secure mode.
>>
>> It works in that way:
>>   - from highest to lowest priority, the first environment that can be
>>   loaded (that has successfully init and whose load function has returned
>>   no errors) will be the main environment,
>>   - then, all the following environment that could be successfully loaded
>>   (same conditions as the main environment) are secondary environment. The
>>   env variables that are defined both in CONFIG_ENV_VAR_WHITELIST_LIST and
>>   in the secondary environments override the ones in the main environment,
>>   - for saving, we save the whole environment to all environments
>>   available, be they main or secondary (it does not matter to save the
>>   whole environment on secondary environments as only the whitelisted
>>   variables will be overriden in the loading process,
>>
>> I have also a few questions that could help me to get the whole thing to
>> work.
>>
>> 1) I can't really get my head around the use of gd->env_addr, what is it used
>> for? It is set in a bunch of different places but only once is it
>> explicitly used (basically to alternate the env_addr between the one
>> associated to main and redundant environment (in NAND for example)).
>>
>> 2) Why do we consider ENV_IS_NOWHERE an invalid environment? The only place I
>> found a use for it was to just say that if the environment is invalid, we
>> should set to default environment (in env_relocate in env/common.c). With
>> my patch series I guess that we could remove this fallback and force
>> ENV_IS_NOWHERE to be always there.
>>
>> 3) There are a few (20) boards that set gd->env_addr and gd->env_valid in
>> their board file. What is the reason to do such a thing? Isn't those
>> overriden anyway by the environment driver?
>>
>> I'm looking forward to getting your feedback on this patch series.
>>
> 
> I certainly understand the goal and it seems generally useful.
> 
> But I wonder whether this is the best way to implement it.
> 
> We could have a UCLASS_ENV uclass, with driver-model drivers which
> load the environment (i.e. have load() and save() methods). Config for
> the drivers would come from the device tree. Useful drivers would be:
> 

I'm not sure the device tree is the place to set/get such info. That has
nothing to do with hardware, it's only logic for the bootloader.

> - one that loads the env from a single location
> - one that loads multiple envs from different locations in priority order
> - one that does what you want
> 
> Then people could set their own policy by adding a driver.
> 

I'll have to document myself on driver-model and how U-Boot implement it
then :)

> I worry that what we have here is quite heavyweight, and will be
> imposed on all users, e.g. the size increase of gd, the new logic.
> Where does it end? I think splitting things up into different use
> cases makes sense.
> 

Agree on that.

> When I did the env refactor I envisaged using driver-model for the
> post-reloc env load/save at some point in the future. It solves the
> problem of getting the config (can use device tree) and different
> boards wanting to do different things (boards can provide an env
> driver).
> 

Overall, I prefer Lukasz's suggestion as it's quicker and easier to
implement and require (I think) less changes in the code.

Thanks for the review,
Quentin
-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[U-Boot] [PATCH 03/11] env: add support for whitelisting variables from secondary environments

[Quentin Schulz]

Hi Lukasz,

On 26/12/2017 12:21, Lukasz Majewski wrote:
> Hi Quentin,
> 
>> +config ENV_VAR_WHITELIST
>> +	bool "Enable overriding some variables from secondary
>> environments"
>> +	help
>> +	  This allows overriding some variables from secondary
>> environments.
>> +	  After the first valid environment is loaded, a secondary
>> environment
>> +	  is pre-loaded and its variables that are present in the
>> whitelist will
>> +	  be added to the current environment or will override
>> existing
>> +	  variables.
> 
> This description seems to do what is already done in the "env import"
> command.
> 

Didn't know that one, thanks.

> It also allows for setting env variables from HUSH prompt.
> 
> Maybe it would be possible to re-use or extend it to suits your needs?
> 

It's pretty much everything I needed except the whitelisting of variables.

While it's possible for env export to take a list of variables to
export, for what I've seen, env import does not take a list of variables
to import.

It's needed since the env I'll import is unsafe. I'll have a look at
that and make an other RFC :)

Thanks for the review, very helpful,

Quentin
-- 
Quentin Schulz, 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: 801 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180103/c0b3f77b/attachment.sig>

[U-Boot] [PATCH 00/11] Introduce variables whitelisting in environment

[Simon Glass]

Hi Quentin,

On 3 January 2018 at 02:18, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> Hi Simon,
>
> On 29/12/2017 04:13, Simon Glass wrote:
>> Hi Quentin,
>>
>> On 22 December 2017 at 14:13, Quentin Schulz
>> <quentin.schulz@free-electrons.com> wrote:
>>> This patch series is based on this[1] patch series from Maxime.
>>>
>>> This is an RFC. It's been only tested in a specific use case on a custom
>>> i.MX6 board. It's known to break compilation on a few boards.
>>>
>>> I have a use case where we want some variables from a first environment to
>>> be overriden by variables from a second environment. For example, we want
>>> to load variables from the default env (ENV_IS_NOWHERE) and then load only
>>> a handful of other variables from, e.g., NAND.
>>>
>>> In our use case, we basically can be sure that the default env in the
>>> U-Boot binary is secure but we want only a few variables to be modified,
>>> thus keeping control over the overall behaviour of U-Boot in secure mode.
>>>
>>> It works in that way:
>>>   - from highest to lowest priority, the first environment that can be
>>>   loaded (that has successfully init and whose load function has returned
>>>   no errors) will be the main environment,
>>>   - then, all the following environment that could be successfully loaded
>>>   (same conditions as the main environment) are secondary environment. The
>>>   env variables that are defined both in CONFIG_ENV_VAR_WHITELIST_LIST and
>>>   in the secondary environments override the ones in the main environment,
>>>   - for saving, we save the whole environment to all environments
>>>   available, be they main or secondary (it does not matter to save the
>>>   whole environment on secondary environments as only the whitelisted
>>>   variables will be overriden in the loading process,
>>>
>>> I have also a few questions that could help me to get the whole thing to
>>> work.
>>>
>>> 1) I can't really get my head around the use of gd->env_addr, what is it used
>>> for? It is set in a bunch of different places but only once is it
>>> explicitly used (basically to alternate the env_addr between the one
>>> associated to main and redundant environment (in NAND for example)).
>>>
>>> 2) Why do we consider ENV_IS_NOWHERE an invalid environment? The only place I
>>> found a use for it was to just say that if the environment is invalid, we
>>> should set to default environment (in env_relocate in env/common.c). With
>>> my patch series I guess that we could remove this fallback and force
>>> ENV_IS_NOWHERE to be always there.
>>>
>>> 3) There are a few (20) boards that set gd->env_addr and gd->env_valid in
>>> their board file. What is the reason to do such a thing? Isn't those
>>> overriden anyway by the environment driver?
>>>
>>> I'm looking forward to getting your feedback on this patch series.
>>>
>>
>> I certainly understand the goal and it seems generally useful.
>>
>> But I wonder whether this is the best way to implement it.
>>
>> We could have a UCLASS_ENV uclass, with driver-model drivers which
>> load the environment (i.e. have load() and save() methods). Config for
>> the drivers would come from the device tree. Useful drivers would be:
>>
>
> I'm not sure the device tree is the place to set/get such info. That has
> nothing to do with hardware, it's only logic for the bootloader.
>
>> - one that loads the env from a single location
>> - one that loads multiple envs from different locations in priority order
>> - one that does what you want
>>
>> Then people could set their own policy by adding a driver.
>>
>
> I'll have to document myself on driver-model and how U-Boot implement it
> then :)
>
>> I worry that what we have here is quite heavyweight, and will be
>> imposed on all users, e.g. the size increase of gd, the new logic.
>> Where does it end? I think splitting things up into different use
>> cases makes sense.
>>
>
> Agree on that.
>
>> When I did the env refactor I envisaged using driver-model for the
>> post-reloc env load/save at some point in the future. It solves the
>> problem of getting the config (can use device tree) and different
>> boards wanting to do different things (boards can provide an env
>> driver).
>>
>
> Overall, I prefer Lukasz's suggestion as it's quicker and easier to
> implement and require (I think) less changes in the code.

Do you mean selecting from different locations? Yes that is a good
thing, but is orthogonal to this series.

Here you are trying to add functionality which will apply to any env
location, or to them as a whole. So we should think of your change as
implementing a new policy rather than a new env location.

Regards,
Simon