lkml.org 
[lkml]   [2017]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2017-12-21 01:24    [W:2.284 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site