Messages in this thread Patch in this message | | | Date | Mon, 3 Jul 2023 17:39:38 -0700 | From | Dan Williams <> | Subject | RE: [PATCH v8 03/14] cxl/hdm: Use stored Component Register mappings to map HDM decoder capability |
| |
Terry Bowman wrote: > From: Robert Richter <rrichter@amd.com> > > Now, that the Component Register mappings are stored, use them to > enable and map the HDM decoder capabilities. The Component Registers > do not need to be probed again for this, remove probing code. > > The HDM capability applies to Endpoints, USPs and VH Host Bridges. The > Endpoint's component register mappings are located in the cxlds and > else in the port's structure. Provide a helper function > cxl_port_get_comp_map() to locate the mappings depending on the > component's type.
This causes a regression that cxl_test tripped over. It's likely something you could reproduce without cxl_test just be reloading the cxl_port driver. Root cause below, but here is what I see on a test run:
# meson test -C build --suite cxl ninja: no work to do. ninja: Entering directory `/root/git/ndctl/build' [113/113] Linking target ndctl/ndctl 1/6 ndctl:cxl / cxl-topology.sh OK 12.78s 2/6 ndctl:cxl / cxl-region-sysfs.sh OK 7.72s 3/6 ndctl:cxl / cxl-labels.sh FAIL 1.53s (exit status 129 or signal 1 SIGHUP) >>> LD_LIBRARY_PATH=/root/git/ndctl/build/cxl/lib:/root/git/ndctl/build/ndctl/lib:/root/git/ndctl/build/daxctl/lib NDCTL=/root/git/ndctl/build/ndctl/ndctl DAXCTL=/root/git/ndctl/build/daxctl/daxctl MALLOC_PERTURB_=67 TEST_PATH=/root/git/ndctl/build/test DATA_PATH=/root/git/ndctl/test /bin/bash /root/git/ndctl/test/cxl-labels.sh
4/6 ndctl:cxl / cxl-create-region.sh OK 17.05s 5/6 ndctl:cxl / cxl-xor-region.sh OK 5.18s 6/6 ndctl:cxl / cxl-security.sh OK 4.68s
Ok: 5 Expected Fail: 0 Fail: 1 Unexpected Pass: 0 Skipped: 0 Timeout: 0
Full log written to /root/git/ndctl/build/meson-logs/testlog.txt
It's not that cxl-labels.sh causes the error, it is loading and unloading the port driver trips over the problem below:
> > Signed-off-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/hdm.c | 64 +++++++++++++++++++++++------------------- > 1 file changed, 35 insertions(+), 29 deletions(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 4449b34a80cc..b0f59e63e0d2 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -81,26 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) > cxlhdm->interleave_mask |= GENMASK(14, 12); > } > > -static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb, > - struct cxl_component_regs *regs) > -{ > - struct cxl_register_map map = { > - .dev = &port->dev, > - .resource = port->component_reg_phys, > - .base = crb, > - .max_size = CXL_COMPONENT_REG_BLOCK_SIZE, > - }; > - > - cxl_probe_component_regs(&port->dev, crb, &map.component_map); > - if (!map.component_map.hdm_decoder.valid) { > - dev_dbg(&port->dev, "HDM decoder registers not implemented\n"); > - /* unique error code to indicate no HDM decoder capability */ > - return -ENODEV; > - } > - > - return cxl_map_component_regs(&map, regs, BIT(CXL_CM_CAP_CAP_ID_HDM)); > -} > - > static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) > { > struct cxl_hdm *cxlhdm; > @@ -145,6 +125,22 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) > return true; > } > > +static struct cxl_register_map *cxl_port_get_comp_map(struct cxl_port *port) > +{ > + /* > + * HDM capability applies to Endpoints, USPs and VH Host > + * Bridges. The Endpoint's component register mappings are > + * located in the cxlds. > + */ > + if (is_cxl_endpoint(port)) { > + struct cxl_memdev *memdev = to_cxl_memdev(port->uport_dev); > + > + return &memdev->cxlds->comp_map;
...but why? The issue here is that the @dev argument in that map is the memdev parent PCI device. However, in this context the @dev for devm operations wants to be &port->dev.
> + } > + > + return &port->comp_map;
...so this is fine, and folding in the following resolves the test failure.
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index b0f59e63e0d2..6f111f487795 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -125,22 +125,6 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) return true; } -static struct cxl_register_map *cxl_port_get_comp_map(struct cxl_port *port) -{ - /* - * HDM capability applies to Endpoints, USPs and VH Host - * Bridges. The Endpoint's component register mappings are - * located in the cxlds. - */ - if (is_cxl_endpoint(port)) { - struct cxl_memdev *memdev = to_cxl_memdev(port->uport_dev); - - return &memdev->cxlds->comp_map; - } - - return &port->comp_map; -} - /** * devm_cxl_setup_hdm - map HDM decoder component registers * @port: cxl_port to map @@ -160,8 +144,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port, cxlhdm->port = port; dev_set_drvdata(dev, cxlhdm); - comp_map = cxl_port_get_comp_map(port); - + comp_map = &port->comp_map; if (comp_map->resource == CXL_RESOURCE_NONE) { if (info && info->mem_enabled) { cxlhdm->decoder_count = info->ranges;
Am I missing why the cxlds->comp_map needs to be reused?
Note that I am out and may not be able to respond further until next week.
| |