Quoting Laurent Pinchart (2013-03-27 02:08:15)

Hi Mike,

Thanks for the patch.

Please see below for a couple of comments.

On Wednesday 27 March 2013 00:09:43 Mike Turquette wrote:

...

Reentrancy into the clock framework from the clk.h api is necessary for clocks that are prepared and unprepared via i2c_transfer (which includes many PMICs and discrete audio chips) as well as for several other use cases.

This patch implements reentrancy by adding two global atomic_t's which track the context of the current caller. Context in this case is the return value from get_current(). One context variable is for slow operations protected by the prepare_mutex and the other is for fast operations protected by the enable_lock spinlock.

The clk.h api implementations are modified to first see if the relevant global lock is already held and if so compare the global context (set by whoever is holding the lock) against their own context (via a call to get_current()). If the two match then this function is a nested call from the one already holding the lock and we procede. If the context does not match then procede to call mutex_lock and busy-wait for the existing task to complete.

This patch does not increase concurrency for unrelated calls into the clock framework. Instead it simply allows reentrancy by the single task which is currently holding the global clock framework lock.

Signed-off-by: Mike Turquette mturquette@linaro.org Cc: Rajagopal Venkat rajagopal.venkat@linaro.org Cc: David Brown davidb@codeaurora.org Cc: Ulf Hansson ulf.hansson@linaro.org Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

drivers/clk/clk.c | 255 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 186 insertions(+), 69 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 5e8ffff..17432a5 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -19,9 +19,12 @@ #include <linux/of.h> #include <linux/device.h> #include <linux/init.h> +#include <linux/sched.h>

static DEFINE_SPINLOCK(enable_lock); static DEFINE_MUTEX(prepare_lock); +static atomic_t prepare_context; +static atomic_t enable_context;

static HLIST_HEAD(clk_root_list); static HLIST_HEAD(clk_orphan_list);

[snip]

...

@@ -566,6 +548,35 @@ struct clk *__clk_lookup(const char *name) return NULL; }

+/*** locking & reentrancy ***/

+static void clk_fwk_lock(void) +{

• /* hold the framework-wide lock, context == NULL */
  

• mutex_lock(&prepare_lock);
  

• /* set context for any reentrant calls */
  

• atomic_set(&prepare_context, (int) get_current());
  

Won't that break on 64-bit architectures with sizeof(void *) != sizeof(int) ?

Ok, I can use atomic64_t in the next version. Good catch.

I wonder if it would make sense to abstract these operations in a generic recursive mutex. Given that it would delay this patch past v3.10 I won't push for that.

Having a nice implementation of recursive mutexes would have saved me some time.

...

+}

+static void clk_fwk_unlock(void) +{

• /* clear the context */
  

• atomic_set(&prepare_context, 0);
  

• /* release the framework-wide lock, context == NULL */
  

• mutex_unlock(&prepare_lock);
  

+}

+static bool clk_is_reentrant(void) +{

• if (mutex_is_locked(&prepare_lock))
  

•         if ((void *) atomic_read(&prepare_context) == get_current())
  

•                 return true;
  

• return false;
  

+}

/*** clk api ***/

void __clk_unprepare(struct clk *clk)

[snip]

...

@@ -877,6 +945,30 @@ static void __clk_recalc_rates(struct clk *clk, unsigned long msg) __clk_recalc_rates(child, msg); }

+unsigned long __clk_get_rate(struct clk *clk) +{

• unsigned long ret;
  

• if (!clk) {
  

•         ret = 0;
  

•         goto out;
  

You could return 0 directly here.

Call me crazy, but I prefer having a single return statement in a function if possible. That goes for the similar review comments below.

...

• }
  

• if (clk->flags & CLK_GET_RATE_NOCACHE)
  

•         __clk_recalc_rates(clk, 0);
  

• ret = clk->rate;
  

• if (clk->flags & CLK_IS_ROOT)
  

•         goto out;
  

And return ret here.

...

• if (!clk->parent)
  

•         ret = 0;
  

+out:

• return ret;
  

+}

