Re: [PATCH v2 3/4] cxl/memdev: Add support for multi PF devices

["Lucero Palau, Alejandro" <alejandro.lucero-palau@amd.com>]

On 01/10/2026 23:11, Dave Jiang wrote:
>
> On 10/1/26 6:20 AM, alucerop@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> A PCI device can present multiple Physical Functions(PFs) but the CXL
>> specs restrict to the first one, PF0, the discovery and management of
>> CXL capabilities accessed through a PF0 BAR. Other non-PF0 PFs need to
>> obtain the CXL.mem range to work with somehow.
>>
>> Add a device link between the cxl region a PF0 memdev is attached to and
>> the non-PF0 wanting to use the CXL region. A CXL region release will
>> trigger such a PF to be released from its driver first.
>>
>> PF0 being unbound from its driver triggers memdev and region release
>> leading to non-PF0s being unbound first keeping the CXL memory use safe.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/core/memdev.c | 92 +++++++++++++++++++++++++++++++++++++++
>>   include/cxl/cxl.h         |  2 +
>>   2 files changed, 94 insertions(+)
>>
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index b3419df586b9..799cb6e75639 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -802,6 +802,98 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>>   	return ERR_PTR(rc);
>>   }
>>   
>> +static int match_memdev_by_parent_device(struct device *dev, const void *data)
>> +{
>> +	const struct device *pf_dev = data;
>> +	struct cxl_memdev *cxlmd;
>> +
>> +	if (!is_cxl_memdev(dev))
>> +		return 0;
>> +
>> +	cxlmd = to_cxl_memdev(dev);
>> +	return (cxlmd->cxlds->dev == pf_dev);
> cxlmd->cxlds can be NULL? How about just do dev->parent == pf_dev instead?


Not for a type2 memdev. This function should only used inside the next 
one. Maybe it is worth a comment.


>> +}
>> +
>> +static int __cxl_get_range_and_link(struct device *pf0, struct device *pfx,
>> +				    struct range *range)
>> +{
>> +	struct device *mem_dev __free(put_device) =
>> +		bus_find_device(&cxl_bus_type, NULL, pf0,
>> +				match_memdev_by_parent_device);
>> +	struct cxl_attach_region *attach;
>> +	struct cxl_memdev *cxlmd;
>> +
>> +	if (!mem_dev)
>> +		return -ENODEV;
>> +
>> +	cxlmd = to_cxl_memdev(mem_dev);
>> +	attach = container_of(cxlmd->attach, struct cxl_attach_region, attach);
> Probably not likely for a type2 device, but is there any possibility that attach == NULL?


As explained in my reply to your patch2 concern,  this can not happen 
due to the device locking. If the memdev is there, the attach is there. 
All that happens at PF0 initialization, with the PF0 device locked. With 
the locking gone, memdev+attach do exist or they do not.

>> +
>> +	/*
>> +	 * The cxlmd object does exist and it can be found in the cxl bus after
>> +	 * creation but before attach probe setting the proper HPA range. If so,
>> +	 * the caller will need to try later.
>> +	 */
>> +	if (attach->hpa_range.end == CXL_RESOURCE_NONE)
>> +		return -EPROBE_DEFER;
>
> Maybe you'll need something like this below to ensure that the region is valid still. And you can drop the above with the below code.
>
>        scoped_guard(rwsem_read, &cxl_rwsem.region) {
>                cxlr = READ_ONCE(attach->cxlr);
>                if (!cxlr)
>                        return -EPROBE_DEFER;
>                get_device(&cxlr->dev);
>        }
>        struct device *region_dev __free(put_device) = &cxlr->dev;
>
>        /*
>         * Region deletion holds regions_lock across xa_erase() and device_del().
>         * Being in the xarray under regions_lock means the region is still
>         * registered, and a link added now is torn down by its deletion. Drop
>         * cxl_rwsem.region above first: regions_lock nests outside it.
>         */
>        cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
>        guard(mutex)(&cxlrd->regions_lock);
>        if (xa_load(&cxlrd->regions, cxlr->id) != cxlr)
>                return -ENODEV;
>
>        /* A decommit releases the region driver after dropping the rwsem */
>        guard(rwsem_read)(&cxl_rwsem.region);
>        if (cxlr->params.state != CXL_CONFIG_COMMIT)
>                return -ENODEV;


Because what I explained before, with the device locking and the region 
only disappearing at PF0 release  which holds the device lock, I do not 
think this finer concurrency protection is needed.


Thank you,

Alejandro.


> DJ
>
>> +
>> +	/*
>> +	 * Create the device link between the region and the consumer device.
>> +	 * AUTOREMOVE_CONSUMER means the link implicitly to be removed if the
>> +	 * consumer unbinds first with no consequences for the supplier.
>> +	 */
>> +	if (!device_link_add(pfx, &attach->cxlr->dev,
>> +			     DL_FLAG_AUTOREMOVE_CONSUMER)) {
>> +		dev_err(pfx, "device link creation failed\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	range->start = attach->hpa_range.start;
>> +	range->end = attach->hpa_range.end;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * cxl_get_range_and_link - register a device link with the region PF0 memdev
>> + * is attached to. The region release will imply the link consumer to be unbound
>> + * from its driver first. Return the cxl region range to work with related to
>> + * PF0 memdev initialization.
>> + *
>> + * @pf0: device to use for finding target memdev and supplier for the link
>> + * @pfx: device to link to PF0's memdev region, the link consumer.
>> + * @range: to be set with the PF0's memdev attach region range.
>> + *
>> + * Return: 0 or error.
>> + */
>> +int cxl_get_range_and_link(struct device *pf0, struct device *pfx,
>> +			   struct range *range)
>> +{
>> +	int rc;
>> +
>> +	if (!pf0 || !pfx)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * PF0 cxl memdev once created and region attached can only be removed
>> +	 * when PF0 unbinds from its driver which implies to obtain the device
>> +	 * lock before the unwinding starts. If this call from other PF races
>> +	 * with such unbinding:
>> +	 *
>> +	 * 1) if this next lock is obtained first, the device link is
>> +	 *    created and the later unwinding will trigger consumer (PF
>> +	 *    calling here) unbinding first.
>> +	 *
>> +	 *  2) if it is the unbinding the one getting the lock first, the
>> +	 *    memdev will not be there aymore.
>> +	 */
>> +	device_lock(pf0);
>> +	rc = __cxl_get_range_and_link(pf0, pfx, range);
>> +	device_unlock(pf0);
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_get_range_and_link, "CXL");
>> +
>>   static long __cxl_memdev_ioctl(struct cxl_memdev *cxlmd, unsigned int cmd,
>>   			       unsigned long arg)
>>   {
>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>> index 802b143de83d..b28dce1f6f76 100644
>> --- a/include/cxl/cxl.h
>> +++ b/include/cxl/cxl.h
>> @@ -228,4 +228,6 @@ struct cxl_memdev *devm_cxl_probe_mem(struct cxl_dev_state *cxlds,
>>   				      struct range *range);
>>   
>>   int cxl_set_capacity(struct cxl_dev_state *cxlds, u64 capacity);
>> +int cxl_get_range_and_link(struct device *pf0, struct device *pfx,
>> +			   struct range *range);
>>   #endif /* __CXL_CXL_H__ */