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

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

[Sylwester Nawrocki]

This patch series implements clock deregistration in the common clock
framework. Detailed changes are listed at each patch. I have included
an additional patch in this series for the omap3isp driver, required 
to avoid regressions.

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):
  omap3isp: Modify clocks registration to avoid circular references
  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: 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                     |  185 +++++++++++++++++++++++++++++++--
 drivers/clk/clk.h                     |   16 +++
 drivers/clk/clkdev.c                  |   12 ++-
 drivers/media/platform/omap3isp/isp.c |   22 ++--
 drivers/media/platform/omap3isp/isp.h |    1 +
 include/linux/clk-private.h           |    5 +
 include/linux/clkdev.h                |    5 +
 11 files changed, 235 insertions(+), 19 deletions(-)
 create mode 100644 drivers/clk/clk.h

-- 
1.7.9.5

[PATCH v7 1/5] omap3isp: Modify clocks registration to avoid circular references

[Sylwester Nawrocki]

The clock core code is going to be modified so clk_get() takes
reference on the clock provider module. Until the potential circular
reference issue is properly addressed, we pass NULL as as the first
argument to clk_register(), in order to disallow sub-devices taking
a reference on the ISP module back trough clk_get(). This should
prevent locking the modules in memory.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
This patch has been "compile tested" only.

---
 drivers/media/platform/omap3isp/isp.c |   22 ++++++++++++++++------
 drivers/media/platform/omap3isp/isp.h |    1 +
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index df3a0ec..286027a 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].clk = 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;
@@ -305,10 +307,15 @@ static int isp_xclk_init(struct isp_device *isp)
 		init.num_parents = 1;

 		xclk->hw.init = &init;
-
-		clk = devm_clk_register(isp->dev, &xclk->hw);
-		if (IS_ERR(clk))
-			return PTR_ERR(clk);
+		/*
+		 * The first argument is NULL in order to avoid circular
+		 * reference, as this driver takes reference on the
+		 * sensor subdevice modules and the sensors would take
+		 * reference on this module through clk_get().
+		 */
+		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 +327,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 +342,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 */
--
1.7.9.5

[PATCH v7 2/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 v6:
 - 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 2cf2ea6..6ab6781 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);
 
@@ -2110,7 +2112,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)
@@ -2154,9 +2167,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;
@@ -2171,7 +2184,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);
@@ -2180,24 +2193,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 v7 3/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>
Reviewed-by: Mike Turquette <mturquette@linaro.org>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
Changes since v6:
 - 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 v7 4/5] clk: Add common __clk_get(), __clk_put() implementations

[Sylwester Nawrocki]

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

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.

The owner module is assigned while the clock is being registered,
in functions _clk_register() and __clk_register().

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v6:
 - squashed into this one the patch assigning module owner
   to struct clk.

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                  |   26 ++++++++++++++++++++++++++
 include/linux/clk-private.h        |    3 +++
 include/linux/clkdev.h             |    5 +++++
 7 files changed, 42 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 6ab6781..25b249c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1812,6 +1812,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)
@@ -1832,6 +1836,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;
@@ -1972,6 +1978,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 v7 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 v6:
 - fixed NULL clk handling and replaced pr_err() with WARN_ON_ONCE().

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_*.
---
 drivers/clk/clk.c           |  121 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/clk-private.h |    2 +
 2 files changed, 120 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 25b249c..7a33961 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 */
@@ -1777,6 +1795,7 @@ int __clk_init(struct device *dev, struct clk *clk)
 
 	clk_debug_register(clk);
 
+	kref_init(&clk->ref);
 out:
 	clk_prepare_unlock();
 
@@ -1912,13 +1931,104 @@ 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_ONCE(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;
+
+       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
+               return;
+
+	clk_prepare_lock();
+
+	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)
@@ -1986,6 +2096,7 @@ int __clk_get(struct clk *clk)
 	if (clk && !try_module_get(clk->owner))
 		return 0;
 
+	kref_get(&clk->ref);
 	return 1;
 }
 
@@ -1994,6 +2105,10 @@ void __clk_put(struct clk *clk)
 	if (WARN_ON_ONCE(IS_ERR(clk)))
 		return;
 
+	clk_prepare_lock();
+	kref_put(&clk->ref, __clk_release);
+	clk_prepare_unlock();
+
 	if (clk)
 		module_put(clk->owner);
 }
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

Re: [PATCH v7 1/5] omap3isp: Modify clocks registration to avoid circular references

[Laurent Pinchart]

Hi Sylwester,

Thank you for the patch.

On Tuesday 29 October 2013 20:51:04 Sylwester Nawrocki wrote:
> The clock core code is going to be modified so clk_get() takes
> reference on the clock provider module. Until the potential circular
> reference issue is properly addressed, we pass NULL as as the first
> argument to clk_register(), in order to disallow sub-devices taking
> a reference on the ISP module back trough clk_get(). This should
> prevent locking the modules in memory.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Do you plan to push this to mainline as part of this patch series ? I don't 
have pending patches for the omap3isp that would conflict with this patch, so 
that would be fine with me.

