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

Koralahalli Channabasappa, Smita
2026-03-20T17:17:48Z

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);
>> }
>