[PATCH v3 0/7] Rewrite clk parent handling

[PATCH v3 0/7] Rewrite clk parent handling

[Stephen Boyd]

There are a couple warts with clk parent handling in the common clk
framework. This patch series addresses one of those warts, parent-child
linkages. We use strings for all parent-child linkages, and this leads
to poorly written code that extracts clk names from struct clk pointers
and makes clk provider drivers use clk consumer APIs. I've converted the
fixed factor clk and I'm looking at converting users of other "basic"
clk types in follow-up patches so we can start moving drivers away from
string matching in the clk namespace.

Changes from v2:
 * Dropped patches that got merged into v5.1-rc1
 * Introduced a new function of_clk_hw_register()
 * Introduced 'index' into clk_parent_data structure 
 * Converted fixed-factor basic type to new design
 * Fixed some bugs in a recent patch to clkdev, leading to
   an essential API for clkdev based lookups working

Changes from v1:
 * Dropped new get_parent_hw, we'll pick it up in a later series
 * Rebased to v5.0-rc6 to avoid conflicts with clk-fixes
 * Renamed 'fallback' to 'name' and 'name' to 'fw_name' in parent map
 * Made sure that clk_hw_get_parent_by_index() never sees an error pointer
   so that we don't mistakenly try to defer an error pointer
 * Fixed index passing mistake on of_clk_get_hw_from_clkspec()
 * Copy over all the data from parent maps and free it correctly too

Stephen Boyd (7):
  clkdev: Hold clocks_mutex while iterating clocks list
  clk: Prepare for clk registration API that uses DT nodes
  clk: Add of_clk_hw_register() API for early clk drivers
  clk: Allow parents to be specified without string names
  clk: Look for parents with clkdev based clk_lookups
  clk: Allow parents to be specified via clkspec index
  clk: fixed-factor: Let clk framework find parent

Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

 drivers/clk/clk-fixed-factor.c |  53 ++++--
 drivers/clk/clk.c              | 326 +++++++++++++++++++++++++--------
 drivers/clk/clk.h              |   2 +
 drivers/clk/clkdev.c           |  38 ++--
 include/linux/clk-provider.h   |  22 +++
 5 files changed, 334 insertions(+), 107 deletions(-)


base-commit: 9e98c678c2d6ae3a17cb2de55d17f69dddaa231b
-- 
Sent by a computer through tubes

[PATCH v3 1/7] clkdev: Hold clocks_mutex while iterating clocks list

[Stephen Boyd]

We recently introduced a change to support devm clk lookups. That change
introduced a code-path that used clk_find() without holding the
'clocks_mutex'. Unfortunately, clk_find() iterates over the 'clocks'
list and so we need to prevent the list from being modified while
iterating over it by holding the mutex. Similarly, we don't need to hold
the 'clocks_mutex' besides when we're dereferencing the clk_lookup
pointer we find or while we're modifying/iterating the 'clocks' list.

Let's ensure that we have the mutex held while iterating the list and
fix the callers of clk_find() to only hold the mutex as long as they
need to. This should fix this race and also nicely move clk creation
outside of the 'clocks_mutex'.

Fixes: 3eee6c7d119c ("clkdev: add managed clkdev lookup registration")
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clkdev.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 8c4435c53f09..db82eee8e209 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -36,7 +36,7 @@ static DEFINE_MUTEX(clocks_mutex);
  * Then we take the most specific entry - with the following
  * order of precedence: dev+con > dev only > con only.
  */
-static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
+static struct clk_lookup *__clk_find(const char *dev_id, const char *con_id)
 {
 	struct clk_lookup *p, *cl = NULL;
 	int match, best_found = 0, best_possible = 0;
@@ -46,6 +46,8 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
 	if (con_id)
 		best_possible += 1;
 
+	lockdep_assert_held(&clocks_mutex);
+
 	list_for_each_entry(p, &clocks, node) {
 		match = 0;
 		if (p->dev_id) {
@@ -70,25 +72,37 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
 	return cl;
 }
 
-static struct clk *__clk_get_sys(struct device *dev, const char *dev_id,
-				 const char *con_id)
+static struct clk_hw *clk_find_hw(const char *dev_id, const char *con_id)
 {
 	struct clk_lookup *cl;
-	struct clk *clk = NULL;
+	struct clk_hw *hw = ERR_PTR(-ENOENT);
 
 	mutex_lock(&clocks_mutex);
+	cl = __clk_find(dev_id, con_id);
+	if (cl)
+		hw = cl->clk_hw;
+	mutex_unlock(&clocks_mutex);
 
-	cl = clk_find(dev_id, con_id);
-	if (!cl)
-		goto out;
+	return hw;
+}
 
-	clk = clk_hw_create_clk(dev, cl->clk_hw, dev_id, con_id);
-	if (IS_ERR(clk))
-		cl = NULL;
-out:
+static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
+{
+	struct clk_lookup *cl;
+
+	mutex_lock(&clocks_mutex);
+	cl = __clk_find(dev_id, con_id);
 	mutex_unlock(&clocks_mutex);
 
-	return cl ? clk : ERR_PTR(-ENOENT);
+	return cl;
+}
+
+static struct clk *__clk_get_sys(struct device *dev, const char *dev_id,
+				 const char *con_id)
+{
+	struct clk_hw *hw = clk_find_hw(dev_id, con_id);
+
+	return clk_hw_create_clk(dev, hw, dev_id, con_id);
 }
 
 struct clk *clk_get_sys(const char *dev_id, const char *con_id)
-- 
Sent by a computer through tubes

[PATCH v3 2/7] clk: Prepare for clk registration API that uses DT nodes

[Stephen Boyd]

Split out the body of the clk_register() function so it can be shared
between the different types of registration APIs (DT, device).

Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 96053a96fe2f..d27775a73e67 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3313,18 +3313,7 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
 	return clk;
 }
 
-/**
- * clk_register - allocate a new clock, register it and return an opaque cookie
- * @dev: device that is registering this clock
- * @hw: link to hardware-specific clock data
- *
- * clk_register is the primary interface for populating the clock tree with new
- * clock nodes.  It returns a pointer to the newly allocated struct clk which
- * cannot be dereferenced by driver code but may be used in conjunction with the
- * rest of the clock API.  In the event of an error clk_register will return an
- * error code; drivers must test for an error code after calling clk_register.
- */
-struct clk *clk_register(struct device *dev, struct clk_hw *hw)
+static struct clk *__clk_register(struct device *dev, struct clk_hw *hw)
 {
 	int i, ret;
 	struct clk_core *core;
@@ -3426,6 +3415,22 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 fail_out:
 	return ERR_PTR(ret);
 }
+
+/**
+ * clk_register - allocate a new clock, register it and return an opaque cookie
+ * @dev: device that is registering this clock
+ * @hw: link to hardware-specific clock data
+ *
+ * clk_register is the primary interface for populating the clock tree with new
+ * clock nodes.  It returns a pointer to the newly allocated struct clk which
+ * cannot be dereferenced by driver code but may be used in conjunction with the
+ * rest of the clock API.  In the event of an error clk_register will return an
+ * error code; drivers must test for an error code after calling clk_register.
+ */
+struct clk *clk_register(struct device *dev, struct clk_hw *hw)
+{
+	return __clk_register(dev, hw);
+}
 EXPORT_SYMBOL_GPL(clk_register);
 
 /**
@@ -3440,7 +3445,7 @@ EXPORT_SYMBOL_GPL(clk_register);
  */
 int clk_hw_register(struct device *dev, struct clk_hw *hw)
 {
-	return PTR_ERR_OR_ZERO(clk_register(dev, hw));
+	return PTR_ERR_OR_ZERO(__clk_register(dev, hw));
 }
 EXPORT_SYMBOL_GPL(clk_hw_register);
 
-- 
Sent by a computer through tubes

[PATCH v3 3/7] clk: Add of_clk_hw_register() API for early clk drivers

[Stephen Boyd]

In some circumstances drivers register clks early and don't have access
to a struct device because the device model isn't initialized yet. Add
an API to let drivers register clks associated with a struct device_node
so that these drivers can participate in getting parent clks through DT.

Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk.c            | 26 +++++++++++++++++++++++---
 include/linux/clk-provider.h |  1 +
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d27775a73e67..4971e9651ca5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -45,6 +45,7 @@ struct clk_core {
 	struct clk_hw		*hw;
 	struct module		*owner;
 	struct device		*dev;
+	struct device_node	*of_node;
 	struct clk_core		*parent;
 	const char		**parent_names;
 	struct clk_core		**parents;
@@ -3313,7 +3314,8 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
 	return clk;
 }
 
-static struct clk *__clk_register(struct device *dev, struct clk_hw *hw)
+static struct clk *
+__clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 {
 	int i, ret;
 	struct clk_core *core;
@@ -3339,6 +3341,7 @@ static struct clk *__clk_register(struct device *dev, struct clk_hw *hw)
 	if (dev && pm_runtime_enabled(dev))
 		core->rpm_enabled = true;
 	core->dev = dev;
+	core->of_node = np;
 	if (dev && dev->driver)
 		core->owner = dev->driver->owner;
 	core->hw = hw;
@@ -3429,7 +3432,7 @@ static struct clk *__clk_register(struct device *dev, struct clk_hw *hw)
  */
 struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 {
-	return __clk_register(dev, hw);
+	return __clk_register(dev, dev->of_node, hw);
 }
 EXPORT_SYMBOL_GPL(clk_register);
 
@@ -3445,10 +3448,27 @@ EXPORT_SYMBOL_GPL(clk_register);
  */
 int clk_hw_register(struct device *dev, struct clk_hw *hw)
 {
-	return PTR_ERR_OR_ZERO(__clk_register(dev, hw));
+	return PTR_ERR_OR_ZERO(__clk_register(dev, dev->of_node, hw));
 }
 EXPORT_SYMBOL_GPL(clk_hw_register);
 
+/*
+ * of_clk_hw_register - register a clk_hw and return an error code
+ * @node: device_node of device that is registering this clock
+ * @hw: link to hardware-specific clock data
+ *
+ * of_clk_hw_register() is the primary interface for populating the clock tree
+ * with new clock nodes when a struct device is not available, but a struct
+ * device_node is. It returns an integer equal to zero indicating success or
+ * less than zero indicating failure. Drivers must test for an error code after
+ * calling of_clk_hw_register().
+ */
+int of_clk_hw_register(struct device_node *node, struct clk_hw *hw)
+{
+	return PTR_ERR_OR_ZERO(__clk_register(NULL, node, hw));
+}
+EXPORT_SYMBOL_GPL(of_clk_hw_register);
+
 /* Free memory allocated for a clock. */
 static void __clk_release(struct kref *ref)
 {
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index b7cf80a71293..7d2d97e15b76 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -773,6 +773,7 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw);
 
 int __must_check clk_hw_register(struct device *dev, struct clk_hw *hw);
 int __must_check devm_clk_hw_register(struct device *dev, struct clk_hw *hw);
+int __must_check of_clk_hw_register(struct device_node *node, struct clk_hw *hw);
 
 void clk_unregister(struct clk *clk);
 void devm_clk_unregister(struct device *dev, struct clk *clk);
-- 
Sent by a computer through tubes

[PATCH v3 4/7] clk: Allow parents to be specified without string names

[Stephen Boyd]

The common clk framework is lacking in ability to describe the clk
topology without specifying strings for every possible parent-child
link. There are a few drawbacks to the current approach:

 1) String comparisons are used for everything, including describing
 topologies that are 'local' to a single clock controller.

 2) clk providers (e.g. i2c clk drivers) need to create globally unique
 clk names to avoid collisions in the clk namespace, leading to awkward
 name generation code in various clk drivers.

 3) DT bindings may not fully describe the clk topology and linkages
 between clk controllers because drivers can easily rely on globally unique
 strings to describe connections between clks.

