[RFC 0/2] Type2 multipf support

[RFC 0/2] Type2 multipf support

[alejandro.lucero-palau]

From: Alejandro Lucero <alucerop@amd.com>

Commit message of first patch explains why this is needed but I want to
add some comments here.

First, the final Type2 basic support was possible once Dan Williams and
I reached an agreement on how to solve the potential unwinding spenarios
linked to a cxl memdev object. The actions triggering this unwinding are:

 - User space unbinding the cxl mem device from the cxl mem driver.
 - User space removing cxl_acpi module.
 - User space unbinding Type2/accelerator pci device from its driver.
 - User space removing Type2/accelerator driver.

The last two trigger the unwinding from the Type2/accelerator driver
exit path, while the first two start the unwinding which in turn invoke
the Type2 driver release from its pci device.

In any case, the decission was to release the Type2 driver always
instead of a degraded functionality if CXL.mem is only part of the full
functionality. This needs to be extended to other non-PF0 PFs, so all
the scenarios listed above ending up releasing those other PFs as well
from their drivers.

This needs to link a memdev to those non-PF0 PFs, and invoke those PFs
release from their drivers when unwinding. This is not complicated per
se but the devil is in the potential race conditions between the
linking/unlinking and the memdev release. I think it does only require
to keep a reference to the memdev device for the time the
linking/unlinking happens, but maybe there are corner cases I did not
think about. I have tested it with real hardware advertising two PFs and
under all the scenarios listed, but stressing this requires another
framework, likely under qemu or adding a new cxl test set.

FWIW, using vanilla 7.2 as cxl next has not the sfc changes yet.

Alejandro Lucero (2):
  cxl/memdev: add support for mutipf device
  sfc: add multipf support

 drivers/cxl/core/memdev.c          | 122 +++++++++++++++++++++++++++++
 drivers/cxl/cxlmem.h               |   1 +
 drivers/net/ethernet/sfc/efx_cxl.c | 106 +++++++++++++++++++++++--
 include/cxl/cxl.h                  |   4 +
 4 files changed, 227 insertions(+), 6 deletions(-)


base-commit: 818bebeb63dd6bf5f4e07e145f6cdbace520a34c
-- 
2.34.1

[RFC 1/2] cxl/memdev: add support for mutipf device

[alejandro.lucero-palau]

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.

Although this could be handled internally by an accelerator/Type2
driver, it requires to properly handle changes to the CXL mem device,
mainly its release by the CXL core, but also potential CXL device
resets. When this release happens, those other PFs need to be told about
it.

Implement a way for non-PF0 PFs to register/unregister to the memdev
linked to the PF0 device. At memdev release, trigger the release of
those non-PF0 PFs devices registered to such memdev from the driver they
are bound to.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/core/memdev.c | 122 ++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxlmem.h      |   1 +
 include/cxl/cxl.h         |   4 ++
 3 files changed, 127 insertions(+)

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index b3419df586b9..327c4da3208f 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -26,6 +26,35 @@ static void cxl_memdev_release(struct device *dev)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
 	struct device *parent = dev->parent;
+	struct device *sibling;
+	unsigned long index;
+
+	/*
+	 * Type2 multipf support implies other non-PF0 PFs could be having a
+	 * temporal reference to the memdev, only for registering/unregistering
+	 * as sibling, requiring to postpone the memdev release and the sibling
+	 * management until no further references. While detach_memdev() calls
+	 * for pf0 release from its driver (parent device of the memdev device)
+	 * it is not safe to invoke for sibling PFs to be detached at that time
+	 * as it could race with PFs registering/unregistering as memdev siblings.
+	 */
+	if (cxlmd->attach) {
+		xa_for_each(&cxlmd->siblings, index, sibling) {
+			device_release_driver(sibling);
+			xa_erase(&cxlmd->siblings, index);
+		}
+
+		/* Several possibilities trigger a memdev release with one being
+		 * its parent device (Type2 device) released from its driver. If
+		 * so, such release is the context for this function, precluding
+		 * the mutex lock and therefore safely avoiding to invoke the
+		 * release again which would trigger a deadlock.
+		 */
+		if (mutex_trylock(&cxlmd->dev.parent->mutex)) {
+			mutex_unlock(&cxlmd->dev.parent->mutex);
+			device_release_driver(cxlmd->dev.parent);
+		}
+	}
 
 	ida_free(&cxl_memdev_ida, cxlmd->id);
 	kfree(cxlmd);
@@ -795,6 +824,7 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
 
 	cdev = &cxlmd->cdev;
 	cdev_init(cdev, fops);
+	xa_init(&cxlmd->siblings);
 	return cxlmd;
 
 err:
@@ -802,6 +832,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);
+}
+
+/**
+ * cxl_get_pf0_memdev - register as PF0's memdev sibling
+ * @pf0: device for PF0 used to match current memdevs.
+ * @pfx: device to register as sibling to PF0's memdev.
+ * @index: where to register the device in the xarray.
+ * @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,
+				      unsigned long index, 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.
+	 */
+
+	xa_store(&cxlmd->siblings, index, pfx, GFP_KERNEL);
+	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)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	range->start =  attach->hpa_range.start;
+	range->end =  attach->hpa_range.end;
+
+	return to_cxl_memdev(mem_dev);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_get_pf0_memdev, "CXL");
+
+/**
+ * cxl_put_pf0_memdev - unregister as PF0's memdev sibling
+ * @pf0: device for PF0 used to match current memdevs.
+ * @pfx: device to register as sibling to PF0's memdev.
+ * @index: where to unregister the device in the xarray.
+ *
+ */
+void cxl_put_pf0_memdev(struct device *pf0, struct device *pfx,
+			unsigned long index)
+{
+	struct cxl_memdev *cxlmd;
+	struct device *mem_dev __free(put_device) =
+		bus_find_device(&cxl_bus_type, NULL, pf0,
+				match_memdev_by_parent_device);
+
+	/*
+	 * This is not an error but a possibility if triggered by PF0 being
+	 * released which triggers the caller driver releasing pfx. It should
+	 * not happen if the caller driver does the release of pfx independently
+	 * but we do not have a simple way to ensure this here.
+	 */
+	if (!mem_dev)
+		return;
+
+	/*
+	 * we got the cxl_memdev and the implicit get_device in bus_find_device
+	 * makes the next steps safe.
+	 */
+
+	cxlmd = to_cxl_memdev(mem_dev);
+	xa_erase(&cxlmd->siblings, index);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_put_pf0_memdev, "CXL");
+
 static long __cxl_memdev_ioctl(struct cxl_memdev *cxlmd, unsigned int cmd,
 			       unsigned long arg)
 {
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index c401e3a1af06..430014c4a046 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -54,6 +54,7 @@
  */
 struct cxl_memdev {
 	struct device dev;
+	struct xarray siblings;
 	struct cdev cdev;
 	struct cxl_dev_state *cxlds;
 	struct work_struct detach_work;
diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
index 802b143de83d..883ce9f1b73f 100644
--- a/include/cxl/cxl.h
+++ b/include/cxl/cxl.h
@@ -228,4 +228,8 @@ 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,
+				      unsigned long index, struct range *range);
+void cxl_put_pf0_memdev(struct device *pf0, struct device *pfx, unsigned long index);
+//struct range *cxl_get_memdev_region_range(struct cxl_memdev *cxlmd);
 #endif /* __CXL_CXL_H__ */
-- 
2.34.1

Re: [RFC 1/2] cxl/memdev: add support for mutipf device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] `cxl_get_pf0_memdev()` returns a pointer after dropping its device reference via `__free()`, causing a Use-After-Free.
- [High] Flawed execution context detection using `mutex_trylock()` leads to silently skipped driver releases.
- [High] `cxl_get_pf0_memdev()` leaves the sibling device in the xarray on the `-EPROBE_DEFER` error path.
- [High] Unconditional dereference of `attach->hpa_range.end` causes a kernel panic if `cxlmd->attach` is NULL.
- [High] Sibling device pointers are stored in the `cxlmd->siblings` xarray without taking a reference count.
- [Medium] The return value of `xa_store()` is ignored, potentially leaving siblings unregistered.
- [Medium] Missing `xa_destroy()` for the `cxlmd->siblings` xarray leads to leaked internal nodes.
--

--- Patch [1]: [RFC 1/2] cxl/memdev: add support for mutipf device ---
commit be73e6cb4711f45100bc8fb00094b292d5fbc718
Author: Alejandro Lucero <alucerop@amd.com>

cxl/memdev: add support for mutipf device

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.

> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index b3419df586b9c..327c4da3208fe 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -26,6 +26,35 @@ static void cxl_memdev_release(struct device *dev)
>  {
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  	struct device *parent = dev->parent;
> +	struct device *sibling;
> +	unsigned long index;
> +
> +	/*
> +	 * Type2 multipf support implies other non-PF0 PFs could be having a

[ ... ]

> +		/* Several possibilities trigger a memdev release with one being
> +		 * its parent device (Type2 device) released from its driver. If
> +		 * so, such release is the context for this function, precluding
> +		 * the mutex lock and therefore safely avoiding to invoke the
> +		 * release again which would trigger a deadlock.
> +		 */
> +		if (mutex_trylock(&cxlmd->dev.parent->mutex)) {

[Severity: High]
Does this trylock reliably detect if it's executing within the context of the
parent device's release callback? If the trylock fails, the code assumes it
is already on the parent release path and skips calling
device_release_driver(). However, couldn't the lock be held by a completely
unrelated thread performing sysfs access, hotplug operations, or power
management? If lock contention occurs, the parent driver release might be
skipped entirely, leaving the parent device permanently bound.

> +			mutex_unlock(&cxlmd->dev.parent->mutex);
> +			device_release_driver(cxlmd->dev.parent);
> +		}
> +	}
>  
>  	ida_free(&cxl_memdev_ida, cxlmd->id);

[Severity: Medium]
Is it necessary to call xa_destroy() for the cxlmd->siblings xarray before
freeing the struct? The xarray is dynamically initialized during alloc, and
if it allocates memory for internal tree nodes, skipping xa_destroy() could
lead to leaked internal nodes even if all entries were logically erased.

>  	kfree(cxlmd);
>  	put_device(parent);
>  }

[ ... ]

> @@ -802,6 +832,98 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,

[ ... ]

> +struct cxl_memdev *cxl_get_pf0_memdev(struct device *pf0, struct device *pfx,
> +				      unsigned long index, 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.
> +	 */
> +
> +	xa_store(&cxlmd->siblings, index, pfx, GFP_KERNEL);

[Severity: High]
Should this xarray store take a reference count on the pfx device before
storing it? Since cxl_get_pf0_memdev() registers the pfx sibling device but
doesn't increment its reference count, if the sibling device is independently
removed and freed, the xarray could retain a dangling pointer. When
cxl_memdev_release() later iterates the xarray and invokes
device_release_driver(sibling), it would dereference this dangling pointer
and cause a use-after-free.

[Severity: Medium]
Is it safe to ignore the return value of xa_store()? Since xa_store()
dynamically allocates memory for new elements and can fail returning an error
pointer, an unhandled failure means the sibling device would be omitted from
the xarray while the function returns success. This might silently compromise
cross-device unbinding logic.

> +	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)

