[RFC PATCH] clk: rework clk_register to use the clk_hw API

[RFC PATCH] clk: rework clk_register to use the clk_hw API

[Jerome Brunet]

This rework the clk_register/unregister functions to use the clk_hw API.
The goal is to pave the way for the removal of the 'clk' member in
struct clk_hw.

This member is used in about 60 drivers, most of them in drivers/clk/.
Some cases will be trivial to remove but some drivers are hacking around
it and will be tougher to deal with.

This change is sent as an RFC because, until there is a clear plan to deal
with drivers above, there is memory penality when using clk_register()
(struct clk is allocated in __clk_hw_register() and clk_register())

Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c | 110 ++++++++++++++++++++++++++--------------------
 1 file changed, 62 insertions(+), 48 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d9946e192cbc..d93062673b2c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3736,8 +3736,8 @@ static void clk_core_free_parent_map(struct clk_core *core)
 	kfree(core->parents);
 }
 
-static struct clk *
-__clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
+static int
+__clk_hw_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 {
 	int ret;
 	struct clk_core *core;
@@ -3801,7 +3801,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 
 	ret = __clk_core_init(core);
 	if (!ret)
-		return hw->clk;
+		return 0;
 
 	clk_prepare_lock();
 	clk_core_unlink_consumer(hw->clk);
@@ -3818,7 +3818,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 fail_name:
 	kfree(core);
 fail_out:
-	return ERR_PTR(ret);
+	return ret;
 }
 
 /**
@@ -3843,6 +3843,24 @@ static struct device_node *dev_or_parent_of_node(struct device *dev)
 	return np;
 }
 
+
+/**
+ * clk_hw_register - register a clk_hw and return an error code
+ * @dev: device that is registering this clock
+ * @hw: link to hardware-specific clock data
+ *
+ * clk_hw_register is the primary interface for populating the clock tree with
+ * new clock nodes. It returns an integer equal to zero indicating success or
+ * less than zero indicating failure. Drivers must test for an error code after
+ * calling clk_hw_register().
+ */
+int clk_hw_register(struct device *dev, struct clk_hw *hw)
+{
+	return __clk_hw_register(dev, dev_or_parent_of_node(dev),
+				 hw);
+}
+EXPORT_SYMBOL_GPL(clk_hw_register);
+
 /**
  * clk_register - allocate a new clock, register it and return an opaque cookie
  * @dev: device that is registering this clock
@@ -3858,26 +3876,19 @@ static struct device_node *dev_or_parent_of_node(struct device *dev)
  */
 struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 {
-	return __clk_register(dev, dev_or_parent_of_node(dev), hw);
-}
-EXPORT_SYMBOL_GPL(clk_register);
+	struct clk *clk;
+	int ret = clk_hw_register(dev, hw);
 
-/**
- * clk_hw_register - register a clk_hw and return an error code
- * @dev: device that is registering this clock
- * @hw: link to hardware-specific clock data
- *
- * clk_hw_register is the primary interface for populating the clock tree with
- * new clock nodes. It returns an integer equal to zero indicating success or
- * less than zero indicating failure. Drivers must test for an error code after
- * calling clk_hw_register().
- */
-int clk_hw_register(struct device *dev, struct clk_hw *hw)
-{
-	return PTR_ERR_OR_ZERO(__clk_register(dev, dev_or_parent_of_node(dev),
-			       hw));
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	clk = clk_hw_get_clk(hw);
+	if (IS_ERR_OR_NULL(clk))
+		clk_hw_unregister(hw);
+
+	return clk;
 }
-EXPORT_SYMBOL_GPL(clk_hw_register);
+EXPORT_SYMBOL_GPL(clk_register);
 
 /*
  * of_clk_hw_register - register a clk_hw and return an error code
@@ -3892,7 +3903,7 @@ EXPORT_SYMBOL_GPL(clk_hw_register);
  */
 int of_clk_hw_register(struct device_node *node, struct clk_hw *hw)
 {
-	return PTR_ERR_OR_ZERO(__clk_register(NULL, node, hw));
+	return __clk_hw_register(NULL, node, hw);
 }
 EXPORT_SYMBOL_GPL(of_clk_hw_register);
 
