Re: [PATCH] arc: remove redundant GCC version checks
On 08/26/2018 08:08 PM, Masahiro Yamada wrote:
> Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6")
> bumped the minimum GCC version to 4.6 for all architectures.
>
> With GCC >= 4.6 assumed, 'upto_gcc44' is empty, 'atleast_gcc44' is y.
>
> Signed-off-by: Masahiro Yamada
Added to ARC/for-curr
Thx,
-Vineet
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
On 08/30/2018 02:51 AM, Will Deacon wrote:
> Yeah, the bit_spin_lock()/__bit_spin_unlock() race described in f75d48644c56a
> boils down to concurrent atomic_long_set_release() vs
> atomic_long_fetch_or_acquire(), which really needs to work.
I don't see how: __clear_bit_unlock() reads @old, flips a bit and then calls
atomic_long_set_release() so the race is not just with set_release.
static inline int test_and_set_bit_lock(unsigned int nr, volatile unsigned
long *p)
{
long old;
unsigned long mask = (1UL << ((nr) % 32));
p += ((nr) / 32);
old = atomic_long_fetch_or_acquire(mask, (atomic_long_t *)p);
return !!(old & mask);
}
static inline void __clear_bit_unlock(unsigned int nr, volatile unsigned
long *p)
{
unsigned long old;
p += ((nr) / 32);
old = // soem typecheck magic on *p
old &= ~(1UL << ((nr) % 32));
atomic_long_set_release((atomic_long_t *)p, old);
}
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
On 08/30/2018 01:45 PM, Peter Zijlstra wrote: >> >> Indeed this is the mother of all issues, I tried and system is clearly hosed >> with >> and works after. >> What's amazing is the commit 4aef66c8ae9 which introduced it is from 2016 ;-) >> Back then we had a retry branch with backoff stuff which I'd reverted for new >> cores and the merge conflict somehow missed it. >> >> @PeterZ I'll create a patch with you as author ? do I need any formal sign >> offs, >> acks etc ? > Well, Will spotted it, give authorship to him, you have my ack per the > above. Oops, sorry for the mixup. I have him as author now and pushed to ARC for-curr (will trickle into linux-next eventually). > FWIW, also ACK on Will's patch to switch you over to asm-generic bitops > entirely. Yeah I'll get it to that and your's after the clear_bit_lock stuff - sorry for hijacking this thread to that topic now -Vineet ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
__clear_bit_lock to use atomic clear_bit (was Re: Patch "asm-generic/bitops/lock.h)
On 08/30/2018 02:44 AM, Peter Zijlstra wrote:
>> Back in 2016, Peter had fixed this file due to a problem I reported on ARC.
>> See
>> commit f75d48644c56a ("bitops: Do not default to __clear_bit() for
>> __clear_bit_unlock()")
>> That made __clear_bit_unlock() use the atomic clear_bit() vs. non-atomic
>> __clear_bit(), effectively making clear_bit_unlock() and
>> __clear_bit_unlock() same.
>>
>> This patch undoes that which could explain the issues you see. @Peter, @Will
>> ?
> Right, so the thinking is that on platforms that suffer that issue,
> atomic_set*() should DTRT. And if you look at your spinlock based atomic
> implementation, you'll note that atomic_set() does indeed do the right
> thing.
>
> arch/arc/include/asm/atomic.h:108
For !LLSC atomics, ARC has always had atomic_set() DTRT even in the git revision
of 2016. The problem was not in atomics, but the asymmetric way slub bit lock
etc
worked (haven't checked if this changed), i.e.
slab_lock() -> bit_spin_lock() -> test_and_set_bit()# atomic
slab_unlock() -> __bit_spin_unlock() -> __clear_bit()# non-atomic
And with v4.19-rc1, we have essentially reverted f75d48644c56a due to
84c6591103db
("locking/atomics, asm-generic/bitops/lock.h: Rewrite using atomic_fetch_*()")
So what we have with 4.19-rc1 is
static inline void __clear_bit_unlock(unsigned int nr, volatile unsigned
long *p)
{
unsigned long old;
p += ((nr) / 32);
old = // some typecheck magic on *p
old &= ~(1UL << ((nr) % 32));
atomic_long_set_release((atomic_long_t *)p, old);
}
So @p is being r-m-w non atomically. The lock variant uses atomic op...
int test_and_set_bit_lock(unsigned int nr, volatile unsigned long *p)
{
...
old = atomic_long_fetch_or_acquire(mask, (atomic_long_t *)p);
}
Now I don't know why we don't see the issue with LLSC atomics, perhaps race
window
reduces due to less verbose code itself etc..
Am I missing something still ?
-Vineet
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
On Thu, Aug 30, 2018 at 08:31:59PM +, Vineet Gupta wrote: > On 08/30/2018 07:29 AM, Peter Zijlstra wrote: > > On Thu, Aug 30, 2018 at 03:23:55PM +0100, Will Deacon wrote: > > > >> Yes, that would be worth trying. However, I also just noticed that the > >> fetch-ops (which are now used to implement test_and_set_bit_lock()) seem > >> to be missing the backwards branch in the LL/SC case. Yet another diff > >> below. > >> > >> Will > >> > >> --->8 > >> > >> diff --git a/arch/arc/include/asm/atomic.h b/arch/arc/include/asm/atomic.h > >> index 4e0072730241..f06c5ed672b3 100644 > >> --- a/arch/arc/include/asm/atomic.h > >> +++ b/arch/arc/include/asm/atomic.h > >> @@ -84,7 +84,7 @@ static inline int atomic_fetch_##op(int i, atomic_t *v) > >> \ > >>"1: llock %[orig], [%[ctr]] \n" \ > >>" " #asm_op " %[val], %[orig], %[i] \n" \ > >>" scond %[val], [%[ctr]]\n" \ > >> - " \n" \ > >> + " bnz 1b \n" \ > >>: [val] "=" (val), \ > >> [orig] "=" (orig) \ > >>: [ctr] "r" (>counter), \ > > ACK!! sorry about that, no idea how I messed that up. > > > > Also, once it all works, they should look at switching to _relaxed > > atomics for LL/SC. > > Indeed this is the mother of all issues, I tried and system is clearly hosed > with > and works after. > What's amazing is the commit 4aef66c8ae9 which introduced it is from 2016 ;-) > Back then we had a retry branch with backoff stuff which I'd reverted for new > cores and the merge conflict somehow missed it. > > @PeterZ I'll create a patch with you as author ? do I need any formal sign > offs, > acks etc ? Well, Will spotted it, give authorship to him, you have my ack per the above. > So after this there are 2 other things to be addresses / looked at still > while we > are still here. > > 1. After 84c6591103db __clear_bit_lock() implementation will be broken (or > atleast > not consistent with what we had after), do we need to reinstate it. > 2. Will's proposed change to remove the underlying issue, but the issue in #1 > remains ? No, like explained, for spinlock based atomics the issue _should_ not exist, and if you look at your atomic_set() implementation for that variant, you'll see it does the right thing by taking the lock. Basically atomic_set() for spinlock based atomics ends up being (void)atomic_xchg(). FWIW, also ACK on Will's patch to switch you over to asm-generic bitops entirely. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 2/2] dt-bindings: Document the Synopsys GPIO via CREG bindings
On Thu, 2018-08-30 at 10:43 +0200, Linus Walleij wrote: > On Tue, Aug 28, 2018 at 1:27 PM Eugeniy Paltsev > wrote: > > > +++ b/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt > > @@ -0,0 +1,49 @@ > > +GPIO via CREG (Control REGisers) driver [snip] > > +- snps,ngpios: Number of GPIO pins. > > Use the existing ngpios attribute for this, see gpio.txt Ok > > +- snps,bit-per-line: Number of bits per each gpio line (see picture). > > + Array the size of "snps,ngpios" > > +- snps,shift: Shift (in bits) of the each GPIO field from the previous one > > in > > + register (see picture). Array the size of "snps,ngpios" > > +- snps,on-val: Value should be set in corresponding field to set > > + output to "1" (see picture). Array the size of "snps,ngpios" > > +- snps,off-val: Value should be set in corresponding field to set > > + output to "0" (see picture). Array the size of "snps,ngpios" > > Move this into a lookup table in the driver instead, and match > the lookup table to the compatible string. The format of the > register is known for a certain compatible, right? Actually I really don't want to hardcode this values into lookup table as I going to use this driver on 3 already upstreamed platforms and at least one upcoming. They all have such CREG pseudo-'GPIOs' differently mapped with different IO lines number, different enable/disable value, etc... Is it really a problem to have this values configured via device tree? If we read them from DT we are able to use this generic and configurable driver to handle both existing and upcoming platforms without the need of patching the driver on every new platform upstreaming. > > Yours, > Linus Walleij -- Eugeniy Paltsev ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
On Thu, Aug 30, 2018 at 02:46:16PM +, Eugeniy Paltsev wrote: > On Thu, 2018-08-30 at 16:17 +0200, Peter Zijlstra wrote: > > On Thu, Aug 30, 2018 at 11:53:17AM +, Eugeniy Paltsev wrote: > > > I can see crashes with LLSC enabled in both SMP running on 4 cores > > > and SMP running on 1 core. > > > > So you're running on LL/SC enabled hardware; that would make Will's > > patch irrelevant (although still a good idea for the hardware that does > > care about that spinlocked atomic crud). > > > > Does something like the below cure things? That would confirm the > > suggestion that the change to __clear_bit_unlock() is the curprit. > > I tested it - this doesn't change anything, the problem still reproduces. OK, 'good' :-) > I'll test it with last Will fix. Yes, that missing "bnz 1b" is very very suspect. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
On Thu, 2018-08-30 at 16:17 +0200, Peter Zijlstra wrote:
> On Thu, Aug 30, 2018 at 11:53:17AM +, Eugeniy Paltsev wrote:
> > I can see crashes with LLSC enabled in both SMP running on 4 cores
> > and SMP running on 1 core.
>
> So you're running on LL/SC enabled hardware; that would make Will's
> patch irrelevant (although still a good idea for the hardware that does
> care about that spinlocked atomic crud).
>
> Does something like the below cure things? That would confirm the
> suggestion that the change to __clear_bit_unlock() is the curprit.
I tested it - this doesn't change anything, the problem still reproduces.
I'll test it with last Will fix.
> If that doesn't cure things, then we've been looking in entirely the
> wrong place.
>
> ---
> diff --git a/include/asm-generic/bitops/lock.h
> b/include/asm-generic/bitops/lock.h
> index 3ae021368f48..79c6978152f8 100644
> --- a/include/asm-generic/bitops/lock.h
> +++ b/include/asm-generic/bitops/lock.h
> @@ -57,12 +57,7 @@ static inline void clear_bit_unlock(unsigned int nr,
> volatile unsigned long *p)
> static inline void __clear_bit_unlock(unsigned int nr,
> volatile unsigned long *p)
> {
> - unsigned long old;
> -
> - p += BIT_WORD(nr);
> - old = READ_ONCE(*p);
> - old &= ~BIT_MASK(nr);
> - atomic_long_set_release((atomic_long_t *)p, old);
> + clear_bit_unlock(nr, p);
> }
>
> /**
--
Eugeniy Paltsev
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
On Thu, Aug 30, 2018 at 04:29:20PM +0200, Peter Zijlstra wrote:
> Also, once it all works, they should look at switching to _relaxed
> atomics for LL/SC.
A little something like so.. should save a few smp_mb().
---
diff --git a/arch/arc/include/asm/atomic.h b/arch/arc/include/asm/atomic.h
index 4e0072730241..714b54c308b0 100644
--- a/arch/arc/include/asm/atomic.h
+++ b/arch/arc/include/asm/atomic.h
@@ -44,7 +44,7 @@ static inline void atomic_##op(int i, atomic_t *v)
\
} \
#define ATOMIC_OP_RETURN(op, c_op, asm_op) \
-static inline int atomic_##op##_return(int i, atomic_t *v) \
+static inline int atomic_##op##_return_relaxed(int i, atomic_t *v) \
{ \
unsigned int val; \
\
@@ -69,8 +69,11 @@ static inline int atomic_##op##_return(int i, atomic_t *v)
\
return val; \
}
+#define atomic_add_return_relaxed atomic_add_return_relaxed
+#define atomic_sub_return_relaxed atomic_sub_return_relaxed
+
#define ATOMIC_FETCH_OP(op, c_op, asm_op) \
-static inline int atomic_fetch_##op(int i, atomic_t *v)
\
+static inline int atomic_fetch_##op##_relaxed(int i, atomic_t *v) \
{ \
unsigned int val, orig; \
\
@@ -96,6 +99,14 @@ static inline int atomic_fetch_##op(int i, atomic_t *v)
\
return orig;\
}
+#define atomic_fetch_add_relaxed atomic_fetch_add_relaxed
+#define atomic_fetch_sub_relaxed atomic_fetch_sub_relaxed
+
+#define atomic_fetch_and_relaxed atomic_fetch_and_relaxed
+#define atomic_fetch_andnot_relaxedatomic_fetch_andnot_relaxed
+#define atomic_fetch_or_relaxedatomic_fetch_or_relaxed
+#define atomic_fetch_xor_relaxed atomic_fetch_xor_relaxed
+
#else /* !CONFIG_ARC_HAS_LLSC */
#ifndef CONFIG_SMP
@@ -379,7 +390,7 @@ static inline void atomic64_##op(long long a, atomic64_t
*v)\
} \
#define ATOMIC64_OP_RETURN(op, op1, op2) \
-static inline long long atomic64_##op##_return(long long a, atomic64_t *v)
\
+static inline long long atomic64_##op##_return_relaxed(long long a, atomic64_t
*v) \
{ \
unsigned long long val; \
\
@@ -401,8 +412,11 @@ static inline long long atomic64_##op##_return(long long
a, atomic64_t *v) \
return val; \
}
+#define atomic64_add_return_relaxedatomic64_add_return_relaxed
+#define atomic64_sub_return_relaxedatomic64_sub_return_relaxed
+
#define ATOMIC64_FETCH_OP(op, op1, op2)
\
-static inline long long atomic64_fetch_##op(long long a, atomic64_t *v)
\
+static inline long long atomic64_fetch_##op##_relaxed(long long a, atomic64_t
*v) \
{ \
unsigned long long val, orig; \
\
@@ -424,6 +438,14 @@ static inline long long atomic64_fetch_##op(long long a,
atomic64_t *v)\
return orig;\
}
+#define atomic64_fetch_add_relaxed atomic64_fetch_add_relaxed
+#define atomic64_fetch_sub_relaxed atomic64_fetch_sub_relaxed
+
+#define atomic64_fetch_and_relaxed atomic64_fetch_and_relaxed
+#define atomic64_fetch_andnot_relaxed atomic64_fetch_andnot_relaxed
+#define atomic64_fetch_or_relaxed atomic64_fetch_or_relaxed
+#define atomic64_fetch_xor_relaxed atomic64_fetch_xor_relaxed
+
#define ATOMIC64_OPS(op, op1, op2) \
ATOMIC64_OP(op, op1, op2) \
ATOMIC64_OP_RETURN(op, op1, op2)\
@@ -434,6 +456,12 @@ static inline long long atomic64_fetch_##op(long long a,
atomic64_t *v)\
ATOMIC64_OPS(add, add.f, adc)
ATOMIC64_OPS(sub, sub.f, sbc)
+
+#undef ATOMIC64_OPS
+#define ATOMIC64_OPS(op, op1, op2) \
+ ATOMIC64_OP(op, op1, op2) \
+
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
On Thu, Aug 30, 2018 at 03:23:55PM +0100, Will Deacon wrote: > Yes, that would be worth trying. However, I also just noticed that the > fetch-ops (which are now used to implement test_and_set_bit_lock()) seem > to be missing the backwards branch in the LL/SC case. Yet another diff > below. > > Will > > --->8 > > diff --git a/arch/arc/include/asm/atomic.h b/arch/arc/include/asm/atomic.h > index 4e0072730241..f06c5ed672b3 100644 > --- a/arch/arc/include/asm/atomic.h > +++ b/arch/arc/include/asm/atomic.h > @@ -84,7 +84,7 @@ static inline int atomic_fetch_##op(int i, atomic_t *v) > \ > "1: llock %[orig], [%[ctr]] \n" \ > " " #asm_op " %[val], %[orig], %[i] \n" \ > " scond %[val], [%[ctr]]\n" \ > - " \n" \ > + " bnz 1b \n" \ > : [val] "=" (val), \ > [orig] "=" (orig) \ > : [ctr] "r" (>counter), \ ACK!! sorry about that, no idea how I messed that up. Also, once it all works, they should look at switching to _relaxed atomics for LL/SC. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
On Thu, Aug 30, 2018 at 04:17:13PM +0200, Peter Zijlstra wrote: > On Thu, Aug 30, 2018 at 11:53:17AM +, Eugeniy Paltsev wrote: > > I can see crashes with LLSC enabled in both SMP running on 4 cores > > and SMP running on 1 core. > > So you're running on LL/SC enabled hardware; that would make Will's > patch irrelevant (although still a good idea for the hardware that does > care about that spinlocked atomic crud). Yeah, that's a good point. I think the !LLSC case is broken without my patch, so we're looking at two bugs... > Does something like the below cure things? That would confirm the > suggestion that the change to __clear_bit_unlock() is the curprit. > > If that doesn't cure things, then we've been looking in entirely the > wrong place. Yes, that would be worth trying. However, I also just noticed that the fetch-ops (which are now used to implement test_and_set_bit_lock()) seem to be missing the backwards branch in the LL/SC case. Yet another diff below. Will --->8 diff --git a/arch/arc/include/asm/atomic.h b/arch/arc/include/asm/atomic.h index 4e0072730241..f06c5ed672b3 100644 --- a/arch/arc/include/asm/atomic.h +++ b/arch/arc/include/asm/atomic.h @@ -84,7 +84,7 @@ static inline int atomic_fetch_##op(int i, atomic_t *v) \ "1: llock %[orig], [%[ctr]] \n" \ " " #asm_op " %[val], %[orig], %[i] \n" \ " scond %[val], [%[ctr]]\n" \ - " \n" \ + " bnz 1b \n" \ : [val] "=" (val), \ [orig] "=" (orig) \ : [ctr] "r" (>counter), \ ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
On Thu, Aug 30, 2018 at 11:53:17AM +, Eugeniy Paltsev wrote:
> I can see crashes with LLSC enabled in both SMP running on 4 cores
> and SMP running on 1 core.
So you're running on LL/SC enabled hardware; that would make Will's
patch irrelevant (although still a good idea for the hardware that does
care about that spinlocked atomic crud).
Does something like the below cure things? That would confirm the
suggestion that the change to __clear_bit_unlock() is the curprit.
If that doesn't cure things, then we've been looking in entirely the
wrong place.
---
diff --git a/include/asm-generic/bitops/lock.h
b/include/asm-generic/bitops/lock.h
index 3ae021368f48..79c6978152f8 100644
--- a/include/asm-generic/bitops/lock.h
+++ b/include/asm-generic/bitops/lock.h
@@ -57,12 +57,7 @@ static inline void clear_bit_unlock(unsigned int nr,
volatile unsigned long *p)
static inline void __clear_bit_unlock(unsigned int nr,
volatile unsigned long *p)
{
- unsigned long old;
-
- p += BIT_WORD(nr);
- old = READ_ONCE(*p);
- old &= ~BIT_MASK(nr);
- atomic_long_set_release((atomic_long_t *)p, old);
+ clear_bit_unlock(nr, p);
}
/**
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
Hi Eugeniy,
On Thu, Aug 30, 2018 at 11:53:17AM +, Eugeniy Paltsev wrote:
> On Thu, 2018-08-30 at 10:51 +0100, Will Deacon wrote:
> > On Thu, Aug 30, 2018 at 11:44:11AM +0200, Peter Zijlstra wrote:
> > > On Wed, Aug 29, 2018 at 09:16:43PM +, Vineet Gupta wrote:
> > > > On 08/29/2018 11:33 AM, Eugeniy Paltsev wrote:
> > > > > Hi Guys,
> > > > > Since v4.19-rc1 we are getting a serious regression on platforms with
> > > > > ARC architecture.
> > > > > The kernel have become unstable and spontaneously crashes on LTP
> > > > > tests execution / IO tests or
> > > > > even on boot.
> > > > >
> > > > > I don't know exactly what breaks but bisect clearly assign the blame
> > > > > to this commit:
> > > > > 84c6591103db ("locking/atomics, asm-generic/bitops/lock.h: Rewrite
> > > > > using atomic_fetch_*()")
> > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_torvalds_linux_commit_84c6591103dbeaf393a092a3fc7b09510825f6b9=DwIBAg=DPL6
> > > > > _X_6JkXFx7AXWqB0tg=ZlJN1MriPUTkBKCrPSx67GmaplEUGcAEk9yPtCLdUXI=6y0FFvkGdIQ6kX2lZ31V99lMfMV-
> > > > > RyWyYhiUGzh0Bi0=GNwmhSynIcWqgZhiOwFEEH_AtbZAH443_L6QH4nw_ls=
> > > > >
> > > > > Reverting the commit solves this problem.
> > > > >
> > > > > I tested v4.19-rc1 on ARM (wandboard, i.mx6, 32bit, quard core,
> > > > > ARMv7) which uses same
> > > > > generic bitops implementation and it works fine.
> > > > >
> > > > > Do you have any ideas what went wrong?
> > > >
> > > > Back in 2016, Peter had fixed this file due to a problem I reported on
> > > > ARC. See
> > > > commit f75d48644c56a ("bitops: Do not default to __clear_bit() for
> > > > __clear_bit_unlock()")
> > > > That made __clear_bit_unlock() use the atomic clear_bit() vs. non-atomic
> > > > __clear_bit(), effectively making clear_bit_unlock() and
> > > > __clear_bit_unlock() same.
> > > >
> > > > This patch undoes that which could explain the issues you see. @Peter,
> > > > @Will ?
> > >
> > > Right, so the thinking is that on platforms that suffer that issue,
> > > atomic_set*() should DTRT. And if you look at your spinlock based atomic
> > > implementation, you'll note that atomic_set() does indeed do the right
> > > thing.
> > >
> > > arch/arc/include/asm/atomic.h:108
> >
> > Yeah, the bit_spin_lock()/__bit_spin_unlock() race described in
> > f75d48644c56a
> > boils down to concurrent atomic_long_set_release() vs
> > atomic_long_fetch_or_acquire(), which really needs to work.
> >
> > I'll keep digging. In the meantime, Vineet, do you have any useful crash
> > logs and do you only see the crashes in certain configurations (e.g. SMP but
> > !CONFIG_ARC_HAS_LLSC)?
>
> We don't have such configuration (SMP with !CONFIG_ARC_HAS_LLSC).
Ok, thanks for confirming. If you could put your .config somewhere that
would be helpful, since the build fails for me with defconfig.
> I can see crashes with LLSC enabled in both SMP running on 4 cores
> and SMP running on 1 core.
>
>
> There are some crash logs (not sure if they are really useful):
> Crashes are quite spontaneous and mostly happens in IO-related code:
Aha: arc seems to have separate spinlocks for protecting bitops and atomics.
This is a seriously bad idea, because you only implement some of the bitops
API in the architecture and rely on asm-generic to give you bitops/lock.h,
assuming that it's going to be implemented in terms of the other parts of
the bitops API (it isn't anymore).
Here's a quick hack (totally untested, since I can't even build an arc kernel)
which moves arc over to asm-generic for all of the bitops API and ditches
the bitops lock. You could probably also drop the ffs() stuff, but I'll
leave that up to you. The "downside" is serialisation between bitops and
atomics, but I actually think you probably want that for xchg/cmpxchg
anyway.
Will
--->8
diff --git a/arch/arc/include/asm/bitops.h b/arch/arc/include/asm/bitops.h
index 8da87feec59a..50b0d5a56e32 100644
--- a/arch/arc/include/asm/bitops.h
+++ b/arch/arc/include/asm/bitops.h
@@ -17,242 +17,6 @@
#include
#include
-#include
-#ifndef CONFIG_ARC_HAS_LLSC
-#include
-#endif
-
-#ifdef CONFIG_ARC_HAS_LLSC
-
-/*
- * Hardware assisted Atomic-R-M-W
- */
-
-#define BIT_OP(op, c_op, asm_op) \
-static inline void op##_bit(unsigned long nr, volatile unsigned long *m)\
-{ \
- unsigned int temp; \
- \
- m += nr >> 5; \
- \
- nr &= 0x1f; \
- \
- __asm__ __volatile__( \
- "1: llock %0, [%1]\n"
Re: [PATCH v2 2/2] dt-bindings: Document the Synopsys GPIO via CREG bindings
On Tue, 2018-08-28 at 20:02 -0500, Rob Herring wrote:
> On Tue, Aug 28, 2018 at 02:27:21PM +0300, Eugeniy Paltsev wrote:
> > This patch adds documentation of device tree bindings for the Synopsys
> > GPIO via CREG driver.
> >
> > Signed-off-by: Eugeniy Paltsev
> > ---
> > .../devicetree/bindings/gpio/snps,creg-gpio.txt| 49
> > ++
> > 1 file changed, 49 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt
> > b/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt
> > new file mode 100644
> > index ..eb022d44ccda
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt
> > @@ -0,0 +1,49 @@
> > +GPIO via CREG (Control REGisers) driver
>
> Bindings don't describe drivers.
>
> > +
> > +This is is single-register MMIO GPIO driver to control such strangely
> > mapped
> > +outputs:
> > +
> > +31118 75 0 < bit number
> > +| || || |
> > +[ not used | gpio-1 | shift-1 | gpio-0 | shift-0 ] < 32 bit MMIO
> > register
> > + ^ ^
> > + | |
> > + | write 0x2 == set output to "1" (on)
> > + | write 0x3 == set output to "0" (off)
> > + |
> > +write 0x1 == set output to "1" (on)
> > +write 0x4 == set output to "0" (off)
>
> What kind of crazy h/w designer designed this?
Actually this fields in register controls some multiplexers, which we want to
use as IO port,
see the example:
/|
/ |
| |-- some internal line
IO PIN ---| |-- logic 0
| |-- logic 1
| |-- not used
\ |
|\|
|
CREG field
> > +Required properties:
> > +- compatible : "snps,creg-gpio"
> > +- reg : Exactly one register range with length 0x4.
> > +- #gpio-cells : Should be one - the pin number.
> > +- gpio-controller : Marks the device node as a GPIO controller.
> > +- snps,ngpios: Number of GPIO pins.
> > +- snps,bit-per-line: Number of bits per each gpio line (see picture).
> > + Array the size of "snps,ngpios"
> > +- snps,shift: Shift (in bits) of the each GPIO field from the previous one
> > in
> > + register (see picture). Array the size of "snps,ngpios"
> > +- snps,on-val: Value should be set in corresponding field to set
> > + output to "1" (see picture). Array the size of "snps,ngpios"
> > +- snps,off-val: Value should be set in corresponding field to set
> > + output to "0" (see picture). Array the size of "snps,ngpios"
>
> Convince me we need to parameterize all this. We try to avoid describing
> h/w like this.
Well, I going to use this driver on 3 already upstreamed platforms and one
upcoming.
They all have such CREG 'GPIOs' differently mapped with different IO lines
number,
different enable/disable value, etc...
So I really want to create some generic and configurable driver to handle both
existing
and upcoming platforms.
> > +
> > +Optional properties:
> > +- snps,default-val: default output field values. Array the size of
> > "snps,ngpios"
> > +
> > +Example (see picture):
> > +
> > +gpio: gpio@f00014b0 {
> > + compatible = "snps,creg-gpio";
> > + reg = <0xf00014b0 0x4>;
> > + gpio-controller;
> > + #gpio-cells = <1>;
> > + snps,ngpios = <2>;
> > + snps,shift = <5 1>;
> > + snps,bit-per-line = <2 3>;
> > + snps,on-val = <2 1>;
> > + snps,off-val = <3 4>;
> > + snps,default-val = <2 1>;
> > +};
> > --
> > 2.14.4
> >
--
Eugeniy Paltsev
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 1/2] GPIO: add single-register gpio via creg driver
On Tue, 2018-08-28 at 11:15 -0700, Randy Dunlap wrote: > Hi, > > I don't see any updates/corrections here. :( My fault - I've forgotten to re-generate new patch via git format-patch and send you previous version. > > On 08/28/2018 04:27 AM, Eugeniy Paltsev wrote: > > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > index 71c0ab46f216..0f9cc1582cab 100644 > > --- a/drivers/gpio/Kconfig > > +++ b/drivers/gpio/Kconfig > > @@ -430,6 +430,15 @@ config GPIO_REG > > A 32-bit single register GPIO fixed in/out implementation. This > > can be used to represent any register as a set of GPIO signals. > > > > +config GPIO_SNPS_CREG > > + bool "GPIO via CREG (Control REGisers) driver" > > + select OF_GPIO > > + help > > + This driver supports GPIOs via CREG on various Synopsys SoCs. > > + This is is single-register MMIO gpio driver for complex cases > > + where only several fields in register belong to GPIO and > > + each GPIO owns field with different length and on/off values. > > + > > config GPIO_SPEAR_SPICS > > bool "ST SPEAr13xx SPI Chip Select as GPIO support" > > depends on PLAT_SPEAR > > -- Eugeniy Paltsev ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
Hi Will,
On Thu, 2018-08-30 at 10:51 +0100, Will Deacon wrote:
> On Thu, Aug 30, 2018 at 11:44:11AM +0200, Peter Zijlstra wrote:
> > On Wed, Aug 29, 2018 at 09:16:43PM +, Vineet Gupta wrote:
> > > On 08/29/2018 11:33 AM, Eugeniy Paltsev wrote:
> > > > Hi Guys,
> > > > Since v4.19-rc1 we are getting a serious regression on platforms with
> > > > ARC architecture.
> > > > The kernel have become unstable and spontaneously crashes on LTP tests
> > > > execution / IO tests or
> > > > even on boot.
> > > >
> > > > I don't know exactly what breaks but bisect clearly assign the blame to
> > > > this commit:
> > > > 84c6591103db ("locking/atomics, asm-generic/bitops/lock.h: Rewrite
> > > > using atomic_fetch_*()")
> > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_torvalds_linux_commit_84c6591103dbeaf393a092a3fc7b09510825f6b9=DwIBAg=DPL6
> > > > _X_6JkXFx7AXWqB0tg=ZlJN1MriPUTkBKCrPSx67GmaplEUGcAEk9yPtCLdUXI=6y0FFvkGdIQ6kX2lZ31V99lMfMV-
> > > > RyWyYhiUGzh0Bi0=GNwmhSynIcWqgZhiOwFEEH_AtbZAH443_L6QH4nw_ls=
> > > >
> > > > Reverting the commit solves this problem.
> > > >
> > > > I tested v4.19-rc1 on ARM (wandboard, i.mx6, 32bit, quard core, ARMv7)
> > > > which uses same
> > > > generic bitops implementation and it works fine.
> > > >
> > > > Do you have any ideas what went wrong?
> > >
> > > Back in 2016, Peter had fixed this file due to a problem I reported on
> > > ARC. See
> > > commit f75d48644c56a ("bitops: Do not default to __clear_bit() for
> > > __clear_bit_unlock()")
> > > That made __clear_bit_unlock() use the atomic clear_bit() vs. non-atomic
> > > __clear_bit(), effectively making clear_bit_unlock() and
> > > __clear_bit_unlock() same.
> > >
> > > This patch undoes that which could explain the issues you see. @Peter,
> > > @Will ?
> >
> > Right, so the thinking is that on platforms that suffer that issue,
> > atomic_set*() should DTRT. And if you look at your spinlock based atomic
> > implementation, you'll note that atomic_set() does indeed do the right
> > thing.
> >
> > arch/arc/include/asm/atomic.h:108
>
> Yeah, the bit_spin_lock()/__bit_spin_unlock() race described in f75d48644c56a
> boils down to concurrent atomic_long_set_release() vs
> atomic_long_fetch_or_acquire(), which really needs to work.
>
> I'll keep digging. In the meantime, Vineet, do you have any useful crash
> logs and do you only see the crashes in certain configurations (e.g. SMP but
> !CONFIG_ARC_HAS_LLSC)?
We don't have such configuration (SMP with !CONFIG_ARC_HAS_LLSC).
I can see crashes with LLSC enabled in both SMP running on 4 cores
and SMP running on 1 core.
There are some crash logs (not sure if they are really useful):
Crashes are quite spontaneous and mostly happens in IO-related code:
Crash on boot:
>8
usb 1-1: new high-speed USB device number 2 using ehci-platform
hub 1-1:1.0: USB hub found
hub 1-1:1.0: 2 ports detected
usb 1-1.1: new high-speed USB device number 3 using ehci-platform
usb-storage 1-1.1:1.0: USB Mass Storage device detected
scsi host0: usb-storage 1-1.1:1.0
scsi 0:0:0:0: Direct-Access Generic STORAGE DEVICE 0272 PQ: 0 ANSI: 0
sd 0:0:0:0: [sda] 15759360 512-byte logical blocks: (8.07 GB/7.51 GiB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 0b 00 00 08
sd 0:0:0:0: [sda] No Caching mode page found
sd 0:0:0:0: [sda] Assuming drive cache: write through
sda: sda1 sda2 sda3 sda4
sd 0:0:0:0: [sda] Attached SCSI removable disk
INFO: task start-stop-daem:85 blocked for more than 10 seconds.
Not tainted 4.19.0-rc1 #2
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
start-stop-daem D085 81 0x
Stack Trace:
__switch_to+0x0/0xac
__schedule+0x1b2/0x730
io_schedule+0x5c/0xc0
__lock_page+0x98/0xdc
find_lock_entry+0x38/0x100
shmem_getpage_gfp.isra.3+0x82/0xbfc
shmem_fault+0x46/0x138
handle_mm_fault+0x5bc/0x924
do_page_fault+0xfc/0x294
ret_from_exception+0x0/0x8
INFO: task start-stop-daem:85 blocked for more than 10 seconds.
Not tainted 4.19.0-rc1 #2
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
start-stop-daem D085 81 0x
>8
>8
BUG: failure at mm/page-writeback.c:2652/clear_page_dirty_for_io()!
gcc generated __builtin_trap
Path: (null)
CPU: 3 PID: 7 Comm: kworker/u8:0 Not tainted 4.18.0-06995-g54dbe75bbf1e #22
Workqueue: writeback wb_workfn (flush-8:0)
[ECR ]: 0x00090005 => gcc generated __builtin_trap
[EFA ]: 0x90162b50
[BLINK ]: clear_page_dirty_for_io+0x13a/0x164
[ERET ]: abort+0x2/0x4
[STAT32]: 0x80080802 : IE K
BTA: 0x9021e356 SP: 0xbf045c64 FP: 0x
LPS: 0x9067fa68 LPE: 0x9067fa7c LPC: 0x
r00: 0x0043 r01: 0xbfb462d4 r02: 0x
r03: 0x90157788 r04: 0x r05: 0x9080c040
r06: 0x0031 r07: 0x r08: 0xa9ee8400
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
On Thu, Aug 30, 2018 at 11:44:11AM +0200, Peter Zijlstra wrote:
> On Wed, Aug 29, 2018 at 09:16:43PM +, Vineet Gupta wrote:
> > On 08/29/2018 11:33 AM, Eugeniy Paltsev wrote:
> > > Hi Guys,
> > > Since v4.19-rc1 we are getting a serious regression on platforms with ARC
> > > architecture.
> > > The kernel have become unstable and spontaneously crashes on LTP tests
> > > execution / IO tests or
> > > even on boot.
> > >
> > > I don't know exactly what breaks but bisect clearly assign the blame to
> > > this commit:
> > > 84c6591103db ("locking/atomics, asm-generic/bitops/lock.h: Rewrite using
> > > atomic_fetch_*()")
> > > https://github.com/torvalds/linux/commit/84c6591103dbeaf393a092a3fc7b09510825f6b9
> > >
> > > Reverting the commit solves this problem.
> > >
> > > I tested v4.19-rc1 on ARM (wandboard, i.mx6, 32bit, quard core, ARMv7)
> > > which uses same
> > > generic bitops implementation and it works fine.
> > >
> > > Do you have any ideas what went wrong?
> >
> > Back in 2016, Peter had fixed this file due to a problem I reported on ARC.
> > See
> > commit f75d48644c56a ("bitops: Do not default to __clear_bit() for
> > __clear_bit_unlock()")
> > That made __clear_bit_unlock() use the atomic clear_bit() vs. non-atomic
> > __clear_bit(), effectively making clear_bit_unlock() and
> > __clear_bit_unlock() same.
> >
> > This patch undoes that which could explain the issues you see. @Peter,
> > @Will ?
>
> Right, so the thinking is that on platforms that suffer that issue,
> atomic_set*() should DTRT. And if you look at your spinlock based atomic
> implementation, you'll note that atomic_set() does indeed do the right
> thing.
>
> arch/arc/include/asm/atomic.h:108
Yeah, the bit_spin_lock()/__bit_spin_unlock() race described in f75d48644c56a
boils down to concurrent atomic_long_set_release() vs
atomic_long_fetch_or_acquire(), which really needs to work.
I'll keep digging. In the meantime, Vineet, do you have any useful crash
logs and do you only see the crashes in certain configurations (e.g. SMP but
!CONFIG_ARC_HAS_LLSC)?
Will
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
On Wed, Aug 29, 2018 at 09:16:43PM +, Vineet Gupta wrote:
> On 08/29/2018 11:33 AM, Eugeniy Paltsev wrote:
> > Hi Guys,
> > Since v4.19-rc1 we are getting a serious regression on platforms with ARC
> > architecture.
> > The kernel have become unstable and spontaneously crashes on LTP tests
> > execution / IO tests or
> > even on boot.
> >
> > I don't know exactly what breaks but bisect clearly assign the blame to
> > this commit:
> > 84c6591103db ("locking/atomics, asm-generic/bitops/lock.h: Rewrite using
> > atomic_fetch_*()")
> > https://github.com/torvalds/linux/commit/84c6591103dbeaf393a092a3fc7b09510825f6b9
> >
> > Reverting the commit solves this problem.
> >
> > I tested v4.19-rc1 on ARM (wandboard, i.mx6, 32bit, quard core, ARMv7)
> > which uses same
> > generic bitops implementation and it works fine.
> >
> > Do you have any ideas what went wrong?
>
> Back in 2016, Peter had fixed this file due to a problem I reported on ARC.
> See
> commit f75d48644c56a ("bitops: Do not default to __clear_bit() for
> __clear_bit_unlock()")
> That made __clear_bit_unlock() use the atomic clear_bit() vs. non-atomic
> __clear_bit(), effectively making clear_bit_unlock() and __clear_bit_unlock()
> same.
>
> This patch undoes that which could explain the issues you see. @Peter, @Will ?
Right, so the thinking is that on platforms that suffer that issue,
atomic_set*() should DTRT. And if you look at your spinlock based atomic
implementation, you'll note that atomic_set() does indeed do the right
thing.
arch/arc/include/asm/atomic.h:108
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Patch "asm-generic/bitops/lock.h: Rewrite using atomic_fetch_" causes kernel crash
On Wed, Aug 29, 2018 at 09:16:43PM +, Vineet Gupta wrote:
> On 08/29/2018 11:33 AM, Eugeniy Paltsev wrote:
> > Hi Guys,
> > Since v4.19-rc1 we are getting a serious regression on platforms with ARC
> > architecture.
> > The kernel have become unstable and spontaneously crashes on LTP tests
> > execution / IO tests or
> > even on boot.
> >
> > I don't know exactly what breaks but bisect clearly assign the blame to
> > this commit:
> > 84c6591103db ("locking/atomics, asm-generic/bitops/lock.h: Rewrite using
> > atomic_fetch_*()")
> > https://github.com/torvalds/linux/commit/84c6591103dbeaf393a092a3fc7b09510825f6b9
> >
> > Reverting the commit solves this problem.
> >
> > I tested v4.19-rc1 on ARM (wandboard, i.mx6, 32bit, quard core, ARMv7)
> > which uses same
> > generic bitops implementation and it works fine.
> >
> > Do you have any ideas what went wrong?
>
> Back in 2016, Peter had fixed this file due to a problem I reported on ARC.
> See
> commit f75d48644c56a ("bitops: Do not default to __clear_bit() for
> __clear_bit_unlock()")
> That made __clear_bit_unlock() use the atomic clear_bit() vs. non-atomic
> __clear_bit(), effectively making clear_bit_unlock() and __clear_bit_unlock()
> same.
>
> This patch undoes that which could explain the issues you see. @Peter, @Will ?
/me grabs arc toolchain (incidentally, make.cross fuzzy matches "arc" to
"sparc", so that was fun for a few minutes).
I'll take a look today, thanks for the report.
Will
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 2/2] dt-bindings: Document the Synopsys GPIO via CREG bindings
On Tue, Aug 28, 2018 at 1:27 PM Eugeniy Paltsev wrote: > +++ b/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt > @@ -0,0 +1,49 @@ > +GPIO via CREG (Control REGisers) driver Speling Also should be "Synopsys GPIO via CREG" as this is likely just for Synopsys and not general purpose. > +This is is single-register MMIO GPIO driver to control such strangely mapped > +outputs: > + > +31118 75 0 < bit number > +| || || | > +[ not used | gpio-1 | shift-1 | gpio-0 | shift-0 ] < 32 bit MMIO > register > + ^ ^ > + | | > + | write 0x2 == set output to "1" (on) > + | write 0x3 == set output to "0" (off) > + | > +write 0x1 == set output to "1" (on) > +write 0x4 == set output to "0" (off) Move this documentation into the driver instead. > +Required properties: > +- compatible : "snps,creg-gpio" > +- reg : Exactly one register range with length 0x4. > +- #gpio-cells : Should be one - the pin number. > +- gpio-controller : Marks the device node as a GPIO controller. OK > +- snps,ngpios: Number of GPIO pins. Use the existing ngpios attribute for this, see gpio.txt > +- snps,bit-per-line: Number of bits per each gpio line (see picture). > + Array the size of "snps,ngpios" > +- snps,shift: Shift (in bits) of the each GPIO field from the previous one in > + register (see picture). Array the size of "snps,ngpios" > +- snps,on-val: Value should be set in corresponding field to set > + output to "1" (see picture). Array the size of "snps,ngpios" > +- snps,off-val: Value should be set in corresponding field to set > + output to "0" (see picture). Array the size of "snps,ngpios" Move this into a lookup table in the driver instead, and match the lookup table to the compatible string. The format of the register is known for a certain compatible, right? > +Optional properties: > +- snps,default-val: default output field values. Array the size of > "snps,ngpios" Default values for different lines can be achieved by hogs if it's OK to tie them up perpetually, else work on creating generic inialization values in gpio.txt and implement that in gpiolib-of.c for everyone. This discussion comes up from time to time. Yours, Linus Walleij ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