/**

• clk_get_rate - return the rate of clk
• @clk: the clk whose rate is being returned

@@ -889,14 +981,22 @@ unsigned long clk_get_rate(struct clk *clk) { unsigned long rate;

• mutex_lock(&prepare_lock);
  

• /*
  

•  * FIXME - any locking here seems heavy weight
  

•  * can clk->rate be replaced with an atomic_t?
  

•  * same logic can likely be applied to prepare_count & enable_count
  

•  */
  
  

• if (clk && (clk->flags & CLK_GET_RATE_NOCACHE))
  

•         __clk_recalc_rates(clk, 0);
  

• if (clk_is_reentrant()) {
  

•         rate = __clk_get_rate(clk);
  

•         goto out;
  

You can return directly here as well.

...

• }
  
  

• clk_fwk_lock();
  rate = __clk_get_rate(clk);
  

• mutex_unlock(&prepare_lock);
  

• clk_fwk_unlock();
  
  

+out: return rate; } EXPORT_SYMBOL_GPL(clk_get_rate); @@ -1073,6 +1173,39 @@ static void clk_change_rate(struct clk *clk) clk_change_rate(child); }

+int __clk_set_rate(struct clk *clk, unsigned long rate) +{

• int ret = 0;
  

• struct clk *top, *fail_clk;
  

• /* bail early if nothing to do */
  

• if (rate == clk->rate)
  

•         return 0;
  

• if ((clk->flags & CLK_SET_RATE_GATE) && clk->prepare_count) {
  

•         return -EBUSY;
  

• }
  

Braces are not necessary.

No harm in having them, but I can remove them in the next version.

Thanks for the review, Mike

...

• /* calculate new rates and get the topmost changed clock */
  

• top = clk_calc_new_rates(clk, rate);
  

• if (!top)
  

•         return -EINVAL;
  

• /* notify that we are about to change rates */
  

• fail_clk = clk_propagate_rate_change(top, PRE_RATE_CHANGE);
  

• if (fail_clk) {
  

•         pr_warn("%s: failed to set %s rate\n", __func__,
  

•                         fail_clk->name);
  

•         clk_propagate_rate_change(top, ABORT_RATE_CHANGE);
  

•         return -EBUSY;
  

• }
  

• /* change the rates */
  

• clk_change_rate(top);
  

• return ret;
  

+}

/**

• clk_set_rate - specify a new rate for clk
• @clk: the clk whose rate is being changed

-- Regards,

Laurent Pinchart

Create locking helpers for the global mutex and global spinlock. The definitions of these helpers will be expanded upon in the next patch which introduces reentrancy into the locking scheme.

Signed-off-by: Mike Turquette mturquette@linaro.org Cc: Rajagopal Venkat rajagopal.venkat@linaro.org Cc: David Brown davidb@codeaurora.org Cc: Ulf Hansson ulf.hansson@linaro.org Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Thomas Gleixner tglx@linutronix.de --- drivers/clk/clk.c | 97 ++++++++++++++++++++++++++++++++--------------------- 1 file changed, 59 insertions(+), 38 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 5e8ffff..bea47d5 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -27,6 +27,27 @@ static HLIST_HEAD(clk_root_list); static HLIST_HEAD(clk_orphan_list); static LIST_HEAD(clk_notifier_list);

+/*** locking ***/ +static void clk_prepare_lock(void) +{ + mutex_lock(&prepare_lock); +} + +static void clk_prepare_unlock(void) +{ + mutex_unlock(&prepare_lock); +} + +static void clk_enable_lock(unsigned long *flags) +{ + spin_lock_irqsave(&enable_lock, *flags); +} + +static void clk_enable_unlock(unsigned long *flags) +{ + spin_unlock_irqrestore(&enable_lock, *flags); +} + /*** debugfs support ***/

#ifdef CONFIG_COMMON_CLK_DEBUG @@ -69,7 +90,7 @@ static int clk_summary_show(struct seq_file *s, void *data) seq_printf(s, " clock enable_cnt prepare_cnt rate\n"); seq_printf(s, "---------------------------------------------------------------------\n");

- mutex_lock(&prepare_lock); + clk_prepare_lock();

hlist_for_each_entry(c, &clk_root_list, child_node) clk_summary_show_subtree(s, c, 0); @@ -77,7 +98,7 @@ static int clk_summary_show(struct seq_file *s, void *data) hlist_for_each_entry(c, &clk_orphan_list, child_node) clk_summary_show_subtree(s, c, 0);

