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

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

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;
 };