Re: [RFC PATCH v2 10/28] cxl/core: Store global list of root ports

[Dan Williams <dan.j.williams@intel.com>]

On Mon, Nov 1, 2021 at 11:43 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-10-31 11:32:45, Dan Williams wrote:
> > On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > CXL root ports (the downstream port to a host bridge) are to be
> > > enumerated by a platform specific driver. In the case of ACPI compliant
> > > systems, this is like the cxl_acpi driver. Root ports are the first
> > > CXL spec defined component that can be "found" by that platform specific
> > > driver.
> > >
> > > By storing a list of these root ports components in lower levels of the
> > > topology (switches and endpoints), have a mechanism to walk up their
> > > device hierarchy to find an enumerated root port. This will be necessary
> > > for region programming.
> > >
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > ---
> > >  drivers/cxl/acpi.c            |  4 ++--
> > >  drivers/cxl/core/bus.c        | 34 +++++++++++++++++++++++++++++++++-
> > >  drivers/cxl/cxl.h             |  5 ++++-
> > >  tools/testing/cxl/mock_acpi.c |  4 ++--
> > >  4 files changed, 41 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > index 8cca0814dfb8..625c5d95b83f 100644
> > > --- a/drivers/cxl/acpi.c
> > > +++ b/drivers/cxl/acpi.c
> > > @@ -231,7 +231,7 @@ __mock int match_add_root_ports(struct pci_dev *pdev, void *data)
> > >                 creg = cxl_reg_block(pdev, &map);
> > >
> > >         port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
> > > -       rc = cxl_add_dport(port, &pdev->dev, port_num, creg);
> > > +       rc = cxl_add_dport(port, &pdev->dev, port_num, creg, true);
> > >         if (rc) {
> > >                 ctx->error = rc;
> > >                 return rc;
> > > @@ -406,7 +406,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
> > >                 dev_dbg(host, "No CHBS found for Host Bridge: %s\n",
> > >                         dev_name(match));
> > >
> > > -       rc = cxl_add_dport(root_port, match, uid, get_chbcr(chbs));
> > > +       rc = cxl_add_dport(root_port, match, uid, get_chbcr(chbs), false);
> > >         if (rc) {
> > >                 dev_err(host, "failed to add downstream port: %s\n",
> > >                         dev_name(match));
> > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > > index dffbd0ac64af..03394a3ae75f 100644
> > > --- a/drivers/cxl/core/bus.c
> > > +++ b/drivers/cxl/core/bus.c
> > > @@ -25,6 +25,8 @@
> > >   */
> > >
> > >  static DEFINE_IDA(cxl_port_ida);
> > > +static LIST_HEAD(cxl_root_ports);
> > > +static DECLARE_RWSEM(root_port_sem);
> >
> > I don't see a need for this new list and lock...
> >
> > >
> > >  static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
> > >                             char *buf)
> > > @@ -268,12 +270,31 @@ struct cxl_port *to_cxl_port(struct device *dev)
> > >  }
> > >  EXPORT_SYMBOL_GPL(to_cxl_port);
> > >
> > > +struct cxl_dport *cxl_get_root_dport(struct device *dev)
> > > +{
> > > +       struct cxl_dport *ret = NULL;
> > > +       struct cxl_dport *dport;
> > > +
> > > +       down_read(&root_port_sem);
> > > +       list_for_each_entry(dport, &cxl_root_ports, root_port_link) {
> > > +               if (dport->dport == dev) {
> > > +                       ret = dport;
> > > +                       break;
> > > +               }
> > > +       }
> > > +
> > > +       up_read(&root_port_sem);
> > > +       return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cxl_get_root_dport);
> >
> > This can be done by walking the existing topology:
> >
> > struct cxl_dport *cxl_get_root_dport(struct device *dev)
> > {
> >         struct device *host = get_cxl_topology_host();
> >         struct cxl_dport *dport, *found = NULL;
> >         struct cxl_port *port;
> >         struct device *root;
> >
> >         if (!host)
> >                 return NULL;
> >
> >         root = device_find_child(host, &root, match_cxl_root_port);
> >         if (!root)
> >                 goto out;
> >         device_lock(root);
> >         port = to_cxl_port(root);
> >         list_for_each_entry (dport, &port->dports, list)
> >                 if (dport->dport == dev) {
> >                         found = dport;
> >                         break;
> >                 }
> >         device_unlock(root);
> >         put_device(root);
> >
> > out:
> >         put_cxl_topology_host(host);
> >         return found;
> > }
> >
> >
> > It occurs to me after writing this that device_lock() for iterating
> > dports can be offloaded to the topology rwsem.
> >
>
> When I originally wrote this function, I didn't have a pointer to the root host.
> Also, you said previously we'd remove the root host. What would you like me to
> do?

Root device retrieval is useful for these scans. However, the role of
overall "devm host" can be removed with careful port-child devm
registration.