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 <[email protected]>
---
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"
@@ -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"
--
1.9.1
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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>
> 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.
--
Pranith
On Wednesday 25 February 2015 11:11:28 Pranith Kumar wrote:
> On Wed, Feb 25, 2015 at 10:58 AM, Arnd Bergmann <[email protected]> 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 <[email protected]>
> >
> > 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 <[email protected]>
>
> 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...)
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
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 <[email protected]> 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 <[email protected]>
> > >
> > > 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...)
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
On Wed, Feb 25, 2015 at 11:21 AM, Arnd Bergmann <[email protected]> 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!
--
Pranith
On Wed, Feb 25, 2015 at 12:02 PM, Russell King - ARM Linux
<[email protected]> 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!
--
Pranith
On Wed, Feb 25, 2015 at 12:14 PM, Russell King - ARM Linux
<[email protected]> 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!
--
Pranith
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
> <[email protected]> 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.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.