[RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements

[RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements

[Masahiro Yamada]

Many refactoring, with detection of circular parent looping.


Changes in v2:
  - Fix a bug.  Return -EINVAL when parent _is_ NULL.

Masahiro Yamada (16):
  clk: remove unused first argument of __clk_init()
  clk: change the argument of __clk_init() into pointer to clk_core
  clk: rename __clk_init() into __clk_core_init()
  clk: remove unnecessary !core->parents conditional
  clk: change sizeof(struct clk *) to sizeof(*core->parents)
  clk: move core->parents allocation to clk_register()
  clk: simplify clk_core_get_parent_by_index()
  clk: drop the initial core->parents look-ups from __clk_core_init()
  clk: replace pr_warn() with pr_err() for fatal cases
  clk: move checking .git_parent to __clk_core_init()
  clk: simplify __clk_init_parent()
  clk: avoid circular clock topology
  clk: walk the orphan clock list more simply
  clk: make sure parent is not NULL in clk_fetch_parent_index()
  clk: simplify clk_fetch_parent_index() function
  clk: slightly optimize clk_core_set_parent()

 drivers/clk/clk.c | 214 ++++++++++++++++++++++--------------------------------
 1 file changed, 85 insertions(+), 129 deletions(-)

-- 
1.9.1

[RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init()

[Masahiro Yamada]

The "struct device *dev" is not used at all in this function.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 3fba5ac..897e5ae 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2286,13 +2286,12 @@ static inline void clk_debug_unregister(struct clk_core *core)
 
 /**
  * __clk_init - initialize the data structures in a struct clk
- * @dev:	device initializing this clk, placeholder for now
  * @clk:	clk being initialized
  *
  * Initializes the lists in struct clk_core, queries the hardware for the
  * parent and rate and sets them both.
  */
-static int __clk_init(struct device *dev, struct clk *clk_user)
+static int __clk_init(struct clk *clk_user)
 {
 	int i, ret = 0;
 	struct clk_core *orphan;
@@ -2575,7 +2574,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 		goto fail_parent_names_copy;
 	}
 
-	ret = __clk_init(dev, hw->clk);
+	ret = __clk_init(hw->clk);
 	if (!ret)
 		return hw->clk;
 
-- 
1.9.1

Re: [RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init()

[Vladimir Zapolskiy]

Hi,

the whole series looks very good, also I haven't found any regressions
(tested on imx6q and lpc32xx).

Please feel free to add

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

On 28.12.2015 12:22, Masahiro Yamada wrote:
> The "struct device *dev" is not used at all in this function.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v2: None
> 
>  drivers/clk/clk.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 3fba5ac..897e5ae 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2286,13 +2286,12 @@ static inline void clk_debug_unregister(struct clk_core *core)
>  
>  /**
>   * __clk_init - initialize the data structures in a struct clk
> - * @dev:	device initializing this clk, placeholder for now
>   * @clk:	clk being initialized
>   *
>   * Initializes the lists in struct clk_core, queries the hardware for the
>   * parent and rate and sets them both.
>   */
> -static int __clk_init(struct device *dev, struct clk *clk_user)
> +static int __clk_init(struct clk *clk_user)
>  {
>  	int i, ret = 0;
>  	struct clk_core *orphan;
> @@ -2575,7 +2574,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>  		goto fail_parent_names_copy;
>  	}
>  
> -	ret = __clk_init(dev, hw->clk);
> +	ret = __clk_init(hw->clk);
>  	if (!ret)
>  		return hw->clk;
>  
> 

--
With best wishes,
Vladimir

Re: [RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init()

[Masahiro Yamada]

Hi Vladimir,

2016-01-08 9:44 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
> Hi,
>
> the whole series looks very good, also I haven't found any regressions
> (tested on imx6q and lpc32xx).
>
> Please feel free to add
>
> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>


It is very good to hear.

Thanks for your review and test!



-- 
Best Regards
Masahiro Yamada

Re: [RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init()

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> The "struct device *dev" is not used at all in this function.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 02/16] clk: change the argument of __clk_init() into pointer to clk_core

[Masahiro Yamada]

The argument clk_user is used only for the clk_user->core.  The rest
of this function only takes care of clk_core.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 897e5ae..623838f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2285,25 +2285,22 @@ static inline void clk_debug_unregister(struct clk_core *core)
 #endif
 
 /**
- * __clk_init - initialize the data structures in a struct clk
- * @clk:	clk being initialized
+ * __clk_init - initialize the data structures in a struct clk_core
+ * @core:	clk_core being initialized
  *
  * Initializes the lists in struct clk_core, queries the hardware for the
  * parent and rate and sets them both.
  */
-static int __clk_init(struct clk *clk_user)
+static int __clk_init(struct clk_core *core)
 {
 	int i, ret = 0;
 	struct clk_core *orphan;
 	struct hlist_node *tmp2;
-	struct clk_core *core;
 	unsigned long rate;
 
-	if (!clk_user)
+	if (!core)
 		return -EINVAL;
 
-	core = clk_user->core;
-
 	clk_prepare_lock();
 
 	/* check to see if a clock with this name is already registered */
@@ -2574,7 +2571,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 		goto fail_parent_names_copy;
 	}
 
-	ret = __clk_init(hw->clk);
+	ret = __clk_init(core);
 	if (!ret)
 		return hw->clk;
 
-- 
1.9.1

Re: [RESEND PATCH v2 02/16] clk: change the argument of __clk_init() into pointer to clk_core

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> The argument clk_user is used only for the clk_user->core.  The rest
> of this function only takes care of clk_core.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 03/16] clk: rename __clk_init() into __clk_core_init()

[Masahiro Yamada]

Now this function takes clk_core as its argument.  __clk_core_init()
would be more suitable for the name of this function.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 623838f..41179f3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2285,13 +2285,13 @@ static inline void clk_debug_unregister(struct clk_core *core)
 #endif
 
 /**
- * __clk_init - initialize the data structures in a struct clk_core
+ * __clk_core_init - initialize the data structures in a struct clk_core
  * @core:	clk_core being initialized
  *
  * Initializes the lists in struct clk_core, queries the hardware for the
  * parent and rate and sets them both.
  */
-static int __clk_init(struct clk_core *core)
+static int __clk_core_init(struct clk_core *core)
 {
 	int i, ret = 0;
 	struct clk_core *orphan;
@@ -2571,7 +2571,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 		goto fail_parent_names_copy;
 	}
 
-	ret = __clk_init(core);
+	ret = __clk_core_init(core);
 	if (!ret)
 		return hw->clk;
 
-- 
1.9.1

Re: [RESEND PATCH v2 03/16] clk: rename __clk_init() into __clk_core_init()

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> Now this function takes clk_core as its argument.  __clk_core_init()
> would be more suitable for the name of this function.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 04/16] clk: remove unnecessary !core->parents conditional

[Masahiro Yamada]

This if-block has been here since the introduction of the common
clock framework.  Now no clock drivers are statically initialized.
core->parent is always NULL at this point.  Drop the redundant
check and the confusing comment.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 41179f3..30c0bbc 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2348,11 +2348,8 @@ static int __clk_core_init(struct clk_core *core)
 	 * in to clk_init during early boot; thus any access to core->parents[]
 	 * must always check for a NULL pointer and try to populate it if
 	 * necessary.
-	 *
-	 * If core->parents is not NULL we skip this entire block.  This allows
-	 * for clock drivers to statically initialize core->parents.
 	 */
-	if (core->num_parents > 1 && !core->parents) {
+	if (core->num_parents > 1) {
 		core->parents = kcalloc(core->num_parents, sizeof(struct clk *),
 					GFP_KERNEL);
 		/*
-- 
1.9.1

Re: [RESEND PATCH v2 04/16] clk: remove unnecessary !core->parents conditional

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> This if-block has been here since the introduction of the common
> clock framework.  Now no clock drivers are statically initialized.
> core->parent is always NULL at this point.  Drop the redundant
> check and the confusing comment.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 05/16] clk: change sizeof(struct clk *) to sizeof(*core->parents)

[Masahiro Yamada]

Now, the clock parent is not "struct clk *", but "struct clk_core *".
Of course, the size of a pointer is always same, but strictly speaking,
sizeof(struct clk *) should be sizeof(struct clk_core *) here.

This mismatch happened when we split the structure into struct clk
and struct clk_core.  For the potential possibility of future renaming,
sizeof(*core->parents) would be better.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 30c0bbc..98745c3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1069,7 +1069,7 @@ static int clk_fetch_parent_index(struct clk_core *core,
 
 	if (!core->parents) {
 		core->parents = kcalloc(core->num_parents,
-					sizeof(struct clk *), GFP_KERNEL);
+					sizeof(*core->parents), GFP_KERNEL);
 		if (!core->parents)
 			return -ENOMEM;
 	}
@@ -1702,7 +1702,7 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
 
 	if (!core->parents)
 		core->parents =
-			kcalloc(core->num_parents, sizeof(struct clk *),
+			kcalloc(core->num_parents, sizeof(*core->parents),
 					GFP_KERNEL);
 
 	ret = clk_core_get_parent_by_index(core, index);
@@ -2350,8 +2350,8 @@ static int __clk_core_init(struct clk_core *core)
 	 * necessary.
 	 */
 	if (core->num_parents > 1) {
-		core->parents = kcalloc(core->num_parents, sizeof(struct clk *),
-					GFP_KERNEL);
+		core->parents = kcalloc(core->num_parents,
+					sizeof(*core->parents), GFP_KERNEL);
 		/*
 		 * clk_core_lookup returns NULL for parents that have not been
 		 * clk_init'd; thus any access to clk->parents[] must check
-- 
1.9.1

Re: [RESEND PATCH v2 05/16] clk: change sizeof(struct clk *) to sizeof(*core->parents)

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> Now, the clock parent is not "struct clk *", but "struct clk_core *".
> Of course, the size of a pointer is always same, but strictly speaking,
> sizeof(struct clk *) should be sizeof(struct clk_core *) here.
> 
> This mismatch happened when we split the structure into struct clk
> and struct clk_core.  For the potential possibility of future renaming,
> sizeof(*core->parents) would be better.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 06/16] clk: move core->parents allocation to clk_register()

[Masahiro Yamada]

Currently, __clk_core_init() allows failure of the kcalloc() for the
core->parents.  So, clk_fetch_parent_index() and __clk_init_parent()
also try to allocate core->parents in case it has not been allocated
yet.  Scattering memory allocation here and there makes things
complicated.

Like other clk_core members, allocate core->parents in clk_register()
and let it fail in case of memory shortage.  If we cannot allocate
such a small piece of memory, the system is already insane.  There is
no point to postpone the memory allocation.

Also, allocate core->parents regardless of core->num_parents.  We want
it even if core->num_parents == 1 because clk_fetch_parent_index()
might be called against the clk_core with a single parent.

If core->num_parents == 0, core->parents is set to ZERO_SIZE_PTR. It
is harmless because no access happens to core->parents in such a case.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 51 +++++++++++++++++++--------------------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 98745c3..b5be02a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1067,13 +1067,6 @@ static int clk_fetch_parent_index(struct clk_core *core,
 {
 	int i;
 
-	if (!core->parents) {
-		core->parents = kcalloc(core->num_parents,
-					sizeof(*core->parents), GFP_KERNEL);
-		if (!core->parents)
-			return -ENOMEM;
-	}
-
 	/*
 	 * find index of new parent clock using cached parent ptrs,
 	 * or if not yet cached, use string name comparison and cache
@@ -1700,11 +1693,6 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
 
 	index = core->ops->get_parent(core->hw);
 
-	if (!core->parents)
-		core->parents =
-			kcalloc(core->num_parents, sizeof(*core->parents),
-					GFP_KERNEL);
-
 	ret = clk_core_get_parent_by_index(core, index);
 
 out:
@@ -2343,26 +2331,15 @@ static int __clk_core_init(struct clk_core *core)
 				__func__, core->name);
 
 	/*
-	 * Allocate an array of struct clk *'s to avoid unnecessary string
-	 * look-ups of clk's possible parents.  This can fail for clocks passed
-	 * in to clk_init during early boot; thus any access to core->parents[]
-	 * must always check for a NULL pointer and try to populate it if
-	 * necessary.
+	 * clk_core_lookup returns NULL for parents that have not been
+	 * clk_init'd; thus any access to clk->parents[] must check
+	 * for a NULL pointer.  We can always perform lazy lookups for
+	 * missing parents later on.
 	 */
-	if (core->num_parents > 1) {
-		core->parents = kcalloc(core->num_parents,
-					sizeof(*core->parents), GFP_KERNEL);
-		/*
-		 * clk_core_lookup returns NULL for parents that have not been
-		 * clk_init'd; thus any access to clk->parents[] must check
-		 * for a NULL pointer.  We can always perform lazy lookups for
-		 * missing parents later on.
-		 */
-		if (core->parents)
-			for (i = 0; i < core->num_parents; i++)
-				core->parents[i] =
-					clk_core_lookup(core->parent_names[i]);
-	}
+	if (core->parents)
+		for (i = 0; i < core->num_parents; i++)
+			core->parents[i] =
+				clk_core_lookup(core->parent_names[i]);
 
 	core->parent = __clk_init_parent(core);
 
@@ -2560,12 +2537,20 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 		}
 	}
 
+	/* avoid unnecessary string look-ups of clk_core's possible parents. */
+	core->parents = kcalloc(core->num_parents, sizeof(*core->parents),
+				GFP_KERNEL);
+	if (!core->parents) {
+		ret = -ENOMEM;
+		goto fail_parents;
+	};
+
 	INIT_HLIST_HEAD(&core->clks);
 
 	hw->clk = __clk_create_clk(hw, NULL, NULL);
 	if (IS_ERR(hw->clk)) {
 		ret = PTR_ERR(hw->clk);
-		goto fail_parent_names_copy;
+		goto fail_parents;
 	}
 
 	ret = __clk_core_init(core);
@@ -2575,6 +2560,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 	__clk_free_clk(hw->clk);
 	hw->clk = NULL;
 
+fail_parents:
+	kfree(core->parents);
 fail_parent_names_copy:
 	while (--i >= 0)
 		kfree_const(core->parent_names[i]);
-- 
1.9.1

Re: [RESEND PATCH v2 06/16] clk: move core->parents allocation to clk_register()

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> @@ -2343,26 +2331,15 @@ static int __clk_core_init(struct clk_core *core)
>  				__func__, core->name);
>  
>  	/*
> -	 * Allocate an array of struct clk *'s to avoid unnecessary string
> -	 * look-ups of clk's possible parents.  This can fail for clocks passed
> -	 * in to clk_init during early boot; thus any access to core->parents[]
> -	 * must always check for a NULL pointer and try to populate it if
> -	 * necessary.
> +	 * clk_core_lookup returns NULL for parents that have not been
> +	 * clk_init'd; thus any access to clk->parents[] must check
> +	 * for a NULL pointer.  We can always perform lazy lookups for
> +	 * missing parents later on.
>  	 */
> -	if (core->num_parents > 1) {
> -		core->parents = kcalloc(core->num_parents,
> -					sizeof(*core->parents), GFP_KERNEL);
> -		/*
> -		 * clk_core_lookup returns NULL for parents that have not been
> -		 * clk_init'd; thus any access to clk->parents[] must check
> -		 * for a NULL pointer.  We can always perform lazy lookups for
> -		 * missing parents later on.
> -		 */
> -		if (core->parents)
> -			for (i = 0; i < core->num_parents; i++)
> -				core->parents[i] =
> -					clk_core_lookup(core->parent_names[i]);
> -	}
> +	if (core->parents)

Do we even need this check? From what I can tell it's always true
now even when core->num_parents == 0. Plus we can always do the
for loop and terminate early when core->num_parents == 0.

> +		for (i = 0; i < core->num_parents; i++)
> +			core->parents[i] =
> +				clk_core_lookup(core->parent_names[i]);
>  
>  	core->parent = __clk_init_parent(core);
>  nfo.html

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND PATCH v2 06/16] clk: move core->parents allocation to clk_register()

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> Currently, __clk_core_init() allows failure of the kcalloc() for the
> core->parents.  So, clk_fetch_parent_index() and __clk_init_parent()
> also try to allocate core->parents in case it has not been allocated
> yet.  Scattering memory allocation here and there makes things
> complicated.
> 
> Like other clk_core members, allocate core->parents in clk_register()
> and let it fail in case of memory shortage.  If we cannot allocate
> such a small piece of memory, the system is already insane.  There is
> no point to postpone the memory allocation.
> 
> Also, allocate core->parents regardless of core->num_parents.  We want
> it even if core->num_parents == 1 because clk_fetch_parent_index()
> might be called against the clk_core with a single parent.
> 
> If core->num_parents == 0, core->parents is set to ZERO_SIZE_PTR. It
> is harmless because no access happens to core->parents in such a case.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 07/16] clk: simplify clk_core_get_parent_by_index()

[Masahiro Yamada]

Drop the "if (!core->parents)" case and refactor the function a bit
because core->parents is always allocated.  (Strictly speaking, it is
ZERO_SIZE_PTR if core->num_parents == 0, but such a case is omitted
by the if-conditional above.)

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b5be02a..ca7849a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -350,13 +350,12 @@ static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core,
 {
 	if (!core || index >= core->num_parents)
 		return NULL;
-	else if (!core->parents)
-		return clk_core_lookup(core->parent_names[index]);
-	else if (!core->parents[index])
-		return core->parents[index] =
-			clk_core_lookup(core->parent_names[index]);
-	else
-		return core->parents[index];
+
+	if (!core->parents[index])
+		core->parents[index] =
+				clk_core_lookup(core->parent_names[index]);
+
+	return core->parents[index];
 }
 
 struct clk_hw *
-- 
1.9.1

Re: [RESEND PATCH v2 07/16] clk: simplify clk_core_get_parent_by_index()

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> Drop the "if (!core->parents)" case and refactor the function a bit
> because core->parents is always allocated.  (Strictly speaking, it is
> ZERO_SIZE_PTR if core->num_parents == 0, but such a case is omitted
> by the if-conditional above.)
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 08/16] clk: drop the initial core->parents look-ups from __clk_core_init()