@@ -3972,25 +3983,25 @@ static void clk_core_evict_parent_cache(struct clk_core *core)
 }
 
 /**
- * clk_unregister - unregister a currently registered clock
- * @clk: clock to unregister
+ * clk_hw_unregister - unregister a currently registered clk_hw
+ * @hw: hardware-specific clock data to unregister
  */
-void clk_unregister(struct clk *clk)
+void clk_hw_unregister(struct clk_hw *hw)
 {
 	unsigned long flags;
 	const struct clk_ops *ops;
 
-	if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
+	if (!hw || WARN_ON_ONCE(IS_ERR(hw)))
 		return;
 
-	clk_debug_unregister(clk->core);
+	clk_debug_unregister(hw->core);
 
 	clk_prepare_lock();
 
-	ops = clk->core->ops;
+	ops = hw->core->ops;
 	if (ops == &clk_nodrv_ops) {
 		pr_err("%s: unregistered clock: %s\n", __func__,
-		       clk->core->name);
+		       hw->core->name);
 		goto unlock;
 	}
 	/*
@@ -3998,50 +4009,53 @@ void clk_unregister(struct clk *clk)
 	 * a reference to this clock.
 	 */
 	flags = clk_enable_lock();
-	clk->core->ops = &clk_nodrv_ops;
+	hw->core->ops = &clk_nodrv_ops;
 	clk_enable_unlock(flags);
 
 	if (ops->terminate)
-		ops->terminate(clk->core->hw);
+		ops->terminate(hw);
 
-	if (!hlist_empty(&clk->core->children)) {
+	if (!hlist_empty(&hw->core->children)) {
 		struct clk_core *child;
 		struct hlist_node *t;
 
 		/* Reparent all children to the orphan list. */
-		hlist_for_each_entry_safe(child, t, &clk->core->children,
+		hlist_for_each_entry_safe(child, t, &hw->core->children,
 					  child_node)
 			clk_core_set_parent_nolock(child, NULL);
 	}
 
-	clk_core_evict_parent_cache(clk->core);
+	clk_core_evict_parent_cache(hw->core);
 
-	hlist_del_init(&clk->core->child_node);
+	hlist_del_init(&hw->core->child_node);
 
-	if (clk->core->prepare_count)
+	if (hw->core->prepare_count)
 		pr_warn("%s: unregistering prepared clock: %s\n",
-					__func__, clk->core->name);
+					__func__, hw->core->name);
 
-	if (clk->core->protect_count)
+	if (hw->core->protect_count)
 		pr_warn("%s: unregistering protected clock: %s\n",
-					__func__, clk->core->name);
+					__func__, hw->core->name);
 
-	kref_put(&clk->core->ref, __clk_release);
-	free_clk(clk);
+	kref_put(&hw->core->ref, __clk_release);
+	free_clk(hw->clk);
 unlock:
 	clk_prepare_unlock();
 }
-EXPORT_SYMBOL_GPL(clk_unregister);
+EXPORT_SYMBOL_GPL(clk_hw_unregister);
 
 /**
- * clk_hw_unregister - unregister a currently registered clk_hw
- * @hw: hardware-specific clock data to unregister
+ * clk_unregister - unregister a currently registered clock
+ * @clk: clock to unregister
  */
-void clk_hw_unregister(struct clk_hw *hw)
+void clk_unregister(struct clk *clk)
 {
-	clk_unregister(hw->clk);
+	struct clk_hw *hw = clk->core->hw;
+
+	clk_put(clk);
+	clk_hw_unregister(hw);
 }
