[U-Boot] armv7: Fix infinite loop for the spl boot

Submitted by seedshope on July 6, 2012, 11:42 a.m.

Details

Message ID 4FF6CF0A.7010300@gmail.com
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Commit Message

seedshope July 6, 2012, 11:42 a.m.
On 07/06/2012 01:35 AM, Albert ARIBAUD <albert.u.boot@aribaud.net> (by
way of Albert ARIBAUD wrote:
> Hi Zhong Hongbo,
> 
> On Thu, 05 Jul 2012 19:53:46 +0800, Zhong Hongbo <bocui107@gmail.com>
> wrote:
>> Hi Albert,
>>
>> Could you applied the patch to the arm tree?
>>
>> Thanks,
>> hongbo
>> On 07/03/2012 07:46 AM, Zhong Hongbo wrote:
>>> From: Zhong Hongbo <bocui107@gmail.com>
>>>
>>> In the spl booting step, When __bss_start is equal to __bss_end__,
>>> The loop will clear all the things in CPU space. If there are have
>>> the same address for this symbol, To skip the clear bss section.
>>>
>>> Signed-off-by: Hongbo Zhong <bocui107@gmail.com>
>>> ---
>>>  arch/arm/cpu/armv7/start.S |    3 +++
>>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
>>> index 76ccef1..c72f337 100644
>>> --- a/arch/arm/cpu/armv7/start.S
>>> +++ b/arch/arm/cpu/armv7/start.S
>>> @@ -258,6 +258,8 @@ clear_bss:
>>>  	/* No relocation for SPL */
>>>  	ldr	r0, =__bss_start
>>>  	ldr	r1, =__bss_end__
>>> +	cmp	r0, r1
>>> +	beq	skip_clbss
>>>  #else
>>>  	ldr	r0, _bss_start_ofs
>>>  	ldr	r1, _bss_end_ofs
>>> @@ -271,6 +273,7 @@ clbss_l:str	r2, [r0]		/*
>>> clear loop...		    */ add	r0, r0, #4
>>>  	cmp	r0, r1
>>>  	bne	clbss_l
>>> +skip_clbss:
> 
> Clearly the loop was wrong in that it should implement a "for (r0 =
> start; r0 < end; r0++)" but actually implements a "for (r0 =
> start; r0 != end; r0++)".
> 

Yes, My new patch have do this.
> I'd rather the loop be fixed to match the intended implementation
> rather than worked around. Please rewrite your patch to turn:
> 

Ok, I just found the issue have found in other arm platfor 2011 yeas,
the detail information as following:

commit 8f1da53508c78789ebeea98a92a3f55c3f84dc5d
Author: Christian Riesch <christian.riesch@omicron.at>
Date:   Wed Nov 30 22:27:37 2011 +0000

    arm, arm926ejs: Fix clear bss loop for zero length bss

    This patch fixes the clear bss loop for bss sections that have
    zero length, i.e., where __bss_start == __bss_end__.

    Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
    Cc: Albert Aribaud <albert.u.boot@aribaud.net>

>>         add     r0, r0, #4
>>         cmp     r0, r1
>>         bne     clbss_l
> 
> Into something like
> 
>> clbss_l:cmp     r0, r1
>>         blo     clbss_d
>>         str     r2, [r0]  /* clear loop...*/
>>         add     r0, r0, #4
>>         b       clbss_l
>> clbss_d:
> 
> Also, as Andreas points out, make sure the same fix is applied to all ARM start.S files which need it.

Ok,

Thanks,
hongbo
> 
> Thanks in advance.
> 
> Amicalement,
>

Comments

Andreas Bießmann July 6, 2012, 11:49 a.m.
Dear Zhong Hongbo,