> ---
> This patch has been "compile tested" only.
> 
> ---
>  drivers/media/platform/omap3isp/isp.c |   22 ++++++++++++++++------
>  drivers/media/platform/omap3isp/isp.h |    1 +
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index df3a0ec..286027a 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].clk = 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;
> @@ -305,10 +307,15 @@ static int isp_xclk_init(struct isp_device *isp)
>  		init.num_parents = 1;
> 
>  		xclk->hw.init = &init;
> -
> -		clk = devm_clk_register(isp->dev, &xclk->hw);
> -		if (IS_ERR(clk))
> -			return PTR_ERR(clk);
> +		/*
> +		 * The first argument is NULL in order to avoid circular
> +		 * reference, as this driver takes reference on the
> +		 * sensor subdevice modules and the sensors would take
> +		 * reference on this module through clk_get().
> +		 */
> +		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 +327,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 +342,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

Re: [PATCH v7 1/5] omap3isp: Modify clocks registration to avoid circular references

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

Hi,

On Tue, Oct 29, 2013 at 11:28:37PM +0100, Laurent Pinchart wrote:
> On Tuesday 29 October 2013 20:51:04 Sylwester Nawrocki wrote:
> > The clock core code is going to be modified so clk_get() takes
> > reference on the clock provider module. Until the potential circular
> > reference issue is properly addressed, we pass NULL as as the first
> > argument to clk_register(), in order to disallow sub-devices taking
> > a reference on the ISP module back trough clk_get(). This should
> > prevent locking the modules in memory.
> > 
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Do you plan to push this to mainline as part of this patch series ? I don't 
> have pending patches for the omap3isp that would conflict with this patch, so 
> that would be fine with me.

I plan to add support for DT to omap3isp + ADP1653 shortly. I have
not yet started to work on this, but expect to send some first RFC
patches in November.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v7 1/5] omap3isp: Modify clocks registration to avoid circular references

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]

Hi Sebastian,

On Wednesday 30 October 2013 00:28:39 Sebastian Reichel wrote:
> On Tue, Oct 29, 2013 at 11:28:37PM +0100, Laurent Pinchart wrote:
> > On Tuesday 29 October 2013 20:51:04 Sylwester Nawrocki wrote:
> > > The clock core code is going to be modified so clk_get() takes
> > > reference on the clock provider module. Until the potential circular
> > > reference issue is properly addressed, we pass NULL as as the first
> > > argument to clk_register(), in order to disallow sub-devices taking
> > > a reference on the ISP module back trough clk_get(). This should
> > > prevent locking the modules in memory.
> > > 
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > 
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Do you plan to push this to mainline as part of this patch series ? I
> > don't have pending patches for the omap3isp that would conflict with this
> > patch, so that would be fine with me.
> 
> I plan to add support for DT to omap3isp + ADP1653 shortly. I have
> not yet started to work on this, but expect to send some first RFC
> patches in November.

That's very nice to hear ! And thanks for the heads up, I would have started 
working on it shortly.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH v7 1/5] omap3isp: Modify clocks registration to avoid circular references

[Sylwester Nawrocki]

Hi Laurent,

(adding Mauro and LMML at Cc)

On 10/29/2013 11:28 PM, Laurent Pinchart wrote:
> Hi Sylwester,
>
> Thank you for the patch.
>
> On Tuesday 29 October 2013 20:51:04 Sylwester Nawrocki wrote:
>> The clock core code is going to be modified so clk_get() takes
>> reference on the clock provider module. Until the potential circular
>> reference issue is properly addressed, we pass NULL as as the first
>> argument to clk_register(), in order to disallow sub-devices taking
>> a reference on the ISP module back trough clk_get(). This should
>> prevent locking the modules in memory.
>>
>> Cc: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>
> Acked-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
>
> Do you plan to push this to mainline as part of this patch series ? I don't
> have pending patches for the omap3isp that would conflict with this patch, so
> that would be fine with me.

Thanks, yes I thought this patch might be merged together through the clk
tree, if Mike is willing to take it and we get yours and Mauro's Ack on it.

