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

[Dave Jiang <dave.jiang@intel.com>]

On 9/22/26 7:07 AM, Lucero Palau, Alejandro wrote:
> 
> On 22/09/2026 00:07, Dave Jiang wrote:
>>
>> On 9/21/26 12:12 PM, 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 | 66 +++++++++++++++++++++++++++++++++++++++
>>>   include/cxl/cxl.h         |  1 +
>>>   2 files changed, 67 insertions(+)
>>>
>>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>>> index b3419df586b9..67be02faa7e1 100644
>>> --- a/drivers/cxl/core/memdev.c
>>> +++ b/drivers/cxl/core/memdev.c
>>> @@ -802,6 +802,72 @@ 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);
>>> +}
>>> +
>>> +/**
>>> + * cxl_get_pf0_memdev - 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.
>>> + *
>>> + * @pf0: device to use for finding target memdev.
>>> + * @pfx: device to link to PF0's memdev region, the link consumer.
>>> + * @range: to be set with the PF0's memdev range.
>>> + *
>>> + * Return: PF0 memdev pointer or error.
>>> + */
>>> +struct cxl_memdev *cxl_get_pf0_memdev(struct device *pf0, struct device *pfx,
>> cxl_link_to_pf0_region() may be a better name? cxl_get_pf0_memdev() hides the intention of linking.
> 
> 
> Uhmm. Not sure. It does hide the linking, but the main point is to get the CXL HPA range to work with, an in kernel parlance to get versus put is what I had in mind. The device link is how safely the PF can use the CXL memory. Noting now that the function description forgot to say about the HPA range ...

Ok just bike shedding here. cxl_link_and_retrieve_pf0_region()?

> 
> 
>>> +                      struct range *range)
>>> +{
>>> +    struct cxl_attach_region *attach;
>>> +    struct cxl_memdev *cxlmd;
>>> +    struct device *mem_dev __free(put_device) =
>>> +        bus_find_device(&cxl_bus_type, NULL, pf0,
>>> +                match_memdev_by_parent_device);
>>> +
>>> +    if (!mem_dev)
>>> +        return ERR_PTR(-ENODEV);
>>> +
>>> +    cxlmd = to_cxl_memdev(mem_dev);
>>> +
>>> +    /*
>>> +     * we got the cxl_memdev and the implicit get_device in bus_find_device
>>> +     * makes the next steps safe.
>>> +     */
>>> +    attach = container_of(cxlmd->attach, struct cxl_attach_region, attach);
>>> +
>>> +    /*
>>> +     * 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 == -1)
>> CXL_RESOURCE_NONE instead of -1?
> 
> 
> OK.
> 
> 
>>
>>> +        return ERR_PTR(-EPROBE_DEFER);
>>> +
>>> +    /*
>>> +     * 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))
>>> +        return ERR_PTR(-ENODEV);
>>> +
>>> +    range->start = attach->hpa_range.start;
>>> +    range->end = attach->hpa_range.end;
>>> +
>>> +    return to_cxl_memdev(mem_dev);
>> Should we bother returning cxl_memdev? Does the SFC driver consumer it at all?
> 
> 
> It does not consume the pointer but it uses the memdev indirectly ... this supports my previous comment about "getting" the memdev, but you are right, the pointer does not need to be given.
> 
> 
> Maybe to return the HPA instead, but the call needs to support EPROBE_DEFER, so returning an int would work. What do you think?

Yeah returning an int would work. Standard errno/success return.

DJ

> 
> 
> Thanks,
> 
> Alejandro.
> 
> 
>> DJ
>>
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(cxl_get_pf0_memdev, "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..e3b1e5be95f8 100644
>>> --- a/include/cxl/cxl.h
>>> +++ b/include/cxl/cxl.h
>>> @@ -228,4 +228,5 @@ 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);
>>> +struct cxl_memdev *cxl_get_pf0_memdev(struct device *pf0, struct device *pfx, struct range *range);
>>>   #endif /* __CXL_CXL_H__ */