Re: [U-Boot] [PATCH] x86: fix memalign() parameter order

[Bin Meng]

On Wed, Feb 17, 2016 at 4:20 PM, Bin Meng  wrote:
> On Sun, Feb 14, 2016 at 11:19 AM, Simon Glass  wrote:
>> On 12 February 2016 at 14:27, Stephen Warren  wrote:
>>> From: Stephen Warren 
>>>
>>> Purely by code inspection, it looks like the parameter order to memalign()
>>> is swapped; its parameters are (align, size). 4096 is a likely desired
>>> alignment, and a variable named size sounds like a size:-)
>>>
>>> Fixes: 45b5a37836d5 ("x86: Add multi-processor init")
>>> Signed-off-by: Stephen Warren 
>>> ---
>>> I've taken a quick look at all the other memalign() calls in U-Boot, and
>>> I /think/ they're all correct.
>>> ---
>>> arch/x86/cpu/mp_init.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/cpu/mp_init.c b/arch/x86/cpu/mp_init.c
>>> index 7917350bff26..fc2fb5bf445c 100644
>>> --- a/arch/x86/cpu/mp_init.c
>>> +++ b/arch/x86/cpu/mp_init.c
>>> @@ -243,7 +243,7 @@ static int load_sipi_vector(atomic_t **ap_countp, int
>>> num_cpus)
>>>
>>> params->stack_size = CONFIG_AP_STACK_SIZE;
>>> size = params->stack_size * num_cpus;
>>> - stack = memalign(size, 4096);
>>> + stack = memalign(4096, size);
>>> if (!stack)
>>> return -ENOMEM;
>>> params->stack_top = (u32)(stack + size);
>>> --
>>> 2.7.0
>>>
>>
>> Reviewed-by: Simon Glass 
>>
>> Thanks. I'm a little surprised this hasn't caused problems with CPU
>> start-up!
>
> Tested on QEMU
> Tested-by: Bin Meng 

applied to u-boot-x86/master, thanks!
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] x86: fix memalign() parameter order

[Bin Meng]

On Sun, Feb 14, 2016 at 11:19 AM, Simon Glass  wrote:
> On 12 February 2016 at 14:27, Stephen Warren  wrote:
>> From: Stephen Warren 
>>
>> Purely by code inspection, it looks like the parameter order to memalign()
>> is swapped; its parameters are (align, size). 4096 is a likely desired
>> alignment, and a variable named size sounds like a size:-)
>>
>> Fixes: 45b5a37836d5 ("x86: Add multi-processor init")
>> Signed-off-by: Stephen Warren 
>> ---
>> I've taken a quick look at all the other memalign() calls in U-Boot, and
>> I /think/ they're all correct.
>> ---
>> arch/x86/cpu/mp_init.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/cpu/mp_init.c b/arch/x86/cpu/mp_init.c
>> index 7917350bff26..fc2fb5bf445c 100644
>> --- a/arch/x86/cpu/mp_init.c
>> +++ b/arch/x86/cpu/mp_init.c
>> @@ -243,7 +243,7 @@ static int load_sipi_vector(atomic_t **ap_countp, int
>> num_cpus)
>>
>> params->stack_size = CONFIG_AP_STACK_SIZE;
>> size = params->stack_size * num_cpus;
>> - stack = memalign(size, 4096);
>> + stack = memalign(4096, size);
>> if (!stack)
>> return -ENOMEM;
>> params->stack_top = (u32)(stack + size);
>> --
>> 2.7.0
>>
>
> Reviewed-by: Simon Glass 
>
> Thanks. I'm a little surprised this hasn't caused problems with CPU
> start-up!

Tested on QEMU
Tested-by: Bin Meng 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] x86: fix memalign() parameter order

[Bin Meng]

Hi Stephen,

