| Message ID | 20221018132341.76259-8-rrichter@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Add support for Restricted CXL hosts (RCD mode) | expand |
Robert Richter wrote: > The dev_is_cxl_root_child() check adds complexity to the control flow > of the iterator loop in devm_cxl_enumerate_ports(). This check is > unnecessary and can safely be removed: If the port of a dport_dev is > connected to the cxl root port, the grandparent of dport_dev will be > null. This is checked early in the iterator loop and the function is > left in this case. > > Drop this check to ease the control flow in devm_cxl_enumerate_ > ports(). > > This change is a prerequisite of factoring out parts of the loop. Ok, so this seems to be where we diverge on how an RCH topology maps into the CXL subsystem object hierarchy. The main observation going through this with Dave before this set came out is that devm_cxl_enumerate_ports() is a nop and should be skipped in the RCH case. devm_cxl_enumerate_ports() is only for discovering intermediate ports between a host bridge and an endpoint. In a CXL VH topology a direct attached endpoint is downstream of a host-bridge's root port. In the CXL RCH topology the Root-Complex-Integrated-Endpoint is a *peer* of a host-bridge's root port. So one level of the hierarchy is removed and devm_cxl_enumerate_ports() can be skipped. The proposal is to have cxl_mem_probe() do something like: if (!cxlds->is_rcd) { rc = devm_cxl_enumerate_ports(cxlmd); if (rc) return rc; } The existing: parent_port = cxl_mem_find_port(cxlmd, &dport); ...should do the right thing as long as cxl_acpi registers the host bridge as a dport with this change: - dport = devm_cxl_add_dport(root_port, match, uid, ... + dport = devm_cxl_add_dport(root_port, pci_root->bus->bridge, uid, ... That way the dport device is not the ACPI device but the 'struct pci_dev' for the host-bridge. With that scheme in place and some cxl-cli fixups from Vishal we are seeing: # cxl list -BEMPTu { "bus":"root0", "provider":"ACPI.CXL", "nr_dports":1, "dports":[ { "dport":"pci0000:38", "id":"0x31" } ], "endpoints:root0":[ { "endpoint":"endpoint1", "host":"mem0", "depth":1, "memdev":{ "memdev":"mem0", "pmem_size":0, "ram_size":"16.00 GiB (17.18 GB)", "serial":"0", "numa_node":0, "host":"0000:38:00.0" } } ] } Does that make sense? I think this patchset gets a lot simpler if it does not try to make devm_cxl_enumerate_ports() understand the RCH topology.
On Thu, 2022-10-20 at 22:38 -0700, Dan Williams wrote: > Robert Richter wrote: > > <..> > > With that scheme in place and some cxl-cli fixups from Vishal we are > seeing: > > # cxl list -BEMPTu > { > "bus":"root0", > "provider":"ACPI.CXL", > "nr_dports":1, > "dports":[ > { > "dport":"pci0000:38", > "id":"0x31" > } > ], > "endpoints:root0":[ > { > "endpoint":"endpoint1", > "host":"mem0", > "depth":1, > "memdev":{ > "memdev":"mem0", > "pmem_size":0, > "ram_size":"16.00 GiB (17.18 GB)", > "serial":"0", > "numa_node":0, > "host":"0000:38:00.0" > } > } > ] > } I was waiting to post the cxl-cli patches for this until the kernel discussion settles - but maybe this topology layout is settled enough? In the meanwhile I've pushed a branch with these fixups here: https://github.com/pmem/ndctl/tree/vv/rch-support > > Does that make sense? I think this patchset gets a lot simpler if it > does not try to make devm_cxl_enumerate_ports() understand the RCH > topology.
Vishal, On 21.10.22 06:32:33, Verma, Vishal L wrote: > I was waiting to post the cxl-cli patches for this until the kernel > discussion settles - but maybe this topology layout is settled enough? > > In the meanwhile I've pushed a branch with these fixups here: > https://github.com/pmem/ndctl/tree/vv/rch-support I sent out the v3 series today, patch #1 contains the change of the port's reference device. You need to adopt the ndctl patches for this. To find the alias in sysfs, you need to lookup the firmware_node instead of the physical_node. I would suggest to change the implemenation to just check for both entries. Thanks, -Robert
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 0431ed860d8e..9af2d91db9bf 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1395,6 +1395,7 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) "found already registered port %s:%s\n", dev_name(&port->dev), dev_name(port->uport)); rc = cxl_add_ep(dport, &cxlmd->dev); + put_device(&port->dev); /* * If the endpoint already exists in the port's list, @@ -1403,19 +1404,11 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) * the parent_port lock as the current port may be being * reaped. */ - if (rc && rc != -EBUSY) { - put_device(&port->dev); + if (rc && rc != -EBUSY) return rc; - } /* Any more ports to add between this one and the root? */ - if (!dev_is_cxl_root_child(&port->dev)) { - put_device(&port->dev); - continue; - } - - put_device(&port->dev); - return 0; + continue; } rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
The dev_is_cxl_root_child() check adds complexity to the control flow of the iterator loop in devm_cxl_enumerate_ports(). This check is unnecessary and can safely be removed: If the port of a dport_dev is connected to the cxl root port, the grandparent of dport_dev will be null. This is checked early in the iterator loop and the function is left in this case. Drop this check to ease the control flow in devm_cxl_enumerate_ ports(). This change is a prerequisite of factoring out parts of the loop. Signed-off-by: Robert Richter <rrichter@amd.com> --- drivers/cxl/core/port.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)