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

[Jerome Brunet]

On jeu. 23 juil. 2026 at 10:40, Brian Masney <bmasney@xxxxxxxxxx> wrote:

> Hi Jerome,
>
> On Thu, Jul 23, 2026 at 03:00:51PM +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@xxxxxxxxxxxx>
>> ---
>> 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
>> """
>> 
>> 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.
>> ---
>> Changes in v2:
>> - Update comment in __clk_register()
>> - Add missing documention for the new parameter of clk_hw_create_clk()
>> - No functional change
>> - Link to v1: https://patch.msgid.link/20260721-clk-provider-pinning-v1-1-63db2e667993@xxxxxxxxxxxx
>
> Reviewed-by: Brian Masney <bmasney@xxxxxxxxxx>
>
> This looks good to me. I'm planning to send Stephen a pull next week for
> content that I think is ready during this development cycle, however I'm
> not going to include this. I suspect he's going to want a kunit test for
> this and I know it's going to be complicated. My suggestion is to post
> some kind of rough test scenario for this, even if it's initially not in
> kunit. As I get some spare time at work, I can help you to see if we can
> get a kunit test together for this.
>
> Brian

I understand but I'm not adding an new API, I'm fixing a real problem in
an existing one in the kernel now. The related API is already used in
some kunit, so there is some coverage already.

The problem was known (as the comment in __clk_register() was showing) and
there was no kunit then ... so it is a bit strange to delay fixing the
problem because there is not kunit associated with it now.

That said, I'll check was I can do. acutally loading and unloading
module in kunit does not really feasible. Maybe poking the owner
field with __clk_hw_get_clk(). This is not really supposed to used
directly (which is why I did not add the documentation around it)
and it's bit fake but I do not really a way around it.

-- 
Jerome