This leads to confusing DT bindings, complicated clk name generation
code, and inefficient string comparisons during clk registration just so
that the clk framework can detect the topology of the clk tree.
Furthermore, some drivers call clk_get() and then __clk_get_name() to
extract the globally unique clk name just so they can specify the parent
of the clk they're registering. We have of_clk_parent_fill() but that
mostly only works for single clks registered from a DT node, which isn't
the norm. Let's simplify this all by introducing two new ways of
specifying clk parents.

The first method is an array of pointers to clk_hw structures
corresponding to the parents at that index. This works for clks that are
registered when we have access to all the clk_hw pointers for the
parents.

The second method is a mix of clk_hw pointers and strings of local and
global parent clk names. If the .fw_name member of the map is set we'll
look for that clk by performing a DT based lookup of the device the clk
is registered with and the .name specified in the map. If that fails,
we'll fallback to the .name member and perform a global clk name lookup
like we've always done before.

Using either one of these new methods is entirely optional. Existing
drivers will continue to work, and they can migrate to this new approach
as they see fit. Eventually, we'll want to get rid of the 'parent_names'
array in struct clk_init_data and use one of these new methods instead.

Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk.c            | 262 ++++++++++++++++++++++++++---------
 include/linux/clk-provider.h |  19 +++
 2 files changed, 219 insertions(+), 62 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4971e9651ca5..7092714e5d5c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list);
 
 /***    private data structures    ***/
 
+struct clk_parent_map {
+	const struct clk_hw	*hw;
+	struct clk_core		*core;
+	const char	 	*fw_name;
+	const char	 	*name;
+};
+
 struct clk_core {
 	const char		*name;
 	const struct clk_ops	*ops;
@@ -47,8 +54,7 @@ struct clk_core {
 	struct device		*dev;
 	struct device_node	*of_node;
 	struct clk_core		*parent;
-	const char		**parent_names;
-	struct clk_core		**parents;
+	struct clk_parent_map	*parents;
 	u8			num_parents;
 	u8			new_parent_index;
 	unsigned long		rate;
@@ -317,17 +323,92 @@ static struct clk_core *clk_core_lookup(const char *name)
 	return NULL;
 }
 
+/**
+ * clk_core_get - Find the parent of a clk using a clock specifier in DT
+ * @core: clk to find parent of
+ * @name: name to search for in 'clock-names' of device providing clk
+ *
+ * This is the preferred method for clk providers to find the parent of a
+ * clk when that parent is external to the clk controller. The parent_names
+ * array is indexed and treated as a local name matching a string in the device
+ * node's 'clock-names' property. This allows clk providers to use their own
+ * namespace instead of looking for a globally unique parent string.
+ *
+ * For example the following DT snippet would allow a clock registered by the
+ * clock-controller@c001 that has a clk_init_data::parent_data array
+ * with 'xtal' in the 'name' member to find the clock provided by the
+ * clock-controller@f00abcd without needing to get the globally unique name of
+ * the xtal clk.
+ *
+ * 	parent: clock-controller@f00abcd {
+ * 		reg = <0xf00abcd 0xabcd>;
+ * 		#clock-cells = <0>;
+ * 	};
+ *
+ * 	clock-controller@c001 {
+ * 		reg = <0xc001 0xf00d>;
+ * 		clocks = <&parent>;
+ * 		clock-names = "xtal";
+ * 		#clock-cells = <1>;
+ * 	};
+ *
+ * Returns: -ENOENT when the provider can't be found or the clk doesn't
+ * exist in the provider. -EINVAL when the name can't be found. NULL when the
+ * provider knows about the clk but it isn't provided on this system.
+ * A valid clk_core pointer when the clk can be found in the provider.
+ */
+static struct clk_core *clk_core_get(struct clk_core *core, const char *name)
+{
+	struct clk_hw *hw;
+	struct device_node *np = core->of_node;
+
+	if (!np)
+		return ERR_PTR(-ENOENT);
+
+	/* TODO: Support clkdev clk_lookups */
+	hw = of_clk_get_hw(np, -1, name);
+	if (IS_ERR_OR_NULL(hw))
+		return ERR_CAST(hw);
+
+	return hw->core;
+}
+
+static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
+{
+	struct clk_parent_map *entry = &core->parents[index];
+	struct clk_core *parent = ERR_PTR(-ENOENT);
+
+	if (entry->hw) {
+		parent = entry->hw->core;
+		/*
+		 * We have a direct reference but it isn't registered yet? Orphan it
+		 * and let clk_reparent() update the orphan status when the parent
+		 * is registered.
+		 */
+		if (!parent)
+			parent = ERR_PTR(-EPROBE_DEFER);
+	} else {
+		if (entry->fw_name)
+			parent = clk_core_get(core, entry->fw_name);
+		if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT)
+			parent = clk_core_lookup(entry->name);
+	}
+
+	/* Only cache it if it's not an error */
+	if (!IS_ERR(parent))
+		entry->core = parent;
+}
+
 static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core,
 							 u8 index)
 {
-	if (!core || index >= core->num_parents)
+	if (!core || index >= core->num_parents || !core->parents)
 		return NULL;
 
-	if (!core->parents[index])
-		core->parents[index] =
-				clk_core_lookup(core->parent_names[index]);
+	if (!core->parents[index].core)
+		clk_core_fill_parent_index(core, index);
 
-	return core->parents[index];
+	return core->parents[index].core;
 }
 
 struct clk_hw *
@@ -1520,15 +1601,15 @@ static int clk_fetch_parent_index(struct clk_core *core,
 		return -EINVAL;
 
 	for (i = 0; i < core->num_parents; i++) {
-		if (core->parents[i] == parent)
+		if (core->parents[i].core == parent)
 			return i;
 
-		if (core->parents[i])
+		if (core->parents[i].core)
 			continue;
 
 		/* Fallback to comparing globally unique names */
-		if (!strcmp(parent->name, core->parent_names[i])) {
-			core->parents[i] = parent;
+		if (!strcmp(parent->name, core->parents[i].name)) {
+			core->parents[i].core = parent;
 			return i;
 		}
 	}
@@ -2294,6 +2375,7 @@ void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent)
 bool clk_has_parent(struct clk *clk, struct clk *parent)
 {
 	struct clk_core *core, *parent_core;
+	int i;
 
 	/* NULL clocks should be nops, so return success if either is NULL. */
 	if (!clk || !parent)
@@ -2306,8 +2388,11 @@ bool clk_has_parent(struct clk *clk, struct clk *parent)
 	if (core->parent == parent_core)
 		return true;
 
-	return match_string(core->parent_names, core->num_parents,
-			    parent_core->name) >= 0;
+	for (i = 0; i < core->num_parents; i++)
+		if (!strcmp(core->parents[i].name, parent_core->name))
+			return true;
+
+	return false;
 }
 EXPORT_SYMBOL_GPL(clk_has_parent);
 
@@ -2890,9 +2975,9 @@ static int possible_parents_show(struct seq_file *s, void *data)
 	int i;
 
 	for (i = 0; i < core->num_parents - 1; i++)
-		seq_printf(s, "%s ", core->parent_names[i]);
+		seq_printf(s, "%s ", core->parents[i].name);
 
-	seq_printf(s, "%s\n", core->parent_names[i]);
+	seq_printf(s, "%s\n", core->parents[i].name);
 
 	return 0;
 }
