[RESEND PATCH 00/10] env: ext4: add test for env in ext4

[RESEND PATCH 00/10] env: ext4: add test for env in ext4

[Patrick Delaunay]

Hi,

[RESEND] with correct serie title (sorry)

In this serie, I add sandbox test with CONFIG_ENV_IS_NOWHERE
activated with other location: at least one CONFIG_ENV_IS_IN_...
is defined and  ENV_IS_IN_DEVICE is automatically defined.

To test this feature, I activate and test ENV_IS_IN_EXT4
in sandbox; I add a new command "env_loc" to change this
ENV location.

This serie depends on previous env test introduced in:
"cmd: env: add option for quiet output on env info"
http://patchwork.ozlabs.org/project/uboot/list/?series=158105

To be able to test invalid file (bad CRC), I also add the support of
the command "env erase" for EXT4 env location.

Regards

Patrick



Patrick Delaunay (10):
  env: add absolute path at CONFIG_ENV_EXT4_FILE
  env: ext4: set gd->env_valid
  env: correctly handle result in env_init
  sandbox: activate env in ext4 support
  sandbox: support the change of env location
  test: environment in ext4
  env: ext4: fix possible compilation issue
  env: ext4: introduce new function env_ext4_save_buffer
  env: ext4: add support of command env erase
  test: sandbox: add test for erase command

 board/sandbox/sandbox.c            |  50 ++++++++++++++
 configs/sandbox64_defconfig        |   5 ++
 configs/sandbox_defconfig          |   5 ++
 configs/sandbox_flattree_defconfig |   5 ++
 configs/sandbox_spl_defconfig      |   5 ++
 env/Kconfig                        |   2 +-
 env/env.c                          |   5 +-
 env/ext4.c                         |  63 +++++++++++++++---
 test/py/tests/test_env.py          | 103 +++++++++++++++++++++++++++++
 9 files changed, 232 insertions(+), 11 deletions(-)

-- 
2.17.1

[RESEND PATCH 01/10] env: add absolute path at CONFIG_ENV_EXT4_FILE

[Patrick Delaunay]

Add the absolute path to the default value of
CONFIG_ENV_EXT4_FILE = "/uboot.env".

This patch avoid the error :
  Saving Environment to EXT4... File System is consistent
  Please supply Absolute path

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

For information, it is the value used today by all the boards:

dragonboard820c_defconfig:29:CONFIG_ENV_EXT4_FILE="/uboot.env"
hikey960_defconfig:25:CONFIG_ENV_EXT4_FILE="/uboot.env"
stm32mp15_basic_defconfig:64:CONFIG_ENV_EXT4_FILE="/uboot.env"
stm32mp15_optee_defconfig:51:CONFIG_ENV_EXT4_FILE="/uboot.env"
stm32mp15_trusted_defconfig:50:CONFIG_ENV_EXT4_FILE="/uboot.env"


 env/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/Kconfig b/env/Kconfig
index 0d6f559b39..8059749701 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -467,7 +467,7 @@ config ENV_EXT4_DEVICE_AND_PART
 config ENV_EXT4_FILE
 	string "Name of the EXT4 file to use for the environment"
 	depends on ENV_IS_IN_EXT4
-	default "uboot.env"
+	default "/uboot.env"
 	help
 	  It's a string of the EXT4 file name. This file use to store the
 	  environment (explicit path to the file)
-- 
2.17.1

[RESEND PATCH 02/10] env: ext4: set gd->env_valid

[Patrick Delaunay]

Add a missing initialization of gd->env_valid in env_ext4_load
as it is already done in some other env device.

Set gd->env_valid = ENV_VALID in env_ext4_save() and env_ext4_load().

This patch allows to have a correct information in 'env info' command.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 env/ext4.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/env/ext4.c b/env/ext4.c
index 1f6b1b5bd8..e3bbf4a4e0 100644
--- a/env/ext4.c
+++ b/env/ext4.c
@@ -31,6 +31,8 @@
 #include <ext4fs.h>
 #include <mmc.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 __weak const char *env_ext4_get_intf(void)
 {
 	return (const char *)CONFIG_ENV_EXT4_INTERFACE;
@@ -79,6 +81,7 @@ static int env_ext4_save(void)
 			CONFIG_ENV_EXT4_FILE, ifname, dev, part);
 		return 1;
 	}
