Re: [PATCH v8 03/14] cxl/hdm: Use stored Component Register mappings to map HDM decoder capability

From: Robert Richter
Date: Fri Jul 14 2023 - 13:54:23 EST

Next message: Rob Herring: "[PATCH] soc: microchip: Explicitly include correct DT includes"
Previous message: Rob Herring: "[PATCH] soc: sunxi: Explicitly include correct DT includes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Dan,

On 03.07.23 17:39:38, Dan Williams wrote:
> Terry Bowman wrote:
> > From: Robert Richter <rrichter@xxxxxxx>
> >
> > 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@xxxxxxx>
> > Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > ---
> > 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.

Next message: Rob Herring: "[PATCH] soc: microchip: Explicitly include correct DT includes"
Previous message: Rob Herring: "[PATCH] soc: sunxi: Explicitly include correct DT includes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]