Re: [PATCH v4 1/3] cxl/core/region: move pmem region driver logic into region_pmem.c

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

On Sun, 22 Mar 2026 09:16:36 -0400
Gregory Price <gourry@gourry.net> wrote:

> core/region.c is overloaded with per-region control logic (pmem, dax,
> sysram, etc). Move the pmem region driver logic from region.c into
> region_pmem.c make it clear that this code only applies to pmem regions.
> 
> No functional changes.
> 
> Signed-off-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/cxl/core/Makefile      |   1 +
>  drivers/cxl/core/core.h        |   1 +
>  drivers/cxl/core/region.c      | 184 --------------------------------
>  drivers/cxl/core/region_pmem.c | 189 +++++++++++++++++++++++++++++++++
>  4 files changed, 191 insertions(+), 184 deletions(-)
>  create mode 100644 drivers/cxl/core/region_pmem.c
> 
> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> index a639a9499972..d1484a0e5eb4 100644
> --- a/drivers/cxl/core/Makefile
> +++ b/drivers/cxl/core/Makefile
> @@ -16,6 +16,7 @@ cxl_core-y += pmu.o
>  cxl_core-y += cdat.o
>  cxl_core-$(CONFIG_TRACING) += trace.o
>  cxl_core-$(CONFIG_CXL_REGION) += region.o
> +cxl_core-$(CONFIG_CXL_REGION) += region_pmem.o

cxl_core-$(CONFIG_CXL_REGION) += region.o region_pmem.o

For me having them on one line makes it more obvious it's
either neither or both. Cost is a bit more churn if you
decide later to split the CONFIG_* up.

>  cxl_core-$(CONFIG_CXL_MCE) += mce.o
>  cxl_core-$(CONFIG_CXL_FEATURES) += features.o
>  cxl_core-$(CONFIG_CXL_EDAC_MEM_FEATURES) += edac.o
Otherwise a couple of white space things. + a passing comment on the lifetime
management being more complex than I'd like.

Fix the white space and you can add
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> diff --git a/drivers/cxl/core/region_pmem.c b/drivers/cxl/core/region_pmem.c
> new file mode 100644
> index 000000000000..138c373a5700
> --- /dev/null
> +++ b/drivers/cxl/core/region_pmem.c

> +const struct device_type cxl_pmem_region_type = {
> +	.name = "cxl_pmem_region",
> +	.release = cxl_pmem_region_release,
> +	.groups = cxl_pmem_region_attribute_groups,
> +};

Blank line here. There is one in the original and I can't see why
we'd want to get rid of it.

> +bool is_cxl_pmem_region(struct device *dev)
> +{
> +	return dev->type == &cxl_pmem_region_type;
> +}
> +EXPORT_SYMBOL_NS_GPL(is_cxl_pmem_region, "CXL");
> +
> +struct cxl_pmem_region *to_cxl_pmem_region(struct device *dev)
> +{
> +	if (dev_WARN_ONCE(dev, !is_cxl_pmem_region(dev),
> +				"not a cxl_pmem_region device\n"))
> +		return NULL;
> +	return container_of(dev, struct cxl_pmem_region, dev);
> +}
> +EXPORT_SYMBOL_NS_GPL(to_cxl_pmem_region, "CXL");

Blank line here probably makes sense as the key has nothing to do
with the EXPORT_SYMBOL...


> +static struct lock_class_key cxl_pmem_region_key;
> +

> +
> +/**
> + * devm_cxl_add_pmem_region() - add a cxl_region-to-nd_region bridge
> + * @cxlr: parent CXL region for this pmem region bridge device
> + *
> + * Return: 0 on success negative error code on failure.

FWIW the lifetime management in here feels way to complex to me.  Not
a problem for this patch and I'm not immediately sure what we can do about it.

> + */
> +int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
> +{
> +	struct cxl_pmem_region *cxlr_pmem;
> +	struct cxl_nvdimm_bridge *cxl_nvb;
> +	struct device *dev;
> +	int rc;
> +
> +	rc = cxl_pmem_region_alloc(cxlr);
> +	if (rc)
> +		return rc;
> +	cxlr_pmem = cxlr->cxlr_pmem;
> +	cxl_nvb = cxlr->cxl_nvb;
> +
> +	dev = &cxlr_pmem->dev;
> +	rc = dev_set_name(dev, "pmem_region%d", cxlr->id);
> +	if (rc)
> +		goto err;
> +
> +	rc = device_add(dev);
> +	if (rc)
> +		goto err;
> +
> +	dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
> +			dev_name(dev));
> +
> +	scoped_guard(device, &cxl_nvb->dev) {
> +		if (cxl_nvb->dev.driver)
> +			rc = devm_add_action_or_reset(&cxl_nvb->dev,
> +					cxlr_pmem_unregister,
> +					cxlr_pmem);
> +		else
> +			rc = -ENXIO;
As an example. If we happen to take this path... Where is the device_add() undone?
> +	}
> +
> +	if (rc)
> +		goto err_bridge;
> +
> +	/* @cxlr carries a reference on @cxl_nvb until cxlr_release_nvdimm */
> +	return devm_add_action_or_reset(&cxlr->dev, cxlr_release_nvdimm, cxlr);
> +
> +err:
> +	put_device(dev);
> +err_bridge:
> +	put_device(&cxl_nvb->dev);
> +	cxlr->cxl_nvb = NULL;
> +	return rc;
> +}