>> ---
>> This patch has been "compile tested" only.
>>
>> ---
>>   drivers/media/platform/omap3isp/isp.c |   22 ++++++++++++++++------
>>   drivers/media/platform/omap3isp/isp.h |    1 +
>>   2 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c
>> b/drivers/media/platform/omap3isp/isp.c index df3a0ec..286027a 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].clk = 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;
>> @@ -305,10 +307,15 @@ static int isp_xclk_init(struct isp_device *isp)
>>   		init.num_parents = 1;
>>
>>   		xclk->hw.init =&init;
>> -
>> -		clk = devm_clk_register(isp->dev,&xclk->hw);
>> -		if (IS_ERR(clk))
>> -			return PTR_ERR(clk);
>> +		/*
>> +		 * The first argument is NULL in order to avoid circular
>> +		 * reference, as this driver takes reference on the
>> +		 * sensor subdevice modules and the sensors would take
>> +		 * reference on this module through clk_get().
>> +		 */
>> +		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 +327,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 +342,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,
Sylwester

Re: [PATCH v7 4/5] clk: Add common __clk_get(), __clk_put() implementations

[Russell King - ARM Linux]

On Tue, Oct 29, 2013 at 08:51:07PM +0100, Sylwester Nawrocki wrote:
> This patch adds common __clk_get(), __clk_put() clkdev helpers that
> replace their platform specific counterparts when the common clock
> API is used.
> 
> 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.
> 
> The owner module is assigned while the clock is being registered,
> in functions _clk_register() and __clk_register().
> 
> 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>

Re: [PATCH v7 0/5] clk: clock deregistration support

[Mike Turquette]

Quoting Sylwester Nawrocki (2013-10-29 12:51:03)
> This patch series implements clock deregistration in the common clock
> framework. Detailed changes are listed at each patch. I have included
> an additional patch in this series for the omap3isp driver, required 
> to avoid regressions.

Taken into clk-next. Thanks for the fine rework!

Regards,
Mike

> 
> 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):
>   omap3isp: Modify clocks registration to avoid circular references
>   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: 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                     |  185 +++++++++++++++++++++++++++++++--
>  drivers/clk/clk.h                     |   16 +++
>  drivers/clk/clkdev.c                  |   12 ++-
>  drivers/media/platform/omap3isp/isp.c |   22 ++--
>  drivers/media/platform/omap3isp/isp.h |    1 +
>  include/linux/clk-private.h           |    5 +
>  include/linux/clkdev.h                |    5 +
>  11 files changed, 235 insertions(+), 19 deletions(-)
>  create mode 100644 drivers/clk/clk.h
> 
> -- 
> 1.7.9.5

Re: [PATCH v7 5/5] clk: Implement clk_unregister()

[Stephen Warren]

On 10/29/2013 01:51 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.

This patch is now in Mike's clk-next and hence next-20131031, and causes
both a WARN and an OOPS when booting the Tegra Dalmore board. (See log
below)

If I do the following to fix some other issues:

1) Apply:
http://www.spinics.net/lists/arm-kernel/msg283619.html
clk: fix boot panic with non-dev-associated clocks

2) Merge some Tegra-specific bug-fixes:
https://lkml.org/lkml/2013/10/29/771
Re: pull request for Tegra clock rework and Tegra124 clock support

... then revert this patch a336ed7 "clk: Implement clk_unregister()",
everything works again.

