[V2,4/7] env: Fix invalid env handling in env_init()

Message ID 20200707185139.2225-4-marex@denx.de
State Superseded
Headers show
Series
  • [V2,1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set
Related show

Commit Message

Marek Vasut July 7, 2020, 6:51 p.m.
This fixes the case where there are multiple environment drivers, one of
them is the default environment one, and it is followed by an environment
driver which does not implement .init() callback. The default environment
driver sets gd->env_valid to ENV_INVALID and returns 0 from its .init()
callback implementation, which is valid behavior for default environment.

Since the subsequent environment driver does not implement .init(), it
also does not modify the $ret variable in the loop. Therefore, the loop
is exited with gd->env_valid=ENV_INVALID and ret=0, which means that the
code further down in env_init() will not reset the environment to the
default one, which is incorrect.

This patch sets the $ret variable back to -ENOENT in case the env_valid
is set to ENV_INVALID by an environment driver, so that the environment
would be correctly reset back to default one, unless a subsequent driver
loads a valid environment.

Signed-off-by: Marek Vasut <marex at denx.de>
---
V2: Reword commit message
---
 env/env.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Tom Rini July 24, 2020, 2:56 p.m. | #1
On Tue, Jul 07, 2020 at 08:51:36PM +0200, Marek Vasut wrote:

> This fixes the case where there are multiple environment drivers, one of

> them is the default environment one, and it is followed by an environment

> driver which does not implement .init() callback. The default environment

> driver sets gd->env_valid to ENV_INVALID and returns 0 from its .init()

> callback implementation, which is valid behavior for default environment.

> 

> Since the subsequent environment driver does not implement .init(), it

> also does not modify the $ret variable in the loop. Therefore, the loop

> is exited with gd->env_valid=ENV_INVALID and ret=0, which means that the

> code further down in env_init() will not reset the environment to the

> default one, which is incorrect.

> 

> This patch sets the $ret variable back to -ENOENT in case the env_valid

> is set to ENV_INVALID by an environment driver, so that the environment

> would be correctly reset back to default one, unless a subsequent driver

> loads a valid environment.

> 

> Signed-off-by: Marek Vasut <marex@denx.de>

> ---

> V2: Reword commit message

> ---

>  env/env.c | 3 +++

>  1 file changed, 3 insertions(+)

> 

> diff --git a/env/env.c b/env/env.c

> index dcc25c030b..024d36fdbe 100644

> --- a/env/env.c

> +++ b/env/env.c

> @@ -300,6 +300,9 @@ int env_init(void)

>  

>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,

>  		      drv->name, ret);

> +

> +		if (gd->env_valid == ENV_INVALID)

> +			ret = -ENOENT;

>  	}

>  

>  	if (!prio)


So, I am not sure about this change.  Given the whole thread that ends
in https://lists.denx.de/pipermail/u-boot/2020-June/417433.html this
particular part of the function is being too clever.  I think it's
intentionally not doing what you're adding right here for some use
cases.  I think that to cleanly achieve the goals of your series we need
to stop letting drv->init be optional so that we then stop doing the
particular we're doing with "ENOENT means runs this common code path for
many envs".  We may also need to make sure the link order in
env/Makefile has nowhere first in all cases, rather than just most cases
like it does now, with a big comment on why.

-- 
Tom
Marek Vasut July 28, 2020, 7:28 a.m. | #2
On 7/24/20 4:56 PM, Tom Rini wrote:
> On Tue, Jul 07, 2020 at 08:51:36PM +0200, Marek Vasut wrote:

> 

>> This fixes the case where there are multiple environment drivers, one of

>> them is the default environment one, and it is followed by an environment

>> driver which does not implement .init() callback. The default environment

>> driver sets gd->env_valid to ENV_INVALID and returns 0 from its .init()

>> callback implementation, which is valid behavior for default environment.

>>

>> Since the subsequent environment driver does not implement .init(), it

>> also does not modify the $ret variable in the loop. Therefore, the loop

>> is exited with gd->env_valid=ENV_INVALID and ret=0, which means that the

>> code further down in env_init() will not reset the environment to the

>> default one, which is incorrect.

>>

>> This patch sets the $ret variable back to -ENOENT in case the env_valid

>> is set to ENV_INVALID by an environment driver, so that the environment

>> would be correctly reset back to default one, unless a subsequent driver

>> loads a valid environment.

>>

>> Signed-off-by: Marek Vasut <marex@denx.de>

>> ---

>> V2: Reword commit message

>> ---

>>  env/env.c | 3 +++

>>  1 file changed, 3 insertions(+)

>>

>> diff --git a/env/env.c b/env/env.c

>> index dcc25c030b..024d36fdbe 100644

>> --- a/env/env.c

>> +++ b/env/env.c

>> @@ -300,6 +300,9 @@ int env_init(void)

>>  

>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,

>>  		      drv->name, ret);

>> +

>> +		if (gd->env_valid == ENV_INVALID)

>> +			ret = -ENOENT;

>>  	}

>>  

>>  	if (!prio)

> 

> So, I am not sure about this change.  Given the whole thread that ends

> in https://lists.denx.de/pipermail/u-boot/2020-June/417433.html this

> particular part of the function is being too clever.  I think it's

> intentionally not doing what you're adding right here for some use

> cases.


Which use-cases ?

> I think that to cleanly achieve the goals of your series we need

> to stop letting drv->init be optional so that we then stop doing the

> particular we're doing with "ENOENT means runs this common code path for

