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

[Heiko Schocher]

Hello Michael,

Am 02.11.2020 um 21:15 schrieb 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?

I tend to implement solution [4] ... I can send a patch...

Simon? Tom? Any suggestions?

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