linux-cxl.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dan Williams <dan.j.williams@intel.com>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: linux-cxl@vger.kernel.org,
	Chet Douglas <chet.r.douglas@intel.com>,
	Alison Schofield <alison.schofield@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Vishal Verma <vishal.l.verma@intel.com>
Subject: Re: [RFC PATCH v2 15/28] cxl/core: Introduce API to scan switch ports
Date: Mon, 1 Nov 2021 18:45:57 -0700	[thread overview]
Message-ID: <CAPcyv4gYiun5mnRbzw_OP_+6Nk+UzwBDHuUazM_QpYcdX7hzKA@mail.gmail.com> (raw)
In-Reply-To: <20211101225616.35cbr5mugssy35du@intel.com>

On Mon, Nov 1, 2021 at 3:56 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-10-31 22:39:43, Dan Williams wrote:
> > On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > The CXL drivers encapsulate the components that direct memory traffic in
> > > an entity known as a cxl_port. Compute Express Link specifies three such
> > > components: hostbridge (ie. a collection of root ports), switches, and
> > > endpoints. There are currently drivers that create these ports for the
> > > hostbridges and the endpoints (cxl_acpi and cxl_mem). The new API
> > > introduced allows callers to initiate a scan down from the hostbridge
> > > and create ports for switches in the CXL topology.
> > >
> > > The intended user of this API is for endpoint devices. An endpoint
> > > device will need to determine if it is CXL.mem capable, which requires
> > > all components in the path from hostbridge to the endpoint to be CXL.mem
> > > capable. Once an endpoint device determines it's connected to a CXL
> > > capable root port, it can call this API to fill in all the ports in
> > > between the hostbridge and itself.
> > >
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > ---
> > >  .../driver-api/cxl/memory-devices.rst         |   6 +
> > >  drivers/cxl/core/Makefile                     |   1 +
> > >  drivers/cxl/core/bus.c                        | 145 ++++++++++++++++++
> > >  drivers/cxl/core/pci.c                        |  99 ++++++++++++
> > >  drivers/cxl/cxl.h                             |   2 +
> > >  drivers/cxl/pci.h                             |   6 +
> > >  drivers/cxl/port.c                            |   2 +-
> > >  tools/testing/cxl/Kbuild                      |   1 +
> > >  8 files changed, 261 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/cxl/core/pci.c
> > >
> > > diff --git a/Documentation/driver-api/cxl/memory-devices.rst b/Documentation/driver-api/cxl/memory-devices.rst
> > > index fbf0393cdddc..547336c95593 100644
> > > --- a/Documentation/driver-api/cxl/memory-devices.rst
> > > +++ b/Documentation/driver-api/cxl/memory-devices.rst
> > > @@ -47,6 +47,12 @@ CXL Core
> > >  .. kernel-doc:: drivers/cxl/core/bus.c
> > >     :identifiers:
> > >
> > > +.. kernel-doc:: drivers/cxl/core/pci.c
> > > +   :doc: cxl pci
> > > +
> > > +.. kernel-doc:: drivers/cxl/core/pci.c
> > > +   :identifiers:
> > > +
> > >  .. kernel-doc:: drivers/cxl/core/pmem.c
> > >     :doc: cxl pmem
> > >
> > > diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> > > index 07eb8e1fb8a6..9d33d2d5bf09 100644
> > > --- a/drivers/cxl/core/Makefile
> > > +++ b/drivers/cxl/core/Makefile
> > > @@ -7,3 +7,4 @@ cxl_core-y += pmem.o
> > >  cxl_core-y += regs.o
> > >  cxl_core-y += memdev.o
> > >  cxl_core-y += mbox.o
> > > +cxl_core-y += pci.o
> > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > > index c7e1894d503b..f10e7d5b22a4 100644
> > > --- a/drivers/cxl/core/bus.c
> > > +++ b/drivers/cxl/core/bus.c
> > > @@ -8,6 +8,7 @@
> > >  #include <linux/idr.h>
> > >  #include <cxlmem.h>
> > >  #include <cxl.h>
> > > +#include <pci.h>
> > >  #include "core.h"
> > >
> > >  /**
> > > @@ -445,6 +446,150 @@ struct cxl_port *devm_cxl_add_port(struct device *uport,
> > >  }
> > >  EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> > >
> > > +void devm_cxl_remove_port(struct cxl_port *port)
> > > +{
> > > +       down_read(&root_host_sem);
> > > +       if (cxl_root_host) {
> > > +               devm_release_action(cxl_root_host, cxl_unlink_uport, port);
> > > +               devm_release_action(cxl_root_host, unregister_port, port);
> > > +       }
> > > +       up_read(&root_host_sem);
> > > +}
> > > +EXPORT_SYMBOL_GPL(devm_cxl_remove_port);
> >
> > If the scan establishes the property that all child ports are devm
> > allocated with their cxl_port-parent, and only if the cxl_port-parent
> > is bound to its driver then I think we don't need to play
> > devm_release_action games().
> >
>
> We had discussed this previously. I was running into an issue when unloading
> cxl_mem. I needed a way to remove the endpoint port and this was your
> recommendation. Are you suggesting if the chain is set up correctly, I don't
> need to do anything?

I think if the chain is set up correctly then you don't need to do
anything special. The endpoint port would be devm registered by the
cxl_memdev driver to its parent cxl_port provided that port is
actively attached to its driver.

> I don't remember exactly what was blowing up but I can try again after things
> are properly parented.

Cool.

>
> > > +
> > > +static int match_port(struct device *dev, const void *data)
> > > +{
> > > +       struct pci_dev *pdev = (struct pci_dev *)data;
> > > +
> > > +       if (dev->type != &cxl_port_type)
> > > +               return 0;
> > > +
> > > +       return to_cxl_port(dev)->uport == &pdev->dev;
> > > +}
> > > +
> > > +static struct cxl_port *find_cxl_port(struct pci_dev *usp)
> > > +{
> > > +       struct device *port_dev;
> > > +
> > > +       if (!pci_is_pcie(usp) || pci_pcie_type(usp) != PCI_EXP_TYPE_UPSTREAM)
> > > +               return NULL;
> > > +
> > > +       port_dev = bus_find_device(&cxl_bus_type, NULL, usp, match_port);
> > > +       if (port_dev)
> > > +               return to_cxl_port(port_dev);
> > > +
> > > +       return NULL;
> > > +}
> > > +
> > > +static int add_upstream_port(struct device *host, struct pci_dev *pdev)
> > > +{
> > > +       struct device *dev = &pdev->dev;
> > > +       struct cxl_port *parent_port;
> > > +       struct cxl_register_map map;
> > > +       struct cxl_port *port;
> > > +       int rc;
> > > +
> > > +       /*
> > > +        * Upstream ports must be connected to a downstream port or root port.
> > > +        * That downstream or root port must have a parent.
> > > +        */
> > > +       if (!pdev->dev.parent->parent)
> > > +               return -ENXIO;
> > > +
> > > +       /* A port is useless if there are no component registers */
> > > +       rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> > > +       if (rc)
> > > +               return rc;
> > > +
> > > +       parent_port = find_cxl_port(to_pci_dev(pdev->dev.parent->parent));
> >
> > This deref chain is unreadable. It wants a helper if it stays, but I
> > can't immediately think of a reason to ever need to look at a
> > grandparent in the hierarchy.
>
> The goal is to be able to find the next PCIe port up in the chain.
>
> My understanding was:
> pdev = PCIe upstream switch
> pdev->dev.parent = PCIe downstream switch connected to pdev.
> pdev->dev.parent->parent = PCIe upstream switch connected to pdev->dev.parent
>
> I was unable to find an idiomatic way to do that. I'm open to suggestions.