[Masahiro Yamada]

The core->parents is a cache to save expensive clock parent look-ups.
It will be filled as needed later.  We do not have to do it here.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ca7849a..3215b2b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2329,17 +2329,6 @@ static int __clk_core_init(struct clk_core *core)
 				"%s: invalid NULL in %s's .parent_names\n",
 				__func__, core->name);
 
-	/*
-	 * clk_core_lookup returns NULL for parents that have not been
-	 * clk_init'd; thus any access to clk->parents[] must check
-	 * for a NULL pointer.  We can always perform lazy lookups for
-	 * missing parents later on.
-	 */
-	if (core->parents)
-		for (i = 0; i < core->num_parents; i++)
-			core->parents[i] =
-				clk_core_lookup(core->parent_names[i]);
-
 	core->parent = __clk_init_parent(core);
 
 	/*
-- 
1.9.1

Re: [RESEND PATCH v2 08/16] clk: drop the initial core->parents look-ups from __clk_core_init()

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> The core->parents is a cache to save expensive clock parent look-ups.
> It will be filled as needed later.  We do not have to do it here.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Ah we drop it here.. I guess my point is not too important then.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND PATCH v2 08/16] clk: drop the initial core->parents look-ups from __clk_core_init()

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> The core->parents is a cache to save expensive clock parent look-ups.
> It will be filled as needed later.  We do not have to do it here.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 09/16] clk: replace pr_warn() with pr_err() for fatal cases

[Masahiro Yamada]

These three cases let clk_register() fail.  They should be considered
as error messages.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 3215b2b..e6e10f5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2302,22 +2302,22 @@ static int __clk_core_init(struct clk_core *core)
 	if (core->ops->set_rate &&
 	    !((core->ops->round_rate || core->ops->determine_rate) &&
 	      core->ops->recalc_rate)) {
-		pr_warning("%s: %s must implement .round_rate or .determine_rate in addition to .recalc_rate\n",
-				__func__, core->name);
+		pr_err("%s: %s must implement .round_rate or .determine_rate in addition to .recalc_rate\n",
+		       __func__, core->name);
 		ret = -EINVAL;
 		goto out;
 	}
 
 	if (core->ops->set_parent && !core->ops->get_parent) {
-		pr_warning("%s: %s must implement .get_parent & .set_parent\n",
-				__func__, core->name);
+		pr_err("%s: %s must implement .get_parent & .set_parent\n",
+		       __func__, core->name);
 		ret = -EINVAL;
 		goto out;
 	}
 
 	if (core->ops->set_rate_and_parent &&
 			!(core->ops->set_parent && core->ops->set_rate)) {
-		pr_warn("%s: %s must implement .set_parent & .set_rate\n",
+		pr_err("%s: %s must implement .set_parent & .set_rate\n",
 				__func__, core->name);
 		ret = -EINVAL;
 		goto out;
-- 
1.9.1

Re: [RESEND PATCH v2 09/16] clk: replace pr_warn() with pr_err() for fatal cases

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> These three cases let clk_register() fail.  They should be considered
> as error messages.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()

[Masahiro Yamada]

The .git_parent is mandatory for multi-parent clocks.  Move the check
to __clk_core_init(), like other callback checkings.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e6e10f5..4ad0a36 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1677,13 +1677,6 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
 		goto out;
 	}
 
-	if (!core->ops->get_parent) {
-		WARN(!core->ops->get_parent,
-			"%s: multi-parent clocks must implement .get_parent\n",
-			__func__);
-		goto out;
-	}
-
 	/*
 	 * Do our best to cache parent clocks in core->parents.  This prevents
 	 * unnecessary and expensive lookups.  We don't set core->parent here;
@@ -2315,6 +2308,11 @@ static int __clk_core_init(struct clk_core *core)
 		goto out;
 	}
 
+	if (core->num_parents > 1 && !core->ops->get_parent) {
+		pr_err("%s: %s must implement .get_parent as it has multi parents\n",
+		       __func__, core->name);
+	}
+
 	if (core->ops->set_rate_and_parent &&
 			!(core->ops->set_parent && core->ops->set_rate)) {
 		pr_err("%s: %s must implement .set_parent & .set_rate\n",
-- 
1.9.1

Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> The .git_parent is mandatory for multi-parent clocks.  Move the check

s/git/get/

Same for the subject.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index e6e10f5..4ad0a36 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1677,13 +1677,6 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
>  		goto out;
>  	}
>  
> -	if (!core->ops->get_parent) {
> -		WARN(!core->ops->get_parent,
> -			"%s: multi-parent clocks must implement .get_parent\n",
> -			__func__);
> -		goto out;
> -	}
> -
>  	/*
>  	 * Do our best to cache parent clocks in core->parents.  This prevents
>  	 * unnecessary and expensive lookups.  We don't set core->parent here;
> @@ -2315,6 +2308,11 @@ static int __clk_core_init(struct clk_core *core)
>  		goto out;
>  	}
>  
> +	if (core->num_parents > 1 && !core->ops->get_parent) {
> +		pr_err("%s: %s must implement .get_parent as it has multi parents\n",
> +		       __func__, core->name);
> +	}
> +

This would seem to allow a case where we may deref a null
get_parent op if it isn't set and we call __clk_init_parent().
The next patch removes that case by restructuring
__clk_init_parent(), so we should just combine these two patches
together and that theoretical problem goes away.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()

[Masahiro Yamada]

Hi Stephen,



2016-02-02 9:50 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 12/28, Masahiro Yamada wrote:
>> The .git_parent is mandatory for multi-parent clocks.  Move the check
>
> s/git/get/
>
> Same for the subject.
>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index e6e10f5..4ad0a36 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1677,13 +1677,6 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
>>               goto out;
>>       }
>>
>> -     if (!core->ops->get_parent) {
>> -             WARN(!core->ops->get_parent,
>> -                     "%s: multi-parent clocks must implement .get_parent\n",
>> -                     __func__);
>> -             goto out;
>> -     }
>> -
>>       /*
>>        * Do our best to cache parent clocks in core->parents.  This prevents
>>        * unnecessary and expensive lookups.  We don't set core->parent here;
>> @@ -2315,6 +2308,11 @@ static int __clk_core_init(struct clk_core *core)
>>               goto out;
>>       }
>>
>> +     if (core->num_parents > 1 && !core->ops->get_parent) {
>> +             pr_err("%s: %s must implement .get_parent as it has multi parents\n",
>> +                    __func__, core->name);
>> +     }
>> +
>
> This would seem to allow a case where we may deref a null
> get_parent op if it isn't set and we call __clk_init_parent().
> The next patch removes that case by restructuring
> __clk_init_parent(), so we should just combine these two patches
> together and that theoretical problem goes away.
>

I think the case never happens.

__clk_init_parent() is a static function that is only called from
__clk_core_init().
So, the null pointer for .get_parent() has been already checked before
__clk_init_parent().




-- 
Best Regards
Masahiro Yamada

Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()

[Stephen Boyd]

On 02/02, Masahiro Yamada wrote:
> 2016-02-02 9:50 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> > On 12/28, Masahiro Yamada wrote:
> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >> index e6e10f5..4ad0a36 100644
> >> --- a/drivers/clk/clk.c
> >> +++ b/drivers/clk/clk.c
> >> @@ -1677,13 +1677,6 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
> >>               goto out;
> >>       }
> >>
> >> -     if (!core->ops->get_parent) {
> >> -             WARN(!core->ops->get_parent,
> >> -                     "%s: multi-parent clocks must implement .get_parent\n",
> >> -                     __func__);
> >> -             goto out;
> >> -     }
> >> -
> >>       /*
> >>        * Do our best to cache parent clocks in core->parents.  This prevents
> >>        * unnecessary and expensive lookups.  We don't set core->parent here;
> >> @@ -2315,6 +2308,11 @@ static int __clk_core_init(struct clk_core *core)
> >>               goto out;
> >>       }
> >>
> >> +     if (core->num_parents > 1 && !core->ops->get_parent) {
> >> +             pr_err("%s: %s must implement .get_parent as it has multi parents\n",
> >> +                    __func__, core->name);
> >> +     }
> >> +
> >
> > This would seem to allow a case where we may deref a null
> > get_parent op if it isn't set and we call __clk_init_parent().
> > The next patch removes that case by restructuring
> > __clk_init_parent(), so we should just combine these two patches
> > together and that theoretical problem goes away.
> >
> 
> I think the case never happens.
> 
> __clk_init_parent() is a static function that is only called from
> __clk_core_init().
> So, the null pointer for .get_parent() has been already checked before
> __clk_init_parent().
> 

Right, this patch moves the check to __clk_core_init(), and it
will print an error and then keep going (because we lost the goto
out part). Once we keep going in __clk_core_init() we'll call
__clk_init_parent() and NULL pointer deref.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()

[Masahiro Yamada]

2016-02-02 11:39 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 02/02, Masahiro Yamada wrote:
>> 2016-02-02 9:50 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
>> > On 12/28, Masahiro Yamada wrote:
>> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> >> index e6e10f5..4ad0a36 100644
>> >> --- a/drivers/clk/clk.c
>> >> +++ b/drivers/clk/clk.c
>> >> @@ -1677,13 +1677,6 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
>> >>               goto out;
>> >>       }
>> >>
>> >> -     if (!core->ops->get_parent) {
>> >> -             WARN(!core->ops->get_parent,
>> >> -                     "%s: multi-parent clocks must implement .get_parent\n",
>> >> -                     __func__);
>> >> -             goto out;
>> >> -     }
>> >> -
>> >>       /*
>> >>        * Do our best to cache parent clocks in core->parents.  This prevents
>> >>        * unnecessary and expensive lookups.  We don't set core->parent here;
>> >> @@ -2315,6 +2308,11 @@ static int __clk_core_init(struct clk_core *core)
>> >>               goto out;
>> >>       }
>> >>
>> >> +     if (core->num_parents > 1 && !core->ops->get_parent) {
>> >> +             pr_err("%s: %s must implement .get_parent as it has multi parents\n",
>> >> +                    __func__, core->name);
>> >> +     }
>> >> +
>> >
>> > This would seem to allow a case where we may deref a null
>> > get_parent op if it isn't set and we call __clk_init_parent().
>> > The next patch removes that case by restructuring
>> > __clk_init_parent(), so we should just combine these two patches
>> > together and that theoretical problem goes away.
>> >
>>
>> I think the case never happens.
>>
>> __clk_init_parent() is a static function that is only called from
>> __clk_core_init().
>> So, the null pointer for .get_parent() has been already checked before
>> __clk_init_parent().
>>
>
> Right, this patch moves the check to __clk_core_init(), and it
> will print an error and then keep going (because we lost the goto
> out part). Once we keep going in __clk_core_init() we'll call
> __clk_init_parent() and NULL pointer deref.
>

Ah, I missed that.

We need

   ret = -EINVAL;
   goto out;

here, I think.

Shall I send the v3 for 08/16?

Or, if it is too late, shall I send a fix-up patch?




-- 
Best Regards
Masahiro Yamada

Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()

[Stephen Boyd]

On 02/02, Masahiro Yamada wrote:
> 2016-02-02 11:39 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> >
> > Right, this patch moves the check to __clk_core_init(), and it
> > will print an error and then keep going (because we lost the goto
> > out part). Once we keep going in __clk_core_init() we'll call
> > __clk_init_parent() and NULL pointer deref.
> >
> 
> Ah, I missed that.
> 
> We need
> 
>    ret = -EINVAL;
>    goto out;
> 
> here, I think.
> 
> Shall I send the v3 for 08/16?
> 
> Or, if it is too late, shall I send a fix-up patch?
> 

I'll split the patch out again and squash in those two lines.
Nothing more to do. Thanks.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()

[Masahiro Yamada]

Hi Stephen,


2016-02-03 5:43 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 02/02, Masahiro Yamada wrote:
>> 2016-02-02 11:39 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
>> >
>> > Right, this patch moves the check to __clk_core_init(), and it
>> > will print an error and then keep going (because we lost the goto
>> > out part). Once we keep going in __clk_core_init() we'll call
>> > __clk_init_parent() and NULL pointer deref.
>> >
>>
>> Ah, I missed that.
>>
>> We need
>>
>>    ret = -EINVAL;
>>    goto out;
>>
>> here, I think.
>>
>> Shall I send the v3 for 08/16?
>>
>> Or, if it is too late, shall I send a fix-up patch?
>>
>
> I'll split the patch out again and squash in those two lines.
> Nothing more to do. Thanks.


Thank you very much for taking care of all of them!



-- 
Best Regards
Masahiro Yamada

Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> The .git_parent is mandatory for multi-parent clocks.  Move the check
> to __clk_core_init(), like other callback checkings.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Squashed with the next commit and applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 11/16] clk: simplify __clk_init_parent()

[Masahiro Yamada]

The translation from the index into clk_core is done by
clk_core_get_parent_by_index().  The if-block for num_parents == 1
case is duplicating the code in the clk_core_get_parent_by_index().

Drop the "if (num_parents == 1)" from the special case.  Instead,
set the index to zero if .get_parent() is missing.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 38 ++++----------------------------------
 1 file changed, 4 insertions(+), 34 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4ad0a36..03b87d8 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1651,44 +1651,14 @@ struct clk *clk_get_parent(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_get_parent);
 
-/*
- * .get_parent is mandatory for clocks with multiple possible parents.  It is
- * optional for single-parent clocks.  Always call .get_parent if it is
- * available and WARN if it is missing for multi-parent clocks.
- *
- * For single-parent clocks without .get_parent, first check to see if the
- * .parents array exists, and if so use it to avoid an expensive tree
- * traversal.  If .parents does not exist then walk the tree.
- */
 static struct clk_core *__clk_init_parent(struct clk_core *core)
 {
-	struct clk_core *ret = NULL;
-	u8 index;
-
-	/* handle the trivial cases */
+	u8 index = 0;
 
-	if (!core->num_parents)
-		goto out;
-
-	if (core->num_parents == 1) {
-		if (IS_ERR_OR_NULL(core->parent))
-			core->parent = clk_core_lookup(core->parent_names[0]);
-		ret = core->parent;
-		goto out;
-	}
+	if (core->ops->get_parent)
+		index = core->ops->get_parent(core->hw);
 
-	/*
-	 * Do our best to cache parent clocks in core->parents.  This prevents
-	 * unnecessary and expensive lookups.  We don't set core->parent here;
-	 * that is done by the calling function.
-	 */
-
-	index = core->ops->get_parent(core->hw);
-
-	ret = clk_core_get_parent_by_index(core, index);
-
-out:
-	return ret;
+	return clk_core_get_parent_by_index(core, index);
 }
 
 static void clk_core_reparent(struct clk_core *core,
-- 
1.9.1

Re: [RESEND PATCH v2 11/16] clk: simplify __clk_init_parent()

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> The translation from the index into clk_core is done by
> clk_core_get_parent_by_index().  The if-block for num_parents == 1
> case is duplicating the code in the clk_core_get_parent_by_index().
> 
> Drop the "if (num_parents == 1)" from the special case.  Instead,
> set the index to zero if .get_parent() is missing.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 12/16] clk: avoid circular clock topology