@@ -3026,7 +3111,7 @@ static inline void clk_debug_unregister(struct clk_core *core)
  */
 static int __clk_core_init(struct clk_core *core)
 {
-	int i, ret;
+	int ret;
 	struct clk_core *orphan;
 	struct hlist_node *tmp2;
 	unsigned long rate;
@@ -3080,12 +3165,6 @@ static int __clk_core_init(struct clk_core *core)
 		goto out;
 	}
 
-	/* throw a WARN if any entries in parent_names are NULL */
-	for (i = 0; i < core->num_parents; i++)
-		WARN(!core->parent_names[i],
-				"%s: invalid NULL in %s's .parent_names\n",
-				__func__, core->name);
-
 	core->parent = __clk_init_parent(core);
 
 	/*
@@ -3314,10 +3393,102 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
 	return clk;
 }
 
+static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
+{
+	const char *dst;
+
+	if (!src) {
+		if (must_exist)
+			return -EINVAL;
+		return 0;
+	}
+
+	*dst_p = dst = kstrdup_const(src, GFP_KERNEL);
+	if (!dst)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int clk_core_populate_parent_map(struct clk_core *core)
+{
+	const struct clk_init_data *init = core->hw->init;
+	u8 num_parents = init->num_parents;
+	const char * const *parent_names = init->parent_names;
+	const struct clk_hw **parent_hws = init->parent_hws;
+	const struct clk_parent_data *parent_data = init->parent_data;
+	int i, ret = 0;
+	struct clk_parent_map *parents, *parent;
+
+	if (!num_parents)
+		return 0;
+
+	/*
+	 * Avoid unnecessary string look-ups of clk_core's possible parents by
+	 * having a cache of names/clk_hw pointers to clk_core pointers.
+	 */
+	parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL);
+	core->parents = parents;
+	if (!parents)
+		return -ENOMEM;
+
+	/* Copy everything over because it might be __initdata */
+	for (i = 0, parent = parents; i < num_parents; i++, parent++) {
+		if (parent_names) {
+			/* throw a WARN if any entries are NULL */
+			WARN(!parent_names[i],
+				"%s: invalid NULL in %s's .parent_names\n",
+				__func__, core->name);
+			ret = clk_cpy_name(&parent->name, parent_names[i],
+					   true);
+		} else if (parent_data) {
+			parent->hw = parent_data[i].hw;
+			ret = clk_cpy_name(&parent->fw_name,
+					   parent_data[i].fw_name, false);
+			if (!ret)
+				ret = clk_cpy_name(&parent->name,
+						   parent_data[i].name,
+						   false);
+		} else if (parent_hws) {
+			parent->hw = parent_hws[i];
+		} else {
+			ret = -EINVAL;
+			WARN(1, "Must specify parents if num_parents > 0\n");
+		}
+
+		if (ret) {
+			do {
+				kfree_const(parents[i].name);
+				kfree_const(parents[i].fw_name);
+			} while (--i >= 0);
+			kfree(parents);
+
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void clk_core_free_parent_map(struct clk_core *core)
+{
+	int i = core->num_parents;
+
+	if (!core->num_parents)
+		return;
+
+	while (--i >= 0) {
+		kfree_const(core->parents[i].name);
+		kfree_const(core->parents[i].fw_name);
+	}
+
+	kfree(core->parents);
+}
+
 static struct clk *
 __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 {
-	int i, ret;
+	int ret;
 	struct clk_core *core;
 
 	core = kzalloc(sizeof(*core), GFP_KERNEL);
@@ -3351,33 +3522,9 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 	core->max_rate = ULONG_MAX;
 	hw->core = core;
 
-	/* allocate local copy in case parent_names is __initdata */
-	core->parent_names = kcalloc(core->num_parents, sizeof(char *),
-					GFP_KERNEL);
-
-	if (!core->parent_names) {
-		ret = -ENOMEM;
-		goto fail_parent_names;
-	}
-
-
-	/* copy each string name in case parent_names is __initdata */
-	for (i = 0; i < core->num_parents; i++) {
-		core->parent_names[i] = kstrdup_const(hw->init->parent_names[i],
-						GFP_KERNEL);
-		if (!core->parent_names[i]) {
-			ret = -ENOMEM;
-			goto fail_parent_names_copy;
-		}
-	}
-
-	/* 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;
+	ret = clk_core_populate_parent_map(core);
+	if (ret)
 		goto fail_parents;
-	};
 
 	INIT_HLIST_HEAD(&core->clks);
 
@@ -3388,7 +3535,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 	hw->clk = alloc_clk(core, NULL, NULL);
 	if (IS_ERR(hw->clk)) {
 		ret = PTR_ERR(hw->clk);
-		goto fail_parents;
+		goto fail_create_clk;
 	}
 
 	clk_core_link_consumer(hw->core, hw->clk);
@@ -3404,13 +3551,9 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 	free_clk(hw->clk);
 	hw->clk = NULL;
 
+fail_create_clk:
+	clk_core_free_parent_map(core);
 fail_parents:
-	kfree(core->parents);
-fail_parent_names_copy:
-	while (--i >= 0)
-		kfree_const(core->parent_names[i]);
-	kfree(core->parent_names);
-fail_parent_names:
 fail_ops:
 	kfree_const(core->name);
 fail_name:
@@ -3473,15 +3616,10 @@ EXPORT_SYMBOL_GPL(of_clk_hw_register);
 static void __clk_release(struct kref *ref)
 {
 	struct clk_core *core = container_of(ref, struct clk_core, ref);
-	int i = core->num_parents;
 
 	lockdep_assert_held(&prepare_lock);
 
-	kfree(core->parents);
-	while (--i >= 0)
-		kfree_const(core->parent_names[i]);
-
-	kfree(core->parent_names);
+	clk_core_free_parent_map(core);
 	kfree_const(core->name);
 	kfree(core);
 }
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 7d2d97e15b76..95d5a9d4e8c3 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -250,6 +250,18 @@ struct clk_ops {
 	void		(*debug_init)(struct clk_hw *hw, struct dentry *dentry);
 };
 
+/**
+ * struct clk_parent_data - clk parent information
+ * @hw: parent clk_hw pointer (used for clk providers with internal clks)
+ * @fw_name: parent name local to provider registering clk
+ * @name: globally unique parent name (used as a fallback)
+ */
+struct clk_parent_data {
+	const struct clk_hw 	*hw;
+	const char		*fw_name;
+	const char		*name;
+};
+
 /**
  * struct clk_init_data - holds init data that's common to all clocks and is
  * shared between the clock provider and the common clock framework.
@@ -257,13 +269,20 @@ struct clk_ops {
  * @name: clock name
  * @ops: operations this clock supports
  * @parent_names: array of string names for all possible parents
+ * @parent_data: array of parent data for all possible parents (when some
+ *               parents are external to the clk controller)
+ * @parent_hws: array of pointers to all possible parents (when all parents
+ *              are internal to the clk controller)
  * @num_parents: number of possible parents
  * @flags: framework-level hints and quirks
  */
 struct clk_init_data {
 	const char		*name;
 	const struct clk_ops	*ops;
+	/* Only one of the following three should be assigned */
 	const char		* const *parent_names;
+	const struct clk_parent_data	*parent_data;
+	const struct clk_hw		**parent_hws;
 	u8			num_parents;
 	unsigned long		flags;
 };
-- 
Sent by a computer through tubes

[PATCH v3 5/7] clk: Look for parents with clkdev based clk_lookups

[Stephen Boyd]

In addition to looking for DT based parents, support clkdev based
clk_lookups. This should allow non-DT based clk drivers to participate
in the parent lookup process.

Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk.c    | 27 ++++++++++++++++++---------
 drivers/clk/clk.h    |  2 ++
 drivers/clk/clkdev.c |  2 +-
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 7092714e5d5c..f8f5ae011c93 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -324,14 +324,15 @@ static struct clk_core *clk_core_lookup(const char *name)
 }
 
 /**
- * clk_core_get - Find the parent of a clk using a clock specifier in DT
+ * clk_core_get - Find the clk_core parent of a clk
  * @core: clk to find parent of
- * @name: name to search for in 'clock-names' of device providing clk
+ * @name: name to search for
  *
  * This is the preferred method for clk providers to find the parent of a
  * clk when that parent is external to the clk controller. The parent_names
  * array is indexed and treated as a local name matching a string in the device
- * node's 'clock-names' property. This allows clk providers to use their own
+ * node's 'clock-names' property or as the 'con_id' matching the device's
+ * dev_name() in a clk_lookup. This allows clk providers to use their own
  * namespace instead of looking for a globally unique parent string.
  *
  * For example the following DT snippet would allow a clock registered by the
@@ -359,15 +360,23 @@ static struct clk_core *clk_core_lookup(const char *name)
  */
 static struct clk_core *clk_core_get(struct clk_core *core, const char *name)
 {
-	struct clk_hw *hw;
+	struct clk_hw *hw = ERR_PTR(-ENOENT);
+	struct device *dev = core->dev;
+	const char *dev_id = dev ? dev_name(dev) : NULL;
 	struct device_node *np = core->of_node;
 
-	if (!np)
-		return ERR_PTR(-ENOENT);
+	if (np)
+		hw = of_clk_get_hw(np, -1, name);
 
-	/* TODO: Support clkdev clk_lookups */
-	hw = of_clk_get_hw(np, -1, name);
-	if (IS_ERR_OR_NULL(hw))
+	/*
+	 * If the DT search above couldn't find the provider or the provider
+	 * didn't know about this clk, fallback to looking up via clkdev based
+	 * clk_lookups
+	 */
+	if (PTR_ERR(hw) == -ENOENT)
+		hw = clk_find_hw(dev_id, name);
+
+	if (IS_ERR(hw))
 		return ERR_CAST(hw);
 
 	return hw->core;
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 553f531cc232..d8400d623b34 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -19,6 +19,8 @@ static inline struct clk_hw *of_clk_get_hw(struct device_node *np,
 }
 #endif
 
+struct clk_hw *clk_find_hw(const char *dev_id, const char *con_id);
+
 #ifdef CONFIG_COMMON_CLK
 struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
 			      const char *dev_id, const char *con_id);
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index db82eee8e209..13e2926ea151 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -72,7 +72,7 @@ static struct clk_lookup *__clk_find(const char *dev_id, const char *con_id)
 	return cl;
 }
 
-static struct clk_hw *clk_find_hw(const char *dev_id, const char *con_id)
+struct clk_hw *clk_find_hw(const char *dev_id, const char *con_id)
 {
 	struct clk_lookup *cl;
 	struct clk_hw *hw = ERR_PTR(-ENOENT);
-- 
Sent by a computer through tubes

[PATCH v3 6/7] clk: Allow parents to be specified via clkspec index

[Stephen Boyd]

Some clk providers are simple DT nodes that only have a 'clocks'
property without having an associated 'clock-names' property. In these
cases, we want to let these clk providers point to their parent clks
without having to dereference the 'clocks' property at probe time to
figure out the parent's globally unique clk name. Let's add an 'index'
property to the parent_data structure so that clk providers can indicate
that their parent is a particular index in the 'clocks' DT property.

Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk.c            | 18 +++++++++++-------
 include/linux/clk-provider.h |  2 ++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f8f5ae011c93..2ea8b426d9f8 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -44,6 +44,7 @@ struct clk_parent_map {
 	struct clk_core		*core;
 	const char	 	*fw_name;
 	const char	 	*name;
+	int			index;
 };
 
 struct clk_core {
@@ -326,7 +327,8 @@ static struct clk_core *clk_core_lookup(const char *name)
 /**
  * clk_core_get - Find the clk_core parent of a clk
  * @core: clk to find parent of
- * @name: name to search for
+ * @name: name to search for (if string based)
+ * @index: index to use for search (if DT index based)
  *
  * This is the preferred method for clk providers to find the parent of a
  * clk when that parent is external to the clk controller. The parent_names
@@ -358,22 +360,23 @@ static struct clk_core *clk_core_lookup(const char *name)
  * provider knows about the clk but it isn't provided on this system.
  * A valid clk_core pointer when the clk can be found in the provider.
  */
-static struct clk_core *clk_core_get(struct clk_core *core, const char *name)
+static struct clk_core *clk_core_get(struct clk_core *core, const char *name,
+				     int index)
 {
 	struct clk_hw *hw = ERR_PTR(-ENOENT);
 	struct device *dev = core->dev;
 	const char *dev_id = dev ? dev_name(dev) : NULL;
 	struct device_node *np = core->of_node;
 
-	if (np)
-		hw = of_clk_get_hw(np, -1, name);
+	if (np && index >= 0)
+		hw = of_clk_get_hw(np, index, name);
 
 	/*
 	 * If the DT search above couldn't find the provider or the provider
 	 * didn't know about this clk, fallback to looking up via clkdev based
 	 * clk_lookups
 	 */
-	if (PTR_ERR(hw) == -ENOENT)
+	if (PTR_ERR(hw) == -ENOENT && name)
 		hw = clk_find_hw(dev_id, name);
 
 	if (IS_ERR(hw))
@@ -397,8 +400,7 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
 		if (!parent)
 			parent = ERR_PTR(-EPROBE_DEFER);
 	} else {
-		if (entry->fw_name)
-			parent = clk_core_get(core, entry->fw_name);
+		parent = clk_core_get(core, entry->fw_name, entry->index);
 		if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT)
 			parent = clk_core_lookup(entry->name);
 	}
@@ -3443,6 +3445,7 @@ static int clk_core_populate_parent_map(struct clk_core *core)
 
 	/* Copy everything over because it might be __initdata */
 	for (i = 0, parent = parents; i < num_parents; i++, parent++) {
+		parent->index = -1;
 		if (parent_names) {
 			/* throw a WARN if any entries are NULL */
 			WARN(!parent_names[i],
@@ -3452,6 +3455,7 @@ static int clk_core_populate_parent_map(struct clk_core *core)
 					   true);
 		} else if (parent_data) {
 			parent->hw = parent_data[i].hw;
+			parent->index = parent_data[i].index;
 			ret = clk_cpy_name(&parent->fw_name,
 					   parent_data[i].fw_name, false);
 			if (!ret)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 95d5a9d4e8c3..12fef24b740f 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -255,11 +255,13 @@ struct clk_ops {
  * @hw: parent clk_hw pointer (used for clk providers with internal clks)
  * @fw_name: parent name local to provider registering clk
  * @name: globally unique parent name (used as a fallback)
+ * @index: parent index local to provider registering clk (if @fw_name absent)
  */
 struct clk_parent_data {
 	const struct clk_hw 	*hw;
 	const char		*fw_name;
 	const char		*name;
+	int			index;
 };
 
 /**
-- 
Sent by a computer through tubes

[PATCH v3 7/7] clk: fixed-factor: Let clk framework find parent

[Stephen Boyd]

Convert this driver to a more modern way of specifying parents now that
we have a way to specify clk parents by DT index. This lets us nicely
avoid a problem where a parent clk name isn't know because the parent
clk hasn't been registered yet.

Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk-fixed-factor.c | 53 +++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 241b3f8c61a9..67cc7e515e42 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -64,12 +64,14 @@ const struct clk_ops clk_fixed_factor_ops = {
 };
 EXPORT_SYMBOL_GPL(clk_fixed_factor_ops);
 
-struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
-		const char *name, const char *parent_name, unsigned long flags,
-		unsigned int mult, unsigned int div)
+static struct clk_hw *
+__clk_hw_register_fixed_factor(struct device *dev, struct device_node *np,
+		const char *name, const char *parent_name, int index,
+		unsigned long flags, unsigned int mult, unsigned int div)
 {
 	struct clk_fixed_factor *fix;
 	struct clk_init_data init;
+	struct clk_parent_data pdata = { .index = index };
 	struct clk_hw *hw;
 	int ret;
 
@@ -85,11 +87,17 @@ struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
 	init.name = name;
 	init.ops = &clk_fixed_factor_ops;
 	init.flags = flags | CLK_IS_BASIC;
-	init.parent_names = &parent_name;
+	if (parent_name)
+		init.parent_names = &parent_name;
+	else
+		init.parent_data = &pdata;
 	init.num_parents = 1;
 
 	hw = &fix->hw;
-	ret = clk_hw_register(dev, hw);
+	if (dev)
+		ret = clk_hw_register(dev, hw);
+	else
+		ret = of_clk_hw_register(np, hw);
 	if (ret) {
 		kfree(fix);
 		hw = ERR_PTR(ret);
@@ -97,6 +105,14 @@ struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
 
 	return hw;
 }
+
+struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
+		const char *name, const char *parent_name, unsigned long flags,
+		unsigned int mult, unsigned int div)
+{
+	return __clk_hw_register_fixed_factor(dev, NULL, name, parent_name, -1, flags,
+					      mult, div);
+}
 EXPORT_SYMBOL_GPL(clk_hw_register_fixed_factor);
 
 struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
@@ -143,11 +159,10 @@ static const struct of_device_id set_rate_parent_matches[] = {
 	{ /* Sentinel */ },
 };
 
-static struct clk *_of_fixed_factor_clk_setup(struct device_node *node)
+static struct clk_hw *_of_fixed_factor_clk_setup(struct device_node *node)
 {
-	struct clk *clk;
+	struct clk_hw *hw;
 	const char *clk_name = node->name;
-	const char *parent_name;
 	unsigned long flags = 0;
 	u32 div, mult;
 	int ret;
@@ -165,30 +180,28 @@ static struct clk *_of_fixed_factor_clk_setup(struct device_node *node)
 	}
 
 	of_property_read_string(node, "clock-output-names", &clk_name);
-	parent_name = of_clk_get_parent_name(node, 0);
 
 	if (of_match_node(set_rate_parent_matches, node))
 		flags |= CLK_SET_RATE_PARENT;
 
-	clk = clk_register_fixed_factor(NULL, clk_name, parent_name, flags,
-					mult, div);
-	if (IS_ERR(clk)) {
+	hw = __clk_hw_register_fixed_factor(NULL, node, clk_name, NULL, 0,
+					    flags, mult, div);
+	if (IS_ERR(hw)) {
 		/*
-		 * If parent clock is not registered, registration would fail.
 		 * Clear OF_POPULATED flag so that clock registration can be
 		 * attempted again from probe function.
 		 */
 		of_node_clear_flag(node, OF_POPULATED);
-		return clk;
+		return ERR_CAST(hw);
 	}
 
-	ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	ret = of_clk_add_hw_provider(node, of_clk_hw_simple_get, hw);
 	if (ret) {
-		clk_unregister(clk);
+		clk_hw_unregister_fixed_factor(hw);
 		return ERR_PTR(ret);
 	}
 
-	return clk;
+	return hw;
 }
 
 /**
@@ -203,17 +216,17 @@ CLK_OF_DECLARE(fixed_factor_clk, "fixed-factor-clock",
 
 static int of_fixed_factor_clk_remove(struct platform_device *pdev)
 {
-	struct clk *clk = platform_get_drvdata(pdev);
+	struct clk_hw *clk = platform_get_drvdata(pdev);
 
 	of_clk_del_provider(pdev->dev.of_node);
-	clk_unregister_fixed_factor(clk);
+	clk_hw_unregister_fixed_factor(clk);
 
 	return 0;
 }
 
 static int of_fixed_factor_clk_probe(struct platform_device *pdev)
 {
-	struct clk *clk;
+	struct clk_hw *clk;
 
 	/*
 	 * This function is not executed when of_fixed_factor_clk_setup
-- 
Sent by a computer through tubes

Re: [PATCH v3 1/7] clkdev: Hold clocks_mutex while iterating clocks list

[Vaittinen, Matti]

Hello Stephen,

Thanks for taking care of this!

On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote:
> We recently introduced a change to support devm clk lookups. That
> change
> introduced a code-path that used clk_find() without holding the
> 'clocks_mutex'. Unfortunately, clk_find() iterates over the 'clocks'
> list and so we need to prevent the list from being modified while
> iterating over it by holding the mutex. Similarly, we don't need to
> hold
> the 'clocks_mutex' besides when we're dereferencing the clk_lookup
> pointer

/// Snip

> -out:
> +static struct clk_lookup *clk_find(const char *dev_id, const char
> *con_id)
> +{
> +	struct clk_lookup *cl;
> +
> +	mutex_lock(&clocks_mutex);
> +	cl = __clk_find(dev_id, con_id);
>  	mutex_unlock(&clocks_mutex);
>  
> -	return cl ? clk : ERR_PTR(-ENOENT);
> +	return cl;
> +}

I am not an expert on this but reading commit message abowe and seeing
the code for clk_find() looks a bit scary. If I understand it
correctly, the clocks_mutex should be held when dereferencing the
clk_lookup returned by clk_find. The clk_find implementation drops the
lock before returning - which makes me think I miss something here. How
can the caller ever safely dereference returned clk_lookup pointer?
Just reading abowe makes me think that lock should be taken by whoever
is calling the clk_find, and dropped only after caller has used the
found clk_lookup for whatever caller intends to use it. Maybe I am
missing something?

Br,
	Matti Vaittinen

Re: [PATCH v3 1/7] clkdev: Hold clocks_mutex while iterating clocks list

[Stephen Boyd]

Quoting Vaittinen, Matti (2019-04-04 23:51:43)
> On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote:
> > We recently introduced a change to support devm clk lookups. That
> > change
> > introduced a code-path that used clk_find() without holding the
> > 'clocks_mutex'. Unfortunately, clk_find() iterates over the 'clocks'
> > list and so we need to prevent the list from being modified while
> > iterating over it by holding the mutex. Similarly, we don't need to
> > hold
> > the 'clocks_mutex' besides when we're dereferencing the clk_lookup
> > pointer
> 
> /// Snip
> 
> > -out:
> > +static struct clk_lookup *clk_find(const char *dev_id, const char
> > *con_id)
> > +{
> > +     struct clk_lookup *cl;
> > +
> > +     mutex_lock(&clocks_mutex);
> > +     cl = __clk_find(dev_id, con_id);
> >       mutex_unlock(&clocks_mutex);
> >  
> > -     return cl ? clk : ERR_PTR(-ENOENT);
> > +     return cl;
> > +}
> 
> I am not an expert on this but reading commit message abowe and seeing
> the code for clk_find() looks a bit scary. If I understand it
> correctly, the clocks_mutex should be held when dereferencing the
> clk_lookup returned by clk_find. The clk_find implementation drops the
> lock before returning - which makes me think I miss something here. How
> can the caller ever safely dereference returned clk_lookup pointer?
> Just reading abowe makes me think that lock should be taken by whoever
> is calling the clk_find, and dropped only after caller has used the
> found clk_lookup for whatever caller intends to use it. Maybe I am
> missing something?
> 

The only user after this patch (devm) is doing a pointer comparison so
it looks OK. But yes, in general there shouldn't be users of clk_find()
that dereference the pointer because there isn't any protection besides
the mutex.

Re: [PATCH v3 1/7] clkdev: Hold clocks_mutex while iterating clocks list

[Matti Vaittinen]

On Fri, Apr 05, 2019 at 01:37:24PM -0700, Stephen Boyd wrote:
> Quoting Vaittinen, Matti (2019-04-04 23:51:43)
> > On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote:
> > > We recently introduced a change to support devm clk lookups. That
> > > change
> > > introduced a code-path that used clk_find() without holding the
> > > 'clocks_mutex'. Unfortunately, clk_find() iterates over the 'clocks'
> > > list and so we need to prevent the list from being modified while
> > > iterating over it by holding the mutex. Similarly, we don't need to
> > > hold
> > > the 'clocks_mutex' besides when we're dereferencing the clk_lookup
> > > pointer
> > 
> > /// Snip
> > 
> > > -out:
> > > +static struct clk_lookup *clk_find(const char *dev_id, const char
> > > *con_id)
> > > +{
> > > +     struct clk_lookup *cl;
> > > +
> > > +     mutex_lock(&clocks_mutex);
> > > +     cl = __clk_find(dev_id, con_id);
> > >       mutex_unlock(&clocks_mutex);
> > >  
> > > -     return cl ? clk : ERR_PTR(-ENOENT);
> > > +     return cl;
> > > +}
> > 
> > I am not an expert on this but reading commit message abowe and seeing
> > the code for clk_find() looks a bit scary. If I understand it
> > correctly, the clocks_mutex should be held when dereferencing the
> > clk_lookup returned by clk_find. The clk_find implementation drops the
> > lock before returning - which makes me think I miss something here. How
> > can the caller ever safely dereference returned clk_lookup pointer?
> > Just reading abowe makes me think that lock should be taken by whoever
> > is calling the clk_find, and dropped only after caller has used the
> > found clk_lookup for whatever caller intends to use it. Maybe I am
> > missing something?
> > 
> 
> The only user after this patch (devm) is doing a pointer comparison so
> it looks OK. But yes, in general there shouldn't be users of clk_find()
> that dereference the pointer because there isn't any protection besides
> the mutex.

If the only (intended) user for clk_find is devm_clk_release_clkdev,
then we might want to write it in devm_clk_release_clkdev - just to
avoid similar errors (as I did with devm) in the future? I might even
consider renaming __clk_find to clk_find or to clk_find_unsafe - but
that's all just nitpicking :) Go with what you like to maintain :D

Best regards
    Matti Vaittinen

-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Re: [PATCH v3 1/7] clkdev: Hold clocks_mutex while iterating clocks list

[Stephen Boyd]

Quoting Matti Vaittinen (2019-04-08 03:49:41)
> On Fri, Apr 05, 2019 at 01:37:24PM -0700, Stephen Boyd wrote:
> > Quoting Vaittinen, Matti (2019-04-04 23:51:43)
> > > On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote:
> > > > We recently introduced a change to support devm clk lookups. That
> > > > change
> > > > introduced a code-path that used clk_find() without holding the
> > > > 'clocks_mutex'. Unfortunately, clk_find() iterates over the 'clocks'
> > > > list and so we need to prevent the list from being modified while
> > > > iterating over it by holding the mutex. Similarly, we don't need to
> > > > hold
> > > > the 'clocks_mutex' besides when we're dereferencing the clk_lookup
> > > > pointer
> > > 
> > > /// Snip
> > > 
> > > > -out:
> > > > +static struct clk_lookup *clk_find(const char *dev_id, const char
> > > > *con_id)
> > > > +{
> > > > +     struct clk_lookup *cl;
> > > > +
> > > > +     mutex_lock(&clocks_mutex);
> > > > +     cl = __clk_find(dev_id, con_id);
> > > >       mutex_unlock(&clocks_mutex);
> > > >  
> > > > -     return cl ? clk : ERR_PTR(-ENOENT);
> > > > +     return cl;
> > > > +}
> > > 
> > > I am not an expert on this but reading commit message abowe and seeing
> > > the code for clk_find() looks a bit scary. If I understand it
> > > correctly, the clocks_mutex should be held when dereferencing the
> > > clk_lookup returned by clk_find. The clk_find implementation drops the
> > > lock before returning - which makes me think I miss something here. How
> > > can the caller ever safely dereference returned clk_lookup pointer?
> > > Just reading abowe makes me think that lock should be taken by whoever
> > > is calling the clk_find, and dropped only after caller has used the
> > > found clk_lookup for whatever caller intends to use it. Maybe I am
> > > missing something?
> > > 
> > 
> > The only user after this patch (devm) is doing a pointer comparison so
> > it looks OK. But yes, in general there shouldn't be users of clk_find()
> > that dereference the pointer because there isn't any protection besides
> > the mutex.
> 
> If the only (intended) user for clk_find is devm_clk_release_clkdev,
> then we might want to write it in devm_clk_release_clkdev - just to
> avoid similar errors (as I did with devm) in the future? I might even
> consider renaming __clk_find to clk_find or to clk_find_unsafe - but
> that's all just nitpicking :) Go with what you like to maintain :D
> 

Sure. I was thinking along the same lines after you asked.

Re: [PATCH v3 3/7] clk: Add of_clk_hw_register() API for early clk drivers

[Jeffrey Hugo]

On 4/4/2019 3:53 PM, Stephen Boyd wrote:
> In some circumstances drivers register clks early and don't have access
> to a struct device because the device model isn't initialized yet. Add
> an API to let drivers register clks associated with a struct device_node
> so that these drivers can participate in getting parent clks through DT.

NACK.  This patch broke boot for me.  I had to pull the below from JTAG. 
  What do you need to debug this?

[    0.000000] Unable to handle kernel read from unreadable memory at 
virtual address 0000000000000280
[    0.000000] Mem abort info:
[    0.000000]   ESR = 0x96000004
[    0.000000]   Exception class = DABT (current EL), IL = 32 bits
[    0.000000]   SET = 0, FnV = 0
[    0.000000]   EA = 0, S1PTW = 0
[    0.000000] Data abort info:
[    0.000000]   ISV = 0, ISS = 0x00000004
[    0.000000]   CM = 0, WnR = 0
[    0.000000] [0000000000000280] user address but active_mm is swapper
[    0.000000] Internal error: Oops: 96000004 [#1] SMP
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.1.0-rc1+ #453
[    0.000000] Hardware name: Qualcomm Technologies, Inc. MSM8998 v1 MTP 
(DT)
[    0.000000] pstate: 60400085 (nZCv daIf +PAN -UAO)
[    0.000000] pc : clk_hw_register+0x24/0x40
[    0.000000] lr : clk_hw_register_fixed_rate_with_accuracy+0xac/0x100
[    0.000000] sp : ffff0000117d3df0
[    0.000000] x29: ffff0000117d3df0 x28: ffff8000fdc7cd28
[    0.000000] x27: dead000000000100 x26: dead000000000200
[    0.000000] x25: 0000000000000001 x24: ffff7dfffe80047c
[    0.000000] x23: 000000000124f800 x22: 0000000000000000
[    0.000000] x21: 0000000000000000 x20: ffff8000f9524c00
[    0.000000] x19: 0000000000000000 x18: ffffffffffffffff
[    0.000000] x17: 0000000000000000 x16: 0000000000000000
[    0.000000] x15: ffff0000117dc708 x14: ffffffffffffffff
[    0.000000] x13: 0000000000000020 x12: 0101010101010101
[    0.000000] x11: 7f7f7f7f7f7f7f7f x10: 02fefeff01fefeff
[    0.000000] x9 : 0000000000000000 x8 : ffff8000f9524c80
[    0.000000] x7 : 0000000000000000 x6 : 000000000000003f
[    0.000000] x5 : 0000000000000040 x4 : 0000000000000000
[    0.000000] x3 : 0000000000000000 x2 : ffff8000f9524c00
[    0.000000] x1 : ffff8000f9524c00 x0 : 0000000000000000
[    0.000000] Process swapper/0 (pid: 0, stack limit = 0x(____ptrval____))
[    0.000000] Call trace:
[    0.000000]  clk_hw_register+0x24/0x40
[    0.000000]  clk_hw_register_fixed_rate_with_accuracy+0xac/0x100
[    0.000000]  _of_fixed_clk_setup+0xa8/0x120
[    0.000000]  of_fixed_clk_setup+0x20/0x2c
[    0.000000]  of_clk_init+0x19c/0x254
[    0.000000]  time_init+0x18/0x4c
[    0.000000]  start_kernel+0x394/0x524
[    0.000000] Code: aa1e03e0 d503201f aa1403e2 aa1303e0 (f9414261)
[    0.000000] ---[ end trace 5d02af5e3cbab7bf ]---
[    0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
[    0.000000] ---[ end Kernel panic - not syncing: Attempted to kill 
the idle task! ]---

> 
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
>   drivers/clk/clk.c            | 26 +++++++++++++++++++++++---
>   include/linux/clk-provider.h |  1 +
>   2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index d27775a73e67..4971e9651ca5 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -45,6 +45,7 @@ struct clk_core {
>   	struct clk_hw		*hw;
>   	struct module		*owner;
>   	struct device		*dev;
> +	struct device_node	*of_node;
>   	struct clk_core		*parent;
>   	const char		**parent_names;
>   	struct clk_core		**parents;
> @@ -3313,7 +3314,8 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
>   	return clk;
>   }
>   
> -static struct clk *__clk_register(struct device *dev, struct clk_hw *hw)
> +static struct clk *
> +__clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
>   {
>   	int i, ret;
>   	struct clk_core *core;
> @@ -3339,6 +3341,7 @@ static struct clk *__clk_register(struct device *dev, struct clk_hw *hw)
>   	if (dev && pm_runtime_enabled(dev))
>   		core->rpm_enabled = true;
>   	core->dev = dev;
> +	core->of_node = np;
>   	if (dev && dev->driver)
>   		core->owner = dev->driver->owner;
>   	core->hw = hw;
> @@ -3429,7 +3432,7 @@ static struct clk *__clk_register(struct device *dev, struct clk_hw *hw)
>    */
>   struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>   {
> -	return __clk_register(dev, hw);
> +	return __clk_register(dev, dev->of_node, hw);
>   }
>   EXPORT_SYMBOL_GPL(clk_register);
>   
> @@ -3445,10 +3448,27 @@ EXPORT_SYMBOL_GPL(clk_register);
>    */
>   int clk_hw_register(struct device *dev, struct clk_hw *hw)
>   {
> -	return PTR_ERR_OR_ZERO(__clk_register(dev, hw));
> +	return PTR_ERR_OR_ZERO(__clk_register(dev, dev->of_node, hw));
>   }
>   EXPORT_SYMBOL_GPL(clk_hw_register);
>   
> +/*
> + * of_clk_hw_register - register a clk_hw and return an error code
> + * @node: device_node of device that is registering this clock
> + * @hw: link to hardware-specific clock data
> + *
> + * of_clk_hw_register() is the primary interface for populating the clock tree
> + * with new clock nodes when a struct device is not available, but a struct
> + * device_node is. It returns an integer equal to zero indicating success or
> + * less than zero indicating failure. Drivers must test for an error code after
> + * calling of_clk_hw_register().
> + */
> +int of_clk_hw_register(struct device_node *node, struct clk_hw *hw)
> +{
> +	return PTR_ERR_OR_ZERO(__clk_register(NULL, node, hw));
> +}
> +EXPORT_SYMBOL_GPL(of_clk_hw_register);
> +
>   /* Free memory allocated for a clock. */
>   static void __clk_release(struct kref *ref)
>   {
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index b7cf80a71293..7d2d97e15b76 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -773,6 +773,7 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw);
>   
>   int __must_check clk_hw_register(struct device *dev, struct clk_hw *hw);
>   int __must_check devm_clk_hw_register(struct device *dev, struct clk_hw *hw);
> +int __must_check of_clk_hw_register(struct device_node *node, struct clk_hw *hw);
>   
>   void clk_unregister(struct clk *clk);
>   void devm_clk_unregister(struct device *dev, struct clk *clk);
> 


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 1/7] clkdev: Hold clocks_mutex while iterating clocks list

[Russell King - ARM Linux admin]

On Mon, Apr 08, 2019 at 10:00:02AM -0700, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2019-04-08 03:49:41)
> > On Fri, Apr 05, 2019 at 01:37:24PM -0700, Stephen Boyd wrote:
> > > Quoting Vaittinen, Matti (2019-04-04 23:51:43)
> > > > On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote:
> > > > > We recently introduced a change to support devm clk lookups. That
> > > > > change
> > > > > introduced a code-path that used clk_find() without holding the
> > > > > 'clocks_mutex'. Unfortunately, clk_find() iterates over the 'clocks'
> > > > > list and so we need to prevent the list from being modified while
> > > > > iterating over it by holding the mutex. Similarly, we don't need to
> > > > > hold
> > > > > the 'clocks_mutex' besides when we're dereferencing the clk_lookup
> > > > > pointer
> > > > 
> > > > /// Snip
> > > > 
> > > > > -out:
> > > > > +static struct clk_lookup *clk_find(const char *dev_id, const char
> > > > > *con_id)
> > > > > +{
> > > > > +     struct clk_lookup *cl;
> > > > > +
> > > > > +     mutex_lock(&clocks_mutex);
> > > > > +     cl = __clk_find(dev_id, con_id);
> > > > >       mutex_unlock(&clocks_mutex);
> > > > >  
> > > > > -     return cl ? clk : ERR_PTR(-ENOENT);
> > > > > +     return cl;
> > > > > +}
> > > > 
> > > > I am not an expert on this but reading commit message abowe and seeing
> > > > the code for clk_find() looks a bit scary. If I understand it
> > > > correctly, the clocks_mutex should be held when dereferencing the
> > > > clk_lookup returned by clk_find. The clk_find implementation drops the
> > > > lock before returning - which makes me think I miss something here. How
> > > > can the caller ever safely dereference returned clk_lookup pointer?
> > > > Just reading abowe makes me think that lock should be taken by whoever
> > > > is calling the clk_find, and dropped only after caller has used the
> > > > found clk_lookup for whatever caller intends to use it. Maybe I am
> > > > missing something?
> > > > 
> > > 
> > > The only user after this patch (devm) is doing a pointer comparison so
> > > it looks OK. But yes, in general there shouldn't be users of clk_find()
> > > that dereference the pointer because there isn't any protection besides
> > > the mutex.
> > 
> > If the only (intended) user for clk_find is devm_clk_release_clkdev,
> > then we might want to write it in devm_clk_release_clkdev - just to
> > avoid similar errors (as I did with devm) in the future? I might even
> > consider renaming __clk_find to clk_find or to clk_find_unsafe - but
> > that's all just nitpicking :) Go with what you like to maintain :D
> > 
> 
> Sure. I was thinking along the same lines after you asked.

This is rubbish.  The reason clk_find() doesn't take the lock is that
you _need_ to hold the lock while you dereference the clk_lookup data.
The lock isn't protecting just the lookup, it protects what you do with
the result of the lookup as well.

So, as I say, adding locking inside clk_find() is completely
misunderstanding the locking here.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v3 1/7] clkdev: Hold clocks_mutex while iterating clocks list