[Severity: High]
Can this unconditionally dereference a NULL attach pointer? Because attach is
at offset 0 within struct cxl_attach_region, if cxlmd->attach is NULL, the
attach pointer evaluates identically to NULL. Standard Type 3 CXL memdevs
lack an attach region, so cxlmd->attach would be NULL, causing an immediate
kernel panic when trying to access attach->hpa_range.end.

[Severity: High]
Does this error path need to clean up the xarray entry before returning?
Since the sibling device was already inserted into cxlmd->siblings, returning
-EPROBE_DEFER here without calling xa_erase() leaves a stale pointer in the
xarray. Later, cxl_memdev_release() could iterate over the xarray and invoke
device_release_driver() on a dangling pointer.

> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	range->start =  attach->hpa_range.start;
> +	range->end =  attach->hpa_range.end;
> +
> +	return to_cxl_memdev(mem_dev);

[Severity: High]
Could this return a pointer after dropping its device reference, causing a
use-after-free? bus_find_device() acquires a reference to the matched device
stored in mem_dev, which is decorated with __free(put_device). Because this
return statement does not inhibit cleanup (for example, by using
no_free_ptr() or return_ptr()), put_device(mem_dev) is automatically executed
at scope exit, instantly dropping the reference while returning an unpinned
pointer.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821155134.260053-1-alejandro.lucero-palau@amd.com?part=1

Re: [RFC 1/2] cxl/memdev: add support for mutipf device

[Richard Cheng]

