LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] x86: fix inline assembly constraints
@ 2008-10-29 16:32 Jike Song
  2008-10-29 16:32 ` Jike Song
  2008-10-29 16:34 ` Ingo Molnar
  0 siblings, 2 replies; 10+ messages in thread
From: Jike Song @ 2008-10-29 16:32 UTC (permalink / raw)
  To: tglx, mingo, hpa; +Cc: linux-kernel



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] x86: fix inline assembly constraints
  2008-10-29 16:32 [PATCH] x86: fix inline assembly constraints Jike Song
@ 2008-10-29 16:32 ` Jike Song
  2008-10-29 16:34 ` Ingo Molnar
  1 sibling, 0 replies; 10+ messages in thread
From: Jike Song @ 2008-10-29 16:32 UTC (permalink / raw)
  To: tglx, mingo, hpa; +Cc: linux-kernel, Jike Song

In atomic_set_mask, *addr should be both read and written.

Signed-off-by: Jike Song <albcamus@gmail.com>
---
 arch/x86/include/asm/atomic_32.h |    8 ++++----
 arch/x86/include/asm/atomic_64.h |    7 ++++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/atomic_32.h b/arch/x86/include/asm/atomic_32.h
index ad5b9f6..23a7c7f 100644
--- a/arch/x86/include/asm/atomic_32.h
+++ b/arch/x86/include/asm/atomic_32.h
@@ -242,12 +242,12 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
 
 /* These are x86-specific, used by some header files */
 #define atomic_clear_mask(mask, addr)				\
-	asm volatile(LOCK_PREFIX "andl %0,%1"			\
-		     : : "r" (~(mask)), "m" (*(addr)) : "memory")
+	asm volatile(LOCK_PREFIX "andl %1, %0"			\
+		     : "+m" (*(addr)) : "r" (~(mask)) : "memory")
 
 #define atomic_set_mask(mask, addr)				\
-	asm volatile(LOCK_PREFIX "orl %0,%1"				\
-		     : : "r" (mask), "m" (*(addr)) : "memory")
+	asm volatile(LOCK_PREFIX "orl %1, %0"			\
+		     : "+m" (*(addr)) : "r" (mask) : "memory")
 
 /* Atomic operations are already serializing on x86 */
 #define smp_mb__before_atomic_dec()	barrier()
diff --git a/arch/x86/include/asm/atomic_64.h b/arch/x86/include/asm/atomic_64.h
index fa59212..31b34f3 100644
--- a/arch/x86/include/asm/atomic_64.h
+++ b/arch/x86/include/asm/atomic_64.h
@@ -451,12 +451,13 @@ static inline void atomic_or_long(unsigned long *v1, unsigned long v2)
 
 /* These are x86-specific, used by some header files */
 #define atomic_clear_mask(mask, addr)					\
-	asm volatile(LOCK_PREFIX "andl %0,%1"				\
-		     : : "r" (~(mask)), "m" (*(addr)) : "memory")
+	asm volatile(LOCK_PREFIX "andl %1, %0"				\
+		     : "+m" (*(addr)) : "r" (~(mask)) : "memory")
 
 #define atomic_set_mask(mask, addr)					\
