Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 13 Mar 2023 18:39:22 -0700 | From | Alison Schofield <> | Subject | Re: [PATCH v8 4/6] cxl/region: Provide region info to the cxl_poison trace event |
| |
On Mon, Mar 13, 2023 at 03:51:31PM -0700, Ira Weiny wrote: > alison.schofield@ wrote: > > From: Alison Schofield <alison.schofield@intel.com> > > > > User space may need to know which region, if any, maps the poison > > address(es) logged in a cxl_poison trace event. Since the mapping > > of DPAs (device physical addresses) to a region can change, the > > kernel must provide this information at the time the poison list > > is read. The event informs user space that at event <timestamp> > > this <region> mapped to this <DPA>, which is poisoned. > > > > The cxl_poison trace event is already wired up to log the region > > name and uuid if it receives param 'struct cxl_region'. > > > > In order to provide that cxl_region, add another method for gathering > > poison - by committed endpoint decoder mappings. This method is only > > available with CONFIG_CXL_REGION and is only used if a region actually > > maps the memdev where poison is being read. After the poison list is > > read for all the mapped resources, poison is read for the unmapped > > resources, and those events are logged without the region info. > > > > Mixed mode decoders are not currently supported in Linux. Add a debug > > message to the poison request path. That will serve as an alert that > > poison list retrieval needs to add support for mixed mode. > > > > The default method remains: read the poison by memdev resource. > > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > [...] > > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c > > index ea996057815e..5e65818d2171 100644 > > --- a/drivers/cxl/core/memdev.c > > +++ b/drivers/cxl/core/memdev.c > > @@ -139,14 +139,29 @@ static ssize_t trigger_poison_list_store(struct device *dev, > > const char *buf, size_t len) > > { > > struct cxl_memdev *cxlmd = to_cxl_memdev(dev); > > + struct cxl_port *port; > > bool trigger; > > int rc; > > > > if (kstrtobool(buf, &trigger) || !trigger) > > return -EINVAL; > > > > + port = dev_get_drvdata(&cxlmd->dev); > > + if (!port || !is_cxl_endpoint(port)) > > + return -EINVAL; > > + > > down_read(&cxl_dpa_rwsem); > > - rc = cxl_get_poison_by_memdev(cxlmd); > > + if (port->commit_end == -1) { > > + /* No regions mapped to this memdev */ > > + rc = cxl_get_poison_by_memdev(cxlmd); > > + } else { > > + /* Regions mapped, collect poison by endpoint */ > > + rc = device_for_each_child(&port->dev, port, > > + cxl_get_poison_by_endpoint); > > + if (rc == 1) > > + rc = 0; > > + } > > + > > up_read(&cxl_dpa_rwsem); > > > > return rc ? rc : len; > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > index f29028148806..1a558adfe32d 100644 > > --- a/drivers/cxl/core/region.c > > +++ b/drivers/cxl/core/region.c > > @@ -2213,6 +2213,102 @@ struct cxl_pmem_region *to_cxl_pmem_region(struct device *dev) > > } > > EXPORT_SYMBOL_NS_GPL(to_cxl_pmem_region, CXL); > > > > +int cxl_get_poison_by_endpoint(struct device *dev, void *data) > > +{ > > + struct cxl_endpoint_decoder *cxled; > > + struct cxl_port *port = data; > > + struct cxl_dev_state *cxlds; > > + struct cxl_memdev *cxlmd; > > + u64 offset, length; > > + int rc = 0; > > + > > + down_read(&cxl_dpa_rwsem); > > Oh just realized aren't we already under the read lock when this is called? > > Replacing this with a lockdep_assert() could make the logic here easier. > > Ira
Yes! Thanks for the catch. That will allow removal of the goto out's!
> > [...]
| ![\](/images/icornerr.gif) |