On Fri, Aug 21, 2026 at 04:51:33PM +0800, alejandro.lucero-palau@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.
> 
> Although this could be handled internally by an accelerator/Type2
> driver, it requires to properly handle changes to the CXL mem device,
> mainly its release by the CXL core, but also potential CXL device
> resets. When this release happens, those other PFs need to be told about
> it.
> 
> Implement a way for non-PF0 PFs to register/unregister to the memdev
> linked to the PF0 device. At memdev release, trigger the release of
> those non-PF0 PFs devices registered to such memdev from the driver they
> are bound to.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/memdev.c | 122 ++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h      |   1 +
>  include/cxl/cxl.h         |   4 ++
>  3 files changed, 127 insertions(+)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index b3419df586b9..327c4da3208f 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -26,6 +26,35 @@ static void cxl_memdev_release(struct device *dev)
>  {
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  	struct device *parent = dev->parent;
> +	struct device *sibling;
> +	unsigned long index;
> +
> +	/*
> +	 * Type2 multipf support implies other non-PF0 PFs could be having a
> +	 * temporal reference to the memdev, only for registering/unregistering
> +	 * as sibling, requiring to postpone the memdev release and the sibling
> +	 * management until no further references. While detach_memdev() calls
> +	 * for pf0 release from its driver (parent device of the memdev device)
> +	 * it is not safe to invoke for sibling PFs to be detached at that time
> +	 * as it could race with PFs registering/unregistering as memdev siblings.
> +	 */
> +	if (cxlmd->attach) {
> +		xa_for_each(&cxlmd->siblings, index, sibling) {
> +			device_release_driver(sibling);
> +			xa_erase(&cxlmd->siblings, index);
> +		}
> +
> +		/* Several possibilities trigger a memdev release with one being
> +		 * its parent device (Type2 device) released from its driver. If
> +		 * so, such release is the context for this function, precluding
> +		 * the mutex lock and therefore safely avoiding to invoke the
> +		 * release again which would trigger a deadlock.
> +		 */
> +		if (mutex_trylock(&cxlmd->dev.parent->mutex)) {
> +			mutex_unlock(&cxlmd->dev.parent->mutex);
> +			device_release_driver(cxlmd->dev.parent);
> +		}
> +	}
>  
>  	ida_free(&cxl_memdev_ida, cxlmd->id);
>  	kfree(cxlmd);
> @@ -795,6 +824,7 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>  
>  	cdev = &cxlmd->cdev;
>  	cdev_init(cdev, fops);
> +	xa_init(&cxlmd->siblings);
>  	return cxlmd;
>  
>  err:
> @@ -802,6 +832,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);
> +}
> +
> +/**
> + * cxl_get_pf0_memdev - register as PF0's memdev sibling
> + * @pf0: device for PF0 used to match current memdevs.
> + * @pfx: device to register as sibling to PF0's memdev.
> + * @index: where to register the device in the xarray.
> + * @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,
> +				      unsigned long index, 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.
> +	 */
> +
> +	xa_store(&cxlmd->siblings, index, pfx, GFP_KERNEL);
> +	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)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	range->start =  attach->hpa_range.start;
> +	range->end =  attach->hpa_range.end;
> +
> +	return to_cxl_memdev(mem_dev);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_pf0_memdev, "CXL");
> +
> +/**
> + * cxl_put_pf0_memdev - unregister as PF0's memdev sibling
> + * @pf0: device for PF0 used to match current memdevs.
> + * @pfx: device to register as sibling to PF0's memdev.
> + * @index: where to unregister the device in the xarray.
> + *
> + */
> +void cxl_put_pf0_memdev(struct device *pf0, struct device *pfx,
> +			unsigned long index)
> +{
> +	struct cxl_memdev *cxlmd;
> +	struct device *mem_dev __free(put_device) =
> +		bus_find_device(&cxl_bus_type, NULL, pf0,
> +				match_memdev_by_parent_device);
> +
> +	/*
> +	 * This is not an error but a possibility if triggered by PF0 being
> +	 * released which triggers the caller driver releasing pfx. It should
> +	 * not happen if the caller driver does the release of pfx independently
> +	 * but we do not have a simple way to ensure this here.
> +	 */
> +	if (!mem_dev)
> +		return;
> +
> +	/*
> +	 * we got the cxl_memdev and the implicit get_device in bus_find_device
> +	 * makes the next steps safe.
> +	 */
> +
> +	cxlmd = to_cxl_memdev(mem_dev);
> +	xa_erase(&cxlmd->siblings, index);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_put_pf0_memdev, "CXL");
> +
>  static long __cxl_memdev_ioctl(struct cxl_memdev *cxlmd, unsigned int cmd,
>  			       unsigned long arg)
>  {
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index c401e3a1af06..430014c4a046 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -54,6 +54,7 @@
>   */
>  struct cxl_memdev {
>  	struct device dev;
> +	struct xarray siblings;
>  	struct cdev cdev;
>  	struct cxl_dev_state *cxlds;
>  	struct work_struct detach_work;
> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> index 802b143de83d..883ce9f1b73f 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -228,4 +228,8 @@ 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,
> +				      unsigned long index, struct range *range);
> +void cxl_put_pf0_memdev(struct device *pf0, struct device *pfx, unsigned long index);
> +//struct range *cxl_get_memdev_region_range(struct cxl_memdev *cxlmd);
>  #endif /* __CXL_CXL_H__ */
> -- 
> 2.34.1
> 
>

Hi Alejandro,

I agree that non-PF0 functions must stop using the CXL range before it disappears.
However, I don't think cxl_memdev_release() is the right place to unbind them.

For Type-2 device, PF0 driver creates the memdev, and therefore the memdev
is tied to the PF0 driver, while CXL core manages its attachment to
the topology.

cxl_memdev_release() is the final object release callback. It can run much
later than the CXL attachment teardown.
For example, an open /dev/cxl/memX file holds a device ref. PF0 and the CXL
region can be torn down while that file remains open. During that time,
other PF driver remain bound with mapping to a range that's no longer valid.

I suggest replacing cxl_get_pf0_memdev() and cxl_put_pf0_memdev() with another
helper, e.g.:

int cxl_memdev_link_consumer(struct device *pf0, struct device *consumer, struct range *range);
 
It should live in cxl/core/memdev.c , and the behavior is something like

1. Find PF0's memdev and take a temp ref.
2. Lock the memdev
3. Verify that the memdev is still registered, driver-bound, attached, and has a valid HPA range
4. Create a managed devce link via device_link_add(consumer, &cxlmd->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
5. Copy the HPA range
6. Unlock the memdev and drop the temp ref

This helper can return only an error code and the range. The sfc driver doesn't need the cxl_memdev pointer then, and
no put helper would be needed.

And driver core would unbind the non-PF0 consumer before unbinding or removing the supplier ( memdev ).

I think this can remove the sibling xarray , raw device pointers and mutex_trylock() context check.

Would this modle work for your teardown requirements ?

Best regards,
Richard Cheng.

Re: [RFC 1/2] cxl/memdev: add support for mutipf device

[Lucero Palau, Alejandro]

On 24/08/2026 09:33, Richard Cheng wrote:
> On Fri, Aug 21, 2026 at 04:51:33PM +0800,alejandro.lucero-palau@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.
>>
>> Although this could be handled internally by an accelerator/Type2
>> driver, it requires to properly handle changes to the CXL mem device,
>> mainly its release by the CXL core, but also potential CXL device
>> resets. When this release happens, those other PFs need to be told about
>> it.
>>
>> Implement a way for non-PF0 PFs to register/unregister to the memdev
>> linked to the PF0 device. At memdev release, trigger the release of
>> those non-PF0 PFs devices registered to such memdev from the driver they
>> are bound to.
>>
>> Signed-off-by: Alejandro Lucero<alucerop@amd.com>
>> ---
>>   drivers/cxl/core/memdev.c | 122 ++++++++++++++++++++++++++++++++++++++
>>   drivers/cxl/cxlmem.h      |   1 +
>>   include/cxl/cxl.h         |   4 ++
>>   3 files changed, 127 insertions(+)
>>
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index b3419df586b9..327c4da3208f 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -26,6 +26,35 @@ static void cxl_memdev_release(struct device *dev)
>>   {
>>   	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>>   	struct device *parent = dev->parent;
>> +	struct device *sibling;
>> +	unsigned long index;
>> +
>> +	/*
>> +	 * Type2 multipf support implies other non-PF0 PFs could be having a
>> +	 * temporal reference to the memdev, only for registering/unregistering
>> +	 * as sibling, requiring to postpone the memdev release and the sibling
>> +	 * management until no further references. While detach_memdev() calls
>> +	 * for pf0 release from its driver (parent device of the memdev device)
>> +	 * it is not safe to invoke for sibling PFs to be detached at that time
>> +	 * as it could race with PFs registering/unregistering as memdev siblings.
>> +	 */
>> +	if (cxlmd->attach) {
>> +		xa_for_each(&cxlmd->siblings, index, sibling) {
>> +			device_release_driver(sibling);
>> +			xa_erase(&cxlmd->siblings, index);
>> +		}
>> +
>> +		/* Several possibilities trigger a memdev release with one being
>> +		 * its parent device (Type2 device) released from its driver. If
>> +		 * so, such release is the context for this function, precluding
>> +		 * the mutex lock and therefore safely avoiding to invoke the
>> +		 * release again which would trigger a deadlock.
>> +		 */
>> +		if (mutex_trylock(&cxlmd->dev.parent->mutex)) {
>> +			mutex_unlock(&cxlmd->dev.parent->mutex);
>> +			device_release_driver(cxlmd->dev.parent);
>> +		}
>> +	}
>>   
>>   	ida_free(&cxl_memdev_ida, cxlmd->id);
>>   	kfree(cxlmd);
>> @@ -795,6 +824,7 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>>   
>>   	cdev = &cxlmd->cdev;
>>   	cdev_init(cdev, fops);
>> +	xa_init(&cxlmd->siblings);
>>   	return cxlmd;
>>   
>>   err:
>> @@ -802,6 +832,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);
>> +}
>> +
>> +/**
>> + * cxl_get_pf0_memdev - register as PF0's memdev sibling
>> + * @pf0: device for PF0 used to match current memdevs.
>> + * @pfx: device to register as sibling to PF0's memdev.
>> + * @index: where to register the device in the xarray.
>> + * @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,
>> +				      unsigned long index, 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.
>> +	 */
>> +
>> +	xa_store(&cxlmd->siblings, index, pfx, GFP_KERNEL);
>> +	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)
>> +		return ERR_PTR(-EPROBE_DEFER);
>> +
>> +	range->start =  attach->hpa_range.start;
>> +	range->end =  attach->hpa_range.end;
>> +
>> +	return to_cxl_memdev(mem_dev);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_get_pf0_memdev, "CXL");
>> +
>> +/**
>> + * cxl_put_pf0_memdev - unregister as PF0's memdev sibling
>> + * @pf0: device for PF0 used to match current memdevs.
>> + * @pfx: device to register as sibling to PF0's memdev.
>> + * @index: where to unregister the device in the xarray.
>> + *
>> + */
>> +void cxl_put_pf0_memdev(struct device *pf0, struct device *pfx,
>> +			unsigned long index)
>> +{
>> +	struct cxl_memdev *cxlmd;
>> +	struct device *mem_dev __free(put_device) =
>> +		bus_find_device(&cxl_bus_type, NULL, pf0,
>> +				match_memdev_by_parent_device);
>> +
>> +	/*
>> +	 * This is not an error but a possibility if triggered by PF0 being
>> +	 * released which triggers the caller driver releasing pfx. It should
>> +	 * not happen if the caller driver does the release of pfx independently
>> +	 * but we do not have a simple way to ensure this here.
>> +	 */
>> +	if (!mem_dev)
>> +		return;
>> +
>> +	/*
>> +	 * we got the cxl_memdev and the implicit get_device in bus_find_device
>> +	 * makes the next steps safe.
>> +	 */
>> +
>> +	cxlmd = to_cxl_memdev(mem_dev);
>> +	xa_erase(&cxlmd->siblings, index);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_put_pf0_memdev, "CXL");
>> +
>>   static long __cxl_memdev_ioctl(struct cxl_memdev *cxlmd, unsigned int cmd,
>>   			       unsigned long arg)
>>   {
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index c401e3a1af06..430014c4a046 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -54,6 +54,7 @@
>>    */
>>   struct cxl_memdev {
>>   	struct device dev;
>> +	struct xarray siblings;
>>   	struct cdev cdev;
>>   	struct cxl_dev_state *cxlds;
>>   	struct work_struct detach_work;
>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>> index 802b143de83d..883ce9f1b73f 100644
>> --- a/include/cxl/cxl.h
>> +++ b/include/cxl/cxl.h
>> @@ -228,4 +228,8 @@ 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,
>> +				      unsigned long index, struct range *range);
>> +void cxl_put_pf0_memdev(struct device *pf0, struct device *pfx, unsigned long index);
>> +//struct range *cxl_get_memdev_region_range(struct cxl_memdev *cxlmd);
>>   #endif /* __CXL_CXL_H__ */
>> -- 
>> 2.34.1
>>
>>
> Hi Alejandro,


Hi Richard,


Thanks for the review.


Replying below to your suggestion.


> I agree that non-PF0 functions must stop using the CXL range before it disappears.
> However, I don't think cxl_memdev_release() is the right place to unbind them.


For solving concurrency issues, I had to delay the real detachment to 
the time memdev is released. This is the problem:


1) PF2 is bound to the driver and probed. The code path reaches the 
point where the PF0 memdev is obtained for adding this PF as a sibling.

