| Message ID | 1366066255-18192-3-git-send-email-swarren@wwwdotorg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 15, 2013 at 04:50:55PM -0600, Stephen Warren wrote: > From: Joseph Lo <josephl@nvidia.com> > > When building kernel with CONFIG_THUMB2_KERNEL, the data pointer in the > assembly may not on the 4 byte alignment. Then causing a data abort when > accessing the pointer. This patch add a ".align" flag in the head of the > pointer. And always using 32-bit ADR Thumb instruction to make sure it > won't build failure. > > Signed-off-by: Joseph Lo <josephl@nvidia.com> > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > arch/arm/mach-tegra/reset-handler.S | 1 + > arch/arm/mach-tegra/sleep.h | 3 ++- > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S > index e6de88a..519a8c5 100644 > --- a/arch/arm/mach-tegra/reset-handler.S > +++ b/arch/arm/mach-tegra/reset-handler.S > @@ -83,6 +83,7 @@ ENDPROC(tegra_resume) > > #ifdef CONFIG_CACHE_L2X0 > .globl l2x0_saved_regs_addr > + .align > l2x0_saved_regs_addr: > .long 0 > #endif > diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h > index 4ffae54..8e9b6af 100644 > --- a/arch/arm/mach-tegra/sleep.h > +++ b/arch/arm/mach-tegra/sleep.h > @@ -92,7 +92,8 @@ > > #ifdef CONFIG_CACHE_L2X0 > .macro l2_cache_resume, tmp1, tmp2, tmp3, phys_l2x0_saved_regs > - adr \tmp1, \phys_l2x0_saved_regs > + ARM( adr \tmp1, \phys_l2x0_saved_regs ) > + THUMB( adr.w \tmp1, \phys_l2x0_saved_regs ) Can you give an example of the assembler error you get without this? The target symbol is local and the assembler can see where it is, so it should choose the correct variant for the adr instruction with no need for the ".w" suffix. If not, it could mean that there is a bug in the version of the assembler you're using. If it's definitely needed, you can append a Thumb-only .w suffix with W(adr) \tmp1, \phys_l2x0_saved_regs ...which is slightly neater. Cheers ---Dave > ldr \tmp1, [\tmp1] > ldr \tmp2, [\tmp1, #L2X0_R_PHY_BASE] > ldr \tmp3, [\tmp2, #L2X0_CTRL] > -- > 1.7.10.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 04/16/2013 08:13 AM, Dave Martin wrote: > On Mon, Apr 15, 2013 at 04:50:55PM -0600, Stephen Warren wrote: >> From: Joseph Lo <josephl@nvidia.com> >> >> When building kernel with CONFIG_THUMB2_KERNEL, the data pointer in the >> assembly may not on the 4 byte alignment. Then causing a data abort when >> accessing the pointer. This patch add a ".align" flag in the head of the >> pointer. And always using 32-bit ADR Thumb instruction to make sure it >> won't build failure. >> >> Signed-off-by: Joseph Lo <josephl@nvidia.com> >> Signed-off-by: Stephen Warren <swarren@nvidia.com> >> --- >> arch/arm/mach-tegra/reset-handler.S | 1 + >> arch/arm/mach-tegra/sleep.h | 3 ++- >> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S >> index e6de88a..519a8c5 100644 >> --- a/arch/arm/mach-tegra/reset-handler.S >> +++ b/arch/arm/mach-tegra/reset-handler.S >> @@ -83,6 +83,7 @@ ENDPROC(tegra_resume) >> >> #ifdef CONFIG_CACHE_L2X0 >> .globl l2x0_saved_regs_addr >> + .align >> l2x0_saved_regs_addr: >> .long 0 >> #endif >> diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h >> index 4ffae54..8e9b6af 100644 >> --- a/arch/arm/mach-tegra/sleep.h >> +++ b/arch/arm/mach-tegra/sleep.h >> @@ -92,7 +92,8 @@ >> >> #ifdef CONFIG_CACHE_L2X0 >> .macro l2_cache_resume, tmp1, tmp2, tmp3, phys_l2x0_saved_regs >> - adr \tmp1, \phys_l2x0_saved_regs >> + ARM( adr \tmp1, \phys_l2x0_saved_regs ) >> + THUMB( adr.w \tmp1, \phys_l2x0_saved_regs ) > > Can you give an example of the assembler error you get without this? arch/arm/mach-tegra/reset-handler.S: Assembler messages: arch/arm/mach-tegra/reset-handler.S:78: Error: invalid immediate for address calculation (value = 0x00000004) This is with gcc-4.5.3, as: GNU assembler (crosstool-NG hg_unknown@20110628.165246) 2.20.1.20100303 > The target symbol is local and the assembler can see where it is, > so it should choose the correct variant for the adr instruction > with no need for the ".w" suffix. > > If not, it could mean that there is a bug in the version of the > assembler you're using. True. Switching to the Ubuntu-packaged ARM cross-compiler that ships with Ubuntu 12.10, I do not see this problem. Still, many people probably still use older compilers. > If it's definitely needed, you can append a Thumb-only .w suffix with > > W(adr) \tmp1, \phys_l2x0_saved_regs > > ...which is slightly neater. Yes, that certainly works too, even without the .align change in reset-handler.S. Arnd, Olof, I guess let's drop this once patch from this series, but apply the other two, and I'll send an updated version of this one patch. Thanks.
On Tue, Apr 16, 2013 at 10:20:11AM -0600, Stephen Warren wrote: > On 04/16/2013 08:13 AM, Dave Martin wrote: > > On Mon, Apr 15, 2013 at 04:50:55PM -0600, Stephen Warren wrote: > >> From: Joseph Lo <josephl@nvidia.com> > >> > >> When building kernel with CONFIG_THUMB2_KERNEL, the data pointer in the > >> assembly may not on the 4 byte alignment. Then causing a data abort when > >> accessing the pointer. This patch add a ".align" flag in the head of the > >> pointer. And always using 32-bit ADR Thumb instruction to make sure it > >> won't build failure. > >> > >> Signed-off-by: Joseph Lo <josephl@nvidia.com> > >> Signed-off-by: Stephen Warren <swarren@nvidia.com> > >> --- > >> arch/arm/mach-tegra/reset-handler.S | 1 + > >> arch/arm/mach-tegra/sleep.h | 3 ++- > >> 2 files changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S > >> index e6de88a..519a8c5 100644 > >> --- a/arch/arm/mach-tegra/reset-handler.S > >> +++ b/arch/arm/mach-tegra/reset-handler.S > >> @@ -83,6 +83,7 @@ ENDPROC(tegra_resume) > >> > >> #ifdef CONFIG_CACHE_L2X0 > >> .globl l2x0_saved_regs_addr > >> + .align > >> l2x0_saved_regs_addr: > >> .long 0 > >> #endif > >> diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h > >> index 4ffae54..8e9b6af 100644 > >> --- a/arch/arm/mach-tegra/sleep.h > >> +++ b/arch/arm/mach-tegra/sleep.h > >> @@ -92,7 +92,8 @@ > >> > >> #ifdef CONFIG_CACHE_L2X0 > >> .macro l2_cache_resume, tmp1, tmp2, tmp3, phys_l2x0_saved_regs > >> - adr \tmp1, \phys_l2x0_saved_regs > >> + ARM( adr \tmp1, \phys_l2x0_saved_regs ) > >> + THUMB( adr.w \tmp1, \phys_l2x0_saved_regs ) > > > > Can you give an example of the assembler error you get without this? > > arch/arm/mach-tegra/reset-handler.S: Assembler messages: > arch/arm/mach-tegra/reset-handler.S:78: Error: invalid immediate for > address calculation (value = 0x00000004) The immediate in the 16-bit form of ADR is alignment-sensitive, and can't address anything that's not on a word boundary. What if you just have the .align, without adr.w? > This is with gcc-4.5.3, as: > GNU assembler (crosstool-NG hg_unknown@20110628.165246) 2.20.1.20100303 > > > The target symbol is local and the assembler can see where it is, > > so it should choose the correct variant for the adr instruction > > with no need for the ".w" suffix. > > > > If not, it could mean that there is a bug in the version of the > > assembler you're using. > > True. Switching to the Ubuntu-packaged ARM cross-compiler that ships > with Ubuntu 12.10, I do not see this problem. > > Still, many people probably still use older compilers. > > > If it's definitely needed, you can append a Thumb-only .w suffix with > > > > W(adr) \tmp1, \phys_l2x0_saved_regs > > > > ...which is slightly neater. > > Yes, that certainly works too, even without the .align change in > reset-handler.S. Sure, although the unaligned access is not such a good idea anyway. Keeping the adr fix is harmless, I just wanted to understand why it's needed. Cheers ---Dave
On 04/17/2013 04:30 AM, Dave Martin wrote: > On Tue, Apr 16, 2013 at 10:20:11AM -0600, Stephen Warren wrote: >> On 04/16/2013 08:13 AM, Dave Martin wrote: >>> On Mon, Apr 15, 2013 at 04:50:55PM -0600, Stephen Warren wrote: >>>> From: Joseph Lo <josephl@nvidia.com> >>>> >>>> When building kernel with CONFIG_THUMB2_KERNEL, the data pointer in the >>>> assembly may not on the 4 byte alignment. Then causing a data abort when >>>> accessing the pointer. This patch add a ".align" flag in the head of the >>>> pointer. And always using 32-bit ADR Thumb instruction to make sure it >>>> won't build failure. >>>> >>>> Signed-off-by: Joseph Lo <josephl@nvidia.com> >>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>>> --- >>>> arch/arm/mach-tegra/reset-handler.S | 1 + >>>> arch/arm/mach-tegra/sleep.h | 3 ++- >>>> 2 files changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S >>>> index e6de88a..519a8c5 100644 >>>> --- a/arch/arm/mach-tegra/reset-handler.S >>>> +++ b/arch/arm/mach-tegra/reset-handler.S >>>> @@ -83,6 +83,7 @@ ENDPROC(tegra_resume) >>>> >>>> #ifdef CONFIG_CACHE_L2X0 >>>> .globl l2x0_saved_regs_addr >>>> + .align >>>> l2x0_saved_regs_addr: >>>> .long 0 >>>> #endif >>>> diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h >>>> index 4ffae54..8e9b6af 100644 >>>> --- a/arch/arm/mach-tegra/sleep.h >>>> +++ b/arch/arm/mach-tegra/sleep.h >>>> @@ -92,7 +92,8 @@ >>>> >>>> #ifdef CONFIG_CACHE_L2X0 >>>> .macro l2_cache_resume, tmp1, tmp2, tmp3, phys_l2x0_saved_regs >>>> - adr \tmp1, \phys_l2x0_saved_regs >>>> + ARM( adr \tmp1, \phys_l2x0_saved_regs ) >>>> + THUMB( adr.w \tmp1, \phys_l2x0_saved_regs ) >>> >>> Can you give an example of the assembler error you get without this? >> >> arch/arm/mach-tegra/reset-handler.S: Assembler messages: >> arch/arm/mach-tegra/reset-handler.S:78: Error: invalid immediate for >> address calculation (value = 0x00000004) > > The immediate in the 16-bit form of ADR is alignment-sensitive, and > can't address anything that's not on a word boundary. What if you just > have the .align, without adr.w? > >> This is with gcc-4.5.3, as: >> GNU assembler (crosstool-NG hg_unknown@20110628.165246) 2.20.1.20100303 >> >>> The target symbol is local and the assembler can see where it is, >>> so it should choose the correct variant for the adr instruction >>> with no need for the ".w" suffix. >>> >>> If not, it could mean that there is a bug in the version of the >>> assembler you're using. >> >> True. Switching to the Ubuntu-packaged ARM cross-compiler that ships >> with Ubuntu 12.10, I do not see this problem. >> >> Still, many people probably still use older compilers. >> >>> If it's definitely needed, you can append a Thumb-only .w suffix with >>> >>> W(adr) \tmp1, \phys_l2x0_saved_regs >>> >>> ...which is slightly neater. >> >> Yes, that certainly works too, even without the .align change in >> reset-handler.S. > > Sure, although the unaligned access is not such a good idea anyway. > > Keeping the adr fix is harmless, I just wanted to understand why it's > needed. With just the following, and no adr.w: > diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S > index e6de88a..519a8c5 100644 > --- a/arch/arm/mach-tegra/reset-handler.S > +++ b/arch/arm/mach-tegra/reset-handler.S > @@ -83,6 +83,7 @@ ENDPROC(tegra_resume) > > #ifdef CONFIG_CACHE_L2X0 > .globl l2x0_saved_regs_addr > + .align > l2x0_saved_regs_addr: > .long 0 > #endif I still get: > arch/arm/mach-tegra/reset-handler.S: Assembler messages: > arch/arm/mach-tegra/reset-handler.S:78: Error: invalid immediate for address calculation (value = 0x00000004) That's with GNU assembler (crosstool-NG hg_unknown@20110628.165246) 2.20.1.20100303. BTW, I'm on vacation for the next 2 weeks starting this afternoon. Hopefully Joseph can keep this thread alive while I'm gone if needed.
On Wed, Apr 17, 2013 at 10:22:02AM -0600, Stephen Warren wrote: > On 04/17/2013 04:30 AM, Dave Martin wrote: > > On Tue, Apr 16, 2013 at 10:20:11AM -0600, Stephen Warren wrote: > >> On 04/16/2013 08:13 AM, Dave Martin wrote: > >>> On Mon, Apr 15, 2013 at 04:50:55PM -0600, Stephen Warren wrote: > >>>> From: Joseph Lo <josephl@nvidia.com> > >>>> > >>>> When building kernel with CONFIG_THUMB2_KERNEL, the data pointer in the > >>>> assembly may not on the 4 byte alignment. Then causing a data abort when > >>>> accessing the pointer. This patch add a ".align" flag in the head of the > >>>> pointer. And always using 32-bit ADR Thumb instruction to make sure it > >>>> won't build failure. > >>>> > >>>> Signed-off-by: Joseph Lo <josephl@nvidia.com> > >>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> > >>>> --- > >>>> arch/arm/mach-tegra/reset-handler.S | 1 + > >>>> arch/arm/mach-tegra/sleep.h | 3 ++- > >>>> 2 files changed, 3 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S > >>>> index e6de88a..519a8c5 100644 > >>>> --- a/arch/arm/mach-tegra/reset-handler.S > >>>> +++ b/arch/arm/mach-tegra/reset-handler.S > >>>> @@ -83,6 +83,7 @@ ENDPROC(tegra_resume) > >>>> > >>>> #ifdef CONFIG_CACHE_L2X0 > >>>> .globl l2x0_saved_regs_addr > >>>> + .align > >>>> l2x0_saved_regs_addr: > >>>> .long 0 > >>>> #endif > >>>> diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h > >>>> index 4ffae54..8e9b6af 100644 > >>>> --- a/arch/arm/mach-tegra/sleep.h > >>>> +++ b/arch/arm/mach-tegra/sleep.h > >>>> @@ -92,7 +92,8 @@ > >>>> > >>>> #ifdef CONFIG_CACHE_L2X0 > >>>> .macro l2_cache_resume, tmp1, tmp2, tmp3, phys_l2x0_saved_regs > >>>> - adr \tmp1, \phys_l2x0_saved_regs > >>>> + ARM( adr \tmp1, \phys_l2x0_saved_regs ) > >>>> + THUMB( adr.w \tmp1, \phys_l2x0_saved_regs ) > >>> > >>> Can you give an example of the assembler error you get without this? > >> > >> arch/arm/mach-tegra/reset-handler.S: Assembler messages: > >> arch/arm/mach-tegra/reset-handler.S:78: Error: invalid immediate for > >> address calculation (value = 0x00000004) > > > > The immediate in the 16-bit form of ADR is alignment-sensitive, and > > can't address anything that's not on a word boundary. What if you just > > have the .align, without adr.w? > > > >> This is with gcc-4.5.3, as: > >> GNU assembler (crosstool-NG hg_unknown@20110628.165246) 2.20.1.20100303 > >> > >>> The target symbol is local and the assembler can see where it is, > >>> so it should choose the correct variant for the adr instruction > >>> with no need for the ".w" suffix. > >>> > >>> If not, it could mean that there is a bug in the version of the > >>> assembler you're using. > >> > >> True. Switching to the Ubuntu-packaged ARM cross-compiler that ships > >> with Ubuntu 12.10, I do not see this problem. > >> > >> Still, many people probably still use older compilers. > >> > >>> If it's definitely needed, you can append a Thumb-only .w suffix with > >>> > >>> W(adr) \tmp1, \phys_l2x0_saved_regs > >>> > >>> ...which is slightly neater. > >> > >> Yes, that certainly works too, even without the .align change in > >> reset-handler.S. > > > > Sure, although the unaligned access is not such a good idea anyway. > > > > Keeping the adr fix is harmless, I just wanted to understand why it's > > needed. > > With just the following, and no adr.w: > > > diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S > > index e6de88a..519a8c5 100644 > > --- a/arch/arm/mach-tegra/reset-handler.S > > +++ b/arch/arm/mach-tegra/reset-handler.S > > @@ -83,6 +83,7 @@ ENDPROC(tegra_resume) > > > > #ifdef CONFIG_CACHE_L2X0 > > .globl l2x0_saved_regs_addr > > + .align > > l2x0_saved_regs_addr: > > .long 0 > > #endif > > I still get: > > > arch/arm/mach-tegra/reset-handler.S: Assembler messages: > > arch/arm/mach-tegra/reset-handler.S:78: Error: invalid immediate for address calculation (value = 0x00000004) > > That's with GNU assembler (crosstool-NG hg_unknown@20110628.165246) > 2.20.1.20100303. > > BTW, I'm on vacation for the next 2 weeks starting this afternoon. > Hopefully Joseph can keep this thread alive while I'm gone if needed. OK. I'm happy to go with the W() so long definitely fixes something :) Cheers ---Dave
On Mon, Apr 15, 2013 at 04:50:55PM -0600, Stephen Warren wrote: > From: Joseph Lo <josephl@nvidia.com> > > When building kernel with CONFIG_THUMB2_KERNEL, the data pointer in the > assembly may not on the 4 byte alignment. Then causing a data abort when > accessing the pointer. This patch add a ".align" flag in the head of the > pointer. And always using 32-bit ADR Thumb instruction to make sure it > won't build failure. > > Signed-off-by: Joseph Lo <josephl@nvidia.com> > Signed-off-by: Stephen Warren <swarren@nvidia.com> With the change to W(adr), Reviewed-by: Dave Martin <dave.martin@linaro.org> > --- > arch/arm/mach-tegra/reset-handler.S | 1 + > arch/arm/mach-tegra/sleep.h | 3 ++- > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S > index e6de88a..519a8c5 100644 > --- a/arch/arm/mach-tegra/reset-handler.S > +++ b/arch/arm/mach-tegra/reset-handler.S > @@ -83,6 +83,7 @@ ENDPROC(tegra_resume) > > #ifdef CONFIG_CACHE_L2X0 > .globl l2x0_saved_regs_addr > + .align > l2x0_saved_regs_addr: > .long 0 > #endif > diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h > index 4ffae54..8e9b6af 100644 > --- a/arch/arm/mach-tegra/sleep.h > +++ b/arch/arm/mach-tegra/sleep.h > @@ -92,7 +92,8 @@ > > #ifdef CONFIG_CACHE_L2X0 > .macro l2_cache_resume, tmp1, tmp2, tmp3, phys_l2x0_saved_regs > - adr \tmp1, \phys_l2x0_saved_regs > + ARM( adr \tmp1, \phys_l2x0_saved_regs ) > + THUMB( adr.w \tmp1, \phys_l2x0_saved_regs ) > ldr \tmp1, [\tmp1] > ldr \tmp2, [\tmp1, #L2X0_R_PHY_BASE] > ldr \tmp3, [\tmp2, #L2X0_CTRL] > -- > 1.7.10.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Apr 16, 2013 at 10:20:11AM -0600, Stephen Warren wrote: > Arnd, Olof, I guess let's drop this once patch from this series, but > apply the other two, and I'll send an updated version of this one patch. > Thanks. Ok.
diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S index e6de88a..519a8c5 100644 --- a/arch/arm/mach-tegra/reset-handler.S +++ b/arch/arm/mach-tegra/reset-handler.S @@ -83,6 +83,7 @@ ENDPROC(tegra_resume) #ifdef CONFIG_CACHE_L2X0 .globl l2x0_saved_regs_addr + .align l2x0_saved_regs_addr: .long 0 #endif diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h index 4ffae54..8e9b6af 100644 --- a/arch/arm/mach-tegra/sleep.h +++ b/arch/arm/mach-tegra/sleep.h @@ -92,7 +92,8 @@ #ifdef CONFIG_CACHE_L2X0 .macro l2_cache_resume, tmp1, tmp2, tmp3, phys_l2x0_saved_regs - adr \tmp1, \phys_l2x0_saved_regs + ARM( adr \tmp1, \phys_l2x0_saved_regs ) + THUMB( adr.w \tmp1, \phys_l2x0_saved_regs ) ldr \tmp1, [\tmp1] ldr \tmp2, [\tmp1, #L2X0_R_PHY_BASE] ldr \tmp3, [\tmp2, #L2X0_CTRL]