| Message ID | 20240510113834.826783-1-jasper@fancydomain.eu |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | env: Invert gd->env_valid for env_mmc_save | expand |
On Fri, May 10, 2024 at 01:38:34PM +0200, jasper@fancydomain.eu wrote: > From: Jasper Orschulko <jasper@fancydomain.eu> > > The A/B update strategy of the env's has a gap in the first 2 calls of saveenv. > The env's are stored twice on the first memory area if: > gd->env_valid == ENV_INVALID. > > u-boot=> saveenv > Saving Environment to MMC... Writing to MMC(1)... OK > u-boot=> saveenv > Saving Environment to MMC... Writing to MMC(1)... OK <-- !!! > u-boot=> saveenv > Saving Environment to MMC... Writing to redundant MMC(1)... OK > u-boot=> saveenv > Saving Environment to MMC... Writing to MMC(1)... OK > > Signed-off-by: Michael Glembotzki <Michael.Glembotzki@iris-sensing.com> > Signed-off-by: Jasper Orschulko <jasper@fancydomain.eu> > --- > env/mmc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/env/mmc.c b/env/mmc.c > index 7afb733e890..2bef30c973c 100644 > --- a/env/mmc.c > +++ b/env/mmc.c > @@ -299,7 +299,7 @@ static int env_mmc_save(void) > ret = 0; > > if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) > - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; > + gd->env_valid = gd->env_valid == ENV_VALID ? ENV_REDUND : ENV_VALID; > > fini: > fini_mmc_for_env(mmc); 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 Author: Michael Heimpold <mhei@heimpold.de> Date: Wed Apr 10 10:36:19 2013 +0000 env_mmc: add support for redundant environment This patch add support for storing the environment redundant on mmc devices. Substantially it re-uses the logic from the NAND implementation, that means using an incremental counter for marking newer data. Signed-off-by: Michael Heimpold <mhei@heimpold.de> as well. Thanks!
Hi Tom, you are right. If this patch is to be merged, we should change all the occurrences. That was an oversight on my part, sorry about that. I'll send an updated patch to the mailing list later this week. My colleague Michael can give more technical background on this than I can, so I'll let him do explaining here ;) Best, Jasper On 17 May 2024 20:51:44 CEST, Tom Rini <trini@konsulko.com> wrote: >On Fri, May 10, 2024 at 01:38:34PM +0200, jasper@fancydomain.eu wrote: > >> From: Jasper Orschulko <jasper@fancydomain.eu> >> >> The A/B update strategy of the env's has a gap in the first 2 calls of saveenv. >> The env's are stored twice on the first memory area if: >> gd->env_valid == ENV_INVALID. >> >> u-boot=> saveenv >> Saving Environment to MMC... Writing to MMC(1)... OK >> u-boot=> saveenv >> Saving Environment to MMC... Writing to MMC(1)... OK <-- !!! >> u-boot=> saveenv >> Saving Environment to MMC... Writing to redundant MMC(1)... OK >> u-boot=> saveenv >> Saving Environment to MMC... Writing to MMC(1)... OK >> >> Signed-off-by: Michael Glembotzki <Michael.Glembotzki@iris-sensing.com> >> Signed-off-by: Jasper Orschulko <jasper@fancydomain.eu> >> --- >> env/mmc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/env/mmc.c b/env/mmc.c >> index 7afb733e890..2bef30c973c 100644 >> --- a/env/mmc.c >> +++ b/env/mmc.c >> @@ -299,7 +299,7 @@ static int env_mmc_save(void) >> ret = 0; >> >> if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) >> - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; >> + gd->env_valid = gd->env_valid == ENV_VALID ? ENV_REDUND : ENV_VALID; >> >> fini: >> fini_mmc_for_env(mmc); > >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 >Author: Michael Heimpold <mhei@heimpold.de> >Date: Wed Apr 10 10:36:19 2013 +0000 > > env_mmc: add support for redundant environment > > This patch add support for storing the environment redundant on > mmc devices. Substantially it re-uses the logic from the NAND implementation, > that means using an incremental counter for marking newer data. > > Signed-off-by: Michael Heimpold <mhei@heimpold.de> > >as well. Thanks! > >-- >Tom
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
diff --git a/env/mmc.c b/env/mmc.c index 7afb733e890..2bef30c973c 100644 --- a/env/mmc.c +++ b/env/mmc.c @@ -299,7 +299,7 @@ static int env_mmc_save(void) ret = 0; if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; + gd->env_valid = gd->env_valid == ENV_VALID ? ENV_REDUND : ENV_VALID; fini: fini_mmc_for_env(mmc);