- mutex_unlock(&prepare_lock); + clk_prepare_unlock();

return 0; } @@ -130,7 +151,7 @@ static int clk_dump(struct seq_file *s, void *data)

seq_printf(s, "{");

- mutex_lock(&prepare_lock); + clk_prepare_lock();

hlist_for_each_entry(c, &clk_root_list, child_node) { if (!first_node) @@ -144,7 +165,7 @@ static int clk_dump(struct seq_file *s, void *data) clk_dump_subtree(s, c, 0); }

- mutex_unlock(&prepare_lock); + clk_prepare_unlock();

seq_printf(s, "}"); return 0; @@ -316,7 +337,7 @@ static int __init clk_debug_init(void) if (!orphandir) return -ENOMEM;

- mutex_lock(&prepare_lock); + clk_prepare_lock();

hlist_for_each_entry(clk, &clk_root_list, child_node) clk_debug_create_subtree(clk, rootdir); @@ -326,7 +347,7 @@ static int __init clk_debug_init(void)

inited = 1;

- mutex_unlock(&prepare_lock); + clk_prepare_unlock();

return 0; } @@ -372,7 +393,7 @@ static void clk_disable_unused_subtree(struct clk *clk) hlist_for_each_entry(child, &clk->children, child_node) clk_disable_unused_subtree(child);

- spin_lock_irqsave(&enable_lock, flags); + clk_enable_lock(&flags);

if (clk->enable_count) goto unlock_out; @@ -393,7 +414,7 @@ static void clk_disable_unused_subtree(struct clk *clk) }

unlock_out: - spin_unlock_irqrestore(&enable_lock, flags); + clk_enable_unlock(&flags);

out: return; @@ -403,7 +424,7 @@ static int clk_disable_unused(void) { struct clk *clk;

- mutex_lock(&prepare_lock); + clk_prepare_lock();

hlist_for_each_entry(clk, &clk_root_list, child_node) clk_disable_unused_subtree(clk); @@ -417,7 +438,7 @@ static int clk_disable_unused(void) hlist_for_each_entry(clk, &clk_orphan_list, child_node) clk_unprepare_unused_subtree(clk);

- mutex_unlock(&prepare_lock); + clk_prepare_unlock();

return 0; } @@ -600,9 +621,9 @@ void __clk_unprepare(struct clk *clk) */ void clk_unprepare(struct clk *clk) { - mutex_lock(&prepare_lock); + clk_prepare_lock(); __clk_unprepare(clk); - mutex_unlock(&prepare_lock); + clk_prepare_unlock(); } EXPORT_SYMBOL_GPL(clk_unprepare);

@@ -648,9 +669,9 @@ int clk_prepare(struct clk *clk) { int ret;

- mutex_lock(&prepare_lock); + clk_prepare_lock(); ret = __clk_prepare(clk); - mutex_unlock(&prepare_lock); + clk_prepare_unlock();

return ret; } @@ -692,9 +713,9 @@ void clk_disable(struct clk *clk) { unsigned long flags;

- spin_lock_irqsave(&enable_lock, flags); + clk_enable_lock(&flags); __clk_disable(clk); - spin_unlock_irqrestore(&enable_lock, flags); + clk_enable_unlock(&flags); } EXPORT_SYMBOL_GPL(clk_disable);

@@ -745,9 +766,9 @@ int clk_enable(struct clk *clk) unsigned long flags; int ret;

- spin_lock_irqsave(&enable_lock, flags); + clk_enable_lock(&flags); ret = __clk_enable(clk); - spin_unlock_irqrestore(&enable_lock, flags); + clk_enable_unlock(&flags);

return ret; } @@ -792,9 +813,9 @@ long clk_round_rate(struct clk *clk, unsigned long rate) { unsigned long ret;

- mutex_lock(&prepare_lock); + clk_prepare_lock(); ret = __clk_round_rate(clk, rate); - mutex_unlock(&prepare_lock); + clk_prepare_unlock();

return ret; } @@ -889,13 +910,13 @@ unsigned long clk_get_rate(struct clk *clk) { unsigned long rate;

- mutex_lock(&prepare_lock); + clk_prepare_lock();

if (clk && (clk->flags & CLK_GET_RATE_NOCACHE)) __clk_recalc_rates(clk, 0);

rate = __clk_get_rate(clk); - mutex_unlock(&prepare_lock); + clk_prepare_unlock();

return rate; } @@ -1100,7 +1121,7 @@ int clk_set_rate(struct clk *clk, unsigned long rate) int ret = 0;

