[PATCH 0/3] s390/arm/arm64: include: asm: atomic.h: use 'unsigned int' and 'atomic_t' instead of 'unsigned long' for atomic_*_mask()

[PATCH 0/3] s390/arm/arm64: include: asm: atomic.h: use 'unsigned int' and 'atomic_t' instead of 'unsigned long' for atomic_*_mask()

[Chen Gang]

For atomic_*_mask(), the 'atomic_t' is 32-bit, so for the 'mask', also
need mach it.


Patch 1/3: s390: include: asm: atomic.h: use 'unsigned int' instead of
'unsigned long' for atomic_*_mask().

Patch 2/3: arm: include: asm: atomic.h: use 'unsigned int' and 'atomic'
instead of 'unsigned long' for atomic_clear_mask().

Patch 3/3: arm64: include: asm: atomic.h: use 'unsigned int' and
'atomic_t' instead of 'unsigned long' for atomic_clear_mask().


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

[PATCH 1/3] s390: include: asm: atomic.h: use 'unsigned int' instead of 'unsigned long' for atomic_*_mask()

[Chen Gang]

The type of 'v->counter' is always 'int', and related inline assembly
code also process 'int', so use 'unsigned int' instead of 'unsigned
long' for the 'mask'.


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

diff --git a/arch/s390/include/asm/atomic.h b/arch/s390/include/asm/atomic.h
index a62ed2d..12c5ec1 100644
--- a/arch/s390/include/asm/atomic.h
+++ b/arch/s390/include/asm/atomic.h
@@ -113,12 +113,12 @@ static inline void atomic_add(int i, atomic_t *v)
 #define atomic_dec_return(_v)		atomic_sub_return(1, _v)
 #define atomic_dec_and_test(_v)		(atomic_sub_return(1, _v) == 0)
 
-static inline void atomic_clear_mask(unsigned long mask, atomic_t *v)
+static inline void atomic_clear_mask(unsigned int mask, atomic_t *v)
 {
 	__ATOMIC_LOOP(v, ~mask, __ATOMIC_AND);
 }
 
-static inline void atomic_set_mask(unsigned long mask, atomic_t *v)
+static inline void atomic_set_mask(unsigned int mask, atomic_t *v)
 {
 	__ATOMIC_LOOP(v, mask, __ATOMIC_OR);
 }
-- 
1.7.7.6

[PATCH 2/3] arm: include: asm: atomic.h: use 'unsigned int' and 'atomic' instead of 'unsigned long' for atomic_clear_mask()

[Chen Gang]

In current kernel wide source, for arm, only s390 scsi drivers use
atomic_clear_mask(), now, s390 itself need use 'unsigned int' and
'atomic_t', so need match s390's atomic_clear_mask().


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

diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index da1c77d..0832a7f 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -134,9 +134,10 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
 	return oldval;
 }
 
-static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
+static inline void atomic_clear_mask(unsigned int mask, atomic_t *ptr)
 {
-	unsigned long tmp, tmp2;
+	unsigned int tmp;
+	unsigned long tmp2;
 
 	__asm__ __volatile__("@ atomic_clear_mask\n"
 "1:	ldrex	%0, [%3]\n"
@@ -144,8 +145,8 @@ static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
 "	strex	%1, %0, [%3]\n"
 "	teq	%1, #0\n"
 "	bne	1b"
-	: "=&r" (tmp), "=&r" (tmp2), "+Qo" (*addr)
-	: "r" (addr), "Ir" (mask)
+	: "=&r" (tmp), "=&r" (tmp2), "+Qo" (ptr->counter)
+	: "r" (&ptr->counter), "Ir" (mask)
 	: "cc");
 }
 
@@ -197,12 +198,12 @@ static inline int atomic_cmpxchg(atomic_t *v, int old, int new)
 	return ret;
 }
 
-static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
+static inline void atomic_clear_mask(unsigned int mask, atomic_t *v)
 {
 	unsigned long flags;
 
 	raw_local_irq_save(flags);
-	*addr &= ~mask;
+	v->counter &= ~mask;
 	raw_local_irq_restore(flags);
 }
 
-- 
1.7.7.6

[PATCH 3/3] arm64: include: asm: atomic.h: use 'unsigned int' and 'atomic_t' instead of 'unsigned long' for atomic_clear_mask()

[Chen Gang]

In current kernel wide source, for arm64, only s390 scsi drivers use
atomic_clear_mask(), now, s390 itself need use 'unsigned int' and
'atomic_t', so need match s390's atomic_clear_mask().


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

diff --git a/arch/arm64/include/asm/atomic.h b/arch/arm64/include/asm/atomic.h
index 8363644..58808fc 100644
--- a/arch/arm64/include/asm/atomic.h
+++ b/arch/arm64/include/asm/atomic.h
@@ -126,16 +126,17 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
 	return oldval;
 }
 
-static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
+static inline void atomic_clear_mask(unsigned int mask, atomic_t *ptr)
 {
-	unsigned long tmp, tmp2;
+	unsigned int tmp;
+	unsigned long tmp2;
 
 	asm volatile("// atomic_clear_mask\n"
-"1:	ldxr	%0, %2\n"
-"	bic	%0, %0, %3\n"
-"	stxr	%w1, %0, %2\n"
+"1:	ldxr	%w0, %2\n"
+"	bic	%w0, %w0, %w3\n"
+"	stxr	%w1, %w0, %2\n"
 "	cbnz	%w1, 1b"
-	: "=&r" (tmp), "=&r" (tmp2), "+Q" (*addr)
+	: "=&r" (tmp), "=&r" (tmp2), "+Q" (ptr->counter)
 	: "Ir" (mask)
 	: "cc");
 }
