| Message ID | 20171227022154.GA7997@codeaurora.org (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On 12/26/2017 08:21 PM, Stephen Boyd wrote: > On 12/26, David Lechner wrote: >> Reentrant calls to clk_enable() are not working on UP systems. This is >> caused by the fact spin_trylock_irqsave() always returns true when >> CONFIG_SMP=n (and CONFIG_DEBUG_SPINLOCK=n) which causes the reference >> counting to not work correctly when clk_enable_lock() is called twice >> before clk_enable_unlock() is called (this happens when clk_enable() >> is called from within another clk_enable()). >> >> This introduces a new set of clk_enable_lock() and clk_enable_unlock() >> functions for UP systems that doesn't use spinlocks but effectively does >> the same thing as the SMP version of the functions. >> >> Signed-off-by: David Lechner <david@lechnology.com> >> --- >> >> Previous discussion of this issue for reference: >> * https://patchwork.kernel.org/patch/10108437/ >> * https://patchwork.kernel.org/patch/10115483/ >> >> >> drivers/clk/clk.c | 39 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index bb1b1f9..259a77f 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -136,6 +136,8 @@ static void clk_prepare_unlock(void) >> mutex_unlock(&prepare_lock); >> } >> >> +#ifdef CONFIG_SMP >> + >> static unsigned long clk_enable_lock(void) >> __acquires(enable_lock) >> { >> @@ -170,6 +172,43 @@ static void clk_enable_unlock(unsigned long flags) >> spin_unlock_irqrestore(&enable_lock, flags); >> } >> >> +#else >> + >> +static unsigned long clk_enable_lock(void) >> + __acquires(enable_lock) >> +{ >> + unsigned long flags; >> + >> + local_irq_save(flags); >> + preempt_disable(); > > Well we certainly don't want to do both irq save and preemption > disabling. If you don't mind humoring my ignorance, why not? This is exactly what spin_lock_irqsave() does on a UP system, which is where I got if from. I'm not arguing that is is right or wrong, just hoping to learn something. > Really all we need to do is save away the current > flags to restore them them later. On UP systems, we would do that > on each call to this function. On SMP systems, we would actually > test the enable_owner and up the enable_refcnt outside of the > locked section because if spin_trylock_irqsave() returns a 0, we > have restored the irq flags to whatever they were before, and > then we test enable_owner. So I suppose if it was safe on SMP to > test enable_owner and up the refcnt without holding any sort of > lock then it's also safe on UP, because current is a "special" > variable. > > Does this ugly hack do the trick? Yes, it does the trick, but the local_save_flags() seems pointless. On the first call, enable_owner == NULL, so spin_lock_irqsave() is called, which writes over the flags variable. For any later calls where enable_owner == current, the flags variable is not used in clk_enable_unlock() - the reference count is just decreased. The flags variable is only used in clk_enable_unlock() in the release of the last reference, in which the flags variable will be the one set by spin_lock_irqsave() in clk_enable_lock(). > I really dislike the semi-colon > barf, but I fail to see another way out of this right now. > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index b56c11f51baf..77b2c5bbba68 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -141,7 +141,8 @@ static unsigned long clk_enable_lock(void) > { > unsigned long flags; > > - if (!spin_trylock_irqsave(&enable_lock, flags)) { > + if ((!IS_ENABLED(CONFIG_SMP) && ({local_save_flags(flags); 1;})) || > + !spin_trylock_irqsave(&enable_lock, flags)) { > if (enable_owner == current) { > enable_refcnt++; > __acquire(enable_lock); > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index b56c11f51baf..77b2c5bbba68 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -141,7 +141,8 @@ static unsigned long clk_enable_lock(void) { unsigned long flags; - if (!spin_trylock_irqsave(&enable_lock, flags)) { + if ((!IS_ENABLED(CONFIG_SMP) && ({local_save_flags(flags); 1;})) || + !spin_trylock_irqsave(&enable_lock, flags)) { if (enable_owner == current) { enable_refcnt++; __acquire(enable_lock);