Re: [PATCH v17 18/22] cxl: Allow region creation by type2 drivers

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 24 Jun 2025 15:13:51 +0100
<alejandro.lucero-palau@amd.com> wrote:

> From: Alejandro Lucero <alucerop@amd.com>
> 
> Creating a CXL region requires userspace intervention through the cxl
> sysfs files. Type2 support should allow accelerator drivers to create
> such cxl region from kernel code.
> 
> Adding that functionality and integrating it with current support for
> memory expanders.
> 
> Support an action by the type2 driver to be linked to the created region
> for unwinding the resources allocated properly.
> 
> Based on https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
One question in here for others (probably Dan). When does it makes sense to
manually request devm region cleanup and when should we let if flow out
as we are failing the CXL creation anyway and it's one of many things to
clean up if that happens.

> ---
>  drivers/cxl/core/region.c | 152 ++++++++++++++++++++++++++++++++++++--
>  drivers/cxl/port.c        |   5 +-
>  include/cxl/cxl.h         |   5 ++
>  3 files changed, 153 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 21cf8c11efe3..4ca5ade54ad9 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2319,6 +2319,12 @@ static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
>  	return rc;
>  }
>  
> +/**
> + * cxl_decoder_kill_region - detach a region from device
> + *
> + * @cxled: endpoint decoder to detach the region from.
> + *

Stray blank line.

> + */
>  void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled)
>  {
>  	down_write(&cxl_region_rwsem);
> @@ -2326,6 +2332,7 @@ void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled)
>  	cxl_region_detach(cxled);
>  	up_write(&cxl_region_rwsem);
>  }
> +EXPORT_SYMBOL_NS_GPL(cxl_decoder_kill_region, "CXL");

> +/**
> + * cxl_create_region - Establish a region given an endpoint decoder
> + * @cxlrd: root decoder to allocate HPA
> + * @cxled: endpoint decoder with reserved DPA capacity
> + * @ways: interleave ways required
> + *
> + * Returns a fully formed region in the commit state and attached to the
> + * cxl_region driver.
> + */
> +struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
> +				     struct cxl_endpoint_decoder **cxled,
> +				     int ways, void (*action)(void *),
> +				     void *data)
> +{
> +	struct cxl_region *cxlr;
> +
> +	scoped_guard(mutex, &cxlrd->range_lock) {
> +		cxlr = __construct_new_region(cxlrd, cxled, ways);
> +		if (IS_ERR(cxlr))
> +			return cxlr;
> +	}
> +
> +	if (device_attach(&cxlr->dev) <= 0) {
> +		dev_err(&cxlr->dev, "failed to create region\n");
> +		drop_region(cxlr);

I'm in two minds about this.  If we were to have wrapped the whole thing
up in a devres group and on failure (so carrying  on without cxl support)
we tidy that group up, then we'd not need to clean this up here.
However we do some local devm cleanup in construct_region today so maybe
keeping this local makes sense...  Dan, maybe you have a better view of
whether cleaning up here is sensible or not?

> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	if (action)
> +		devm_add_action_or_reset(&cxlr->dev, action, data);

This is a little odd looking (and can fail so should be error checkeD)
I'd push the devm registration to the caller.


> +
> +	return cxlr;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_create_region, "CXL");
> +
>  int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>  {
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);