-- 
1.7.7.6

Re: [PATCH 3/3] arm64: include: asm: atomic.h: use 'unsigned int' and 'atomic_t' instead of 'unsigned long' for atomic_clear_mask()

[Will Deacon]

On Thu, Oct 10, 2013 at 03:35:21AM +0100, Chen Gang wrote:
> In current kernel wide source, for arm64, only s390 scsi drivers use
> atomic_clear_mask(), now, s390 itself need use 'unsigned int' and
> 'atomic_t', so need match s390's atomic_clear_mask().
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  arch/arm64/include/asm/atomic.h |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/atomic.h b/arch/arm64/include/asm/atomic.h
> index 8363644..58808fc 100644
> --- a/arch/arm64/include/asm/atomic.h
> +++ b/arch/arm64/include/asm/atomic.h
> @@ -126,16 +126,17 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>  	return oldval;
>  }
>  
> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
> +static inline void atomic_clear_mask(unsigned int mask, atomic_t *ptr)
>  {
> -	unsigned long tmp, tmp2;
> +	unsigned int tmp;

Same comment here as for ARM; I think you want a signed int.

Will

Re: [PATCH 3/3] arm64: include: asm: atomic.h: use 'unsigned int' and 'atomic_t' instead of 'unsigned long' for atomic_clear_mask()

[Chen Gang]

On 10/10/2013 06:07 PM, Will Deacon wrote:
> On Thu, Oct 10, 2013 at 03:35:21AM +0100, Chen Gang wrote:
>> In current kernel wide source, for arm64, only s390 scsi drivers use
>> atomic_clear_mask(), now, s390 itself need use 'unsigned int' and
>> 'atomic_t', so need match s390's atomic_clear_mask().
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  arch/arm64/include/asm/atomic.h |   13 +++++++------
>>  1 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/atomic.h b/arch/arm64/include/asm/atomic.h
>> index 8363644..58808fc 100644
>> --- a/arch/arm64/include/asm/atomic.h
>> +++ b/arch/arm64/include/asm/atomic.h
>> @@ -126,16 +126,17 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>>  	return oldval;
>>  }
>>  
>> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
>> +static inline void atomic_clear_mask(unsigned int mask, atomic_t *ptr)
>>  {
>> -	unsigned long tmp, tmp2;
>> +	unsigned int tmp;
> 
> Same comment here as for ARM; I think you want a signed int.
> 

OK, replied in patch 2/3 for ARM.

BTW: do arm64 need atomic_clear_mask()?


> Will
> 
> 

Thanks.
-- 
Chen Gang

Re: [PATCH 3/3] arm64: include: asm: atomic.h: use 'unsigned int' and 'atomic_t' instead of 'unsigned long' for atomic_clear_mask()

[Will Deacon]

On Thu, Oct 10, 2013 at 12:03:52PM +0100, Chen Gang wrote:
> On 10/10/2013 06:07 PM, Will Deacon wrote:
> > On Thu, Oct 10, 2013 at 03:35:21AM +0100, Chen Gang wrote:
> >> In current kernel wide source, for arm64, only s390 scsi drivers use
> >> atomic_clear_mask(), now, s390 itself need use 'unsigned int' and
> >> 'atomic_t', so need match s390's atomic_clear_mask().
> >>
> >>
> >> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> >> ---
> >>  arch/arm64/include/asm/atomic.h |   13 +++++++------
> >>  1 files changed, 7 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/arm64/include/asm/atomic.h b/arch/arm64/include/asm/atomic.h
> >> index 8363644..58808fc 100644
> >> --- a/arch/arm64/include/asm/atomic.h
> >> +++ b/arch/arm64/include/asm/atomic.h
> >> @@ -126,16 +126,17 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
> >>  	return oldval;
> >>  }
> >>  
> >> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
> >> +static inline void atomic_clear_mask(unsigned int mask, atomic_t *ptr)
> >>  {
> >> -	unsigned long tmp, tmp2;
> >> +	unsigned int tmp;
> > 
> > Same comment here as for ARM; I think you want a signed int.
> > 
> 
> OK, replied in patch 2/3 for ARM.
> 
> BTW: do arm64 need atomic_clear_mask()?

No. Neither ARM nor arm64 need this function.

Will

Re: [PATCH 3/3] arm64: include: asm: atomic.h: use 'unsigned int' and 'atomic_t' instead of 'unsigned long' for atomic_clear_mask()

[Chen Gang]

On 10/10/2013 10:23 PM, Will Deacon wrote:
> On Thu, Oct 10, 2013 at 12:03:52PM +0100, Chen Gang wrote:
>> On 10/10/2013 06:07 PM, Will Deacon wrote:
>>> On Thu, Oct 10, 2013 at 03:35:21AM +0100, Chen Gang wrote:
>>>> In current kernel wide source, for arm64, only s390 scsi drivers use
>>>> atomic_clear_mask(), now, s390 itself need use 'unsigned int' and
>>>> 'atomic_t', so need match s390's atomic_clear_mask().
>>>>
>>>>
>>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>>> ---
>>>>  arch/arm64/include/asm/atomic.h |   13 +++++++------
>>>>  1 files changed, 7 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/atomic.h b/arch/arm64/include/asm/atomic.h
>>>> index 8363644..58808fc 100644
>>>> --- a/arch/arm64/include/asm/atomic.h
>>>> +++ b/arch/arm64/include/asm/atomic.h
>>>> @@ -126,16 +126,17 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>>>>  	return oldval;
>>>>  }
>>>>  
>>>> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
>>>> +static inline void atomic_clear_mask(unsigned int mask, atomic_t *ptr)
>>>>  {
>>>> -	unsigned long tmp, tmp2;
>>>> +	unsigned int tmp;
>>>
>>> Same comment here as for ARM; I think you want a signed int.
>>>
>>
>> OK, replied in patch 2/3 for ARM.
>>
>> BTW: do arm64 need atomic_clear_mask()?
> 
> No. Neither ARM nor arm64 need this function.
> 

OK, thank you for your confirmation.

Hmm... can we remove atomic_clear_mask() from ARM and arm64? (in my
opinion, if not need, better to remove it).

> Will
> 
> 

Thanks.
-- 
Chen Gang

Re: [PATCH 3/3] arm64: include: asm: atomic.h: use 'unsigned int' and 'atomic_t' instead of 'unsigned long' for atomic_clear_mask()

[Will Deacon]

On Fri, Oct 11, 2013 at 02:18:26AM +0100, Chen Gang wrote:
> On 10/10/2013 10:23 PM, Will Deacon wrote:
> > On Thu, Oct 10, 2013 at 12:03:52PM +0100, Chen Gang wrote:
> >> BTW: do arm64 need atomic_clear_mask()?
> > 
> > No. Neither ARM nor arm64 need this function.
> 
> OK, thank you for your confirmation.
> 
> Hmm... can we remove atomic_clear_mask() from ARM and arm64? (in my
> opinion, if not need, better to remove it).

Yes!

I mentioned this a few times already...

Will

Re: [PATCH 3/3] arm64: include: asm: atomic.h: use 'unsigned int' and 'atomic_t' instead of 'unsigned long' for atomic_clear_mask()

[Chen Gang]

On 10/11/2013 06:44 PM, Will Deacon wrote:
> On Fri, Oct 11, 2013 at 02:18:26AM +0100, Chen Gang wrote:
>> On 10/10/2013 10:23 PM, Will Deacon wrote:
>>> On Thu, Oct 10, 2013 at 12:03:52PM +0100, Chen Gang wrote:
>>>> BTW: do arm64 need atomic_clear_mask()?
>>>
>>> No. Neither ARM nor arm64 need this function.
>>
>> OK, thank you for your confirmation.
>>
>> Hmm... can we remove atomic_clear_mask() from ARM and arm64? (in my
>> opinion, if not need, better to remove it).
> 
> Yes!
> 

OK, thanks.

> I mentioned this a few times already...
> 

Hmm... firstly, you mentioned "except architectures, this is only used
under s390" (so I reply we need follow s390), and then you mentioned
"they are useless for arm/arm64" (so I ask whether it can be removed).

Excuse me, as a 'programmer' (at least, I am a 'programmer'), when
programming (assume our discussing belongs to programming), I have to
consider precisely (especially for confirmation words).  ;-)

