
On Fri, Jul 19, 2019 at 09:38:11AM +0200, Wolfgang Denk wrote:
Dear AKASHI Takahiro,
In message 20190717082525.891-3-takahiro.akashi@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: wd@denx.de panic: kernel trap (ignored)