Skip to contentSkip to conversation

clk: a collection of various clean-ups and improvements

PATCH RESEND Applied Masahiro Yamada 4 participants 44 messages Dec 28, 2015 – Feb 3, 2016 Common clk

lore
Share & tools
lore

Masahiro Yamada

author
#0 lore raw

v2 cover clk: a collection of various clean-ups and improvements

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(-)
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH RESEND v2 1/16 clk: remove unused first argument of __clk_init()

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

Signed-off-by Masahiro Yamada <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/clk/clk.c +2 -3
index 3fba5ac..897e5ae 100644
@@ -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;
Signature · 1 line
1.9.1
Reviewed-by on v2 1/16
#2 lore raw

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

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 <[email protected]>
Quoted 31 lines · Masahiro Yamada

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

Signed-off-by Masahiro Yamada <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/clk/clk.c +1 -2
index 3fba5ac..897e5ae 100644
@@ -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;

drivers/clk/clk.c struct clk *clk_register(struct device *dev, struct clk_hw *hw) · line 2580

Quoted 8 lines
drivers/clk/clk.c +1 -1
@@ -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

author
#3 lore raw

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

Hi Vladimir,

2016-01-08 9:44 GMT+09:00 Vladimir Zapolskiy <[email protected]>:

Quoted 8 lines

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 <[email protected]>

It is very good to hear.

Thanks for your review and test!

Signature · 2 lines
Best Regards
Masahiro Yamada
Applied
#4 lore raw

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

In reply to Masahiro Yamada
Quoted 4 lines · Masahiro Yamada

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

Signed-off-by Masahiro Yamada <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#5 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
drivers/clk/clk.c +5 -8
index 897e5ae..623838f 100644
@@ -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;
Signature · 1 line
1.9.1
Applied
#6 lore raw

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

Quoted 5 lines · 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 <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#7 lore raw

PATCH RESEND v2 3/16 clk: rename __clk_init() into __clk_core_init()

In reply to 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 <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/clk/clk.c +3 -3
index 623838f..41179f3 100644
@@ -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;
Signature · 1 line
1.9.1
Applied
#8 lore raw

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

Quoted 5 lines · 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 <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#9 lore raw

PATCH RESEND v2 4/16 clk: remove unnecessary !core->parents conditional

