| 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: <3563734.XAFRqVoOGU@fdefranc-mobl3>
Date: Tue, 15 Jul 2025 00:09:48 +0200
From: "Fabio M. De Francesco" <fabio.m.de.francesco@...ux.intel.com>
To: linux-cxl@...r.kernel.org, Dan Williams <dan.j.williams@...el.com>
Cc: linux-kernel@...r.kernel.org, Davidlohr Bueso <dave@...olabs.net>,
Jonathan Cameron <jonathan.cameron@...wei.com>,
Dave Jiang <dave.jiang@...el.com>,
Alison Schofield <alison.schofield@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>,
"Peter Zijlstra (Intel)" <peterz@...radead.org>
Subject:
Re: [PATCH v3 7/8] cxl/region: Consolidate cxl_decoder_kill_region() and
cxl_region_detach()
On Saturday, July 12, 2025 1:49:31 AM Central European Summer Time Dan Williams wrote:
> Both detach_target() and cxld_unregister() want to tear down a cxl_region
> when an endpoint decoder is either detached or destroyed.
>
> When a region is to be destroyed cxl_region_detach() releases
> cxl_region_rwsem unbinds the cxl_region driver and re-acquires the rwsem.
>
> This "reverse" locking pattern is difficult to reason about, not amenable
> to scope-based cleanup, and the minor differences in the calling context of
> detach_target() and cxld_unregister() currently results in the
> cxl_decoder_kill_region() wrapper.
>
> Introduce cxl_decoder_detach() to wrap a core __cxl_decoder_detach() that
> serves both cases. I.e. either detaching a known position in a region
> (interruptible), or detaching an endpoint decoder if it is found to be a
> member of a region (uninterruptible).
>
> Cc: Davidlohr Bueso <dave@...olabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@...wei.com>
> Cc: Dave Jiang <dave.jiang@...el.com>
> Cc: Alison Schofield <alison.schofield@...el.com>
> Cc: Vishal Verma <vishal.l.verma@...el.com>
> Cc: Ira Weiny <ira.weiny@...el.com>
> Acked-by: "Peter Zijlstra (Intel)" <peterz@...radead.org>
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
Reviewed-by: Fabio M. De Francesco <fabio.m.de.francesco@...ux.intel.com>
> ---
> drivers/cxl/core/core.h | 15 +++++-
> drivers/cxl/core/port.c | 9 ++--
> drivers/cxl/core/region.c | 103 ++++++++++++++++++++++----------------
> 3 files changed, 75 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 29b61828a847..2be37084409f 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -12,6 +12,11 @@ extern const struct device_type cxl_pmu_type;
>
> extern struct attribute_group cxl_base_attribute_group;
>
> +enum cxl_detach_mode {
> + DETACH_ONLY,
> + DETACH_INVALIDATE,
> +};
> +
> #ifdef CONFIG_CXL_REGION
> extern struct device_attribute dev_attr_create_pmem_region;
> extern struct device_attribute dev_attr_create_ram_region;
> @@ -20,7 +25,11 @@ extern struct device_attribute dev_attr_region;
> extern const struct device_type cxl_pmem_region_type;
> extern const struct device_type cxl_dax_region_type;
> extern const struct device_type cxl_region_type;
> -void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled);
> +
> +int cxl_decoder_detach(struct cxl_region *cxlr,
> + struct cxl_endpoint_decoder *cxled, int pos,
> + enum cxl_detach_mode mode);
> +
> #define CXL_REGION_ATTR(x) (&dev_attr_##x.attr)
> #define CXL_REGION_TYPE(x) (&cxl_region_type)
> #define SET_CXL_REGION_ATTR(x) (&dev_attr_##x.attr),
> @@ -48,7 +57,9 @@ static inline int cxl_get_poison_by_endpoint(struct cxl_port *port)
> {
> return 0;
> }
> -static inline void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled)
> +static inline int cxl_decoder_detach(struct cxl_region *cxlr,
> + struct cxl_endpoint_decoder *cxled,
> + int pos, enum cxl_detach_mode mode)
> {
> }
> static inline int cxl_region_init(void)
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index eb46c6764d20..087a20a9ee1c 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -2001,12 +2001,9 @@ EXPORT_SYMBOL_NS_GPL(cxl_decoder_add, "CXL");
>
> static void cxld_unregister(void *dev)
> {
> - struct cxl_endpoint_decoder *cxled;
> -
> - if (is_endpoint_decoder(dev)) {
> - cxled = to_cxl_endpoint_decoder(dev);
> - cxl_decoder_kill_region(cxled);
> - }
> + if (is_endpoint_decoder(dev))
> + cxl_decoder_detach(NULL, to_cxl_endpoint_decoder(dev), -1,
> + DETACH_INVALIDATE);
>
> device_unregister(dev);
> }
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 2a97fa9a394f..4314aaed8ad8 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2135,27 +2135,43 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> return 0;
> }
>
> -static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
> +static struct cxl_region *
> +__cxl_decoder_detach(struct cxl_region *cxlr,
> + struct cxl_endpoint_decoder *cxled, int pos,
> + enum cxl_detach_mode mode)
> {
> - struct cxl_port *iter, *ep_port = cxled_to_port(cxled);
> - struct cxl_region *cxlr = cxled->cxld.region;
> struct cxl_region_params *p;
> - int rc = 0;
>
> lockdep_assert_held_write(&cxl_region_rwsem);
>
> - if (!cxlr)
> - return 0;
> + if (!cxled) {
> + p = &cxlr->params;
>
> - p = &cxlr->params;
> - get_device(&cxlr->dev);
> + if (pos >= p->interleave_ways) {
> + dev_dbg(&cxlr->dev, "position %d out of range %d\n",
> + pos, p->interleave_ways);
> + return ERR_PTR(-ENXIO);
> + }
> +
> + if (!p->targets[pos])
> + return NULL;
> + cxled = p->targets[pos];
> + } else {
> + cxlr = cxled->cxld.region;
> + if (!cxlr)
> + return NULL;
> + p = &cxlr->params;
> + }
> +
> + if (mode == DETACH_INVALIDATE)
> + cxled->part = -1;
>
> if (p->state > CXL_CONFIG_ACTIVE) {
> cxl_region_decode_reset(cxlr, p->interleave_ways);
> p->state = CXL_CONFIG_ACTIVE;
> }
>
> - for (iter = ep_port; !is_cxl_root(iter);
> + for (struct cxl_port *iter = cxled_to_port(cxled); !is_cxl_root(iter);
> iter = to_cxl_port(iter->dev.parent))
> cxl_port_detach_region(iter, cxlr, cxled);
>
> @@ -2166,7 +2182,7 @@ static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
> dev_WARN_ONCE(&cxlr->dev, 1, "expected %s:%s at position %d\n",
> dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> cxled->pos);
> - goto out;
> + return NULL;
> }
>
> if (p->state == CXL_CONFIG_ACTIVE) {
> @@ -2180,21 +2196,42 @@ static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
> .end = -1,
> };
>
> - /* notify the region driver that one of its targets has departed */
> - up_write(&cxl_region_rwsem);
> - device_release_driver(&cxlr->dev);
> - down_write(&cxl_region_rwsem);
> -out:
> - put_device(&cxlr->dev);
> - return rc;
> + get_device(&cxlr->dev);
> + return cxlr;
> }
>
> -void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled)
> +/*
> + * Cleanup a decoder's interest in a region. There are 2 cases to
> + * handle, removing an unknown @cxled from a known position in a region
> + * (detach_target()) or removing a known @cxled from an unknown @cxlr
> + * (cxld_unregister())
> + *
> + * When the detachment finds a region release the region driver.
> + */
> +int cxl_decoder_detach(struct cxl_region *cxlr,
> + struct cxl_endpoint_decoder *cxled, int pos,
> + enum cxl_detach_mode mode)
> {
> - down_write(&cxl_region_rwsem);
> - cxled->part = -1;
> - cxl_region_detach(cxled);
> + struct cxl_region *detach;
> +
> + /* when the decoder is being destroyed lock unconditionally */
> + if (mode == DETACH_INVALIDATE)
> + down_write(&cxl_region_rwsem);
> + else {
> + int rc = down_write_killable(&cxl_region_rwsem);
> +
> + if (rc)
> + return rc;
> + }
> +
> + detach = __cxl_decoder_detach(cxlr, cxled, pos, mode);
> up_write(&cxl_region_rwsem);
> +
> + if (detach) {
> + device_release_driver(&detach->dev);
> + put_device(&detach->dev);
> + }
> + return 0;
> }
>
> static int attach_target(struct cxl_region *cxlr,
> @@ -2225,29 +2262,7 @@ static int attach_target(struct cxl_region *cxlr,
>
> static int detach_target(struct cxl_region *cxlr, int pos)
> {
> - struct cxl_region_params *p = &cxlr->params;
> - int rc;
> -
> - rc = down_write_killable(&cxl_region_rwsem);
> - if (rc)
> - return rc;
> -
> - if (pos >= p->interleave_ways) {
> - dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
> - p->interleave_ways);
> - rc = -ENXIO;
> - goto out;
> - }
> -
> - if (!p->targets[pos]) {
> - rc = 0;
> - goto out;
> - }
> -
> - rc = cxl_region_detach(p->targets[pos]);
> -out:
> - up_write(&cxl_region_rwsem);
> - return rc;
> + return cxl_decoder_detach(cxlr, NULL, pos, DETACH_ONLY);
> }
>
> static size_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos,
> --
> 2.50.0
>
>
>
Powered by blists - more mailing lists