On Fri, Jul 19, 2019 at 09:38:11AM +0200, Wolfgang Denk wrote:
> Dear AKASHI Takahiro,
>
> In message <20190717082525.891-3-takahiro.aka...@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.
I didn't rename them.
> > -int env_import(const char *buf, int check)
> > +int env_import_ext(const char *buf, enum env_context ctx, int check)
env_import() is re-defined using env_import_ext() below.
> 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.
I described in my cover letter.
> 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.
Okay, but this is not specific to this function.
I'm going to add detailed function descriptions
if you agree with these new interfaces. Do you?
>
> > - 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.
Please read path#14 where a fat driver is modified to maintain UEFI
variables in U-Boot environment hash tables.
If UEFI variables need not be maintained in a single U-Boot environment,
my whole patch set goes in vain. (This is my "Plan B" implementation.)
> > - 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?
Handling for default (initial) variables may vary context to context.
It should be implemented depending on their requirements.
> > +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?
Yes and no.
My current focus is to determine if my approach is acceptable or not.
> > - pr_err("Cannot export environment: errno = %d\n", errno);
> > + pr_err("Cannot export environment: errno = %d\n",
> > + errno);
>
> Why did you change this?
Maybe a mistake?
> > - 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.
envp will be returned to a caller via env_out.
> Speaking about memory... what is the overall code / data size impact
> of this patch set?
No statistics yet.
>
> > --- 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.
Yes, I think that I have not paid enough attentions to SPL case.
I will appreciate your suggestions.
> 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.
Yeah, but see my comment below.
>
> > -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.
NAK.
env_get_location is currently defined as a weak function.
So this hack is necessary, in particular, on a system where
UEFI is not enabled and env_get_location is yet overwritten.
> > - 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.
There are already bunch of code where enum is interchangeably
used as an integer.
> 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...
It seems to me that this comment is conflicting with your assertion
below.
> ...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.
But in this way, we will have to handle contexts differently
depending on whether it is ENVCTX_UBOOT or not.
> > --- 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.
Let me think, but I don't have a good idea yet.
> > 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.
This is a transition state as I mentioned in "known issues/TODOs"
in my cover letter.
If you agree, I can remove all the existing interfaces
but only when all the storage drivers are converted.
Thanks,
-Takahiro Akashi
> > /* 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: w...@denx.de
> panic: kernel trap (ignored)
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
