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

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

[Ulf Hansson]

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

Issues with racing for fetching spinlocks is present in the
clk_set_parent API. This patchset is trying to fixup these issues.

In short, while updating the clock tree toplogy the spinlock must be held
to prevent enable_count from being messed up.

Patch 1 and 2 prepares for patch 3, which is where the real issue are
resolved.

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

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

 drivers/clk/clk.c |  184 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 122 insertions(+), 62 deletions(-)

-- 
1.7.10

[PATCH V2 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>
---
 drivers/clk/clk.c |   74 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 29 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 253792a..d73fb0f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -280,6 +280,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
@@ -333,6 +366,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 */
@@ -1214,16 +1250,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)
@@ -1231,27 +1259,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);
 }
 
@@ -1364,7 +1378,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	}
 
 	/* propagate rate recalculation downstream */
-	__clk_reparent(clk, parent);
+	clk_reparent(clk, parent);
+	clk_debug_reparent(clk, parent);
+	__clk_recalc_rates(clk, POST_RATE_CHANGE);
 
 out:
 	mutex_unlock(&prepare_lock);
-- 
1.7.10

[PATCH V2 2/3] clk: Improve errorhandling for clk_set_parent

[Ulf Hansson]

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

By verifying all the needed static information before doing the clk
notifications, we minimize number of unwanted rollback notifications
with ABORT_RATE_CHANGE message to occur.

Additionally make sure the parent are valid pointer before using it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/clk/clk.c |   48 +++++++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d73fb0f..c7eb0d7 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1269,15 +1269,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);
@@ -1297,11 +1292,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;
+	struct clk *old_parent = clk->parent;
 
 	/* migrate prepare and enable */
 	if (clk->prepare_count)
@@ -1314,7 +1312,7 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
 	spin_unlock_irqrestore(&enable_lock, flags);
 
 	/* change clock input source */
-	ret = clk->ops->set_parent(clk->hw, i);
+	ret = clk->ops->set_parent(clk->hw, p_index);
 
 	/* clean up old prepare and enable */
 	spin_lock_irqsave(&enable_lock, flags);
@@ -1325,7 +1323,6 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
 	if (clk->prepare_count)
 		__clk_unprepare(old_parent);
 
-out:
 	return ret;
 }
 
@@ -1344,8 +1341,9 @@ out:
 int clk_set_parent(struct clk *clk, struct clk *parent)
 {
 	int ret = 0;
+	u8 p_index;
 
-	if (!clk || !clk->ops)
+	if (!clk || !clk->ops || !parent)
 		return -EINVAL;
 
 	if (!clk->ops->set_parent)
@@ -1357,6 +1355,21 @@ 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 */
+	p_index = clk_fetch_parent_index(clk, parent);
+	if (p_index == clk->num_parents) {
+		pr_debug("%s: clock %s is not a possible parent of clock %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);
@@ -1365,11 +1378,8 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	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 V2 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>
---
 drivers/clk/clk.c |   68 +++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c7eb0d7..fee7628 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1300,30 +1300,69 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent, u8 p_index)
 	unsigned long flags;
 	int ret;
 	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 */
 	spin_lock_irqsave(&enable_lock, flags);
-	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);
+
 	spin_unlock_irqrestore(&enable_lock, flags);
 
 	/* change clock input source */
 	ret = clk->ops->set_parent(clk->hw, p_index);
+	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.
+		 */
+		spin_lock_irqsave(&enable_lock, flags);
 
-	/* clean up old prepare and enable */
-	spin_lock_irqsave(&enable_lock, flags);
-	if (clk->enable_count)
+		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);
+		}
+
+		spin_unlock_irqrestore(&enable_lock, flags);
+
+		if (clk->prepare_count)
+			__clk_unprepare(parent);
+
+		return ret;
+	}
+
+	/* clean up enable for old parent if migration was done */
+	if (migrated_enable) {
+		spin_lock_irqsave(&enable_lock, flags);
 		__clk_disable(old_parent);
-	spin_unlock_irqrestore(&enable_lock, flags);
+		spin_unlock_irqrestore(&enable_lock, 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;
 }
 
 /**
@@ -1381,16 +1420,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);
-	clk_debug_reparent(clk, parent);
-	__clk_recalc_rates(clk, POST_RATE_CHANGE);
+	else
+		__clk_recalc_rates(clk, POST_RATE_CHANGE);
 
 out:
 	mutex_unlock(&prepare_lock);
-- 
1.7.10

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

[Mike Turquette]

Quoting Ulf Hansson (2013-03-21 06:48:11)
> +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);
>  }
>  
> @@ -1364,7 +1378,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>         }
>  
>         /* propagate rate recalculation downstream */
> -       __clk_reparent(clk, parent);
> +       clk_reparent(clk, parent);
> +       clk_debug_reparent(clk, parent);
> +       __clk_recalc_rates(clk, POST_RATE_CHANGE);
>  

This is an interesting change.  Why not call __clk_reparent here instead
of open coding an identical sequence?

Regards,
Mike

[PATCH V2 2/3] clk: Improve errorhandling for clk_set_parent

[Mike Turquette]

Quoting Ulf Hansson (2013-03-21 06:48:12)
> @@ -1344,8 +1341,9 @@ out:
>  int clk_set_parent(struct clk *clk, struct clk *parent)
>  {
>         int ret = 0;
> +       u8 p_index;
>  
> -       if (!clk || !clk->ops)
> +       if (!clk || !clk->ops || !parent)
>                 return -EINVAL;
>  

A NULL clock is valid according to the clk.h api.  I would like to allow
parent to be NULL, resulting in a migration from the real clock tree to
the orphans list.

This feature was apparently buggy for some time and Rajagopal sent me a
patch off-list to fix it.  Rajagopal, can you post your patch publicly?

Thanks,
Mike

[PATCH V2 2/3] clk: Improve errorhandling for clk_set_parent

[Rajagopal Venkat]

On 22 March 2013 02:30, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Ulf Hansson (2013-03-21 06:48:12)
>> @@ -1344,8 +1341,9 @@ out:
>>  int clk_set_parent(struct clk *clk, struct clk *parent)
>>  {
>>         int ret = 0;
>> +       u8 p_index;
>>
>> -       if (!clk || !clk->ops)
>> +       if (!clk || !clk->ops || !parent)
>>                 return -EINVAL;
>>
>
> A NULL clock is valid according to the clk.h api.  I would like to allow
> parent to be NULL, resulting in a migration from the real clock tree to
> the orphans list.
>
> This feature was apparently buggy for some time and Rajagopal sent me a
> patch off-list to fix it.  Rajagopal, can you post your patch publicly?
>

Patch is available at https://patchwork.kernel.org/patch/2012221/
Do you want me to rebase it on top of this patchset?

> Thanks,
> Mike



-- 
Regards,
Rajagopal

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

[Ulf Hansson]

On 21 March 2013 21:53, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Ulf Hansson (2013-03-21 06:48:11)
>> +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);
>>  }
>>
>> @@ -1364,7 +1378,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>>         }
>>
>>         /* propagate rate recalculation downstream */
>> -       __clk_reparent(clk, parent);
>> +       clk_reparent(clk, parent);
>> +       clk_debug_reparent(clk, parent);
>> +       __clk_recalc_rates(clk, POST_RATE_CHANGE);
>>
>
> This is an interesting change.  Why not call __clk_reparent here instead
> of open coding an identical sequence?

