diff mbox series

[v3,08/14] cmd: env: add env select command

Message ID 20200625075958.9868-9-patrick.delaunay@st.com
State Accepted
Commit a97d22ebba2305f2d0aee714544c72c6a53026d9
Headers show
Series env: ext4: corrections and add test for env in ext4 | expand

Commit Message

Patrick Delaunay June 25, 2020, 7:59 a.m. UTC
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 at 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(-)

Comments

Tom Rini June 26, 2020, 8:54 p.m. UTC | #1
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 at 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!
Patrick Delaunay June 30, 2020, 11:42 a.m. UTC | #2
Hi Tom

> From: Tom Rini <trini at 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 at 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 mbox series

Patch

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