[PATCH v2 0/4] Type2 multipf support

[PATCH v2 0/4] Type2 multipf support

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

Changes since v1:
 
 - Simplify drivers core required change
 - Change name in the new accelerator API function (Dave)
 - Do not return cxl memdev but int (Dave)
 - Refactor sfc cxl initialization for the two cases (Jonathan)
 - Restrict sfc_cxl_map to only mapping (Jonathan)
 - Addressing some sashiko reports

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.

This patchset adds support for getting the CXL HPA range other non-PF0s
can use based on what PF0 did initialize. This implies CXL for those PFs
can only be used if PF0 is bound and successfully initialised CXL. This
needs to cover the potential race between PFs probing where non-PF0s
will be deferred if the expected CXL is not ready yet. Moreover, those
PFs can not keep using such CXL memory if PF0 is unbound what can
happen in different scenarios.

This first version after the RFC uses device links as proposed by
Richard Cheng which simplifies the support avoiding specific handling of
CXL memdev siblings as the RFC did. Using device links needs a change to
this core kernel functionality for allowing suppliers without power
management initialised covered in patch 1. The second patch adds a new
field to cxl_attach_region struct for facilitating the device link using
the cxl region device where the Type2 memdev is attached to.

As stated with the RFC:

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.

I have tested this patchset  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.

The base is vanilla 7.3-rc4.

Alejandro Lucero (4):
  driver core: Check for supplier requiring PM at link creation
  cxl/region: Add region reference in memdev attach
  cxl/memdev: Add support for multi PF devices
  sfc: add multipf support

 drivers/base/core.c                |   2 +-
 drivers/cxl/core/memdev.c          |  92 ++++++++++++++++++++++++++
 drivers/cxl/core/region.c          |   1 +
 drivers/cxl/cxlmem.h               |   2 +
 drivers/net/ethernet/sfc/efx_cxl.c | 100 ++++++++++++++++++++++++++---
 include/cxl/cxl.h                  |   2 +
 6 files changed, 189 insertions(+), 10 deletions(-)


base-commit: 93f51579e7df248780214094418f205253383cc5
-- 
2.34.1

[PATCH v2 1/4] driver core: Check for supplier requiring PM at link creation

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

PM initialization could not be necessary for some devices.

Avoid checking for supplier PM initialization if so.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/base/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4c0c373998a1..bf0513beafad 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -840,7 +840,7 @@ struct device_link *device_link_add(struct device *consumer,
 	 * SYNC_STATE_ONLY link, we don't check for reverse dependencies
 	 * because it only affects sync_state() callbacks.
 	 */
-	if (!device_pm_initialized(supplier)
+	if ((!device_pm_not_required(supplier) && !device_pm_initialized(supplier))
 	    || (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
 		  device_is_dependent(consumer, supplier))) {
 		link = NULL;
-- 
2.34.1

[PATCH v2 2/4] cxl/region: Add region reference in memdev attach

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

Use a new field in cxl_attach_region struct for easily link it with the
region the memdev is attached to.

This facilitates device links creation where such a region is the supplier
with non-PF0 physical functions wanting to use the CXL region being the
consumers.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 27e63e6dab7c..78ca7ebc3e55 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -4132,6 +4132,7 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
 	if (rc)
 		return rc;
 
+	attach->cxlr = cxlr;
 	attach->hpa_range = (struct range) {
 		.start = cxlr->params.res->start,
 		.end = cxlr->params.res->end,
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index c401e3a1af06..c598561b8e5f 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -104,6 +104,7 @@ struct cxl_memdev_attach {
 /**
  * struct cxl_attach_region - coordinate mapping a region at memdev registration
  * @attach: common core attachment descriptor
+ * @cxlr: cxl region the memdev is attached to.
  * @hpa_range: physical address range of the region
  *
  * For the common simple case of a CXL device with private (non-general purpose
@@ -112,6 +113,7 @@ struct cxl_memdev_attach {
  */
 struct cxl_attach_region {
 	struct cxl_memdev_attach attach;
+	struct cxl_region *cxlr;
 	struct range hpa_range;
 };
 
-- 
2.34.1

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

[alucerop]

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);
+}
+
+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);
+
+	/*
+	 * 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;
+
+	/*
+	 * 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__ */
-- 
2.34.1

[PATCH v2 4/4] sfc: add multipf support

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

Use CXL core accelerator API for linking a non-PF0 PF to the CXL region
its related PF0 CXL memdev is attached to, allowing non-PF0 PF release if
such a CXL region is released itself. This can occur in different
scenarios like PF0 release or CXL memdev release.

Obtain the CXL HPA region to work with and the ioremap based on such
HPA and an offset based on the PF index.

Refactor CXL initialization with different code paths for PF0 and non
PF0 PFs.

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

diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
index 348d7404cd7a..f884580cb528 100644
--- a/drivers/net/ethernet/sfc/efx_cxl.c
+++ b/drivers/net/ethernet/sfc/efx_cxl.c
@@ -13,8 +13,69 @@
 #include "efx_cxl.h"
 
 #define EFX_CTPIO_BUFFER_SIZE	SZ_256M
+#define EFX_CTPIO_BUFFER_PER_PF_SIZE	SZ_8M
 
-int efx_cxl_init(struct efx_probe_data *probe_data)
+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;
+	}
+	return 0;
+}
+
+static int efx_cxl_non_pf0_init(struct efx_probe_data *probe_data)
+{
+	struct efx_nic *efx = &probe_data->efx;
+	struct pci_dev *pci_dev = efx->pci_dev;
+	struct range cxl_pio_range;
+	struct efx_cxl *cxl;
+	u64 devfn;
+
+	devfn = PCI_FUNC(pci_dev->devfn);
+
+	struct pci_dev *pf0_pci_dev __free(pci_dev_put) =
+		pci_get_slot(pci_dev->bus, PCI_DEVFN(PCI_SLOT(devfn), 0));
+
+	/* 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;
+
+	if (!cxl_get_range_and_link(&pf0_pci_dev->dev, &pci_dev->dev,
+				    &cxl_pio_range))
+		return  -EPROBE_DEFER;
+
+	cxl = kzalloc_obj(*cxl, GFP_KERNEL);
+	if (!cxl)
+		return -ENOMEM;
+
+	if (cxl_map(probe_data, cxl, (u64)devfn, cxl_pio_range)) {
+		kfree(cxl);
+		return -ENOMEM;
+	}
+
+	probe_data->cxl = cxl;
+	probe_data->cxl_pio_initialised = true;
+
+	return 0;
+}
+
+static int efx_cxl_pf0_init(struct efx_probe_data *probe_data)
 {
 	struct efx_nic *efx = &probe_data->efx;
 	struct pci_dev *pci_dev = efx->pci_dev;
@@ -80,26 +141,47 @@ 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) {
-		pci_err(pci_dev, "CXL ioremap region (%pra) failed\n",
-			&cxl_pio_range);
+	if (cxl_map(probe_data, cxl, 0, cxl_pio_range))
 		return -ENOMEM;
-	}
 
-	probe_data->cxl_pio_initialised = true;
 	probe_data->cxl = cxl;
-
+	probe_data->cxl_pio_initialised = true;
 	return 0;
 }
 
+int efx_cxl_init(struct efx_probe_data *probe_data)
+{
+	struct efx_nic *efx = &probe_data->efx;
+	struct pci_dev *pci_dev = efx->pci_dev;
+	u8 devfn;
+
+	if (efx->type->is_vf)
+		return 0;
+
+	/* are we PF0? */
+	devfn = PCI_FUNC(pci_dev->devfn);
+	if (devfn == 0)
+		return efx_cxl_pf0_init(probe_data);
+	else
+		return efx_cxl_non_pf0_init(probe_data);
+}
+
 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;
 
 	iounmap(probe_data->cxl->ctpio_cxl);
+
+	devfn = PCI_FUNC(pci_dev->devfn);
+	if (devfn == 0)
+		return;
+
+	kfree(probe_data->cxl);
 }
 
 MODULE_IMPORT_NS("CXL");
-- 
2.34.1

Re: [PATCH v2 1/4] driver core: Check for supplier requiring PM at link creation

[Dave Jiang]

