[PATCH RFC 0/2] Clock unregistration support in the common clock framework

[PATCH RFC 0/2] Clock unregistration support in the common clock framework

[Sylwester Nawrocki]

Hello,

This short patch set aims to fix issues in the common clock framework WRT
support of the clock suppliers as loadable modules. The thread [1] might
be a good summary and pre-requisiste reading on what this patch series is
trying to achieve.

The first patch adds common implementation of the __clk_get(), __clk_put()
helpers for the common clock framework. Currently these functions are empty
and the modules that registered clocks are prone to removal and leaving
invalid clock references.

The second patch adds implementation of the clk_unregister() function and
is based on patch [2].

I have some doubts whether we need to be taking reference on a module in
clk_get() _and_ use kref to keep track of references to a clock. Taking
reference on a module only seems insufficient, since a clock supplier
driver can be unbound from its device through sysfs, even if the module
stays in place.

We could be doing only reference counting on the clock, but then there
are issues as Russell clearly explained in [1]. It is not obvious what
to do with a clock when it has consumers and its supplier driver is
being deinitialized/unloaded [3]. IMHO clock suppliers should be normally
prevented from being removed when resources they provide are in use,
otherwise it all may get a bit hairy.

Thanks,
Sylwester

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/183302.html
[2] http://www.spinics.net/lists/arm-kernel/msg247548.html
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/183292.html


Sylwester Nawrocki (2):
  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                  |  147 +++++++++++++++++++++++++++++++++++-
 include/linux/clk-private.h        |    5 ++
 include/linux/clkdev.h             |    5 ++
 7 files changed, 162 insertions(+), 3 deletions(-)

--
1.7.9.5

[PATCH RFC 1/2] 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 enabled.

An owner module pointer is added to struct clk so a reference
to the clock supplier module is taken when the clock has active
consumers.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 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                  |   24 ++++++++++++++++++++++++
 include/linux/clk-private.h        |    3 +++
 include/linux/clkdev.h             |    5 +++++
 7 files changed, 40 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 54a191c..4877bd6 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1850,6 +1850,30 @@ 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 (WARN_ON((!clk)))
+		return 0;
+
+	if (!try_module_get(clk->owner))
+		return 0;
+
+	return 1;
+}
+EXPORT_SYMBOL(__clk_get);
+
+void __clk_put(struct clk *clk)
+{
+	if (!clk || IS_ERR(clk))
+		return;
+
+	module_put(clk->owner);
+}
+EXPORT_SYMBOL(__clk_put);
+
 /***        clk rate change notifiers        ***/

 /**
diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
index dd7adff..b7c0b58 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 RFC 2/2] 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 its driver remove() callback
called on it.

Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---

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           |  123 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/clk-private.h |    2 +
 2 files changed, 122 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4877bd6..327f83f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -342,6 +342,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
@@ -432,6 +447,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 */
@@ -1655,6 +1673,7 @@ int __clk_init(struct device *dev, struct clk *clk)

 	clk_debug_register(clk);

+	kref_init(&clk->ref);
 out:
 	clk_prepare_unlock();

@@ -1784,13 +1803,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_dummy_prepare_enable(struct clk_hw *hw)
+{
+	return -ENXIO;
+}
+
+static void clk_dummy_disable_unprepare(struct clk_hw *hw)
+{
+	WARN_ON(1);
+}
+
+static int clk_dummy_set_rate(struct clk_hw *hw, unsigned long rate,
+					unsigned long parent_rate)
+{
+	return -ENXIO;
+}
+
+static int clk_dummy_set_parent(struct clk_hw *hw, u8 index)
+{
+	return -ENXIO;
+}
+
+static const struct clk_ops clk_dummy_ops = {
+	.enable		= clk_dummy_prepare_enable,
+	.disable	= clk_dummy_disable_unprepare,
+	.prepare	= clk_dummy_prepare_enable,
+	.unprepare	= clk_dummy_disable_unprepare,
+	.set_rate	= clk_dummy_set_rate,
+	.set_parent	= clk_dummy_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_dummy_ops) {
+		pr_err("%s: unregistered clock: %s\n", __func__, clk->name);
+		goto out;
+	}
+	/*
+	 * Assign dummy clock ops for consumers that might still hold
+	 * a reference to this clock.
+	 */
+	flags = clk_enable_lock();
+	clk->ops = &clk_dummy_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)
@@ -1861,6 +1973,7 @@ int __clk_get(struct clk *clk)
 	if (!try_module_get(clk->owner))
 		return 0;

