[PATCH V4 0/3] clk: Fixup issues for clk_set_parent

[PATCH V4 0/3] clk: Fixup issues for clk_set_parent

[Ulf Hansson]

From: Ulf Hansson <ulf.hansson@linaro.org>

These issues exist in the clk_set_parent API:

* Race issue for spinlocks when updating the clock tree toplogy.
* The feature of reparent to the orphan list is broken.

This patchset fixes both problems. Patch 1 prepares for patch 2 and
patch 3, which are where the real issues are resolved.

Changes in v4:
	- Rebased on top Mike's clk reentrancy patches.

Changes in v3:
	- Fix review comments from Mike.
	- Include fixup for allow reparent to the orphan list.

Changes in v2:
	- Do not remove the existing __clk_reparent API.
	- Rebase patches.

Ulf Hansson (3):
  clk: Restructure code for __clk_reparent
  clk: Fixup errorhandling for clk_set_parent
  clk: Fixup locking issues for clk_set_parent

 drivers/clk/clk.c |  193 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 130 insertions(+), 63 deletions(-)

-- 
1.7.10

[PATCH V4 1/3] clk: Restructure code for __clk_reparent

[Ulf Hansson]

From: Ulf Hansson <ulf.hansson@linaro.org>

Split __clk_reparent into three pieces, one for doing the actual
reparent for updating the clock tree topology, one for the
COMMON_CLK_DEBUG code and one for doing the rate recalculation.

This patch also makes it possible to hold the spinlock over the
update of the clock tree topology, which could not be done before
when both debugfs updates and clock rate updates was done within
the same function.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Rajagopal Venkat <rajagopal.venkat@linaro.org>
---
 drivers/clk/clk.c |   70 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 28 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0230c9d..013a3c7 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -343,6 +343,39 @@ out:
 }
 
 /**
+ * clk_debug_reparent - reparent clk node in the debugfs clk tree
+ * @clk: the clk being reparented
+ * @new_parent: the new clk parent, may be NULL
+ *
+ * Rename clk entry in the debugfs clk tree if debugfs has been
+ * initialized.  Otherwise it bails out early since the debugfs clk tree
+ * will be created lazily by clk_debug_init as part of a late_initcall.
+ *
+ * Caller must hold prepare_lock.
+ */
+static void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
+{
+	struct dentry *d;
+	struct dentry *new_parent_d;
+
+	if (!inited)
+		return;
+
+	if (new_parent)
+		new_parent_d = new_parent->dentry;
+	else
+		new_parent_d = orphandir;
+
+	d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
+			new_parent_d, clk->name);
+	if (d)
+		clk->dentry = d;
+	else
+		pr_debug("%s: failed to rename debugfs entry for %s\n",
+				__func__, clk->name);
+}
+
+/**
  * clk_debug_init - lazily create the debugfs clk tree visualization
  *
  * clks are often initialized very early during boot before memory can
@@ -396,6 +429,9 @@ static int __init clk_debug_init(void)
 late_initcall(clk_debug_init);
 #else
 static inline int clk_debug_register(struct clk *clk) { return 0; }
+static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
+{
+}
 #endif
 
 /* caller must hold prepare_lock */
@@ -1277,16 +1313,8 @@ out:
 	return ret;
 }
 
-void __clk_reparent(struct clk *clk, struct clk *new_parent)
+static void clk_reparent(struct clk *clk, struct clk *new_parent)
 {
-#ifdef CONFIG_COMMON_CLK_DEBUG
-	struct dentry *d;
-	struct dentry *new_parent_d;
-#endif
-
-	if (!clk || !new_parent)
-		return;
-
 	hlist_del(&clk->child_node);
 
 	if (new_parent)
@@ -1294,27 +1322,13 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
 	else
 		hlist_add_head(&clk->child_node, &clk_orphan_list);
 
-#ifdef CONFIG_COMMON_CLK_DEBUG
-	if (!inited)
-		goto out;
-
-	if (new_parent)
-		new_parent_d = new_parent->dentry;
-	else
-		new_parent_d = orphandir;
-
-	d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
-			new_parent_d, clk->name);
-	if (d)
-		clk->dentry = d;
-	else
-		pr_debug("%s: failed to rename debugfs entry for %s\n",
-				__func__, clk->name);
-out:
-#endif
-
 	clk->parent = new_parent;
