| Message ID | 1424878581-11701-1-git-send-email-bobby.prani@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 25 February 2015 10:36:20 Pranith Kumar wrote: > This patch adds support for a half-word xchg() for ARM using ldrexh/strexh > instructions. It also fixes an asm comment for __cmpxchg2. > > Currently using a half-word xchg() results in the following splat on an ARMv7 > machine. > > [ 45.833303] xchg: bad data size: pc 0xbe806020, ptr 0xeb18deee, size 2 > [ 45.833324] ------------[ cut here ]------------ > [ 45.837939] kernel BUG at /dvs/git/dirty/git-master_linux/kernel/arch/arm/kernel/traps.c:727! > > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> Unfortunately, the BUG message seems incomplete, can you reproduce this with CONFIG_DEBUG_BUGVERBOSE enabled? > arch/arm/include/asm/cmpxchg.h | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/include/asm/cmpxchg.h b/arch/arm/include/asm/cmpxchg.h > index abb2c37..9505cca 100644 > --- a/arch/arm/include/asm/cmpxchg.h > +++ b/arch/arm/include/asm/cmpxchg.h > @@ -50,6 +50,16 @@ static inline unsigned long __xchg(unsigned long x, volatile void *ptr, int size > : "r" (x), "r" (ptr) > : "memory", "cc"); > break; > + case 2: > + asm volatile("@ __xchg2\n" > + "1: ldrexh %0, [%3]\n" > + " strexh %1, %2, [%3]\n" > + " teq %1, #0\n" > + " bne 1b" > + : "=&r" (ret), "=&r" (tmp) > + : "r" (x), "r" (ptr) > + : "memory", "cc"); > + break; > case 4: > asm volatile("@ __xchg4\n" > "1: ldrex %0, [%3]\n" Does this work on all ARMv6 or just ARMv6k? Arnd
On Wed, Feb 25, 2015 at 10:58 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 25 February 2015 10:36:20 Pranith Kumar wrote: >> This patch adds support for a half-word xchg() for ARM using ldrexh/strexh >> instructions. It also fixes an asm comment for __cmpxchg2. >> >> Currently using a half-word xchg() results in the following splat on an ARMv7 >> machine. >> >> [ 45.833303] xchg: bad data size: pc 0xbe806020, ptr 0xeb18deee, size 2 >> [ 45.833324] ------------[ cut here ]------------ >> [ 45.837939] kernel BUG at /dvs/git/dirty/git-master_linux/kernel/arch/arm/kernel/traps.c:727! >> >> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > > Unfortunately, the BUG message seems incomplete, can you reproduce this > with CONFIG_DEBUG_BUGVERBOSE enabled? The bug here is in a module caused when xchg() was used on uint16_t variable. It is caused by the __bad_xchg() for 2 byte swap. More information: [ 45.833303] xchg: bad data size: pc 0xbe806020, ptr 0xeb18deee, size 2 [ 45.833324] ------------[ cut here ]------------ [ 45.837939] kernel BUG at /dvs/git/dirty/git-master_linux/kernel/arch/arm/kernel/traps.c:727! [ 45.846450] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM [ 45.852275] Modules linked in: test(O+) nvhost_vi [ 45.857012] CPU: 0 PID: 1848 Comm: insmod Tainted: G O 3.10.24-g6a2d13a #1 [ 45.864744] task: ee406580 ti: eb18c000 task.ti: eb18c000 [ 45.870141] PC is at __bad_xchg+0x24/0x28 [ 45.874146] LR is at __bad_xchg+0x24/0x28 > >> arch/arm/include/asm/cmpxchg.h | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/include/asm/cmpxchg.h b/arch/arm/include/asm/cmpxchg.h >> index abb2c37..9505cca 100644 >> --- a/arch/arm/include/asm/cmpxchg.h >> +++ b/arch/arm/include/asm/cmpxchg.h >> @@ -50,6 +50,16 @@ static inline unsigned long __xchg(unsigned long x, volatile void *ptr, int size >> : "r" (x), "r" (ptr) >> : "memory", "cc"); >> break; >> + case 2: >> + asm volatile("@ __xchg2\n" >> + "1: ldrexh %0, [%3]\n" >> + " strexh %1, %2, [%3]\n" >> + " teq %1, #0\n" >> + " bne 1b" >> + : "=&r" (ret), "=&r" (tmp) >> + : "r" (x), "r" (ptr) >> + : "memory", "cc"); >> + break; >> case 4: >> asm volatile("@ __xchg4\n" >> "1: ldrex %0, [%3]\n" > > Does this work on all ARMv6 or just ARMv6k? > ldrexh/strexh is being used in cmpxchg() in the same file in a similar manner, and the comment there says that it works for all ARCH >= ARMv6k, so not ARMv6 I guess.
On Wednesday 25 February 2015 11:11:28 Pranith Kumar wrote: > On Wed, Feb 25, 2015 at 10:58 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wednesday 25 February 2015 10:36:20 Pranith Kumar wrote: > >> This patch adds support for a half-word xchg() for ARM using ldrexh/strexh > >> instructions. It also fixes an asm comment for __cmpxchg2. > >> > >> Currently using a half-word xchg() results in the following splat on an ARMv7 > >> machine. > >> > >> [ 45.833303] xchg: bad data size: pc 0xbe806020, ptr 0xeb18deee, size 2 > >> [ 45.833324] ------------[ cut here ]------------ > >> [ 45.837939] kernel BUG at /dvs/git/dirty/git-master_linux/kernel/arch/arm/kernel/traps.c:727! > >> > >> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > > > > Unfortunately, the BUG message seems incomplete, can you reproduce this > > with CONFIG_DEBUG_BUGVERBOSE enabled? > > The bug here is in a module caused when xchg() was used on uint16_t > variable. It is caused by the __bad_xchg() for 2 byte swap. > > More information: > [ 45.833303] xchg: bad data size: pc 0xbe806020, ptr 0xeb18deee, size 2 > [ 45.833324] ------------[ cut here ]------------ > [ 45.837939] kernel BUG at > /dvs/git/dirty/git-master_linux/kernel/arch/arm/kernel/traps.c:727! > [ 45.846450] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM > [ 45.852275] Modules linked in: test(O+) nvhost_vi > [ 45.857012] CPU: 0 PID: 1848 Comm: insmod Tainted: G O > 3.10.24-g6a2d13a #1 > [ 45.864744] task: ee406580 ti: eb18c000 task.ti: eb18c000 > [ 45.870141] PC is at __bad_xchg+0x24/0x28 > [ 45.874146] LR is at __bad_xchg+0x24/0x28 I'm more interested in the backtrace here, it's possible we should fix the driver instead. > >> arch/arm/include/asm/cmpxchg.h | 18 +++++++++++++++++- > >> 1 file changed, 17 insertions(+), 1 deletion(-) > >> > >> diff --git a/arch/arm/include/asm/cmpxchg.h b/arch/arm/include/asm/cmpxchg.h > >> index abb2c37..9505cca 100644 > >> --- a/arch/arm/include/asm/cmpxchg.h > >> +++ b/arch/arm/include/asm/cmpxchg.h > >> @@ -50,6 +50,16 @@ static inline unsigned long __xchg(unsigned long x, volatile void *ptr, int size > >> : "r" (x), "r" (ptr) > >> : "memory", "cc"); > >> break; > >> + case 2: > >> + asm volatile("@ __xchg2\n" > >> + "1: ldrexh %0, [%3]\n" > >> + " strexh %1, %2, [%3]\n" > >> + " teq %1, #0\n" > >> + " bne 1b" > >> + : "=&r" (ret), "=&r" (tmp) > >> + : "r" (x), "r" (ptr) > >> + : "memory", "cc"); > >> + break; > >> case 4: > >> asm volatile("@ __xchg4\n" > >> "1: ldrex %0, [%3]\n" > > > > Does this work on all ARMv6 or just ARMv6k? > > > > ldrexh/strexh is being used in cmpxchg() in the same file in a similar > manner, and the comment there says that it works for all ARCH >= > ARMv6k, so not ARMv6 I guess. Ok, then you need to put the same check in __xchg too. I'm not sure about the 1-byte case here, because that is already used in ARMv6 __xchg. Arnd
On Wed, Feb 25, 2015 at 04:58:35PM +0100, Arnd Bergmann wrote: > On Wednesday 25 February 2015 10:36:20 Pranith Kumar wrote: > > This patch adds support for a half-word xchg() for ARM using ldrexh/strexh > > instructions. It also fixes an asm comment for __cmpxchg2. > > > > Currently using a half-word xchg() results in the following splat on an ARMv7 > > machine. > > > > [ 45.833303] xchg: bad data size: pc 0xbe806020, ptr 0xeb18deee, size 2 > > [ 45.833324] ------------[ cut here ]------------ > > [ 45.837939] kernel BUG at /dvs/git/dirty/git-master_linux/kernel/arch/arm/kernel/traps.c:727! > > > > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > > Unfortunately, the BUG message seems incomplete, can you reproduce this > with CONFIG_DEBUG_BUGVERBOSE enabled? That isn't because CONFIG_DEBUG_BUGVERBOSE isn't set. It's because the message author decided that the remainder of the kernel bug message wasn't useful to report. After all, it's just a load of hex numbers and symbolic information. Who would want that junk. (Some people really don't get this: we print stuff from the kernel because it gives _us_ useful information to debug problems like this, but because they don't understand it themselves, they decide they can omit it from their bug reports...)
On Wed, Feb 25, 2015 at 05:21:54PM +0100, Arnd Bergmann wrote: > On Wednesday 25 February 2015 11:11:28 Pranith Kumar wrote: > > On Wed, Feb 25, 2015 at 10:58 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > > On Wednesday 25 February 2015 10:36:20 Pranith Kumar wrote: > > >> This patch adds support for a half-word xchg() for ARM using ldrexh/strexh > > >> instructions. It also fixes an asm comment for __cmpxchg2. > > >> > > >> Currently using a half-word xchg() results in the following splat on an ARMv7 > > >> machine. > > >> > > >> [ 45.833303] xchg: bad data size: pc 0xbe806020, ptr 0xeb18deee, size 2 > > >> [ 45.833324] ------------[ cut here ]------------ > > >> [ 45.837939] kernel BUG at /dvs/git/dirty/git-master_linux/kernel/arch/arm/kernel/traps.c:727! > > >> > > >> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > > > > > > Unfortunately, the BUG message seems incomplete, can you reproduce this > > > with CONFIG_DEBUG_BUGVERBOSE enabled? > > > > The bug here is in a module caused when xchg() was used on uint16_t > > variable. It is caused by the __bad_xchg() for 2 byte swap. > > > > More information: > > [ 45.833303] xchg: bad data size: pc 0xbe806020, ptr 0xeb18deee, size 2 > > [ 45.833324] ------------[ cut here ]------------ > > [ 45.837939] kernel BUG at > > /dvs/git/dirty/git-master_linux/kernel/arch/arm/kernel/traps.c:727! > > [ 45.846450] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM > > [ 45.852275] Modules linked in: test(O+) nvhost_vi > > [ 45.857012] CPU: 0 PID: 1848 Comm: insmod Tainted: G O > > 3.10.24-g6a2d13a #1 > > [ 45.864744] task: ee406580 ti: eb18c000 task.ti: eb18c000 > > [ 45.870141] PC is at __bad_xchg+0x24/0x28 > > [ 45.874146] LR is at __bad_xchg+0x24/0x28 > > I'm more interested in the backtrace here, it's possible we should fix the > driver instead. Actually, I think we ought to get rid of __bad_xchg() so that cases like this cause a link error instead of a runtime error, just like we do in other cases as well. That's something that goes back ages (it used to be called something like invalidptr in 2.0 kernels...)
On Wed, Feb 25, 2015 at 11:21 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> >> More information: >> [ 45.833303] xchg: bad data size: pc 0xbe806020, ptr 0xeb18deee, size 2 >> [ 45.833324] ------------[ cut here ]------------ >> [ 45.837939] kernel BUG at >> /dvs/git/dirty/git-master_linux/kernel/arch/arm/kernel/traps.c:727! >> [ 45.846450] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM >> [ 45.852275] Modules linked in: test(O+) nvhost_vi >> [ 45.857012] CPU: 0 PID: 1848 Comm: insmod Tainted: G O >> 3.10.24-g6a2d13a #1 >> [ 45.864744] task: ee406580 ti: eb18c000 task.ti: eb18c000 >> [ 45.870141] PC is at __bad_xchg+0x24/0x28 >> [ 45.874146] LR is at __bad_xchg+0x24/0x28 > > I'm more interested in the backtrace here, it's possible we should fix the > driver instead. I should have been more clearer. I apologize. This is in a test module I wrote trying out the various synchronization primitives. This is not a problem in any current driver I am using. > >> >> arch/arm/include/asm/cmpxchg.h | 18 +++++++++++++++++- >> >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/arch/arm/include/asm/cmpxchg.h b/arch/arm/include/asm/cmpxchg.h >> >> index abb2c37..9505cca 100644 >> >> --- a/arch/arm/include/asm/cmpxchg.h >> >> +++ b/arch/arm/include/asm/cmpxchg.h >> >> @@ -50,6 +50,16 @@ static inline unsigned long __xchg(unsigned long x, volatile void *ptr, int size >> >> : "r" (x), "r" (ptr) >> >> : "memory", "cc"); >> >> break; >> >> + case 2: >> >> + asm volatile("@ __xchg2\n" >> >> + "1: ldrexh %0, [%3]\n" >> >> + " strexh %1, %2, [%3]\n" >> >> + " teq %1, #0\n" >> >> + " bne 1b" >> >> + : "=&r" (ret), "=&r" (tmp) >> >> + : "r" (x), "r" (ptr) >> >> + : "memory", "cc"); >> >> + break; >> >> case 4: >> >> asm volatile("@ __xchg4\n" >> >> "1: ldrex %0, [%3]\n" >> > >> > Does this work on all ARMv6 or just ARMv6k? >> > >> >> ldrexh/strexh is being used in cmpxchg() in the same file in a similar >> manner, and the comment there says that it works for all ARCH >= >> ARMv6k, so not ARMv6 I guess. > > Ok, then you need to put the same check in __xchg too. I'm not sure > about the 1-byte case here, because that is already used in ARMv6 > __xchg. > OK, I will update the patch with the check. Looking closely I see what you are saying. In __xchg(), for the 1 byte case we are using ldrexb/strexb while the test checks for version >= 6. Documentation online says that ldrex{h/b} are supported only for ARMv6k or greater. So I think the check in __xchg() should be changed to one similar in cmpxchg(). Is that what you meant? If so, i will send in an updated patch. Thanks!
On Wed, Feb 25, 2015 at 12:02 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: >> Unfortunately, the BUG message seems incomplete, can you reproduce this >> with CONFIG_DEBUG_BUGVERBOSE enabled? > > That isn't because CONFIG_DEBUG_BUGVERBOSE isn't set. It's because > the message author decided that the remainder of the kernel bug message > wasn't useful to report. After all, it's just a load of hex numbers > and symbolic information. Who would want that junk. > > (Some people really don't get this: we print stuff from the kernel > because it gives _us_ useful information to debug problems like this, > but because they don't understand it themselves, they decide they can > omit it from their bug reports...) > I apologize for not pasting the entire back trace. It was in a test module I wrote testing synchronization primitives and did not think the back trace would be useful to report. I will include verbose details from now on. Thanks!
On Wed, Feb 25, 2015 at 12:14 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > Actually, I think we ought to get rid of __bad_xchg() so that cases > like this cause a link error instead of a runtime error, just like we > do in other cases as well. > > That's something that goes back ages (it used to be called something > like invalidptr in 2.0 kernels...) That sounds like a good idea. I will cook up a patch doing this. Thanks!
On Wed, Feb 25, 2015 at 12:34:35PM -0500, Pranith Kumar wrote: > On Wed, Feb 25, 2015 at 12:14 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > Actually, I think we ought to get rid of __bad_xchg() so that cases > > like this cause a link error instead of a runtime error, just like we > > do in other cases as well. > > > > That's something that goes back ages (it used to be called something > > like invalidptr in 2.0 kernels...) > > That sounds like a good idea. I will cook up a patch doing this. I've just removed it on my build system's tree, and we'll see tomorrow morning how it fairs. If it spits out a lot of errors, we won't be able to remove it.
diff --git a/arch/arm/include/asm/cmpxchg.h b/arch/arm/include/asm/cmpxchg.h index abb2c37..9505cca 100644 --- a/arch/arm/include/asm/cmpxchg.h +++ b/arch/arm/include/asm/cmpxchg.h @@ -50,6 +50,16 @@ static inline unsigned long __xchg(unsigned long x, volatile void *ptr, int size : "r" (x), "r" (ptr) : "memory", "cc"); break; + case 2: + asm volatile("@ __xchg2\n" + "1: ldrexh %0, [%3]\n" + " strexh %1, %2, [%3]\n" + " teq %1, #0\n" + " bne 1b" + : "=&r" (ret), "=&r" (tmp) + : "r" (x), "r" (ptr) + : "memory", "cc"); + break; case 4: asm volatile("@ __xchg4\n" "1: ldrex %0, [%3]\n" @@ -93,6 +103,12 @@ static inline unsigned long __xchg(unsigned long x, volatile void *ptr, int size : "memory", "cc"); break; #endif + case 2: + raw_local_irq_save(flags); + ret = *(volatile uint16_t *)ptr; + *(volatile uint16_t *)ptr = x; + raw_local_irq_restore(flags); + break; default: __bad_xchg(ptr, size), ret = 0; break; @@ -158,7 +174,7 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old, break; case 2: do { - asm volatile("@ __cmpxchg1\n" + asm volatile("@ __cmpxchg2\n" " ldrexh %1, [%2]\n" " mov %0, #0\n" " teq %1, %3\n"
This patch adds support for a half-word xchg() for ARM using ldrexh/strexh instructions. It also fixes an asm comment for __cmpxchg2. Currently using a half-word xchg() results in the following splat on an ARMv7 machine. [ 45.833303] xchg: bad data size: pc 0xbe806020, ptr 0xeb18deee, size 2 [ 45.833324] ------------[ cut here ]------------ [ 45.837939] kernel BUG at /dvs/git/dirty/git-master_linux/kernel/arch/arm/kernel/traps.c:727! Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> --- arch/arm/include/asm/cmpxchg.h | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)