Quoting Ulf Hansson (2013-02-28 01:54:34)
> On 28 February 2013 05:49, Mike Turquette <mturque...@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
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
