[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 at 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
=====================================================================