On 21-11-22 18:17:34, Jonathan Cameron wrote:
> On Fri, 19 Nov 2021 16:02:49 -0800
> Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote:
>
> > Add a driver that is capable of determining whether a device is in a
> > CXL.mem routed part of the topology.
> >
> > This driver allows a higher level driver - such as one controlling CXL
> > regions, which is itself a set of CXL devices - to easily determine if
> > the CXL devices are CXL.mem capable by checking if the driver has bound.
> > CXL memory device services may also be provided by this driver though
> > none are needed as of yet. cxl_mem also plays the part of registering
> > itself as an endpoint port, which is a required step to enumerate the
> > device's HDM decoder resources.
> >
> > Even though cxl_mem driver is the only consumer of the new
> > cxl_scan_ports() introduced in cxl_core, because that functionality has
> > PCIe specificity it is kept out of this driver.
> >
> > As part of this patch, find_dport_by_dev() is promoted to the cxl_core's
> > set of APIs for use by the new driver.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@xxxxxxxxx>
> >
> Main thing in here is the mysterious private data. I'd drop
> that until we have patches that set it in the same series.
It's used, it just got leaked into the wrong patch (b37c9a7eca3f ("cxl/port:
Introduce a port driver")). I'll fix it up so it gets added here.
/* Minor layering violation */
static int dvsec_range_used(struct cxl_port *port)
{
struct cxl_endpoint_dvsec_info *info;
int i, ret = 0;
if (!is_endpoint_port(port))
return 0;
info = port->data;
for (i = 0; i < info->ranges; i++)
if (info->range[i].size)
ret |= 1 << i;
return ret;
}
>
>
>
>
> ...
>
> > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > index acfa212eea21..cab3aabd5abc 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"
> >
> > /**
> > @@ -436,7 +437,7 @@ static int devm_cxl_link_uport(struct device *host, struct cxl_port *port)
> >
> > static struct cxl_port *cxl_port_alloc(struct device *uport,
> > resource_size_t component_reg_phys,
> > - struct cxl_port *parent_port)
> > + struct cxl_port *parent_port, void *data)
> > {
> > struct cxl_port *port;
> > struct device *dev;
> > @@ -465,6 +466,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
> >
> > port->uport = uport;
> > port->component_reg_phys = component_reg_phys;
> > + port->data = data;
> > ida_init(&port->decoder_ida);
> > INIT_LIST_HEAD(&port->dports);
> >
> > @@ -485,16 +487,17 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
> > * @uport: "physical" device implementing this upstream port
> > * @component_reg_phys: (optional) for configurable cxl_port instances
> > * @parent_port: next hop up in the CXL memory decode hierarchy
> > + * @data: opaque data to be used by the port driver
> > */
> > struct cxl_port *devm_cxl_add_port(struct device *uport,
> > resource_size_t component_reg_phys,
> > - struct cxl_port *parent_port)
> > + struct cxl_port *parent_port, void *data)
> > {
> > struct device *dev, *host;
> > struct cxl_port *port;
> > int rc;
> >
> > - port = cxl_port_alloc(uport, component_reg_phys, parent_port);
> > + port = cxl_port_alloc(uport, component_reg_phys, parent_port, data);
> > if (IS_ERR(port))
> > return port;
> >
> > @@ -531,6 +534,113 @@ struct cxl_port *devm_cxl_add_port(struct device *uport,
> > }
> > EXPORT_SYMBOL_NS_GPL(devm_cxl_add_port, CXL);
> >
>
>
> ...
>
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > new file mode 100644
> > index 000000000000..818e30571e4d
> > --- /dev/null
> > +++ b/drivers/cxl/core/pci.c
> > @@ -0,0 +1,119 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> > +#include <linux/device.h>
> > +#include <linux/pci.h>
> > +#include <cxl.h>
> > +#include <pci.h>
> > +#include "core.h"
> > +
> > +/**
> > + * DOC: cxl core pci
> > + *
> > + * Compute Express Link protocols are layered on top of PCIe. CXL core provides
> > + * a set of helpers for CXL interactions which occur via PCIe.
> > + */
> > +
> > +/**
> > + * find_parent_cxl_port() - Finds parent port through PCIe mechanisms
> > + * @pdev: PCIe USP or DSP to find an upstream port for
> > + *
> > + * Once all CXL ports are enumerated, there is no need to reference the PCIe
> > + * parallel universe as all downstream ports are contained in a linked list, and
> > + * all upstream ports are accessible via pointer. During the enumeration, it is
> > + * very convenient to be able to peak up one level in the hierarchy without
> > + * needing the established relationship between data structures so that the
> > + * parenting can be done as the ports/dports are created.
> > + *
> > + * A reference is kept to the found port.
> > + */
> > +struct cxl_port *find_parent_cxl_port(struct pci_dev *pdev)
> > +{
> > + struct device *parent_dev, *gparent_dev;
> > +
> > + /* Parent is either a downstream port, or root port */
> > + parent_dev = get_device(pdev->dev.parent);
> > +
> > + if (is_cxl_switch_usp(&pdev->dev)) {
> > + if (dev_WARN_ONCE(&pdev->dev,
>
> maybe put the condition var in a local variable to reduce the indent and get something
> more readable?
>
> > + pci_pcie_type(pdev) !=
> > + PCI_EXP_TYPE_DOWNSTREAM &&
> > + pci_pcie_type(pdev) !=
> > + PCI_EXP_TYPE_ROOT_PORT,
> > + "Parent not downstream\n"))
> > + goto err;
> > +
> > + /*
> > + * Grandparent is either an upstream port or a platform device that has
> > + * been added as a cxl_port already.
> > + */
> > + gparent_dev = get_device(parent_dev->parent);
> > + put_device(parent_dev);
> > +
> > + return to_cxl_port(gparent_dev);
> > + } else if (is_cxl_switch_dsp(&pdev->dev)) {
> > + if (dev_WARN_ONCE(&pdev->dev,
> > + pci_pcie_type(pdev) != PCI_EXP_TYPE_UPSTREAM,
> > + "Parent not upstream"))
> > + goto err;
> > + return to_cxl_port(parent_dev);
> > + }
> > +
> > +err:
> > + dev_WARN(&pdev->dev, "Invalid topology\n");
> > + put_device(parent_dev);
> > + return NULL;
> > +}
> > +
>
> ...
>
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index f8354241c5a3..3bda806f4244 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -296,6 +296,7 @@ struct cxl_port {
> > * @port: reference to cxl_port that contains this downstream port
> > * @list: node for a cxl_port's list of cxl_dport instances
> > * @root_port_link: node for global list of root ports
> > + * @data: Opaque data passed by other drivers, used by port driver
>
> Is this used yet? possible leave introducing this until we need it
> as not obvious here what it will be for.
>
Yep...
> > */
> > struct cxl_dport {
> > struct device *dport;
> > @@ -304,16 +305,20 @@ struct cxl_dport {
> > struct cxl_port *port;
> > struct list_head list;
> > struct list_head root_port_link;
> > + void *data;
> > };
> >
> > struct cxl_port *to_cxl_port(struct device *dev);
> > struct cxl_port *devm_cxl_add_port(struct device *uport,
> > resource_size_t component_reg_phys,
> > - struct cxl_port *parent_port);
> > + struct cxl_port *parent_port, void *data);
> > +void cxl_scan_ports(struct cxl_dport *root_port);
>
> ...
>
> > +
> > +static int create_endpoint(struct device *dev, struct cxl_port *parent,
> > + struct cxl_dport *dport)
> > +{
> > + struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> > + struct cxl_dev_state *cxlds = cxlmd->cxlds;
> > + struct cxl_endpoint_dvsec_info *info = cxlds->info;
> > + struct cxl_port *endpoint;
> > + int rc;
> > +
> > + endpoint =
> > + devm_cxl_add_port(dev, cxlds->component_reg_phys, parent, info);
>
> I'd just have that on one line, or break it somewhere in the parameter list.
> Right now it just looks odd and saves maybe 4 characters in line length.
>
> > + if (IS_ERR(endpoint))
> > + return PTR_ERR(endpoint);
> > +
> > + rc = sysfs_create_link(&cxlmd->dev.kobj, &dport->dport->kobj,
> > + "root_port");
>
> Not obvious to me what this link is for. Maybe needs a docs update
> somewhere?
>
Okay. IIRC this was a request from Dan to help userspace tooling but I might be
mistaken on that.
> > + if (rc) {
> > + device_del(&endpoint->dev);
> > + return rc;
> > + }
> > + dev_dbg(dev, "add: %s\n", dev_name(&endpoint->dev));
> > +
> > + return devm_add_action_or_reset(dev, remove_endpoint, cxlmd);
> > +}
> > +
> > +static int cxl_mem_probe(struct device *dev)
> > +{
> > + struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> > + struct cxl_port *hostbridge, *parent_port;
> > + struct walk_ctx ctx = { NULL, false };
>
> Perhaps use c99 style to show what is being initialized inside the walk ctx.
> Will make it more obvious these are the right values.
>
> > + struct cxl_dport *dport;
> > + int rc;
> > +
> > + rc = wait_for_media(cxlmd);
> > + if (rc) {
> > + dev_err(dev, "Media not active (%d)\n", rc);
> > + return rc;
> > + }
> > +
> > + walk_to_root_port(dev, &ctx);
> > +
> > + /*
> > + * Couldn't find a CXL capable root port. This may happen even with a
> > + * CXL capable topology if cxl_acpi hasn't completed yet. A rescan will
> > + * occur.
> > + */
> > + if (!ctx.root_port)
> > + return -ENODEV;
> > +
> > + hostbridge = ctx.root_port->port;
> > + device_lock(&hostbridge->dev);
> > +
> > + /* hostbridge has no port driver, the topology isn't enabled yet */
> > + if (!hostbridge->dev.driver) {
> > + device_unlock(&hostbridge->dev);
> > + return -ENODEV;
> > + }
> > +
> > + /* No switch + found root port means we're done */
> > + if (!ctx.has_switch) {
> > + parent_port = to_cxl_port(&hostbridge->dev);
> > + dport = ctx.root_port;
> > + goto out;
> > + }
> > +
> > + /* Walk down from the root port and add all switches */
> > + cxl_scan_ports(ctx.root_port);
> > +
> > + /* If parent is a dport the endpoint is good to go. */
> > + parent_port = to_cxl_port(dev->parent->parent);
> > + dport = cxl_find_dport_by_dev(parent_port, dev->parent);
> > + if (!dport) {
> > + rc = -ENODEV;
> > + goto err_out;
> > + }
> > +
> > +out:
> > + rc = create_endpoint(dev, parent_port, dport);
> > + if (rc)
> > + goto err_out;
> > +
> > + cxlmd->root_port = ctx.root_port;
> > +
> > +err_out:
> > + device_unlock(&hostbridge->dev);
> > + return rc;
> > +}
> > +
>
> > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
> > index 2a48cd65bf59..3fd0909522f2 100644
> > --- a/drivers/cxl/pci.h
> > +++ b/drivers/cxl/pci.h
> > @@ -15,6 +15,7 @@
> >
> > /* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */
> > #define CXL_DVSEC_PCIE_DEVICE 0
> > +
>
> Noise that shouldn't be in this patch.
>
I wish there was some automation I had to catch this kind of thing... Thanks.
> > #define CXL_DVSEC_PCIE_DEVICE_CAP_OFFSET 0xA
> > #define CXL_DVSEC_PCIE_DEVICE_MEM_CAPABLE BIT(2)
> > #define CXL_DVSEC_PCIE_DEVICE_HDM_COUNT_MASK GENMASK(5, 4)
> > @@ -64,4 +65,7 @@ enum cxl_regloc_type {
> > ((resource_size_t)(pci_resource_start(pdev, (map)->barno) + \
> > (map)->block_offset))
> >
> > +bool is_cxl_switch_usp(struct device *dev);
> > +bool is_cxl_switch_dsp(struct device *dev);
> > +
> > #endif /* __CXL_PCI_H__ */
>
>
