[PATCH v5 0/6] CXL port and decoder enumeration

[PATCH v5 0/6] CXL port and decoder enumeration

[Dan Williams]

Changes since v4 [1]:
- Rework the object model to better account for downstream ports and
  enumerate decode capabilities.
- Include the definition of CFMWS
- Reference CFMWS in the changelogs and implementation
- Drop mention of pcie_portdriver for CXL switch enumeration. This will
  be handled by CXL path validation when an endpoint wants to contribute
  to a memory region.

[1]: http://lore.kernel.org/r/162096970332.1865304.10280028741091576940.stgit@dwillia2-desk3.amr.corp.intel.com

---

The recently published CXL Fixed Memory Window Structure (CFMWS)
extension to the CXL Early Discovery Table (CEDT) provides a platform
firmware mechanism to enumerate CXL memory resources. The table data
indicates which CXL memory ranges were configured by platform BIOS, and
which address ranges are available to support hot plug and dynamic
provisioning of CXL memory regions.

CXL Port Topology:

The enumeration starts with the ACPI0017 driver registering a 'struct
cxl_port' object to establish the top of a port topology. It then
scans the ACPI bus looking for CXL Host Bridges (ACPI0016 instances). A
cxl_port represents one or more decoder resources between a 'uport'
(upstream port) and one or more 'dport' (downstream port) instances.
System software must not assume that 'struct cxl_port' device names will
be static from one boot to the next. It will generally be the case that
the root cxl_port starts at id '0' and the host bridges are enumerated
in the same order starting at id '1', but that is not guaranteed.

A 'uport' is a device that implements a decode. It can either be a
platform firmware device like ACPI0017 where the decode is described by
an ACPI data-structure, or a PCIe switch where the upstream port of the
switch implements a CXL DVSEC pointing to component registers with the
HDM decoder capability (see CXL 2.0 section 8.2.5.12 CXL HDM Decoder
Capability Structure).

Once a uport and its corresponding dport instances are collected into a
cxl_port the actual decode resources are then modeled as cxl_decode
objects that are children of their parent cxl_port. The 'decode' object
has a 1:1 relationship with ether CFMWS entries at the root level, or
hardware HDM decoder register instances in a PCIe device's CXL component
register space at any level of a CXL switch hierarchy. In addition to
the interleave geometry and address range a decode object conveys the
target list (targeted dports) in interleave order. The dport id in a
target list is either its ACPI _UID for Host Bridge targets, or the
"port number" field from the link capabilities register in the PCIe
"Express" capability [2].

Here is a tree(1) topology of QEMU emulating a single-ported
host-bridge:

    /sys/bus/cxl/devices/root0
    ├── devtype
    ├── dport0 -> ../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
    ├── port1
    │   ├── decoder1.0
    │   │   ├── devtype
    │   │   ├── end
    │   │   ├── locked
    │   │   ├── start
    │   │   ├── subsystem -> ../../../../bus/cxl
    │   │   ├── target_list
    │   │   ├── target_type
    │   │   └── uevent
    │   ├── devtype
    │   ├── dport0 -> ../../pci0000:34/0000:34:00.0
    │   ├── subsystem -> ../../../bus/cxl
    │   ├── uevent
    │   └── uport -> ../../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
    ├── subsystem -> ../../bus/cxl
    ├── uevent
    └── uport -> ../platform/ACPI0017:00


* The root port is singleton only by convention. A given uport device
  like ACPI0017 could create a root level port per CFMWS entry. This
  patch set chooses to implement 1 port at the root level and list all
  CFMWS decode entries under that port regardless of which dport host
  bridges are targeted.

[2]: CXL 2.0 8.2.5.12.8 CXL HDM Decoder 0 Target List Low Register
     (Offset 24h) ...The Target Port Identifier for a given Downstream Port
     is reported via Port Number field in Link Capabilities Register. (See
     PCI Express Base Specification).

---

Dan Williams (6):
      cxl/acpi: Local definition of ACPICA infrastructure
      cxl/acpi: Introduce cxl_root, the root of a cxl_port topology
      cxl/Kconfig: Default drivers to CONFIG_CXL_BUS
      cxl/acpi: Add downstream port data to cxl_port instances
      cxl/acpi: Enumerate host bridge root ports
      cxl/acpi: Introduce cxl_decoder objects


 Documentation/ABI/testing/sysfs-bus-cxl |   94 ++++++
 drivers/cxl/Kconfig                     |   17 +
 drivers/cxl/Makefile                    |    2 
 drivers/cxl/acpi.c                      |  193 ++++++++++++
 drivers/cxl/acpi.h                      |   48 +++
 drivers/cxl/core.c                      |  514 +++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h                       |   92 ++++++
 7 files changed, 960 insertions(+)
 create mode 100644 drivers/cxl/acpi.c
 create mode 100644 drivers/cxl/acpi.h

base-commit: 605a5e41db7d8c930fb80115686991c4c1d08ee4

[PATCH v5 1/6] cxl/acpi: Local definition of ACPICA infrastructure

[Dan Williams]

The recently released CXL specification change (ECN) for the CXL Fixed
Memory Window Structure (CFMWS) extension to the CXL Early Discovery
Table (CEDT) enables a large amount of functionality. It defines the
root of a CXL memory topology and is needed for all OS flows for CXL
provisioning CXL memory expanders. For ease of merging and tree
management add the new ACPI definition locally (drivers/cxl/acpi.h) in
such a way that they will not collide with the eventual arrival of the
definitions through the ACPICA project to their final location
(drivers/acpi/actbl1.h).

The definitions in drivers/cxl/acpi.h can be dropped post -rc1.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Co-developed-by: Alison Schofield <alison.schofield@intel.com>
Co-developed-by: Erik Kaneda <erik.kaneda@intel.com>
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Erik Kaneda <erik.kaneda@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/acpi.h |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 drivers/cxl/acpi.h

diff --git a/drivers/cxl/acpi.h b/drivers/cxl/acpi.h
new file mode 100644
index 000000000000..1482c19e7227
--- /dev/null
+++ b/drivers/cxl/acpi.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright(c) 2021 Intel Corporation. */
+#ifndef __CXL_ACPI_H__
+#define __CXL_ACPI_H__
+
+#ifndef ACPI_CEDT_CHBS_VERSION_CXL20
+/*
+ * NOTE: These definitions are temporary and to be deleted in v5.14-rc1
+ * when the identical definitions become available from
+ * include/acpi/actbl1.h.
+ */
+
+#define ACPI_CEDT_TYPE_CFMWS 1
+#define ACPI_CEDT_TYPE_RESERVED 2
+
+#define ACPI_CEDT_CHBS_VERSION_CXL11 (0)
+#define ACPI_CEDT_CHBS_VERSION_CXL20 (1)
+
+#define ACPI_CEDT_CHBS_LENGTH_CXL11 (0x2000)
+#define ACPI_CEDT_CHBS_LENGTH_CXL20 (0x10000)
+
+struct acpi_cedt_cfmws {
+	struct acpi_cedt_header header;
+	u32 reserved1;
+	u64 base_hpa;
+	u64 window_size;
+	u8 interleave_ways;
+	u8 interleave_arithmetic;
+	u16 reserved2;
+	u32 granularity;
+	u16 restrictions;
+	u16 qtg_id;
+	u32 interleave_targets[];
+};
+
+/* Values for Interleave Arithmetic field above */
+
+#define ACPI_CEDT_CFMWS_ARITHMETIC_MODULO (0)
+
+/* Values for Restrictions field above */
+
+#define ACPI_CEDT_CFMWS_RESTRICT_TYPE2 (1)
+#define ACPI_CEDT_CFMWS_RESTRICT_TYPE3 (1 << 1)
+#define ACPI_CEDT_CFMWS_RESTRICT_VOLATILE (1 << 2)
+#define ACPI_CEDT_CFMWS_RESTRICT_PMEM (1 << 3)
+#define ACPI_CEDT_CFMWS_RESTRICT_FIXED (1 << 4)
+#endif /* ACPI_CEDT_CHBS_VERSION_CXL20 */
+#endif /* __CXL_ACPI_H__ */

Re: [PATCH v5 1/6] cxl/acpi: Local definition of ACPICA infrastructure

[Rafael J. Wysocki]

On Sun, Jun 6, 2021 at 8:05 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> The recently released CXL specification change (ECN) for the CXL Fixed
> Memory Window Structure (CFMWS) extension to the CXL Early Discovery
> Table (CEDT) enables a large amount of functionality. It defines the
> root of a CXL memory topology and is needed for all OS flows for CXL
> provisioning CXL memory expanders. For ease of merging and tree
> management add the new ACPI definition locally (drivers/cxl/acpi.h) in
> such a way that they will not collide with the eventual arrival of the
> definitions through the ACPICA project to their final location
> (drivers/acpi/actbl1.h).

I've just applied the ACPICA series including this change which can be
made available as a forward-only branch in my tree, if that helps.

> The definitions in drivers/cxl/acpi.h can be dropped post -rc1.
>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Co-developed-by: Alison Schofield <alison.schofield@intel.com>
> Co-developed-by: Erik Kaneda <erik.kaneda@intel.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Erik Kaneda <erik.kaneda@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/acpi.h |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 drivers/cxl/acpi.h
>
> diff --git a/drivers/cxl/acpi.h b/drivers/cxl/acpi.h
> new file mode 100644
> index 000000000000..1482c19e7227
> --- /dev/null
> +++ b/drivers/cxl/acpi.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2021 Intel Corporation. */
> +#ifndef __CXL_ACPI_H__
> +#define __CXL_ACPI_H__
> +
> +#ifndef ACPI_CEDT_CHBS_VERSION_CXL20
> +/*
> + * NOTE: These definitions are temporary and to be deleted in v5.14-rc1
> + * when the identical definitions become available from
> + * include/acpi/actbl1.h.
> + */
> +
> +#define ACPI_CEDT_TYPE_CFMWS 1
> +#define ACPI_CEDT_TYPE_RESERVED 2
> +
> +#define ACPI_CEDT_CHBS_VERSION_CXL11 (0)
> +#define ACPI_CEDT_CHBS_VERSION_CXL20 (1)
> +
> +#define ACPI_CEDT_CHBS_LENGTH_CXL11 (0x2000)
> +#define ACPI_CEDT_CHBS_LENGTH_CXL20 (0x10000)
> +
> +struct acpi_cedt_cfmws {
> +       struct acpi_cedt_header header;
> +       u32 reserved1;
> +       u64 base_hpa;
> +       u64 window_size;
> +       u8 interleave_ways;
> +       u8 interleave_arithmetic;
> +       u16 reserved2;
> +       u32 granularity;
> +       u16 restrictions;
> +       u16 qtg_id;
> +       u32 interleave_targets[];
> +};
> +
> +/* Values for Interleave Arithmetic field above */
> +
> +#define ACPI_CEDT_CFMWS_ARITHMETIC_MODULO (0)
> +
> +/* Values for Restrictions field above */
> +
> +#define ACPI_CEDT_CFMWS_RESTRICT_TYPE2 (1)
> +#define ACPI_CEDT_CFMWS_RESTRICT_TYPE3 (1 << 1)
> +#define ACPI_CEDT_CFMWS_RESTRICT_VOLATILE (1 << 2)
> +#define ACPI_CEDT_CFMWS_RESTRICT_PMEM (1 << 3)
> +#define ACPI_CEDT_CFMWS_RESTRICT_FIXED (1 << 4)
> +#endif /* ACPI_CEDT_CHBS_VERSION_CXL20 */
> +#endif /* __CXL_ACPI_H__ */
>

Re: [PATCH v5 1/6] cxl/acpi: Local definition of ACPICA infrastructure

[Dan Williams]

On Mon, Jun 7, 2021 at 5:26 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sun, Jun 6, 2021 at 8:05 AM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > The recently released CXL specification change (ECN) for the CXL Fixed
> > Memory Window Structure (CFMWS) extension to the CXL Early Discovery
> > Table (CEDT) enables a large amount of functionality. It defines the
> > root of a CXL memory topology and is needed for all OS flows for CXL
> > provisioning CXL memory expanders. For ease of merging and tree
> > management add the new ACPI definition locally (drivers/cxl/acpi.h) in
> > such a way that they will not collide with the eventual arrival of the
> > definitions through the ACPICA project to their final location
> > (drivers/acpi/actbl1.h).
>
> I've just applied the ACPICA series including this change which can be
> made available as a forward-only branch in my tree, if that helps.

Yes, please, that would be my preference. When I created this patch
the concern was that a stable branch was possibly weeks away.

Re: [PATCH v5 1/6] cxl/acpi: Local definition of ACPICA infrastructure

[Dan Williams]

On Mon, Jun 7, 2021 at 10:03 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Mon, Jun 7, 2021 at 5:26 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Sun, Jun 6, 2021 at 8:05 AM Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > The recently released CXL specification change (ECN) for the CXL Fixed
> > > Memory Window Structure (CFMWS) extension to the CXL Early Discovery
> > > Table (CEDT) enables a large amount of functionality. It defines the
> > > root of a CXL memory topology and is needed for all OS flows for CXL
> > > provisioning CXL memory expanders. For ease of merging and tree
> > > management add the new ACPI definition locally (drivers/cxl/acpi.h) in
> > > such a way that they will not collide with the eventual arrival of the
> > > definitions through the ACPICA project to their final location
> > > (drivers/acpi/actbl1.h).
> >
> > I've just applied the ACPICA series including this change which can be
> > made available as a forward-only branch in my tree, if that helps.
>
> Yes, please, that would be my preference. When I created this patch
> the concern was that a stable branch was possibly weeks away.

Rafael, I see "4a2c1dcfaf59 ACPICA: Add the CFMWS structure definition
to the CEDT table" in your tree, I can safely assume that commit will
not rebase at this point? I'll likely rewind your acpica branch to
that point and merge there to avoid carrying any unrelated follow-on
commits.

Re: [PATCH v5 1/6] cxl/acpi: Local definition of ACPICA infrastructure

[Rafael J. Wysocki]

On Tue, Jun 8, 2021 at 8:13 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Mon, Jun 7, 2021 at 10:03 AM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > On Mon, Jun 7, 2021 at 5:26 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Sun, Jun 6, 2021 at 8:05 AM Dan Williams <dan.j.williams@intel.com> wrote:
> > > >
> > > > The recently released CXL specification change (ECN) for the CXL Fixed
> > > > Memory Window Structure (CFMWS) extension to the CXL Early Discovery
> > > > Table (CEDT) enables a large amount of functionality. It defines the
> > > > root of a CXL memory topology and is needed for all OS flows for CXL
> > > > provisioning CXL memory expanders. For ease of merging and tree
> > > > management add the new ACPI definition locally (drivers/cxl/acpi.h) in
> > > > such a way that they will not collide with the eventual arrival of the
> > > > definitions through the ACPICA project to their final location
> > > > (drivers/acpi/actbl1.h).
> > >
> > > I've just applied the ACPICA series including this change which can be
> > > made available as a forward-only branch in my tree, if that helps.
> >
> > Yes, please, that would be my preference. When I created this patch
> > the concern was that a stable branch was possibly weeks away.
>
> Rafael, I see "4a2c1dcfaf59 ACPICA: Add the CFMWS structure definition
> to the CEDT table" in your tree, I can safely assume that commit will
> not rebase at this point?

Yes, please.

> I'll likely rewind your acpica branch to
> that point and merge there to avoid carrying any unrelated follow-on
> commits.

Sure.

[PATCH v5 2/6] cxl/acpi: Introduce cxl_root, the root of a cxl_port topology

[Dan Williams]

While CXL builds upon the PCI software model for enumeration and
endpoint control, a static platform component is required to bootstrap
the CXL memory layout. Similar to how ACPI identifies root-level PCI
memory resources the ACPI identifies the address space and interleave
configuration for CXL Memory.

In addition to identifying the host bridges, ACPI is responsible for
enumerating the CXL memory space that can be addressed by downstream
decoders. This is similar to the requirement for ACPI to publish
resources reported by _CRS for PCI host bridges. Specifically ACPI
publishes a table, CXL Early Discovery Table (CEDT), which includes a
list of CXL Memory resource, CXL Fixed Memory Window Structures (CFMWS).

For now introduce the core infrastructure for a cxl_port hierarchy
starting with a root level anchor represented by the ACPI0017 device.

Follow on changes model support for the configurable decode capabilities
of cxl_port instances.

Co-developed-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-cxl |   11 ++
 drivers/cxl/Kconfig                     |   15 +++
 drivers/cxl/Makefile                    |    2 
 drivers/cxl/acpi.c                      |   39 ++++++++
 drivers/cxl/core.c                      |  160 +++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h                       |   23 ++++
 6 files changed, 250 insertions(+)
 create mode 100644 drivers/cxl/acpi.c

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 2fe7490ad6a8..fb996ced7629 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -24,3 +24,14 @@ Description:
 		(RO) "Persistent Only Capacity" as bytes. Represents the
 		identically named field in the Identify Memory Device Output
 		Payload in the CXL-2.0 specification.
+
+What:		/sys/bus/cxl/devices/portX/uport
+Date:		May, 2021
+KernelVersion:	v5.14
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		CXL port objects are enumerated from either a platform firmware
+		device (ACPI0017 and ACPI0016) or PCIe switch upstream port with
+		CXL component registers. The 'uport' symlink connects the CXL
+		portX object to the device that published the CXL port
+		capability.
diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
index 5483ba92b6da..d2573f6aef91 100644
--- a/drivers/cxl/Kconfig
+++ b/drivers/cxl/Kconfig
@@ -45,4 +45,19 @@ config CXL_MEM_RAW_COMMANDS
 	  potential impact to memory currently in use by the kernel.
 
 	  If developing CXL hardware or the driver say Y, otherwise say N.
+
+config CXL_ACPI
+	tristate "CXL ACPI: Platform Support"
+	depends on ACPI
+	help
+	  Enable support for host managed device memory (HDM) resources
+	  published by a platform's ACPI CXL memory layout description.  See
+	  Chapter 9.14.1 CXL Early Discovery Table (CEDT) in the CXL 2.0
+	  specification, and CXL Fixed Memory Window Structures (CEDT.CFMWS)
+	  (https://www.computeexpresslink.org/spec-landing). The CXL core
+	  consumes these resource to publish the root of a cxl_port decode
+	  hierarchy to map regions that represent System RAM, or Persistent
+	  Memory regions to be managed by LIBNVDIMM.
+
+	  If unsure say 'm'.
 endif
diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index d9d282dc15be..a29efb3e8ad2 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -1,7 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_CXL_BUS) += cxl_core.o
 obj-$(CONFIG_CXL_MEM) += cxl_pci.o
+obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
 
 ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
 cxl_core-y := core.o
 cxl_pci-y := pci.o
+cxl_acpi-y := acpi.o
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
new file mode 100644
index 000000000000..556d25ab6966
--- /dev/null
+++ b/drivers/cxl/acpi.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/acpi.h>
+#include "cxl.h"
+
+static int cxl_acpi_probe(struct platform_device *pdev)
+{
+	struct cxl_port *root_port;
+	struct device *host = &pdev->dev;
+
+	root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
+	if (IS_ERR(root_port))
+		return PTR_ERR(root_port);
+	dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
+
+	return 0;
+}
+
+static const struct acpi_device_id cxl_acpi_ids[] = {
+	{ "ACPI0017", 0 },
+	{ "", 0 },
+};
+MODULE_DEVICE_TABLE(acpi, cxl_acpi_ids);
+
+static struct platform_driver cxl_acpi_driver = {
+	.probe = cxl_acpi_probe,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.acpi_match_table = cxl_acpi_ids,
+	},
+};
+
+module_platform_driver(cxl_acpi_driver);
+MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(CXL);
diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index 853666d8a9f5..dbbb34618d7d 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -4,6 +4,8 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/slab.h>
+#include <linux/idr.h>
 #include "cxl.h"
 
 /**
@@ -13,6 +15,164 @@
  * point for cross-device interleave coordination through cxl ports.
  */
 