In reply to 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 <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/clk/clk.c +1 -4
index 41179f3..30c0bbc 100644
@@ -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);
 		/*
Signature · 1 line
1.9.1
Applied
#10 lore raw

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

Quoted 7 lines · 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 <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#11 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/clk/clk.c +4 -4
index 30c0bbc..98745c3 100644
@@ -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
Signature · 1 line
1.9.1
Applied
#12 lore raw

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

Quoted 10 lines · 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 <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#13 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 51 +++++++++++++++++++--------------------------------
 1 file changed, 19 insertions(+), 32 deletions(-)
drivers/clk/clk.c +19 -32
index 98745c3..b5be02a 100644
@@ -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]);
Signature · 1 line
1.9.1
#14 lore raw

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

Quoted 19 lines · Masahiro Yamada
drivers/clk/clk.c +4 -10
@@ -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

drivers/clk/clk.c static int __clk_core_init(struct clk_core *core) · line 2339

Quoted 10 lines
drivers/clk/clk.c +1 -9
-		 * 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.

Quoted 6 lines
drivers/clk/clk.c +3 -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
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Applied
#15 lore raw

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

In reply to Masahiro Yamada
Quoted 15 lines · 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.

Quoted 5 lines

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 <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#16 lore raw

PATCH RESEND v2 7/16 clk: simplify clk_core_get_parent_by_index()

In reply to 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 <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/clk/clk.c +6 -7
index b5be02a..ca7849a 100644
@@ -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 *
Signature · 1 line
1.9.1
Applied
#17 lore raw

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

Quoted 7 lines · 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 <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#18 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 11 -----------
 1 file changed, 11 deletions(-)
drivers/clk/clk.c +0 -11
index ca7849a..3215b2b 100644
@@ -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);
 
 	/*
Signature · 1 line
1.9.1
#19 lore raw

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

Quoted 5 lines · 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 <[email protected]>

---

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

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Applied
#20 lore raw

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

In reply to Masahiro Yamada
Quoted 5 lines · 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 <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#21 lore raw

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

In reply to Masahiro Yamada

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

Signed-off-by Masahiro Yamada <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/clk/clk.c +5 -5
index 3215b2b..e6e10f5 100644
@@ -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;
Signature · 1 line
1.9.1
Applied
#22 lore raw

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

Quoted 5 lines · Masahiro Yamada

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

Signed-off-by Masahiro Yamada <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#23 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
drivers/clk/clk.c +5 -7
index e6e10f5..4ad0a36 100644
@@ -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",
Signature · 1 line
1.9.1
#24 lore raw

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

Quoted 1 line · Masahiro Yamada

The .git_parent is mandatory for multi-parent clocks. Move the check

s/git/get/

Same for the subject.

Quoted 19 lines
drivers/clk/clk.c +0 -7
index e6e10f5..4ad0a36 100644
@@ -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;

drivers/clk/clk.c static int __clk_core_init(struct clk_core *core) · line 2315

Quoted 8 lines
drivers/clk/clk.c +5 -0
@@ -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.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#25 lore raw

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

Hi Stephen,

2016-02-02 9:50 GMT+09:00 Stephen Boyd <[email protected]>:

Quoted 29 lines · Masahiro Yamada
Quoted 1 line

The .git_parent is mandatory for multi-parent clocks. Move the check

s/git/get/

Same for the subject.

Quoted 22 lines
drivers/clk/clk.c +0 -7
index e6e10f5..4ad0a36 100644
@@ -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;
      }
Quoted 12 lines

drivers/clk/clk.c static int __clk_core_init(struct clk_core *core) · line 2315

Quoted 5 lines
drivers/clk/clk.c +5 -0
+     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().

Signature · 2 lines
Best Regards
Masahiro Yamada
#26 lore raw

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

Quoted 37 lines · Masahiro Yamada

2016-02-02 9:50 GMT+09:00 Stephen Boyd <[email protected]>:

Quoted 23 lines · Masahiro Yamada
Quoted 22 lines
drivers/clk/clk.c +0 -7
index e6e10f5..4ad0a36 100644
@@ -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;
      }
Quoted 12 lines

drivers/clk/clk.c static int __clk_core_init(struct clk_core *core) · line 2315

Quoted 5 lines
drivers/clk/clk.c +5 -0
+     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.

Quoted 7 lines

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.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#27 lore raw

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

2016-02-02 11:39 GMT+09:00 Stephen Boyd <[email protected]>:

Quoted 46 lines · Masahiro Yamada
Quoted 37 lines

2016-02-02 9:50 GMT+09:00 Stephen Boyd <[email protected]>:

Quoted 23 lines · Masahiro Yamada
Quoted 22 lines
drivers/clk/clk.c +0 -7
index e6e10f5..4ad0a36 100644
@@ -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;
      }
Quoted 12 lines

drivers/clk/clk.c static int __clk_core_init(struct clk_core *core) · line 2315

Quoted 5 lines
drivers/clk/clk.c +5 -0
+     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.

Quoted 7 lines

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

Quoted 5 lines

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?

Signature · 2 lines
Best Regards
Masahiro Yamada
#28 lore raw

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

Quoted 12 lines · Masahiro Yamada

2016-02-02 11:39 GMT+09:00 Stephen Boyd <[email protected]>:

Quoted 6 lines

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

Quoted 9 lines
   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.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#29 lore raw

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

Hi Stephen,

2016-02-03 5:43 GMT+09:00 Stephen Boyd <[email protected]>:

Quoted 18 lines · Masahiro Yamada
Quoted 17 lines

2016-02-02 11:39 GMT+09:00 Stephen Boyd <[email protected]>:

Quoted 6 lines

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.

Quoted 7 lines
Quoted 4 lines

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!

Signature · 2 lines
Best Regards
Masahiro Yamada
#30 lore raw

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

In reply to Masahiro Yamada
Quoted 5 lines · 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 <[email protected]>

---

Squashed with the next commit and applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#31 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 38 ++++----------------------------------
 1 file changed, 4 insertions(+), 34 deletions(-)
drivers/clk/clk.c +4 -34
index 4ad0a36..03b87d8 100644
@@ -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,
Signature · 1 line
1.9.1
Applied
#32 lore raw

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

Quoted 9 lines · 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 <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#33 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
drivers/clk/clk.c +40 -0
index 03b87d8..a4f66fe 100644
@@ -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);
 
 	/*
Signature · 1 line
1.9.1
Applied
#34 lore raw

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

Quoted 12 lines · 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.)

Quoted 8 lines

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 <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#35 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)
drivers/clk/clk.c +6 -15
index a4f66fe..efd093a 100644
@@ -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
Signature · 1 line
1.9.1
Applied
#36 lore raw

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

Quoted 5 lines · 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 <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#37 lore raw

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

In reply to 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 <[email protected]>

---

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

 drivers/clk/clk.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/clk/clk.c +3 -0
index efd093a..151cbe8 100644
@@ -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
Signature · 1 line
1.9.1
Applied
#38 lore raw

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

Quoted 8 lines · 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 <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#39 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)
drivers/clk/clk.c +2 -16
index 151cbe8..5b9aec0 100644
@@ -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;
 }
Signature · 1 line
1.9.1
Applied
#40 lore raw

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

Quoted 5 lines · 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 <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#41 lore raw

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

In reply to Masahiro Yamada

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

Signed-off-by Masahiro Yamada <[email protected]>

---

Changes in v2: None

 drivers/clk/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/clk/clk.c +1 -1
index 5b9aec0..e81301b 100644
@@ -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 */
Signature · 1 line
1.9.1
Applied
#42 lore raw

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

Quoted 5 lines · Masahiro Yamada

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

Signed-off-by Masahiro Yamada <[email protected]>

---

Applied to clk-next

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
#43 lore raw
In reply to Masahiro Yamada

Quoting Masahiro Yamada (2015-12-28 02:22:54)

Quoted 1 line

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

Quoted 27 lines

=

=

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()
=

Quoted 1 line

drivers/clk/clk.c | 214 ++++++++++++++++++++++--------------------------=

------

Quoted 7 lines
 1 file changed, 85 insertions(+), 129 deletions(-)

=

-- =

1.9.1
=20