[Vaittinen, Matti]

On Mon, 2019-04-08 at 23:21 +0100, Russell King - ARM Linux admin
wrote:
> On Mon, Apr 08, 2019 at 10:00:02AM -0700, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2019-04-08 03:49:41)
> > > On Fri, Apr 05, 2019 at 01:37:24PM -0700, Stephen Boyd wrote:
> > > > Quoting Vaittinen, Matti (2019-04-04 23:51:43)
> > > > > On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote:
> > > > > > We recently introduced a change to support devm clk
> > > > > > lookups. That
> > > > > > change
> > > > > > introduced a code-path that used clk_find() without holding
> > > > > > the
> > > > > > 'clocks_mutex'. Unfortunately, clk_find() iterates over the
> > > > > > 'clocks'
> > > > > > list and so we need to prevent the list from being modified
> > > > > > while
> > > > > > iterating over it by holding the mutex. Similarly, we don't
> > > > > > need to
> > > > > > hold
> > > > > > the 'clocks_mutex' besides when we're dereferencing the
> > > > > > clk_lookup
> > > > > > pointer
> > > > > 
> > > > > /// Snip
> > > > > 
> > > > > > -out:
> > > > > > +static struct clk_lookup *clk_find(const char *dev_id,
> > > > > > const char
> > > > > > *con_id)
> > > > > > +{
> > > > > > +     struct clk_lookup *cl;
> > > > > > +
> > > > > > +     mutex_lock(&clocks_mutex);
> > > > > > +     cl = __clk_find(dev_id, con_id);
> > > > > >       mutex_unlock(&clocks_mutex);
> > > > > >  
> > > > > > -     return cl ? clk : ERR_PTR(-ENOENT);
> > > > > > +     return cl;
> > > > > > +}
> > > > > 
> > > > > I am not an expert on this but reading commit message abowe
> > > > > and seeing
> > > > > the code for clk_find() looks a bit scary. If I understand it
> > > > > correctly, the clocks_mutex should be held when dereferencing
> > > > > the
> > > > > clk_lookup returned by clk_find. The clk_find implementation
> > > > > drops the
> > > > > lock before returning - which makes me think I miss something
> > > > > here. How
> > > > > can the caller ever safely dereference returned clk_lookup
> > > > > pointer?
> > > > > Just reading abowe makes me think that lock should be taken
> > > > > by whoever
> > > > > is calling the clk_find, and dropped only after caller has
> > > > > used the
> > > > > found clk_lookup for whatever caller intends to use it. Maybe
> > > > > I am
> > > > > missing something?
> > > > > 
> > > > 
> > > > The only user after this patch (devm) is doing a pointer
> > > > comparison so
> > > > it looks OK. But yes, in general there shouldn't be users of
> > > > clk_find()
> > > > that dereference the pointer because there isn't any protection
> > > > besides
> > > > the mutex.
> > > 
> > > If the only (intended) user for clk_find is
> > > devm_clk_release_clkdev,
> > > then we might want to write it in devm_clk_release_clkdev - just
> > > to
> > > avoid similar errors (as I did with devm) in the future? I might
> > > even
> > > consider renaming __clk_find to clk_find or to clk_find_unsafe -
> > > but
> > > that's all just nitpicking :) Go with what you like to maintain
> > > :D
> > > 
> > 
> > Sure. I was thinking along the same lines after you asked.
> 
> This is rubbish.  The reason clk_find() doesn't take the lock is that
> you _need_ to hold the lock while you dereference the clk_lookup
> data.