+static DEFINE_IDA(cxl_port_ida);
+
+static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	return sysfs_emit(buf, "%s\n", dev->type->name);
+}
+static DEVICE_ATTR_RO(devtype);
+
+static struct attribute *cxl_base_attributes[] = {
+	&dev_attr_devtype.attr,
+	NULL,
+};
+
+static struct attribute_group cxl_base_attribute_group = {
+	.attrs = cxl_base_attributes,
+};
+
+static void cxl_port_release(struct device *dev)
+{
+	struct cxl_port *port = to_cxl_port(dev);
+
+	ida_free(&cxl_port_ida, port->id);
+	kfree(port);
+}
+
+static const struct attribute_group *cxl_port_attribute_groups[] = {
+	&cxl_base_attribute_group,
+	NULL,
+};
+
+static const struct device_type cxl_port_type = {
+	.name = "cxl_port",
+	.release = cxl_port_release,
+	.groups = cxl_port_attribute_groups,
+};
+
+struct cxl_port *to_cxl_port(struct device *dev)
+{
+	if (dev_WARN_ONCE(dev, dev->type != &cxl_port_type,
+			  "not a cxl_port device\n"))
+		return NULL;
+	return container_of(dev, struct cxl_port, dev);
+}
+
+static void unregister_dev(void *dev)
+{
+	device_unregister(dev);
+}
+
+static void cxl_unlink_uport(void *_port)
+{
+	struct cxl_port *port = _port;
+
+	sysfs_remove_link(&port->dev.kobj, "uport");
+}
+
+static int devm_cxl_link_uport(struct device *host, struct cxl_port *port)
+{
+	int rc;
+
+	rc = sysfs_create_link(&port->dev.kobj, &port->uport->kobj, "uport");
+	if (rc)
+		return rc;
+	return devm_add_action_or_reset(host, cxl_unlink_uport, 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 *port;
+	struct device *dev;
+	int rc;
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return ERR_PTR(-ENOMEM);
+
+	rc = ida_alloc(&cxl_port_ida, GFP_KERNEL);
+	if (rc < 0)
+		goto err;
+	port->id = rc;
+
+	/*
+	 * The top-level cxl_port "cxl_root" does not have a cxl_port as
+	 * its parent and it does not have any corresponding component
+	 * registers as its decode is described by a fixed platform
+	 * description.
+	 */
+	dev = &port->dev;
+	if (parent_port)
+		dev->parent = &parent_port->dev;
+	else
+		dev->parent = uport;
+
+	port->uport = uport;
+	port->component_reg_phys = component_reg_phys;
+
+	device_initialize(dev);
+	device_set_pm_not_required(dev);
+	dev->bus = &cxl_bus_type;
+	dev->type = &cxl_port_type;
+
+	return port;
+
+err:
+	kfree(port);
+	return ERR_PTR(rc);
+}
+
+/**
+ * devm_cxl_add_port - register a cxl_port in CXL memory decode hierarchy
+ * @host: host device for devm operations
+ * @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
+ */
+struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
+				   resource_size_t component_reg_phys,
+				   struct cxl_port *parent_port)
+{
+	struct cxl_port *port;
+	struct device *dev;
+	int rc;
+
+	port = cxl_port_alloc(uport, component_reg_phys, parent_port);
+	if (IS_ERR(port))
+		return port;
+
+	dev = &port->dev;
+	if (parent_port)
+		rc = dev_set_name(dev, "port%d", port->id);
+	else
+		rc = dev_set_name(dev, "root%d", port->id);
+	if (rc)
+		goto err;
+
+	rc = device_add(dev);
+	if (rc)
+		goto err;
+
+	rc = devm_add_action_or_reset(host, unregister_dev, dev);
+	if (rc)
+		return ERR_PTR(rc);
+
+	rc = devm_cxl_link_uport(host, port);
+	if (rc)
+		return ERR_PTR(rc);
+
+	return port;
+
+err:
+	put_device(dev);
+	return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_GPL(devm_cxl_add_port);
+
 /**
  * cxl_probe_component_regs() - Detect CXL Component register blocks
  * @dev: Host device of the @base mapping
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 2c47e9cffd44..46c81165c210 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -145,5 +145,28 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 			struct cxl_device_regs *regs,
 			struct cxl_register_map *map);
 
+#define CXL_RESOURCE_NONE ((resource_size_t) -1)
+
+/**
+ * struct cxl_port - logical collection of upstream port devices and
+ *		     downstream port devices to construct a CXL memory
+ *		     decode hierarchy.
+ * @dev: this port's device
+ * @uport: PCI or platform device implementing the upstream port capability
+ * @id: id for port device-name
+ * @component_regs_phys: component register capability base address (optional)
+ */
+struct cxl_port {
+	struct device dev;
+	struct device *uport;
+	int id;
+	resource_size_t component_reg_phys;
+};
+
+struct cxl_port *to_cxl_port(struct device *dev);
+struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
+				   resource_size_t component_reg_phys,
+				   struct cxl_port *parent_port);
+
 extern struct bus_type cxl_bus_type;
 #endif /* __CXL_H__ */

Re: [PATCH v5 2/6] cxl/acpi: Introduce cxl_root, the root of a cxl_port topology

[Rafael J. Wysocki]

On Sun, Jun 6, 2021 at 8:05 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> While CXL builds upon the PCI software model for enumeration and
> endpoint control, a static platform component is required to bootstrap
> the CXL memory layout. Similar to how ACPI identifies root-level PCI
> memory resources the ACPI identifies the address space and interleave
> configuration for CXL Memory.
>
> In addition to identifying the host bridges, ACPI is responsible for
> enumerating the CXL memory space that can be addressed by downstream
> decoders. This is similar to the requirement for ACPI to publish
> resources reported by _CRS for PCI host bridges. Specifically ACPI
> publishes a table, CXL Early Discovery Table (CEDT), which includes a
> list of CXL Memory resource, CXL Fixed Memory Window Structures (CFMWS).
>
> For now introduce the core infrastructure for a cxl_port hierarchy
> starting with a root level anchor represented by the ACPI0017 device.
>
> Follow on changes model support for the configurable decode capabilities
> of cxl_port instances.
>
> Co-developed-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

From the ACPI perspective:

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |   11 ++
>  drivers/cxl/Kconfig                     |   15 +++
>  drivers/cxl/Makefile                    |    2
>  drivers/cxl/acpi.c                      |   39 ++++++++
>  drivers/cxl/core.c                      |  160 +++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h                       |   23 ++++
>  6 files changed, 250 insertions(+)
>  create mode 100644 drivers/cxl/acpi.c
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 2fe7490ad6a8..fb996ced7629 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -24,3 +24,14 @@ Description:
>                 (RO) "Persistent Only Capacity" as bytes. Represents the
>                 identically named field in the Identify Memory Device Output
>                 Payload in the CXL-2.0 specification.
> +
> +What:          /sys/bus/cxl/devices/portX/uport
> +Date:          May, 2021
> +KernelVersion: v5.14
> +Contact:       linux-cxl@vger.kernel.org
> +Description:
> +               CXL port objects are enumerated from either a platform firmware
> +               device (ACPI0017 and ACPI0016) or PCIe switch upstream port with
> +               CXL component registers. The 'uport' symlink connects the CXL
> +               portX object to the device that published the CXL port
> +               capability.
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 5483ba92b6da..d2573f6aef91 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -45,4 +45,19 @@ config CXL_MEM_RAW_COMMANDS
>           potential impact to memory currently in use by the kernel.
>
>           If developing CXL hardware or the driver say Y, otherwise say N.
> +
> +config CXL_ACPI
> +       tristate "CXL ACPI: Platform Support"
> +       depends on ACPI
> +       help
> +         Enable support for host managed device memory (HDM) resources
> +         published by a platform's ACPI CXL memory layout description.  See
> +         Chapter 9.14.1 CXL Early Discovery Table (CEDT) in the CXL 2.0
> +         specification, and CXL Fixed Memory Window Structures (CEDT.CFMWS)
> +         (https://www.computeexpresslink.org/spec-landing). The CXL core
> +         consumes these resource to publish the root of a cxl_port decode
> +         hierarchy to map regions that represent System RAM, or Persistent
> +         Memory regions to be managed by LIBNVDIMM.
> +
> +         If unsure say 'm'.
>  endif
> diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> index d9d282dc15be..a29efb3e8ad2 100644
> --- a/drivers/cxl/Makefile
> +++ b/drivers/cxl/Makefile
> @@ -1,7 +1,9 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_CXL_BUS) += cxl_core.o
>  obj-$(CONFIG_CXL_MEM) += cxl_pci.o
> +obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
>
>  ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
>  cxl_core-y := core.o
>  cxl_pci-y := pci.o
> +cxl_acpi-y := acpi.o
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> new file mode 100644
> index 000000000000..556d25ab6966
> --- /dev/null
> +++ b/drivers/cxl/acpi.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/acpi.h>
> +#include "cxl.h"
> +
> +static int cxl_acpi_probe(struct platform_device *pdev)
> +{
> +       struct cxl_port *root_port;
> +       struct device *host = &pdev->dev;
> +
> +       root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
> +       if (IS_ERR(root_port))
> +               return PTR_ERR(root_port);
> +       dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
> +
> +       return 0;
> +}
> +
> +static const struct acpi_device_id cxl_acpi_ids[] = {
> +       { "ACPI0017", 0 },
> +       { "", 0 },
> +};
> +MODULE_DEVICE_TABLE(acpi, cxl_acpi_ids);
> +
> +static struct platform_driver cxl_acpi_driver = {
> +       .probe = cxl_acpi_probe,
> +       .driver = {
> +               .name = KBUILD_MODNAME,
> +               .acpi_match_table = cxl_acpi_ids,
> +       },
> +};
> +
> +module_platform_driver(cxl_acpi_driver);
> +MODULE_LICENSE("GPL v2");
> +MODULE_IMPORT_NS(CXL);
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index 853666d8a9f5..dbbb34618d7d 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -4,6 +4,8 @@
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/slab.h>
> +#include <linux/idr.h>
>  #include "cxl.h"
>
>  /**
> @@ -13,6 +15,164 @@
>   * point for cross-device interleave coordination through cxl ports.
>   */
>
> +static DEFINE_IDA(cxl_port_ida);
> +
> +static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
> +                           char *buf)
> +{
> +       return sysfs_emit(buf, "%s\n", dev->type->name);
> +}
> +static DEVICE_ATTR_RO(devtype);
> +
> +static struct attribute *cxl_base_attributes[] = {
> +       &dev_attr_devtype.attr,
> +       NULL,
> +};
> +
> +static struct attribute_group cxl_base_attribute_group = {
> +       .attrs = cxl_base_attributes,
> +};
> +
> +static void cxl_port_release(struct device *dev)
> +{
> +       struct cxl_port *port = to_cxl_port(dev);
> +
> +       ida_free(&cxl_port_ida, port->id);
> +       kfree(port);
> +}
> +
> +static const struct attribute_group *cxl_port_attribute_groups[] = {
> +       &cxl_base_attribute_group,
> +       NULL,
> +};
> +
> +static const struct device_type cxl_port_type = {
> +       .name = "cxl_port",
> +       .release = cxl_port_release,
> +       .groups = cxl_port_attribute_groups,
> +};
> +
> +struct cxl_port *to_cxl_port(struct device *dev)
> +{
> +       if (dev_WARN_ONCE(dev, dev->type != &cxl_port_type,
> +                         "not a cxl_port device\n"))
> +               return NULL;
> +       return container_of(dev, struct cxl_port, dev);
> +}
> +
> +static void unregister_dev(void *dev)
> +{
> +       device_unregister(dev);
> +}
> +
> +static void cxl_unlink_uport(void *_port)
> +{
> +       struct cxl_port *port = _port;
> +
> +       sysfs_remove_link(&port->dev.kobj, "uport");
> +}
> +
> +static int devm_cxl_link_uport(struct device *host, struct cxl_port *port)
> +{
> +       int rc;
> +
> +       rc = sysfs_create_link(&port->dev.kobj, &port->uport->kobj, "uport");
> +       if (rc)
> +               return rc;
> +       return devm_add_action_or_reset(host, cxl_unlink_uport, 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 *port;
> +       struct device *dev;
> +       int rc;
> +
> +       port = kzalloc(sizeof(*port), GFP_KERNEL);
> +       if (!port)
> +               return ERR_PTR(-ENOMEM);
> +
> +       rc = ida_alloc(&cxl_port_ida, GFP_KERNEL);
> +       if (rc < 0)
> +               goto err;
> +       port->id = rc;
> +
> +       /*
> +        * The top-level cxl_port "cxl_root" does not have a cxl_port as
> +        * its parent and it does not have any corresponding component
> +        * registers as its decode is described by a fixed platform
> +        * description.
> +        */
> +       dev = &port->dev;
> +       if (parent_port)
> +               dev->parent = &parent_port->dev;
> +       else
> +               dev->parent = uport;
> +
> +       port->uport = uport;
> +       port->component_reg_phys = component_reg_phys;
> +
> +       device_initialize(dev);
> +       device_set_pm_not_required(dev);
> +       dev->bus = &cxl_bus_type;
> +       dev->type = &cxl_port_type;
> +
> +       return port;
> +
> +err:
> +       kfree(port);
> +       return ERR_PTR(rc);
> +}
> +
> +/**
> + * devm_cxl_add_port - register a cxl_port in CXL memory decode hierarchy
> + * @host: host device for devm operations
> + * @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
> + */
> +struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> +                                  resource_size_t component_reg_phys,
> +                                  struct cxl_port *parent_port)
> +{
> +       struct cxl_port *port;
> +       struct device *dev;
> +       int rc;
> +
> +       port = cxl_port_alloc(uport, component_reg_phys, parent_port);
> +       if (IS_ERR(port))
> +               return port;
> +
> +       dev = &port->dev;
> +       if (parent_port)
> +               rc = dev_set_name(dev, "port%d", port->id);
> +       else
> +               rc = dev_set_name(dev, "root%d", port->id);
> +       if (rc)
> +               goto err;
> +
> +       rc = device_add(dev);
> +       if (rc)
> +               goto err;
> +
> +       rc = devm_add_action_or_reset(host, unregister_dev, dev);
> +       if (rc)
> +               return ERR_PTR(rc);
> +
> +       rc = devm_cxl_link_uport(host, port);
> +       if (rc)
> +               return ERR_PTR(rc);
> +
> +       return port;
> +
> +err:
> +       put_device(dev);
> +       return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> +
>  /**
>   * cxl_probe_component_regs() - Detect CXL Component register blocks
>   * @dev: Host device of the @base mapping
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 2c47e9cffd44..46c81165c210 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -145,5 +145,28 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>                         struct cxl_device_regs *regs,
>                         struct cxl_register_map *map);
>
> +#define CXL_RESOURCE_NONE ((resource_size_t) -1)
> +
> +/**
> + * struct cxl_port - logical collection of upstream port devices and
> + *                  downstream port devices to construct a CXL memory
> + *                  decode hierarchy.
> + * @dev: this port's device
> + * @uport: PCI or platform device implementing the upstream port capability
> + * @id: id for port device-name
> + * @component_regs_phys: component register capability base address (optional)
> + */
> +struct cxl_port {
> +       struct device dev;
> +       struct device *uport;
> +       int id;
> +       resource_size_t component_reg_phys;
> +};
> +
> +struct cxl_port *to_cxl_port(struct device *dev);
> +struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> +                                  resource_size_t component_reg_phys,
> +                                  struct cxl_port *parent_port);
> +
>  extern struct bus_type cxl_bus_type;
>  #endif /* __CXL_H__ */
>

Re: [PATCH v5 2/6] cxl/acpi: Introduce cxl_root, the root of a cxl_port topology

[Ben Widawsky]