+	gd->env_valid = ENV_VALID;
 
 	puts("done\n");
 	return 0;
@@ -125,7 +128,11 @@ static int env_ext4_load(void)
 		goto err_env_relocate;
 	}
 
-	return env_import(buf, 1);
+	err = env_import(buf, 1);
+	if (!err)
+		gd->env_valid = ENV_VALID;
+
+	return err;
 
 err_env_relocate:
 	env_set_default(NULL, 0);
-- 
2.17.1

[RESEND PATCH 03/10] env: correctly handle result in env_init

[Patrick Delaunay]

Don't return error with ret=-ENOENT when the optional ops drv->init
is absent but only if env_driver_lookup don't found driver.

This patch correct an issue for the code
  if (!env_init())
     env_load()
When only ext4 is supported (CONFIG_ENV_IS_IN_EXT4),
as the backend env/ext4.c doesn't define an ops .init

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 env/env.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/env/env.c b/env/env.c
index 9237bb9c74..e4df1715e4 100644
--- a/env/env.c
+++ b/env/env.c
@@ -292,7 +292,10 @@ int env_init(void)
 	int prio;
 
 	for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
-		if (!drv->init || !(ret = drv->init()))
+		ret = 0;
+		if (drv->init)
+			ret = drv->init();
+		if (!ret)
 			env_set_inited(drv->location);
 
 		debug("%s: Environment %s init done (ret=%d)\n", __func__,
-- 
2.17.1

[RESEND PATCH 04/10] sandbox: activate env in ext4 support

[Patrick Delaunay]

The default environment is still used with "ENVL_NOWHERE"
indicated by the weak function env_get_location() and
activated by CONFIG_ENV_IS_NOWHERE.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 board/sandbox/sandbox.c            | 12 ++++++++++++
 configs/sandbox64_defconfig        |  4 ++++
 configs/sandbox_defconfig          |  4 ++++
 configs/sandbox_flattree_defconfig |  4 ++++
 configs/sandbox_spl_defconfig      |  4 ++++
 5 files changed, 28 insertions(+)

diff --git a/board/sandbox/sandbox.c b/board/sandbox/sandbox.c
index 0c3d245dff..01f356be31 100644
--- a/board/sandbox/sandbox.c
+++ b/board/sandbox/sandbox.c
@@ -7,6 +7,7 @@
 #include <cpu_func.h>
 #include <cros_ec.h>
 #include <dm.h>
+#include <env_internal.h>
 #include <init.h>
 #include <led.h>
 #include <os.h>
@@ -44,6 +45,17 @@ unsigned long timer_read_counter(void)
 }
 #endif
 
