On Mon, Jan 31, 2022 at 10:11 AM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
>
> On Wed, 26 Jan 2022 12:16:52 -0800
> Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
>
> > From: Ben Widawsky <ben.widawsky@xxxxxxxxx>
> >
> > The need for a CXL port driver and a dedicated cxl_bus_type is driven by
> > a need to simultaneously support 2 independent physical memory decode
> > domains (cache coherent CXL.mem and uncached PCI.mmio) that also
> > intersect at a single PCIe device node. A CXL Port is a device that
> > advertises a CXL Component Register block with an "HDM Decoder
> > Capability Structure".
> >
> > >From Documentation/driver-api/cxl/memory-devices.rst:
> >
> > Similar to how a RAID driver takes disk objects and assembles them into
> > a new logical device, the CXL subsystem is tasked to take PCIe and ACPI
> > objects and assemble them into a CXL.mem decode topology. The need for
> > runtime configuration of the CXL.mem topology is also similar to RAID in
> > that different environments with the same hardware configuration may
> > decide to assemble the topology in contrasting ways. One may choose
> > performance (RAID0) striping memory across multiple Host Bridges and
> > endpoints while another may opt for fault tolerance and disable any
> > striping in the CXL.mem topology.
> >
> > The port driver identifies whether an endpoint Memory Expander is
> > connected to a CXL topology. If an active (bound to the 'cxl_port'
> > driver) CXL Port is not found at every PCIe Switch Upstream port and an
> > active "root" CXL Port then the device is just a plain PCIe endpoint
> > only capable of participating in PCI.mmio and DMA cycles, not CXL.mem
> > coherent interleave sets.
> >
> > The 'cxl_port' driver lets the CXL subsystem leverage driver-core
> > infrastructure for setup and teardown of register resources and
> > communicating device activation status to userspace. The cxl_bus_type
> > can rendezvous the async arrival of platform level CXL resources (via
> > the 'cxl_acpi' driver) with the asynchronous enumeration of Memory
> > Expander endpoints, while also implementing a hierarchical locking model
> > independent of the associated 'struct pci_dev' locking model. The
> > locking for dport and decoder enumeration is now handled in the core
> > rather than callers.
> >
> > For now the port driver only enumerates and registers CXL resources
> > (downstream port metadata and decoder resources) later it will be used
> > to take action on its decoders in response to CXL.mem region
> > provisioning requests.
>
> >
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > Signed-off-by: Ben Widawsky <ben.widawsky@xxxxxxxxx>
> > [djbw: add theory of operation document, move enumeration infra to core]
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>
> Nice docs. A few comments inline
>
> All trivial though, so
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
>
>
> ...
>
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index 2b09d04d3568..682e7cdbcc9c 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -40,6 +40,11 @@ static int cxl_device_id(struct device *dev)
>
> ...
>
> >
> > +/*
> > + * Since root-level CXL dports cannot be enumerated by PCI they are not
> > + * enumerated by the common port driver that acquires the port lock over
> > + * dport add/remove. Instead, root dports are manually added by a
> > + * platform driver and cond_port_lock() is used to take the missing port
> > + * lock in that case.
> > + */
> > +static void cond_port_lock(struct cxl_port *port)
>
> Could the naming here make it clear what the condition is?
> cxl_port_lock_if_root(), or something like that?
Sure, how about cond_cxl_root_lock()? Where the cond_ prefix is
matching other helpers like cond_resched().
>
> > +{
> > + if (is_cxl_root(port))
> > + cxl_device_lock(&port->dev);
> > +}
> > +
> > +static void cond_port_unlock(struct cxl_port *port)
> > +{
> > + if (is_cxl_root(port))
> > + cxl_device_unlock(&port->dev);
> > +}
> > +
> > static void cxl_dport_remove(void *data)
> > {
> > struct cxl_dport *dport = data;
> > struct cxl_port *port = dport->port;
> >
> > - cxl_device_lock(&port->dev);
> > + cond_port_lock(port);
> > list_del_init(&dport->list);
> > - cxl_device_unlock(&port->dev);
> > + cond_port_unlock(port);
> > put_device(dport->dport);
> > }
> >
> > @@ -588,7 +615,9 @@ struct cxl_dport *devm_cxl_add_dport(struct device *host, struct cxl_port *port,
> > dport->component_reg_phys = component_reg_phys;
> > dport->port = port;
> >
> > + cond_port_lock(port);
> > rc = add_dport(port, dport);
> > + cond_port_unlock(port);
> > if (rc)
> > return ERR_PTR(rc);
> >
> > @@ -887,6 +916,7 @@ static int cxl_bus_probe(struct device *dev)
> > rc = to_cxl_drv(dev->driver)->probe(dev);
> > cxl_nested_unlock(dev);
> >
> > + dev_dbg(dev, "probe: %d\n", rc);
>
> This feels a little bit odd to see in this patch.
> I'd be tempted to drop it.
Ok.
>
>
> > return rc;
> > }
> >
>
> >
> > #define MODULE_ALIAS_CXL(type) MODULE_ALIAS("cxl:t" __stringify(type) "*")
> > #define CXL_MODALIAS_FMT "cxl:t%d"
> > diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> > index 103636fda198..47640f19e899 100644
> > --- a/drivers/cxl/cxlpci.h
> > +++ b/drivers/cxl/cxlpci.h
> > @@ -2,6 +2,7 @@
> > /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
> > #ifndef __CXL_PCI_H__
> > #define __CXL_PCI_H__
> > +#include <linux/pci.h>
>
> Why in this patch?
Oh, I'll mention this in the changelog. Up until now all the users of
cxlpci.h also included linux/pci.h on their own, but port.c did not
leading to:
drivers/cxl/cxlpci.h: In function ‘cxl_regmap_to_base’:
drivers/cxl/cxlpci.h:57:16: error: implicit declaration of function
‘pci_resource_start’;
...since cxlpci.h ships the dependency it should also carry the include.
>
> > #include "cxl.h"
> >
> > #define CXL_MEMORY_PROGIF 0x10
>
>
> > diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> > index 3045d7cba0db..3e2a529875ea 100644
> > --- a/tools/testing/cxl/Kbuild
> > +++ b/tools/testing/cxl/Kbuild
> > @@ -26,6 +26,12 @@ obj-m += cxl_pmem.o
> > cxl_pmem-y := $(CXL_SRC)/pmem.o
> > cxl_pmem-y += config_check.o
> >
> > +obj-m += cxl_port.o
> > +
> > +cxl_port-y := $(CXL_SRC)/port.o
> > +cxl_port-y += config_check.o
> > +
>
> trivial but one blank line seems like enough.
Sure.
>
> > +
> > obj-m += cxl_core.o
> >
> > cxl_core-y := $(CXL_CORE_SRC)/port.o
>
>
