Re: [PATCH] clk: Fix race conditions between clk_set_parent() and clk_enable()

[Turquette, Mike]

On Tue, May 15, 2012 at 1:09 PM, Saravana Kannan <skannan@xxxxxxxxxxxxxx> wrote:
> On 05/15/2012 01:00 PM, Sascha Hauer wrote:
>>
>> On Tue, May 15, 2012 at 12:51:06PM -0700, Saravana Kannan wrote:
>>>>>
>>>>>        ret = clk->ops->set_parent(clk->hw, i);
>>>>
>>>>
>>>> You call ->set_parent while holding a spinlock. This won't work with i2c
>>>> clocks.
>>>
>>>
>>> I did account for that. I explained it in the commit text. Please
>>> let me know if any part of that is not clear or is not correct.
>>>
>>
>> I missed this part in the commit log. I have no idea whether we can live
>> with this limitation though.
>>
>> Sascha
>>
>
> It's not really an artificial limitation of the patch. This has to be
> enforced if the clock is to be managed correctly while allowing .set_parent
> to NOT be atomic.
>
> There is no way to guarantee that the enable/disable is properly propagated
> to the parent clock if we can't guarantee mutual exclusion between changing
> parents and calling enable/disable.
>

We know for sure that __clk_enable was propagated since it won't
return until it is done.  The bigger issue here is the inherent
prepare_lock/enable_lock raciness, which this patch does not solve.
Your approach above is like putting a band-aid gunshot wound :-)

This change essentially forces clocks with sleeping .set_parent
callbacks to be gated during clk_set_parent or else cause a big WARN
and dump_stack.  What is really needed is a way to synchronize all of
the clock operations and drop these race conditions.  Until that
problem is solved OR until it is proven impossible to fix with the
API's given to us I won't be taking this patch.  It is invalid for the
set of clocks that don't set the CLK_SET_PARENT_GATE flag AND have
.set_parent callbacks which might sleep.

> Since we can't do mutual exclusion be using spinlock (since .set_parent is
> NOT atomic for these clocks), then only other way of ensuring mutual
> exclusion is to force an unprepare and then mutually exclude a prepare while
> changing the parent. This by association (can't enable unprepared clock)
> mutually excludes the changing of parent and calling enable/disable.

Right, but this is a workaround to a larger endemic problem.  I
completely get what you're trying to do but this fix isn't enough.

Thanks,
Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/