/* prevent racing with updates to the clock topology */ - mutex_lock(&prepare_lock); + clk_prepare_lock();

/* bail early if nothing to do */ if (rate == clk->rate) @@ -1132,7 +1153,7 @@ int clk_set_rate(struct clk *clk, unsigned long rate) clk_change_rate(top);

out: - mutex_unlock(&prepare_lock); + clk_prepare_unlock();

return ret; } @@ -1148,9 +1169,9 @@ struct clk *clk_get_parent(struct clk *clk) { struct clk *parent;

- mutex_lock(&prepare_lock); + clk_prepare_lock(); parent = __clk_get_parent(clk); - mutex_unlock(&prepare_lock); + clk_prepare_unlock();

return parent; } @@ -1294,19 +1315,19 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent) __clk_prepare(parent);

/* FIXME replace with clk_is_enabled(clk) someday */ - spin_lock_irqsave(&enable_lock, flags); + clk_enable_lock(&flags); if (clk->enable_count) __clk_enable(parent); - spin_unlock_irqrestore(&enable_lock, flags); + clk_enable_unlock(&flags);

/* change clock input source */ ret = clk->ops->set_parent(clk->hw, i);

/* clean up old prepare and enable */ - spin_lock_irqsave(&enable_lock, flags); + clk_enable_lock(&flags); if (clk->enable_count) __clk_disable(old_parent); - spin_unlock_irqrestore(&enable_lock, flags); + clk_enable_unlock(&flags);

if (clk->prepare_count) __clk_unprepare(old_parent); @@ -1338,7 +1359,7 @@ int clk_set_parent(struct clk *clk, struct clk *parent) return -ENOSYS;

/* prevent racing with updates to the clock topology */ - mutex_lock(&prepare_lock); + clk_prepare_lock();

if (clk->parent == parent) goto out; @@ -1367,7 +1388,7 @@ int clk_set_parent(struct clk *clk, struct clk *parent) __clk_reparent(clk, parent);

out: - mutex_unlock(&prepare_lock); + clk_prepare_unlock();

return ret; } @@ -1390,7 +1411,7 @@ int __clk_init(struct device *dev, struct clk *clk) if (!clk) return -EINVAL;

- mutex_lock(&prepare_lock); + clk_prepare_lock();

/* check to see if a clock with this name is already registered */ if (__clk_lookup(clk->name)) { @@ -1514,7 +1535,7 @@ int __clk_init(struct device *dev, struct clk *clk) clk_debug_register(clk);

out: - mutex_unlock(&prepare_lock); + clk_prepare_unlock();

return ret; } @@ -1748,7 +1769,7 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb) if (!clk || !nb) return -EINVAL;

- mutex_lock(&prepare_lock); + clk_prepare_lock();

/* search the list of notifiers for this clk */ list_for_each_entry(cn, &clk_notifier_list, node) @@ -1772,7 +1793,7 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb) clk->notifier_count++;

out: - mutex_unlock(&prepare_lock); + clk_prepare_unlock();

return ret; } @@ -1797,7 +1818,7 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb) if (!clk || !nb) return -EINVAL;

- mutex_lock(&prepare_lock); + clk_prepare_lock();

list_for_each_entry(cn, &clk_notifier_list, node) if (cn->clk == clk) @@ -1818,7 +1839,7 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb) ret = -ENOENT; }

- mutex_unlock(&prepare_lock); + clk_prepare_unlock();

return ret; }

Reentrancy into the clock framework is necessary for clock operations that result in nested calls to the clk api. A common example is a clock that is prepared via an i2c transaction, such as a clock inside of a discrete audio chip or a power management IC. The i2c subsystem itself will use the clk api resulting in a deadlock:

clk_prepare(audio_clk) i2c_transfer(..) clk_prepare(i2c_controller_clk)

The ability to reenter the clock framework prevents this deadlock.

