[PATCH v2 0/9] env: ext4: corrections and add test for env in ext4

[PATCH v2 0/9] env: ext4: corrections and add test for env in ext4

[Patrick Delaunay]

Hi,

V2 is only a rebase and adaptation of the serie [1].

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 [2]
"cmd: env: add option for quiet output on env info"

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

[1] http://patchwork.ozlabs.org/project/uboot/list/?series=158160
[2] http://patchwork.ozlabs.org/project/uboot/list/?series=183438

Regards

Patrick


Changes in v2:
- change cmd_tbl_t to struct cmd_tbl
- use CONFIG_IS_ENABLED to set .erase (same as .save)

Patrick Delaunay (9):
  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: 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            |  52 +++++++++++++++
 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                         |  54 ++++++++++++---
 test/py/tests/test_env.py          | 103 +++++++++++++++++++++++++++++
 9 files changed, 226 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v2 1/9] 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_trusted_defconfig:50:CONFIG_ENV_EXT4_FILE="/uboot.env"


(no changes since v1)

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

diff --git a/env/Kconfig b/env/Kconfig
index ca7fef682b..9dad1cf8c1 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -469,7 +469,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

[PATCH v2 1/9] env: add absolute path at CONFIG_ENV_EXT4_FILE

[Tom Rini]

On Tue, Jun 16, 2020 at 09:40:40AM +0200, Patrick Delaunay wrote:

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

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200618/49f28b4f/attachment.sig>

[PATCH v2 2/9] 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>
---

(no changes since v1)

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

diff --git a/env/ext4.c b/env/ext4.c
index 8e90bb71b7..ac9f126bec 100644
--- a/env/ext4.c
+++ b/env/ext4.c
@@ -32,6 +32,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;
@@ -124,7 +127,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

[PATCH v2 2/9] env: ext4: set gd->env_valid

[Tom Rini]

On Tue, Jun 16, 2020 at 09:40:41AM +0200, Patrick Delaunay wrote:

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

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200618/827490d9/attachment.sig>

[PATCH v2 3/9] 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 doesn'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>
---

(no changes since v1)

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

diff --git a/env/env.c b/env/env.c
index dcc25c030b..819c88f729 100644
--- a/env/env.c
+++ b/env/env.c
@@ -295,7 +295,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

[PATCH v2 3/9] env: correctly handle result in env_init

[Tom Rini]

On Tue, Jun 16, 2020 at 09:40:42AM +0200, Patrick Delaunay wrote:

> Don't return error with ret=-ENOENT when the optional ops drv->init
> is absent but only if env_driver_lookup doesn'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>
> ---
> 
> (no changes since v1)
> 
>  env/env.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/env/env.c b/env/env.c
> index dcc25c030b..819c88f729 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -295,7 +295,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__,

I'm adding in Marek here because this reminds me of similar questions /
concerns I had looking in to his series.  At root, I think we're not
being consistent in each of our env backing implementations about where
flags such as ENV_VALID are set, and return values / checks of
functions.

Just outside of the start of the patch context here, we set ret to
-ENOENT and just past this, if still -ENOENT we say ENV_VALID and point
at the default environment.

But, I don't follow the patch commit message here.  If we don't have
drv->init we call env_set_inited(drv->location) but we won't have change
ret to 0, which means that later on down the function we go back to
default environment.

So isn't this a problem in most environment cases then?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200618/e4e8eac0/attachment.sig>

[PATCH v2 3/9] env: correctly handle result in env_init

[Patrick DELAUNAY]

Hi Tom and Marek,

> From: Tom Rini <trini@konsulko.com>
> Sent: jeudi 18 juin 2020 21:16
> 
> On Tue, Jun 16, 2020 at 09:40:42AM +0200, Patrick Delaunay wrote:
> 
> > Don't return error with ret=-ENOENT when the optional ops drv->init is
> > absent but only if env_driver_lookup doesn'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>
> > ---
> >
> > (no changes since v1)
> >
> >  env/env.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/env/env.c b/env/env.c
> > index dcc25c030b..819c88f729 100644
> > --- a/env/env.c
> > +++ b/env/env.c
> > @@ -295,7 +295,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__,
> 
> I'm adding in Marek here because this reminds me of similar questions / concerns
> I had looking in to his series.  At root, I think we're not being consistent in each of
> our env backing implementations about where flags such as ENV_VALID are set,
> and return values / checks of functions.
> 
> Just outside of the start of the patch context here, we set ret to -ENOENT and just
> past this, if still -ENOENT we say ENV_VALID and point at the default
> environment.
> 
> But, I don't follow the patch commit message here.  If we don't have
> drv->init we call env_set_inited(drv->location) but we won't have change
> ret to 0, which means that later on down the function we go back to default
> environment.

The cause of the issue is because the init() ops is optional in "struct env_driver".

When this opt is absent, I assume that the initialization is not mandatory but
this inititialization need to be tagged in gd->env_has_init with the call of
env_set_inited() function 

