lkml.org 
[lkml]   [2022]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 13/15] cxl/acpi: Rework devm_cxl_enumerate_ports() to support RCD mode
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

\
 
 \ /
  Last update: 2022-09-01 09:53    [W:0.075 / U:0.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site