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

[Jerome Brunet]

On mer. 22 juil. 2026 at 08:32, Brian Masney <bmasney@xxxxxxxxxx> 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@xxxxxxxxxx> 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@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
>> >> """
>> >> 
>> >> 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