[PATCH] env: Invert gd->env_valid for env_mmc_save
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);
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
participants (4)
-
Jasper Orschulko -
jasper＠fancydomain.eu
-
Michael Glembotzki -
Tom Rini