Re: [PATCH v20 01/22] cxl/mem: Arrange for always-synchronous memdev attach

Alejandro Lucero Palau
2025-11-14T11:10:59Z

On 11/12/25 14:53, Jonathan Cameron wrote:
> On Mon, 10 Nov 2025 15:36:36 +0000
> alejandro.lucero-palau@amd.com wrote:
>
>> From: Dan Williams <dan.j.williams@intel.com>
>>
>> In preparation for CXL accelerator drivers that have a hard dependency on
>> CXL capability initialization, arrange for the endpoint probe result to be
>> conveyed to the caller of devm_cxl_add_memdev().
>>
>> As it stands cxl_pci does not care about the attach state of the cxl_memdev
>> because all generic memory expansion functionality can be handled by the
>> cxl_core. For accelerators, that driver needs to know perform driver
>> specific initialization if CXL is available, or exectute a fallback to PCIe
>> only operation.
>>
>> By moving devm_cxl_add_memdev() to cxl_mem.ko it removes async module
>> loading as one reason that a memdev may not be attached upon return from
>> devm_cxl_add_memdev().
>>
>> The diff is busy as this moves cxl_memdev_alloc() down below the definition
>> of cxl_memdev_fops and introduces devm_cxl_memdev_add_or_reset() to
>> preclude needing to export more symbols from the cxl_core.
>>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Alejandro, read submitting patches again. Whilst the first sign off should
> indeed by Dan's this also needs one from you as a 'handler' of the patch.
>
> Be very careful checking these tag chains. If they are wrong no one can
> merge the set and it just acts as a silly blocker.

Hi Jonathan,

I did the amend but it is true I did some work on it. Would it be enough
to add my signed-off-by along with Dan's one?

> I would have split this up and made the changes to cxl_memdev_alloc in
> a precursor patch (use of __free is obvious one) then could have stated
> that that was simply moved in this patch.

OK. I think I was fixing a bug in original Dan's patch regarding cxlmd
release in case of error inside devm_cxl_add_memdev, but I think the bug
is in the current code of that function as it is not properly released
if error after a successful allocation. So splitting the patch could
allow to make this clearer and adding the Fixes tag as well.

> There are other changes in there that are really hard to spot though
> and I think there are some bugs lurking in error paths.

I did spot one after your comment, checking cxlmd pointer is not an
error pointer inside __cxlmd_free. If you spotted something else, please
tell me :-)

Thank you!

