Re: [PATCH] cxl: Adjust the startup priority of cxl_pmem to be higher than that of cxl_acpi

[Dan Williams]

Cui Chao wrote:

Hi Cui, this looks good, a few notes below.

> During the cxl_acpi probe process, it checks whether the cxl_nvb device
> and driver have been attached. Currently, the startup priority of the
> cxl_pmem driver is lower than that of the cxl_acpi driver. At this point,
> the cxl_nvb driver has not yet been registered on the cxl_bus, causing
> the attachment check to fail. This results in a failure to add the root
> nvdimm bridge, leading to a cxl_acpi probe failure and ultimately
> affecting the subsequent loading of cxl drivers. As a consequence, only
> one mem device object exists on the cxl_bus, while the cxl_port device
> objects and decoder device objects are missing.
> 
> The solution is to raise the startup priority of cxl_pmem to be higher
> than that of cxl_acpi, ensuring that the cxl_pmem driver is registered
> before the aforementioned attachment check occurs.

Missing:

Fixes: e7e222ad73d9 ("cxl: Move devm_cxl_add_nvdimm_bridge() to cxl_pmem.ko")

> Co-developed-by: Wang Yinfeng <wangyinfeng@xxxxxxxxxxxxxx>
> Signed-off-by: Wang Yinfeng <wangyinfeng@xxxxxxxxxxxxxx>
> Signed-off-by: Cui Chao <cuichao1753@xxxxxxxxxxxxxx>
> ---
>  drivers/cxl/pmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index 082ec0f1c3a0..261dff7ced9f 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -554,7 +554,7 @@ static __exit void cxl_pmem_exit(void)
>  
>  MODULE_DESCRIPTION("CXL PMEM: Persistent Memory Support");
>  MODULE_LICENSE("GPL v2");
> -module_init(cxl_pmem_init);
> +subsys_initcall(cxl_pmem_init);

Yes, this is a problem, but longer term I do not think we want to get
into this game of different initcall levels for different modules, and
dispersing the documentation for all these concerns.

So I want to match the initcall level of cxl_acpi and move the init
order management to the Makefile to disambiguate which initialization
happens first for the same level. There is already some documentation in
the Makefile about this case.

This matches what happens with all the other CXL modules that share
"module_init()", but fallback to Makefile order when built-in.

Specifically what I am proposing is a patch like below. However, my
change change is not suitable for v7.0-rc which can take your one-line
fix. So, for that you can add:

Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>

...and then I will follow up to move it to the new proposed scheme:

-- 8< --
Subject: cxl: Introduce cxl_subsys_initcall

From: Dan Williams <dan.j.williams@xxxxxxxxx>

Create a single definition of the early initcall expectation
of the CXL subsystem. Use that single definition to collect all the
documentation about how to order dependencies both within the subsystem
(drivers/cxl/Makefile order) and outside the subsystem (producer / consumer
dependencies).

Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
---
 drivers/cxl/Makefile    |    4 +++-
 drivers/cxl/acpi.c      |    7 +------
 drivers/cxl/core/port.c |    2 +-
 drivers/cxl/cxl.h       |   10 ++++++++++
 drivers/cxl/pmem.c      |    2 +-
 drivers/cxl/port.c      |    6 +-----
 6 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index 2caa90fa4bf2..d449bffde205 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -7,6 +7,8 @@
 # - 'mem' and 'pmem' before endpoint drivers so that memdevs are
 #   immediately enabled
 # - 'pci' last, also mirrors the hardware enumeration hierarchy
+# - 'pmem' before 'acpi' because acpi wants to create cxl_nvdimm_bridge
+#   devices
 obj-y += core/
 obj-$(CONFIG_CXL_PORT) += cxl_port.o
 obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
@@ -15,7 +17,7 @@ obj-$(CONFIG_CXL_MEM) += cxl_mem.o
 obj-$(CONFIG_CXL_PCI) += cxl_pci.o
 
 cxl_port-y := port.o
-cxl_acpi-y := acpi.o
 cxl_pmem-y := pmem.o security.o
+cxl_acpi-y := acpi.o
 cxl_mem-y := mem.o
 cxl_pci-y := pci.o
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 127537628817..6d158c7b6ca7 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -995,12 +995,7 @@ static void __exit cxl_acpi_exit(void)
 	cxl_bus_drain();
 }
 
-/*
- * Load before dax_hmem sees 'Soft Reserved' CXL ranges. Use
- * subsys_initcall_sync() since there is an order dependency with
- * subsys_initcall(efisubsys_init), which must run first.
- */
-subsys_initcall_sync(cxl_acpi_init);
+cxl_subsys_initcall(cxl_acpi_init);
 
 /*
  * Arrange for host-bridge ports to be active synchronous with
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 0c5957d1d329..aca31cdbc5a3 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2549,7 +2549,7 @@ static void cxl_core_exit(void)
 	debugfs_remove_recursive(cxl_debugfs);
 }
 
-subsys_initcall(cxl_core_init);
+cxl_subsys_initcall(cxl_core_init);
 module_exit(cxl_core_exit);
 MODULE_DESCRIPTION("CXL: Core Compute Express Link support");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 9b947286eb9b..53086757cc56 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -909,6 +909,16 @@ void cxl_driver_unregister(struct cxl_driver *cxl_drv);
 #define module_cxl_driver(__cxl_driver) \
 	module_driver(__cxl_driver, cxl_driver_register, cxl_driver_unregister)
 
+/*
+ * When built-in, some CXL modules need to load before others, see
+ * drivers/cxl/Makefile for that order.
+ *
+ * CXL has init dependencies with other subsystems. It consumes EFI runtime
+ * services (subsys_initcall), it produces IORES_DESC_CXL for dax_hmem to
+ * consume (device_initcall)
+ */
+#define cxl_subsys_initcall subsys_initcall_sync
+
 #define CXL_DEVICE_NVDIMM_BRIDGE	1
 #define CXL_DEVICE_NVDIMM		2
 #define CXL_DEVICE_PORT			3
diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index 082ec0f1c3a0..d5c5ffc72e17 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -554,7 +554,7 @@ static __exit void cxl_pmem_exit(void)
 
 MODULE_DESCRIPTION("CXL PMEM: Persistent Memory Support");
 MODULE_LICENSE("GPL v2");
-module_init(cxl_pmem_init);
+cxl_subsys_initcall(cxl_pmem_init);
 module_exit(cxl_pmem_exit);
 MODULE_IMPORT_NS("CXL");
 MODULE_ALIAS_CXL(CXL_DEVICE_NVDIMM_BRIDGE);
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index ada51948d52f..4c1385953801 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -326,11 +326,7 @@ static int __init cxl_port_init(void)
 {
 	return cxl_driver_register(&cxl_port_driver);
 }
-/*
- * Be ready to immediately enable ports emitted by the platform CXL root
- * (e.g. cxl_acpi) when CONFIG_CXL_PORT=y.
- */
-subsys_initcall(cxl_port_init);
+cxl_subsys_initcall(cxl_port_init);
 
 static void __exit cxl_port_exit(void)
 {