On 06.07.2012 13:42, Zhong Hongbo wrote:
> On 07/06/2012 01:35 AM, Albert ARIBAUD <albert.u.boot@aribaud.net> (by
> way of Albert ARIBAUD wrote:
>> Hi Zhong Hongbo,
>>
>> On Thu, 05 Jul 2012 19:53:46 +0800, Zhong Hongbo <bocui107@gmail.com>
>> wrote:

<snip>

> 
> Ok, I just found the issue have found in other arm platfor 2011 yeas,
> the detail information as following:
> 
> commit 8f1da53508c78789ebeea98a92a3f55c3f84dc5d
> Author: Christian Riesch <christian.riesch@omicron.at>
> Date:   Wed Nov 30 22:27:37 2011 +0000
> 
>     arm, arm926ejs: Fix clear bss loop for zero length bss
> 
>     This patch fixes the clear bss loop for bss sections that have
>     zero length, i.e., where __bss_start == __bss_end__.
> 
>     Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>     Cc: Albert Aribaud <albert.u.boot@aribaud.net>
> 
> diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
> index 339c5ed..bb4d00b 100644
> --- a/arch/arm/cpu/arm926ejs/start.S
> +++ b/arch/arm/cpu/arm926ejs/start.S
> @@ -301,10 +301,12 @@ clear_bss:
>  #endif
>         mov     r2, #0x00000000         /* clear
>     */
> 
> -clbss_l:str    r2, [r0]                /* clear loop...
>     */
> +clbss_l:cmp    r0, r1                  /* clear loop... */
> +       bhs     clbss_e                 /* if reached end of bss, exit */
> +       str     r2, [r0]
>         add     r0, r0, #4
> -       cmp     r0, r1
> -       bne     clbss_l
> +       b       clbss_l
> +clbss_e:
>>> clbss_l:str     r2, [r0]  /* clear loop...*/
>>>         add     r0, r0, #4
>>>         cmp     r0, r1
>>>         bne     clbss_l
>>
>> Into something like
>>
>>> clbss_l:cmp     r0, r1
>>>         blo     clbss_d
>>>         str     r2, [r0]  /* clear loop...*/
>>>         add     r0, r0, #4
>>>         b       clbss_l
>>> clbss_d:
>>
>> Also, as Andreas points out, make sure the same fix is applied to all ARM start.S files which need it.
> 
> Ok,

I think this is the thread we will proceed.
So as I understand we had missed in 2011 to update the other start.S
with a fix found by Christian Riesch for arm926ejs. So please do update
_all_ start.S (of arm;) now. I think they do have all the same error
cause we copied it over from one to the other when implementing the 'new
relocation' stuff.

Best regards

Andreas Bießmann
seedshope July 6, 2012, 11:53 a.m.
On 07/06/2012 07:49 PM, Andreas Bießmann wrote:
> Dear Zhong Hongbo,
> 
> On 06.07.2012 13:42, Zhong Hongbo wrote:
>> On 07/06/2012 01:35 AM, Albert ARIBAUD <albert.u.boot@aribaud.net> (by
>> way of Albert ARIBAUD wrote:
>>> Hi Zhong Hongbo,
>>>
>>> On Thu, 05 Jul 2012 19:53:46 +0800, Zhong Hongbo <bocui107@gmail.com>
>>> wrote:
> 
> <snip>
> 
>>
>> Ok, I just found the issue have found in other arm platfor 2011 yeas,
>> the detail information as following:
>>
>> commit 8f1da53508c78789ebeea98a92a3f55c3f84dc5d
>> Author: Christian Riesch <christian.riesch@omicron.at>
>> Date:   Wed Nov 30 22:27:37 2011 +0000
>>
>>     arm, arm926ejs: Fix clear bss loop for zero length bss
>>
>>     This patch fixes the clear bss loop for bss sections that have
>>     zero length, i.e., where __bss_start == __bss_end__.
>>
>>     Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>>     Cc: Albert Aribaud <albert.u.boot@aribaud.net>
>>
>> diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
>> index 339c5ed..bb4d00b 100644
>> --- a/arch/arm/cpu/arm926ejs/start.S
>> +++ b/arch/arm/cpu/arm926ejs/start.S
>> @@ -301,10 +301,12 @@ clear_bss:
>>  #endif
>>         mov     r2, #0x00000000         /* clear
>>     */
>>
>> -clbss_l:str    r2, [r0]                /* clear loop...
>>     */
>> +clbss_l:cmp    r0, r1                  /* clear loop... */
>> +       bhs     clbss_e                 /* if reached end of bss, exit */
>> +       str     r2, [r0]
>>         add     r0, r0, #4
>> -       cmp     r0, r1
>> -       bne     clbss_l
>> +       b       clbss_l
>> +clbss_e:
>>>> clbss_l:str     r2, [r0]  /* clear loop...*/
>>>>         add     r0, r0, #4
>>>>         cmp     r0, r1
>>>>         bne     clbss_l
>>>
>>> Into something like
>>>
>>>> clbss_l:cmp     r0, r1
>>>>         blo     clbss_d
>>>>         str     r2, [r0]  /* clear loop...*/
>>>>         add     r0, r0, #4
>>>>         b       clbss_l
>>>> clbss_d:
>>>
>>> Also, as Andreas points out, make sure the same fix is applied to all ARM start.S files which need it.
>>
>> Ok,
> 
> I think this is the thread we will proceed.
> So as I understand we had missed in 2011 to update the other start.S
> with a fix found by Christian Riesch for arm926ejs. So please do update
> _all_ start.S (of arm;) now. I think they do have all the same error
> cause we copied it over from one to the other when implementing the 'new
> relocation' stuff.
Agree, I will do it now.

Thanks,
hongbo
> 
> Best regards
> 
> Andreas Bießmann
> 
>

Patch hide | download patch | download mbox

diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
index 339c5ed..bb4d00b 100644
--- a/arch/arm/cpu/arm926ejs/start.S
+++ b/arch/arm/cpu/arm926ejs/start.S
@@ -301,10 +301,12 @@  clear_bss:
 #endif
        mov     r2, #0x00000000         /* clear
    */

-clbss_l:str    r2, [r0]                /* clear loop...
    */
+clbss_l:cmp    r0, r1                  /* clear loop... */
+       bhs     clbss_e                 /* if reached end of bss, exit */
+       str     r2, [r0]
        add     r0, r0, #4
-       cmp     r0, r1
-       bne     clbss_l
+       b       clbss_l
+clbss_e:
>> clbss_l:str     r2, [r0]  /* clear loop...*/