AW: [PATCH] env: Invert gd->env_valid for env_mmc_save

[Michael Glembotzki]

Hi Tom,

> ​Can you please explain a little more on how you get to this problem? The
> same code / test exists in env/fat.c, env/sf.c and env/ubi.c. In the
> case of env/mmc.c it's been that way since introduction in:
> commit d196bd880347373237d73e0d115b4d51c68cf2ad

Sure! We have limited the writable u-boot envs to a few (ENV_WRITELIST) and
otherwise only use the default set. That means our env_get_location looks
like this:

enum env_location env_get_location(enum env_operation op, int prio)
{
    if (op == ENVOP_SAVE) {
        if (prio == 0)
            return ENVL_MMC;
    } else {
        if (prio == 0)
            return ENVL_NOWHERE;
        if (prio == 1)
            return ENVL_MMC;
    }
    return ENVL_UNKNOWN;
}

The env location when saving:
0. EMMC

The env location when loading:
0. NOWHERE (default envs) and is appended by the filtered envs from the
1. EMMC

We also have CONFIG_ENV_OFFSET_REDUND and CONFIG_SYS_REDUNDAND_ENVIRONMENT
active.

When booting for the first time, the following happens to the u-boot env's
during load():
1. ​env/nowhere.c: env_nowhere_load sets gd->env_valid = ENV_INVALID
2. env/mmc.c: env_mmc_load dont touch gd->env_valid and stays ENV_INVALID
  env_mmc_load -> env_import_redund -> env_check_redund -> return -ENOMSG

gd->env_valid remains unchanged at ENV_INVALID because there are no valid
env's in both MMC areas (bad CRC) and this creates a problem with
env_mmc_save.

env_mmc_save saves to slot A in the ENV_INVALID and ENV_REDUND cases.

    if (gd->env_valid == ENV_VALID)
        copy = 1;
    mmc_get_env_addr(mmc, copy, &offset);
    printf("Writing to %sMMC(%d)... ", copy ? "redundant " : "", dev);
    write_env(mmc, CONFIG_ENV_SIZE, offset, (u_char *)env_new);


Does it perhaps make sense to set gd->env_valid == ENV_VALID in
env_nowhere_load? Otherwise, I have 2 suggestions. Either we use the patch
provided, or I think my preferred solution would be to change:

    if (gd->env_valid == ENV_VALID)
        copy = 1;
            
in

    if (gd->env_valid != ENV_REDUND)
        copy = 1;

For the other storage locations env/fat.c, env/sf.c, env/ubi.c and
probably others, you would have to straighten this out accordingly. But I
could currently only test it for MMC. What do you think?

Another note: Once a valid save has been made, the problem no longer
occurs. And the problem doesn't occur with the default env_get_location
because gd->env_valid is set to ENV_VALID in env_init (env/enc.c).

Best regards
Michael