lkml.org 
[lkml]   [2017]   [Dec]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] clk: fix reentrancy of clk_enable() on UP systems
From
Date
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);
>


\
 
 \ /
  Last update: 2017-12-29 05:27    [W:0.326 / U:0.520 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site