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

[Michael Walle]

Am 2020-11-02 08:00, schrieb 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?

This works for me...

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

.. and also this.

So we have four solutions
(1) revert the series
(2) apply my patch
(3) use the first solution from Heiko
(4) use the second solution from Heiko

I'm fine with all four. If it will be (3) or (4) will you prepare a
patch, Heiko?

-michael