Other use cases exist such as allowing .set_rate callbacks to call clk_set_parent to achieve the best rate, or to save power in certain configurations. Yet another example is performing pinctrl operations from a clk_ops callback. Calls into the pinctrl subsystem may call clk_{un}prepare on an unrelated clock. Allowing for nested calls to reenter the clock framework enables both of these use cases.

Reentrancy is implemented by two global pointers that track the owner currently holding a global lock. One pointer tracks the owner during sleepable, mutex-protected operations and the other one tracks the owner during non-interruptible, spinlock-protected operations.

When the clk framework is entered we try to hold the global lock. If it is held we compare the current task id against the current owner; a match implies a nested call and we reenter. If the values do not match then we block on the lock until it is released.

Signed-off-by: Mike Turquette mturquette@linaro.org Cc: Rajagopal Venkat rajagopal.venkat@linaro.org Cc: David Brown davidb@codeaurora.org Cc: Ulf Hansson ulf.hansson@linaro.org Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Thomas Gleixner tglx@linutronix.de --- Changes since v4: * remove uneccesary atomic operations * remove casting bugs * place reentrancy logic into locking helper functions * improve debugging with reference counting and WARNs

drivers/clk/clk.c | 43 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index bea47d5..fe7c054 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -19,10 +19,17 @@ #include <linux/of.h> #include <linux/device.h> #include <linux/init.h> +#include <linux/sched.h>

static DEFINE_SPINLOCK(enable_lock); static DEFINE_MUTEX(prepare_lock);

+static struct task_struct *prepare_owner; +static struct task_struct *enable_owner; + +static int prepare_refcnt; +static int enable_refcnt; + static HLIST_HEAD(clk_root_list); static HLIST_HEAD(clk_orphan_list); static LIST_HEAD(clk_notifier_list); @@ -30,21 +37,53 @@ static LIST_HEAD(clk_notifier_list); /*** locking ***/ static void clk_prepare_lock(void) { - mutex_lock(&prepare_lock); + if (!mutex_trylock(&prepare_lock)) { + if (prepare_owner == current) { + prepare_refcnt++; + return; + } + mutex_lock(&prepare_lock); + } + WARN_ON_ONCE(prepare_owner != NULL); + WARN_ON_ONCE(prepare_refcnt != 0); + prepare_owner = current; + prepare_refcnt = 1; }

static void clk_prepare_unlock(void) { + WARN_ON_ONCE(prepare_owner != current); + WARN_ON_ONCE(prepare_refcnt == 0); + + if (--prepare_refcnt) + return; + prepare_owner = NULL; mutex_unlock(&prepare_lock); }

static void clk_enable_lock(unsigned long *flags) { - spin_lock_irqsave(&enable_lock, *flags); + if (!spin_trylock_irqsave(&enable_lock, *flags)) { + if (enable_owner == current) { + enable_refcnt++; + return; + } + spin_lock_irqsave(&enable_lock, *flags); + } + WARN_ON_ONCE(enable_owner != NULL); + WARN_ON_ONCE(enable_refcnt != 0); + enable_owner = current; + enable_refcnt = 1; }

static void clk_enable_unlock(unsigned long *flags) { + WARN_ON_ONCE(enable_owner != current); + WARN_ON_ONCE(enable_refcnt == 0); + + if (--enable_refcnt) + return; + enable_owner = NULL; spin_unlock_irqrestore(&enable_lock, *flags); }

On 28 March 2013 21:59, Mike Turquette mturquette@linaro.org wrote:

Create locking helpers for the global mutex and global spinlock. The definitions of these helpers will be expanded upon in the next patch which introduces reentrancy into the locking scheme.

Signed-off-by: Mike Turquette mturquette@linaro.org Cc: Rajagopal Venkat rajagopal.venkat@linaro.org Cc: David Brown davidb@codeaurora.org Cc: Ulf Hansson ulf.hansson@linaro.org Tested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Thomas Gleixner tglx@linutronix.de

---

Changes since v5:

• pass flags by value instead of by reference in clk_enable_{un}lock

drivers/clk/clk.c | 99 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 61 insertions(+), 38 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 5e8ffff..0b5d612 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -27,6 +27,29 @@ static HLIST_HEAD(clk_root_list); static HLIST_HEAD(clk_orphan_list); static LIST_HEAD(clk_notifier_list);

