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

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

[Brian Masney]

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@baylibre.com>
> ---
> 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@baylibre.com

Reviewed-by: Brian Masney <bmasney@redhat.com>

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

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

[Jerome Brunet]

On jeu. 23 juil. 2026 at 10:40, Brian Masney <bmasney@redhat.com> 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@baylibre.com>
>> ---
>> 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@baylibre.com
>
> Reviewed-by: Brian Masney <bmasney@redhat.com>
>
> 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

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@redhat.com> 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@baylibre.com>
> >> ---
> >> 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@baylibre.com
> >
> > Reviewed-by: Brian Masney <bmasney@redhat.com>
> >
> > 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