-EXPORT_SYMBOL_GPL(clk_hw_unregister);
+EXPORT_SYMBOL_GPL(clk_unregister);
 
 static void devm_clk_release(struct device *dev, void *res)
 {
-- 
2.25.4

Re: [RFC PATCH] clk: rework clk_register to use the clk_hw API

[kbuild test robot]

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

Hi Jerome,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on clk/clk-next]
[also build test WARNING on v5.7-rc6 next-20200519]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Jerome-Brunet/clk-rework-clk_register-to-use-the-clk_hw-API/20200520-011000
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: x86_64-defconfig (attached as .config)

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

drivers/clk/clk.c: In function 'clk_register':
drivers/clk/clk.c:3868:8: error: implicit declaration of function 'clk_hw_get_clk'; did you mean '__clk_hw_set_clk'? [-Werror=implicit-function-declaration]
clk = clk_hw_get_clk(hw);
^~~~~~~~~~~~~~
__clk_hw_set_clk
>> drivers/clk/clk.c:3868:6: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
clk = clk_hw_get_clk(hw);
^
cc1: some warnings being treated as errors

vim +3868 drivers/clk/clk.c

  3846	
  3847	/**
  3848	 * clk_register - allocate a new clock, register it and return an opaque cookie
  3849	 * @dev: device that is registering this clock
  3850	 * @hw: link to hardware-specific clock data
  3851	 *
  3852	 * clk_register is the *deprecated* interface for populating the clock tree with
  3853	 * new clock nodes. Use clk_hw_register() instead.
  3854	 *
  3855	 * Returns: a pointer to the newly allocated struct clk which
  3856	 * cannot be dereferenced by driver code but may be used in conjunction with the
  3857	 * rest of the clock API.  In the event of an error clk_register will return an
  3858	 * error code; drivers must test for an error code after calling clk_register.
  3859	 */
  3860	struct clk *clk_register(struct device *dev, struct clk_hw *hw)
  3861	{
  3862		struct clk *clk;
  3863		int ret = clk_hw_register(dev, hw);
  3864	
  3865		if (ret < 0)
  3866			return ERR_PTR(ret);
  3867	
> 3868		clk = clk_hw_get_clk(hw);
  3869		if (IS_ERR_OR_NULL(clk))
  3870			clk_hw_unregister(hw);
  3871	
  3872		return clk;
  3873	}
  3874	EXPORT_SYMBOL_GPL(clk_register);
  3875	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 29095 bytes --]

Re: [RFC PATCH] clk: rework clk_register to use the clk_hw API

[kbuild test robot]

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

Hi Jerome,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on clk/clk-next]
[also build test ERROR on v5.7-rc6 next-20200519]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Jerome-Brunet/clk-rework-clk_register-to-use-the-clk_hw-API/20200520-011000
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: arm-randconfig-r033-20200519 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 135b877874fae96b4372c8a3fbfaa8ff44ff86e3)
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/clk/clk.c:3868:8: error: implicit declaration of function 'clk_hw_get_clk' [-Werror,-Wimplicit-function-declaration]
clk = clk_hw_get_clk(hw);
^
drivers/clk/clk.c:3868:8: note: did you mean '__clk_hw_set_clk'?
include/linux/clk-provider.h:1121:20: note: '__clk_hw_set_clk' declared here
static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
^
>> drivers/clk/clk.c:3868:6: warning: incompatible integer to pointer conversion assigning to 'struct clk *' from 'int' [-Wint-conversion]
clk = clk_hw_get_clk(hw);
^ ~~~~~~~~~~~~~~~~~~
1 warning and 1 error generated.