On 10/1/26 6:20 AM, alucerop@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> PM initialization could not be necessary for some devices.
> 
> Avoid checking for supplier PM initialization if so.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/base/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 4c0c373998a1..bf0513beafad 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -840,7 +840,7 @@ struct device_link *device_link_add(struct device *consumer,
>  	 * SYNC_STATE_ONLY link, we don't check for reverse dependencies
>  	 * because it only affects sync_state() callbacks.
>  	 */
> -	if (!device_pm_initialized(supplier)
> +	if ((!device_pm_not_required(supplier) && !device_pm_initialized(supplier))
>  	    || (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
>  		  device_is_dependent(consumer, supplier))) {
>  		link = NULL;

A no PM supplier can now be linked at any point: before device_add(), while it fails, or after device_del(). Maybe replace with a helper like this?

static bool device_link_supplier_ready(struct device *supplier)
{
      /* no PM devices never enter dpm_list, so check registration directly */
      if (device_pm_not_required(supplier))
              return device_is_registered(supplier);

      return device_pm_initialized(supplier);
}

...

      if (!device_link_supplier_ready(supplier) ||
          (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
           device_is_dependent(consumer, supplier))) {
              link = NULL;
              goto out;
      }

Re: [PATCH v2 2/4] cxl/region: Add region reference in memdev attach

[Dave Jiang]

On 10/1/26 6:20 AM, alucerop@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> Use a new field in cxl_attach_region struct for easily link it with the
> region the memdev is attached to.
> 
> This facilitates device links creation where such a region is the supplier
> with non-PF0 physical functions wanting to use the CXL region being the
> consumers.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/region.c | 1 +
>  drivers/cxl/cxlmem.h      | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 27e63e6dab7c..78ca7ebc3e55 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -4132,6 +4132,7 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>  	if (rc)
>  		return rc;
>  
> +	attach->cxlr = cxlr;
>  	attach->hpa_range = (struct range) {
>  		.start = cxlr->params.res->start,
>  		.end = cxlr->params.res->end,
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index c401e3a1af06..c598561b8e5f 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -104,6 +104,7 @@ struct cxl_memdev_attach {
>  /**
>   * struct cxl_attach_region - coordinate mapping a region at memdev registration
>   * @attach: common core attachment descriptor
> + * @cxlr: cxl region the memdev is attached to.
>   * @hpa_range: physical address range of the region
>   *
>   * For the common simple case of a CXL device with private (non-general purpose
> @@ -112,6 +113,7 @@ struct cxl_memdev_attach {
>   */
>  struct cxl_attach_region {
>  	struct cxl_memdev_attach attach;
> +	struct cxl_region *cxlr;
>  	struct range hpa_range;
>  };
>  

attach->cxlr is never cleared when the region goes away. Unbinding the endpoint port runs endpoint_unregister_region(), which unregisters the region and drops its reference. PF0 stays bound until the detach work runs.

A non-PF0 probe in that window still finds the memdev and calls device_link_add() on the freed region.

How about something like this?

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 27e63e6dab7c..38ca73f12b84 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -4076,6 +4076,23 @@ static int first_mapped_decoder(struct device *dev, const void *data)
 	return 0;
 }
 
+/*
+ * Invalidate @attach before the region goes away so that
+ * cxl_get_range_and_link() can not pick up a stale region.
+ */
+static void endpoint_detach_attach_region(void *_attach)
+{
+	struct cxl_attach_region *attach = _attach;
+	struct cxl_region *cxlr;
+
+	scoped_guard(rwsem_write, &cxl_rwsem.region) {
+		cxlr = attach->cxlr;
+		WRITE_ONCE(attach->cxlr, NULL);
+		attach->hpa_range = DEFINE_RANGE(0, -1);
+	}
+	endpoint_unregister_region(cxlr);
+}
+
 /*
  * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
  * simple case of single mapped decoder per memdev.
@@ -4127,15 +4144,23 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
 
 	/* Only teardown regions that pass validation, ignore the rest */
 	get_device(&cxlr->dev);
-	rc = devm_add_action_or_reset(&endpoint->dev,
-				      endpoint_unregister_region, cxlr);
-	if (rc)
+	/*
+	 * Not devm_add_action_or_reset(): the reset path would take
+	 * cxl_rwsem.region for write while it is held for read here. The
+	 * endpoint lock keeps the action from running before @attach is set.
+	 */
+	rc = devm_add_action(&endpoint->dev, endpoint_detach_attach_region,
+			     attach);
+	if (rc) {
+		put_device(&cxlr->dev);
 		return rc;
+	}
 
 	attach->hpa_range = (struct range) {
 		.start = cxlr->params.res->start,
 		.end = cxlr->params.res->end,
 	};
+	WRITE_ONCE(attach->cxlr, cxlr);
 	return 0;
 }
 EXPORT_SYMBOL_FOR_MODULES(cxl_memdev_attach_region, "cxl_mem");
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index c401e3a1af06..7cd3a69cd5f5 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -104,6 +104,8 @@ struct cxl_memdev_attach {
 /**
  * struct cxl_attach_region - coordinate mapping a region at memdev registration
  * @attach: common core attachment descriptor
+ * @cxlr: cxl region the memdev is attached to, cleared under cxl_rwsem.region
+ *	before the region is unregistered
  * @hpa_range: physical address range of the region
  *
  * For the common simple case of a CXL device with private (non-general purpose
@@ -112,6 +114,7 @@ struct cxl_memdev_attach {
  */
 struct cxl_attach_region {
 	struct cxl_memdev_attach attach;
+	struct cxl_region *cxlr;
 	struct range hpa_range;
 };
 

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

[Dave Jiang]

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;

      /* 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__ */

Re: [PATCH v2 4/4] sfc: add multipf support

[Dave Jiang]

On 10/1/26 6:20 AM, alucerop@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> Use CXL core accelerator API for linking a non-PF0 PF to the CXL region
> its related PF0 CXL memdev is attached to, allowing non-PF0 PF release if
> such a CXL region is released itself. This can occur in different
> scenarios like PF0 release or CXL memdev release.
> 
> Obtain the CXL HPA region to work with and the ioremap based on such
> HPA and an offset based on the PF index.
> 
> Refactor CXL initialization with different code paths for PF0 and non
> PF0 PFs.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/net/ethernet/sfc/efx_cxl.c | 100 ++++++++++++++++++++++++++---
>  1 file changed, 91 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 348d7404cd7a..f884580cb528 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -13,8 +13,69 @@
>  #include "efx_cxl.h"
>  
>  #define EFX_CTPIO_BUFFER_SIZE	SZ_256M
> +#define EFX_CTPIO_BUFFER_PER_PF_SIZE	SZ_8M
>  
> -int efx_cxl_init(struct efx_probe_data *probe_data)
> +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;
> +	}
> +	return 0;
> +}
> +
> +static int efx_cxl_non_pf0_init(struct efx_probe_data *probe_data)
> +{
> +	struct efx_nic *efx = &probe_data->efx;
> +	struct pci_dev *pci_dev = efx->pci_dev;
> +	struct range cxl_pio_range;
> +	struct efx_cxl *cxl;
> +	u64 devfn;
> +
> +	devfn = PCI_FUNC(pci_dev->devfn);

I think you will want to use pci_dev->devfn directly. If you do this, devfn is 2:0 and PCI_SLOT() will evaluate to 0.

Also, does this device need to handle ARI?
> +
> +	struct pci_dev *pf0_pci_dev __free(pci_dev_put) =
> +		pci_get_slot(pci_dev->bus, PCI_DEVFN(PCI_SLOT(devfn), 0));
> +
> +	/* 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;
> +
> +	if (!cxl_get_range_and_link(&pf0_pci_dev->dev, &pci_dev->dev,
> +				    &cxl_pio_range))

Is this error check inverted?

DJ

> +		return  -EPROBE_DEFER;
> +
> +	cxl = kzalloc_obj(*cxl, GFP_KERNEL);
> +	if (!cxl)
> +		return -ENOMEM;
> +
> +	if (cxl_map(probe_data, cxl, (u64)devfn, cxl_pio_range)) {
> +		kfree(cxl);
> +		return -ENOMEM;
> +	}
> +
> +	probe_data->cxl = cxl;
> +	probe_data->cxl_pio_initialised = true;
> +
> +	return 0;
> +}
> +
> +static int efx_cxl_pf0_init(struct efx_probe_data *probe_data)
>  {
>  	struct efx_nic *efx = &probe_data->efx;
>  	struct pci_dev *pci_dev = efx->pci_dev;
> @@ -80,26 +141,47 @@ 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) {
> -		pci_err(pci_dev, "CXL ioremap region (%pra) failed\n",
> -			&cxl_pio_range);
> +	if (cxl_map(probe_data, cxl, 0, cxl_pio_range))
>  		return -ENOMEM;
> -	}
>  
> -	probe_data->cxl_pio_initialised = true;
>  	probe_data->cxl = cxl;
> -
> +	probe_data->cxl_pio_initialised = true;
>  	return 0;
>  }
>  
> +int efx_cxl_init(struct efx_probe_data *probe_data)
> +{
> +	struct efx_nic *efx = &probe_data->efx;
> +	struct pci_dev *pci_dev = efx->pci_dev;
> +	u8 devfn;
> +
> +	if (efx->type->is_vf)
> +		return 0;
> +
> +	/* are we PF0? */
> +	devfn = PCI_FUNC(pci_dev->devfn);
> +	if (devfn == 0)
> +		return efx_cxl_pf0_init(probe_data);
> +	else
> +		return efx_cxl_non_pf0_init(probe_data);
> +}
> +
>  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;
>  
>  	iounmap(probe_data->cxl->ctpio_cxl);
> +
> +	devfn = PCI_FUNC(pci_dev->devfn);
> +	if (devfn == 0)
> +		return;
> +
> +	kfree(probe_data->cxl);
>  }
>  
>  MODULE_IMPORT_NS("CXL");

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

[Dave Jiang]

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__ */
> 
> 

Re: [PATCH v2 1/4] driver core: Check for supplier requiring PM at link creation

[Lucero Palau, Alejandro]

On 01/10/2026 21:31, Dave Jiang wrote:
>
> On 10/1/26 6:20 AM, alucerop@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> PM initialization could not be necessary for some devices.
>>
>> Avoid checking for supplier PM initialization if so.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/base/core.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 4c0c373998a1..bf0513beafad 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -840,7 +840,7 @@ struct device_link *device_link_add(struct device *consumer,
>>   	 * SYNC_STATE_ONLY link, we don't check for reverse dependencies
>>   	 * because it only affects sync_state() callbacks.
>>   	 */
>> -	if (!device_pm_initialized(supplier)
>> +	if ((!device_pm_not_required(supplier) && !device_pm_initialized(supplier))
>>   	    || (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
>>   		  device_is_dependent(consumer, supplier))) {
>>   		link = NULL;
> A no PM supplier can now be linked at any point: before device_add(), while it fails, or after device_del(). Maybe replace with a helper like this?


I would say you can not use a device as supplier before device_add() 
happens for such a supplier, and if it does happen after device_del(), 
something is wrong with the caller.


Your suggestion is likely making the code more legible, but it does not 
change the functionality I added. Does it? Not saying it would not help, 
but I can not understand your comment for suggesting it which seems to 
point to potential problems I did not see.


> static bool device_link_supplier_ready(struct device *supplier)
> {
>        /* no PM devices never enter dpm_list, so check registration directly */
>        if (device_pm_not_required(supplier))
>                return device_is_registered(supplier);
>
>        return device_pm_initialized(supplier);
> }
>
> ...
>
>        if (!device_link_supplier_ready(supplier) ||
>            (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
>             device_is_dependent(consumer, supplier))) {
>                link = NULL;
>                goto out;
>        }

Re: [PATCH v2 2/4] cxl/region: Add region reference in memdev attach

[Lucero Palau, Alejandro]

On 01/10/2026 22:38, Dave Jiang wrote:
>
> On 10/1/26 6:20 AM, alucerop@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Use a new field in cxl_attach_region struct for easily link it with the
>> region the memdev is attached to.
>>
>> This facilitates device links creation where such a region is the supplier
>> with non-PF0 physical functions wanting to use the CXL region being the
>> consumers.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/core/region.c | 1 +
>>   drivers/cxl/cxlmem.h      | 2 ++
>>   2 files changed, 3 insertions(+)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 27e63e6dab7c..78ca7ebc3e55 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -4132,6 +4132,7 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>>   	if (rc)
>>   		return rc;
>>   
>> +	attach->cxlr = cxlr;
>>   	attach->hpa_range = (struct range) {
>>   		.start = cxlr->params.res->start,
>>   		.end = cxlr->params.res->end,
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index c401e3a1af06..c598561b8e5f 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -104,6 +104,7 @@ struct cxl_memdev_attach {
>>   /**
>>    * struct cxl_attach_region - coordinate mapping a region at memdev registration
>>    * @attach: common core attachment descriptor
>> + * @cxlr: cxl region the memdev is attached to.
>>    * @hpa_range: physical address range of the region
>>    *
>>    * For the common simple case of a CXL device with private (non-general purpose
>> @@ -112,6 +113,7 @@ struct cxl_memdev_attach {
>>    */
>>   struct cxl_attach_region {
>>   	struct cxl_memdev_attach attach;
>> +	struct cxl_region *cxlr;
>>   	struct range hpa_range;
>>   };
>>   
> attach->cxlr is never cleared when the region goes away. Unbinding the endpoint port runs endpoint_unregister_region(), which unregisters the region and drops its reference. PF0 stays bound until the detach work runs.
>
> A non-PF0 probe in that window still finds the memdev and calls device_link_add() on the freed region.


I do not think so. This version, see next patch, relies on locking the 
supplier, PF0, before getting the memdev and potentially using the 
attach region. Only on PF0 release can such a memdev and region 
disappear, so I think this is enough.

I added comments in the exported function, cxl_get_range_and_link() 
which does the locking before calling the internal function 
__cxl_get_range_and_link() which looks for the memdev and the attach 
region. In fact, it should not be possible to obtain the PF0 memdev 
reference and the attach region not there yet, but the code is still 
checking that possibility as a sanity check.


Thank you,

Alejandro.


> How about something like this?
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 27e63e6dab7c..38ca73f12b84 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -4076,6 +4076,23 @@ static int first_mapped_decoder(struct device *dev, const void *data)
>   	return 0;
>   }
>   
> +/*
> + * Invalidate @attach before the region goes away so that
> + * cxl_get_range_and_link() can not pick up a stale region.
> + */
> +static void endpoint_detach_attach_region(void *_attach)
> +{
> +	struct cxl_attach_region *attach = _attach;
> +	struct cxl_region *cxlr;
> +
> +	scoped_guard(rwsem_write, &cxl_rwsem.region) {
> +		cxlr = attach->cxlr;
> +		WRITE_ONCE(attach->cxlr, NULL);
> +		attach->hpa_range = DEFINE_RANGE(0, -1);
> +	}
> +	endpoint_unregister_region(cxlr);
> +}
> +
>   /*
>    * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
>    * simple case of single mapped decoder per memdev.
> @@ -4127,15 +4144,23 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>   
>   	/* Only teardown regions that pass validation, ignore the rest */
>   	get_device(&cxlr->dev);
> -	rc = devm_add_action_or_reset(&endpoint->dev,
> -				      endpoint_unregister_region, cxlr);
> -	if (rc)
> +	/*
> +	 * Not devm_add_action_or_reset(): the reset path would take
> +	 * cxl_rwsem.region for write while it is held for read here. The
> +	 * endpoint lock keeps the action from running before @attach is set.
> +	 */
> +	rc = devm_add_action(&endpoint->dev, endpoint_detach_attach_region,
> +			     attach);
> +	if (rc) {
> +		put_device(&cxlr->dev);
>   		return rc;
> +	}
>   
>   	attach->hpa_range = (struct range) {
>   		.start = cxlr->params.res->start,
>   		.end = cxlr->params.res->end,
>   	};
> +	WRITE_ONCE(attach->cxlr, cxlr);
>   	return 0;
>   }
>   EXPORT_SYMBOL_FOR_MODULES(cxl_memdev_attach_region, "cxl_mem");
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index c401e3a1af06..7cd3a69cd5f5 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -104,6 +104,8 @@ struct cxl_memdev_attach {
>   /**
>    * struct cxl_attach_region - coordinate mapping a region at memdev registration
>    * @attach: common core attachment descriptor
> + * @cxlr: cxl region the memdev is attached to, cleared under cxl_rwsem.region
> + *	before the region is unregistered
>    * @hpa_range: physical address range of the region
>    *
>    * For the common simple case of a CXL device with private (non-general purpose
> @@ -112,6 +114,7 @@ struct cxl_memdev_attach {
>    */
>   struct cxl_attach_region {
>   	struct cxl_memdev_attach attach;
> +	struct cxl_region *cxlr;
>   	struct range hpa_range;
>   };
>   

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

[Lucero Palau, Alejandro]

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__ */

Re: [PATCH v2 4/4] sfc: add multipf support

[Lucero Palau, Alejandro]

On 01/10/2026 23:32, Dave Jiang wrote:
>
> On 10/1/26 6:20 AM, alucerop@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Use CXL core accelerator API for linking a non-PF0 PF to the CXL region
>> its related PF0 CXL memdev is attached to, allowing non-PF0 PF release if
>> such a CXL region is released itself. This can occur in different
>> scenarios like PF0 release or CXL memdev release.
>>
>> Obtain the CXL HPA region to work with and the ioremap based on such
>> HPA and an offset based on the PF index.
>>
>> Refactor CXL initialization with different code paths for PF0 and non
>> PF0 PFs.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/net/ethernet/sfc/efx_cxl.c | 100 ++++++++++++++++++++++++++---
>>   1 file changed, 91 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> index 348d7404cd7a..f884580cb528 100644
>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -13,8 +13,69 @@
>>   #include "efx_cxl.h"
>>   
>>   #define EFX_CTPIO_BUFFER_SIZE	SZ_256M
>> +#define EFX_CTPIO_BUFFER_PER_PF_SIZE	SZ_8M
>>   
>> -int efx_cxl_init(struct efx_probe_data *probe_data)
>> +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;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int efx_cxl_non_pf0_init(struct efx_probe_data *probe_data)
>> +{
>> +	struct efx_nic *efx = &probe_data->efx;
>> +	struct pci_dev *pci_dev = efx->pci_dev;
>> +	struct range cxl_pio_range;
>> +	struct efx_cxl *cxl;
>> +	u64 devfn;
>> +
>> +	devfn = PCI_FUNC(pci_dev->devfn);
> I think you will want to use pci_dev->devfn directly. If you do this, devfn is 2:0 and PCI_SLOT() will evaluate to 0.


Uhmmm, I think with the refactoring I'm not using devfn properly here. 
Let me study this further.


> Also, does this device need to handle ARI?


Not for sfc devices.


>> +
>> +	struct pci_dev *pf0_pci_dev __free(pci_dev_put) =
>> +		pci_get_slot(pci_dev->bus, PCI_DEVFN(PCI_SLOT(devfn), 0));
>> +
>> +	/* 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;
>> +
>> +	if (!cxl_get_range_and_link(&pf0_pci_dev->dev, &pci_dev->dev,
>> +				    &cxl_pio_range))
> Is this error check inverted?


Yes. I'll fix it.


Thanks!


> DJ
>
>> +		return  -EPROBE_DEFER;
>> +
>> +	cxl = kzalloc_obj(*cxl, GFP_KERNEL);
>> +	if (!cxl)
>> +		return -ENOMEM;
>> +
>> +	if (cxl_map(probe_data, cxl, (u64)devfn, cxl_pio_range)) {
>> +		kfree(cxl);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	probe_data->cxl = cxl;
>> +	probe_data->cxl_pio_initialised = true;
>> +
>> +	return 0;
>> +}
>> +
>> +static int efx_cxl_pf0_init(struct efx_probe_data *probe_data)
>>   {
>>   	struct efx_nic *efx = &probe_data->efx;
>>   	struct pci_dev *pci_dev = efx->pci_dev;
>> @@ -80,26 +141,47 @@ 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) {
>> -		pci_err(pci_dev, "CXL ioremap region (%pra) failed\n",
>> -			&cxl_pio_range);
>> +	if (cxl_map(probe_data, cxl, 0, cxl_pio_range))
>>   		return -ENOMEM;
>> -	}
>>   
>> -	probe_data->cxl_pio_initialised = true;
>>   	probe_data->cxl = cxl;
>> -
>> +	probe_data->cxl_pio_initialised = true;
>>   	return 0;
>>   }
>>   
>> +int efx_cxl_init(struct efx_probe_data *probe_data)
>> +{
>> +	struct efx_nic *efx = &probe_data->efx;
>> +	struct pci_dev *pci_dev = efx->pci_dev;
>> +	u8 devfn;
>> +
>> +	if (efx->type->is_vf)
>> +		return 0;
>> +
>> +	/* are we PF0? */
>> +	devfn = PCI_FUNC(pci_dev->devfn);
>> +	if (devfn == 0)
>> +		return efx_cxl_pf0_init(probe_data);
>> +	else
>> +		return efx_cxl_non_pf0_init(probe_data);
>> +}
>> +
>>   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;
>>   
>>   	iounmap(probe_data->cxl->ctpio_cxl);
>> +
>> +	devfn = PCI_FUNC(pci_dev->devfn);
>> +	if (devfn == 0)
>> +		return;
>> +
>> +	kfree(probe_data->cxl);
>>   }
>>   
>>   MODULE_IMPORT_NS("CXL");

Re: [PATCH v2 2/4] cxl/region: Add region reference in memdev attach

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] cxl/region: UAF due to dangling attach->cxlr pointer after endpoint teardown
--

commit c1388a867928fb269ed84b5e755417256bbf4bd5
Author: Alejandro Lucero <alucerop@amd.com>

cxl/region: Add region reference in memdev attach

This commit adds a new field in the cxl_attach_region struct to link it with
the region the memdev is attached to. This facilitates device links creation
where such a region is the supplier with non-PF0 physical functions wanting to
use the CXL region being the consumers.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 27e63e6dab7c8..78ca7ebc3e557 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -4132,6 +4132,7 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>  	if (rc)
>  		return rc;
>  
> +	attach->cxlr = cxlr;

[Severity: High]
Could this introduce a regression by leaving a dangling pointer that leads to a
use-after-free?

When the CXL port topology (endpoint) is unbound or torn down,
endpoint_unregister_region() is triggered via devm on the endpoint device.
This drops the reference to the region and potentially frees it.

However, the attach struct (bound to the parent PCIe PF0 device) outlives the
endpoint and retains this dangling attach->cxlr pointer, along with the
non-zero hpa_range.

If a secondary PF driver probes later, could it check the unmodified hpa_range
and dereference attach->cxlr->dev when creating a device link? Should there be
a corresponding cleanup action registered to clear this pointer?

>  	attach->hpa_range = (struct range) {
>  		.start = cxlr->params.res->start,
>  		.end = cxlr->params.res->end,

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001132023.17032-1-alucerop@amd.com?part=2

Re: [PATCH v2 1/4] driver core: Check for supplier requiring PM at link creation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] core: missing registration check in device_link_add() for no_pm devices
--

commit b2ba746c71946921fa9666578a3ca843070e29f1
Author: Alejandro Lucero <alucerop@amd.com>

driver core: Check for supplier requiring PM at link creation

This commit updates device_link_add() to skip the PM initialization
check for devices that do not require PM, allowing links to be
established for such devices.

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 4c0c373998a19..bf0513beafad4 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c

[ ... ]

> @@ -840,7 +840,7 @@ struct device_link *device_link_add(struct device *consumer,
>  	 * SYNC_STATE_ONLY link, we don't check for reverse dependencies
>  	 * because it only affects sync_state() callbacks.
>  	 */
> -	if (!device_pm_initialized(supplier)
> +	if ((!device_pm_not_required(supplier) && !device_pm_initialized(supplier))

[Severity: Medium]
Does this inadvertently bypass the registration check for devices where
device_pm_not_required() is true?

The comment immediately above states: "If the supplier has not been fully
registered yet ... return NULL". Previously, device_pm_initialized()
implicitly enforced this because it wraps device_is_registered().

If a caller invokes device_link_add() with a supplier device that has
no_pm set to true but has not been successfully registered, bypassing this
check entirely means the unregistered supplier is allowed through. Since an
unregistered device will never go through device_del(), device_links_purge()
will never run to clean up the link. Can this result in a permanent leak
of the struct device_link and device refcounts?

Would it make sense to explicitly add a !device_is_registered(supplier)
fallback check here to ensure the core API contract is maintained?

>  	    || (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
>  		  device_is_dependent(consumer, supplier))) {
>  		link = NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001132023.17032-1-alucerop@amd.com?part=1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] cxl/memdev: NULL pointer dereference when accessing missing cxlmd->attach
- [High] cxl/memdev: data race leading to NULL dereference of attach->cxlr
- [High] cxl/memdev: TOCTOU NULL pointer dereference on cxlmd->cxlds
--

commit ad669ced94a784d488cc194089a6921ef73d1404
Author: Alejandro Lucero <alucerop@amd.com>
Subject: cxl/memdev: Add support for multi PF devices

This commit enables non-PF0 Physical Functions to access CXL memory by
establishing a device link between the CXL region attached to the PF0 memdev
and the non-PF0 devices. Unbinding the PF0 device will naturally release the
memdev and region, safely unwinding the non-PF0 drivers first.

> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index b3419df586b9c..799cb6e756390 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);

[Severity: High]
Does this create a race condition that could lead to a NULL pointer
dereference? When bus_find_device() iterates over the CXL bus using
match_memdev_by_parent_device(), it evaluates cxlmd->cxlds->dev == pf_dev
without holding a lock protecting cxlmd->cxlds.

Concurrently, if an unrelated cxl_memdev on the bus is shutting down,
cxl_memdev_shutdown() explicitly sets cxlmd->cxlds = NULL. If the matching
loop encounters this shutting-down memdev, could it trigger a deterministic
NULL pointer dereference?

Since dev->parent is stably assigned to cxlds->dev at creation, could this
check use dev->parent == pf_dev to bypass this race entirely?

> +}
> +
> +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);
> +
> +	/*
> +	 * 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)

[Severity: High]
Could this blindly dereference a missing cxlmd->attach? When
cxl_get_range_and_link() retrieves a cxl_memdev by parent device, if the
target device was created without an attach callback (e.g., via
devm_cxl_add_classdev()), cxlmd->attach is NULL.

Since container_of(NULL, struct cxl_attach_region, attach) evaluates exactly
to NULL because the member is at offset 0, evaluating attach->hpa_range.end
would trigger an immediate kernel crash. Should this exported subsystem API
validate its state first to prevent remote subsystems from inducing crashes?

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

[Severity: High]
Does reading attach->hpa_range.end and attach->cxlr here lack synchronization
against the cxl_mem probe writer?

While __cxl_get_range_and_link() relies on device_lock(pf0) to serialize
access:

cxl_get_range_and_link() {
    ...
    device_lock(pf0);
    rc = __cxl_get_range_and_link(pf0, pfx, range);
    device_unlock(pf0);
    ...
}

cxl_memdev_attach_region() writes to attach->cxlr and attach->hpa_range
during the cxl_mem driver probe, which executes asynchronously under the
child's device lock (device_lock(&cxlmd->dev)).

Due to the lack of synchronization and memory barriers between these distinct
lock contexts, could a reader observe an updated non-NONE hpa_range.end while
attach->cxlr remains NULL, passing &attach->cxlr->dev to device_link_add()
and crashing the kernel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001132023.17032-1-alucerop@amd.com?part=3

Re: [PATCH v2 4/4] sfc: add multipf support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] sfc: inverted error check causes uninitialized memory mapping
- [High] sfc: incorrect slot extraction links non-PF0 to wrong PF0 device
--

commit f448ba0f4a9e4a95bf07bd91e1ee5f0a1a822aff
Author: Alejandro Lucero <alucerop@amd.com>

sfc: add multipf support

This commit introduces multipf support by using the CXL core accelerator API
to link non-PF0 PFs to the CXL region of their associated PF0. It obtains
the CXL HPA region and maps it using an offset based on the PF index.

> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 348d7404cd7aa..f884580cb5286 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
[ ... ]
> @@ -13,8 +13,69 @@
[ ... ]
> +static int efx_cxl_non_pf0_init(struct efx_probe_data *probe_data)
> +{
> +	struct efx_nic *efx = &probe_data->efx;
> +	struct pci_dev *pci_dev = efx->pci_dev;
> +	struct range cxl_pio_range;
> +	struct efx_cxl *cxl;
> +	u64 devfn;
> +
> +	devfn = PCI_FUNC(pci_dev->devfn);
> +
> +	struct pci_dev *pf0_pci_dev __free(pci_dev_put) =
> +		pci_get_slot(pci_dev->bus, PCI_DEVFN(PCI_SLOT(devfn), 0));

[Severity: High]
Does this code query the correct PCI slot? The devfn variable is assigned the
result of PCI_FUNC(pci_dev->devfn), which extracts only the 3-bit function
number. Passing this isolated function number into PCI_SLOT() will always
yield 0 since it shifts right by 3.

This means non-PF0 functions will query slot 0 instead of their actual PCI
slot. If the NIC is located in a slot other than 0 and another CXL-enabled
NIC is in slot 0, would this cause the driver to link its non-PF0 function
to the wrong NIC's CXL region, bypassing mutual exclusion by taking the wrong
device's lock in cxl_get_range_and_link()?

> +
> +	/* 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;
> +
> +	if (!cxl_get_range_and_link(&pf0_pci_dev->dev, &pci_dev->dev,
> +				    &cxl_pio_range))
> +		return  -EPROBE_DEFER;

[Severity: High]
Is the error check here inverted? If cxl_get_range_and_link() successfully
finds a region (returning 0), this condition evaluates to true and aborts
with -EPROBE_DEFER.

Conversely, if it fails (e.g., due to a missing region), it returns an error
code which evaluates as true, meaning the condition fails and the code falls
through. This leaves the cxl_pio_range stack variable uninitialized.

> +
> +	cxl = kzalloc_obj(*cxl, GFP_KERNEL);
> +	if (!cxl)
> +		return -ENOMEM;
> +
> +	if (cxl_map(probe_data, cxl, (u64)devfn, cxl_pio_range)) {

[Severity: High]
Following up on the inverted check above, if cxl_get_range_and_link() fails
and we fall through, won't we pass the uninitialized cxl_pio_range stack
variable by value to cxl_map()?

Looking at cxl_map() in this same file:

cxl_map(...) {
    ...
    cxl_pio_pf_start = cxl_pio_range.start + ...;
    cxl->ctpio_cxl = ioremap_wc(cxl_pio_pf_start, ...);
    ...
}

Could this result in ioremap_wc() mapping arbitrary physical memory based
on the uninitialized stack data?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001132023.17032-1-alucerop@amd.com?part=4

Re: [PATCH v2 1/4] driver core: Check for supplier requiring PM at link creation

[Dave Jiang]

[-- Attachment #1: Type: text/plain, Size: 3516 bytes --]



On 10/1/26 9:32 PM, Lucero Palau, Alejandro wrote:
> 
> On 01/10/2026 21:31, Dave Jiang wrote:
>>
>> On 10/1/26 6:20 AM, alucerop@amd.com wrote:
>>> From: Alejandro Lucero <alucerop@amd.com>
>>>
>>> PM initialization could not be necessary for some devices.
>>>
>>> Avoid checking for supplier PM initialization if so.
>>>
>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>> ---
>>>   drivers/base/core.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>> index 4c0c373998a1..bf0513beafad 100644
>>> --- a/drivers/base/core.c
>>> +++ b/drivers/base/core.c
>>> @@ -840,7 +840,7 @@ struct device_link *device_link_add(struct device *consumer,
>>>        * SYNC_STATE_ONLY link, we don't check for reverse dependencies
>>>        * because it only affects sync_state() callbacks.
>>>        */
>>> -    if (!device_pm_initialized(supplier)
>>> +    if ((!device_pm_not_required(supplier) && !device_pm_initialized(supplier))
>>>           || (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
>>>             device_is_dependent(consumer, supplier))) {
>>>           link = NULL;
>> A no PM supplier can now be linked at any point: before device_add(), while it fails, or after device_del(). Maybe replace with a helper like this?
> 
> 
> I would say you can not use a device as supplier before device_add() happens for such a supplier, and if it does happen after device_del(), something is wrong with the caller.

Yes that would be a bug, and device_link_add() is suppose to catch it. For PM devices, the device_pm_initialized() test is the gate. The change you made skips that test for no PM device case. I had LLM created a test module for verification, attached.

Essentially the logic in this patch removed the check for 2 states that the original code used to block.
1. before device_add(supplier)
2. after device_del(supplier) 

> 
> 
> Your suggestion is likely making the code more legible, but it does not change the functionality I added. Does it? Not saying it would not help, but I can not understand your comment for suggesting it which seems to point to potential problems I did not see.
> 

It does.
- before device_add(supplier): this patch creates the link, and the helper refuses it.
- after device_add(supplier): both create it.
- after device_del(supplier), before last put_device: this patch creates the link, and the helper refuses it.

delete_region() or root decoder teardown can unregister the region while the endpoint is still bound. cxl_get_range_and_link() can still be called on a region that has already been through device_del().

Without the helper, it's possible where the PFx driver can device_link_add() a deleted region that is still around due to endpoint still holds a reference.

DJ


> 
>> static bool device_link_supplier_ready(struct device *supplier)
>> {
>>        /* no PM devices never enter dpm_list, so check registration directly */
>>        if (device_pm_not_required(supplier))
>>                return device_is_registered(supplier);
>>
>>        return device_pm_initialized(supplier);
>> }
>>
>> ...
>>
>>        if (!device_link_supplier_ready(supplier) ||
>>            (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
>>             device_is_dependent(consumer, supplier))) {
>>                link = NULL;
>>                goto out;
>>        }

[-- Attachment #2: mock-pfx.patch --]
[-- Type: text/x-patch, Size: 4509 bytes --]

commit 0551e91e63c6009b71792133e0ef0cff1c0af6dd
Author: Dave Jiang <dave.jiang@intel.com>
Date:   Thu Oct 1 14:13:19 2026 -0700

    TEST ONLY: cxl/test: mock non-PF0 consumer for cxl_get_range_and_link()
    
    Not for submission. Exercises the multi-PF device-link path against the
    cxl_test type-2 accelerator, and self-tests device_link_add() on a no_pm
    supplier across registration.

diff --git a/tools/testing/cxl/test/Kbuild b/tools/testing/cxl/test/Kbuild
index 9a24ddc28488..279d89be4a4d 100644
--- a/tools/testing/cxl/test/Kbuild
+++ b/tools/testing/cxl/test/Kbuild
@@ -6,11 +6,13 @@ obj-m += cxl_mock.o
 obj-m += cxl_mock_mem.o
 obj-m += cxl_translate.o
 obj-m += cxl_mock_accel.o
+obj-m += cxl_mock_pfx.o
 
 cxl_test-y := cxl.o
 cxl_test-y += hmem_test.o
 cxl_mock-y := mock.o
 cxl_mock_mem-y := mem.o
 cxl_mock_accel-y := accel.o
+cxl_mock_pfx-y := pfx.o
 
 KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
diff --git a/tools/testing/cxl/test/pfx.c b/tools/testing/cxl/test/pfx.c
new file mode 100644
index 000000000000..55b752ebe9ca
--- /dev/null
+++ b/tools/testing/cxl/test/pfx.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * TEST ONLY: mock non-PF0 consumer. Its probe links to the region attached
+ * to the cxl_test type-2 accelerator via cxl_get_range_and_link().
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <cxl/cxl.h>
+
+static char *pf0_name = "cxl_type2_accel.0";
+module_param(pf0_name, charp, 0444);
+
+static struct platform_device *pfx_pdev;
+
+static int cxl_mock_pfx_probe(struct platform_device *pdev)
+{
+	struct device *pf0;
+	struct range range;
+	int rc;
+
+	pf0 = bus_find_device_by_name(&platform_bus_type, NULL, pf0_name);
+	if (!pf0) {
+		dev_info(&pdev->dev, "pfx_test: %s not found\n", pf0_name);
+		return -ENODEV;
+	}
+
+	rc = cxl_get_range_and_link(pf0, &pdev->dev, &range);
+	put_device(pf0);
+	if (rc) {
+		dev_info(&pdev->dev, "pfx_test: link rc=%d\n", rc);
+		/* don't let -EPROBE_DEFER requeue us behind the test's back */
+		return rc == -EPROBE_DEFER ? -EAGAIN : rc;
+	}
+
+	dev_info(&pdev->dev, "pfx_test: link rc=0 range=%pra\n", &range);
+	return 0;
+}
+
+static void cxl_mock_pfx_remove(struct platform_device *pdev)
+{
+	dev_info(&pdev->dev, "pfx_test: removed\n");
+}
+
+static struct platform_driver cxl_mock_pfx_driver = {
+	.probe = cxl_mock_pfx_probe,
+	.remove = cxl_mock_pfx_remove,
+	.driver = {
+		.name = "cxl_mock_pfx",
+	},
+};
+
+static void dev_release(struct device *dev)
+{
+	kfree(dev);
+}
+
+/* device_link_add() must refuse a no_pm supplier that is not registered */
+static void devlink_no_pm_selftest(struct device *consumer)
+{
+	struct device_link *link;
+	struct device *sup;
+	int pass = 0;
+
+	sup = kzalloc_obj(*sup);
+	if (!sup)
+		return;
+	device_initialize(sup);
+	sup->release = dev_release;
+	device_set_pm_not_required(sup);
+	dev_set_name(sup, "pfx_test_supplier");
+
+	link = device_link_add(consumer, sup, DL_FLAG_STATELESS);
+	pr_info("pfx_test: no_pm before device_add: link %s\n",
+		link ? "CREATED (FAIL)" : "refused (PASS)");
+	if (link)
+		device_link_del(link);
+	else
+		pass++;
+
+	if (device_add(sup)) {
+		put_device(sup);
+		return;
+	}
+
+	link = device_link_add(consumer, sup, DL_FLAG_STATELESS);
+	pr_info("pfx_test: no_pm after device_add: link %s\n",
+		link ? "created (PASS)" : "REFUSED (FAIL)");
+	if (link) {
+		device_link_del(link);
+		pass++;
+	}
+
+	device_del(sup);
+	link = device_link_add(consumer, sup, DL_FLAG_STATELESS);
+	pr_info("pfx_test: no_pm after device_del: link %s\n",
+		link ? "CREATED (FAIL)" : "refused (PASS)");
+	if (link)
+		device_link_del(link);
+	else
+		pass++;
+
+	put_device(sup);
+	pr_info("pfx_test: no_pm selftest %d/3 passed\n", pass);
+}
+
+static int __init cxl_mock_pfx_init(void)
+{
+	int rc;
+
+	pfx_pdev = platform_device_register_simple("cxl_mock_pfx", 1, NULL, 0);
+	if (IS_ERR(pfx_pdev))
+		return PTR_ERR(pfx_pdev);
+
+	devlink_no_pm_selftest(&pfx_pdev->dev);
+
+	rc = platform_driver_register(&cxl_mock_pfx_driver);
+	if (rc)
+		platform_device_unregister(pfx_pdev);
+	return rc;
+}
+module_init(cxl_mock_pfx_init);
+
+static void __exit cxl_mock_pfx_exit(void)
+{
+	platform_driver_unregister(&cxl_mock_pfx_driver);
+	platform_device_unregister(pfx_pdev);
+}
+module_exit(cxl_mock_pfx_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("cxl_test: TEST ONLY mock non-PF0 consumer");
+MODULE_IMPORT_NS("CXL");

Re: [PATCH v2 2/4] cxl/region: Add region reference in memdev attach

[Dave Jiang]

[-- Attachment #1: Type: text/plain, Size: 6892 bytes --]



On 10/1/26 9:41 PM, Lucero Palau, Alejandro wrote:
> 
> On 01/10/2026 22:38, Dave Jiang wrote:
>>
>> On 10/1/26 6:20 AM, alucerop@amd.com wrote:
>>> From: Alejandro Lucero <alucerop@amd.com>
>>>
>>> Use a new field in cxl_attach_region struct for easily link it with the
>>> region the memdev is attached to.
>>>
>>> This facilitates device links creation where such a region is the supplier
>>> with non-PF0 physical functions wanting to use the CXL region being the
>>> consumers.
>>>
>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>> ---
>>>   drivers/cxl/core/region.c | 1 +
>>>   drivers/cxl/cxlmem.h      | 2 ++
>>>   2 files changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index 27e63e6dab7c..78ca7ebc3e55 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -4132,6 +4132,7 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>>>       if (rc)
>>>           return rc;
>>>   +    attach->cxlr = cxlr;
>>>       attach->hpa_range = (struct range) {
>>>           .start = cxlr->params.res->start,
>>>           .end = cxlr->params.res->end,
>>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>>> index c401e3a1af06..c598561b8e5f 100644
>>> --- a/drivers/cxl/cxlmem.h
>>> +++ b/drivers/cxl/cxlmem.h
>>> @@ -104,6 +104,7 @@ struct cxl_memdev_attach {
>>>   /**
>>>    * struct cxl_attach_region - coordinate mapping a region at memdev registration
>>>    * @attach: common core attachment descriptor
>>> + * @cxlr: cxl region the memdev is attached to.
>>>    * @hpa_range: physical address range of the region
>>>    *
>>>    * For the common simple case of a CXL device with private (non-general purpose
>>> @@ -112,6 +113,7 @@ struct cxl_memdev_attach {
>>>    */
>>>   struct cxl_attach_region {
>>>       struct cxl_memdev_attach attach;
>>> +    struct cxl_region *cxlr;
>>>       struct range hpa_range;
>>>   };
>>>   
>> attach->cxlr is never cleared when the region goes away. Unbinding the endpoint port runs endpoint_unregister_region(), which unregisters the region and drops its reference. PF0 stays bound until the detach work runs.
>>
>> A non-PF0 probe in that window still finds the memdev and calls device_link_add() on the freed region.
> 
> 
> I do not think so. This version, see next patch, relies on locking the supplier, PF0, before getting the memdev and potentially using the attach region. Only on PF0 release can such a memdev and region disappear, so I think this is enough.
> 

The memdev, yes. It's devm on PF0. The region, no. Unbinding the endpoint port, or cxl_mem from the memdev, runs endpoint_unregister_region() under the endpoint's lock, not PF0's. PF0's release is only queued (schedule_detach() -> detach_memdev()). Until that work runs, PF0 is bound, the memdev is found, hpa_range is valid, and attach->cxlr points at a freed region. Root teardown (kill_regions()) and delete_region also unregister the region without touching PF0.

I attached an LLM generated test kernel module you can use to reproduce the KASAN complaint. Commit log provides instructions.
BUG: KASAN: slab-use-after-free in device_link_add+0x521/0xa80
 cxl_get_range_and_link+0xa9/0x120 [cxl_core]

DJ

> I added comments in the exported function, cxl_get_range_and_link() which does the locking before calling the internal function __cxl_get_range_and_link() which looks for the memdev and the attach region. In fact, it should not be possible to obtain the PF0 memdev reference and the attach region not there yet, but the code is still checking that possibility as a sanity check.
> 
> 
> Thank you,
> 
> Alejandro.
> 
> 
>> How about something like this?
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 27e63e6dab7c..38ca73f12b84 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -4076,6 +4076,23 @@ static int first_mapped_decoder(struct device *dev, const void *data)
>>       return 0;
>>   }
>>   +/*
>> + * Invalidate @attach before the region goes away so that
>> + * cxl_get_range_and_link() can not pick up a stale region.
>> + */
>> +static void endpoint_detach_attach_region(void *_attach)
>> +{
>> +    struct cxl_attach_region *attach = _attach;
>> +    struct cxl_region *cxlr;
>> +
>> +    scoped_guard(rwsem_write, &cxl_rwsem.region) {
>> +        cxlr = attach->cxlr;
>> +        WRITE_ONCE(attach->cxlr, NULL);
>> +        attach->hpa_range = DEFINE_RANGE(0, -1);
>> +    }
>> +    endpoint_unregister_region(cxlr);
>> +}
>> +
>>   /*
>>    * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
>>    * simple case of single mapped decoder per memdev.
>> @@ -4127,15 +4144,23 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>>         /* Only teardown regions that pass validation, ignore the rest */
>>       get_device(&cxlr->dev);
>> -    rc = devm_add_action_or_reset(&endpoint->dev,
>> -                      endpoint_unregister_region, cxlr);
>> -    if (rc)
>> +    /*
>> +     * Not devm_add_action_or_reset(): the reset path would take
>> +     * cxl_rwsem.region for write while it is held for read here. The
>> +     * endpoint lock keeps the action from running before @attach is set.
>> +     */
>> +    rc = devm_add_action(&endpoint->dev, endpoint_detach_attach_region,
>> +                 attach);
>> +    if (rc) {
>> +        put_device(&cxlr->dev);
>>           return rc;
>> +    }
>>         attach->hpa_range = (struct range) {
>>           .start = cxlr->params.res->start,
>>           .end = cxlr->params.res->end,
>>       };
>> +    WRITE_ONCE(attach->cxlr, cxlr);
>>       return 0;
>>   }
>>   EXPORT_SYMBOL_FOR_MODULES(cxl_memdev_attach_region, "cxl_mem");
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index c401e3a1af06..7cd3a69cd5f5 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -104,6 +104,8 @@ struct cxl_memdev_attach {
>>   /**
>>    * struct cxl_attach_region - coordinate mapping a region at memdev registration
>>    * @attach: common core attachment descriptor
>> + * @cxlr: cxl region the memdev is attached to, cleared under cxl_rwsem.region
>> + *    before the region is unregistered
>>    * @hpa_range: physical address range of the region
>>    *
>>    * For the common simple case of a CXL device with private (non-general purpose
>> @@ -112,6 +114,7 @@ struct cxl_memdev_attach {
>>    */
>>   struct cxl_attach_region {
>>       struct cxl_memdev_attach attach;
>> +    struct cxl_region *cxlr;
>>       struct range hpa_range;
>>   };
>>   
> 

[-- Attachment #2: 0001-TEST-ONLY-cxl-test-mock-non-PF0-consumer-for-cxl_get.patch --]
[-- Type: text/x-patch, Size: 4306 bytes --]

From d52f0776cbb6e1df45a6511010bce12f81cb2313 Mon Sep 17 00:00:00 2001
From: Dave Jiang <dave.jiang@intel.com>
Date: Thu, 1 Oct 2026 14:13:19 -0700
Subject: [PATCH] TEST ONLY: cxl/test: mock non-PF0 consumer for
 cxl_get_range_and_link()

Not for submission. Add cxl_mock_pfx, a platform driver whose probe calls
cxl_get_range_and_link() against the cxl_test type-2 accelerator
(cxl_type2_accel.0), the way a non-PF0 function would.

To race the device link against region teardown:

  modprobe cxl_test type2_test=1
  modprobe cxl_mock_pfx
  D=/sys/bus/platform/drivers/cxl_mock_pfx
  EP=$(ls /sys/bus/cxl/devices | grep endpoint)
  for i in $(seq 200); do
      echo cxl_mock_pfx.1 > $D/unbind 2>/dev/null
      echo cxl_mock_pfx.1 > $D/bind 2>/dev/null
  done &
  sleep 0.2
  echo $EP > /sys/bus/cxl/drivers/cxl_port/unbind
  wait

The endpoint name depends on the topology, so EP is looked up rather
than hard coded. type2_test=1 creates a single accelerator, so there is
one endpoint.

Repeat from the modprobe of cxl_test if the window is missed. With
KASAN, a stale attach->cxlr shows up as a slab-use-after-free in
device_link_add() called from cxl_get_range_and_link().

The use-after-free is only reported reliably with KASAN enabled. Without
it, the stale access may go unnoticed. Tested with:

  CONFIG_KASAN=y
  CONFIG_KASAN_GENERIC=y
---
 tools/testing/cxl/test/Kbuild |  2 +
 tools/testing/cxl/test/pfx.c  | 77 +++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)
 create mode 100644 tools/testing/cxl/test/pfx.c

diff --git a/tools/testing/cxl/test/Kbuild b/tools/testing/cxl/test/Kbuild
index 9a24ddc28488..279d89be4a4d 100644
--- a/tools/testing/cxl/test/Kbuild
+++ b/tools/testing/cxl/test/Kbuild
@@ -6,11 +6,13 @@ obj-m += cxl_mock.o
 obj-m += cxl_mock_mem.o
 obj-m += cxl_translate.o
 obj-m += cxl_mock_accel.o
+obj-m += cxl_mock_pfx.o
 
 cxl_test-y := cxl.o
 cxl_test-y += hmem_test.o
 cxl_mock-y := mock.o
 cxl_mock_mem-y := mem.o
 cxl_mock_accel-y := accel.o
+cxl_mock_pfx-y := pfx.o
 
 KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
diff --git a/tools/testing/cxl/test/pfx.c b/tools/testing/cxl/test/pfx.c
new file mode 100644
index 000000000000..39c0c081c896
--- /dev/null
+++ b/tools/testing/cxl/test/pfx.c
@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * TEST ONLY: mock non-PF0 consumer. Its probe links to the region attached
+ * to the cxl_test type-2 accelerator via cxl_get_range_and_link().
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <cxl/cxl.h>
+
+static char *pf0_name = "cxl_type2_accel.0";
+module_param(pf0_name, charp, 0444);
+
+static struct platform_device *pfx_pdev;
+
+static int cxl_mock_pfx_probe(struct platform_device *pdev)
+{
+	struct device *pf0;
+	struct range range;
+	int rc;
+
+	pf0 = bus_find_device_by_name(&platform_bus_type, NULL, pf0_name);
+	if (!pf0) {
+		dev_info(&pdev->dev, "pfx_test: %s not found\n", pf0_name);
+		return -ENODEV;
+	}
+
+	rc = cxl_get_range_and_link(pf0, &pdev->dev, &range);
+	put_device(pf0);
+	if (rc) {
+		dev_info(&pdev->dev, "pfx_test: link rc=%d\n", rc);
+		/* don't let -EPROBE_DEFER requeue us behind the test's back */
+		return rc == -EPROBE_DEFER ? -EAGAIN : rc;
+	}
+
+	dev_info(&pdev->dev, "pfx_test: link rc=0 range=%pra\n", &range);
+	return 0;
+}
+
+static void cxl_mock_pfx_remove(struct platform_device *pdev)
+{
+	dev_info(&pdev->dev, "pfx_test: removed\n");
+}
+
+static struct platform_driver cxl_mock_pfx_driver = {
+	.probe = cxl_mock_pfx_probe,
+	.remove = cxl_mock_pfx_remove,
+	.driver = {
+		.name = "cxl_mock_pfx",
+	},
+};
+
+static int __init cxl_mock_pfx_init(void)
+{
+	int rc;
+
+	pfx_pdev = platform_device_register_simple("cxl_mock_pfx", 1, NULL, 0);
+	if (IS_ERR(pfx_pdev))
+		return PTR_ERR(pfx_pdev);
+
+	rc = platform_driver_register(&cxl_mock_pfx_driver);
+	if (rc)
+		platform_device_unregister(pfx_pdev);
+	return rc;
+}
+module_init(cxl_mock_pfx_init);
+
+static void __exit cxl_mock_pfx_exit(void)
+{
+	platform_driver_unregister(&cxl_mock_pfx_driver);
+	platform_device_unregister(pfx_pdev);
+}
+module_exit(cxl_mock_pfx_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("cxl_test: TEST ONLY mock non-PF0 consumer");
+MODULE_IMPORT_NS("CXL");
-- 
2.54.0

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

[Dave Jiang]

On 10/1/26 9:50 PM, Lucero Palau, Alejandro wrote:
> 
> 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.

See my response to patch 2.

DJ

> 
> 
> 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__ */

Re: [PATCH v2 2/4] cxl/region: Add region reference in memdev attach

[Lucero Palau, Alejandro]

On 02/10/2026 16:52, Dave Jiang wrote:
>
> On 10/1/26 9:41 PM, Lucero Palau, Alejandro wrote:
>> On 01/10/2026 22:38, Dave Jiang wrote:
>>> On 10/1/26 6:20 AM, alucerop@amd.com wrote:
>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>
>>>> Use a new field in cxl_attach_region struct for easily link it with the
>>>> region the memdev is attached to.
>>>>
>>>> This facilitates device links creation where such a region is the supplier
>>>> with non-PF0 physical functions wanting to use the CXL region being the
>>>> consumers.
>>>>
>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>> ---
>>>>    drivers/cxl/core/region.c | 1 +
>>>>    drivers/cxl/cxlmem.h      | 2 ++
>>>>    2 files changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>> index 27e63e6dab7c..78ca7ebc3e55 100644
>>>> --- a/drivers/cxl/core/region.c
>>>> +++ b/drivers/cxl/core/region.c
>>>> @@ -4132,6 +4132,7 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>>>>        if (rc)
>>>>            return rc;
>>>>    +    attach->cxlr = cxlr;
>>>>        attach->hpa_range = (struct range) {
>>>>            .start = cxlr->params.res->start,
>>>>            .end = cxlr->params.res->end,
>>>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>>>> index c401e3a1af06..c598561b8e5f 100644
>>>> --- a/drivers/cxl/cxlmem.h
>>>> +++ b/drivers/cxl/cxlmem.h
>>>> @@ -104,6 +104,7 @@ struct cxl_memdev_attach {
>>>>    /**
>>>>     * struct cxl_attach_region - coordinate mapping a region at memdev registration
>>>>     * @attach: common core attachment descriptor
>>>> + * @cxlr: cxl region the memdev is attached to.
>>>>     * @hpa_range: physical address range of the region
>>>>     *
>>>>     * For the common simple case of a CXL device with private (non-general purpose
>>>> @@ -112,6 +113,7 @@ struct cxl_memdev_attach {
>>>>     */
>>>>    struct cxl_attach_region {
>>>>        struct cxl_memdev_attach attach;
>>>> +    struct cxl_region *cxlr;
>>>>        struct range hpa_range;
>>>>    };
>>>>    
>>> attach->cxlr is never cleared when the region goes away. Unbinding the endpoint port runs endpoint_unregister_region(), which unregisters the region and drops its reference. PF0 stays bound until the detach work runs.
>>>
>>> A non-PF0 probe in that window still finds the memdev and calls device_link_add() on the freed region.
>>
>> I do not think so. This version, see next patch, relies on locking the supplier, PF0, before getting the memdev and potentially using the attach region. Only on PF0 release can such a memdev and region disappear, so I think this is enough.
>>

Hi Dave,


> The memdev, yes. It's devm on PF0. The region, no. Unbinding the endpoint port, or cxl_mem from the memdev, runs endpoint_unregister_region() under the endpoint's lock, not PF0's. PF0's release is only queued (schedule_detach() -> detach_memdev()). Until that work runs, PF0 is bound, the memdev is found, hpa_range is valid, and attach->cxlr points at a freed region. Root teardown (kill_regions()) and delete_region also unregister the region without touching PF0.


You are partially right. The problem is not that async detach_memdev() 
but the fact the region can be removed without the PF0 being aware ...


I was relying on the memdev being unregister first then the related 
device released later on, with the important part being the memdev being 
unregister making it unavailable to the other PFs because it can not be 
found in the cxl bus. I'm pretty sure about that scenario but I was not 
counting on sysfs unbinds for the endpoint device ...


I need to think further about this because I think it is wrong to remove 
the region in this case without PF0 or the memdev owner and original 
region consumer still unaware of it. One thing I tried to discuss with 
Dan was why we have the unbinding options for cxl_mem and cxl_port 
drivers. What is the point? I know this is standard linux device model, 
but the unbinding could do nothing if we decide so. Couldn't we? 
Otherwise, If there is a good reason for having this functionality where 
the unwinding can happen through different starting points, we should 
document it.


What I'm going to try is to link the region removal to Type2 driver 
removal as well, not necessarily with device links but through 
devm_action_or_reset links as we are already doing for other cases. And 
I think to document the different objects/devices involved and its 
lifetime depending on the current unwinding supported would be good to 
have, so I will work on that as well.


Thank you,

Alejandro


>
> I attached an LLM generated test kernel module you can use to reproduce the KASAN complaint. Commit log provides instructions.
> BUG: KASAN: slab-use-after-free in device_link_add+0x521/0xa80
>   cxl_get_range_and_link+0xa9/0x120 [cxl_core]
>
> DJ
>
>> I added comments in the exported function, cxl_get_range_and_link() which does the locking before calling the internal function __cxl_get_range_and_link() which looks for the memdev and the attach region. In fact, it should not be possible to obtain the PF0 memdev reference and the attach region not there yet, but the code is still checking that possibility as a sanity check.
>>
>>
>> Thank you,
>>
>> Alejandro.
>>
>>
>>> How about something like this?
>>>
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index 27e63e6dab7c..38ca73f12b84 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -4076,6 +4076,23 @@ static int first_mapped_decoder(struct device *dev, const void *data)
>>>        return 0;
>>>    }
>>>    +/*
>>> + * Invalidate @attach before the region goes away so that
>>> + * cxl_get_range_and_link() can not pick up a stale region.
>>> + */
>>> +static void endpoint_detach_attach_region(void *_attach)
>>> +{
>>> +    struct cxl_attach_region *attach = _attach;
>>> +    struct cxl_region *cxlr;
>>> +
>>> +    scoped_guard(rwsem_write, &cxl_rwsem.region) {
>>> +        cxlr = attach->cxlr;
>>> +        WRITE_ONCE(attach->cxlr, NULL);
>>> +        attach->hpa_range = DEFINE_RANGE(0, -1);
>>> +    }
>>> +    endpoint_unregister_region(cxlr);
>>> +}
>>> +
>>>    /*
>>>     * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
>>>     * simple case of single mapped decoder per memdev.
>>> @@ -4127,15 +4144,23 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>>>          /* Only teardown regions that pass validation, ignore the rest */
>>>        get_device(&cxlr->dev);
>>> -    rc = devm_add_action_or_reset(&endpoint->dev,
>>> -                      endpoint_unregister_region, cxlr);
>>> -    if (rc)
>>> +    /*
>>> +     * Not devm_add_action_or_reset(): the reset path would take
>>> +     * cxl_rwsem.region for write while it is held for read here. The
>>> +     * endpoint lock keeps the action from running before @attach is set.
>>> +     */
>>> +    rc = devm_add_action(&endpoint->dev, endpoint_detach_attach_region,
>>> +                 attach);
>>> +    if (rc) {
>>> +        put_device(&cxlr->dev);
>>>            return rc;
>>> +    }
>>>          attach->hpa_range = (struct range) {
>>>            .start = cxlr->params.res->start,
>>>            .end = cxlr->params.res->end,
>>>        };
>>> +    WRITE_ONCE(attach->cxlr, cxlr);
>>>        return 0;
>>>    }
>>>    EXPORT_SYMBOL_FOR_MODULES(cxl_memdev_attach_region, "cxl_mem");
>>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>>> index c401e3a1af06..7cd3a69cd5f5 100644
>>> --- a/drivers/cxl/cxlmem.h
>>> +++ b/drivers/cxl/cxlmem.h
>>> @@ -104,6 +104,8 @@ struct cxl_memdev_attach {
>>>    /**
>>>     * struct cxl_attach_region - coordinate mapping a region at memdev registration
>>>     * @attach: common core attachment descriptor
>>> + * @cxlr: cxl region the memdev is attached to, cleared under cxl_rwsem.region
>>> + *    before the region is unregistered
>>>     * @hpa_range: physical address range of the region
>>>     *
>>>     * For the common simple case of a CXL device with private (non-general purpose
>>> @@ -112,6 +114,7 @@ struct cxl_memdev_attach {
>>>     */
>>>    struct cxl_attach_region {
>>>        struct cxl_memdev_attach attach;
>>> +    struct cxl_region *cxlr;
>>>        struct range hpa_range;
>>>    };
>>>    

Re: [PATCH v2 2/4] cxl/region: Add region reference in memdev attach

[Dave Jiang]

On 10/8/26 6:50 AM, Lucero Palau, Alejandro wrote:
> 
> On 02/10/2026 16:52, Dave Jiang wrote:
>>
>> On 10/1/26 9:41 PM, Lucero Palau, Alejandro wrote:
>>> On 01/10/2026 22:38, Dave Jiang wrote:
>>>> On 10/1/26 6:20 AM, alucerop@amd.com wrote:
>>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>>
>>>>> Use a new field in cxl_attach_region struct for easily link it with the
>>>>> region the memdev is attached to.
>>>>>
>>>>> This facilitates device links creation where such a region is the supplier
>>>>> with non-PF0 physical functions wanting to use the CXL region being the
>>>>> consumers.
>>>>>
>>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>>> ---
>>>>>    drivers/cxl/core/region.c | 1 +
>>>>>    drivers/cxl/cxlmem.h      | 2 ++
>>>>>    2 files changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>>> index 27e63e6dab7c..78ca7ebc3e55 100644
>>>>> --- a/drivers/cxl/core/region.c
>>>>> +++ b/drivers/cxl/core/region.c
>>>>> @@ -4132,6 +4132,7 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>>>>>        if (rc)
>>>>>            return rc;
>>>>>    +    attach->cxlr = cxlr;
>>>>>        attach->hpa_range = (struct range) {
>>>>>            .start = cxlr->params.res->start,
>>>>>            .end = cxlr->params.res->end,
>>>>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>>>>> index c401e3a1af06..c598561b8e5f 100644
>>>>> --- a/drivers/cxl/cxlmem.h
>>>>> +++ b/drivers/cxl/cxlmem.h
>>>>> @@ -104,6 +104,7 @@ struct cxl_memdev_attach {
>>>>>    /**
>>>>>     * struct cxl_attach_region - coordinate mapping a region at memdev registration
>>>>>     * @attach: common core attachment descriptor
>>>>> + * @cxlr: cxl region the memdev is attached to.
>>>>>     * @hpa_range: physical address range of the region
>>>>>     *
>>>>>     * For the common simple case of a CXL device with private (non-general purpose
>>>>> @@ -112,6 +113,7 @@ struct cxl_memdev_attach {
>>>>>     */
>>>>>    struct cxl_attach_region {
>>>>>        struct cxl_memdev_attach attach;
>>>>> +    struct cxl_region *cxlr;
>>>>>        struct range hpa_range;
>>>>>    };
>>>>>    
>>>> attach->cxlr is never cleared when the region goes away. Unbinding the endpoint port runs endpoint_unregister_region(), which unregisters the region and drops its reference. PF0 stays bound until the detach work runs.
>>>>
>>>> A non-PF0 probe in that window still finds the memdev and calls device_link_add() on the freed region.
>>>
>>> I do not think so. This version, see next patch, relies on locking the supplier, PF0, before getting the memdev and potentially using the attach region. Only on PF0 release can such a memdev and region disappear, so I think this is enough.
>>>
> 
> Hi Dave,
> 
> 
>> The memdev, yes. It's devm on PF0. The region, no. Unbinding the endpoint port, or cxl_mem from the memdev, runs endpoint_unregister_region() under the endpoint's lock, not PF0's. PF0's release is only queued (schedule_detach() -> detach_memdev()). Until that work runs, PF0 is bound, the memdev is found, hpa_range is valid, and attach->cxlr points at a freed region. Root teardown (kill_regions()) and delete_region also unregister the region without touching PF0.
> 
> 
> You are partially right. The problem is not that async detach_memdev() but the fact the region can be removed without the PF0 being aware ...
> 
> 
> I was relying on the memdev being unregister first then the related device released later on, with the important part being the memdev being unregister making it unavailable to the other PFs because it can not be found in the cxl bus. I'm pretty sure about that scenario but I was not counting on sysfs unbinds for the endpoint device ...
> 
> 
> I need to think further about this because I think it is wrong to remove the region in this case without PF0 or the memdev owner and original region consumer still unaware of it. One thing I tried to discuss with Dan was why we have the unbinding options for cxl_mem and cxl_port drivers. What is the point? I know this is standard linux device model, but the unbinding could do nothing if we decide so. Couldn't we? Otherwise, If there is a good reason for having this functionality where the unwinding can happen through different starting points, we should document it.
> 
> 
> What I'm going to try is to link the region removal to Type2 driver removal as well, not necessarily with device links but through devm_action_or_reset links as we are already doing for other cases. And I think to document the different objects/devices involved and its lifetime depending on the current unwinding supported would be good to have, so I will work on that as well.
> 
> 

So there are multiple paths a region can go away without PF0 knowing and sysfs is only one of the triggers.
1. endpoint port teardown -> endpoint_unregister_region()
2. root decoder teardown -> kill_regions()
3. userspace delete region
4. some other places calling unregister_region(). 

Using suppress_bind_attrs only hides the sysfs files. There's also device hot-remove and module unload in addition to root decoder action and user action. So doing that is definitely the wrong way to go.

The path of PF0 going first is fine. The memdev goes with it. The bug path is region going first. Something needs to deal with the stale attach->cxlr. You need a region-side teardown clearing attach->cxlr under the cxl_rwsem.region before unregistering. And readers need to check under the same lock. The endpoint_detach_attach_region() proposal does close that hole.

DJ

> Thank you,
> 
> Alejandro
> 
> 
>>
>> I attached an LLM generated test kernel module you can use to reproduce the KASAN complaint. Commit log provides instructions.
>> BUG: KASAN: slab-use-after-free in device_link_add+0x521/0xa80
>>   cxl_get_range_and_link+0xa9/0x120 [cxl_core]
>>
>> DJ
>>
>>> I added comments in the exported function, cxl_get_range_and_link() which does the locking before calling the internal function __cxl_get_range_and_link() which looks for the memdev and the attach region. In fact, it should not be possible to obtain the PF0 memdev reference and the attach region not there yet, but the code is still checking that possibility as a sanity check.
>>>
>>>
>>> Thank you,
>>>
>>> Alejandro.
>>>
>>>
>>>> How about something like this?
>>>>
>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>> index 27e63e6dab7c..38ca73f12b84 100644
>>>> --- a/drivers/cxl/core/region.c
>>>> +++ b/drivers/cxl/core/region.c
>>>> @@ -4076,6 +4076,23 @@ static int first_mapped_decoder(struct device *dev, const void *data)
>>>>        return 0;
>>>>    }
>>>>    +/*
>>>> + * Invalidate @attach before the region goes away so that
>>>> + * cxl_get_range_and_link() can not pick up a stale region.
>>>> + */
>>>> +static void endpoint_detach_attach_region(void *_attach)
>>>> +{
>>>> +    struct cxl_attach_region *attach = _attach;
>>>> +    struct cxl_region *cxlr;
>>>> +
>>>> +    scoped_guard(rwsem_write, &cxl_rwsem.region) {
>>>> +        cxlr = attach->cxlr;
>>>> +        WRITE_ONCE(attach->cxlr, NULL);
>>>> +        attach->hpa_range = DEFINE_RANGE(0, -1);
>>>> +    }
>>>> +    endpoint_unregister_region(cxlr);
>>>> +}
>>>> +
>>>>    /*
>>>>     * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
>>>>     * simple case of single mapped decoder per memdev.
>>>> @@ -4127,15 +4144,23 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>>>>          /* Only teardown regions that pass validation, ignore the rest */
>>>>        get_device(&cxlr->dev);
>>>> -    rc = devm_add_action_or_reset(&endpoint->dev,
>>>> -                      endpoint_unregister_region, cxlr);
>>>> -    if (rc)
>>>> +    /*
>>>> +     * Not devm_add_action_or_reset(): the reset path would take
>>>> +     * cxl_rwsem.region for write while it is held for read here. The
>>>> +     * endpoint lock keeps the action from running before @attach is set.
>>>> +     */
>>>> +    rc = devm_add_action(&endpoint->dev, endpoint_detach_attach_region,
>>>> +                 attach);
>>>> +    if (rc) {
>>>> +        put_device(&cxlr->dev);
>>>>            return rc;
>>>> +    }
>>>>          attach->hpa_range = (struct range) {
>>>>            .start = cxlr->params.res->start,
>>>>            .end = cxlr->params.res->end,
>>>>        };
>>>> +    WRITE_ONCE(attach->cxlr, cxlr);
>>>>        return 0;
>>>>    }
>>>>    EXPORT_SYMBOL_FOR_MODULES(cxl_memdev_attach_region, "cxl_mem");
>>>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>>>> index c401e3a1af06..7cd3a69cd5f5 100644
>>>> --- a/drivers/cxl/cxlmem.h
>>>> +++ b/drivers/cxl/cxlmem.h
>>>> @@ -104,6 +104,8 @@ struct cxl_memdev_attach {
>>>>    /**
>>>>     * struct cxl_attach_region - coordinate mapping a region at memdev registration
>>>>     * @attach: common core attachment descriptor
>>>> + * @cxlr: cxl region the memdev is attached to, cleared under cxl_rwsem.region
>>>> + *    before the region is unregistered
>>>>     * @hpa_range: physical address range of the region
>>>>     *
>>>>     * For the common simple case of a CXL device with private (non-general purpose
>>>> @@ -112,6 +114,7 @@ struct cxl_memdev_attach {
>>>>     */
>>>>    struct cxl_attach_region {
>>>>        struct cxl_memdev_attach attach;
>>>> +    struct cxl_region *cxlr;
>>>>        struct range hpa_range;
>>>>    };
>>>>    

Re: [PATCH v2 2/4] cxl/region: Add region reference in memdev attach

[Lucero Palau, Alejandro]

On 08/10/2026 17:18, Dave Jiang wrote:
>
> On 10/8/26 6:50 AM, Lucero Palau, Alejandro wrote:
>> On 02/10/2026 16:52, Dave Jiang wrote:
>>> On 10/1/26 9:41 PM, Lucero Palau, Alejandro wrote:
>>>> On 01/10/2026 22:38, Dave Jiang wrote:
>>>>> On 10/1/26 6:20 AM, alucerop@amd.com wrote:
>>>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>>>
>>>>>> Use a new field in cxl_attach_region struct for easily link it with the
>>>>>> region the memdev is attached to.
>>>>>>
>>>>>> This facilitates device links creation where such a region is the supplier
>>>>>> with non-PF0 physical functions wanting to use the CXL region being the
>>>>>> consumers.
>>>>>>
>>>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>>>> ---
>>>>>>     drivers/cxl/core/region.c | 1 +
>>>>>>     drivers/cxl/cxlmem.h      | 2 ++
>>>>>>     2 files changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>>>> index 27e63e6dab7c..78ca7ebc3e55 100644
>>>>>> --- a/drivers/cxl/core/region.c
>>>>>> +++ b/drivers/cxl/core/region.c
>>>>>> @@ -4132,6 +4132,7 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>>>>>>         if (rc)
>>>>>>             return rc;
>>>>>>     +    attach->cxlr = cxlr;
>>>>>>         attach->hpa_range = (struct range) {
>>>>>>             .start = cxlr->params.res->start,
>>>>>>             .end = cxlr->params.res->end,
>>>>>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>>>>>> index c401e3a1af06..c598561b8e5f 100644
>>>>>> --- a/drivers/cxl/cxlmem.h
>>>>>> +++ b/drivers/cxl/cxlmem.h
>>>>>> @@ -104,6 +104,7 @@ struct cxl_memdev_attach {
>>>>>>     /**
>>>>>>      * struct cxl_attach_region - coordinate mapping a region at memdev registration
>>>>>>      * @attach: common core attachment descriptor
>>>>>> + * @cxlr: cxl region the memdev is attached to.
>>>>>>      * @hpa_range: physical address range of the region
>>>>>>      *
>>>>>>      * For the common simple case of a CXL device with private (non-general purpose
>>>>>> @@ -112,6 +113,7 @@ struct cxl_memdev_attach {
>>>>>>      */
>>>>>>     struct cxl_attach_region {
>>>>>>         struct cxl_memdev_attach attach;
>>>>>> +    struct cxl_region *cxlr;
>>>>>>         struct range hpa_range;
>>>>>>     };
>>>>>>     
>>>>> attach->cxlr is never cleared when the region goes away. Unbinding the endpoint port runs endpoint_unregister_region(), which unregisters the region and drops its reference. PF0 stays bound until the detach work runs.
>>>>>
>>>>> A non-PF0 probe in that window still finds the memdev and calls device_link_add() on the freed region.
>>>> I do not think so. This version, see next patch, relies on locking the supplier, PF0, before getting the memdev and potentially using the attach region. Only on PF0 release can such a memdev and region disappear, so I think this is enough.
>>>>
>> Hi Dave,
>>
>>
>>> The memdev, yes. It's devm on PF0. The region, no. Unbinding the endpoint port, or cxl_mem from the memdev, runs endpoint_unregister_region() under the endpoint's lock, not PF0's. PF0's release is only queued (schedule_detach() -> detach_memdev()). Until that work runs, PF0 is bound, the memdev is found, hpa_range is valid, and attach->cxlr points at a freed region. Root teardown (kill_regions()) and delete_region also unregister the region without touching PF0.
>>
>> You are partially right. The problem is not that async detach_memdev() but the fact the region can be removed without the PF0 being aware ...
>>
>>
>> I was relying on the memdev being unregister first then the related device released later on, with the important part being the memdev being unregister making it unavailable to the other PFs because it can not be found in the cxl bus. I'm pretty sure about that scenario but I was not counting on sysfs unbinds for the endpoint device ...
>>
>>
>> I need to think further about this because I think it is wrong to remove the region in this case without PF0 or the memdev owner and original region consumer still unaware of it. One thing I tried to discuss with Dan was why we have the unbinding options for cxl_mem and cxl_port drivers. What is the point? I know this is standard linux device model, but the unbinding could do nothing if we decide so. Couldn't we? Otherwise, If there is a good reason for having this functionality where the unwinding can happen through different starting points, we should document it.
>>
>>
>> What I'm going to try is to link the region removal to Type2 driver removal as well, not necessarily with device links but through devm_action_or_reset links as we are already doing for other cases. And I think to document the different objects/devices involved and its lifetime depending on the current unwinding supported would be good to have, so I will work on that as well.
>>
>>
> So there are multiple paths a region can go away without PF0 knowing and sysfs is only one of the triggers.
> 1. endpoint port teardown -> endpoint_unregister_region()
> 2. root decoder teardown -> kill_regions()
> 3. userspace delete region
> 4. some other places calling unregister_region().
>
> Using suppress_bind_attrs only hides the sysfs files. There's also device hot-remove and module unload in addition to root decoder action and user action. So doing that is definitely the wrong way to go.
>
> The path of PF0 going first is fine. The memdev goes with it. The bug path is region going first. Something needs to deal with the stale attach->cxlr. You need a region-side teardown clearing attach->cxlr under the cxl_rwsem.region before unregistering. And readers need to check under the same lock. The endpoint_detach_attach_region() proposal does close that hole.


I tried to discuss this with Dan unsuccessfully, so I hope I can make my 
point clear: having so many ways of cxl objects/devices being destroyed 
is, IMO, wrong. Moreover, some user actions on things created by a Type2 
driver should not be so easy accessed, and definitely, having a region 
unregistered and released with its main consumer and owner completely 
unaware, should not be happening.


Dan and I addressed some concerns with "these options" but it is worse 
after realising now port and region can also suffer from unbinding 
actions. We contemplated memdev unbinding and that is supported, and 
acpi module removal as well (all the unwinding is hopefully right for 
sfc driver removal), but the fact is, current Type2 support is unsound. 
It is likely good enough with current usage expectations but something 
to improve/fix.


All this user space potential actions were implemented mainly for 
testing (I guess you know this). I did ask Dan about it, and I was 
expecting use cases where HDM decoders and regions are dynamically 
created, which makes a lot of sense to me, but the fact is all is 
relying on firmware/BIOS configuration. Richard is working on adding 
this functionality for Type2 and pmems, and Jonathan considers it 
theoretically useful as well, but the way is going to be handled 
requires, IMO, further thinking and maybe a change before someone starts 
using it (does anyone know about users now?).


As a summary, if we allow user space actions (at least for Type2) they 
need to be consistent and somehow protected.


Finally, you did not answer my question: what is the point user space 
removing and endpoint port handled by a Type2 driver? What about the cxl 
region? Maybe I am missing a necessity I can not see here, so please, 
help me to understand this if that is the case.


Thanks,

Alejandro.


>
> DJ
>
>> Thank you,
>>
>> Alejandro
>>
>>
>>> I attached an LLM generated test kernel module you can use to reproduce the KASAN complaint. Commit log provides instructions.
>>> BUG: KASAN: slab-use-after-free in device_link_add+0x521/0xa80
>>>    cxl_get_range_and_link+0xa9/0x120 [cxl_core]
>>>
>>> DJ
>>>
>>>> I added comments in the exported function, cxl_get_range_and_link() which does the locking before calling the internal function __cxl_get_range_and_link() which looks for the memdev and the attach region. In fact, it should not be possible to obtain the PF0 memdev reference and the attach region not there yet, but the code is still checking that possibility as a sanity check.
>>>>
>>>>
>>>> Thank you,
>>>>
>>>> Alejandro.
>>>>
>>>>
>>>>> How about something like this?
>>>>>
>>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>>> index 27e63e6dab7c..38ca73f12b84 100644
>>>>> --- a/drivers/cxl/core/region.c
>>>>> +++ b/drivers/cxl/core/region.c
>>>>> @@ -4076,6 +4076,23 @@ static int first_mapped_decoder(struct device *dev, const void *data)
>>>>>         return 0;
>>>>>     }
>>>>>     +/*
>>>>> + * Invalidate @attach before the region goes away so that
>>>>> + * cxl_get_range_and_link() can not pick up a stale region.
>>>>> + */
>>>>> +static void endpoint_detach_attach_region(void *_attach)
>>>>> +{
>>>>> +    struct cxl_attach_region *attach = _attach;
>>>>> +    struct cxl_region *cxlr;
>>>>> +
>>>>> +    scoped_guard(rwsem_write, &cxl_rwsem.region) {
>>>>> +        cxlr = attach->cxlr;
>>>>> +        WRITE_ONCE(attach->cxlr, NULL);
>>>>> +        attach->hpa_range = DEFINE_RANGE(0, -1);
>>>>> +    }
>>>>> +    endpoint_unregister_region(cxlr);
>>>>> +}
>>>>> +
>>>>>     /*
>>>>>      * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
>>>>>      * simple case of single mapped decoder per memdev.
>>>>> @@ -4127,15 +4144,23 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>>>>>           /* Only teardown regions that pass validation, ignore the rest */
>>>>>         get_device(&cxlr->dev);
>>>>> -    rc = devm_add_action_or_reset(&endpoint->dev,
>>>>> -                      endpoint_unregister_region, cxlr);
>>>>> -    if (rc)
>>>>> +    /*
>>>>> +     * Not devm_add_action_or_reset(): the reset path would take
>>>>> +     * cxl_rwsem.region for write while it is held for read here. The
>>>>> +     * endpoint lock keeps the action from running before @attach is set.
>>>>> +     */
>>>>> +    rc = devm_add_action(&endpoint->dev, endpoint_detach_attach_region,
>>>>> +                 attach);
>>>>> +    if (rc) {
>>>>> +        put_device(&cxlr->dev);
>>>>>             return rc;
>>>>> +    }
>>>>>           attach->hpa_range = (struct range) {
>>>>>             .start = cxlr->params.res->start,
>>>>>             .end = cxlr->params.res->end,
>>>>>         };
>>>>> +    WRITE_ONCE(attach->cxlr, cxlr);
>>>>>         return 0;
>>>>>     }
>>>>>     EXPORT_SYMBOL_FOR_MODULES(cxl_memdev_attach_region, "cxl_mem");
>>>>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>>>>> index c401e3a1af06..7cd3a69cd5f5 100644
>>>>> --- a/drivers/cxl/cxlmem.h
>>>>> +++ b/drivers/cxl/cxlmem.h
>>>>> @@ -104,6 +104,8 @@ struct cxl_memdev_attach {
>>>>>     /**
>>>>>      * struct cxl_attach_region - coordinate mapping a region at memdev registration
>>>>>      * @attach: common core attachment descriptor
>>>>> + * @cxlr: cxl region the memdev is attached to, cleared under cxl_rwsem.region
>>>>> + *    before the region is unregistered
>>>>>      * @hpa_range: physical address range of the region
>>>>>      *
>>>>>      * For the common simple case of a CXL device with private (non-general purpose
>>>>> @@ -112,6 +114,7 @@ struct cxl_memdev_attach {
>>>>>      */
>>>>>     struct cxl_attach_region {
>>>>>         struct cxl_memdev_attach attach;
>>>>> +    struct cxl_region *cxlr;
>>>>>         struct range hpa_range;
>>>>>     };
>>>>>     

Re: [PATCH v2 2/4] cxl/region: Add region reference in memdev attach

[Dave Jiang]

On 10/8/26 11:07 AM, Lucero Palau, Alejandro wrote:
> 
> On 08/10/2026 17:18, Dave Jiang wrote:
>>
>> On 10/8/26 6:50 AM, Lucero Palau, Alejandro wrote:
>>> On 02/10/2026 16:52, Dave Jiang wrote:
>>>> On 10/1/26 9:41 PM, Lucero Palau, Alejandro wrote:
>>>>> On 01/10/2026 22:38, Dave Jiang wrote:
>>>>>> On 10/1/26 6:20 AM, alucerop@amd.com wrote:
>>>>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>>>>
>>>>>>> Use a new field in cxl_attach_region struct for easily link it with the
>>>>>>> region the memdev is attached to.
>>>>>>>
>>>>>>> This facilitates device links creation where such a region is the supplier
>>>>>>> with non-PF0 physical functions wanting to use the CXL region being the
>>>>>>> consumers.
>>>>>>>
>>>>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>>>>> ---
>>>>>>>     drivers/cxl/core/region.c | 1 +
>>>>>>>     drivers/cxl/cxlmem.h      | 2 ++
>>>>>>>     2 files changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>>>>> index 27e63e6dab7c..78ca7ebc3e55 100644
>>>>>>> --- a/drivers/cxl/core/region.c
>>>>>>> +++ b/drivers/cxl/core/region.c
>>>>>>> @@ -4132,6 +4132,7 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>>>>>>>         if (rc)
>>>>>>>             return rc;
>>>>>>>     +    attach->cxlr = cxlr;
>>>>>>>         attach->hpa_range = (struct range) {
>>>>>>>             .start = cxlr->params.res->start,
>>>>>>>             .end = cxlr->params.res->end,
>>>>>>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>>>>>>> index c401e3a1af06..c598561b8e5f 100644
>>>>>>> --- a/drivers/cxl/cxlmem.h
>>>>>>> +++ b/drivers/cxl/cxlmem.h
>>>>>>> @@ -104,6 +104,7 @@ struct cxl_memdev_attach {
>>>>>>>     /**
>>>>>>>      * struct cxl_attach_region - coordinate mapping a region at memdev registration
>>>>>>>      * @attach: common core attachment descriptor
>>>>>>> + * @cxlr: cxl region the memdev is attached to.
>>>>>>>      * @hpa_range: physical address range of the region
>>>>>>>      *
>>>>>>>      * For the common simple case of a CXL device with private (non-general purpose
>>>>>>> @@ -112,6 +113,7 @@ struct cxl_memdev_attach {
>>>>>>>      */
>>>>>>>     struct cxl_attach_region {
>>>>>>>         struct cxl_memdev_attach attach;
>>>>>>> +    struct cxl_region *cxlr;
>>>>>>>         struct range hpa_range;
>>>>>>>     };
>>>>>>>     
>>>>>> attach->cxlr is never cleared when the region goes away. Unbinding the endpoint port runs endpoint_unregister_region(), which unregisters the region and drops its reference. PF0 stays bound until the detach work runs.
>>>>>>
>>>>>> A non-PF0 probe in that window still finds the memdev and calls device_link_add() on the freed region.
>>>>> I do not think so. This version, see next patch, relies on locking the supplier, PF0, before getting the memdev and potentially using the attach region. Only on PF0 release can such a memdev and region disappear, so I think this is enough.
>>>>>
>>> Hi Dave,
>>>
>>>
>>>> The memdev, yes. It's devm on PF0. The region, no. Unbinding the endpoint port, or cxl_mem from the memdev, runs endpoint_unregister_region() under the endpoint's lock, not PF0's. PF0's release is only queued (schedule_detach() -> detach_memdev()). Until that work runs, PF0 is bound, the memdev is found, hpa_range is valid, and attach->cxlr points at a freed region. Root teardown (kill_regions()) and delete_region also unregister the region without touching PF0.
>>>
>>> You are partially right. The problem is not that async detach_memdev() but the fact the region can be removed without the PF0 being aware ...
>>>
>>>
>>> I was relying on the memdev being unregister first then the related device released later on, with the important part being the memdev being unregister making it unavailable to the other PFs because it can not be found in the cxl bus. I'm pretty sure about that scenario but I was not counting on sysfs unbinds for the endpoint device ...
>>>
>>>
>>> I need to think further about this because I think it is wrong to remove the region in this case without PF0 or the memdev owner and original region consumer still unaware of it. One thing I tried to discuss with Dan was why we have the unbinding options for cxl_mem and cxl_port drivers. What is the point? I know this is standard linux device model, but the unbinding could do nothing if we decide so. Couldn't we? Otherwise, If there is a good reason for having this functionality where the unwinding can happen through different starting points, we should document it.
>>>
>>>
>>> What I'm going to try is to link the region removal to Type2 driver removal as well, not necessarily with device links but through devm_action_or_reset links as we are already doing for other cases. And I think to document the different objects/devices involved and its lifetime depending on the current unwinding supported would be good to have, so I will work on that as well.
>>>
>>>
>> So there are multiple paths a region can go away without PF0 knowing and sysfs is only one of the triggers.
>> 1. endpoint port teardown -> endpoint_unregister_region()
>> 2. root decoder teardown -> kill_regions()
>> 3. userspace delete region
>> 4. some other places calling unregister_region().
>>
>> Using suppress_bind_attrs only hides the sysfs files. There's also device hot-remove and module unload in addition to root decoder action and user action. So doing that is definitely the wrong way to go.
>>
>> The path of PF0 going first is fine. The memdev goes with it. The bug path is region going first. Something needs to deal with the stale attach->cxlr. You need a region-side teardown clearing attach->cxlr under the cxl_rwsem.region before unregistering. And readers need to check under the same lock. The endpoint_detach_attach_region() proposal does close that hole.
> 
> 
> I tried to discuss this with Dan unsuccessfully, so I hope I can make my point clear: having so many ways of cxl objects/devices being destroyed is, IMO, wrong. Moreover, some user actions on things created by a Type2 driver should not be so easy accessed, and definitely, having a region unregistered and released with its main consumer and owner completely unaware, should not be happening.
> 
> 
> Dan and I addressed some concerns with "these options" but it is worse after realising now port and region can also suffer from unbinding actions. We contemplated memdev unbinding and that is supported, and acpi module removal as well (all the unwinding is hopefully right for sfc driver removal), but the fact is, current Type2 support is unsound. It is likely good enough with current usage expectations but something to improve/fix.

Removing the acpi module will also cause the issue I pointed out. So that isn't safe either. The only path that's good right now is sfc driver removal or the device going away.

> 
> 
> All this user space potential actions were implemented mainly for testing (I guess you know this). I did ask Dan about it, and I was expecting use cases where HDM decoders and regions are dynamically created, which makes a lot of sense to me, but the fact is all is relying on firmware/BIOS configuration. Richard is working on adding this functionality for Type2 and pmems, and Jonathan considers it theoretically useful as well, but the way is going to be handled requires, IMO, further thinking and maybe a change before someone starts using it (does anyone know about users now?).
> 
> 
> As a summary, if we allow user space actions (at least for Type2) they need to be consistent and somehow protected.
> 
> 
> Finally, you did not answer my question: what is the point user space removing and endpoint port handled by a Type2 driver? What about the cxl region? Maybe I am missing a necessity I can not see here, so please, help me to understand this if that is the case.

Shouldn't does not mean does not exist. Sure I can agree with you that under normal operations, certain things a sane user should avoid doing for type2. But it is possible currently and those issues can be triggered. However you feel about the current CXL architecture, here we are with where it is. You can either consider the smaller changes I suggested to keep the attach->cxlr sane (or with some other means) and make what you need working now with raised the issue addressed, and come back with hashing out the larger grievances later, or keep beating this horse.... "It's silly for users to do that and therefore the issue can be ignored" is not a good enough reason for me look the other way and merge the code.

> 
> 
> Thanks,
> 
> Alejandro.
> 
> 
>>
>> DJ
>>
>>> Thank you,
>>>
>>> Alejandro
>>>
>>>
>>>> I attached an LLM generated test kernel module you can use to reproduce the KASAN complaint. Commit log provides instructions.
>>>> BUG: KASAN: slab-use-after-free in device_link_add+0x521/0xa80
>>>>    cxl_get_range_and_link+0xa9/0x120 [cxl_core]
>>>>
>>>> DJ
>>>>
>>>>> I added comments in the exported function, cxl_get_range_and_link() which does the locking before calling the internal function __cxl_get_range_and_link() which looks for the memdev and the attach region. In fact, it should not be possible to obtain the PF0 memdev reference and the attach region not there yet, but the code is still checking that possibility as a sanity check.
>>>>>
>>>>>
>>>>> Thank you,
>>>>>
>>>>> Alejandro.
>>>>>
>>>>>
>>>>>> How about something like this?
>>>>>>
>>>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>>>> index 27e63e6dab7c..38ca73f12b84 100644
>>>>>> --- a/drivers/cxl/core/region.c
>>>>>> +++ b/drivers/cxl/core/region.c
>>>>>> @@ -4076,6 +4076,23 @@ static int first_mapped_decoder(struct device *dev, const void *data)
>>>>>>         return 0;
>>>>>>     }
>>>>>>     +/*
>>>>>> + * Invalidate @attach before the region goes away so that
>>>>>> + * cxl_get_range_and_link() can not pick up a stale region.
>>>>>> + */
>>>>>> +static void endpoint_detach_attach_region(void *_attach)
>>>>>> +{
>>>>>> +    struct cxl_attach_region *attach = _attach;
>>>>>> +    struct cxl_region *cxlr;
>>>>>> +
>>>>>> +    scoped_guard(rwsem_write, &cxl_rwsem.region) {
>>>>>> +        cxlr = attach->cxlr;
>>>>>> +        WRITE_ONCE(attach->cxlr, NULL);
>>>>>> +        attach->hpa_range = DEFINE_RANGE(0, -1);
>>>>>> +    }
>>>>>> +    endpoint_unregister_region(cxlr);
>>>>>> +}
>>>>>> +
>>>>>>     /*
>>>>>>      * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
>>>>>>      * simple case of single mapped decoder per memdev.
>>>>>> @@ -4127,15 +4144,23 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>>>>>>           /* Only teardown regions that pass validation, ignore the rest */
>>>>>>         get_device(&cxlr->dev);
>>>>>> -    rc = devm_add_action_or_reset(&endpoint->dev,
>>>>>> -                      endpoint_unregister_region, cxlr);
>>>>>> -    if (rc)
>>>>>> +    /*
>>>>>> +     * Not devm_add_action_or_reset(): the reset path would take
>>>>>> +     * cxl_rwsem.region for write while it is held for read here. The
>>>>>> +     * endpoint lock keeps the action from running before @attach is set.
>>>>>> +     */
>>>>>> +    rc = devm_add_action(&endpoint->dev, endpoint_detach_attach_region,
>>>>>> +                 attach);
>>>>>> +    if (rc) {
>>>>>> +        put_device(&cxlr->dev);
>>>>>>             return rc;
>>>>>> +    }
>>>>>>           attach->hpa_range = (struct range) {
>>>>>>             .start = cxlr->params.res->start,
>>>>>>             .end = cxlr->params.res->end,
>>>>>>         };
>>>>>> +    WRITE_ONCE(attach->cxlr, cxlr);
>>>>>>         return 0;
>>>>>>     }
>>>>>>     EXPORT_SYMBOL_FOR_MODULES(cxl_memdev_attach_region, "cxl_mem");
>>>>>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>>>>>> index c401e3a1af06..7cd3a69cd5f5 100644
>>>>>> --- a/drivers/cxl/cxlmem.h
>>>>>> +++ b/drivers/cxl/cxlmem.h
>>>>>> @@ -104,6 +104,8 @@ struct cxl_memdev_attach {
>>>>>>     /**
>>>>>>      * struct cxl_attach_region - coordinate mapping a region at memdev registration
>>>>>>      * @attach: common core attachment descriptor
>>>>>> + * @cxlr: cxl region the memdev is attached to, cleared under cxl_rwsem.region
>>>>>> + *    before the region is unregistered
>>>>>>      * @hpa_range: physical address range of the region
>>>>>>      *
>>>>>>      * For the common simple case of a CXL device with private (non-general purpose
>>>>>> @@ -112,6 +114,7 @@ struct cxl_memdev_attach {
>>>>>>      */
>>>>>>     struct cxl_attach_region {
>>>>>>         struct cxl_memdev_attach attach;
>>>>>> +    struct cxl_region *cxlr;
>>>>>>         struct range hpa_range;
>>>>>>     };
>>>>>>     

Re: [PATCH v2 2/4] cxl/region: Add region reference in memdev attach

[Lucero Palau, Alejandro]

On 08/10/2026 22:05, Dave Jiang wrote:
>
> On 10/8/26 11:07 AM, Lucero Palau, Alejandro wrote:


<snip>

>>
>> Dan and I addressed some concerns with "these options" but it is worse after realising now port and region can also suffer from unbinding actions. We contemplated memdev unbinding and that is supported, and acpi module removal as well (all the unwinding is hopefully right for sfc driver removal), but the fact is, current Type2 support is unsound. It is likely good enough with current usage expectations but something to improve/fix.
> Removing the acpi module will also cause the issue I pointed out. So that isn't safe either. The only path that's good right now is sfc driver removal or the device going away.


No. Adding multi PF support brings new problems. I need to look at the 
other unbinding options I was not contemplating, but acpi module and mem 
unbinding are safe. Maybe not correct semantically, but safe.


>>
>> All this user space potential actions were implemented mainly for testing (I guess you know this). I did ask Dan about it, and I was expecting use cases where HDM decoders and regions are dynamically created, which makes a lot of sense to me, but the fact is all is relying on firmware/BIOS configuration. Richard is working on adding this functionality for Type2 and pmems, and Jonathan considers it theoretically useful as well, but the way is going to be handled requires, IMO, further thinking and maybe a change before someone starts using it (does anyone know about users now?).
>>
>>
>> As a summary, if we allow user space actions (at least for Type2) they need to be consistent and somehow protected.
>>
>>
>> Finally, you did not answer my question: what is the point user space removing and endpoint port handled by a Type2 driver? What about the cxl region? Maybe I am missing a necessity I can not see here, so please, help me to understand this if that is the case.
> Shouldn't does not mean does not exist. Sure I can agree with you that under normal operations, certain things a sane user should avoid doing for type2. But it is possible currently and those issues can be triggered. However you feel about the current CXL architecture, here we are with where it is. You can either consider the smaller changes I suggested to keep the attach->cxlr sane (or with some other means) and make what you need working now with raised the issue addressed, and come back with hashing out the larger grievances later, or keep beating this horse.... "It's silly for users to do that and therefore the issue can be ignored" is not a good enough reason for me look the other way and merge the code.


I'm not denying the problem. I just do not want to add some new 
functionality which comes with these new issues, at least until I can 
understand it fully. What you propose is, I think, correct, and fixing 
at least some of the issues. But I think this is a good opportunity for 
trying to address this sysfs functionality, or at least to discuss it. 
As I said, also when basic Type2 support upstream effort started, Type2 
CXL should not be "open" to user space as Type3 (or not by default), 
although I think this complexity and so many different unwinding paths 
should be avoided ... or documented the reason behind it.


So, I will work on some documentation about all this, with cxl devices 
lifespan and those different unwinding paths, emphasising the different 
theoretical needs between Type2 and Type3. Once the unwinding paths are 
identified and documented, someone  can add the reason/use case behind 
it, or maybe some problems with them we are not seeing now.

Re: [PATCH v2 2/4] cxl/region: Add region reference in memdev attach

[Dave Jiang]

[-- Attachment #1: Type: text/plain, Size: 4245 bytes --]



On 10/8/26 11:58 PM, Lucero Palau, Alejandro wrote:
> 
> On 08/10/2026 22:05, Dave Jiang wrote:
>>
>> On 10/8/26 11:07 AM, Lucero Palau, Alejandro wrote:
> 
> 
> <snip>
> 
>>>
>>> Dan and I addressed some concerns with "these options" but it is worse after realising now port and region can also suffer from unbinding actions. We contemplated memdev unbinding and that is supported, and acpi module removal as well (all the unwinding is hopefully right for sfc driver removal), but the fact is, current Type2 support is unsound. It is likely good enough with current usage expectations but something to improve/fix.
>> Removing the acpi module will also cause the issue I pointed out. So that isn't safe either. The only path that's good right now is sfc driver removal or the device going away.
> 
> 
> No. Adding multi PF support brings new problems. I need to look at the other unbinding options I was not contemplating, but acpi module and mem unbinding are safe. Maybe not correct semantically, but safe.

I can hit a KASAN use after free issue with cxl_acpi module removal. I attached the LLM generated test patch with reproduction steps in commit log. Removal of cxl_acpi causes region removal and thus PF1 can hit a stale region ptr from attach struct.

BUG: KASAN: slab-use-after-free in device_link_add+0x521/0xa80
 cxl_get_range_and_link+0xa9/0x120 [cxl_core]


> 
> 
>>>
>>> All this user space potential actions were implemented mainly for testing (I guess you know this). I did ask Dan about it, and I was expecting use cases where HDM decoders and regions are dynamically created, which makes a lot of sense to me, but the fact is all is relying on firmware/BIOS configuration. Richard is working on adding this functionality for Type2 and pmems, and Jonathan considers it theoretically useful as well, but the way is going to be handled requires, IMO, further thinking and maybe a change before someone starts using it (does anyone know about users now?).
>>>
>>>
>>> As a summary, if we allow user space actions (at least for Type2) they need to be consistent and somehow protected.
>>>
>>>
>>> Finally, you did not answer my question: what is the point user space removing and endpoint port handled by a Type2 driver? What about the cxl region? Maybe I am missing a necessity I can not see here, so please, help me to understand this if that is the case.
>> Shouldn't does not mean does not exist. Sure I can agree with you that under normal operations, certain things a sane user should avoid doing for type2. But it is possible currently and those issues can be triggered. However you feel about the current CXL architecture, here we are with where it is. You can either consider the smaller changes I suggested to keep the attach->cxlr sane (or with some other means) and make what you need working now with raised the issue addressed, and come back with hashing out the larger grievances later, or keep beating this horse.... "It's silly for users to do that and therefore the issue can be ignored" is not a good enough reason for me look the other way and merge the code.
> 
> 
> I'm not denying the problem. I just do not want to add some new functionality which comes with these new issues, at least until I can understand it fully. What you propose is, I think, correct, and fixing at least some of the issues. But I think this is a good opportunity for trying to address this sysfs functionality, or at least to discuss it. As I said, also when basic Type2 support upstream effort started, Type2 CXL should not be "open" to user space as Type3 (or not by default), although I think this complexity and so many different unwinding paths should be avoided ... or documented the reason behind it.
> 
>

Agreed on we should talk about this as a community and decide on next steps for type2. Documentation is always good. 
> So, I will work on some documentation about all this, with cxl devices lifespan and those different unwinding paths, emphasising the different theoretical needs between Type2 and Type3. Once the unwinding paths are identified and documented, someone  can add the reason/use case behind it, or maybe some problems with them we are not seeing now.

Thank you! Appreciate you doing that. 
> 
> 
> 

[-- Attachment #2: 0001-TEST-ONLY-cxl-test-mock-non-PF0-consumer-for-cxl_get.patch --]
[-- Type: text/x-patch, Size: 5168 bytes --]

From 7c8a2b3aaeb54c39f54ed1226934136904ab3062 Mon Sep 17 00:00:00 2001
From: Dave Jiang <dave.jiang@intel.com>
Date: Thu, 1 Oct 2026 14:13:19 -0700
Subject: [PATCH] TEST ONLY: cxl/test: mock non-PF0 consumer for
 cxl_get_range_and_link()

Not for submission. Add cxl_mock_pfx, a platform driver whose probe calls
cxl_get_range_and_link() against the cxl_test type-2 accelerator
(cxl_type2_accel.0), the way a non-PF0 function would.

Unbinding cxl_acpi tears down the port hierarchy, including the endpoint
and the region attached to cxl_type2_accel.0. PF0 is released later, from
the memdev detach work. A non-PF0 probe in between still finds the memdev
and links to the dead region through attach->cxlr.

To reproduce, on a KASAN kernel:

  # reload if cxl_test was loaded without type2_test
  modprobe -r cxl_mock_pfx cxl_test 2>/dev/null
  modprobe cxl_test type2_test=1
  modprobe cxl_mock_pfx
  until [ -e /sys/bus/cxl/devices/region0 ]; do sleep 0.1; done

  D=/sys/bus/platform/drivers/cxl_mock_pfx
  for i in $(seq 400); do
      echo cxl_mock_pfx.1 > $D/unbind 2>/dev/null
      echo cxl_mock_pfx.1 > $D/bind 2>/dev/null
  done &
  sleep 0.2
  echo cxl_acpi.0 > /sys/bus/platform/drivers/cxl_acpi/unbind
  wait
  dmesg | grep -A20 'BUG: KASAN'

Expected:

  BUG: KASAN: slab-use-after-free in device_link_add+0x521/0xa80
   cxl_get_range_and_link+0xa9/0x120 [cxl_core]

A WARN in device_links_driver_bound() comes first: a non-PF0 probe links
to the region after its driver is gone. That link holds the last region
reference, so unbinding cxl_mock_pfx.1 frees the region. The next probe
uses the freed region.

On v2 of the series applied to v7.3-rc4, this hit on the first pass in 3
of 3 boots. The same bind/unbind loop without the cxl_acpi unbind ran 10
passes with no KASAN report or WARN. Repeat from the modprobe of cxl_test
if the window is missed.

cxl_test holds a reference on cxl_acpi, so "modprobe -r cxl_acpi" fails
here. The driver unbind runs the same teardown that module removal does.
Unbinding the endpoint from cxl_port instead of cxl_acpi hits the same
window.

The use-after-free is only reported reliably with KASAN:

  CONFIG_KASAN=y
  CONFIG_KASAN_GENERIC=y

Assisted-by: LLM
---
 tools/testing/cxl/test/Kbuild |  2 +
 tools/testing/cxl/test/pfx.c  | 77 +++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)
 create mode 100644 tools/testing/cxl/test/pfx.c

diff --git a/tools/testing/cxl/test/Kbuild b/tools/testing/cxl/test/Kbuild
index 9a24ddc28488..279d89be4a4d 100644
--- a/tools/testing/cxl/test/Kbuild
+++ b/tools/testing/cxl/test/Kbuild
@@ -6,11 +6,13 @@ obj-m += cxl_mock.o
 obj-m += cxl_mock_mem.o
 obj-m += cxl_translate.o
 obj-m += cxl_mock_accel.o
+obj-m += cxl_mock_pfx.o
 
 cxl_test-y := cxl.o
 cxl_test-y += hmem_test.o
 cxl_mock-y := mock.o
 cxl_mock_mem-y := mem.o
 cxl_mock_accel-y := accel.o
+cxl_mock_pfx-y := pfx.o
 
 KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
diff --git a/tools/testing/cxl/test/pfx.c b/tools/testing/cxl/test/pfx.c
new file mode 100644
index 000000000000..39c0c081c896
--- /dev/null
+++ b/tools/testing/cxl/test/pfx.c
@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * TEST ONLY: mock non-PF0 consumer. Its probe links to the region attached
+ * to the cxl_test type-2 accelerator via cxl_get_range_and_link().
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <cxl/cxl.h>
+
+static char *pf0_name = "cxl_type2_accel.0";
+module_param(pf0_name, charp, 0444);
+
+static struct platform_device *pfx_pdev;
+
+static int cxl_mock_pfx_probe(struct platform_device *pdev)
+{
+	struct device *pf0;
+	struct range range;
+	int rc;
+
+	pf0 = bus_find_device_by_name(&platform_bus_type, NULL, pf0_name);
+	if (!pf0) {
+		dev_info(&pdev->dev, "pfx_test: %s not found\n", pf0_name);
+		return -ENODEV;
+	}
+
+	rc = cxl_get_range_and_link(pf0, &pdev->dev, &range);
+	put_device(pf0);
+	if (rc) {
+		dev_info(&pdev->dev, "pfx_test: link rc=%d\n", rc);
+		/* don't let -EPROBE_DEFER requeue us behind the test's back */
+		return rc == -EPROBE_DEFER ? -EAGAIN : rc;
+	}
+
+	dev_info(&pdev->dev, "pfx_test: link rc=0 range=%pra\n", &range);
+	return 0;
+}
+
+static void cxl_mock_pfx_remove(struct platform_device *pdev)
+{
+	dev_info(&pdev->dev, "pfx_test: removed\n");
+}
+
+static struct platform_driver cxl_mock_pfx_driver = {
+	.probe = cxl_mock_pfx_probe,
+	.remove = cxl_mock_pfx_remove,
+	.driver = {
+		.name = "cxl_mock_pfx",
+	},
+};
+
+static int __init cxl_mock_pfx_init(void)
+{
+	int rc;
+
+	pfx_pdev = platform_device_register_simple("cxl_mock_pfx", 1, NULL, 0);
+	if (IS_ERR(pfx_pdev))
+		return PTR_ERR(pfx_pdev);
+
+	rc = platform_driver_register(&cxl_mock_pfx_driver);
+	if (rc)
+		platform_device_unregister(pfx_pdev);
+	return rc;
+}
+module_init(cxl_mock_pfx_init);
+
+static void __exit cxl_mock_pfx_exit(void)
+{
+	platform_driver_unregister(&cxl_mock_pfx_driver);
+	platform_device_unregister(pfx_pdev);
+}
+module_exit(cxl_mock_pfx_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("cxl_test: TEST ONLY mock non-PF0 consumer");
+MODULE_IMPORT_NS("CXL");
-- 
2.54.0