[U-Boot] [PATCH] env_mmc: fix buffer allocation for armv7
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 */
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.
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
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.
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
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@denx.de It's all Klatchian to me. - Terry Pratchett & Stephen Briggs, _The Discworld Companion_ _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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
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.
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
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.
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
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!
participants (3)
-
Markus Niebel -
Tom Rini -
Wolfgang Denk