[U-Boot,02/11] env_mmc.c: Make the non-redundant env_relocate_spec use malloc not stack

Submitted by Tom Rini on Sept. 26, 2013, 8:27 p.m.

Details

Message ID 1380227287-26057-3-git-send-email-trini@ti.com
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Tom Rini Sept. 26, 2013, 8:27 p.m.
Switch the case of non-redundant non-embedded environment to use malloc
to allocate buffers, rather than place them on the stack, like the
redundant case does.

Signed-off-by: Tom Rini <trini@ti.com>
---
 common/env_mmc.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Wolfgang Denk Oct. 5, 2013, 8 p.m.
Dear Tom Rini,

In message <1380227287-26057-3-git-send-email-trini@ti.com> you wrote:
> Switch the case of non-redundant non-embedded environment to use malloc
> to allocate buffers, rather than place them on the stack, like the
> redundant case does.

What exactly would be the benefit of this change?  It just adds code
size and execution time and makes the code more complex, without any
appearent advanteages?

Best regards,

Wolfgang Denk
Tom Rini Oct. 6, 2013, 8:40 p.m.
On Sat, Oct 05, 2013 at 10:00:13PM +0200, Wolfgang Denk wrote:
> Dear Tom Rini,
> 
> In message <1380227287-26057-3-git-send-email-trini@ti.com> you wrote:
> > Switch the case of non-redundant non-embedded environment to use malloc
> > to allocate buffers, rather than place them on the stack, like the
> > redundant case does.
> 
> What exactly would be the benefit of this change?  It just adds code
> size and execution time and makes the code more complex, without any
> appearent advanteages?

The main advantage is that we can use this code in environments with
less than CONFIG_ENV_SIZE worth of stack available.  Arguably it makes
the behaviour and code paths similar for redundant and non-redundant
cases (but someone posted a patch to make the redundant case use the
stack).
Wolfgang Denk Oct. 7, 2013, 5:41 a.m.
Dear Tom,

In message <20131006204046.GN15917@bill-the-cat> you wrote:
> 
> > In message <1380227287-26057-3-git-send-email-trini@ti.com> you wrote:
> > > Switch the case of non-redundant non-embedded environment to use malloc
> > > to allocate buffers, rather than place them on the stack, like the
> > > redundant case does.
> > 
> > What exactly would be the benefit of this change?  It just adds code
> > size and execution time and makes the code more complex, without any
> > appearent advanteages?
>
> The main advantage is that we can use this code in environments with
> less than CONFIG_ENV_SIZE worth of stack available.  Arguably it makes
> the behaviour and code paths similar for redundant and non-redundant
> cases (but someone posted a patch to make the redundant case use the
> stack).

Instead of stack size you pay for that with additional size of the
malloc() arena.  As I just explained in the other mail [1], this is
even worse to deal with and will result in less memoy available for
other purposes.

And yes, we definitely should use the stack for the redundant case as
well.  Do you want me to submit such a patch?

[1] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/171217


Best regards,

Wolfgang Denk

Patch hide | download patch | download mbox

diff --git a/common/env_mmc.c b/common/env_mmc.c
index 65aafa9..0ab11d3 100644
--- a/common/env_mmc.c
+++ b/common/env_mmc.c
@@ -274,11 +274,18 @@  err:
 void env_relocate_spec(void)
 {
 #if !defined(ENV_IS_EMBEDDED)
-	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
 	struct mmc *mmc = find_mmc_device(CONFIG_SYS_MMC_ENV_DEV);
+	env_t *ep;
 	u32 offset;
 	int ret;
 
+	ep = (env_t *)malloc(CONFIG_ENV_SIZE);
+	if (ep == NULL) {
+		puts("Can't allocate buffers for environment\n");
+		ret = 1;
+		goto err;
+	}
+
 	if (init_mmc_for_env(mmc)) {
 		ret = 1;
 		goto err;
@@ -289,12 +296,12 @@  void env_relocate_spec(void)
 		goto fini;
 	}
 
-	if (read_env(mmc, CONFIG_ENV_SIZE, offset, buf)) {
+	if (read_env(mmc, CONFIG_ENV_SIZE, offset, ep)) {
 		ret = 1;
 		goto fini;
 	}
 
-	env_import(buf, 1);
+	env_import((char *)ep, 1);
 	ret = 0;
 
 fini:
@@ -302,6 +309,8 @@  fini:
 err:
 	if (ret)
 		set_default_env(NULL);
+
+	free(ep);
 #endif
 }
 #endif /* CONFIG_ENV_OFFSET_REDUND */