> [    0.000000] ------------[ cut here ]------------
> [    0.000000] WARNING: CPU: 0 PID: 0 at include/linux/kref.h:47 __clk_get+0x6c/0x84()
> [    0.000000] Modules linked in:
> [    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.12.0-rc3-00069-g850d089 #172
> [    0.000000] [<c00158f0>] (unwind_backtrace+0x0/0xf8) from [<c00117a8>] (show_stack+0x10/0x14)
> [    0.000000] [<c00117a8>] (show_stack+0x10/0x14) from [<c053b0b4>] (dump_stack+0x80/0xc4)
> [    0.000000] [<c053b0b4>] (dump_stack+0x80/0xc4) from [<c00251f4>] (warn_slowpath_common+0x64/0x88)
> [    0.000000] [<c00251f4>] (warn_slowpath_common+0x64/0x88) from [<c0025234>] (warn_slowpath_null+0x1c/0x24)
> [    0.000000] [<c0025234>] (warn_slowpath_null+0x1c/0x24) from [<c03a3f98>] (__clk_get+0x6c/0x84)
> [    0.000000] [<c03a3f98>] (__clk_get+0x6c/0x84) from [<c03a17e0>] (of_clk_get+0x5c/0x74)
> [    0.000000] [<c03a17e0>] (of_clk_get+0x5c/0x74) from [<c03a1830>] (of_clk_get_by_name+0x38/0xb4)
> [    0.000000] [<c03a1830>] (of_clk_get_by_name+0x38/0xb4) from [<c073e114>] (tegra_pmc_init+0x70/0x298)
> [    0.000000] [<c073e114>] (tegra_pmc_init+0x70/0x298) from [<c073de68>] (tegra_dt_init_irq+0x10/0x20)
> [    0.000000] [<c073de68>] (tegra_dt_init_irq+0x10/0x20) from [<c0739384>] (init_IRQ+0x24/0x2c)
> [    0.000000] [<c0739384>] (init_IRQ+0x24/0x2c) from [<c0737910>] (start_kernel+0x1a0/0x30c)
> [    0.000000] [<c0737910>] (start_kernel+0x1a0/0x30c) from [<80008074>] (0x80008074)
> [    0.000000] ---[ end trace 1b75b31a2719ed1c ]---
...
> [   14.674455] Unable to handle kernel paging request at virtual address 6b6b6cc3
> [   14.681719] pgd = c0004000
> [   14.684517] [6b6b6cc3] *pgd=00000000
> [   14.688166] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> [   14.693507] Modules linked in:
> [   14.696640] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W    3.12.0-rc3-00069-g850d089 #172
> [   14.705385] task: ee07ea40 ti: ee098000 task.ti: ee098000
> [   14.710837] PC is at module_put+0x28/0x78
> [   14.714916] LR is at tegra_ehci_probe+0xac/0x3b8
> [   14.719575] pc : [<c0077b18>]    lr : [<c031d910>]    psr: 20000113
> [   14.719575] sp : ee099e60  ip : ee018740  fp : 00000000
> [   14.731092] r10: fffffdfb  r9 : ee098000  r8 : fffffdfb
> [   14.736350] r7 : c058ad18  r6 : ee112410  r5 : ee0186c0  r4 : ee098000
> [   14.742909] r3 : 00000001  r2 : 00000001  r1 : 000004d0  r0 : 6b6b6b6b
> [   14.749473] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
> [   14.756824] Control: 10c5387d  Table: 8000406a  DAC: 00000015
> [   14.762601] Process swapper/0 (pid: 1, stack limit = 0xee098240)
> [   14.768640] Stack: (0xee099e60 to 0xee09a000)
> [   14.773045] 9e60: ee365800 c031d910 c031d864 c0808e58 ee112410 00000000 c079e924 c074fe3c
> [   14.781273] 9e80: c07374e0 c02a225c c02a2244 c02a1108 00000000 ee112410 c079e924 ee112444
> [   14.789498] 9ea0: 00000000 c02a12b0 00000000 c079e924 c02a1224 c029f73c ee0880e0 ee1135f4
> [   14.797722] 9ec0: c079e924 ee37fb40 c0798bd0 c02a07dc c06a645c c079e924 c079e924 00000006
> [   14.805946] 9ee0: c0760ab8 c07c0900 c074fe3c c02a18c8 c02a2284 c076d118 00000006 c00089d8
> [   14.814170] 9f00: c07e60d8 ee133840 c0543d24 000000bf c07c0900 00000001 00000000 c0781dd8
> [   14.822393] 9f20: 60000113 00000001 00000008 c182b1d2 c055e284 c003d620 ee099f64 c004741c
> [   14.830617] 9f40: c06abae4 c070618c 00000006 00000006 c0781dc8 c076d118 00000006 c0760ab8
> [   14.838841] 9f60: c07c0900 000000b9 c0760ac4 c07374e0 00000000 c0737b78 00000006 00000006
> [   14.847064] 9f80: c07374e0 ffffffff 00000000 c0536aec 00000000 00000000 00000000 00000000
> [   14.855288] 9fa0: 00000000 c0536af4 00000000 c000e678 00000000 00000000 00000000 00000000
> [   14.863509] 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [   14.871732] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ffffffff ffffffff
> [   14.879994] [<c0077b18>] (module_put+0x28/0x78) from [<c031d910>] (tegra_ehci_probe+0xac/0x3b8)
> [   14.888782] [<c031d910>] (tegra_ehci_probe+0xac/0x3b8) from [<c02a225c>] (platform_drv_probe+0x18/0x1c)
> [   14.898247] [<c02a225c>] (platform_drv_probe+0x18/0x1c) from [<c02a1108>] (driver_probe_device+0x108/0x224)
> [   14.908050] [<c02a1108>] (driver_probe_device+0x108/0x224) from [<c02a12b0>] (__driver_attach+0x8c/0x90)
> [   14.917607] [<c02a12b0>] (__driver_attach+0x8c/0x90) from [<c029f73c>] (bus_for_each_dev+0x54/0x88)
> [   14.926729] [<c029f73c>] (bus_for_each_dev+0x54/0x88) from [<c02a07dc>] (bus_add_driver+0xd4/0x258)
> [   14.935837] [<c02a07dc>] (bus_add_driver+0xd4/0x258) from [<c02a18c8>] (driver_register+0x78/0xf4)
> [   14.944862] [<c02a18c8>] (driver_register+0x78/0xf4) from [<c00089d8>] (do_one_initcall+0xe4/0x140)
> [   14.953982] [<c00089d8>] (do_one_initcall+0xe4/0x140) from [<c0737b78>] (kernel_init_freeable+0xfc/0x1c4)
> [   14.963624] [<c0737b78>] (kernel_init_freeable+0xfc/0x1c4) from [<c0536af4>] (kernel_init+0x8/0xe4)
> [   14.972748] [<c0536af4>] (kernel_init+0x8/0xe4) from [<c000e678>] (ret_from_fork+0x14/0x3c)
> [   14.981155] Code: e5943004 e2833001 e5843004 f57ff05a (e5903158) 
> [   14.987438] ---[ end trace 1b75b31a2719ed1d ]---
> [   14.992109] note: swapper/0[1] exited with preempt_count 1
> [   14.997752] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> [   14.997752] 
> [   15.006970] CPU2: stopping
> [   15.009756] CPU: 2 PID: 0 Comm: swapper/2 Tainted: G      D W    3.12.0-rc3-00069-g850d089 #172
> [   15.018572] [<c00158f0>] (unwind_backtrace+0x0/0xf8) from [<c00117a8>] (show_stack+0x10/0x14)
> [   15.027180] [<c00117a8>] (show_stack+0x10/0x14) from [<c053b0b4>] (dump_stack+0x80/0xc4)
> [   15.035341] [<c053b0b4>] (dump_stack+0x80/0xc4) from [<c00142e8>] (handle_IPI+0xfc/0x120)
> [   15.043587] [<c00142e8>] (handle_IPI+0xfc/0x120) from [<c0008704>] (gic_handle_irq+0x54/0x5c)
> [   15.052183] [<c0008704>] (gic_handle_irq+0x54/0x5c) from [<c00122a0>] (__irq_svc+0x40/0x70)
> [   15.060573] Exception stack(0xee0b7fa0 to 0xee0b7fe8)
> [   15.065671] 7fa0: ffffffed 010d0000 c077b968 00000000 ee0b6000 c07c0747 00000001 c07c0747
> [   15.073900] 7fc0: c077a44c c077a3d4 c05430cc 00000000 00000001 ee0b7fe8 c000f1dc c000f1d4
> [   15.082114] 7fe0: 60000113 ffffffff
> [   15.085679] [<c00122a0>] (__irq_svc+0x40/0x70) from [<c000f1d4>] (arch_cpu_idle+0x28/0x38)
> [   15.094019] [<c000f1d4>] (arch_cpu_idle+0x28/0x38) from [<c0061d58>] (cpu_startup_entry+0x60/0x134)
> [   15.103127] [<c0061d58>] (cpu_startup_entry+0x60/0x134) from [<800087a4>] (0x800087a4)
> [   15.111089] CPU3: stopping
> [   15.113872] CPU: 3 PID: 0 Comm: swapper/3 Tainted: G      D W    3.12.0-rc3-00069-g850d089 #172
> [   15.122680] [<c00158f0>] (unwind_backtrace+0x0/0xf8) from [<c00117a8>] (show_stack+0x10/0x14)
> [   15.131284] [<c00117a8>] (show_stack+0x10/0x14) from [<c053b0b4>] (dump_stack+0x80/0xc4)
> [   15.139441] [<c053b0b4>] (dump_stack+0x80/0xc4) from [<c00142e8>] (handle_IPI+0xfc/0x120)
> [   15.147683] [<c00142e8>] (handle_IPI+0xfc/0x120) from [<c0008704>] (gic_handle_irq+0x54/0x5c)
> [   15.156280] [<c0008704>] (gic_handle_irq+0x54/0x5c) from [<c00122a0>] (__irq_svc+0x40/0x70)
> [   15.164669] Exception stack(0xee0b9fa0 to 0xee0b9fe8)
> [   15.169768] 9fa0: ffffffed 010d8000 c077b968 00000000 ee0b8000 c07c0747 00000001 c07c0747
> [   15.177994] 9fc0: c077a44c c077a3d4 c05430cc 00000000 00000001 ee0b9fe8 c000f1dc c000f1d4
> [   15.186208] 9fe0: 60000113 ffffffff
> [   15.189771] [<c00122a0>] (__irq_svc+0x40/0x70) from [<c000f1d4>] (arch_cpu_idle+0x28/0x38)
> [   15.198108] [<c000f1d4>] (arch_cpu_idle+0x28/0x38) from [<c0061d58>] (cpu_startup_entry+0x60/0x134)
> [   15.207215] [<c0061d58>] (cpu_startup_entry+0x60/0x134) from [<800087a4>] (0x800087a4)
> [   15.215178] CPU0: stopping
> [   15.217953] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G      D W    3.12.0-rc3-00069-g850d089 #172
> [   15.226758] [<c00158f0>] (unwind_backtrace+0x0/0xf8) from [<c00117a8>] (show_stack+0x10/0x14)
> [   15.235363] [<c00117a8>] (show_stack+0x10/0x14) from [<c053b0b4>] (dump_stack+0x80/0xc4)
> [   15.243522] [<c053b0b4>] (dump_stack+0x80/0xc4) from [<c00142e8>] (handle_IPI+0xfc/0x120)
> [   15.251765] [<c00142e8>] (handle_IPI+0xfc/0x120) from [<c0008704>] (gic_handle_irq+0x54/0x5c)
> [   15.260363] [<c0008704>] (gic_handle_irq+0x54/0x5c) from [<c00122a0>] (__irq_svc+0x40/0x70)
> [   15.268754] Exception stack(0xc0773f68 to 0xc0773fb0)
> [   15.273851] 3f60:                   ffffffed 010c0000 c077b968 00000000 c0772000 c07c0747
> [   15.282080] 3f80: 00000001 c07c0747 c077a44c c077a3d4 c05430cc 00000000 00000020 c0773fb0
> [   15.290297] 3fa0: c000f1dc c000f1d4 60000113 ffffffff
> [   15.295421] [<c00122a0>] (__irq_svc+0x40/0x70) from [<c000f1d4>] (arch_cpu_idle+0x28/0x38)
> [   15.303757] [<c000f1d4>] (arch_cpu_idle+0x28/0x38) from [<c0061d58>] (cpu_startup_entry+0x60/0x134)
> [   15.312874] [<c0061d58>] (cpu_startup_entry+0x60/0x134) from [<c0737a28>] (start_kernel+0x2b8/0x30c)
> [   15.322069] [<c0737a28>] (start_kernel+0x2b8/0x30c) from [<80008074>] (0x80008074)

Re: [PATCH v7 5/5] clk: Implement clk_unregister()

[Sylwester Nawrocki]

On 31/10/13 19:32, Stephen Warren wrote:
> On 10/29/2013 01:51 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.
>
> This patch is now in Mike's clk-next and hence next-20131031, and causes
> both a WARN and an OOPS when booting the Tegra Dalmore board. (See log
> below)
> 
> If I do the following to fix some other issues:
> 
> 1) Apply:
> http://www.spinics.net/lists/arm-kernel/msg283619.html
> clk: fix boot panic with non-dev-associated clocks
> 
> 2) Merge some Tegra-specific bug-fixes:
> https://lkml.org/lkml/2013/10/29/771
> Re: pull request for Tegra clock rework and Tegra124 clock support
> 
> ... then revert this patch a336ed7 "clk: Implement clk_unregister()",
> everything works again.

