Re: [PATCH v5 3/3] clk: imx: imx8qxp: add soft dependency on SCU power domain driver

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Relying on MODULE_SOFTDEP for driver probe ordering is an anti-pattern that fails to guarantee resource availability and is completely ineffective in built-in/module configurations.
--

--- Patch [3]: [PATCH v5 3/3] clk: imx: imx8qxp: add soft dependency on SCU power domain driver ---
commit b23cf3ab8ab04f852f48d1fd72b4b4fa914a8cd0
Author: Zhipeng Wang <[email protected]>

clk: imx: imx8qxp: add soft dependency on SCU power domain driver

This commit adds a MODULE_SOFTDEP to ensure the SCU power domain module
loads before the clock module. This is intended to work around failures
when attaching power domains to clock devices before the provider has
probed.

> diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c
> index c781425a005ef..17302ee628b76 100644
> --- a/drivers/clk/imx/clk-imx8qxp.c
> +++ b/drivers/clk/imx/clk-imx8qxp.c
> @@ -373,3 +373,4 @@ module_exit(imx8qxp_clk_exit);
>  MODULE_AUTHOR("Aisheng Dong <[email protected]>");
>  MODULE_DESCRIPTION("NXP i.MX8QXP clock driver");
>  MODULE_LICENSE("GPL v2");
> +MODULE_SOFTDEP("pre: scu_pd");

[Severity: High]
Does this softdep approach fully resolve the issue if one driver is built-in
and the other is a module?

Relying on module load ordering doesn't guarantee resource availability if
the kernel is configured with CONFIG_CLK_IMX8QXP=y and CONFIG_IMX_SCU_PD=m,
or if the power domain provider simply defers its probe. If the power
domains are not attached, the clocks are still registered and exposed.
Accessing them without an active power domain can lead to hardware hangs.

Looking at drivers/clk/imx/clk-scu.c, it appears the root of the problem is
that imx_clk_scu_alloc_dev() ignores the error instead of propagating it:

imx_clk_scu_alloc_dev()
    ...
    ret = imx_clk_scu_attach_pd(&pdev->dev, rsrc_id);
    if (ret)
        pr_warn("%s: failed to attached the power domain %d\n", name, ret);
    ...

Would it be safer to propagate the error (like -EPROBE_DEFER) when
imx_clk_scu_attach_pd() fails, rather than registering the clock hardware in
a broken state and relying on module load order?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3