+/*** locking ***/ +static void clk_prepare_lock(void) +{

•   mutex_lock(&prepare_lock);
  

+}

+static void clk_prepare_unlock(void) +{

•   mutex_unlock(&prepare_lock);
  

+}

+static unsigned long clk_enable_lock(void) +{

•   unsigned long flags;
  

•   spin_lock_irqsave(&enable_lock, flags);
  

•   return flags;
  

+}

+static void clk_enable_unlock(unsigned long flags) +{

•   spin_unlock_irqrestore(&enable_lock, flags);
  

+}

/*** debugfs support ***/

#ifdef CONFIG_COMMON_CLK_DEBUG @@ -69,7 +92,7 @@ static int clk_summary_show(struct seq_file *s, void *data) seq_printf(s, " clock enable_cnt prepare_cnt rate\n"); seq_printf(s, "---------------------------------------------------------------------\n");

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
  
    hlist_for_each_entry(c, &clk_root_list, child_node)
            clk_summary_show_subtree(s, c, 0);
  

@@ -77,7 +100,7 @@ static int clk_summary_show(struct seq_file *s, void *data) hlist_for_each_entry(c, &clk_orphan_list, child_node) clk_summary_show_subtree(s, c, 0);

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  
    return 0;
  

} @@ -130,7 +153,7 @@ static int clk_dump(struct seq_file *s, void *data)

    seq_printf(s, "{");

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
  
    hlist_for_each_entry(c, &clk_root_list, child_node) {
            if (!first_node)
  

@@ -144,7 +167,7 @@ static int clk_dump(struct seq_file *s, void *data) clk_dump_subtree(s, c, 0); }

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  
    seq_printf(s, "}");
    return 0;
  

@@ -316,7 +339,7 @@ static int __init clk_debug_init(void) if (!orphandir) return -ENOMEM;

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
  
    hlist_for_each_entry(clk, &clk_root_list, child_node)
            clk_debug_create_subtree(clk, rootdir);
  

@@ -326,7 +349,7 @@ static int __init clk_debug_init(void)

    inited = 1;

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  
    return 0;
  

} @@ -372,7 +395,7 @@ static void clk_disable_unused_subtree(struct clk *clk) hlist_for_each_entry(child, &clk->children, child_node) clk_disable_unused_subtree(child);

•   spin_lock_irqsave(&enable_lock, flags);
  

•   flags = clk_enable_lock();
  
    if (clk->enable_count)
            goto unlock_out;
  

@@ -393,7 +416,7 @@ static void clk_disable_unused_subtree(struct clk *clk) }

unlock_out:

•   spin_unlock_irqrestore(&enable_lock, flags);
  

•   clk_enable_unlock(flags);
  
  

out: return; @@ -403,7 +426,7 @@ static int clk_disable_unused(void) { struct clk *clk;

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
  
    hlist_for_each_entry(clk, &clk_root_list, child_node)
            clk_disable_unused_subtree(clk);
  

@@ -417,7 +440,7 @@ static int clk_disable_unused(void) hlist_for_each_entry(clk, &clk_orphan_list, child_node) clk_unprepare_unused_subtree(clk);

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  
    return 0;
  

} @@ -600,9 +623,9 @@ void __clk_unprepare(struct clk *clk) */ void clk_unprepare(struct clk *clk) {

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
    __clk_unprepare(clk);
  

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  

} EXPORT_SYMBOL_GPL(clk_unprepare);

@@ -648,9 +671,9 @@ int clk_prepare(struct clk *clk) { int ret;

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
    ret = __clk_prepare(clk);
  

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  
    return ret;
  

} @@ -692,9 +715,9 @@ void clk_disable(struct clk *clk) { unsigned long flags;

•   spin_lock_irqsave(&enable_lock, flags);
  

•   flags = clk_enable_lock();
    __clk_disable(clk);
  

•   spin_unlock_irqrestore(&enable_lock, flags);
  

•   clk_enable_unlock(flags);
  

} EXPORT_SYMBOL_GPL(clk_disable);

@@ -745,9 +768,9 @@ int clk_enable(struct clk *clk) unsigned long flags; int ret;

•   spin_lock_irqsave(&enable_lock, flags);
  

•   flags = clk_enable_lock();
    ret = __clk_enable(clk);
  