[Masahiro Yamada]

Currently, clk_register() never checks a circular parent looping,
but clock providers could register such an insane clock topology.
For example, "clk_a" could have "clk_b" as a parent, and vice versa.
In this case, clk_core_reparent() creates a circular parent list
and __clk_recalc_accuracies() calls itself recursively forever.

The core infrastructure should be kind enough to bail out, showing
an appropriate error message in such a case.  This helps to easily
find a bug in clock providers.  (uh, I made such a silly mistake
when I was implementing my clock providers first.  I was upset
because the kernel did not respond, without any error message.)

This commit adds a new helper function, __clk_is_ancestor().  It
returns true if the second argument is a possible ancestor of the
first one.  If a clock core is a possible ancestor of itself, it
would make a loop when it were registered.  That should be detected
as an error.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 03b87d8..a4f66fe 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2235,6 +2235,38 @@ static inline void clk_debug_unregister(struct clk_core *core)
 #endif
 
 /**
+ * __clk_is_ancestor - check if a clk_core is a possible ancestor of another
+ * @core: clock core
+ * @ancestor: ancestor clock core
+ *
+ * Returns true if there is a possibility that @ancestor can be an ancestor
+ * of @core, false otherwise.
+ *
+ * This function can be used against @core or @ancestor that has not been
+ * registered yet.
+ */
+static bool __clk_is_ancestor(struct clk_core *core, struct clk_core *ancestor)
+{
+	struct clk_core *parent;
+	int i;
+
+	for (i = 0; i < core->num_parents; i++) {
+		parent = clk_core_get_parent_by_index(core, i);
+		/*
+		 * If ancestor has not been added to clk_{root,orphan}_list
+		 * yet, clk_core_lookup() cannot find it.  If parent is NULL,
+		 * compare the name strings, too.
+		 */
+		if ((parent && (parent == ancestor ||
+				__clk_is_ancestor(parent, ancestor))) ||
+		    (!parent && !strcmp(core->parent_names[i], ancestor->name)))
+			return true;
+	}
+
+	return false;
+}
+
+/**
  * __clk_core_init - initialize the data structures in a struct clk_core
  * @core:	clk_core being initialized
  *
@@ -2297,6 +2329,14 @@ static int __clk_core_init(struct clk_core *core)
 				"%s: invalid NULL in %s's .parent_names\n",
 				__func__, core->name);
 
+	/* If core is an ancestor of itself, it would make a loop. */
+	if (__clk_is_ancestor(core, core)) {
+		pr_err("%s: %s would create circular parent\n", __func__,
+		       core->name);
+		ret = -EINVAL;
+		goto out;
+	}
+
 	core->parent = __clk_init_parent(core);
 
 	/*
-- 
1.9.1

Re: [RESEND PATCH v2 12/16] clk: avoid circular clock topology

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> Currently, clk_register() never checks a circular parent looping,
> but clock providers could register such an insane clock topology.
> For example, "clk_a" could have "clk_b" as a parent, and vice versa.
> In this case, clk_core_reparent() creates a circular parent list
> and __clk_recalc_accuracies() calls itself recursively forever.
> 
> The core infrastructure should be kind enough to bail out, showing
> an appropriate error message in such a case.  This helps to easily
> find a bug in clock providers.  (uh, I made such a silly mistake
> when I was implementing my clock providers first.  I was upset
> because the kernel did not respond, without any error message.)
> 
> This commit adds a new helper function, __clk_is_ancestor().  It
> returns true if the second argument is a possible ancestor of the
> first one.  If a clock core is a possible ancestor of itself, it
> would make a loop when it were registered.  That should be detected
> as an error.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 13/16] clk: walk the orphan clock list more simply

[Masahiro Yamada]

This loop can be much simpler.  If a new parent is available for
orphan clocks, __clk_init_parent(orphan) can detect it.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a4f66fe..efd093a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2402,24 +2402,15 @@ static int __clk_core_init(struct clk_core *core)
 	core->rate = core->req_rate = rate;
 
 	/*
-	 * walk the list of orphan clocks and reparent any that are children of
-	 * this clock
+	 * walk the list of orphan clocks and reparent any that newly finds a
+	 * parent.
 	 */
 	hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