Does it still crash when you apply this patch
http://www.spinics.net/lists/arm-kernel/msg283550.html ?

--
Thanks,
Sylwester

Re: [PATCH v7 5/5] clk: Implement clk_unregister()

[Stephen Warren]

On 10/31/2013 12:55 PM, Sylwester Nawrocki wrote:
> On 31/10/13 19:32, Stephen Warren wrote:
>> On 10/29/2013 01:51 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.
>>
>> This patch is now in Mike's clk-next and hence next-20131031, and causes
>> both a WARN and an OOPS when booting the Tegra Dalmore board. (See log
>> below)
>>
>> If I do the following to fix some other issues:
>>
>> 1) Apply:
>> http://www.spinics.net/lists/arm-kernel/msg283619.html
>> clk: fix boot panic with non-dev-associated clocks
>>
>> 2) Merge some Tegra-specific bug-fixes:
>> https://lkml.org/lkml/2013/10/29/771
>> Re: pull request for Tegra clock rework and Tegra124 clock support
>>
>> ... then revert this patch a336ed7 "clk: Implement clk_unregister()",
>> everything works again.
> 
> Does it still crash when you apply this patch
> http://www.spinics.net/lists/arm-kernel/msg283550.html ?

Yes, that seems to work, thanks.

Re: [PATCH v7 0/5] clk: clock deregistration support

