diff mbox

[U-Boot,V7,5/5] omap-common: fixes BSS overwriting problem

Message ID 1320078187-28423-6-git-send-email-simonschwarzcor@gmail.com
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Simon Schwarz Oct. 31, 2011, 4:23 p.m. UTC 
From: Simon Schwarz <simonschwarzcor@googlemail.com>

spl_nand overwrote BSS section because it reads a whole block everytime. Now
loads the block to spare area and just copy the needed junk to destination.
Whole block read is necessary for ecc check!

Signed-off-by: Simon Schwarz <simonschwarzcor@gmail.com>
---

V2 changes:
nothing

V3 changes:
nothing

V4 changes:
nothing

V5 changes:
nothing

V6 changes:
nothing

V7 changes:
nothing
---
 arch/arm/cpu/armv7/omap-common/spl_nand.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Comments

Stefano Babic Dec. 6, 2011, 5:18 p.m. UTC | #1 
On 31/10/2011 17:23, Simon Schwarz wrote:
> From: Simon Schwarz <simonschwarzcor@googlemail.com>
> 
> spl_nand overwrote BSS section because it reads a whole block everytime. Now
> loads the block to spare area and just copy the needed junk to destination.
> Whole block read is necessary for ecc check!
> 
> Signed-off-by: Simon Schwarz <simonschwarzcor@gmail.com>
> ---

> @@ -71,7 +71,8 @@ void spl_nand_load_image(void)
>  			CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
>  		spl_parse_image_header(header);
>  		nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS,
> -			spl_image.size, (void *)spl_image.load_addr);
> +			spl_image.size,
> +			(void *)spl_image.load_addr - sizeof(header));
                                                             ^---


Do you mean maybe sizeof(*header) ?

However, spl_image.load_addr was already decremented in
spl_parse_image_header() and correctly set to 64 byte before the load
address. Do we really need it ?

I found the readon of the kernel corrupt image. We are setting a very
hard address in /nand_spl_simple.c:

ecc_calc = (u_char *)(CONFIG_SYS_SDRAM_BASE + 0x10000);

Because the image for a TI SOC is loaded at 0x80008000, we have a
conflict and the image is corrupted where the ECC is computed.

It is not a really good idea to fix in this way where to compute the
ECC. Should be not better to put it in the CONFIG_SYS_INIT_RAM_ADDR area ?

Best regards,
Stefano Babic
Simon Schwarz Dec. 6, 2011, 5:53 p.m. UTC | #2 
On 12/06/2011 06:18 PM, Stefano Babic wrote:
> On 31/10/2011 17:23, Simon Schwarz wrote:
>> From: Simon Schwarz<simonschwarzcor@googlemail.com>
>>
>> spl_nand overwrote BSS section because it reads a whole block everytime. Now
>> loads the block to spare area and just copy the needed junk to destination.
>> Whole block read is necessary for ecc check!
>>
>> Signed-off-by: Simon Schwarz<simonschwarzcor@gmail.com>
>> ---
>
>> @@ -71,7 +71,8 @@ void spl_nand_load_image(void)
>>   			CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
>>   		spl_parse_image_header(header);
>>   		nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS,
>> -			spl_image.size, (void *)spl_image.load_addr);
>> +			spl_image.size,
>> +			(void *)spl_image.load_addr - sizeof(header));
>                                                               ^---
>
>
> Do you mean maybe sizeof(*header) ?
>
> However, spl_image.load_addr was already decremented in
> spl_parse_image_header() and correctly set to 64 byte before the load
> address. Do we really need it ?
>
> I found the readon of the kernel corrupt image. We are setting a very
> hard address in /nand_spl_simple.c:
>
> ecc_calc = (u_char *)(CONFIG_SYS_SDRAM_BASE + 0x10000);
>
> Because the image for a TI SOC is loaded at 0x80008000, we have a
> conflict and the image is corrupted where the ECC is computed.
>
> It is not a really good idea to fix in this way where to compute the
> ECC. Should be not better to put it in the CONFIG_SYS_INIT_RAM_ADDR area ?
>
> Best regards,
> Stefano Babic
>

Hmm. This is from the former nand_spl.

Why not use malloc for this? Thanks to changes in the FAT driver we now 
have it in the SPL.

I don't see a reason to have this in SRAM when SDRAM is available.

