[PATCH v6 0/5] clk: clock deregistration support

[PATCH v6 0/5] clk: clock deregistration support

[Sylwester Nawrocki]

This patch series implements clock deregistration in the common clock
framework. Comparing to v5 it only includes further corrections of NULL
clock handling.

Changes since v5:
 - fixed NULL clock handling in __clk_get(), __clk_put (patch 5/5).

Changes since v4:
 - removed stray struct module forward declaration in patch 3/5.

Changes since v3:
 - replaced WARN_ON() with WARN_ON_ONCE() in clk_nodrv_disable_unprepare()
   callback.

Changes since v2:
 - reordered the patches so the race condition is fixed before it can
   actually cause any issues,
 - fixed handling of NULL clock pointers in __clk_get(), __clk_put(),
 - added patch adding actual asignment of clk->owner; more details are
   discussed in that specific patch.

Changes since v1:
 - moved of_clk_{lock, unlock}, __of_clk_get_from_provider() function
   declaractions to a local header,
 - renamed clk_dummy_* to clk_nodrv_*.

Sylwester Nawrocki (5):
  clk: Provide not locked variant of of_clk_get_from_provider()
  clkdev: Fix race condition in clock lookup from device tree
  clk: Add common __clk_get(), __clk_put() implementations
  clk: Assign module owner of a clock being registered
  clk: Implement clk_unregister

 arch/arm/include/asm/clkdev.h      |    2 +
 arch/blackfin/include/asm/clkdev.h |    2 +
 arch/mips/include/asm/clkdev.h     |    2 +
 arch/sh/include/asm/clkdev.h       |    2 +
 drivers/clk/clk.c                  |  188 +++++++++++++++++++++++++++++++++---
 drivers/clk/clk.h                  |   16 +++
 drivers/clk/clkdev.c               |   12 ++-
 include/linux/clk-private.h        |    5 +
 include/linux/clkdev.h             |    5 +
 9 files changed, 221 insertions(+), 13 deletions(-)
 create mode 100644 drivers/clk/clk.h

--
1.7.9.5

[PATCH v6 1/5] clk: Provide not locked variant of of_clk_get_from_provider()

[Sylwester Nawrocki]

Add helper functions for the of_clk_providers list locking and
an unlocked variant of of_clk_get_from_provider().
These functions are intended to be used in the clkdev to avoid
race condition in the device tree based clock look up in clk_get().

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
Changes since v3:
 - none.

Changes since v2:
 - fixed typo in clk.h.

Changes since v1:
 - moved the function declaractions to a local header.
---
 drivers/clk/clk.c |   38 ++++++++++++++++++++++++++++++--------
 drivers/clk/clk.h |   16 ++++++++++++++++
 2 files changed, 46 insertions(+), 8 deletions(-)
 create mode 100644 drivers/clk/clk.h

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2db08c0..8f18564 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -21,6 +21,8 @@
 #include <linux/init.h>
 #include <linux/sched.h>
 
+#include "clk.h"
+
 static DEFINE_SPINLOCK(enable_lock);
 static DEFINE_MUTEX(prepare_lock);
 
@@ -2103,7 +2105,18 @@ static const struct of_device_id __clk_of_table_sentinel
 	__used __section(__clk_of_table_end);
 
 static LIST_HEAD(of_clk_providers);
-static DEFINE_MUTEX(of_clk_lock);
+static DEFINE_MUTEX(of_clk_mutex);
+
+/* of_clk_provider list locking helpers */
+void of_clk_lock(void)
+{
+	mutex_lock(&of_clk_mutex);
+}
+
+void of_clk_unlock(void)
+{
+	mutex_unlock(&of_clk_mutex);
+}
 
 struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec,
 				     void *data)