I think we all agreed on this already. Stephen pointed out that the
current user(s) of clk_find() do _not_ dereference the pointer.

> The lock isn't protecting just the lookup, it protects what you do
> with
> the result of the lookup as well.

And we agreed on this too.

Br,
	Matti Vaittinen

Re: [PATCH v3 1/7] clkdev: Hold clocks_mutex while iterating clocks list

[Russell King - ARM Linux admin]

On Tue, Apr 09, 2019 at 05:31:48AM +0000, Vaittinen, Matti wrote:
> On Mon, 2019-04-08 at 23:21 +0100, Russell King - ARM Linux admin
> wrote:
> > On Mon, Apr 08, 2019 at 10:00:02AM -0700, Stephen Boyd wrote:
> > > Quoting Matti Vaittinen (2019-04-08 03:49:41)
> > > > On Fri, Apr 05, 2019 at 01:37:24PM -0700, Stephen Boyd wrote:
> > > > > Quoting Vaittinen, Matti (2019-04-04 23:51:43)
> > > > > > On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote:
> > > > > > > We recently introduced a change to support devm clk
> > > > > > > lookups. That
> > > > > > > change
> > > > > > > introduced a code-path that used clk_find() without holding
> > > > > > > the
> > > > > > > 'clocks_mutex'. Unfortunately, clk_find() iterates over the
> > > > > > > 'clocks'
> > > > > > > list and so we need to prevent the list from being modified
> > > > > > > while
> > > > > > > iterating over it by holding the mutex. Similarly, we don't
> > > > > > > need to
> > > > > > > hold
> > > > > > > the 'clocks_mutex' besides when we're dereferencing the
> > > > > > > clk_lookup
> > > > > > > pointer
> > > > > > 
> > > > > > /// Snip
> > > > > > 
> > > > > > > -out:
> > > > > > > +static struct clk_lookup *clk_find(const char *dev_id,
> > > > > > > const char
> > > > > > > *con_id)
> > > > > > > +{
> > > > > > > +     struct clk_lookup *cl;
> > > > > > > +
> > > > > > > +     mutex_lock(&clocks_mutex);
> > > > > > > +     cl = __clk_find(dev_id, con_id);
> > > > > > >       mutex_unlock(&clocks_mutex);
> > > > > > >  
> > > > > > > -     return cl ? clk : ERR_PTR(-ENOENT);
> > > > > > > +     return cl;
> > > > > > > +}
> > > > > > 
> > > > > > I am not an expert on this but reading commit message abowe
> > > > > > and seeing
> > > > > > the code for clk_find() looks a bit scary. If I understand it
> > > > > > correctly, the clocks_mutex should be held when dereferencing
> > > > > > the
> > > > > > clk_lookup returned by clk_find. The clk_find implementation
> > > > > > drops the
> > > > > > lock before returning - which makes me think I miss something
> > > > > > here. How
> > > > > > can the caller ever safely dereference returned clk_lookup
> > > > > > pointer?
> > > > > > Just reading abowe makes me think that lock should be taken
> > > > > > by whoever
> > > > > > is calling the clk_find, and dropped only after caller has
> > > > > > used the
> > > > > > found clk_lookup for whatever caller intends to use it. Maybe
> > > > > > I am
> > > > > > missing something?
> > > > > > 
> > > > > 
> > > > > The only user after this patch (devm) is doing a pointer
> > > > > comparison so
> > > > > it looks OK. But yes, in general there shouldn't be users of
> > > > > clk_find()
> > > > > that dereference the pointer because there isn't any protection
> > > > > besides
> > > > > the mutex.
> > > > 
> > > > If the only (intended) user for clk_find is
> > > > devm_clk_release_clkdev,
> > > > then we might want to write it in devm_clk_release_clkdev - just
> > > > to
> > > > avoid similar errors (as I did with devm) in the future? I might
> > > > even
> > > > consider renaming __clk_find to clk_find or to clk_find_unsafe -
> > > > but
> > > > that's all just nitpicking :) Go with what you like to maintain
> > > > :D
> > > > 
> > > 
> > > Sure. I was thinking along the same lines after you asked.
> > 
> > This is rubbish.  The reason clk_find() doesn't take the lock is that
> > you _need_ to hold the lock while you dereference the clk_lookup
> > data.
> 
> I think we all agreed on this already. Stephen pointed out that the
> current user(s) of clk_find() do _not_ dereference the pointer.

