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

[Brian Masney]

On Thu, Jul 23, 2026 at 07:03:11PM +0200, Jerome Brunet wrote:
> 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.

I agree that it's fixing a real problem, and also agree that a kunit
test for this is likely going to be nasty and overly complicated. Let's
wait and see what Stephen says.

Personally I think we merge this right after -rc1 is released to give
time for additional testing in -next.

Brian