And the ENV backend is FOUND (don't return -ENOENT)

else the next call of env_has_inited(drv->location) always failed : in env_load()

I see the error  in EXT4 env backend,.all the other backend as a env_init() function

But some othe backend don't define the .init operation and have the issue

eeprom.c:235:U_BOOT_ENV_LOCATION(eeprom) = {
ext4.c:135:U_BOOT_ENV_LOCATION(ext4) = {
fat.c:128:U_BOOT_ENV_LOCATION(fat) = { 
mmc.c:393:U_BOOT_ENV_LOCATION(mmc) = {
onenand.c:108:U_BOOT_ENV_LOCATION(onenand) = {
sata.c:117:U_BOOT_ENV_LOCATION(sata) = { 
ubi.c:179:U_BOOT_ENV_LOCATION(ubi) = {

The other don't have issue:

flash.c:358:U_BOOT_ENV_LOCATION(flash) = {
flash.c:368:	.init		= env_flash_init,
nand.c:382:U_BOOT_ENV_LOCATION(nand) = {
nand.c:389:	.init		= env_nand_init,
nowhere.c:30:U_BOOT_ENV_LOCATION(nowhere) = {
nowhere.c:32:	.init		= env_nowhere_init,
nvram.c:117:U_BOOT_ENV_LOCATION(nvram) = {
nvram.c:122:	.init		= env_nvram_init,
remote.c:54:U_BOOT_ENV_LOCATION(remote) = {
remote.c:59:	.init		= env_remote_init,
sf.c:306:U_BOOT_ENV_LOCATION(sf) = {
sf.c:312:	.init		= env_sf_init,

> So isn't this a problem in most environment cases then?  Thanks!

I don't sure which environment configuration can case issue (only one ENV without drc->init() function)
But no issue if at least one CONFIG_ENV_IS_ is activated with driver implementing init ops 

But I see the issue in SANDBOX when I activate EXT4 only target. (CONFIG_ENV_IS_IN_EXT4), 
And no more issue when I add CONFIG_ENV_IS_NOWHERE.

PS: no direct issue if env_init result is not checked
       but I check this result in the sandbox tests in next patches:
	if (!env_init())
	     env_load()
 
       but anyway inconsistent value of gd->env_has_init 
       which can be a problem for any env_has_inited() calls


Regards

Patrick

[PATCH v2 3/9] env: correctly handle result in env_init

[Tom Rini]

On Fri, Jun 19, 2020 at 02:14:00PM +0000, Patrick DELAUNAY wrote:
> Hi Tom and Marek,
> 
> > From: Tom Rini <trini@konsulko.com>
> > Sent: jeudi 18 juin 2020 21:16
> > 
> > On Tue, Jun 16, 2020 at 09:40:42AM +0200, Patrick Delaunay wrote:
> > 
> > > Don't return error with ret=-ENOENT when the optional ops drv->init is
> > > absent but only if env_driver_lookup doesn'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>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  env/env.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/env/env.c b/env/env.c
> > > index dcc25c030b..819c88f729 100644
> > > --- a/env/env.c
> > > +++ b/env/env.c
> > > @@ -295,7 +295,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__,
> > 
> > I'm adding in Marek here because this reminds me of similar questions / concerns
> > I had looking in to his series.  At root, I think we're not being consistent in each of
> > our env backing implementations about where flags such as ENV_VALID are set,
> > and return values / checks of functions.
> > 
> > Just outside of the start of the patch context here, we set ret to -ENOENT and just
> > past this, if still -ENOENT we say ENV_VALID and point at the default
> > environment.
> > 
> > But, I don't follow the patch commit message here.  If we don't have
> > drv->init we call env_set_inited(drv->location) but we won't have change
> > ret to 0, which means that later on down the function we go back to default
> > environment.
> 
> The cause of the issue is because the init() ops is optional in "struct env_driver".

Right.

> When this opt is absent, I assume that the initialization is not mandatory but
> this inititialization need to be tagged in gd->env_has_init with the call of
> env_set_inited() function 

So when drv->init isn't set we are already calling env_set_inited(...).
If that's not the case, what's going on?

> And the ENV backend is FOUND (don't return -ENOENT)
> 
> else the next call of env_has_inited(drv->location) always failed : in env_load()
> 
> I see the error  in EXT4 env backend,.all the other backend as a env_init() function
> 
> But some othe backend don't define the .init operation and have the issue
> 
> eeprom.c:235:U_BOOT_ENV_LOCATION(eeprom) = {
> ext4.c:135:U_BOOT_ENV_LOCATION(ext4) = {
> fat.c:128:U_BOOT_ENV_LOCATION(fat) = { 
> mmc.c:393:U_BOOT_ENV_LOCATION(mmc) = {
> onenand.c:108:U_BOOT_ENV_LOCATION(onenand) = {
> sata.c:117:U_BOOT_ENV_LOCATION(sata) = { 
> ubi.c:179:U_BOOT_ENV_LOCATION(ubi) = {
> 
> The other don't have issue:
> 
> flash.c:358:U_BOOT_ENV_LOCATION(flash) = {
> flash.c:368:	.init		= env_flash_init,
> nand.c:382:U_BOOT_ENV_LOCATION(nand) = {
> nand.c:389:	.init		= env_nand_init,
> nowhere.c:30:U_BOOT_ENV_LOCATION(nowhere) = {
> nowhere.c:32:	.init		= env_nowhere_init,
> nvram.c:117:U_BOOT_ENV_LOCATION(nvram) = {
> nvram.c:122:	.init		= env_nvram_init,
> remote.c:54:U_BOOT_ENV_LOCATION(remote) = {
> remote.c:59:	.init		= env_remote_init,
> sf.c:306:U_BOOT_ENV_LOCATION(sf) = {
> sf.c:312:	.init		= env_sf_init,

Right, there should be a problem showing up on a ton of locations, not
just ext4 which is why I'm concerned / confused here.  While ext4 isn't
as widely used yet as I would expect, FAT/MMC are.

> > So isn't this a problem in most environment cases then?  Thanks!
> 
> I don't sure which environment configuration can case issue (only one ENV without drc->init() function)
> But no issue if at least one CONFIG_ENV_IS_ is activated with driver implementing init ops 
> 
> But I see the issue in SANDBOX when I activate EXT4 only target. (CONFIG_ENV_IS_IN_EXT4), 
> And no more issue when I add CONFIG_ENV_IS_NOWHERE.
> 
> PS: no direct issue if env_init result is not checked
>        but I check this result in the sandbox tests in next patches:
> 	if (!env_init())
> 	     env_load()
>  
>        but anyway inconsistent value of gd->env_has_init 
>        which can be a problem for any env_has_inited() calls

Right.  I think there's some bigger inconsistency going on here that
needs to be fixed.  I'm also confused / concerned how you're not seeing
env_set_inite(..) being called.  if (!NULL) is true.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200619/3bde9267/attachment.sig>

[PATCH v2 3/9] env: correctly handle result in env_init

[Patrick DELAUNAY]

Hi Tom,

> From: Tom Rini <trini@konsulko.com>
> Sent: vendredi 19 juin 2020 20:05
> 
> On Fri, Jun 19, 2020 at 02:14:00PM +0000, Patrick DELAUNAY wrote:
> > Hi Tom and Marek,
> >
> > > From: Tom Rini <trini@konsulko.com>
> > > Sent: jeudi 18 juin 2020 21:16
> > >
> > > On Tue, Jun 16, 2020 at 09:40:42AM +0200, Patrick Delaunay wrote:
> > >
> > > > Don't return error with ret=-ENOENT when the optional ops
> > > > drv->init is absent but only if env_driver_lookup doesn'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>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  env/env.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/env/env.c b/env/env.c index dcc25c030b..819c88f729
> > > > 100644
> > > > --- a/env/env.c
> > > > +++ b/env/env.c
> > > > @@ -295,7 +295,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__,
> > >
> > > I'm adding in Marek here because this reminds me of similar
> > > questions / concerns I had looking in to his series.  At root, I
> > > think we're not being consistent in each of our env backing
> > > implementations about where flags such as ENV_VALID are set, and return
> values / checks of functions.
> > >
> > > Just outside of the start of the patch context here, we set ret to
> > > -ENOENT and just past this, if still -ENOENT we say ENV_VALID and
> > > point at the default environment.
> > >
> > > But, I don't follow the patch commit message here.  If we don't have
> > > drv->init we call env_set_inited(drv->location) but we won't have
> > > drv->change
> > > ret to 0, which means that later on down the function we go back to
> > > default environment.
> >
> > The cause of the issue is because the init() ops is optional in "struct
> env_driver".
> 
> Right.
> 
> > When this opt is absent, I assume that the initialization is not
> > mandatory but this inititialization need to be tagged in
> > gd->env_has_init with the call of
> > env_set_inited() function
> 
> So when drv->init isn't set we are already calling env_set_inited(...).
> If that's not the case, what's going on?
> 
> > And the ENV backend is FOUND (don't return -ENOENT)
> >
> > else the next call of env_has_inited(drv->location) always failed : in
> > env_load()
> >
> > I see the error  in EXT4 env backend,.all the other backend as a
> > env_init() function
> >
> > But some othe backend don't define the .init operation and have the
> > issue
> >
> > eeprom.c:235:U_BOOT_ENV_LOCATION(eeprom) = {
> > ext4.c:135:U_BOOT_ENV_LOCATION(ext4) = {
> > fat.c:128:U_BOOT_ENV_LOCATION(fat) = {
> > mmc.c:393:U_BOOT_ENV_LOCATION(mmc) = {
> > onenand.c:108:U_BOOT_ENV_LOCATION(onenand) = {
> > sata.c:117:U_BOOT_ENV_LOCATION(sata) = {
> > ubi.c:179:U_BOOT_ENV_LOCATION(ubi) = {
> >
> > The other don't have issue:
> >
> > flash.c:358:U_BOOT_ENV_LOCATION(flash) = {
> > flash.c:368:	.init		= env_flash_init,
> > nand.c:382:U_BOOT_ENV_LOCATION(nand) = {
> > nand.c:389:	.init		= env_nand_init,
> > nowhere.c:30:U_BOOT_ENV_LOCATION(nowhere) = {
> > nowhere.c:32:	.init		= env_nowhere_init,
> > nvram.c:117:U_BOOT_ENV_LOCATION(nvram) = {
> > nvram.c:122:	.init		= env_nvram_init,
> > remote.c:54:U_BOOT_ENV_LOCATION(remote) = {
> > remote.c:59:	.init		= env_remote_init,
> > sf.c:306:U_BOOT_ENV_LOCATION(sf) = {
> > sf.c:312:	.init		= env_sf_init,
> 
> Right, there should be a problem showing up on a ton of locations, not just ext4
> which is why I'm concerned / confused here.  While ext4 isn't as widely used yet
> as I would expect, FAT/MMC are.
> 
> > > So isn't this a problem in most environment cases then?  Thanks!
> >
> > I don't sure which environment configuration can case issue (only one
> > ENV without drc->init() function) But no issue if at least one
> > CONFIG_ENV_IS_ is activated with driver implementing init ops
> >
> > But I see the issue in SANDBOX when I activate EXT4 only target.
> > (CONFIG_ENV_IS_IN_EXT4), And no more issue when I add
> CONFIG_ENV_IS_NOWHERE.
> >
> > PS: no direct issue if env_init result is not checked
> >        but I check this result in the sandbox tests in next patches:
> > 	if (!env_init())
> > 	     env_load()
> >
> >        but anyway inconsistent value of gd->env_has_init
> >        which can be a problem for any env_has_inited() calls
> 
> Right.  I think there's some bigger inconsistency going on here that needs to be
> fixed.  I'm also confused / concerned how you're not seeing
> env_set_inite(..) being called.  if (!NULL) is true.  Thanks!

I  was confused also...
and I check again the code

And I make a error in my first analysis.

For the case where init ops is not defined in only one backend.

	ret = -ENOENT

And the last test is true

	if (ret == -ENOENT) {
		gd->env_addr = (ulong)&default_environment[0];
		gd->env_valid = ENV_VALID;

		return 0;
	}

In  fact this function return the LAST result for 'drv->init()' call and
that it is strange when several backend are activated.

So this function have no issue when only one ENV backend is activated,
and it is the configuration today for most of boards...

I will change my patch in the V3 serie:
env_init() return 0 if, at least, one backend is correctly initialized
(when no ops init  or the ops init result is OK)
 
But I don't understood 2 thinks in the last test

1/ Why set ENV Is set to VALID:

	gd->env_valid = ENV_VALID;

	in nowhere backend, for same case of default env, it is set ENV_INVALID....

2/ Why flags is not update
 
	gd->flags |= GD_FLG_ENV_DEFAULT;


But as it s error case, in should never occurs 
And I will sent a separate patch for this part to review.

Patrick

[PATCH v2 3/9] env: correctly handle result in env_init

[Tom Rini]

On Tue, Jun 23, 2020 at 01:13:55PM +0000, Patrick DELAUNAY wrote:
> Hi Tom,
> 
> > From: Tom Rini <trini@konsulko.com>
> > Sent: vendredi 19 juin 2020 20:05
> > 
> > On Fri, Jun 19, 2020 at 02:14:00PM +0000, Patrick DELAUNAY wrote:
> > > Hi Tom and Marek,
> > >
> > > > From: Tom Rini <trini@konsulko.com>
> > > > Sent: jeudi 18 juin 2020 21:16
> > > >
> > > > On Tue, Jun 16, 2020 at 09:40:42AM +0200, Patrick Delaunay wrote:
> > > >
> > > > > Don't return error with ret=-ENOENT when the optional ops
> > > > > drv->init is absent but only if env_driver_lookup doesn'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>
> > > > > ---
> > > > >
> > > > > (no changes since v1)
> > > > >
> > > > >  env/env.c | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/env/env.c b/env/env.c index dcc25c030b..819c88f729
> > > > > 100644
> > > > > --- a/env/env.c
> > > > > +++ b/env/env.c
> > > > > @@ -295,7 +295,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__,
> > > >
> > > > I'm adding in Marek here because this reminds me of similar
> > > > questions / concerns I had looking in to his series.  At root, I
> > > > think we're not being consistent in each of our env backing
> > > > implementations about where flags such as ENV_VALID are set, and return
> > values / checks of functions.
> > > >
> > > > Just outside of the start of the patch context here, we set ret to
> > > > -ENOENT and just past this, if still -ENOENT we say ENV_VALID and
> > > > point at the default environment.
> > > >
> > > > But, I don't follow the patch commit message here.  If we don't have
> > > > drv->init we call env_set_inited(drv->location) but we won't have
> > > > drv->change
> > > > ret to 0, which means that later on down the function we go back to
> > > > default environment.
> > >
> > > The cause of the issue is because the init() ops is optional in "struct
> > env_driver".
> > 
> > Right.
> > 
> > > When this opt is absent, I assume that the initialization is not
> > > mandatory but this inititialization need to be tagged in
> > > gd->env_has_init with the call of
> > > env_set_inited() function
> > 
> > So when drv->init isn't set we are already calling env_set_inited(...).
> > If that's not the case, what's going on?
> > 
> > > And the ENV backend is FOUND (don't return -ENOENT)
> > >
> > > else the next call of env_has_inited(drv->location) always failed : in
> > > env_load()
> > >
> > > I see the error  in EXT4 env backend,.all the other backend as a
> > > env_init() function
> > >
> > > But some othe backend don't define the .init operation and have the
> > > issue
> > >
> > > eeprom.c:235:U_BOOT_ENV_LOCATION(eeprom) = {
> > > ext4.c:135:U_BOOT_ENV_LOCATION(ext4) = {
> > > fat.c:128:U_BOOT_ENV_LOCATION(fat) = {
> > > mmc.c:393:U_BOOT_ENV_LOCATION(mmc) = {
> > > onenand.c:108:U_BOOT_ENV_LOCATION(onenand) = {
> > > sata.c:117:U_BOOT_ENV_LOCATION(sata) = {
> > > ubi.c:179:U_BOOT_ENV_LOCATION(ubi) = {
> > >
> > > The other don't have issue:
> > >
> > > flash.c:358:U_BOOT_ENV_LOCATION(flash) = {
> > > flash.c:368:	.init		= env_flash_init,
> > > nand.c:382:U_BOOT_ENV_LOCATION(nand) = {
> > > nand.c:389:	.init		= env_nand_init,
> > > nowhere.c:30:U_BOOT_ENV_LOCATION(nowhere) = {
> > > nowhere.c:32:	.init		= env_nowhere_init,
> > > nvram.c:117:U_BOOT_ENV_LOCATION(nvram) = {
> > > nvram.c:122:	.init		= env_nvram_init,
> > > remote.c:54:U_BOOT_ENV_LOCATION(remote) = {
> > > remote.c:59:	.init		= env_remote_init,
> > > sf.c:306:U_BOOT_ENV_LOCATION(sf) = {
> > > sf.c:312:	.init		= env_sf_init,
> > 
> > Right, there should be a problem showing up on a ton of locations, not just ext4
> > which is why I'm concerned / confused here.  While ext4 isn't as widely used yet
> > as I would expect, FAT/MMC are.
> > 
> > > > So isn't this a problem in most environment cases then?  Thanks!
> > >
> > > I don't sure which environment configuration can case issue (only one
> > > ENV without drc->init() function) But no issue if at least one
> > > CONFIG_ENV_IS_ is activated with driver implementing init ops
> > >
> > > But I see the issue in SANDBOX when I activate EXT4 only target.
> > > (CONFIG_ENV_IS_IN_EXT4), And no more issue when I add
> > CONFIG_ENV_IS_NOWHERE.
> > >
> > > PS: no direct issue if env_init result is not checked
> > >        but I check this result in the sandbox tests in next patches:
> > > 	if (!env_init())
> > > 	     env_load()
> > >
> > >        but anyway inconsistent value of gd->env_has_init
> > >        which can be a problem for any env_has_inited() calls
> > 
> > Right.  I think there's some bigger inconsistency going on here that needs to be
> > fixed.  I'm also confused / concerned how you're not seeing
> > env_set_inite(..) being called.  if (!NULL) is true.  Thanks!
> 
> I  was confused also...
> and I check again the code

Thanks.  It's a very tricky section of the code and I think I got some
part of it wrong too.  What got me off-track was unrolling the test
isn't required in what you tried, just adding:
	ret = 0; // We found at least one env exits.
before if (!drv->init || !(ret = drv->init()))
would do the same.  That said...

> And I make a error in my first analysis.
> 
> For the case where init ops is not defined in only one backend.
> 
> 	ret = -ENOENT
> 
> And the last test is true
> 
> 	if (ret == -ENOENT) {
> 		gd->env_addr = (ulong)&default_environment[0];
> 		gd->env_valid = ENV_VALID;
> 
> 		return 0;
> 	}
> 
> In  fact this function return the LAST result for 'drv->init()' call and
> that it is strange when several backend are activated.

Yes.  Things are very fragile in the multi-env case.  There being
underlying bugs would not surprise me.

> So this function have no issue when only one ENV backend is activated,
> and it is the configuration today for most of boards...

Right, the common case is the way things have worked historically, env
exists in one defined location and when that fails (device not
present), we get the built-in environment and saveenv needs to fail
rather than crash the board (due to writing to non-existent HW, etc).

> I will change my patch in the V3 serie:
> env_init() return 0 if, at least, one backend is correctly initialized
> (when no ops init  or the ops init result is OK)
>  
> But I don't understood 2 thinks in the last test
> 
> 1/ Why set ENV Is set to VALID:
> 
> 	gd->env_valid = ENV_VALID;
> 
> 	in nowhere backend, for same case of default env, it is set ENV_INVALID....

A good question.  I see:
commit eeba55cb4a8a29a47d0d26692c188b47ba6bf396
Author: Tom Rini <trini@konsulko.com>
Date:   Sat Aug 19 22:27:57 2017 -0400

    env: Correct case of no sub-init function

changed that from setting it to 0 to setting it to ENV_VALID, where 0
meant ENV_INVALID at the time, but the comments in that commit say it
used to be setting it to valid.  Which brings us back to:
commit 203e94f6c9ca03e260175ce240f5856507395585
Author: Simon Glass <sjg@chromium.org>
Date:   Thu Aug 3 12:21:56 2017 -0600

    env: Add an enum for environment state

as adding a enum for 0 = ENV_INVALID, 1 = ENV_VALID, 2 = redundant.

And there's basically part of a longer series to clean up the
environment code.  In particular:
commit 7938822a6b75b69fff9793b6b1769dddf1249525
Author: Simon Glass <sjg@chromium.org>
Date:   Thu Aug 3 12:22:02 2017 -0600

    env: Drop common init() function

is the root of this particular question.  Before this change, the logic
was "For the init function in of an env location, point at the in-memory
default environment, declare it valid".  With this change the new common env init
function (now env_init() then env_init_new() as functionality migrated
to it) when we did not have an explicit drv->init() call we need to
point at the in-memory default environment and (was wrong at the time,
later fixed to...) mark env as valid.

So today, ret = -ENOENT is for the case of no explicit drv->init()
function so do what those env drivers were doing before of just pointing
gd->env_addr to the default environment and mark it valid.

What we're doing today works but feels too clever, given the amount of
time it takes to dig in to see what is supposed to be going on.

In the case of one environment location, this logic works as expected.
In the case of multiple environments, I would need to write down all of
the possible combinations and walk the code to be sure what happens,
especially since it's link-order dependent on the order we try drivers
in.

> 2/ Why flags is not update
>  
> 	gd->flags |= GD_FLG_ENV_DEFAULT;
> 
> 
> But as it s error case, in should never occurs 
> And I will sent a separate patch for this part to review.

With all of the above in mind, it's not an error case, but being clever
with variable names and our errno meanings.  So we don't set this flag
here because the "default" drv->init() that we're making this snippet of
code act like never did that.  We only set this flag later on when we're
at the point where we know that we cannot get at a stored environment.
the "default_environment" variable plays the role of both being the
literal default environment as well as being the allocated space we use
and move around as gd->env_addr in many cases.  The env_init() code
block for -ENOENT is to set things up for that case.

Please let me know if this makes sense, or if you think I misread
something (which is quite possible).  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200623/af89f17c/attachment.sig>

[PATCH v2 3/9] env: correctly handle result in env_init

[Patrick DELAUNAY]

Hi Tom,

> From: Tom Rini <trini@konsulko.com>
> Sent: mardi 23 juin 2020 17:17
> 
> On Tue, Jun 23, 2020 at 01:13:55PM +0000, Patrick DELAUNAY wrote:
> > Hi Tom,
> >
> > > From: Tom Rini <trini@konsulko.com>
> > > Sent: vendredi 19 juin 2020 20:05
> > >
> > > On Fri, Jun 19, 2020 at 02:14:00PM +0000, Patrick DELAUNAY wrote:
> > > > Hi Tom and Marek,
> > > >
> > > > > From: Tom Rini <trini@konsulko.com>
> > > > > Sent: jeudi 18 juin 2020 21:16
> > > > >
> > > > > On Tue, Jun 16, 2020 at 09:40:42AM +0200, Patrick Delaunay wrote:
> > > > >
> > > > > > Don't return error with ret=-ENOENT when the optional ops
> > > > > > drv->init is absent but only if env_driver_lookup doesn'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>
> > > > > > ---
> > > > > >
> > > > > > (no changes since v1)
> > > > > >
> > > > > >  env/env.c | 5 ++++-
> > > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/env/env.c b/env/env.c index
> > > > > > dcc25c030b..819c88f729
> > > > > > 100644
> > > > > > --- a/env/env.c
> > > > > > +++ b/env/env.c
> > > > > > @@ -295,7 +295,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__,
> > > > >
> > > > > I'm adding in Marek here because this reminds me of similar
> > > > > questions / concerns I had looking in to his series.  At root, I
> > > > > think we're not being consistent in each of our env backing
> > > > > implementations about where flags such as ENV_VALID are set, and
> > > > > return
> > > values / checks of functions.
> > > > >
> > > > > Just outside of the start of the patch context here, we set ret
> > > > > to -ENOENT and just past this, if still -ENOENT we say ENV_VALID
> > > > > and point at the default environment.
> > > > >
> > > > > But, I don't follow the patch commit message here.  If we don't
> > > > > have
> > > > > drv->init we call env_set_inited(drv->location) but we won't
> > > > > drv->have change
> > > > > ret to 0, which means that later on down the function we go back
> > > > > to default environment.
> > > >
> > > > The cause of the issue is because the init() ops is optional in
> > > > "struct
> > > env_driver".
> > >
> > > Right.
> > >
> > > > When this opt is absent, I assume that the initialization is not
> > > > mandatory but this inititialization need to be tagged in
> > > > gd->env_has_init with the call of
> > > > env_set_inited() function
> > >
> > > So when drv->init isn't set we are already calling env_set_inited(...).
> > > If that's not the case, what's going on?
> > >
> > > > And the ENV backend is FOUND (don't return -ENOENT)
> > > >
> > > > else the next call of env_has_inited(drv->location) always failed
> > > > : in
> > > > env_load()
> > > >
> > > > I see the error  in EXT4 env backend,.all the other backend as a
> > > > env_init() function
> > > >
> > > > But some othe backend don't define the .init operation and have
> > > > the issue
> > > >
> > > > eeprom.c:235:U_BOOT_ENV_LOCATION(eeprom) = {
> > > > ext4.c:135:U_BOOT_ENV_LOCATION(ext4) = {
> > > > fat.c:128:U_BOOT_ENV_LOCATION(fat) = {
> > > > mmc.c:393:U_BOOT_ENV_LOCATION(mmc) = {
> > > > onenand.c:108:U_BOOT_ENV_LOCATION(onenand) = {
> > > > sata.c:117:U_BOOT_ENV_LOCATION(sata) = {
> > > > ubi.c:179:U_BOOT_ENV_LOCATION(ubi) = {
> > > >
> > > > The other don't have issue:
> > > >
> > > > flash.c:358:U_BOOT_ENV_LOCATION(flash) = {
> > > > flash.c:368:	.init		= env_flash_init,
> > > > nand.c:382:U_BOOT_ENV_LOCATION(nand) = {
> > > > nand.c:389:	.init		= env_nand_init,
> > > > nowhere.c:30:U_BOOT_ENV_LOCATION(nowhere) = {
> > > > nowhere.c:32:	.init		= env_nowhere_init,
> > > > nvram.c:117:U_BOOT_ENV_LOCATION(nvram) = {
> > > > nvram.c:122:	.init		= env_nvram_init,
> > > > remote.c:54:U_BOOT_ENV_LOCATION(remote) = {
> > > > remote.c:59:	.init		= env_remote_init,
> > > > sf.c:306:U_BOOT_ENV_LOCATION(sf) = {
> > > > sf.c:312:	.init		= env_sf_init,
> > >
> > > Right, there should be a problem showing up on a ton of locations,
> > > not just ext4 which is why I'm concerned / confused here.  While
> > > ext4 isn't as widely used yet as I would expect, FAT/MMC are.
> > >
> > > > > So isn't this a problem in most environment cases then?  Thanks!
> > > >
> > > > I don't sure which environment configuration can case issue (only
> > > > one ENV without drc->init() function) But no issue if at least one
> > > > CONFIG_ENV_IS_ is activated with driver implementing init ops
> > > >
> > > > But I see the issue in SANDBOX when I activate EXT4 only target.
> > > > (CONFIG_ENV_IS_IN_EXT4), And no more issue when I add
> > > CONFIG_ENV_IS_NOWHERE.
> > > >
> > > > PS: no direct issue if env_init result is not checked
> > > >        but I check this result in the sandbox tests in next patches:
> > > > 	if (!env_init())
> > > > 	     env_load()
> > > >
> > > >        but anyway inconsistent value of gd->env_has_init
> > > >        which can be a problem for any env_has_inited() calls
> > >
> > > Right.  I think there's some bigger inconsistency going on here that
> > > needs to be fixed.  I'm also confused / concerned how you're not
> > > seeing
> > > env_set_inite(..) being called.  if (!NULL) is true.  Thanks!
> >
> > I  was confused also...
> > and I check again the code
> 
> Thanks.  It's a very tricky section of the code and I think I got some part of it
> wrong too.  What got me off-track was unrolling the test isn't required in what you
> tried, just adding:
> 	ret = 0; // We found at least one env exits.
> before if (!drv->init || !(ret = drv->init())) would do the same.  That said...
> 
> > And I make a error in my first analysis.
> >
> > For the case where init ops is not defined in only one backend.
> >
> > 	ret = -ENOENT
> >
> > And the last test is true
> >
> > 	if (ret == -ENOENT) {
> > 		gd->env_addr = (ulong)&default_environment[0];
> > 		gd->env_valid = ENV_VALID;
> >
> > 		return 0;
> > 	}
> >
> > In  fact this function return the LAST result for 'drv->init()' call
> > and that it is strange when several backend are activated.
> 
> Yes.  Things are very fragile in the multi-env case.  There being underlying bugs
> would not surprise me.

I dig deeper in the code and I agree: I will just sent a v3 with minimal update. 

The multi-env cases it could be  long story.

> > So this function have no issue when only one ENV backend is activated,
> > and it is the configuration today for most of boards...
> 
> Right, the common case is the way things have worked historically, env exists in
> one defined location and when that fails (device not present), we get the built-in
> environment and saveenv needs to fail rather than crash the board (due to writing
> to non-existent HW, etc).

Yes tested today on sandbox
env save failed to avoid crash but without any trace.

=> I push a patch to add trace is that make sense.
[PATCH] env: add failing trace in env_save
http://patchwork.ozlabs.org/project/uboot/list/?series=185459


> > I will change my patch in the V3 serie:
> > env_init() return 0 if, at least, one backend is correctly initialized
> > (when no ops init  or the ops init result is OK)
> >
> > But I don't understood 2 thinks in the last test
> >
> > 1/ Why set ENV Is set to VALID:
> >
> > 	gd->env_valid = ENV_VALID;
> >
> > 	in nowhere backend, for same case of default env, it is set
> ENV_INVALID....
> 
> A good question.  I see:
> commit eeba55cb4a8a29a47d0d26692c188b47ba6bf396
> Author: Tom Rini <trini@konsulko.com>
> Date:   Sat Aug 19 22:27:57 2017 -0400
> 
>     env: Correct case of no sub-init function
> 
> changed that from setting it to 0 to setting it to ENV_VALID, where 0 meant
> ENV_INVALID at the time, but the comments in that commit say it used to be
> setting it to valid.  Which brings us back to:
> commit 203e94f6c9ca03e260175ce240f5856507395585
> Author: Simon Glass <sjg@chromium.org>
> Date:   Thu Aug 3 12:21:56 2017 -0600
> 
>     env: Add an enum for environment state
> 
> as adding a enum for 0 = ENV_INVALID, 1 = ENV_VALID, 2 = redundant.
> 
> And there's basically part of a longer series to clean up the environment code.  In
> particular:
> commit 7938822a6b75b69fff9793b6b1769dddf1249525
> Author: Simon Glass <sjg@chromium.org>
> Date:   Thu Aug 3 12:22:02 2017 -0600
> 
>     env: Drop common init() function
> 
> is the root of this particular question.  Before this change, the logic was "For the
> init function in of an env location, point at the in-memory default environment,
> declare it valid".  With this change the new common env init function (now
> env_init() then env_init_new() as functionality migrated to it) when we did not have
> an explicit drv->init() call we need to point at the in-memory default environment
> and (was wrong at the time, later fixed to...) mark env as valid.
> 
> So today, ret = -ENOENT is for the case of no explicit drv->init() function so do
> what those env drivers were doing before of just pointing
> gd->env_addr to the default environment and mark it valid.
> 
> What we're doing today works but feels too clever, given the amount of time it
> takes to dig in to see what is supposed to be going on.
> 
> In the case of one environment location, this logic works as expected.
> In the case of multiple environments, I would need to write down all of the possible
> combinations and walk the code to be sure what happens, especially since it's
> link-order dependent on the order we try drivers in.

I check the code deeper and
I see many incoherency in multi-env support

For example sometime the global ENV configuration 
	gd->env_valid = ENV_VALID;

is initialiazed in init function of each backend...
for me it should be initialzed only on the load ops when the ENV is really valid.

and for default ENV configuration

we have
	gd->env_addr	= (ulong)&default_environment[0];
	gd->env_valid	= ENV_INVALID;

	in this case set gd->env_addr with ENV_INVALID is not necessary 

or
	gd->env_addr	= (ulong)&default_environment[0];
	gd->env_valid	= ENV_VALID;

it seems incoherent or uneeded with

int env_get_char(int index)
{
	if (gd->env_valid == ENV_INVALID)
		return default_environment[index];
	else
		return env_get_char_spec(index);
}

I will continue to check the code deeper but it seems 
out of the scope for this serie.

I will sent a v3 soon.

> > 2/ Why flags is not update
> >
> > 	gd->flags |= GD_FLG_ENV_DEFAULT;
> >
> >
> > But as it s error case, in should never occurs And I will sent a
> > separate patch for this part to review.
> 
> With all of the above in mind, it's not an error case, but being clever with variable
> names and our errno meanings.  So we don't set this flag here because the
> "default" drv->init() that we're making this snippet of code act like never did that.
> We only set this flag later on when we're at the point where we know that we
> cannot get at a stored environment.
> the "default_environment" variable plays the role of both being the literal default
> environment as well as being the allocated space we use and move around as gd-
> >env_addr in many cases.  The env_init() code block for -ENOENT is to set
> things up for that case.
> 
> Please let me know if this makes sense, or if you think I misread something
> (which is quite possible).  Thanks!

Yes that make sense: it is fallback to default ENV when all .init failed, I will update
my patch in V3 with minimal update of this part and with a new comment.

The env code use many gloabal tag, sometime managed by common code,
sometime managed by backend... it is difficult to know the responsibility of each
part in multi-env context.

I will continue check of ENV code in background (and perhaps activate and test a other
ENV backend in sandbox)

Thanks for your time.

Patrick

[PATCH v2 3/9] env: correctly handle result in env_init

[Tom Rini]

On Wed, Jun 24, 2020 at 11:19:50AM +0000, Patrick DELAUNAY wrote:
> Hi Tom,
> 
> > From: Tom Rini <trini@konsulko.com>
> > Sent: mardi 23 juin 2020 17:17
> > 
> > On Tue, Jun 23, 2020 at 01:13:55PM +0000, Patrick DELAUNAY wrote:
> > > Hi Tom,
> > >
> > > > From: Tom Rini <trini@konsulko.com>
> > > > Sent: vendredi 19 juin 2020 20:05
> > > >
> > > > On Fri, Jun 19, 2020 at 02:14:00PM +0000, Patrick DELAUNAY wrote:
> > > > > Hi Tom and Marek,
> > > > >
> > > > > > From: Tom Rini <trini@konsulko.com>
> > > > > > Sent: jeudi 18 juin 2020 21:16
> > > > > >
> > > > > > On Tue, Jun 16, 2020 at 09:40:42AM +0200, Patrick Delaunay wrote:
> > > > > >
> > > > > > > Don't return error with ret=-ENOENT when the optional ops
> > > > > > > drv->init is absent but only if env_driver_lookup doesn'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>
> > > > > > > ---
> > > > > > >
> > > > > > > (no changes since v1)
> > > > > > >
> > > > > > >  env/env.c | 5 ++++-
> > > > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/env/env.c b/env/env.c index
> > > > > > > dcc25c030b..819c88f729
> > > > > > > 100644
> > > > > > > --- a/env/env.c
> > > > > > > +++ b/env/env.c
> > > > > > > @@ -295,7 +295,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__,
> > > > > >
> > > > > > I'm adding in Marek here because this reminds me of similar
> > > > > > questions / concerns I had looking in to his series.  At root, I
> > > > > > think we're not being consistent in each of our env backing
> > > > > > implementations about where flags such as ENV_VALID are set, and
> > > > > > return
> > > > values / checks of functions.
> > > > > >
> > > > > > Just outside of the start of the patch context here, we set ret
> > > > > > to -ENOENT and just past this, if still -ENOENT we say ENV_VALID
> > > > > > and point at the default environment.
> > > > > >
> > > > > > But, I don't follow the patch commit message here.  If we don't
> > > > > > have
> > > > > > drv->init we call env_set_inited(drv->location) but we won't
> > > > > > drv->have change
> > > > > > ret to 0, which means that later on down the function we go back
> > > > > > to default environment.
> > > > >
> > > > > The cause of the issue is because the init() ops is optional in
> > > > > "struct
> > > > env_driver".
> > > >
> > > > Right.
> > > >
> > > > > When this opt is absent, I assume that the initialization is not
> > > > > mandatory but this inititialization need to be tagged in
> > > > > gd->env_has_init with the call of
> > > > > env_set_inited() function
> > > >
> > > > So when drv->init isn't set we are already calling env_set_inited(...).
> > > > If that's not the case, what's going on?
> > > >
> > > > > And the ENV backend is FOUND (don't return -ENOENT)
> > > > >
> > > > > else the next call of env_has_inited(drv->location) always failed
> > > > > : in
> > > > > env_load()
> > > > >
> > > > > I see the error  in EXT4 env backend,.all the other backend as a
> > > > > env_init() function
> > > > >
> > > > > But some othe backend don't define the .init operation and have
> > > > > the issue
> > > > >
> > > > > eeprom.c:235:U_BOOT_ENV_LOCATION(eeprom) = {
> > > > > ext4.c:135:U_BOOT_ENV_LOCATION(ext4) = {
> > > > > fat.c:128:U_BOOT_ENV_LOCATION(fat) = {
> > > > > mmc.c:393:U_BOOT_ENV_LOCATION(mmc) = {
> > > > > onenand.c:108:U_BOOT_ENV_LOCATION(onenand) = {
> > > > > sata.c:117:U_BOOT_ENV_LOCATION(sata) = {
> > > > > ubi.c:179:U_BOOT_ENV_LOCATION(ubi) = {
> > > > >
> > > > > The other don't have issue:
> > > > >
> > > > > flash.c:358:U_BOOT_ENV_LOCATION(flash) = {
> > > > > flash.c:368:	.init		= env_flash_init,
> > > > > nand.c:382:U_BOOT_ENV_LOCATION(nand) = {
> > > > > nand.c:389:	.init		= env_nand_init,
> > > > > nowhere.c:30:U_BOOT_ENV_LOCATION(nowhere) = {
> > > > > nowhere.c:32:	.init		= env_nowhere_init,
> > > > > nvram.c:117:U_BOOT_ENV_LOCATION(nvram) = {
> > > > > nvram.c:122:	.init		= env_nvram_init,
> > > > > remote.c:54:U_BOOT_ENV_LOCATION(remote) = {
> > > > > remote.c:59:	.init		= env_remote_init,
> > > > > sf.c:306:U_BOOT_ENV_LOCATION(sf) = {
> > > > > sf.c:312:	.init		= env_sf_init,
> > > >
> > > > Right, there should be a problem showing up on a ton of locations,
> > > > not just ext4 which is why I'm concerned / confused here.  While
> > > > ext4 isn't as widely used yet as I would expect, FAT/MMC are.
> > > >
> > > > > > So isn't this a problem in most environment cases then?  Thanks!
> > > > >
> > > > > I don't sure which environment configuration can case issue (only
> > > > > one ENV without drc->init() function) But no issue if at least one
> > > > > CONFIG_ENV_IS_ is activated with driver implementing init ops
> > > > >
> > > > > But I see the issue in SANDBOX when I activate EXT4 only target.
> > > > > (CONFIG_ENV_IS_IN_EXT4), And no more issue when I add
> > > > CONFIG_ENV_IS_NOWHERE.
> > > > >
> > > > > PS: no direct issue if env_init result is not checked
> > > > >        but I check this result in the sandbox tests in next patches:
> > > > > 	if (!env_init())
> > > > > 	     env_load()
> > > > >
> > > > >        but anyway inconsistent value of gd->env_has_init
> > > > >        which can be a problem for any env_has_inited() calls
> > > >
> > > > Right.  I think there's some bigger inconsistency going on here that
> > > > needs to be fixed.  I'm also confused / concerned how you're not
> > > > seeing
> > > > env_set_inite(..) being called.  if (!NULL) is true.  Thanks!
> > >
> > > I  was confused also...
> > > and I check again the code
> > 
> > Thanks.  It's a very tricky section of the code and I think I got some part of it
> > wrong too.  What got me off-track was unrolling the test isn't required in what you
> > tried, just adding:
> > 	ret = 0; // We found at least one env exits.
> > before if (!drv->init || !(ret = drv->init())) would do the same.  That said...
> > 
> > > And I make a error in my first analysis.
> > >
> > > For the case where init ops is not defined in only one backend.
> > >
> > > 	ret = -ENOENT
> > >
> > > And the last test is true
> > >
> > > 	if (ret == -ENOENT) {
> > > 		gd->env_addr = (ulong)&default_environment[0];
> > > 		gd->env_valid = ENV_VALID;
> > >
> > > 		return 0;
> > > 	}
> > >
> > > In  fact this function return the LAST result for 'drv->init()' call
> > > and that it is strange when several backend are activated.
> > 
> > Yes.  Things are very fragile in the multi-env case.  There being underlying bugs
> > would not surprise me.
> 
> I dig deeper in the code and I agree: I will just sent a v3 with minimal update. 
> 
> The multi-env cases it could be  long story.
> 
> > > So this function have no issue when only one ENV backend is activated,
> > > and it is the configuration today for most of boards...
> > 
> > Right, the common case is the way things have worked historically, env exists in
> > one defined location and when that fails (device not present), we get the built-in
> > environment and saveenv needs to fail rather than crash the board (due to writing
> > to non-existent HW, etc).
> 
> Yes tested today on sandbox
> env save failed to avoid crash but without any trace.
> 
> => I push a patch to add trace is that make sense.
> [PATCH] env: add failing trace in env_save
> http://patchwork.ozlabs.org/project/uboot/list/?series=185459
> 
> 
> > > I will change my patch in the V3 serie:
> > > env_init() return 0 if, at least, one backend is correctly initialized
> > > (when no ops init  or the ops init result is OK)
> > >
> > > But I don't understood 2 thinks in the last test
> > >
> > > 1/ Why set ENV Is set to VALID:
> > >
> > > 	gd->env_valid = ENV_VALID;
> > >
> > > 	in nowhere backend, for same case of default env, it is set
> > ENV_INVALID....
> > 
> > A good question.  I see:
> > commit eeba55cb4a8a29a47d0d26692c188b47ba6bf396
> > Author: Tom Rini <trini@konsulko.com>
> > Date:   Sat Aug 19 22:27:57 2017 -0400
> > 
> >     env: Correct case of no sub-init function
> > 
> > changed that from setting it to 0 to setting it to ENV_VALID, where 0 meant
> > ENV_INVALID at the time, but the comments in that commit say it used to be
> > setting it to valid.  Which brings us back to:
> > commit 203e94f6c9ca03e260175ce240f5856507395585
> > Author: Simon Glass <sjg@chromium.org>
> > Date:   Thu Aug 3 12:21:56 2017 -0600
> > 
> >     env: Add an enum for environment state
> > 
> > as adding a enum for 0 = ENV_INVALID, 1 = ENV_VALID, 2 = redundant.
> > 
> > And there's basically part of a longer series to clean up the environment code.  In
> > particular:
> > commit 7938822a6b75b69fff9793b6b1769dddf1249525
> > Author: Simon Glass <sjg@chromium.org>
> > Date:   Thu Aug 3 12:22:02 2017 -0600
> > 
> >     env: Drop common init() function
> > 
> > is the root of this particular question.  Before this change, the logic was "For the
> > init function in of an env location, point at the in-memory default environment,
> > declare it valid".  With this change the new common env init function (now
> > env_init() then env_init_new() as functionality migrated to it) when we did not have
> > an explicit drv->init() call we need to point at the in-memory default environment
> > and (was wrong at the time, later fixed to...) mark env as valid.
> > 
> > So today, ret = -ENOENT is for the case of no explicit drv->init() function so do
> > what those env drivers were doing before of just pointing
> > gd->env_addr to the default environment and mark it valid.
> > 
> > What we're doing today works but feels too clever, given the amount of time it
> > takes to dig in to see what is supposed to be going on.
> > 
> > In the case of one environment location, this logic works as expected.
> > In the case of multiple environments, I would need to write down all of the possible
> > combinations and walk the code to be sure what happens, especially since it's
> > link-order dependent on the order we try drivers in.
> 
> I check the code deeper and
> I see many incoherency in multi-env support
> 
> For example sometime the global ENV configuration 
> 	gd->env_valid = ENV_VALID;
> 
> is initialiazed in init function of each backend...
> for me it should be initialzed only on the load ops when the ENV is really valid.
> 
> and for default ENV configuration
> 
> we have
> 	gd->env_addr	= (ulong)&default_environment[0];
> 	gd->env_valid	= ENV_INVALID;
> 
> 	in this case set gd->env_addr with ENV_INVALID is not necessary 
> 
> or
> 	gd->env_addr	= (ulong)&default_environment[0];
> 	gd->env_valid	= ENV_VALID;
> 
> it seems incoherent or uneeded with
> 
> int env_get_char(int index)
> {
> 	if (gd->env_valid == ENV_INVALID)
> 		return default_environment[index];
> 	else
> 		return env_get_char_spec(index);
> }
> 
> I will continue to check the code deeper but it seems 
> out of the scope for this serie.
> 
> I will sent a v3 soon.
> 
> > > 2/ Why flags is not update
> > >
> > > 	gd->flags |= GD_FLG_ENV_DEFAULT;
> > >
> > >
> > > But as it s error case, in should never occurs And I will sent a
> > > separate patch for this part to review.
> > 
> > With all of the above in mind, it's not an error case, but being clever with variable
> > names and our errno meanings.  So we don't set this flag here because the
> > "default" drv->init() that we're making this snippet of code act like never did that.
> > We only set this flag later on when we're at the point where we know that we
> > cannot get at a stored environment.
> > the "default_environment" variable plays the role of both being the literal default
> > environment as well as being the allocated space we use and move around as gd-
> > >env_addr in many cases.  The env_init() code block for -ENOENT is to set
> > things up for that case.
> > 
> > Please let me know if this makes sense, or if you think I misread something
> > (which is quite possible).  Thanks!
> 
> Yes that make sense: it is fallback to default ENV when all .init failed, I will update
> my patch in V3 with minimal update of this part and with a new comment.
> 
> The env code use many gloabal tag, sometime managed by common code,
> sometime managed by backend... it is difficult to know the responsibility of each
> part in multi-env context.
> 
> I will continue check of ENV code in background (and perhaps activate and test a other
> ENV backend in sandbox)
> 
> Thanks for your time.

Thanks for looking here as well.  Any patches you can add to clarify
things or make them more consistent are greatly appreciated.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200624/85e7226b/attachment.sig>

[PATCH v2 4/9] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 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 1372003018..9cb5fe5c75 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 a2410b3368..b70272c0b0 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -81,6 +81,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_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
 CONFIG_REGMAP=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 0c9e0b9f1f..715f5dc39d 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -90,6 +90,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_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
 CONFIG_REGMAP=y
diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig
index bc4819f998..ce806270bd 100644
--- a/configs/sandbox_flattree_defconfig
+++ b/configs/sandbox_flattree_defconfig
@@ -64,6 +64,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_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
 CONFIG_REGMAP=y
diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig
index 64f2ed5102..ea11c9bded 100644
--- a/configs/sandbox_spl_defconfig
+++ b/configs/sandbox_spl_defconfig
@@ -80,6 +80,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_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
 CONFIG_SPL_DM=y
-- 
2.17.1

[PATCH v2 5/9] 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>
---

Changes in v2:
- change cmd_tbl_t to struct cmd_tbl

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

diff --git a/board/sandbox/sandbox.c b/board/sandbox/sandbox.c
index 9cb5fe5c75..bd99141fa8 100644
--- a/board/sandbox/sandbox.c
+++ b/board/sandbox/sandbox.c
@@ -4,6 +4,7 @@
  */
 
 #include <common.h>
+#include <command.h>
 #include <cpu_func.h>
 #include <cros_ec.h>
 #include <dm.h>
@@ -21,6 +22,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 +57,45 @@ 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(struct cmd_tbl *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

[PATCH v2 5/9] sandbox: support the change of env location

[Simon Glass]

On Tue, 16 Jun 2020 at 01:40, 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>
> ---
>
> Changes in v2:
> - change cmd_tbl_t to struct cmd_tbl
>
>  board/sandbox/sandbox.c | 42 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)

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

[PATCH v2 5/9] sandbox: support the change of env location

[Tom Rini]

On Tue, Jun 16, 2020 at 09:40:44AM +0200, Patrick Delaunay 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>
> ---
> 
> Changes in v2:
> - change cmd_tbl_t to struct cmd_tbl
> 
>  board/sandbox/sandbox.c | 42 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)

This is for testing, which is why it's on sandbox?  But I think we
should have this be a generic opt-in feature as changing where
environment is saved at run time has use cases when we have multiple
available.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200618/a7c0e7df/attachment.sig>

[PATCH v2 5/9] sandbox: support the change of env location

[Patrick DELAUNAY]

Hi,

> From: Tom Rini <trini@konsulko.com>
> Sent: jeudi 18 juin 2020 21:17
> 
> On Tue, Jun 16, 2020 at 09:40:44AM +0200, Patrick Delaunay 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>
> > ---
> >
> > Changes in v2:
> > - change cmd_tbl_t to struct cmd_tbl
> >
> >  board/sandbox/sandbox.c | 42
> > ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> This is for testing, which is why it's on sandbox?  But I think we should have this
> be a generic opt-in feature as changing where environment is saved at run time
> has use cases when we have multiple available.  Thanks!

Yes in my mind it was only for testing on sandbox....

But  I agree, I can a add a opt-in generic command to select and load ENV on one target.

Someting as "env load [<target>] " which loads with the request backend and update gd->env_load_prio

With <target> = name of the name define in backend with ENV_NAME macro
And using the default location gd->env_load_prio when absent.

Or split in 2 new commands

- env select <target>
- env load

Perhaps this last proposal with 2 command is more flexible.... 
to be combined with other command (env save / env erase)

if this proposal is OK, I will work on it.....

Patrick

[PATCH v2 5/9] sandbox: support the change of env location

[Tom Rini]

On Fri, Jun 19, 2020 at 02:40:06PM +0000, Patrick DELAUNAY wrote:
> Hi,
> 
> > From: Tom Rini <trini@konsulko.com>
> > Sent: jeudi 18 juin 2020 21:17
> > 
> > On Tue, Jun 16, 2020 at 09:40:44AM +0200, Patrick Delaunay 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>
> > > ---
> > >
> > > Changes in v2:
> > > - change cmd_tbl_t to struct cmd_tbl
> > >
> > >  board/sandbox/sandbox.c | 42
> > > ++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 41 insertions(+), 1 deletion(-)
> > 
> > This is for testing, which is why it's on sandbox?  But I think we should have this
> > be a generic opt-in feature as changing where environment is saved at run time
> > has use cases when we have multiple available.  Thanks!
> 
> Yes in my mind it was only for testing on sandbox....
> 
> But  I agree, I can a add a opt-in generic command to select and load ENV on one target.
> 
> Someting as "env load [<target>] " which loads with the request backend and update gd->env_load_prio
> 
> With <target> = name of the name define in backend with ENV_NAME macro
> And using the default location gd->env_load_prio when absent.
> 
> Or split in 2 new commands
> 
> - env select <target>
> - env load
> 
> Perhaps this last proposal with 2 command is more flexible.... 
> to be combined with other command (env save / env erase)
> 
> if this proposal is OK, I will work on it.....

Sounds good to me, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200619/b992f1a8/attachment.sig>

[PATCH v2 6/9] 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 an empty EXT4 file system is created in
persistent data dir: env.ext4.img.

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

(no changes since v1)

 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..6f1d94b953 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

[PATCH v2 6/9] test: environment in ext4

[Stephen Warren]

On 6/16/20 1:40 AM, Patrick Delaunay wrote:
> Add basic test to persistent environment in ext4:
> save and load in host ext4 file 'uboot.env'.
> 
> On first execution an 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):

> +    if os.path.exists(persistent):
> +        c.log.action('Disk image file ' + persistent + ' already exists')
> +    else:
> +        try:
> +            check_call('rm -f %s' % persistent, shell=True)

This should be run with the results logged to the overall test log file
so that if there are failures, it's possible to see what they were. Use
util.run_and_log() for this.

Also, this particular command doesn't seem useful, since 4 lines above
the code verified that the file doesn't exist.

> + 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

Nit: That blank line is a bit odd.

> +    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)
> +    """

Nit: Wrap the trailing """ onto the same line; no need to force it to be
a multi-line string. Also a comman may be better rather than a
docstring. Same for the other docstring later.

> +    call('rm -f %s' % fs_img, shell=True)

This won't happen if the test fails earlier. Should there be a
try/except block or wrapper function with exception handling to resolve
this?

[PATCH v2 6/9] test: environment in ext4

[Patrick DELAUNAY]

Hi,

> From: Stephen Warren <swarren@wwwdotorg.org>
> Sent: lundi 22 juin 2020 20:57
> 
> On 6/16/20 1:40 AM, Patrick Delaunay wrote:
> > Add basic test to persistent environment in ext4:
> > save and load in host ext4 file 'uboot.env'.
> >
> > On first execution an 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):
> 
> > +    if os.path.exists(persistent):
> > +        c.log.action('Disk image file ' + persistent + ' already exists')
> > +    else:
> > +        try:
> > +            check_call('rm -f %s' % persistent, shell=True)
> 
> This should be run with the results logged to the overall test log file so that if there
> are failures, it's possible to see what they were. Use
> util.run_and_log() for this.

Ok, I will modifiy this part (I copy ext4 file create is copied form py/tests/test_fs/conftest.py:166 mk_fs() )

> Also, this particular command doesn't seem useful, since 4 lines above the code
> verified that the file doesn't exist.

Yes not needed, I remove this line.

> > + 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
> 
> Nit: That blank line is a bit odd.

OK
 
> > +    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)
> > +    """
> 
> Nit: Wrap the trailing """ onto the same line; no need to force it to be a multi-line
> string. Also a comman may be better rather than a docstring. Same for the other
> docstring later.

OK, I don't realized that it was docstring.
 
> > +    call('rm -f %s' % fs_img, shell=True)
> 
> This won't happen if the test fails earlier. Should there be a try/except block or
> wrapper function with exception handling to resolve this?

Yes, I add it for V3

Patrick

[PATCH v2 7/9] 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>
---

(no changes since v1)

 env/ext4.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/env/ext4.c b/env/ext4.c
index ac9f126bec..027a721b69 100644
--- a/env/ext4.c
+++ b/env/ext4.c
@@ -44,9 +44,8 @@ __weak const char *env_ext4_get_dev_part(void)
 	return (const char *)CONFIG_ENV_EXT4_DEVICE_AND_PART;
 }
 
-static int env_ext4_save(void)
+static int env_ext4_save_buffer(env_t *env_new)
 {
-	env_t	env_new;
 	struct blk_desc *dev_desc = NULL;
 	struct disk_partition 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,26 @@ static int env_ext4_save(void)
 			CONFIG_ENV_EXT4_FILE, ifname, dev, part);
 		return 1;
 	}
-	gd->env_valid = ENV_VALID;
 
+	return 0;
+}
+
+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;
 }
 
-- 
2.17.1

[PATCH v2 8/9] 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>
---

Changes in v2:
- use CONFIG_IS_ENABLED to set .erase (same as .save)

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

diff --git a/env/ext4.c b/env/ext4.c
index 027a721b69..7a104c1615 100644
--- a/env/ext4.c
+++ b/env/ext4.c
@@ -99,6 +99,23 @@ static int env_ext4_save(void)
 	return 0;
 }
 
+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;
+}
+
 static int env_ext4_load(void)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
@@ -156,4 +173,6 @@ U_BOOT_ENV_LOCATION(ext4) = {
 	ENV_NAME("EXT4")
 	.load		= env_ext4_load,
 	.save		= ENV_SAVE_PTR(env_ext4_save),
+	.erase		= CONFIG_IS_ENABLED(CMD_ERASEENV) ? env_ext4_erase :
+							    NULL,
 };
-- 
2.17.1

[PATCH v2 9/9] 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>
---

(no changes since v1)

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

diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index b70272c0b0..3913b6392e 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -27,6 +27,7 @@ CONFIG_CMD_BOOTEFI_HELLO=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_EFI=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 715f5dc39d..c59e20579d 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -31,6 +31,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_EFI=y
diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig
index ce806270bd..99b750b4f8 100644
--- a/configs/sandbox_flattree_defconfig
+++ b/configs/sandbox_flattree_defconfig
@@ -24,6 +24,7 @@ CONFIG_CMD_BOOTEFI_HELLO=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 ea11c9bded..52e7625bd7 100644
--- a/configs/sandbox_spl_defconfig
+++ b/configs/sandbox_spl_defconfig
@@ -32,6 +32,7 @@ CONFIG_CMD_BOOTEFI_HELLO=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 6f1d94b953..a71b4c2571 100644
--- a/test/py/tests/test_env.py
+++ b/test/py/tests/test_env.py
@@ -453,6 +453,22 @@ def test_env_ext4(state_test_env):
     response = c.run_command('echo $?')
     assert response == "0"
 
+    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

[PATCH v2 9/9] test: sandbox: add test for erase command

[Simon Glass]

On Tue, 16 Jun 2020 at 01:41, 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>
> ---
>
> (no changes since v1)
>
>  configs/sandbox64_defconfig        |  1 +
>  configs/sandbox_defconfig          |  1 +
>  configs/sandbox_flattree_defconfig |  1 +
>  configs/sandbox_spl_defconfig      |  1 +
>  test/py/tests/test_env.py          | 16 ++++++++++++++++
>  5 files changed, 20 insertions(+)
>

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

[PATCH v2 9/9] test: sandbox: add test for erase command

[Stephen Warren]

On 6/16/20 1:40 AM, Patrick Delaunay wrote:
> Add test for the erase command tested on ENV in EXT4.

Acked-by: Stephen Warren <swarren@nvidia.com>