I will send one patch for removing it from arm and arm64.

Thanks.

> Will
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


Thanks.
-- 
Chen Gang

-- 
Chen Gang

[PATCH] arm/arm64: remove atomic_clear_mask() in "include/asm/atomic.h"

[Chen Gang]

In current kernel wide source code, except other architectures, only
s390 scsi drivers use atomic_clear_mask(), and arm/arm64 need not
support s390 drivers.

So remove atomic_clear_mask() from "arm[64]/include/asm/atomic.h".


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

diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index da1c77d..3b3ae49fa 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -134,21 +134,6 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
 	return oldval;
 }
 
-static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
-{
-	unsigned long tmp, tmp2;
-
-	__asm__ __volatile__("@ atomic_clear_mask\n"
-"1:	ldrex	%0, [%3]\n"
-"	bic	%0, %0, %4\n"
-"	strex	%1, %0, [%3]\n"
-"	teq	%1, #0\n"
-"	bne	1b"
-	: "=&r" (tmp), "=&r" (tmp2), "+Qo" (*addr)
-	: "r" (addr), "Ir" (mask)
-	: "cc");
-}
-
 #else /* ARM_ARCH_6 */
 
 #ifdef CONFIG_SMP
@@ -197,15 +182,6 @@ static inline int atomic_cmpxchg(atomic_t *v, int old, int new)
 	return ret;
 }
 
-static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
-{
-	unsigned long flags;
-
-	raw_local_irq_save(flags);
-	*addr &= ~mask;
-	raw_local_irq_restore(flags);
-}
-
 #endif /* __LINUX_ARM_ARCH__ */
 
 #define atomic_xchg(v, new) (xchg(&((v)->counter), new))