On 21-06-05 23:05:04, Dan Williams wrote:
> While CXL builds upon the PCI software model for enumeration and
> endpoint control, a static platform component is required to bootstrap
> the CXL memory layout. Similar to how ACPI identifies root-level PCI
> memory resources the ACPI identifies the address space and interleave
> configuration for CXL Memory.
> 
> In addition to identifying the host bridges, ACPI is responsible for
> enumerating the CXL memory space that can be addressed by downstream
> decoders. This is similar to the requirement for ACPI to publish
> resources reported by _CRS for PCI host bridges. Specifically ACPI
> publishes a table, CXL Early Discovery Table (CEDT), which includes a
> list of CXL Memory resource, CXL Fixed Memory Window Structures (CFMWS).
> 
> For now introduce the core infrastructure for a cxl_port hierarchy
> starting with a root level anchor represented by the ACPI0017 device.
> 
> Follow on changes model support for the configurable decode capabilities
> of cxl_port instances.
> 
> Co-developed-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |   11 ++
>  drivers/cxl/Kconfig                     |   15 +++
>  drivers/cxl/Makefile                    |    2 
>  drivers/cxl/acpi.c                      |   39 ++++++++
>  drivers/cxl/core.c                      |  160 +++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h                       |   23 ++++
>  6 files changed, 250 insertions(+)
>  create mode 100644 drivers/cxl/acpi.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 2fe7490ad6a8..fb996ced7629 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -24,3 +24,14 @@ Description:
>  		(RO) "Persistent Only Capacity" as bytes. Represents the
>  		identically named field in the Identify Memory Device Output
>  		Payload in the CXL-2.0 specification.
> +
> +What:		/sys/bus/cxl/devices/portX/uport
> +Date:		May, 2021
> +KernelVersion:	v5.14
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		CXL port objects are enumerated from either a platform firmware
> +		device (ACPI0017 and ACPI0016) or PCIe switch upstream port with
> +		CXL component registers. The 'uport' symlink connects the CXL
> +		portX object to the device that published the CXL port
> +		capability.
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 5483ba92b6da..d2573f6aef91 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -45,4 +45,19 @@ config CXL_MEM_RAW_COMMANDS
>  	  potential impact to memory currently in use by the kernel.
>  
>  	  If developing CXL hardware or the driver say Y, otherwise say N.
> +
> +config CXL_ACPI
> +	tristate "CXL ACPI: Platform Support"
> +	depends on ACPI
> +	help
> +	  Enable support for host managed device memory (HDM) resources
> +	  published by a platform's ACPI CXL memory layout description.  See
> +	  Chapter 9.14.1 CXL Early Discovery Table (CEDT) in the CXL 2.0
> +	  specification, and CXL Fixed Memory Window Structures (CEDT.CFMWS)
> +	  (https://www.computeexpresslink.org/spec-landing). The CXL core
> +	  consumes these resource to publish the root of a cxl_port decode
> +	  hierarchy to map regions that represent System RAM, or Persistent
> +	  Memory regions to be managed by LIBNVDIMM.
> +
> +	  If unsure say 'm'.
>  endif
> diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> index d9d282dc15be..a29efb3e8ad2 100644
> --- a/drivers/cxl/Makefile
> +++ b/drivers/cxl/Makefile
> @@ -1,7 +1,9 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_CXL_BUS) += cxl_core.o
>  obj-$(CONFIG_CXL_MEM) += cxl_pci.o
> +obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
>  
>  ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
>  cxl_core-y := core.o
>  cxl_pci-y := pci.o
> +cxl_acpi-y := acpi.o
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> new file mode 100644
> index 000000000000..556d25ab6966
> --- /dev/null
> +++ b/drivers/cxl/acpi.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/acpi.h>
> +#include "cxl.h"
> +
> +static int cxl_acpi_probe(struct platform_device *pdev)
> +{
> +	struct cxl_port *root_port;
> +	struct device *host = &pdev->dev;
> +
> +	root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
> +	if (IS_ERR(root_port))
> +		return PTR_ERR(root_port);
> +	dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id cxl_acpi_ids[] = {
> +	{ "ACPI0017", 0 },
> +	{ "", 0 },
> +};
> +MODULE_DEVICE_TABLE(acpi, cxl_acpi_ids);
> +
> +static struct platform_driver cxl_acpi_driver = {
> +	.probe = cxl_acpi_probe,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.acpi_match_table = cxl_acpi_ids,
> +	},
> +};
> +
> +module_platform_driver(cxl_acpi_driver);
> +MODULE_LICENSE("GPL v2");
> +MODULE_IMPORT_NS(CXL);
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index 853666d8a9f5..dbbb34618d7d 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -4,6 +4,8 @@
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/slab.h>
> +#include <linux/idr.h>
>  #include "cxl.h"
>  
>  /**
> @@ -13,6 +15,164 @@
>   * point for cross-device interleave coordination through cxl ports.
>   */
>  
> +static DEFINE_IDA(cxl_port_ida);
> +
> +static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	return sysfs_emit(buf, "%s\n", dev->type->name);
> +}
> +static DEVICE_ATTR_RO(devtype);
> +
> +static struct attribute *cxl_base_attributes[] = {
> +	&dev_attr_devtype.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group cxl_base_attribute_group = {
> +	.attrs = cxl_base_attributes,
> +};
> +
> +static void cxl_port_release(struct device *dev)
> +{
> +	struct cxl_port *port = to_cxl_port(dev);
> +
> +	ida_free(&cxl_port_ida, port->id);
> +	kfree(port);
> +}
> +
> +static const struct attribute_group *cxl_port_attribute_groups[] = {
> +	&cxl_base_attribute_group,
> +	NULL,
> +};
> +
> +static const struct device_type cxl_port_type = {
> +	.name = "cxl_port",
> +	.release = cxl_port_release,
> +	.groups = cxl_port_attribute_groups,
> +};
> +
> +struct cxl_port *to_cxl_port(struct device *dev)
> +{
> +	if (dev_WARN_ONCE(dev, dev->type != &cxl_port_type,
> +			  "not a cxl_port device\n"))
> +		return NULL;
> +	return container_of(dev, struct cxl_port, dev);
> +}
> +
> +static void unregister_dev(void *dev)
> +{
> +	device_unregister(dev);
> +}
> +
> +static void cxl_unlink_uport(void *_port)
> +{
> +	struct cxl_port *port = _port;
> +
> +	sysfs_remove_link(&port->dev.kobj, "uport");
> +}
> +
> +static int devm_cxl_link_uport(struct device *host, struct cxl_port *port)
> +{
> +	int rc;
> +
> +	rc = sysfs_create_link(&port->dev.kobj, &port->uport->kobj, "uport");
> +	if (rc)
> +		return rc;
> +	return devm_add_action_or_reset(host, cxl_unlink_uport, 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 *port;
> +	struct device *dev;
> +	int rc;
> +
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rc = ida_alloc(&cxl_port_ida, GFP_KERNEL);
> +	if (rc < 0)
> +		goto err;
> +	port->id = rc;
> +
> +	/*
> +	 * The top-level cxl_port "cxl_root" does not have a cxl_port as
> +	 * its parent and it does not have any corresponding component
> +	 * registers as its decode is described by a fixed platform
> +	 * description.
> +	 */
> +	dev = &port->dev;
> +	if (parent_port)
> +		dev->parent = &parent_port->dev;
> +	else
> +		dev->parent = uport;
> +
> +	port->uport = uport;
> +	port->component_reg_phys = component_reg_phys;
> +
> +	device_initialize(dev);
> +	device_set_pm_not_required(dev);
> +	dev->bus = &cxl_bus_type;
> +	dev->type = &cxl_port_type;
> +
> +	return port;
> +
> +err:
> +	kfree(port);
> +	return ERR_PTR(rc);
> +}
> +
> +/**
> + * devm_cxl_add_port - register a cxl_port in CXL memory decode hierarchy
> + * @host: host device for devm operations
> + * @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
> + */
> +struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> +				   resource_size_t component_reg_phys,
> +				   struct cxl_port *parent_port)
> +{
> +	struct cxl_port *port;
> +	struct device *dev;
> +	int rc;
> +
> +	port = cxl_port_alloc(uport, component_reg_phys, parent_port);
> +	if (IS_ERR(port))
> +		return port;
> +
> +	dev = &port->dev;
> +	if (parent_port)
> +		rc = dev_set_name(dev, "port%d", port->id);
> +	else
> +		rc = dev_set_name(dev, "root%d", port->id);
> +	if (rc)
> +		goto err;
> +
> +	rc = device_add(dev);
> +	if (rc)
> +		goto err;
> +
> +	rc = devm_add_action_or_reset(host, unregister_dev, dev);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	rc = devm_cxl_link_uport(host, port);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	return port;
> +
> +err:
> +	put_device(dev);
> +	return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> +
>  /**
>   * cxl_probe_component_regs() - Detect CXL Component register blocks
>   * @dev: Host device of the @base mapping
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 2c47e9cffd44..46c81165c210 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -145,5 +145,28 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>  			struct cxl_device_regs *regs,
>  			struct cxl_register_map *map);
>  
> +#define CXL_RESOURCE_NONE ((resource_size_t) -1)
> +
> +/**
> + * struct cxl_port - logical collection of upstream port devices and
> + *		     downstream port devices to construct a CXL memory
> + *		     decode hierarchy.
> + * @dev: this port's device
> + * @uport: PCI or platform device implementing the upstream port capability
> + * @id: id for port device-name
> + * @component_regs_phys: component register capability base address (optional)
> + */
> +struct cxl_port {
> +	struct device dev;
> +	struct device *uport;
> +	int id;
> +	resource_size_t component_reg_phys;
> +};
> +

I think the new kernel doc info needs to be added in the
Documentation/driver-api/cxl/ somewhere. We haven't yet told sphinx about this
file.

> +struct cxl_port *to_cxl_port(struct device *dev);
> +struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> +				   resource_size_t component_reg_phys,
> +				   struct cxl_port *parent_port);
> +
>  extern struct bus_type cxl_bus_type;
>  #endif /* __CXL_H__ */
> 

Re: [PATCH v5 2/6] cxl/acpi: Introduce cxl_root, the root of a cxl_port topology

[Jonathan Cameron]

On Sat, 5 Jun 2021 23:05:04 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> While CXL builds upon the PCI software model for enumeration and
> endpoint control, a static platform component is required to bootstrap
> the CXL memory layout. Similar to how ACPI identifies root-level PCI
> memory resources the ACPI identifies the address space and interleave
> configuration for CXL Memory.
> 
> In addition to identifying the host bridges, ACPI is responsible for
> enumerating the CXL memory space that can be addressed by downstream
> decoders. This is similar to the requirement for ACPI to publish
> resources reported by _CRS for PCI host bridges. Specifically ACPI
> publishes a table, CXL Early Discovery Table (CEDT), which includes a
> list of CXL Memory resource, CXL Fixed Memory Window Structures (CFMWS).
> 
> For now introduce the core infrastructure for a cxl_port hierarchy
> starting with a root level anchor represented by the ACPI0017 device.
> 
> Follow on changes model support for the configurable decode capabilities
> of cxl_port instances.
> 
> Co-developed-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Two trivial comments inline.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |   11 ++
>  drivers/cxl/Kconfig                     |   15 +++
>  drivers/cxl/Makefile                    |    2 
>  drivers/cxl/acpi.c                      |   39 ++++++++
>  drivers/cxl/core.c                      |  160 +++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h                       |   23 ++++
>  6 files changed, 250 insertions(+)
>  create mode 100644 drivers/cxl/acpi.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 2fe7490ad6a8..fb996ced7629 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -24,3 +24,14 @@ Description:
>  		(RO) "Persistent Only Capacity" as bytes. Represents the
>  		identically named field in the Identify Memory Device Output
>  		Payload in the CXL-2.0 specification.
> +
> +What:		/sys/bus/cxl/devices/portX/uport
> +Date:		May, 2021
> +KernelVersion:	v5.14
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		CXL port objects are enumerated from either a platform firmware
> +		device (ACPI0017 and ACPI0016) or PCIe switch upstream port with
> +		CXL component registers. The 'uport' symlink connects the CXL
> +		portX object to the device that published the CXL port
> +		capability.

Is this a complete list of ABI added? Looks like we also add devtype
attribute in this series.

Mind you I just realized I didn't document the proposed CDAT file either yet.


> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 5483ba92b6da..d2573f6aef91 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -45,4 +45,19 @@ config CXL_MEM_RAW_COMMANDS
>  	  potential impact to memory currently in use by the kernel.
>  
>  	  If developing CXL hardware or the driver say Y, otherwise say N.
> +
> +config CXL_ACPI
> +	tristate "CXL ACPI: Platform Support"
> +	depends on ACPI
> +	help
> +	  Enable support for host managed device memory (HDM) resources
> +	  published by a platform's ACPI CXL memory layout description.  See
> +	  Chapter 9.14.1 CXL Early Discovery Table (CEDT) in the CXL 2.0
> +	  specification, and CXL Fixed Memory Window Structures (CEDT.CFMWS)
> +	  (https://www.computeexpresslink.org/spec-landing). The CXL core
> +	  consumes these resource to publish the root of a cxl_port decode
> +	  hierarchy to map regions that represent System RAM, or Persistent
> +	  Memory regions to be managed by LIBNVDIMM.
> +
> +	  If unsure say 'm'.
>  endif
> diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> index d9d282dc15be..a29efb3e8ad2 100644
> --- a/drivers/cxl/Makefile
> +++ b/drivers/cxl/Makefile
> @@ -1,7 +1,9 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_CXL_BUS) += cxl_core.o
>  obj-$(CONFIG_CXL_MEM) += cxl_pci.o
> +obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
>  
>  ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
>  cxl_core-y := core.o
>  cxl_pci-y := pci.o
> +cxl_acpi-y := acpi.o
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> new file mode 100644
> index 000000000000..556d25ab6966
> --- /dev/null
> +++ b/drivers/cxl/acpi.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/acpi.h>
> +#include "cxl.h"
> +
> +static int cxl_acpi_probe(struct platform_device *pdev)
> +{
> +	struct cxl_port *root_port;
> +	struct device *host = &pdev->dev;
> +
> +	root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
> +	if (IS_ERR(root_port))
> +		return PTR_ERR(root_port);
> +	dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id cxl_acpi_ids[] = {
> +	{ "ACPI0017", 0 },
> +	{ "", 0 },
> +};
> +MODULE_DEVICE_TABLE(acpi, cxl_acpi_ids);
> +
> +static struct platform_driver cxl_acpi_driver = {
> +	.probe = cxl_acpi_probe,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.acpi_match_table = cxl_acpi_ids,
> +	},
> +};
> +
> +module_platform_driver(cxl_acpi_driver);
> +MODULE_LICENSE("GPL v2");
> +MODULE_IMPORT_NS(CXL);
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index 853666d8a9f5..dbbb34618d7d 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -4,6 +4,8 @@
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/slab.h>
> +#include <linux/idr.h>
>  #include "cxl.h"
>  
>  /**
> @@ -13,6 +15,164 @@
>   * point for cross-device interleave coordination through cxl ports.
>   */
>  
> +static DEFINE_IDA(cxl_port_ida);
> +
> +static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	return sysfs_emit(buf, "%s\n", dev->type->name);

I guess it's really small so doesn't matter that much, but not so nice
that we are gaining multiple instances of this same function.

> +}
> +static DEVICE_ATTR_RO(devtype);
> +
> +static struct attribute *cxl_base_attributes[] = {
> +	&dev_attr_devtype.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group cxl_base_attribute_group = {
> +	.attrs = cxl_base_attributes,
> +};
> +
> +static void cxl_port_release(struct device *dev)
> +{
> +	struct cxl_port *port = to_cxl_port(dev);
> +
> +	ida_free(&cxl_port_ida, port->id);
> +	kfree(port);
> +}
> +
> +static const struct attribute_group *cxl_port_attribute_groups[] = {
> +	&cxl_base_attribute_group,
> +	NULL,
> +};
> +
> +static const struct device_type cxl_port_type = {
> +	.name = "cxl_port",
> +	.release = cxl_port_release,
> +	.groups = cxl_port_attribute_groups,
> +};
> +
> +struct cxl_port *to_cxl_port(struct device *dev)
> +{
> +	if (dev_WARN_ONCE(dev, dev->type != &cxl_port_type,
> +			  "not a cxl_port device\n"))
> +		return NULL;
> +	return container_of(dev, struct cxl_port, dev);
> +}
> +
> +static void unregister_dev(void *dev)
> +{
> +	device_unregister(dev);
> +}
> +
> +static void cxl_unlink_uport(void *_port)
> +{
> +	struct cxl_port *port = _port;
> +
> +	sysfs_remove_link(&port->dev.kobj, "uport");
> +}
> +
> +static int devm_cxl_link_uport(struct device *host, struct cxl_port *port)
> +{
> +	int rc;
> +
> +	rc = sysfs_create_link(&port->dev.kobj, &port->uport->kobj, "uport");
> +	if (rc)
> +		return rc;
> +	return devm_add_action_or_reset(host, cxl_unlink_uport, 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 *port;
> +	struct device *dev;
> +	int rc;
> +
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rc = ida_alloc(&cxl_port_ida, GFP_KERNEL);
> +	if (rc < 0)
> +		goto err;
> +	port->id = rc;
> +
> +	/*
> +	 * The top-level cxl_port "cxl_root" does not have a cxl_port as
> +	 * its parent and it does not have any corresponding component
> +	 * registers as its decode is described by a fixed platform
> +	 * description.
> +	 */
> +	dev = &port->dev;
> +	if (parent_port)
> +		dev->parent = &parent_port->dev;
> +	else
> +		dev->parent = uport;
> +
> +	port->uport = uport;
> +	port->component_reg_phys = component_reg_phys;
> +
> +	device_initialize(dev);
> +	device_set_pm_not_required(dev);
> +	dev->bus = &cxl_bus_type;
> +	dev->type = &cxl_port_type;
> +
> +	return port;
> +
> +err:
> +	kfree(port);
> +	return ERR_PTR(rc);
> +}
> +
> +/**
> + * devm_cxl_add_port - register a cxl_port in CXL memory decode hierarchy
> + * @host: host device for devm operations
> + * @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
> + */
> +struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> +				   resource_size_t component_reg_phys,
> +				   struct cxl_port *parent_port)
> +{
> +	struct cxl_port *port;
> +	struct device *dev;
> +	int rc;
> +
> +	port = cxl_port_alloc(uport, component_reg_phys, parent_port);
> +	if (IS_ERR(port))
> +		return port;
> +
> +	dev = &port->dev;
> +	if (parent_port)
> +		rc = dev_set_name(dev, "port%d", port->id);
> +	else
> +		rc = dev_set_name(dev, "root%d", port->id);
> +	if (rc)
> +		goto err;
> +
> +	rc = device_add(dev);
> +	if (rc)
> +		goto err;
> +
> +	rc = devm_add_action_or_reset(host, unregister_dev, dev);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	rc = devm_cxl_link_uport(host, port);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	return port;
> +
> +err:
> +	put_device(dev);
> +	return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> +
>  /**
>   * cxl_probe_component_regs() - Detect CXL Component register blocks
>   * @dev: Host device of the @base mapping
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 2c47e9cffd44..46c81165c210 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -145,5 +145,28 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>  			struct cxl_device_regs *regs,
>  			struct cxl_register_map *map);
>  
> +#define CXL_RESOURCE_NONE ((resource_size_t) -1)
> +
> +/**
> + * struct cxl_port - logical collection of upstream port devices and
> + *		     downstream port devices to construct a CXL memory
> + *		     decode hierarchy.
> + * @dev: this port's device
> + * @uport: PCI or platform device implementing the upstream port capability
> + * @id: id for port device-name
> + * @component_regs_phys: component register capability base address (optional)
> + */
> +struct cxl_port {
> +	struct device dev;
> +	struct device *uport;
> +	int id;
> +	resource_size_t component_reg_phys;
> +};
> +
> +struct cxl_port *to_cxl_port(struct device *dev);
> +struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> +				   resource_size_t component_reg_phys,
> +				   struct cxl_port *parent_port);
> +
>  extern struct bus_type cxl_bus_type;
>  #endif /* __CXL_H__ */
> 

Re: [PATCH v5 2/6] cxl/acpi: Introduce cxl_root, the root of a cxl_port topology

[Dan Williams]

On Tue, Jun 8, 2021 at 4:28 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Sat, 5 Jun 2021 23:05:04 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > While CXL builds upon the PCI software model for enumeration and
> > endpoint control, a static platform component is required to bootstrap
> > the CXL memory layout. Similar to how ACPI identifies root-level PCI
> > memory resources the ACPI identifies the address space and interleave
> > configuration for CXL Memory.
> >
> > In addition to identifying the host bridges, ACPI is responsible for
> > enumerating the CXL memory space that can be addressed by downstream
> > decoders. This is similar to the requirement for ACPI to publish
> > resources reported by _CRS for PCI host bridges. Specifically ACPI
> > publishes a table, CXL Early Discovery Table (CEDT), which includes a
> > list of CXL Memory resource, CXL Fixed Memory Window Structures (CFMWS).
> >
> > For now introduce the core infrastructure for a cxl_port hierarchy
> > starting with a root level anchor represented by the ACPI0017 device.
> >
> > Follow on changes model support for the configurable decode capabilities
> > of cxl_port instances.
> >
> > Co-developed-by: Alison Schofield <alison.schofield@intel.com>
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Two trivial comments inline.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>

Thanks, as always, for taking a look.

> > ---
> >  Documentation/ABI/testing/sysfs-bus-cxl |   11 ++
> >  drivers/cxl/Kconfig                     |   15 +++
> >  drivers/cxl/Makefile                    |    2
> >  drivers/cxl/acpi.c                      |   39 ++++++++
> >  drivers/cxl/core.c                      |  160 +++++++++++++++++++++++++++++++
> >  drivers/cxl/cxl.h                       |   23 ++++
> >  6 files changed, 250 insertions(+)
> >  create mode 100644 drivers/cxl/acpi.c
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > index 2fe7490ad6a8..fb996ced7629 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cxl
> > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > @@ -24,3 +24,14 @@ Description:
> >               (RO) "Persistent Only Capacity" as bytes. Represents the
> >               identically named field in the Identify Memory Device Output
> >               Payload in the CXL-2.0 specification.
> > +
> > +What:                /sys/bus/cxl/devices/portX/uport
> > +Date:                May, 2021
> > +KernelVersion:       v5.14
> > +Contact:     linux-cxl@vger.kernel.org
> > +Description:
> > +             CXL port objects are enumerated from either a platform firmware
> > +             device (ACPI0017 and ACPI0016) or PCIe switch upstream port with
> > +             CXL component registers. The 'uport' symlink connects the CXL
> > +             portX object to the device that published the CXL port
> > +             capability.
>
> Is this a complete list of ABI added? Looks like we also add devtype
> attribute in this series.

devtype is the only missing one, I'll add it.

> Mind you I just realized I didn't document the proposed CDAT file either yet.

Hey, we all need prompts to add documentation. I only remembered to
add these here because you commented on it last time.

> > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> > index 5483ba92b6da..d2573f6aef91 100644
> > --- a/drivers/cxl/Kconfig
> > +++ b/drivers/cxl/Kconfig
> > @@ -45,4 +45,19 @@ config CXL_MEM_RAW_COMMANDS
> >         potential impact to memory currently in use by the kernel.
> >
> >         If developing CXL hardware or the driver say Y, otherwise say N.
> > +
> > +config CXL_ACPI
> > +     tristate "CXL ACPI: Platform Support"
> > +     depends on ACPI
> > +     help
> > +       Enable support for host managed device memory (HDM) resources
> > +       published by a platform's ACPI CXL memory layout description.  See
> > +       Chapter 9.14.1 CXL Early Discovery Table (CEDT) in the CXL 2.0
> > +       specification, and CXL Fixed Memory Window Structures (CEDT.CFMWS)
> > +       (https://www.computeexpresslink.org/spec-landing). The CXL core
> > +       consumes these resource to publish the root of a cxl_port decode
> > +       hierarchy to map regions that represent System RAM, or Persistent
> > +       Memory regions to be managed by LIBNVDIMM.
> > +
> > +       If unsure say 'm'.
> >  endif
> > diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> > index d9d282dc15be..a29efb3e8ad2 100644
> > --- a/drivers/cxl/Makefile
> > +++ b/drivers/cxl/Makefile
> > @@ -1,7 +1,9 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_CXL_BUS) += cxl_core.o
> >  obj-$(CONFIG_CXL_MEM) += cxl_pci.o
> > +obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
> >
> >  ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
> >  cxl_core-y := core.o
> >  cxl_pci-y := pci.o
> > +cxl_acpi-y := acpi.o
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > new file mode 100644
> > index 000000000000..556d25ab6966
> > --- /dev/null
> > +++ b/drivers/cxl/acpi.c
> > @@ -0,0 +1,39 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/acpi.h>
> > +#include "cxl.h"
> > +
> > +static int cxl_acpi_probe(struct platform_device *pdev)
> > +{
> > +     struct cxl_port *root_port;
> > +     struct device *host = &pdev->dev;
> > +
> > +     root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
> > +     if (IS_ERR(root_port))
> > +             return PTR_ERR(root_port);
> > +     dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct acpi_device_id cxl_acpi_ids[] = {
> > +     { "ACPI0017", 0 },
> > +     { "", 0 },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, cxl_acpi_ids);
> > +
> > +static struct platform_driver cxl_acpi_driver = {
> > +     .probe = cxl_acpi_probe,
> > +     .driver = {
> > +             .name = KBUILD_MODNAME,
> > +             .acpi_match_table = cxl_acpi_ids,
> > +     },
> > +};
> > +
> > +module_platform_driver(cxl_acpi_driver);
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_IMPORT_NS(CXL);
> > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > index 853666d8a9f5..dbbb34618d7d 100644
> > --- a/drivers/cxl/core.c
> > +++ b/drivers/cxl/core.c
> > @@ -4,6 +4,8 @@
> >  #include <linux/device.h>
> >  #include <linux/module.h>
> >  #include <linux/pci.h>
> > +#include <linux/slab.h>
> > +#include <linux/idr.h>
> >  #include "cxl.h"
> >
> >  /**
> > @@ -13,6 +15,164 @@
> >   * point for cross-device interleave coordination through cxl ports.
> >   */
> >
> > +static DEFINE_IDA(cxl_port_ida);
> > +
> > +static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
> > +                         char *buf)
> > +{
> > +     return sysfs_emit(buf, "%s\n", dev->type->name);
>
> I guess it's really small so doesn't matter that much, but not so nice
> that we are gaining multiple instances of this same function.
>

This is only the second one unless I'm missing something. I do think
it might be worth placing a common definition in the driver-core.
Although it might be the case that I'm the only one that thinks having
devtype as both an attribute and a uevent variable (DEVTYPE) is
worthwhile. All the other variables in a dev_uevent() appear somewhere
in sysfs, so that's a decent argument in my opinion to make devtype a
core attribute.

[PATCH v5 3/6] cxl/Kconfig: Default drivers to CONFIG_CXL_BUS

[Dan Williams]

CONFIG_CXL_BUS is default 'n' as expected for new functionality. When
that is enabled do not make the end user hunt for all the expected
sub-options to enable. For example CONFIG_CXL_BUS without CONFIG_CXL_MEM
is an odd/expert configuration, so is CONFIG_CXL_MEM without
CONFIG_CXL_ACPI (on ACPI capable platforms). Default CONFIG_CXL_MEM and
CONFIG_CXL_ACPI to CONFIG_CXL_BUS.

Acked-by: Ben Widawsky <ben.widawsky@intel.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/Kconfig |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
index d2573f6aef91..1a44b173dcbc 100644
--- a/drivers/cxl/Kconfig
+++ b/drivers/cxl/Kconfig
@@ -15,6 +15,7 @@ if CXL_BUS
 
 config CXL_MEM
 	tristate "CXL.mem: Memory Devices"
+	default CXL_BUS
 	help
 	  The CXL.mem protocol allows a device to act as a provider of
 	  "System RAM" and/or "Persistent Memory" that is fully coherent
@@ -49,6 +50,7 @@ config CXL_MEM_RAW_COMMANDS
 config CXL_ACPI
 	tristate "CXL ACPI: Platform Support"
 	depends on ACPI
+	default CXL_BUS
 	help
 	  Enable support for host managed device memory (HDM) resources
 	  published by a platform's ACPI CXL memory layout description.  See

[PATCH v5 4/6] cxl/acpi: Add downstream port data to cxl_port instances

[Dan Williams]

In preparation for infrastructure that enumerates and configures the CXL
decode mechanism of an upstream port to its downstream ports, add a
representation for a CXL downstream port.

On ACPI systems the top-most logical downstream ports in the hierarchy
are the host bridges (ACPI0016 devices) that decode the memory windows
described by the CXL Early Discovery Table Fixed Memory Window
Structures (CEDT.CFMWS).

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-cxl |   13 ++++
 drivers/cxl/acpi.c                      |   44 ++++++++++++++
 drivers/cxl/core.c                      |   95 ++++++++++++++++++++++++++++++-
 drivers/cxl/cxl.h                       |   21 +++++++
 4 files changed, 169 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index fb996ced7629..0cb31b7ad17b 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -35,3 +35,16 @@ Description:
 		CXL component registers. The 'uport' symlink connects the CXL
 		portX object to the device that published the CXL port
 		capability.
+
+What:		/sys/bus/cxl/devices/portX/dportY
+Date:		June, 2021
+KernelVersion:	v5.14
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		CXL port objects are enumerated from either a platform firmware
+		device (ACPI0017 and ACPI0016) or PCIe switch upstream port with
+		CXL component registers. The 'dportY' symlink identifies one or
+		more downstream ports that the upstream port may target in its
+		decode of CXL memory resources.  The 'Y' integer reflects the
+		hardware port unique-id used in the hardware decoder target
+		list.
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 556d25ab6966..0ae7464b603d 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -5,19 +5,61 @@
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/acpi.h>
+#include <linux/pci.h>
 #include "cxl.h"
 
+static struct acpi_device *to_cxl_host_bridge(struct device *dev)
+{
+	struct acpi_device *adev = to_acpi_device(dev);
+
+	if (strcmp(acpi_device_hid(adev), "ACPI0016") == 0)
+		return adev;
+	return NULL;
+}
+
+static int add_host_bridge_dport(struct device *match, void *arg)
+{
+	int rc;
+	acpi_status status;
+	unsigned long long uid;
+	struct cxl_port *root_port = arg;
+	struct device *host = root_port->dev.parent;
+	struct acpi_device *bridge = to_cxl_host_bridge(match);
+
+	if (!bridge)
+		return 0;
+
+	status = acpi_evaluate_integer(bridge->handle, METHOD_NAME__UID, NULL,
+				       &uid);
+	if (status != AE_OK) {
+		dev_err(host, "unable to retrieve _UID of %s\n",
+			dev_name(match));
+		return -ENODEV;
+	}
+
+	rc = cxl_add_dport(root_port, match, uid, CXL_RESOURCE_NONE);
+	if (rc) {
+		dev_err(host, "failed to add downstream port: %s\n",
+			dev_name(match));
+		return rc;
+	}
+	dev_dbg(host, "add dport%llu: %s\n", uid, dev_name(match));
+	return 0;
+}
+
 static int cxl_acpi_probe(struct platform_device *pdev)
 {
 	struct cxl_port *root_port;
 	struct device *host = &pdev->dev;
+	struct acpi_device *adev = ACPI_COMPANION(host);
 
 	root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
 	if (IS_ERR(root_port))
 		return PTR_ERR(root_port);
 	dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
 
-	return 0;
+	return bus_for_each_dev(adev->dev.bus, NULL, root_port,
+				add_host_bridge_dport);
 }
 
 static const struct acpi_device_id cxl_acpi_ids[] = {
diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index dbbb34618d7d..4418b30cce4f 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -33,10 +33,22 @@ static struct attribute_group cxl_base_attribute_group = {
 	.attrs = cxl_base_attributes,
 };
 
+static void cxl_dport_release(struct cxl_dport *dport)
+{
+	put_device(dport->dport);
+	list_del(&dport->list);
+	kfree(dport);
+}
+
 static void cxl_port_release(struct device *dev)
 {
 	struct cxl_port *port = to_cxl_port(dev);
+	struct cxl_dport *dport, *_d;
 
+	device_lock(dev);
+	list_for_each_entry_safe(dport, _d, &port->dports, list)
+		cxl_dport_release(dport);
+	device_unlock(dev);
 	ida_free(&cxl_port_ida, port->id);
 	kfree(port);
 }
@@ -60,9 +72,22 @@ struct cxl_port *to_cxl_port(struct device *dev)
 	return container_of(dev, struct cxl_port, dev);
 }
 
-static void unregister_dev(void *dev)
+static void unregister_port(void *_port)
 {
-	device_unregister(dev);
+	struct cxl_port *port = _port;
+	struct cxl_dport *dport;
+
+	device_lock(&port->dev);
+	list_for_each_entry(dport, &port->dports, list) {
+		char link_name[CXL_TARGET_STRLEN];
+
+		if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d",
+			     dport->port_id) >= CXL_TARGET_STRLEN)
+			continue;
+		sysfs_remove_link(&port->dev.kobj, link_name);
+	}
+	device_unlock(&port->dev);
+	device_unregister(&port->dev);
 }
 
 static void cxl_unlink_uport(void *_port)
