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

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

On 10/1/26 3:11 PM, 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?
> 
>> +}
>> +
>> +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?
> 
>> +
>> +	/*
>> +	 * 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;
And also

      /*
       * The link only unbinds @pfx when the region driver is released, so a
       * region without a driver would leave @pfx bound past region removal.
       * Hold the device lock so the driver can not go away underneath.
       */
      guard(device)(&cxlr->dev);
      if (!cxlr->dev.driver)
              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;
> 
> 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__ */
> 
>