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

[Tom Rini]

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 at 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
-------------- 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/20200728/4804817f/attachment.sig>