+	kref_get(&clk->ref);
 	return 1;
 }
 EXPORT_SYMBOL(__clk_get);
@@ -1870,6 +1983,10 @@ void __clk_put(struct clk *clk)
 	if (!clk || IS_ERR(clk))
 		return;

+	clk_prepare_lock();
+	kref_put(&clk->ref, __clk_release);
+	clk_prepare_unlock();
+
 	module_put(clk->owner);
 }
 EXPORT_SYMBOL(__clk_put);
diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
index b7c0b58..36c1fc8 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>

 /*
@@ -47,6 +48,7 @@ struct clk {
 #ifdef CONFIG_COMMON_CLK_DEBUG
 	struct dentry		*dentry;
 #endif
+	struct kref		ref;
 };

 /*
--
1.7.9.5

[PATCH RFC 2/2] clk: implement clk_unregister

[Mike Turquette]

Quoting Sylwester Nawrocki (2013-08-06 08:51:57)
> +/*
> + * 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_dummy_prepare_enable(struct clk_hw *hw)
> +{
> +       return -ENXIO;
> +}
> +
> +static void clk_dummy_disable_unprepare(struct clk_hw *hw)
> +{
> +       WARN_ON(1);
> +}
> +
> +static int clk_dummy_set_rate(struct clk_hw *hw, unsigned long rate,
> +                                       unsigned long parent_rate)
> +{
> +       return -ENXIO;
> +}
> +
> +static int clk_dummy_set_parent(struct clk_hw *hw, u8 index)
> +{
> +       return -ENXIO;
> +}
> +
> +static const struct clk_ops clk_dummy_ops = {
> +       .enable         = clk_dummy_prepare_enable,
> +       .disable        = clk_dummy_disable_unprepare,
> +       .prepare        = clk_dummy_prepare_enable,
> +       .unprepare      = clk_dummy_disable_unprepare,
> +       .set_rate       = clk_dummy_set_rate,
> +       .set_parent     = clk_dummy_set_parent,
> +};

Don't use "clk_dummy_*" here. The use of dummy often implies that
operations will return success in the absence of actual hardware but
these return an error, and rightly so. So maybe rename the functions and
clk_ops instance to something like "clk_nodev_*" or "clk_missing_*"?

> +
>  /**
>   * 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_dummy_ops) {
> +               pr_err("%s: unregistered clock: %s\n", __func__, clk->name);
> +               goto out;
> +       }
> +       /*
> +        * Assign dummy clock ops for consumers that might still hold
> +        * a reference to this clock.
> +        */
> +       flags = clk_enable_lock();
> +       clk->ops = &clk_dummy_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);
> +       }

This looks pretty good. A remaining problem is re-loading the clock
provider module will have string name conflicts with the old
unregistered clocks (but not yet released) clocks during calls to
__clk_lookup.

The best solution would be to refactor clk.c to not use string name
lookups but that is probably too big of an issue for the purpose of this
series (but it will happen some day).

A short term solution would be to poison the clock's string name here.
Reallocate the clk->name string with some poison data so that name
conflicts don't occur. What do you think?

Regards,
Mike