2) PF0 memdev detach happens. The non-PF0 functions attached need to be 
told triggering their driver's release.


Both are going to modify the list of attached non-PF0 functions, so some 
locking is required, or the memdev could go without awareness of the 
incoming sibling. For example, the xa_store will happen, as a memdev 
reference was taken, but the memdev detachment based on xa_for_each 
could miss it (RCU read) and nothing else will not look later on for it, 
leaving PF2 using it as the driver release call does not happen. Note 
same situation but for PF2 being independently release do not need the 
locking since xarrays intrinsics will be enough and no problem if 
xa_erase happens twice as the index is unique per PF.


The problem with locking is if the second thread gets it, the first one 
will be waiting. Then the second thread will call for PF2 detachment 
implying call to driver release ... which will try to get the PF2 device 
lock in use by the independent PF2 release ... leading to a deadlock. 
Checking if the lock can be obtained is not enough for dismissing the 
call to driver release.


> For Type-2 device, PF0 driver creates the memdev, and therefore the memdev
> is tied to the PF0 driver, while CXL core manages its attachment to
> the topology.
>
> cxl_memdev_release() is the final object release callback. It can run much
> later than the CXL attachment teardown.
> For example, an open /dev/cxl/memX file holds a device ref. PF0 and the CXL
> region can be torn down while that file remains open. During that time,
> other PF driver remain bound with mapping to a range that's no longer valid.


