Message-ID: <688bd79c-91aa-4d67-8291-dd0b222bebbf@intel.com>
Date: Fri, 14 Nov 2025 08:24:41 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Alejandro Lucero Palau <alucerop@....com>,
 Jonathan Cameron <jonathan.cameron@...wei.com>,
 alejandro.lucero-palau@....com
Cc: linux-cxl@...r.kernel.org, netdev@...r.kernel.org,
 dan.j.williams@...el.com, edward.cree@....com, davem@...emloft.net,
 kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com
Subject: Re: [PATCH v20 01/22] cxl/mem: Arrange for always-synchronous memdev
 attach



On 11/14/25 4:10 AM, Alejandro Lucero Palau wrote:
> 
> On 11/12/25 14:53, Jonathan Cameron wrote:
>> On Mon, 10 Nov 2025 15:36:36 +0000
>> alejandro.lucero-palau@....com wrote:
>>
>>> From: Dan Williams <dan.j.williams@...el.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@...el.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?

Yes. Essentially whenever you are posting someone else's patches as part of your series, you need to add your sign off after theirs for those patches. Similar to I sign off all the patches I ask Linus to pull even though I did not work on them.

DJ> 
> 
>> 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");

Powered by blists - more mailing lists