[PATCH] clk: fix self-consuming provider module pinning

[PATCH] clk: fix self-consuming provider module pinning

[Jerome Brunet]

clk_hw_get_clk() lets a provider get a struct clk for one of its own
struct clk_hw.

When a struct clk is created, the module usage count of the provider
is unconditionally increased. For a self-consuming provider, this means
it pins itself and the module can never be unloaded.

Increasing the module usage count should only be done when the consumer
lives in a different module from the provider. Use THIS_MODULE to
capture caller's module and increase the module usage count accordingly.

It is OK for consumer-only APIs such as clk_get() or of_clk_get() to
pass a NULL owner. As a result, any provider module will get pinned,
same as before.

Fixes: 30d6f8c15d2c ("clk: add api to get clk consumer from clk_hw")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
This issue has been present for a while. Virtually all users of
clk_hw_get_clk() are affected. The majority are compiled as builtins
according to the defconfigs. It is not problem in this case but it is
if the configuration is changed to module.

The following modules are using clk_hw_get_clk() and are compiled as
module with some shipped defconfigs:
* drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_pll.c
* drivers/phy/cadence/phy-cadence-sierra.c
* drivers/pwm/pwm-meson.c
* sound/soc/codecs/lpass-va-macro.c

Currently those module cannot be unloaded once they have been loaded.

"""
rmmod: ERROR: Module blabla-module is in use
"""

I initially thought about using the dev parameter and compare it
against the clock own device but this proved unreliable. A clock
does not always have a backing device and some paths, such as
clk_get_sys(), do not provide a device either.

With this applied, we can get back to removing the direct usage
of the struct clk in struct clk_hw and eventually remove this
struct member entirely.
---
 drivers/clk/clk.c            | 40 ++++++++++++++++++++++------------------
 drivers/clk/clk.h            |  5 +++--
 drivers/clk/clkdev.c         |  4 ++--
 include/linux/clk-provider.h | 17 ++++++++++++++++-
 4 files changed, 43 insertions(+), 23 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 048adfa86a5d..519160f0bb8d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -115,6 +115,7 @@ struct clk {
 	unsigned long max_rate;
 	unsigned int exclusive_count;
 	struct hlist_node clks_node;
+	bool pinning;
 };
 
 /***           runtime pm          ***/
@@ -4147,7 +4148,8 @@ static void free_clk(struct clk *clk)
  * used by the framework and clk provider respectively.
  */
 struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
