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

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

On 12/3/25 7:21 PM, Dan Williams wrote:
> 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>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

This can go into fixes right? Does not look like a dependency for the rest of the series.

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