+enum env_location env_get_location(enum env_operation op, int prio)
+{
+	/* only one location supported */
+	if (prio != 0)
+		return ENVL_UNKNOWN;
+
+	gd->env_load_prio = 0;
+
+	return ENVL_NOWHERE;
+}
+
 int dram_init(void)
 {
 	gd->ram_size = CONFIG_SYS_SDRAM_SIZE;
diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index cdcb0acbdc..6172259924 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -75,6 +75,10 @@ CONFIG_OF_CONTROL=y
 CONFIG_OF_LIVE=y
 CONFIG_OF_HOSTFILE=y
 CONFIG_DEFAULT_DEVICE_TREE="sandbox64"
+CONFIG_ENV_IS_NOWHERE=y
+CONFIG_ENV_IS_IN_EXT4=y
+CONFIG_ENV_EXT4_INTERFACE="host"
+CONFIG_ENV_EXT4_DEVICE_AND_PART="0:0"
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 33a103edab..28a6211189 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -84,6 +84,10 @@ CONFIG_OF_CONTROL=y
 CONFIG_OF_LIVE=y
 CONFIG_OF_HOSTFILE=y
 CONFIG_DEFAULT_DEVICE_TREE="sandbox"
+CONFIG_ENV_IS_NOWHERE=y
+CONFIG_ENV_IS_IN_EXT4=y
+CONFIG_ENV_EXT4_INTERFACE="host"
+CONFIG_ENV_EXT4_DEVICE_AND_PART="0:0"
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig
index 2bfbb66453..1324aaca37 100644
--- a/configs/sandbox_flattree_defconfig
+++ b/configs/sandbox_flattree_defconfig
@@ -59,6 +59,10 @@ CONFIG_AMIGA_PARTITION=y
 CONFIG_OF_CONTROL=y
 CONFIG_OF_HOSTFILE=y
 CONFIG_DEFAULT_DEVICE_TREE="sandbox"
+CONFIG_ENV_IS_NOWHERE=y
+CONFIG_ENV_IS_IN_EXT4=y
+CONFIG_ENV_EXT4_INTERFACE="host"
+CONFIG_ENV_EXT4_DEVICE_AND_PART="0:0"
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig
index 3bf27c974a..eadcdb9f43 100644
--- a/configs/sandbox_spl_defconfig
+++ b/configs/sandbox_spl_defconfig
@@ -76,6 +76,10 @@ CONFIG_SPL_OF_CONTROL=y
 CONFIG_OF_HOSTFILE=y
 CONFIG_DEFAULT_DEVICE_TREE="sandbox"
 CONFIG_SPL_OF_PLATDATA=y
+CONFIG_ENV_IS_NOWHERE=y
+CONFIG_ENV_IS_IN_EXT4=y
+CONFIG_ENV_EXT4_INTERFACE="host"
+CONFIG_ENV_EXT4_DEVICE_AND_PART="0:0"
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
-- 
2.17.1

[RESEND PATCH 05/10] sandbox: support the change of env location

[Patrick Delaunay]

Add support of environment location with a new sandbox command
'env_loc'.

When the user change the environment location with the command
'env_loc <location>' the env is reinitialized and saved;
the GD_FLG_ENV_DEFAULT flag is also updated.

When the user set the same env location, the environment is
re-loaded.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 board/sandbox/sandbox.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/board/sandbox/sandbox.c b/board/sandbox/sandbox.c
index 01f356be31..023a71d5f0 100644
--- a/board/sandbox/sandbox.c
+++ b/board/sandbox/sandbox.c
@@ -21,6 +21,9 @@
  */
 gd_t *gd;
 
+/* env location: current location used during test */
+static enum env_location sandbox_env_location = ENVL_NOWHERE;
+
 /* Add a simple GPIO device */
 U_BOOT_DEVICE(gpio_sandbox) = {
 	.name = "gpio_sandbox",
@@ -53,9 +56,44 @@ enum env_location env_get_location(enum env_operation op, int prio)
 
 	gd->env_load_prio = 0;
 
-	return ENVL_NOWHERE;
+	return sandbox_env_location;
 }
 
+static int do_env_loc(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	enum env_location loc;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	loc = (enum env_location)simple_strtoul(argv[1], NULL, 10);
+	if (loc >= ENVL_COUNT)
+		return CMD_RET_FAILURE;
+
+	if (sandbox_env_location != loc) {
+		sandbox_env_location = loc;
+		if (loc == ENVL_NOWHERE) {
+			gd->flags |= GD_FLG_ENV_DEFAULT;
+			gd->env_valid = ENV_VALID;
+		} else {
+			if (gd->flags & GD_FLG_ENV_DEFAULT) {
+				gd->flags &= ~GD_FLG_ENV_DEFAULT;
+				if (!env_init())
+					env_save();
+			}
+		}
+	} else {
+		if (!env_init())
+			env_load();
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+U_BOOT_CMD(env_loc, 2, 1, do_env_loc,
+	   "set the environment location", "[loc]"
+);
+
 int dram_init(void)
 {
 	gd->ram_size = CONFIG_SYS_SDRAM_SIZE;
-- 
2.17.1

[RESEND PATCH 06/10] test: environment in ext4

[Patrick Delaunay]

Add basic test to persistent environment in ext4:
save and load in host ext4 file 'uboot.env'.

On first execution a empty EXT4 file system is created in
persistent data dir: env.ext4.img.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 test/py/tests/test_env.py | 87 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
index cbdb41031c..d35ad888a7 100644
--- a/test/py/tests/test_env.py
+++ b/test/py/tests/test_env.py
@@ -4,6 +4,10 @@
 
 # Test operation of shell commands relating to environment variables.
 
+import os
+import os.path
+from subprocess import call, check_call
+
 import pytest
 import u_boot_utils
 
@@ -380,3 +384,86 @@ def test_env_info_quiet(state_test_env):
     assert response == ""
     response = c.run_command('echo $?')
     assert response == "1"
+
+def mk_env_ext4(state_test_env):
+    c = state_test_env.u_boot_console
+
+    """Create a empty ext4 file system volume.
+    """
+    filename = 'env.ext4.img'
+    persistent = c.config.persistent_data_dir + '/' + filename
+    fs_img = c.config.result_dir  + '/' + filename
+
+    if os.path.exists(persistent):
+        c.log.action('Disk image file ' + persistent + ' already exists')
+    else:
+        try:
+            check_call('rm -f %s' % persistent, shell=True)
+            check_call('dd if=/dev/zero of=%s bs=1M count=16'
+                % persistent, shell=True)
+            check_call('mkfs.ext4 -O ^metadata_csum %s' % persistent, shell=True)
+        except CalledProcessError:
+            call('rm -f %s' % persistent, shell=True)
+            raise
+
+    call('cp -f %s %s' % (persistent, fs_img), shell=True)
+    return fs_img
+
+ at pytest.mark.boardspec('sandbox')
+ at pytest.mark.buildconfigspec('cmd_nvedit_info')
+ at pytest.mark.buildconfigspec('cmd_echo')
+ at pytest.mark.buildconfigspec('env_is_in_ext4')
+def test_env_ext4(state_test_env):
+
+    c = state_test_env.u_boot_console
+
+    fs_img = mk_env_ext4(state_test_env)
+    c.run_command('host bind 0  %s' % fs_img)
+
+    response = c.run_command('ext4ls host 0:0')
+    assert 'uboot.env' not in response
+
+    """ env_location: ENVL_EXT4 (2)
+    """
+    response = c.run_command('env_loc 2')
+    assert 'Saving Environment to EXT4' in response
+
+    response = c.run_command('env_loc 2')
+    assert 'Loading Environment from EXT4... OK' in response
+
+    response = c.run_command('ext4ls host 0:0')
+    assert '8192 uboot.env' in response
+
+    response = c.run_command('env info')
+    assert 'env_valid = valid' in response
+    assert 'env_ready = true' in response
+    assert 'env_use_default = false' in response
+
+    response = c.run_command('env info -p -d')
+    assert 'Environment was loaded from persistent storage' in response
+    assert 'Environment can be persisted' in response
+
+    response = c.run_command('env info -d -q')
+    assert response == ""
+    response = c.run_command('echo $?')
+    assert response == "1"
+
+    response = c.run_command('env info -p -q')
+    assert response == ""
+    response = c.run_command('echo $?')
+    assert response == "0"
+
+    """ restore env_location: ENVL_NOWHERE (12)
+    """
+    c.run_command('env_loc 12')
+
+    response = c.run_command('env info')
+    assert 'env_valid = valid' in response
+    assert 'env_ready = true' in response
+    assert 'env_use_default = true' in response
+
+    response = c.run_command('env info -p -d')
+    assert 'Default environment is used' in response
+    assert 'Environment cannot be persisted' in response
+
+    call('rm -f %s' % fs_img, shell=True)
-- 
2.17.1

[RESEND PATCH 07/10] env: ext4: fix possible compilation issue

[Patrick Delaunay]

Fix possible compilation issue in env ext4 support when
CONFIG_CMD_SAVEENV is not activated.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 env/ext4.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/env/ext4.c b/env/ext4.c
index e3bbf4a4e0..aa77261649 100644
--- a/env/ext4.c
+++ b/env/ext4.c
@@ -144,5 +144,7 @@ U_BOOT_ENV_LOCATION(ext4) = {
 	.location	= ENVL_EXT4,
 	ENV_NAME("EXT4")
 	.load		= env_ext4_load,
+#ifdef CONFIG_CMD_SAVEENV
 	.save		= env_save_ptr(env_ext4_save),
+#endif
 };
-- 
2.17.1

[RESEND PATCH 08/10] env: ext4: introduce new function env_ext4_save_buffer

[Patrick Delaunay]

Split the function env_ext4_save to prepare the erase support

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 env/ext4.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/env/ext4.c b/env/ext4.c
index aa77261649..49ed06659f 100644
--- a/env/ext4.c
+++ b/env/ext4.c
@@ -43,10 +43,9 @@ __weak const char *env_ext4_get_dev_part(void)
 	return (const char *)CONFIG_ENV_EXT4_DEVICE_AND_PART;
 }
 
-#ifdef CONFIG_CMD_SAVEENV
-static int env_ext4_save(void)
+#if defined(CONFIG_CMD_SAVEENV)
+static int env_ext4_save_buffer(env_t *env_new)
 {
-	env_t	env_new;
 	struct blk_desc *dev_desc = NULL;
 	disk_partition_t info;
 	int dev, part;
@@ -54,10 +53,6 @@ static int env_ext4_save(void)
 	const char *ifname = env_ext4_get_intf();
 	const char *dev_and_part = env_ext4_get_dev_part();
 
-	err = env_export(&env_new);
-	if (err)
-		return err;
-
 	part = blk_get_device_part_str(ifname, dev_and_part,
 				       &dev_desc, &info, 1);
 	if (part < 0)
@@ -72,7 +67,7 @@ static int env_ext4_save(void)
 		return 1;
 	}
 
-	err = ext4fs_write(CONFIG_ENV_EXT4_FILE, (void *)&env_new,
+	err = ext4fs_write(CONFIG_ENV_EXT4_FILE, (void *)env_new,
 			   sizeof(env_t), FILETYPE_REG);
 	ext4fs_close();
 
@@ -81,9 +76,28 @@ static int env_ext4_save(void)
 			CONFIG_ENV_EXT4_FILE, ifname, dev, part);
 		return 1;
 	}
-	gd->env_valid = ENV_VALID;
 
+	return 0;
+}
+#endif
+
+#ifdef CONFIG_CMD_SAVEENV
+static int env_ext4_save(void)
+{
+	env_t	env_new;
+	int err;
+
+	err = env_export(&env_new);
+	if (err)
+		return err;
+
+	err = env_ext4_save_buffer(&env_new);
+	if (err)
+		return err;
+
+	gd->env_valid = ENV_VALID;
 	puts("done\n");
+
 	return 0;
 }
 #endif /* CONFIG_CMD_SAVEENV */
-- 
2.17.1

[RESEND PATCH 09/10] env: ext4: add support of command env erase

[Patrick Delaunay]

Add support of opts erase for env in ext4,
this opts is used by command 'env erase'.

This command only fill the env file (CONFIG_ENV_EXT4_FILE)
with 0, the CRC and the saved environment becomes invalid.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 env/ext4.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/env/ext4.c b/env/ext4.c
index 49ed06659f..aec2a33fad 100644
--- a/env/ext4.c
+++ b/env/ext4.c
@@ -43,7 +43,7 @@ __weak const char *env_ext4_get_dev_part(void)
 	return (const char *)CONFIG_ENV_EXT4_DEVICE_AND_PART;
 }
 