Which is actually another incorrect statement - clk_get_sys()
dereferences it, but Stephen's patch does rearrange the code there.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v3 1/7] clkdev: Hold clocks_mutex while iterating clocks list

[Stephen Boyd]

Quoting Russell King - ARM Linux admin (2019-04-09 01:57:17)
> On Tue, Apr 09, 2019 at 05:31:48AM +0000, Vaittinen, Matti wrote:
> > On Mon, 2019-04-08 at 23:21 +0100, Russell King - ARM Linux admin
> > wrote:
> > > On Mon, Apr 08, 2019 at 10:00:02AM -0700, Stephen Boyd wrote:
> > > > > 
> > > > 
> > > > Sure. I was thinking along the same lines after you asked.
> > > 
> > > This is rubbish.  The reason clk_find() doesn't take the lock is that
> > > you _need_ to hold the lock while you dereference the clk_lookup
> > > data.
> > 
> > I think we all agreed on this already. Stephen pointed out that the
> > current user(s) of clk_find() do _not_ dereference the pointer.
> 
> Which is actually another incorrect statement - clk_get_sys()
> dereferences it, but Stephen's patch does rearrange the code there.
> 

I've split this patch into two. One to fix the locking of the new
clk_find() user and the other to introduce a clk_find_hw() API that lets
me use it later on in this series. I'll resend the series with this
update.