-	asm volatile(LOCK_PREFIX "orl %0,%1"				\
+	asm volatile(LOCK_PREFIX "orl %1, %0"				\
 		     : : "r" ((unsigned)(mask)), "m" (*(addr))		\
+		     : "+m" (*(addr)) : "r" ((unsigned)(mask))		\
 		     : "memory")
 
 /* Atomic operations are already serializing on x86 */
-- 
1.6.0.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: fix inline assembly constraints
  2008-10-29 16:32 [PATCH] x86: fix inline assembly constraints Jike Song
  2008-10-29 16:32 ` Jike Song
@ 2008-10-29 16:34 ` Ingo Molnar
  2008-10-30  2:31   ` Jike Song
  1 sibling, 1 reply; 10+ messages in thread
From: Ingo Molnar @ 2008-10-29 16:34 UTC (permalink / raw)
  To: Jike Song; +Cc: tglx, mingo, hpa, linux-kernel


* Jike Song <albcamus@gmail.com> wrote:

> 
> --

that was an easy patch to act upon ;-)

	Ingo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: fix inline assembly constraints
  2008-10-29 16:34 ` Ingo Molnar
@ 2008-10-30  2:31   ` Jike Song
  2008-10-30  3:54     ` H. Peter Anvin
  2008-10-30 19:29     ` Ingo Molnar
  0 siblings, 2 replies; 10+ messages in thread
From: Jike Song @ 2008-10-30  2:31 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: tglx, mingo, hpa, linux-kernel

On Thu, Oct 30, 2008 at 12:34 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Jike Song <albcamus@gmail.com> wrote:
>>
>> --
>
> that was an easy patch to act upon ;-)
>
>        Ingo
>
Sorry Ingo, I missed your point. Do you mean this patch is trivial or
unnecessary?

Besides, by looking at the inline assembly in kernel,  I found lots of
codes like this:

        static inline void atomic_add(int i, atomic_t *v)
        {
	        asm volatile(LOCK_PREFIX "addl %1,%0"
		             : "=m" (v->counter)
		             : "ir" (i), "m" (v->counter));
        }


Yes, it works. But a little ugly..  Should this be cleaned-up with the
following?

        : "+m" (v->counter)
        : "ir" (i)

If you agrees, I'll send out the patch; otherwise I won't wasting your time ;-)


-- 
Thanks,
Jike

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: fix inline assembly constraints
  2008-10-30  2:31   ` Jike Song
@ 2008-10-30  3:54     ` H. Peter Anvin
  2008-10-30  6:40       ` Jike Song
  2008-10-30 19:29     ` Ingo Molnar
  1 sibling, 1 reply; 10+ messages in thread
From: H. Peter Anvin @ 2008-10-30  3:54 UTC (permalink / raw)
  To: Jike Song; +Cc: Ingo Molnar, tglx, mingo, linux-kernel

Jike Song wrote:
> 
> Besides, by looking at the inline assembly in kernel,  I found lots of
> codes like this:
> 
>         static inline void atomic_add(int i, atomic_t *v)
>         {
> 	        asm volatile(LOCK_PREFIX "addl %1,%0"
> 		             : "=m" (v->counter)
> 		             : "ir" (i), "m" (v->counter));
>         }
> 
> 
> Yes, it works. But a little ugly..  Should this be cleaned-up with the
> following?
> 
>         : "+m" (v->counter)
>         : "ir" (i)
> 
> If you agrees, I'll send out the patch; otherwise I won't wasting your time ;-)
> 

Please don't change them just to change them, if there is no actual
error.  You never know when you're going to trigger a new bug in some
weird version of gcc.

	-hpa

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: fix inline assembly constraints
  2008-10-30  3:54     ` H. Peter Anvin
@ 2008-10-30  6:40       ` Jike Song
  2008-10-30 14:51         ` H. Peter Anvin
  0 siblings, 1 reply; 10+ messages in thread
From: Jike Song @ 2008-10-30  6:40 UTC (permalink / raw)
  To: H. Peter Anvin; +Cc: Ingo Molnar, tglx, mingo, linux-kernel

On Thu, Oct 30, 2008 at 11:54 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> Jike Song wrote:
>>
>> Besides, by looking at the inline assembly in kernel,  I found lots of
>> codes like this:
>>
>>         static inline void atomic_add(int i, atomic_t *v)
>>         {
>>               asm volatile(LOCK_PREFIX "addl %1,%0"
>>                            : "=m" (v->counter)
>>                            : "ir" (i), "m" (v->counter));
>>         }
>>
>>
>> Yes, it works. But a little ugly..  Should this be cleaned-up with the
>> following?
>>
>>         : "+m" (v->counter)
>>         : "ir" (i)
>>
>> If you agrees, I'll send out the patch; otherwise I won't wasting your time ;-)
>>
>
> Please don't change them just to change them, if there is no actual
> error.  You never know when you're going to trigger a new bug in some
> weird version of gcc.
>
>        -hpa

Yes, sometimes gcc did have bugs with its obscure inline asm
conventions. But I think the change of x86-64 atomic operations should
be OK. Anyway, the "+" constraint is more clear than a "=m" output and
 a "m" input.

The 32-bit atomic ops were already changed to "+m".(commit
b862f3b099f3ea672c7438c0b282ce8201d39dfc)

-- 
Thanks,
Jike

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: fix inline assembly constraints
  2008-10-30  6:40       ` Jike Song
@ 2008-10-30 14:51         ` H. Peter Anvin
  0 siblings, 0 replies; 10+ messages in thread