diff --git a/arch/arm64/include/asm/atomic.h b/arch/arm64/include/asm/atomic.h
index 8363644..01de5aa 100644
--- a/arch/arm64/include/asm/atomic.h
+++ b/arch/arm64/include/asm/atomic.h
@@ -126,20 +126,6 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
 	return oldval;
 }
 
-static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
-{
-	unsigned long tmp, tmp2;
-
-	asm volatile("// atomic_clear_mask\n"
-"1:	ldxr	%0, %2\n"
-"	bic	%0, %0, %3\n"
-"	stxr	%w1, %0, %2\n"
-"	cbnz	%w1, 1b"
-	: "=&r" (tmp), "=&r" (tmp2), "+Q" (*addr)
-	: "Ir" (mask)
-	: "cc");
-}
-
 #define atomic_xchg(v, new) (xchg(&((v)->counter), new))
 
 static inline int __atomic_add_unless(atomic_t *v, int a, int u)
-- 
1.7.7.6

Re: [PATCH] arm/arm64: remove atomic_clear_mask() in "include/asm/atomic.h"

[Richard Weinberger]

On Fri, Oct 11, 2013 at 1:47 PM, Chen Gang <gang.chen@asianux.com> wrote:
> In current kernel wide source code, except other architectures, only
> s390 scsi drivers use atomic_clear_mask(), and arm/arm64 need not
> support s390 drivers.
>
> So remove atomic_clear_mask() from "arm[64]/include/asm/atomic.h".

Is it really worth removing such a primitive?
If someone needs it later he has to implement it from scratch and
introduces bugs...

>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  arch/arm/include/asm/atomic.h   |   24 ------------------------
>  arch/arm64/include/asm/atomic.h |   14 --------------
>  2 files changed, 0 insertions(+), 38 deletions(-)
>
> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
> index da1c77d..3b3ae49fa 100644
> --- a/arch/arm/include/asm/atomic.h
> +++ b/arch/arm/include/asm/atomic.h
> @@ -134,21 +134,6 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>         return oldval;
>  }
>
> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
> -{
> -       unsigned long tmp, tmp2;
> -
> -       __asm__ __volatile__("@ atomic_clear_mask\n"
> -"1:    ldrex   %0, [%3]\n"
> -"      bic     %0, %0, %4\n"
> -"      strex   %1, %0, [%3]\n"
> -"      teq     %1, #0\n"
> -"      bne     1b"
> -       : "=&r" (tmp), "=&r" (tmp2), "+Qo" (*addr)
> -       : "r" (addr), "Ir" (mask)
> -       : "cc");
> -}
> -
>  #else /* ARM_ARCH_6 */
>
>  #ifdef CONFIG_SMP
> @@ -197,15 +182,6 @@ static inline int atomic_cmpxchg(atomic_t *v, int old, int new)
>         return ret;
>  }
>
> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
> -{
> -       unsigned long flags;
> -
> -       raw_local_irq_save(flags);
> -       *addr &= ~mask;
> -       raw_local_irq_restore(flags);
> -}
> -
>  #endif /* __LINUX_ARM_ARCH__ */
>
>  #define atomic_xchg(v, new) (xchg(&((v)->counter), new))
> diff --git a/arch/arm64/include/asm/atomic.h b/arch/arm64/include/asm/atomic.h
> index 8363644..01de5aa 100644
> --- a/arch/arm64/include/asm/atomic.h
> +++ b/arch/arm64/include/asm/atomic.h
> @@ -126,20 +126,6 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>         return oldval;
>  }
>
> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
> -{
> -       unsigned long tmp, tmp2;
> -
> -       asm volatile("// atomic_clear_mask\n"
> -"1:    ldxr    %0, %2\n"
> -"      bic     %0, %0, %3\n"
> -"      stxr    %w1, %0, %2\n"
> -"      cbnz    %w1, 1b"
> -       : "=&r" (tmp), "=&r" (tmp2), "+Q" (*addr)
> -       : "Ir" (mask)
> -       : "cc");
> -}
> -
>  #define atomic_xchg(v, new) (xchg(&((v)->counter), new))
>
>  static inline int __atomic_add_unless(atomic_t *v, int a, int u)
> --
> 1.7.7.6
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Thanks,
//richard

Re: [PATCH] arm/arm64: remove atomic_clear_mask() in "include/asm/atomic.h"

[Will Deacon]

On Fri, Oct 11, 2013 at 01:08:17PM +0100, Richard Weinberger wrote:
> On Fri, Oct 11, 2013 at 1:47 PM, Chen Gang <gang.chen@asianux.com> wrote:
> > In current kernel wide source code, except other architectures, only
> > s390 scsi drivers use atomic_clear_mask(), and arm/arm64 need not
> > support s390 drivers.
> >
> > So remove atomic_clear_mask() from "arm[64]/include/asm/atomic.h".
> 
> Is it really worth removing such a primitive?
> If someone needs it later he has to implement it from scratch and
> introduces bugs...

The version we have (on ARM64 anyway) already has bugs. Given the choice
between fixing code that has no callers and simply removing it, I'd go for
the latter.

Will

Re: [PATCH] arm/arm64: remove atomic_clear_mask() in "include/asm/atomic.h"

[Richard Weinberger]

