| Message ID | 5257E539.9080902@asianux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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/
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
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
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 >
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 > > >
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 >> > >
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 >> >> >> >
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
Hello Maintainers: Are these related patches for "arm[64]/include/asm/atomic.h" OK? If also need me do something, please let me know, and I will/should try. Thanks. On 10/13/2013 06:28 AM, Catalin Marinas wrote: > 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 > >
On Mon, Nov 04, 2013 at 07:36:42AM +0000, Chen Gang wrote: > Hello Maintainers: > > Are these related patches for "arm[64]/include/asm/atomic.h" OK? If also > need me do something, please let me know, and I will/should try. They're sitting in the patch system, so you don't need to do anything else unless there is a merge conflict. Will
On 11/04/2013 06:07 PM, Will Deacon wrote: > On Mon, Nov 04, 2013 at 07:36:42AM +0000, Chen Gang wrote: >> > Hello Maintainers: >> > >> > Are these related patches for "arm[64]/include/asm/atomic.h" OK? If also >> > need me do something, please let me know, and I will/should try. > They're sitting in the patch system, so you don't need to do anything else > unless there is a merge conflict. OK, thanks. And excuse me, I am not quite familiar with our kernel version merging, so send the original redundant mail. Thanks.
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)
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(-)