[U-Boot] [RFC, PATCH v4 02/16] env: extend interfaces to import/export U-Boot environment per context
Wolfgang Denk
wd at denx.de
Fri Jul 19 07:38:11 UTC 2019
Dear AKASHI Takahiro,
In message <20190717082525.891-3-takahiro.akashi at linaro.org> you wrote:
> The following interfaces are extended to accept an additional argument,
> context:
> env_import() -> env_import_ext()
> env_export() -> env_export_ext()
> env_load() -> env_load_ext()
> env_save() -> env_save_ext()
Please don't such renames, see previous comments.
> -int env_import(const char *buf, int check)
> +int env_import_ext(const char *buf, enum env_context ctx, int check)
I think this needs at least additional documentation.
>From the explantions so far, a context is always associated with a
variable, i. e. it is a property of the variable. Here it becomes
clear that the context has an additional meaning, separate storage.
It should be documented that one block of variables such as handled
by the env import / export routines will always only process
veriables of a specific context.
> - if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0,
> - 0, NULL)) {
> + if (himport_ext(&env_htab, ctx, (char *)ep->data, ep->data_size,
> + /*
> + * FIXME:
> + * H_NOCLEAR is necessary here to handle
> + * multiple contexts simultaneously.
> + * This, however, breaks backward compatibility.
> + */
> + '\0', H_NOCLEAR, 0, 0, NULL)) {
This is the result of a deign bug. You can never have "multiple
contexts simultaneously", because the code as you designed it always
has only one context per data block - and I can;t see how this could
be changed, as the external storage format ('name=value\0') does not
provide a way to include variable-specific context information.
> - set_default_env("import failed", 0);
> + if (ctx == ENVCTX_UBOOT)
> + set_default_env("import failed", 0);
Should we not also have default settings for other contexts as well?
> +int env_import_redund_ext(const char *buf1, int buf1_read_fail,
> + const char *buf2, int buf2_read_fail,
> + enum env_context ctx)
> +{
> + /* TODO */
> + return 0;
!!
Do you plan to implement redundant storage for UEFI data as well? It
would make sense, wouldn't it?
> - pr_err("Cannot export environment: errno = %d\n", errno);
> + pr_err("Cannot export environment: errno = %d\n",
> + errno);
Why did you change this?
> - env_out->crc = crc32(0, env_out->data, ENV_SIZE);
> + if (*env_out) {
> + envp = *env_out;
> + } else {
> + envp = malloc(sizeof(*envp) + len);
> + if (!envp) {
> + pr_err("Out of memory\n");
> + free(data);
> + return 1;
> + }
> + *env_out = envp;
> +
> + envp->data_size = len;
> + memcpy(envp->data, data, len);
> + free(data);
> + }
It seems you have a memory leak here. I cannot find a free(envp)
anywhere.
Speaking about memory... what is the overall code / data size impact
of this patch set?
> --- a/env/env.c
> +++ b/env/env.c
> @@ -85,12 +85,12 @@ static enum env_location env_locations[] = {
> #endif
> };
>
> -static bool env_has_inited(enum env_location location)
> +static bool env_has_inited(enum env_context ctx, enum env_location location)
> {
> - return gd->env_has_init & BIT(location);
> + return gd->env_has_init[ctx] & BIT(location);
> }
This should be re-considered. GD is a tight resource, so increasing
it's size should be really justified. Also, I wonder if we really
should initialize all contexts the same. We should keep in mind
that many boards already need the (U-Boot) environment in SPL, but
there resource usage is critical in most cases.
I think it would make a lot of sense to initialize only the U-Boot
environment early (like in SPL), and delay this for all other
contexts until U-Boot porper is running, where we have sufficient
resources available.
> -static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
> +static struct env_driver *env_driver_lookup(enum env_context ctx,
> + enum env_operation op, int prio)
> {
> - enum env_location loc = env_get_location(op, prio);
> + enum env_location loc;
> struct env_driver *drv;
>
> + if (ctx == ENVCTX_UBOOT)
> + loc = env_get_location(op, prio);
> + else
> + loc = env_get_location_ext(ctx, op, prio);
This makes no sense. ENVCTX_UBOOT is just one of the available
contexts; no "if" should be needed here.
> - if (!drv->load)
> + if ((ctx == ENVCTX_UBOOT && !drv->load) ||
> + (ctx != ENVCTX_UBOOT && !drv->load_ext))
> continue;
Ditto here.
> - ret = drv->load();
> + if (ctx == ENVCTX_UBOOT)
> + ret = drv->load();
> + else
> + ret = drv->load_ext(ctx);
...and here.
> - env_get_location(ENVOP_LOAD, best_prio);
> + if (ctx == ENVCTX_UBOOT)
> + env_get_location(ENVOP_LOAD, best_prio);
> + else
> + env_get_location_ext(ctx, ENVOP_LOAD, best_prio);
...and here.
> - if (!drv->save)
> + if ((ctx == ENVCTX_UBOOT && !drv->save) ||
> + (ctx != ENVCTX_UBOOT && !drv->save_ext))
> return -ENODEV;
...and here.
> - if (!env_has_inited(drv->location))
> + if (!env_has_inited(ctx, drv->location))
> return -ENODEV;
...and here.
> - ret = drv->save();
> + if (ctx == ENVCTX_UBOOT)
> + ret = drv->save();
> + else
> + ret = drv->save_ext(ctx);
...and here.
> int env_init(void)
> {
> struct env_driver *drv;
> int ret = -ENOENT;
> - int prio;
> + int ctx, prio;
Error. Context is an enum.
> + /* other than ENVCTX_UBOOT */
> + for (ctx = 1; ctx < ENVCTX_COUNT; ctx++) {
Ugly code. "ctx" is an enum, so "1" is a magic number here that
should not be used.
Please fix.
> - for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
> + /* ENVCTX_UBOOT */
> + ret = -ENOENT;
> + for (prio = 0; (drv = env_driver_lookup(ENVCTX_UBOOT, ENVOP_INIT,
> + prio));
> + prio++) {
See problem above. U-Boot context should not need separate code, it
is just one context among others...
...unless you really split initialization into early init (U-Boot,
eventually already in SPL) and late init (all other contexts,
delayed until U-Boot proper).
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -20,6 +20,7 @@
> */
>
> #ifndef __ASSEMBLY__
> +#include <environment.h>
> #include <fdtdec.h>
> #include <membuff.h>
> #include <linux/list.h>
> @@ -50,8 +51,10 @@ typedef struct global_data {
> #endif
> unsigned long env_addr; /* Address of Environment struct */
> unsigned long env_valid; /* Environment valid? enum env_valid */
> - unsigned long env_has_init; /* Bitmask of boolean of struct env_location offsets */
> - int env_load_prio; /* Priority of the loaded environment */
> + unsigned long env_has_init[ENVCTX_COUNT_MAX];
> + /* Bitmask of boolean of struct env_location offsets */
> + int env_load_prio[ENVCTX_COUNT_MAX];
> + /* Priority of the loaded environment */
NAK. Please keep this information out of GD. This is a tight
resource, we must not extend it for such purposes.
> --- a/include/env_default.h
> +++ b/include/env_default.h
> @@ -15,6 +15,7 @@ env_t environment __UBOOT_ENV_SECTION__(environment) = {
> #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> 1, /* Flags: valid */
> #endif
> + ENV_SIZE,
Full NAK!!!
You *MUST NOT* change the data structure of the environment on
external storage, as this breaks compatibility with all existing
systems. This is a strict no-go.
> diff --git a/include/environment.h b/include/environment.h
> index cd966761416e..9fa085a9b728 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -134,21 +134,31 @@ extern unsigned long nand_env_oob_offset;
> #include "compiler.h"
>
> #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> -# define ENV_HEADER_SIZE (sizeof(uint32_t) + 1)
> -
> # define ACTIVE_FLAG 1
> # define OBSOLETE_FLAG 0
> +
> +#define ENVIRONMENT_HEADER \
> + uint32_t crc; /* CRC32 over data bytes */ \
> + uint32_t data_size; /* Environment data size */ \
> + unsigned char flags; /* active/obsolete flags */
> #else
> -# define ENV_HEADER_SIZE (sizeof(uint32_t))
> +#define ENVIRONMENT_HEADER \
> + uint32_t crc; /* CRC32 over data bytes */ \
> + uint32_t data_size; /* Environment data size */
> #endif
>
> +typedef struct environment_hdr {
> + ENVIRONMENT_HEADER
> + unsigned char data[]; /* Environment data */
> +} env_hdr_t;
> +
> +# define ENV_HEADER_SIZE (sizeof(env_hdr_t))
> +
> +/* For compatibility */
> #define ENV_SIZE (CONFIG_ENV_SIZE - ENV_HEADER_SIZE)
>
> typedef struct environment_s {
> - uint32_t crc; /* CRC32 over data bytes */
> -#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> - unsigned char flags; /* active/obsolete flags */
> -#endif
> + ENVIRONMENT_HEADER
> unsigned char data[ENV_SIZE]; /* Environment data */
> } env_t;
Full NAK!!!
You *MUST NOT* change the data structure of the environment on
external storage, as this breaks compatibility with all existing
systems. This is a strict no-go.
> */
> int (*load)(void);
>
> + /**
> + * load_ext() - Load given environment context from storage
> + *
> + * This method is optional. If not provided, no environment will be
> + * loaded.
> + *
> + * @ctx: context to be loaded
> + * Return: 0 if OK, -ve on error
> + */
> + int (*load_ext)(enum env_context ctx);
> +
> /**
> * save() - Save the environment to storage
> *
> @@ -225,6 +251,16 @@ struct env_driver {
> */
> int (*save)(void);
>
> + /**
> + * save_ext() - Save given environment context to storage
> + *
> + * This method is required for 'saveenv' to work.
> + *
> + * @ctx: context to be saved
> + * Return: 0 if OK, -ve on error
> + */
> + int (*save_ext)(enum env_context ctx);
> +
> /**
> * init() - Set up the initial pre-relocation environment
> *
> @@ -234,6 +270,17 @@ struct env_driver {
> * other -ve on error
> */
> int (*init)(void);
> +
> + /**
> + * init() - Set up the initial pre-relocation environment
> + *
> + * This method is optional.
> + *
> + * @ctx: context to be saved
> + * @return 0 if OK, -ENOENT if no initial environment could be found,
> + * other -ve on error
> + */
> + int (*init_ext)(enum env_context ctx);
NAK. We should ot need always pairs of functions foo() and
foo_ext(). There should always be only foo(), which supports
contexts.
> /* Import from binary representation into hash table */
> int env_import(const char *buf, int check);
> +int env_import_ext(const char *buf, enum env_context ctx, int check);
>
> /* Export from hash table into binary representation */
> int env_export(env_t *env_out);
> +int env_export_ext(env_hdr_t **env_out, enum env_context ctx);
>
> #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> /* Select and import one of two redundant environments */
> int env_import_redund(const char *buf1, int buf1_status,
> const char *buf2, int buf2_status);
> +int env_import_redund_ext(const char *buf1, int buf1_status,
> + const char *buf2, int buf2_status,
> + enum env_context ctx);
> #endif
Ditto here and following.
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
panic: kernel trap (ignored)
More information about the U-Boot
mailing list