[U-Boot] [PATCH] env_mmc: fix buffer allocation for armv7

[U-Boot] [PATCH] env_mmc: fix buffer allocation for armv7

[Markus Niebel]

From: Markus Niebel <Markus.Niebel@tqs.de>

commit d196bd880347373237d73e0d115b4d51c68cf2ad adds
redundand environment to mmc. The usage of malloc in
env_relocate_spec triggers cache errors on armv7.

Tested on a not mainlined i.MX53 board:

Board: TQMa53
I2C:   ready
DRAM:  512 MiB
MMC:   FSL_SDHC: 0, FSL_SDHC: 1
ERROR: v7_dcache_inval_range - start address is not aligned - 0x8f57c2d8
ERROR: v7_dcache_inval_range - stop address is not aligned - 0x8f57e2d8
ERROR: v7_dcache_inval_range - start address is not aligned - 0x8f57e2e0
ERROR: v7_dcache_inval_range - stop address is not aligned - 0x8f5802e0
Using default environment

Signed-off-by: Markus Niebel <Markus.Niebel@tqs.de>
---
 common/env_mmc.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/common/env_mmc.c b/common/env_mmc.c
index 65aafa9..204d23b 100644
--- a/common/env_mmc.c
+++ b/common/env_mmc.c
@@ -192,11 +192,12 @@ void env_relocate_spec(void)
 	u32 offset1, offset2;
 	int read1_fail = 0, read2_fail = 0;
 	int crc1_ok = 0, crc2_ok = 0;
-	env_t *ep, *tmp_env1, *tmp_env2;
+	env_t *ep;
 	int ret;
 
-	tmp_env1 = (env_t *)malloc(CONFIG_ENV_SIZE);
-	tmp_env2 = (env_t *)malloc(CONFIG_ENV_SIZE);
+	ALLOC_CACHE_ALIGN_BUFFER(env_t, tmp_env1, 1);
+	ALLOC_CACHE_ALIGN_BUFFER(env_t, tmp_env2, 1);
+
 	if (tmp_env1 == NULL || tmp_env2 == NULL) {
 		puts("Can't allocate buffers for environment\n");
 		ret = 1;
@@ -266,8 +267,6 @@ err:
 	if (ret)
 		set_default_env(NULL);
 
-	free(tmp_env1);
-	free(tmp_env2);
 #endif
 }
 #else /* ! CONFIG_ENV_OFFSET_REDUND */
-- 
1.7.9.5

[U-Boot] [PATCH] env_mmc: fix buffer allocation for armv7

[Tom Rini]

On Fri, Oct 04, 2013 at 03:48:03PM +0200, Markus Niebel wrote:

> From: Markus Niebel <Markus.Niebel@tqs.de>
> 
> commit d196bd880347373237d73e0d115b4d51c68cf2ad adds
> redundand environment to mmc. The usage of malloc in
> env_relocate_spec triggers cache errors on armv7.
> 
> Tested on a not mainlined i.MX53 board:
> 
> Board: TQMa53
> I2C:   ready
> DRAM:  512 MiB
> MMC:   FSL_SDHC: 0, FSL_SDHC: 1
> ERROR: v7_dcache_inval_range - start address is not aligned - 0x8f57c2d8
> ERROR: v7_dcache_inval_range - stop address is not aligned - 0x8f57e2d8
> ERROR: v7_dcache_inval_range - start address is not aligned - 0x8f57e2e0
> ERROR: v7_dcache_inval_range - stop address is not aligned - 0x8f5802e0
> Using default environment
> 
> Signed-off-by: Markus Niebel <Markus.Niebel@tqs.de>

I really don't like this.  We're now allocating for example 256KiB on
the stack, rather than malloc.  I posted a patch recently to convert the
non-redundant case to malloc instead for this reason.  I believe the
answer is we need to be using memalign here, like
common/bouncebuf.c::bounce_buffer_start does.  Can you do this?  If not,
can you test a patch?  Thanks.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131004/c09e8ca6/attachment.pgp>

[U-Boot] [PATCH] env_mmc: fix buffer allocation for armv7

