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

[Richard Cheng <icheng@nvidia.com>]

On Wed, Oct 14, 2026 at 06:17:44PM +0800, Lucero Palau, Alejandro wrote:
> 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.
> 
>

Hi Alejandro,

I see, I'll rephrase them to be more precise in v3.
 
> > , 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.
> 
> 

Sure thing.


> > -	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?
> 
> 

I do think so, the helper here can install an EP-managed region-unregistration
operation, so attachment has a managed lifetime side effect in addition to
just returning the HPA range. That's why I used the devm_ prefix.

Is that acceptable ?

Best regards,
Richard Cheng.


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