| Message ID | 4eb4755b-7a06-6cd9-7c9d-6d088d05ab19@gmail.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | clk: change clk_hw_create_clk() to avoid being unable to remove module | expand |
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()?
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.
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?
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.
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); }
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(-)