@@ -113,6 +138,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
 
 	port->uport = uport;
 	port->component_reg_phys = component_reg_phys;
+	INIT_LIST_HEAD(&port->dports);
 
 	device_initialize(dev);
 	device_set_pm_not_required(dev);
@@ -157,7 +183,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
 	if (rc)
 		goto err;
 
-	rc = devm_add_action_or_reset(host, unregister_dev, dev);
+	rc = devm_add_action_or_reset(host, unregister_port, port);
 	if (rc)
 		return ERR_PTR(rc);
 
@@ -173,6 +199,69 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
 }
 EXPORT_SYMBOL_GPL(devm_cxl_add_port);
 
+static int add_dport(struct cxl_port *port, struct cxl_dport *new)
+{
+	struct cxl_dport *dport, *dup = NULL;
+
+	device_lock(&port->dev);
+	list_for_each_entry (dport, &port->dports, list)
+		if (new->port_id == dport->port_id) {
+			dup = dport;
+			goto err;
+		}
+	list_add_tail(&new->list, &port->dports);
+	device_unlock(&port->dev);
+
+	return 0;
+
+err:
+	device_unlock(&port->dev);
+	dev_err(&port->dev,
+		"unable to add dport%d-%s non-unique port id (%s)\n",
+		new->port_id, dev_name(new->dport), dev_name(dup->dport));
+	return -EEXIST;
+}
+
+/*
+ * Append downstream port data to a cxl_port, 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)
+{
+	char link_name[CXL_TARGET_STRLEN];
+	struct cxl_dport *dport;
+	int rc;
+
+	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
+	    CXL_TARGET_STRLEN)
+		return -EINVAL;
+
+	dport = kzalloc(sizeof(*dport), GFP_KERNEL);
+	if (!dport)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&dport->list);
+	dport->dport = get_device(dport_dev);
+	dport->port_id = port_id;
+	dport->component_reg_phys = component_reg_phys;
+	dport->port = port;
+
+	rc = add_dport(port, dport);
+	if (rc)
+		goto err;
+
+	rc = sysfs_create_link(&port->dev.kobj, &dport_dev->kobj, link_name);
+	if (rc)
+		goto err;
+
+	return 0;
+err:
+	cxl_dport_release(dport);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(cxl_add_dport);
+
 /**
  * cxl_probe_component_regs() - Detect CXL Component register blocks
  * @dev: Host device of the @base mapping
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 46c81165c210..0b89fcfe728a 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -146,6 +146,7 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 			struct cxl_register_map *map);
 
 #define CXL_RESOURCE_NONE ((resource_size_t) -1)
+#define CXL_TARGET_STRLEN 20
 
 /**
  * struct cxl_port - logical collection of upstream port devices and
@@ -154,19 +155,39 @@ int cxl_map_device_regs(struct pci_dev *pdev,
  * @dev: this port's device
  * @uport: PCI or platform device implementing the upstream port capability
  * @id: id for port device-name
+ * @dports: cxl_dport instances referenced by decoders
  * @component_regs_phys: component register capability base address (optional)
  */
 struct cxl_port {
 	struct device dev;
 	struct device *uport;
 	int id;
+	struct list_head dports;
 	resource_size_t component_reg_phys;
 };
 
+/**
+ * struct cxl_dport - CXL downstream port
+ * @dport: PCI bridge or firmware device representing the downstream link
+ * @port_id: unique hardware identifier for dport in decoder target list
+ * @component_reg_phys: downstream port component registers
+ * @port: reference to cxl_port that contains this downstream port
+ * @list: node for a cxl_port's list of cxl_dport instances
+ */
+struct cxl_dport {
+	struct device *dport;
+	int port_id;
+	resource_size_t component_reg_phys;
+	struct cxl_port *port;
+	struct list_head list;
+};
+
 struct cxl_port *to_cxl_port(struct device *dev);
 struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
 				   resource_size_t component_reg_phys,
 				   struct cxl_port *parent_port);
 
+int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
+		  resource_size_t component_reg_phys);
 extern struct bus_type cxl_bus_type;
 #endif /* __CXL_H__ */

Re: [PATCH v5 4/6] cxl/acpi: Add downstream port data to cxl_port instances

[Jonathan Cameron]

On Sat, 5 Jun 2021 23:05:17 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> In preparation for infrastructure that enumerates and configures the CXL
> decode mechanism of an upstream port to its downstream ports, add a
> representation for a CXL downstream port.
> 
> On ACPI systems the top-most logical downstream ports in the hierarchy
> are the host bridges (ACPI0016 devices) that decode the memory windows
> described by the CXL Early Discovery Table Fixed Memory Window
> Structures (CEDT.CFMWS).
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |   13 ++++
>  drivers/cxl/acpi.c                      |   44 ++++++++++++++
>  drivers/cxl/core.c                      |   95 ++++++++++++++++++++++++++++++-
>  drivers/cxl/cxl.h                       |   21 +++++++
>  4 files changed, 169 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index fb996ced7629..0cb31b7ad17b 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -35,3 +35,16 @@ Description:
>  		CXL component registers. The 'uport' symlink connects the CXL
>  		portX object to the device that published the CXL port
>  		capability.
> +
> +What:		/sys/bus/cxl/devices/portX/dportY
> +Date:		June, 2021
> +KernelVersion:	v5.14
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		CXL port objects are enumerated from either a platform firmware
> +		device (ACPI0017 and ACPI0016) or PCIe switch upstream port with
> +		CXL component registers. The 'dportY' symlink identifies one or
> +		more downstream ports that the upstream port may target in its
> +		decode of CXL memory resources.  The 'Y' integer reflects the
> +		hardware port unique-id used in the hardware decoder target
> +		list.
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 556d25ab6966..0ae7464b603d 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -5,19 +5,61 @@
>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/acpi.h>
> +#include <linux/pci.h>

Not immediately seeing why this include is added in this patch.

>  #include "cxl.h"
>  
> +static struct acpi_device *to_cxl_host_bridge(struct device *dev)
> +{
> +	struct acpi_device *adev = to_acpi_device(dev);
> +
> +	if (strcmp(acpi_device_hid(adev), "ACPI0016") == 0)
> +		return adev;
> +	return NULL;
> +}
> +
> +static int add_host_bridge_dport(struct device *match, void *arg)
> +{
> +	int rc;
> +	acpi_status status;
> +	unsigned long long uid;
> +	struct cxl_port *root_port = arg;
> +	struct device *host = root_port->dev.parent;
> +	struct acpi_device *bridge = to_cxl_host_bridge(match);
> +
> +	if (!bridge)
> +		return 0;
> +
> +	status = acpi_evaluate_integer(bridge->handle, METHOD_NAME__UID, NULL,
> +				       &uid);
> +	if (status != AE_OK) {
> +		dev_err(host, "unable to retrieve _UID of %s\n",
> +			dev_name(match));
> +		return -ENODEV;
> +	}
> +
> +	rc = cxl_add_dport(root_port, match, uid, CXL_RESOURCE_NONE);
> +	if (rc) {
> +		dev_err(host, "failed to add downstream port: %s\n",
> +			dev_name(match));
> +		return rc;
> +	}
> +	dev_dbg(host, "add dport%llu: %s\n", uid, dev_name(match));
> +	return 0;
> +}
> +
>  static int cxl_acpi_probe(struct platform_device *pdev)
>  {
>  	struct cxl_port *root_port;
>  	struct device *host = &pdev->dev;
> +	struct acpi_device *adev = ACPI_COMPANION(host);
>  
>  	root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
>  	if (IS_ERR(root_port))
>  		return PTR_ERR(root_port);
>  	dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
>  
> -	return 0;
> +	return bus_for_each_dev(adev->dev.bus, NULL, root_port,
> +				add_host_bridge_dport);
>  }
>  
>  static const struct acpi_device_id cxl_acpi_ids[] = {
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index dbbb34618d7d..4418b30cce4f 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -33,10 +33,22 @@ static struct attribute_group cxl_base_attribute_group = {
>  	.attrs = cxl_base_attributes,
>  };
>  
> +static void cxl_dport_release(struct cxl_dport *dport)
> +{
> +	put_device(dport->dport);
> +	list_del(&dport->list);
This ordering isn't the reverse of what happens cxl_add_dport()
That would be

list_del()
put_device()
kfree()

If there is a strong reason for that I'd like to see a comment here.

> +	kfree(dport);
> +}
> +
>  static void cxl_port_release(struct device *dev)
>  {
>  	struct cxl_port *port = to_cxl_port(dev);
> +	struct cxl_dport *dport, *_d;
>  
> +	device_lock(dev);
> +	list_for_each_entry_safe(dport, _d, &port->dports, list)
> +		cxl_dport_release(dport);
> +	device_unlock(dev);
>  	ida_free(&cxl_port_ida, port->id);
>  	kfree(port);
>  }
> @@ -60,9 +72,22 @@ struct cxl_port *to_cxl_port(struct device *dev)
>  	return container_of(dev, struct cxl_port, dev);
>  }
>  
> -static void unregister_dev(void *dev)
> +static void unregister_port(void *_port)
>  {
> -	device_unregister(dev);
> +	struct cxl_port *port = _port;
> +	struct cxl_dport *dport;
> +
> +	device_lock(&port->dev);
> +	list_for_each_entry(dport, &port->dports, list) {
> +		char link_name[CXL_TARGET_STRLEN];
> +
> +		if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d",
> +			     dport->port_id) >= CXL_TARGET_STRLEN)
> +			continue;
> +		sysfs_remove_link(&port->dev.kobj, link_name);
> +	}
> +	device_unlock(&port->dev);
> +	device_unregister(&port->dev);
>  }
>  
>  static void cxl_unlink_uport(void *_port)
> @@ -113,6 +138,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
>  
>  	port->uport = uport;
>  	port->component_reg_phys = component_reg_phys;
> +	INIT_LIST_HEAD(&port->dports);
>  
>  	device_initialize(dev);
>  	device_set_pm_not_required(dev);
> @@ -157,7 +183,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
>  	if (rc)
>  		goto err;
>  
> -	rc = devm_add_action_or_reset(host, unregister_dev, dev);
> +	rc = devm_add_action_or_reset(host, unregister_port, port);
>  	if (rc)
>  		return ERR_PTR(rc);
>  
> @@ -173,6 +199,69 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
>  }
>  EXPORT_SYMBOL_GPL(devm_cxl_add_port);
>  
> +static int add_dport(struct cxl_port *port, struct cxl_dport *new)
> +{
> +	struct cxl_dport *dport, *dup = NULL;
> +
> +	device_lock(&port->dev);
> +	list_for_each_entry (dport, &port->dports, list)
> +		if (new->port_id == dport->port_id) {
> +			dup = dport;
> +			goto err;
> +		}
> +	list_add_tail(&new->list, &port->dports);
> +	device_unlock(&port->dev);
> +
> +	return 0;
> +
> +err:
> +	device_unlock(&port->dev);
> +	dev_err(&port->dev,
> +		"unable to add dport%d-%s non-unique port id (%s)\n",
> +		new->port_id, dev_name(new->dport), dev_name(dup->dport));

As there is potential that you might end up with other errors in here long term,
why not move this to the point where the error is detected?
I think you are fine doing it under the mutex.  Obviously indent will be a deeper
than ideal.

> +	return -EEXIST;
> +}
> +
> +/*

This is a bit inconsistent wrt to what functions get full kernel-doc.
My personal preference would be all the exported functions + any others
where it is particularly useful.

> + * Append downstream port data to a cxl_port, 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)
> +{
> +	char link_name[CXL_TARGET_STRLEN];
> +	struct cxl_dport *dport;
> +	int rc;
> +
> +	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
> +	    CXL_TARGET_STRLEN)
> +		return -EINVAL;
> +
> +	dport = kzalloc(sizeof(*dport), GFP_KERNEL);
> +	if (!dport)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&dport->list);
> +	dport->dport = get_device(dport_dev);
> +	dport->port_id = port_id;
> +	dport->component_reg_phys = component_reg_phys;
> +	dport->port = port;
> +
> +	rc = add_dport(port, dport);
> +	if (rc)

If you get an error here, it's not been added to the list, but
in the cxl_dport_release() you remove it from the list. I think you 
just want to put and free the device here.


> +		goto err;
> +
> +	rc = sysfs_create_link(&port->dev.kobj, &dport_dev->kobj, link_name);
> +	if (rc)
> +		goto err;
> +
> +	return 0;
> +err:
> +	cxl_dport_release(dport);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(cxl_add_dport);
> +
>  /**
>   * cxl_probe_component_regs() - Detect CXL Component register blocks
>   * @dev: Host device of the @base mapping
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 46c81165c210..0b89fcfe728a 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -146,6 +146,7 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>  			struct cxl_register_map *map);
>  
>  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
> +#define CXL_TARGET_STRLEN 20
>  
>  /**
>   * struct cxl_port - logical collection of upstream port devices and
> @@ -154,19 +155,39 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>   * @dev: this port's device
>   * @uport: PCI or platform device implementing the upstream port capability
>   * @id: id for port device-name
> + * @dports: cxl_dport instances referenced by decoders
>   * @component_regs_phys: component register capability base address (optional)
>   */
>  struct cxl_port {
>  	struct device dev;
>  	struct device *uport;
>  	int id;
> +	struct list_head dports;
>  	resource_size_t component_reg_phys;
>  };
>  
> +/**
> + * struct cxl_dport - CXL downstream port
> + * @dport: PCI bridge or firmware device representing the downstream link
> + * @port_id: unique hardware identifier for dport in decoder target list
> + * @component_reg_phys: downstream port component registers
> + * @port: reference to cxl_port that contains this downstream port
> + * @list: node for a cxl_port's list of cxl_dport instances
> + */
> +struct cxl_dport {
> +	struct device *dport;
> +	int port_id;
> +	resource_size_t component_reg_phys;
> +	struct cxl_port *port;
> +	struct list_head list;
> +};
> +
>  struct cxl_port *to_cxl_port(struct device *dev);
>  struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
>  				   resource_size_t component_reg_phys,
>  				   struct cxl_port *parent_port);
>  
> +int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
> +		  resource_size_t component_reg_phys);
>  extern struct bus_type cxl_bus_type;
>  #endif /* __CXL_H__ */
> 