Oh ok, I see it now, but I think this can be done in pure CXL terms
and generic devices with the assumption that the parent device of a
cxl_memdev must be a dport. Then this works whether the parent port is
a platform device like ACPI or cxl_test, or a PCIe device.

static int port_has_dport(struct device *dev, const void *dport_dev)
{
        int found = 0;
        struct cxl_port *port;
        struct cxl_dport *dport;

        if (dev->type != &cxl_port_type)
                return 0;
        port = to_cxl_port(dev);

        device_lock(&port->dev);
        list_for_each_entry (dport, &port->dports, list)
                if (dport->dport == dport_dev) {
                        found = 1;
                        break;
                }
        device_unlock(&port->dev);

        return found;
}

struct cxl_port *find_parent_cxl_port(struct cxl_memdev *cxlmd)
{
        return bus_find_device(&cxl_bus_type, NULL, cxlmd->dev.parent,
                               port_has_dport);
}

> >
> > > +       if (!parent_port)
> > > +               return -ENODEV;
> > > +
> > > +       port = devm_cxl_add_port(dev, cxl_reg_block(pdev, &map), parent_port);
> >
> > This is broken because the pci device being used here does not have a
> > driver that knows about CXL bus events.
>
> I don't understand this, but I'd like to. Doesn't this make a port device which
> gets probed by the port driver? Why does the PCI device matter?

