Re: [RFC][PATCH] mips: Fix arch_spin_unlock()

[Paul E. McKenney]

On Thu, Nov 12, 2015 at 02:50:00PM +0000, Måns Rullgård wrote:
> "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx> writes:
> 
> > On Thu, Nov 12, 2015 at 01:31:23PM +0100, Peter Zijlstra wrote:
> >> Hi
> >> 
> >> I think the MIPS arch_spin_unlock() is borken.
> >> 
> >> spin_unlock() must have RELEASE semantics, these require that no LOADs
> >> nor STOREs leak out from the critical section.
> >> 
> >> >From what I know MIPS has a relaxed memory model which allows reads to
> >> pass stores, and as implemented arch_spin_unlock() only issues a wmb
> >> which doesn't order prior reads vs later stores.
> >> 
> >> Therefore upgrade the wmb() to smp_mb().
> >> 
> >> (Also, why the unconditional wmb, as opposed to smp_wmb() ?)
> >
> > One guess is that they want to order I/O accesses within the critical
> > section?
> 
> Isn't that what mmiowb() is for?

Indeed it is.  Perhaps they didn't trust the device drivers that they
care about to use it?  Anyway, just my guess.  Just out of curiosity,
what is your guess?

							Thanx, Paul

> >> Maybe-Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> >> ---
> >> diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
> >> index 40196bebe849..b2ca13f06152 100644
> >> --- a/arch/mips/include/asm/spinlock.h
> >> +++ b/arch/mips/include/asm/spinlock.h
> >> @@ -140,7 +140,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
> >>  static inline void arch_spin_unlock(arch_spinlock_t *lock)
> >>  {
> >>  	unsigned int serving_now = lock->h.serving_now + 1;
> >> -	wmb();
> >> +	smp_mb();
> >>  	lock->h.serving_now = (u16)serving_now;
> >>  	nudge_writes();
> >>  }
> >> 
> >
> 
> -- 
> Måns Rullgård
> mans@xxxxxxxxx
> 

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