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

login
register
mail settings
Submitter Tom Rini
Date Sept. 26, 2013, 8:27 p.m.
Message ID <1380227287-26057-3-git-send-email-trini@ti.com>
Download mbox | patch
Permalink /patch/278258/
State Superseded
Delegated to: Tom Rini
Headers show

Comments

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

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