From d6488fee66472b468ed88d265b14aa3f04dc3bdf Mon Sep 17 00:00:00 2001 From: Huang Ying Date: Fri, 8 Dec 2023 11:06:36 +0800 Subject: cxl/port: Fix decoder initialization when nr_targets > interleave_ways The decoder_populate_targets() helper walks all of the targets in a port and makes sure they can be looked up in @target_map. Where @target_map is a lookup table from target position to target id (corresponding to a cxl_dport instance). However @target_map is only responsible for conveying the active dport instances as indicated by interleave_ways. When nr_targets > interleave_ways it results in decoder_populate_targets() walking off the end of the valid entries in @target_map. Given target_map is initialized to 0 it results in the dport lookup failing if position 0 is not mapped to a dport with an id of 0: cxl_port port3: Failed to populate active decoder targets cxl_port port3: Failed to add decoder cxl_port port3: Failed to add decoder3.0 cxl_bus_probe: cxl_port port3: probe: -6 This bug also highlights that when the decoder's ->targets[] array is written in cxl_port_setup_targets() it is missing a hold of the targets_lock to synchronize against sysfs readers of the target list. A fix for that is saved for a later patch. Fixes: a5c258021689 ("cxl/bus: Populate the target list at decoder create") Cc: Signed-off-by: Huang, Ying [djbw: rewrite the changelog, find the Fixes: tag] Co-developed-by: Dan Williams Reviewed-by: Alison Schofield Reviewed-by: Dave Jiang Signed-off-by: Dan Williams --- drivers/cxl/core/port.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/cxl') diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index b7c93bb18f6e..57495cdc181f 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1644,7 +1644,7 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd, return -EINVAL; write_seqlock(&cxlsd->target_lock); - for (i = 0; i < cxlsd->nr_targets; i++) { + for (i = 0; i < cxlsd->cxld.interleave_ways; i++) { struct cxl_dport *dport = find_dport(port, target_map[i]); if (!dport) { -- cgit v1.2.3 From 5459e186a5c9f412334321cff58d70dcb0e48a04 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Thu, 21 Dec 2023 21:05:01 -0800 Subject: cxl/port: Fix missing target list lock cxl_port_setup_targets() modifies the ->targets[] array of a switch decoder. target_list_show() expects to be able to emit a coherent snapshot of that array by "holding" ->target_lock for read. The target_lock is held for write during initialization of the ->targets[] array, but it is not held for write during cxl_port_setup_targets(). The ->target_lock() predates the introduction of @cxl_region_rwsem. That semaphore protects changes to host-physical-address (HPA) decode which is precisely what writes to a switch decoder's target list affects. Replace ->target_lock with @cxl_region_rwsem. Now the side-effect of snapshotting a unstable view of a decoder's target list is likely benign so the Fixes: tag is presumptive. Fixes: 27b3f8d13830 ("cxl/region: Program target lists") Reviewed-by: Alison Schofield Reviewed-by: Dave Jiang Signed-off-by: Dan Williams --- drivers/cxl/core/port.c | 22 +++++++--------------- drivers/cxl/cxl.h | 2 -- 2 files changed, 7 insertions(+), 17 deletions(-) (limited to 'drivers/cxl') diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 57495cdc181f..6b386771cc25 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -172,14 +172,10 @@ static ssize_t target_list_show(struct device *dev, { struct cxl_switch_decoder *cxlsd = to_cxl_switch_decoder(dev); ssize_t offset; - unsigned int seq; int rc; - do { - seq = read_seqbegin(&cxlsd->target_lock); - rc = emit_target_list(cxlsd, buf); - } while (read_seqretry(&cxlsd->target_lock, seq)); - + guard(rwsem_read)(&cxl_region_rwsem); + rc = emit_target_list(cxlsd, buf); if (rc < 0) return rc; offset = rc; @@ -1633,7 +1629,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL); static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd, struct cxl_port *port, int *target_map) { - int i, rc = 0; + int i; if (!target_map) return 0; @@ -1643,19 +1639,16 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd, if (xa_empty(&port->dports)) return -EINVAL; - write_seqlock(&cxlsd->target_lock); + guard(rwsem_write)(&cxl_region_rwsem); for (i = 0; i < cxlsd->cxld.interleave_ways; i++) { struct cxl_dport *dport = find_dport(port, target_map[i]); - if (!dport) { - rc = -ENXIO; - break; - } + if (!dport) + return -ENXIO; cxlsd->target[i] = dport; } - write_sequnlock(&cxlsd->target_lock); - return rc; + return 0; } struct cxl_dport *cxl_hb_modulo(struct cxl_root_decoder *cxlrd, int pos) @@ -1725,7 +1718,6 @@ static int cxl_switch_decoder_init(struct cxl_port *port, return -EINVAL; cxlsd->nr_targets = nr_targets; - seqlock_init(&cxlsd->target_lock); return cxl_decoder_init(port, &cxlsd->cxld); } diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 687043ece101..62fa96f8567e 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -412,7 +412,6 @@ struct cxl_endpoint_decoder { /** * struct cxl_switch_decoder - Switch specific CXL HDM Decoder * @cxld: base cxl_decoder object - * @target_lock: coordinate coherent reads of the target list * @nr_targets: number of elements in @target * @target: active ordered target list in current decoder configuration * @@ -424,7 +423,6 @@ struct cxl_endpoint_decoder { */ struct cxl_switch_decoder { struct cxl_decoder cxld; - seqlock_t target_lock; int nr_targets; struct cxl_dport *target[]; }; -- cgit v1.2.3