-#if defined(CONFIG_CMD_SAVEENV)
+#if defined(CONFIG_CMD_SAVEENV) || defined(CONFIG_CMD_ERASEENV)
 static int env_ext4_save_buffer(env_t *env_new)
 {
 	struct blk_desc *dev_desc = NULL;
@@ -102,6 +102,25 @@ static int env_ext4_save(void)
 }
 #endif /* CONFIG_CMD_SAVEENV */
 
+#if defined(CONFIG_CMD_ERASEENV)
+static int env_ext4_erase(void)
+{
+	env_t	env_new;
+	int err;
+
+	memset(&env_new, 0, sizeof(env_t));
+
+	err = env_ext4_save_buffer(&env_new);
+	if (err)
+		return err;
+
+	gd->env_valid = ENV_INVALID;
+	puts("done\n");
+
+	return 0;
+}
+#endif
+
 static int env_ext4_load(void)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
@@ -161,4 +180,7 @@ U_BOOT_ENV_LOCATION(ext4) = {
 #ifdef CONFIG_CMD_SAVEENV
 	.save		= env_save_ptr(env_ext4_save),
 #endif
+#if defined(CONFIG_CMD_ERASEENV)
+	.erase		= env_ext4_erase,
+#endif
 };
-- 
2.17.1

