lkml.org 
[lkml]   [2012]   [Aug]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 1/4] [RFC] clk: new locking scheme for reentrancy
From
Hi Mike,

On Thu, Aug 16, 2012 at 5:13 AM, Mike Turquette <mturquette@linaro.org> wrote:
> The global prepare_lock mutex prevents concurrent operations in the clk
> api. This incurs a performance penalty when unrelated clock subtrees
> are contending for the lock.
>
> Additionally there are use cases which benefit from reentrancy into the
> clk api. A simple example is reparenting a mux clock with a call to
> clk_set_rate. Patch #4 in this series demonstrates this without the use
> of internal helper functions.
>
> A more complex example is performing dynamic frequency and voltage
> scaling from clk_set_rate. Patches #2 and #3 in this series demonstrate
> this.
>
> This commit affects users of the global prepare_lock mutex, namely
> clk_prepare, clk_unprepare, clk_set_rate and clk_set_parent.
>
> This patch introduces an enum inside of struct clk which tracks whether
> the framework has LOCKED or UNLOCKED the clk.
>
> Access to clk->state must be protected by the global prepare_lock mutex.
> In the future maybe the global mutex can be dropped and all operations
> will only use a global spinlock to protect access to the per-clk enums.
> A general outline of the new locking scheme is as follows:
>
> 1) hold the global prepare_lock mutex
> 2) traverse the tree checking to make sure that any clk's needed are
> UNLOCKED and not LOCKED
> a) if a clk in the subtree of affected clk's is LOCKED then
> release the global lock, wait_for_completion and then try
> again up to a maximum of WAIT_TRIES times
> b) After WAIT_TRIES times return -EBUSY
> 3) if all clk's are UNLOCKED then mark them as LOCKED
> 4) release the global prepare_lock mutex
> 5) do the real work
> 6) hold the global prepare_lock mutex
> 7) set all of the clocks previously marked as LOCKED to UNLOCKED
> 8) release the global prepare_lock mutex and return
>
> The primary down-side to this approach is that the clk api's might
> return -EBUSY due to lock contention. This is only after having tried
> several times. Bailing out like this is necessary to prevent deadlocks.
>
> The enum approach in this version of the patchset does not benefit from
> lockdep checking the lock order (but neither did v1). It is possible
> for circular dependencies to pop up for the careless developer and
> bailing out after a number of unsuccessful tries is the sanest strategy.
>
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> ---
> drivers/clk/clk.c | 354 +++++++++++++++++++++++++++++++++++++-----
> include/linux/clk-private.h | 1 +
> include/linux/clk-provider.h | 4 +-
> 3 files changed, 318 insertions(+), 41 deletions(-)
>

> +}
> +
> +void __clk_unprepare(struct clk *clk, struct clk *top)

Why do you need to change the signature of __clk_prepare and
__clk_unprepare functions ?
I mean i did not understand the use of passing struct clk *top? As i
understand, it tells when you reach at the last
clk struct in the tree which needs to be prepared/unprepared. Do we
have extra benefit of this or if i am missing something?

> +{
> if (clk->ops->unprepare)
> clk->ops->unprepare(clk->hw);
>
> - __clk_unprepare(clk->parent);
> + if (clk != top)
> + __clk_unprepare(clk->parent, top);
> +}
> +
> +static void __clk_prepare_unlock(struct clk *clk, struct clk *top)
> +{
> + clk->state = UNLOCKED;
> +
> + if (clk != top)
> + __clk_prepare_unlock(clk->parent, top);
> }
>
> /**
> @@ -404,35 +437,94 @@ void __clk_unprepare(struct clk *clk)
> */
> void clk_unprepare(struct clk *clk)
> {
> + struct clk *top = ERR_PTR(-EBUSY);
> + int tries = 0;
> +
> + /*
> + * walk the list of parents checking clk->state along the way. If all
> + * clk->state is UNLOCKED then continue. If a clk->state is LOCKED then
> + * bail out with -EBUSY.
> + */
> + while (IS_ERR(top)) {
> + mutex_lock(&prepare_lock);
> + top = __clk_unprepare_lock(clk);
> + mutex_unlock(&prepare_lock);
> +
> + if (IS_ERR(top)) {
> + pr_debug("%s: %s failed with %ld on attempt %d\n",
> + __func__, clk->name, PTR_ERR(top),
> + tries);
> + wait_for_completion(&clk_completion);
> + if (WAIT_TRIES == ++tries)
> + break;
> + } else
> + break;

Braces around else part please.

> + }
> +
> + if (WAIT_TRIES == tries) {
> + pr_warning("%s: failed to lock clocks; cyclical dependency?\n",
> + __func__);
> + return;
> + }
> +
> + /* unprepare the list of clocks from clk to top */
> + __clk_unprepare(clk, top);
> +

> + /* unprepare the list of clocks from clk to top */
> + __clk_prepare(clk, top);

You mean prepare right ? :)



Regards,
Pankaj Kumar


\
 
 \ /
  Last update: 2012-08-27 20:21    [W:0.075 / U:0.676 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site