Regards
Simon
Simon Schwarz Dec. 6, 2011, 6:06 p.m. UTC | #3 
On 12/06/2011 06:18 PM, Stefano Babic wrote:
> On 31/10/2011 17:23, Simon Schwarz wrote:
>> From: Simon Schwarz<simonschwarzcor@googlemail.com>
>>
>> spl_nand overwrote BSS section because it reads a whole block everytime. Now
>> loads the block to spare area and just copy the needed junk to destination.
>> Whole block read is necessary for ecc check!
>>
>> Signed-off-by: Simon Schwarz<simonschwarzcor@gmail.com>
>> ---
>
>> @@ -71,7 +71,8 @@ void spl_nand_load_image(void)
>>   			CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
>>   		spl_parse_image_header(header);
>>   		nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS,
>> -			spl_image.size, (void *)spl_image.load_addr);
>> +			spl_image.size,
>> +			(void *)spl_image.load_addr - sizeof(header));
>                                                               ^---
>
>
> Do you mean maybe sizeof(*header) ?
>
> However, spl_image.load_addr was already decremented in
> spl_parse_image_header() and correctly set to 64 byte before the load
> address. Do we really need it ?
>
[SNIP]
> Best regards,
> Stefano Babic
>

You are right -> V9. Lets see if we get it to two digits ;)

Regards
Simon
Stefano Babic Dec. 6, 2011, 6:21 p.m. UTC | #4 
On 06/12/2011 18:53, Simon Schwarz wrote:

> Hmm. This is from the former nand_spl.

Yes, I know. Until the load address does not conflict with the fix
offset, we have never seen this problem.

> Why not use malloc for this?

Of course, malloc - I want only to get rid of such hardcoded values ;-)

> Thanks to changes in the FAT driver we now
> have it in the SPL.
> 
> I don't see a reason to have this in SRAM when SDRAM is available.

No reason at all, of course.

Stefano
Wolfgang Denk Dec. 6, 2011, 6:21 p.m. UTC | #5 
Dear Simon Schwarz,

In message <4EDE56AB.3070504@gmail.com> you wrote:
>
> Why not use malloc for this? Thanks to changes in the FAT driver we now 
> have it in the SPL.

No, please do NOT do that!!!

I knew I should have never accepted to allow this in the SPL fromt he
beginning, and indeed it was a mistake not to block this.

Please now don't use it without thinking if it's really, really
needed!


I recommend NOT to use malloc() at all in SPL code.

[Guess I should have a more carefull eye on all SPL related
submissions and install a "grep-malloc-and-NAK" script. :-( ]

Best regards,

Wolfgang Denk
Scott Wood Dec. 6, 2011, 7:42 p.m. UTC | #6 
On 12/06/2011 11:53 AM, Simon Schwarz wrote:
> On 12/06/2011 06:18 PM, Stefano Babic wrote:
>> I found the readon of the kernel corrupt image. We are setting a very
>> hard address in /nand_spl_simple.c:
>>
>> ecc_calc = (u_char *)(CONFIG_SYS_SDRAM_BASE + 0x10000);
>>
>> Because the image for a TI SOC is loaded at 0x80008000, we have a
>> conflict and the image is corrupted where the ECC is computed.
>>
>> It is not a really good idea to fix in this way where to compute the
>> ECC. Should be not better to put it in the CONFIG_SYS_INIT_RAM_ADDR
>> area ?

There's a patch to fix this (with some minor changes requested):
http://patchwork.ozlabs.org/patch/128018/

>> Best regards,
>> Stefano Babic
>>
> 
> Hmm. This is from the former nand_spl.
> 
> Why not use malloc for this? Thanks to changes in the FAT driver we now
> have it in the SPL.
> 
> I don't see a reason to have this in SRAM when SDRAM is available.

nand_spl_simple.c is used by some very small SPL targets -- 4K or so.
malloc doesn't fit.

-Scott
diff mbox

Patch

diff --git a/arch/arm/cpu/armv7/omap-common/spl_nand.c b/arch/arm/cpu/armv7/omap-common/spl_nand.c
index 1b9e171..903255b 100644
--- a/arch/arm/cpu/armv7/omap-common/spl_nand.c
+++ b/arch/arm/cpu/armv7/omap-common/spl_nand.c
@@ -71,7 +71,8 @@  void spl_nand_load_image(void)
 			CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
 		spl_parse_image_header(header);
 		nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS,
-			spl_image.size, (void *)spl_image.load_addr);
+			spl_image.size,
+			(void *)spl_image.load_addr - sizeof(header));
 	} else
 #endif
 	{