Am 11.10.2013 14:28, schrieb Will Deacon:
> On Fri, Oct 11, 2013 at 01:08:17PM +0100, Richard Weinberger wrote:
>> On Fri, Oct 11, 2013 at 1:47 PM, Chen Gang <gang.chen@asianux.com> wrote:
>>> In current kernel wide source code, except other architectures, only
>>> s390 scsi drivers use atomic_clear_mask(), and arm/arm64 need not
>>> support s390 drivers.
>>>
>>> So remove atomic_clear_mask() from "arm[64]/include/asm/atomic.h".
>>
>> Is it really worth removing such a primitive?
>> If someone needs it later he has to implement it from scratch and
>> introduces bugs...
> 
> The version we have (on ARM64 anyway) already has bugs. Given the choice
> between fixing code that has no callers and simply removing it, I'd go for
> the latter.

Yeah, if it's broken and has no real users, send it to hell. :)

Thanks,
//richard

Re: [PATCH] arm/arm64: remove atomic_clear_mask() in "include/asm/atomic.h"

[Chen Gang]

On 10/11/2013 09:03 PM, Richard Weinberger wrote:
> Am 11.10.2013 14:28, schrieb Will Deacon:
>> On Fri, Oct 11, 2013 at 01:08:17PM +0100, Richard Weinberger wrote:
>>> On Fri, Oct 11, 2013 at 1:47 PM, Chen Gang <gang.chen@asianux.com> wrote:
>>>> In current kernel wide source code, except other architectures, only
>>>> s390 scsi drivers use atomic_clear_mask(), and arm/arm64 need not
>>>> support s390 drivers.
>>>>
>>>> So remove atomic_clear_mask() from "arm[64]/include/asm/atomic.h".
>>>
>>> Is it really worth removing such a primitive?
>>> If someone needs it later he has to implement it from scratch and
>>> introduces bugs...
>>
>> The version we have (on ARM64 anyway) already has bugs. Given the choice
>> between fixing code that has no callers and simply removing it, I'd go for
>> the latter.
> 
> Yeah, if it's broken and has no real users, send it to hell. :)
> 

OK, thanks.


Hmm... at least, the original API definition is not well enough: "need
use 'unsigned int' and 'atomic_t' instead of 'unsigned long' for the
type of parameters".

But can we say "under arm64, it must be a bug"? (although I agree it is
very easy to let callers miss using it -- then may cause issue).

In my opinion, it belongs to "API definition issue" not implementation
bug: "if all callers are carefully enough, it will not make issues"
(e.g. in "./kernel" sub-system, we can meet many such kinds of things).



Thanks.

> Thanks,
> //richard
> 
> 
> 

-- 
Chen Gang

Re: [PATCH] arm/arm64: remove atomic_clear_mask() in "include/asm/atomic.h"

[Chen Gang]

On 10/12/2013 09:36 AM, Chen Gang wrote:
> On 10/11/2013 09:03 PM, Richard Weinberger wrote:
>> Am 11.10.2013 14:28, schrieb Will Deacon:
>>> On Fri, Oct 11, 2013 at 01:08:17PM +0100, Richard Weinberger wrote:
>>>> On Fri, Oct 11, 2013 at 1:47 PM, Chen Gang <gang.chen@asianux.com> wrote:
>>>>> In current kernel wide source code, except other architectures, only
>>>>> s390 scsi drivers use atomic_clear_mask(), and arm/arm64 need not
>>>>> support s390 drivers.
>>>>>
>>>>> So remove atomic_clear_mask() from "arm[64]/include/asm/atomic.h".
>>>>
>>>> Is it really worth removing such a primitive?
>>>> If someone needs it later he has to implement it from scratch and
>>>> introduces bugs...
>>>
>>> The version we have (on ARM64 anyway) already has bugs. Given the choice
>>> between fixing code that has no callers and simply removing it, I'd go for
>>> the latter.
>>
>> Yeah, if it's broken and has no real users, send it to hell. :)
>>
> 
> OK, thanks.
> 
> 
> Hmm... at least, the original API definition is not well enough: "need
> use 'unsigned int' and 'atomic_t' instead of 'unsigned long' for the
> type of parameters".
> 
> But can we say "under arm64, it must be a bug"? (although I agree it is
> very easy to let callers miss using it -- then may cause issue).
> 
> In my opinion, it belongs to "API definition issue" not implementation
> bug: "if all callers are carefully enough, it will not make issues"
> (e.g. in "./kernel" sub-system, we can meet many such kinds of things).
> 

For "./kernel" sub-system, it really it is, if necessary, I can provide
3 samples.  ;-)

> 
> 
> Thanks.
> 
>> Thanks,
>> //richard
>>
>>
>>
> 


-- 
Chen Gang

Re: [PATCH] arm/arm64: remove atomic_clear_mask() in "include/asm/atomic.h"

[Will Deacon]

On Fri, Oct 11, 2013 at 12:47:05PM +0100, Chen Gang wrote:
> In current kernel wide source code, except other architectures, only
> s390 scsi drivers use atomic_clear_mask(), and arm/arm64 need not
> support s390 drivers.
> 
> So remove atomic_clear_mask() from "arm[64]/include/asm/atomic.h".

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

Catalin, are you happy for me to send this via the ARM tree?

Will

> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  arch/arm/include/asm/atomic.h   |   24 ------------------------
>  arch/arm64/include/asm/atomic.h |   14 --------------
>  2 files changed, 0 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
> index da1c77d..3b3ae49fa 100644
> --- a/arch/arm/include/asm/atomic.h
> +++ b/arch/arm/include/asm/atomic.h
> @@ -134,21 +134,6 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>  	return oldval;
>  }
>  
> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
> -{
> -	unsigned long tmp, tmp2;
> -
> -	__asm__ __volatile__("@ atomic_clear_mask\n"
> -"1:	ldrex	%0, [%3]\n"
> -"	bic	%0, %0, %4\n"
> -"	strex	%1, %0, [%3]\n"
> -"	teq	%1, #0\n"
> -"	bne	1b"
> -	: "=&r" (tmp), "=&r" (tmp2), "+Qo" (*addr)
> -	: "r" (addr), "Ir" (mask)
> -	: "cc");
> -}
> -
>  #else /* ARM_ARCH_6 */
>  
>  #ifdef CONFIG_SMP
> @@ -197,15 +182,6 @@ static inline int atomic_cmpxchg(atomic_t *v, int old, int new)
>  	return ret;
>  }
>  
> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
> -{
> -	unsigned long flags;
> -
> -	raw_local_irq_save(flags);
> -	*addr &= ~mask;
> -	raw_local_irq_restore(flags);
> -}
> -
>  #endif /* __LINUX_ARM_ARCH__ */
>  
>  #define atomic_xchg(v, new) (xchg(&((v)->counter), new))
> diff --git a/arch/arm64/include/asm/atomic.h b/arch/arm64/include/asm/atomic.h
> index 8363644..01de5aa 100644
> --- a/arch/arm64/include/asm/atomic.h
> +++ b/arch/arm64/include/asm/atomic.h
> @@ -126,20 +126,6 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>  	return oldval;
>  }
>  
> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
> -{
> -	unsigned long tmp, tmp2;
> -
> -	asm volatile("// atomic_clear_mask\n"
> -"1:	ldxr	%0, %2\n"
> -"	bic	%0, %0, %3\n"
> -"	stxr	%w1, %0, %2\n"
> -"	cbnz	%w1, 1b"
> -	: "=&r" (tmp), "=&r" (tmp2), "+Q" (*addr)
> -	: "Ir" (mask)
> -	: "cc");
> -}
> -
>  #define atomic_xchg(v, new) (xchg(&((v)->counter), new))
>  
>  static inline int __atomic_add_unless(atomic_t *v, int a, int u)
> -- 
> 1.7.7.6
> 

Re: [PATCH] arm/arm64: remove atomic_clear_mask() in "include/asm/atomic.h"

[Chen Gang]

On 10/12/2013 12:55 AM, Will Deacon wrote:
> On Fri, Oct 11, 2013 at 12:47:05PM +0100, Chen Gang wrote:
>> In current kernel wide source code, except other architectures, only
>> s390 scsi drivers use atomic_clear_mask(), and arm/arm64 need not
>> support s390 drivers.
>>
>> So remove atomic_clear_mask() from "arm[64]/include/asm/atomic.h".
> 
>   Acked-by: Will Deacon <will.deacon@arm.com>
> 

Thank you for your whole work.

> Catalin, are you happy for me to send this via the ARM tree?
> 
> Will
> 


Thanks.

--
Chen Gang

>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  arch/arm/include/asm/atomic.h   |   24 ------------------------
>>  arch/arm64/include/asm/atomic.h |   14 --------------
>>  2 files changed, 0 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
>> index da1c77d..3b3ae49fa 100644
>> --- a/arch/arm/include/asm/atomic.h
>> +++ b/arch/arm/include/asm/atomic.h
>> @@ -134,21 +134,6 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>>  	return oldval;
>>  }
>>  
>> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
>> -{
>> -	unsigned long tmp, tmp2;
>> -
>> -	__asm__ __volatile__("@ atomic_clear_mask\n"
>> -"1:	ldrex	%0, [%3]\n"
>> -"	bic	%0, %0, %4\n"
>> -"	strex	%1, %0, [%3]\n"
>> -"	teq	%1, #0\n"
>> -"	bne	1b"
>> -	: "=&r" (tmp), "=&r" (tmp2), "+Qo" (*addr)
>> -	: "r" (addr), "Ir" (mask)
>> -	: "cc");
>> -}
>> -
>>  #else /* ARM_ARCH_6 */
>>  
>>  #ifdef CONFIG_SMP
>> @@ -197,15 +182,6 @@ static inline int atomic_cmpxchg(atomic_t *v, int old, int new)
>>  	return ret;
>>  }
>>  
>> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
>> -{
>> -	unsigned long flags;
>> -
>> -	raw_local_irq_save(flags);
>> -	*addr &= ~mask;
>> -	raw_local_irq_restore(flags);
>> -}
>> -
>>  #endif /* __LINUX_ARM_ARCH__ */
>>  
>>  #define atomic_xchg(v, new) (xchg(&((v)->counter), new))
>> diff --git a/arch/arm64/include/asm/atomic.h b/arch/arm64/include/asm/atomic.h
>> index 8363644..01de5aa 100644
>> --- a/arch/arm64/include/asm/atomic.h
>> +++ b/arch/arm64/include/asm/atomic.h
>> @@ -126,20 +126,6 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>>  	return oldval;
>>  }
>>  
>> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
>> -{
>> -	unsigned long tmp, tmp2;
>> -
>> -	asm volatile("// atomic_clear_mask\n"
>> -"1:	ldxr	%0, %2\n"
>> -"	bic	%0, %0, %3\n"
>> -"	stxr	%w1, %0, %2\n"
>> -"	cbnz	%w1, 1b"
>> -	: "=&r" (tmp), "=&r" (tmp2), "+Q" (*addr)
>> -	: "Ir" (mask)
>> -	: "cc");
>> -}
>> -
>>  #define atomic_xchg(v, new) (xchg(&((v)->counter), new))
>>  
>>  static inline int __atomic_add_unless(atomic_t *v, int a, int u)
>> -- 
>> 1.7.7.6
>>
> 
> 


