[PATCH] ARM: include: asm: atomic.h: use type cast 's64' for the return value of atomic64_add_return().

[PATCH] ARM: include: asm: atomic.h: use type cast 's64' for the return value of atomic64_add_return().

[Chen Gang]

The return value of atomic64_add_return() is u64 which never less than
zero, so need type cast 's64' for comparing in atomic64_add_negative().

The related error: (allmodconfig for S5PV210, with "EXTRA_CFLAGS=-W"):

  kernel/events/core.c:5404:2: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/arm/include/asm/atomic.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index da1c77d..8cf005d 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -475,7 +475,7 @@ static inline int atomic64_add_unless(atomic64_t *v, u64 a, u64 u)
 	return ret;
 }
 
-#define atomic64_add_negative(a, v)	(atomic64_add_return((a), (v)) < 0)
+#define atomic64_add_negative(a, v)	((s64)atomic64_add_return((a), (v)) < 0)
 #define atomic64_inc(v)			atomic64_add(1LL, (v))
 #define atomic64_inc_return(v)		atomic64_add_return(1LL, (v))
 #define atomic64_inc_and_test(v)	(atomic64_inc_return(v) == 0)
-- 
1.7.7.6

[PATCH] ARM: include: asm: atomic.h: use type cast 's64' for the return value of atomic64_add_return().

[Will Deacon]

Hello,

On Sat, Sep 21, 2013 at 12:06:47PM +0100, Chen Gang wrote:
> The return value of atomic64_add_return() is u64 which never less than
> zero, so need type cast 's64' for comparing in atomic64_add_negative().
> 
> The related error: (allmodconfig for S5PV210, with "EXTRA_CFLAGS=-W"):
> 
>   kernel/events/core.c:5404:2: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  arch/arm/include/asm/atomic.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
> index da1c77d..8cf005d 100644
> --- a/arch/arm/include/asm/atomic.h
> +++ b/arch/arm/include/asm/atomic.h
> @@ -475,7 +475,7 @@ static inline int atomic64_add_unless(atomic64_t *v, u64 a, u64 u)
>  	return ret;
>  }
>  
> -#define atomic64_add_negative(a, v)	(atomic64_add_return((a), (v)) < 0)
> +#define atomic64_add_negative(a, v)	((s64)atomic64_add_return((a), (v)) < 0)
>  #define atomic64_inc(v)			atomic64_add(1LL, (v))
>  #define atomic64_inc_return(v)		atomic64_add_return(1LL, (v))
>  #define atomic64_inc_and_test(v)	(atomic64_inc_return(v) == 0)

Is this the right fix? It looks more like atomic[64]_t should be signed, but
some 32-bit architectures (ARM, x86, tile) are actually implementing
atomic64_t as u64. Furthermore, there are discrepencies in the operands to
the various atomic64_* function (long long vs u64) which probably need
sorting out.

Since the 32-bit interface is well defined (Documentation/atomic_ops.txt),
I think we should just follow the same signedness rules for the 64-bit
versions.

Will

[PATCH] ARM: include: asm: atomic.h: use type cast 's64' for the return value of atomic64_add_return().

[Russell King - ARM Linux]

On Tue, Sep 24, 2013 at 10:30:41AM +0100, Will Deacon wrote:
> On Sat, Sep 21, 2013 at 12:06:47PM +0100, Chen Gang wrote:
> > diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
> > index da1c77d..8cf005d 100644
> > --- a/arch/arm/include/asm/atomic.h
> > +++ b/arch/arm/include/asm/atomic.h
> > @@ -475,7 +475,7 @@ static inline int atomic64_add_unless(atomic64_t *v, u64 a, u64 u)
> >  	return ret;
> >  }
> >  
> > -#define atomic64_add_negative(a, v)	(atomic64_add_return((a), (v)) < 0)
> > +#define atomic64_add_negative(a, v)	((s64)atomic64_add_return((a), (v)) < 0)
> >  #define atomic64_inc(v)			atomic64_add(1LL, (v))
> >  #define atomic64_inc_return(v)		atomic64_add_return(1LL, (v))
> >  #define atomic64_inc_and_test(v)	(atomic64_inc_return(v) == 0)
> 
> Is this the right fix? It looks more like atomic[64]_t should be signed, but
> some 32-bit architectures (ARM, x86, tile) are actually implementing
> atomic64_t as u64. Furthermore, there are discrepencies in the operands to
> the various atomic64_* function (long long vs u64) which probably need
> sorting out.

Even though our underlying type is u64, we could change the arguments to
be 'long long' as per the asm-generic/atomic64.h version.  Remember that
this is ARM specific code, and we know that long long == 64-bit int.

[PATCH] ARM: include: asm: atomic.h: use type cast 's64' for the return value of atomic64_add_return().

[Chen Gang]

On 09/24/2013 05:30 PM, Will Deacon wrote:
> Hello,
> 
> On Sat, Sep 21, 2013 at 12:06:47PM +0100, Chen Gang wrote:
>> The return value of atomic64_add_return() is u64 which never less than
>> zero, so need type cast 's64' for comparing in atomic64_add_negative().
>>
>> The related error: (allmodconfig for S5PV210, with "EXTRA_CFLAGS=-W"):
>>
>>   kernel/events/core.c:5404:2: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  arch/arm/include/asm/atomic.h |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
>> index da1c77d..8cf005d 100644
>> --- a/arch/arm/include/asm/atomic.h
>> +++ b/arch/arm/include/asm/atomic.h
>> @@ -475,7 +475,7 @@ static inline int atomic64_add_unless(atomic64_t *v, u64 a, u64 u)
>>  	return ret;
>>  }
>>  
>> -#define atomic64_add_negative(a, v)	(atomic64_add_return((a), (v)) < 0)
>> +#define atomic64_add_negative(a, v)	((s64)atomic64_add_return((a), (v)) < 0)
>>  #define atomic64_inc(v)			atomic64_add(1LL, (v))
>>  #define atomic64_inc_return(v)		atomic64_add_return(1LL, (v))
>>  #define atomic64_inc_and_test(v)	(atomic64_inc_return(v) == 0)
> 
> Is this the right fix? It looks more like atomic[64]_t should be signed, but
> some 32-bit architectures (ARM, x86, tile) are actually implementing
> atomic64_t as u64. Furthermore, there are discrepencies in the operands to
> the various atomic64_* function (long long vs u64) which probably need
> sorting out.
> 
> Since the 32-bit interface is well defined (Documentation/atomic_ops.txt),
> I think we should just follow the same signedness rules for the 64-bit
> versions.
> 

That sounds reasonable, excluding arm and tile_32, all are defined as
signed (even for related 32-bits functions, arm and tile are also use
signed).

For atomic64_add_negative(), like arm, tile also has the same issue
(unsigned type is never less than zero).

Tomorrow, I will send patch v2 for arm, and another fix patch for tile.
:-)

Thanks.


The related search:

  arm/include/asm/atomic.h:314:static inline u64 atomic64_add_return(u64 i, atomic64_t *v) 
  tile/include/asm/atomic_32.h:123:static inline u64 atomic64_add_return(u64 i, atomic64_t *v) 

  ia64/include/asm/atomic.h:137:#define atomic64_add_return(i,v)                                  \   

  alpha/include/asm/atomic.h:115:static __inline__ long atomic64_add_return(long i, atomic64_t * v)
  arm64/include/asm/atomic.h:194:static inline long atomic64_add_return(long i, atomic64_t *v) 
  frv/include/asm/atomic.h:152:extern long long atomic64_add_return(long long i, atomic64_t *v);
  mips/include/asm/atomic.h:499:static __inline__ long atomic64_add_return(long i, atomic64_t * v)
  parisc/include/asm/atomic.h:156:__atomic64_add_return(s64 i, atomic64_t *v) 
  parisc/include/asm/atomic.h:190:#define atomic64_add_return(i,v)        (__atomic64_add_return( ((s64)(i)),(v)))
  powerpc/include/asm/atomic.h:310:static __inline__ long atomic64_add_return(long a, atomic64_t *v) 
  s390/include/asm/atomic.h:199:static inline long long atomic64_add_return(long long i, atomic64_t *v) 
  s390/include/asm/atomic.h:284:static inline long long atomic64_add_return(long long i, atomic64_t *v) 
  sparc/include/asm/atomic_64.h:42:#define atomic64_add_return(i, v) atomic64_add_ret(i, v)
  tile/include/asm/atomic_64.h:73:static inline long atomic64_add_return(long i, atomic64_t *v) 
  x86/include/asm/atomic64_32.h:134:static inline long long atomic64_add_return(long long i, atomic64_t *v) 
  x86/include/asm/atomic64_64.h:171:static inline long atomic64_add_return(long i, atomic64_t *v) 


> Will
> 
> 

Thanks
-- 
Chen Gang

[PATCH] ARM: include: asm: atomic.h: use type cast 's64' for the return value of atomic64_add_return().

[Chen Gang]

On 09/24/2013 06:27 PM, Russell King - ARM Linux wrote:
> On Tue, Sep 24, 2013 at 10:30:41AM +0100, Will Deacon wrote:
>> On Sat, Sep 21, 2013 at 12:06:47PM +0100, Chen Gang wrote:
>>> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
>>> index da1c77d..8cf005d 100644
>>> --- a/arch/arm/include/asm/atomic.h
>>> +++ b/arch/arm/include/asm/atomic.h
>>> @@ -475,7 +475,7 @@ static inline int atomic64_add_unless(atomic64_t *v, u64 a, u64 u)
>>>  	return ret;
>>>  }
>>>  
>>> -#define atomic64_add_negative(a, v)	(atomic64_add_return((a), (v)) < 0)
>>> +#define atomic64_add_negative(a, v)	((s64)atomic64_add_return((a), (v)) < 0)
>>>  #define atomic64_inc(v)			atomic64_add(1LL, (v))
>>>  #define atomic64_inc_return(v)		atomic64_add_return(1LL, (v))
>>>  #define atomic64_inc_and_test(v)	(atomic64_inc_return(v) == 0)
>>
>> Is this the right fix? It looks more like atomic[64]_t should be signed, but
>> some 32-bit architectures (ARM, x86, tile) are actually implementing
>> atomic64_t as u64. Furthermore, there are discrepencies in the operands to
>> the various atomic64_* function (long long vs u64) which probably need
>> sorting out.
> 
> Even though our underlying type is u64, we could change the arguments to
> be 'long long' as per the asm-generic/atomic64.h version.  Remember that
> this is ARM specific code, and we know that long long == 64-bit int.
> 
> 

That sounds reasonable, too, I will change them in patch v2 for arm
(also for tile).

Thanks.
-- 
Chen Gang

[PATCH v2] ARM: include: asm: use 'long long' instead of 'u64' within atomic.h

[Chen Gang]

atomic* value is signed value, and atomic* functions need also process
signed value (parameter value, and return value), so 32-bit arm need
use 'long long' instead of 'u64'.

After replacement, it will also fix a bug for atomic64_add_negative():
"u64 is never less than 0".