> +
> +       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)
> @@ -1861,6 +1973,7 @@ int __clk_get(struct clk *clk)
>         if (!try_module_get(clk->owner))
>                 return 0;
> 
> +       kref_get(&clk->ref);
>         return 1;
>  }
>  EXPORT_SYMBOL(__clk_get);
> @@ -1870,6 +1983,10 @@ void __clk_put(struct clk *clk)
>         if (!clk || IS_ERR(clk))
>                 return;
> 
> +       clk_prepare_lock();
> +       kref_put(&clk->ref, __clk_release);
> +       clk_prepare_unlock();
> +
>         module_put(clk->owner);
>  }
>  EXPORT_SYMBOL(__clk_put);
> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
> index b7c0b58..36c1fc8 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>
> 
>  /*
> @@ -47,6 +48,7 @@ struct clk {
>  #ifdef CONFIG_COMMON_CLK_DEBUG
>         struct dentry           *dentry;
>  #endif
> +       struct kref             ref;
>  };
> 
>  /*
> --
> 1.7.9.5

[PATCH RFC 2/2] clk: implement clk_unregister

[Sylwester Nawrocki]

On 08/16/2013 11:48 PM, Mike Turquette wrote:
> Quoting Sylwester Nawrocki (2013-08-06 08:51:57)
>> +/*
>> + * 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_dummy_prepare_enable(struct clk_hw *hw)
>> +{
>> +       return -ENXIO;
>> +}
>> +
>> +static void clk_dummy_disable_unprepare(struct clk_hw *hw)
>> +{
>> +       WARN_ON(1);
>> +}
>> +
>> +static int clk_dummy_set_rate(struct clk_hw *hw, unsigned long rate,
>> +                                       unsigned long parent_rate)
>> +{
>> +       return -ENXIO;
>> +}
>> +
>> +static int clk_dummy_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> +       return -ENXIO;
>> +}
>> +
>> +static const struct clk_ops clk_dummy_ops = {
>> +       .enable         = clk_dummy_prepare_enable,
>> +       .disable        = clk_dummy_disable_unprepare,
>> +       .prepare        = clk_dummy_prepare_enable,
>> +       .unprepare      = clk_dummy_disable_unprepare,
>> +       .set_rate       = clk_dummy_set_rate,
>> +       .set_parent     = clk_dummy_set_parent,
>> +};
> 
> Don't use "clk_dummy_*" here. The use of dummy often implies that
> operations will return success in the absence of actual hardware but
> these return an error, and rightly so. So maybe rename the functions and
> clk_ops instance to something like "clk_nodev_*" or "clk_missing_*"?

Hmm, this is more about a driver being removed rather than the device.
Then perhaps we could make it __clk_nodrv_* or clk_nodrv_* ?

>>  /**
>>   * 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_dummy_ops) {
>> +               pr_err("%s: unregistered clock: %s\n", __func__, clk->name);
>> +               goto out;
>> +       }
>> +       /*
>> +        * Assign dummy clock ops for consumers that might still hold
>> +        * a reference to this clock.
>> +        */
>> +       flags = clk_enable_lock();
>> +       clk->ops = &clk_dummy_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);
>> +       }
> 
> This looks pretty good. A remaining problem is re-loading the clock
> provider module will have string name conflicts with the old
> unregistered clocks (but not yet released) clocks during calls to
> __clk_lookup.

But the clock is being dropped from the clock tree immediately in this
function. After the hlist_del_init() call below the clock is not present
on any clocks list. Upon clock release only the memory allocated for
the clock is freed.

> The best solution would be to refactor clk.c to not use string name
> lookups but that is probably too big of an issue for the purpose of this
> series (but it will happen some day).
> 
> A short term solution would be to poison the clock's string name here.
> Reallocate the clk->name string with some poison data so that name
> conflicts don't occur. What do you think?

This shouldn't be necessary, for the reason described above. I've tested
multiple registrations when a clock was being referenced by a consumer
driver and it worked well.

I'm still a bit unsure about the kref reference counting, but I'd would
assume it is good to have. It prevents the kernel to crash in some
situations. Many other subsystems/drivers crash miserably when a driver
gets unbound using the sysfs "unbind" attribute. However, if it is assumed
that user space needs to keep track of respective resource references
and should know what it does when unbinding drivers, then we could probably
do without the kref. I'm seriously sceptical though about letting user
space to crash the kernel in fairly simple steps, it just doesn't sound
right.

> Regards,
> Mike
> 
>> +
>> +       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)
>> @@ -1861,6 +1973,7 @@ int __clk_get(struct clk *clk)
>>         if (!try_module_get(clk->owner))
>>                 return 0;
>>
>> +       kref_get(&clk->ref);
>>         return 1;
>>  }
>>  EXPORT_SYMBOL(__clk_get);
>> @@ -1870,6 +1983,10 @@ void __clk_put(struct clk *clk)
>>         if (!clk || IS_ERR(clk))
>>                 return;
>>
>> +       clk_prepare_lock();
>> +       kref_put(&clk->ref, __clk_release);
>> +       clk_prepare_unlock();
>> +
>>         module_put(clk->owner);
>>  }
>>  EXPORT_SYMBOL(__clk_put);

--
Regards,
Sylwester

[PATCH RFC 2/2] clk: implement clk_unregister

[Mike Turquette]