I am reacting to the first argument of this call being @dev that came
from the pci_dev that was passed in to be the "host" for the devm
operation. The devm release action triggers at driver unbind of that
host device, but that doesn't make sense because the driver for a
switch has nothing to do with CXL operation.

>
> (I'll mention again, switch code is not tested).
>
> >
> > > +       put_device(&parent_port->dev);
> > > +       if (IS_ERR(port))
> > > +               dev_err(dev, "Failed to add upstream port %ld\n",
> > > +                       PTR_ERR(port));
> > > +       else
> > > +               dev_dbg(dev, "Added CXL port\n");
> > > +
> > > +       return rc;
> > > +}
> > > +
> > > +static int add_downstream_port(struct pci_dev *pdev)
> > > +{
> > > +       resource_size_t creg = CXL_RESOURCE_NONE;
> > > +       struct device *dev = &pdev->dev;
> > > +       struct cxl_port *parent_port;
> > > +       struct cxl_register_map map;
> > > +       u32 lnkcap, port_num;
> > > +       int rc;
> > > +
> > > +       /*
> > > +        * Ports are to be scanned from top down. Therefore, the upstream port
> > > +        * must already exist.
> > > +        */
> > > +       parent_port = find_cxl_port(to_pci_dev(pdev->dev.parent));
> > > +       if (!parent_port)
> > > +               return -ENODEV;
> > > +
> > > +       /*
> > > +        * The spec mandates component registers are present but the
> > > +        * driver does not.
> >
> > What is this trying to convey?
> >
>
> That I'm not validating the hardware, and even though component registers are
> mandatory, the driver will move on even if they're not found. This functionality
> may need to change in the future and so I left the comment there.

I think that could be conveyed without comment with something like:

        if (rc)
                creg = CXL_RESOURCE_NONE;
        else
                creg = cxl_reg_block(pdev, &map);

  reply	other threads:[~2021-11-02  1:46 UTC|newest]

Thread overview: 112+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-22 18:36 [RFC PATCH v2 00/28] CXL Region Creation / HDM decoder programming Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 01/28] cxl: Rename CXL_MEM to CXL_PCI Ben Widawsky
2021-10-29 20:15   ` Dan Williams
2021-10-29 21:20     ` Ben Widawsky
2021-10-29 21:39       ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 02/28] cxl: Move register block enumeration to core Ben Widawsky
2021-10-29 20:23   ` Dan Williams
2021-10-29 21:23     ` Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 03/28] cxl/acpi: Map component registers for Root Ports Ben Widawsky
2021-10-29 20:28   ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 04/28] cxl: Add helper for new drivers Ben Widawsky
2021-10-29 20:30   ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 05/28] cxl/core: Convert decoder range to resource Ben Widawsky
2021-10-29 20:50   ` Dan Williams
2021-10-29 21:26     ` Ben Widawsky
2021-10-29 22:22       ` Dan Williams
2021-10-29 22:37         ` Ben Widawsky
2021-11-01 14:33           ` Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 06/28] cxl: Introduce endpoint decoders Ben Widawsky
2021-10-29 21:00   ` Dan Williams
2021-10-29 22:02     ` Ben Widawsky
2021-10-29 22:25       ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 07/28] cxl/core: Move target population locking to caller Ben Widawsky
2021-10-29 23:03   ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 08/28] cxl/port: Introduce a port driver Ben Widawsky
2021-10-30  1:37   ` Dan Williams
2021-10-31 17:53     ` Dan Williams
2021-10-31 18:10       ` Dan Williams
2021-11-01 17:36         ` Ben Widawsky
2021-11-01 17:53     ` Ben Widawsky
2021-11-01 17:54       ` Ben Widawsky
2021-11-02  3:31       ` Dan Williams
2021-11-02 16:27         ` Ben Widawsky
2021-11-02 17:21           ` Dan Williams
2021-11-02 16:58         ` Ben Widawsky
2021-11-04 19:10           ` Dan Williams
2021-11-04 19:49             ` Ben Widawsky
2021-11-04 20:04               ` Dan Williams
2021-11-04 21:25                 ` Ben Widawsky
2021-11-04 16:37     ` Ben Widawsky
2021-11-04 19:17       ` Dan Williams
2021-11-04 19:46         ` Ben Widawsky
2021-11-04 20:00           ` Dan Williams
2021-11-04 21:26             ` Ben Widawsky
2021-11-03 15:18   ` Jonathan Cameron
2021-10-22 18:36 ` [RFC PATCH v2 09/28] cxl/acpi: Map single port host bridge component registers Ben Widawsky
2021-10-31 18:03   ` Dan Williams
2021-11-01 17:07     ` Ben Widawsky
2021-11-02  2:15       ` Dan Williams
2021-11-02 16:31         ` Ben Widawsky
2021-11-02 17:46           ` Dan Williams
2021-11-02 17:57             ` Ben Widawsky
2021-11-02 18:10               ` Dan Williams
2021-11-02 18:27                 ` Ben Widawsky
2021-11-02 18:49                   ` Dan Williams
2021-11-02 21:15                     ` Ben Widawsky
2021-11-02 21:34                       ` Dan Williams
2021-11-02 21:47                         ` Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 10/28] cxl/core: Store global list of root ports Ben Widawsky
2021-10-31 18:32   ` Dan Williams
2021-11-01 18:43     ` Ben Widawsky
2021-11-02  2:04       ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 11/28] cxl/acpi: Rescan bus at probe completion Ben Widawsky
2021-10-31 19:25   ` Dan Williams
2021-11-01 18:56     ` Ben Widawsky
2021-11-01 21:45       ` Ben Widawsky
2021-11-02  1:56         ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 12/28] cxl/core: Store component register base for memdevs Ben Widawsky
2021-10-31 20:13   ` Dan Williams
2021-11-01 21:50     ` Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 13/28] cxl: Flesh out register names Ben Widawsky
2021-10-31 20:18   ` Dan Williams
2021-11-01 22:00     ` Ben Widawsky
2021-11-02  1:53       ` Dan Williams
2021-11-03 15:53   ` Jonathan Cameron
2021-11-03 16:03     ` Ben Widawsky
2021-11-03 16:42       ` Jonathan Cameron
2021-11-03 17:05         ` Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 14/28] cxl: Hide devm host for ports Ben Widawsky
2021-10-31 21:14   ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 15/28] cxl/core: Introduce API to scan switch ports Ben Widawsky
2021-11-01  5:39   ` Dan Williams
2021-11-01 22:56     ` Ben Widawsky
2021-11-02  1:45       ` Dan Williams [this message]
2021-11-02 16:39         ` Ben Widawsky
2021-11-02 20:00           ` Dan Williams
2021-11-16 16:50         ` Ben Widawsky
2021-11-16 17:51           ` Dan Williams
2021-11-16 18:02             ` Ben Widawsky
2021-11-03 16:08   ` Jonathan Cameron
2021-11-10 17:49     ` Ben Widawsky
2021-11-10 18:10       ` Jonathan Cameron
2021-11-10 21:03         ` Dan Williams
2021-10-22 18:36 ` [RFC PATCH v2 16/28] cxl: Introduce cxl_mem driver Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 17/28] cxl: Disable switch hierarchies for now Ben Widawsky
2021-10-22 18:36 ` [RFC PATCH v2 18/28] cxl/region: Add region creation ABI Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 19/28] cxl/region: Introduce concept of region configuration Ben Widawsky
2021-12-15 17:47   ` Jonathan Cameron
2021-10-22 18:37 ` [RFC PATCH v2 20/28] cxl/region: Introduce a cxl_region driver Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 21/28] cxl/acpi: Handle address space allocation Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 22/28] cxl/region: Address " Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 23/28] cxl/region: Implement XHB verification Ben Widawsky
2022-01-06 16:55   ` Jonathan Cameron
2022-01-06 16:58     ` Ben Widawsky
2022-01-06 17:33       ` Jonathan Cameron
2022-01-06 18:10         ` Jonathan Cameron
2022-01-06 18:34           ` Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 24/28] cxl/region: HB port config verification Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 25/28] cxl/region: Record host bridge target list Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 26/28] cxl/mem: Store the endpoint's uport Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 27/28] cxl/region: Gather HDM decoder resources Ben Widawsky
2021-10-22 18:37 ` [RFC PATCH v2 28/28] cxl: Program decoders for regions Ben Widawsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPcyv4gYiun5mnRbzw_OP_+6Nk+UzwBDHuUazM_QpYcdX7hzKA@mail.gmail.com \
    --to=dan.j.williams@intel.com \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=alison.schofield@intel.com \
    --cc=ben.widawsky@intel.com \
    --cc=chet.r.douglas@intel.com \
    --cc=ira.weiny@intel.com \
    --cc=linux-cxl@vger.kernel.org \
    --cc=vishal.l.verma@intel.com \
    --subject='Re: [RFC PATCH v2 15/28] cxl/core: Introduce API to scan switch ports' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).