[Jonas Jensen]

Hi,

Just letting you know, the following warning from __clk_get is now
printed, and not printed after revert (git revert
0b35b92fb3600a2f9ca114a6142db95f760d55f5).

Is the driver doing something it shouldn't be doing?

moxart_of_pll_clk_init() source can be found here:
http://www.spinics.net/lists/arm-kernel/msg278572.html

boot log:
Uncompressing Linux... done, booting the kernel.
[    0.000000] Booting Linux on physical CPU 0x0
[    0.000000] Linux version 3.12.0-rc7-next-20131031+ (i@Ildjarn)
(gcc version 4.6.3 (crosstool-NG 1.16.0) ) #1043 PREEMPT Fri Nov 1
09:34:07 CET 2013
[    0.000000] CPU: FA526 [66015261] revision 1 (ARMv4), cr=0000397f
[    0.000000] CPU: VIVT data cache, VIVT instruction cache
[    0.000000] Machine model: MOXA UC-7112-LX
[    0.000000] Memory policy: ECC disabled, Data cache writeback
[    0.000000] On node 0 totalpages: 8192
[    0.000000] free_area_init_node: node 0, pgdat c03802ac,
node_mem_map c0941000
[    0.000000]   Normal zone: 72 pages used for memmap
[    0.000000]   Normal zone: 0 pages reserved
[    0.000000]   Normal zone: 8192 pages, LIFO batch:0
[    0.000000] pcpu-alloc: s0 r0 d32768 u32768 alloc=1*32768
[    0.000000] pcpu-alloc: [0] 0
[    0.000000] Built 1 zonelists in Zone order, mobility grouping on.
Total pages: 8120
[    0.000000] Kernel command line: debug loglevel=9
console=ttyS0,115200n8 root=/dev/mmcblk0p1 rw rootwait
[    0.000000] PID hash table entries: 128 (order: -3, 512 bytes)
[    0.000000] Dentry cache hash table entries: 4096 (order: 2, 16384 bytes)
[    0.000000] Inode-cache hash table entries: 2048 (order: 1, 8192 bytes)
[    0.000000] Memory: 22920K/32768K available (2725K kernel code,
106K rwdata, 560K rodata, 156K init, 5880K bss, 9848K reserved)
[    0.000000] Virtual kernel memory layout:
[    0.000000]     vector  : 0xffff0000 - 0xffff1000   (   4 kB)
[    0.000000]     fixmap  : 0xfff00000 - 0xfffe0000   ( 896 kB)
[    0.000000]     vmalloc : 0xc2800000 - 0xff000000   ( 968 MB)
[    0.000000]     lowmem  : 0xc0000000 - 0xc2000000   (  32 MB)
[    0.000000]       .text : 0xc0008000 - 0xc033d84c   (3287 kB)
[    0.000000]       .init : 0xc033e000 - 0xc0365394   ( 157 kB)
[    0.000000]       .data : 0xc0366000 - 0xc0380ae0   ( 107 kB)
[    0.000000]        .bss : 0xc0380aec - 0xc093ebbc   (5881 kB)
[    0.000000] SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
[    0.000000] Preemptible hierarchical RCU implementation.
[    0.000000] NR_IRQS:16 nr_irqs:16 16
[    0.000000] ------------[ cut here ]------------
[    0.000000] WARNING: CPU: 0 PID: 0 at include/linux/kref.h:47
__clk_get+0x54/0x68()
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
3.12.0-rc7-next-20131031+ #1043
[    0.000000] [<c000d214>] (unwind_backtrace+0x0/0xf4) from
[<c000b964>] (show_stack+0x18/0x1c)
[    0.000000] [<c000b964>] (show_stack+0x18/0x1c) from [<c02715e0>]
(dump_stack+0x20/0x28)
[    0.000000] [<c02715e0>] (dump_stack+0x20/0x28) from [<c0013ab0>]
(warn_slowpath_common+0x64/0x84)
[    0.000000] [<c0013ab0>] (warn_slowpath_common+0x64/0x84) from
[<c0013ba4>] (warn_slowpath_null+0x24/0x2c)
[    0.000000] [<c0013ba4>] (warn_slowpath_null+0x24/0x2c) from
[<c01e5c00>] (__clk_get+0x54/0x68)
[    0.000000] [<c01e5c00>] (__clk_get+0x54/0x68) from [<c01e334c>]
(of_clk_get+0x64/0x7c)
[    0.000000] [<c01e334c>] (of_clk_get+0x64/0x7c) from [<c03508f0>]
(moxart_of_pll_clk_init+0xd8/0x15c)
[    0.000000] [<c03508f0>] (moxart_of_pll_clk_init+0xd8/0x15c) from
[<c0350588>] (of_clk_init+0x48/0x70)
[    0.000000] [<c0350588>] (of_clk_init+0x48/0x70) from [<c03425f0>]
(moxart_init_time+0x14/0x1c)
[    0.000000] [<c03425f0>] (moxart_init_time+0x14/0x1c) from
[<c034005c>] (time_init+0x28/0x3c)
[    0.000000] [<c034005c>] (time_init+0x28/0x3c) from [<c033e954>]
(start_kernel+0x1d0/0x2dc)
[    0.000000] [<c033e954>] (start_kernel+0x1d0/0x2dc) from
[<00008040>] (0x8040)
[    0.000000] ---[ end trace 3406ff24bd97382e ]---
[    0.000000] sched_clock: 32 bits at 100 Hz, resolution 10000000ns,
wraps every 21474836480000000ns
[    0.000000] Lock dependency validator: Copyright (c) 2006 Red Hat,
Inc., Ingo Molnar
[    0.000000] ... MAX_LOCKDEP_SUBCLASSES:  8
[    0.000000] ... MAX_LOCK_DEPTH:          48
[    0.000000] ... MAX_LOCKDEP_KEYS:        8191
[    0.000000] ... CLASSHASH_SIZE:          4096
[    0.000000] ... MAX_LOCKDEP_ENTRIES:     16384
[    0.000000] ... MAX_LOCKDEP_CHAINS:      32768
[    0.000000] ... CHAINHASH_SIZE:          16384
[    0.000000]  memory used by lock dependency info: 3695 kB
[    0.000000]  per task-struct memory footprint: 1152 bytes
[    0.000000] kmemleak: Kernel memory leak detector disabled
[    0.000000] ODEBUG: 0 of 0 active objects replaced
[    0.000000] kmemleak: Early log buffer exceeded (672), please
increase DEBUG_KMEMLEAK_EARLY_LOG_SIZE