> many envs".  We may also need to make sure the link order in

> env/Makefile has nowhere first in all cases, rather than just most cases

> like it does now, with a big comment on why.


So, would it make sense to apply the rest and revisit this patch
separately (likely with ST on CC) so the writeable list won't miss this
release ?
Tom Rini July 28, 2020, 12:39 p.m. | #3
On Tue, Jul 28, 2020 at 09:28:17AM +0200, Marek Vasut wrote:
> On 7/24/20 4:56 PM, Tom Rini wrote:

> > On Tue, Jul 07, 2020 at 08:51:36PM +0200, Marek Vasut wrote:

> > 

> >> This fixes the case where there are multiple environment drivers, one of

> >> them is the default environment one, and it is followed by an environment

> >> driver which does not implement .init() callback. The default environment

> >> driver sets gd->env_valid to ENV_INVALID and returns 0 from its .init()

> >> callback implementation, which is valid behavior for default environment.

> >>

> >> Since the subsequent environment driver does not implement .init(), it

> >> also does not modify the $ret variable in the loop. Therefore, the loop

> >> is exited with gd->env_valid=ENV_INVALID and ret=0, which means that the

> >> code further down in env_init() will not reset the environment to the

> >> default one, which is incorrect.

> >>

> >> This patch sets the $ret variable back to -ENOENT in case the env_valid

> >> is set to ENV_INVALID by an environment driver, so that the environment

> >> would be correctly reset back to default one, unless a subsequent driver

> >> loads a valid environment.

> >>

> >> Signed-off-by: Marek Vasut <marex@denx.de>

> >> ---

> >> V2: Reword commit message

> >> ---

> >>  env/env.c | 3 +++

> >>  1 file changed, 3 insertions(+)

> >>

> >> diff --git a/env/env.c b/env/env.c

> >> index dcc25c030b..024d36fdbe 100644

> >> --- a/env/env.c

> >> +++ b/env/env.c

> >> @@ -300,6 +300,9 @@ int env_init(void)

> >>  

> >>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,

> >>  		      drv->name, ret);

> >> +

> >> +		if (gd->env_valid == ENV_INVALID)

> >> +			ret = -ENOENT;

> >>  	}

> >>  

> >>  	if (!prio)

> > 

> > So, I am not sure about this change.  Given the whole thread that ends

> > in https://lists.denx.de/pipermail/u-boot/2020-June/417433.html this

> > particular part of the function is being too clever.  I think it's

> > intentionally not doing what you're adding right here for some use

> > cases.

> 

> Which use-cases ?


flash specifically sets env_valid to ENV_INVALID and returns 0, and uses
the default environment location, when env is invalid.  NAND, when
embedded, sets addr to 0 when invalid, ENV_INVALID and returns 0.
NOWHERE is the real funny case, it says ENV_INVALID and
default_environment and returns 0.

Which all brings me back to my "too clever" statement.  Only REMOTE
ever returns non-zero in it's init function.

> > I think that to cleanly achieve the goals of your series we need

> > to stop letting drv->init be optional so that we then stop doing the

> > particular we're doing with "ENOENT means runs this common code path for

> > many envs".  We may also need to make sure the link order in

> > env/Makefile has nowhere first in all cases, rather than just most cases

> > like it does now, with a big comment on why.

> 

> So, would it make sense to apply the rest and revisit this patch

> separately (likely with ST on CC) so the writeable list won't miss this

> release ?


If you're OK with that, yes.

-- 
Tom
Marek Vasut July 28, 2020, 1:15 p.m. | #4
On 7/28/20 2:39 PM, Tom Rini wrote:
[...]
>>> So, I am not sure about this change.  Given the whole thread that ends

>>> in https://lists.denx.de/pipermail/u-boot/2020-June/417433.html this

>>> particular part of the function is being too clever.  I think it's

>>> intentionally not doing what you're adding right here for some use

>>> cases.

>>

>> Which use-cases ?

> 

> flash specifically sets env_valid to ENV_INVALID and returns 0, and uses

> the default environment location, when env is invalid.  NAND, when

> embedded, sets addr to 0 when invalid, ENV_INVALID and returns 0.

> NOWHERE is the real funny case, it says ENV_INVALID and

> default_environment and returns 0.

> 

> Which all brings me back to my "too clever" statement.  Only REMOTE

> ever returns non-zero in it's init function.


Which makes me think there is probably no good way out which would not
break one usecase or the other.

>>> I think that to cleanly achieve the goals of your series we need

>>> to stop letting drv->init be optional so that we then stop doing the

>>> particular we're doing with "ENOENT means runs this common code path for

>>> many envs".  We may also need to make sure the link order in

>>> env/Makefile has nowhere first in all cases, rather than just most cases

>>> like it does now, with a big comment on why.

>>

>> So, would it make sense to apply the rest and revisit this patch

>> separately (likely with ST on CC) so the writeable list won't miss this

>> release ?

> 

> If you're OK with that, yes.


Yes, fine.

Patch

diff --git a/env/env.c b/env/env.c
index dcc25c030b..024d36fdbe 100644
--- a/env/env.c
+++ b/env/env.c
@@ -300,6 +300,9 @@  int env_init(void)
 
 		debug("%s: Environment %s init done (ret=%d)\n", __func__,
 		      drv->name, ret);
+
+		if (gd->env_valid == ENV_INVALID)
+			ret = -ENOENT;
 	}
 
 	if (!prio)