[PATCH V4 3/3] clk: Fixup locking issues for clk_set_parent
Ulf Hansson
ulf.hansson at linaro.org
Thu Apr 4 15:28:06 EDT 2013
On 4 April 2013 12:35, Rajagopal Venkat <rajagopal.venkat at linaro.org> wrote:
> On 3 April 2013 02:39, Ulf Hansson <ulf.hansson at stericsson.com> wrote:
>> From: Ulf Hansson <ulf.hansson at linaro.org>
>>
>> Updating the clock tree topology must be protected with the spinlock
>> when doing clk_set_parent, otherwise we can not handle the migration
>> of the enable_count in a safe manner.
>
> Why is not safe to update tree topology with no spinlock?
clk_enable|disable propagates upwards in the tree. While in the middle
of changing parents, you could end up in operating on more than one
parent. This must be prevented and is done by holding the enable lock.
>
>>
>> While issuing the .set_parent callback to make the clk-hw perform the
>> switch to the new parent, we can not hold the spinlock since it is must
>> be allowed to be slow path. This complicates error handling, but is still
>> possible to achieve.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson at linaro.org>
>> Cc: Rajagopal Venkat <rajagopal.venkat at linaro.org>
>> ---
>> drivers/clk/clk.c | 67 +++++++++++++++++++++++++++++++++++++++++------------
>> 1 file changed, 52 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index c83e8e5..de6b459 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1363,31 +1363,71 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent, u8 p_index)
>> unsigned long flags;
>> int ret = 0;
>> struct clk *old_parent = clk->parent;
>> + bool migrated_enable = false;
>>
>> - /* migrate prepare and enable */
>> + /* migrate prepare */
>> if (clk->prepare_count)
>> __clk_prepare(parent);
>>
>> - /* FIXME replace with clk_is_enabled(clk) someday */
>> flags = clk_enable_lock();
>> - if (clk->enable_count)
>> +
>> + /* migrate enable */
>> + if (clk->enable_count) {
>> __clk_enable(parent);
>> + migrated_enable = true;
>> + }
>> +
>> + /* update the clk tree topology */
>> + clk_reparent(clk, parent);
>> +
>> clk_enable_unlock(flags);
>>
>> /* change clock input source */
>> if (parent && clk->ops->set_parent)
>> ret = clk->ops->set_parent(clk->hw, p_index);
>>
>> - /* clean up old prepare and enable */
>> - flags = clk_enable_lock();
>> - if (clk->enable_count)
>> + if (ret) {
>> + /*
>> + * The error handling is tricky due to that we need to release
>> + * the spinlock while issuing the .set_parent callback. This
>> + * means the new parent might have been enabled/disabled in
>
> But then new parent is reference counted with __clk_enable(parent)
> isn't it? Even if other children are disabling newparent, it is still
> alive. Am I missing anything here?
Since we have released and later fetched the enable_lock, the clk
might very well have been enabled|disabled in between. Thus we can not
solely trust the enable count.
If we have migrated the enable, we need to keep track of this so we
can roll back that change.
>
>> + * between, which must be considered when doing rollback.
>> + */
>> + flags = clk_enable_lock();
>> +
>> + clk_reparent(clk, old_parent);
>> +
>> + if (migrated_enable && clk->enable_count) {
>> + __clk_disable(parent);
>> + } else if (migrated_enable && (clk->enable_count == 0)) {
>
> Will this condition happen at all? the reference counting should
> prevent this AFAICT. Can this error path be simplified?
I am trying to handle the scenarios described below. If you think we
can simplify error handling please advise me, I know the code looks a
bit tricky.
Scenario 1 (migration done):
1. Fetch enable_lock.
2. clk->enable_count is > 0, so we decide to migrate it for the new parent.
3. Update clock tree topology.
4. Release enable_lock.
5 a. A client does clk_disable(clk) and clk->enable_count reaches 0.
Then a reference to the new parent will also be decreased.
5 b. A client does clk_enable(clk) so clk->enable_count is still > 0.
This means no reference change is propagated to the new parent.
6. clk->ops->set_parent fails, we need error handling.
- If 5a, we need to decrease a reference for the old parent to reflect
that clk->enable_count has reached 0.
- If 5b, we need to revert the increased reference count for the new parent.
Scenario 2 (no migration):
1. Fetch enable_lock.
2. clk->enable_count is 0, so no migration done.
3. Update clock tree topology.
4. Release enable_lock.
5. A client does clk_enable(clk) so clk->enable_count is > 0. Then a
reference to the new parent will also be increased.
6. clk->ops->set_parent fails, we need error handling.
- We need to revert the reference change on the new parent and instead
transfer it to the old parent.
>
>> + __clk_disable(old_parent);
>> + } else if (!migrated_enable && clk->enable_count) {
>> + __clk_disable(parent);
>> + __clk_enable(old_parent);
>> + }
>> +
>> + clk_enable_unlock(flags);
>> +
>> + if (clk->prepare_count)
>> + __clk_unprepare(parent);
>> +
>> + return ret;
>> + }
>> +
>> + /* clean up enable for old parent if migration was done */
>> + if (migrated_enable) {
>
> Reaching here, the old_parent should be disabled in any case. Is this
> 'if (migrated_enable)' check required?
Why is it disabled?
Where did we decrease a reference to it?
>
>> + flags = clk_enable_lock();
>> __clk_disable(old_parent);
>> - clk_enable_unlock(flags);
>> + clk_enable_unlock(flags);
>> + }
>>
>> + /* clean up prepare for old parent if migration was done */
>> if (clk->prepare_count)
>> __clk_unprepare(old_parent);
>>
>> - return ret;
>> + /* update debugfs with new clk tree topology */
>> + clk_debug_reparent(clk, parent);
>> + return 0;
>> }
>>
>> /**
>> @@ -1450,14 +1490,11 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>> /* do the re-parent */
>> ret = __clk_set_parent(clk, parent, p_index);
>>
>> - /* propagate ABORT_RATE_CHANGE if .set_parent failed */
>> - if (ret) {
>> + /* propagate rate recalculation accordingly */
>> + if (ret)
>> __clk_recalc_rates(clk, ABORT_RATE_CHANGE);
>> - goto out;
>> - }
>> -
>> - /* propagate rate recalculation downstream */
>> - __clk_reparent(clk, parent);
>> + else
>> + __clk_recalc_rates(clk, POST_RATE_CHANGE);
>>
>> out:
>> clk_prepare_unlock();
>> --
>> 1.7.10
>>
>
>
>
> --
> Regards,
> Rajagopal
Kind regards
Ulf Hansson
More information about the linux-arm-kernel
mailing list