[Wolfgang Denk]

Dear Tom Rini,

In message <20131004170203.GL15917@bill-the-cat> you wrote:
> 
> I really don't like this.  We're now allocating for example 256KiB on
> the stack, rather than malloc.  I posted a patch recently to convert the
> non-redundant case to malloc instead for this reason.  I believe the
> answer is we need to be using memalign here, like
> common/bouncebuf.c::bounce_buffer_start does.  Can you do this?  If not,
> can you test a patch?  Thanks.

What exactly don't you like in using the stack for temporary data
buffers?  That's what it has been invented for.  Using malloc() is
only useful when the allocated buffers neet to be kept across file
scope, which appears not to be the case here.

For file scope buffers, usign the satck is the most efficient and
preferred approach - it's fast and results in minimal (virtually no)
code.

Why do you hesitate to use the stack?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Humanity has the  stars  in  its  future,  and  that  future  is  too
important  to be lost under the burden of juvenile folly and ignorant
superstition.                                          - Isaac Asimov

[U-Boot] [PATCH] env_mmc: fix buffer allocation for armv7

[Tom Rini]

On Sat, Oct 05, 2013 at 09:57:28PM +0200, Wolfgang Denk wrote:
> Dear Tom Rini,
> 
> In message <20131004170203.GL15917@bill-the-cat> you wrote:
> > 
> > I really don't like this.  We're now allocating for example 256KiB on
> > the stack, rather than malloc.  I posted a patch recently to convert the
> > non-redundant case to malloc instead for this reason.  I believe the
> > answer is we need to be using memalign here, like
> > common/bouncebuf.c::bounce_buffer_start does.  Can you do this?  If not,
> > can you test a patch?  Thanks.
> 
> What exactly don't you like in using the stack for temporary data
> buffers?  That's what it has been invented for.  Using malloc() is
> only useful when the allocated buffers neet to be kept across file
> scope, which appears not to be the case here.
> 
> For file scope buffers, usign the satck is the most efficient and
> preferred approach - it's fast and results in minimal (virtually no)
> code.
> 
> Why do you hesitate to use the stack?

Mainly to allow us to work in restricted stack areas like SPL where we
simply may not have that much space available.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131006/c2026da9/attachment.pgp>

[U-Boot] [PATCH] env_mmc: fix buffer allocation for armv7

[Wolfgang Denk]

Dear Tom,

In message <20131006204214.GO15917@bill-the-cat> you wrote:
> 
> > Why do you hesitate to use the stack?
> 
> Mainly to allow us to work in restricted stack areas like SPL where we
> simply may not have that much space available.

But malloc() is drawing from the very same resource as the stack, even
more so: with a buffer on the stack, the memory isfreed completeky
upon return from the fucntion, with no reminders left.  With malloc()
you need to statically allocate the malloc pool size for the whole
runtime, and allocating the buffers may fragment tha area, so even
after freeing the buffers there is less space left for other purposes.

Especially in memory-tight situations you want to avoid malloc().

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It's all Klatchian to me.
        - Terry Pratchett & Stephen Briggs, _The Discworld Companion_

[U-Boot] [PATCH] env_mmc: fix buffer allocation for armv7

[Markus Niebel]

Hello,

Am 04.10.2013 19:02, wrote Tom Rini:
> On Fri, Oct 04, 2013 at 03:48:03PM +0200, Markus Niebel wrote:
> 
>> From: Markus Niebel <Markus.Niebel@tqs.de>
>>
>> commit d196bd880347373237d73e0d115b4d51c68cf2ad adds
>> redundand environment to mmc. The usage of malloc in
>> env_relocate_spec triggers cache errors on armv7.
>>
>> Tested on a not mainlined i.MX53 board:
>>
>> Board: TQMa53
>> I2C:   ready
>> DRAM:  512 MiB
>> MMC:   FSL_SDHC: 0, FSL_SDHC: 1
>> ERROR: v7_dcache_inval_range - start address is not aligned - 0x8f57c2d8
>> ERROR: v7_dcache_inval_range - stop address is not aligned - 0x8f57e2d8
>> ERROR: v7_dcache_inval_range - start address is not aligned - 0x8f57e2e0
>> ERROR: v7_dcache_inval_range - stop address is not aligned - 0x8f5802e0
>> Using default environment
>>
>> Signed-off-by: Markus Niebel <Markus.Niebel@tqs.de>
> 
> I really don't like this.  We're now allocating for example 256KiB on
> the stack, rather than malloc.  I posted a patch recently to convert the
> non-redundant case to malloc instead for this reason.  I believe the
> answer is we need to be using memalign here, like
> common/bouncebuf.c::bounce_buffer_start does.  Can you do this?  If not,
> can you test a patch?  Thanks.
> 
Thanks for reviewing. I will just follow the ongoing discussion and wait for consensus on how to fix the error before I do tests.