From: H. Peter Anvin @ 2008-10-30 14:51 UTC (permalink / raw)
  To: Jike Song; +Cc: Ingo Molnar, tglx, mingo, linux-kernel

Jike Song wrote:
> 
> Yes, sometimes gcc did have bugs with its obscure inline asm
> conventions. But I think the change of x86-64 atomic operations should
> be OK. Anyway, the "+" constraint is more clear than a "=m" output and
>  a "m" input.
> 
> The 32-bit atomic ops were already changed to "+m".(commit
> b862f3b099f3ea672c7438c0b282ce8201d39dfc)
> 

You *THINK*.  It's very easy to *THINK* that gcc won't do something
utterly moronic, and you'd be wrong.

Just changing it for the sake of churn is pointless... if there is a
bug, then we have to take the risk anyway, but if it is already correct,
then there is no point in provoking a bug.  Not *your* bug, because your
code is correct, but gcc's bug.

FWIW, the reason that code doesn't use "+m" is because a version of gcc
which we no longer support didn't handle it.  That by itself isn't a
reason to keep it, but there is also no reason to just "tidy" it, IMNSHO.

	-hpa

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: fix inline assembly constraints
  2008-10-30  2:31   ` Jike Song
  2008-10-30  3:54     ` H. Peter Anvin
@ 2008-10-30 19:29     ` Ingo Molnar
  2008-10-31  2:12       ` Jike Song
  1 sibling, 1 reply; 10+ messages in thread
From: Ingo Molnar @ 2008-10-30 19:29 UTC (permalink / raw)
  To: Jike Song; +Cc: tglx, mingo, hpa, linux-kernel


* Jike Song <albcamus@gmail.com> wrote:

> On Thu, Oct 30, 2008 at 12:34 AM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Jike Song <albcamus@gmail.com> wrote:
> >>
> >> --
> >
> > that was an easy patch to act upon ;-)
> >
> >        Ingo
> >
> Sorry Ingo, I missed your point. Do you mean this patch is trivial or
> unnecessary?

nah, it was a joke - you sent two mails, one of them was empty ;-)

	Ingo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: fix inline assembly constraints
  2008-10-30 19:29     ` Ingo Molnar
@ 2008-10-31  2:12       ` Jike Song
  0 siblings, 0 replies; 10+ messages in thread
From: Jike Song @ 2008-10-31  2:12 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: tglx, mingo, hpa, linux-kernel

Ingo Molnar wrote:
>
>  nah, it was a joke - you sent two mails, one of them was empty ;-)
>
>  Ingo

Yeah, I had problems with git-send-email, it always send two emails(the
first being empty) if I specify --compose ;-) Will take care next time;)

Jike

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: fix inline assembly constraints
       [not found]   ` <fa.rs0w7okAn1If1ilwD81OTzT4rKg@ifi.uio.no>
@ 2008-10-30  3:52     ` Robert Hancock
  0 siblings, 0 replies; 10+ messages in thread
From: Robert Hancock @ 2008-10-30  3:52 UTC (permalink / raw)
  To: Jike Song; +Cc: Ingo Molnar, mingo, linux-kernel

Jike Song wrote:
> On Thu, Oct 30, 2008 at 12:34 AM, Ingo Molnar <mingo@elte.hu> wrote:
>> * Jike Song <albcamus@gmail.com> wrote:
>>> --
>> that was an easy patch to act upon ;-)
>>
>>        Ingo
>>
> Sorry Ingo, I missed your point. Do you mean this patch is trivial or
> unnecessary?

Your first message was empty.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2008-10-31  2:15 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-29 16:32 [PATCH] x86: fix inline assembly constraints Jike Song
2008-10-29 16:32 ` Jike Song
2008-10-29 16:34 ` Ingo Molnar
2008-10-30  2:31   ` Jike Song
2008-10-30  3:54     ` H. Peter Anvin
2008-10-30  6:40       ` Jike Song
2008-10-30 14:51         ` H. Peter Anvin
2008-10-30 19:29     ` Ingo Molnar
2008-10-31  2:12       ` Jike Song
     [not found] <fa.IZJ3RdF6xWbbMqWVWyYRstzZBsk@ifi.uio.no>
     [not found] ` <fa.L4yER0Vo1brqQzpHIySgWqUN5UM@ifi.uio.no>
     [not found]   ` <fa.rs0w7okAn1If1ilwD81OTzT4rKg@ifi.uio.no>
2008-10-30  3:52     ` Robert Hancock

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).