Right. I did not think about this possibility, but I think the memdev is 
safe to be used as long as the related CXL region is present which 
requires to preserve the endpoint HDM (plus the Root Port related HDM). 
In other words, I think for this case extra device references to 
cxlr->dev are necessary. I need to think about this with more care.


> I suggest replacing cxl_get_pf0_memdev() and cxl_put_pf0_memdev() with another
> helper, e.g.:
>
> int cxl_memdev_link_consumer(struct device *pf0, struct device *consumer, struct range *range);
>   
> It should live in cxl/core/memdev.c , and the behavior is something like
>
> 1. Find PF0's memdev and take a temp ref.
> 2. Lock the memdev
> 3. Verify that the memdev is still registered, driver-bound, attached, and has a valid HPA range
> 4. Create a managed devce link via device_link_add(consumer, &cxlmd->dev, DL_FLAG_AUTOREMOVE_CONSUMER);


Interesting approach.


Not sure this could do the proper thing though. 
DL_FLAG_AUTOREMOVE_CONSUMER seems to remove the link, cxlmd->dev in your 
case, when consume driver unbinds ... but it is the other way what we 
need. Maybe I do not understand well all the implications with this 
approach, so let me study it.

Thanks!

> 5. Copy the HPA range
> 6. Unlock the memdev and drop the temp ref
>
> This helper can return only an error code and the range. The sfc driver doesn't need the cxl_memdev pointer then, and
> no put helper would be needed.
>
> And driver core would unbind the non-PF0 consumer before unbinding or removing the supplier ( memdev ).
>
> I think this can remove the sibling xarray , raw device pointers and mutex_trylock() context check.
>
> Would this modle work for your teardown requirements ?
>
> Best regards,
> Richard Cheng.

[RFC 2/2] sfc: add multipf support

[alejandro.lucero-palau]

From: Alejandro Lucero <alucerop@amd.com>

Use CXL core accelerator API for registering non-PF0 PFs to the memdev
linked to the PF0, along with its complementary unregister.

Adapt the ioremap call per PF to be an offset based on the PF function
index and a hardcoded per PF CXL.mem slot size.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/net/ethernet/sfc/efx_cxl.c | 106 +++++++++++++++++++++++++++--
 1 file changed, 100 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
index 348d7404cd7a..e45c8dd1969c 100644
--- a/drivers/net/ethernet/sfc/efx_cxl.c
+++ b/drivers/net/ethernet/sfc/efx_cxl.c
@@ -13,6 +13,48 @@
 #include "efx_cxl.h"
 
 #define EFX_CTPIO_BUFFER_SIZE	SZ_256M
+#define EFX_CTPIO_BUFFER_PER_PF_SIZE	SZ_8M
+
+#define X4_PF_DEVICE_ID 0x0c03
+
+static struct pci_dev *get_pf0_pci_device(struct pci_dev *pfx)
+{
+	struct pci_dev *pf0_pci_dev;
+
+	while ((pf0_pci_dev = pci_get_device(PCI_VENDOR_ID_SOLARFLARE,
+					     X4_PF_DEVICE_ID, pf0_pci_dev))
+					     != NULL) {
+		/* With multiple X4 installed check against pci_slot as well. */
+		if (pf0_pci_dev->slot == pfx->slot &&
+		    PCI_FUNC(pf0_pci_dev->devfn) == 0)
+			break;
+	}
+	return pf0_pci_dev;
+}
+
+static int cxl_map(struct efx_probe_data *probe_data, struct efx_cxl *cxl,
+		   u64 devfn, struct range cxl_pio_range)
+{
+	struct efx_nic *efx = &probe_data->efx;
+	struct pci_dev *pci_dev = efx->pci_dev;
+	u64 cxl_pio_pf_start;
+
+	cxl_pio_pf_start = cxl_pio_range.start + devfn *
+			   EFX_CTPIO_BUFFER_PER_PF_SIZE;
+
+	cxl->ctpio_cxl = ioremap_wc(cxl_pio_pf_start,
+				    EFX_CTPIO_BUFFER_PER_PF_SIZE);
+	if (!cxl->ctpio_cxl) {
+		pci_err(pci_dev, "CXL ioremap region (%pra) failed\n",
+			&cxl_pio_range);
+		return -ENOMEM;
+	}
+
+	probe_data->cxl = cxl;
+	probe_data->cxl_pio_initialised = true;
+
+	return 0;
+}
 
 int efx_cxl_init(struct efx_probe_data *probe_data)
 {
@@ -20,9 +62,48 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
 	struct pci_dev *pci_dev = efx->pci_dev;
 	struct range cxl_pio_range;
 	struct efx_cxl *cxl;
+	u8 devfn;
 	u16 dvsec;
 	int rc;
 
+	if (efx->type->is_vf)
+		return 0;
+
+	/* are we PF0? */
+	devfn = PCI_FUNC(pci_dev->devfn);
+	if (devfn != 0) {
+		struct pci_dev *pf0_pci_dev;
+		struct cxl_memdev *cxlmd;
+
+		pf0_pci_dev = get_pf0_pci_device(pci_dev);
+
+		/* This should not happen! */
+		if (!pf0_pci_dev)
+			return 0;
+
+		/* Is the PF0 device configured with and using CXL? */
+		if (!pcie_is_cxl(pf0_pci_dev))
+			return 0;
+
+		cxlmd = cxl_get_pf0_memdev(&pf0_pci_dev->dev, &pci_dev->dev,
+					   devfn, &cxl_pio_range);
+
+		if (IS_ERR(cxlmd))
+			return -EPROBE_DEFER;
+
+		cxl = kzalloc_obj(struct cxl);
+		if (!cxl)
+			return -ENOMEM;
+
+		cxl->cxlmd = cxlmd;
+
+		if (!cxl_map(probe_data, cxl, (u64)devfn, cxl_pio_range)) {
+			kfree(cxl);
+			return -ENOMEM;
+		}
+		return 0;
+	}
+
 	/* Is the device configured with and using CXL? */
 	if (!pcie_is_cxl(pci_dev))
 		return 0;
@@ -80,25 +161,38 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
 		return PTR_ERR(cxl->cxlmd);
 	}
 