By lazyness when rebasing patches I decided to keep it. :-) Well, my
idea here was also to make it visible how these three calls will be
split out to be called from three different places.

Do you want me to fixup?

Br
Uffe

>
> Regards,
> Mike

[PATCH V2 2/3] clk: Improve errorhandling for clk_set_parent

[Ulf Hansson]

On 21 March 2013 22:00, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Ulf Hansson (2013-03-21 06:48:12)
>> @@ -1344,8 +1341,9 @@ out:
>>  int clk_set_parent(struct clk *clk, struct clk *parent)
>>  {
>>         int ret = 0;
>> +       u8 p_index;
>>
>> -       if (!clk || !clk->ops)
>> +       if (!clk || !clk->ops || !parent)
>>                 return -EINVAL;
>>
>
> A NULL clock is valid according to the clk.h api.  I would like to allow
> parent to be NULL, resulting in a migration from the real clock tree to
> the orphans list.

Sorry, did not think of this. Will correct.

>
> This feature was apparently buggy for some time and Rajagopal sent me a
> patch off-list to fix it.  Rajagopal, can you post your patch publicly?

So, that is also why I removed this. Anyway, I guess it is somewhat
easier for me to make a fixup so we can accept NULL as parents. I give
it a try.

>
> Thanks,
> Mike

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

[Mike Turquette]

Quoting Ulf Hansson (2013-03-22 04:08:09)
> On 21 March 2013 21:53, Mike Turquette <mturquette@linaro.org> wrote:
> > Quoting Ulf Hansson (2013-03-21 06:48:11)
> >> +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);
> >>  }
> >>
> >> @@ -1364,7 +1378,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
> >>         }
> >>
> >>         /* propagate rate recalculation downstream */
> >> -       __clk_reparent(clk, parent);
> >> +       clk_reparent(clk, parent);
> >> +       clk_debug_reparent(clk, parent);
> >> +       __clk_recalc_rates(clk, POST_RATE_CHANGE);
> >>
> >
> > This is an interesting change.  Why not call __clk_reparent here instead
> > of open coding an identical sequence?
> 
> By lazyness when rebasing patches I decided to keep it. :-) Well, my
> idea here was also to make it visible how these three calls will be
> split out to be called from three different places.
> 
> Do you want me to fixup?
> 

Please do.  It seems you'll be sending another version anyways so just
roll it in :)

Thanks much,
Mike

> Br
> Uffe
> 
> >
> > Regards,
> > Mike

[PATCH V2 2/3] clk: Improve errorhandling for clk_set_parent

[Mike Turquette]

Quoting Rajagopal Venkat (2013-03-22 02:59:17)
> On 22 March 2013 02:30, Mike Turquette <mturquette@linaro.org> wrote:
> > Quoting Ulf Hansson (2013-03-21 06:48:12)
> >> @@ -1344,8 +1341,9 @@ out:
> >>  int clk_set_parent(struct clk *clk, struct clk *parent)
> >>  {
> >>         int ret = 0;
> >> +       u8 p_index;
> >>
> >> -       if (!clk || !clk->ops)
> >> +       if (!clk || !clk->ops || !parent)
> >>                 return -EINVAL;
> >>
> >
> > A NULL clock is valid according to the clk.h api.  I would like to allow
> > parent to be NULL, resulting in a migration from the real clock tree to
> > the orphans list.
> >
> > This feature was apparently buggy for some time and Rajagopal sent me a
> > patch off-list to fix it.  Rajagopal, can you post your patch publicly?
> >
> 
> Patch is available at https://patchwork.kernel.org/patch/2012221/
> Do you want me to rebase it on top of this patchset?
> 

Maybe.  Let's see what Ulf's next version of the clk_set_parent locking
patch looks like.

Thanks,
Mike

> > Thanks,
> > Mike
> 
> 
> 
> -- 
> Regards,
> Rajagopal