[PATCH 0/4] remove (more) env callback code for SPL

[PATCH 0/4] remove (more) env callback code for SPL

[Rasmus Villemoes]

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(-)

-- 
2.23.0

[PATCH 1/4] env: remove callback.o for an SPL build

[Rasmus Villemoes]

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__ */
-- 
2.23.0

[PATCH 2/4] lib/hashtable.c: create helper for calling env_entry::callback

[Rasmus Villemoes]

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);
-- 
2.23.0

[PATCH 3/4] lib/hashtable.c: don't test ->callback in SPL

[Rasmus Villemoes]

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;
 }
 
-- 
2.23.0

[PATCH 4/4] make env_entry::callback conditional on !CONFIG_SPL_BUILD

[Rasmus Villemoes]

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;
 
-- 
2.23.0

[PATCH 1/4] env: remove callback.o for an SPL build

[Simon Glass]

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>

[PATCH 2/4] lib/hashtable.c: create helper for calling env_entry::callback

[Simon Glass]

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>

[PATCH 4/4] make env_entry::callback conditional on !CONFIG_SPL_BUILD

[Simon Glass]

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>

[PATCH 3/4] lib/hashtable.c: don't test ->callback in SPL

[Simon Glass]

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
>

[PATCH 1/4] env: remove callback.o for an SPL build

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200424/d05d8301/attachment.sig>

[PATCH 2/4] lib/hashtable.c: create helper for calling env_entry::callback

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200424/32687a7c/attachment.sig>

[PATCH 3/4] lib/hashtable.c: don't test ->callback in SPL

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200424/1eda1f6d/attachment.sig>

[PATCH 4/4] make env_entry::callback conditional on !CONFIG_SPL_BUILD

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200424/ee4d9bb4/attachment.sig>