-- 
Chen Gang

Re: [PATCH] arm/arm64: remove atomic_clear_mask() in "include/asm/atomic.h"

[Catalin Marinas]

On 11 Oct 2013, at 17:55, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Oct 11, 2013 at 12:47:05PM +0100, Chen Gang wrote:
>> In current kernel wide source code, except other architectures, only
>> s390 scsi drivers use atomic_clear_mask(), and arm/arm64 need not
>> support s390 drivers.
>> 
>> So remove atomic_clear_mask() from "arm[64]/include/asm/atomic.h".
> 
>  Acked-by: Will Deacon <will.deacon@arm.com>
> 
> Catalin, are you happy for me to send this via the ARM tree?

Fine by me.

Catalin

Re: [PATCH 2/3] arm: include: asm: atomic.h: use 'unsigned int' and 'atomic' instead of 'unsigned long' for atomic_clear_mask()

[Will Deacon]

On Thu, Oct 10, 2013 at 03:34:02AM +0100, Chen Gang wrote:
> In current kernel wide source, for arm, only s390 scsi drivers use
> atomic_clear_mask(), now, s390 itself need use 'unsigned int' and
> 'atomic_t', so need match s390's atomic_clear_mask().
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  arch/arm/include/asm/atomic.h |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
> index da1c77d..0832a7f 100644
> --- a/arch/arm/include/asm/atomic.h
> +++ b/arch/arm/include/asm/atomic.h
> @@ -134,9 +134,10 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>  	return oldval;
>  }
>  
> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
> +static inline void atomic_clear_mask(unsigned int mask, atomic_t *ptr)
>  {
> -	unsigned long tmp, tmp2;
> +	unsigned int tmp;

I reckon this should be int (the mask parameter is unsigned, but
atomic_t.counter is signed).

> +	unsigned long tmp2;
>  
>  	__asm__ __volatile__("@ atomic_clear_mask\n"
>  "1:	ldrex	%0, [%3]\n"
> @@ -144,8 +145,8 @@ static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
>  "	strex	%1, %0, [%3]\n"
>  "	teq	%1, #0\n"
>  "	bne	1b"
> -	: "=&r" (tmp), "=&r" (tmp2), "+Qo" (*addr)
> -	: "r" (addr), "Ir" (mask)
> +	: "=&r" (tmp), "=&r" (tmp2), "+Qo" (ptr->counter)
> +	: "r" (&ptr->counter), "Ir" (mask)
>  	: "cc");
>  }
>  
> @@ -197,12 +198,12 @@ static inline int atomic_cmpxchg(atomic_t *v, int old, int new)
>  	return ret;
>  }
>  
> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
> +static inline void atomic_clear_mask(unsigned int mask, atomic_t *v)
>  {
>  	unsigned long flags;
>  
>  	raw_local_irq_save(flags);
> -	*addr &= ~mask;
> +	v->counter &= ~mask;
>  	raw_local_irq_restore(flags);
>  }

This is now identical to asm-generic/atomic.h. I wonder whether we could
just #include that file for the ARMv6 case? You'd need to check the
differences carefully.

Finally, I still question the need for the clear_mask function anyway. We
don't implement set_mask, and these functions are only used by either other
arch code or in drivers that don't work on ARM anyway.

Will

Re: [PATCH 2/3] arm: include: asm: atomic.h: use 'unsigned int' and 'atomic' instead of 'unsigned long' for atomic_clear_mask()

[Chen Gang]

