| Message ID | 20200625075958.9868-9-patrick.delaunay@st.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | env: ext4: corrections and add test for env in ext4 | expand |
On Thu, Jun 25, 2020 at 09:59:52AM +0200, Patrick Delaunay wrote: > Add the new command 'env select' to force the persistent storage > of environment, saved in gd->env_load_prio. > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> [snip] > + /* search priority by driver */ > + for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) { > + if (entry->location == env_get_location(ENVOP_LOAD, prio)) { > + /* when priority change, reset the ENV flags */ > + if (gd->env_load_prio != prio) { > + gd->env_load_prio = prio; > + gd->env_valid = ENV_INVALID; > + gd->flags &= ~GD_FLG_ENV_DEFAULT; > + } > + printf("OK\n"); > + return 0; > + } > + } So, after we do this, is some follow up env command required to initialize the environment to now exist somewhere else? Or will we have initialized all configured locations during boot, and don't have to? But what will happen if we select say "nand" but it's not present so didn't init. Will things fail gracefully (not panic) ? Thanks!
Hi Tom > From: Tom Rini <trini@konsulko.com> > Sent: vendredi 26 juin 2020 22:55 > > On Thu, Jun 25, 2020 at 09:59:52AM +0200, Patrick Delaunay wrote: > > > Add the new command 'env select' to force the persistent storage of > > environment, saved in gd->env_load_prio. > > > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> > [snip] > > + /* search priority by driver */ > > + for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) { > > + if (entry->location == env_get_location(ENVOP_LOAD, prio)) { > > + /* when priority change, reset the ENV flags */ > > + if (gd->env_load_prio != prio) { > > + gd->env_load_prio = prio; > > + gd->env_valid = ENV_INVALID; > > + gd->flags &= ~GD_FLG_ENV_DEFAULT; > > + } > > + printf("OK\n"); > > + return 0; > > + } > > + } > > So, after we do this, is some follow up env command required to initialize the > environment to now exist somewhere else? Or will we have initialized all > configured locations during boot, and don't have to? > But what will happen if we select say "nand" but it's not present so didn't init. Will > things fail gracefully (not panic) ? Thanks! I was not sure if a automatic load was needed in this command, as I add a sparate load command in this patch I don't add an automatic load => and default env is used My expected sequence for the env commands was: env select <target> env load ... env save when user try to select a not compiled backend the command return "driver not found" (tested on sandbox, I will add unitary test for this point) or when backend is not accessible by any priority, the select command return pritority not found manual test on sandbox, with env_locations[] = { ENVL_NOWHERE }; => env select EXT4 Select Environment on EXT4: priority not found I don't think that abort can occur for other commands because - the env backend for all priotity are initialized at boot (env_init use the same loop than env select) - in all env function (env_load / env_reload / env_save it is tested again (to check if the backend was correctly initilializaed, if .init() retur 0= if (!env_has_inited(drv->location)) return -ENODEV; - all direct acces (in env_get_char for example) are intercepted because gd->env_valid == ENV_INVALID => by default (without explicite reload) the default environent is used, exactly when the load failed. => "gd->env_addr" is never used but user can also execute the sequence env select <target> env save and here the command behavior is strange.... because the default is forced on each select env select EXT4 env load env select MMC (reset on default environment) env save => I expect at the end of the sequence the EXT4 env was copied in MMC.... But in finally only the default environment is saved in MMC I don't sure of the expected behavior by user for these commands.... what it is better solution ? add a option to select command ? force the load after the select ? Patrick
diff --git a/cmd/Kconfig b/cmd/Kconfig index 679b1c32b4..5673b56343 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -616,6 +616,11 @@ config CMD_NVEDIT_LOAD Load all environment variables from the compiled-in persistent storage. +config CMD_NVEDIT_SELECT + bool "env select" + help + Select the compiled-in persistent storage of environment variables. + endmenu menu "Memory commands" diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 7a39d9cd66..9fc33d7db1 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -802,6 +802,15 @@ static int do_env_load(struct cmd_tbl *cmdtp, int flag, int argc, return env_reload() ? 1 : 0; } #endif + +#if defined(CONFIG_CMD_NVEDIT_SELECT) +static int do_env_select(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + return env_select(argv[1]) ? 1 : 0; +} +#endif + #endif /* CONFIG_SPL_BUILD */ int env_match(uchar *s1, int i2) @@ -1367,6 +1376,9 @@ static struct cmd_tbl cmd_env_sub[] = { #if defined(CONFIG_CMD_ERASEENV) U_BOOT_CMD_MKENT(erase, 1, 0, do_env_erase, "", ""), #endif +#endif +#if defined(CONFIG_CMD_NVEDIT_SELECT) + U_BOOT_CMD_MKENT(select, 2, 0, do_env_select, "", ""), #endif U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 0, do_env_set, "", ""), #if defined(CONFIG_CMD_ENV_EXISTS) @@ -1456,6 +1468,9 @@ static char env_help_text[] = #if defined(CONFIG_CMD_NVEDIT_LOAD) "env load - load environment\n" #endif +#if defined(CONFIG_CMD_NVEDIT_SELECT) + "env select [target] - select environment target\n" +#endif #if defined(CONFIG_CMD_NVEDIT_EFI) "env set -e [-nv][-bs][-rt][-at][-a][-i addr,size][-v] name [arg ...]\n" " - set UEFI variable; unset if '-i' or 'arg' not specified\n" diff --git a/env/env.c b/env/env.c index 7ad9623ef2..91c76133b0 100644 --- a/env/env.c +++ b/env/env.c @@ -340,3 +340,45 @@ int env_init(void) return ret; } + +int env_select(const char *name) +{ + struct env_driver *drv; + const int n_ents = ll_entry_count(struct env_driver, env_driver); + struct env_driver *entry; + int prio; + bool found = false; + + printf("Select Environment on %s: ", name); + + /* search ENV driver by name */ + drv = ll_entry_start(struct env_driver, env_driver); + for (entry = drv; entry != drv + n_ents; entry++) { + if (!strcmp(entry->name, name)) { + found = true; + break; + } + } + + if (!found) { + printf("driver not found\n"); + return -ENODEV; + } + + /* search priority by driver */ + for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) { + if (entry->location == env_get_location(ENVOP_LOAD, prio)) { + /* when priority change, reset the ENV flags */ + if (gd->env_load_prio != prio) { + gd->env_load_prio = prio; + gd->env_valid = ENV_INVALID; + gd->flags &= ~GD_FLG_ENV_DEFAULT; + } + printf("OK\n"); + return 0; + } + } + printf("priority not found\n"); + + return -ENODEV; +} diff --git a/include/env.h b/include/env.h index 68e0f4fa56..665857f032 100644 --- a/include/env.h +++ b/include/env.h @@ -286,6 +286,13 @@ int env_save(void); */ int env_erase(void); +/** + * env_select() - Select the environment storage + * + * @return 0 if OK, -ve on error + */ +int env_select(const char *name); + /** * env_import() - Import from a binary representation into hash table * @@ -349,5 +356,4 @@ int env_get_char(int index); * This is used for those unfortunate archs with crappy toolchains */ void env_reloc(void); - #endif
Add the new command 'env select' to force the persistent storage of environment, saved in gd->env_load_prio. Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> --- Changes in v3: - new: add 'env select' command cmd/Kconfig | 5 +++++ cmd/nvedit.c | 15 +++++++++++++++ env/env.c | 42 ++++++++++++++++++++++++++++++++++++++++++ include/env.h | 8 +++++++- 4 files changed, 69 insertions(+), 1 deletion(-)