Messages in this thread Patch in this message | | | Date | Fri, 14 Jul 2023 19:46:00 +0200 | From | Robert Richter <> | Subject | Re: [PATCH v8 03/14] cxl/hdm: Use stored Component Register mappings to map HDM decoder capability |
| |
Dan,
On 03.07.23 17:39:38, Dan Williams wrote: > 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
I have tried various combinations of revisions and setups (patch #3, #5 and #14, ndctl:pending/v77, cxl_test only with both, "ACPI0017" and cxl_pci_probe() disabled), but could not reproduce this. Tests always pass for me:
[51/51] Linking target ndctl/ndctl 1/6 ndctl:cxl / cxl-topology.sh OK 18.59s 2/6 ndctl:cxl / cxl-region-sysfs.sh OK 12.26s 3/6 ndctl:cxl / cxl-labels.sh OK 28.25s 4/6 ndctl:cxl / cxl-create-region.sh OK 19.56s 5/6 ndctl:cxl / cxl-xor-region.sh OK 10.57s 6/6 ndctl:cxl / cxl-security.sh OK 9.77s
Ok: 6 Expected Fail: 0 Fail: 0 Unexpected Pass: 0 Skipped: 0 Timeout: 0
Full log written to /root/ndctl/build/meson-logs/testlog.txt
> > 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.
The cxl_pci driver stores the comp_map of the endpoint in the cxlds structure, struct cxl_port is not yet available at this point. See patch #2 of this series ("cxl/pci: Store the endpoint's Component Register mappings in struct cxl_dev_state").
> > > + } > > + > > + 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;
Can you check if the following works instead, I think the pre-initialization is missing in cxl_mock_mem_probe() for cxl_test:
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index d6d067fbee97..4c4e33de4d74 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -1333,6 +1333,8 @@ struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev) mutex_init(&mds->mbox_mutex); mutex_init(&mds->event.log_lock); mds->cxlds.dev = dev; + mds->cxlds.comp_map.dev = dev; + mds->cxlds.comp_map.resource = CXL_RESOURCE_NONE; mds->cxlds.type = CXL_DEVTYPE_CLASSMEM; return mds; -- 2.30.2 The cxl_pci driver always has something valid or fails otherwise.
If that works the change should be merged into patch #2.
Thanks,
-Robert
> 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.
| |