[PATCH v4] clk: allow reentrant calls into the clk framework

[Mike Turquette]

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 at linaro.org>
> > Cc: Rajagopal Venkat <rajagopal.venkat at linaro.org>
> > Cc: David Brown <davidb at codeaurora.org>
> > Cc: Ulf Hansson <ulf.hansson at linaro.org>
> > Cc: Laurent Pinchart <laurent.pinchart at 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