-			      const char *dev_id, const char *con_id)
+			      const char *dev_id, const char *con_id,
+			      struct module *owner)
 {
 	struct clk *clk;
 	struct clk_core *core;
@@ -4162,7 +4164,13 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
 		return clk;
 	clk->dev = dev;
 
-	if (!try_module_get(core->owner)) {
+	/*
+	 * Pin the provider module only when the consumer lives in a different
+	 * module. A provider getting a clk from its own clk_hw would otherwise
+	 * pin itself and could never be unloaded.
+	 */
+	clk->pinning = owner != core->owner;
+	if (clk->pinning && !try_module_get(core->owner)) {
 		free_clk(clk);
 		return ERR_PTR(-ENOENT);
 	}
@@ -4173,24 +4181,19 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
 	return clk;
 }
 
-/**
- * clk_hw_get_clk - get clk consumer given an clk_hw
- * @hw: clk_hw associated with the clk being consumed
- * @con_id: connection ID string on device
- *
- * Returns: new clk consumer
- * This is the function to be used by providers which need
- * to get a consumer clk and act on the clock element
- * Calls to this function must be balanced with calls clk_put()
+/*
+ * Internal helper backing the clk_hw_get_clk() macro, which passes the caller's
+ * module via THIS_MODULE.
  */
-struct clk *clk_hw_get_clk(struct clk_hw *hw, const char *con_id)
+struct clk *__clk_hw_get_clk(struct clk_hw *hw, const char *con_id,
+			     struct module *owner)
 {
 	struct device *dev = hw->core->dev;
 	const char *name = dev ? dev_name(dev) : NULL;
 
-	return clk_hw_create_clk(dev, hw, name, con_id);
+	return clk_hw_create_clk(dev, hw, name, con_id, owner);
 }
-EXPORT_SYMBOL(clk_hw_get_clk);
+EXPORT_SYMBOL(__clk_hw_get_clk);
 
 static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
 {
@@ -4712,7 +4715,7 @@ struct clk *devm_clk_hw_get_clk(struct device *dev, struct clk_hw *hw,
 	if (!clkp)
 		return ERR_PTR(-ENOMEM);
 
-	clk = clk_hw_get_clk(hw, con_id);
+	clk = __clk_hw_get_clk(hw, con_id, dev->driver->owner);
 	if (!IS_ERR(clk)) {
 		*clkp = clk;
 		devres_add(dev, clkp);
@@ -4759,7 +4762,8 @@ void __clk_put(struct clk *clk)
 
 	owner = clk->core->owner;
 	kref_put(&clk->core->ref, __clk_release);
-	module_put(owner);
+	if (clk->pinning)
+		module_put(owner);
 	free_clk(clk);
 }
 
@@ -5280,7 +5284,7 @@ struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
 {
 	struct clk_hw *hw = of_clk_get_hw_from_clkspec(clkspec);
 
-	return clk_hw_create_clk(NULL, hw, NULL, __func__);
+	return clk_hw_create_clk(NULL, hw, NULL, __func__, NULL);
 }
 EXPORT_SYMBOL_GPL(of_clk_get_from_provider);
 
@@ -5307,7 +5311,7 @@ static struct clk *__of_clk_get(struct device_node *np,
 {
 	struct clk_hw *hw = of_clk_get_hw(np, index, con_id);
 
-	return clk_hw_create_clk(NULL, hw, dev_id, con_id);
+	return clk_hw_create_clk(NULL, hw, dev_id, con_id, NULL);
 }
 
 struct clk *of_clk_get(struct device_node *np, int index)
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 2d801900cad5..5009380863a9 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -23,13 +23,14 @@ struct clk_hw *clk_find_hw(const char *dev_id, const char *con_id);
 
 #ifdef CONFIG_COMMON_CLK
 struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
-			      const char *dev_id, const char *con_id);
+			      const char *dev_id, const char *con_id,
+			      struct module *owner);
 void __clk_put(struct clk *clk);
 #else
 /* All these casts to avoid ifdefs in clkdev... */
 static inline struct clk *
 clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id,
-		  const char *con_id)
+		  const char *con_id, struct module *owner)
 {
 	return (struct clk *)hw;
 }
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index abaa0f9e0083..919fc680ae4d 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -88,7 +88,7 @@ static struct clk *__clk_get_sys(struct device *dev, const char *dev_id,
 {
 	struct clk_hw *hw = clk_find_hw(dev_id, con_id);
 
-	return clk_hw_create_clk(dev, hw, dev_id, con_id);
+	return clk_hw_create_clk(dev, hw, dev_id, con_id, NULL);
 }
 
 struct clk *clk_get_sys(const char *dev_id, const char *con_id)
@@ -105,7 +105,7 @@ struct clk *clk_get(struct device *dev, const char *con_id)
 	if (dev && dev->of_node) {
 		hw = of_clk_get_hw(dev->of_node, 0, con_id);
 		if (!IS_ERR(hw) || PTR_ERR(hw) == -EPROBE_DEFER)
-			return clk_hw_create_clk(dev, hw, dev_id, con_id);
+			return clk_hw_create_clk(dev, hw, dev_id, con_id, NULL);
 	}
 
 	return __clk_get_sys(dev, dev_id, con_id);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index b01a38fef8cf..22e131b8c97b 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -37,6 +37,7 @@ struct clk;
 struct clk_hw;
 struct clk_core;
 struct dentry;
+struct module;
 
 /**
  * struct clk_rate_request - Structure encoding the clk constraints that
@@ -1401,10 +1402,24 @@ static inline struct clk_hw *__clk_get_hw(struct clk *clk)
 }
 #endif
 
-struct clk *clk_hw_get_clk(struct clk_hw *hw, const char *con_id);
+struct clk *__clk_hw_get_clk(struct clk_hw *hw, const char *con_id,
+			     struct module *owner);
 struct clk *devm_clk_hw_get_clk(struct device *dev, struct clk_hw *hw,
 				const char *con_id);
 
+/**
+ * clk_hw_get_clk - get clk consumer given a clk_hw
+ * @hw: clk_hw associated with the clk being consumed
+ * @con_id: connection ID string on device
+ *
+ * Return: new clk consumer
+ * This is the function to be used by providers which need
+ * to get a consumer clk and act on the clock element
+ * Calls to this function must be balanced with calls to clk_put()
+ */
+#define clk_hw_get_clk(hw, con_id) \
+	__clk_hw_get_clk((hw), (con_id), THIS_MODULE)
+
 unsigned int clk_hw_get_num_parents(const struct clk_hw *hw);
 struct clk_hw *clk_hw_get_parent(const struct clk_hw *hw);
 struct clk_hw *clk_hw_get_parent_by_index(const struct clk_hw *hw,

---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260721-clk-provider-pinning-123741b285da

Best regards,
--  
Jerome

Re: [PATCH] clk: fix self-consuming provider module pinning

[Brian Masney]

Hi Jerome,

On Tue, Jul 21, 2026 at 12:08:17PM +0200, Jerome Brunet wrote:
> clk_hw_get_clk() lets a provider get a struct clk for one of its own
> struct clk_hw.
> 
> When a struct clk is created, the module usage count of the provider
> is unconditionally increased. For a self-consuming provider, this means
> it pins itself and the module can never be unloaded.
> 
> Increasing the module usage count should only be done when the consumer
> lives in a different module from the provider. Use THIS_MODULE to
> capture caller's module and increase the module usage count accordingly.
> 
> It is OK for consumer-only APIs such as clk_get() or of_clk_get() to
> pass a NULL owner. As a result, any provider module will get pinned,
> same as before.
> 
> Fixes: 30d6f8c15d2c ("clk: add api to get clk consumer from clk_hw")
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
> This issue has been present for a while. Virtually all users of
> clk_hw_get_clk() are affected. The majority are compiled as builtins
> according to the defconfigs. It is not problem in this case but it is
> if the configuration is changed to module.
> 
> The following modules are using clk_hw_get_clk() and are compiled as
> module with some shipped defconfigs:
> * drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_pll.c
> * drivers/phy/cadence/phy-cadence-sierra.c
> * drivers/pwm/pwm-meson.c
> * sound/soc/codecs/lpass-va-macro.c
> 
> Currently those module cannot be unloaded once they have been loaded.
> 
> """
> rmmod: ERROR: Module blabla-module is in use
> """
> 
> I initially thought about using the dev parameter and compare it
> against the clock own device but this proved unreliable. A clock
> does not always have a backing device and some paths, such as
> clk_get_sys(), do not provide a device either.
> 
> With this applied, we can get back to removing the direct usage
> of the struct clk in struct clk_hw and eventually remove this
> struct member entirely.

__clk_register() has this comment:

        /*
         * Don't call clk_hw_create_clk() here because that would pin the
         * provider module to itself and prevent it from ever being removed.
         */
        hw->clk = alloc_clk(core, NULL, NULL);

With your change, can this code path be updated to use clk_hw_create_clk() ?

Brian

Re: [PATCH] clk: fix self-consuming provider module pinning

[Jerome Brunet]

On mar. 21 juil. 2026 at 18:48, Brian Masney <bmasney@redhat.com> wrote:

> Hi Jerome,
>
> On Tue, Jul 21, 2026 at 12:08:17PM +0200, Jerome Brunet wrote:
>> clk_hw_get_clk() lets a provider get a struct clk for one of its own
>> struct clk_hw.
>> 
>> When a struct clk is created, the module usage count of the provider
>> is unconditionally increased. For a self-consuming provider, this means
>> it pins itself and the module can never be unloaded.
>> 
>> Increasing the module usage count should only be done when the consumer
>> lives in a different module from the provider. Use THIS_MODULE to
>> capture caller's module and increase the module usage count accordingly.
>> 
>> It is OK for consumer-only APIs such as clk_get() or of_clk_get() to
>> pass a NULL owner. As a result, any provider module will get pinned,
>> same as before.
>> 
>> Fixes: 30d6f8c15d2c ("clk: add api to get clk consumer from clk_hw")
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>> This issue has been present for a while. Virtually all users of
>> clk_hw_get_clk() are affected. The majority are compiled as builtins
>> according to the defconfigs. It is not problem in this case but it is
>> if the configuration is changed to module.
>> 
>> The following modules are using clk_hw_get_clk() and are compiled as
>> module with some shipped defconfigs:
>> * drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_pll.c
>> * drivers/phy/cadence/phy-cadence-sierra.c
>> * drivers/pwm/pwm-meson.c
>> * sound/soc/codecs/lpass-va-macro.c
>> 
>> Currently those module cannot be unloaded once they have been loaded.
>> 
>> """
>> rmmod: ERROR: Module blabla-module is in use
>> """
>> 
>> I initially thought about using the dev parameter and compare it
>> against the clock own device but this proved unreliable. A clock
>> does not always have a backing device and some paths, such as
>> clk_get_sys(), do not provide a device either.
>> 
>> With this applied, we can get back to removing the direct usage
>> of the struct clk in struct clk_hw and eventually remove this
>> struct member entirely.
>
> __clk_register() has this comment:
>
>         /*
>          * Don't call clk_hw_create_clk() here because that would pin the
>          * provider module to itself and prevent it from ever being removed.
>          */
>         hw->clk = alloc_clk(core, NULL, NULL);
>
> With your change, can this code path be updated to use clk_hw_create_clk() ?
>

Indeed the comment no longer applies but I think keeping alloc
here is best. It avoids "linking a consumer" here where there should not
be one. All clocks would appear to have consumer in the clk summary, it
would weird. 

The end goal is remove the hw->clk. I've restarted the work to rid the
kernel of using this. Some rework don't need clk_hw_create_clk() and are
not blocked by this patch but most will.

Once this is done, we could probably merge clk_hw_create_clk() and
alloc(), the split won't have purpose anymore.

> Brian

-- 
Jerome

Re: [PATCH] clk: fix self-consuming provider module pinning

[Brian Masney]

Hi Jerome,

On Wed, Jul 22, 2026 at 09:49:33AM +0200, Jerome Brunet wrote:
> On mar. 21 juil. 2026 at 18:48, Brian Masney <bmasney@redhat.com> wrote:
> > On Tue, Jul 21, 2026 at 12:08:17PM +0200, Jerome Brunet wrote:
> >> clk_hw_get_clk() lets a provider get a struct clk for one of its own
> >> struct clk_hw.
> >> 
> >> When a struct clk is created, the module usage count of the provider
> >> is unconditionally increased. For a self-consuming provider, this means
> >> it pins itself and the module can never be unloaded.
> >> 
> >> Increasing the module usage count should only be done when the consumer
> >> lives in a different module from the provider. Use THIS_MODULE to
> >> capture caller's module and increase the module usage count accordingly.
> >> 
> >> It is OK for consumer-only APIs such as clk_get() or of_clk_get() to
> >> pass a NULL owner. As a result, any provider module will get pinned,
> >> same as before.
> >> 
> >> Fixes: 30d6f8c15d2c ("clk: add api to get clk consumer from clk_hw")
> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >> ---
> >> This issue has been present for a while. Virtually all users of
> >> clk_hw_get_clk() are affected. The majority are compiled as builtins
> >> according to the defconfigs. It is not problem in this case but it is
> >> if the configuration is changed to module.
> >> 
> >> The following modules are using clk_hw_get_clk() and are compiled as
> >> module with some shipped defconfigs:
> >> * drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_pll.c
> >> * drivers/phy/cadence/phy-cadence-sierra.c
> >> * drivers/pwm/pwm-meson.c
> >> * sound/soc/codecs/lpass-va-macro.c
> >> 
> >> Currently those module cannot be unloaded once they have been loaded.
> >> 
> >> """
> >> rmmod: ERROR: Module blabla-module is in use
> >> """
> >> 
> >> I initially thought about using the dev parameter and compare it
> >> against the clock own device but this proved unreliable. A clock
> >> does not always have a backing device and some paths, such as
> >> clk_get_sys(), do not provide a device either.
> >> 
> >> With this applied, we can get back to removing the direct usage
> >> of the struct clk in struct clk_hw and eventually remove this
> >> struct member entirely.
> >
> > __clk_register() has this comment:
> >
> >         /*
> >          * Don't call clk_hw_create_clk() here because that would pin the
> >          * provider module to itself and prevent it from ever being removed.
> >          */
> >         hw->clk = alloc_clk(core, NULL, NULL);
> >
> > With your change, can this code path be updated to use clk_hw_create_clk() ?
> >
> 
> Indeed the comment no longer applies but I think keeping alloc
> here is best. It avoids "linking a consumer" here where there should not
> be one. All clocks would appear to have consumer in the clk summary, it
> would weird. 
> 
> The end goal is remove the hw->clk. I've restarted the work to rid the
> kernel of using this. Some rework don't need clk_hw_create_clk() and are
> not blocked by this patch but most will.
> 
> Once this is done, we could probably merge clk_hw_create_clk() and
> alloc(), the split won't have purpose anymore.

That all sounds good.

I know that Stephen likes to see any changes that are made to clk.c have
associated kunit tests to avoid any breakage in the clk core. There's
already a bunch of tests in clk_test.c.

Brian

Re: [PATCH] clk: fix self-consuming provider module pinning

[Jerome Brunet]

On mer. 22 juil. 2026 at 08:32, Brian Masney <bmasney@redhat.com> wrote:

> Hi Jerome,
>
> On Wed, Jul 22, 2026 at 09:49:33AM +0200, Jerome Brunet wrote:
>> On mar. 21 juil. 2026 at 18:48, Brian Masney <bmasney@redhat.com> wrote:
>> > On Tue, Jul 21, 2026 at 12:08:17PM +0200, Jerome Brunet wrote:
>> >> clk_hw_get_clk() lets a provider get a struct clk for one of its own
>> >> struct clk_hw.
>> >> 
>> >> When a struct clk is created, the module usage count of the provider
>> >> is unconditionally increased. For a self-consuming provider, this means
>> >> it pins itself and the module can never be unloaded.
>> >> 
>> >> Increasing the module usage count should only be done when the consumer
>> >> lives in a different module from the provider. Use THIS_MODULE to
>> >> capture caller's module and increase the module usage count accordingly.
>> >> 
>> >> It is OK for consumer-only APIs such as clk_get() or of_clk_get() to
>> >> pass a NULL owner. As a result, any provider module will get pinned,
>> >> same as before.
>> >> 
>> >> Fixes: 30d6f8c15d2c ("clk: add api to get clk consumer from clk_hw")
>> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> >> ---
>> >> This issue has been present for a while. Virtually all users of
>> >> clk_hw_get_clk() are affected. The majority are compiled as builtins
>> >> according to the defconfigs. It is not problem in this case but it is
>> >> if the configuration is changed to module.
>> >> 
>> >> The following modules are using clk_hw_get_clk() and are compiled as
>> >> module with some shipped defconfigs:
>> >> * drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_pll.c
>> >> * drivers/phy/cadence/phy-cadence-sierra.c
>> >> * drivers/pwm/pwm-meson.c
>> >> * sound/soc/codecs/lpass-va-macro.c
>> >> 
>> >> Currently those module cannot be unloaded once they have been loaded.
>> >> 
>> >> """
>> >> rmmod: ERROR: Module blabla-module is in use
>> >> """
>> >> 
>> >> I initially thought about using the dev parameter and compare it
>> >> against the clock own device but this proved unreliable. A clock
>> >> does not always have a backing device and some paths, such as
>> >> clk_get_sys(), do not provide a device either.
>> >> 
>> >> With this applied, we can get back to removing the direct usage
>> >> of the struct clk in struct clk_hw and eventually remove this
>> >> struct member entirely.
>> >
>> > __clk_register() has this comment:
>> >
>> >         /*
>> >          * Don't call clk_hw_create_clk() here because that would pin the
>> >          * provider module to itself and prevent it from ever being removed.
>> >          */
>> >         hw->clk = alloc_clk(core, NULL, NULL);
>> >
>> > With your change, can this code path be updated to use clk_hw_create_clk() ?
>> >
>> 
>> Indeed the comment no longer applies but I think keeping alloc
>> here is best. It avoids "linking a consumer" here where there should not
>> be one. All clocks would appear to have consumer in the clk summary, it
>> would weird. 
>> 
>> The end goal is remove the hw->clk. I've restarted the work to rid the
>> kernel of using this. Some rework don't need clk_hw_create_clk() and are
>> not blocked by this patch but most will.
>> 
>> Once this is done, we could probably merge clk_hw_create_clk() and
>> alloc(), the split won't have purpose anymore.
>
> That all sounds good.
>
> I know that Stephen likes to see any changes that are made to clk.c have
> associated kunit tests to avoid any breakage in the clk core. There's
> already a bunch of tests in clk_test.c.

I know. What I'm fixing here is a bit odd to test though.

The way I did it was to make a dummy module that registered a clock and
requested the consumer pointer from it. Test pass if you can rmmod it.

Not sure how that would translate to kunit to be honest

>
> Brian

-- 
Jerome