-		if (orphan->num_parents && orphan->ops->get_parent) {
-			i = orphan->ops->get_parent(orphan->hw);
-			if (i >= 0 && i < orphan->num_parents &&
-			    !strcmp(core->name, orphan->parent_names[i]))
-				clk_core_reparent(orphan, core);
-			continue;
-		}
+		struct clk_core *parent = __clk_init_parent(orphan);
 
-		for (i = 0; i < orphan->num_parents; i++)
-			if (!strcmp(core->name, orphan->parent_names[i])) {
-				clk_core_reparent(orphan, core);
-				break;
-			}
-	 }
+		if (parent)
+			clk_core_reparent(orphan, parent);
+	}
 
 	/*
 	 * optional platform-specific magic
-- 
1.9.1

Re: [RESEND PATCH v2 13/16] clk: walk the orphan clock list more simply

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> This loop can be much simpler.  If a new parent is available for
> orphan clocks, __clk_init_parent(orphan) can detect it.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 14/16] clk: make sure parent is not NULL in clk_fetch_parent_index()

[Masahiro Yamada]

If parent is given with NULL, clk_fetch_parent_index() could return
a positive index value.

Currently, parent is checked by the callers of this function, but
it would be safer to do it in this function.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - Fix a bug.  Return -EINVAL when parent _is_ NULL.

 drivers/clk/clk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index efd093a..151cbe8 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1066,6 +1066,9 @@ static int clk_fetch_parent_index(struct clk_core *core,
 {
 	int i;
 
+	if (!parent)
+		return -EINVAL;
+
 	/*
 	 * find index of new parent clock using cached parent ptrs,
 	 * or if not yet cached, use string name comparison and cache
-- 
1.9.1

Re: [RESEND PATCH v2 14/16] clk: make sure parent is not NULL in clk_fetch_parent_index()

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> If parent is given with NULL, clk_fetch_parent_index() could return
> a positive index value.
> 
> Currently, parent is checked by the callers of this function, but
> it would be safer to do it in this function.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 15/16] clk: simplify clk_fetch_parent_index() function

[Masahiro Yamada]

The clk_core_get_parent_by_index can be used as a helper function
to simplify the implementation of clk_fetch_parent_index().

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 151cbe8..5b9aec0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1069,23 +1069,9 @@ static int clk_fetch_parent_index(struct clk_core *core,
 	if (!parent)
 		return -EINVAL;
 
-	/*
-	 * find index of new parent clock using cached parent ptrs,
-	 * or if not yet cached, use string name comparison and cache
-	 * them now to avoid future calls to clk_core_lookup.
-	 */
-	for (i = 0; i < core->num_parents; i++) {
-		if (core->parents[i] == parent)
-			return i;
-
-		if (core->parents[i])
-			continue;
-
-		if (!strcmp(core->parent_names[i], parent->name)) {
-			core->parents[i] = clk_core_lookup(parent->name);
+	for (i = 0; i < core->num_parents; i++)
+		if (clk_core_get_parent_by_index(core, i) == parent)
 			return i;
-		}
-	}
 
 	return -EINVAL;
 }