On 10/10/2013 05:58 PM, Will Deacon wrote:
> On Thu, Oct 10, 2013 at 03:34:02AM +0100, Chen Gang wrote:
>> In current kernel wide source, for arm, only s390 scsi drivers use
>> atomic_clear_mask(), now, s390 itself need use 'unsigned int' and
>> 'atomic_t', so need match s390's atomic_clear_mask().
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  arch/arm/include/asm/atomic.h |   13 +++++++------
>>  1 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
>> index da1c77d..0832a7f 100644
>> --- a/arch/arm/include/asm/atomic.h
>> +++ b/arch/arm/include/asm/atomic.h
>> @@ -134,9 +134,10 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>>  	return oldval;
>>  }
>>  
>> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
>> +static inline void atomic_clear_mask(unsigned int mask, atomic_t *ptr)
>>  {
>> -	unsigned long tmp, tmp2;
>> +	unsigned int tmp;
> 
> I reckon this should be int (the mask parameter is unsigned, but
> atomic_t.counter is signed).

For 'ldrex' and 'strex' (loading/storing instruction), it is really
better to match 'atomic_t.counter', but for 'bic' (operating
instruction), it is better to match 'mask'.

In my opinion, for signed/unsigned, 'operating' has higher priority than
'loading/storing' (especially for *mask functions, by default, suggest
using unsigned).

Commonly, for loading/storing (e.g. 'ldrex', 'strex'), must be sure of
bits wide (signed/unsigned will not cause real issues), but for
operating, signed/unsigned may cause real issues.


> 
>> +	unsigned long tmp2;
>>  
>>  	__asm__ __volatile__("@ atomic_clear_mask\n"
>>  "1:	ldrex	%0, [%3]\n"
>> @@ -144,8 +145,8 @@ static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
>>  "	strex	%1, %0, [%3]\n"
>>  "	teq	%1, #0\n"
>>  "	bne	1b"
>> -	: "=&r" (tmp), "=&r" (tmp2), "+Qo" (*addr)
>> -	: "r" (addr), "Ir" (mask)
>> +	: "=&r" (tmp), "=&r" (tmp2), "+Qo" (ptr->counter)
>> +	: "r" (&ptr->counter), "Ir" (mask)
>>  	: "cc");
>>  }
>>  
>> @@ -197,12 +198,12 @@ static inline int atomic_cmpxchg(atomic_t *v, int old, int new)
>>  	return ret;
>>  }
>>  
>> -static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
>> +static inline void atomic_clear_mask(unsigned int mask, atomic_t *v)
>>  {
>>  	unsigned long flags;
>>  
>>  	raw_local_irq_save(flags);
>> -	*addr &= ~mask;
>> +	v->counter &= ~mask;
>>  	raw_local_irq_restore(flags);
>>  }
> 
> This is now identical to asm-generic/atomic.h. I wonder whether we could
> just #include that file for the ARMv6 case? You'd need to check the
> differences carefully.
> 

If most of functions for ARMv6 case can use "asm-generic/atomic.h", your
idea sounds good to me, although we don't need 'atomic_set_mask' (it is
inconsistent with 'atomic_clear_mask' in "asm-generic/atomic.h").

> Finally, I still question the need for the clear_mask function anyway. We
> don't implement set_mask, and these functions are only used by either other
> arch code or in drivers that don't work on ARM anyway.
> 

Hmm... can we remove atomic_*_mask() for both arm and arm64?

It seems before get a conclusion, it is necessary to let arm and arm64
pass 'allmodconfig' firstly (and then try to remove these functions to
see the compiling result).

I will/should try 'allmodconfig' for them, but excuse me, it needs
waiting (I am just trying 'arc' architecture with 'allmodconfig', and
already delayed, because I have no enough time resources on it :-( ).

> Will
> 
> 

Thanks.
-- 
Chen Gang

Re: [PATCH 1/3] s390: include: asm: atomic.h: use 'unsigned int' instead of 'unsigned long' for atomic_*_mask()

[Heiko Carstens]

On Thu, Oct 10, 2013 at 10:31:22AM +0800, Chen Gang wrote:
> The type of 'v->counter' is always 'int', and related inline assembly
> code also process 'int', so use 'unsigned int' instead of 'unsigned
> long' for the 'mask'.
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  arch/s390/include/asm/atomic.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/include/asm/atomic.h b/arch/s390/include/asm/atomic.h
> index a62ed2d..12c5ec1 100644
> --- a/arch/s390/include/asm/atomic.h
> +++ b/arch/s390/include/asm/atomic.h
> @@ -113,12 +113,12 @@ static inline void atomic_add(int i, atomic_t *v)
>  #define atomic_dec_return(_v)		atomic_sub_return(1, _v)
>  #define atomic_dec_and_test(_v)		(atomic_sub_return(1, _v) == 0)
> 
> -static inline void atomic_clear_mask(unsigned long mask, atomic_t *v)
> +static inline void atomic_clear_mask(unsigned int mask, atomic_t *v)
>  {

Applied. Thanks!

Re: [PATCH 1/3] s390: include: asm: atomic.h: use 'unsigned int' instead of 'unsigned long' for atomic_*_mask()

[Chen Gang]

On 10/10/2013 03:25 PM, Heiko Carstens wrote:
> On Thu, Oct 10, 2013 at 10:31:22AM +0800, Chen Gang wrote:
>> The type of 'v->counter' is always 'int', and related inline assembly
>> code also process 'int', so use 'unsigned int' instead of 'unsigned
>> long' for the 'mask'.
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  arch/s390/include/asm/atomic.h |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/atomic.h b/arch/s390/include/asm/atomic.h
>> index a62ed2d..12c5ec1 100644
>> --- a/arch/s390/include/asm/atomic.h
>> +++ b/arch/s390/include/asm/atomic.h
>> @@ -113,12 +113,12 @@ static inline void atomic_add(int i, atomic_t *v)
>>  #define atomic_dec_return(_v)		atomic_sub_return(1, _v)
>>  #define atomic_dec_and_test(_v)		(atomic_sub_return(1, _v) == 0)
>>
>> -static inline void atomic_clear_mask(unsigned long mask, atomic_t *v)
>> +static inline void atomic_clear_mask(unsigned int mask, atomic_t *v)
>>  {
> 
> Applied. Thanks!
> 
> 
> 

Thank you very much!!

-- 
Chen Gang