| Message ID | 170322553909.110939.1669280651596703652.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | Accepted |
| Commit | cef295b57778d5795466c731e67408274e95a718 |
| Headers | show |
| Series | cxl/port: Fix missing target list lock | expand |
On 12/21/23 23:12, Dan Williams wrote: > 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 by holding ->target_lock. 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") > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/port.c | 22 +++++++--------------- > drivers/cxl/cxl.h | 2 -- > 2 files changed, 7 insertions(+), 17 deletions(-) > > 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[]; > }; > >
On Thu, Dec 21, 2023 at 10:12:19PM -0800, Dan Williams wrote: > 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 by holding ->target_lock. 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") > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
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[]; };
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 by holding ->target_lock. 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") Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/port.c | 22 +++++++--------------- drivers/cxl/cxl.h | 2 -- 2 files changed, 7 insertions(+), 17 deletions(-)