[PATCH] env: Leave invalid env for nowhere location

[Marek Vasut]

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

>> 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() ?

> 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