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

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[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

Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/5] fdtdec: introduce fdtdec_get_config_property

Rasmus Villemoes
In reply to this post by 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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Rasmus Villemoes
In reply to this post by 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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Rasmus Villemoes
In reply to this post by 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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Rasmus Villemoes
In reply to this post by Rasmus Villemoes
Signed-off-by: Rasmus Villemoes <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Wolfgang Denk
In reply to this post by Rasmus Villemoes
Dear Rasmus,

In message <[hidden email]> 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 <[hidden email]>

Naked-by: Wolfgang Denk <[hidden email]>

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: [hidden email]
Don't have a battle of wits with an unarmed opponent.
Reply | Threaded
Open this post in threaded view
|

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

Wolfgang Denk
In reply to this post by Rasmus Villemoes
Dear Rasmus Villemoes,

In message <[hidden email]> 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: [hidden email]
Real Programmers always confuse Christmas and Halloween because
OCT 31 == DEC 25 !  - Andrew Rutherford ([hidden email])
Reply | Threaded
Open this post in threaded view
|

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

Simon Glass-3
In reply to this post by Rasmus Villemoes
On Tue, 10 Nov 2020 at 13:26, Rasmus Villemoes
<[hidden email]> wrote:

>
> Nobody should modify the string returned by
> fdtdec_get_config_string(), so make it return a const pointer.
>
> Signed-off-by: Rasmus Villemoes <[hidden email]>
> ---
>  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 <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

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

Simon Glass-3
In reply to this post by Wolfgang Denk
Hi Rasmus,

On Thu, 12 Nov 2020 at 12:59, Wolfgang Denk <[hidden email]> wrote:

>
> Dear Rasmus Villemoes,
>
> In message <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/5] fdtdec: introduce fdtdec_get_config_property

Simon Glass-3
In reply to this post by Rasmus Villemoes
Hi Rasmus,

On Tue, 10 Nov 2020 at 13:26, Rasmus Villemoes
<[hidden email]> 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 <[hidden email]>
> ---
>  include/fdtdec.h | 14 ++++++++++++++
>  lib/fdtdec.c     | 27 +++++++++++++--------------
>  2 files changed, 27 insertions(+), 14 deletions(-)
>

Reviewed-by: Simon Glass <[hidden email]>

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
Reply | Threaded
Open this post in threaded view
|

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

Rasmus Villemoes
In reply to this post by Simon Glass-3
On 17/11/2020 00.52, Simon Glass wrote:

> Hi Rasmus,
>
> On Thu, 12 Nov 2020 at 12:59, Wolfgang Denk <[hidden email]> wrote:
>>
>> Dear Rasmus Villemoes,
>>
>> In message <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

sbabic
In reply to this post by Rasmus Villemoes
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 <[hidden email]>
> ---
>  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: [hidden email]
=====================================================================
Reply | Threaded
Open this post in threaded view
|

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

Wolfgang Denk
In reply to this post by Simon Glass-3
Dear Simon,

In message <CAPnjgZ3Tdkr-r0D1q-nk48pfGvoMG4ZodUJLh_ZxMQAQb3E=[hidden email]> 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: [hidden email]
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
Reply | Threaded
Open this post in threaded view
|

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

Tom Rini-4
On Tue, Nov 17, 2020 at 12:31:07PM +0100, Wolfgang Denk wrote:

> Dear Simon,
>
> In message <CAPnjgZ3Tdkr-r0D1q-nk48pfGvoMG4ZodUJLh_ZxMQAQb3E=[hidden email]> 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

signature.asc (673 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Simon Glass-3
Hi,

On Tue, 17 Nov 2020 at 11:23, Tom Rini <[hidden email]> wrote:

>
> On Tue, Nov 17, 2020 at 12:31:07PM +0100, Wolfgang Denk wrote:
> > Dear Simon,
> >
> > In message <CAPnjgZ3Tdkr-r0D1q-nk48pfGvoMG4ZodUJLh_ZxMQAQb3E=[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Wolfgang Denk
In reply to this post by Rasmus Villemoes
Dear Rasmus,

In message <[hidden email]> 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: [hidden email]
The use of COBOL cripples the mind; its teaching  should,  therefore,
be regarded as a criminal offense.                   - E. W. Dijkstra
Reply | Threaded
Open this post in threaded view
|

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

Wolfgang Denk
In reply to this post by Tom Rini-4
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: [hidden email]
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."
Reply | Threaded
Open this post in threaded view
|

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

Wolfgang Denk
In reply to this post by Simon Glass-3
Dear Simon,

In message <[hidden email]> 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: [hidden email]
Is a computer language with goto's totally Wirth-less?
Reply | Threaded
Open this post in threaded view
|

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

Rasmus Villemoes
In reply to this post by Wolfgang Denk
On 20/11/2020 11.13, Wolfgang Denk wrote:

> Dear Rasmus,
>
> In message <[hidden email]> 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
12