6.6-stable review patch. If anyone has any objections, please let me know. ------------------ From: Dan Williams [ Upstream commit 4dd86ca99ffcc413cbf79063fd9956ef54e0ca91 ] Sungwoo noticed that the sysfs trigger to delete a region may try to delete a region multiple times. It also has no exclusion relative to the kernel releasing the region via CXL root device teardown. Instead of installing new cxl root devres actions per region, use the existing root decoder unregistration event to remove all remaining regions. An xarray of regions replaces a devres list of regions. This handles 3 separate issues with the old approach: 1/ sysfs users racing to delete the same region: no longer possible now that the regions_lock is held over the lookup and deletion. 2/ multiple actions triggering deletion of the same region: solved by erasing regions while holding @regions_lock, and only proceeding on successful erasure. 3/ userspace racing devres_release_all() to trigger the devres not found warning: solved by sysfs unregistration not requiring a release action Fixes: 779dd20cfb56 ("cxl/region: Add region creation support") Reported-by: Sungwoo Kim Closes: http://lore.kernel.org/20260427032010.916681-2-iam@sung-woo.kim Signed-off-by: Dan Williams Reviewed-by: Alejandro Lucero Tested-by: ALejandro Lucero Reviewed-by: Dave Jiang Link: https://patch.msgid.link/20260519210158.1499795-3-djbw@kernel.org Signed-off-by: Dave Jiang Signed-off-by: Sasha Levin --- drivers/cxl/core/core.h | 2 ++ drivers/cxl/core/port.c | 5 +++ drivers/cxl/core/region.c | 65 ++++++++++++++++++++++----------------- drivers/cxl/cxl.h | 4 +++ 4 files changed, 48 insertions(+), 28 deletions(-) diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h index 6444cc827c9ce..3f05fa7c65a75 100644 --- a/drivers/cxl/core/core.h +++ b/drivers/cxl/core/core.h @@ -28,6 +28,7 @@ int cxl_region_init(void); void cxl_region_exit(void); int cxl_get_poison_by_endpoint(struct cxl_port *port); struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa); +void kill_regions(struct cxl_root_decoder *cxlrd); #else static inline @@ -42,6 +43,7 @@ static inline int cxl_get_poison_by_endpoint(struct cxl_port *port) static inline void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled) { } +static inline void kill_regions(struct cxl_root_decoder *cxlrd) { }; static inline int cxl_region_init(void) { return 0; diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 7746e2691802c..2589354b0ca44 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -433,6 +433,8 @@ static void cxl_root_decoder_release(struct device *dev) if (atomic_read(&cxlrd->region_id) >= 0) memregion_free(atomic_read(&cxlrd->region_id)); + mutex_destroy(&cxlrd->regions_lock); + xa_destroy(&cxlrd->regions); __cxl_decoder_release(&cxlrd->cxlsd.cxld); kfree(cxlrd); } @@ -1708,6 +1710,7 @@ struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port, cxlrd->calc_hb = calc_hb; mutex_init(&cxlrd->regions_lock); + xa_init(&cxlrd->regions); cxld = &cxlsd->cxld; cxld->dev.type = &cxl_decoder_root_type; @@ -1894,6 +1897,8 @@ static void cxld_unregister(void *dev) cxled = to_cxl_endpoint_decoder(dev); cxl_decoder_kill_region(cxled); } + if (is_root_decoder(dev)) + kill_regions(to_cxl_root_decoder(dev)); device_unregister(dev); } diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 5fc2a6a50ae94..873ee24a34317 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -2193,13 +2193,14 @@ static struct cxl_region *to_cxl_region(struct device *dev) return container_of(dev, struct cxl_region, dev); } -static void unregister_region(void *dev) +static void unregister_region(struct cxl_region *cxlr) { - struct cxl_region *cxlr = to_cxl_region(dev); + struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent); struct cxl_region_params *p = &cxlr->params; int i; - device_del(dev); + xa_erase(&cxlrd->regions, cxlr->id); + device_del(&cxlr->dev); /* * Now that region sysfs is shutdown, the parameter block is now @@ -2210,7 +2211,7 @@ static void unregister_region(void *dev) detach_target(cxlr, i); cxl_region_iomem_release(cxlr); - put_device(dev); + put_device(&cxlr->dev); } static struct lock_class_key cxl_region_key; @@ -2243,6 +2244,19 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i return cxlr; } +/* unwind all remaining regions */ +void kill_regions(struct cxl_root_decoder *cxlrd) +{ + unsigned long index; + struct cxl_region *cxlr; + + guard(mutex)(&cxlrd->regions_lock); + /* no more region creation */ + cxlrd->dead = true; + xa_for_each(&cxlrd->regions, index, cxlr) + unregister_region(cxlr); +} + /** * devm_cxl_add_region - Adds a region to a decoder * @cxlrd: root decoder @@ -2281,9 +2295,11 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, if (rc) goto err; - rc = devm_add_action_or_reset(port->uport_dev, unregister_region, cxlr); - if (rc) + rc = xa_insert(&cxlrd->regions, cxlr->id, cxlr, GFP_KERNEL); + if (rc) { + unregister_region(cxlr); return ERR_PTR(rc); + } dev_dbg(port->uport_dev, "%s: created %s\n", dev_name(&cxlrd->cxlsd.cxld.dev), dev_name(dev)); @@ -2316,6 +2332,9 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, { int rc; + if (cxlrd->dead) + return ERR_PTR(-ENXIO); + switch (mode) { case CXL_DECODER_RAM: case CXL_DECODER_PMEM: @@ -2407,40 +2426,31 @@ static ssize_t region_show(struct device *dev, struct device_attribute *attr, } DEVICE_ATTR_RO(region); -static struct cxl_region * -cxl_find_region_by_name(struct cxl_root_decoder *cxlrd, const char *name) -{ - struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld; - struct device *region_dev; - - region_dev = device_find_child_by_name(&cxld->dev, name); - if (!region_dev) - return ERR_PTR(-ENODEV); - - return to_cxl_region(region_dev); -} - static ssize_t delete_region_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t len) { struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); - struct cxl_port *port = to_cxl_port(dev->parent); struct cxl_region *cxlr; - int rc; + int rc, id; rc = mutex_lock_interruptible(&cxlrd->regions_lock); if (rc) return rc; - cxlr = cxl_find_region_by_name(cxlrd, buf); - if (IS_ERR(cxlr)) { + rc = sscanf(buf, "region%d\n", &id); + if (rc != 1) { mutex_unlock(&cxlrd->regions_lock); - return PTR_ERR(cxlr); + return -EINVAL; } - devm_release_action(port->uport_dev, unregister_region, cxlr); - put_device(&cxlr->dev); + cxlr = xa_load(&cxlrd->regions, id); + if (!cxlr || !sysfs_streq(buf, dev_name(&cxlr->dev))) { + mutex_unlock(&cxlrd->regions_lock); + return -ENODEV; + } + + unregister_region(cxlr); mutex_unlock(&cxlrd->regions_lock); return len; @@ -2958,7 +2968,6 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, struct cxl_endpoint_decoder *cxled) { struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); - struct cxl_port *port = cxlrd_to_port(cxlrd); struct range *hpa = &cxled->cxld.hpa_range; struct cxl_region_params *p; struct cxl_region *cxlr; @@ -3033,7 +3042,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, err: up_write(&cxl_region_rwsem); - devm_release_action(port->uport_dev, unregister_region, cxlr); + unregister_region(cxlr); return ERR_PTR(rc); } diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 9dcc8a4d5128a..7a1b1721860d7 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -442,6 +442,8 @@ typedef struct cxl_dport *(*cxl_calc_hb_fn)(struct cxl_root_decoder *cxlrd, * @calc_hb: which host bridge covers the n'th position by granularity * @platform_data: platform specific configuration data * @regions_lock: sync region discovery, construction, and deletion + * @regions: regions to remove at root decoder destruct time + * @dead: root decoder dead to region creation * @cxlsd: base cxl switch decoder */ struct cxl_root_decoder { @@ -450,6 +452,8 @@ struct cxl_root_decoder { cxl_calc_hb_fn calc_hb; void *platform_data; struct mutex regions_lock; + struct xarray regions; + bool dead; struct cxl_switch_decoder cxlsd; }; -- 2.53.0