+}
 
+void __clk_reparent(struct clk *clk, struct clk *new_parent)
+{
+	clk_reparent(clk, new_parent);
+	clk_debug_reparent(clk, new_parent);
 	__clk_recalc_rates(clk, POST_RATE_CHANGE);
 }
 
-- 
1.7.10

[PATCH V4 2/3] clk: Fixup errorhandling for clk_set_parent

[Ulf Hansson]

From: Ulf Hansson <ulf.hansson@linaro.org>

Fixup the broken feature of allowing reparent of a clk to the
orhpan list and vice verse. When operating on a single-parent
clk, the .set_parent callback for the clk hw is optional to
implement, but for a multi-parent clk it is mandatory.

Moreover improve the errorhandling by verifying the prerequisites
before triggering clk notifiers. This will prevent unnecessary
rollback with ABORT_RATE_CHANGE.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Rajagopal Venkat <rajagopal.venkat@linaro.org>
---
 drivers/clk/clk.c |   56 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 013a3c7..c83e8e5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1332,15 +1332,10 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
 	__clk_recalc_rates(clk, POST_RATE_CHANGE);
 }
 
-static int __clk_set_parent(struct clk *clk, struct clk *parent)
+static u8 clk_fetch_parent_index(struct clk *clk, struct clk *parent)
 {
-	struct clk *old_parent;
-	unsigned long flags;
-	int ret = -EINVAL;
 	u8 i;
 
-	old_parent = clk->parent;
-
 	if (!clk->parents)
 		clk->parents = kzalloc((sizeof(struct clk*) * clk->num_parents),
 								GFP_KERNEL);
@@ -1360,11 +1355,14 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
 		}
 	}
 
-	if (i == clk->num_parents) {
-		pr_debug("%s: clock %s is not a possible parent of clock %s\n",
-				__func__, parent->name, clk->name);
-		goto out;
-	}
+	return i;
+}
+
+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;
 
 	/* migrate prepare and enable */
 	if (clk->prepare_count)
@@ -1377,7 +1375,8 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
 	clk_enable_unlock(flags);
 
 	/* change clock input source */
-	ret = clk->ops->set_parent(clk->hw, i);
+	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();
@@ -1388,7 +1387,6 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
 	if (clk->prepare_count)
 		__clk_unprepare(old_parent);
 
-out:
 	return ret;
 }
 
