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

Alejandro Lucero Palau
2025-10-29T11:20:07Z

On 10/7/25 13:40, Jonathan Cameron wrote:
> On Mon, 6 Oct 2025 11:01:09 +0100
> <alejandro.lucero-palau@amd.com> wrote:
>
>> From: Alejandro Lucero <alucerop@amd.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, SoB chain broken here which makes this currently unmergeable.
>
> Should definitely have your SoB as you sent the patch to the list and need
> to make a statement that you believe it to be fine to do so (see the Certificate
> of origin stuff in the docs). Also, From should always be one of the authors.
> If Dan wrote this as the SoB suggests then From should be set to him..
>
> git commit --amend --author="Dan Williams <dan.j.williams@intel.com>"
>
> Will fix that up. Then either you add your SoB on basis you just 'handled'
> the patch but didn't make substantial changes, or your SoB and a Codeveloped-by
> if you did make major changes. If it is minor stuff you can an
> a sign off with # what changed
> comment next to it.

Understood. I'll ask Dan what he prefers.

>
> A few minor comments inline.
>
> Thanks,
>
> Jonathan
>
>
>> ---
>> drivers/cxl/Kconfig | 2 +-
>> drivers/cxl/core/memdev.c | 97 ++++++++++++++++-----------------------
>> drivers/cxl/mem.c | 30 ++++++++++++
>> drivers/cxl/private.h | 11 +++++
>> 4 files changed, 82 insertions(+), 58 deletions(-)
>> create mode 100644 drivers/cxl/private.h
>>
>> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
>> index 028201e24523..111e05615f09 100644
>> --- a/drivers/cxl/Kconfig
>> +++ b/drivers/cxl/Kconfig
>> @@ -22,6 +22,7 @@ if CXL_BUS
>> config CXL_PCI
>> tristate "PCI manageability"
>> default CXL_BUS
>> + select CXL_MEM
>> help
>> The CXL specification defines a "CXL memory device" sub-class in the
>> PCI "memory controller" base class of devices. Device's identified by
>> @@ -89,7 +90,6 @@ config CXL_PMEM
>>
>> config CXL_MEM
>> tristate "CXL: Memory Expansion"
>> - depends on CXL_PCI
>> default CXL_BUS
>> help
>> The CXL.mem protocol allows a device to act as a provider of "System
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index c569e00a511f..2bef231008df 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> -
>> -err:
>> - kfree(cxlmd);
>> - return ERR_PTR(rc);
>> }
>> +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)
>> @@ -1023,50 +1012,44 @@ 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 device *dev;
>> struct cdev *cdev;
>> int rc;
>>
>> - cxlmd = cxl_memdev_alloc(cxlds, &cxl_memdev_fops);
>> - if (IS_ERR(cxlmd))
>> - return cxlmd;
>> + cxlmd = kzalloc(sizeof(*cxlmd), GFP_KERNEL);
> It's a little bit non obvious due to the device initialize mid way
> through this, but given there are no error paths after that you can
> currently just do.
> struct cxl_memdev *cxlmd __free(kfree) =
> cxl_memdev_alloc(cxlds, &cxl_memdev_fops);
> and
> return_ptr(cxlmd);
>
> in the good path. That lets you then just return rather than having
> the goto err: handling for the error case that currently frees this
> manually.
>
> Unlike the change below, this one I think is definitely worth making.

I agree so I'll do it. The below suggestion is also needed ...

>
>> + if (!cxlmd)
>> + return ERR_PTR(-ENOMEM);
>>
>> - dev = &cxlmd->dev;
>> - rc = dev_set_name(dev, "mem%d", cxlmd->id);
>> - if (rc)
>> + rc = ida_alloc_max(&cxl_memdev_ida, CXL_MEM_MAX_DEVS - 1, GFP_KERNEL);
>> + if (rc < 0)
>> goto err;
>> -
>> - /*
>> - * Activate ioctl operations, no cxl_memdev_rwsem manipulation
>> - * needed as this is ordered with cdev_add() publishing the device.
>> - */
>> + cxlmd->id = rc;
>> + cxlmd->depth = -1;
>> cxlmd->cxlds = cxlds;
>> cxlds->cxlmd = cxlmd;
>>
>> - cdev = &cxlmd->cdev;
>> - rc = cdev_device_add(cdev, dev);
>> - if (rc)
>> - goto err;
>> + 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);
>>
>> - rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
>> - if (rc)
>> - return ERR_PTR(rc);
>> + cdev = &cxlmd->cdev;
>> + cdev_init(cdev, &cxl_memdev_fops);
>> return cxlmd;
>>
>> err:
>> - /*
>> - * The cdev was briefly live, shutdown any ioctl operations that
>> - * saw that state.
>> - */
>> - cxl_memdev_shutdown(dev);
>> - put_device(dev);
>> + kfree(cxlmd);
>> return ERR_PTR(rc);
>> }
>> -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 f7dc0ba8905d..144749b9c818 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -7,6 +7,7 @@
>>
>> #include "cxlmem.h"
>> #include "cxlpci.h"
>> +#include "private.h"
>> #include "core/core.h"
>>
>> /**
>> @@ -203,6 +204,34 @@ static int cxl_mem_probe(struct device *dev)
>> return devm_add_action_or_reset(dev, enable_suspend, NULL);
>> }
>>
>> +/**
>> + * 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 = cxl_memdev_alloc(cxlds);
> Bit marginal but you could do a DEFINE_FREE() for cxlmd
> similar to the one that exists for put_cxl_port
>
> You would then need to steal the pointer for the devm_ call at the
> end of this function.

We are not freeing cxlmd in case of errors after we got the allocation,
so I think it makes sense.

Thank you.

>
>> + int rc;
>> +
>> + if (IS_ERR(cxlmd))
>> + return cxlmd;
>> +
>> + rc = dev_set_name(&cxlmd->dev, "mem%d", cxlmd->id);
>> + if (rc) {
>> + put_device(&cxlmd->dev);
>> + return ERR_PTR(rc);
>> + }
>> +
>> + return devm_cxl_memdev_add_or_reset(host, cxlmd);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(devm_cxl_add_memdev, "CXL");