Regards,
Jonas

Re: [PATCH v7 0/5] clk: clock deregistration support

[Sylwester Nawrocki]

Hi Jonas,

On 11/01/2013 02:56 PM, Jonas Jensen wrote:
> Hi,
>
> Just letting you know, the following warning from __clk_get is now
> printed, and not printed after revert (git revert
> 0b35b92fb3600a2f9ca114a6142db95f760d55f5).

It is recommended to quote also human readable patch summary line,
so it's more immediately clear which patch you refer to.

> Is the driver doing something it shouldn't be doing?

I don't think so. That is a known issue, it shouldn't be happening when
you apply $subject patch series onto todays -next. If it does please
let me know.

Is the warning still triggered when you apply this patch:
http://www.spinics.net/lists/arm-kernel/msg283550.html
onto next-20131031 instead of reverting ?

> moxart_of_pll_clk_init() source can be found here:
> http://www.spinics.net/lists/arm-kernel/msg278572.html

The driver seems OK from a brief look.  Thanks for the feedback.

> boot log:
> Uncompressing Linux... done, booting the kernel.
> [    0.000000] Booting Linux on physical CPU 0x0
> [    0.000000] Linux version 3.12.0-rc7-next-20131031+ (i@Ildjarn)

> [    0.000000] ------------[ cut here ]------------
> [    0.000000] WARNING: CPU: 0 PID: 0 at include/linux/kref.h:47
> __clk_get+0x54/0x68()
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
> 3.12.0-rc7-next-20131031+ #1043
> [    0.000000] [<c000d214>] (unwind_backtrace+0x0/0xf4) from
> [<c000b964>] (show_stack+0x18/0x1c)
> [    0.000000] [<c000b964>] (show_stack+0x18/0x1c) from [<c02715e0>]
> (dump_stack+0x20/0x28)
> [    0.000000] [<c02715e0>] (dump_stack+0x20/0x28) from [<c0013ab0>]
> (warn_slowpath_common+0x64/0x84)
> [    0.000000] [<c0013ab0>] (warn_slowpath_common+0x64/0x84) from
> [<c0013ba4>] (warn_slowpath_null+0x24/0x2c)
> [    0.000000] [<c0013ba4>] (warn_slowpath_null+0x24/0x2c) from
> [<c01e5c00>] (__clk_get+0x54/0x68)
> [    0.000000] [<c01e5c00>] (__clk_get+0x54/0x68) from [<c01e334c>]
> (of_clk_get+0x64/0x7c)
> [    0.000000] [<c01e334c>] (of_clk_get+0x64/0x7c) from [<c03508f0>]
> (moxart_of_pll_clk_init+0xd8/0x15c)
> [    0.000000] [<c03508f0>] (moxart_of_pll_clk_init+0xd8/0x15c) from
> [<c0350588>] (of_clk_init+0x48/0x70)
> [    0.000000] [<c0350588>] (of_clk_init+0x48/0x70) from [<c03425f0>]
> (moxart_init_time+0x14/0x1c)
> [    0.000000] [<c03425f0>] (moxart_init_time+0x14/0x1c) from
> [<c034005c>] (time_init+0x28/0x3c)
> [    0.000000] [<c034005c>] (time_init+0x28/0x3c) from [<c033e954>]
> (start_kernel+0x1d0/0x2dc)
> [    0.000000] [<c033e954>] (start_kernel+0x1d0/0x2dc) from
> [<00008040>] (0x8040)
> [    0.000000] ---[ end trace 3406ff24bd97382e ]---

Regards,
Sylwester

Re: [PATCH v7 0/5] clk: clock deregistration support

[Jonas Jensen]

On 1 November 2013 17:41, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> It is recommended to quote also human readable patch summary line,
> so it's more immediately clear which patch you refer to.

Sorry about that. This was tested on a clean (some added moxart
drivers but no other patches) next-20131031, reverting the following
patch:
"clkdev: Fix race condition in clock lookup from device tree" -
0b35b92fb3600a2f9ca114a6142db95f760d55f5

> Is the warning still triggered when you apply this patch:
> http://www.spinics.net/lists/arm-kernel/msg283550.html
> onto next-20131031 instead of reverting ?

After patch, the warning is no longer triggered.

Thanks,
Jonas