[PATCH] env: env_sf: don't set .init op if not needed

[Heiko Schocher]

Hello Michael,

Am 01.11.2020 um 14:38 schrieb Michael Walle:
> Commit 92765f45bb95 ("env: Access Environment in SPI flashes before
> relocation") at least breaks the Kontron sl28 board. I guess it also
> breaks others which use a (late) SPI environment.
> 
> Unfortunately, we cannot set the .init op and fall back to the same
> behavior as it would be unset. Thus guard the .init op by #if's.
> 
> Fixes: 92765f45bb95 ("env: Access Environment in SPI flashes before relocation")
> Signed-off-by: Michael Walle <michael at walle.cc>
> ---
>   env/sf.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/env/sf.c b/env/sf.c
> index 2eb2de1a4e..18d44a7ddc 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -385,7 +385,7 @@ out:
>   }
>   #endif
>   
> -static int env_sf_init(void)
> +static int __maybe_unused env_sf_init(void)
>   {
>   #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
>   	return env_sf_init_addr();
> @@ -393,8 +393,13 @@ static int env_sf_init(void)
>   	return env_sf_init_early();
>   #endif
>   	/*
> -	 * we must return with 0 if there is nothing done,
> -	 * else env_set_inited() get not called in env_init()
> +	 * We shouldn't end up here. Unfortunately, there is no
> +	 * way to return a value which yields the same behavior
> +	 * as if the .init op wouldn't be set at all. See
> +	 * env_init(); env_set_inited() is only called if we
> +	 * return 0, but the default environment is only loaded
> +	 * if -ENOENT is returned. Therefore, we need the ugly
> +	 * ifdeferry for the .init op.
>   	 */
>   	return 0;
>   }
> @@ -404,5 +409,7 @@ U_BOOT_ENV_LOCATION(sf) = {
>   	ENV_NAME("SPIFlash")
>   	.load		= env_sf_load,
>   	.save		= CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) : NULL,
> +#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || defined(CONFIG_ENV_SPI_EARLY)
>   	.init		= env_sf_init,
> +#endif
>   };
> 

Ok, tested this patch on an imx6 based board with SPI NOR and it works.

But.... there is a problem with environment in spi nor and ENV_APPEND
enabled, with current implementation (also before my patches applied):

I enabled now ENV_APPEND on this board and

CONFIG_ENV_IS_NOWHERE
CONFIG_ENV_IS_IN_SPI_FLASH

and the Environment from SPI NOR never loaded as gd->env_valid is
always ENV_INVALID and env_load() never called from env_relocate().

What do you think about following patch:

diff --git a/env/sf.c b/env/sf.c
index 2eb2de1a4e..7f3491b458 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -393,9 +393,13 @@ static int env_sf_init(void)
         return env_sf_init_early();
  #endif
         /*
-        * we must return with 0 if there is nothing done,
-        * else env_set_inited() get not called in env_init()
+        * We must return with 0 if there is nothing done,
+        * to get inited bit set in env_init().
+        * We need to set env_valid to ENV_VALID, so later
+        * env_load() loads the Environment from SPI NOR.
          */
+       gd->env_addr = (ulong)&default_environment[0];
+       gd->env_valid = ENV_VALID;
         return 0;
  }

Can you try it?

Another option would be to reutrn -ENOENT and set init bit also
when a init function returns -ENOENT:

diff --git a/env/env.c b/env/env.c
index 42c7d8155e..37b4b54cb7 100644
--- a/env/env.c
+++ b/env/env.c
@@ -329,6 +329,8 @@ int env_init(void)
         for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
                 if (!drv->init || !(ret = drv->init()))
                         env_set_inited(drv->location);
+               if (ret == -ENOENT)
+                       env_set_inited(drv->location);

                 debug("%s: Environment %s init done (ret=%d)\n", __func__,
                       drv->name, ret);
diff --git a/env/sf.c b/env/sf.c
index 2eb2de1a4e..66279fb4f4 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -396,7 +396,7 @@ static int env_sf_init(void)
          * we must return with 0 if there is nothing done,
          * else env_set_inited() get not called in env_init()
          */
-       return 0;
+       return -ENOENT;
  }

But this may has impact on other environment drivers ... but may is
the cleaner approach as env_init() later sets the default environment
if ret is -ENOENT ...

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de