> Jonathan
>
>> ---
>> drivers/cxl/Kconfig | 2 +-
>> drivers/cxl/core/memdev.c | 101 ++++++++++++++------------------------
>> drivers/cxl/mem.c | 41 ++++++++++++++++
>> drivers/cxl/private.h | 10 ++++
>> 4 files changed, 90 insertions(+), 64 deletions(-)
>> create mode 100644 drivers/cxl/private.h
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index e370d733e440..14b4601faf66 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -8,6 +8,7 @@
>> #include <linux/idr.h>
>> #include <linux/pci.h>
>> #include <cxlmem.h>
>> +#include "private.h"
>> #include "trace.h"
>> #include "core.h"
>>
>> @@ -648,42 +649,25 @@ static void detach_memdev(struct work_struct *work)
>>
>> static struct lock_class_key cxl_memdev_key;
>>
>> -static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>> - const struct file_operations *fops)
>> +int devm_cxl_memdev_add_or_reset(struct device *host, struct cxl_memdev *cxlmd)
>> {
>> - struct cxl_memdev *cxlmd;
>> - struct device *dev;
>> - struct cdev *cdev;
>> + struct device *dev = &cxlmd->dev;
>> + struct cdev *cdev = &cxlmd->cdev;
>> int rc;
>>
>> - cxlmd = kzalloc(sizeof(*cxlmd), GFP_KERNEL);
>> - if (!cxlmd)
>> - return ERR_PTR(-ENOMEM);
>> -
>> - rc = ida_alloc_max(&cxl_memdev_ida, CXL_MEM_MAX_DEVS - 1, GFP_KERNEL);
>> - if (rc < 0)
>> - goto err;
>> - cxlmd->id = rc;
>> - cxlmd->depth = -1;
>> -
>> - dev = &cxlmd->dev;
>> - device_initialize(dev);
>> - lockdep_set_class(&dev->mutex, &cxl_memdev_key);
>> - dev->parent = cxlds->dev;
>> - dev->bus = &cxl_bus_type;
>> - dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
>> - dev->type = &cxl_memdev_type;
>> - device_set_pm_not_required(dev);
>> - INIT_WORK(&cxlmd->detach_work, detach_memdev);
>> -
>> - cdev = &cxlmd->cdev;
>> - cdev_init(cdev, fops);
>> - return cxlmd;
>> + rc = cdev_device_add(cdev, dev);
>> + if (rc) {
>> + /*
>> + * The cdev was briefly live, shutdown any ioctl operations that
>> + * saw that state.
>> + */
>> + cxl_memdev_shutdown(dev);
>> + return rc;
>> + }
>>
>> -err:
>> - kfree(cxlmd);
>> - return ERR_PTR(rc);
>> + return devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
>> }
>> +EXPORT_SYMBOL_NS_GPL(devm_cxl_memdev_add_or_reset, "CXL");
>>
>> static long __cxl_memdev_ioctl(struct cxl_memdev *cxlmd, unsigned int cmd,
>> unsigned long arg)
>> @@ -1051,50 +1035,41 @@ static const struct file_operations cxl_memdev_fops = {
>> .llseek = noop_llseek,
>> };
>>
>> -struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
>> - struct cxl_dev_state *cxlds)
>> +struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds)
>> {
>> - struct cxl_memdev *cxlmd;
>> + struct cxl_memdev *cxlmd __free(kfree) =
>> + kzalloc(sizeof(*cxlmd), GFP_KERNEL);
> Trivial and perhaps not worth the hassle.
> I'd pull this out of the declarations block to have
>
> struct device *dev;
> struct cdev *cdev;
> int rc;
>
> struct cxl_memdev *cxlmd __free(kfree) =
> kzalloc(sizeof(*cxlmd), GFP_KERNEL);
> if (!cxlmd)
> return ERR_PTR(-ENOMEM);
>
> That is treat the __free() related statement as an inline declaration of
> the type we only really allow for these.
>
>
>> struct device *dev;
>> struct cdev *cdev;
>> int rc;
>>
>> - cxlmd = cxl_memdev_alloc(cxlds, &cxl_memdev_fops);
>> - if (IS_ERR(cxlmd))
>> - return cxlmd;
>>
>> - dev = &cxlmd->dev;
>> - rc = dev_set_name(dev, "mem%d", cxlmd->id);
>> - if (rc)
>> - goto err;
>> + if (!cxlmd)
>> + return ERR_PTR(-ENOMEM);
>>
>> - /*
>> - * Activate ioctl operations, no cxl_memdev_rwsem manipulation
>> - * needed as this is ordered with cdev_add() publishing the device.
>> - */
>> + rc = ida_alloc_max(&cxl_memdev_ida, CXL_MEM_MAX_DEVS - 1, GFP_KERNEL);
>> + if (rc < 0)
>> + return ERR_PTR(rc);
>> + cxlmd->id = rc;
>> + cxlmd->depth = -1;
>> cxlmd->cxlds = cxlds;
>> cxlds->cxlmd = cxlmd;
> These two lines weren't previously in cxl_memdev_alloc()
> I'd like a statement in the commit message of why they are now. It seems
> harmless because they are still ordered before the add and are
> ultimately freed
>
> I'm not immediately spotting why they now are. This whole code shift
> and complex diff is enough of a pain I'd be tempted to do the move first
> so that we can then see what is actually changed much more easily.
>
>
>>
>> - cdev = &cxlmd->cdev;
>> - rc = cdev_device_add(cdev, dev);
>> - if (rc)
>> - goto err;
>> -
>> - rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
>> - if (rc)
>> - return ERR_PTR(rc);
>> - return cxlmd;
>> + dev = &cxlmd->dev;
>> + device_initialize(dev);
>> + lockdep_set_class(&dev->mutex, &cxl_memdev_key);
>> + dev->parent = cxlds->dev;
>> + dev->bus = &cxl_bus_type;
>> + dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
>> + dev->type = &cxl_memdev_type;
>> + device_set_pm_not_required(dev);
>> + INIT_WORK(&cxlmd->detach_work, detach_memdev);
>>
>> -err:
>> - /*
>> - * The cdev was briefly live, shutdown any ioctl operations that
>> - * saw that state.
>> - */
>> - cxl_memdev_shutdown(dev);
>> - put_device(dev);
>> - return ERR_PTR(rc);
>> + cdev = &cxlmd->cdev;
>> + cdev_init(cdev, &cxl_memdev_fops);
>> + return_ptr(cxlmd);
>> }
>> -EXPORT_SYMBOL_NS_GPL(devm_cxl_add_memdev, "CXL");
>> +EXPORT_SYMBOL_NS_GPL(cxl_memdev_alloc, "CXL");
>>
>> static void sanitize_teardown_notifier(void *data)
>> {
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index d2155f45240d..fa5d901ee817 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -7,6 +7,7 @@
>>
>> #include "cxlmem.h"
>> #include "cxlpci.h"
>> +#include "private.h"
>>
>> /**
>> * DOC: cxl mem
>> @@ -202,6 +203,45 @@ static int cxl_mem_probe(struct device *dev)
>> return devm_add_action_or_reset(dev, enable_suspend, NULL);
>> }
>>
>> +static void __cxlmd_free(struct cxl_memdev *cxlmd)
>> +{
>> + cxlmd->cxlds->cxlmd = NULL;
>> + put_device(&cxlmd->dev);
>> + kfree(cxlmd);
>> +}
>> +
>> +DEFINE_FREE(cxlmd_free, struct cxl_memdev *, __cxlmd_free(_T))
>> +
>> +/**
>> + * devm_cxl_add_memdev - Add a CXL memory device
>> + * @host: devres alloc/release context and parent for the memdev
>> + * @cxlds: CXL device state to associate with the memdev
>> + *
>> + * Upon return the device will have had a chance to attach to the
>> + * cxl_mem driver, but may fail if the CXL topology is not ready
>> + * (hardware CXL link down, or software platform CXL root not attached)
>> + */
>> +struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
>> + struct cxl_dev_state *cxlds)
>> +{
>> + struct cxl_memdev *cxlmd __free(cxlmd_free) = cxl_memdev_alloc(cxlds);
>> + int rc;
>> +
>> + if (IS_ERR(cxlmd))
>> + return cxlmd;
>> +
>> + rc = dev_set_name(&cxlmd->dev, "mem%d", cxlmd->id);
>> + if (rc)
>> + return ERR_PTR(rc);
>> +
>> + rc = devm_cxl_memdev_add_or_reset(host, cxlmd);
>> + if (rc)
>> + return ERR_PTR(rc);
> Is the reference tracking right here? If the above call fails
> then it is possible cxl_memdev_unregister() has been called
> or just cxl_memdev_shutdown().
>
> If nothing else (and I suspect there is worse but haven't
> counted references) that will set
> cxlmd->cxlds = NULL;
> s part of cxl_memdev_shutdown()
> The __cxlmd_free() then dereferences that and boom.
>
>
>> +
>> + return no_free_ptr(cxlmd);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(devm_cxl_add_memdev, "CXL");