Re: [PATCH v24 08/11] cxl: Export function for unwinding cxl by accelerators

Cheatham, Benjamin
2026-03-27T19:37:02Z

On 3/23/2026 6:31 AM, alejandro.lucero-palau@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
>
> Add cxl_unregister_region() to the accelerator driver API
> for a clean exit.
>
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/cxl/core/region.c | 17 ++++++++++++-----
> include/cxl/cxl.h | 1 +
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 9c3ba911d865..8bb53a095290 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2468,9 +2468,8 @@ static struct cxl_region *to_cxl_region(struct device *dev)
> return container_of(dev, struct cxl_region, dev);
> }
>
> -static void unregister_region(void *_cxlr)
> +void cxl_unregister_region(struct cxl_region *cxlr)
> {
> - struct cxl_region *cxlr = _cxlr;
> struct cxl_region_params *p = &cxlr->params;
> int i;
>
> @@ -2489,6 +2488,14 @@ static void unregister_region(void *_cxlr)
> cxl_region_iomem_release(cxlr);
> put_device(&cxlr->dev);
> }
> +EXPORT_SYMBOL_NS_GPL(cxl_unregister_region, "CXL");
> +
> +static void __unregister_region(void *_cxlr)
> +{
> + struct cxl_region *cxlr = _cxlr;
> +
> + return cxl_unregister_region(cxlr);
> +}

After taking another look, I have another possible solution: Leave cxl_unregister_region() as-is and
remove this wrapper function, but only register it below when the decoder type is HOSTONLYMEM
(with the below devm_release_action() calls updated accordingly). Then you'd add a disclaimer
that the accelerator driver should call cxl_unregister_region() during it's cleanup to cxl_get_committed_decoder().

I'm not sure that works out better than just letting the CXL core clean up the region and not touching it in
the accelerator driver though. I could see the above approach causing some region lifetime issues, but I'm
not well versed enough on that to actually know what they'd be :/.

Thanks,
Ben

>
> static struct lock_class_key cxl_region_key;
>
> @@ -2642,7 +2649,7 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
> if (rc)
> goto err;
>
> - rc = devm_add_action_or_reset(port->uport_dev, unregister_region, cxlr);
> + rc = devm_add_action_or_reset(port->uport_dev, __unregister_region, cxlr);
> if (rc)
> return ERR_PTR(rc);
>
> @@ -2797,7 +2804,7 @@ static ssize_t delete_region_store(struct device *dev,
> if (IS_ERR(cxlr))
> return PTR_ERR(cxlr);
>
> - devm_release_action(port->uport_dev, unregister_region, cxlr);
> + devm_release_action(port->uport_dev, __unregister_region, cxlr);
> put_device(&cxlr->dev);
>
> return len;
> @@ -3963,7 +3970,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>
> rc = __construct_region(cxlr, ctx);
> if (rc) {
> - devm_release_action(port->uport_dev, unregister_region, cxlr);
> + devm_release_action(port->uport_dev, __unregister_region, cxlr);
> return ERR_PTR(rc);
> }
>
> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> index 122b3331469d..b8cdc604480a 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -236,4 +236,5 @@ struct cxl_endpoint_decoder *cxl_get_committed_decoder(struct cxl_memdev *cxlmd,
> struct cxl_region **cxlr);
> struct range;
> int cxl_get_region_range(struct cxl_region *region, struct range *range);
> +void cxl_unregister_region(struct cxl_region *cxlr);
> #endif /* __CXL_CXL_H__ */