Re: [PATCH v6 8/9] dax/hmem, cxl: Defer and resolve ownership of Soft Reserved memory ranges

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

Hi Dan,

Thanks for the detailed feedback. I have put together a pseudo code 
sketch incorporating all your points. Could you confirm I have 
understood the direction correctly, or if I'm missing anything?

On 3/11/2026 7:28 PM, Dan Williams wrote:
> Smita Koralahalli 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 dax_hmem
>> probe, schedule deferred work and wait for the CXL stack to complete
>> enumeration and region assembly before deciding ownership.
>>
>> Evaluate ownership of Soft Reserved ranges based on CXL region
>> containment.
>>
>>     - If all Soft Reserved ranges are fully contained within committed CXL
>>       regions, DROP handling Soft Reserved ranges from dax_hmem and allow
>>       dax_cxl to bind.
>>
>>     - If any Soft Reserved range is not fully claimed by committed CXL
>>       region, REGISTER the Soft Reserved ranges with dax_hmem.
>>
>> Use dax_cxl_mode to coordinate ownership decisions for Soft Reserved
>> ranges. Once, ownership resolution is complete, flush the deferred work
>> from dax_cxl before allowing dax_cxl to bind.
>>
>> This enforces a strict ownership. Either CXL fully claims the Soft
>> reserved ranges or it relinquishes it entirely.
> 
> We have had multiple suggestions during the course of developing this
> state machine, but I can not see reading this changelog or the
> implementation that the full / final state machine is laid out with all
> the old ideas cleaned out of the implementation.
> 
> For example, I think this has my "untested!" suggestion from:
> 
> http://lore.kernel.org/697acf78acf70_3095100c@dwillia2-mobl4.notmuch
> 
> ...but it does not have the explanation of why it turned out to be
> suitable and fits the end goal state machine.
> 
> It also has the original definition of "enum dax_cxl_mode". However,
> with the recent simplification proposal to stop doing total CXL unwind I
> think it allows for a more straightforward state machine. For example,
> the "drop" state is now automatic simply by losing the race with
> dax_hmem, right?
> 
> I think we are close, just some final complexity shaving.
> 
> So, with the decision to stop tearing down CXL this state machine only
> has 3 requirements.
> 
> 1/ CXL enumeration needs to start before dax_hmem invokes
>     wait_for_device_probe().
> 
> 2/ dax_cxl driver registration needs to be postponed until after
>     dax_hmem has dispositioned all its regions.
> 
> 3/ No probe path can flush the work because of the wait_for_device_probe().
> 
> Requirement 1/ is met by patch1. Requirement 2/ partially met, has a
> proposal here around flushing the work from a separate workqueue
> invocation, but I think you want the dependency directly on the dax_hmem
> module (if enabled). Requirement 3/ not achieved.
> 
> For 3/ I think we can borrow what cxl_mem_probe() does and do:
> 
> if (work_pending(&dax_hmem_work))
> 	return -EBUSY;
> 
> ...if for some reason someone really wants to rebind the dax_hmem driver
> they need to flush the queue, and that can be achived by a flush_work()
> in the module_exit() path.
> 
> This does mean that patch 7 in this series disappears because bus.c has
> no role to play in this mess. It is just dax_hmem and dax_cxl getting
> their ordering straight.
> 
> Some notes on the implications follow:
> 
>> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
>> ---
>>   drivers/dax/bus.c       |  3 ++
>>   drivers/dax/bus.h       | 19 ++++++++++
>>   drivers/dax/cxl.c       |  1 +
>>   drivers/dax/hmem/hmem.c | 78 +++++++++++++++++++++++++++++++++++++++--
>>   4 files changed, 99 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
>> index 92b88952ede1..81985bcc70f9 100644
>> --- a/drivers/dax/bus.c
>> +++ b/drivers/dax/bus.c
>> @@ -25,6 +25,9 @@ DECLARE_RWSEM(dax_region_rwsem);
>>    */
>>   DECLARE_RWSEM(dax_dev_rwsem);
>>   
>> +enum dax_cxl_mode dax_cxl_mode = DAX_CXL_MODE_DEFER;
>> +EXPORT_SYMBOL_NS_GPL(dax_cxl_mode, "CXL");
>> +
>>   static DEFINE_MUTEX(dax_hmem_lock);
>>   static dax_hmem_deferred_fn hmem_deferred_fn;
>>   static void *dax_hmem_data;
>> diff --git a/drivers/dax/bus.h b/drivers/dax/bus.h
>> index b58a88e8089c..82616ff52fd1 100644
>> --- a/drivers/dax/bus.h
>> +++ b/drivers/dax/bus.h
>> @@ -41,6 +41,25 @@ struct dax_device_driver {
>>   	void (*remove)(struct dev_dax *dev);
>>   };
>>   
>> +/*
>> + * enum dax_cxl_mode - State machine to determine ownership for CXL
>> + * tagged Soft Reserved memory ranges.
>> + * @DAX_CXL_MODE_DEFER: Ownership resolution pending. Set while waiting
>> + * for CXL enumeration and region assembly to complete.
>> + * @DAX_CXL_MODE_REGISTER: CXL regions do not fully cover Soft Reserved
>> + * ranges. Fall back to registering those ranges via dax_hmem.
>> + * @DAX_CXL_MODE_DROP: All Soft Reserved ranges intersecting CXL windows
>> + * are fully contained within committed CXL regions. Drop HMEM handling
>> + * and allow dax_cxl to bind.
> 
> With the above, dax_cxl_mode disappears.
> 
>> 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();
> 
> Looks ok, as long as that symbol is from dax_hmem.ko which gets you the
> load dependency (requirement 2/).
> 
> Might also want to make sure that all this deferral mess disappears when
> CONFIG_DEV_DAX_HMEM=n.
> 
>>   	cxl_driver_register(&cxl_dax_region_driver);
>>   }
>>   
>> diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
>> index 1e3424358490..85854e25254b 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;
>> @@ -69,8 +70,18 @@ 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;
>> +		switch (dax_cxl_mode) {
>> +		case DAX_CXL_MODE_DEFER:
>> +			dev_dbg(host, "deferring range to CXL: %pr\n", res);
>> +			dax_hmem_queue_work();
> 
> This case is just a flag that determines if the work queue has completed
> its one run. So I expect this something like:
> 
> if (!dax_hmem_initial_probe) {
> 	queue_work()
> 	return;
> }
> 
> Otherwise just go ahead and register because dax_cxl by this time has
> had a chance to have a say and the system falls back to "first come /
> first served" mode. In other words the simplification of not cleaning up
> goes both ways. dax_hmem naturally fails if dax_cxl already claimed the
> address range.
> 
>> +			return 0;
>> +		case DAX_CXL_MODE_REGISTER:
>> +			dev_dbg(host, "registering CXL range: %pr\n", res);
>> +			break;
>> +		case DAX_CXL_MODE_DROP:
>> +			dev_dbg(host, "dropping CXL range: %pr\n", res);
>> +			return 0;
>> +		}
>>   	}
>>   
>>   	rc = region_intersects_soft_reserve(res->start, resource_size(res));
>> @@ -123,8 +134,70 @@ 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 hmem_register_device(host, target_nid, res);
>> +
>> +	return 0;
>> +}
>> +
>> +static int soft_reserve_has_cxl_match(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) {
>> +		if (!cxl_region_contains_soft_reserve((struct resource *)res))
>> +			return 1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void process_defer_work(void *data)
>> +{
>> +	struct platform_device *pdev = data;
>> +	int rc;
>> +
>> +	/* relies on cxl_acpi and cxl_pci having had a chance to load */
>> +	wait_for_device_probe();
>> +
>> +	rc = walk_hmem_resources(&pdev->dev, soft_reserve_has_cxl_match);
>> +
>> +	if (!rc) {
>> +		dax_cxl_mode = DAX_CXL_MODE_DROP;
>> +		dev_dbg(&pdev->dev, "All Soft Reserved ranges claimed by CXL\n");
>> +	} else {
>> +		dax_cxl_mode = DAX_CXL_MODE_REGISTER;
>> +		dev_warn(&pdev->dev,
>> +			 "Soft Reserved not fully contained in CXL; using HMEM\n");
>> +	}
>> +
>> +	walk_hmem_resources(&pdev->dev, hmem_register_cxl_device);
> 
> I do not think we need to do 2 passes. Just do one
> hmem_register_cxl_device() pass that skips a range when
> cxl_region_contains_resource() has it covered, otherwise register an
> hmem device.
> 
>> +}
>> +
>> +static void kill_defer_work(void *data)
>> +{
>> +	struct platform_device *pdev = data;
>> +
>> +	dax_hmem_flush_work();
>> +	dax_hmem_unregister_work(process_defer_work, pdev);
>> +}
>> +
>>   static int dax_hmem_platform_probe(struct platform_device *pdev)
>>   {
>> +	int rc;
> 
> This wants a work_pending() return -EBUSY per above.
> 
>> +	rc = dax_hmem_register_work(process_defer_work, pdev);
>> +	if (rc)
>> +		return rc;
> 
> The work does not need to be registered every time. Remember this is
> only a one-shot problem at first kernel boot, not every time this
> platform device is probed. After the workqueue has run at least once it
> never needs to be invoked again if dax_hmem is reloaded.
> 
> A flag for "dax_hmem flushed initial device probe at least once" needs
> to live in drivers/dax/hmem/device.c and be cleared by
> process_defer_work().
> 
>> +
>> +	rc = devm_add_action_or_reset(&pdev->dev, kill_defer_work, pdev);
>> +	if (rc)
>> +		return rc;
>> +
>>   	return walk_hmem_resources(&pdev->dev, hmem_register_device);
>>   }
> 
> The hunk that is missing is that dax_hmem_exit() should flush the work,
> and process_defer_work() should give up if the device has been unbound
> before it runs. Hopefully that last suggestion does not make lockdep
> unhappy about running process_defer_work under the hmem_platform
> device_lock(). I *think* it should be ok and solves the TOCTOU race in
> hmem_register_device() around whether we are in the pre or post initial
> probe world.