Quoting Sylwester Nawrocki (2013-08-19 11:04:33)
> On 08/16/2013 11:48 PM, Mike Turquette wrote:
> > Quoting Sylwester Nawrocki (2013-08-06 08:51:57)
> >> +/*
> >> + * 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_dummy_prepare_enable(struct clk_hw *hw)
> >> +{
> >> +       return -ENXIO;
> >> +}
> >> +
> >> +static void clk_dummy_disable_unprepare(struct clk_hw *hw)
> >> +{
> >> +       WARN_ON(1);
> >> +}
> >> +
> >> +static int clk_dummy_set_rate(struct clk_hw *hw, unsigned long rate,
> >> +                                       unsigned long parent_rate)
> >> +{
> >> +       return -ENXIO;
> >> +}
> >> +
> >> +static int clk_dummy_set_parent(struct clk_hw *hw, u8 index)
> >> +{
> >> +       return -ENXIO;
> >> +}
> >> +
> >> +static const struct clk_ops clk_dummy_ops = {
> >> +       .enable         = clk_dummy_prepare_enable,
> >> +       .disable        = clk_dummy_disable_unprepare,
> >> +       .prepare        = clk_dummy_prepare_enable,
> >> +       .unprepare      = clk_dummy_disable_unprepare,
> >> +       .set_rate       = clk_dummy_set_rate,
> >> +       .set_parent     = clk_dummy_set_parent,
> >> +};
> > 
> > Don't use "clk_dummy_*" here. The use of dummy often implies that
> > operations will return success in the absence of actual hardware but
> > these return an error, and rightly so. So maybe rename the functions and
> > clk_ops instance to something like "clk_nodev_*" or "clk_missing_*"?
> 
> Hmm, this is more about a driver being removed rather than the device.
> Then perhaps we could make it __clk_nodrv_* or clk_nodrv_* ?

clk_nodrv_* sounds good.

> 
> >>  /**
> >>   * 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_dummy_ops) {
> >> +               pr_err("%s: unregistered clock: %s\n", __func__, clk->name);
> >> +               goto out;
> >> +       }
> >> +       /*
> >> +        * Assign dummy clock ops for consumers that might still hold
> >> +        * a reference to this clock.
> >> +        */
> >> +       flags = clk_enable_lock();
> >> +       clk->ops = &clk_dummy_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);
> >> +       }
> > 
> > This looks pretty good. A remaining problem is re-loading the clock
> > provider module will have string name conflicts with the old
> > unregistered clocks (but not yet released) clocks during calls to
> > __clk_lookup.
> 
> But the clock is being dropped from the clock tree immediately in this
> function. After the hlist_del_init() call below the clock is not present
> on any clocks list. Upon clock release only the memory allocated for
> the clock is freed.

You are correct. Not sure why I thought that the clock being
unregistered was also getting pushed to the orphan list.

> 
> > The best solution would be to refactor clk.c to not use string name
> > lookups but that is probably too big of an issue for the purpose of this
> > series (but it will happen some day).
> > 
> > A short term solution would be to poison the clock's string name here.
> > Reallocate the clk->name string with some poison data so that name
> > conflicts don't occur. What do you think?
> 
> This shouldn't be necessary, for the reason described above. I've tested
> multiple registrations when a clock was being referenced by a consumer
> driver and it worked well.
> 
> I'm still a bit unsure about the kref reference counting, but I'd would
> assume it is good to have. It prevents the kernel to crash in some
> situations. Many other subsystems/drivers crash miserably when a driver
> gets unbound using the sysfs "unbind" attribute. However, if it is assumed
> that user space needs to keep track of respective resource references
> and should know what it does when unbinding drivers, then we could probably
> do without the kref. I'm seriously sceptical though about letting user
> space to crash the kernel in fairly simple steps, it just doesn't sound
> right.

Let's leave the kref bits in. If we can prove that they are unnecessary
in the future then they can always be removed.

This series looks good, barring the s/dummy/no_drv/ rename.  Russell's
ACK is needed for patch #1.

Regards,
Mike

> 
> > Regards,
> > Mike
> > 
> >> +
> >> +       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)
> >> @@ -1861,6 +1973,7 @@ int __clk_get(struct clk *clk)
> >>         if (!try_module_get(clk->owner))
> >>                 return 0;
> >>
> >> +       kref_get(&clk->ref);
> >>         return 1;
> >>  }
> >>  EXPORT_SYMBOL(__clk_get);
> >> @@ -1870,6 +1983,10 @@ void __clk_put(struct clk *clk)
> >>         if (!clk || IS_ERR(clk))
> >>                 return;
> >>
> >> +       clk_prepare_lock();
> >> +       kref_put(&clk->ref, __clk_release);
> >> +       clk_prepare_unlock();
> >> +
> >>         module_put(clk->owner);
> >>  }
> >>  EXPORT_SYMBOL(__clk_put);
> 
> --
> Regards,
> Sylwester