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

[Ben Widawsky <ben.widawsky@intel.com>]

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?

> > +
> >  static void unregister_port(void *_port)
> >  {
> >         struct cxl_port *port = _port;
> >         struct cxl_dport *dport;
> >
> >         device_lock(&port->dev);
> > +       down_read(&root_port_sem);
> >         list_for_each_entry(dport, &port->dports, list) {
> >                 char link_name[CXL_TARGET_STRLEN];
> >
> > @@ -281,7 +302,10 @@ static void unregister_port(void *_port)
> >                              dport->port_id) >= CXL_TARGET_STRLEN)
> >                         continue;
> >                 sysfs_remove_link(&port->dev.kobj, link_name);
> > +
> > +               list_del_init(&dport->root_port_link);
> >         }
> > +       up_read(&root_port_sem);
> >         device_unlock(&port->dev);
> >         device_unregister(&port->dev);
> >  }
> > @@ -431,12 +455,13 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *new)
> >   * @dport_dev: firmware or PCI device representing the dport
> >   * @port_id: identifier for this dport in a decoder's target list
> >   * @component_reg_phys: optional location of CXL component registers
> > + * @root_port: is this a root port (hostbridge downstream)
> >   *
> >   * Note that all allocations and links are undone by cxl_port deletion
> >   * and release.
> >   */
> >  int cxl_add_dport(struct cxl_port *port, struct device *dport_dev, int port_id,
> > -                 resource_size_t component_reg_phys)
> > +                 resource_size_t component_reg_phys, bool root_port)
> 
> With the above implementation there's no need to add this last parameter.