vim +/clk_hw_get_clk +3868 drivers/clk/clk.c

  3846	
  3847	/**
  3848	 * clk_register - allocate a new clock, register it and return an opaque cookie
  3849	 * @dev: device that is registering this clock
  3850	 * @hw: link to hardware-specific clock data
  3851	 *
  3852	 * clk_register is the *deprecated* interface for populating the clock tree with
  3853	 * new clock nodes. Use clk_hw_register() instead.
  3854	 *
  3855	 * Returns: a pointer to the newly allocated struct clk which
  3856	 * cannot be dereferenced by driver code but may be used in conjunction with the
  3857	 * rest of the clock API.  In the event of an error clk_register will return an
  3858	 * error code; drivers must test for an error code after calling clk_register.
  3859	 */
  3860	struct clk *clk_register(struct device *dev, struct clk_hw *hw)
  3861	{
  3862		struct clk *clk;
  3863		int ret = clk_hw_register(dev, hw);
  3864	
  3865		if (ret < 0)
  3866			return ERR_PTR(ret);
  3867	
> 3868		clk = clk_hw_get_clk(hw);
  3869		if (IS_ERR_OR_NULL(clk))
  3870			clk_hw_unregister(hw);
  3871	
  3872		return clk;
  3873	}
  3874	EXPORT_SYMBOL_GPL(clk_register);
  3875	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 28007 bytes --]

Re: [RFC PATCH] clk: rework clk_register to use the clk_hw API

[kbuild test robot]

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

Hi Jerome,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on clk/clk-next]
[also build test ERROR on v5.7-rc6 next-20200519]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Jerome-Brunet/clk-rework-clk_register-to-use-the-clk_hw-API/20200520-011000
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: i386-debian-10.3 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

drivers/clk/clk.c: In function 'clk_register':
>> drivers/clk/clk.c:3868:8: error: implicit declaration of function 'clk_hw_get_clk'; did you mean '__clk_hw_set_clk'? [-Werror=implicit-function-declaration]
clk = clk_hw_get_clk(hw);
^~~~~~~~~~~~~~
__clk_hw_set_clk
drivers/clk/clk.c:3868:6: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
clk = clk_hw_get_clk(hw);
^
cc1: some warnings being treated as errors

vim +3868 drivers/clk/clk.c

  3846	
  3847	/**
  3848	 * clk_register - allocate a new clock, register it and return an opaque cookie
  3849	 * @dev: device that is registering this clock
  3850	 * @hw: link to hardware-specific clock data
  3851	 *
  3852	 * clk_register is the *deprecated* interface for populating the clock tree with
  3853	 * new clock nodes. Use clk_hw_register() instead.
  3854	 *
  3855	 * Returns: a pointer to the newly allocated struct clk which
  3856	 * cannot be dereferenced by driver code but may be used in conjunction with the
  3857	 * rest of the clock API.  In the event of an error clk_register will return an
  3858	 * error code; drivers must test for an error code after calling clk_register.
  3859	 */
  3860	struct clk *clk_register(struct device *dev, struct clk_hw *hw)
  3861	{
  3862		struct clk *clk;
  3863		int ret = clk_hw_register(dev, hw);
  3864	
  3865		if (ret < 0)
  3866			return ERR_PTR(ret);
  3867	
> 3868		clk = clk_hw_get_clk(hw);
  3869		if (IS_ERR_OR_NULL(clk))
  3870			clk_hw_unregister(hw);
  3871	
  3872		return clk;
  3873	}
  3874	EXPORT_SYMBOL_GPL(clk_register);
  3875	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 34525 bytes --]

Re: [RFC PATCH] clk: rework clk_register to use the clk_hw API

[Stephen Boyd]

Quoting Jerome Brunet (2020-05-19 10:07:33)
> This rework the clk_register/unregister functions to use the clk_hw API.
> The goal is to pave the way for the removal of the 'clk' member in
> struct clk_hw.
> 
> This member is used in about 60 drivers, most of them in drivers/clk/.
> Some cases will be trivial to remove but some drivers are hacking around
> it and will be tougher to deal with.
> 
> This change is sent as an RFC because, until there is a clear plan to deal
> with drivers above, there is memory penality when using clk_register()
> (struct clk is allocated in __clk_hw_register() and clk_register())
> 
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

Do you plan to resend this? I'm inclined to just apply it and we can
sort out the memory penalty problem later. I'm not even sure what the
penalty is quite honestly. Drivers can call the clk_hw_get_clk() API you
introduced and generate clk pointer so we should be able to fully hide
the clk generated in clk_register() from them and not even allocate it?