Re: [PATCH v5 4/6] cxl/acpi: Add downstream port data to cxl_port instances

[Dan Williams]

On Tue, Jun 8, 2021 at 4:49 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Sat, 5 Jun 2021 23:05:17 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > In preparation for infrastructure that enumerates and configures the CXL
> > decode mechanism of an upstream port to its downstream ports, add a
> > representation for a CXL downstream port.
> >
> > On ACPI systems the top-most logical downstream ports in the hierarchy
> > are the host bridges (ACPI0016 devices) that decode the memory windows
> > described by the CXL Early Discovery Table Fixed Memory Window
> > Structures (CEDT.CFMWS).
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-cxl |   13 ++++
> >  drivers/cxl/acpi.c                      |   44 ++++++++++++++
> >  drivers/cxl/core.c                      |   95 ++++++++++++++++++++++++++++++-
> >  drivers/cxl/cxl.h                       |   21 +++++++
> >  4 files changed, 169 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > index fb996ced7629..0cb31b7ad17b 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cxl
> > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > @@ -35,3 +35,16 @@ Description:
> >               CXL component registers. The 'uport' symlink connects the CXL
> >               portX object to the device that published the CXL port
> >               capability.
> > +
> > +What:                /sys/bus/cxl/devices/portX/dportY
> > +Date:                June, 2021
> > +KernelVersion:       v5.14
> > +Contact:     linux-cxl@vger.kernel.org
> > +Description:
> > +             CXL port objects are enumerated from either a platform firmware
> > +             device (ACPI0017 and ACPI0016) or PCIe switch upstream port with
> > +             CXL component registers. The 'dportY' symlink identifies one or
> > +             more downstream ports that the upstream port may target in its
> > +             decode of CXL memory resources.  The 'Y' integer reflects the
> > +             hardware port unique-id used in the hardware decoder target
> > +             list.
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 556d25ab6966..0ae7464b603d 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -5,19 +5,61 @@
> >  #include <linux/device.h>
> >  #include <linux/kernel.h>
> >  #include <linux/acpi.h>
> > +#include <linux/pci.h>
>
> Not immediately seeing why this include is added in this patch.

My neither, it belongs in the next patch.

>
> >  #include "cxl.h"
> >
> > +static struct acpi_device *to_cxl_host_bridge(struct device *dev)
> > +{
> > +     struct acpi_device *adev = to_acpi_device(dev);
> > +
> > +     if (strcmp(acpi_device_hid(adev), "ACPI0016") == 0)
> > +             return adev;
> > +     return NULL;
> > +}
> > +
> > +static int add_host_bridge_dport(struct device *match, void *arg)
> > +{
> > +     int rc;
> > +     acpi_status status;
> > +     unsigned long long uid;
> > +     struct cxl_port *root_port = arg;
> > +     struct device *host = root_port->dev.parent;
> > +     struct acpi_device *bridge = to_cxl_host_bridge(match);
> > +
> > +     if (!bridge)
> > +             return 0;
> > +
> > +     status = acpi_evaluate_integer(bridge->handle, METHOD_NAME__UID, NULL,
> > +                                    &uid);
> > +     if (status != AE_OK) {
> > +             dev_err(host, "unable to retrieve _UID of %s\n",
> > +                     dev_name(match));
> > +             return -ENODEV;
> > +     }
> > +
> > +     rc = cxl_add_dport(root_port, match, uid, CXL_RESOURCE_NONE);
> > +     if (rc) {
> > +             dev_err(host, "failed to add downstream port: %s\n",
> > +                     dev_name(match));
> > +             return rc;
> > +     }
> > +     dev_dbg(host, "add dport%llu: %s\n", uid, dev_name(match));
> > +     return 0;
> > +}
> > +
> >  static int cxl_acpi_probe(struct platform_device *pdev)
> >  {
> >       struct cxl_port *root_port;
> >       struct device *host = &pdev->dev;
> > +     struct acpi_device *adev = ACPI_COMPANION(host);
> >
> >       root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
> >       if (IS_ERR(root_port))
> >               return PTR_ERR(root_port);
> >       dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
> >
> > -     return 0;
> > +     return bus_for_each_dev(adev->dev.bus, NULL, root_port,
> > +                             add_host_bridge_dport);
> >  }
> >
> >  static const struct acpi_device_id cxl_acpi_ids[] = {
> > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > index dbbb34618d7d..4418b30cce4f 100644
> > --- a/drivers/cxl/core.c
> > +++ b/drivers/cxl/core.c
> > @@ -33,10 +33,22 @@ static struct attribute_group cxl_base_attribute_group = {
> >       .attrs = cxl_base_attributes,
> >  };
> >
> > +static void cxl_dport_release(struct cxl_dport *dport)
> > +{
> > +     put_device(dport->dport);
> > +     list_del(&dport->list);
> This ordering isn't the reverse of what happens cxl_add_dport()
> That would be
>
> list_del()
> put_device()
> kfree()
>
> If there is a strong reason for that I'd like to see a comment here.

No strong reason, I'll reorder.

>
> > +     kfree(dport);
> > +}
> > +
> >  static void cxl_port_release(struct device *dev)
> >  {
> >       struct cxl_port *port = to_cxl_port(dev);
> > +     struct cxl_dport *dport, *_d;
> >
> > +     device_lock(dev);
> > +     list_for_each_entry_safe(dport, _d, &port->dports, list)
> > +             cxl_dport_release(dport);
> > +     device_unlock(dev);
> >       ida_free(&cxl_port_ida, port->id);
> >       kfree(port);
> >  }
> > @@ -60,9 +72,22 @@ struct cxl_port *to_cxl_port(struct device *dev)
> >       return container_of(dev, struct cxl_port, dev);
> >  }
> >
> > -static void unregister_dev(void *dev)
> > +static void unregister_port(void *_port)
> >  {
> > -     device_unregister(dev);
> > +     struct cxl_port *port = _port;
> > +     struct cxl_dport *dport;
> > +
> > +     device_lock(&port->dev);
> > +     list_for_each_entry(dport, &port->dports, list) {
> > +             char link_name[CXL_TARGET_STRLEN];
> > +
> > +             if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d",
> > +                          dport->port_id) >= CXL_TARGET_STRLEN)
> > +                     continue;
> > +             sysfs_remove_link(&port->dev.kobj, link_name);
> > +     }
> > +     device_unlock(&port->dev);
> > +     device_unregister(&port->dev);
> >  }
> >
> >  static void cxl_unlink_uport(void *_port)
> > @@ -113,6 +138,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
> >
> >       port->uport = uport;
> >       port->component_reg_phys = component_reg_phys;
> > +     INIT_LIST_HEAD(&port->dports);
> >
> >       device_initialize(dev);
> >       device_set_pm_not_required(dev);
> > @@ -157,7 +183,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> >       if (rc)
> >               goto err;
> >
> > -     rc = devm_add_action_or_reset(host, unregister_dev, dev);
> > +     rc = devm_add_action_or_reset(host, unregister_port, port);
> >       if (rc)
> >               return ERR_PTR(rc);
> >
> > @@ -173,6 +199,69 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> >  }
> >  EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> >
> > +static int add_dport(struct cxl_port *port, struct cxl_dport *new)
> > +{
> > +     struct cxl_dport *dport, *dup = NULL;
> > +
> > +     device_lock(&port->dev);
> > +     list_for_each_entry (dport, &port->dports, list)
> > +             if (new->port_id == dport->port_id) {
> > +                     dup = dport;
> > +                     goto err;
> > +             }
> > +     list_add_tail(&new->list, &port->dports);
> > +     device_unlock(&port->dev);
> > +
> > +     return 0;
> > +
> > +err:
> > +     device_unlock(&port->dev);
> > +     dev_err(&port->dev,
> > +             "unable to add dport%d-%s non-unique port id (%s)\n",
> > +             new->port_id, dev_name(new->dport), dev_name(dup->dport));
>
> As there is potential that you might end up with other errors in here long term,
> why not move this to the point where the error is detected?
> I think you are fine doing it under the mutex.  Obviously indent will be a deeper
> than ideal.

Hmm, I'll take a look. Might add a dup finder helper to keep the
indent under control.

>
> > +     return -EEXIST;
> > +}
> > +
> > +/*
>
> This is a bit inconsistent wrt to what functions get full kernel-doc.
> My personal preference would be all the exported functions + any others
> where it is particularly useful.

I agree with the sentiment for globally exported symbols. In this case
they are in the "CXL" module namespace and privately defined in
drivers/cxl/ headers. That said, I did document devm_add_cxl_port(),
so there's no good reason to skip the documentation on the other
devm_cxl_add_* routines... will fix.

> > + * Append downstream port data to a cxl_port, 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)
> > +{
> > +     char link_name[CXL_TARGET_STRLEN];
> > +     struct cxl_dport *dport;
> > +     int rc;
> > +
> > +     if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
> > +         CXL_TARGET_STRLEN)
> > +             return -EINVAL;
> > +
> > +     dport = kzalloc(sizeof(*dport), GFP_KERNEL);
> > +     if (!dport)
> > +             return -ENOMEM;
> > +
> > +     INIT_LIST_HEAD(&dport->list);
> > +     dport->dport = get_device(dport_dev);
> > +     dport->port_id = port_id;
> > +     dport->component_reg_phys = component_reg_phys;
> > +     dport->port = port;
> > +
> > +     rc = add_dport(port, dport);
> > +     if (rc)
>
> If you get an error here, it's not been added to the list, but
> in the cxl_dport_release() you remove it from the list. I think you
> just want to put and free the device here.

The delete is innocuous because of the INIT_LIST_HEAD() above. So the
delete will end up doing the right thing and leaving the entry empty
again, and that saves the need for custom code to handle that case.

Re: [PATCH v5 4/6] cxl/acpi: Add downstream port data to cxl_port instances

[Jonathan Cameron]

On Tue, 8 Jun 2021 16:58:19 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Tue, Jun 8, 2021 at 4:49 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Sat, 5 Jun 2021 23:05:17 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >  
> > > In preparation for infrastructure that enumerates and configures the CXL
> > > decode mechanism of an upstream port to its downstream ports, add a
> > > representation for a CXL downstream port.
> > >
> > > On ACPI systems the top-most logical downstream ports in the hierarchy
> > > are the host bridges (ACPI0016 devices) that decode the memory windows
> > > described by the CXL Early Discovery Table Fixed Memory Window
> > > Structures (CEDT.CFMWS).
> > >
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-cxl |   13 ++++
> > >  drivers/cxl/acpi.c                      |   44 ++++++++++++++
> > >  drivers/cxl/core.c                      |   95 ++++++++++++++++++++++++++++++-
> > >  drivers/cxl/cxl.h                       |   21 +++++++
> > >  4 files changed, 169 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > > index fb996ced7629..0cb31b7ad17b 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-cxl
> > > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > > @@ -35,3 +35,16 @@ Description:
> > >               CXL component registers. The 'uport' symlink connects the CXL
> > >               portX object to the device that published the CXL port
> > >               capability.
> > > +
> > > +What:                /sys/bus/cxl/devices/portX/dportY
> > > +Date:                June, 2021
> > > +KernelVersion:       v5.14
> > > +Contact:     linux-cxl@vger.kernel.org
> > > +Description:
> > > +             CXL port objects are enumerated from either a platform firmware
> > > +             device (ACPI0017 and ACPI0016) or PCIe switch upstream port with
> > > +             CXL component registers. The 'dportY' symlink identifies one or
> > > +             more downstream ports that the upstream port may target in its
> > > +             decode of CXL memory resources.  The 'Y' integer reflects the
> > > +             hardware port unique-id used in the hardware decoder target
> > > +             list.
> > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > index 556d25ab6966..0ae7464b603d 100644
> > > --- a/drivers/cxl/acpi.c
> > > +++ b/drivers/cxl/acpi.c
> > > @@ -5,19 +5,61 @@
> > >  #include <linux/device.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/acpi.h>
> > > +#include <linux/pci.h>  
> >
> > Not immediately seeing why this include is added in this patch.  
> 
> My neither, it belongs in the next patch.
> 
> >  
> > >  #include "cxl.h"
> > >
> > > +static struct acpi_device *to_cxl_host_bridge(struct device *dev)
> > > +{
> > > +     struct acpi_device *adev = to_acpi_device(dev);
> > > +
> > > +     if (strcmp(acpi_device_hid(adev), "ACPI0016") == 0)
> > > +             return adev;
> > > +     return NULL;
> > > +}
> > > +
> > > +static int add_host_bridge_dport(struct device *match, void *arg)
> > > +{
> > > +     int rc;
> > > +     acpi_status status;
> > > +     unsigned long long uid;
> > > +     struct cxl_port *root_port = arg;
> > > +     struct device *host = root_port->dev.parent;
> > > +     struct acpi_device *bridge = to_cxl_host_bridge(match);
> > > +
> > > +     if (!bridge)
> > > +             return 0;
> > > +
> > > +     status = acpi_evaluate_integer(bridge->handle, METHOD_NAME__UID, NULL,
> > > +                                    &uid);
> > > +     if (status != AE_OK) {
> > > +             dev_err(host, "unable to retrieve _UID of %s\n",
> > > +                     dev_name(match));
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     rc = cxl_add_dport(root_port, match, uid, CXL_RESOURCE_NONE);
> > > +     if (rc) {
> > > +             dev_err(host, "failed to add downstream port: %s\n",
> > > +                     dev_name(match));
> > > +             return rc;
> > > +     }
> > > +     dev_dbg(host, "add dport%llu: %s\n", uid, dev_name(match));
> > > +     return 0;
> > > +}
> > > +
> > >  static int cxl_acpi_probe(struct platform_device *pdev)
> > >  {
> > >       struct cxl_port *root_port;
> > >       struct device *host = &pdev->dev;
> > > +     struct acpi_device *adev = ACPI_COMPANION(host);
> > >
> > >       root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
> > >       if (IS_ERR(root_port))
> > >               return PTR_ERR(root_port);
> > >       dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
> > >
> > > -     return 0;
> > > +     return bus_for_each_dev(adev->dev.bus, NULL, root_port,
> > > +                             add_host_bridge_dport);
> > >  }
> > >
> > >  static const struct acpi_device_id cxl_acpi_ids[] = {
> > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > > index dbbb34618d7d..4418b30cce4f 100644
> > > --- a/drivers/cxl/core.c
> > > +++ b/drivers/cxl/core.c
> > > @@ -33,10 +33,22 @@ static struct attribute_group cxl_base_attribute_group = {
> > >       .attrs = cxl_base_attributes,
> > >  };
> > >
> > > +static void cxl_dport_release(struct cxl_dport *dport)
> > > +{
> > > +     put_device(dport->dport);
> > > +     list_del(&dport->list);  
> > This ordering isn't the reverse of what happens cxl_add_dport()
> > That would be
> >
> > list_del()
> > put_device()
> > kfree()
> >
> > If there is a strong reason for that I'd like to see a comment here.  
> 
> No strong reason, I'll reorder.
> 
> >  
> > > +     kfree(dport);
> > > +}
> > > +
> > >  static void cxl_port_release(struct device *dev)
> > >  {
> > >       struct cxl_port *port = to_cxl_port(dev);
> > > +     struct cxl_dport *dport, *_d;
> > >
> > > +     device_lock(dev);
> > > +     list_for_each_entry_safe(dport, _d, &port->dports, list)
> > > +             cxl_dport_release(dport);
> > > +     device_unlock(dev);
> > >       ida_free(&cxl_port_ida, port->id);
> > >       kfree(port);
> > >  }
> > > @@ -60,9 +72,22 @@ struct cxl_port *to_cxl_port(struct device *dev)
> > >       return container_of(dev, struct cxl_port, dev);
> > >  }
> > >
> > > -static void unregister_dev(void *dev)
> > > +static void unregister_port(void *_port)
> > >  {
> > > -     device_unregister(dev);
> > > +     struct cxl_port *port = _port;
> > > +     struct cxl_dport *dport;
> > > +
> > > +     device_lock(&port->dev);
> > > +     list_for_each_entry(dport, &port->dports, list) {
> > > +             char link_name[CXL_TARGET_STRLEN];
> > > +
> > > +             if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d",
> > > +                          dport->port_id) >= CXL_TARGET_STRLEN)
> > > +                     continue;
> > > +             sysfs_remove_link(&port->dev.kobj, link_name);
> > > +     }
> > > +     device_unlock(&port->dev);
> > > +     device_unregister(&port->dev);
> > >  }
> > >
> > >  static void cxl_unlink_uport(void *_port)
> > > @@ -113,6 +138,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
> > >
> > >       port->uport = uport;
> > >       port->component_reg_phys = component_reg_phys;
> > > +     INIT_LIST_HEAD(&port->dports);
> > >
> > >       device_initialize(dev);
> > >       device_set_pm_not_required(dev);
> > > @@ -157,7 +183,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> > >       if (rc)
> > >               goto err;
> > >
> > > -     rc = devm_add_action_or_reset(host, unregister_dev, dev);
> > > +     rc = devm_add_action_or_reset(host, unregister_port, port);
> > >       if (rc)
> > >               return ERR_PTR(rc);
> > >
> > > @@ -173,6 +199,69 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> > >  }
> > >  EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> > >
> > > +static int add_dport(struct cxl_port *port, struct cxl_dport *new)
> > > +{
> > > +     struct cxl_dport *dport, *dup = NULL;
> > > +
> > > +     device_lock(&port->dev);
> > > +     list_for_each_entry (dport, &port->dports, list)
> > > +             if (new->port_id == dport->port_id) {
> > > +                     dup = dport;
> > > +                     goto err;
> > > +             }
> > > +     list_add_tail(&new->list, &port->dports);
> > > +     device_unlock(&port->dev);
> > > +
> > > +     return 0;
> > > +
> > > +err:
> > > +     device_unlock(&port->dev);
> > > +     dev_err(&port->dev,
> > > +             "unable to add dport%d-%s non-unique port id (%s)\n",
> > > +             new->port_id, dev_name(new->dport), dev_name(dup->dport));  
> >
> > As there is potential that you might end up with other errors in here long term,
> > why not move this to the point where the error is detected?
> > I think you are fine doing it under the mutex.  Obviously indent will be a deeper
> > than ideal.  
> 
> Hmm, I'll take a look. Might add a dup finder helper to keep the
> indent under control.
> 
> >  
> > > +     return -EEXIST;
> > > +}
> > > +
> > > +/*  
> >
> > This is a bit inconsistent wrt to what functions get full kernel-doc.
> > My personal preference would be all the exported functions + any others
> > where it is particularly useful.  
> 
> I agree with the sentiment for globally exported symbols. In this case
> they are in the "CXL" module namespace and privately defined in
> drivers/cxl/ headers. That said, I did document devm_add_cxl_port(),
> so there's no good reason to skip the documentation on the other
> devm_cxl_add_* routines... will fix.

Maybe we should consider using symbol namespaces for CXL?
EXPORT_SYMBOL_NS_GPL() etc


> 
> > > + * Append downstream port data to a cxl_port, 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)
> > > +{
> > > +     char link_name[CXL_TARGET_STRLEN];
> > > +     struct cxl_dport *dport;
> > > +     int rc;
> > > +
> > > +     if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
> > > +         CXL_TARGET_STRLEN)
> > > +             return -EINVAL;
> > > +
> > > +     dport = kzalloc(sizeof(*dport), GFP_KERNEL);
> > > +     if (!dport)
> > > +             return -ENOMEM;
> > > +
> > > +     INIT_LIST_HEAD(&dport->list);
> > > +     dport->dport = get_device(dport_dev);
> > > +     dport->port_id = port_id;
> > > +     dport->component_reg_phys = component_reg_phys;
> > > +     dport->port = port;
> > > +
> > > +     rc = add_dport(port, dport);
> > > +     if (rc)  
> >
> > If you get an error here, it's not been added to the list, but
> > in the cxl_dport_release() you remove it from the list. I think you
> > just want to put and free the device here.  
> 
> The delete is innocuous because of the INIT_LIST_HEAD() above. So the
> delete will end up doing the right thing and leaving the entry empty
> again, and that saves the need for custom code to handle that case.

Ah fair enough. I'd missed that INIT.  Not sure I'm keen on that
approach as it's not in the 'obviously correct' category but it's your
code to maintain so I'm not that fussed.

Jonathan

Re: [PATCH v5 4/6] cxl/acpi: Add downstream port data to cxl_port instances

[Dan Williams]

On Wed, Jun 9, 2021 at 4:28 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
> > > This is a bit inconsistent wrt to what functions get full kernel-doc.
> > > My personal preference would be all the exported functions + any others
> > > where it is particularly useful.
> >
> > I agree with the sentiment for globally exported symbols. In this case
> > they are in the "CXL" module namespace and privately defined in
> > drivers/cxl/ headers. That said, I did document devm_add_cxl_port(),
> > so there's no good reason to skip the documentation on the other
> > devm_cxl_add_* routines... will fix.
>
> Maybe we should consider using symbol namespaces for CXL?
> EXPORT_SYMBOL_NS_GPL() etc

In fact, we already are using that, it's just implicit from the
Makefile with this line:

ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL

> > > > + * Append downstream port data to a cxl_port, 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)
> > > > +{
> > > > +     char link_name[CXL_TARGET_STRLEN];
> > > > +     struct cxl_dport *dport;
> > > > +     int rc;
> > > > +
> > > > +     if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
> > > > +         CXL_TARGET_STRLEN)
> > > > +             return -EINVAL;
> > > > +
> > > > +     dport = kzalloc(sizeof(*dport), GFP_KERNEL);
> > > > +     if (!dport)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     INIT_LIST_HEAD(&dport->list);
> > > > +     dport->dport = get_device(dport_dev);
> > > > +     dport->port_id = port_id;
> > > > +     dport->component_reg_phys = component_reg_phys;
> > > > +     dport->port = port;
> > > > +
> > > > +     rc = add_dport(port, dport);
> > > > +     if (rc)
> > >
> > > If you get an error here, it's not been added to the list, but
> > > in the cxl_dport_release() you remove it from the list. I think you
> > > just want to put and free the device here.
> >
> > The delete is innocuous because of the INIT_LIST_HEAD() above. So the
> > delete will end up doing the right thing and leaving the entry empty
> > again, and that saves the need for custom code to handle that case.
>
> Ah fair enough. I'd missed that INIT.  Not sure I'm keen on that
> approach as it's not in the 'obviously correct' category but it's your
> code to maintain so I'm not that fussed.

I think it's in a similar spirit to devm and the lack of
ida_destroy(), try not to write unwind code if at all possible...

[PATCH v5 5/6] cxl/acpi: Enumerate host bridge root ports

[Dan Williams]

While the resources enumerated by the CEDT.CFMWS identify a cxl_port
with host bridges as downstream ports, host bridges themselves are
upstream ports that decode to downstream ports represented by PCIe Root
Ports. Walk the PCIe Root Ports connected to a CXL Host Bridge,
identified by the ACPI0016 _HID, and add each one as a cxl_dport of the
host bridge cxl_port.

For now, component registers are not enumerated, only the first order
uport / dport relationships.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/acpi.c |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 95 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 0ae7464b603d..ec324bf063b8 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -8,6 +8,48 @@
 #include <linux/pci.h>
 #include "cxl.h"
 
+struct cxl_walk_context {
+	struct device *dev;
+	struct pci_bus *root;
+	struct cxl_port *port;
+	int error;
+	int count;
+};
+
+static int match_add_root_ports(struct pci_dev *pdev, void *data)
+{
+	struct cxl_walk_context *ctx = data;
+	struct pci_bus *root_bus = ctx->root;
+	struct cxl_port *port = ctx->port;
+	int type = pci_pcie_type(pdev);
+	struct device *dev = ctx->dev;
+	u32 lnkcap, port_num;
+	int rc;
+
+	if (pdev->bus != root_bus)
+		return 0;
+	if (!pci_is_pcie(pdev))
+		return 0;
+	if (type != PCI_EXP_TYPE_ROOT_PORT)
+		return 0;
+	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
+				  &lnkcap) != PCIBIOS_SUCCESSFUL)
+		return 0;
+
+	/* TODO walk DVSEC to find component register base */
+	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
+	rc = cxl_add_dport(port, &pdev->dev, port_num, CXL_RESOURCE_NONE);
+	if (rc) {
+		ctx->error = rc;
+		return rc;
+	}
+	ctx->count++;
+
+	dev_dbg(dev, "add dport%d: %s\n", port_num, dev_name(&pdev->dev));
+
+	return 0;
+}
+
 static struct acpi_device *to_cxl_host_bridge(struct device *dev)
 {
 	struct acpi_device *adev = to_acpi_device(dev);
@@ -17,6 +59,44 @@ static struct acpi_device *to_cxl_host_bridge(struct device *dev)
 	return NULL;
 }
 
