| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1b9d3e1c-7b8a-4c9d-8788-4f901f985079@amd.com>
Date: Mon, 12 Jan 2026 15:10:47 -0600
From: "Cheatham, Benjamin" <benjamin.cheatham@....com>
To: Gregory Price <gourry@...rry.net>, <linux-cxl@...r.kernel.org>
CC: <linux-kernel@...r.kernel.org>, <kernel-team@...a.com>,
<dave@...olabs.net>, <jonathan.cameron@...wei.com>, <dave.jiang@...el.com>,
<alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
<ira.weiny@...el.com>, <dan.j.williams@...el.com>
Subject: Re: [PATCH 3/6] cxl/core/region: move pmem memctrl logic into
memctrl/pmem_region
On 1/12/2026 10:35 AM, Gregory Price wrote:
> Move the pmem_region logic from region.c into memctrl/pmem_region.c.
> Restrict the valid controllers for pmem to the pmem controller.
> Simplify the controller selection logic in region probe.
>
> Cc:
May want to forward this to whoever this Cc tag was meant for :).
One nit below, otherwise this looks good to me:
Reviewed-by: Ben Cheatham <benjamin.cheatham@....com>
> Signed-off-by: Gregory Price <gourry@...rry.net>
> ---
> drivers/cxl/core/core.h | 1 +
> drivers/cxl/core/memctrl/Makefile | 1 +
> drivers/cxl/core/memctrl/memctrl.c | 2 +
> drivers/cxl/core/memctrl/pmem_region.c | 191 +++++++++++++++++++++
> drivers/cxl/core/region.c | 221 +++----------------------
> drivers/cxl/cxl.h | 2 +
> 6 files changed, 217 insertions(+), 201 deletions(-)
> create mode 100644 drivers/cxl/core/memctrl/pmem_region.c
>
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 18cb84950500..59175890a6ac 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -46,6 +46,7 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> u64 dpa);
> int cxl_enable_memctrl(struct cxl_region *cxlr);
> int devm_cxl_add_dax_region(struct cxl_region *cxlr);
> +int devm_cxl_add_pmem_region(struct cxl_region *cxlr);
>
> #else
> static inline u64 cxl_dpa_to_hpa(struct cxl_region *cxlr,
> diff --git a/drivers/cxl/core/memctrl/Makefile b/drivers/cxl/core/memctrl/Makefile
> index 1c52c7d75570..efffc8ba2c0b 100644
> --- a/drivers/cxl/core/memctrl/Makefile
> +++ b/drivers/cxl/core/memctrl/Makefile
> @@ -3,3 +3,4 @@
> cxl_core-$(CONFIG_CXL_REGION) += memctrl/memctrl.o
> cxl_core-$(CONFIG_CXL_REGION) += memctrl/dax_region.o
> cxl_core-$(CONFIG_CXL_REGION) += memctrl/sysram_region.o
> +cxl_core-$(CONFIG_CXL_REGION) += memctrl/pmem_region.o
> diff --git a/drivers/cxl/core/memctrl/memctrl.c b/drivers/cxl/core/memctrl/memctrl.c
> index 40ffb59353bb..1b661465bdeb 100644
> --- a/drivers/cxl/core/memctrl/memctrl.c
> +++ b/drivers/cxl/core/memctrl/memctrl.c
> @@ -36,6 +36,8 @@ int cxl_enable_memctrl(struct cxl_region *cxlr)
> return devm_cxl_add_dax_region(cxlr);
> case CXL_MEMCTRL_SYSRAM:
> return devm_cxl_add_sysram_region(cxlr);
> + case CXL_MEMCTRL_PMEM:
> + return devm_cxl_add_pmem_region(cxlr);
> default:
> return -EINVAL;
> }
> diff --git a/drivers/cxl/core/memctrl/pmem_region.c b/drivers/cxl/core/memctrl/pmem_region.c
> new file mode 100644
> index 000000000000..57668dd82d71
> --- /dev/null
> +++ b/drivers/cxl/core/memctrl/pmem_region.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <cxlmem.h>
> +#include <cxl.h>
> +#include "../core.h"
> +
> +static void cxl_pmem_region_release(struct device *dev)
> +{
> + struct cxl_pmem_region *cxlr_pmem = to_cxl_pmem_region(dev);
> + int i;
> +
> + for (i = 0; i < cxlr_pmem->nr_mappings; i++) {
> + struct cxl_memdev *cxlmd = cxlr_pmem->mapping[i].cxlmd;
> +
> + put_device(&cxlmd->dev);
> + }
> +
> + kfree(cxlr_pmem);
> +}
> +
> +static const struct attribute_group *cxl_pmem_region_attribute_groups[] = {
> + &cxl_base_attribute_group,
> + NULL,
> +};
> +
> +const struct device_type cxl_pmem_region_type = {
> + .name = "cxl_pmem_region",
> + .release = cxl_pmem_region_release,
> + .groups = cxl_pmem_region_attribute_groups,
> +};
> +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");
Missing blank line above.
> +static struct lock_class_key cxl_pmem_region_key;
> +
> +static int cxl_pmem_region_alloc(struct cxl_region *cxlr)
> +{
> + struct cxl_region_params *p = &cxlr->params;
> + struct cxl_nvdimm_bridge *cxl_nvb;
> + struct device *dev;
> + int i;
> +
> + guard(rwsem_read)(&cxl_rwsem.region);
> + if (p->state != CXL_CONFIG_COMMIT)
> + return -ENXIO;
> +
> + struct cxl_pmem_region *cxlr_pmem __free(kfree) =
> + kzalloc(struct_size(cxlr_pmem, mapping, p->nr_targets), GFP_KERNEL);
> + if (!cxlr_pmem)
> + return -ENOMEM;
> +
> + cxlr_pmem->hpa_range.start = p->res->start;
> + cxlr_pmem->hpa_range.end = p->res->end;
> +
> + /* Snapshot the region configuration underneath the cxl_rwsem.region */
> + cxlr_pmem->nr_mappings = p->nr_targets;
> + for (i = 0; i < p->nr_targets; i++) {
> + struct cxl_endpoint_decoder *cxled = p->targets[i];
> + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> + struct cxl_pmem_region_mapping *m = &cxlr_pmem->mapping[i];
> +
> + /*
> + * Regions never span CXL root devices, so by definition the
> + * bridge for one device is the same for all.
> + */
> + if (i == 0) {
> + cxl_nvb = cxl_find_nvdimm_bridge(cxlmd->endpoint);
> + if (!cxl_nvb)
> + return -ENODEV;
> + cxlr->cxl_nvb = cxl_nvb;
> + }
> + m->cxlmd = cxlmd;
> + get_device(&cxlmd->dev);
> + m->start = cxled->dpa_res->start;
> + m->size = resource_size(cxled->dpa_res);
> + m->position = i;
> + }
> +
> + dev = &cxlr_pmem->dev;
> + device_initialize(dev);
> + lockdep_set_class(&dev->mutex, &cxl_pmem_region_key);
> + device_set_pm_not_required(dev);
> + dev->parent = &cxlr->dev;
> + dev->bus = &cxl_bus_type;
> + dev->type = &cxl_pmem_region_type;
> + cxlr_pmem->cxlr = cxlr;
> + cxlr->cxlr_pmem = no_free_ptr(cxlr_pmem);
> +
> + return 0;
> +}
> +
> +static void cxlr_pmem_unregister(void *_cxlr_pmem)
> +{
> + struct cxl_pmem_region *cxlr_pmem = _cxlr_pmem;
> + struct cxl_region *cxlr = cxlr_pmem->cxlr;
> + struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb;
> +
> + /*
> + * Either the bridge is in ->remove() context under the device_lock(),
> + * or cxlr_release_nvdimm() is cancelling the bridge's release action
> + * for @cxlr_pmem and doing it itself (while manually holding the bridge
> + * lock).
> + */
> + device_lock_assert(&cxl_nvb->dev);
> + cxlr->cxlr_pmem = NULL;
> + cxlr_pmem->cxlr = NULL;
> + device_unregister(&cxlr_pmem->dev);
> +}
> +
> +static void cxlr_release_nvdimm(void *_cxlr)
> +{
> + struct cxl_region *cxlr = _cxlr;
> + struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb;
> +
> + scoped_guard(device, &cxl_nvb->dev) {
> + if (cxlr->cxlr_pmem)
> + devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister,
> + cxlr->cxlr_pmem);
> + }
> + cxlr->cxl_nvb = NULL;
> + put_device(&cxl_nvb->dev);
> +}
> +
> +/**
> + * 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.
> + */
> +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;
> + }
> +
> + 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;
> +}
> +
> +
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index eeab091f043a..85c20a09246d 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -642,6 +642,9 @@ static ssize_t ctrl_show(struct device *dev, struct device_attribute *attr,
> case CXL_MEMCTRL_SYSRAM:
> desc = "sysram";
> break;
> + case CXL_MEMCTRL_PMEM:
> + desc = "pmem";
> + break;
> default:
> desc = "";
> break;
> @@ -661,6 +664,10 @@ static ssize_t ctrl_store(struct device *dev, struct device_attribute *attr,
> if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
> return rc;
>
> + /* PMEM only has one controller - the pmem controller */
> + if (cxlr->mode == CXL_PARTMODE_PMEM)
> + return -EBUSY;
> +
> if (p->state >= CXL_CONFIG_COMMIT)
> return -EBUSY;
>
> @@ -2648,7 +2655,11 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
> return cxlr;
> cxlr->mode = mode;
> cxlr->type = type;
> - cxlr->memctrl = CXL_MEMCTRL_NONE;
> +
> + if (mode == CXL_PARTMODE_PMEM)
> + cxlr->memctrl = CXL_MEMCTRL_PMEM;
> + else
> + cxlr->memctrl = CXL_MEMCTRL_NONE;
>
> dev = &cxlr->dev;
> rc = dev_set_name(dev, "region%d", id);
> @@ -2797,46 +2808,6 @@ static ssize_t delete_region_store(struct device *dev,
> }
> DEVICE_ATTR_WO(delete_region);
>
> -static void cxl_pmem_region_release(struct device *dev)
> -{
> - struct cxl_pmem_region *cxlr_pmem = to_cxl_pmem_region(dev);
> - int i;
> -
> - for (i = 0; i < cxlr_pmem->nr_mappings; i++) {
> - struct cxl_memdev *cxlmd = cxlr_pmem->mapping[i].cxlmd;
> -
> - put_device(&cxlmd->dev);
> - }
> -
> - kfree(cxlr_pmem);
> -}
> -
> -static const struct attribute_group *cxl_pmem_region_attribute_groups[] = {
> - &cxl_base_attribute_group,
> - NULL,
> -};
> -
> -const struct device_type cxl_pmem_region_type = {
> - .name = "cxl_pmem_region",
> - .release = cxl_pmem_region_release,
> - .groups = cxl_pmem_region_attribute_groups,
> -};
> -
> -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");
> -
> struct cxl_poison_context {
> struct cxl_port *port;
> int part;
> @@ -3268,64 +3239,6 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> return -ENXIO;
> }
>
> -static struct lock_class_key cxl_pmem_region_key;
> -
> -static int cxl_pmem_region_alloc(struct cxl_region *cxlr)
> -{
> - struct cxl_region_params *p = &cxlr->params;
> - struct cxl_nvdimm_bridge *cxl_nvb;
> - struct device *dev;
> - int i;
> -
> - guard(rwsem_read)(&cxl_rwsem.region);
> - if (p->state != CXL_CONFIG_COMMIT)
> - return -ENXIO;
> -
> - struct cxl_pmem_region *cxlr_pmem __free(kfree) =
> - kzalloc(struct_size(cxlr_pmem, mapping, p->nr_targets), GFP_KERNEL);
> - if (!cxlr_pmem)
> - return -ENOMEM;
> -
> - cxlr_pmem->hpa_range.start = p->res->start;
> - cxlr_pmem->hpa_range.end = p->res->end;
> -
> - /* Snapshot the region configuration underneath the cxl_rwsem.region */
> - cxlr_pmem->nr_mappings = p->nr_targets;
> - for (i = 0; i < p->nr_targets; i++) {
> - struct cxl_endpoint_decoder *cxled = p->targets[i];
> - struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> - struct cxl_pmem_region_mapping *m = &cxlr_pmem->mapping[i];
> -
> - /*
> - * Regions never span CXL root devices, so by definition the
> - * bridge for one device is the same for all.
> - */
> - if (i == 0) {
> - cxl_nvb = cxl_find_nvdimm_bridge(cxlmd->endpoint);
> - if (!cxl_nvb)
> - return -ENODEV;
> - cxlr->cxl_nvb = cxl_nvb;
> - }
> - m->cxlmd = cxlmd;
> - get_device(&cxlmd->dev);
> - m->start = cxled->dpa_res->start;
> - m->size = resource_size(cxled->dpa_res);
> - m->position = i;
> - }
> -
> - dev = &cxlr_pmem->dev;
> - device_initialize(dev);
> - lockdep_set_class(&dev->mutex, &cxl_pmem_region_key);
> - device_set_pm_not_required(dev);
> - dev->parent = &cxlr->dev;
> - dev->bus = &cxl_bus_type;
> - dev->type = &cxl_pmem_region_type;
> - cxlr_pmem->cxlr = cxlr;
> - cxlr->cxlr_pmem = no_free_ptr(cxlr_pmem);
> -
> - return 0;
> -}
> -
> static void cxl_dax_region_release(struct device *dev)
> {
> struct cxl_dax_region *cxlr_dax = to_cxl_dax_region(dev);
> @@ -3358,92 +3271,6 @@ struct cxl_dax_region *to_cxl_dax_region(struct device *dev)
> }
> EXPORT_SYMBOL_NS_GPL(to_cxl_dax_region, "CXL");
>
> -static void cxlr_pmem_unregister(void *_cxlr_pmem)
> -{
> - struct cxl_pmem_region *cxlr_pmem = _cxlr_pmem;
> - struct cxl_region *cxlr = cxlr_pmem->cxlr;
> - struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb;
> -
> - /*
> - * Either the bridge is in ->remove() context under the device_lock(),
> - * or cxlr_release_nvdimm() is cancelling the bridge's release action
> - * for @cxlr_pmem and doing it itself (while manually holding the bridge
> - * lock).
> - */
> - device_lock_assert(&cxl_nvb->dev);
> - cxlr->cxlr_pmem = NULL;
> - cxlr_pmem->cxlr = NULL;
> - device_unregister(&cxlr_pmem->dev);
> -}
> -
> -static void cxlr_release_nvdimm(void *_cxlr)
> -{
> - struct cxl_region *cxlr = _cxlr;
> - struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb;
> -
> - scoped_guard(device, &cxl_nvb->dev) {
> - if (cxlr->cxlr_pmem)
> - devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister,
> - cxlr->cxlr_pmem);
> - }
> - cxlr->cxl_nvb = NULL;
> - put_device(&cxl_nvb->dev);
> -}
> -
> -/**
> - * 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.
> - */
> -static 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;
> - }
> -
> - 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;
> -}
> -
> static int match_decoder_by_range(struct device *dev, const void *data)
> {
> const struct range *r1, *r2 = data;
> @@ -3929,26 +3756,18 @@ static int cxl_region_probe(struct device *dev)
> return rc;
> }
>
> - switch (cxlr->mode) {
> - case CXL_PARTMODE_PMEM:
> - rc = devm_cxl_region_edac_register(cxlr);
> - if (rc)
> - dev_dbg(&cxlr->dev, "CXL EDAC registration for region_id=%d failed\n",
> - cxlr->id);
> -
> - return devm_cxl_add_pmem_region(cxlr);
> - case CXL_PARTMODE_RAM:
> - rc = devm_cxl_region_edac_register(cxlr);
> - if (rc)
> - dev_dbg(&cxlr->dev, "CXL EDAC registration for region_id=%d failed\n",
> - cxlr->id);
> -
> - return cxl_enable_memctrl(cxlr);
> - default:
> + if (cxlr->mode > CXL_PARTMODE_PMEM) {
> dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",
> cxlr->mode);
> return -ENXIO;
> }
> +
> + rc = devm_cxl_region_edac_register(cxlr);
> + if (rc)
> + dev_dbg(&cxlr->dev, "CXL EDAC registration for region_id=%d failed\n",
> + cxlr->id);
> +
> + return cxl_enable_memctrl(cxlr);
> }
>
> static struct cxl_driver cxl_region_driver = {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index bb4f877b4e8f..c69d27a2e97d 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -509,12 +509,14 @@ enum cxl_partition_mode {
> * Auto - either BIOS-configured as SysRAM, or default to DAX
> * DAX - creates a dax_region controller for the cxl_region
> * SYSRAM - hotplugs the region directly as System RAM
> + * PMEM - persistent memory controller (nvdimm)
> */
> enum cxl_memctrl_mode {
> CXL_MEMCTRL_NONE,
> CXL_MEMCTRL_AUTO,
> CXL_MEMCTRL_DAX,
> CXL_MEMCTRL_SYSRAM,
> + CXL_MEMCTRL_PMEM,
> };
>
> /*
Powered by blists - more mailing lists