-- 
1.9.1

Re: [RESEND PATCH v2 15/16] clk: simplify clk_fetch_parent_index() function

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> The clk_core_get_parent_by_index can be used as a helper function
> to simplify the implementation of clk_fetch_parent_index().
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH v2 16/16] clk: slightly optimize clk_core_set_parent()

[Masahiro Yamada]

If clk_fetch_parent_index() fails, p_rate is unused.  Move the
assignment after the error checking.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5b9aec0..e81301b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1730,13 +1730,13 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 	/* try finding the new parent index */
 	if (parent) {
 		p_index = clk_fetch_parent_index(core, parent);
-		p_rate = parent->rate;
 		if (p_index < 0) {
 			pr_debug("%s: clk %s can not be parent of clk %s\n",
 					__func__, parent->name, core->name);
 			ret = p_index;
 			goto out;
 		}
+		p_rate = parent->rate;
 	}
 
 	/* propagate PRE_RATE_CHANGE notifications */
-- 
1.9.1

Re: [RESEND PATCH v2 16/16] clk: slightly optimize clk_core_set_parent()

[Stephen Boyd]

On 12/28, Masahiro Yamada wrote:
> If clk_fetch_parent_index() fails, p_rate is unused.  Move the
> assignment after the error checking.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements

[Michael Turquette]