[RESEND PATCH 10/10] test: sandbox: add test for erase command

[Patrick Delaunay]

Add test for the erase command tested on ENV in EXT4.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 configs/sandbox64_defconfig        |  1 +
 configs/sandbox_defconfig          |  1 +
 configs/sandbox_flattree_defconfig |  1 +
 configs/sandbox_spl_defconfig      |  1 +
 test/py/tests/test_env.py          | 20 ++++++++++++++++++--
 5 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index 6172259924..a92dc957e8 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -25,6 +25,7 @@ CONFIG_CMD_BOOTZ=y
 # CONFIG_CMD_ELF is not set
 CONFIG_CMD_ASKENV=y
 CONFIG_CMD_GREPENV=y
+CONFIG_CMD_ERASEENV=y
 CONFIG_CMD_ENV_CALLBACK=y
 CONFIG_CMD_ENV_FLAGS=y
 CONFIG_CMD_NVEDIT_INFO=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 28a6211189..82a980e652 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -29,6 +29,7 @@ CONFIG_CMD_ABOOTIMG=y
 # CONFIG_CMD_ELF is not set
 CONFIG_CMD_ASKENV=y
 CONFIG_CMD_GREPENV=y
+CONFIG_CMD_ERASEENV=y
 CONFIG_CMD_ENV_CALLBACK=y
 CONFIG_CMD_ENV_FLAGS=y
 CONFIG_CMD_NVEDIT_INFO=y
diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig
index 1324aaca37..93d587fe38 100644
--- a/configs/sandbox_flattree_defconfig
+++ b/configs/sandbox_flattree_defconfig
@@ -22,6 +22,7 @@ CONFIG_CMD_BOOTZ=y
 # CONFIG_CMD_ELF is not set
 CONFIG_CMD_ASKENV=y
 CONFIG_CMD_GREPENV=y
+CONFIG_CMD_ERASEENV=y
 CONFIG_CMD_NVEDIT_INFO=y
 CONFIG_LOOPW=y
 CONFIG_CMD_MD5SUM=y
diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig
index eadcdb9f43..2337eade06 100644
--- a/configs/sandbox_spl_defconfig
+++ b/configs/sandbox_spl_defconfig
@@ -31,6 +31,7 @@ CONFIG_CMD_BOOTZ=y
 # CONFIG_CMD_ELF is not set
 CONFIG_CMD_ASKENV=y
 CONFIG_CMD_GREPENV=y
+CONFIG_CMD_ERASEENV=y
 CONFIG_CMD_ENV_CALLBACK=y
 CONFIG_CMD_ENV_FLAGS=y
 CONFIG_CMD_NVEDIT_INFO=y
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
index d35ad888a7..a71b4c2571 100644
--- a/test/py/tests/test_env.py
+++ b/test/py/tests/test_env.py
@@ -423,7 +423,7 @@ def test_env_ext4(state_test_env):
     response = c.run_command('ext4ls host 0:0')
     assert 'uboot.env' not in response
 
-    """ env_location: ENVL_EXT4 (2)
+    """ env location: ENVL_EXT4 (2)
     """
     response = c.run_command('env_loc 2')
     assert 'Saving Environment to EXT4' in response
@@ -453,7 +453,23 @@ def test_env_ext4(state_test_env):
     response = c.run_command('echo $?')
     assert response == "0"
 
-    """ restore env_location: ENVL_NOWHERE (12)
+    response = c.run_command('env erase')
+    assert 'OK' in response
+
+    response = c.run_command('env_loc 2')
+    assert 'Loading Environment from EXT4... ' in response
+    assert 'bad CRC, using default environment' in response
+
+    response = c.run_command('env info')
+    assert 'env_valid = invalid' in response
+    assert 'env_ready = true' in response
+    assert 'env_use_default = true' in response
+
+    response = c.run_command('env info -p -d')
+    assert 'Default environment is used' in response
+    assert 'Environment can be persisted' in response
+
+    """ restore env location: ENVL_NOWHERE (12)
     """
     c.run_command('env_loc 12')
 
-- 
2.17.1

[RESEND PATCH 04/10] sandbox: activate env in ext4 support

[Simon Glass]

On Wed, 12 Feb 2020 at 11:45, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> The default environment is still used with "ENVL_NOWHERE"
> indicated by the weak function env_get_location() and
> activated by CONFIG_ENV_IS_NOWHERE.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  board/sandbox/sandbox.c            | 12 ++++++++++++
>  configs/sandbox64_defconfig        |  4 ++++
>  configs/sandbox_defconfig          |  4 ++++
>  configs/sandbox_flattree_defconfig |  4 ++++
>  configs/sandbox_spl_defconfig      |  4 ++++
>  5 files changed, 28 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

At some point I'd really like the environment device/info to be in the
device tree.

[RESEND PATCH 05/10] sandbox: support the change of env location

[Simon Glass]

Hi Patrick,

