[PATCH] clk: change clk_hw_create_clk() to avoid being unable to remove module

[PATCH] clk: change clk_hw_create_clk() to avoid being unable to remove module

[Heiner Kallweit]

With clk_hw_create_clk() we have the problem that module unloading
is impossible if consumer and provider module owner are the same and
refcount is incremented. See also following comment in __clk_register().

/*
 * Don't call clk_hw_create_clk() here because that would pin the
 * provider module to itself and prevent it from ever being removed.
 */

I think this also affects any usage of clk_hw_get_clk(). To deal with
this let's increment the refcount only if owners are different.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/clk/clk.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 27c30a533..e9bf961d4 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -103,6 +103,7 @@ struct clk {
 	unsigned long max_rate;
 	unsigned int exclusive_count;
 	struct hlist_node clks_node;
+	bool put_core_owner;
 };
 
 /***           runtime pm          ***/
@@ -3969,6 +3970,7 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
 {
 	struct clk *clk;
 	struct clk_core *core;
+	struct module *owner;
 
 	/* This is to allow this function to be chained to others */
 	if (IS_ERR_OR_NULL(hw))
@@ -3980,9 +3982,18 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
 		return clk;
 	clk->dev = dev;
 
-	if (!try_module_get(core->owner)) {
-		free_clk(clk);
-		return ERR_PTR(-ENOENT);
+	owner = (dev && dev->driver) ? dev->driver->owner : NULL;
+	/*
+	 * Avoid being unable to remove module if consumer and
+	 * provider have the same owner.
+	 */
+	if (owner != core->owner) {
+		if (try_module_get(core->owner)) {
+			clk->put_core_owner = true;
+		} else {
+			free_clk(clk);
+			return ERR_PTR(-ENOENT);
+		}
 	}
 
 	kref_get(&core->ref);
@@ -4560,7 +4571,8 @@ void __clk_put(struct clk *clk)
 
 	clk_prepare_unlock();
 
-	module_put(owner);
+	if (clk->put_core_owner)
+		module_put(owner);
 
 	free_clk(clk);
 }
-- 
2.40.0

Re: [PATCH] clk: change clk_hw_create_clk() to avoid being unable to remove module

[Stephen Boyd]

Quoting Heiner Kallweit (2023-04-13 14:39:28)
> With clk_hw_create_clk() we have the problem that module unloading
> is impossible if consumer and provider module owner are the same and
> refcount is incremented. See also following comment in __clk_register().

Do you never call clk_put() on the clk that you get from
clk_hw_create_clk()?

Re: [PATCH] clk: change clk_hw_create_clk() to avoid being unable to remove module

[Heiner Kallweit]

On 14.04.2023 00:29, Stephen Boyd wrote:
> Quoting Heiner Kallweit (2023-04-13 14:39:28)
>> With clk_hw_create_clk() we have the problem that module unloading
>> is impossible if consumer and provider module owner are the same and
>> refcount is incremented. See also following comment in __clk_register().
> 
> Do you never call clk_put() on the clk that you get from
> clk_hw_create_clk()?

In my case clk_put() is called from a devm release hook. Same issue
we'd have if clk_put would be called from the drivers remove callback.
clk_put would be unreachable because the incremented module refcount
prevents module removal.

Re: [PATCH] clk: change clk_hw_create_clk() to avoid being unable to remove module

[Stephen Boyd]

Quoting Heiner Kallweit (2023-04-13 23:01:13)
> On 14.04.2023 00:29, Stephen Boyd wrote:
> > Quoting Heiner Kallweit (2023-04-13 14:39:28)
> >> With clk_hw_create_clk() we have the problem that module unloading
> >> is impossible if consumer and provider module owner are the same and
> >> refcount is incremented. See also following comment in __clk_register().
> > 
> > Do you never call clk_put() on the clk that you get from
> > clk_hw_create_clk()?
> 
> In my case clk_put() is called from a devm release hook. Same issue
> we'd have if clk_put would be called from the drivers remove callback.
> clk_put would be unreachable because the incremented module refcount
> prevents module removal.
> 

Ok. You could unbind the device in sysfs though, right?

Re: [PATCH] clk: change clk_hw_create_clk() to avoid being unable to remove module

[Heiner Kallweit]

On 18.04.2023 02:43, Stephen Boyd wrote:
> Quoting Heiner Kallweit (2023-04-13 23:01:13)
>> On 14.04.2023 00:29, Stephen Boyd wrote:
>>> Quoting Heiner Kallweit (2023-04-13 14:39:28)
>>>> With clk_hw_create_clk() we have the problem that module unloading
>>>> is impossible if consumer and provider module owner are the same and
>>>> refcount is incremented. See also following comment in __clk_register().
>>>
>>> Do you never call clk_put() on the clk that you get from
>>> clk_hw_create_clk()?
>>
>> In my case clk_put() is called from a devm release hook. Same issue
>> we'd have if clk_put would be called from the drivers remove callback.
>> clk_put would be unreachable because the incremented module refcount
>> prevents module removal.
>>
> 
> Ok. You could unbind the device in sysfs though, right?

I *think* this should be possible, right.