RE: [PATCH 1/6] cxl/mem: Fix devm_cxl_memdev_edac_release() confusion

[Shiju Jose <shiju.jose@huawei.com>]

>-----Original Message-----
>From: Dan Williams <dan.j.williams@intel.com>
>Sent: 04 December 2025 02:22
>To: dave.jiang@intel.com
>Cc: linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org;
>Smita.KoralahalliChannabasappa@amd.com; alison.schofield@intel.com;
>terry.bowman@amd.com; alejandro.lucero-palau@amd.com; linux-
>pci@vger.kernel.org; Jonathan Cameron <jonathan.cameron@huawei.com>;
>Shiju Jose <shiju.jose@huawei.com>
>Subject: [PATCH 1/6] cxl/mem: Fix devm_cxl_memdev_edac_release() confusion
>
>A device release method is only for undoing allocations on the path to preparing
>the device for device_add(). In contrast, devm allocations are post device_add(),
>are acquired during / after ->probe() and are released synchronous with -
>>remove().
>
>So, a "devm" helper in a "release" method is a clear anti-pattern.
>
>Move this devm release action where it belongs, an action created at edac object
>creation time. Otherwise, this leaks resources until
>cxl_memdev_release() time which may be long after these xarray and error
>record caches have gone idle.
>
>Note, this also fixes up the type of @cxlmd->err_rec_array which needlessly
>dropped type-safety.
>
>Fixes: 0b5ccb0de1e2 ("cxl/edac: Support for finding memory operation
>attributes from the current boot")
>Cc: Dave Jiang <dave.jiang@intel.com>
>Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>Cc: Shiju Jose <shiju.jose@huawei.com>
>Cc: Alison Schofield <alison.schofield@intel.com>
>Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Tested-by: Shiju Jose <shiju.jose@huawei.com>
Reviewed-by: Shiju Jose <shiju.jose@huawei.com>
>---
> drivers/cxl/cxlmem.h      |  5 +--
> drivers/cxl/core/edac.c   | 64 ++++++++++++++++++++++-----------------
> drivers/cxl/core/memdev.c |  1 -
> 3 files changed, 38 insertions(+), 32 deletions(-)
>
>diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index
>434031a0c1f7..c12ab4fc9512 100644
>--- a/drivers/cxl/cxlmem.h
>+++ b/drivers/cxl/cxlmem.h
>@@ -63,7 +63,7 @@ struct cxl_memdev {
> 	int depth;
> 	u8 scrub_cycle;
> 	int scrub_region_id;
>-	void *err_rec_array;
>+	struct cxl_mem_err_rec *err_rec_array;
> };
>
> static inline struct cxl_memdev *to_cxl_memdev(struct device *dev) @@ -877,7
>+877,6 @@ int devm_cxl_memdev_edac_register(struct cxl_memdev *cxlmd);
>int devm_cxl_region_edac_register(struct cxl_region *cxlr);  int
>cxl_store_rec_gen_media(struct cxl_memdev *cxlmd, union cxl_event *evt);  int
>cxl_store_rec_dram(struct cxl_memdev *cxlmd, union cxl_event *evt); -void
>devm_cxl_memdev_edac_release(struct cxl_memdev *cxlmd);  #else  static
>inline int devm_cxl_memdev_edac_register(struct cxl_memdev *cxlmd)  { return
>0; } @@ -889,8 +888,6 @@ static inline int cxl_store_rec_gen_media(struct
>cxl_memdev *cxlmd,  static inline int cxl_store_rec_dram(struct cxl_memdev
>*cxlmd,
> 				     union cxl_event *evt)
> { return 0; }
>-static inline void devm_cxl_memdev_edac_release(struct cxl_memdev *cxlmd) -
>{ return; }  #endif
>
> #ifdef CONFIG_CXL_SUSPEND
>diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c index
>79994ca9bc9f..81160260e26b 100644
>--- a/drivers/cxl/core/edac.c
>+++ b/drivers/cxl/core/edac.c
>@@ -1988,6 +1988,40 @@ static int cxl_memdev_soft_ppr_init(struct
>cxl_memdev *cxlmd,
> 	return 0;
> }
>
>+static void err_rec_free(void *_cxlmd)
>+{
>+	struct cxl_memdev *cxlmd = _cxlmd;
>+	struct cxl_mem_err_rec *array_rec = cxlmd->err_rec_array;
>+	struct cxl_event_gen_media *rec_gen_media;
>+	struct cxl_event_dram *rec_dram;
>+	unsigned long index;
>+
>+	cxlmd->err_rec_array = NULL;
>+	xa_for_each(&array_rec->rec_dram, index, rec_dram)
>+		kfree(rec_dram);
>+	xa_destroy(&array_rec->rec_dram);
>+
>+	xa_for_each(&array_rec->rec_gen_media, index, rec_gen_media)
>+		kfree(rec_gen_media);
>+	xa_destroy(&array_rec->rec_gen_media);
>+	kfree(array_rec);
>+}
>+
>+static int devm_cxl_memdev_setup_err_rec(struct cxl_memdev *cxlmd) {
>+	struct cxl_mem_err_rec *array_rec =
>+		kzalloc(sizeof(*array_rec), GFP_KERNEL);
>+
>+	if (!array_rec)
>+		return -ENOMEM;
>+
>+	xa_init(&array_rec->rec_gen_media);
>+	xa_init(&array_rec->rec_dram);
>+	cxlmd->err_rec_array = array_rec;
>+
>+	return devm_add_action_or_reset(&cxlmd->dev, err_rec_free, cxlmd); }
>+
> int devm_cxl_memdev_edac_register(struct cxl_memdev *cxlmd)  {
> 	struct edac_dev_feature ras_features[CXL_NR_EDAC_DEV_FEATURES];
>@@ -2038,15 +2072,9 @@ int devm_cxl_memdev_edac_register(struct
>cxl_memdev *cxlmd)
> 		}
>
> 		if (repair_inst) {
>-			struct cxl_mem_err_rec *array_rec =
>-				devm_kzalloc(&cxlmd->dev, sizeof(*array_rec),
>-					     GFP_KERNEL);
>-			if (!array_rec)
>-				return -ENOMEM;
>-
>-			xa_init(&array_rec->rec_gen_media);
>-			xa_init(&array_rec->rec_dram);
>-			cxlmd->err_rec_array = array_rec;
>+			rc = devm_cxl_memdev_setup_err_rec(cxlmd);
>+			if (rc)
>+				return rc;
> 		}
> 	}
>
>@@ -2088,22 +2116,4 @@ int devm_cxl_region_edac_register(struct
>cxl_region *cxlr)  }  EXPORT_SYMBOL_NS_GPL(devm_cxl_region_edac_register,
>"CXL");
>
>-void devm_cxl_memdev_edac_release(struct cxl_memdev *cxlmd) -{
>-	struct cxl_mem_err_rec *array_rec = cxlmd->err_rec_array;
>-	struct cxl_event_gen_media *rec_gen_media;
>-	struct cxl_event_dram *rec_dram;
>-	unsigned long index;
>-
>-	if (!IS_ENABLED(CONFIG_CXL_EDAC_MEM_REPAIR) || !array_rec)
>-		return;
>-
>-	xa_for_each(&array_rec->rec_dram, index, rec_dram)
>-		kfree(rec_dram);
>-	xa_destroy(&array_rec->rec_dram);
>
>-	xa_for_each(&array_rec->rec_gen_media, index, rec_gen_media)
>-		kfree(rec_gen_media);
>-	xa_destroy(&array_rec->rec_gen_media);
>-}
>-EXPORT_SYMBOL_NS_GPL(devm_cxl_memdev_edac_release, "CXL"); diff --git
>a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c index
>e370d733e440..4dff7f44d908 100644
>--- a/drivers/cxl/core/memdev.c
>+++ b/drivers/cxl/core/memdev.c
>@@ -27,7 +27,6 @@ static void cxl_memdev_release(struct device *dev)
> 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>
> 	ida_free(&cxl_memdev_ida, cxlmd->id);
>-	devm_cxl_memdev_edac_release(cxlmd);
> 	kfree(cxlmd);
> }
>
>--
>2.51.1
>