The modifications are:

  in vim, use "1,% s/\<u64\>/long long/g" command.
  remove '__aligned(8)' which is useless for 64-bit.
  be sure of 80 column limitation after replacement.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/arm/include/asm/atomic.h |   49 +++++++++++++++++++++--------------------
 1 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index da1c77d..a715ac0 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -238,15 +238,15 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u)
 
 #ifndef CONFIG_GENERIC_ATOMIC64
 typedef struct {
-	u64 __aligned(8) counter;
+	long long counter;
 } atomic64_t;
 
 #define ATOMIC64_INIT(i) { (i) }
 
 #ifdef CONFIG_ARM_LPAE
-static inline u64 atomic64_read(const atomic64_t *v)
+static inline long long atomic64_read(const atomic64_t *v)
 {
-	u64 result;
+	long long result;
 
 	__asm__ __volatile__("@ atomic64_read\n"
 "	ldrd	%0, %H0, [%1]"
@@ -257,7 +257,7 @@ static inline u64 atomic64_read(const atomic64_t *v)
 	return result;
 }
 
-static inline void atomic64_set(atomic64_t *v, u64 i)
+static inline void atomic64_set(atomic64_t *v, long long i)
 {
 	__asm__ __volatile__("@ atomic64_set\n"
 "	strd	%2, %H2, [%1]"
@@ -266,9 +266,9 @@ static inline void atomic64_set(atomic64_t *v, u64 i)
 	);
 }
 #else
-static inline u64 atomic64_read(const atomic64_t *v)
+static inline long long atomic64_read(const atomic64_t *v)
 {
-	u64 result;
+	long long result;
 
 	__asm__ __volatile__("@ atomic64_read\n"
 "	ldrexd	%0, %H0, [%1]"
@@ -279,9 +279,9 @@ static inline u64 atomic64_read(const atomic64_t *v)
 	return result;
 }
 
-static inline void atomic64_set(atomic64_t *v, u64 i)
+static inline void atomic64_set(atomic64_t *v, long long i)
 {
-	u64 tmp;
+	long long tmp;
 
 	__asm__ __volatile__("@ atomic64_set\n"
 "1:	ldrexd	%0, %H0, [%2]\n"
@@ -294,9 +294,9 @@ static inline void atomic64_set(atomic64_t *v, u64 i)
 }
 #endif
 
-static inline void atomic64_add(u64 i, atomic64_t *v)
+static inline void atomic64_add(long long i, atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	__asm__ __volatile__("@ atomic64_add\n"
@@ -311,9 +311,9 @@ static inline void atomic64_add(u64 i, atomic64_t *v)
 	: "cc");
 }
 
-static inline u64 atomic64_add_return(u64 i, atomic64_t *v)
+static inline long long atomic64_add_return(long long i, atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	smp_mb();
@@ -334,9 +334,9 @@ static inline u64 atomic64_add_return(u64 i, atomic64_t *v)
 	return result;
 }
 
-static inline void atomic64_sub(u64 i, atomic64_t *v)
+static inline void atomic64_sub(long long i, atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	__asm__ __volatile__("@ atomic64_sub\n"
@@ -351,9 +351,9 @@ static inline void atomic64_sub(u64 i, atomic64_t *v)
 	: "cc");
 }
 
-static inline u64 atomic64_sub_return(u64 i, atomic64_t *v)
+static inline long long atomic64_sub_return(long long i, atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	smp_mb();
@@ -374,9 +374,10 @@ static inline u64 atomic64_sub_return(u64 i, atomic64_t *v)
 	return result;
 }
 
-static inline u64 atomic64_cmpxchg(atomic64_t *ptr, u64 old, u64 new)
+static inline long long atomic64_cmpxchg(atomic64_t *ptr, long long old,
+					long long new)
 {
-	u64 oldval;
+	long long oldval;
 	unsigned long res;
 
 	smp_mb();
@@ -398,9 +399,9 @@ static inline u64 atomic64_cmpxchg(atomic64_t *ptr, u64 old, u64 new)
 	return oldval;
 }
 
-static inline u64 atomic64_xchg(atomic64_t *ptr, u64 new)
+static inline long long atomic64_xchg(atomic64_t *ptr, long long new)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	smp_mb();
@@ -419,9 +420,9 @@ static inline u64 atomic64_xchg(atomic64_t *ptr, u64 new)
 	return result;
 }
 
-static inline u64 atomic64_dec_if_positive(atomic64_t *v)
+static inline long long atomic64_dec_if_positive(atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	smp_mb();
@@ -445,9 +446,9 @@ static inline u64 atomic64_dec_if_positive(atomic64_t *v)
 	return result;
 }
 
-static inline int atomic64_add_unless(atomic64_t *v, u64 a, u64 u)
+static inline int atomic64_add_unless(atomic64_t *v, long long a, long long u)
 {
-	u64 val;
+	long long val;
 	unsigned long tmp;
 	int ret = 1;
 
-- 
1.7.7.6

[PATCH v2] ARM: include: asm: use 'long long' instead of 'u64' within atomic.h

[Will Deacon]

On Wed, Sep 25, 2013 at 03:25:19AM +0100, Chen Gang wrote:
> atomic* value is signed value, and atomic* functions need also process
> signed value (parameter value, and return value), so 32-bit arm need
> use 'long long' instead of 'u64'.
> 
> After replacement, it will also fix a bug for atomic64_add_negative():
> "u64 is never less than 0".
> 
> The modifications are:
> 
>   in vim, use "1,% s/\<u64\>/long long/g" command.
>   remove '__aligned(8)' which is useless for 64-bit.
>   be sure of 80 column limitation after replacement.
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>

Looks better to me, thanks. While you're here, we could also replace the use
of `unsigned long' with `int' for the 32-bit atomics, then the whole header
is consistent with the generic types.

Will

[PATCH v2] ARM: include: asm: use 'long long' instead of 'u64' within atomic.h

[Chen Gang]

On 09/26/2013 12:07 AM, Will Deacon wrote:
> On Wed, Sep 25, 2013 at 03:25:19AM +0100, Chen Gang wrote:
>> atomic* value is signed value, and atomic* functions need also process
>> signed value (parameter value, and return value), so 32-bit arm need
>> use 'long long' instead of 'u64'.
>>
>> After replacement, it will also fix a bug for atomic64_add_negative():
>> "u64 is never less than 0".
>>
>> The modifications are:
>>
>>   in vim, use "1,% s/\<u64\>/long long/g" command.
>>   remove '__aligned(8)' which is useless for 64-bit.
>>   be sure of 80 column limitation after replacement.
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> 
> Looks better to me, thanks. While you're here, we could also replace the use
> of `unsigned long' with `int' for the 32-bit atomics, then the whole header
> is consistent with the generic types.
> 
> Will
> 
> 

Hmm... at least, it seems we can let "use 'int' for 32-bit atomics" in
another patch.


Firstly, this 'fix' is not belong to API, and either, it has no related
'standard' 'demo' in "asm-generic/" (it is architecture independent, so
no related inline assembly code).

After a random check, another 3 architectures (maybe more) are also may
using 'unsigned long': "arm64/include/asm/atomic.h",
"sh/include/asm/atomic-llsc.h", and "xtensa/include/asm/atomic.h".

And as far as I know, for a register related variable, 'unsigned long'
is also a common using way for both 32-bit and 64-bit (at least, it is a
simple way to prevent issues).


Thanks.
-- 
Chen Gang

[PATCH v2] ARM: include: asm: use 'long long' instead of 'u64' within atomic.h

[Will Deacon]

On Thu, Sep 26, 2013 at 03:00:30AM +0100, Chen Gang wrote:
> On 09/26/2013 12:07 AM, Will Deacon wrote:
> > On Wed, Sep 25, 2013 at 03:25:19AM +0100, Chen Gang wrote:
> >> atomic* value is signed value, and atomic* functions need also process
> >> signed value (parameter value, and return value), so 32-bit arm need
> >> use 'long long' instead of 'u64'.
> >>
> >> After replacement, it will also fix a bug for atomic64_add_negative():
> >> "u64 is never less than 0".
> >>
> >> The modifications are:
> >>
> >>   in vim, use "1,% s/\<u64\>/long long/g" command.
> >>   remove '__aligned(8)' which is useless for 64-bit.
> >>   be sure of 80 column limitation after replacement.
> >>
> >>
> >> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> > 
> > Looks better to me, thanks. While you're here, we could also replace the use
> > of `unsigned long' with `int' for the 32-bit atomics, then the whole header
> > is consistent with the generic types.
> > 
> > Will
> > 
> > 
> 
> Hmm... at least, it seems we can let "use 'int' for 32-bit atomics" in
> another patch.

Sure, it can be a follow-up to this one.

> Firstly, this 'fix' is not belong to API, and either, it has no related
> 'standard' 'demo' in "asm-generic/" (it is architecture independent, so
> no related inline assembly code).

I was simply going by the types of atomic_t and atomic64_t, which are both
constructed using signed types.

> After a random check, another 3 architectures (maybe more) are also may
> using 'unsigned long': "arm64/include/asm/atomic.h",
> "sh/include/asm/atomic-llsc.h", and "xtensa/include/asm/atomic.h".
> 
> And as far as I know, for a register related variable, 'unsigned long'
> is also a common using way for both 32-bit and 64-bit (at least, it is a
> simple way to prevent issues).

Maybe, but atomic_t is always 32-bit and atomic64_t is always 64-bit, so I
don't think unsigned long buys you anything with regards to sizing.

Will

[PATCH v2] ARM: include: asm: use 'long long' instead of 'u64' within atomic.h

[Chen Gang]

On 09/26/2013 06:04 PM, Will Deacon wrote:
> On Thu, Sep 26, 2013 at 03:00:30AM +0100, Chen Gang wrote:
>> On 09/26/2013 12:07 AM, Will Deacon wrote:
>>> On Wed, Sep 25, 2013 at 03:25:19AM +0100, Chen Gang wrote:
>>>> atomic* value is signed value, and atomic* functions need also process
>>>> signed value (parameter value, and return value), so 32-bit arm need
>>>> use 'long long' instead of 'u64'.
>>>>
>>>> After replacement, it will also fix a bug for atomic64_add_negative():
>>>> "u64 is never less than 0".
>>>>
>>>> The modifications are:
>>>>
>>>>   in vim, use "1,% s/\<u64\>/long long/g" command.
>>>>   remove '__aligned(8)' which is useless for 64-bit.
>>>>   be sure of 80 column limitation after replacement.
>>>>
>>>>
>>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>>
>>> Looks better to me, thanks. While you're here, we could also replace the use
>>> of `unsigned long' with `int' for the 32-bit atomics, then the whole header
>>> is consistent with the generic types.
>>>
>>> Will
>>>
>>>
>>
>> Hmm... at least, it seems we can let "use 'int' for 32-bit atomics" in
>> another patch.
> 
> Sure, it can be a follow-up to this one.
> 

OK, if it is really a useful patch too, I will follow-up.


>> Firstly, this 'fix' is not belong to API, and either, it has no related
>> 'standard' 'demo' in "asm-generic/" (it is architecture independent, so
>> no related inline assembly code).
> 
> I was simply going by the types of atomic_t and atomic64_t, which are both
> constructed using signed types.
> 

So, it is not quite necessary (but still better) to let it 'consistent'
with 'generic' types (in fact, I feel, for a register related variable,
'unsigned long' is more 'generic' than 'int').

Since it is not belong to API, if some architectures have some reasons
(or excuse) to keep 'inconsistent' with 'generic' types, it is still
acceptable.


>> After a random check, another 3 architectures (maybe more) are also may
>> using 'unsigned long': "arm64/include/asm/atomic.h",
>> "sh/include/asm/atomic-llsc.h", and "xtensa/include/asm/atomic.h".
>>
>> And as far as I know, for a register related variable, 'unsigned long'
>> is also a common using way for both 32-bit and 64-bit (at least, it is a
>> simple way to prevent issues).
> 
> Maybe, but atomic_t is always 32-bit and atomic64_t is always 64-bit, so I
> don't think unsigned long buys you anything with regards to sizing.
> 

'unsigned long' can be used as a register related variable, it is always
32-bit under 32-bit machine, and always 64-bit under 64-bit machine.

So can use it for both arm and arm64, for arm, it can not cause issue,
and for arm64, it is also OK (if changed to 'int' under arm64, may cause
real issue).

So I feel, current arm/arm64 implementation for 'unsigned long' is well
done, not need additional improvement.

> Will
> 
> 

Thanks.
-- 
Chen Gang

[PATCH v2] ARM: include: asm: use 'long long' instead of 'u64' within atomic.h

[Will Deacon]

On Thu, Sep 26, 2013 at 12:03:43PM +0100, Chen Gang wrote:
> On 09/26/2013 06:04 PM, Will Deacon wrote:
> > On Thu, Sep 26, 2013 at 03:00:30AM +0100, Chen Gang wrote:
> >> On 09/26/2013 12:07 AM, Will Deacon wrote:
> >>> On Wed, Sep 25, 2013 at 03:25:19AM +0100, Chen Gang wrote:
> >>>> atomic* value is signed value, and atomic* functions need also process
> >>>> signed value (parameter value, and return value), so 32-bit arm need
> >>>> use 'long long' instead of 'u64'.
> >>>>
> >>>> After replacement, it will also fix a bug for atomic64_add_negative():
> >>>> "u64 is never less than 0".
> >>>>
> >>>> The modifications are:
> >>>>
> >>>>   in vim, use "1,% s/\<u64\>/long long/g" command.
> >>>>   remove '__aligned(8)' which is useless for 64-bit.
> >>>>   be sure of 80 column limitation after replacement.
> >>>>
> >>>>
> >>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> >>>
> >>> Looks better to me, thanks. While you're here, we could also replace the use
> >>> of `unsigned long' with `int' for the 32-bit atomics, then the whole header
> >>> is consistent with the generic types.
> >>>
> >>> Will
> >>>
> >>>
> >>
> >> Hmm... at least, it seems we can let "use 'int' for 32-bit atomics" in
> >> another patch.
> > 
> > Sure, it can be a follow-up to this one.
> > 
> 
> OK, if it is really a useful patch too, I will follow-up.

It's a cosmetic change to bring the signedness of our 32-bit atomics in-line
with your proposal for 64-bit atomics.

> 'unsigned long' can be used as a register related variable, it is always
> 32-bit under 32-bit machine, and always 64-bit under 64-bit machine.

Not necessarily (c.f. ILP32).

> So can use it for both arm and arm64, for arm, it can not cause issue,
> and for arm64, it is also OK (if changed to 'int' under arm64, may cause
> real issue).

arm and arm64 have different instructions sets. There's no way the inline
assembly used to implement the atomic operations can be made portable
between them.

Will

[PATCH v2] ARM: include: asm: use 'long long' instead of 'u64' within atomic.h

[Chen Gang]

On 09/27/2013 07:06 PM, Will Deacon wrote:
> On Thu, Sep 26, 2013 at 12:03:43PM +0100, Chen Gang wrote:
>> On 09/26/2013 06:04 PM, Will Deacon wrote:
>>> On Thu, Sep 26, 2013 at 03:00:30AM +0100, Chen Gang wrote:
>>>> On 09/26/2013 12:07 AM, Will Deacon wrote:
>>>>> On Wed, Sep 25, 2013 at 03:25:19AM +0100, Chen Gang wrote:
>>>>>> atomic* value is signed value, and atomic* functions need also process
>>>>>> signed value (parameter value, and return value), so 32-bit arm need
>>>>>> use 'long long' instead of 'u64'.
>>>>>>
>>>>>> After replacement, it will also fix a bug for atomic64_add_negative():
>>>>>> "u64 is never less than 0".
>>>>>>
>>>>>> The modifications are:
>>>>>>
>>>>>>   in vim, use "1,% s/\<u64\>/long long/g" command.
>>>>>>   remove '__aligned(8)' which is useless for 64-bit.
>>>>>>   be sure of 80 column limitation after replacement.
>>>>>>
>>>>>>
>>>>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>>>>
>>>>> Looks better to me, thanks. While you're here, we could also replace the use
>>>>> of `unsigned long' with `int' for the 32-bit atomics, then the whole header
>>>>> is consistent with the generic types.
>>>>>
>>>>> Will
>>>>>
>>>>>
>>>>
>>>> Hmm... at least, it seems we can let "use 'int' for 32-bit atomics" in
>>>> another patch.
>>>
>>> Sure, it can be a follow-up to this one.
>>>
>>
>> OK, if it is really a useful patch too, I will follow-up.
> 
> It's a cosmetic change to bring the signedness of our 32-bit atomics in-line
> with your proposal for 64-bit atomics.
> 
>> 'unsigned long' can be used as a register related variable, it is always
>> 32-bit under 32-bit machine, and always 64-bit under 64-bit machine.
> 
> Not necessarily (c.f. ILP32).
> 
>> So can use it for both arm and arm64, for arm, it can not cause issue,
>> and for arm64, it is also OK (if changed to 'int' under arm64, may cause
>> real issue).
> 
> arm and arm64 have different instructions sets. There's no way the inline
> assembly used to implement the atomic operations can be made portable
> between them.
> 


Hmm... if you like, I will send a follow-up patch for it, the reason is:

It is belong to internal implementation, not belong to API, so if it is
harmless, better to follow related maintainers' 'hobby' or 'tastes', it
will let the related code more clearer.  :-)


I will wait 1-2 days to let another reviewers check, if no reply, I will
send 2 patches for it. (if you already applied the "u64 -> long long"
patch, please let me know, and I will send one patch enough).


> Will
> 
> 

Thanks.
-- 
Chen Gang

[PATCH 0/2] ARM: include: asm: change functions' and variables' types in atomic.h

[Chen Gang]

It contents 2 patches:

Patch 1: use 'long long' instead of 'u64' for within atomic.h.

Patch 2: use 'int' instead of 'unsigned long' for normal register
variables within atomic.h


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/arm/include/asm/atomic.h |   77
+++++++++++++++++++++--------------------
 1 files changed, 39 insertions(+), 38 deletions(-)

[PATCH 1/2] ARM: include: asm: use 'long long' instead of 'u64' within atomic.h

[Chen Gang]

atomic* value is signed value, and atomic* functions need also process
signed value (parameter value, and return value), so 32-bit arm need
use 'long long' instead of 'u64'.

After replacement, it will also fix a bug for atomic64_add_negative():
"u64 is never less than 0".

The modifications are:

  in vim, use "1,% s/\<u64\>/long long/g" command.
  remove '__aligned(8)' which is useless for 64-bit.
  be sure of 80 column limitation after replacement.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/arm/include/asm/atomic.h |   49 +++++++++++++++++++++--------------------
 1 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index da1c77d..a715ac0 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -238,15 +238,15 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u)
 
 #ifndef CONFIG_GENERIC_ATOMIC64
 typedef struct {
-	u64 __aligned(8) counter;
+	long long counter;
 } atomic64_t;
 
 #define ATOMIC64_INIT(i) { (i) }
 
 #ifdef CONFIG_ARM_LPAE
-static inline u64 atomic64_read(const atomic64_t *v)
+static inline long long atomic64_read(const atomic64_t *v)
 {
-	u64 result;
+	long long result;
 
 	__asm__ __volatile__("@ atomic64_read\n"
 "	ldrd	%0, %H0, [%1]"
@@ -257,7 +257,7 @@ static inline u64 atomic64_read(const atomic64_t *v)
 	return result;
 }
 
-static inline void atomic64_set(atomic64_t *v, u64 i)
+static inline void atomic64_set(atomic64_t *v, long long i)
 {
 	__asm__ __volatile__("@ atomic64_set\n"
 "	strd	%2, %H2, [%1]"
@@ -266,9 +266,9 @@ static inline void atomic64_set(atomic64_t *v, u64 i)
 	);
 }
 #else
-static inline u64 atomic64_read(const atomic64_t *v)
+static inline long long atomic64_read(const atomic64_t *v)
 {
-	u64 result;
+	long long result;
 
 	__asm__ __volatile__("@ atomic64_read\n"
 "	ldrexd	%0, %H0, [%1]"
@@ -279,9 +279,9 @@ static inline u64 atomic64_read(const atomic64_t *v)
 	return result;
 }
 
-static inline void atomic64_set(atomic64_t *v, u64 i)
+static inline void atomic64_set(atomic64_t *v, long long i)
 {
-	u64 tmp;
+	long long tmp;
 
 	__asm__ __volatile__("@ atomic64_set\n"
 "1:	ldrexd	%0, %H0, [%2]\n"
@@ -294,9 +294,9 @@ static inline void atomic64_set(atomic64_t *v, u64 i)
 }
 #endif
 
-static inline void atomic64_add(u64 i, atomic64_t *v)
+static inline void atomic64_add(long long i, atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	__asm__ __volatile__("@ atomic64_add\n"
@@ -311,9 +311,9 @@ static inline void atomic64_add(u64 i, atomic64_t *v)
 	: "cc");
 }
 
-static inline u64 atomic64_add_return(u64 i, atomic64_t *v)
+static inline long long atomic64_add_return(long long i, atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	smp_mb();
@@ -334,9 +334,9 @@ static inline u64 atomic64_add_return(u64 i, atomic64_t *v)
 	return result;
 }
 
-static inline void atomic64_sub(u64 i, atomic64_t *v)
+static inline void atomic64_sub(long long i, atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	__asm__ __volatile__("@ atomic64_sub\n"
@@ -351,9 +351,9 @@ static inline void atomic64_sub(u64 i, atomic64_t *v)
 	: "cc");
 }
 
-static inline u64 atomic64_sub_return(u64 i, atomic64_t *v)
+static inline long long atomic64_sub_return(long long i, atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	smp_mb();
@@ -374,9 +374,10 @@ static inline u64 atomic64_sub_return(u64 i, atomic64_t *v)
 	return result;
 }
 
-static inline u64 atomic64_cmpxchg(atomic64_t *ptr, u64 old, u64 new)
+static inline long long atomic64_cmpxchg(atomic64_t *ptr, long long old,
+					long long new)
 {
-	u64 oldval;
+	long long oldval;
 	unsigned long res;
 
 	smp_mb();
@@ -398,9 +399,9 @@ static inline u64 atomic64_cmpxchg(atomic64_t *ptr, u64 old, u64 new)
 	return oldval;
 }
 
-static inline u64 atomic64_xchg(atomic64_t *ptr, u64 new)
+static inline long long atomic64_xchg(atomic64_t *ptr, long long new)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	smp_mb();
@@ -419,9 +420,9 @@ static inline u64 atomic64_xchg(atomic64_t *ptr, u64 new)
 	return result;
 }
 
-static inline u64 atomic64_dec_if_positive(atomic64_t *v)
+static inline long long atomic64_dec_if_positive(atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	smp_mb();
@@ -445,9 +446,9 @@ static inline u64 atomic64_dec_if_positive(atomic64_t *v)
 	return result;
 }
 
-static inline int atomic64_add_unless(atomic64_t *v, u64 a, u64 u)
+static inline int atomic64_add_unless(atomic64_t *v, long long a, long long u)
 {
-	u64 val;
+	long long val;
 	unsigned long tmp;
 	int ret = 1;
 
-- 
1.7.7.6

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Chen Gang]

"arc/arm" will be never on 64-bit, it is mainly on 32-bit (may also can
be on 16-bit).

So better to use 'int' instead of 'unsigned long' for normal register
variable (on 16-bit, 'int' is allowed to be 16-bit, so historically,
often use 'int' for normal register variables).


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/arm/include/asm/atomic.h |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index a715ac0..9f94ee7 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -38,7 +38,7 @@
  */
 static inline void atomic_add(int i, atomic_t *v)
 {
-	unsigned long tmp;
+	int tmp;
 	int result;

 	__asm__ __volatile__("@ atomic_add\n"
@@ -54,7 +54,7 @@ static inline void atomic_add(int i, atomic_t *v)

 static inline int atomic_add_return(int i, atomic_t *v)
 {
-	unsigned long tmp;
+	int tmp;
 	int result;

 	smp_mb();
@@ -76,7 +76,7 @@ static inline int atomic_add_return(int i, atomic_t *v)

 static inline void atomic_sub(int i, atomic_t *v)
 {
-	unsigned long tmp;
+	int tmp;
 	int result;

 	__asm__ __volatile__("@ atomic_sub\n"
@@ -92,7 +92,7 @@ static inline void atomic_sub(int i, atomic_t *v)

 static inline int atomic_sub_return(int i, atomic_t *v)
 {
-	unsigned long tmp;
+	int tmp;
 	int result;

 	smp_mb();
@@ -114,7 +114,7 @@ static inline int atomic_sub_return(int i, atomic_t *v)

 static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
 {
-	unsigned long oldval, res;
+	int oldval, res;

 	smp_mb();

@@ -136,7 +136,7 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int
old, int new)

 static inline void atomic_clear_mask(unsigned long mask, unsigned long
*addr)
 {
-	unsigned long tmp, tmp2;
+	int tmp, tmp2;

 	__asm__ __volatile__("@ atomic_clear_mask\n"
 "1:	ldrex	%0, [%3]\n"
@@ -297,7 +297,7 @@ static inline void atomic64_set(atomic64_t *v, long
long i)
 static inline void atomic64_add(long long i, atomic64_t *v)
 {
 	long long result;
-	unsigned long tmp;
+	int tmp;

 	__asm__ __volatile__("@ atomic64_add\n"
 "1:	ldrexd	%0, %H0, [%3]\n"
@@ -314,7 +314,7 @@ static inline void atomic64_add(long long i,
atomic64_t *v)
 static inline long long atomic64_add_return(long long i, atomic64_t *v)
 {
 	long long result;
-	unsigned long tmp;
+	int tmp;

 	smp_mb();

@@ -337,7 +337,7 @@ static inline long long atomic64_add_return(long
long i, atomic64_t *v)
 static inline void atomic64_sub(long long i, atomic64_t *v)
 {
 	long long result;
-	unsigned long tmp;
+	int tmp;

 	__asm__ __volatile__("@ atomic64_sub\n"
 "1:	ldrexd	%0, %H0, [%3]\n"
@@ -354,7 +354,7 @@ static inline void atomic64_sub(long long i,
atomic64_t *v)
 static inline long long atomic64_sub_return(long long i, atomic64_t *v)
 {
 	long long result;
-	unsigned long tmp;
+	int tmp;

 	smp_mb();

@@ -378,7 +378,7 @@ static inline long long atomic64_cmpxchg(atomic64_t
*ptr, long long old,
 					long long new)
 {
 	long long oldval;
-	unsigned long res;
+	int res;

 	smp_mb();

@@ -402,7 +402,7 @@ static inline long long atomic64_cmpxchg(atomic64_t
*ptr, long long old,
 static inline long long atomic64_xchg(atomic64_t *ptr, long long new)
 {
 	long long result;
-	unsigned long tmp;
+	int tmp;

 	smp_mb();

@@ -423,7 +423,7 @@ static inline long long atomic64_xchg(atomic64_t
*ptr, long long new)
 static inline long long atomic64_dec_if_positive(atomic64_t *v)
 {
 	long long result;
-	unsigned long tmp;
+	int tmp;

 	smp_mb();

@@ -449,7 +449,7 @@ static inline long long
atomic64_dec_if_positive(atomic64_t *v)
 static inline int atomic64_add_unless(atomic64_t *v, long long a, long
long u)
 {
 	long long val;
-	unsigned long tmp;
+	int tmp;
 	int ret = 1;

 	smp_mb();
-- 
1.7.7.6

[PATCH 1/2] ARM: include: asm: use 'long long' instead of 'u64' within atomic.h

[Will Deacon]

On Sun, Sep 29, 2013 at 04:43:54AM +0100, Chen Gang wrote:
> atomic* value is signed value, and atomic* functions need also process
> signed value (parameter value, and return value), so 32-bit arm need
> use 'long long' instead of 'u64'.
> 
> After replacement, it will also fix a bug for atomic64_add_negative():
> "u64 is never less than 0".
> 
> The modifications are:
> 
>   in vim, use "1,% s/\<u64\>/long long/g" command.
>   remove '__aligned(8)' which is useless for 64-bit.
>   be sure of 80 column limitation after replacement.
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>

Acked-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Will Deacon]

On Sun, Sep 29, 2013 at 04:52:28AM +0100, Chen Gang wrote:
> "arc/arm" will be never on 64-bit, it is mainly on 32-bit (may also can
> be on 16-bit).
> 
> So better to use 'int' instead of 'unsigned long' for normal register
> variable (on 16-bit, 'int' is allowed to be 16-bit, so historically,
> often use 'int' for normal register variables).

This commit message doesn't make a blind bit of sense! arch/arm/ is a 32-bit
architecture in the sense that int will always be 32-bit there. This patch
is just a cosmetic change, bringing our atomic_t manipulation code inline
with the atomic_t type definition.

> @@ -297,7 +297,7 @@ static inline void atomic64_set(atomic64_t *v, long
> long i)
>  static inline void atomic64_add(long long i, atomic64_t *v)
>  {
>  	long long result;
> -	unsigned long tmp;
> +	int tmp;

Please leave the atomic64_* functions alone here; the reasoning I explained
above doesn't apply to them. Whilst int may work, it seems gratuitous to
make this change for no reason.

Will

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Chen Gang]

On 10/01/2013 12:11 AM, Will Deacon wrote:
> On Sun, Sep 29, 2013 at 04:52:28AM +0100, Chen Gang wrote:
>> "arc/arm" will be never on 64-bit, it is mainly on 32-bit (may also can
>> be on 16-bit).
>>
>> So better to use 'int' instead of 'unsigned long' for normal register
>> variable (on 16-bit, 'int' is allowed to be 16-bit, so historically,
>> often use 'int' for normal register variables).
> 
> This commit message doesn't make a blind bit of sense! arch/arm/ is a 32-bit
> architecture in the sense that int will always be 32-bit there. This patch
> is just a cosmetic change, bringing our atomic_t manipulation code inline
> with the atomic_t type definition.
> 

OK, thanks. That means: "arm means arm 32-bit, arm64 means arm 64-bit.
The current Linux kernel main line does not support arm 16-bit".

Since "bringing our atomic_t ... with the atomic_t type definition", can
we use 'atomic_t" instead of 'unsigned long'?

And can we use 'atomic64_t" instead of 'unsigned long' in atomic64_*()?


>> @@ -297,7 +297,7 @@ static inline void atomic64_set(atomic64_t *v, long
>> long i)
>>  static inline void atomic64_add(long long i, atomic64_t *v)
>>  {
>>  	long long result;
>> -	unsigned long tmp;
>> +	int tmp;
> 
> Please leave the atomic64_* functions alone here; the reasoning I explained
> above doesn't apply to them. Whilst int may work, it seems gratuitous to
> make this change for no reason.
> 

For 32-bit, it seems we can not use 'atomic64_t' instead of 'unsigned
long' in atomic64_*().  ;-)

For 32-bit, the original 'unsigned long' in atomic64_add() is correct,
or is also a bug?


> Will
> 
> 

In fact, if arm only for 32-bit, I really don't know why we need 'int'
instead of some 'unsigned long' (neither can let patch comment correct,
nor know which 'unsigned long' need be instead of).  :-(

It seems we need be careful to not let this patch make new bug for arm.



Thanks.
-- 
Chen Gang

[PATCH 1/2] ARM: include: asm: use 'long long' instead of 'u64' within atomic.h

[Chen Gang]

On 10/01/2013 12:07 AM, Will Deacon wrote:
> On Sun, Sep 29, 2013 at 04:43:54AM +0100, Chen Gang wrote:
>> atomic* value is signed value, and atomic* functions need also process
>> signed value (parameter value, and return value), so 32-bit arm need
>> use 'long long' instead of 'u64'.
>>
>> After replacement, it will also fix a bug for atomic64_add_negative():
>> "u64 is never less than 0".
>>
>> The modifications are:
>>
>>   in vim, use "1,% s/\<u64\>/long long/g" command.
>>   remove '__aligned(8)' which is useless for 64-bit.
>>   be sure of 80 column limitation after replacement.
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> 
> Acked-by: Will Deacon <will.deacon@arm.com>
> 
> Will
> 
> 

Thanks.

-- 
Chen Gang

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Will Deacon]

On Tue, Oct 01, 2013 at 03:05:27AM +0100, Chen Gang wrote:
> On 10/01/2013 12:11 AM, Will Deacon wrote:
> > On Sun, Sep 29, 2013 at 04:52:28AM +0100, Chen Gang wrote:
> >> "arc/arm" will be never on 64-bit, it is mainly on 32-bit (may also can
> >> be on 16-bit).
> >>
> >> So better to use 'int' instead of 'unsigned long' for normal register
> >> variable (on 16-bit, 'int' is allowed to be 16-bit, so historically,
> >> often use 'int' for normal register variables).
> > 
> > This commit message doesn't make a blind bit of sense! arch/arm/ is a 32-bit
> > architecture in the sense that int will always be 32-bit there. This patch
> > is just a cosmetic change, bringing our atomic_t manipulation code inline
> > with the atomic_t type definition.
> > 
> 
> OK, thanks. That means: "arm means arm 32-bit, arm64 means arm 64-bit.
> The current Linux kernel main line does not support arm 16-bit".
> 
> Since "bringing our atomic_t ... with the atomic_t type definition", can
> we use 'atomic_t" instead of 'unsigned long'?
> 
> And can we use 'atomic64_t" instead of 'unsigned long' in atomic64_*()?

That's probably a bit dodgy, since they are typedefs to compound types
which, if ever extended, would fall to bits if we tried to pack them into a
single register.

Will

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Chen Gang]

On 10/01/2013 05:01 PM, Will Deacon wrote:
> On Tue, Oct 01, 2013 at 03:05:27AM +0100, Chen Gang wrote:
>> On 10/01/2013 12:11 AM, Will Deacon wrote:
>>> On Sun, Sep 29, 2013 at 04:52:28AM +0100, Chen Gang wrote:
>>>> "arc/arm" will be never on 64-bit, it is mainly on 32-bit (may also can
>>>> be on 16-bit).
>>>>
>>>> So better to use 'int' instead of 'unsigned long' for normal register
>>>> variable (on 16-bit, 'int' is allowed to be 16-bit, so historically,
>>>> often use 'int' for normal register variables).
>>>
>>> This commit message doesn't make a blind bit of sense! arch/arm/ is a 32-bit
>>> architecture in the sense that int will always be 32-bit there. This patch
>>> is just a cosmetic change, bringing our atomic_t manipulation code inline
>>> with the atomic_t type definition.
>>>
>>
>> OK, thanks. That means: "arm means arm 32-bit, arm64 means arm 64-bit.
>> The current Linux kernel main line does not support arm 16-bit".
>>
>> Since "bringing our atomic_t ... with the atomic_t type definition", can
>> we use 'atomic_t" instead of 'unsigned long'?
>>
>> And can we use 'atomic64_t" instead of 'unsigned long' in atomic64_*()?
> 
> That's probably a bit dodgy, since they are typedefs to compound types
> which, if ever extended, would fall to bits if we tried to pack them into a
> single register.
> 

Excuse me, my English is not quite well, I guess your meaning is: "in
'atomic.h', for arm/arm64, let register variables type equal to related
atomic type: use 64-bit type if 'atomic64_t', 32-bit type if 'atomic_t'.

If what I guess is correct, please continue reading, or please help
repeat again (not need reading below contents), thanks.


Can we say: "under arm, using 'unsigned long' for register related
variables in atomic64_*() is not suitable (although not a bug), need use
'long long' (which is 'atomic64_t' under arm) instead of"?

And under arm64, can we use 'int' (which is 'atomic_t' under 64-bit arm)
instead of 'unsigned long' for register related variables in atomic_*()?

I feel, if both of them above are correct, your idea sounds reasonable,
or it seems your idea is a little complex (at least, keeping original
implementation no touch is still not a bad idea).


> Will
> 
> 

Thanks.
-- 
Chen Gang

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Will Deacon]

On Tue, Oct 01, 2013 at 01:03:44PM +0100, Chen Gang wrote:
> On 10/01/2013 05:01 PM, Will Deacon wrote:
> > On Tue, Oct 01, 2013 at 03:05:27AM +0100, Chen Gang wrote:
> >> OK, thanks. That means: "arm means arm 32-bit, arm64 means arm 64-bit.
> >> The current Linux kernel main line does not support arm 16-bit".
> >>
> >> Since "bringing our atomic_t ... with the atomic_t type definition", can
> >> we use 'atomic_t" instead of 'unsigned long'?
> >>
> >> And can we use 'atomic64_t" instead of 'unsigned long' in atomic64_*()?
> > 
> > That's probably a bit dodgy, since they are typedefs to compound types
> > which, if ever extended, would fall to bits if we tried to pack them into a
> > single register.
> > 
> 
> Excuse me, my English is not quite well, I guess your meaning is: "in
> 'atomic.h', for arm/arm64, let register variables type equal to related
> atomic type: use 64-bit type if 'atomic64_t', 32-bit type if 'atomic_t'.

Sorry, I think I've confused you. This should be a simple change:

  On ARM:

  - The atomic_t typedef is a struct containing an int.
  - The atomic64_t typedef is a struct containing a long long.

  On arm64:

  - The atomic_t typedef is a struct containing an int.
  - The atomic64_t typedef is a struct containing a long.

Now, your first patch made the handling of atomic64_t on ARM use long long
instead of u64. That's good, because it fixes a signedness bug.

The second patch is just a clean-up, because our atomic_* functions on ARM
interchangeably use int and unsigned long when working with atomic_t types.
This can be tidied up by using int whenever we are reading or writing an
atomic_t, thus matching the type definition for that structure member.

So, for example, atomic_add does not need changing because it always uses
int to hold the atomic_t variable (the unsigned long holds the status
returned from the strex instruction). atomic_cmpxchg, however, loads the
atomic_t into oldval, so that should be int.

Given that this patch doesn't gain us anything in terms of functionality,
feel free to ignore it and I can take a look when there's a rainy day (which
should be real soon now). I also need to take a closer look at
atomic_clear_mask for arm64, because it looks broken to me.

Will

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Chen Gang]

On 10/02/2013 06:41 PM, Will Deacon wrote:
> On Tue, Oct 01, 2013 at 01:03:44PM +0100, Chen Gang wrote:
>> On 10/01/2013 05:01 PM, Will Deacon wrote:
>>> On Tue, Oct 01, 2013 at 03:05:27AM +0100, Chen Gang wrote:
>>>> OK, thanks. That means: "arm means arm 32-bit, arm64 means arm 64-bit.
>>>> The current Linux kernel main line does not support arm 16-bit".
>>>>
>>>> Since "bringing our atomic_t ... with the atomic_t type definition", can
>>>> we use 'atomic_t" instead of 'unsigned long'?
>>>>
>>>> And can we use 'atomic64_t" instead of 'unsigned long' in atomic64_*()?
>>>
>>> That's probably a bit dodgy, since they are typedefs to compound types
>>> which, if ever extended, would fall to bits if we tried to pack them into a
>>> single register.
>>>
>>
>> Excuse me, my English is not quite well, I guess your meaning is: "in
>> 'atomic.h', for arm/arm64, let register variables type equal to related
>> atomic type: use 64-bit type if 'atomic64_t', 32-bit type if 'atomic_t'.
> 
> Sorry, I think I've confused you. This should be a simple change:
> 

Oh, no sorry, my English is really not quite well, and I am not quite
familiar with arm (even inline assembly language grammar, which I
should familiar).

I really should see the deals about it.


>   On ARM:
> 
>   - The atomic_t typedef is a struct containing an int.
>   - The atomic64_t typedef is a struct containing a long long.
> 
>   On arm64:
> 
>   - The atomic_t typedef is a struct containing an int.
>   - The atomic64_t typedef is a struct containing a long.
> 

Yeah.

> Now, your first patch made the handling of atomic64_t on ARM use long long
> instead of u64. That's good, because it fixes a signedness bug.
> 

OK, thanks.

> The second patch is just a clean-up, because our atomic_* functions on ARM
> interchangeably use int and unsigned long when working with atomic_t types.
> This can be tidied up by using int whenever we are reading or writing an
> atomic_t, thus matching the type definition for that structure member.
> 
> So, for example, atomic_add does not need changing because it always uses
> int to hold the atomic_t variable (the unsigned long holds the status
> returned from the strex instruction). atomic_cmpxchg, however, loads the
> atomic_t into oldval, so that should be int.
> 

OK, thank you for your details information, originally I did not check
the inline assembly code in details (which I should do).

And what you said above is reasonable to me.

After a check, I think for atomic64_*() in arm have no this kind of
issue.


> Given that this patch doesn't gain us anything in terms of functionality,
> feel free to ignore it and I can take a look when there's a rainy day (which
> should be real soon now). I also need to take a closer look at
> atomic_clear_mask for arm64, because it looks broken to me.
> 

OK, thanks. 

Hmm... after read arm64 again, at least for me, it has no this kind of
issue.

:-)

> Will
> 
> 


So the related patch is below (still based on Patch 1):

-----------------------------patch begin--------------------------------

arm/include/asm/atomic.h: use 'int' instead of 'unsigned long' for 'oldval' in atomic_cmpxchg().

  For atomic_cmpxchg(), the type of 'oldval' need be 'int' to match the
  type of "*ptr" (used by 'ldrex' instruction) and 'old' (used by 'teq'
  instruction).


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/arm/include/asm/atomic.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index a715ac0..9ee7e01 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -114,7 +114,8 @@ static inline int atomic_sub_return(int i, atomic_t *v)
 
 static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
 {
-	unsigned long oldval, res;
+	int oldval;
+	unsigned long res;
 
 	smp_mb();
 
-- 
1.7.7.6

-----------------------------patch end----------------------------------


Thanks.
-- 
Chen Gang

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Chen Gang]

If need send patch v2 for it, I should try (if so, need patch 1 also
send again?).

Thanks.

On 10/02/2013 11:19 PM, Chen Gang wrote:
> On 10/02/2013 06:41 PM, Will Deacon wrote:
>> On Tue, Oct 01, 2013 at 01:03:44PM +0100, Chen Gang wrote:
>>> On 10/01/2013 05:01 PM, Will Deacon wrote:
>>>> On Tue, Oct 01, 2013 at 03:05:27AM +0100, Chen Gang wrote:
>>>>> OK, thanks. That means: "arm means arm 32-bit, arm64 means arm 64-bit.
>>>>> The current Linux kernel main line does not support arm 16-bit".
>>>>>
>>>>> Since "bringing our atomic_t ... with the atomic_t type definition", can
>>>>> we use 'atomic_t" instead of 'unsigned long'?
>>>>>
>>>>> And can we use 'atomic64_t" instead of 'unsigned long' in atomic64_*()?
>>>>
>>>> That's probably a bit dodgy, since they are typedefs to compound types
>>>> which, if ever extended, would fall to bits if we tried to pack them into a
>>>> single register.
>>>>
>>>
>>> Excuse me, my English is not quite well, I guess your meaning is: "in
>>> 'atomic.h', for arm/arm64, let register variables type equal to related
>>> atomic type: use 64-bit type if 'atomic64_t', 32-bit type if 'atomic_t'.
>>
>> Sorry, I think I've confused you. This should be a simple change:
>>
> 
> Oh, no sorry, my English is really not quite well, and I am not quite
> familiar with arm (even inline assembly language grammar, which I
> should familiar).
> 
> I really should see the deals about it.
> 
> 
>>   On ARM:
>>
>>   - The atomic_t typedef is a struct containing an int.
>>   - The atomic64_t typedef is a struct containing a long long.
>>
>>   On arm64:
>>
>>   - The atomic_t typedef is a struct containing an int.
>>   - The atomic64_t typedef is a struct containing a long.
>>
> 
> Yeah.
> 
>> Now, your first patch made the handling of atomic64_t on ARM use long long
>> instead of u64. That's good, because it fixes a signedness bug.
>>
> 
> OK, thanks.
> 
>> The second patch is just a clean-up, because our atomic_* functions on ARM
>> interchangeably use int and unsigned long when working with atomic_t types.
>> This can be tidied up by using int whenever we are reading or writing an
>> atomic_t, thus matching the type definition for that structure member.
>>
>> So, for example, atomic_add does not need changing because it always uses
>> int to hold the atomic_t variable (the unsigned long holds the status
>> returned from the strex instruction). atomic_cmpxchg, however, loads the
>> atomic_t into oldval, so that should be int.
>>
> 
> OK, thank you for your details information, originally I did not check
> the inline assembly code in details (which I should do).
> 
> And what you said above is reasonable to me.
> 
> After a check, I think for atomic64_*() in arm have no this kind of
> issue.
> 
> 
>> Given that this patch doesn't gain us anything in terms of functionality,
>> feel free to ignore it and I can take a look when there's a rainy day (which
>> should be real soon now). I also need to take a closer look at
>> atomic_clear_mask for arm64, because it looks broken to me.
>>
> 
> OK, thanks. 
> 
> Hmm... after read arm64 again, at least for me, it has no this kind of
> issue.
> 
> :-)
> 
>> Will
>>
>>
> 
> 
> So the related patch is below (still based on Patch 1):
> 
> -----------------------------patch begin--------------------------------
> 
> arm/include/asm/atomic.h: use 'int' instead of 'unsigned long' for 'oldval' in atomic_cmpxchg().
> 
>   For atomic_cmpxchg(), the type of 'oldval' need be 'int' to match the
>   type of "*ptr" (used by 'ldrex' instruction) and 'old' (used by 'teq'
>   instruction).
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  arch/arm/include/asm/atomic.h |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
> index a715ac0..9ee7e01 100644
> --- a/arch/arm/include/asm/atomic.h
> +++ b/arch/arm/include/asm/atomic.h
> @@ -114,7 +114,8 @@ static inline int atomic_sub_return(int i, atomic_t *v)
>  
>  static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>  {
> -	unsigned long oldval, res;
> +	int oldval;
> +	unsigned long res;
>  
>  	smp_mb();
>  
> 


-- 
Chen Gang

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Will Deacon]

On Thu, Oct 03, 2013 at 11:05:31AM +0100, Chen Gang wrote:
> If need send patch v2 for it, I should try (if so, need patch 1 also
> send again?).

Almost, comments below.

> > So the related patch is below (still based on Patch 1):
> > 
> > -----------------------------patch begin--------------------------------
> > 
> > arm/include/asm/atomic.h: use 'int' instead of 'unsigned long' for 'oldval' in atomic_cmpxchg().
> > 
> >   For atomic_cmpxchg(), the type of 'oldval' need be 'int' to match the
> >   type of "*ptr" (used by 'ldrex' instruction) and 'old' (used by 'teq'
> >   instruction).
> > 
> > 
> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
> > ---
> >  arch/arm/include/asm/atomic.h |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
> > index a715ac0..9ee7e01 100644
> > --- a/arch/arm/include/asm/atomic.h
> > +++ b/arch/arm/include/asm/atomic.h
> > @@ -114,7 +114,8 @@ static inline int atomic_sub_return(int i, atomic_t *v)
> >  
> >  static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
> >  {
> > -	unsigned long oldval, res;
> > +	int oldval;
> > +	unsigned long res;
> >  
> >  	smp_mb();

I think you should also fix atomic_clear_mask to take (unsigned int mask,
atomic_t *v). That would require a third patch fixing asm-generic, where
atomic_clear_mask and atomic_set_mask have different types for the mask
parameter.

The problem with arm64 is that we're using *unsigned long* for 32-bit
clear_mask, which is definitely wrong because it's 64-bit (another patch to
fix this!).

Finally, is atomic_clear_mask even used anywhere outside of
drivers/s390/scsi/? If not, perhaps we can just drop this function from arm
and arm64 after all.

Will

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Chen Gang]

On 10/04/2013 12:32 AM, Will Deacon wrote:
> On Thu, Oct 03, 2013 at 11:05:31AM +0100, Chen Gang wrote:
>> If need send patch v2 for it, I should try (if so, need patch 1 also
>> send again?).
> 
> Almost, comments below.
> 
>>> So the related patch is below (still based on Patch 1):
>>>
>>> -----------------------------patch begin--------------------------------
>>>
>>> arm/include/asm/atomic.h: use 'int' instead of 'unsigned long' for 'oldval' in atomic_cmpxchg().
>>>
>>>   For atomic_cmpxchg(), the type of 'oldval' need be 'int' to match the
>>>   type of "*ptr" (used by 'ldrex' instruction) and 'old' (used by 'teq'
>>>   instruction).
>>>
>>>
>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>> ---
>>>  arch/arm/include/asm/atomic.h |    3 ++-
>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
>>> index a715ac0..9ee7e01 100644
>>> --- a/arch/arm/include/asm/atomic.h
>>> +++ b/arch/arm/include/asm/atomic.h
>>> @@ -114,7 +114,8 @@ static inline int atomic_sub_return(int i, atomic_t *v)
>>>  
>>>  static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>>>  {
>>> -	unsigned long oldval, res;
>>> +	int oldval;
>>> +	unsigned long res;
>>>  
>>>  	smp_mb();
> 
> I think you should also fix atomic_clear_mask to take (unsigned int mask,
> atomic_t *v). That would require a third patch fixing asm-generic, where
> atomic_clear_mask and atomic_set_mask have different types for the mask
> parameter.
> 
> The problem with arm64 is that we're using *unsigned long* for 32-bit
> clear_mask, which is definitely wrong because it's 64-bit (another patch to
> fix this!).
> 

At least, that is not a bug.

In fact, in my opinion, atomic_set/clear_mask() can be for both 32-bit
and 64-bit (no 'atomic64_set/clear_mask' in 'asm-generic').

> Finally, is atomic_clear_mask even used anywhere outside of
> drivers/s390/scsi/? If not, perhaps we can just drop this function from arm
> and arm64 after all.
> 

Please see my original patch which may related for our discussion:
https://lkml.org/lkml/2013/6/9/4

In fact, that already included my opinion which may related with our
discussion, and has get acked-by for s390, but still suspending :-(


> Will
> 
> 

Thanks.
-- 
Chen Gang

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Will Deacon]

On Fri, Oct 04, 2013 at 10:51:56AM +0100, Chen Gang wrote:
> On 10/04/2013 12:32 AM, Will Deacon wrote:
> > The problem with arm64 is that we're using *unsigned long* for 32-bit
> > clear_mask, which is definitely wrong because it's 64-bit (another patch to
> > fix this!).
> > 
> 
> At least, that is not a bug.

Sure it is. What if the adjacent 32-bit value was being accessed by another
CPU under a spinlock?

Will

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Will Deacon]

On Fri, Oct 04, 2013 at 04:37:42PM +0100, Will Deacon wrote:
> On Fri, Oct 04, 2013 at 10:51:56AM +0100, Chen Gang wrote:
> > On 10/04/2013 12:32 AM, Will Deacon wrote:
> > > The problem with arm64 is that we're using *unsigned long* for 32-bit
> > > clear_mask, which is definitely wrong because it's 64-bit (another patch to
> > > fix this!).
> > > 
> > 
> > At least, that is not a bug.
> 
> Sure it is. What if the adjacent 32-bit value was being accessed by another
> CPU under a spinlock?

(Oh, ok, that would still work on arm because of the way the exclusive
monitor is implemented, but we shouldn't rely on that).

Will

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Chen Gang]

On 10/04/2013 11:42 PM, Will Deacon wrote:
> On Fri, Oct 04, 2013 at 04:37:42PM +0100, Will Deacon wrote:
>> On Fri, Oct 04, 2013 at 10:51:56AM +0100, Chen Gang wrote:
>>> On 10/04/2013 12:32 AM, Will Deacon wrote:
>>>> The problem with arm64 is that we're using *unsigned long* for 32-bit
>>>> clear_mask, which is definitely wrong because it's 64-bit (another patch to
>>>> fix this!).
>>>>
>>>
>>> At least, that is not a bug.
>>
>> Sure it is. What if the adjacent 32-bit value was being accessed by another
>> CPU under a spinlock?
> 
> (Oh, ok, that would still work on arm because of the way the exclusive
> monitor is implemented, but we shouldn't rely on that).
>

Hmm... in my opinion, we need divide atomic_*_mask() into 32-bit and
64-bit versions. We already have demands to only use 32-bit value to
express mask (can save size), and may have demands to use 64-bit too.

If so, can easily standard all atomic_*_mask() which are in asm-generic
and various architectures: use atomic_*_mask() for 32-bit mask, and use
atomic64_*_mask() for 64-bit mask (can delayed before get real demands).

Since it is about API, so it is related with asm-generic and all other
architectures interfaces, it is better to consider about them firstly
before the fix under arm64.


And also, excuse me, I am not quite familiar with "exclusive monitor",
could you please provide more details about it?


> Will
> 
> 


Thanks.
-- 
Chen Gang

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Chen Gang]

On 10/05/2013 07:55 AM, Chen Gang wrote:
> On 10/04/2013 11:42 PM, Will Deacon wrote:
>> On Fri, Oct 04, 2013 at 04:37:42PM +0100, Will Deacon wrote:
>>> On Fri, Oct 04, 2013 at 10:51:56AM +0100, Chen Gang wrote:
>>>> On 10/04/2013 12:32 AM, Will Deacon wrote:
>>>>> The problem with arm64 is that we're using *unsigned long* for 32-bit
>>>>> clear_mask, which is definitely wrong because it's 64-bit (another patch to
>>>>> fix this!).
>>>>>
>>>>
>>>> At least, that is not a bug.
>>>
>>> Sure it is. What if the adjacent 32-bit value was being accessed by another
>>> CPU under a spinlock?
>>
>> (Oh, ok, that would still work on arm because of the way the exclusive
>> monitor is implemented, but we shouldn't rely on that).
>>
> 
> Hmm... in my opinion, we need divide atomic_*_mask() into 32-bit and
> 64-bit versions. We already have demands to only use 32-bit value to
> express mask (can save size), and may have demands to use 64-bit too.
> 
> If so, can easily standard all atomic_*_mask() which are in asm-generic
> and various architectures: use atomic_*_mask() for 32-bit mask, and use
> atomic64_*_mask() for 64-bit mask (can delayed before get real demands).
> 
> Since it is about API, so it is related with asm-generic and all other
> architectures interfaces, it is better to consider about them firstly
> before the fix under arm64.
> 
> 
> And also, excuse me, I am not quite familiar with "exclusive monitor",
> could you please provide more details about it?
> 

If in arm64 with allmodconfig, we only need atomic_*_mask() for 32-bit
mask, don't need 64-bit, we can use 'unsigned int' instead of 'unsigned
long' (need not consider about asm-generic and other architectures).



> 
>> Will
>>
>>
> 
> 
> Thanks.
> 


-- 
Chen Gang

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Chen Gang]

Excluding current discussion, the 2 patches (patch 1/2, patch 2/2) for
arm are both OK?

Thanks.

On 10/05/2013 08:11 AM, Chen Gang wrote:
> On 10/05/2013 07:55 AM, Chen Gang wrote:
>> On 10/04/2013 11:42 PM, Will Deacon wrote:
>>> On Fri, Oct 04, 2013 at 04:37:42PM +0100, Will Deacon wrote:
>>>> On Fri, Oct 04, 2013 at 10:51:56AM +0100, Chen Gang wrote:
>>>>> On 10/04/2013 12:32 AM, Will Deacon wrote:
>>>>>> The problem with arm64 is that we're using *unsigned long* for 32-bit
>>>>>> clear_mask, which is definitely wrong because it's 64-bit (another patch to
>>>>>> fix this!).
>>>>>>
>>>>>
>>>>> At least, that is not a bug.
>>>>
>>>> Sure it is. What if the adjacent 32-bit value was being accessed by another
>>>> CPU under a spinlock?
>>>
>>> (Oh, ok, that would still work on arm because of the way the exclusive
>>> monitor is implemented, but we shouldn't rely on that).
>>>
>>
>> Hmm... in my opinion, we need divide atomic_*_mask() into 32-bit and
>> 64-bit versions. We already have demands to only use 32-bit value to
>> express mask (can save size), and may have demands to use 64-bit too.
>>
>> If so, can easily standard all atomic_*_mask() which are in asm-generic
>> and various architectures: use atomic_*_mask() for 32-bit mask, and use
>> atomic64_*_mask() for 64-bit mask (can delayed before get real demands).
>>
>> Since it is about API, so it is related with asm-generic and all other
>> architectures interfaces, it is better to consider about them firstly
>> before the fix under arm64.
>>
>>
>> And also, excuse me, I am not quite familiar with "exclusive monitor",
>> could you please provide more details about it?
>>
> 
> If in arm64 with allmodconfig, we only need atomic_*_mask() for 32-bit
> mask, don't need 64-bit, we can use 'unsigned int' instead of 'unsigned
> long' (need not consider about asm-generic and other architectures).
> 
> 
> 
>>
>>> Will
>>>
>>>
>>
>>
>> Thanks.
>>
> 
> 


-- 
Chen Gang

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Will Deacon]

On Sat, Oct 05, 2013 at 12:55:18AM +0100, Chen Gang wrote:
> On 10/04/2013 11:42 PM, Will Deacon wrote:
> > On Fri, Oct 04, 2013 at 04:37:42PM +0100, Will Deacon wrote:
> >> On Fri, Oct 04, 2013 at 10:51:56AM +0100, Chen Gang wrote:
> >>> On 10/04/2013 12:32 AM, Will Deacon wrote:
> >>>> The problem with arm64 is that we're using *unsigned long* for 32-bit
> >>>> clear_mask, which is definitely wrong because it's 64-bit (another patch to
> >>>> fix this!).
> >>>>
> >>>
> >>> At least, that is not a bug.
> >>
> >> Sure it is. What if the adjacent 32-bit value was being accessed by another
> >> CPU under a spinlock?
> > 
> > (Oh, ok, that would still work on arm because of the way the exclusive
> > monitor is implemented, but we shouldn't rely on that).
> >
> 
> Hmm... in my opinion, we need divide atomic_*_mask() into 32-bit and
> 64-bit versions. We already have demands to only use 32-bit value to
> express mask (can save size), and may have demands to use 64-bit too.

Right, but isn't this code only used by that s390 SCSI driver? Why do other
architectures even need to bother implementing it?

> And also, excuse me, I am not quite familiar with "exclusive monitor",
> could you please provide more details about it?

You'll need to take a look at the ARM ARM, in particular the section about
`Synchronisation and Semaphores'.

Will

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Will Deacon]

On Tue, Oct 08, 2013 at 05:10:12AM +0100, Chen Gang wrote:
> Excluding current discussion, the 2 patches (patch 1/2, patch 2/2) for
> arm are both OK?

I've kind of lost track. Could you send those patches again (as a fresh
series) please?

Will

[PATCH v2 0/2] ARM: include: asm: change functions' and variables' types in atomic.h

[Chen Gang]

It contents 2 patches:

Patch 1: use 'long long' instead of 'u64' for within atomic.h.

Patch 2: use 'int' instead of 'unsigned long' for 'oldval' in
atomic_cmpxchg().


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/arm/include/asm/atomic.h |   52 +++++++++++++++++++++-------------------
 1 files changed, 27 insertions(+), 25 deletions(-)

[PATCH v2 1/2] ARM: include: asm: use 'long long' instead of 'u64' within atomic.h

[Chen Gang]

atomic* value is signed value, and atomic* functions need also process
signed value (parameter value, and return value), so 32-bit arm need
use 'long long' instead of 'u64'.

After replacement, it will also fix a bug for atomic64_add_negative():
"u64 is never less than 0".

The modifications are:

  in vim, use "1,% s/\<u64\>/long long/g" command.
  remove '__aligned(8)' which is useless for 64-bit.
  be sure of 80 column limitation after replacement.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/arm/include/asm/atomic.h |   49 +++++++++++++++++++++--------------------
 1 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index da1c77d..a715ac0 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -238,15 +238,15 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u)
 
 #ifndef CONFIG_GENERIC_ATOMIC64
 typedef struct {
-	u64 __aligned(8) counter;
+	long long counter;
 } atomic64_t;
 
 #define ATOMIC64_INIT(i) { (i) }
 
 #ifdef CONFIG_ARM_LPAE
-static inline u64 atomic64_read(const atomic64_t *v)
+static inline long long atomic64_read(const atomic64_t *v)
 {
-	u64 result;
+	long long result;
 
 	__asm__ __volatile__("@ atomic64_read\n"
 "	ldrd	%0, %H0, [%1]"
@@ -257,7 +257,7 @@ static inline u64 atomic64_read(const atomic64_t *v)
 	return result;
 }
 
-static inline void atomic64_set(atomic64_t *v, u64 i)
+static inline void atomic64_set(atomic64_t *v, long long i)
 {
 	__asm__ __volatile__("@ atomic64_set\n"
 "	strd	%2, %H2, [%1]"
@@ -266,9 +266,9 @@ static inline void atomic64_set(atomic64_t *v, u64 i)
 	);
 }
 #else
-static inline u64 atomic64_read(const atomic64_t *v)
+static inline long long atomic64_read(const atomic64_t *v)
 {
-	u64 result;
+	long long result;
 
 	__asm__ __volatile__("@ atomic64_read\n"
 "	ldrexd	%0, %H0, [%1]"
@@ -279,9 +279,9 @@ static inline u64 atomic64_read(const atomic64_t *v)
 	return result;
 }
 
-static inline void atomic64_set(atomic64_t *v, u64 i)
+static inline void atomic64_set(atomic64_t *v, long long i)
 {
-	u64 tmp;
+	long long tmp;
 
 	__asm__ __volatile__("@ atomic64_set\n"
 "1:	ldrexd	%0, %H0, [%2]\n"
@@ -294,9 +294,9 @@ static inline void atomic64_set(atomic64_t *v, u64 i)
 }
 #endif
 
-static inline void atomic64_add(u64 i, atomic64_t *v)
+static inline void atomic64_add(long long i, atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	__asm__ __volatile__("@ atomic64_add\n"
@@ -311,9 +311,9 @@ static inline void atomic64_add(u64 i, atomic64_t *v)
 	: "cc");
 }
 
-static inline u64 atomic64_add_return(u64 i, atomic64_t *v)
+static inline long long atomic64_add_return(long long i, atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	smp_mb();
@@ -334,9 +334,9 @@ static inline u64 atomic64_add_return(u64 i, atomic64_t *v)
 	return result;
 }
 
-static inline void atomic64_sub(u64 i, atomic64_t *v)
+static inline void atomic64_sub(long long i, atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	__asm__ __volatile__("@ atomic64_sub\n"
@@ -351,9 +351,9 @@ static inline void atomic64_sub(u64 i, atomic64_t *v)
 	: "cc");
 }
 
-static inline u64 atomic64_sub_return(u64 i, atomic64_t *v)
+static inline long long atomic64_sub_return(long long i, atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	smp_mb();
@@ -374,9 +374,10 @@ static inline u64 atomic64_sub_return(u64 i, atomic64_t *v)
 	return result;
 }
 
-static inline u64 atomic64_cmpxchg(atomic64_t *ptr, u64 old, u64 new)
+static inline long long atomic64_cmpxchg(atomic64_t *ptr, long long old,
+					long long new)
 {
-	u64 oldval;
+	long long oldval;
 	unsigned long res;
 
 	smp_mb();
@@ -398,9 +399,9 @@ static inline u64 atomic64_cmpxchg(atomic64_t *ptr, u64 old, u64 new)
 	return oldval;
 }
 
-static inline u64 atomic64_xchg(atomic64_t *ptr, u64 new)
+static inline long long atomic64_xchg(atomic64_t *ptr, long long new)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	smp_mb();
@@ -419,9 +420,9 @@ static inline u64 atomic64_xchg(atomic64_t *ptr, u64 new)
 	return result;
 }
 
-static inline u64 atomic64_dec_if_positive(atomic64_t *v)
+static inline long long atomic64_dec_if_positive(atomic64_t *v)
 {
-	u64 result;
+	long long result;
 	unsigned long tmp;
 
 	smp_mb();
@@ -445,9 +446,9 @@ static inline u64 atomic64_dec_if_positive(atomic64_t *v)
 	return result;
 }
 
-static inline int atomic64_add_unless(atomic64_t *v, u64 a, u64 u)
+static inline int atomic64_add_unless(atomic64_t *v, long long a, long long u)
 {
-	u64 val;
+	long long val;
 	unsigned long tmp;
 	int ret = 1;
 
-- 
1.7.7.6

[PATCH v2 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for 'oldval' in atomic_cmpxchg().

[Chen Gang]

For atomic_cmpxchg(), the type of 'oldval' need be 'int' to match the
type of "*ptr" (used by 'ldrex' instruction) and 'old' (used by 'teq'
instruction).

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/arm/include/asm/atomic.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index a715ac0..9ee7e01 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -114,7 +114,8 @@ static inline int atomic_sub_return(int i, atomic_t *v)
 
 static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
 {
-	unsigned long oldval, res;
+	int oldval;
+	unsigned long res;
 
 	smp_mb();
 
-- 
1.7.7.6

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Chen Gang]

On 10/08/2013 06:34 PM, Will Deacon wrote:
> On Tue, Oct 08, 2013 at 05:10:12AM +0100, Chen Gang wrote:
>> Excluding current discussion, the 2 patches (patch 1/2, patch 2/2) for
>> arm are both OK?
> 
> I've kind of lost track. Could you send those patches again (as a fresh
> series) please?
> 

OK, thanks. I send it, and please check.

> Will
> 
> 

Thanks.
-- 
Chen Gang

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Chen Gang]

On 10/08/2013 06:33 PM, Will Deacon wrote:
> On Sat, Oct 05, 2013 at 12:55:18AM +0100, Chen Gang wrote:
>> On 10/04/2013 11:42 PM, Will Deacon wrote:
>>> On Fri, Oct 04, 2013 at 04:37:42PM +0100, Will Deacon wrote:
>>>> On Fri, Oct 04, 2013 at 10:51:56AM +0100, Chen Gang wrote:
>>>>> On 10/04/2013 12:32 AM, Will Deacon wrote:
>>>>>> The problem with arm64 is that we're using *unsigned long* for 32-bit
>>>>>> clear_mask, which is definitely wrong because it's 64-bit (another patch to
>>>>>> fix this!).
>>>>>>
>>>>>
>>>>> At least, that is not a bug.
>>>>
>>>> Sure it is. What if the adjacent 32-bit value was being accessed by another
>>>> CPU under a spinlock?
>>>
>>> (Oh, ok, that would still work on arm because of the way the exclusive
>>> monitor is implemented, but we shouldn't rely on that).
>>>
>>
>> Hmm... in my opinion, we need divide atomic_*_mask() into 32-bit and
>> 64-bit versions. We already have demands to only use 32-bit value to
>> express mask (can save size), and may have demands to use 64-bit too.
> 
> Right, but isn't this code only used by that s390 SCSI driver? Why do other
> architectures even need to bother implementing it?
> 

Hmm... for atomic_set_mask(), "drivers/gpu/drm/i915/i915_irq.c" also
uses it (it is about Intel Graphics).

For some architectures (e.g. frv, mn10300, mr32r, blackfin, and of cause
s390), still use atomic_[set/clear]_mask().

And they are inline functions in atomic.h, individual kernel modules may
use them (which source code is not merged into kernel). Can we skip
these individual modules? (I guess we can, but need think of carefully).


>> And also, excuse me, I am not quite familiar with "exclusive monitor",
>> could you please provide more details about it?
> 
> You'll need to take a look at the ARM ARM, in particular the section about
> `Synchronisation and Semaphores'.
> 

Do you mean "ARM ARM" is a book name? (at least, I really need read more
things to familiar with ARM)


> Will
> 
> 

Thanks.
-- 
Chen Gang

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Will Deacon]

On Tue, Oct 08, 2013 at 12:29:14PM +0100, Chen Gang wrote:
> On 10/08/2013 06:33 PM, Will Deacon wrote:
> > On Sat, Oct 05, 2013 at 12:55:18AM +0100, Chen Gang wrote:
> >> Hmm... in my opinion, we need divide atomic_*_mask() into 32-bit and
> >> 64-bit versions. We already have demands to only use 32-bit value to
> >> express mask (can save size), and may have demands to use 64-bit too.
> > 
> > Right, but isn't this code only used by that s390 SCSI driver? Why do other
> > architectures even need to bother implementing it?
> > 
> Hmm... for atomic_set_mask(), "drivers/gpu/drm/i915/i915_irq.c" also
> uses it (it is about Intel Graphics).

I don't think we care too much about the Intel graphics driver on ARM at the
moment.

> For some architectures (e.g. frv, mn10300, mr32r, blackfin, and of cause
> s390), still use atomic_[set/clear]_mask().

We don't care about those either.

> And they are inline functions in atomic.h, individual kernel modules may
> use them (which source code is not merged into kernel). Can we skip
> these individual modules? (I guess we can, but need think of carefully).

or those. If somebody starts complaining, then we can add things back.

> >> And also, excuse me, I am not quite familiar with "exclusive monitor",
> >> could you please provide more details about it?
> > 
> > You'll need to take a look at the ARM ARM, in particular the section about
> > `Synchronisation and Semaphores'.
> > 
> 
> Do you mean "ARM ARM" is a book name? (at least, I really need read more
> things to familiar with ARM)

Yes, the hilariouly titled "ARM Architecture Reference Manual". You can find
it on arm.com, but you have to register:

ARMv7: https://silver.arm.com/download/download.tm?pv=1299246
ARMv8 (beta): https://silver.arm.com/download/download.tm?pv=1448511

Enjoy!

Will

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Chen Gang]

On 10/09/2013 01:49 AM, Will Deacon wrote:
> On Tue, Oct 08, 2013 at 12:29:14PM +0100, Chen Gang wrote:
>> Hmm... for atomic_set_mask(), "drivers/gpu/drm/i915/i915_irq.c" also
>> uses it (it is about Intel Graphics).
> 
> I don't think we care too much about the Intel graphics driver on ARM at the
> moment.
> 
>> For some architectures (e.g. frv, mn10300, mr32r, blackfin, and of cause
>> s390), still use atomic_[set/clear]_mask().
> 
> We don't care about those either.
> 
>> And they are inline functions in atomic.h, individual kernel modules may
>> use them (which source code is not merged into kernel). Can we skip
>> these individual modules? (I guess we can, but need think of carefully).
> 
> or those. If somebody starts complaining, then we can add things back.
> 

OK, thanks. For s390, it need use 'unsigned int' instead of 'unsigned
long' (which already acked by related maintainers).

So at least, we can use "unsigned int" instead of "unsigned long" for
our arm64. :-)

I will send related patch for arm64 (also re-send related patch for s390).

Thanks.

>>>> And also, excuse me, I am not quite familiar with "exclusive monitor",
>>>> could you please provide more details about it?
>>>
>>> You'll need to take a look at the ARM ARM, in particular the section about
>>> `Synchronisation and Semaphores'.
>>>
>>
>> Do you mean "ARM ARM" is a book name? (at least, I really need read more
>> things to familiar with ARM)
> 
> Yes, the hilariouly titled "ARM Architecture Reference Manual". You can find
> it on arm.com, but you have to register:
> 
> ARMv7: https://silver.arm.com/download/download.tm?pv=1299246
> ARMv8 (beta): https://silver.arm.com/download/download.tm?pv=1448511
> 

Thank you very much for your information.

> Enjoy!
> 
> Will
> 
> 

Thanks.
-- 
Chen Gang

[PATCH 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for normal register variables within atomic.h

[Chen Gang]

On 10/09/2013 08:18 AM, Chen Gang wrote:
> On 10/09/2013 01:49 AM, Will Deacon wrote:
>> On Tue, Oct 08, 2013 at 12:29:14PM +0100, Chen Gang wrote:
>>> Hmm... for atomic_set_mask(), "drivers/gpu/drm/i915/i915_irq.c" also
>>> uses it (it is about Intel Graphics).
>>
>> I don't think we care too much about the Intel graphics driver on ARM at the
>> moment.
>>
>>> For some architectures (e.g. frv, mn10300, mr32r, blackfin, and of cause
>>> s390), still use atomic_[set/clear]_mask().
>>
>> We don't care about those either.
>>
>>> And they are inline functions in atomic.h, individual kernel modules may
>>> use them (which source code is not merged into kernel). Can we skip
>>> these individual modules? (I guess we can, but need think of carefully).
>>
>> or those. If somebody starts complaining, then we can add things back.
>>
> 
> OK, thanks. For s390, it need use 'unsigned int' instead of 'unsigned
> long' (which already acked by related maintainers).
> 
> So at least, we can use "unsigned int" instead of "unsigned long" for
> our arm64. :-)
> 
> I will send related patch for arm64 (also re-send related patch for s390).
> 

For arm, we still can do like this (but recommend after the 2 current
patches applied).

For arm64, related inline assembly code also need be changed  (for
'bic', 'ldxr' and 'stxr', need use '%w0' instead of '%0', and  '%w3'
instead of '%3'), and 'tmp' also need be changed to unsigned int.


Thanks.

> Thanks.
> 
>>>>> And also, excuse me, I am not quite familiar with "exclusive monitor",
>>>>> could you please provide more details about it?
>>>>
>>>> You'll need to take a look at the ARM ARM, in particular the section about
>>>> `Synchronisation and Semaphores'.
>>>>
>>>
>>> Do you mean "ARM ARM" is a book name? (at least, I really need read more
>>> things to familiar with ARM)
>>
>> Yes, the hilariouly titled "ARM Architecture Reference Manual". You can find
>> it on arm.com, but you have to register:
>>
>> ARMv7: https://silver.arm.com/download/download.tm?pv=1299246
>> ARMv8 (beta): https://silver.arm.com/download/download.tm?pv=1448511
>>
> 
> Thank you very much for your information.
> 
>> Enjoy!
>>
>> Will
>>
>>
> 
> Thanks.
> 


-- 
Chen Gang

[PATCH v2 1/2] ARM: include: asm: use 'long long' instead of 'u64' within atomic.h

[Will Deacon]

On Tue, Oct 08, 2013 at 11:57:55AM +0100, Chen Gang wrote:
> atomic* value is signed value, and atomic* functions need also process
> signed value (parameter value, and return value), so 32-bit arm need
> use 'long long' instead of 'u64'.
> 
> After replacement, it will also fix a bug for atomic64_add_negative():
> "u64 is never less than 0".
> 
> The modifications are:
> 
>   in vim, use "1,% s/\<u64\>/long long/g" command.
>   remove '__aligned(8)' which is useless for 64-bit.
>   be sure of 80 column limitation after replacement.
> 

Looks fine by me.

  Acked-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH v2 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for 'oldval' in atomic_cmpxchg().

[Will Deacon]

On Tue, Oct 08, 2013 at 11:59:15AM +0100, Chen Gang wrote:
> For atomic_cmpxchg(), the type of 'oldval' need be 'int' to match the
> type of "*ptr" (used by 'ldrex' instruction) and 'old' (used by 'teq'
> instruction).
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  arch/arm/include/asm/atomic.h |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
> index a715ac0..9ee7e01 100644
> --- a/arch/arm/include/asm/atomic.h
> +++ b/arch/arm/include/asm/atomic.h
> @@ -114,7 +114,8 @@ static inline int atomic_sub_return(int i, atomic_t *v)
>  
>  static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>  {
> -	unsigned long oldval, res;
> +	int oldval;
> +	unsigned long res;
>  
>  	smp_mb();

As discussed, this is completely cosmetic, but does at least keep the file
consistent.

  Reviewed-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH v2 2/2] ARM: include: asm: use 'int' instead of 'unsigned long' for 'oldval' in atomic_cmpxchg().

[Chen Gang]

On 10/09/2013 06:48 PM, Will Deacon wrote:
> On Tue, Oct 08, 2013 at 11:59:15AM +0100, Chen Gang wrote:
>> For atomic_cmpxchg(), the type of 'oldval' need be 'int' to match the
>> type of "*ptr" (used by 'ldrex' instruction) and 'old' (used by 'teq'
>> instruction).
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  arch/arm/include/asm/atomic.h |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
>> index a715ac0..9ee7e01 100644
>> --- a/arch/arm/include/asm/atomic.h
>> +++ b/arch/arm/include/asm/atomic.h
>> @@ -114,7 +114,8 @@ static inline int atomic_sub_return(int i, atomic_t *v)
>>  
>>  static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>>  {
>> -	unsigned long oldval, res;
>> +	int oldval;
>> +	unsigned long res;
>>  
>>  	smp_mb();
> 
> As discussed, this is completely cosmetic, but does at least keep the file
> consistent.
> 
>   Reviewed-by: Will Deacon <will.deacon@arm.com>
> 

Hmm... it seems better to let you as "Signed-of-by" too, for this is
mainly found by you. :-)

And I feel, these kinds of files are the dresses of 'arm32/64' (just
like "asm-generic/*" are the dresses of 'arch'), so not only consider
about whether useful, but also need consider about whether clean.

> Will
> 
> 

Thanks.
-- 
Chen Gang