| Message ID | 20220831081603.3415-14-rrichter@amd.com |
|---|---|
| State | Superseded |
| Delegated to: | Dan Williams |
| Headers | show |
| Series | cxl: Add support for Restricted CXL hosts (RCD mode) | expand |
On Wed, 31 Aug 2022 10:16:01 +0200 Robert Richter <rrichter@amd.com> wrote: > RCD mode has a different enumeration scheme other than in CXL VH mode. > An RCD is directly connected to an RCH without downstream and upstream > ports showing up in between in the PCI hierarchy. Due to the direct > connection of RCD and RCH, the host bridge is always the RCD's parent > instead of the grandparent. Mentioned earlier, but that's not quite true. There is a reference in the spec to allowing it to be under a root port (some sort of virtual structure - I'm not sure of 'why' you would that though.)( > Modify devm_cxl_enumerate_ports() > respectively. Don't line wrap above. > > Implement this by introducing a function to determine the device's > downstream port. The 'for' loop is adjusted for RCD mode and in this > case find_cxl_port() will always find the host's associated port and > the loop iteration stops. > > Signed-off-by: Robert Richter <rrichter@amd.com> > --- > drivers/cxl/core/port.c | 36 ++++++++++++++++++++++++------------ > 1 file changed, 24 insertions(+), 12 deletions(-) > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 61e9915162d5..08b99423dbf8 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -1084,6 +1084,22 @@ static struct device *grandparent(struct device *dev) > return NULL; > } > > +static struct device *cxl_mem_dport_dev(struct cxl_memdev *cxlmd) > +{ > + struct device *dev = cxlmd->dev.parent; > + struct pci_dev *pdev = to_pci_dev(cxlmd->dev.parent); to_pci_dev(dev); > + > + /* > + * An RCiEP is directly connected to the root bridge without > + * any PCI bridges/ports in between. Reduce the parent level > + * for those. > + */ > + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END) > + return dev; > + > + return dev->parent; Switching from grandparent to this is a little confusing because it's done in two steps. Perhaps use return grandparent(cmlmd->dev); here to keep that connection and rename dev in this function to parent. Far too many devices in here with similar names for it to be easy to read. > +} > + > static void delete_endpoint(void *data) > { > struct cxl_memdev *cxlmd = data; > @@ -1339,7 +1355,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, > int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > { > struct device *dev = &cxlmd->dev; > - struct device *iter; > + struct device *dport_dev; > int rc; > > rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd); > @@ -1352,25 +1368,21 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > * attempt fails. > */ > retry: > - for (iter = dev; iter; iter = grandparent(iter)) { > - struct device *dport_dev = grandparent(iter); > + for (dport_dev = cxl_mem_dport_dev(cxlmd); dport_dev; > + dport_dev = grandparent(dport_dev)) { I don't like looping for the RCD case as it relies a bit too much on subtle relationships between devices and parent. Perhaps better to factor out the contents of the loop, then handle the RCD case separately from the main loop. I haven't tried it, so perhaps that looks even less readable. > struct device *uport_dev; > struct cxl_dport *dport; > struct cxl_port *port; > > - if (!dport_dev) > - return 0; > - > uport_dev = dport_dev->parent; > if (!uport_dev) { > - dev_warn(dev, "at %s no parent for dport: %s\n", > - dev_name(iter), dev_name(dport_dev)); > + dev_warn(dev, "no parent for dport: %s\n", > + dev_name(dport_dev)); > return -ENXIO; > } > > - dev_dbg(dev, "scan: iter: %s dport_dev: %s parent: %s\n", > - dev_name(iter), dev_name(dport_dev), > - dev_name(uport_dev)); > + dev_dbg(dev, "scan: dport_dev: %s parent: %s\n", > + dev_name(dport_dev), dev_name(uport_dev)); > port = find_cxl_port(dport_dev, &dport); > if (port) { > dev_dbg(&cxlmd->dev, > @@ -1418,7 +1430,7 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_ports, CXL); > struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, > struct cxl_dport **dport) > { > - return find_cxl_port(grandparent(&cxlmd->dev), dport); > + return find_cxl_port(cxl_mem_dport_dev(cxlmd), dport); > } > EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL); >
On 31.08.22 13:11:19, Jonathan Cameron wrote: > On Wed, 31 Aug 2022 10:16:01 +0200 > Robert Richter <rrichter@amd.com> wrote: > > > RCD mode has a different enumeration scheme other than in CXL VH mode. > > An RCD is directly connected to an RCH without downstream and upstream > > ports showing up in between in the PCI hierarchy. Due to the direct > > connection of RCD and RCH, the host bridge is always the RCD's parent > > instead of the grandparent. > Mentioned earlier, but that's not quite true. There is a reference in > the spec to allowing it to be under a root port (some sort of virtual structure > - I'm not sure of 'why' you would that though.)( Yes, but software view is still the same, see other mail. > > > Modify devm_cxl_enumerate_ports() > > respectively. > > Don't line wrap above. > > > > > Implement this by introducing a function to determine the device's > > downstream port. The 'for' loop is adjusted for RCD mode and in this > > case find_cxl_port() will always find the host's associated port and > > the loop iteration stops. > > > > Signed-off-by: Robert Richter <rrichter@amd.com> > > --- > > drivers/cxl/core/port.c | 36 ++++++++++++++++++++++++------------ > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > > index 61e9915162d5..08b99423dbf8 100644 > > --- a/drivers/cxl/core/port.c > > +++ b/drivers/cxl/core/port.c > > @@ -1084,6 +1084,22 @@ static struct device *grandparent(struct device *dev) > > return NULL; > > } > > > > +static struct device *cxl_mem_dport_dev(struct cxl_memdev *cxlmd) > > +{ > > + struct device *dev = cxlmd->dev.parent; > > + struct pci_dev *pdev = to_pci_dev(cxlmd->dev.parent); > > to_pci_dev(dev); Ok. > > > + > > + /* > > + * An RCiEP is directly connected to the root bridge without > > + * any PCI bridges/ports in between. Reduce the parent level > > + * for those. > > + */ > > + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END) > > + return dev; > > + > > + return dev->parent; > Switching from grandparent to this is a little confusing because it's > done in two steps. Perhaps use > return grandparent(cmlmd->dev); > here to keep that connection and rename dev in this function to parent. > > Far too many devices in here with similar names for it to be easy > to read. Can improve this a little. > > > > +} > > + > > static void delete_endpoint(void *data) > > { > > struct cxl_memdev *cxlmd = data; > > @@ -1339,7 +1355,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, > > int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > { > > struct device *dev = &cxlmd->dev; > > - struct device *iter; > > + struct device *dport_dev; > > int rc; > > > > rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd); > > @@ -1352,25 +1368,21 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > * attempt fails. > > */ > > retry: > > - for (iter = dev; iter; iter = grandparent(iter)) { > > - struct device *dport_dev = grandparent(iter); > > + for (dport_dev = cxl_mem_dport_dev(cxlmd); dport_dev; > > + dport_dev = grandparent(dport_dev)) { > > I don't like looping for the RCD case as it relies a bit too > much on subtle relationships between devices and parent. > > Perhaps better to factor out the contents of the loop, then handle > the RCD case separately from the main loop. > I haven't tried it, so perhaps that looks even less readable. I see your point here, will take a look. Thanks, -Robert
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 61e9915162d5..08b99423dbf8 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1084,6 +1084,22 @@ static struct device *grandparent(struct device *dev) return NULL; } +static struct device *cxl_mem_dport_dev(struct cxl_memdev *cxlmd) +{ + struct device *dev = cxlmd->dev.parent; + struct pci_dev *pdev = to_pci_dev(cxlmd->dev.parent); + + /* + * An RCiEP is directly connected to the root bridge without + * any PCI bridges/ports in between. Reduce the parent level + * for those. + */ + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END) + return dev; + + return dev->parent; +} + static void delete_endpoint(void *data) { struct cxl_memdev *cxlmd = data; @@ -1339,7 +1355,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) { struct device *dev = &cxlmd->dev; - struct device *iter; + struct device *dport_dev; int rc; rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd); @@ -1352,25 +1368,21 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) * attempt fails. */ retry: - for (iter = dev; iter; iter = grandparent(iter)) { - struct device *dport_dev = grandparent(iter); + for (dport_dev = cxl_mem_dport_dev(cxlmd); dport_dev; + dport_dev = grandparent(dport_dev)) { struct device *uport_dev; struct cxl_dport *dport; struct cxl_port *port; - if (!dport_dev) - return 0; - uport_dev = dport_dev->parent; if (!uport_dev) { - dev_warn(dev, "at %s no parent for dport: %s\n", - dev_name(iter), dev_name(dport_dev)); + dev_warn(dev, "no parent for dport: %s\n", + dev_name(dport_dev)); return -ENXIO; } - dev_dbg(dev, "scan: iter: %s dport_dev: %s parent: %s\n", - dev_name(iter), dev_name(dport_dev), - dev_name(uport_dev)); + dev_dbg(dev, "scan: dport_dev: %s parent: %s\n", + dev_name(dport_dev), dev_name(uport_dev)); port = find_cxl_port(dport_dev, &dport); if (port) { dev_dbg(&cxlmd->dev, @@ -1418,7 +1430,7 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_ports, CXL); struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, struct cxl_dport **dport) { - return find_cxl_port(grandparent(&cxlmd->dev), dport); + return find_cxl_port(cxl_mem_dport_dev(cxlmd), dport); } EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL);
RCD mode has a different enumeration scheme other than in CXL VH mode. An RCD is directly connected to an RCH without downstream and upstream ports showing up in between in the PCI hierarchy. Due to the direct connection of RCD and RCH, the host bridge is always the RCD's parent instead of the grandparent. Modify devm_cxl_enumerate_ports() respectively. Implement this by introducing a function to determine the device's downstream port. The 'for' loop is adjusted for RCD mode and in this case find_cxl_port() will always find the host's associated port and the loop iteration stops. Signed-off-by: Robert Richter <rrichter@amd.com> --- drivers/cxl/core/port.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-)