[RFC PATCH] arm: asm/cmpxchg.h: Add support half-word xchg()

[RFC PATCH] arm: asm/cmpxchg.h: Add support half-word xchg()

[Pranith Kumar]

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(-)

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

[RFC PATCH] arm: asm/cmpxchg.h: Add support half-word xchg()

[Arnd Bergmann]

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

[RFC PATCH] arm: asm/cmpxchg.h: Add support half-word xchg()

[Pranith Kumar]

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.

-- 
Pranith

[RFC PATCH] arm: asm/cmpxchg.h: Add support half-word xchg()

[Arnd Bergmann]

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

[RFC PATCH] arm: asm/cmpxchg.h: Add support half-word xchg()

[Russell King - ARM Linux]

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...)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[RFC PATCH] arm: asm/cmpxchg.h: Add support half-word xchg()

[Russell King - ARM Linux]

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...)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[RFC PATCH] arm: asm/cmpxchg.h: Add support half-word xchg()

[Pranith Kumar]

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!
-- 
Pranith

[RFC PATCH] arm: asm/cmpxchg.h: Add support half-word xchg()

[Pranith Kumar]

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!
-- 
Pranith

[RFC PATCH] arm: asm/cmpxchg.h: Add support half-word xchg()

[Pranith Kumar]

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!
-- 
Pranith

[RFC PATCH] arm: asm/cmpxchg.h: Add support half-word xchg()

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.