-	cxl->ctpio_cxl = ioremap_wc(cxl_pio_range.start,
-				    range_len(&cxl_pio_range));
-	if (!cxl->ctpio_cxl) {
+	if (!cxl_map(probe_data, cxl, 0, cxl_pio_range)) {
 		pci_err(pci_dev, "CXL ioremap region (%pra) failed\n",
 			&cxl_pio_range);
 		return -ENOMEM;
 	}
 
-	probe_data->cxl_pio_initialised = true;
-	probe_data->cxl = cxl;
-
 	return 0;
 }
 
 void efx_cxl_exit(struct efx_probe_data *probe_data)
 {
+	struct efx_nic *efx = &probe_data->efx;
+	struct pci_dev *pci_dev = efx->pci_dev;
+	u8 devfn;
+
 	if (!probe_data->cxl)
 		return;
 
+	/* are we PF0? */
+	devfn = PCI_FUNC(pci_dev->devfn);
+	if (devfn != 0) {
+		struct pci_dev *pf0_pci_dev;
+
+		pf0_pci_dev = get_pf0_pci_device(pci_dev);
+
+		/* This should not happen! */
+		if (!pf0_pci_dev)
+			return;
+
+		cxl_put_pf0_memdev(&pf0_pci_dev->dev, &pci_dev->dev, devfn);
+	}
+
 	iounmap(probe_data->cxl->ctpio_cxl);
 }
 
-- 
2.34.1

Re: [RFC 2/2] sfc: add multipf support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] The variable `pf0_pci_dev` is used uninitialized as the starting pointer for the `pci_get_device()` search, leading to memory corruption or crashes.
- [High] The return value of `cxl_map()` is checked using inverted logic, treating success as failure (causing a Use-After-Free) and failure as success (silently bypassing initialization failure).
- [High] The `struct pci_dev` reference acquired by `get_pf0_pci_device()` is never released, resulting in a persistent object leak.
- [High] Memory is dynamically allocated for the undefined or incorrect type `struct cxl` instead of `struct efx_cxl`.
- [High] The PF0 device matching logic compares `pci_dev->slot`, which can be NULL, potentially associating unrelated physical devices.
- [Medium] Missing teardown of `cxl_get_pf0_memdev()` on error paths in `efx_cxl_init()`.
- [Medium] The manually allocated `struct efx_cxl` for non-PF0 devices is permanently leaked on driver teardown.
- [Medium] An early return in the `efx_cxl_exit()` non-PF0 path permanently leaks the I/O memory mapping.
--

--- Patch [2]: [RFC 2/2] sfc: add multipf support ---
commit e798dff2eb0bc8eafa5e5bca36069e0989d743a9
Author: Alejandro Lucero <alucerop@amd.com>

sfc: add multipf support

Use CXL core accelerator API for registering non-PF0 PFs to the memdev
linked to the PF0, along with its complementary unregister. Adapt the
ioremap call per PF to be an offset based on the PF function index and
a hardcoded per PF CXL.mem slot size.

> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 348d7404cd7aa..e45c8dd1969c2 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -13,6 +13,48 @@
>  #include "efx_cxl.h"
>  
>  #define EFX_CTPIO_BUFFER_SIZE	SZ_256M
> +#define EFX_CTPIO_BUFFER_PER_PF_SIZE	SZ_8M
> +
> +#define X4_PF_DEVICE_ID 0x0c03
> +
> +static struct pci_dev *get_pf0_pci_device(struct pci_dev *pfx)
> +{
> +	struct pci_dev *pf0_pci_dev;
> +
> +	while ((pf0_pci_dev = pci_get_device(PCI_VENDOR_ID_SOLARFLARE,
> +					     X4_PF_DEVICE_ID, pf0_pci_dev))
> +					     != NULL) {

[Severity: High]
Since pf0_pci_dev is an uninitialized stack variable on the first iteration,
will pci_get_device() attempt to drop a reference on a wild pointer via its
internal pci_dev_put() call?

> +		/* With multiple X4 installed check against pci_slot as well. */
> +		if (pf0_pci_dev->slot == pfx->slot &&

[Severity: High]
Could the slot pointers be NULL on systems where PCI slots are not described
by firmware?

If so, a NULL == NULL comparison could inadvertently match the first PF0 of
any X4 card in the system, even if it is a completely different physical
adapter. Does this require checking bus or domain topology instead?

> +		    PCI_FUNC(pf0_pci_dev->devfn) == 0)
> +			break;
> +	}
> +	return pf0_pci_dev;
> +}

