On Thu, 19 Mar 2026 01:14:59 +0000
Smita Koralahalli <[email protected]> wrote:
The current probe time ownership check for Soft Reserved memory based
solely on CXL window intersection is insufficient. dax_hmem probing is not
always guaranteed to run after CXL enumeration and region assembly, which
can lead to incorrect ownership decisions before the CXL stack has
finished publishing windows and assembling committed regions.
Introduce deferred ownership handling for Soft Reserved ranges that
intersect CXL windows. When such a range is encountered during the
initial dax_hmem probe, schedule deferred work to wait for the CXL stack
to complete enumeration and region assembly before deciding ownership.
Once the deferred work runs, evaluate each Soft Reserved range
individually: if a CXL region fully contains the range, skip it and let
dax_cxl bind. Otherwise, register it with dax_hmem. This per-range
ownership model avoids the need for CXL region teardown and
alloc_dax_region() resource exclusion prevents double claiming.
Introduce a boolean flag dax_hmem_initial_probe to live inside device.c
so it survives module reload. Ensure dax_cxl defers driver registration
until dax_hmem has completed ownership resolution. dax_cxl calls
dax_hmem_flush_work() before cxl_driver_register(), which both waits for
the deferred work to complete and creates a module symbol dependency that
forces dax_hmem.ko to load before dax_cxl.
Co-developed-by: Dan Williams <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
Signed-off-by: Smita Koralahalli <[email protected]>
Hi Smita,
I think this is very likely to be what is causing the bug Alison
saw in cxl_test.
It looks to be possible to flush work before the work structure has
been configured. Even though it's not on a work queue and there is
nothing to do, there are early sanity checks that fail giving the warning
Alison reported.
A couple of ways to fix that inline. I'd be tempted to both initialize
the function statically and gate against flushing if the whole thing isn't
set up yet.
Jonathan
---
drivers/dax/bus.h | 7 +++++
drivers/dax/cxl.c | 1 +
drivers/dax/hmem/device.c | 3 ++
drivers/dax/hmem/hmem.c | 66 +++++++++++++++++++++++++++++++++++++--
4 files changed, 75 insertions(+), 2 deletions(-)
diff --git a/drivers/dax/bus.h b/drivers/dax/bus.h
index cbbf64443098..ebbfe2d6da14 100644
--- a/drivers/dax/bus.h
+++ b/drivers/dax/bus.h
@@ -49,6 +49,13 @@ void dax_driver_unregister(struct dax_device_driver
*dax_drv);
void kill_dev_dax(struct dev_dax *dev_dax);
bool static_dev_dax(struct dev_dax *dev_dax);
+#if IS_ENABLED(CONFIG_DEV_DAX_HMEM)
+extern bool dax_hmem_initial_probe;
+void dax_hmem_flush_work(void);
+#else
+static inline void dax_hmem_flush_work(void) { }
+#endif
+
#define MODULE_ALIAS_DAX_DEVICE(type) \
MODULE_ALIAS("dax:t" __stringify(type) "*")
#define DAX_DEVICE_MODALIAS_FMT "dax:t%d"
diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c
index a2136adfa186..3ab39b77843d 100644
--- a/drivers/dax/cxl.c
+++ b/drivers/dax/cxl.c
@@ -44,6 +44,7 @@ static struct cxl_driver cxl_dax_region_driver = {
static void cxl_dax_region_driver_register(struct work_struct *work)
{
+ dax_hmem_flush_work();
cxl_driver_register(&cxl_dax_region_driver);
}
diff --git a/drivers/dax/hmem/device.c b/drivers/dax/hmem/device.c
index 56e3cbd181b5..991a4bf7d969 100644
--- a/drivers/dax/hmem/device.c
+++ b/drivers/dax/hmem/device.c
@@ -8,6 +8,9 @@
static bool nohmem;
module_param_named(disable, nohmem, bool, 0444);
+bool dax_hmem_initial_probe;
+EXPORT_SYMBOL_GPL(dax_hmem_initial_probe);
+
static bool platform_initialized;
static DEFINE_MUTEX(hmem_resource_lock);
static struct resource hmem_active = {
diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
index 1e3424358490..8c574123bd3b 100644
--- a/drivers/dax/hmem/hmem.c
+++ b/drivers/dax/hmem/hmem.c
@@ -3,6 +3,7 @@
#include <linux/memregion.h>
#include <linux/module.h>
#include <linux/dax.h>
+#include <cxl/cxl.h>
#include "../bus.h"
static bool region_idle;
@@ -58,6 +59,19 @@ static void release_hmem(void *pdev)
platform_device_unregister(pdev);
}
+struct dax_defer_work {
+ struct platform_device *pdev;
+ struct work_struct work;
+};
+
+static struct dax_defer_work dax_hmem_work;
static struct dax_defer_work dax_hmem_work = {
.work = __WORK_INITIALIZER(&dax_hmem_work.work,
process_defer_work),
};
or something similar.
+
+void dax_hmem_flush_work(void)
+{
+ flush_work(&dax_hmem_work.work);
+}
+EXPORT_SYMBOL_GPL(dax_hmem_flush_work);
+
static int hmem_register_device(struct device *host, int target_nid,
const struct resource *res)
{
@@ -69,8 +83,11 @@ static int hmem_register_device(struct device *host, int
target_nid,
if (IS_ENABLED(CONFIG_DEV_DAX_CXL) &&
region_intersects(res->start, resource_size(res), IORESOURCE_MEM,
IORES_DESC_CXL) != REGION_DISJOINT) {
- dev_dbg(host, "deferring range to CXL: %pr\n", res);
- return 0;
+ if (!dax_hmem_initial_probe) {
+ dev_dbg(host, "deferring range to CXL: %pr\n", res);
+ queue_work(system_long_wq, &dax_hmem_work.work);
+ return 0;
+ }
}
rc = region_intersects_soft_reserve(res->start, resource_size(res));
@@ -123,8 +140,48 @@ static int hmem_register_device(struct device *host, int
target_nid,
return rc;
}
+static int hmem_register_cxl_device(struct device *host, int target_nid,
+ const struct resource *res)
+{
+ if (region_intersects(res->start, resource_size(res), IORESOURCE_MEM,
+ IORES_DESC_CXL) == REGION_DISJOINT)
+ return 0;
+
+ if (cxl_region_contains_resource((struct resource *)res)) {
+ dev_dbg(host, "CXL claims resource, dropping: %pr\n", res);
+ return 0;
+ }
+
+ dev_dbg(host, "CXL did not claim resource, registering: %pr\n", res);
+ return hmem_register_device(host, target_nid, res);
+}
+
+static void process_defer_work(struct work_struct *w)
+{
+ struct dax_defer_work *work = container_of(w, typeof(*work), work);
+ struct platform_device *pdev = work->pdev;
If you do the suggested __INITIALIZE_WORK() then I'd add
a paranoid
if (!work->pdev)
return;
We don't actually queue the work before pdev is set, but that might
be obvious once we spilt up assigning the function and the data
it uses.
+
+ wait_for_device_probe();
+
+ guard(device)(&pdev->dev);
+ if (!pdev->dev.driver)
+ return;
+
+ dax_hmem_initial_probe = true;
+ walk_hmem_resources(&pdev->dev, hmem_register_cxl_device);
+}
+
static int dax_hmem_platform_probe(struct platform_device *pdev)
{
+ if (work_pending(&dax_hmem_work.work))
+ return -EBUSY;
+
+ if (!dax_hmem_work.pdev) {
+ get_device(&pdev->dev);
+ dax_hmem_work.pdev = pdev;
Using the pdev rather than dev breaks the pattern of doing a get_device()
and assigning in one line. This is a bit ugly.
dax_hmem_work.pdev = to_pci_dev(get_device(&pdev->dev));
but perhaps makes the association tighter than current code.
+ INIT_WORK(&dax_hmem_work.work, process_defer_work);
See above. I think assigning the work function should be static
which should resolve the issue Alison was seeing as then it should
be fine to call flush_work() on the item that isn't on a work queue
yet but is initialized.
+ }
+
return walk_hmem_resources(&pdev->dev, hmem_register_device);
}
@@ -162,6 +219,11 @@ static __init int dax_hmem_init(void)
static __exit void dax_hmem_exit(void)
{
+ flush_work(&dax_hmem_work.work);
I think this needs to be under the if (dax_hmem_work.pdev)
Not sure there is any guarantee dax_hmem_platform_probe() has run
before we get here otherwise. Alternative is to assign
the work function statically.
+
+ if (dax_hmem_work.pdev)
+ put_device(&dax_hmem_work.pdev->dev);
+
platform_driver_unregister(&dax_hmem_driver);
platform_driver_unregister(&dax_hmem_platform_driver);
}
