Re: [RFC PATCH v2 1/3] cxl/mem: Separate provider registration from region attachment

["Lucero Palau, Alejandro" <alejandro.lucero-palau@amd.com>]

Hi Richard,


I think this is good stuff. Just some comments below.

Thanks!


On 07/10/2026 10:05, Richard Cheng wrote:
> devm_cxl_probe_mem() requires a committed region when registering a
> provider-owned memdev. If FW has not created a region


This is mixing kernel parlance with BIOS potential actions like setting 
HDM decoders.


> , registration
> fails before the provider can use the endpoint topology to provision
> one.
>
> Make the attachment probe callback optional while retaining a non-NULL
> descriptor to preserve provider ownership and handling of CXL link loss.
> Install endpoint cleanup for FW-discovered provider regions even when
> the provider never requests attachment.
>
> This establishes the registration and attachment APIs needed for
> explicit region provisioning without introducing region creation or
> allocation policy.
>
> Signed-off-by: Richard Cheng <icheng@nvidia.com>
> ---
> Changelog:
>
> v1 -> v2:
> - Rework the patch into a registration/attachment refactor, following
>    Alejandro's request to separate region provisioning from memdev
> registration.
> - Add devm_cxl_register_mem() to establish a provider-owned memdev and
>    its endpoint without requiring a comitted region.
> - Add devm_cxl_attach_mem_region() to obtain the HPA range of an
>    existing committed, single-target region after registration.
> - Make the region probe callback optional while preserving provider
>    ownership, synchronous endpoint setup, and hdling of CXL link loss.
> ---
>   drivers/cxl/core/region.c | 146 ++++++++++++++++++++++++++------------
>   drivers/cxl/cxlmem.h      |  22 ++++--
>   drivers/cxl/mem.c         |  69 +++++++++++++++++-
>   include/cxl/cxl.h         |   2 +
>   4 files changed, 188 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5ef0ca0694ff..7a64a730587d 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -4111,67 +4111,123 @@ static int first_mapped_decoder(struct device *dev, const void *data)
>   	return 0;
>   }
>   
> -/*
> - * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
> - * simple case of single mapped decoder per memdev.
> - */
> -int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
> +static int unregister_memdev_region(struct device *dev, void *data)
>   {
> -	struct cxl_attach_region *attach =
> -		container_of(cxlmd->attach, typeof(*attach), attach);
> -	struct cxl_port *endpoint = cxlmd->endpoint;
>   	struct cxl_endpoint_decoder *cxled;
>   	struct cxl_region *cxlr;
> -	int rc;
>   
> -	/* hold endpoint lock to setup autoremove of the region */
> +	if (!is_endpoint_decoder(dev))
> +		return 0;
> +
> +	cxled = to_cxl_endpoint_decoder(dev);
> +	scoped_guard(rwsem_read, &cxl_rwsem.region) {
> +		cxlr = cxled->cxld.region;
> +		if (!cxlr)
> +			return 0;
> +		get_device(&cxlr->dev);
> +	}
> +
> +	/* Unregistration needs the region write lock. */
> +	endpoint_unregister_region(cxlr);
> +	return 0;
> +}
> +
> +static void endpoint_unregister_regions(void *data)
> +{
> +	struct cxl_port *endpoint = data;
> +
> +	device_for_each_child(&endpoint->dev, NULL, unregister_memdev_region);
> +}
> +
> +int cxl_memdev_setup_region_cleanup(struct cxl_memdev *cxlmd)
> +{
> +	struct cxl_port *endpoint = cxlmd->endpoint;
> +
> +	device_lock_assert(&cxlmd->dev);
> +	if (IS_ERR_OR_NULL(endpoint))
> +		return -ENXIO;
> +
>   	guard(device)(&endpoint->dev);
> -	if (!endpoint->dev.driver)
> +	if (!endpoint->dev.driver || endpoint->dead)
>   		return -ENXIO;
> -	guard(rwsem_read)(&cxl_rwsem.region);
> -	guard(rwsem_read)(&cxl_rwsem.dpa);
>   
>   	/*
> -	 * TODO auto-instantiate a region, for now assume this will find an
> -	 * auto-region
> +	 * Endpoint probe may discover provider-owned firmware regions even if
> +	 * the provider never requests their HPA range. Run before decoder
> +	 * teardown so those regions are unregistered, not just detached.
>   	 */


I would put this as a description for the function instead of a comment 
here.


> -	struct device *dev __free(put_device) =
> -		device_find_child(&endpoint->dev, NULL, first_mapped_decoder);
> -
> -	if (!dev) {
> -		dev_dbg(cxlmd->cxlds->dev, "no region found for memdev %s\n",
> -			dev_name(&cxlmd->dev));
> -		return -ENXIO;
> -	}
> +	return devm_add_action_or_reset(&endpoint->dev,
> +				      endpoint_unregister_regions, endpoint);
> +}
> +EXPORT_SYMBOL_FOR_MODULES(cxl_memdev_setup_region_cleanup, "cxl_mem");
>   
> -	cxled = to_cxl_endpoint_decoder(dev);
> -	cxlr = cxled->cxld.region;
> +/* Caller holds the memdev lock; attach to a single mapped decoder. */
> +int cxl_memdev_attach_region(struct cxl_memdev *cxlmd, struct range *hpa_range)
> +{
> +	struct cxl_port *endpoint = cxlmd->endpoint;
> +	struct cxl_endpoint_decoder *cxled;
> +	struct cxl_region *cxlr;
> +	int rc;
>   
> -	if (cxlr->params.state < CXL_CONFIG_COMMIT) {
> -		dev_dbg(cxlmd->cxlds->dev,
> -			"region %s not committed for memdev %s\n",
> -			dev_name(&cxlr->dev), dev_name(&cxlmd->dev));
> +	device_lock_assert(&cxlmd->dev);
> +	if (IS_ERR_OR_NULL(endpoint))
>   		return -ENXIO;
> -	}
>   
> -	if (cxlr->params.nr_targets > 1) {
> -		dev_dbg(cxlmd->cxlds->dev,
> -			"Only attach to local non-interleaved region\n");
> +	/* hold endpoint lock to setup autoremove of the region */
> +	guard(device)(&endpoint->dev);
> +	if (!endpoint->dev.driver || endpoint->dead)
>   		return -ENXIO;
> -	}
>   
> -	/* 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)
> -		return rc;
> +	scoped_guard(rwsem_read, &cxl_rwsem.region) {
> +		guard(rwsem_read)(&cxl_rwsem.dpa);
>   
> -	attach->hpa_range = (struct range) {
> -		.start = cxlr->params.res->start,
> -		.end = cxlr->params.res->end,
> -	};
> -	return 0;
> +		struct device *dev __free(put_device) =
> +			device_find_child(&endpoint->dev, NULL,
> +					  first_mapped_decoder);
> +		if (!dev) {
> +			dev_dbg(cxlmd->cxlds->dev,
> +				"no region found for memdev %s\n",
> +				dev_name(&cxlmd->dev));
> +			return -ENXIO;
> +		}
> +
> +		cxled = to_cxl_endpoint_decoder(dev);
> +		cxlr = cxled->cxld.region;
> +		if (cxlr->params.state < CXL_CONFIG_COMMIT) {
> +			dev_dbg(cxlmd->cxlds->dev,
> +				"region %s not committed for memdev %s\n",
> +				dev_name(&cxlr->dev), dev_name(&cxlmd->dev));
> +			return -ENXIO;
> +		}
> +
> +		if (cxlr->params.nr_targets > 1) {
> +			dev_dbg(cxlmd->cxlds->dev,
> +				"Only attach to local non-interleaved region\n");
> +			return -ENXIO;
> +		}
> +		if (!cxlr->params.res)
> +			return -ENXIO;
> +
> +		/* Only teardown regions that pass validation, ignore the rest. */
> +		if (!devm_is_action_added(&endpoint->dev,
> +					  endpoint_unregister_region, cxlr)) {
> +			get_device(&cxlr->dev);
> +			rc = devm_add_action(&endpoint->dev,
> +					     endpoint_unregister_region, cxlr);
> +			if (rc)
> +				break;
> +		}
> +
> +		*hpa_range = (struct range) {
> +			.start = cxlr->params.res->start,
> +			.end = cxlr->params.res->end,
> +		};
> +		return 0;
> +	}
> +
> +	/* devm_add_action() failed; teardown needs the region write lock. */
> +	endpoint_unregister_region(cxlr);
> +	return rc;
>   }
>   EXPORT_SYMBOL_FOR_MODULES(cxl_memdev_attach_region, "cxl_mem");
>   
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index c401e3a1af06..8c050bc308bd 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -97,6 +97,13 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
>   	return is_cxl_memdev(port->uport_dev);
>   }
>   
> +/**
> + * struct cxl_memdev_attach - provider ownership and CXL link requirements
> + * @probe: optional region probe callback, called with the memdev locked
> + *
> + * A non-NULL descriptor requires successful synchronous endpoint setup and
> + * preserves provider ownership even when no region probe is requested.
> + */
>   struct cxl_memdev_attach {
>   	int (*probe)(struct cxl_memdev *cxlmd);
>   };
> @@ -107,8 +114,8 @@ struct cxl_memdev_attach {
>    * @hpa_range: physical address range of the region
>    *
>    * For the common simple case of a CXL device with private (non-general purpose
> - * / "accelerator") memory, enumerate firmware instantiated region, or
> - * instantiate a region for the device's capacity. Destroy the region on detach.
> + * / "accelerator") memory, enumerate a firmware-instantiated region and
> + * report its range. Destroy the region on detach.
>    */
>   struct cxl_attach_region {
>   	struct cxl_memdev_attach attach;
> @@ -116,12 +123,19 @@ struct cxl_attach_region {
>   };
>   
>   #ifdef CONFIG_CXL_REGION
> -int cxl_memdev_attach_region(struct cxl_memdev *cxlmd);
> +int cxl_memdev_attach_region(struct cxl_memdev *cxlmd, struct range *hpa_range);
> +int cxl_memdev_setup_region_cleanup(struct cxl_memdev *cxlmd);
>   #else
> -static inline int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
> +static inline int cxl_memdev_attach_region(struct cxl_memdev *cxlmd,
> +					   struct range *hpa_range)
>   {
>   	return -EOPNOTSUPP;
>   }
> +
> +static inline int cxl_memdev_setup_region_cleanup(struct cxl_memdev *cxlmd)
> +{
> +	return 0;
> +}
>   #endif
>   
>   struct cxl_memdev *devm_cxl_add_classdev(struct cxl_dev_state *cxlds);
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 3959ec963026..aa08d88ab104 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -172,7 +172,10 @@ static int cxl_mem_probe(struct device *dev)
>   	}
>   
>   	if (cxlmd->attach) {
> -		rc = cxlmd->attach->probe(cxlmd);
> +		if (cxlmd->attach->probe)
> +			rc = cxlmd->attach->probe(cxlmd);
> +		else
> +			rc = cxl_memdev_setup_region_cleanup(cxlmd);
>   		if (rc)
>   			return rc;
>   	}
> @@ -215,6 +218,68 @@ struct cxl_memdev *devm_cxl_add_classdev(struct cxl_dev_state *cxlds)
>   }
>   EXPORT_SYMBOL_NS_GPL(devm_cxl_add_classdev, "CXL");
>   
> +/**
> + * devm_cxl_register_mem - Register a provider-owned CXL memory device
> + * @cxlds: CXL device state to associate with the memdev
> + *
> + * Establish the CXL port topology and endpoint synchronously, without requiring
> + * a committed region. The provider retains ownership of its memory, including
> + * any firmware-discovered regions, and must detach if the CXL link is lost.
> + *
> + * The parent of the resulting device and the devm context for allocations is
> + * @cxlds->dev. Returns the registered memdev or an ERR_PTR() on failure.
> + */
> +struct cxl_memdev *devm_cxl_register_mem(struct cxl_dev_state *cxlds)
> +{
> +	struct cxl_memdev_attach *attach;
> +
> +	attach = devm_kzalloc(cxlds->dev, sizeof(*attach), GFP_KERNEL);
> +	if (!attach)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return __devm_cxl_add_memdev(cxlds, attach);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_register_mem, "CXL");
> +
> +/**
> + * devm_cxl_attach_mem_region - Attach a registered memdev to its region
> + * @cxlmd: provider-owned memdev returned by devm_cxl_register_mem()
> + * @hpa_range: CXL.mem physical address range result
> + *
> + * Attach to an existing committed, single-target region. This does not create
> + * or program a region. Repeated attachment to the same region returns the same
> + * range without adding another cleanup action. Failure leaves the memdev
> + * registered so that the provider can decide how to proceed.
> + *
> + * The region is removed when the endpoint detaches. Returns zero on success or
> + * a negative errno; @hpa_range is empty on failure.
> + */
> +int devm_cxl_attach_mem_region(struct cxl_memdev *cxlmd,


is this devm prefix required here?


> +			       struct range *hpa_range)
> +{
> +	if (!hpa_range)
> +		return -EINVAL;
> +	*hpa_range = DEFINE_RANGE(0, -1);
> +
> +	if (!cxlmd->attach)
> +		return -EINVAL;
> +
> +	guard(device)(&cxlmd->dev);
> +	if (!cxlmd->dev.driver || !cxlmd->cxlds)
> +		return -ENXIO;
> +
> +	return cxl_memdev_attach_region(cxlmd, hpa_range);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_attach_mem_region, "CXL");
> +
> +static int cxl_probe_mem_region(struct cxl_memdev *cxlmd)
> +{
> +	struct cxl_attach_region *attach =
> +		container_of(cxlmd->attach, typeof(*attach), attach);
> +
> +	return cxl_memdev_attach_region(cxlmd, &attach->hpa_range);
> +}
> +
>   /**
>    * devm_cxl_probe_mem - Add a CXL memory device and probe its region
>    * @cxlds: CXL device state to associate with the memdev
> @@ -242,7 +307,7 @@ struct cxl_memdev *devm_cxl_probe_mem(struct cxl_dev_state *cxlds,
>   
>   	*attach = (struct cxl_attach_region) {
>   		.attach = {
> -			   .probe = cxl_memdev_attach_region,
> +			   .probe = cxl_probe_mem_region,
>   		},
>   		.hpa_range = { 0, -1 },
>   	};
> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> index 802b143de83d..3019e3ea5f09 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -224,6 +224,8 @@ struct cxl_dev_state *_devm_cxl_dev_state_create(struct device *dev,
>   						      sizeof(drv_struct), mbox);	\
>   	})
>   
> +struct cxl_memdev *devm_cxl_register_mem(struct cxl_dev_state *cxlds);
> +int devm_cxl_attach_mem_region(struct cxl_memdev *cxlmd, struct range *hpa_range);
>   struct cxl_memdev *devm_cxl_probe_mem(struct cxl_dev_state *cxlds,
>   				      struct range *range);
>