@@ -2147,9 +2160,9 @@ int of_clk_add_provider(struct device_node *np,
 	cp->data = data;
 	cp->get = clk_src_get;
 
-	mutex_lock(&of_clk_lock);
+	mutex_lock(&of_clk_mutex);
 	list_add(&cp->link, &of_clk_providers);
-	mutex_unlock(&of_clk_lock);
+	mutex_unlock(&of_clk_mutex);
 	pr_debug("Added clock from %s\n", np->full_name);
 
 	return 0;
@@ -2164,7 +2177,7 @@ void of_clk_del_provider(struct device_node *np)
 {
 	struct of_clk_provider *cp;
 
-	mutex_lock(&of_clk_lock);
+	mutex_lock(&of_clk_mutex);
 	list_for_each_entry(cp, &of_clk_providers, link) {
 		if (cp->node == np) {
 			list_del(&cp->link);
@@ -2173,24 +2186,33 @@ void of_clk_del_provider(struct device_node *np)
 			break;
 		}
 	}
-	mutex_unlock(&of_clk_lock);
+	mutex_unlock(&of_clk_mutex);
 }
 EXPORT_SYMBOL_GPL(of_clk_del_provider);
 
-struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
+struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec)
 {
 	struct of_clk_provider *provider;
 	struct clk *clk = ERR_PTR(-ENOENT);
 
 	/* Check if we have such a provider in our array */
-	mutex_lock(&of_clk_lock);
 	list_for_each_entry(provider, &of_clk_providers, link) {
 		if (provider->node == clkspec->np)
 			clk = provider->get(clkspec, provider->data);
 		if (!IS_ERR(clk))
 			break;
 	}
-	mutex_unlock(&of_clk_lock);
+
+	return clk;
+}
+
+struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
+{
+	struct clk *clk;
+
+	mutex_lock(&of_clk_mutex);
+	clk = __of_clk_get_from_provider(clkspec);
+	mutex_unlock(&of_clk_mutex);
 
 	return clk;
 }
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
new file mode 100644
index 0000000..795cc9f
--- /dev/null
+++ b/drivers/clk/clk.h
@@ -0,0 +1,16 @@
+/*
+ * linux/drivers/clk/clk.h
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Sylwester Nawrocki <s.nawrocki@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
+struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
+void of_clk_lock(void);
+void of_clk_unlock(void);
+#endif
-- 
1.7.9.5

[PATCH v6 2/5] clkdev: Fix race condition in clock lookup from device tree

[Sylwester Nawrocki]

There is currently a race condition in the device tree part of clk_get()
function, since the pointer returned from of_clk_get_by_name() may become
invalid before __clk_get() call. E.g. due to the clock provider driver
remove() callback being called in between of_clk_get_by_name() and
__clk_get().

Fix this by doing both the look up and __clk_get() operations with the
clock providers list mutex held. This ensures that the clock pointer
returned from __of_clk_get_from_provider() call and passed to __clk_get()
is valid, as long as the clock supplier module first removes its clock
provider instance and then does clk_unregister() on the corresponding
clocks.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
Changes since v2:
 - none.

Changes since v1:
 - include "clk.h".
---
 drivers/clk/clkdev.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 442a313..48f6721 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -21,6 +21,8 @@
 #include <linux/clkdev.h>
 #include <linux/of.h>
 
+#include "clk.h"
+
 static LIST_HEAD(clocks);
 static DEFINE_MUTEX(clocks_mutex);
 
@@ -39,7 +41,13 @@ struct clk *of_clk_get(struct device_node *np, int index)
 	if (rc)
 		return ERR_PTR(rc);
 
-	clk = of_clk_get_from_provider(&clkspec);
+	of_clk_lock();
+	clk = __of_clk_get_from_provider(&clkspec);
+
+	if (!IS_ERR(clk) && !__clk_get(clk))
+		clk = ERR_PTR(-ENOENT);
+
+	of_clk_unlock();
 	of_node_put(clkspec.np);
 	return clk;
 }
@@ -157,7 +165,7 @@ struct clk *clk_get(struct device *dev, const char *con_id)
 
 	if (dev) {
 		clk = of_clk_get_by_name(dev->of_node, con_id);
-		if (!IS_ERR(clk) && __clk_get(clk))
+		if (!IS_ERR(clk))
 			return clk;
 	}
 
-- 
1.7.9.5

[PATCH v6 3/5] clk: Add common __clk_get(), __clk_put() implementations

[Sylwester Nawrocki]

This patch adds common __clk_get(), __clk_put() clkdev helpers which
replace their platform specific counterparts when the common clock
API is enabled.

The owner module pointer field is added to struct clk so a reference
to the clock supplier module can be taken by the clock consumers.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v5:
 - none.

Changes since v4:
 - dropped unnecessary struct module forward declaration from
   clk-provider.h

Changes since v3:
 - dropped exporting of __clk_get(), __clk_put().

Changes since v2:
 - fixed handling of NULL clock pointers in __clk_get(), __clk_put();
---
 arch/arm/include/asm/clkdev.h      |    2 ++
 arch/blackfin/include/asm/clkdev.h |    2 ++
 arch/mips/include/asm/clkdev.h     |    2 ++
 arch/sh/include/asm/clkdev.h       |    2 ++
 drivers/clk/clk.c                  |   20 ++++++++++++++++++++
 include/linux/clk-private.h        |    3 +++
 include/linux/clkdev.h             |    5 +++++
 7 files changed, 36 insertions(+)

diff --git a/arch/arm/include/asm/clkdev.h b/arch/arm/include/asm/clkdev.h
index 80751c1..4e8a4b2 100644
--- a/arch/arm/include/asm/clkdev.h
+++ b/arch/arm/include/asm/clkdev.h
@@ -14,12 +14,14 @@
 
 #include <linux/slab.h>
 
+#ifndef CONFIG_COMMON_CLK
 #ifdef CONFIG_HAVE_MACH_CLKDEV
 #include <mach/clkdev.h>
 #else
 #define __clk_get(clk)	({ 1; })
 #define __clk_put(clk)	do { } while (0)
 #endif
+#endif
 
 static inline struct clk_lookup_alloc *__clkdev_alloc(size_t size)
 {
diff --git a/arch/blackfin/include/asm/clkdev.h b/arch/blackfin/include/asm/clkdev.h
index 9053bed..7ac2436 100644
--- a/arch/blackfin/include/asm/clkdev.h
+++ b/arch/blackfin/include/asm/clkdev.h
@@ -8,7 +8,9 @@ static inline struct clk_lookup_alloc *__clkdev_alloc(size_t size)
 	return kzalloc(size, GFP_KERNEL);
 }
 
+#ifndef CONFIG_COMMON_CLK
 #define __clk_put(clk)
 #define __clk_get(clk) ({ 1; })
+#endif
 
 #endif
diff --git a/arch/mips/include/asm/clkdev.h b/arch/mips/include/asm/clkdev.h
index 2624754..1b3ad7b 100644
--- a/arch/mips/include/asm/clkdev.h
+++ b/arch/mips/include/asm/clkdev.h
@@ -14,8 +14,10 @@
 
 #include <linux/slab.h>
 
+#ifndef CONFIG_COMMON_CLK
 #define __clk_get(clk)	({ 1; })
 #define __clk_put(clk)	do { } while (0)
+#endif
 
 static inline struct clk_lookup_alloc *__clkdev_alloc(size_t size)
 {
diff --git a/arch/sh/include/asm/clkdev.h b/arch/sh/include/asm/clkdev.h
index 6ba9186..c419014 100644
--- a/arch/sh/include/asm/clkdev.h
+++ b/arch/sh/include/asm/clkdev.h
@@ -25,7 +25,9 @@ static inline struct clk_lookup_alloc *__clkdev_alloc(size_t size)
 		return kzalloc(size, GFP_KERNEL);
 }
 
+#ifndef CONFIG_COMMON_CLK
 #define __clk_put(clk)
 #define __clk_get(clk) ({ 1; })
+#endif
 
 #endif /* __CLKDEV_H__ */
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8f18564..dcf061a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1965,6 +1965,26 @@ void devm_clk_unregister(struct device *dev, struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(devm_clk_unregister);
 
+/*
+ * clkdev helpers
+ */
+int __clk_get(struct clk *clk)
+{
+	if (clk && !try_module_get(clk->owner))
+		return 0;
+
+	return 1;
+}
+
+void __clk_put(struct clk *clk)
+{
+	if (WARN_ON_ONCE(IS_ERR(clk)))
+		return;
+
+	if (clk)
+		module_put(clk->owner);
+}
+
 /***        clk rate change notifiers        ***/
 
 /**
diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
index 8138c94..8cb1865 100644
--- a/include/linux/clk-private.h
+++ b/include/linux/clk-private.h
@@ -25,10 +25,13 @@
 
 #ifdef CONFIG_COMMON_CLK
 
+struct module;
+
 struct clk {
 	const char		*name;
 	const struct clk_ops	*ops;
 	struct clk_hw		*hw;
+	struct module		*owner;
 	struct clk		*parent;
 	const char		**parent_names;
 	struct clk		**parents;
diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h
index a6a6f60..94bad77 100644
--- a/include/linux/clkdev.h
+++ b/include/linux/clkdev.h
@@ -43,4 +43,9 @@ int clk_add_alias(const char *, const char *, char *, struct device *);
 int clk_register_clkdev(struct clk *, const char *, const char *, ...);
 int clk_register_clkdevs(struct clk *, struct clk_lookup *, size_t);
 
+#ifdef CONFIG_COMMON_CLK
+int __clk_get(struct clk *clk);
+void __clk_put(struct clk *clk);
+#endif
+
 #endif
-- 
1.7.9.5

[PATCH v6 4/5] clk: Assign module owner of a clock being registered

[Sylwester Nawrocki]

Use dev->driver->owner as the owner module of a clock, it ensures
reference on the module is taken in the __clk_get(), __clk_put()
helpers.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v5:
 - none.
---
 drivers/clk/clk.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index dcf061a..ac80e13 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1805,6 +1805,10 @@ struct clk *__clk_register(struct device *dev, struct clk_hw *hw)
 	clk->flags = hw->init->flags;
 	clk->parent_names = hw->init->parent_names;
 	clk->num_parents = hw->init->num_parents;
+	if (dev && dev->driver)
+		clk->owner = dev->driver->owner;
+	else
+		clk->owner = NULL;
 
 	ret = __clk_init(dev, clk);
 	if (ret)
@@ -1825,6 +1829,8 @@ static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk)
 		goto fail_name;
 	}
 	clk->ops = hw->init->ops;
+	if (dev && dev->driver)
+		clk->owner = dev->driver->owner;
 	clk->hw = hw;
 	clk->flags = hw->init->flags;
 	clk->num_parents = hw->init->num_parents;
-- 
1.7.9.5

[PATCH v6 5/5] clk: Implement clk_unregister

[Sylwester Nawrocki]

clk_unregister() is currently not implemented and it is required when
a clock provider module needs to be unloaded.

Normally the clock supplier module is prevented to be unloaded by
taking reference on the module in clk_get().

For cases when the clock supplier module deinitializes despite the
consumers of its clocks holding a reference on the module, e.g. when
the driver is unbound through "unbind" sysfs attribute, there are
empty clock ops added. These ops are assigned temporarily to struct
clk and used until all consumers release the clock, to avoid invoking
callbacks from the module which just got removed.

Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v5:
- ensure clk->kref is not referenced when the passed clk is NULL.

Changes since v4:
 - none.

Changes since v3:
 - Use WARN_ON_ONCE() rather than WARN_ON() in clk_nodrv_disable_unprepare()
   callback.

Changes since v2:
 - none.

Changes since RFC v1:
 - renamed clk_dummy_* to clk_nodrv_*.

Changes since v3 of the original patch [1]:
 - reparent all children to the orphan list instead of leaving
   the clock unregistered when it has child clocks,
 - removed unnecessary prerequisite checks in clk_debug_unregister(),
 - struct clk is now being freed only when the last clock consumer
   calls clk_put(),
 - empty clock ops are used after clk_unregister() has been called
   until all references to the clock are released and the clock
   object is freed.

[1] http://www.spinics.net/lists/arm-kernel/msg247548.html
---
 drivers/clk/clk.c           |  134 ++++++++++++++++++++++++++++++++++++++++---
 include/linux/clk-private.h |    2 +
 2 files changed, 128 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ac80e13..27f8c42 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -344,6 +344,21 @@ out:
 	return ret;
 }
 
+ /**
+ * clk_debug_unregister - remove a clk node from the debugfs clk tree
+ * @clk: the clk being removed from the debugfs clk tree
+ *
+ * Dynamically removes a clk and all it's children clk nodes from the
+ * debugfs clk tree if clk->dentry points to debugfs created by
+ * clk_debug_register in __clk_init.
+ *
+ * Caller must hold prepare_lock.
+ */
+static void clk_debug_unregister(struct clk *clk)
+{
+	debugfs_remove_recursive(clk->dentry);
+}
+
 /**
  * clk_debug_reparent - reparent clk node in the debugfs clk tree
  * @clk: the clk being reparented
@@ -434,6 +449,9 @@ static inline int clk_debug_register(struct clk *clk) { return 0; }
 static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
 {
 }
+static inline void clk_debug_unregister(struct clk *clk)
+{
+}
 #endif
 
 /* caller must hold prepare_lock */
@@ -1770,6 +1788,7 @@ int __clk_init(struct device *dev, struct clk *clk)
 
 	clk_debug_register(clk);
 
+	kref_init(&clk->ref);
 out:
 	clk_prepare_unlock();
 
@@ -1905,13 +1924,106 @@ fail_out:
 }
 EXPORT_SYMBOL_GPL(clk_register);
 
+/*
+ * Free memory allocated for a clock.
+ * Caller must hold prepare_lock.
+ */
+static void __clk_release(struct kref *ref)
+{
+	struct clk *clk = container_of(ref, struct clk, ref);
+	int i = clk->num_parents;
+
+	kfree(clk->parents);
+	while (--i >= 0)
+		kfree(clk->parent_names[i]);
+
+	kfree(clk->parent_names);
+	kfree(clk->name);
+	kfree(clk);
+}
+
+/*
+ * Empty clk_ops for unregistered clocks. These are used temporarily
+ * after clk_unregister() was called on a clock and until last clock
+ * consumer calls clk_put() and the struct clk object is freed.
+ */
+static int clk_nodrv_prepare_enable(struct clk_hw *hw)
+{
+	return -ENXIO;
+}
+
+static void clk_nodrv_disable_unprepare(struct clk_hw *hw)
+{
+	WARN_ON(1);
+}
+
+static int clk_nodrv_set_rate(struct clk_hw *hw, unsigned long rate,
+					unsigned long parent_rate)
+{
+	return -ENXIO;
+}
+
+static int clk_nodrv_set_parent(struct clk_hw *hw, u8 index)
+{
+	return -ENXIO;
+}
+
+static const struct clk_ops clk_nodrv_ops = {
+	.enable		= clk_nodrv_prepare_enable,
+	.disable	= clk_nodrv_disable_unprepare,
+	.prepare	= clk_nodrv_prepare_enable,
+	.unprepare	= clk_nodrv_disable_unprepare,
+	.set_rate	= clk_nodrv_set_rate,
+	.set_parent	= clk_nodrv_set_parent,
+};
+
 /**
  * clk_unregister - unregister a currently registered clock
  * @clk: clock to unregister
- *
- * Currently unimplemented.
  */
-void clk_unregister(struct clk *clk) {}
+void clk_unregister(struct clk *clk)
+{
+	unsigned long flags;
+
+	clk_prepare_lock();
+
+	if (!clk || IS_ERR(clk)) {
+		pr_err("%s: invalid clock: %p\n", __func__, clk);
+		goto out;
+	}
+
+	if (clk->ops == &clk_nodrv_ops) {
+		pr_err("%s: unregistered clock: %s\n", __func__, clk->name);
+		goto out;
+	}
+	/*
+	 * Assign empty clock ops for consumers that might still hold
+	 * a reference to this clock.
+	 */
+	flags = clk_enable_lock();
+	clk->ops = &clk_nodrv_ops;
+	clk_enable_unlock(flags);
+
+	if (!hlist_empty(&clk->children)) {
+		struct clk *child;
+
+		/* Reparent all children to the orphan list. */
+		hlist_for_each_entry(child, &clk->children, child_node)
+			clk_set_parent(child, NULL);
+	}
+
+	clk_debug_unregister(clk);
+
+	hlist_del_init(&clk->child_node);
+
+	if (clk->prepare_count)
+		pr_warn("%s: unregistering prepared clock: %s\n",
+					__func__, clk->name);
+
+	kref_put(&clk->ref, __clk_release);
+out:
+	clk_prepare_unlock();
+}
 EXPORT_SYMBOL_GPL(clk_unregister);
 
 static void devm_clk_release(struct device *dev, void *res)