Re: [PATCH v3 3/7] clk: Add of_clk_hw_register() API for early clk drivers

[Stephen Boyd]

Quoting Jeffrey Hugo (2019-04-08 14:46:11)
> On 4/4/2019 3:53 PM, Stephen Boyd wrote:
> > In some circumstances drivers register clks early and don't have access
> > to a struct device because the device model isn't initialized yet. Add
> > an API to let drivers register clks associated with a struct device_node
> > so that these drivers can participate in getting parent clks through DT.
> 
> NACK.  This patch broke boot for me.  I had to pull the below from JTAG. 

No early printk support on this board? Sadness.

>   What do you need to debug this?
> 

Hopefully nothing.

> [    0.000000] Unable to handle kernel read from unreadable memory at 
> virtual address 0000000000000280

I guess you pass NULL as dev to clk_hw_register(). Will fix! Thanks for
testing.

> [    0.000000] Mem abort info:

Re: [PATCH v3 3/7] clk: Add of_clk_hw_register() API for early clk drivers

[Stephen Boyd]

Quoting Jeffrey Hugo (2019-04-08 14:46:11)
> On 4/4/2019 3:53 PM, Stephen Boyd wrote:
> > In some circumstances drivers register clks early and don't have access
> > to a struct device because the device model isn't initialized yet. Add
> > an API to let drivers register clks associated with a struct device_node
> > so that these drivers can participate in getting parent clks through DT.
> 
> NACK.  This patch broke boot for me.  I had to pull the below from JTAG. 
>   What do you need to debug this?
> 

