[PATCH 0/5] allow default environment to be amended from dtb

[PATCH 0/5] allow default environment to be amended from dtb

[Rasmus Villemoes]

These patches introduce the ability to amend the default environment
baked into the U-Boot binary by a DT property under /config. This is
useful to create a few different images based on the same U-Boot
binary, e.g. one for normal use, one for development and one for
bootstrapping the board. It's also useful when SPL loads U-Boot from a
FIT image and decides between multiple possible control DTBs; those
control DTBs can then contain tweaks of the default environment
suitable for that particular board variant.

There are already various /config/ properties one can use for some of
this; e.g. /config/bootdelay overrides any bootdelay set in the
environment. This is intended as a flexible way to achieve these kinds
of tweaks instead of adding more /config/* properties. [It should be
noted that adding "bootdelay=123" to /config/default-environment is
not a drop-in replacement for setting /config/bootdelay to 123, as the
latter takes effect whether the environment is the default one or one
loaded from storage.]

It does not affect the case where an environment is loaded from a
storage device, nor is there any change if the new CONFIG_ option is
not selected.

Rasmus Villemoes (5):
  fdtdec: make fdtdec_get_config_string() return const char*
  fdtdec: introduce fdtdec_get_config_property
  env: make env_set_default_vars() return void
  env: allow default environment to be amended from control dtb
  test: add tests for default environment

 arch/arm/mach-exynos/include/mach/mipi_dsim.h |   2 +-
 arch/arm/mach-rockchip/rk3188/rk3188.c        |   2 +-
 board/dhelectronics/dh_stm32mp1/board.c       |   2 +-
 board/firefly/firefly-rk3288/firefly-rk3288.c |   2 +-
 board/st/stm32mp1/stm32mp1.c                  |   2 +-
 cmd/nvedit.c                                  |  37 +++++++
 common/cli.c                                  |   2 +-
 configs/sandbox64_defconfig                   |   1 +
 configs/sandbox_defconfig                     |   1 +
 env/Kconfig                                   |  21 ++++
 env/common.c                                  |  23 +++-
 include/configs/sandbox.h                     |   8 +-
 include/env.h                                 |   2 +-
 include/fdtdec.h                              |  16 ++-
 lib/fdtdec.c                                  |  29 +++--
 test/env/Makefile                             |   1 +
 test/env/default.c                            | 102 ++++++++++++++++++
 17 files changed, 227 insertions(+), 26 deletions(-)
 create mode 100644 test/env/default.c

-- 
2.23.0

[PATCH 1/5] fdtdec: make fdtdec_get_config_string() return const char*

[Rasmus Villemoes]

Nobody should modify the string returned by
fdtdec_get_config_string(), so make it return a const pointer.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/arm/mach-exynos/include/mach/mipi_dsim.h | 2 +-
 arch/arm/mach-rockchip/rk3188/rk3188.c        | 2 +-
 board/dhelectronics/dh_stm32mp1/board.c       | 2 +-
 board/firefly/firefly-rk3288/firefly-rk3288.c | 2 +-
 board/st/stm32mp1/stm32mp1.c                  | 2 +-
 common/cli.c                                  | 2 +-
 include/fdtdec.h                              | 2 +-
 lib/fdtdec.c                                  | 4 ++--
 8 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-exynos/include/mach/mipi_dsim.h b/arch/arm/mach-exynos/include/mach/mipi_dsim.h
index 20e6ce7f72..1cdea29b78 100644
--- a/arch/arm/mach-exynos/include/mach/mipi_dsim.h
+++ b/arch/arm/mach-exynos/include/mach/mipi_dsim.h
@@ -313,7 +313,7 @@ struct mipi_dsim_master_ops {
  * @platform_data: lcd panel specific platform data.
  */
 struct mipi_dsim_lcd_device {
-	char			*name;
+	const char		*name;
 	int			id;
 	int			bus_id;
 	int			reverse_panel;
diff --git a/arch/arm/mach-rockchip/rk3188/rk3188.c b/arch/arm/mach-rockchip/rk3188/rk3188.c
index 222953ab94..1f0adac62a 100644
--- a/arch/arm/mach-rockchip/rk3188/rk3188.c
+++ b/arch/arm/mach-rockchip/rk3188/rk3188.c
@@ -111,7 +111,7 @@ static int setup_led(void)
 {
 #ifdef CONFIG_SPL_LED
 	struct udevice *dev;
-	char *led_name;
+	const char *led_name;
 	int ret;
 
 	led_name = fdtdec_get_config_string(gd->fdt_blob, "u-boot,boot-led");
diff --git a/board/dhelectronics/dh_stm32mp1/board.c b/board/dhelectronics/dh_stm32mp1/board.c
index f42d395098..c59bf26223 100644
--- a/board/dhelectronics/dh_stm32mp1/board.c
+++ b/board/dhelectronics/dh_stm32mp1/board.c
@@ -372,7 +372,7 @@ int g_dnl_bind_fixup(struct usb_device_descriptor *dev, const char *name)
 #ifdef CONFIG_LED
 static int get_led(struct udevice **dev, char *led_string)
 {
-	char *led_name;
+	const char *led_name;
 	int ret;
 
 	led_name = fdtdec_get_config_string(gd->fdt_blob, led_string);
diff --git a/board/firefly/firefly-rk3288/firefly-rk3288.c b/board/firefly/firefly-rk3288/firefly-rk3288.c
index bd8a32cf7b..a2bef1c928 100644
--- a/board/firefly/firefly-rk3288/firefly-rk3288.c
+++ b/board/firefly/firefly-rk3288/firefly-rk3288.c
@@ -14,7 +14,7 @@ static int setup_led(void)
 {
 #ifdef CONFIG_SPL_LED
 	struct udevice *dev;
-	char *led_name;
+	const char *led_name;
 	int ret;
 
 	led_name = fdtdec_get_config_string(gd->fdt_blob, "u-boot,boot-led");
diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
index 03a19af930..82798773f5 100644
--- a/board/st/stm32mp1/stm32mp1.c
+++ b/board/st/stm32mp1/stm32mp1.c
@@ -226,7 +226,7 @@ int g_dnl_bind_fixup(struct usb_device_descriptor *dev, const char *name)
 
 static int get_led(struct udevice **dev, char *led_string)
 {
-	char *led_name;
+	const char *led_name;
 	int ret;
 
 	led_name = fdtdec_get_config_string(gd->fdt_blob, led_string);
diff --git a/common/cli.c b/common/cli.c
index 6635ab2bcf..f0e8f2880c 100644
--- a/common/cli.c
+++ b/common/cli.c
@@ -156,7 +156,7 @@ int do_run(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 bool cli_process_fdt(const char **cmdp)
 {
 	/* Allow the fdt to override the boot command */
-	char *env = fdtdec_get_config_string(gd->fdt_blob, "bootcmd");
+	const char *env = fdtdec_get_config_string(gd->fdt_blob, "bootcmd");
 	if (env)
 		*cmdp = env;
 	/*
diff --git a/include/fdtdec.h b/include/fdtdec.h
index 62d1660973..a037f6ed9c 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -778,7 +778,7 @@ int fdtdec_get_config_bool(const void *blob, const char *prop_name);
  * @param prop_name     property name to look up
  * @returns property string, NULL on error.
  */
-char *fdtdec_get_config_string(const void *blob, const char *prop_name);
+const char *fdtdec_get_config_string(const void *blob, const char *prop_name);
 
 /*
  * Look up a property in a node and return its contents in a byte
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index ee1bd41b08..25a71bc8f9 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -879,7 +879,7 @@ int fdtdec_get_config_bool(const void *blob, const char *prop_name)
 	return prop != NULL;
 }
 
-char *fdtdec_get_config_string(const void *blob, const char *prop_name)
+const char *fdtdec_get_config_string(const void *blob, const char *prop_name)
 {
 	const char *nodep;
 	int nodeoffset;
@@ -894,7 +894,7 @@ char *fdtdec_get_config_string(const void *blob, const char *prop_name)
 	if (!nodep)
 		return NULL;
 
-	return (char *)nodep;
+	return nodep;
 }
 
 u64 fdtdec_get_number(const fdt32_t *ptr, unsigned int cells)
-- 
2.23.0

[PATCH 1/5] fdtdec: make fdtdec_get_config_string() return const char*

[Simon Glass]

On Tue, 10 Nov 2020 at 13:26, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Nobody should modify the string returned by
> fdtdec_get_config_string(), so make it return a const pointer.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  arch/arm/mach-exynos/include/mach/mipi_dsim.h | 2 +-
>  arch/arm/mach-rockchip/rk3188/rk3188.c        | 2 +-
>  board/dhelectronics/dh_stm32mp1/board.c       | 2 +-
>  board/firefly/firefly-rk3288/firefly-rk3288.c | 2 +-
>  board/st/stm32mp1/stm32mp1.c                  | 2 +-
>  common/cli.c                                  | 2 +-
>  include/fdtdec.h                              | 2 +-
>  lib/fdtdec.c                                  | 4 ++--
>  8 files changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 2/5] fdtdec: introduce fdtdec_get_config_property

[Rasmus Villemoes]

Add a wrapper for retrieving a given property from the /config node as
a (blob, length) pair. A later patch will need the length of the
property and thus cannot just use fdtdec_get_config_string(). Rewrite
that to use the new wrapper.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 include/fdtdec.h | 14 ++++++++++++++
 lib/fdtdec.c     | 27 +++++++++++++--------------
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index a037f6ed9c..ff1453a100 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -747,6 +747,20 @@ int fdtdec_get_bool(const void *blob, int node, const char *prop_name);
  */
 int fdtdec_get_child_count(const void *blob, int node);
 
+/**
+ * Look in the FDT for a config item with the given name and a pointer
+ * to its value.
+ *
+ * @param blob          FDT blob
+ * @param prop_name     property name to look up
+ * @param lenp          if non-NULL and the property is found, *lenp is
+ *                      set to the length of the property value
+ *
+ * @returns property value, NULL on error.
+ */
+const void *fdtdec_get_config_property(const void *blob, const char *prop_name,
+		int *lenp);
+
 /**
  * Look in the FDT for a config item with the given name and return its value
  * as a 32-bit integer. The property must have at least 4 bytes of data. The
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 25a71bc8f9..2442470af8 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -853,6 +853,18 @@ const u8 *fdtdec_locate_byte_array(const void *blob, int node,
 	return cell;
 }
 
+const void *fdtdec_get_config_property(const void *blob, const char *prop_name,
+				       int *len)
+{
+	int config_node;
+
+	debug("%s: %s\n", __func__, prop_name);
+	config_node = fdt_path_offset(blob, "/config");
+	if (config_node < 0)
+		return NULL;
+	return fdt_getprop(blob, config_node, prop_name, len);
+}
+
 int fdtdec_get_config_int(const void *blob, const char *prop_name,
 			  int default_val)
 {
@@ -881,20 +893,7 @@ int fdtdec_get_config_bool(const void *blob, const char *prop_name)
 
 const char *fdtdec_get_config_string(const void *blob, const char *prop_name)
 {
-	const char *nodep;
-	int nodeoffset;
-	int len;
-
-	debug("%s: %s\n", __func__, prop_name);
-	nodeoffset = fdt_path_offset(blob, "/config");
-	if (nodeoffset < 0)
-		return NULL;
-
-	nodep = fdt_getprop(blob, nodeoffset, prop_name, &len);
-	if (!nodep)
-		return NULL;
-
-	return nodep;
+	return fdtdec_get_config_property(blob, prop_name, NULL);
 }
 
 u64 fdtdec_get_number(const fdt32_t *ptr, unsigned int cells)
-- 
2.23.0

[PATCH 2/5] fdtdec: introduce fdtdec_get_config_property

[Simon Glass]

Hi Rasmus,

On Tue, 10 Nov 2020 at 13:26, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Add a wrapper for retrieving a given property from the /config node as
> a (blob, length) pair. A later patch will need the length of the
> property and thus cannot just use fdtdec_get_config_string(). Rewrite
> that to use the new wrapper.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  include/fdtdec.h | 14 ++++++++++++++
>  lib/fdtdec.c     | 27 +++++++++++++--------------
>  2 files changed, 27 insertions(+), 14 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

But please adjust the fdtdec_get_config_bool() to use your new function too.

Also fdtdec_get_config_prop() is a better name IMO because it is shorter.

> diff --git a/include/fdtdec.h b/include/fdtdec.h
> index a037f6ed9c..ff1453a100 100644
> --- a/include/fdtdec.h
> +++ b/include/fdtdec.h
> @@ -747,6 +747,20 @@ int fdtdec_get_bool(const void *blob, int node, const char *prop_name);
>   */
>  int fdtdec_get_child_count(const void *blob, int node);
>
> +/**
> + * Look in the FDT for a config item with the given name and a pointer
> + * to its value.
> + *
> + * @param blob          FDT blob
> + * @param prop_name     property name to look up
> + * @param lenp          if non-NULL and the property is found, *lenp is
> + *                      set to the length of the property value
> + *
> + * @returns property value, NULL on error.
> + */
> +const void *fdtdec_get_config_property(const void *blob, const char *prop_name,
> +               int *lenp);
> +
>  /**
>   * Look in the FDT for a config item with the given name and return its value
>   * as a 32-bit integer. The property must have at least 4 bytes of data. The
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 25a71bc8f9..2442470af8 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -853,6 +853,18 @@ const u8 *fdtdec_locate_byte_array(const void *blob, int node,
>         return cell;
>  }
>
> +const void *fdtdec_get_config_property(const void *blob, const char *prop_name,
> +                                      int *len)
> +{
> +       int config_node;
> +
> +       debug("%s: %s\n", __func__, prop_name);
> +       config_node = fdt_path_offset(blob, "/config");
> +       if (config_node < 0)
> +               return NULL;
> +       return fdt_getprop(blob, config_node, prop_name, len);
> +}
> +
>  int fdtdec_get_config_int(const void *blob, const char *prop_name,
>                           int default_val)
>  {
> @@ -881,20 +893,7 @@ int fdtdec_get_config_bool(const void *blob, const char *prop_name)
>
>  const char *fdtdec_get_config_string(const void *blob, const char *prop_name)
>  {
> -       const char *nodep;
> -       int nodeoffset;
> -       int len;
> -
> -       debug("%s: %s\n", __func__, prop_name);
> -       nodeoffset = fdt_path_offset(blob, "/config");
> -       if (nodeoffset < 0)
> -               return NULL;
> -
> -       nodep = fdt_getprop(blob, nodeoffset, prop_name, &len);
> -       if (!nodep)
> -               return NULL;
> -
> -       return nodep;
> +       return fdtdec_get_config_property(blob, prop_name, NULL);
>  }
>
>  u64 fdtdec_get_number(const fdt32_t *ptr, unsigned int cells)
> --
> 2.23.0
>

Regards,
Simon

[PATCH 3/5] env: make env_set_default_vars() return void

[Rasmus Villemoes]

Unlike most other functions declared in env.h, the return value of
this one also isn't documented. It only has a single caller, which
ignores the return value. And the companion, env_set_default(),
already has void as return type.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 env/common.c  | 4 ++--
 include/env.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/env/common.c b/env/common.c
index 6c32a9b479..7363da849b 100644
--- a/env/common.c
+++ b/env/common.c
@@ -94,14 +94,14 @@ void env_set_default(const char *s, int flags)
 
 
 /* [re]set individual variables to their value in the default environment */
-int env_set_default_vars(int nvars, char * const vars[], int flags)
+void env_set_default_vars(int nvars, char * const vars[], int flags)
 {
 	/*
 	 * Special use-case: import from default environment
 	 * (and use \0 as a separator)
 	 */
 	flags |= H_NOCLEAR | H_DEFAULT;
-	return himport_r(&env_htab, (const char *)default_environment,
+	himport_r(&env_htab, (const char *)default_environment,
 				sizeof(default_environment), '\0',
 				flags, 0, nvars, vars);
 }
diff --git a/include/env.h b/include/env.h
index c15339a93f..5bff6c4d72 100644
--- a/include/env.h
+++ b/include/env.h
@@ -256,7 +256,7 @@ void env_fix_drivers(void);
  * @vars: List of variables to set/reset
  * @flags: Flags controlling matching (H_... - see search.h)
  */
-int env_set_default_vars(int nvars, char *const vars[], int flags);
+void env_set_default_vars(int nvars, char *const vars[], int flags);
 
 /**
  * env_load() - Load the environment from storage
-- 
2.23.0

[PATCH 3/5] env: make env_set_default_vars() return void

[Wolfgang Denk]

Dear Rasmus,

In message <20201110202603.20944-4-rasmus.villemoes@prevas.dk> you wrote:
> Unlike most other functions declared in env.h, the return value of
> this one also isn't documented. It only has a single caller, which
> ignores the return value. And the companion, env_set_default(),
> already has void as return type.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Naked-by: Wolfgang Denk <wd@denx.de>

This is not a good idea.  himport_r() can run into problems, and
then it returns an error code. Ignoring errors is never correct.

Instead, the caller should be fixed to add proper error handling.

env_set_default() at least prints a proper error message.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Don't have a battle of wits with an unarmed opponent.

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Rasmus Villemoes]

It can be useful to use the same U-Boot binary for multiple purposes,
say the normal one, one for developers that allow breaking into the
U-Boot shell, and one for use during bootstrapping which runs a
special-purpose bootcmd. To that end, allow the control dtb to contain
a /config/default-enviroment property, whose value will be used to
amend the default environment baked into the U-Boot binary itself.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 cmd/nvedit.c | 37 +++++++++++++++++++++++++++++++++++++
 env/Kconfig  | 21 +++++++++++++++++++++
 env/common.c | 19 +++++++++++++++++++
 3 files changed, 77 insertions(+)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 7fce723800..eda8b3b9d2 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -703,6 +703,39 @@ char *from_env(const char *envvar)
 	return ret;
 }
 
+static int env_get_f_fdt(const char *name, char *buf, unsigned len)
+{
+	const char *env;
+	int plen, nlen, n;
+
+	if (!IS_ENABLED(CONFIG_ENV_AMEND_DEFAULT_FROM_FDT))
+		return 0;
+
+	env = fdtdec_get_config_property(gd->fdt_blob, "default-environment",
+					 &plen);
+	if (!env)
+		return 0;
+
+	nlen = strlen(name);
+	while (plen > nlen) {
+		if (memcmp(name, env, nlen) == 0 && env[nlen] == '=') {
+			/* Found. Copy value. */
+			n = strlcpy(buf, &env[nlen + 1], len);
+			if (n < len)
+				return n;
+
+			printf("env_buf [%u bytes] too small for value of \"%s\"\n",
+			       len, name);
+			return len;
+		}
+		/* Skip this key=val pair. */
+		n = strlen(env) + 1;
+		plen -= n;
+		env += n;
+	}
+	return 0;
+}
+
 /*
  * Look up variable from environment for restricted C runtime env.
  */
@@ -710,6 +743,10 @@ int env_get_f(const char *name, char *buf, unsigned len)
 {
 	int i, nxt, c;
 
+	i = env_get_f_fdt(name, buf, len);
+	if (i)
+		return i;
+
 	for (i = 0; env_get_char(i) != '\0'; i = nxt + 1) {
 		int val, n;
 
diff --git a/env/Kconfig b/env/Kconfig
index 67ce93061b..66bbac42c7 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -646,6 +646,27 @@ config DELAY_ENVIRONMENT
 	  later by U-Boot code. With CONFIG_OF_CONTROL this is instead
 	  controlled by the value of /config/load-environment.
 
+config ENV_AMEND_DEFAULT_FROM_FDT
+	bool "Amend default environment by /config/default-environment property"
+	depends on OF_CONTROL
+	help
+	  The default environment built into the U-Boot binary is a
+	  static array defined from various CONFIG_ options, or via
+	  CONFIG_DEFAULT_ENV_FILE. Selecting this option means that
+	  whenever that default environment is used (either for
+	  populating the initial environment, or for resetting
+	  specific variables to their default value), the device tree
+	  property /config/default-environment is also consulted, and
+	  values found there have precedence over those in the static
+	  array. That property should be a series of "key=value"
+	  pairs, e.g.
+
+	  /config {
+	      default-environment = "ipaddr=1.2.3.4",
+	                            "bootcmd=tftp $loadaddr foo.itb; bootm $loadaddr",
+				    "bootdelay=5";
+	  }
+
 config ENV_APPEND
 	bool "Always append the environment with new data"
 	default n
diff --git a/env/common.c b/env/common.c
index 7363da849b..8d0e45fde6 100644
--- a/env/common.c
+++ b/env/common.c
@@ -63,6 +63,22 @@ char *env_get_default(const char *name)
 	return ret_val;
 }
 
+static void env_amend_default_from_fdt(int flags, int nvars, char *const vars[])
+{
+	const void *val;
+	int len;
+
+	if (!IS_ENABLED(CONFIG_ENV_AMEND_DEFAULT_FROM_FDT))
+		return;
+
+	val = fdtdec_get_config_property(gd->fdt_blob, "default-environment",
+					 &len);
+	if (!val)
+		return;
+
+	himport_r(&env_htab, val, len, '\0', flags, 0, nvars, vars);
+}
+
 void env_set_default(const char *s, int flags)
 {
 	if (sizeof(default_environment) > ENV_SIZE) {
@@ -88,6 +104,8 @@ void env_set_default(const char *s, int flags)
 		pr_err("## Error: Environment import failed: errno = %d\n",
 		       errno);
 
+	env_amend_default_from_fdt(flags | H_NOCLEAR, 0, NULL);
+
 	gd->flags |= GD_FLG_ENV_READY;
 	gd->flags |= GD_FLG_ENV_DEFAULT;
 }
@@ -104,6 +122,7 @@ void env_set_default_vars(int nvars, char * const vars[], int flags)
 	himport_r(&env_htab, (const char *)default_environment,
 				sizeof(default_environment), '\0',
 				flags, 0, nvars, vars);
+	env_amend_default_from_fdt(flags, nvars, vars);
 }
 
 /*
-- 
2.23.0

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Wolfgang Denk]

Dear Rasmus Villemoes,

In message <20201110202603.20944-5-rasmus.villemoes@prevas.dk> you wrote:
> It can be useful to use the same U-Boot binary for multiple purposes,
> say the normal one, one for developers that allow breaking into the
> U-Boot shell, and one for use during bootstrapping which runs a
> special-purpose bootcmd. To that end, allow the control dtb to contain
> a /config/default-enviroment property, whose value will be used to
> amend the default environment baked into the U-Boot binary itself.

No, this is not what should be done.

Please try to get used to the idea behind the so called "default
environment".  Only now I realize that this was a badly chosen name,
but last_resort_in_case_of_emergencies_environment would have had
other problems.

The default environment is something which is NOT INTENDED for
regular use.  it is what you will fall back to in case (and ONLY in
that case) when your regular persistent environment cannot be used,
for example because it is not readable (I/O errors or such) or not
properly initialized or corrupted (CRC checksum error).

It is not the intended use but still somewhat acceptable to use it
as initial data to populate the regular environment in other cases,
too.  But that's it.

Apending data to it is not acceptable.  If you need to append data,
then only to the regular environment.


And please, for the sake of avoiding further confusiion, please do
not name this "default-environment".

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Real Programmers always confuse Christmas and Halloween because
OCT 31 == DEC 25 !  - Andrew Rutherford (andrewr at ucs.adelaide.edu.au)

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Simon Glass]

Hi Rasmus,

On Thu, 12 Nov 2020 at 12:59, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Rasmus Villemoes,
>
> In message <20201110202603.20944-5-rasmus.villemoes@prevas.dk> you wrote:
> > It can be useful to use the same U-Boot binary for multiple purposes,
> > say the normal one, one for developers that allow breaking into the
> > U-Boot shell, and one for use during bootstrapping which runs a
> > special-purpose bootcmd. To that end, allow the control dtb to contain
> > a /config/default-enviroment property, whose value will be used to
> > amend the default environment baked into the U-Boot binary itself.
>
> No, this is not what should be done.
>
> Please try to get used to the idea behind the so called "default
> environment".  Only now I realize that this was a badly chosen name,
> but last_resort_in_case_of_emergencies_environment would have had
> other problems.
>
> The default environment is something which is NOT INTENDED for
> regular use.  it is what you will fall back to in case (and ONLY in
> that case) when your regular persistent environment cannot be used,
> for example because it is not readable (I/O errors or such) or not
> properly initialized or corrupted (CRC checksum error).
>
> It is not the intended use but still somewhat acceptable to use it
> as initial data to populate the regular environment in other cases,
> too.  But that's it.
>
> Apending data to it is not acceptable.  If you need to append data,
> then only to the regular environment.
>
>
> And please, for the sake of avoiding further confusiion, please do
> not name this "default-environment".

Apart from what Wolfgang says here, it does seem useful.

I wonder if we should have a way to load the (whole) environment from DT?

Regards,
Simon

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Rasmus Villemoes]

On 17/11/2020 00.52, Simon Glass wrote:
> Hi Rasmus,
> 
> On Thu, 12 Nov 2020 at 12:59, Wolfgang Denk <wd@denx.de> wrote:
>>
>> Dear Rasmus Villemoes,
>>
>> In message <20201110202603.20944-5-rasmus.villemoes@prevas.dk> you wrote:
>>> It can be useful to use the same U-Boot binary for multiple purposes,
>>> say the normal one, one for developers that allow breaking into the
>>> U-Boot shell, and one for use during bootstrapping which runs a
>>> special-purpose bootcmd. To that end, allow the control dtb to contain
>>> a /config/default-enviroment property, whose value will be used to
>>> amend the default environment baked into the U-Boot binary itself.
>>
>> No, this is not what should be done.
>>
>> Please try to get used to the idea behind the so called "default
>> environment".  Only now I realize that this was a badly chosen name,
>> but last_resort_in_case_of_emergencies_environment would have had
>> other problems.
>>
>> The default environment is something which is NOT INTENDED for
>> regular use.  it is what you will fall back to in case (and ONLY in
>> that case) when your regular persistent environment cannot be used,
>> for example because it is not readable (I/O errors or such) or not
>> properly initialized or corrupted (CRC checksum error).

You seem to be ignoring the many cases where one chooses, for
simplicity, robustness and/or security, not to have a writable
environment at all.

>> It is not the intended use but still somewhat acceptable to use it
>> as initial data to populate the regular environment in other cases,
>> too.  But that's it.
>>
>> Apending data to it is not acceptable.  If you need to append data,
>> then only to the regular environment.

I'm not really doing anything other than allowing a
CONFIG_ENV_EXTRA_SETTINGS to live in .dtb rather than cooking all of it
into the U-Boot binary itself. That's where board-specific additions are
supposed to live nowadays I'd assume.

>> And please, for the sake of avoiding further confusiion, please do
>> not name this "default-environment".

Sure. So would you be ok with some /config/extra-environment node which
gets appended after the normal environment has been loaded? Then if I
set CONFIG_ENV_IS_NOWHERE, I'd get what I have now. The only problem
with that is that it might be a little weird to have static content of
the chosen control dtb override something that might have been loaded
from a writable storage location. But that's not really an intended
combination anyway, and I can probably add a knob that allows one to
choose whether the .dtb settings should be applied or not.

> Apart from what Wolfgang says here, it does seem useful.
> 
> I wonder if we should have a way to load the (whole) environment from DT?

That will be my second choice, i.e. adding a "CONFIG_ENV_IS_IN_DTB"
which obviously doesn't support saving, but has the advantages I'm after
here of using one U-Boot binary with slightly different environments.

Rasmus

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Wolfgang Denk]

Dear Rasmus,

In message <76615995-6700-1b3e-b598-4913e9882c26@prevas.dk> you wrote:
>
> >> The default environment is something which is NOT INTENDED for
> >> regular use.  it is what you will fall back to in case (and ONLY in
> >> that case) when your regular persistent environment cannot be used,
> >> for example because it is not readable (I/O errors or such) or not
> >> properly initialized or corrupted (CRC checksum error).
>
> You seem to be ignoring the many cases where one chooses, for
> simplicity, robustness and/or security, not to have a writable
> environment at all.

You seem to ignore what I write.

Yes, there are many use cases where no writable environment is
needed / wanted, but this is not what the default environment was
intended for.

For such cases some null driver similar to /dev/null should be used.

> I'm not really doing anything other than allowing a
> CONFIG_ENV_EXTRA_SETTINGS to live in .dtb rather than cooking all of it
> into the U-Boot binary itself. That's where board-specific additions are
> supposed to live nowadays I'd assume.

Your statement is incorrect.  CONFIG_ENV_EXTRA_SETTINGS is something
that is processed at compile time, while your code attempts to
modify the default environment in run time.  these are totally
different things.

> >> And please, for the sake of avoiding further confusiion, please do
> >> not name this "default-environment".
>
> Sure. So would you be ok with some /config/extra-environment node which
> gets appended after the normal environment has been loaded? Then if I

In principle yes, I see that this is a nice and useful feature.

Just the notation of "append" seems wrong to me.  We already have
"env import" which can import additional / new environmane settings
in different formats.  What I think should be done is extending this
by support for a new format (you may call it driver if you want)
to import environment settings from a DTB.

> set CONFIG_ENV_IS_NOWHERE, I'd get what I have now. The only problem

Can't you see that this is not logical?  If the environment is
nowhere, then how can you add something to it?

> with that is that it might be a little weird to have static content of
> the chosen control dtb override something that might have been loaded
> from a writable storage location. But that's not really an intended
> combination anyway, and I can probably add a knob that allows one to
> choose whether the .dtb settings should be applied or not.

This is not weird in any way - this is what the "env import" command
does by definition: it imports environment settigns from some other
storage.

> > I wonder if we should have a way to load the (whole) environment from DT?
>
> That will be my second choice, i.e. adding a "CONFIG_ENV_IS_IN_DTB"
> which obviously doesn't support saving, but has the advantages I'm after
> here of using one U-Boot binary with slightly different environments.

I see no difference here.  "env import" into an empty environment
does just that.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The use of COBOL cripples the mind; its teaching  should,  therefore,
be regarded as a criminal offense.                   - E. W. Dijkstra

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Rasmus Villemoes]

On 20/11/2020 11.13, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <76615995-6700-1b3e-b598-4913e9882c26@prevas.dk> you wrote:
>>
>> Sure. So would you be ok with some /config/extra-environment node which
>> gets appended after the normal environment has been loaded? Then if I
> 
> In principle yes, I see that this is a nice and useful feature.
> 
> Just the notation of "append" seems wrong to me.  We already have
> "env import" which can import additional / new environmane settings
> in different formats.  What I think should be done is extending this
> by support for a new format (you may call it driver if you want)
> to import environment settings from a DTB.
> 
>> set CONFIG_ENV_IS_NOWHERE, I'd get what I have now. The only problem
> 
> Can't you see that this is not logical?  If the environment is
> nowhere, then how can you add something to it?

That's silly. Even with CONFIG_ENV_IS_NOWHERE, "env set" still works
just fine. So of course one can add something to the (runtime)
environment, even if the source of the original runtime environment
happened to be default_environment[] and those changes cannot be persisted.

>>> I wonder if we should have a way to load the (whole) environment from DT?
>>
>> That will be my second choice, i.e. adding a "CONFIG_ENV_IS_IN_DTB"
>> which obviously doesn't support saving, but has the advantages I'm after
>> here of using one U-Boot binary with slightly different environments.
> 
> I see no difference here.  "env import" into an empty environment
> does just that.

The problem is, by the time it's possible to do an "env import" (no
sooner than $preboot is executed I assume?) or any other shell command,
U-Boot may already have read a bunch variables affecting its execution
from the environment.

So I think that for my _current_ use cases, doing it via a preboot
command may be enough - from reading the code, it does seem that e.g.
bootretry is only read after the preboot command has run.

I'll see if I can come up with something else.

Rasmus

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Wolfgang Denk]

Dear Rasmus,

In message <2edc1fb5-e723-fbd6-56da-bc0dea282343@prevas.dk> you wrote:
>
> >> set CONFIG_ENV_IS_NOWHERE, I'd get what I have now. The only problem
> > 
> > Can't you see that this is not logical?  If the environment is
> > nowhere, then how can you add something to it?
>
> That's silly. Even with CONFIG_ENV_IS_NOWHERE, "env set" still works
> just fine. So of course one can add something to the (runtime)

I'm sorry, but it seems you lack basic understanding of the design of
the environment system in U-Boot.

I have explained this repeatedly in the last few weeks, so here once
more:

CONFIG_ENV_IS_* defines, where the persistent storage for the
environment is located, that is where it is initialized from at
reset, and where it is saved to when you use the "env save" command.

Of course ""env set" still works perfectly fine on the in-memory
copy of the environment, as this has nothing to do with whether you
can save this data to some persistent storage or not.


> environment, even if the source of the original runtime environment
> happened to be default_environment[] and those changes cannot be persisted.

And this is where the mis-use starts.  default_environment[] has
never been intended to be used as regular data to initialize the
in-memory copy of the environment from.  It was there to be used in
emergency cases only, like then there were I/O errors on the storage
device or the saved copy of the environment had been corrupted, so
the checksum would not macth.


> > I see no difference here.  "env import" into an empty environment
> > does just that.
>
> The problem is, by the time it's possible to do an "env import" (no
> sooner than $preboot is executed I assume?) or any other shell command,
> U-Boot may already have read a bunch variables affecting its execution
> from the environment.

You can call the C function equivalent of the "env import" shell
command right ater the initialization of the environment - i. e. at
lease as early as any of your code can be used.

> So I think that for my _current_ use cases, doing it via a preboot
> command may be enough - from reading the code, it does seem that e.g.
> bootretry is only read after the preboot command has run.

I was not talking about implementing it by scripts only.  I just
wanted to point to the existing functionality - whether you call
this from th shell or from C code is your choice.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Just think of a computer as hardware you can program."
- Nigel de la Tierre

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ3Tdkr-r0D1q-nk48pfGvoMG4ZodUJLh_ZxMQAQb3E=wg@mail.gmail.com> you wrote:
>
> > Apending data to it is not acceptable.  If you need to append data,
> > then only to the regular environment.
> >
> > And please, for the sake of avoiding further confusiion, please do
> > not name this "default-environment".
>
> Apart from what Wolfgang says here, it does seem useful.

On the other hand I wonder in which way "appending to the existing
environment" is different from what "env import" does?

> I wonder if we should have a way to load the (whole) environment from DT?

You mean adding a DT type specifier to the "env import" command?

Sounds good to me...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The White Rabbit put on his spectacles. "Where shall I begin,  please
your Majesty ?" he asked.
"Begin at the beginning,", the King said, very gravely,  "and  go  on
till you come to the end: then stop."                -- Lewis Carroll

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Tom Rini]

On Tue, Nov 17, 2020 at 12:31:07PM +0100, Wolfgang Denk wrote:
> Dear Simon,
> 
> In message <CAPnjgZ3Tdkr-r0D1q-nk48pfGvoMG4ZodUJLh_ZxMQAQb3E=wg@mail.gmail.com> you wrote:
> >
> > > Apending data to it is not acceptable.  If you need to append data,
> > > then only to the regular environment.
> > >
> > > And please, for the sake of avoiding further confusiion, please do
> > > not name this "default-environment".
> >
> > Apart from what Wolfgang says here, it does seem useful.
> 
> On the other hand I wonder in which way "appending to the existing
> environment" is different from what "env import" does?
> 
> > I wonder if we should have a way to load the (whole) environment from DT?
> 
> You mean adding a DT type specifier to the "env import" command?
> 
> Sounds good to me...

Adding in Marek as well since I believe he's been doing things with
append-only environment, and it would be good to make sure we have
something that fits everyones needs, and doesn't break what people are
already doing as well.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201117/f0d21e7f/attachment.sig>

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Simon Glass]

Hi,

On Tue, 17 Nov 2020 at 11:23, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Nov 17, 2020 at 12:31:07PM +0100, Wolfgang Denk wrote:
> > Dear Simon,
> >
> > In message <CAPnjgZ3Tdkr-r0D1q-nk48pfGvoMG4ZodUJLh_ZxMQAQb3E=wg@mail.gmail.com> you wrote:
> > >
> > > > Apending data to it is not acceptable.  If you need to append data,
> > > > then only to the regular environment.
> > > >
> > > > And please, for the sake of avoiding further confusiion, please do
> > > > not name this "default-environment".
> > >
> > > Apart from what Wolfgang says here, it does seem useful.
> >
> > On the other hand I wonder in which way "appending to the existing
> > environment" is different from what "env import" does?
> >
> > > I wonder if we should have a way to load the (whole) environment from DT?
> >
> > You mean adding a DT type specifier to the "env import" command?
> >
> > Sounds good to me...
>
> Adding in Marek as well since I believe he's been doing things with
> append-only environment, and it would be good to make sure we have
> something that fits everyones needs, and doesn't break what people are
> already doing as well.

Some years ago I did a series to allow the environment to come from a
text file, thus avoiding the \0 stuff. Now binman has a 'u-boot-env'
entry type, allowing creating an environment from a text file, with
suitable checksumming.

There is some advantage to having a default environment compiled into
U-Boot that covers everything needed to boot. For one, the environment
can be clobbered from userspace, which would otherwise render the
device unbootable. For another, it is more secure to avoid loading
unsigned data (the environment) from flash. Generally, for a secure
boot, one would need to avoid loading the environment, at least
without a lot of careful filtering.

Regards,
Simon

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ1C3TQWkUuNQc0BHOGE957s0K-nxFmW3PVO_ObOcKwv+Q@mail.gmail.com> you wrote:
>
> Some years ago I did a series to allow the environment to come from a
> text file, thus avoiding the \0 stuff.

"env import -t" does that, you know?

> Now binman has a 'u-boot-env'
> entry type, allowing creating an environment from a text file, with
> suitable checksumming.

"env import -b" ...

> There is some advantage to having a default environment compiled into
> U-Boot that covers everything needed to boot. For one, the environment
> can be clobbered from userspace, which would otherwise render the
> device unbootable. For another, it is more secure to avoid loading
> unsigned data (the environment) from flash. Generally, for a secure
> boot, one would need to avoid loading the environment, at least
> without a lot of careful filtering.

One idea behind my rewrite of the environment handling (when I added
hast table support) was that there should be more than one way to
initialize the environment.  Until then, we always had exactly one
fixed location for the environment, probaly with a redundant copy.

The code we have now actuially allows for a much greater
flexibility.  You can initialize the environment from a selection of
copies, and (now, with proper driver support) also from several
devices.  If doen correctly, we could implement things like
"profiles", where for example each user (or use case) can select his
specific profile, initialize the environment from that, and save
change to that.  Ths could - for example - be used to switch between
"development" and "production" modes. A "reset to factory defaults"
would then just be an import from the (read-only) factory-defaults
copy.  etc.

Importing from a DT is just a logical extension as it is considered
just another storage device / driver.  [In a Unix environment, all
these would just be "files".]

It's all there.  We just have to use it.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Is a computer language with goto's totally Wirth-less?

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Simon Glass]

Hi Wolfgang,

On Fri, 20 Nov 2020 at 04:28, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon,
>
> In message <CAPnjgZ1C3TQWkUuNQc0BHOGE957s0K-nxFmW3PVO_ObOcKwv+Q@mail.gmail.com> you wrote:
> >
> > Some years ago I did a series to allow the environment to come from a
> > text file, thus avoiding the \0 stuff.
>
> "env import -t" does that, you know?

Not to derail the discussion, but I mean as a way of specifying the
default environment.

>
> > Now binman has a 'u-boot-env'
> > entry type, allowing creating an environment from a text file, with
> > suitable checksumming.
>
> "env import -b" ...

For the binman case, this is an environment area which is loaded by
U-Boot on boot, so does not need any such command.

>
> > There is some advantage to having a default environment compiled into
> > U-Boot that covers everything needed to boot. For one, the environment
> > can be clobbered from userspace, which would otherwise render the
> > device unbootable. For another, it is more secure to avoid loading
> > unsigned data (the environment) from flash. Generally, for a secure
> > boot, one would need to avoid loading the environment, at least
> > without a lot of careful filtering.
>
> One idea behind my rewrite of the environment handling (when I added
> hast table support) was that there should be more than one way to
> initialize the environment.  Until then, we always had exactly one
> fixed location for the environment, probaly with a redundant copy.
>
> The code we have now actuially allows for a much greater
> flexibility.  You can initialize the environment from a selection of
> copies, and (now, with proper driver support) also from several
> devices.  If doen correctly, we could implement things like
> "profiles", where for example each user (or use case) can select his
> specific profile, initialize the environment from that, and save
> change to that.  Ths could - for example - be used to switch between
> "development" and "production" modes. A "reset to factory defaults"
> would then just be an import from the (read-only) factory-defaults
> copy.  etc.
>
> Importing from a DT is just a logical extension as it is considered
> just another storage device / driver.  [In a Unix environment, all
> these would just be "files".]
>
> It's all there.  We just have to use it.

OK I'll let you figure this out with Rasmus.

Regards,
Simon

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ1mnMwoTZJSMGEnRpcbc7Bs4GhzBmd=D5y5-mrnb+gpnw@mail.gmail.com> you wrote:
>
> > "env import -t" does that, you know?
>
> Not to derail the discussion, but I mean as a way of specifying the
> default environment.

That was a build-time option, then.  Sorry, I now remeber.

> OK I'll let you figure this out with Rasmus.

Hm... 

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Love is an ideal thing, marriage a real thing; a  confusion  of  the
real with the ideal never goes unpunished."                  - Goethe

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Wolfgang Denk]

Dear Tom,

In message <20201117182353.GB5340@bill-the-cat> you wrote:
> 
> Adding in Marek as well since I believe he's been doing things with
> append-only environment, and it would be good to make sure we have
> something that fits everyones needs, and doesn't break what people are
> already doing as well.

Actually we should stop people from doing things the wrong way.
Misusing the "default environment" is not a good thing.

That does not mean that the use cases should not be supported - but
in a correct way.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Der Verlierer sagt: "Es k?nnte m?glich sein, aber es ist zu schwierig."
Der Gewinner sagt: "Es k?nnte schwierig sein, aber es ist m?glich."

[PATCH 4/5] env: allow default environment to be amended from control dtb

[Stefano Babic]

Hi Rasmus,

On 10.11.20 21:26, Rasmus Villemoes wrote:
> It can be useful to use the same U-Boot binary for multiple purposes,
> say the normal one, one for developers that allow breaking into the
> U-Boot shell, and one for use during bootstrapping which runs a
> special-purpose bootcmd. To that end, allow the control dtb to contain
> a /config/default-enviroment property, whose value will be used to
> amend the default environment baked into the U-Boot binary itself.
> 

I have not checked the patch itself, but I disagree on the concept.

What you suggest works outside a context where the environment is not
just set by U-Boot. Simple use case is when you want to update the
software and after the update you need to set the environment. The user
space application needs to know the starting environment, else it breaks
the board. It is already a pain that the default environemtn is really
used as "the environment", that is the device simply boots. With your
changes, we have then a set of "default" environment and on user space
you cannot know which of them was taken by U-Boot (because this is
dynamically done by U-Boot reading the fit Image).

So from my point of view, this is NAK.

Anyway, why do we try to addd everything to the "default" environment
aka CONFIG_EXTRA_ENV_SETTINGS instead of storing together with u-boot a
valid copy of the environment ?

Best regards,
Stefano

> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  cmd/nvedit.c | 37 +++++++++++++++++++++++++++++++++++++
>  env/Kconfig  | 21 +++++++++++++++++++++
>  env/common.c | 19 +++++++++++++++++++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 7fce723800..eda8b3b9d2 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -703,6 +703,39 @@ char *from_env(const char *envvar)
>  	return ret;
>  }
>  
> +static int env_get_f_fdt(const char *name, char *buf, unsigned len)
> +{
> +	const char *env;
> +	int plen, nlen, n;
> +
> +	if (!IS_ENABLED(CONFIG_ENV_AMEND_DEFAULT_FROM_FDT))
> +		return 0;
> +
> +	env = fdtdec_get_config_property(gd->fdt_blob, "default-environment",
> +					 &plen);
> +	if (!env)
> +		return 0;
> +
> +	nlen = strlen(name);
> +	while (plen > nlen) {
> +		if (memcmp(name, env, nlen) == 0 && env[nlen] == '=') {
> +			/* Found. Copy value. */
> +			n = strlcpy(buf, &env[nlen + 1], len);
> +			if (n < len)
> +				return n;
> +
> +			printf("env_buf [%u bytes] too small for value of \"%s\"\n",
> +			       len, name);
> +			return len;
> +		}
> +		/* Skip this key=val pair. */
> +		n = strlen(env) + 1;
> +		plen -= n;
> +		env += n;
> +	}
> +	return 0;
> +}
> +
>  /*
>   * Look up variable from environment for restricted C runtime env.
>   */
> @@ -710,6 +743,10 @@ int env_get_f(const char *name, char *buf, unsigned len)
>  {
>  	int i, nxt, c;
>  
> +	i = env_get_f_fdt(name, buf, len);
> +	if (i)
> +		return i;
> +
>  	for (i = 0; env_get_char(i) != '\0'; i = nxt + 1) {
>  		int val, n;
>  
> diff --git a/env/Kconfig b/env/Kconfig
> index 67ce93061b..66bbac42c7 100644
> --- a/env/Kconfig
> +++ b/env/Kconfig
> @@ -646,6 +646,27 @@ config DELAY_ENVIRONMENT
>  	  later by U-Boot code. With CONFIG_OF_CONTROL this is instead
>  	  controlled by the value of /config/load-environment.
>  
> +config ENV_AMEND_DEFAULT_FROM_FDT
> +	bool "Amend default environment by /config/default-environment property"
> +	depends on OF_CONTROL
> +	help
> +	  The default environment built into the U-Boot binary is a
> +	  static array defined from various CONFIG_ options, or via
> +	  CONFIG_DEFAULT_ENV_FILE. Selecting this option means that
> +	  whenever that default environment is used (either for
> +	  populating the initial environment, or for resetting
> +	  specific variables to their default value), the device tree
> +	  property /config/default-environment is also consulted, and
> +	  values found there have precedence over those in the static
> +	  array. That property should be a series of "key=value"
> +	  pairs, e.g.
> +
> +	  /config {
> +	      default-environment = "ipaddr=1.2.3.4",
> +	                            "bootcmd=tftp $loadaddr foo.itb; bootm $loadaddr",
> +				    "bootdelay=5";
> +	  }
> +
>  config ENV_APPEND
>  	bool "Always append the environment with new data"
>  	default n
> diff --git a/env/common.c b/env/common.c
> index 7363da849b..8d0e45fde6 100644
> --- a/env/common.c
> +++ b/env/common.c
> @@ -63,6 +63,22 @@ char *env_get_default(const char *name)
>  	return ret_val;
>  }
>  
> +static void env_amend_default_from_fdt(int flags, int nvars, char *const vars[])
> +{
> +	const void *val;
> +	int len;
> +
> +	if (!IS_ENABLED(CONFIG_ENV_AMEND_DEFAULT_FROM_FDT))
> +		return;
> +
> +	val = fdtdec_get_config_property(gd->fdt_blob, "default-environment",
> +					 &len);
> +	if (!val)
> +		return;
> +
> +	himport_r(&env_htab, val, len, '\0', flags, 0, nvars, vars);
> +}
> +
>  void env_set_default(const char *s, int flags)
>  {
>  	if (sizeof(default_environment) > ENV_SIZE) {
> @@ -88,6 +104,8 @@ void env_set_default(const char *s, int flags)
>  		pr_err("## Error: Environment import failed: errno = %d\n",
>  		       errno);
>  
> +	env_amend_default_from_fdt(flags | H_NOCLEAR, 0, NULL);
> +
>  	gd->flags |= GD_FLG_ENV_READY;
>  	gd->flags |= GD_FLG_ENV_DEFAULT;
>  }
> @@ -104,6 +122,7 @@ void env_set_default_vars(int nvars, char * const vars[], int flags)
>  	himport_r(&env_htab, (const char *)default_environment,
>  				sizeof(default_environment), '\0',
>  				flags, 0, nvars, vars);
> +	env_amend_default_from_fdt(flags, nvars, vars);
>  }
>  
>  /*
> 


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[PATCH 5/5] test: add tests for default environment

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 configs/sandbox64_defconfig |   1 +
 configs/sandbox_defconfig   |   1 +
 include/configs/sandbox.h   |   8 ++-
 test/env/Makefile           |   1 +
 test/env/default.c          | 102 ++++++++++++++++++++++++++++++++++++
 5 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 test/env/default.c

diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index dc993cd13a..fa8f5190ed 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -91,6 +91,7 @@ CONFIG_ENV_IS_NOWHERE=y
 CONFIG_ENV_IS_IN_EXT4=y
 CONFIG_ENV_EXT4_INTERFACE="host"
 CONFIG_ENV_EXT4_DEVICE_AND_PART="0:0"
+CONFIG_ENV_AMEND_DEFAULT_FROM_FDT=y
 CONFIG_BOOTP_SEND_HOSTNAME=y
 CONFIG_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index f2a767a4cd..f1a4ecbf01 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -105,6 +105,7 @@ CONFIG_ENV_IS_NOWHERE=y
 CONFIG_ENV_IS_IN_EXT4=y
 CONFIG_ENV_EXT4_INTERFACE="host"
 CONFIG_ENV_EXT4_DEVICE_AND_PART="0:0"
+CONFIG_ENV_AMEND_DEFAULT_FROM_FDT=y
 CONFIG_BOOTP_SEND_HOSTNAME=y
 CONFIG_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index e0708fe573..928a80d872 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -106,11 +106,17 @@
 	"scriptaddr=0x1000\0" \
 	"pxefile_addr_r=0x2000\0"
 
+#define TEST_ENV_SETTINGS \
+	"test0=a\0"	  \
+	"test1=b\0"	  \
+	"test2=c\0"
+
 #define CONFIG_EXTRA_ENV_SETTINGS \
 	SANDBOX_SERIAL_SETTINGS \
 	SANDBOX_ETH_SETTINGS \
 	BOOTENV \
-	MEM_LAYOUT_ENV_SETTINGS
+	MEM_LAYOUT_ENV_SETTINGS \
+	TEST_ENV_SETTINGS
 
 #ifndef CONFIG_SPL_BUILD
 #define CONFIG_SYS_IDE_MAXBUS		1
diff --git a/test/env/Makefile b/test/env/Makefile
index 5c8eae31b0..740b9c522e 100644
--- a/test/env/Makefile
+++ b/test/env/Makefile
@@ -5,3 +5,4 @@
 obj-y += cmd_ut_env.o
 obj-y += attr.o
 obj-y += hashtable.o
+obj-y += default.o
diff --git a/test/env/default.c b/test/env/default.c
new file mode 100644
index 0000000000..36fae5d783
--- /dev/null
+++ b/test/env/default.c
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <common.h>
+#include <command.h>
+#include <env_attr.h>
+#include <test/env.h>
+#include <test/ut.h>
+#include <fdt_support.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static char *testvars[] = {"test0", "test1", "test2", "test3"};
+
+static int test_amend_default_from_fdt(struct unit_test_state *uts)
+{
+	static const char def_env[] = "test1=x\0test2=\0test3=y";
+	void *blob = (void*)gd->fdt_blob;
+	const char *val;
+
+	ut_assert(fdt_find_or_add_subnode(blob, 0, "config") >= 0);
+	ut_assertok(fdt_find_and_setprop(blob, "/config", "default-environment",
+					  def_env, sizeof(def_env), 1));
+
+	env_set("test0", NULL);
+	env_set_default_vars(ARRAY_SIZE(testvars), testvars, 0);
+
+	val = env_get("test0");
+	ut_assertnonnull(val);
+	ut_asserteq_str("a", val);
+
+	val = env_get("test1");
+	ut_assertnonnull(val);
+	ut_asserteq_str("x", val);
+
+	val = env_get("test2");
+	ut_assertnull(val);
+
+	val = env_get("test3");
+	ut_assertnonnull(val);
+	ut_asserteq_str("y", val);
+
+	return 0;
+}
+
+static int env_test_default(struct unit_test_state *uts)
+{
+	const char *val;
+	int ret = 0;
+	void *blob;
+	const void *orig_blob;
+	int blob_sz;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(testvars); ++i) {
+		ut_assertok(env_set(testvars[i], "ggg"));
+		val = env_get(testvars[i]);
+		ut_assertnonnull(val);
+		ut_asserteq_str(val, "ggg");
+	}
+
+	env_set_default_vars(ARRAY_SIZE(testvars), testvars, 0);
+
+	val = env_get("test0");
+	ut_assertnonnull(val);
+	ut_asserteq_str("a", val);
+
+	val = env_get("test1");
+	ut_assertnonnull(val);
+	ut_asserteq_str("b", val);
+
+	val = env_get("test2");
+	ut_assertnonnull(val);
+	ut_asserteq_str("c", val);
+
+	/*
+	 * env_set_default_vars() leaves existing variables alone that
+	 * are not defined in the default environment.
+	 */
+	val = env_get("test3");
+	ut_assertnonnull(val);
+	ut_asserteq_str("ggg", val);
+
+	if (!IS_ENABLED(CONFIG_ENV_AMEND_DEFAULT_FROM_FDT))
+		return ret;
+
+	blob_sz = fdt_totalsize(gd->fdt_blob) + 256;
+	blob = malloc(blob_sz);
+	ut_assertnonnull(blob);
+
+	/* Make a writable copy of the fdt blob */
+	ut_assertok(fdt_open_into(gd->fdt_blob, blob, blob_sz));
+	orig_blob = gd->fdt_blob;
+	gd->fdt_blob = blob;
+
+	ret = test_amend_default_from_fdt(uts);
+
+	gd->fdt_blob = orig_blob;
+	free(blob);
+
+	return ret;
+}
+ENV_TEST(env_test_default, 0);
-- 
2.23.0