Messages in this thread | | | Date | Wed, 20 Dec 2017 16:23:47 -0800 | From | Stephen Boyd <> | Subject | Re: [PATCH] clk: fix spin_lock/unlock imbalance on bad clk_enable() reentrancy |
| |
On 12/20, David Lechner wrote: > On 12/20/2017 02:33 PM, David Lechner wrote: > > > >So, as you can see, we get 4 warnings here. There is no problem > >with any clock provider or consumer (as far as I can tell). The > >bug here is that spin_trylock_irqsave() always returns true on > >non-SMP systems, which messes up the reference counting. > > > >usb20_phy_clk_enable() currently works because mach-davinci does > >not use the common clock framework. However, I am trying to move > >it to the common clock framework, which is how I discovered this > >bug. > > One more thing I mentioned previously, but is worth mentioning again > in detail is that if you enable CONFIG_DEBUG_SPINLOCK, that changes > the behavior of spin_trylock_irqsave() on non-SMP systems. It no > longer always returns true and so everything works as expected in > the call chain that I described previously. > > The difference is that with CONFIG_DEBUG_SPINLOCK=n, we have > > #define arch_spin_trylock(lock) ({ barrier(); (void)(lock); 1; }) > > But if CONFIG_DEBUG_SPINLOCK=y, then we have > > static inline int arch_spin_trylock(arch_spinlock_t *lock) > { > char oldval = lock->slock; > > lock->slock = 0; > barrier(); > > return oldval > 0; > } > > This comes from include/linux/spinlock_up.h, which is included from > include/linux/spinlock.h > > #ifdef CONFIG_SMP > # include <asm/spinlock.h> > #else > # include <linux/spinlock_up.h> > #endif > > > So, the question I have is: what is the actual "correct" behavior of > spin_trylock_irqsave()? Is it really supposed to always return true > when CONFIG_DEBUG_SPINLOCK=n and CONFIG_SMP=n or is this a bug?
Thanks for doing the analysis in this thread.
When CONFIG_DEBUG_SPINLOCK=n and CONFIG_SMP=n, spinlocks are compiler barriers, that's it. So even if it is a bug to always return true, I fail to see how we can detect that a spinlock is already held in this configuration and return true or false.
I suppose the best option is to make clk_enable_lock() and clk_enable_unlock() into nops or pure owner/refcount/barrier updates when CONFIG_SMP=n. We pretty much just need the barrier semantics when there's only a single CPU.
-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
| |