Markus

[U-Boot] [PATCH] env_mmc: fix buffer allocation for armv7

[Tom Rini]

On Mon, Oct 07, 2013 at 07:34:24AM +0200, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20131006204214.GO15917@bill-the-cat> you wrote:
> > 
> > > Why do you hesitate to use the stack?
> > 
> > Mainly to allow us to work in restricted stack areas like SPL where we
> > simply may not have that much space available.
> 
> But malloc() is drawing from the very same resource as the stack, even
> more so: with a buffer on the stack, the memory isfreed completeky
> upon return from the fucntion, with no reminders left.  With malloc()
> you need to statically allocate the malloc pool size for the whole
> runtime, and allocating the buffers may fragment tha area, so even
> after freeing the buffers there is less space left for other purposes.
> 
> Especially in memory-tight situations you want to avoid malloc().

Ah, but in these cases we don't have stack room, period.  We have a
malloc pool.  So unless we make SPL move its stack pointer into DDR from
wherever we set the initial one to, the other option here is to just
restrict real env support to NOR (and we already don't allow embedded
env, since that's embedded within U-Boot, not SPL).

> 
> Best regards,
> 
> Wolfgang Denk
> 
> -- 
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> It's all Klatchian to me.
>         - Terry Pratchett & Stephen Briggs, _The Discworld Companion_
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131007/f878ad42/attachment.pgp>

[U-Boot] [PATCH] env_mmc: fix buffer allocation for armv7

[Wolfgang Denk]

Dear Tom,

In message <20131007122020.GT15917@bill-the-cat> you wrote:
> 
> > But malloc() is drawing from the very same resource as the stack, even
> > more so: with a buffer on the stack, the memory isfreed completeky
> > upon return from the fucntion, with no reminders left.  With malloc()
> > you need to statically allocate the malloc pool size for the whole
> > runtime, and allocating the buffers may fragment tha area, so even
> > after freeing the buffers there is less space left for other purposes.
> > 
> > Especially in memory-tight situations you want to avoid malloc().
>
> Ah, but in these cases we don't have stack room, period.  We have a
> malloc pool.  So unless we make SPL move its stack pointer into DDR from
> wherever we set the initial one to, the other option here is to just
> restrict real env support to NOR (and we already don't allow embedded
> env, since that's embedded within U-Boot, not SPL).

Well, if we have DDR such that we can use it for the malloc arena, we
also should use it for the stack.  Or is there a good reason for not
doing this?  It would solve all these issues at the root...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Q:  Do you know what the death rate around here is?
A:  One per person.

[U-Boot] [PATCH] env_mmc: fix buffer allocation for armv7

[Tom Rini]

On Mon, Oct 07, 2013 at 03:58:02PM +0200, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20131007122020.GT15917@bill-the-cat> you wrote:
> > 
> > > But malloc() is drawing from the very same resource as the stack, even
> > > more so: with a buffer on the stack, the memory isfreed completeky
> > > upon return from the fucntion, with no reminders left.  With malloc()
> > > you need to statically allocate the malloc pool size for the whole
> > > runtime, and allocating the buffers may fragment tha area, so even
> > > after freeing the buffers there is less space left for other purposes.
> > > 
> > > Especially in memory-tight situations you want to avoid malloc().
> >
> > Ah, but in these cases we don't have stack room, period.  We have a
> > malloc pool.  So unless we make SPL move its stack pointer into DDR from
> > wherever we set the initial one to, the other option here is to just
> > restrict real env support to NOR (and we already don't allow embedded
> > env, since that's embedded within U-Boot, not SPL).
> 
> Well, if we have DDR such that we can use it for the malloc arena, we
> also should use it for the stack.  Or is there a good reason for not
> doing this?  It would solve all these issues at the root...

Making SPL more complex for everyone?  We would need to do a fairly
good-sized re-jigger of SPL to setup and swap around stack pointers like
we do in full U-Boot.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131008/242ff7d8/attachment.pgp>

[U-Boot] [PATCH] env_mmc: fix buffer allocation for armv7

[Wolfgang Denk]

Dear Tom,

In message <20131008134456.GB15917@bill-the-cat> you wrote:
> 
> > Well, if we have DDR such that we can use it for the malloc arena, we
> > also should use it for the stack.  Or is there a good reason for not
> > doing this?  It would solve all these issues at the root...
> 
> Making SPL more complex for everyone?  We would need to do a fairly
> good-sized re-jigger of SPL to setup and swap around stack pointers like
> we do in full U-Boot.

Hm,  I'm not convinced.  As proposed, we make the code bigger, less
efficient, more error prone and more ugly for everyone, not only for
SPL users.  Aslo, this might not be the only place where buffers or
such may be kept on the stack.  I hope you don't want to change all
these?

Really, if we have the resources, we should use them.  If RAM is
abailable, it should also be used for the stack.  Just using it for
malloc() is neither fish nor fowl.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Have you lived in this village all your life?"        "No, not yet."

[U-Boot] [PATCH] env_mmc: fix buffer allocation for armv7

[Tom Rini]

On Tue, Oct 08, 2013 at 08:17:01PM +0200, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20131008134456.GB15917@bill-the-cat> you wrote:
> > 
> > > Well, if we have DDR such that we can use it for the malloc arena, we
> > > also should use it for the stack.  Or is there a good reason for not
> > > doing this?  It would solve all these issues at the root...
> > 
> > Making SPL more complex for everyone?  We would need to do a fairly
> > good-sized re-jigger of SPL to setup and swap around stack pointers like
> > we do in full U-Boot.
> 
> Hm,  I'm not convinced.  As proposed, we make the code bigger, less
> efficient, more error prone and more ugly for everyone, not only for
> SPL users.  Aslo, this might not be the only place where buffers or
> such may be kept on the stack.  I hope you don't want to change all
> these?
> 
> Really, if we have the resources, we should use them.  If RAM is
> abailable, it should also be used for the stack.  Just using it for
> malloc() is neither fish nor fowl.

I'll ceed the point and re-work things on my series then.  Markus, your
patch is good as-is and I shall pick it up shortly.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131011/1b527155/attachment.pgp>

[U-Boot] env_mmc: fix buffer allocation for armv7

[Tom Rini]

On Fri, Oct 04, 2013 at 03:48:03PM +0200, Markus Niebel wrote:

> From: Markus Niebel <Markus.Niebel@tqs.de>
> 
> commit d196bd880347373237d73e0d115b4d51c68cf2ad adds
> redundand environment to mmc. The usage of malloc in
> env_relocate_spec triggers cache errors on armv7.
> 
> Tested on a not mainlined i.MX53 board:
> 
> Board: TQMa53
> I2C:   ready
> DRAM:  512 MiB
> MMC:   FSL_SDHC: 0, FSL_SDHC: 1
> ERROR: v7_dcache_inval_range - start address is not aligned - 0x8f57c2d8
> ERROR: v7_dcache_inval_range - stop address is not aligned - 0x8f57e2d8
> ERROR: v7_dcache_inval_range - start address is not aligned - 0x8f57e2e0
> ERROR: v7_dcache_inval_range - stop address is not aligned - 0x8f5802e0
> Using default environment
> 
> Signed-off-by: Markus Niebel <Markus.Niebel@tqs.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131014/1a434e44/attachment.pgp>