Re: [PATCH v7 6/7] dax/hmem, cxl: Defer and resolve Soft Reserved ownership

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

Hi Jonathan,

Thanks for all the comments. I will fix all of them in v8.

Thanks
Smita

On 3/19/2026 7:29 AM, Jonathan Cameron wrote:
> On Thu, 19 Mar 2026 01:14:59 +0000
> Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> 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 <dan.j.williams@intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> 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);
>>   }
>