Re: [PATCH v7 3/7] dax/cxl, hmem: Initialize hmem early and defer dax_cxl binding

["Koralahalli Channabasappa, Smita" <skoralah@amd.com>]

Hi Dan,

On 3/19/2026 4:07 PM, Dan Williams wrote:
> Koralahalli Channabasappa, Smita wrote:
> [..]
>>> I agree with Jonathan's comments in Patch 6, using __WORK_INITIALIZER or
>>> initializing work in dax_hmem_init() and gating flush on pdev will fix
>>> the WARN — I will add both for v8. But I think the WARN is likely
>>> indicating an ordering issue here..
> 
> Yes, Jonathan is right, static initialization is also my expecation.
> 
>>> On initial boot, the Makefile ordering ensures dax_hmem_init() runs
>>> before cxl_dax_region_init(), so both work items land on system_long_wq
>>> in the right order and dax_hmem's deferred work is queued before
>>> dax_cxl's driver registration work.
> 
> There is nothing that guarantees that 2 work items in system_long_wq run
> in submission order. Unlikely that matters given the explicit flushing.
> 
>>> On module reload which Alison is trying here I dont think, modules are
>>> loaded by Makefile order. I think dax_cxl's workqueue is calling
>>> dax_hmem_flush_work() before dax_hmem probe has had a chance to queue
>>> its work, so flush_work() flushes nothing and dax_cxl registers its
>>> driver without waiting.
> 
> Module load order does not matter after initial probe completion.
> 
> ...and dax_hmem is guaranteed to always load before dax_cxl due to the
> symbol dependency of dax_hmem_flush_work().
> 
>>> __WORK_INITIALIZER fixes the WARN, but doesn't fix the race I guess if
>>> we are hitting that here..
>>>
>>> [   34.673051] initcall dax_hmem_init+0x0/0xff0 [dax_hmem] returned 0
>>> after 2225 usecs
>>> [   34.676011] calling  cxl_dax_region_init+0x0/0xff0 [dax_cxl] @ 1059
>>>
>>> These two lines indicate cxl_dax started after dax_hmem_init() returns
>>> but I dont think that guarantees dax_hmem_platform_probe() has actually
>>> run..
>>>
>>> I dont know if wait_for_device_probe() in cxl_dax_region_driver_register
>>> might help..
>>>
>>> Thanks
>>> Smita
>>
>> Actually, thinking about this more..
>>
>> dax_hmem_initial_probe lives in device.c (built-in) so it survives
>> module reload. On reload it's still true from the first boot. This means
>> hmem_register_device() skips the deferral path entirely..
> 
> Yes, that is the expectation.
> 
>> The problem is this bypasses the cxl_region_contains_resource() check
>> that the deferred work normally does. On first boot,
>> process_defer_work() walks each range and decides per-range: if CXL
>> covers it, skip. If not, register with HMEM. On reload, that check never
>> happens — whoever registers first via alloc_dax_region() wins,
>> regardless of whether CXL actually covers the range.
> 
> Yes, I think you have hit on a real issue. There is no point in having
> dax_hmem auto-attach on driver reload. If userspace unloads the driver
> it gets to keep the pieces. So that means something like this:
> 
> diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
> index 15e462589b92..7478bc78a698 100644
> --- a/drivers/dax/hmem/hmem.c
> +++ b/drivers/dax/hmem/hmem.c
> @@ -112,10 +112,12 @@ static int hmem_register_device(struct device *host, int target_nid,
>   	    region_intersects(res->start, resource_size(res), IORESOURCE_MEM,
>   			      IORES_DESC_CXL) != REGION_DISJOINT) {
>   		if (!dax_hmem_initial_probe) {
> -			dev_dbg(host, "deferring range to CXL: %pr\n", res);
> +			dev_dbg(host, "await CXL initial probe: %pr\n", res);
>   			queue_work(system_long_wq, &dax_hmem_work.work);
>   			return 0;
>   		}
> +		dev_dbg(host, "deferring range to CXL: %pr\n", res);
> +		return 0;
>   	}

One issue with the reload fix - At boot, hmem_register_cxl_device() 
calls hmem_register_device() to register ranges that aren't claimed by 
CXL. But hmem_register_device() now always returns 0 for those ranges at 
boot.

I was thinking factoring out registration logic into 
__hmem_register_device() and have hmem_register_cxl_device() call that 
directly, bypassing the CXL gating. Something like:


+static int __hmem_register_device(...)
+{
+	/* Remaining in hmem_register_device after the CXL check */
+}

static int hmem_register_device(..)
{
	if (IS_ENABLED(CONFIG_DEV_DAX_CXL) && .. {
+		if (!dax_hmem_initial_probe_done) {
+			queue_work(system_long_wq, &dax_hmem_work);
+			return 0;
+		}
+		return 0;
+	}

+	return __hmem_register_device(host, target_nid, res);
}

+static int hmem_register_cxl_device(...)
+{
	...
+	return __hmem_register_device(host, target_nid, res);
+}

+static void process_defer_work(...)
+{
+	...
-	dax_hmem_initial_probe = true;
-	walk_hmem_resources(&pdev->dev, hmem_register_cxl_device);
+	if (!dax_hmem_initial_probe) {
+		dax_hmem_initial_probe = true;
+		walk_hmem_resources(.., hmem_register_cxl_device);
+	}
..
+}

Tracing it

At boot:

probe -> walk(hmem_register_device)
    CXL range, !dax_hmem_initial_probe -> queue_work, return 0
    non-CXL ranges -> __hmem_register_device -> registers

process_defer_work:
    !dax_hmem_initial_probe
       dax_hmem_initial_probe = true
       walk(hmem_register_cxl_device)
       CXL covers -> return 0
       CXL doesn't cover -> __hmem_register_device()
          no CXL check again, straight to registration..

On reload:

probe -> walk(hmem_register_device)
    CXL range, dax_hmem_initial_probe = true, "your return 0" -> skips
    non-CXL ranges -> __hmem_register_device -> registers

process_defer_work:
    dax_hmem_initial_probe = true -> skip the walk entirely..

Or do you think this can be simplified better and the above approach has 
some caveats?

Thanks
Smita
>   
>   	rc = region_intersects_soft_reserve(res->start, resource_size(res));
> 
> ---
> 
> ...because if userspace wants to reload the dax_hmem driver, then it
> needs to pick what happens with the CXL intersection. Userspace can
> always unload cxl_acpi to force everything back to dax_hmem.
> 
> Now, you might say, "but this means that if the initial probe results in
> a partial result of some regions in dax_hmem and others in dax_cxl, that
> state can not be recovered outside of a reboot". I think that is ok.
> This mechanism is automatic best-effort workaround for bugs / missing
> capabilities in the CXL driver. Module reload fidelity is out of scope.
> 
>> So if dax_cxl registers first on reload, it could claim a range that CXL
>> doesn't actually cover, and dax_hmem would lose a range it should own..
> 
> With the above change, dax_cxl always wins in the "reload" scenario iff
> cxl_acpi is loaded. Otherwise dax_hmem owns all the Soft Reserved.
> 
>> I dont know if Im thinking through this right..
> 
> You definitely identified the need for that fixup above.