[PATCH] env: Leave invalid env for nowhere location

[Kunihiko Hayashi]

Hi Marek,

On 2021/06/07 3:08, Marek Vasut wrote:
> On 6/3/21 6:15 PM, Kunihiko Hayashi wrote:
>> Hi Marek,
> 
> Hi,
> 
>> Sorry for rate reply.
> 
> No worries, same here.
> 
>> On 2021/05/25 16:35, Marek Vasut wrote:
>>> On 5/12/21 4:09 PM, Kunihiko Hayashi wrote:
>>>> When CONFIG_ENV_IS_NOWHERE is enabled, env_nowhere_init() sets ENV_INVALID
>>>> to gd->env_valid, and sets default_environment before relocation to
>>>> gd->env_addr. After that, env_init() switches gd->env_valid to ENV_VALID
>>>> by the previous fix.
>>>>
>>>> If gd->env_valid is ENV_INVALID, env_get_char() returns relocated
>>>> default_environment, however, env_get_char() returns gd->env_addr before
>>>> relocation since gd->env_valid is ENV_VALID, and access to gd->env_addr
>>>> will cause a fault.
>>>>
>>>> This leaves gd->env_valid as ENV_INVALID for "nowhere" location.
>>>
>>> So do I understand this correctly that _after_ relocation, env_init() is
>>> called and env_init() does not update gd->env_addr to the relocated one?
>>
>> In my understandings, env_init() belongs to init_sequence_f[]
>> and env_init() is called before relocation.
> 
> You're right.
> 
> So the env update after relocation should then be done in env_relocate().

Yes. I understand that the relocated gd->env_addr is used in env_relocate().

>>> I would expect that after relocation, if all you have is env_nowhere
>>> driver, the env_nowhere_init() is called again from the first for() loop
>>> of env_init() [1], which would set gd->env_valid to ENV_INVALID [1], and
>>> that for() loop would exit with ret = -ENOENT [2], so then the last part
>>> of env_init() would check for ret == -ENOENT and update gd->env_addr to
>>> relocated default_environment [3].
>>>
>>> 324  int env_init(void)
>>> 325  {
>>> 326    struct env_driver *drv;
>>> 327    int ret = -ENOENT;
>>> 328    int prio;
>>> 329
>>> 330    for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
>>>            /* Part [1] */
>>> 331      if (!drv->init || !(ret = drv->init()))
>>> 332        env_set_inited(drv->location);
>>> 333      if (ret == -ENOENT)
>>> 334        env_set_inited(drv->location);
>>> 335
>>> 336      debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>> 337            drv->name, ret);
>>> 338
>>>            /* Part [2] */
>>> 339      if (gd->env_valid == ENV_INVALID)
>>> 340        ret = -ENOENT;
>>> 341    }
>>> 342
>>> 343    if (!prio)
>>> 344      return -ENODEV;
>>> 345
>>>          /* Part [3] */
>>> 346    if (ret == -ENOENT) {
>>>            /* This should be relocated default_environment address */
>>> 347      gd->env_addr = (ulong)&default_environment[0];
>>> 348      gd->env_valid = ENV_VALID;
>>> 349
>>> 350      return 0;
>>> 351    }
>>> 352
>>> 353    return ret;
>>> 354  }
>>>
>>> Or am I missing something obvious ?
>>
>> These are called before relocation, and update gd->env_addr to non-relocated
>> default_environment by [3].
>>
>> After that, gd->env_addr is relocated in initr_reloc_global_data()
>> if CONFIG_SYS_RELOC_GD_ENV_ADDR is defined.
>>
>> | #ifdef CONFIG_SYS_RELOC_GD_ENV_ADDR
>> |     /*
>> |      * Relocate the early env_addr pointer unless we know it is not inside
>> |      * the binary. Some systems need this and for the rest, it doesn't hurt.
>> |      */
>> |     gd->env_addr += gd->reloc_off;
>> | #endif
>>
> 
> Shouldn't the post-relocation env update happen in env_relocate() ?

Usually env_relocate() calls env_load() that uses relocated gd->env_addr.
It's no problem.

If CONFIG_SYS_TEXT_BASE is zero, gd->reloc_off becomes illegal.
CONFIG_SYS_RELOC_GD_ENV_ADDR should be disabled in that case.

 >
>> However, I misunderstood my situation.
>> gd->reloc_off doesn't have the proper value because CONFIG_SYS_TEXT_BASE
>> is zero due to CONFIG_POSITION_INDENENDENT=y.
>>
>> gd->reloc_off is calculated with CONFIG_SYS_TEXT_BASE in setup_reloc().
>>
>> |     gd->reloc_off = gd->relocaddr - CONFIG_SYS_TEXT_BASE;
>>
>> gd->env_addr is added with gd->reloc_off (== gd->relocaddr - 0),
>> as a result, gd->env_addr has wrong address.
>>
>> In this case, I think the proper solution is to undefine
>> CONFIG_SYS_RELOC_GD_ENV_ADDR.
>>
>> My patch isn't necessary no longer and your patch also works with
>> "nowhere".
> OK

Thank you,

---
Best Regards
Kunihiko Hayashi