@@ -1976,19 +2088,25 @@ EXPORT_SYMBOL_GPL(devm_clk_unregister);
  */
 int __clk_get(struct clk *clk)
 {
-	if (clk && !try_module_get(clk->owner))
-		return 0;
+	if (clk) {
+		if (!try_module_get(clk->owner))
+			return 0;
 
+		kref_get(&clk->ref);
+	}
 	return 1;
 }
 
 void __clk_put(struct clk *clk)
 {
-	if (WARN_ON_ONCE(IS_ERR(clk)))
+	if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
 		return;
 
-	if (clk)
-		module_put(clk->owner);
+	clk_prepare_lock();
+	kref_put(&clk->ref, __clk_release);
+	clk_prepare_unlock();
+
+	module_put(clk->owner);
 }
 
 /***        clk rate change notifiers        ***/
diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
index 8cb1865..72c65e0 100644
--- a/include/linux/clk-private.h
+++ b/include/linux/clk-private.h
@@ -12,6 +12,7 @@
 #define __LINUX_CLK_PRIVATE_H
 
 #include <linux/clk-provider.h>
+#include <linux/kref.h>
 #include <linux/list.h>
 
 /*
@@ -50,6 +51,7 @@ struct clk {
 #ifdef CONFIG_COMMON_CLK_DEBUG
 	struct dentry		*dentry;
 #endif
+	struct kref		ref;
 };
 
 /*
-- 
1.7.9.5

[PATCH v6 5/5] clk: Implement clk_unregister

[Sylwester Nawrocki]

On 08/30/2013 04:53 PM, Sylwester Nawrocki wrote:
> clk_unregister() is currently not implemented and it is required when
> a clock provider module needs to be unloaded.
> 
> Normally the clock supplier module is prevented to be unloaded by
> taking reference on the module in clk_get().
> 
> For cases when the clock supplier module deinitializes despite the
> consumers of its clocks holding a reference on the module, e.g. when
> the driver is unbound through "unbind" sysfs attribute, there are
> empty clock ops added. These ops are assigned temporarily to struct
> clk and used until all consumers release the clock, to avoid invoking
> callbacks from the module which just got removed.
> 
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
[...]
>  /**
>   * clk_unregister - unregister a currently registered clock
>   * @clk: clock to unregister
> - *
> - * Currently unimplemented.
>   */
> -void clk_unregister(struct clk *clk) {}
> +void clk_unregister(struct clk *clk)
> +{
> +	unsigned long flags;
> +
> +	clk_prepare_lock();
> +
> +	if (!clk || IS_ERR(clk)) {
> +		pr_err("%s: invalid clock: %p\n", __func__, clk);
> +		goto out;
> +	}

Actually this check could be done before taking the mutex. And to handle
NULL clocks properly it should be something like:

       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
               return;

I will hold on with posting a corrected version until there are any
further comments.

> +	if (clk->ops == &clk_nodrv_ops) {
> +		pr_err("%s: unregistered clock: %s\n", __func__, clk->name);
> +		goto out;
> +	}
> +	/*
> +	 * Assign empty clock ops for consumers that might still hold
> +	 * a reference to this clock.
> +	 */
> +	flags = clk_enable_lock();
> +	clk->ops = &clk_nodrv_ops;
> +	clk_enable_unlock(flags);
> +
> +	if (!hlist_empty(&clk->children)) {
> +		struct clk *child;
> +
> +		/* Reparent all children to the orphan list. */
> +		hlist_for_each_entry(child, &clk->children, child_node)
> +			clk_set_parent(child, NULL);
> +	}
> +
> +	clk_debug_unregister(clk);
> +
> +	hlist_del_init(&clk->child_node);
> +
> +	if (clk->prepare_count)
> +		pr_warn("%s: unregistering prepared clock: %s\n",
> +					__func__, clk->name);
> +
> +	kref_put(&clk->ref, __clk_release);
> +out:
> +	clk_prepare_unlock();
> +}
>  EXPORT_SYMBOL_GPL(clk_unregister);

[PATCH v6 0/5] clk: clock deregistration support

[Sylwester Nawrocki]

On 08/30/2013 04:53 PM, Sylwester Nawrocki wrote:
> This patch series implements clock deregistration in the common clock
> framework. Comparing to v5 it only includes further corrections of NULL
> clock handling.
[...]
>    clk: Provide not locked variant of of_clk_get_from_provider()
>    clkdev: Fix race condition in clock lookup from device tree
>    clk: Add common __clk_get(), __clk_put() implementations
>    clk: Assign module owner of a clock being registered
>    clk: Implement clk_unregister

Hi Mike, Russell,

Would you have any further comments/suggestions on this series ?

I have inspected all callers of clk_register() and all should be fine
with regards to dereferencing dev->driver. The first argument to this
function is either NULL or clk_register() is being called in drivers'
probe() callback, which ensures dev->driver won't change due to holding
dev->mutex.

The only issue I found might be at the omap3isp driver, which provides
clock to its sub-drivers and takes reference on the sub-driver modules.
When sub-driver calls clk_get() all modules would get locked in memory,
due to circular reference. One solution to that could be to pass NULL
struct device pointer, as in the below patch.

---------8<------------------
 From ca5963041aad67e31324cb5d4d5e2cfce1706d4f Mon Sep 17 00:00:00 2001
From: Sylwester Nawrocki <s.nawrocki@samsung.com>
Date: Thu, 19 Sep 2013 23:52:04 +0200
Subject: [PATCH] omap3isp: Pass NULL device pointer to clk_register()

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
  drivers/media/platform/omap3isp/isp.c |   15 ++++++++++-----
  drivers/media/platform/omap3isp/isp.h |    1 +
  2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c 
b/drivers/media/platform/omap3isp/isp.c
index df3a0ec..d7f3c98 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -290,9 +290,11 @@ static int isp_xclk_init(struct isp_device *isp)
  	struct clk_init_data init;
  	unsigned int i;

+	for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i)
+		isp->xclks[i] = ERR_PTR(-EINVAL);
+
  	for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i) {
  		struct isp_xclk *xclk = &isp->xclks[i];
-		struct clk *clk;

  		xclk->isp = isp;
  		xclk->id = i == 0 ? ISP_XCLK_A : ISP_XCLK_B;
@@ -306,9 +308,9 @@ static int isp_xclk_init(struct isp_device *isp)

  		xclk->hw.init = &init;

-		clk = devm_clk_register(isp->dev, &xclk->hw);
-		if (IS_ERR(clk))
-			return PTR_ERR(clk);
+		xclk->clk = clk_register(NULL, &xclk->hw);
+		if (IS_ERR(xclk->clk))
+			return PTR_ERR(xclk->clk);

  		if (pdata->xclks[i].con_id == NULL &&
  		    pdata->xclks[i].dev_id == NULL)
@@ -320,7 +322,7 @@ static int isp_xclk_init(struct isp_device *isp)

  		xclk->lookup->con_id = pdata->xclks[i].con_id;
  		xclk->lookup->dev_id = pdata->xclks[i].dev_id;
-		xclk->lookup->clk = clk;
+		xclk->lookup->clk = xclk->clk;

  		clkdev_add(xclk->lookup);
  	}
@@ -335,6 +337,9 @@ static void isp_xclk_cleanup(struct isp_device *isp)
  	for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i) {
  		struct isp_xclk *xclk = &isp->xclks[i];

+		if (!IS_ERR(xclk->clk))
+			clk_unregister(xclk->clk);
+
  		if (xclk->lookup)
  			clkdev_drop(xclk->lookup);
  	}
diff --git a/drivers/media/platform/omap3isp/isp.h 
b/drivers/media/platform/omap3isp/isp.h
index cd3eff4..1498f2b 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -135,6 +135,7 @@ struct isp_xclk {
  	struct isp_device *isp;
  	struct clk_hw hw;
  	struct clk_lookup *lookup;
+	struct clk *clk;
  	enum isp_xclk_id id;

  	spinlock_t lock;	/* Protects enabled and divider */
-- 
---------8<------------------


Alternatively an additional argument could be added to the clk_register*()
functions. Something like:

---------8<------------------
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 27f8c42..ef934ac 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1837,7 +1837,8 @@ struct clk *__clk_register(struct device *dev, 
struct clk_hw *hw)
  }
  EXPORT_SYMBOL_GPL(__clk_register);

-static int _clk_register(struct device *dev, struct clk_hw *hw, struct 
clk *clk)
+static int _clk_register(struct device *dev, struct clk_hw *hw, struct 
clk *clk,
+							struct module *owner)
  {
  	int i, ret;

@@ -1877,6 +1878,8 @@ static int _clk_register(struct device *dev, 
struct clk_hw *hw, struct clk *clk)
  		}
  	}

+	clk->owner = owner;
+
  	ret = __clk_init(dev, clk);
  	if (!ret)
  		return 0;
@@ -1892,7 +1895,7 @@ fail_name:
  }

  /**
- * clk_register - allocate a new clock, register it and return an 
opaque cookie
+ * ___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
   *
@@ -1902,7 +1905,8 @@ fail_name:
   * 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)
+struct clk *___clk_register(struct device *dev, struct clk_hw *hw,
+						struct module *owner)
  {
  	int ret;
  	struct clk *clk;
@@ -1914,7 +1918,7 @@ struct clk *clk_register(struct device *dev, 
struct clk_hw *hw)
  		goto fail_out;
  	}

-	ret = _clk_register(dev, hw, clk);
+	ret = _clk_register(dev, hw, clk, owner);
  	if (!ret)
  		return clk;

@@ -1922,7 +1926,7 @@ struct clk *clk_register(struct device *dev, 
struct clk_hw *hw)
  fail_out:
  	return ERR_PTR(ret);
  }
-EXPORT_SYMBOL_GPL(clk_register);
+EXPORT_SYMBOL_GPL(___clk_register);

  /*
   * Free memory allocated for a clock.
@@ -2040,7 +2044,8 @@ static void devm_clk_release(struct device *dev, 
void *res)
   * automatically clk_unregister()ed on driver detach. See 
clk_register() for
   * more information.
   */
-struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw)
+struct clk *__devm_clk_register(struct device *dev, struct clk_hw *hw,
+						struct module *owner)
  {
  	struct clk *clk;
  	int ret;
@@ -2049,7 +2054,7 @@ struct clk *devm_clk_register(struct device *dev, 
struct clk_hw *hw)
  	if (!clk)
  		return ERR_PTR(-ENOMEM);

-	ret = _clk_register(dev, hw, clk);
+	ret = _clk_register(dev, hw, clk, owner);
  	if (!ret) {
  		devres_add(dev, clk);
  	} else {
@@ -2059,7 +2064,7 @@ struct clk *devm_clk_register(struct device *dev, 
struct clk_hw *hw)

  	return clk;
  }
-EXPORT_SYMBOL_GPL(devm_clk_register);
+EXPORT_SYMBOL_GPL(__devm_clk_register);

  static int devm_clk_match(struct device *dev, void *res, void *data)
  {
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 13623f3..c656ebf 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -31,6 +31,7 @@
  #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate 
change */

  struct clk_hw;
+struct module;

  /**
   * struct clk_ops -  Callback operations for hardware clocks; these are to
@@ -436,8 +437,21 @@ struct clk *clk_register_composite(struct device 
*dev, const char *name,
   * 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);
-struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw);
+struct clk *___clk_register(struct device *dev, struct clk_hw *hw,
+		struct module *owner);
+
+static inline struct clk *clk_register(struct device *dev, struct 
clk_hw *hw)
+{
+	return ___clk_register(dev, hw, THIS_MODULE);
+}
+
+struct clk *__devm_clk_register(struct device *dev, struct clk_hw *hw,
+				struct module *owner);
+static inline struct clk *devm_clk_register(struct device *dev,
+					    struct clk_hw *hw)
+{
+	return __devm_clk_register(dev, hw, THIS_MODULE);
+}

  void clk_unregister(struct clk *clk);
  void devm_clk_unregister(struct device *dev, struct clk *clk);

---------8<------------------

Similarly it would need to be done for the remaining clk_register*() 
functions,
which have much longer arguments list.

It's a bit messy, since there is already __clk_register() function with 
double
underscore prefix. Perhaps that could be renamed to something else so 
all the
functions taking struct module * parameter are prefixed with double 
underscore.


I would squash patches 3/5 and 4/5 in the next iteration, if it is decided
to keep using dev->driver->owner.

--
Thanks,
Sylwester

[PATCH v6 0/5] clk: clock deregistration support

[Laurent Pinchart]

Hi Sylwester,

On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote:
> On 08/30/2013 04:53 PM, Sylwester Nawrocki wrote:
> > This patch series implements clock deregistration in the common clock
> > framework. Comparing to v5 it only includes further corrections of NULL
> > clock handling.
> 
> [...]
> 
> >    clk: Provide not locked variant of of_clk_get_from_provider()
> >    clkdev: Fix race condition in clock lookup from device tree
> >    clk: Add common __clk_get(), __clk_put() implementations
> >    clk: Assign module owner of a clock being registered
> >    clk: Implement clk_unregister
> 
> Hi Mike, Russell,
> 
> Would you have any further comments/suggestions on this series ?
> 
> I have inspected all callers of clk_register() and all should be fine
> with regards to dereferencing dev->driver. The first argument to this
> function is either NULL or clk_register() is being called in drivers'
> probe() callback, which ensures dev->driver won't change due to holding
> dev->mutex.
> 
> The only issue I found might be at the omap3isp driver, which provides
> clock to its sub-drivers and takes reference on the sub-driver modules.
> When sub-driver calls clk_get() all modules would get locked in memory,
> due to circular reference. One solution to that could be to pass NULL
> struct device pointer, as in the below patch.

Doesn't that introduce race conditions ? If the sub-drivers require the clock, 
they want to be sure that the clock won't disappear beyond their backs. I 
agree that the circular dependency needs to be solved somehow, but we probably 
need a more generic solution. The problem will become more widespread in the 
future with DT-based device instantiation in both V4L2 and KMS.

> ---------8<------------------
>  From ca5963041aad67e31324cb5d4d5e2cfce1706d4f Mon Sep 17 00:00:00 2001
> From: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Date: Thu, 19 Sep 2013 23:52:04 +0200
> Subject: [PATCH] omap3isp: Pass NULL device pointer to clk_register()
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>   drivers/media/platform/omap3isp/isp.c |   15 ++++++++++-----
>   drivers/media/platform/omap3isp/isp.h |    1 +
>   2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c
> index df3a0ec..d7f3c98 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -290,9 +290,11 @@ static int isp_xclk_init(struct isp_device *isp)
>   	struct clk_init_data init;
>   	unsigned int i;
> 
> +	for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i)
> +		isp->xclks[i] = ERR_PTR(-EINVAL);
> +
>   	for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i) {
>   		struct isp_xclk *xclk = &isp->xclks[i];
> -		struct clk *clk;
> 
>   		xclk->isp = isp;
>   		xclk->id = i == 0 ? ISP_XCLK_A : ISP_XCLK_B;
> @@ -306,9 +308,9 @@ static int isp_xclk_init(struct isp_device *isp)
> 
>   		xclk->hw.init = &init;
> 
> -		clk = devm_clk_register(isp->dev, &xclk->hw);
> -		if (IS_ERR(clk))
> -			return PTR_ERR(clk);
> +		xclk->clk = clk_register(NULL, &xclk->hw);
> +		if (IS_ERR(xclk->clk))
> +			return PTR_ERR(xclk->clk);
> 
>   		if (pdata->xclks[i].con_id == NULL &&
>   		    pdata->xclks[i].dev_id == NULL)
> @@ -320,7 +322,7 @@ static int isp_xclk_init(struct isp_device *isp)
> 
>   		xclk->lookup->con_id = pdata->xclks[i].con_id;
>   		xclk->lookup->dev_id = pdata->xclks[i].dev_id;
> -		xclk->lookup->clk = clk;
> +		xclk->lookup->clk = xclk->clk;
> 
>   		clkdev_add(xclk->lookup);
>   	}
> @@ -335,6 +337,9 @@ static void isp_xclk_cleanup(struct isp_device *isp)
>   	for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i) {
>   		struct isp_xclk *xclk = &isp->xclks[i];
> 
> +		if (!IS_ERR(xclk->clk))
> +			clk_unregister(xclk->clk);
> +
>   		if (xclk->lookup)
>   			clkdev_drop(xclk->lookup);
>   	}
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h
> index cd3eff4..1498f2b 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -135,6 +135,7 @@ struct isp_xclk {
>   	struct isp_device *isp;
>   	struct clk_hw hw;
>   	struct clk_lookup *lookup;
> +	struct clk *clk;
>   	enum isp_xclk_id id;
> 
>   	spinlock_t lock;	/* Protects enabled and divider */
> ---------8<------------------
> 
> Alternatively an additional argument could be added to the clk_register*()
> functions. Something like:
> 
> ---------8<------------------
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 27f8c42..ef934ac 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1837,7 +1837,8 @@ struct clk *__clk_register(struct device *dev,
> struct clk_hw *hw)
> 
>   }
>   EXPORT_SYMBOL_GPL(__clk_register);
> 
> -static int _clk_register(struct device *dev, struct clk_hw *hw, struct
> clk *clk)
> +static int _clk_register(struct device *dev, struct clk_hw *hw, struct
> clk *clk,
> +                         struct module *owner)
>   {
>         int i, ret;
> 
> @@ -1877,6 +1878,8 @@ static int _clk_register(struct device *dev,
> struct clk_hw *hw, struct clk *clk)
>                 }
>         }
> 
> +       clk->owner = owner;
> +
>         ret = __clk_init(dev, clk);
>         if (!ret)
>                 return 0;
> @@ -1892,7 +1895,7 @@ fail_name:
>   }
>   
>   /**
> - * clk_register - allocate a new clock, register it and return an
> opaque cookie
> + * ___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
>    *
> @@ -1902,7 +1905,8 @@ fail_name:
>    * 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)
> +struct clk *___clk_register(struct device *dev, struct clk_hw *hw,
> +                                               struct module *owner)
>   {
>         int ret;
>         struct clk *clk;
> @@ -1914,7 +1918,7 @@ struct clk *clk_register(struct device *dev,
> struct clk_hw *hw)
>                 goto fail_out;
>         }
> -       ret = _clk_register(dev, hw, clk);
> +       ret = _clk_register(dev, hw, clk, owner);
>         if (!ret)
>                 return clk;
> 
> @@ -1922,7 +1926,7 @@ struct clk *clk_register(struct device *dev,
> struct clk_hw *hw)
>   fail_out:
>         return ERR_PTR(ret);
>   }
> -EXPORT_SYMBOL_GPL(clk_register);
> +EXPORT_SYMBOL_GPL(___clk_register);
> 
>   /*
>    * Free memory allocated for a clock.
> @@ -2040,7 +2044,8 @@ static void devm_clk_release(struct device *dev,
> void *res)
>    * automatically clk_unregister()ed on driver detach. See
>    * clk_register() for more information.
>    */
> -struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw)
> +struct clk *__devm_clk_register(struct device *dev, struct clk_hw *hw,
> +                                               struct module *owner)
>   {
>         struct clk *clk;
>         int ret;
> @@ -2049,7 +2054,7 @@ struct clk *devm_clk_register(struct device *dev,
> struct clk_hw *hw)
>         if (!clk)
>                 return ERR_PTR(-ENOMEM);
> 
> -       ret = _clk_register(dev, hw, clk);
> +       ret = _clk_register(dev, hw, clk, owner);
>         if (!ret) {
>                 devres_add(dev, clk);
>         } else {
> @@ -2059,7 +2064,7 @@ struct clk *devm_clk_register(struct device *dev,
> struct clk_hw *hw)
>         return clk;
>   }
> -EXPORT_SYMBOL_GPL(devm_clk_register);
> +EXPORT_SYMBOL_GPL(__devm_clk_register);
> 
>   static int devm_clk_match(struct device *dev, void *res, void *data)
>   {
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 13623f3..c656ebf 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -31,6 +31,7 @@
>   #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate
> change */
> 
>  struct clk_hw;
> +struct module;
> 
>   /**
>    * struct clk_ops -  Callback operations for hardware clocks; these are to
> @@ -436,8 +437,21 @@ struct clk *clk_register_composite(struct device
> *dev, const char *name,
>    * 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);
> -struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw);
> +struct clk *___clk_register(struct device *dev, struct clk_hw *hw,
> +               struct module *owner);
> +
> +static inline struct clk *clk_register(struct device *dev, struct
> clk_hw *hw)
> +{
> +       return ___clk_register(dev, hw, THIS_MODULE);
> +}
> +
> +struct clk *__devm_clk_register(struct device *dev, struct clk_hw *hw,
> +                               struct module *owner);
> +static inline struct clk *devm_clk_register(struct device *dev,
> +                                           struct clk_hw *hw)
> +{
> +       return __devm_clk_register(dev, hw, THIS_MODULE);
> +}
> 
>   void clk_unregister(struct clk *clk);
>   void devm_clk_unregister(struct device *dev, struct clk *clk);
> ---------8<------------------
> 
> Similarly it would need to be done for the remaining clk_register*()
> functions, which have much longer arguments list.
> 
> It's a bit messy, since there is already __clk_register() function with
> double underscore prefix. Perhaps that could be renamed to something else so
> all the functions taking struct module * parameter are prefixed with double
> underscore.
> 
> I would squash patches 3/5 and 4/5 in the next iteration, if it is decided
> to keep using dev->driver->owner.
-- 
Regards,

Laurent Pinchart

[PATCH v6 0/5] clk: clock deregistration support

[Sylwester Nawrocki]

Hi Laurent,

On 09/25/2013 11:47 AM, Laurent Pinchart wrote:
> Doesn't that introduce race conditions ? If the sub-drivers require the clock,
> they want to be sure that the clock won't disappear beyond their backs. I
> agree that the circular dependency needs to be solved somehow, but we probably
> need a more generic solution. The problem will become more widespread in the
> future with DT-based device instantiation in both V4L2 and KMS.

It doesn't introduce any new race conditions AFAICT. I doubt all these 
issues
can be resolved in one single step. Currently the modular clock 
providers are
seriously broken, there is no reference tracking and the clock consumers 
can
easily get into a state where they have invalid references to clocks 
supplied
by already unregistered drivers.

With this patch series the clock consumer drivers will not crash thanks 
to the
clock object reference counting. So the worst thing may happen is a 
clock left
in an unexpected state.

However there should be no problems with the v4l2-async API, the host driver
in its de-initialization routine unregisters its sub-drivers (they 
should stop
using the clock when notified of such an event), only then the host would
unregister the clock (subsequently the sub-drivers get re-attached and put
into deferred probing state).

There may be issues when a sub-driver's file handle is opened while the host
is about to de-initialize. But I doubt resolution of such problems belongs
to the common clock framework. I have been trying to improve the situation
in small steps, rather than waiting forever for a perfect solution.

Do you perhaps have any ideas WRT to a "more generic solution" ? In general
I have been trying to avoid using v4l2-clk and add what's missing in the
common clock framework.

Perhaps we should leave only the kref part in the __clk_get(), __clk_put()
hooks and be taking reference to a clock in clk_prepare() and releasing it
in clk_unprepare() ? This way circular reference would exist only between
clk_prepare(), clk_unprepare() calls.

Assuming a driver prepares clock in device's open() and unprepares in device
close() handler perhaps it could all work better, without relying on the 
host
to ensure the resource reference tracking. I'm not sure if that is not 
making
too many assumptions for a generic API.


Thanks for the feedback.

--
Regards,
Sylwester

[PATCH v6 0/5] clk: clock deregistration support

[Laurent Pinchart]

Hi Sylwester,

On Wednesday 25 September 2013 22:51:08 Sylwester Nawrocki wrote:
> On 09/25/2013 11:47 AM, Laurent Pinchart wrote:
> > Doesn't that introduce race conditions ? If the sub-drivers require the
> > clock, they want to be sure that the clock won't disappear beyond their
> > backs. I agree that the circular dependency needs to be solved somehow,
> > but we probably need a more generic solution. The problem will become
> > more widespread in the future with DT-based device instantiation in both
> > V4L2 and KMS.
> 
> It doesn't introduce any new race conditions AFAICT. I doubt all these
> issues can be resolved in one single step. Currently the modular clock
> providers are seriously broken, there is no reference tracking and the clock
> consumers can easily get into a state where they have invalid references to
> clocks supplied by already unregistered drivers.
> 
> With this patch series the clock consumer drivers will not crash thanks
> to the clock object reference counting. So the worst thing may happen is a
> clock left in an unexpected state.
> 
> However there should be no problems with the v4l2-async API, the host driver
> in its de-initialization routine unregisters its sub-drivers (they should
> stop using the clock when notified of such an event), only then the host
> would unregister the clock (subsequently the sub-drivers get re-attached and
> put into deferred probing state).

That in itself is a workaround I believe. Unbinding/rebinding devices from/to 
drivers isn't something the v4l core should do.

> There may be issues when a sub-driver's file handle is opened while the host
> is about to de-initialize. But I doubt resolution of such problems belongs
> to the common clock framework. I have been trying to improve the situation
> in small steps, rather than waiting forever for a perfect solution.
> 
> Do you perhaps have any ideas WRT to a "more generic solution" ? In general
> I have been trying to avoid using v4l2-clk and add what's missing in the
> common clock framework.
> 
> Perhaps we should leave only the kref part in the __clk_get(), __clk_put()
> hooks and be taking reference to a clock in clk_prepare() and releasing it
> in clk_unprepare() ? This way circular reference would exist only between
> clk_prepare(), clk_unprepare() calls.
> 
> Assuming a driver prepares clock in device's open() and unprepares in device
> close() handler perhaps it could all work better, without relying on the
> host to ensure the resource reference tracking. I'm not sure if that is not
> making too many assumptions for a generic API.

This is indeed an architecture decision that goes beyond the boundaries of the 
clock framework. The question boils down to how we want to acquire/release and 
refcount resources. Should drivers acquire and release hotpluggable resources 
at probe and remove time respectively, or only when they need them ? Or should 
they acquire them at probe them and be notified when they should release them 
? The first option adds an overhead but could help solving the circular 
dependency problem in a simpler way.

-- 
Regards,

Laurent Pinchart

[PATCH v6 0/5] clk: clock deregistration support

[Sylwester Nawrocki]

Hi,

(adding linux-media mailing list at Cc)

On 09/25/2013 11:47 AM, Laurent Pinchart wrote:
> On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote:
[...]
>> The only issue I found might be at the omap3isp driver, which provides
>> clock to its sub-drivers and takes reference on the sub-driver modules.
>> When sub-driver calls clk_get() all modules would get locked in memory,
>> due to circular reference. One solution to that could be to pass NULL
>> struct device pointer, as in the below patch.
>
> Doesn't that introduce race conditions ? If the sub-drivers require the clock,
> they want to be sure that the clock won't disappear beyond their backs. I
> agree that the circular dependency needs to be solved somehow, but we probably
> need a more generic solution. The problem will become more widespread in the
> future with DT-based device instantiation in both V4L2 and KMS.

I'm wondering whether subsystems and drivers itself should be written so
they deal with such dependencies they are aware of.

There is similar situation in the regulator API, regulator_get() simply
takes a reference on a module providing the regulator object.

Before a "more generic solution" is available, what do you think about
keeping obtaining a reference on a clock provider module in clk_get() and
doing clk_get(), clk_prepare_enable(), ..., clk_unprepare_disable(),
clk_put() in sub-driver whenever a clock is actively used, to avoid
permanent circular reference ?

--
Thanks,
Sylwester

>> ---------8<------------------
>>   From ca5963041aad67e31324cb5d4d5e2cfce1706d4f Mon Sep 17 00:00:00 2001
>> From: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Date: Thu, 19 Sep 2013 23:52:04 +0200
>> Subject: [PATCH] omap3isp: Pass NULL device pointer to clk_register()
>>
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> ---
>>    drivers/media/platform/omap3isp/isp.c |   15 ++++++++++-----
>>    drivers/media/platform/omap3isp/isp.h |    1 +
>>    2 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c
>> b/drivers/media/platform/omap3isp/isp.c
>> index df3a0ec..d7f3c98 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -290,9 +290,11 @@ static int isp_xclk_init(struct isp_device *isp)
>>    	struct clk_init_data init;
>>    	unsigned int i;
>>
>> +	for (i = 0; i<  ARRAY_SIZE(isp->xclks); ++i)
>> +		isp->xclks[i] = ERR_PTR(-EINVAL);
>> +
>>    	for (i = 0; i<  ARRAY_SIZE(isp->xclks); ++i) {
>>    		struct isp_xclk *xclk =&isp->xclks[i];
>> -		struct clk *clk;
>>
>>    		xclk->isp = isp;
>>    		xclk->id = i == 0 ? ISP_XCLK_A : ISP_XCLK_B;
>> @@ -306,9 +308,9 @@ static int isp_xclk_init(struct isp_device *isp)
>>
>>    		xclk->hw.init =&init;
>>
>> -		clk = devm_clk_register(isp->dev,&xclk->hw);
>> -		if (IS_ERR(clk))
>> -			return PTR_ERR(clk);
>> +		xclk->clk = clk_register(NULL,&xclk->hw);
>> +		if (IS_ERR(xclk->clk))
>> +			return PTR_ERR(xclk->clk);
>>
>>    		if (pdata->xclks[i].con_id == NULL&&
>>    		pdata->xclks[i].dev_id == NULL)
>> @@ -320,7 +322,7 @@ static int isp_xclk_init(struct isp_device *isp)
>>
>>    		xclk->lookup->con_id = pdata->xclks[i].con_id;
>>    		xclk->lookup->dev_id = pdata->xclks[i].dev_id;
>> -		xclk->lookup->clk = clk;
>> +		xclk->lookup->clk = xclk->clk;
>>
>>    		clkdev_add(xclk->lookup);
>>    	}
>> @@ -335,6 +337,9 @@ static void isp_xclk_cleanup(struct isp_device *isp)
>>    	for (i = 0; i<  ARRAY_SIZE(isp->xclks); ++i) {
>>    		struct isp_xclk *xclk =&isp->xclks[i];
>>
>> +		if (!IS_ERR(xclk->clk))
>> +			clk_unregister(xclk->clk);
>> +
>>    		if (xclk->lookup)
>>    			clkdev_drop(xclk->lookup);
>>    	}
>> diff --git a/drivers/media/platform/omap3isp/isp.h
>> b/drivers/media/platform/omap3isp/isp.h
>> index cd3eff4..1498f2b 100644
>> --- a/drivers/media/platform/omap3isp/isp.h
>> +++ b/drivers/media/platform/omap3isp/isp.h
>> @@ -135,6 +135,7 @@ struct isp_xclk {
>>    	struct isp_device *isp;
>>    	struct clk_hw hw;
>>    	struct clk_lookup *lookup;
>> +	struct clk *clk;
>>    	enum isp_xclk_id id;
>>
>>    	spinlock_t lock;	/* Protects enabled and divider */
>> ---------8<------------------

[PATCH v6 0/5] clk: clock deregistration support

[Mike Turquette]

Quoting Sylwester Nawrocki (2013-10-15 13:04:17)
> Hi,
> 
> (adding linux-media mailing list at Cc)
> 
> On 09/25/2013 11:47 AM, Laurent Pinchart wrote:
> > On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote:
> [...]
> >> The only issue I found might be at the omap3isp driver, which provides
> >> clock to its sub-drivers and takes reference on the sub-driver modules.
> >> When sub-driver calls clk_get() all modules would get locked in memory,
> >> due to circular reference. One solution to that could be to pass NULL
> >> struct device pointer, as in the below patch.
> >
> > Doesn't that introduce race conditions ? If the sub-drivers require the clock,
> > they want to be sure that the clock won't disappear beyond their backs. I
> > agree that the circular dependency needs to be solved somehow, but we probably
> > need a more generic solution. The problem will become more widespread in the
> > future with DT-based device instantiation in both V4L2 and KMS.
> 
> I'm wondering whether subsystems and drivers itself should be written so
> they deal with such dependencies they are aware of.
> 
> There is similar situation in the regulator API, regulator_get() simply
> takes a reference on a module providing the regulator object.
> 
> Before a "more generic solution" is available, what do you think about
> keeping obtaining a reference on a clock provider module in clk_get() and
> doing clk_get(), clk_prepare_enable(), ..., clk_unprepare_disable(),
> clk_put() in sub-driver whenever a clock is actively used, to avoid
> permanent circular reference ?

Laurent,

Did you have any feedback on this proposal? I would like to merge these
patches so that folks with clock driver modules can use them properly.
We can fix up things in the core code as we figure them out.

Regards,
Mike

> 
> --
> Thanks,
> Sylwester
> 
> >> ---------8<------------------
> >>   From ca5963041aad67e31324cb5d4d5e2cfce1706d4f Mon Sep 17 00:00:00 2001
> >> From: Sylwester Nawrocki<s.nawrocki@samsung.com>
> >> Date: Thu, 19 Sep 2013 23:52:04 +0200
> >> Subject: [PATCH] omap3isp: Pass NULL device pointer to clk_register()
> >>
> >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> >> ---
> >>    drivers/media/platform/omap3isp/isp.c |   15 ++++++++++-----
> >>    drivers/media/platform/omap3isp/isp.h |    1 +
> >>    2 files changed, 11 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/omap3isp/isp.c
> >> b/drivers/media/platform/omap3isp/isp.c
> >> index df3a0ec..d7f3c98 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -290,9 +290,11 @@ static int isp_xclk_init(struct isp_device *isp)
> >>      struct clk_init_data init;
> >>      unsigned int i;
> >>
> >> +    for (i = 0; i<  ARRAY_SIZE(isp->xclks); ++i)
> >> +            isp->xclks[i] = ERR_PTR(-EINVAL);
> >> +
> >>      for (i = 0; i<  ARRAY_SIZE(isp->xclks); ++i) {
> >>              struct isp_xclk *xclk =&isp->xclks[i];
> >> -            struct clk *clk;
> >>
> >>              xclk->isp = isp;
> >>              xclk->id = i == 0 ? ISP_XCLK_A : ISP_XCLK_B;
> >> @@ -306,9 +308,9 @@ static int isp_xclk_init(struct isp_device *isp)
> >>
> >>              xclk->hw.init =&init;
> >>
> >> -            clk = devm_clk_register(isp->dev,&xclk->hw);
> >> -            if (IS_ERR(clk))
> >> -                    return PTR_ERR(clk);
> >> +            xclk->clk = clk_register(NULL,&xclk->hw);
> >> +            if (IS_ERR(xclk->clk))
> >> +                    return PTR_ERR(xclk->clk);
> >>
> >>              if (pdata->xclks[i].con_id == NULL&&
> >>              pdata->xclks[i].dev_id == NULL)
> >> @@ -320,7 +322,7 @@ static int isp_xclk_init(struct isp_device *isp)
> >>
> >>              xclk->lookup->con_id = pdata->xclks[i].con_id;
> >>              xclk->lookup->dev_id = pdata->xclks[i].dev_id;
> >> -            xclk->lookup->clk = clk;
> >> +            xclk->lookup->clk = xclk->clk;
> >>
> >>              clkdev_add(xclk->lookup);
> >>      }
> >> @@ -335,6 +337,9 @@ static void isp_xclk_cleanup(struct isp_device *isp)
> >>      for (i = 0; i<  ARRAY_SIZE(isp->xclks); ++i) {
> >>              struct isp_xclk *xclk =&isp->xclks[i];
> >>
> >> +            if (!IS_ERR(xclk->clk))
> >> +                    clk_unregister(xclk->clk);
> >> +
> >>              if (xclk->lookup)
> >>                      clkdev_drop(xclk->lookup);
> >>      }
> >> diff --git a/drivers/media/platform/omap3isp/isp.h
> >> b/drivers/media/platform/omap3isp/isp.h
> >> index cd3eff4..1498f2b 100644
> >> --- a/drivers/media/platform/omap3isp/isp.h
> >> +++ b/drivers/media/platform/omap3isp/isp.h
> >> @@ -135,6 +135,7 @@ struct isp_xclk {
> >>      struct isp_device *isp;
> >>      struct clk_hw hw;
> >>      struct clk_lookup *lookup;
> >> +    struct clk *clk;
> >>      enum isp_xclk_id id;
> >>
> >>      spinlock_t lock;        /* Protects enabled and divider */
> >> ---------8<------------------

[PATCH v6 0/5] clk: clock deregistration support

[Laurent Pinchart]

On Monday 28 October 2013 12:54:01 Mike Turquette wrote:
> Quoting Sylwester Nawrocki (2013-10-15 13:04:17)
> 
> > Hi,
> > 
> > (adding linux-media mailing list at Cc)
> > 
> > On 09/25/2013 11:47 AM, Laurent Pinchart wrote:
> > > On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote:
> > [...]
> > 
> > >> The only issue I found might be at the omap3isp driver, which provides
> > >> clock to its sub-drivers and takes reference on the sub-driver modules.
> > >> When sub-driver calls clk_get() all modules would get locked in memory,
> > >> due to circular reference. One solution to that could be to pass NULL
> > >> struct device pointer, as in the below patch.
> > > 
> > > Doesn't that introduce race conditions ? If the sub-drivers require the
> > > clock, they want to be sure that the clock won't disappear beyond their
> > > backs. I agree that the circular dependency needs to be solved somehow,
> > > but we probably need a more generic solution. The problem will become
> > > more widespread in the future with DT-based device instantiation in
> > > both V4L2 and KMS.
> > 
> > I'm wondering whether subsystems and drivers itself should be written so
> > they deal with such dependencies they are aware of.
> > 
> > There is similar situation in the regulator API, regulator_get() simply
> > takes a reference on a module providing the regulator object.
> > 
> > Before a "more generic solution" is available, what do you think about
> > keeping obtaining a reference on a clock provider module in clk_get() and
> > doing clk_get(), clk_prepare_enable(), ..., clk_unprepare_disable(),
> > clk_put() in sub-driver whenever a clock is actively used, to avoid
> > permanent circular reference ?
> 
> Laurent,
> 
> Did you have any feedback on this proposal? I would like to merge these
> patches so that folks with clock driver modules can use them properly.
> We can fix up things in the core code as we figure them out.

I've just replied to Sylwester's original patch. I'm basically OK with the 
idea as a temporary solution.

> > >> ---------8<------------------
> > >> 
> > >>   From ca5963041aad67e31324cb5d4d5e2cfce1706d4f Mon Sep 17 00:00:00
> > >>   2001
> > >> 
> > >> From: Sylwester Nawrocki<s.nawrocki@samsung.com>
> > >> Date: Thu, 19 Sep 2013 23:52:04 +0200
> > >> Subject: [PATCH] omap3isp: Pass NULL device pointer to clk_register()
> > >> 
> > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> > >> ---
> > >> 
> > >>    drivers/media/platform/omap3isp/isp.c |   15 ++++++++++-----
> > >>    drivers/media/platform/omap3isp/isp.h |    1 +
> > >>    2 files changed, 11 insertions(+), 5 deletions(-)
> > >> 
> > >> diff --git a/drivers/media/platform/omap3isp/isp.c
> > >> b/drivers/media/platform/omap3isp/isp.c
> > >> index df3a0ec..d7f3c98 100644
> > >> --- a/drivers/media/platform/omap3isp/isp.c
> > >> +++ b/drivers/media/platform/omap3isp/isp.c
> > >> @@ -290,9 +290,11 @@ static int isp_xclk_init(struct isp_device *isp)
> > >> 
> > >>      struct clk_init_data init;
> > >>      unsigned int i;
> > >> 
> > >> +    for (i = 0; i<  ARRAY_SIZE(isp->xclks); ++i)
> > >> +            isp->xclks[i] = ERR_PTR(-EINVAL);
> > >> +
> > >> 
> > >>      for (i = 0; i<  ARRAY_SIZE(isp->xclks); ++i) {
> > >>      
> > >>              struct isp_xclk *xclk =&isp->xclks[i];
> > >> 
> > >> -            struct clk *clk;
> > >> 
> > >>              xclk->isp = isp;
> > >>              xclk->id = i == 0 ? ISP_XCLK_A : ISP_XCLK_B;
> > >> 
> > >> @@ -306,9 +308,9 @@ static int isp_xclk_init(struct isp_device *isp)
> > >> 
> > >>              xclk->hw.init =&init;
> > >> 
> > >> -            clk = devm_clk_register(isp->dev,&xclk->hw);
> > >> -            if (IS_ERR(clk))
> > >> -                    return PTR_ERR(clk);
> > >> +            xclk->clk = clk_register(NULL,&xclk->hw);
> > >> +            if (IS_ERR(xclk->clk))
> > >> +                    return PTR_ERR(xclk->clk);
> > >> 
> > >>              if (pdata->xclks[i].con_id == NULL&&
> > >>              pdata->xclks[i].dev_id == NULL)
> > >> 
> > >> @@ -320,7 +322,7 @@ static int isp_xclk_init(struct isp_device *isp)
> > >> 
> > >>              xclk->lookup->con_id = pdata->xclks[i].con_id;
> > >>              xclk->lookup->dev_id = pdata->xclks[i].dev_id;
> > >> 
> > >> -            xclk->lookup->clk = clk;
> > >> +            xclk->lookup->clk = xclk->clk;
> > >> 
> > >>              clkdev_add(xclk->lookup);
> > >>      
> > >>      }
> > >> 
> > >> @@ -335,6 +337,9 @@ static void isp_xclk_cleanup(struct isp_device
> > >> *isp)
> > >> 
> > >>      for (i = 0; i<  ARRAY_SIZE(isp->xclks); ++i) {
> > >>      
> > >>              struct isp_xclk *xclk =&isp->xclks[i];
> > >> 
> > >> +            if (!IS_ERR(xclk->clk))
> > >> +                    clk_unregister(xclk->clk);
> > >> +
> > >> 
> > >>              if (xclk->lookup)
> > >>              
> > >>                      clkdev_drop(xclk->lookup);
> > >>      
> > >>      }
> > >> 
> > >> diff --git a/drivers/media/platform/omap3isp/isp.h
> > >> b/drivers/media/platform/omap3isp/isp.h
> > >> index cd3eff4..1498f2b 100644
> > >> --- a/drivers/media/platform/omap3isp/isp.h
> > >> +++ b/drivers/media/platform/omap3isp/isp.h
> > >> @@ -135,6 +135,7 @@ struct isp_xclk {
> > >> 
> > >>      struct isp_device *isp;
> > >>      struct clk_hw hw;
> > >>      struct clk_lookup *lookup;
> > >> 
> > >> +    struct clk *clk;
> > >> 
> > >>      enum isp_xclk_id id;
> > >>      
> > >>      spinlock_t lock;        /* Protects enabled and divider */
> > >> 
> > >> ---------8<------------------
-- 
Regards,

Laurent Pinchart

[PATCH v6 0/5] clk: clock deregistration support

[Laurent Pinchart]

Hi Sylwester,

On Tuesday 15 October 2013 22:04:17 Sylwester Nawrocki wrote:
> Hi,
> 
> (adding linux-media mailing list at Cc)
> 
> On 09/25/2013 11:47 AM, Laurent Pinchart wrote:
> > On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote:
> [...]
> 
> >> The only issue I found might be at the omap3isp driver, which provides
> >> clock to its sub-drivers and takes reference on the sub-driver modules.
> >> When sub-driver calls clk_get() all modules would get locked in memory,
> >> due to circular reference. One solution to that could be to pass NULL
> >> struct device pointer, as in the below patch.
> > 
> > Doesn't that introduce race conditions ? If the sub-drivers require the
> > clock, they want to be sure that the clock won't disappear beyond their
> > backs. I agree that the circular dependency needs to be solved somehow,
> > but we probably need a more generic solution. The problem will become
> > more widespread in the future with DT-based device instantiation in both
> > V4L2 and KMS.
> 
> I'm wondering whether subsystems and drivers itself should be written so
> they deal with such dependencies they are aware of.
> 
> There is similar situation in the regulator API, regulator_get() simply
> takes a reference on a module providing the regulator object.
> 
> Before a "more generic solution" is available, what do you think about
> keeping obtaining a reference on a clock provider module in clk_get() and
> doing clk_get(), clk_prepare_enable(), ..., clk_unprepare_disable(),
> clk_put() in sub-driver whenever a clock is actively used, to avoid
> permanent circular reference ?

That's a workaround I can live with in the short term, as long as we work on a 
generic solution to this problem. It will bite us back in the not too distant 
future if we pretend to forget about it.

-- 
Regards,

Laurent Pinchart

[PATCH v6 0/5] clk: clock deregistration support

[Mike Turquette]

Quoting Sylwester Nawrocki (2013-09-24 14:38:44)
> On 08/30/2013 04:53 PM, Sylwester Nawrocki wrote:
> > This patch series implements clock deregistration in the common clock
> > framework. Comparing to v5 it only includes further corrections of NULL
> > clock handling.
> [...]
> >    clk: Provide not locked variant of of_clk_get_from_provider()
> >    clkdev: Fix race condition in clock lookup from device tree
> >    clk: Add common __clk_get(), __clk_put() implementations
> >    clk: Assign module owner of a clock being registered
> >    clk: Implement clk_unregister
> 
> Hi Mike, Russell,
> 
> Would you have any further comments/suggestions on this series ?

I don't have any further comments. The changes look good to me and push
things in the right direction. I'll wait for Laurent's feedback on the
omap3isp driver implications though.

Regards,
Mike

> 
> I have inspected all callers of clk_register() and all should be fine
> with regards to dereferencing dev->driver. The first argument to this
> function is either NULL or clk_register() is being called in drivers'
> probe() callback, which ensures dev->driver won't change due to holding
> dev->mutex.
> 
> The only issue I found might be at the omap3isp driver, which provides
> clock to its sub-drivers and takes reference on the sub-driver modules.
> When sub-driver calls clk_get() all modules would get locked in memory,
> due to circular reference. One solution to that could be to pass NULL
> struct device pointer, as in the below patch.
> 
> ---------8<------------------
>  From ca5963041aad67e31324cb5d4d5e2cfce1706d4f Mon Sep 17 00:00:00 2001
> From: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Date: Thu, 19 Sep 2013 23:52:04 +0200
> Subject: [PATCH] omap3isp: Pass NULL device pointer to clk_register()
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>   drivers/media/platform/omap3isp/isp.c |   15 ++++++++++-----
>   drivers/media/platform/omap3isp/isp.h |    1 +
>   2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c 
> b/drivers/media/platform/omap3isp/isp.c
> index df3a0ec..d7f3c98 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -290,9 +290,11 @@ static int isp_xclk_init(struct isp_device *isp)
>         struct clk_init_data init;
>         unsigned int i;
> 
> +       for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i)
> +               isp->xclks[i] = ERR_PTR(-EINVAL);
> +
>         for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i) {
>                 struct isp_xclk *xclk = &isp->xclks[i];
> -               struct clk *clk;
> 
>                 xclk->isp = isp;
>                 xclk->id = i == 0 ? ISP_XCLK_A : ISP_XCLK_B;
> @@ -306,9 +308,9 @@ static int isp_xclk_init(struct isp_device *isp)
> 
>                 xclk->hw.init = &init;
> 
> -               clk = devm_clk_register(isp->dev, &xclk->hw);
> -               if (IS_ERR(clk))
> -                       return PTR_ERR(clk);
> +               xclk->clk = clk_register(NULL, &xclk->hw);
> +               if (IS_ERR(xclk->clk))
> +                       return PTR_ERR(xclk->clk);
> 
>                 if (pdata->xclks[i].con_id == NULL &&
>                     pdata->xclks[i].dev_id == NULL)
> @@ -320,7 +322,7 @@ static int isp_xclk_init(struct isp_device *isp)
> 
>                 xclk->lookup->con_id = pdata->xclks[i].con_id;
>                 xclk->lookup->dev_id = pdata->xclks[i].dev_id;
> -               xclk->lookup->clk = clk;
> +               xclk->lookup->clk = xclk->clk;
> 
>                 clkdev_add(xclk->lookup);
>         }
> @@ -335,6 +337,9 @@ static void isp_xclk_cleanup(struct isp_device *isp)
>         for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i) {
>                 struct isp_xclk *xclk = &isp->xclks[i];
> 
> +               if (!IS_ERR(xclk->clk))
> +                       clk_unregister(xclk->clk);
> +
>                 if (xclk->lookup)
>                         clkdev_drop(xclk->lookup);
>         }
> diff --git a/drivers/media/platform/omap3isp/isp.h 
> b/drivers/media/platform/omap3isp/isp.h
> index cd3eff4..1498f2b 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -135,6 +135,7 @@ struct isp_xclk {
>         struct isp_device *isp;
>         struct clk_hw hw;
>         struct clk_lookup *lookup;
> +       struct clk *clk;
>         enum isp_xclk_id id;
> 
>         spinlock_t lock;        /* Protects enabled and divider */
> -- 
> ---------8<------------------
> 
> 
> Alternatively an additional argument could be added to the clk_register*()
> functions. Something like:
> 
> ---------8<------------------
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 27f8c42..ef934ac 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1837,7 +1837,8 @@ struct clk *__clk_register(struct device *dev, 
> struct clk_hw *hw)
>   }
>   EXPORT_SYMBOL_GPL(__clk_register);
> 
> -static int _clk_register(struct device *dev, struct clk_hw *hw, struct 
> clk *clk)
> +static int _clk_register(struct device *dev, struct clk_hw *hw, struct 
> clk *clk,
> +                                                       struct module *owner)
>   {
>         int i, ret;
> 
> @@ -1877,6 +1878,8 @@ static int _clk_register(struct device *dev, 
> struct clk_hw *hw, struct clk *clk)
>                 }
>         }
> 
> +       clk->owner = owner;
> +
>         ret = __clk_init(dev, clk);
>         if (!ret)
>                 return 0;
> @@ -1892,7 +1895,7 @@ fail_name:
>   }
> 
>   /**
> - * clk_register - allocate a new clock, register it and return an 
> opaque cookie
> + * ___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
>    *
> @@ -1902,7 +1905,8 @@ fail_name:
>    * 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)
> +struct clk *___clk_register(struct device *dev, struct clk_hw *hw,
> +                                               struct module *owner)
>   {
>         int ret;
>         struct clk *clk;
> @@ -1914,7 +1918,7 @@ struct clk *clk_register(struct device *dev, 
> struct clk_hw *hw)
>                 goto fail_out;
>         }
> 
> -       ret = _clk_register(dev, hw, clk);
> +       ret = _clk_register(dev, hw, clk, owner);
>         if (!ret)
>                 return clk;
> 
> @@ -1922,7 +1926,7 @@ struct clk *clk_register(struct device *dev, 
> struct clk_hw *hw)
>   fail_out:
>         return ERR_PTR(ret);
>   }
> -EXPORT_SYMBOL_GPL(clk_register);
> +EXPORT_SYMBOL_GPL(___clk_register);
> 
>   /*
>    * Free memory allocated for a clock.
> @@ -2040,7 +2044,8 @@ static void devm_clk_release(struct device *dev, 
> void *res)
>    * automatically clk_unregister()ed on driver detach. See 
> clk_register() for
>    * more information.
>    */
> -struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw)
> +struct clk *__devm_clk_register(struct device *dev, struct clk_hw *hw,
> +                                               struct module *owner)
>   {
>         struct clk *clk;
>         int ret;
> @@ -2049,7 +2054,7 @@ struct clk *devm_clk_register(struct device *dev, 
> struct clk_hw *hw)
>         if (!clk)
>                 return ERR_PTR(-ENOMEM);
> 
> -       ret = _clk_register(dev, hw, clk);
> +       ret = _clk_register(dev, hw, clk, owner);
>         if (!ret) {
>                 devres_add(dev, clk);
>         } else {
> @@ -2059,7 +2064,7 @@ struct clk *devm_clk_register(struct device *dev, 
> struct clk_hw *hw)
> 
>         return clk;
>   }
> -EXPORT_SYMBOL_GPL(devm_clk_register);
> +EXPORT_SYMBOL_GPL(__devm_clk_register);
> 
>   static int devm_clk_match(struct device *dev, void *res, void *data)
>   {
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 13623f3..c656ebf 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -31,6 +31,7 @@
>   #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate 
> change */
> 
>   struct clk_hw;
> +struct module;
> 
>   /**
>    * struct clk_ops -  Callback operations for hardware clocks; these are to
> @@ -436,8 +437,21 @@ struct clk *clk_register_composite(struct device 
> *dev, const char *name,
>    * 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);
> -struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw);
> +struct clk *___clk_register(struct device *dev, struct clk_hw *hw,
> +               struct module *owner);
> +
> +static inline struct clk *clk_register(struct device *dev, struct 
> clk_hw *hw)
> +{
> +       return ___clk_register(dev, hw, THIS_MODULE);
> +}
> +
> +struct clk *__devm_clk_register(struct device *dev, struct clk_hw *hw,
> +                               struct module *owner);
> +static inline struct clk *devm_clk_register(struct device *dev,
> +                                           struct clk_hw *hw)
> +{
> +       return __devm_clk_register(dev, hw, THIS_MODULE);
> +}
> 
>   void clk_unregister(struct clk *clk);
>   void devm_clk_unregister(struct device *dev, struct clk *clk);
> 
> ---------8<------------------
> 
> Similarly it would need to be done for the remaining clk_register*() 
> functions,
> which have much longer arguments list.
> 
> It's a bit messy, since there is already __clk_register() function with 
> double
> underscore prefix. Perhaps that could be renamed to something else so 
> all the
> functions taking struct module * parameter are prefixed with double 
> underscore.
> 
> 
> I would squash patches 3/5 and 4/5 in the next iteration, if it is decided
> to keep using dev->driver->owner.
> 
> --
> Thanks,
> Sylwester

[PATCH v6 0/5] clk: clock deregistration support

[Laurent Pinchart]

Hi Sylwester,

On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote:
> On 08/30/2013 04:53 PM, Sylwester Nawrocki wrote:
> > This patch series implements clock deregistration in the common clock
> > framework. Comparing to v5 it only includes further corrections of NULL
> > clock handling.
> 
> [...]
> 
> >    clk: Provide not locked variant of of_clk_get_from_provider()
> >    clkdev: Fix race condition in clock lookup from device tree
> >    clk: Add common __clk_get(), __clk_put() implementations
> >    clk: Assign module owner of a clock being registered
> >    clk: Implement clk_unregister
> 
> Hi Mike, Russell,
> 
> Would you have any further comments/suggestions on this series ?
> 
> I have inspected all callers of clk_register() and all should be fine
> with regards to dereferencing dev->driver. The first argument to this
> function is either NULL or clk_register() is being called in drivers'
> probe() callback, which ensures dev->driver won't change due to holding
> dev->mutex.
> 
> The only issue I found might be at the omap3isp driver, which provides
> clock to its sub-drivers and takes reference on the sub-driver modules.
> When sub-driver calls clk_get() all modules would get locked in memory,
> due to circular reference. One solution to that could be to pass NULL
> struct device pointer, as in the below patch.
> 
> ---------8<------------------
>  From ca5963041aad67e31324cb5d4d5e2cfce1706d4f Mon Sep 17 00:00:00 2001
> From: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Date: Thu, 19 Sep 2013 23:52:04 +0200
> Subject: [PATCH] omap3isp: Pass NULL device pointer to clk_register()
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>   drivers/media/platform/omap3isp/isp.c |   15 ++++++++++-----
>   drivers/media/platform/omap3isp/isp.h |    1 +
>   2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c
> index df3a0ec..d7f3c98 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -290,9 +290,11 @@ static int isp_xclk_init(struct isp_device *isp)
>   	struct clk_init_data init;
>   	unsigned int i;
> 
> +	for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i)
> +		isp->xclks[i] = ERR_PTR(-EINVAL);

I don't think you've compile-tested this :-)

> +
>   	for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i) {
>   		struct isp_xclk *xclk = &isp->xclks[i];
> -		struct clk *clk;
> 
>   		xclk->isp = isp;
>   		xclk->id = i == 0 ? ISP_XCLK_A : ISP_XCLK_B;
> @@ -306,9 +308,9 @@ static int isp_xclk_init(struct isp_device *isp)
> 
>   		xclk->hw.init = &init;
> 
> -		clk = devm_clk_register(isp->dev, &xclk->hw);
> -		if (IS_ERR(clk))
> -			return PTR_ERR(clk);
> +		xclk->clk = clk_register(NULL, &xclk->hw);
> +		if (IS_ERR(xclk->clk))
> +			return PTR_ERR(xclk->clk);

This doesn't introduce any regression in the sense that it will trade a 
problem for another one, so I'm fine with it in the short. Could you add a 
small comment above the clk_register() call to explain why the first argument 
is NULL ?

> 
>   		if (pdata->xclks[i].con_id == NULL &&
>   		    pdata->xclks[i].dev_id == NULL)
> @@ -320,7 +322,7 @@ static int isp_xclk_init(struct isp_device *isp)
> 
>   		xclk->lookup->con_id = pdata->xclks[i].con_id;
>   		xclk->lookup->dev_id = pdata->xclks[i].dev_id;
> -		xclk->lookup->clk = clk;
> +		xclk->lookup->clk = xclk->clk;
> 
>   		clkdev_add(xclk->lookup);
>   	}
> @@ -335,6 +337,9 @@ static void isp_xclk_cleanup(struct isp_device *isp)
>   	for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i) {
>   		struct isp_xclk *xclk = &isp->xclks[i];
> 
> +		if (!IS_ERR(xclk->clk))
> +			clk_unregister(xclk->clk);
> +
>   		if (xclk->lookup)
>   			clkdev_drop(xclk->lookup);
>   	}
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h
> index cd3eff4..1498f2b 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -135,6 +135,7 @@ struct isp_xclk {
>   	struct isp_device *isp;
>   	struct clk_hw hw;
>   	struct clk_lookup *lookup;
> +	struct clk *clk;
>   	enum isp_xclk_id id;
> 
>   	spinlock_t lock;	/* Protects enabled and divider */

-- 
Regards,

Laurent Pinchart

[PATCH v6 0/5] clk: clock deregistration support

[Sylwester Nawrocki]

Hi Laurent,

On 10/28/2013 10:05 PM, Laurent Pinchart wrote:
> On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote:
>> On 08/30/2013 04:53 PM, Sylwester Nawrocki wrote:
>>> This patch series implements clock deregistration in the common clock
>>> framework. Comparing to v5 it only includes further corrections of NULL
>>> clock handling.
[...]
>> ---------8<------------------
>>   From ca5963041aad67e31324cb5d4d5e2cfce1706d4f Mon Sep 17 00:00:00 2001
>> From: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Date: Thu, 19 Sep 2013 23:52:04 +0200
>> Subject: [PATCH] omap3isp: Pass NULL device pointer to clk_register()
>>
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> ---
>>    drivers/media/platform/omap3isp/isp.c |   15 ++++++++++-----
>>    drivers/media/platform/omap3isp/isp.h |    1 +
>>    2 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c
>> b/drivers/media/platform/omap3isp/isp.c
>> index df3a0ec..d7f3c98 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -290,9 +290,11 @@ static int isp_xclk_init(struct isp_device *isp)
>>    	struct clk_init_data init;
>>    	unsigned int i;
>>
>> +	for (i = 0; i<  ARRAY_SIZE(isp->xclks); ++i)
>> +		isp->xclks[i] = ERR_PTR(-EINVAL);
>
> I don't think you've compile-tested this :-)

Thank you for the comments. Yeah, I messed up this, I thought this part
got recompiled but it didn't. I've fixed this in the recently posted
patch.

>> +
>>    	for (i = 0; i<  ARRAY_SIZE(isp->xclks); ++i) {
>>    		struct isp_xclk *xclk =&isp->xclks[i];
>> -		struct clk *clk;
>>
>>    		xclk->isp = isp;
>>    		xclk->id = i == 0 ? ISP_XCLK_A : ISP_XCLK_B;
>> @@ -306,9 +308,9 @@ static int isp_xclk_init(struct isp_device *isp)
>>
>>    		xclk->hw.init =&init;
>>
>> -		clk = devm_clk_register(isp->dev,&xclk->hw);
>> -		if (IS_ERR(clk))
>> -			return PTR_ERR(clk);
>> +		xclk->clk = clk_register(NULL,&xclk->hw);
>> +		if (IS_ERR(xclk->clk))
>> +			return PTR_ERR(xclk->clk);
>
> This doesn't introduce any regression in the sense that it will trade a
> problem for another one, so I'm fine with it in the short. Could you add a
> small comment above the clk_register() call to explain why the first argument
> is NULL ?

I'm not entirely happy about doing something like that. Nevertheless I 
didn't
hear so far any better proposals and I guess it could be treated as a short
term modification while we're working on proper handling of those circular
references.

--
Thanks,
Sylwester