+/*
+ * A host bridge is a dport to a CFMWS decode and it is a uport to the
+ * dport (PCIe Root Ports) in the host bridge.
+ */
+static int add_host_bridge_uport(struct device *match, void *arg)
+{
+	struct acpi_device *bridge = to_cxl_host_bridge(match);
+	struct cxl_port *root_port = arg;
+	struct device *host = root_port->dev.parent;
+	struct acpi_pci_root *pci_root;
+	struct cxl_walk_context ctx;
+	struct cxl_port *port;
+
+	if (!bridge)
+		return 0;
+
+	pci_root = acpi_pci_find_root(bridge->handle);
+	if (!pci_root)
+		return -ENXIO;
+
+	/* TODO: fold in CEDT.CHBS retrieval */
+	port = devm_cxl_add_port(host, match, CXL_RESOURCE_NONE, root_port);
+	if (IS_ERR(port))
+		return PTR_ERR(port);
+	dev_dbg(host, "%s: add: %s\n", dev_name(match), dev_name(&port->dev));
+
+	ctx = (struct cxl_walk_context){
+		.dev = host,
+		.root = pci_root->bus,
+		.port = port,
+	};
+	pci_walk_bus(pci_root->bus, match_add_root_ports, &ctx);
+
+	if (ctx.count == 0)
+		return -ENODEV;
+	return ctx.error;
+}
+
 static int add_host_bridge_dport(struct device *match, void *arg)
 {
 	int rc;
@@ -49,6 +129,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 
 static int cxl_acpi_probe(struct platform_device *pdev)
 {
+	int rc;
 	struct cxl_port *root_port;
 	struct device *host = &pdev->dev;
 	struct acpi_device *adev = ACPI_COMPANION(host);
@@ -58,8 +139,20 @@ static int cxl_acpi_probe(struct platform_device *pdev)
 		return PTR_ERR(root_port);
 	dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
 
-	return bus_for_each_dev(adev->dev.bus, NULL, root_port,
-				add_host_bridge_dport);
+	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
+			      add_host_bridge_dport);
+	if (rc)
+		return rc;
+
+	/*
+	 * Root level scanned with host-bridge as dports, now scan host-bridges
+	 * for their role as CXL uports to their CXL-capable PCIe Root Ports.
+	 */
+	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
+			      add_host_bridge_uport);
+	if (rc)
+		dev_err(host, "failed to scan host bridges\n");
+	return 0;
 }
 
 static const struct acpi_device_id cxl_acpi_ids[] = {

Re: [PATCH v5 5/6] cxl/acpi: Enumerate host bridge root ports

[Jonathan Cameron]

On Sat, 5 Jun 2021 23:05:22 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> While the resources enumerated by the CEDT.CFMWS identify a cxl_port
> with host bridges as downstream ports, host bridges themselves are
> upstream ports that decode to downstream ports represented by PCIe Root
> Ports. Walk the PCIe Root Ports connected to a CXL Host Bridge,
> identified by the ACPI0016 _HID, and add each one as a cxl_dport of the
> host bridge cxl_port.
> 
> For now, component registers are not enumerated, only the first order
> uport / dport relationships.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
LGTM

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/acpi.c |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 95 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 0ae7464b603d..ec324bf063b8 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -8,6 +8,48 @@
>  #include <linux/pci.h>
>  #include "cxl.h"
>  
> +struct cxl_walk_context {
> +	struct device *dev;
> +	struct pci_bus *root;
> +	struct cxl_port *port;
> +	int error;
> +	int count;
> +};
> +
> +static int match_add_root_ports(struct pci_dev *pdev, void *data)
> +{
> +	struct cxl_walk_context *ctx = data;
> +	struct pci_bus *root_bus = ctx->root;
> +	struct cxl_port *port = ctx->port;
> +	int type = pci_pcie_type(pdev);
> +	struct device *dev = ctx->dev;
> +	u32 lnkcap, port_num;
> +	int rc;
> +
> +	if (pdev->bus != root_bus)
> +		return 0;
> +	if (!pci_is_pcie(pdev))
> +		return 0;
> +	if (type != PCI_EXP_TYPE_ROOT_PORT)
> +		return 0;
> +	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
> +				  &lnkcap) != PCIBIOS_SUCCESSFUL)
> +		return 0;
> +
> +	/* TODO walk DVSEC to find component register base */
> +	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
> +	rc = cxl_add_dport(port, &pdev->dev, port_num, CXL_RESOURCE_NONE);
> +	if (rc) {
> +		ctx->error = rc;
> +		return rc;
> +	}
> +	ctx->count++;
> +
> +	dev_dbg(dev, "add dport%d: %s\n", port_num, dev_name(&pdev->dev));
> +
> +	return 0;
> +}
> +
>  static struct acpi_device *to_cxl_host_bridge(struct device *dev)
>  {
>  	struct acpi_device *adev = to_acpi_device(dev);
> @@ -17,6 +59,44 @@ static struct acpi_device *to_cxl_host_bridge(struct device *dev)
>  	return NULL;
>  }
>  
> +/*
> + * A host bridge is a dport to a CFMWS decode and it is a uport to the
> + * dport (PCIe Root Ports) in the host bridge.
> + */
> +static int add_host_bridge_uport(struct device *match, void *arg)
> +{
> +	struct acpi_device *bridge = to_cxl_host_bridge(match);
> +	struct cxl_port *root_port = arg;
> +	struct device *host = root_port->dev.parent;
> +	struct acpi_pci_root *pci_root;
> +	struct cxl_walk_context ctx;
> +	struct cxl_port *port;
> +
> +	if (!bridge)
> +		return 0;
> +
> +	pci_root = acpi_pci_find_root(bridge->handle);
> +	if (!pci_root)
> +		return -ENXIO;
> +
> +	/* TODO: fold in CEDT.CHBS retrieval */
> +	port = devm_cxl_add_port(host, match, CXL_RESOURCE_NONE, root_port);
> +	if (IS_ERR(port))
> +		return PTR_ERR(port);
> +	dev_dbg(host, "%s: add: %s\n", dev_name(match), dev_name(&port->dev));
> +
> +	ctx = (struct cxl_walk_context){
> +		.dev = host,
> +		.root = pci_root->bus,
> +		.port = port,
> +	};
> +	pci_walk_bus(pci_root->bus, match_add_root_ports, &ctx);
> +
> +	if (ctx.count == 0)
> +		return -ENODEV;
> +	return ctx.error;
> +}
> +
>  static int add_host_bridge_dport(struct device *match, void *arg)
>  {
>  	int rc;
> @@ -49,6 +129,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>  
>  static int cxl_acpi_probe(struct platform_device *pdev)
>  {
> +	int rc;
>  	struct cxl_port *root_port;
>  	struct device *host = &pdev->dev;
>  	struct acpi_device *adev = ACPI_COMPANION(host);
> @@ -58,8 +139,20 @@ static int cxl_acpi_probe(struct platform_device *pdev)
>  		return PTR_ERR(root_port);
>  	dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
>  
> -	return bus_for_each_dev(adev->dev.bus, NULL, root_port,
> -				add_host_bridge_dport);
> +	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
> +			      add_host_bridge_dport);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Root level scanned with host-bridge as dports, now scan host-bridges
> +	 * for their role as CXL uports to their CXL-capable PCIe Root Ports.
> +	 */
> +	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
> +			      add_host_bridge_uport);
> +	if (rc)
> +		dev_err(host, "failed to scan host bridges\n");
> +	return 0;
>  }
>  
>  static const struct acpi_device_id cxl_acpi_ids[] = {
> 

[PATCH v5 6/6] cxl/acpi: Introduce cxl_decoder objects

[Dan Williams]

A cxl_decoder is a child of a cxl_port. It represents a hardware
decoder configuration of an upstream port to one or more of its
downstream ports. The decoder is either represented in standard HDM
decoder registers (see CXL 2.0 section 8.2.5.12 CXL HDM Decoder
Capability Structure), or it is a static decode configuration
communicated by platform firmware (see the CXL Early Discovery Table:
Fixed Memory Window Structure).

The firmware described and hardware described decoders differ slightly
leading to 2 different sub-types of decoders, cxl_decoder_root and
cxl_decoder_switch. At the root level the decode capabilities restrict
what can be mapped beneath them. Mid-level switch decoders are
configured for either acclerator (type-2) or memory-expander (type-3)
operation, but they are otherwise agnostic to the type of memory
(volatile vs persistent) being mapped.

Here is an example topology from a single-ported host-bridge environment
without CFMWS decodes enumerated.

/sys/bus/cxl/devices/root0
├── devtype
├── dport0 -> ../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
├── port1
│   ├── decoder1.0
│   │   ├── devtype
│   │   ├── end
│   │   ├── locked
│   │   ├── start
│   │   ├── subsystem -> ../../../../bus/cxl
│   │   ├── target_list
│   │   ├── target_type
│   │   └── uevent
│   ├── devtype
│   ├── dport0 -> ../../pci0000:34/0000:34:00.0
│   ├── subsystem -> ../../../bus/cxl
│   ├── uevent
│   └── uport -> ../../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
├── subsystem -> ../../bus/cxl
├── uevent
└── uport -> ../platform/ACPI0017:00

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-cxl |   70 ++++++++
 drivers/cxl/acpi.c                      |   21 ++
 drivers/cxl/core.c                      |  265 +++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h                       |   48 ++++++
 4 files changed, 403 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 0cb31b7ad17b..f16f18e77578 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -48,3 +48,73 @@ Description:
 		decode of CXL memory resources.  The 'Y' integer reflects the
 		hardware port unique-id used in the hardware decoder target
 		list.
+
+What:		/sys/bus/cxl/devices/decoderX.Y
+Date:		June, 2021
+KernelVersion:	v5.14
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		CXL decoder objects are enumerated from either a platform
+		firmware description, or a CXL HDM decoder register set in a
+		PCIe device (see CXL 2.0 section 8.2.5.12 CXL HDM Decoder
+		Capability Structure). The 'X' in decoderX.Y represents the
+		cxl_port container of this decoder, and 'Y' represents the
+		instance id of a given decoder resource.
+
+What:		/sys/bus/cxl/devices/decoderX.Y/{start,end}
+Date:		June, 2021
+KernelVersion:	v5.14
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		The 'start' and 'end' attributes together convey the physical
+		address base and last addressable byte of the decoder's decode
+		window. For decoders of devtype "cxl_decoder_root" the address
+		range is fixed. For decoders of devtype "cxl_decoder_switch" the
+		address is bounded by the decode range of the cxl_port ancestor
+		of the decoder's cxl_port, and dynamically updates based on the
+		active memory regions in that address space.
+
+What:		/sys/bus/cxl/devices/decoderX.Y/locked
+Date:		June, 2021
+KernelVersion:	v5.14
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		CXL HDM decoders have the capability to lock the configuration
+		until the next device reset. For decoders of devtype
+		"cxl_decoder_root" there is no standard facility to unlock them.
+		For decoders of devtype "cxl_decoder_switch" a secondary bus
+		reset, of the PCIe bridge that provides the bus for this
+		decoders uport, unlocks / resets the decoder.
+
+What:		/sys/bus/cxl/devices/decoderX.Y/target_list
+Date:		June, 2021
+KernelVersion:	v5.14
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		Display a comma separated list of the current decoder target
+		configuration. The list is ordered by the current configured
+		interleave order of the decoder's dport instances. Each entry in
+		the list is a dport id.
+
+What:		/sys/bus/cxl/devices/decoderX.Y/cap_{pmem,ram,type2,type3}
+Date:		June, 2021
+KernelVersion:	v5.14
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		When a CXL decoder is of devtype "cxl_decoder_root", it
+		represents a fixed memory window identified by platform
+		firmware. A fixed window may only support a subset of memory
+		types. The 'cap_*' attributes indicate whether persistent
+		memory, volatile memory, accelerator memory, and / or expander
+		memory may be mapped behind this decoder's memory window.
+
+What:		/sys/bus/cxl/devices/decoderX.Y/target_type
+Date:		June, 2021
+KernelVersion:	v5.14
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		When a CXL decoder is of devtype "cxl_decoder_switch", it can
+		optionally decode either accelerator memory (type-2) or expander
+		memory (type-3). The 'target_type' attribute indicates the
+		current setting which may dynamically change based on what
+		memory regions are activated in this decode hierarchy.
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index ec324bf063b8..6f203ba7fc1d 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -70,6 +70,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 	struct device *host = root_port->dev.parent;
 	struct acpi_pci_root *pci_root;
 	struct cxl_walk_context ctx;
+	struct cxl_decoder *cxld;
 	struct cxl_port *port;
 
 	if (!bridge)
@@ -94,7 +95,25 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 
 	if (ctx.count == 0)
 		return -ENODEV;
-	return ctx.error;
+	if (ctx.error)
+		return ctx.error;
+
+	/* TODO: Scan CHBCR for HDM Decoder resources */
+
+	/*
+	 * In the single-port host-bridge case there are no HDM decoders
+	 * in the CHBCR and a 1:1 passthrough decode is implied.
+	 */
+	if (ctx.count == 1) {
+		cxld = devm_cxl_add_decoder(host, port, 1, 0, -2, 1, 0,
+					    CXL_DECODER_EXPANDER, 0);
+		if (IS_ERR(cxld))
+			return PTR_ERR(cxld);
+
+		dev_dbg(host, "add: %s\n", dev_name(&cxld->dev));
+	}
+
+	return 0;
 }
 
 static int add_host_bridge_dport(struct device *match, void *arg)
diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index 4418b30cce4f..781efe51f186 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -33,6 +33,168 @@ static struct attribute_group cxl_base_attribute_group = {
 	.attrs = cxl_base_attributes,
 };
 
+static ssize_t start_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+
+	return sysfs_emit(buf, "%#llx\n", cxld->range.start);
+}
+static DEVICE_ATTR_RO(start);
+
+static ssize_t end_show(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+
+	return sysfs_emit(buf, "%#llx\n", cxld->range.end);
+}
+static DEVICE_ATTR_RO(end);
+
+#define CXL_DECODER_FLAG_ATTR(name, flag)                            \
+static ssize_t name##_show(struct device *dev,                       \
+			   struct device_attribute *attr, char *buf) \
+{                                                                    \
+	struct cxl_decoder *cxld = to_cxl_decoder(dev);              \
+                                                                     \
+	return sysfs_emit(buf, "%s\n",                               \
+			  (cxld->flags & (flag)) ? "1" : "0");       \
+}                                                                    \
+static DEVICE_ATTR_RO(name)
+
+CXL_DECODER_FLAG_ATTR(cap_pmem, CXL_DECODER_F_PMEM);
+CXL_DECODER_FLAG_ATTR(cap_ram, CXL_DECODER_F_RAM);
+CXL_DECODER_FLAG_ATTR(cap_type2, CXL_DECODER_F_TYPE2);
+CXL_DECODER_FLAG_ATTR(cap_type3, CXL_DECODER_F_TYPE3);
+CXL_DECODER_FLAG_ATTR(locked, CXL_DECODER_F_LOCK);
+
+static ssize_t target_type_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+
+	switch (cxld->target_type) {
+	case CXL_DECODER_ACCELERATOR:
+		return sysfs_emit(buf, "accelerator\n");
+	case CXL_DECODER_EXPANDER:
+		return sysfs_emit(buf, "expander\n");
+	}
+	return -ENXIO;
+}
+DEVICE_ATTR_RO(target_type);
+
+static ssize_t target_list_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+	ssize_t offset = 0;
+	int i, rc = 0;
+
+	device_lock(dev);
+	for (i = 0; i < cxld->interleave_ways; i++) {
+		struct cxl_dport *dport = cxld->target[i];
+		struct cxl_dport *next = NULL;
+
+		if (!dport)
+			break;
+
+		if (i + 1 < cxld->interleave_ways)
+			next = cxld->target[i + 1];
+		rc = sysfs_emit_at(buf, offset, "%d%s", dport->port_id,
+				   next ? "," : "");
+		if (rc < 0)
+			break;
+		offset += rc;
+	}
+	device_unlock(dev);
+
+	if (rc < 0)
+		return rc;
+
+	rc = sysfs_emit_at(buf, offset, "\n");
+	if (rc < 0)
+		return rc;
+
+	return offset + rc;
+}
+DEVICE_ATTR_RO(target_list);
+
+static struct attribute *cxl_decoder_base_attrs[] = {
+	&dev_attr_start.attr,
+	&dev_attr_end.attr,
+	&dev_attr_locked.attr,
+	&dev_attr_target_list.attr,
+	NULL,
+};
+
+static struct attribute_group cxl_decoder_base_attribute_group = {
+	.attrs = cxl_decoder_base_attrs,
+};
+
+static struct attribute *cxl_decoder_root_attrs[] = {
+	&dev_attr_cap_pmem.attr,
+	&dev_attr_cap_ram.attr,
+	&dev_attr_cap_type2.attr,
+	&dev_attr_cap_type3.attr,
+	NULL,
+};
+
+static struct attribute_group cxl_decoder_root_attribute_group = {
+	.attrs = cxl_decoder_root_attrs,
+};
+
+static const struct attribute_group *cxl_decoder_root_attribute_groups[] = {
+	&cxl_decoder_root_attribute_group,
+	&cxl_decoder_base_attribute_group,
+	&cxl_base_attribute_group,
+	NULL,
+};
+
+static struct attribute *cxl_decoder_switch_attrs[] = {
+	&dev_attr_target_type.attr,
+	NULL,
+};
+
+static struct attribute_group cxl_decoder_switch_attribute_group = {
+	.attrs = cxl_decoder_switch_attrs,
+};
+
+static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = {
+	&cxl_decoder_switch_attribute_group,
+	&cxl_decoder_base_attribute_group,
+	&cxl_base_attribute_group,
+	NULL,
+};
+
+static void cxl_decoder_release(struct device *dev)
+{
+	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+	struct cxl_port *port = to_cxl_port(dev->parent);
+
+	ida_free(&port->decoder_ida, cxld->id);
+	kfree(cxld);
+}
+
+static const struct device_type cxl_decoder_switch_type = {
+	.name = "cxl_decoder_switch",
+	.release = cxl_decoder_release,
+	.groups = cxl_decoder_switch_attribute_groups,
+};
+
+static const struct device_type cxl_decoder_root_type = {
+	.name = "cxl_decoder_root",
+	.release = cxl_decoder_release,
+	.groups = cxl_decoder_root_attribute_groups,
+};
+
+struct cxl_decoder *to_cxl_decoder(struct device *dev)
+{
+	if (dev_WARN_ONCE(dev, dev->type->release != cxl_decoder_release,
+			  "not a cxl_decoder device\n"))
+		return NULL;
+	return container_of(dev, struct cxl_decoder, dev);
+}
+
 static void cxl_dport_release(struct cxl_dport *dport)
 {
 	put_device(dport->dport);
@@ -138,6 +300,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
 
 	port->uport = uport;
 	port->component_reg_phys = component_reg_phys;
+	ida_init(&port->decoder_ida);
 	INIT_LIST_HEAD(&port->dports);
 
 	device_initialize(dev);
@@ -262,6 +425,108 @@ int cxl_add_dport(struct cxl_port *port, struct device *dport_dev, int port_id,
 }
 EXPORT_SYMBOL_GPL(cxl_add_dport);
 
+static struct cxl_decoder *
+cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
+		  resource_size_t len, int interleave_ways,
+		  int interleave_granularity, enum cxl_decoder_type type,
+		  unsigned long flags)
+{
+	struct cxl_decoder *cxld;
+	struct device *dev;
+	int rc = 0;
+
+	if (interleave_ways < 1)
+		return ERR_PTR(-EINVAL);
+
+	device_lock(&port->dev);
+	if (list_empty(&port->dports))
+		rc = -EINVAL;
+	device_unlock(&port->dev);
+	if (rc)
+		return ERR_PTR(rc);
+
+	cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
+	if (!cxld)
+		return ERR_PTR(-ENOMEM);
+
+	rc = ida_alloc(&port->decoder_ida, GFP_KERNEL);
+	if (rc < 0)
+		goto err;
+
+	*cxld = (struct cxl_decoder) {
+		.id = rc,
+		.range = {
+			.start = base,
+			.end = base + len - 1,
+		},
+		.flags = flags,
+		.interleave_ways = interleave_ways,
+		.interleave_granularity = interleave_granularity,
+		.target_type = type,
+	};
+
+	/* handle implied target_list */
+	if (interleave_ways == 1)
+		cxld->target[0] =
+			list_first_entry(&port->dports, struct cxl_dport, list);
+	dev = &cxld->dev;
+	device_initialize(dev);
+	device_set_pm_not_required(dev);
+	dev->parent = &port->dev;
+	dev->bus = &cxl_bus_type;
+
+	/* root ports do not have a cxl_port_type parent */
+	if (port->dev.parent->type == &cxl_port_type)
+		dev->type = &cxl_decoder_switch_type;
+	else
+		dev->type = &cxl_decoder_root_type;
+
+	return cxld;
+err:
+	kfree(cxld);
+	return ERR_PTR(rc);
+}
+
+static void unregister_dev(void *dev)
+{
+	device_unregister(dev);
+}
+
+struct cxl_decoder *
+devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
+		     resource_size_t base, resource_size_t len,
+		     int interleave_ways, int interleave_granularity,
+		     enum cxl_decoder_type type, unsigned long flags)
+{
+	struct cxl_decoder *cxld;
+	struct device *dev;
+	int rc;
+
+	cxld = cxl_decoder_alloc(port, nr_targets, base, len, interleave_ways,
+				 interleave_granularity, type, flags);
+	if (IS_ERR(cxld))
+		return cxld;
+
+	dev = &cxld->dev;
+	rc = dev_set_name(dev, "decoder%d.%d", port->id, cxld->id);
+	if (rc)
+		goto err;
+
+	rc = device_add(dev);
+	if (rc)
+		goto err;
+
+	rc = devm_add_action_or_reset(host, unregister_dev, dev);
+	if (rc)
+		return ERR_PTR(rc);
+	return cxld;
+
+err:
+	put_device(dev);
+	return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_GPL(devm_cxl_add_decoder);
+
 /**
  * cxl_probe_component_regs() - Detect CXL Component register blocks
  * @dev: Host device of the @base mapping
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 0b89fcfe728a..f350c92d37d2 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -148,6 +148,44 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 #define CXL_RESOURCE_NONE ((resource_size_t) -1)
 #define CXL_TARGET_STRLEN 20
 
+/*
+ * cxl_decoder flags that define the type of memory / devices this
+ * decoder supports as well as configuration lock status See "CXL 2.0
+ * 8.2.5.12.7 CXL HDM Decoder 0 Control Register" for details.
+ */
+#define CXL_DECODER_F_RAM   BIT(0)
+#define CXL_DECODER_F_PMEM  BIT(1)
+#define CXL_DECODER_F_TYPE2 BIT(2)
+#define CXL_DECODER_F_TYPE3 BIT(3)
+#define CXL_DECODER_F_LOCK  BIT(4)
+#define CXL_DECODER_F_MASK  GENMASK(4, 0)
+
+enum cxl_decoder_type {
+       CXL_DECODER_ACCELERATOR = 2,
+       CXL_DECODER_EXPANDER = 3,
+};
+
+/**
+ * struct cxl_decoder - CXL address range decode configuration
+ * @range: address range considered by this decoder
+ * @id: kernel device name id
+ * @interleave_ways: number of cxl_dports in this decode
+ * @interleave_granularity: data stride per dport
+ * @target_type: accelerator vs expander (type2 vs type3) selector
+ * @flags: memory type capabilities and locking
+ * @target: active ordered target list in current decoder configuration
+ */
+struct cxl_decoder {
+	struct device dev;
+	int id;
+	struct range range;
+	int interleave_ways;
+	int interleave_granularity;
+	enum cxl_decoder_type target_type;
+	unsigned long flags;
+	struct cxl_dport *target[];
+};
+
 /**
  * struct cxl_port - logical collection of upstream port devices and
  *		     downstream port devices to construct a CXL memory
@@ -156,6 +194,7 @@ int cxl_map_device_regs(struct pci_dev *pdev,
  * @uport: PCI or platform device implementing the upstream port capability
  * @id: id for port device-name
  * @dports: cxl_dport instances referenced by decoders
+ * @decoder_ida: allocator for decoder ids
  * @component_regs_phys: component register capability base address (optional)
  */
 struct cxl_port {
@@ -163,6 +202,7 @@ struct cxl_port {
 	struct device *uport;
 	int id;
 	struct list_head dports;
+	struct ida decoder_ida;
 	resource_size_t component_reg_phys;
 };
 
@@ -189,5 +229,13 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
 
 int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
 		  resource_size_t component_reg_phys);
+
+struct cxl_decoder *to_cxl_decoder(struct device *dev);
+struct cxl_decoder *
+devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
+		     resource_size_t base, resource_size_t len,
+		     int interleave_ways, int interleave_granularity,
+		     enum cxl_decoder_type type, unsigned long flags);
+
 extern struct bus_type cxl_bus_type;
 #endif /* __CXL_H__ */

Re: [PATCH v5 6/6] cxl/acpi: Introduce cxl_decoder objects

[Jonathan Cameron]

On Sat, 5 Jun 2021 23:05:27 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> A cxl_decoder is a child of a cxl_port. It represents a hardware
> decoder configuration of an upstream port to one or more of its
> downstream ports. The decoder is either represented in standard HDM
> decoder registers (see CXL 2.0 section 8.2.5.12 CXL HDM Decoder
> Capability Structure), or it is a static decode configuration
> communicated by platform firmware (see the CXL Early Discovery Table:
> Fixed Memory Window Structure).
> 
> The firmware described and hardware described decoders differ slightly
> leading to 2 different sub-types of decoders, cxl_decoder_root and
> cxl_decoder_switch. At the root level the decode capabilities restrict
> what can be mapped beneath them. Mid-level switch decoders are
> configured for either acclerator (type-2) or memory-expander (type-3)
> operation, but they are otherwise agnostic to the type of memory
> (volatile vs persistent) being mapped.
> 
> Here is an example topology from a single-ported host-bridge environment
> without CFMWS decodes enumerated.
> 
> /sys/bus/cxl/devices/root0
> ├── devtype
> ├── dport0 -> ../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
> ├── port1
> │   ├── decoder1.0
> │   │   ├── devtype
> │   │   ├── end
> │   │   ├── locked
> │   │   ├── start
> │   │   ├── subsystem -> ../../../../bus/cxl
> │   │   ├── target_list
> │   │   ├── target_type
> │   │   └── uevent
> │   ├── devtype
> │   ├── dport0 -> ../../pci0000:34/0000:34:00.0
> │   ├── subsystem -> ../../../bus/cxl
> │   ├── uevent
> │   └── uport -> ../../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
> ├── subsystem -> ../../bus/cxl
> ├── uevent
> └── uport -> ../platform/ACPI0017:00
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

One trivial docs issue and a suggestion that -2 is a bit too magic.
Otherwise looks good to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |   70 ++++++++
>  drivers/cxl/acpi.c                      |   21 ++
>  drivers/cxl/core.c                      |  265 +++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h                       |   48 ++++++
>  4 files changed, 403 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 0cb31b7ad17b..f16f18e77578 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -48,3 +48,73 @@ Description:
>  		decode of CXL memory resources.  The 'Y' integer reflects the
>  		hardware port unique-id used in the hardware decoder target
>  		list.
> +
> +What:		/sys/bus/cxl/devices/decoderX.Y
> +Date:		June, 2021
> +KernelVersion:	v5.14
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		CXL decoder objects are enumerated from either a platform
> +		firmware description, or a CXL HDM decoder register set in a
> +		PCIe device (see CXL 2.0 section 8.2.5.12 CXL HDM Decoder
> +		Capability Structure). The 'X' in decoderX.Y represents the
> +		cxl_port container of this decoder, and 'Y' represents the
> +		instance id of a given decoder resource.
> +
> +What:		/sys/bus/cxl/devices/decoderX.Y/{start,end}
> +Date:		June, 2021
> +KernelVersion:	v5.14
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		The 'start' and 'end' attributes together convey the physical
> +		address base and last addressable byte of the decoder's decode
> +		window. For decoders of devtype "cxl_decoder_root" the address
> +		range is fixed. For decoders of devtype "cxl_decoder_switch" the
> +		address is bounded by the decode range of the cxl_port ancestor
> +		of the decoder's cxl_port, and dynamically updates based on the
> +		active memory regions in that address space.
> +
> +What:		/sys/bus/cxl/devices/decoderX.Y/locked
> +Date:		June, 2021
> +KernelVersion:	v5.14
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		CXL HDM decoders have the capability to lock the configuration
> +		until the next device reset. For decoders of devtype
> +		"cxl_decoder_root" there is no standard facility to unlock them.
> +		For decoders of devtype "cxl_decoder_switch" a secondary bus
> +		reset, of the PCIe bridge that provides the bus for this
> +		decoders uport, unlocks / resets the decoder.
> +
> +What:		/sys/bus/cxl/devices/decoderX.Y/target_list
> +Date:		June, 2021
> +KernelVersion:	v5.14
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		Display a comma separated list of the current decoder target
> +		configuration. The list is ordered by the current configured
> +		interleave order of the decoder's dport instances. Each entry in
> +		the list is a dport id.
> +
> +What:		/sys/bus/cxl/devices/decoderX.Y/cap_{pmem,ram,type2,type3}
> +Date:		June, 2021
> +KernelVersion:	v5.14
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		When a CXL decoder is of devtype "cxl_decoder_root", it
> +		represents a fixed memory window identified by platform
> +		firmware. A fixed window may only support a subset of memory
> +		types. The 'cap_*' attributes indicate whether persistent
> +		memory, volatile memory, accelerator memory, and / or expander
> +		memory may be mapped behind this decoder's memory window.
> +
> +What:		/sys/bus/cxl/devices/decoderX.Y/target_type
> +Date:		June, 2021
> +KernelVersion:	v5.14
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		When a CXL decoder is of devtype "cxl_decoder_switch", it can
> +		optionally decode either accelerator memory (type-2) or expander
> +		memory (type-3). The 'target_type' attribute indicates the
> +		current setting which may dynamically change based on what
> +		memory regions are activated in this decode hierarchy.

Nice docs.

> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index ec324bf063b8..6f203ba7fc1d 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -70,6 +70,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  	struct device *host = root_port->dev.parent;
>  	struct acpi_pci_root *pci_root;
>  	struct cxl_walk_context ctx;
> +	struct cxl_decoder *cxld;
>  	struct cxl_port *port;
>  
>  	if (!bridge)
> @@ -94,7 +95,25 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  
>  	if (ctx.count == 0)
>  		return -ENODEV;
> -	return ctx.error;
> +	if (ctx.error)
> +		return ctx.error;
> +
> +	/* TODO: Scan CHBCR for HDM Decoder resources */
> +
> +	/*
> +	 * In the single-port host-bridge case there are no HDM decoders
> +	 * in the CHBCR and a 1:1 passthrough decode is implied.
> +	 */
> +	if (ctx.count == 1) {
> +		cxld = devm_cxl_add_decoder(host, port, 1, 0, -2, 1, 0,

-2?  I'm guessing that has some special meaning and perhaps warrants
a nice define to let us know what that is.

Given this interface is a bit long perhaps even wroth a comment here on
why the values are what they are?

> +					    CXL_DECODER_EXPANDER, 0);
> +		if (IS_ERR(cxld))
> +			return PTR_ERR(cxld);
> +
> +		dev_dbg(host, "add: %s\n", dev_name(&cxld->dev));
> +	}
> +
> +	return 0;
>  }
>  
>  static int add_host_bridge_dport(struct device *match, void *arg)
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index 4418b30cce4f..781efe51f186 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -33,6 +33,168 @@ static struct attribute_group cxl_base_attribute_group = {
>  	.attrs = cxl_base_attributes,
>  };
>  
> +static ssize_t start_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct cxl_decoder *cxld = to_cxl_decoder(dev);
> +
> +	return sysfs_emit(buf, "%#llx\n", cxld->range.start);
> +}
> +static DEVICE_ATTR_RO(start);
> +
> +static ssize_t end_show(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct cxl_decoder *cxld = to_cxl_decoder(dev);
> +
> +	return sysfs_emit(buf, "%#llx\n", cxld->range.end);
> +}
> +static DEVICE_ATTR_RO(end);
> +
> +#define CXL_DECODER_FLAG_ATTR(name, flag)                            \
> +static ssize_t name##_show(struct device *dev,                       \
> +			   struct device_attribute *attr, char *buf) \
> +{                                                                    \
> +	struct cxl_decoder *cxld = to_cxl_decoder(dev);              \
> +                                                                     \
> +	return sysfs_emit(buf, "%s\n",                               \
> +			  (cxld->flags & (flag)) ? "1" : "0");       \
> +}                                                                    \
> +static DEVICE_ATTR_RO(name)
> +
> +CXL_DECODER_FLAG_ATTR(cap_pmem, CXL_DECODER_F_PMEM);
> +CXL_DECODER_FLAG_ATTR(cap_ram, CXL_DECODER_F_RAM);
> +CXL_DECODER_FLAG_ATTR(cap_type2, CXL_DECODER_F_TYPE2);
> +CXL_DECODER_FLAG_ATTR(cap_type3, CXL_DECODER_F_TYPE3);
> +CXL_DECODER_FLAG_ATTR(locked, CXL_DECODER_F_LOCK);
> +
> +static ssize_t target_type_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct cxl_decoder *cxld = to_cxl_decoder(dev);
> +
> +	switch (cxld->target_type) {
> +	case CXL_DECODER_ACCELERATOR:
> +		return sysfs_emit(buf, "accelerator\n");
> +	case CXL_DECODER_EXPANDER:
> +		return sysfs_emit(buf, "expander\n");
> +	}
> +	return -ENXIO;
> +}
> +DEVICE_ATTR_RO(target_type);
> +
> +static ssize_t target_list_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct cxl_decoder *cxld = to_cxl_decoder(dev);
> +	ssize_t offset = 0;
> +	int i, rc = 0;
> +
> +	device_lock(dev);
> +	for (i = 0; i < cxld->interleave_ways; i++) {
> +		struct cxl_dport *dport = cxld->target[i];
> +		struct cxl_dport *next = NULL;
> +
> +		if (!dport)
> +			break;
> +
> +		if (i + 1 < cxld->interleave_ways)
> +			next = cxld->target[i + 1];
> +		rc = sysfs_emit_at(buf, offset, "%d%s", dport->port_id,
> +				   next ? "," : "");
> +		if (rc < 0)
> +			break;
> +		offset += rc;
> +	}
> +	device_unlock(dev);
> +
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = sysfs_emit_at(buf, offset, "\n");
> +	if (rc < 0)
> +		return rc;
> +
> +	return offset + rc;
> +}
> +DEVICE_ATTR_RO(target_list);
> +
> +static struct attribute *cxl_decoder_base_attrs[] = {
> +	&dev_attr_start.attr,
> +	&dev_attr_end.attr,
> +	&dev_attr_locked.attr,
> +	&dev_attr_target_list.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group cxl_decoder_base_attribute_group = {
> +	.attrs = cxl_decoder_base_attrs,
> +};
> +
> +static struct attribute *cxl_decoder_root_attrs[] = {
> +	&dev_attr_cap_pmem.attr,
> +	&dev_attr_cap_ram.attr,
> +	&dev_attr_cap_type2.attr,
> +	&dev_attr_cap_type3.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group cxl_decoder_root_attribute_group = {
> +	.attrs = cxl_decoder_root_attrs,
> +};
> +
> +static const struct attribute_group *cxl_decoder_root_attribute_groups[] = {
> +	&cxl_decoder_root_attribute_group,
> +	&cxl_decoder_base_attribute_group,
> +	&cxl_base_attribute_group,
> +	NULL,
> +};
> +
> +static struct attribute *cxl_decoder_switch_attrs[] = {
> +	&dev_attr_target_type.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group cxl_decoder_switch_attribute_group = {
> +	.attrs = cxl_decoder_switch_attrs,
> +};
> +
> +static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = {
> +	&cxl_decoder_switch_attribute_group,
> +	&cxl_decoder_base_attribute_group,
> +	&cxl_base_attribute_group,
> +	NULL,
> +};
> +
> +static void cxl_decoder_release(struct device *dev)
> +{
> +	struct cxl_decoder *cxld = to_cxl_decoder(dev);
> +	struct cxl_port *port = to_cxl_port(dev->parent);
> +
> +	ida_free(&port->decoder_ida, cxld->id);
> +	kfree(cxld);
> +}
> +
> +static const struct device_type cxl_decoder_switch_type = {
> +	.name = "cxl_decoder_switch",
> +	.release = cxl_decoder_release,
> +	.groups = cxl_decoder_switch_attribute_groups,
> +};
> +
> +static const struct device_type cxl_decoder_root_type = {
> +	.name = "cxl_decoder_root",
> +	.release = cxl_decoder_release,
> +	.groups = cxl_decoder_root_attribute_groups,
> +};
> +
> +struct cxl_decoder *to_cxl_decoder(struct device *dev)
> +{
> +	if (dev_WARN_ONCE(dev, dev->type->release != cxl_decoder_release,
> +			  "not a cxl_decoder device\n"))
> +		return NULL;
> +	return container_of(dev, struct cxl_decoder, dev);
> +}
> +
>  static void cxl_dport_release(struct cxl_dport *dport)
>  {
>  	put_device(dport->dport);
> @@ -138,6 +300,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
>  
>  	port->uport = uport;
>  	port->component_reg_phys = component_reg_phys;
> +	ida_init(&port->decoder_ida);
>  	INIT_LIST_HEAD(&port->dports);
>  
>  	device_initialize(dev);
> @@ -262,6 +425,108 @@ int cxl_add_dport(struct cxl_port *port, struct device *dport_dev, int port_id,
>  }
>  EXPORT_SYMBOL_GPL(cxl_add_dport);
>  
> +static struct cxl_decoder *
> +cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> +		  resource_size_t len, int interleave_ways,
> +		  int interleave_granularity, enum cxl_decoder_type type,
> +		  unsigned long flags)
> +{
> +	struct cxl_decoder *cxld;
> +	struct device *dev;
> +	int rc = 0;
> +
> +	if (interleave_ways < 1)
> +		return ERR_PTR(-EINVAL);
> +
> +	device_lock(&port->dev);
> +	if (list_empty(&port->dports))
> +		rc = -EINVAL;
> +	device_unlock(&port->dev);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
> +	if (!cxld)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rc = ida_alloc(&port->decoder_ida, GFP_KERNEL);
> +	if (rc < 0)
> +		goto err;
> +
> +	*cxld = (struct cxl_decoder) {
> +		.id = rc,
> +		.range = {
> +			.start = base,
> +			.end = base + len - 1,
> +		},
> +		.flags = flags,
> +		.interleave_ways = interleave_ways,
> +		.interleave_granularity = interleave_granularity,
> +		.target_type = type,
> +	};
> +
> +	/* handle implied target_list */
> +	if (interleave_ways == 1)
> +		cxld->target[0] =
> +			list_first_entry(&port->dports, struct cxl_dport, list);
> +	dev = &cxld->dev;
> +	device_initialize(dev);
> +	device_set_pm_not_required(dev);
> +	dev->parent = &port->dev;
> +	dev->bus = &cxl_bus_type;
> +
> +	/* root ports do not have a cxl_port_type parent */
> +	if (port->dev.parent->type == &cxl_port_type)
> +		dev->type = &cxl_decoder_switch_type;
> +	else
> +		dev->type = &cxl_decoder_root_type;
> +
> +	return cxld;
> +err:
> +	kfree(cxld);
> +	return ERR_PTR(rc);
> +}
> +
> +static void unregister_dev(void *dev)
> +{
> +	device_unregister(dev);
> +}
> +
> +struct cxl_decoder *
> +devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
> +		     resource_size_t base, resource_size_t len,
> +		     int interleave_ways, int interleave_granularity,
> +		     enum cxl_decoder_type type, unsigned long flags)
> +{
> +	struct cxl_decoder *cxld;
> +	struct device *dev;
> +	int rc;
> +
> +	cxld = cxl_decoder_alloc(port, nr_targets, base, len, interleave_ways,
> +				 interleave_granularity, type, flags);
> +	if (IS_ERR(cxld))
> +		return cxld;
> +
> +	dev = &cxld->dev;
> +	rc = dev_set_name(dev, "decoder%d.%d", port->id, cxld->id);
> +	if (rc)
> +		goto err;
> +
> +	rc = device_add(dev);
> +	if (rc)
> +		goto err;
> +
> +	rc = devm_add_action_or_reset(host, unregister_dev, dev);
> +	if (rc)
> +		return ERR_PTR(rc);
> +	return cxld;
> +
> +err:
> +	put_device(dev);
> +	return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_GPL(devm_cxl_add_decoder);
> +
>  /**
>   * cxl_probe_component_regs() - Detect CXL Component register blocks
>   * @dev: Host device of the @base mapping
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 0b89fcfe728a..f350c92d37d2 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -148,6 +148,44 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
>  #define CXL_TARGET_STRLEN 20
>  
> +/*
> + * cxl_decoder flags that define the type of memory / devices this
> + * decoder supports as well as configuration lock status See "CXL 2.0
> + * 8.2.5.12.7 CXL HDM Decoder 0 Control Register" for details.
> + */
> +#define CXL_DECODER_F_RAM   BIT(0)
> +#define CXL_DECODER_F_PMEM  BIT(1)
> +#define CXL_DECODER_F_TYPE2 BIT(2)
> +#define CXL_DECODER_F_TYPE3 BIT(3)
> +#define CXL_DECODER_F_LOCK  BIT(4)
> +#define CXL_DECODER_F_MASK  GENMASK(4, 0)
> +
> +enum cxl_decoder_type {
> +       CXL_DECODER_ACCELERATOR = 2,
> +       CXL_DECODER_EXPANDER = 3,
> +};
> +
> +/**
> + * struct cxl_decoder - CXL address range decode configuration

Parameters in different order from in structure and not all there.

> + * @range: address range considered by this decoder
> + * @id: kernel device name id
> + * @interleave_ways: number of cxl_dports in this decode
> + * @interleave_granularity: data stride per dport
> + * @target_type: accelerator vs expander (type2 vs type3) selector
> + * @flags: memory type capabilities and locking
> + * @target: active ordered target list in current decoder configuration
> + */
> +struct cxl_decoder {
> +	struct device dev;
> +	int id;
> +	struct range range;
> +	int interleave_ways;
> +	int interleave_granularity;
> +	enum cxl_decoder_type target_type;
> +	unsigned long flags;
> +	struct cxl_dport *target[];
> +};
> +
>  /**
>   * struct cxl_port - logical collection of upstream port devices and
>   *		     downstream port devices to construct a CXL memory
> @@ -156,6 +194,7 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>   * @uport: PCI or platform device implementing the upstream port capability
>   * @id: id for port device-name
>   * @dports: cxl_dport instances referenced by decoders
> + * @decoder_ida: allocator for decoder ids
>   * @component_regs_phys: component register capability base address (optional)
>   */
>  struct cxl_port {
> @@ -163,6 +202,7 @@ struct cxl_port {
>  	struct device *uport;
>  	int id;
>  	struct list_head dports;
> +	struct ida decoder_ida;
>  	resource_size_t component_reg_phys;
>  };
>  
> @@ -189,5 +229,13 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
>  
>  int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
>  		  resource_size_t component_reg_phys);
> +
> +struct cxl_decoder *to_cxl_decoder(struct device *dev);
> +struct cxl_decoder *
> +devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
> +		     resource_size_t base, resource_size_t len,
> +		     int interleave_ways, int interleave_granularity,
> +		     enum cxl_decoder_type type, unsigned long flags);
> +
>  extern struct bus_type cxl_bus_type;
>  #endif /* __CXL_H__ */
> 

Re: [PATCH v5 6/6] cxl/acpi: Introduce cxl_decoder objects

[Dan Williams]

On Tue, Jun 8, 2021 at 6:06 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Sat, 5 Jun 2021 23:05:27 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > A cxl_decoder is a child of a cxl_port. It represents a hardware
> > decoder configuration of an upstream port to one or more of its
> > downstream ports. The decoder is either represented in standard HDM
> > decoder registers (see CXL 2.0 section 8.2.5.12 CXL HDM Decoder
> > Capability Structure), or it is a static decode configuration
> > communicated by platform firmware (see the CXL Early Discovery Table:
> > Fixed Memory Window Structure).
> >
> > The firmware described and hardware described decoders differ slightly
> > leading to 2 different sub-types of decoders, cxl_decoder_root and
> > cxl_decoder_switch. At the root level the decode capabilities restrict
> > what can be mapped beneath them. Mid-level switch decoders are
> > configured for either acclerator (type-2) or memory-expander (type-3)
> > operation, but they are otherwise agnostic to the type of memory
> > (volatile vs persistent) being mapped.
> >
> > Here is an example topology from a single-ported host-bridge environment
> > without CFMWS decodes enumerated.
> >
> > /sys/bus/cxl/devices/root0
> > ├── devtype
> > ├── dport0 -> ../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
> > ├── port1
> > │   ├── decoder1.0
> > │   │   ├── devtype
> > │   │   ├── end
> > │   │   ├── locked
> > │   │   ├── start
> > │   │   ├── subsystem -> ../../../../bus/cxl
> > │   │   ├── target_list
> > │   │   ├── target_type
> > │   │   └── uevent
> > │   ├── devtype
> > │   ├── dport0 -> ../../pci0000:34/0000:34:00.0
> > │   ├── subsystem -> ../../../bus/cxl
> > │   ├── uevent
> > │   └── uport -> ../../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
> > ├── subsystem -> ../../bus/cxl
> > ├── uevent
> > └── uport -> ../platform/ACPI0017:00
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> One trivial docs issue and a suggestion that -2 is a bit too magic.
> Otherwise looks good to me.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-cxl |   70 ++++++++
> >  drivers/cxl/acpi.c                      |   21 ++
> >  drivers/cxl/core.c                      |  265 +++++++++++++++++++++++++++++++
> >  drivers/cxl/cxl.h                       |   48 ++++++
> >  4 files changed, 403 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > index 0cb31b7ad17b..f16f18e77578 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cxl
> > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > @@ -48,3 +48,73 @@ Description:
> >               decode of CXL memory resources.  The 'Y' integer reflects the
> >               hardware port unique-id used in the hardware decoder target
> >               list.
> > +
> > +What:                /sys/bus/cxl/devices/decoderX.Y
> > +Date:                June, 2021
> > +KernelVersion:       v5.14
> > +Contact:     linux-cxl@vger.kernel.org
> > +Description:
> > +             CXL decoder objects are enumerated from either a platform
> > +             firmware description, or a CXL HDM decoder register set in a
> > +             PCIe device (see CXL 2.0 section 8.2.5.12 CXL HDM Decoder
> > +             Capability Structure). The 'X' in decoderX.Y represents the
> > +             cxl_port container of this decoder, and 'Y' represents the
> > +             instance id of a given decoder resource.
> > +
> > +What:                /sys/bus/cxl/devices/decoderX.Y/{start,end}
> > +Date:                June, 2021
> > +KernelVersion:       v5.14
> > +Contact:     linux-cxl@vger.kernel.org
> > +Description:
> > +             The 'start' and 'end' attributes together convey the physical
> > +             address base and last addressable byte of the decoder's decode
> > +             window. For decoders of devtype "cxl_decoder_root" the address
> > +             range is fixed. For decoders of devtype "cxl_decoder_switch" the
> > +             address is bounded by the decode range of the cxl_port ancestor
> > +             of the decoder's cxl_port, and dynamically updates based on the
> > +             active memory regions in that address space.
> > +
> > +What:                /sys/bus/cxl/devices/decoderX.Y/locked
> > +Date:                June, 2021
> > +KernelVersion:       v5.14
> > +Contact:     linux-cxl@vger.kernel.org
> > +Description:
> > +             CXL HDM decoders have the capability to lock the configuration
> > +             until the next device reset. For decoders of devtype
> > +             "cxl_decoder_root" there is no standard facility to unlock them.
> > +             For decoders of devtype "cxl_decoder_switch" a secondary bus
> > +             reset, of the PCIe bridge that provides the bus for this
> > +             decoders uport, unlocks / resets the decoder.
> > +
> > +What:                /sys/bus/cxl/devices/decoderX.Y/target_list
> > +Date:                June, 2021
> > +KernelVersion:       v5.14
> > +Contact:     linux-cxl@vger.kernel.org
> > +Description:
> > +             Display a comma separated list of the current decoder target
> > +             configuration. The list is ordered by the current configured
> > +             interleave order of the decoder's dport instances. Each entry in
> > +             the list is a dport id.
> > +
> > +What:                /sys/bus/cxl/devices/decoderX.Y/cap_{pmem,ram,type2,type3}
> > +Date:                June, 2021
> > +KernelVersion:       v5.14
> > +Contact:     linux-cxl@vger.kernel.org
> > +Description:
> > +             When a CXL decoder is of devtype "cxl_decoder_root", it
> > +             represents a fixed memory window identified by platform
> > +             firmware. A fixed window may only support a subset of memory
> > +             types. The 'cap_*' attributes indicate whether persistent
> > +             memory, volatile memory, accelerator memory, and / or expander
> > +             memory may be mapped behind this decoder's memory window.
> > +
> > +What:                /sys/bus/cxl/devices/decoderX.Y/target_type
> > +Date:                June, 2021
> > +KernelVersion:       v5.14
> > +Contact:     linux-cxl@vger.kernel.org
> > +Description:
> > +             When a CXL decoder is of devtype "cxl_decoder_switch", it can
> > +             optionally decode either accelerator memory (type-2) or expander
> > +             memory (type-3). The 'target_type' attribute indicates the
> > +             current setting which may dynamically change based on what
> > +             memory regions are activated in this decode hierarchy.
>
> Nice docs.
>
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index ec324bf063b8..6f203ba7fc1d 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -70,6 +70,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> >       struct device *host = root_port->dev.parent;
> >       struct acpi_pci_root *pci_root;
> >       struct cxl_walk_context ctx;
> > +     struct cxl_decoder *cxld;
> >       struct cxl_port *port;
> >
> >       if (!bridge)
> > @@ -94,7 +95,25 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> >
> >       if (ctx.count == 0)
> >               return -ENODEV;
> > -     return ctx.error;
> > +     if (ctx.error)
> > +             return ctx.error;
> > +
> > +     /* TODO: Scan CHBCR for HDM Decoder resources */
> > +
> > +     /*
> > +      * In the single-port host-bridge case there are no HDM decoders
> > +      * in the CHBCR and a 1:1 passthrough decode is implied.
> > +      */
> > +     if (ctx.count == 1) {
> > +             cxld = devm_cxl_add_decoder(host, port, 1, 0, -2, 1, 0,
>
> -2?  I'm guessing that has some special meaning and perhaps warrants
> a nice define to let us know what that is.

Um, yes, I think I went back and forth on whether this should be a
zero-length range starting at zero or a zero-length range starting at
UINT64_MAX, and I ended up botching it with a range of 0 to UINT64_MAX
- 1. I'll fix this up to provide a "passthrough" version of
devm_cxl_add_decoder() for the cases like this where a port does not
adjust the decode from the parent port down to the next level.

>
> Given this interface is a bit long perhaps even wroth a comment here on
> why the values are what they are?

The passthrough helper will address this.