•   spin_unlock_irqrestore(&enable_lock, flags);
  

•   clk_enable_unlock(flags);
  
    return ret;
  

} @@ -792,9 +815,9 @@ long clk_round_rate(struct clk *clk, unsigned long rate) { unsigned long ret;

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
    ret = __clk_round_rate(clk, rate);
  

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  
    return ret;
  

} @@ -889,13 +912,13 @@ unsigned long clk_get_rate(struct clk *clk) { unsigned long rate;

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
  
    if (clk && (clk->flags & CLK_GET_RATE_NOCACHE))
            __clk_recalc_rates(clk, 0);
  
    rate = __clk_get_rate(clk);
  

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  
    return rate;
  

} @@ -1100,7 +1123,7 @@ int clk_set_rate(struct clk *clk, unsigned long rate) int ret = 0;

    /* prevent racing with updates to the clock topology */

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
  
    /* bail early if nothing to do */
    if (rate == clk->rate)
  

@@ -1132,7 +1155,7 @@ int clk_set_rate(struct clk *clk, unsigned long rate) clk_change_rate(top);

out:

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  
    return ret;
  

} @@ -1148,9 +1171,9 @@ struct clk *clk_get_parent(struct clk *clk) { struct clk *parent;

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
    parent = __clk_get_parent(clk);
  

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  
    return parent;
  

} @@ -1294,19 +1317,19 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent) __clk_prepare(parent);

    /* FIXME replace with clk_is_enabled(clk) someday */

•   spin_lock_irqsave(&enable_lock, flags);
  

•   flags = clk_enable_lock();
    if (clk->enable_count)
            __clk_enable(parent);
  

•   spin_unlock_irqrestore(&enable_lock, flags);
  

•   clk_enable_unlock(flags);
  
    /* change clock input source */
    ret = clk->ops->set_parent(clk->hw, i);
  
    /* clean up old prepare and enable */
  

•   spin_lock_irqsave(&enable_lock, flags);
  

•   flags = clk_enable_lock();
    if (clk->enable_count)
            __clk_disable(old_parent);
  

•   spin_unlock_irqrestore(&enable_lock, flags);
  

•   clk_enable_unlock(flags);
  
    if (clk->prepare_count)
            __clk_unprepare(old_parent);
  

@@ -1338,7 +1361,7 @@ int clk_set_parent(struct clk *clk, struct clk *parent) return -ENOSYS;

    /* prevent racing with updates to the clock topology */

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
  
    if (clk->parent == parent)
            goto out;
  

@@ -1367,7 +1390,7 @@ int clk_set_parent(struct clk *clk, struct clk *parent) __clk_reparent(clk, parent);

out:

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  
    return ret;
  

} @@ -1390,7 +1413,7 @@ int __clk_init(struct device *dev, struct clk *clk) if (!clk) return -EINVAL;

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
  
    /* check to see if a clock with this name is already registered */
    if (__clk_lookup(clk->name)) {
  

@@ -1514,7 +1537,7 @@ int __clk_init(struct device *dev, struct clk *clk) clk_debug_register(clk);

out:

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  
    return ret;
  

} @@ -1748,7 +1771,7 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb) if (!clk || !nb) return -EINVAL;

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
  
    /* search the list of notifiers for this clk */
    list_for_each_entry(cn, &clk_notifier_list, node)
  

@@ -1772,7 +1795,7 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb) clk->notifier_count++;

out:

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  
    return ret;
  

} @@ -1797,7 +1820,7 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb) if (!clk || !nb) return -EINVAL;

•   mutex_lock(&prepare_lock);
  

•   clk_prepare_lock();
  
    list_for_each_entry(cn, &clk_notifier_list, node)
            if (cn->clk == clk)
  

@@ -1818,7 +1841,7 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb) ret = -ENOENT; }

•   mutex_unlock(&prepare_lock);
  

•   clk_prepare_unlock();
  
    return ret;
  

}

1.7.10.4

Reviewed-by: Ulf Hansson ulf.hansson@linaro.org

On 28 March 2013 21:59, Mike Turquette mturquette@linaro.org wrote:

Reentrancy into the clock framework is necessary for clock operations that result in nested calls to the clk api. A common example is a clock that is prepared via an i2c transaction, such as a clock inside of a discrete audio chip or a power management IC. The i2c subsystem itself will use the clk api resulting in a deadlock:

clk_prepare(audio_clk) i2c_transfer(..) clk_prepare(i2c_controller_clk)

The ability to reenter the clock framework prevents this deadlock.

Other use cases exist such as allowing .set_rate callbacks to call clk_set_parent to achieve the best rate, or to save power in certain configurations. Yet another example is performing pinctrl operations from a clk_ops callback. Calls into the pinctrl subsystem may call clk_{un}prepare on an unrelated clock. Allowing for nested calls to reenter the clock framework enables both of these use cases.

Reentrancy is implemented by two global pointers that track the owner currently holding a global lock. One pointer tracks the owner during sleepable, mutex-protected operations and the other one tracks the owner during non-interruptible, spinlock-protected operations.

When the clk framework is entered we try to hold the global lock. If it is held we compare the current task against the current owner; a match implies a nested call and we reenter. If the values do not match then we block on the lock until it is released.

Signed-off-by: Mike Turquette mturquette@linaro.org Cc: Rajagopal Venkat rajagopal.venkat@linaro.org Cc: David Brown davidb@codeaurora.org Cc: Ulf Hansson ulf.hansson@linaro.org Tested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Thomas Gleixner tglx@linutronix.de

---

Changes since v5:

• fixed up typo in changelog

Changes since v4:

• remove uneccesary atomic operations
• remove casting bugs
• place reentrancy logic into locking helper functions
• improve debugging with reference counting and WARNs

drivers/clk/clk.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 0b5d612..0230c9d 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -19,10 +19,17 @@ #include <linux/of.h> #include <linux/device.h> #include <linux/init.h> +#include <linux/sched.h>

static DEFINE_SPINLOCK(enable_lock); static DEFINE_MUTEX(prepare_lock);

+static struct task_struct *prepare_owner; +static struct task_struct *enable_owner;

+static int prepare_refcnt; +static int enable_refcnt;

static HLIST_HEAD(clk_root_list); static HLIST_HEAD(clk_orphan_list); static LIST_HEAD(clk_notifier_list); @@ -30,23 +37,56 @@ static LIST_HEAD(clk_notifier_list); /*** locking ***/ static void clk_prepare_lock(void) {

•   mutex_lock(&prepare_lock);
  

•   if (!mutex_trylock(&prepare_lock)) {
  

•           if (prepare_owner == current) {
  

•                   prepare_refcnt++;
  

•                   return;
  

•           }
  

•           mutex_lock(&prepare_lock);
  

•   }
  

•   WARN_ON_ONCE(prepare_owner != NULL);
  

•   WARN_ON_ONCE(prepare_refcnt != 0);
  

•   prepare_owner = current;
  

•   prepare_refcnt = 1;
  

}

static void clk_prepare_unlock(void) {

•   WARN_ON_ONCE(prepare_owner != current);
  

•   WARN_ON_ONCE(prepare_refcnt == 0);
  

•   if (--prepare_refcnt)
  

•           return;
  

•   prepare_owner = NULL;
    mutex_unlock(&prepare_lock);
  

}

static unsigned long clk_enable_lock(void) { unsigned long flags;

•   spin_lock_irqsave(&enable_lock, flags);
  

•   if (!spin_trylock_irqsave(&enable_lock, flags)) {
  

•           if (enable_owner == current) {
  

•                   enable_refcnt++;
  

•                   return flags;
  

•           }
  

•           spin_lock_irqsave(&enable_lock, flags);
  

•   }
  

•   WARN_ON_ONCE(enable_owner != NULL);
  

•   WARN_ON_ONCE(enable_refcnt != 0);
  

•   enable_owner = current;
  

•   enable_refcnt = 1;
    return flags;
  

}

static void clk_enable_unlock(unsigned long flags) {

•   WARN_ON_ONCE(enable_owner != current);
  

•   WARN_ON_ONCE(enable_refcnt == 0);
  

•   if (--enable_refcnt)
  

•           return;
  

•   enable_owner = NULL;
    spin_unlock_irqrestore(&enable_lock, flags);
  

}

-- 1.7.10.4

Great piece of work!

Reviewed-by: Ulf Hansson ulf.hansson@linaro.org