Clock unregistration support in the common clock framework

drivers/clk/clk.c +24 -0

+/*
+ * 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;

drivers/clk/clk.c +21 -3

+
 /**
  * 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();

drivers/clk/clk.c +18 -0

+
+       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);

include/linux/clk-private.h +2 -1

index b7c0b58..36c1fc8 100644
@@ -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

Quoting Sylwester Nawrocki (2013-08-06 08:51:57)

Quoted 31 lines

drivers/clk/clk.c +31 -0

+/*
+ * 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,

drivers/clk/clk.c fail_out: · line 1857

Quoted 3 lines

drivers/clk/clk.c +3 -0

+       .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_*"?

Quoted 34 lines

/**
* clk_unregister - unregister a currently registered clock
* @clk: clock to unregister
- *
- * Currently unimplemented.
*/

drivers/clk/clk.c +27 -1

-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. */

drivers/clk/clk.c fail_out: · line 1892

Quoted 3 lines

drivers/clk/clk.c +3 -0

+               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

Quoted 28 lines

drivers/clk/clk.c +14 -0

+
+       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;
 

drivers/clk/clk.c void __clk_put(struct clk *clk) · line 1992

Quoted 7 lines

drivers/clk/clk.c +4 -0

+       clk_prepare_lock();
+       kref_put(&clk->ref, __clk_release);
+       clk_prepare_unlock();
+
        module_put(clk->owner);
 }
 EXPORT_SYMBOL(__clk_put);

Quoted 32 lines

Quoting Sylwester Nawrocki (2013-08-06 08:51:57)

Quoted 31 lines

drivers/clk/clk.c +31 -0

+/*
+ * 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,

Quoted 8 lines

drivers/clk/clk.c fail_out: · line 1857

Quoted 3 lines

drivers/clk/clk.c +3 -0

+       .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_* ?

Quoted 35 lines

Quoted 35 lines

/**
* clk_unregister - unregister a currently registered clock
* @clk: clock to unregister
- *
- * Currently unimplemented.
*/

drivers/clk/clk.c +27 -1

-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. */

Quoted 8 lines

drivers/clk/clk.c fail_out: · line 1892

Quoted 3 lines

drivers/clk/clk.c +3 -0

+               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.

Quoted 8 lines

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.

Quoted 32 lines

Regards,
Mike

Quoted 28 lines

drivers/clk/clk.c +14 -0

+
+       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;
 

Quoted 7 lines

drivers/clk/clk.c void __clk_put(struct clk *clk) · line 1992

Quoted 7 lines

drivers/clk/clk.c +4 -0

+       clk_prepare_lock();
+       kref_put(&clk->ref, __clk_release);
+       clk_prepare_unlock();
+
        module_put(clk->owner);
 }
 EXPORT_SYMBOL(__clk_put);

--
Regards,
Sylwester