| Message ID | 1455312476-23875-1-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Accepted |
| Commit | 4fd64d02b2bb7fe583c1246c79b9658223d96442 |
| Delegated to: | Bin Meng |
| Headers | show |
Hi Stephen, On Sat, Feb 13, 2016 at 5:27 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > From: Stephen Warren <swarren@nvidia.com> > > 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 <swarren@nvidia.com> > --- Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > 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
On 12 February 2016 at 14:27, Stephen Warren <swarren@wwwdotorg.org> wrote: > From: Stephen Warren <swarren@nvidia.com> > > 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 <swarren@nvidia.com> > --- > 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 <sjg@chromium.org> Thanks. I'm a little surprised this hasn't caused problems with CPU start-up! - Simon
On Sun, Feb 14, 2016 at 11:19 AM, Simon Glass <sjg@chromium.org> wrote: > On 12 February 2016 at 14:27, Stephen Warren <swarren@wwwdotorg.org> wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> 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 <swarren@nvidia.com> >> --- >> 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 <sjg@chromium.org> > > Thanks. I'm a little surprised this hasn't caused problems with CPU > start-up! Tested on QEMU Tested-by: Bin Meng <bmeng.cn@gmail.com>
On Wed, Feb 17, 2016 at 4:20 PM, Bin Meng <bmeng.cn@gmail.com> wrote: > On Sun, Feb 14, 2016 at 11:19 AM, Simon Glass <sjg@chromium.org> wrote: >> On 12 February 2016 at 14:27, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> From: Stephen Warren <swarren@nvidia.com> >>> >>> 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 <swarren@nvidia.com> >>> --- >>> 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 <sjg@chromium.org> >> >> Thanks. I'm a little surprised this hasn't caused problems with CPU >> start-up! > > Tested on QEMU > Tested-by: Bin Meng <bmeng.cn@gmail.com> applied to u-boot-x86/master, thanks!
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);