[PATCH 0/4] remove (more) env callback code for SPL
The actual environment callbacks are automatically dropped from an SPL build, but the support code (env/callback.o) for associating a callback to an environment variable is still there.
These patches reduce a CONFIG_SPL_ENV_SUPPORT=y SPL image by about 1K (at least for my ppc build), and another ~2K of run-time memory.
===
Aside:
While poking around in this code, I noticed a few somewhat related bugs in callback.o: (1) The caching of env_get(".callbacks") in env_callback_init() is dubious at best: It gets called during the initial import, so env_get ends up calling env_get_f. So either we cache a value of NULL (no .callbacks variable in initial environment), or we cache a value of gd->env_buf. Now, of course env_get_f will soon no longer be used, so there's not a big risk that gd->env_buf will be overwritten, but it's rather fragile. In either case (2), the cache is not invalidated or updated by the on_callbacks() callback associated to .callbacks itself.
Finally, (3) with CONFIG_REGEX=y, that on_callbacks() function has the unfortunate effect of removing itself as the callback associated to .callbacks: When .callbacks is initially added to the environment, env_callback_init() correctly associates on_callbacks, because it uses env_attr_lookup() (which is regex-aware) on the ENV_CALLBACK_STATIC_LIST. Now, when .callbacks is set the next time, on_callbacks() is called, starts by clearing all callbacks, including the one for .callbacks. It then tries to initialize them again, but it uses env_attr_walk() (which roughly speaking just splits the given string on , and : and calls back to the user's handler) on ENV_CALLBACK_STATIC_LIST, which means that set_callback() gets called with the string ".callbacks" - and such a variable does not exist. set_callback() is never called with ".callbacks" as name, so .callbacks (and e.g. eth5addr - anything else which is supposed to be matched by a regex) now no longer has a callback.
Perhaps this can all be fixed by just having on_callbacks update the cached static callback_list with value, then do a hwalk_r(&env_htab, env_callback_init) - no need for either set_callback or clear_callback.
Rasmus Villemoes (4): env: remove callback.o for an SPL build lib/hashtable.c: create helper for calling env_entry::callback lib/hashtable.c: don't test ->callback in SPL make env_entry::callback conditional on !CONFIG_SPL_BUILD
env/Makefile | 2 +- env/callback.c | 2 ++ env/flags.c | 1 - include/env_callback.h | 6 ++++++ include/search.h | 2 ++ lib/hashtable.c | 26 +++++++++++++++++--------- 6 files changed, 28 insertions(+), 11 deletions(-)
env.h says this about about callback declarations (U_BOOT_ENV_CALLBACK):
* For SPL these are silently dropped to reduce code size, since environment * callbacks are not supported with SPL.
So env_callback_init() does a lot of work to not find anything in the guaranteed empty env_clbk list. Drop callback.o entirely from the link and stub out the only public function defined in callback.o. This cuts about 600 bytes from the SPL on my ppc build.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- env/Makefile | 2 +- include/env_callback.h | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/env/Makefile b/env/Makefile index e2a165b8f1..c4ad654328 100644 --- a/env/Makefile +++ b/env/Makefile @@ -7,9 +7,9 @@ obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += common.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += env.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o -obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += callback.o
ifndef CONFIG_SPL_BUILD +obj-y += callback.o obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o extra-$(CONFIG_ENV_IS_EMBEDDED) += embedded.o obj-$(CONFIG_ENV_IS_IN_EEPROM) += embedded.o diff --git a/include/env_callback.h b/include/env_callback.h index 74da20eec3..05e9516a0f 100644 --- a/include/env_callback.h +++ b/include/env_callback.h @@ -72,6 +72,12 @@ "serial#:serialno," \ CONFIG_ENV_CALLBACK_LIST_STATIC
+#ifndef CONFIG_SPL_BUILD void env_callback_init(struct env_entry *var_entry); +#else +static inline void env_callback_init(struct env_entry *var_entry) +{ +} +#endif
#endif /* __ENV_CALLBACK_H__ */
On Thu, 27 Feb 2020 at 05:56, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
env.h says this about about callback declarations (U_BOOT_ENV_CALLBACK):
- For SPL these are silently dropped to reduce code size, since environment
- callbacks are not supported with SPL.
So env_callback_init() does a lot of work to not find anything in the guaranteed empty env_clbk list. Drop callback.o entirely from the link and stub out the only public function defined in callback.o. This cuts about 600 bytes from the SPL on my ppc build.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
env/Makefile | 2 +- include/env_callback.h | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Thu, Feb 27, 2020 at 01:56:10PM +0000, Rasmus Villemoes wrote:
env.h says this about about callback declarations (U_BOOT_ENV_CALLBACK):
- For SPL these are silently dropped to reduce code size, since environment
- callbacks are not supported with SPL.
So env_callback_init() does a lot of work to not find anything in the guaranteed empty env_clbk list. Drop callback.o entirely from the link and stub out the only public function defined in callback.o. This cuts about 600 bytes from the SPL on my ppc build.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
In SPL, environment callbacks are not supported, so e->callback is always NULL. Removing this makes the SPL a little smaller (about 400 bytes in my ppc build) with no functional change.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- lib/hashtable.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/lib/hashtable.c b/lib/hashtable.c index 574ec6af86..c4e1e2bd45 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -226,8 +226,10 @@ static int do_callback(const struct env_entry *e, const char *name, const char *value, enum env_op op, int flags) { +#ifndef CONFIG_SPL_BUILD if (e->callback) return e->callback(name, value, op, flags); +#endif return 0; }
On Thu, 27 Feb 2020 at 05:56, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
In SPL, environment callbacks are not supported, so e->callback is always NULL. Removing this makes the SPL a little smaller (about 400 bytes in my ppc build) with no functional change.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
lib/hashtable.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/lib/hashtable.c b/lib/hashtable.c index 574ec6af86..c4e1e2bd45 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -226,8 +226,10 @@ static int do_callback(const struct env_entry *e, const char *name, const char *value, enum env_op op, int flags) { +#ifndef CONFIG_SPL_BUILD if (e->callback) return e->callback(name, value, op, flags); +#endif
blank line before return
return 0;}
-- 2.23.0
On Thu, Feb 27, 2020 at 01:56:11PM +0000, Rasmus Villemoes wrote:
In SPL, environment callbacks are not supported, so e->callback is always NULL. Removing this makes the SPL a little smaller (about 400 bytes in my ppc build) with no functional change.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
This is preparation for compiling out the "call the callback" code and associated error handling for SPL, where ->callback is always NULL.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- lib/hashtable.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/lib/hashtable.c b/lib/hashtable.c index 907e8a642f..574ec6af86 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -222,6 +222,15 @@ int hmatch_r(const char *match, int last_idx, struct env_entry **retval, return 0; }
+static int +do_callback(const struct env_entry *e, const char *name, const char *value, + enum env_op op, int flags) +{ + if (e->callback) + return e->callback(name, value, op, flags); + return 0; +} + /* * Compare an existing entry with the desired key, and overwrite if the action * is ENV_ENTER. This is simply a helper function for hsearch_r(). @@ -247,9 +256,8 @@ static inline int _compare_and_overwrite_entry(struct env_entry item, }
/* If there is a callback, call it */ - if (htab->table[idx].entry.callback && - htab->table[idx].entry.callback(item.key, - item.data, env_op_overwrite, flag)) { + if (do_callback(&htab->table[idx].entry, item.key, + item.data, env_op_overwrite, flag)) { debug("callback() rejected setting variable " "%s, skipping it!\n", item.key); __set_errno(EINVAL); @@ -402,9 +410,8 @@ int hsearch_r(struct env_entry item, enum env_action action, }
/* If there is a callback, call it */ - if (htab->table[idx].entry.callback && - htab->table[idx].entry.callback(item.key, item.data, - env_op_create, flag)) { + if (do_callback(&htab->table[idx].entry, item.key, item.data, + env_op_create, flag)) { debug("callback() rejected setting variable " "%s, skipping it!\n", item.key); _hdelete(item.key, htab, &htab->table[idx].entry, idx); @@ -473,8 +480,8 @@ int hdelete_r(const char *key, struct hsearch_data *htab, int flag) }
/* If there is a callback, call it */ - if (htab->table[idx].entry.callback && - htab->table[idx].entry.callback(key, NULL, env_op_delete, flag)) { + if (do_callback(&htab->table[idx].entry, key, NULL, + env_op_delete, flag)) { debug("callback() rejected deleting variable " "%s, skipping it!\n", key); __set_errno(EINVAL);
On Thu, 27 Feb 2020 at 05:56, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
This is preparation for compiling out the "call the callback" code and associated error handling for SPL, where ->callback is always NULL.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
lib/hashtable.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Thu, Feb 27, 2020 at 01:56:11PM +0000, Rasmus Villemoes wrote:
This is preparation for compiling out the "call the callback" code and associated error handling for SPL, where ->callback is always NULL.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
The callback member of struct env_entry is always NULL for an SPL build. Removing it thus saves a bit of run-time memory in the SPL (when CONFIG_SPL_ENV_SUPPORT=y) since struct env_entry is embedded in struct env_entry_node - i.e. about 2KB for the normal case of 512+change hash table entries.
Two small fixups are needed for this, all other references to the callback member are already under !CONFIG_SPL_BUILD: Don't initialize .callback in set_flags() - hsearch_r doesn't use that value anyway. And make env_callback_init() initialize ->callback to NULL for a new entry instead of relying on an unused or deleted entry having NULL in ->callback.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- env/callback.c | 2 ++ env/flags.c | 1 - include/search.h | 2 ++ lib/hashtable.c | 1 - 4 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/env/callback.c b/env/callback.c index f0904cfdc5..4054b9ef58 100644 --- a/env/callback.c +++ b/env/callback.c @@ -55,6 +55,8 @@ void env_callback_init(struct env_entry *var_entry) first_call = 0; }
+ var_entry->callback = NULL; + /* look in the ".callbacks" var for a reference to this variable */ if (callback_list != NULL) ret = env_attr_lookup(callback_list, var_name, callback_name); diff --git a/env/flags.c b/env/flags.c index 418d6cc742..b88fe7ba9c 100644 --- a/env/flags.c +++ b/env/flags.c @@ -457,7 +457,6 @@ static int set_flags(const char *name, const char *value, void *priv)
e.key = name; e.data = NULL; - e.callback = NULL; hsearch_r(e, ENV_FIND, &ep, &env_htab, 0);
/* does the env variable actually exist? */ diff --git a/include/search.h b/include/search.h index 0469a852e0..8f87dc72ce 100644 --- a/include/search.h +++ b/include/search.h @@ -29,8 +29,10 @@ enum env_action { struct env_entry { const char *key; char *data; +#ifndef CONFIG_SPL_BUILD int (*callback)(const char *name, const char *value, enum env_op op, int flags); +#endif int flags; };
diff --git a/lib/hashtable.c b/lib/hashtable.c index c4e1e2bd45..f82f2463cf 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -450,7 +450,6 @@ static void _hdelete(const char *key, struct hsearch_data *htab, debug("hdelete: DELETING key "%s"\n", key); free((void *)ep->key); free(ep->data); - ep->callback = NULL; ep->flags = 0; htab->table[idx].used = USED_DELETED;
On Thu, 27 Feb 2020 at 05:56, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
The callback member of struct env_entry is always NULL for an SPL build. Removing it thus saves a bit of run-time memory in the SPL (when CONFIG_SPL_ENV_SUPPORT=y) since struct env_entry is embedded in struct env_entry_node - i.e. about 2KB for the normal case of 512+change hash table entries.
Two small fixups are needed for this, all other references to the callback member are already under !CONFIG_SPL_BUILD: Don't initialize .callback in set_flags() - hsearch_r doesn't use that value anyway. And make env_callback_init() initialize ->callback to NULL for a new entry instead of relying on an unused or deleted entry having NULL in ->callback.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
env/callback.c | 2 ++ env/flags.c | 1 - include/search.h | 2 ++ lib/hashtable.c | 1 - 4 files changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Thu, Feb 27, 2020 at 01:56:12PM +0000, Rasmus Villemoes wrote:
The callback member of struct env_entry is always NULL for an SPL build. Removing it thus saves a bit of run-time memory in the SPL (when CONFIG_SPL_ENV_SUPPORT=y) since struct env_entry is embedded in struct env_entry_node - i.e. about 2KB for the normal case of 512+change hash table entries.
Two small fixups are needed for this, all other references to the callback member are already under !CONFIG_SPL_BUILD: Don't initialize .callback in set_flags() - hsearch_r doesn't use that value anyway. And make env_callback_init() initialize ->callback to NULL for a new entry instead of relying on an unused or deleted entry having NULL in ->callback.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
participants (3)
-
Rasmus Villemoes -
Simon Glass -
Tom Rini