diff mbox series

[v2,07/12] cxl: Remove dev_is_cxl_root_child() check in devm_cxl_enumerate_ports()

Message ID 20221018132341.76259-8-rrichter@amd.com
State New
Headers
Series cxl: Add support for Restricted CXL hosts (RCD mode) |

Commit Message

Robert Richter Oct. 18, 2022, 1:23 p.m. UTC 
  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(-)

Comments

Dan Williams Oct. 21, 2022, 5:38 a.m. UTC | #1 
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.
Verma, Vishal L Oct. 21, 2022, 6:32 a.m. UTC | #2 
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.
Robert Richter Nov. 9, 2022, 2:33 p.m. UTC | #3 
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 mbox series

Patch

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);