Re: [RFC PATCH] clk: rework clk_register to use the clk_hw API

[Jerome Brunet]

On Sat 14 Nov 2020 at 22:02, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Jerome Brunet (2020-05-19 10:07:33)
>> This rework the clk_register/unregister functions to use the clk_hw API.
>> The goal is to pave the way for the removal of the 'clk' member in
>> struct clk_hw.
>> 
>> This member is used in about 60 drivers, most of them in drivers/clk/.
>> Some cases will be trivial to remove but some drivers are hacking around
>> it and will be tougher to deal with.
>> 
>> This change is sent as an RFC because, until there is a clear plan to deal
>> with drivers above, there is memory penality when using clk_register()
>> (struct clk is allocated in __clk_hw_register() and clk_register())
>> 
>> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>
> Do you plan to resend this? I'm inclined to just apply it and we can
> sort out the memory penalty problem later.

I had forgot about it :)
It does not apply so I'll fix this and resend

Could you publish an immutable branch with the clk_hw_get_clk(),
devm_clk_notifier and the meson stuff you have applied ?

I'd like to base my next PR on it since Kevin submitted a change which
depends on it. It will also help me rebase this change.

> I'm not even sure what the
> penalty is quite honestly. Drivers can call the clk_hw_get_clk() API you
> introduced and generate clk pointer so we should be able to fully hide
> the clk generated in clk_register() from them and not even allocate it?

Eventually, it is the idea and there would no penality anymore.
At the moment, a fair amount of "struct clk_hw" user rely on the ".clk"
member being present, even if it is bad. All those need modification
before we can kill the .clk member.

I was thinking of a progressive approach:
* Migrate clk_register()
* Nicely ask user the direct users ".clk" to update
* Once satisfied, remove ".clk"

Re: [RFC PATCH] clk: rework clk_register to use the clk_hw API

[Stephen Boyd]

Quoting Jerome Brunet (2020-11-16 07:13:59)
> 
> On Sat 14 Nov 2020 at 22:02, Stephen Boyd <sboyd@kernel.org> wrote:
> 
> > Quoting Jerome Brunet (2020-05-19 10:07:33)
> >> This rework the clk_register/unregister functions to use the clk_hw API.
> >> The goal is to pave the way for the removal of the 'clk' member in
> >> struct clk_hw.
> >> 
> >> This member is used in about 60 drivers, most of them in drivers/clk/.
> >> Some cases will be trivial to remove but some drivers are hacking around
> >> it and will be tougher to deal with.
> >> 
> >> This change is sent as an RFC because, until there is a clear plan to deal
> >> with drivers above, there is memory penality when using clk_register()
> >> (struct clk is allocated in __clk_hw_register() and clk_register())
> >> 
> >> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >
> > Do you plan to resend this? I'm inclined to just apply it and we can
> > sort out the memory penalty problem later.
> 
> I had forgot about it :)
> It does not apply so I'll fix this and resend

Cool. I forgot too, just found by searching your email :)

> 
> Could you publish an immutable branch with the clk_hw_get_clk(),
> devm_clk_notifier and the meson stuff you have applied ?

Yeah no problem! It's clk-hw branch.

> 
> > I'm not even sure what the
> > penalty is quite honestly. Drivers can call the clk_hw_get_clk() API you
> > introduced and generate clk pointer so we should be able to fully hide
> > the clk generated in clk_register() from them and not even allocate it?
> 
> Eventually, it is the idea and there would no penality anymore.
> At the moment, a fair amount of "struct clk_hw" user rely on the ".clk"
> member being present, even if it is bad. All those need modification
> before we can kill the .clk member.
> 
> I was thinking of a progressive approach:
> * Migrate clk_register()
> * Nicely ask user the direct users ".clk" to update
> * Once satisfied, remove ".clk"
> 

Sounds good to me.