[ ... ]

> @@ -20,9 +62,48 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>  	struct pci_dev *pci_dev = efx->pci_dev;
>  	struct range cxl_pio_range;
>  	struct efx_cxl *cxl;
> +	u8 devfn;
>  	u16 dvsec;
>  	int rc;
>  
> +	if (efx->type->is_vf)
> +		return 0;
> +
> +	/* are we PF0? */
> +	devfn = PCI_FUNC(pci_dev->devfn);
> +	if (devfn != 0) {
> +		struct pci_dev *pf0_pci_dev;
> +		struct cxl_memdev *cxlmd;
> +
> +		pf0_pci_dev = get_pf0_pci_device(pci_dev);

[Severity: High]
Are we leaking the pf0_pci_dev reference here?

The pci_get_device() function increments the reference count of the returned
PCI device. Is there a missing pci_dev_put(pf0_pci_dev) after this function
is done using it?

> +
> +		/* This should not happen! */
> +		if (!pf0_pci_dev)
> +			return 0;
> +
> +		/* Is the PF0 device configured with and using CXL? */
> +		if (!pcie_is_cxl(pf0_pci_dev))
> +			return 0;
> +
> +		cxlmd = cxl_get_pf0_memdev(&pf0_pci_dev->dev, &pci_dev->dev,
> +					   devfn, &cxl_pio_range);
> +
> +		if (IS_ERR(cxlmd))
> +			return -EPROBE_DEFER;
> +
> +		cxl = kzalloc_obj(struct cxl);

[Severity: High]
Is the struct type passed to kzalloc_obj() correct?

It looks like this allocates memory for 'struct cxl' instead of the intended
'struct efx_cxl'. Will this lead to heap out-of-bounds writes when fields like
cxl->cxlmd or cxl->ctpio_cxl are accessed?

> +		if (!cxl)
> +			return -ENOMEM;

[Severity: Medium]
If this allocation fails, does the code leak the sibling entry created earlier
by cxl_get_pf0_memdev()?

Should there be a call to cxl_put_pf0_memdev() before returning?

> +
> +		cxl->cxlmd = cxlmd;
> +
> +		if (!cxl_map(probe_data, cxl, (u64)devfn, cxl_pio_range)) {

[Severity: High]
Is the return check for cxl_map() inverted here?

Looking at cxl_map(), it returns 0 on success. If cxl_map() succeeds, the
condition (!0) evaluates to true.

Does this mean the success path incorrectly triggers error handling, which
calls kfree(cxl) while leaving a dangling pointer in probe_data->cxl,
causing a use-after-free?

> +			kfree(cxl);
> +			return -ENOMEM;
> +		}
> +		return 0;
> +	}

[ ... ]

> @@ -80,25 +161,38 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>  		return PTR_ERR(cxl->cxlmd);
>  	}
>  
> -	cxl->ctpio_cxl = ioremap_wc(cxl_pio_range.start,
> -				    range_len(&cxl_pio_range));
> -	if (!cxl->ctpio_cxl) {
> +	if (!cxl_map(probe_data, cxl, 0, cxl_pio_range)) {

[Severity: High]
Does this have the same inverted return logic as the non-PF0 case?

If cxl_map() returns 0 on success, will this evaluate to true on success,
incorrectly logging an error and returning -ENOMEM?

>  		pci_err(pci_dev, "CXL ioremap region (%pra) failed\n",
>  			&cxl_pio_range);
>  		return -ENOMEM;
>  	}
>  
> -	probe_data->cxl_pio_initialised = true;
> -	probe_data->cxl = cxl;
> -
>  	return 0;
>  }
>  
>  void efx_cxl_exit(struct efx_probe_data *probe_data)
>  {
> +	struct efx_nic *efx = &probe_data->efx;
> +	struct pci_dev *pci_dev = efx->pci_dev;
> +	u8 devfn;
> +
>  	if (!probe_data->cxl)
>  		return;
>  
> +	/* are we PF0? */
> +	devfn = PCI_FUNC(pci_dev->devfn);
> +	if (devfn != 0) {
> +		struct pci_dev *pf0_pci_dev;
> +
> +		pf0_pci_dev = get_pf0_pci_device(pci_dev);
> +
> +		/* This should not happen! */
> +		if (!pf0_pci_dev)
> +			return;

[Severity: Medium]
If this early return is taken, does it bypass the trailing iounmap() call
and permanently leak the CXL PIO mapping?

> +
> +		cxl_put_pf0_memdev(&pf0_pci_dev->dev, &pci_dev->dev, devfn);

[Severity: High]
Does get_pf0_pci_device() leak another reference to the PCI device here,
similar to the init path?

> +	}
> +
>  	iounmap(probe_data->cxl->ctpio_cxl);

[Severity: Medium]
For non-PF0 devices, probe_data->cxl is allocated dynamically using
kzalloc_obj() during initialization. Is there a missing kfree(probe_data->cxl)
in this teardown path?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821155134.260053-1-alejandro.lucero-palau@amd.com?part=2