On Wed, 12 Feb 2020 at 11:45, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> Add support of environment location with a new sandbox command
> 'env_loc'.
>
> When the user change the environment location with the command
> 'env_loc <location>' the env is reinitialized and saved;
> the GD_FLG_ENV_DEFAULT flag is also updated.
>
> When the user set the same env location, the environment is
> re-loaded.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  board/sandbox/sandbox.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/board/sandbox/sandbox.c b/board/sandbox/sandbox.c
> index 01f356be31..023a71d5f0 100644
> --- a/board/sandbox/sandbox.c
> +++ b/board/sandbox/sandbox.c
> @@ -21,6 +21,9 @@
>   */
>  gd_t *gd;
>
> +/* env location: current location used during test */
> +static enum env_location sandbox_env_location = ENVL_NOWHERE;
> +
>  /* Add a simple GPIO device */
>  U_BOOT_DEVICE(gpio_sandbox) = {
>         .name = "gpio_sandbox",
> @@ -53,9 +56,44 @@ enum env_location env_get_location(enum env_operation op, int prio)
>
>         gd->env_load_prio = 0;
>
> -       return ENVL_NOWHERE;
> +       return sandbox_env_location;
>  }
>
> +static int do_env_loc(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])

This should be a sub-command of 'env' I think.

Regards,
Simon

[RESEND PATCH 10/10] test: sandbox: add test for erase command

[Simon Glass]

Hi Patrick,

On Wed, 12 Feb 2020 at 11:45, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> Add test for the erase command tested on ENV in EXT4.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  configs/sandbox64_defconfig        |  1 +
>  configs/sandbox_defconfig          |  1 +
>  configs/sandbox_flattree_defconfig |  1 +
>  configs/sandbox_spl_defconfig      |  1 +
>  test/py/tests/test_env.py          | 20 ++++++++++++++++++--
>  5 files changed, 22 insertions(+), 2 deletions(-)

Can you write this test in C?

See ut_check_console_line() in mainline which should help if you need
to check console output.

Regards,
Simon

[RESEND PATCH 06/10] test: environment in ext4

[Stephen Warren]

On 2/12/20 11:44 AM, Patrick Delaunay wrote:
> Add basic test to persistent environment in ext4:
> save and load in host ext4 file 'uboot.env'.
> 
> On first execution a empty EXT4 file system is created in
> persistent data dir: env.ext4.img.

> diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py

> +def mk_env_ext4(state_test_env):
> +    c = state_test_env.u_boot_console
> +
> +    """Create a empty ext4 file system volume.
> +    """
> +    filename = 'env.ext4.img'
> +    persistent = c.config.persistent_data_dir + '/' + filename

The test seems to want to delete this file at the end, and re-create it 
each time. Why store it in the persistent data directory in that case?

> +    if os.path.exists(persistent):
> +        c.log.action('Disk image file ' + persistent + ' already exists')

... and why just delete it and re-create it if a stale copy exists?

> +    else:
> +        try:
> +            check_call('rm -f %s' % persistent, shell=True)

why not:

     check_call('rm -f ' + persistent)

That has simpler string operations, and I don't think any of these 
commands need shell=True, since they don't contain any shell 
metacharacters? Same comment for most of the other commands.

> +def test_env_ext4(state_test_env):

> +    """ env_location: ENVL_EXT4 (2)
> +    """

I think that'd be better as a regular comment:

     # env_location: ENVL_EXT4 (2)

If there's a reason to make it a docstring, lets put the trailing """ on 
the same line since it's a short piece of text.

> +    response = c.run_command('env_loc 2')
> +    assert 'Saving Environment to EXT4' in response
> +
> +    response = c.run_command('env_loc 2')
> +    assert 'Loading Environment from EXT4... OK' in response

Can't both assert invocations run on the same response? Or, does the 
env_loc command do different things based on some other state; that 
would feel pretty nasty.

[RESEND PATCH 10/10] test: sandbox: add test for erase command

[Stephen Warren]

On 2/12/20 11:45 AM, Patrick Delaunay wrote:
> Add test for the erase command tested on ENV in EXT4.

> -    """ env_location: ENVL_EXT4 (2)
> +    """ env location: ENVL_EXT4 (2)

> -    """ restore env_location: ENVL_NOWHERE (12)
> +    """ restore env location: ENVL_NOWHERE (12)

Was there a whitespace mistake in the earlier path or something? I can't 
see a diff here...