Quoting Masahiro Yamada (2015-12-28 02:22:54)
> Many refactoring, with detection of circular parent looping.

Thanks for the set of fixes. I'm looking over them now and will apply
after 4.5-rc1 drops if there are no issues.

Regards,
Mike

> 
> 
> Changes in v2:
>   - Fix a bug.  Return -EINVAL when parent _is_ NULL.
> 
> Masahiro Yamada (16):
>   clk: remove unused first argument of __clk_init()
>   clk: change the argument of __clk_init() into pointer to clk_core
>   clk: rename __clk_init() into __clk_core_init()
>   clk: remove unnecessary !core->parents conditional
>   clk: change sizeof(struct clk *) to sizeof(*core->parents)
>   clk: move core->parents allocation to clk_register()
>   clk: simplify clk_core_get_parent_by_index()
>   clk: drop the initial core->parents look-ups from __clk_core_init()
>   clk: replace pr_warn() with pr_err() for fatal cases
>   clk: move checking .git_parent to __clk_core_init()
>   clk: simplify __clk_init_parent()
>   clk: avoid circular clock topology
>   clk: walk the orphan clock list more simply
>   clk: make sure parent is not NULL in clk_fetch_parent_index()
>   clk: simplify clk_fetch_parent_index() function
>   clk: slightly optimize clk_core_set_parent()
> 
>  drivers/clk/clk.c | 214 ++++++++++++++++++++++--------------------------------
>  1 file changed, 85 insertions(+), 129 deletions(-)
> 
> -- 
> 1.9.1
>