@@ -1407,11 +1405,14 @@ out:
 int clk_set_parent(struct clk *clk, struct clk *parent)
 {
 	int ret = 0;
+	u8 p_index = 0;
+	unsigned long p_rate = 0;
 
 	if (!clk || !clk->ops)
 		return -EINVAL;
 
-	if (!clk->ops->set_parent)
+	/* verify ops for for multi-parent clks */
+	if ((clk->num_parents > 1) && (!clk->ops->set_parent))
 		return -ENOSYS;
 
 	/* prevent racing with updates to the clock topology */
@@ -1420,19 +1421,34 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	if (clk->parent == parent)
 		goto out;
 
+	/* check that we are allowed to re-parent if the clock is in use */
+	if ((clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	/* try finding the new parent index */
+	if (parent) {
+		p_index = clk_fetch_parent_index(clk, parent);
+		p_rate = parent->rate;
+		if (p_index == clk->num_parents) {
+			pr_debug("%s: clk %s can not be parent of clk %s\n",
+					__func__, parent->name, clk->name);
+			ret = -EINVAL;
+			goto out;
+		}
+	}
+
 	/* propagate PRE_RATE_CHANGE notifications */
 	if (clk->notifier_count)
-		ret = __clk_speculate_rates(clk, parent->rate);
+		ret = __clk_speculate_rates(clk, p_rate);
 
 	/* abort if a driver objects */
 	if (ret == NOTIFY_STOP)
 		goto out;
 
-	/* only re-parent if the clock is not in use */
-	if ((clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count)
-		ret = -EBUSY;
-	else
-		ret = __clk_set_parent(clk, parent);
+	/* do the re-parent */
+	ret = __clk_set_parent(clk, parent, p_index);
 
 	/* propagate ABORT_RATE_CHANGE if .set_parent failed */
 	if (ret) {
-- 
1.7.10

[PATCH V4 3/3] clk: Fixup locking issues for clk_set_parent

[Ulf Hansson]

From: Ulf Hansson <ulf.hansson@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.

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@linaro.org>
Cc: Rajagopal Venkat <rajagopal.venkat@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
+		 * 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)) {
+			__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) {
+		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

[PATCH V4 3/3] clk: Fixup locking issues for clk_set_parent

[Rajagopal Venkat]

On 3 April 2013 02:39, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
> From: Ulf Hansson <ulf.hansson@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?

>
> 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@linaro.org>
> Cc: Rajagopal Venkat <rajagopal.venkat@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?

> +                * 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?

> +                       __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?

> +               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

[PATCH V4 3/3] clk: Fixup locking issues for clk_set_parent

[Ulf Hansson]

On 4 April 2013 12:35, Rajagopal Venkat <rajagopal.venkat@linaro.org> wrote:
> On 3 April 2013 02:39, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
>> From: Ulf Hansson <ulf.hansson@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@linaro.org>
>> Cc: Rajagopal Venkat <rajagopal.venkat@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

[PATCH V4 3/3] clk: Fixup locking issues for clk_set_parent

[Rajagopal Venkat]

On 5 April 2013 00:58, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 4 April 2013 12:35, Rajagopal Venkat <rajagopal.venkat@linaro.org> wrote:
>> On 3 April 2013 02:39, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
>>> From: Ulf Hansson <ulf.hansson@linaro.org>

Sorry for the delayed response.

>>>
>>> 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.

I see.

>
>>
>>>
>>> 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@linaro.org>
>>> Cc: Rajagopal Venkat <rajagopal.venkat@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.

I am confused here. The code comments talks about new parent, "This
means the new parent might have been enabled/disabled in between". But
your review comments talks about clk in consideration which I agree.
Please update the comments.

>
> 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.

How about detaching clk subtree from old parent and attaching to
orphan list while in transition. Depending on clk->ops->set_parent()
outcome, attach the clk subtree to either new/old parent. I haven't
thought much about it, check if this helps.

> 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



-- 
Regards,
Rajagopal

[PATCH V4 0/3] clk: Fixup issues for clk_set_parent

[Mike Turquette]

Quoting Ulf Hansson (2013-04-02 14:09:36)
> From: Ulf Hansson <ulf.hansson@linaro.org>
> 
> These issues exist in the clk_set_parent API:
> 
> * Race issue for spinlocks when updating the clock tree toplogy.
> * The feature of reparent to the orphan list is broken.
> 

Taken into clk-next.

Thanks,
Mike

> This patchset fixes both problems. Patch 1 prepares for patch 2 and
> patch 3, which are where the real issues are resolved.
> 
> Changes in v4:
>         - Rebased on top Mike's clk reentrancy patches.
> 
> Changes in v3:
>         - Fix review comments from Mike.
>         - Include fixup for allow reparent to the orphan list.
> 
> Changes in v2:
>         - Do not remove the existing __clk_reparent API.
>         - Rebase patches.
> 
> Ulf Hansson (3):
>   clk: Restructure code for __clk_reparent
>   clk: Fixup errorhandling for clk_set_parent
>   clk: Fixup locking issues for clk_set_parent
> 
>  drivers/clk/clk.c |  193 ++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 130 insertions(+), 63 deletions(-)
> 
> -- 
> 1.7.10