Here's a patch to try to squash in:

---8<----
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 709492d901a1..040ce083c89e 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3662,7 +3662,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
  */
 struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 {
-	return __clk_register(dev, dev->of_node, hw);
+	return __clk_register(dev, dev_of_node(dev), hw);
 }
 EXPORT_SYMBOL_GPL(clk_register);
 
@@ -3678,7 +3678,7 @@ EXPORT_SYMBOL_GPL(clk_register);
  */
 int clk_hw_register(struct device *dev, struct clk_hw *hw)
 {
-	return PTR_ERR_OR_ZERO(__clk_register(dev, dev->of_node, hw));
+	return PTR_ERR_OR_ZERO(__clk_register(dev, dev_of_node(dev), hw));
 }
 EXPORT_SYMBOL_GPL(clk_hw_register);
 

Re: [PATCH v3 3/7] clk: Add of_clk_hw_register() API for early clk drivers

[Jeffrey Hugo]

On 4/10/2019 10:53 AM, Stephen Boyd wrote:
> Quoting Jeffrey Hugo (2019-04-08 14:46:11)
>> On 4/4/2019 3:53 PM, Stephen Boyd wrote:
>>> In some circumstances drivers register clks early and don't have access
>>> to a struct device because the device model isn't initialized yet. Add
>>> an API to let drivers register clks associated with a struct device_node
>>> so that these drivers can participate in getting parent clks through DT.
>>
>> NACK.  This patch broke boot for me.  I had to pull the below from JTAG.
>>    What do you need to debug this?
>>
> 
> Here's a patch to try to squash in:

No dice.  Same issue.

> 
> ---8<----
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 709492d901a1..040ce083c89e 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3662,7 +3662,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
>    */
>   struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>   {
> -	return __clk_register(dev, dev->of_node, hw);
> +	return __clk_register(dev, dev_of_node(dev), hw);
>   }
>   EXPORT_SYMBOL_GPL(clk_register);
>   
> @@ -3678,7 +3678,7 @@ EXPORT_SYMBOL_GPL(clk_register);
>    */
>   int clk_hw_register(struct device *dev, struct clk_hw *hw)
>   {
> -	return PTR_ERR_OR_ZERO(__clk_register(dev, dev->of_node, hw));
> +	return PTR_ERR_OR_ZERO(__clk_register(dev, dev_of_node(dev), hw));
>   }
>   EXPORT_SYMBOL_GPL(clk_hw_register);
>   
> 


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 3/7] clk: Add of_clk_hw_register() API for early clk drivers

[Stephen Boyd]

Quoting Jeffrey Hugo (2019-04-10 12:39:16)
> On 4/10/2019 10:53 AM, Stephen Boyd wrote:
> > Quoting Jeffrey Hugo (2019-04-08 14:46:11)
> >> On 4/4/2019 3:53 PM, Stephen Boyd wrote:
> >>> In some circumstances drivers register clks early and don't have access
> >>> to a struct device because the device model isn't initialized yet. Add
> >>> an API to let drivers register clks associated with a struct device_node
> >>> so that these drivers can participate in getting parent clks through DT.
> >>
> >> NACK.  This patch broke boot for me.  I had to pull the below from JTAG.
> >>    What do you need to debug this?
> >>
> > 
> > Here's a patch to try to squash in:
> 
> No dice.  Same issue.

Argh! dev_of_node() doesn't check for NULL device. :-/ I want to be
extremely lazy! Let's get this merged too. Thanks for the testing.

---8<---
diff --git a/include/linux/device.h b/include/linux/device.h
index b425a7ee04ce..0370dd0b3ae7 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1231,7 +1231,7 @@ static inline void device_lock_assert(struct device *dev)
 
 static inline struct device_node *dev_of_node(struct device *dev)
 {
-	if (!IS_ENABLED(CONFIG_OF))
+	if (!IS_ENABLED(CONFIG_OF) || !dev)
 		return NULL;
 	return dev->of_node;
 }

Re: [PATCH v3 3/7] clk: Add of_clk_hw_register() API for early clk drivers

[Jeffrey Hugo]

On 4/10/2019 3:45 PM, Stephen Boyd wrote:
> Quoting Jeffrey Hugo (2019-04-10 12:39:16)
>> On 4/10/2019 10:53 AM, Stephen Boyd wrote:
>>> Quoting Jeffrey Hugo (2019-04-08 14:46:11)
>>>> On 4/4/2019 3:53 PM, Stephen Boyd wrote:
>>>>> In some circumstances drivers register clks early and don't have access
>>>>> to a struct device because the device model isn't initialized yet. Add
>>>>> an API to let drivers register clks associated with a struct device_node
>>>>> so that these drivers can participate in getting parent clks through DT.
>>>>
>>>> NACK.  This patch broke boot for me.  I had to pull the below from JTAG.
>>>>     What do you need to debug this?
>>>>
>>>
>>> Here's a patch to try to squash in:
>>
>> No dice.  Same issue.
> 
> Argh! dev_of_node() doesn't check for NULL device. :-/ I want to be
> extremely lazy! Let's get this merged too. Thanks for the testing.

Fixed.  Will continue to test the rest of the series and let you know.

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 3/7] clk: Add of_clk_hw_register() API for early clk drivers

[Jeffrey Hugo]

On 4/10/2019 4:18 PM, Jeffrey Hugo wrote:
> On 4/10/2019 3:45 PM, Stephen Boyd wrote:
>> Quoting Jeffrey Hugo (2019-04-10 12:39:16)
>>> On 4/10/2019 10:53 AM, Stephen Boyd wrote:
>>>> Quoting Jeffrey Hugo (2019-04-08 14:46:11)
>>>>> On 4/4/2019 3:53 PM, Stephen Boyd wrote:
>>>>>> In some circumstances drivers register clks early and don't have 
>>>>>> access
>>>>>> to a struct device because the device model isn't initialized yet. 
>>>>>> Add
>>>>>> an API to let drivers register clks associated with a struct 
>>>>>> device_node
>>>>>> so that these drivers can participate in getting parent clks 
>>>>>> through DT.
>>>>>
>>>>> NACK.  This patch broke boot for me.  I had to pull the below from 
>>>>> JTAG.
>>>>>     What do you need to debug this?
>>>>>
>>>>
>>>> Here's a patch to try to squash in:
>>>
>>> No dice.  Same issue.
>>
>> Argh! dev_of_node() doesn't check for NULL device. :-/ I want to be
>> extremely lazy! Let's get this merged too. Thanks for the testing.
> 
> Fixed.  Will continue to test the rest of the series and let you know.
> 

With the two changes we discussed, for the entire series:
Tested-by: Jeffrey Hugo <jhugo@codeaurora.org>

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.