* [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements
@ 2015-12-28 10:22 Masahiro Yamada
2015-12-28 10:22 ` [RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init() Masahiro Yamada
` (16 more replies)
0 siblings, 17 replies; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:22 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init()
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
@ 2015-12-28 10:22 ` Masahiro Yamada
2016-01-08 0:44 ` Vladimir Zapolskiy
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:22 ` [RESEND PATCH v2 02/16] clk: change the argument of __clk_init() into pointer to clk_core Masahiro Yamada
` (15 subsequent siblings)
16 siblings, 2 replies; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:22 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 02/16] clk: change the argument of __clk_init() into pointer to clk_core
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
2015-12-28 10:22 ` [RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init() Masahiro Yamada
@ 2015-12-28 10:22 ` Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:22 ` [RESEND PATCH v2 03/16] clk: rename __clk_init() into __clk_core_init() Masahiro Yamada
` (14 subsequent siblings)
16 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:22 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 03/16] clk: rename __clk_init() into __clk_core_init()
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
2015-12-28 10:22 ` [RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init() Masahiro Yamada
2015-12-28 10:22 ` [RESEND PATCH v2 02/16] clk: change the argument of __clk_init() into pointer to clk_core Masahiro Yamada
@ 2015-12-28 10:22 ` Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:22 ` [RESEND PATCH v2 04/16] clk: remove unnecessary !core->parents conditional Masahiro Yamada
` (13 subsequent siblings)
16 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:22 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 04/16] clk: remove unnecessary !core->parents conditional
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
` (2 preceding siblings ...)
2015-12-28 10:22 ` [RESEND PATCH v2 03/16] clk: rename __clk_init() into __clk_core_init() Masahiro Yamada
@ 2015-12-28 10:22 ` Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:22 ` [RESEND PATCH v2 05/16] clk: change sizeof(struct clk *) to sizeof(*core->parents) Masahiro Yamada
` (12 subsequent siblings)
16 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:22 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 05/16] clk: change sizeof(struct clk *) to sizeof(*core->parents)
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
` (3 preceding siblings ...)
2015-12-28 10:22 ` [RESEND PATCH v2 04/16] clk: remove unnecessary !core->parents conditional Masahiro Yamada
@ 2015-12-28 10:22 ` Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 06/16] clk: move core->parents allocation to clk_register() Masahiro Yamada
` (11 subsequent siblings)
16 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:22 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 06/16] clk: move core->parents allocation to clk_register()
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
` (4 preceding siblings ...)
2015-12-28 10:22 ` [RESEND PATCH v2 05/16] clk: change sizeof(struct clk *) to sizeof(*core->parents) Masahiro Yamada
@ 2015-12-28 10:23 ` Masahiro Yamada
2016-02-02 0:23 ` Stephen Boyd
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 07/16] clk: simplify clk_core_get_parent_by_index() Masahiro Yamada
` (10 subsequent siblings)
16 siblings, 2 replies; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:23 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 07/16] clk: simplify clk_core_get_parent_by_index()
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
` (5 preceding siblings ...)
2015-12-28 10:23 ` [RESEND PATCH v2 06/16] clk: move core->parents allocation to clk_register() Masahiro Yamada
@ 2015-12-28 10:23 ` Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 08/16] clk: drop the initial core->parents look-ups from __clk_core_init() Masahiro Yamada
` (9 subsequent siblings)
16 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:23 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 08/16] clk: drop the initial core->parents look-ups from __clk_core_init()
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
` (6 preceding siblings ...)
2015-12-28 10:23 ` [RESEND PATCH v2 07/16] clk: simplify clk_core_get_parent_by_index() Masahiro Yamada
@ 2015-12-28 10:23 ` Masahiro Yamada
2016-02-02 0:25 ` Stephen Boyd
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 09/16] clk: replace pr_warn() with pr_err() for fatal cases Masahiro Yamada
` (8 subsequent siblings)
16 siblings, 2 replies; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:23 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 09/16] clk: replace pr_warn() with pr_err() for fatal cases
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
` (7 preceding siblings ...)
2015-12-28 10:23 ` [RESEND PATCH v2 08/16] clk: drop the initial core->parents look-ups from __clk_core_init() Masahiro Yamada
@ 2015-12-28 10:23 ` Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init() Masahiro Yamada
` (7 subsequent siblings)
16 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:23 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
` (8 preceding siblings ...)
2015-12-28 10:23 ` [RESEND PATCH v2 09/16] clk: replace pr_warn() with pr_err() for fatal cases Masahiro Yamada
@ 2015-12-28 10:23 ` Masahiro Yamada
2016-02-02 0:50 ` Stephen Boyd
2016-02-02 1:08 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 11/16] clk: simplify __clk_init_parent() Masahiro Yamada
` (6 subsequent siblings)
16 siblings, 2 replies; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:23 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 11/16] clk: simplify __clk_init_parent()
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
` (9 preceding siblings ...)
2015-12-28 10:23 ` [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init() Masahiro Yamada
@ 2015-12-28 10:23 ` Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 12/16] clk: avoid circular clock topology Masahiro Yamada
` (5 subsequent siblings)
16 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:23 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 12/16] clk: avoid circular clock topology
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
` (10 preceding siblings ...)
2015-12-28 10:23 ` [RESEND PATCH v2 11/16] clk: simplify __clk_init_parent() Masahiro Yamada
@ 2015-12-28 10:23 ` Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 13/16] clk: walk the orphan clock list more simply Masahiro Yamada
` (4 subsequent siblings)
16 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:23 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 13/16] clk: walk the orphan clock list more simply
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
` (11 preceding siblings ...)
2015-12-28 10:23 ` [RESEND PATCH v2 12/16] clk: avoid circular clock topology Masahiro Yamada
@ 2015-12-28 10:23 ` Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 14/16] clk: make sure parent is not NULL in clk_fetch_parent_index() Masahiro Yamada
` (3 subsequent siblings)
16 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:23 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 14/16] clk: make sure parent is not NULL in clk_fetch_parent_index()
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
` (12 preceding siblings ...)
2015-12-28 10:23 ` [RESEND PATCH v2 13/16] clk: walk the orphan clock list more simply Masahiro Yamada
@ 2015-12-28 10:23 ` Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 15/16] clk: simplify clk_fetch_parent_index() function Masahiro Yamada
` (2 subsequent siblings)
16 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:23 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 15/16] clk: simplify clk_fetch_parent_index() function
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
` (13 preceding siblings ...)
2015-12-28 10:23 ` [RESEND PATCH v2 14/16] clk: make sure parent is not NULL in clk_fetch_parent_index() Masahiro Yamada
@ 2015-12-28 10:23 ` Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 16/16] clk: slightly optimize clk_core_set_parent() Masahiro Yamada
2016-01-04 23:57 ` Michael Turquette
16 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:23 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [RESEND PATCH v2 16/16] clk: slightly optimize clk_core_set_parent()
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
` (14 preceding siblings ...)
2015-12-28 10:23 ` [RESEND PATCH v2 15/16] clk: simplify clk_fetch_parent_index() function Masahiro Yamada
@ 2015-12-28 10:23 ` Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
2016-01-04 23:57 ` Michael Turquette
16 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2015-12-28 10:23 UTC (permalink / raw)
To: linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
@ 2016-01-04 23:57 ` Michael Turquette
2015-12-28 10:22 ` [RESEND PATCH v2 02/16] clk: change the argument of __clk_init() into pointer to clk_core Masahiro Yamada
` (15 subsequent siblings)
16 siblings, 0 replies; 45+ messages in thread
From: Michael Turquette @ 2016-01-04 23:57 UTC (permalink / raw)
To: Masahiro Yamada, linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, linux-kernel
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
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements
@ 2016-01-04 23:57 ` Michael Turquette
0 siblings, 0 replies; 45+ messages in thread
From: Michael Turquette @ 2016-01-04 23:57 UTC (permalink / raw)
To: Masahiro Yamada, linux-clk; +Cc: Masahiro Yamada, Stephen Boyd, linux-kernel
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
>=20
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init()
2015-12-28 10:22 ` [RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init() Masahiro Yamada
@ 2016-01-08 0:44 ` Vladimir Zapolskiy
2016-01-08 3:56 ` Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
1 sibling, 1 reply; 45+ messages in thread
From: Vladimir Zapolskiy @ 2016-01-08 0:44 UTC (permalink / raw)
To: Masahiro Yamada, linux-clk; +Cc: Stephen Boyd, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init()
2016-01-08 0:44 ` Vladimir Zapolskiy
@ 2016-01-08 3:56 ` Masahiro Yamada
0 siblings, 0 replies; 45+ messages in thread
From: Masahiro Yamada @ 2016-01-08 3:56 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: linux-clk, Stephen Boyd, Michael Turquette, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 06/16] clk: move core->parents allocation to clk_register()
2015-12-28 10:23 ` [RESEND PATCH v2 06/16] clk: move core->parents allocation to clk_register() Masahiro Yamada
@ 2016-02-02 0:23 ` Stephen Boyd
2016-02-02 1:07 ` Stephen Boyd
1 sibling, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 0:23 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 08/16] clk: drop the initial core->parents look-ups from __clk_core_init()
2015-12-28 10:23 ` [RESEND PATCH v2 08/16] clk: drop the initial core->parents look-ups from __clk_core_init() Masahiro Yamada
@ 2016-02-02 0:25 ` Stephen Boyd
2016-02-02 1:07 ` Stephen Boyd
1 sibling, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 0:25 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()
2015-12-28 10:23 ` [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init() Masahiro Yamada
@ 2016-02-02 0:50 ` Stephen Boyd
2016-02-02 2:19 ` Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
1 sibling, 1 reply; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 0:50 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init()
2015-12-28 10:22 ` [RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init() Masahiro Yamada
2016-01-08 0:44 ` Vladimir Zapolskiy
@ 2016-02-02 1:07 ` Stephen Boyd
1 sibling, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:07 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 02/16] clk: change the argument of __clk_init() into pointer to clk_core
2015-12-28 10:22 ` [RESEND PATCH v2 02/16] clk: change the argument of __clk_init() into pointer to clk_core Masahiro Yamada
@ 2016-02-02 1:07 ` Stephen Boyd
0 siblings, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:07 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 03/16] clk: rename __clk_init() into __clk_core_init()
2015-12-28 10:22 ` [RESEND PATCH v2 03/16] clk: rename __clk_init() into __clk_core_init() Masahiro Yamada
@ 2016-02-02 1:07 ` Stephen Boyd
0 siblings, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:07 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 04/16] clk: remove unnecessary !core->parents conditional
2015-12-28 10:22 ` [RESEND PATCH v2 04/16] clk: remove unnecessary !core->parents conditional Masahiro Yamada
@ 2016-02-02 1:07 ` Stephen Boyd
0 siblings, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:07 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 05/16] clk: change sizeof(struct clk *) to sizeof(*core->parents)
2015-12-28 10:22 ` [RESEND PATCH v2 05/16] clk: change sizeof(struct clk *) to sizeof(*core->parents) Masahiro Yamada
@ 2016-02-02 1:07 ` Stephen Boyd
0 siblings, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:07 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 06/16] clk: move core->parents allocation to clk_register()
2015-12-28 10:23 ` [RESEND PATCH v2 06/16] clk: move core->parents allocation to clk_register() Masahiro Yamada
2016-02-02 0:23 ` Stephen Boyd
@ 2016-02-02 1:07 ` Stephen Boyd
1 sibling, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:07 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 07/16] clk: simplify clk_core_get_parent_by_index()
2015-12-28 10:23 ` [RESEND PATCH v2 07/16] clk: simplify clk_core_get_parent_by_index() Masahiro Yamada
@ 2016-02-02 1:07 ` Stephen Boyd
0 siblings, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:07 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 08/16] clk: drop the initial core->parents look-ups from __clk_core_init()
2015-12-28 10:23 ` [RESEND PATCH v2 08/16] clk: drop the initial core->parents look-ups from __clk_core_init() Masahiro Yamada
2016-02-02 0:25 ` Stephen Boyd
@ 2016-02-02 1:07 ` Stephen Boyd
1 sibling, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:07 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 09/16] clk: replace pr_warn() with pr_err() for fatal cases
2015-12-28 10:23 ` [RESEND PATCH v2 09/16] clk: replace pr_warn() with pr_err() for fatal cases Masahiro Yamada
@ 2016-02-02 1:07 ` Stephen Boyd
0 siblings, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:07 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()
2015-12-28 10:23 ` [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init() Masahiro Yamada
2016-02-02 0:50 ` Stephen Boyd
@ 2016-02-02 1:08 ` Stephen Boyd
1 sibling, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:08 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 11/16] clk: simplify __clk_init_parent()
2015-12-28 10:23 ` [RESEND PATCH v2 11/16] clk: simplify __clk_init_parent() Masahiro Yamada
@ 2016-02-02 1:08 ` Stephen Boyd
0 siblings, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:08 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 12/16] clk: avoid circular clock topology
2015-12-28 10:23 ` [RESEND PATCH v2 12/16] clk: avoid circular clock topology Masahiro Yamada
@ 2016-02-02 1:08 ` Stephen Boyd
0 siblings, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:08 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 13/16] clk: walk the orphan clock list more simply
2015-12-28 10:23 ` [RESEND PATCH v2 13/16] clk: walk the orphan clock list more simply Masahiro Yamada
@ 2016-02-02 1:08 ` Stephen Boyd
0 siblings, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:08 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 14/16] clk: make sure parent is not NULL in clk_fetch_parent_index()
2015-12-28 10:23 ` [RESEND PATCH v2 14/16] clk: make sure parent is not NULL in clk_fetch_parent_index() Masahiro Yamada
@ 2016-02-02 1:08 ` Stephen Boyd
0 siblings, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:08 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 15/16] clk: simplify clk_fetch_parent_index() function
2015-12-28 10:23 ` [RESEND PATCH v2 15/16] clk: simplify clk_fetch_parent_index() function Masahiro Yamada
@ 2016-02-02 1:08 ` Stephen Boyd
0 siblings, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:08 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 16/16] clk: slightly optimize clk_core_set_parent()
2015-12-28 10:23 ` [RESEND PATCH v2 16/16] clk: slightly optimize clk_core_set_parent() Masahiro Yamada
@ 2016-02-02 1:08 ` Stephen Boyd
0 siblings, 0 replies; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 1:08 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, linux-kernel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()
2016-02-02 0:50 ` Stephen Boyd
@ 2016-02-02 2:19 ` Masahiro Yamada
2016-02-02 2:39 ` Stephen Boyd
0 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2016-02-02 2:19 UTC (permalink / raw)
To: Stephen Boyd; +Cc: linux-clk, Michael Turquette, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()
2016-02-02 2:19 ` Masahiro Yamada
@ 2016-02-02 2:39 ` Stephen Boyd
2016-02-02 2:48 ` Masahiro Yamada
0 siblings, 1 reply; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 2:39 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()
2016-02-02 2:39 ` Stephen Boyd
@ 2016-02-02 2:48 ` Masahiro Yamada
2016-02-02 20:43 ` Stephen Boyd
0 siblings, 1 reply; 45+ messages in thread
From: Masahiro Yamada @ 2016-02-02 2:48 UTC (permalink / raw)
To: Stephen Boyd; +Cc: linux-clk, Michael Turquette, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()
2016-02-02 2:48 ` Masahiro Yamada
@ 2016-02-02 20:43 ` Stephen Boyd
2016-02-03 2:09 ` Masahiro Yamada
0 siblings, 1 reply; 45+ messages in thread
From: Stephen Boyd @ 2016-02-02 20:43 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-clk, Michael Turquette, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()
2016-02-02 20:43 ` Stephen Boyd
@ 2016-02-03 2:09 ` Masahiro Yamada
0 siblings, 0 replies; 45+ messages in thread
From: Masahiro Yamada @ 2016-02-03 2:09 UTC (permalink / raw)
To: Stephen Boyd; +Cc: linux-clk, Michael Turquette, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 45+ messages in thread
end of thread, other threads:[~2016-02-03 2:09 UTC | newest]
Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-28 10:22 [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Masahiro Yamada
2015-12-28 10:22 ` [RESEND PATCH v2 01/16] clk: remove unused first argument of __clk_init() Masahiro Yamada
2016-01-08 0:44 ` Vladimir Zapolskiy
2016-01-08 3:56 ` Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:22 ` [RESEND PATCH v2 02/16] clk: change the argument of __clk_init() into pointer to clk_core Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:22 ` [RESEND PATCH v2 03/16] clk: rename __clk_init() into __clk_core_init() Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:22 ` [RESEND PATCH v2 04/16] clk: remove unnecessary !core->parents conditional Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:22 ` [RESEND PATCH v2 05/16] clk: change sizeof(struct clk *) to sizeof(*core->parents) Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 06/16] clk: move core->parents allocation to clk_register() Masahiro Yamada
2016-02-02 0:23 ` Stephen Boyd
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 07/16] clk: simplify clk_core_get_parent_by_index() Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 08/16] clk: drop the initial core->parents look-ups from __clk_core_init() Masahiro Yamada
2016-02-02 0:25 ` Stephen Boyd
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 09/16] clk: replace pr_warn() with pr_err() for fatal cases Masahiro Yamada
2016-02-02 1:07 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init() Masahiro Yamada
2016-02-02 0:50 ` Stephen Boyd
2016-02-02 2:19 ` Masahiro Yamada
2016-02-02 2:39 ` Stephen Boyd
2016-02-02 2:48 ` Masahiro Yamada
2016-02-02 20:43 ` Stephen Boyd
2016-02-03 2:09 ` Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 11/16] clk: simplify __clk_init_parent() Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 12/16] clk: avoid circular clock topology Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 13/16] clk: walk the orphan clock list more simply Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 14/16] clk: make sure parent is not NULL in clk_fetch_parent_index() Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 15/16] clk: simplify clk_fetch_parent_index() function Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
2015-12-28 10:23 ` [RESEND PATCH v2 16/16] clk: slightly optimize clk_core_set_parent() Masahiro Yamada
2016-02-02 1:08 ` Stephen Boyd
2016-01-04 23:57 ` [RESEND PATCH v2 00/16] clk: a collection of various clean-ups and improvements Michael Turquette
2016-01-04 23:57 ` Michael Turquette
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.