/* hmem.c */

+static struct platform_device *hmem_pdev;
+static void process_defer_work(struct work_struct *work);
+static DECLARE_WORK(dax_hmem_work, process_defer_work);

+void dax_hmem_flush_work(void)
+{
+	flush_work(&dax_hmem_work);
+}
+EXPORT_SYMBOL_GPL(dax_hmem_flush_work);

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;
+	}
...
}

+static int hmem_register_cxl_device(..)
+{
+	if (region_intersects(..IORES_CXL..) == REGION_DISJOINT)
+		return 0;
+
+	if (cxl_region_contains_soft_reserve(res))
+		return 0;
+
+	return hmem_register_device(host, target_nid, res);
+}

+static void process_defer_work(struct work_struct *work)
+{
+	wait_for_device_probe();
+	/* Flag lives in device.c */
+	dax_hmem_initial_probe_done = true;
+	walk_hmem_resources(&hmem_pdev->dev, hmem_register_cxl_device);
+}

static int dax_hmem_platform_probe(struct platform_device *pdev)
{
+	if (work_pending(&dax_hmem_work))
+		return -EBUSY;

+	hmem_pdev = pdev;
	return walk_hmem_resources(&dev->dev, hmem_register_device);
}

static void __exit dax_hmem_exit(void)
{
+	flush_work(&dax_hmem_work);
..

/* cxl.c */

static void cxl_dax_region_driver_register(struct work_struct *work)
{
+	dax_hmem_flush_work();
	cxl_driver_register(&cxl_dax_region_driver);
}

/* bus.h */

+#if IS_ENABLED(CONFIG_DEV_DAX_HMEM)
+void dax_hmem_flush_work(void);
+#else
+static inline void dax_hmem_flush_work(void) { }
+#endif

A few things I want to confirm:

1. Patch 7 (bus.c helpers) drops entirely — no register/unregister API, 
no mutex, no typedef. Everything lives in hmem.c.

2. enum dax_cxl_mode drops — replaced by the single bool 
dax_hmem_initial_probe_done in device.c.

3. dax_hmem_flush_work() exported from dax_hmem.ko so cxl.c gets the 
module dependency for requirement 2.

Thanks
Smita