On Sat, Feb 13, 2016 at 5:27 AM, Stephen Warren  wrote:
> From: Stephen Warren 
>
> Purely by code inspection, it looks like the parameter order to memalign()
> is swapped; its parameters are (align, size). 4096 is a likely desired
> alignment, and a variable named size sounds like a size:-)
>

Good catch! It was lucky that this did not corrupt any important data before ..

> Fixes: 45b5a37836d5 ("x86: Add multi-processor init")
> Signed-off-by: Stephen Warren 
> ---

Reviewed-by: Bin Meng 

> I've taken a quick look at all the other memalign() calls in U-Boot, and
> I /think/ they're all correct.
> ---
>  arch/x86/cpu/mp_init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/cpu/mp_init.c b/arch/x86/cpu/mp_init.c
> index 7917350bff26..fc2fb5bf445c 100644
> --- a/arch/x86/cpu/mp_init.c
> +++ b/arch/x86/cpu/mp_init.c
> @@ -243,7 +243,7 @@ static int load_sipi_vector(atomic_t **ap_countp, int 
> num_cpus)
>
> params->stack_size = CONFIG_AP_STACK_SIZE;
> size = params->stack_size * num_cpus;
> -   stack = memalign(size, 4096);
> +   stack = memalign(4096, size);
> if (!stack)
> return -ENOMEM;
> params->stack_top = (u32)(stack + size);
> --

Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] x86: fix memalign() parameter order

[Simon Glass]

On 12 February 2016 at 14:27, Stephen Warren  wrote:
> From: Stephen Warren 
>
> Purely by code inspection, it looks like the parameter order to memalign()
> is swapped; its parameters are (align, size). 4096 is a likely desired
> alignment, and a variable named size sounds like a size:-)
>
> Fixes: 45b5a37836d5 ("x86: Add multi-processor init")
> Signed-off-by: Stephen Warren 
> ---
> I've taken a quick look at all the other memalign() calls in U-Boot, and
> I /think/ they're all correct.
> ---
> arch/x86/cpu/mp_init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/cpu/mp_init.c b/arch/x86/cpu/mp_init.c
> index 7917350bff26..fc2fb5bf445c 100644
> --- a/arch/x86/cpu/mp_init.c
> +++ b/arch/x86/cpu/mp_init.c
> @@ -243,7 +243,7 @@ static int load_sipi_vector(atomic_t **ap_countp, int
num_cpus)
>
> params->stack_size = CONFIG_AP_STACK_SIZE;
> size = params->stack_size * num_cpus;
> - stack = memalign(size, 4096);
> + stack = memalign(4096, size);
> if (!stack)
> return -ENOMEM;
> params->stack_top = (u32)(stack + size);
> --
> 2.7.0
>

Reviewed-by: Simon Glass 

Thanks. I'm a little surprised this hasn't caused problems with CPU
start-up!

- Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] x86: fix memalign() parameter order

[Stephen Warren]

From: Stephen Warren 

Purely by code inspection, it looks like the parameter order to memalign()
is swapped; its parameters are (align, size). 4096 is a likely desired
alignment, and a variable named size sounds like a size:-)

Fixes: 45b5a37836d5 ("x86: Add multi-processor init")
Signed-off-by: Stephen Warren 
---
I've taken a quick look at all the other memalign() calls in U-Boot, and
I /think/ they're all correct.
---
 arch/x86/cpu/mp_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/cpu/mp_init.c b/arch/x86/cpu/mp_init.c
index 7917350bff26..fc2fb5bf445c 100644
--- a/arch/x86/cpu/mp_init.c
+++ b/arch/x86/cpu/mp_init.c
@@ -243,7 +243,7 @@ static int load_sipi_vector(atomic_t **ap_countp, int 
num_cpus)
 
params->stack_size = CONFIG_AP_STACK_SIZE;
size = params->stack_size * num_cpus;
-   stack = memalign(size, 4096);
+   stack = memalign(4096, size);
if (!stack)
return -ENOMEM;
params->stack_top = (u32)(stack + size);
-- 
2.7.0

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot