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

[Patrick DELAUNAY]

Hi Tom and Marek,

> From: Tom Rini <trini at 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 at 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