[PATCH 1/5] clk: allow reentrant calls into the clk framework

[mturquette@linaro.org (Mike Turquette)]

Quoting Ulf Hansson (2013-02-28 01:54:34)
> On 28 February 2013 05:49, Mike Turquette <mturquette@linaro.org> wrote:
> > @@ -703,10 +744,29 @@ int clk_enable(struct clk *clk)
> >         unsigned long flags;
> >         int ret;
> >
> > +       /* this call re-enters if it is from the same context */
> > +       if (spin_is_locked(&enable_lock) || mutex_is_locked(&prepare_lock)) {
> > +               if ((void *) atomic_read(&context) == get_current()) {
> > +                       ret = __clk_enable(clk);
> > +                       goto out;
> > +               }
> > +       }
> 
> I beleive the clk_enable|disable code will be racy. What do you think
> about this scenario:
> 
> 1. Thread 1, calls clk_prepare -> clk is not reentrant -> mutex_lock
> -> set_context to thread1.
> 2. Thread 2, calls clk_enable -> above "if" will mean that get_current
> returns thread 1 context and then clk_enable continues ->
> spin_lock_irqsave -> set_context to thread 2.
> 3. Thread 1 continues and triggers a reentancy for clk_prepare -> clk
> is not reentrant (since thread 2 has set a new context) -> mutex_lock
> and we will hang forever.
> 
> Do you think above scenario could happen?
> 
> I think the solution would be to invent another "static atomic_t
> context;" which is used only for fast path functions
> (clk_enable|disable). That should do the trick I think.

Ulf,

You are correct.  In fact I have a branch that has two separate context
pointers, one for mutex-protected functions and one for
spinlock-protected functions.  Somehow I managed to discard that change
before settling on the final version that was published.

I'll add the change back in.

Thanks for the review,
Mike