Re: [PATCH] clk: scu/imx8qxp: do not register driver in probe()

[Daniel Baluta <daniel.baluta@oss.nxp.com>]

On 2/11/26 16:43, Alexander Stein wrote:
> Hi Danilo,
>
> Am Mittwoch, 11. Februar 2026, 15:23:16 CET schrieb Danilo Krummrich:
>> imx_clk_scu_init() registers the imx_clk_scu_driver while commonly being
>> called from IMX driver's probe() callbacks.
>>
>> However, it neither makes sense to register drivers from probe()
>> callbacks of other drivers, nor does the driver core allow registering
>> drivers with a device lock already being held.
>>
>> The latter was revealed by commit dc23806a7c47 ("driver core: enforce
>> device_lock for driver_match_device()") leading to a deadlock condition
>> described in [1].
>>
>> Additionally, nothing seems to unregister the imx_clk_scu_driver once
>> the corresponding driver module is unloaded, which leaves the
>> driver-core with a dangling pointer.
>>
>> Hence, register the imx_clk_scu_driver from module_init() and unregister
>> it in module_exit().
>>
>> Fixes: dc23806a7c47 ("driver core: enforce device_lock for driver_match_device()")
>> Fixes: 220175cd3979 ("clk: imx: scu: fix build break when compiled as modules")
>> Reported-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>> Closes: https://lore.kernel.org/lkml/13955113.uLZWGnKmhe@steina-w/
>> Link: https://lore.kernel.org/lkml/DFU7CEPUSG9A.1KKGVW4HIPMSH@kernel.org/ [1]
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> Thanks for the patch.
> Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com> # TQMa8x/MBa8x
>
>> ---
>> I plan to send the driver-core PR for 7.0-rc1 soon, which will also contain
>> commit dc23806a7c47 ("driver core: enforce device_lock for
>> driver_match_device()").
>>
>> It also contains an IOMMU commit similar to this one:
>> https://patch.msgid.link/20260121141215.29658-1-dakr@kernel.org
>>
>> The commit was originally scheduled for 6.19-rc7, but was deferred to 7.0-rc1 in
>> case more of those cases pop up (which now happened).
>> ---
>>  drivers/clk/imx/clk-imx8qxp.c | 24 +++++++++++++++++++++++-
>>  drivers/clk/imx/clk-scu.c     | 12 +++++++++++-
>>  drivers/clk/imx/clk-scu.h     |  2 ++
>>  3 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c
>> index 3ae162625bb1..d89a2f40771e 100644
>> --- a/drivers/clk/imx/clk-imx8qxp.c
>> +++ b/drivers/clk/imx/clk-imx8qxp.c
>> @@ -346,7 +346,29 @@ static struct platform_driver imx8qxp_clk_driver = {
>>  	},
>>  	.probe = imx8qxp_clk_probe,
>>  };
>> -module_platform_driver(imx8qxp_clk_driver);
>> +
>> +static int __init imx8qxp_init(void)
>>
>>
I would call this imx8qxp_clk_init. Same for imx8qxp_exit.


>> +{
>> +	int ret;
>> +
>> +	ret = platform_driver_register(&imx8qxp_clk_driver);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = imx_clk_scu_module_init();
>> +	if (ret)
>> +		platform_driver_unregister(&imx8qxp_clk_driver);
>> +
>> +	return ret;
>>
Also, because the logical flow is that CLK driver is uing SCU for calls I would first call
imx_clk_scu_module_init and then register the imx8qxp_clk driver. 

But there is no functionality issues your your approach too, just a better logical flow.

Thanks,
Daniel.