[PATCH v6 00/12] cxl: Add support for Restricted CXL hosts (RCD mode)

[PATCH v6 00/12] cxl: Add support for Restricted CXL hosts (RCD mode)

[Dan Williams]

Changes since v4 [1] (v5 was a partial re-roll)
- Add commentary in cxl_nvd_unregister() to clarify the locking
  (Jonathan)
- Improve the comment about why there are 2 devm_add_action_or_reset()
  calls in devm_cxl_add_nvdimm() (Jonathan)
- Fix devm_cxl_add_nvdimm() error exit ordering to mirror the init order
  (Jonathan)
- Add a comment to clarify why the nvdimm bridge lookup only needs to be
  performed on one device in the region in cxl_pmem_region_alloc()
  (Jonathan)
- Add a comment to clarify the locking in cxlr_pmem_unregister()
  (Jonathan)
- cxl_nvdimm_bridge_probe() whitespace fixup (Jonathan)
- Exit add_host_bridge_uport() before devm_cxl_register_pci_bus() since
  that registration is unused in the RCH case (Robert)
- Fold RCRB retrieval into cxl_get_chbcr() (Robert)
- Drop 'enum cxl_dport_mode' and just assume it based on whether @rcrb
  is valid (Robert)
- Add kdoc for devm_cxl_add_rch_dport() (Robert)
- Move Upstream Port RCRB detection to offset0 returning < U32_MAX (Robert)
- Add an error message for failure to access Downstream Port RCRB
  (Robert)
- Improve changelog for "[PATCH v6 9/12] cxl/mem: Move
  devm_cxl_add_endpoint() from cxl_core to cxl_mem" (Robert)
- Drop unnecessary movement of @dport and @port variable declarations
  (Robert)
- Fix cxl_mem call to cxl_rcrb_to_component() to use CXL_RCRB_UPSTREAM
  (Dave)
- Fix up changelog of "[PATCH v6 12/12] cxl/acpi: Set ACPI's CXL _OSC to
  indicate CXL1.1 support" (Robert)

[1]: http://lore.kernel.org/r/166931487492.2104015.15204324083515120776.stgit@dwillia2-xfh.jf.intel.com

I note that the cxl_pmem_wq still appears to be proving difficult to
review. Open to ideas about how to move that along. Otherwise, since folks
seem to be happy with "[PATCH v6 10/12] cxl/port: Add RCD endpoint port
enumeration" that depends on that rework, I might just move ahead with
that implict ack, but do holler if anything looks amiss.

---

>From [PATCH v6 10/12] cxl/port: Add RCD endpoint port enumeration

Unlike a CXL memory expander in a VH topology that has at least one
intervening 'struct cxl_port' instance between itself and the CXL root
device, an RCD attaches one-level higher. For example:

               VH
          ┌──────────┐
          │ ACPI0017 │
          │  root0   │
          └─────┬────┘
                │
          ┌─────┴────┐
          │  dport0  │
    ┌─────┤ ACPI0016 ├─────┐
    │     │  port1   │     │
    │     └────┬─────┘     │
    │          │           │
 ┌──┴───┐   ┌──┴───┐   ┌───┴──┐
 │dport0│   │dport1│   │dport2│
 │ RP0  │   │ RP1  │   │ RP2  │
 └──────┘   └──┬───┘   └──────┘
               │
           ┌───┴─────┐
           │endpoint0│
           │  port2  │
           └─────────┘

...vs:

              RCH
          ┌──────────┐
          │ ACPI0017 │
          │  root0   │
          └────┬─────┘
               │
           ┌───┴────┐
           │ dport0 │
           │ACPI0016│
           └───┬────┘
               │
          ┌────┴─────┐
          │endpoint0 │
          │  port1   │
          └──────────┘

So arrange for endpoint port in the RCH/RCD case to appear directly
connected to the host-bridge in its singular role as a dport. Compare
that to the VH case where the host-bridge serves a dual role as a
'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for
the Root Ports in the Root Complex that are modeled as 'cxl_dport'
instances in the CXL topology.

Another deviation from the VH case is that RCDs may need to look up
their component registers from the Root Complex Register Block (RCRB).
That platform firmware specified RCRB area is cached by the cxl_acpi
driver and conveyed via the host-bridge dport to the cxl_mem driver to
perform the cxl_rcrb_to_component() lookup for the endpoint port
(See 9.11.8 CXL Devices Attached to an RCH for the lookup of the
upstream port component registers).

---

Dan Williams (9):
      cxl/acpi: Simplify cxl_nvdimm_bridge probing
      cxl/region: Drop redundant pmem region release handling
      cxl/pmem: Refactor nvdimm device registration, delete the workqueue
      cxl/pmem: Remove the cxl_pmem_wq and related infrastructure
      cxl/acpi: Move rescan to the workqueue
      tools/testing/cxl: Make mock CEDT parsing more robust
      cxl/mem: Move devm_cxl_add_endpoint() from cxl_core to cxl_mem
      cxl/port: Add RCD endpoint port enumeration
      tools/testing/cxl: Add an RCH topology

Robert Richter (2):
      cxl/ACPI: Register CXL host ports by bridge device
      cxl/acpi: Extract component registers of restricted hosts from RCRB

Terry Bowman (1):
      cxl/acpi: Set ACPI's CXL _OSC to indicate RCD mode support


 drivers/acpi/pci_root.c       |    1 
 drivers/cxl/acpi.c            |  102 +++++++++---
 drivers/cxl/core/core.h       |    8 -
 drivers/cxl/core/pmem.c       |  102 ++++++++----
 drivers/cxl/core/port.c       |  118 ++++++++------
 drivers/cxl/core/region.c     |   64 +++++++
 drivers/cxl/core/regs.c       |   64 +++++++
 drivers/cxl/cxl.h             |   46 +++--
 drivers/cxl/cxlmem.h          |   15 ++
 drivers/cxl/mem.c             |   72 ++++++++
 drivers/cxl/pci.c             |   13 +-
 drivers/cxl/pmem.c            |  351 +++++------------------------------------
 tools/testing/cxl/Kbuild      |    1 
 tools/testing/cxl/test/cxl.c  |  171 ++++++++++++++++++--
 tools/testing/cxl/test/mem.c  |   40 ++++-
 tools/testing/cxl/test/mock.c |   19 ++
 tools/testing/cxl/test/mock.h |    3 
 17 files changed, 711 insertions(+), 479 deletions(-)

base-commit: 3b39fd6cf12ceda2a2582dcb9b9ee9f4d197b857

[PATCH v6 01/12] cxl/acpi: Simplify cxl_nvdimm_bridge probing

[Dan Williams]

The 'struct cxl_nvdimm_bridge' object advertises platform CXL PMEM
resources. It coordinates with libnvdimm to attach nvdimm devices and
regions for each corresponding CXL object. That coordination is
complicated, i.e. difficult to reason about, and it turns out redundant.
It is already the case that the CXL core knows how to tear down a
cxl_region when a cxl_memdev goes through ->remove(), so that pathway
can be extended to directly cleanup cxl_nvdimm and cxl_pmem_region
objects.

Towards the goal of ripping out the cxl_nvdimm_bridge state machine,
arrange for cxl_acpi to optionally pre-load the cxl_pmem driver so that
the nvdimm bridge is active synchronously with
devm_cxl_add_nvdimm_bridge(), and remove all the bind attributes for the
cxl_nvdimm* objects since the cxl root device and cxl_memdev bind
attributes are sufficient.

Tested-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/acpi.c |    1 +
 drivers/cxl/pmem.c |    9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index fb9f72813067..c540da0cbf1e 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -539,3 +539,4 @@ module_platform_driver(cxl_acpi_driver);
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(CXL);
 MODULE_IMPORT_NS(ACPI);
+MODULE_SOFTDEP("pre: cxl_pmem");
diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index 4c627d67281a..946e171e7d4a 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -99,6 +99,9 @@ static struct cxl_driver cxl_nvdimm_driver = {
 	.name = "cxl_nvdimm",
 	.probe = cxl_nvdimm_probe,
 	.id = CXL_DEVICE_NVDIMM,
+	.drv = {
+		.suppress_bind_attrs = true,
+	},
 };
 
 static int cxl_pmem_get_config_size(struct cxl_dev_state *cxlds,
@@ -360,6 +363,9 @@ static struct cxl_driver cxl_nvdimm_bridge_driver = {
 	.probe = cxl_nvdimm_bridge_probe,
 	.remove = cxl_nvdimm_bridge_remove,
 	.id = CXL_DEVICE_NVDIMM_BRIDGE,
+	.drv = {
+		.suppress_bind_attrs = true,
+	},
 };
 
 static int match_cxl_nvdimm(struct device *dev, void *data)
@@ -583,6 +589,9 @@ static struct cxl_driver cxl_pmem_region_driver = {
 	.name = "cxl_pmem_region",
 	.probe = cxl_pmem_region_probe,
 	.id = CXL_DEVICE_PMEM_REGION,
+	.drv = {
+		.suppress_bind_attrs = true,
+	},
 };
 
 /*

Re: [PATCH v6 01/12] cxl/acpi: Simplify cxl_nvdimm_bridge probing

[Jonathan Cameron]

On Thu, 01 Dec 2022 13:33:26 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> The 'struct cxl_nvdimm_bridge' object advertises platform CXL PMEM
> resources. It coordinates with libnvdimm to attach nvdimm devices and
> regions for each corresponding CXL object. That coordination is
> complicated, i.e. difficult to reason about, and it turns out redundant.
> It is already the case that the CXL core knows how to tear down a
> cxl_region when a cxl_memdev goes through ->remove(), so that pathway
> can be extended to directly cleanup cxl_nvdimm and cxl_pmem_region
> objects.
> 
> Towards the goal of ripping out the cxl_nvdimm_bridge state machine,
> arrange for cxl_acpi to optionally pre-load the cxl_pmem driver so that
> the nvdimm bridge is active synchronously with
> devm_cxl_add_nvdimm_bridge(), and remove all the bind attributes for the
> cxl_nvdimm* objects since the cxl root device and cxl_memdev bind
> attributes are sufficient.
> 
> Tested-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Seems reasonable and I can't see a disadvantage in doing this...
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  drivers/cxl/acpi.c |    1 +
>  drivers/cxl/pmem.c |    9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index fb9f72813067..c540da0cbf1e 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -539,3 +539,4 @@ module_platform_driver(cxl_acpi_driver);
>  MODULE_LICENSE("GPL v2");
>  MODULE_IMPORT_NS(CXL);
>  MODULE_IMPORT_NS(ACPI);
> +MODULE_SOFTDEP("pre: cxl_pmem");
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index 4c627d67281a..946e171e7d4a 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -99,6 +99,9 @@ static struct cxl_driver cxl_nvdimm_driver = {
>  	.name = "cxl_nvdimm",
>  	.probe = cxl_nvdimm_probe,
>  	.id = CXL_DEVICE_NVDIMM,
> +	.drv = {
> +		.suppress_bind_attrs = true,
> +	},
>  };
>  
>  static int cxl_pmem_get_config_size(struct cxl_dev_state *cxlds,
> @@ -360,6 +363,9 @@ static struct cxl_driver cxl_nvdimm_bridge_driver = {
>  	.probe = cxl_nvdimm_bridge_probe,
>  	.remove = cxl_nvdimm_bridge_remove,
>  	.id = CXL_DEVICE_NVDIMM_BRIDGE,
> +	.drv = {
> +		.suppress_bind_attrs = true,
> +	},
>  };
>  
>  static int match_cxl_nvdimm(struct device *dev, void *data)
> @@ -583,6 +589,9 @@ static struct cxl_driver cxl_pmem_region_driver = {
>  	.name = "cxl_pmem_region",
>  	.probe = cxl_pmem_region_probe,
>  	.id = CXL_DEVICE_PMEM_REGION,
> +	.drv = {
> +		.suppress_bind_attrs = true,
> +	},
>  };
>  
>  /*
> 

[PATCH v6 02/12] cxl/region: Drop redundant pmem region release handling

[Dan Williams]

Now that a cxl_nvdimm object can only experience ->remove() via an
unregistration event (because the cxl_nvdimm bind attributes are
suppressed), additional cleanups are possible.

It is already the case that the removal of a cxl_memdev object triggers
->remove() on any associated region. With that mechanism in place there
is no need for the cxl_nvdimm removal to trigger the same. Just rely on
cxl_region_detach() to tear down the whole cxl_pmem_region.

Tested-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pmem.c |    2 -
 drivers/cxl/cxl.h       |    1 -
 drivers/cxl/pmem.c      |   90 -----------------------------------------------
 3 files changed, 93 deletions(-)

diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
index 36aa5070d902..1d12a8206444 100644
--- a/drivers/cxl/core/pmem.c
+++ b/drivers/cxl/core/pmem.c
@@ -188,7 +188,6 @@ static void cxl_nvdimm_release(struct device *dev)
 {
 	struct cxl_nvdimm *cxl_nvd = to_cxl_nvdimm(dev);
 
-	xa_destroy(&cxl_nvd->pmem_regions);
 	kfree(cxl_nvd);
 }
 
@@ -231,7 +230,6 @@ static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_memdev *cxlmd)
 
 	dev = &cxl_nvd->dev;
 	cxl_nvd->cxlmd = cxlmd;
-	xa_init(&cxl_nvd->pmem_regions);
 	device_initialize(dev);
 	lockdep_set_class(&dev->mutex, &cxl_nvdimm_key);
 	device_set_pm_not_required(dev);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 7d07127eade3..4ac7938eaf6c 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -424,7 +424,6 @@ struct cxl_nvdimm {
 	struct device dev;
 	struct cxl_memdev *cxlmd;
 	struct cxl_nvdimm_bridge *bridge;
-	struct xarray pmem_regions;
 };
 
 struct cxl_pmem_region_mapping {
diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index 946e171e7d4a..652f00fc68ca 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -27,26 +27,7 @@ static void clear_exclusive(void *cxlds)
 
 static void unregister_nvdimm(void *nvdimm)
 {
-	struct cxl_nvdimm *cxl_nvd = nvdimm_provider_data(nvdimm);
-	struct cxl_nvdimm_bridge *cxl_nvb = cxl_nvd->bridge;
-	struct cxl_pmem_region *cxlr_pmem;
-	unsigned long index;
-
-	device_lock(&cxl_nvb->dev);
-	dev_set_drvdata(&cxl_nvd->dev, NULL);
-	xa_for_each(&cxl_nvd->pmem_regions, index, cxlr_pmem) {
-		get_device(&cxlr_pmem->dev);
-		device_unlock(&cxl_nvb->dev);
-
-		device_release_driver(&cxlr_pmem->dev);
-		put_device(&cxlr_pmem->dev);
-
-		device_lock(&cxl_nvb->dev);
-	}
-	device_unlock(&cxl_nvb->dev);
-
 	nvdimm_delete(nvdimm);
-	cxl_nvd->bridge = NULL;
 }
 
 static int cxl_nvdimm_probe(struct device *dev)
@@ -243,21 +224,6 @@ static int cxl_nvdimm_release_driver(struct device *dev, void *cxl_nvb)
 	return 0;
 }
 
-static int cxl_pmem_region_release_driver(struct device *dev, void *cxl_nvb)
-{
-	struct cxl_pmem_region *cxlr_pmem;
-
-	if (!is_cxl_pmem_region(dev))
-		return 0;
-
-	cxlr_pmem = to_cxl_pmem_region(dev);
-	if (cxlr_pmem->bridge != cxl_nvb)
-		return 0;
-
-	device_release_driver(dev);
-	return 0;
-}
-
 static void offline_nvdimm_bus(struct cxl_nvdimm_bridge *cxl_nvb,
 			       struct nvdimm_bus *nvdimm_bus)
 {
@@ -269,8 +235,6 @@ static void offline_nvdimm_bus(struct cxl_nvdimm_bridge *cxl_nvb,
 	 * nvdimm_bus_unregister() rips the nvdimm objects out from
 	 * underneath them.
 	 */
-	bus_for_each_dev(&cxl_bus_type, NULL, cxl_nvb,
-			 cxl_pmem_region_release_driver);
 	bus_for_each_dev(&cxl_bus_type, NULL, cxl_nvb,
 			 cxl_nvdimm_release_driver);
 	nvdimm_bus_unregister(nvdimm_bus);
@@ -378,48 +342,6 @@ static void unregister_nvdimm_region(void *nd_region)
 	nvdimm_region_delete(nd_region);
 }
 
-static int cxl_nvdimm_add_region(struct cxl_nvdimm *cxl_nvd,
-				 struct cxl_pmem_region *cxlr_pmem)
-{
-	int rc;
-
-	rc = xa_insert(&cxl_nvd->pmem_regions, (unsigned long)cxlr_pmem,
-		       cxlr_pmem, GFP_KERNEL);
-	if (rc)
-		return rc;
-
-	get_device(&cxlr_pmem->dev);
-	return 0;
-}
-
-static void cxl_nvdimm_del_region(struct cxl_nvdimm *cxl_nvd,
-				  struct cxl_pmem_region *cxlr_pmem)
-{
-	/*
-	 * It is possible this is called without a corresponding
-	 * cxl_nvdimm_add_region for @cxlr_pmem
-	 */
-	cxlr_pmem = xa_erase(&cxl_nvd->pmem_regions, (unsigned long)cxlr_pmem);
-	if (cxlr_pmem)
-		put_device(&cxlr_pmem->dev);
-}
-
-static void release_mappings(void *data)
-{
-	int i;
-	struct cxl_pmem_region *cxlr_pmem = data;
-	struct cxl_nvdimm_bridge *cxl_nvb = cxlr_pmem->bridge;
-
-	device_lock(&cxl_nvb->dev);
-	for (i = 0; i < cxlr_pmem->nr_mappings; i++) {
-		struct cxl_pmem_region_mapping *m = &cxlr_pmem->mapping[i];
-		struct cxl_nvdimm *cxl_nvd = m->cxl_nvd;
-
-		cxl_nvdimm_del_region(cxl_nvd, cxlr_pmem);
-	}
-	device_unlock(&cxl_nvb->dev);
-}
-
 static void cxlr_pmem_remove_resource(void *res)
 {
 	remove_resource(res);
@@ -508,10 +430,6 @@ static int cxl_pmem_region_probe(struct device *dev)
 		goto out_nvb;
 	}
 
-	rc = devm_add_action_or_reset(dev, release_mappings, cxlr_pmem);
-	if (rc)
-		goto out_nvd;
-
 	for (i = 0; i < cxlr_pmem->nr_mappings; i++) {
 		struct cxl_pmem_region_mapping *m = &cxlr_pmem->mapping[i];
 		struct cxl_memdev *cxlmd = m->cxlmd;
@@ -538,14 +456,6 @@ static int cxl_pmem_region_probe(struct device *dev)
 			goto out_nvd;
 		}
 
-		/*
-		 * Pin the region per nvdimm device as those may be released
-		 * out-of-order with respect to the region, and a single nvdimm
-		 * maybe associated with multiple regions
-		 */
-		rc = cxl_nvdimm_add_region(cxl_nvd, cxlr_pmem);
-		if (rc)
-			goto out_nvd;
 		m->cxl_nvd = cxl_nvd;
 		mappings[i] = (struct nd_mapping_desc) {
 			.nvdimm = nvdimm,

Re: [PATCH v6 02/12] cxl/region: Drop redundant pmem region release handling

[Jonathan Cameron]

On Thu, 01 Dec 2022 13:33:32 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Now that a cxl_nvdimm object can only experience ->remove() via an
> unregistration event (because the cxl_nvdimm bind attributes are
> suppressed), additional cleanups are possible.
> 
> It is already the case that the removal of a cxl_memdev object triggers
> ->remove() on any associated region. With that mechanism in place there  
> is no need for the cxl_nvdimm removal to trigger the same. Just rely on
> cxl_region_detach() to tear down the whole cxl_pmem_region.
> 
> Tested-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Seems logical. There was a bunch of stuff left in some of this where I didn't
follow why it was still there, but that's an artifact of how the series is built
up which is fair enough. FWIW

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

> ---
>  drivers/cxl/core/pmem.c |    2 -
>  drivers/cxl/cxl.h       |    1 -
>  drivers/cxl/pmem.c      |   90 -----------------------------------------------
>  3 files changed, 93 deletions(-)
> 
> diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
> index 36aa5070d902..1d12a8206444 100644
> --- a/drivers/cxl/core/pmem.c
> +++ b/drivers/cxl/core/pmem.c
> @@ -188,7 +188,6 @@ static void cxl_nvdimm_release(struct device *dev)
>  {
>  	struct cxl_nvdimm *cxl_nvd = to_cxl_nvdimm(dev);
>  
> -	xa_destroy(&cxl_nvd->pmem_regions);
>  	kfree(cxl_nvd);
>  }
>  
> @@ -231,7 +230,6 @@ static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_memdev *cxlmd)
>  
>  	dev = &cxl_nvd->dev;
>  	cxl_nvd->cxlmd = cxlmd;
> -	xa_init(&cxl_nvd->pmem_regions);
>  	device_initialize(dev);
>  	lockdep_set_class(&dev->mutex, &cxl_nvdimm_key);
>  	device_set_pm_not_required(dev);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 7d07127eade3..4ac7938eaf6c 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -424,7 +424,6 @@ struct cxl_nvdimm {
>  	struct device dev;
>  	struct cxl_memdev *cxlmd;
>  	struct cxl_nvdimm_bridge *bridge;
> -	struct xarray pmem_regions;
>  };
>  
>  struct cxl_pmem_region_mapping {
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index 946e171e7d4a..652f00fc68ca 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -27,26 +27,7 @@ static void clear_exclusive(void *cxlds)
>  
>  static void unregister_nvdimm(void *nvdimm)
>  {
> -	struct cxl_nvdimm *cxl_nvd = nvdimm_provider_data(nvdimm);
> -	struct cxl_nvdimm_bridge *cxl_nvb = cxl_nvd->bridge;
> -	struct cxl_pmem_region *cxlr_pmem;
> -	unsigned long index;
> -
> -	device_lock(&cxl_nvb->dev);
> -	dev_set_drvdata(&cxl_nvd->dev, NULL);
> -	xa_for_each(&cxl_nvd->pmem_regions, index, cxlr_pmem) {
> -		get_device(&cxlr_pmem->dev);
> -		device_unlock(&cxl_nvb->dev);
> -
> -		device_release_driver(&cxlr_pmem->dev);
> -		put_device(&cxlr_pmem->dev);
> -
> -		device_lock(&cxl_nvb->dev);
> -	}
> -	device_unlock(&cxl_nvb->dev);
> -
>  	nvdimm_delete(nvdimm);
> -	cxl_nvd->bridge = NULL;
>  }
>  
>  static int cxl_nvdimm_probe(struct device *dev)
> @@ -243,21 +224,6 @@ static int cxl_nvdimm_release_driver(struct device *dev, void *cxl_nvb)
>  	return 0;
>  }
>  
> -static int cxl_pmem_region_release_driver(struct device *dev, void *cxl_nvb)
> -{
> -	struct cxl_pmem_region *cxlr_pmem;
> -
> -	if (!is_cxl_pmem_region(dev))
> -		return 0;
> -
> -	cxlr_pmem = to_cxl_pmem_region(dev);
> -	if (cxlr_pmem->bridge != cxl_nvb)
> -		return 0;
> -
> -	device_release_driver(dev);
> -	return 0;
> -}
> -
>  static void offline_nvdimm_bus(struct cxl_nvdimm_bridge *cxl_nvb,
>  			       struct nvdimm_bus *nvdimm_bus)
>  {
> @@ -269,8 +235,6 @@ static void offline_nvdimm_bus(struct cxl_nvdimm_bridge *cxl_nvb,
>  	 * nvdimm_bus_unregister() rips the nvdimm objects out from
>  	 * underneath them.
>  	 */
> -	bus_for_each_dev(&cxl_bus_type, NULL, cxl_nvb,
> -			 cxl_pmem_region_release_driver);
>  	bus_for_each_dev(&cxl_bus_type, NULL, cxl_nvb,
>  			 cxl_nvdimm_release_driver);
>  	nvdimm_bus_unregister(nvdimm_bus);
> @@ -378,48 +342,6 @@ static void unregister_nvdimm_region(void *nd_region)
>  	nvdimm_region_delete(nd_region);
>  }
>  
> -static int cxl_nvdimm_add_region(struct cxl_nvdimm *cxl_nvd,
> -				 struct cxl_pmem_region *cxlr_pmem)
> -{
> -	int rc;
> -
> -	rc = xa_insert(&cxl_nvd->pmem_regions, (unsigned long)cxlr_pmem,
> -		       cxlr_pmem, GFP_KERNEL);
> -	if (rc)
> -		return rc;
> -
> -	get_device(&cxlr_pmem->dev);
> -	return 0;
> -}
> -
> -static void cxl_nvdimm_del_region(struct cxl_nvdimm *cxl_nvd,
> -				  struct cxl_pmem_region *cxlr_pmem)
> -{
> -	/*
> -	 * It is possible this is called without a corresponding
> -	 * cxl_nvdimm_add_region for @cxlr_pmem
> -	 */
> -	cxlr_pmem = xa_erase(&cxl_nvd->pmem_regions, (unsigned long)cxlr_pmem);
> -	if (cxlr_pmem)
> -		put_device(&cxlr_pmem->dev);
> -}
> -
> -static void release_mappings(void *data)
> -{
> -	int i;
> -	struct cxl_pmem_region *cxlr_pmem = data;
> -	struct cxl_nvdimm_bridge *cxl_nvb = cxlr_pmem->bridge;
> -
> -	device_lock(&cxl_nvb->dev);
> -	for (i = 0; i < cxlr_pmem->nr_mappings; i++) {
> -		struct cxl_pmem_region_mapping *m = &cxlr_pmem->mapping[i];
> -		struct cxl_nvdimm *cxl_nvd = m->cxl_nvd;
> -
> -		cxl_nvdimm_del_region(cxl_nvd, cxlr_pmem);
> -	}
> -	device_unlock(&cxl_nvb->dev);
> -}
> -
>  static void cxlr_pmem_remove_resource(void *res)
>  {
>  	remove_resource(res);
> @@ -508,10 +430,6 @@ static int cxl_pmem_region_probe(struct device *dev)
>  		goto out_nvb;
>  	}
>  
> -	rc = devm_add_action_or_reset(dev, release_mappings, cxlr_pmem);
> -	if (rc)
> -		goto out_nvd;
> -
>  	for (i = 0; i < cxlr_pmem->nr_mappings; i++) {
>  		struct cxl_pmem_region_mapping *m = &cxlr_pmem->mapping[i];
>  		struct cxl_memdev *cxlmd = m->cxlmd;
> @@ -538,14 +456,6 @@ static int cxl_pmem_region_probe(struct device *dev)
>  			goto out_nvd;
>  		}
>  
> -		/*
> -		 * Pin the region per nvdimm device as those may be released
> -		 * out-of-order with respect to the region, and a single nvdimm
> -		 * maybe associated with multiple regions
> -		 */
> -		rc = cxl_nvdimm_add_region(cxl_nvd, cxlr_pmem);
> -		if (rc)
> -			goto out_nvd;
>  		m->cxl_nvd = cxl_nvd;
>  		mappings[i] = (struct nd_mapping_desc) {
>  			.nvdimm = nvdimm,
> 

[PATCH v6 03/12] cxl/pmem: Refactor nvdimm device registration, delete the workqueue

[Dan Williams]

The three objects 'struct cxl_nvdimm_bridge', 'struct cxl_nvdimm', and
'struct cxl_pmem_region' manage CXL persistent memory resources. The
bridge represents base platform resources, the nvdimm represents one or
more endpoints, and the region is a collection of nvdimms that
contribute to an assembled address range.

Their relationship is such that a region is torn down if any component
endpoints are removed. All regions and endpoints are torn down if the
foundational bridge device goes down.

A workqueue was deployed to manage these interdependencies, but it is
difficult to reason about, and fragile. A recent attempt to take the CXL
root device lock in the cxl_mem driver was reported by lockdep as
colliding with the flush_work() in the cxl_pmem flows.

Instead of the workqueue, arrange for all pmem/nvdimm devices to be torn
down immediately and hierarchically. A similar change is made to both
the 'cxl_nvdimm' and 'cxl_pmem_region' objects. For bisect-ability both
changes are made in the same patch which unfortunately makes the patch
bigger than desired.

Arrange for cxl_memdev and cxl_region to register a cxl_nvdimm and
cxl_pmem_region as a devres release action of the bridge device.
Additionally, include a devres release action of the cxl_memdev or
cxl_region device that triggers the bridge's release action if an endpoint
exits before the bridge. I.e. this allows either unplugging the bridge,
or unplugging and endpoint to result in the same cleanup actions.

To keep the patch smaller the cleanup of the now defunct workqueue
infrastructure is saved for a follow-on patch.

Tested-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pmem.c      |   78 +++++++++++++++++++++++----
 drivers/cxl/core/region.c    |   64 +++++++++++++++++++++-
 drivers/cxl/cxl.h            |    7 ++
 drivers/cxl/cxlmem.h         |    4 +
 drivers/cxl/mem.c            |    9 +++
 drivers/cxl/pci.c            |    3 -
 drivers/cxl/pmem.c           |  121 ++++++++++++------------------------------
 tools/testing/cxl/test/mem.c |    3 -
 8 files changed, 181 insertions(+), 108 deletions(-)

diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
index 1d12a8206444..4d36805079ad 100644
--- a/drivers/cxl/core/pmem.c
+++ b/drivers/cxl/core/pmem.c
@@ -219,7 +219,8 @@ EXPORT_SYMBOL_NS_GPL(to_cxl_nvdimm, CXL);
 
 static struct lock_class_key cxl_nvdimm_key;
 
-static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_memdev *cxlmd)
+static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_nvdimm_bridge *cxl_nvb,
+					   struct cxl_memdev *cxlmd)
 {
 	struct cxl_nvdimm *cxl_nvd;
 	struct device *dev;
@@ -230,6 +231,7 @@ static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_memdev *cxlmd)
 
 	dev = &cxl_nvd->dev;
 	cxl_nvd->cxlmd = cxlmd;
+	cxlmd->cxl_nvd = cxl_nvd;
 	device_initialize(dev);
 	lockdep_set_class(&dev->mutex, &cxl_nvdimm_key);
 	device_set_pm_not_required(dev);
@@ -240,27 +242,60 @@ static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_memdev *cxlmd)
 	return cxl_nvd;
 }
 
-static void cxl_nvd_unregister(void *dev)
+static void cxl_nvd_unregister(void *_cxl_nvd)
 {
-	device_unregister(dev);
+	struct cxl_nvdimm *cxl_nvd = _cxl_nvd;
+	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
+	struct cxl_nvdimm_bridge *cxl_nvb = cxlmd->cxl_nvb;
+
+	/*
+	 * Either the bridge is in ->remove() context under the device_lock(),
+	 * or cxlmd_release_nvdimm() is cancelling the bridge's release action
+	 * for @cxl_nvd and doing it itself (while manually holding the bridge
+	 * lock).
+	 */
+	device_lock_assert(&cxl_nvb->dev);
+	cxl_nvd->cxlmd = NULL;
+	cxlmd->cxl_nvd = NULL;
+	device_unregister(&cxl_nvd->dev);
+}
+
+static void cxlmd_release_nvdimm(void *_cxlmd)
+{
+	struct cxl_memdev *cxlmd = _cxlmd;
+	struct cxl_nvdimm_bridge *cxl_nvb = cxlmd->cxl_nvb;
+
+	device_lock(&cxl_nvb->dev);
+	if (cxlmd->cxl_nvd)
+		devm_release_action(&cxl_nvb->dev, cxl_nvd_unregister,
+				    cxlmd->cxl_nvd);
+	device_unlock(&cxl_nvb->dev);
+	put_device(&cxl_nvb->dev);
 }
 
 /**
  * devm_cxl_add_nvdimm() - add a bridge between a cxl_memdev and an nvdimm
- * @host: same host as @cxlmd
  * @cxlmd: cxl_memdev instance that will perform LIBNVDIMM operations
  *
  * Return: 0 on success negative error code on failure.
  */
-int devm_cxl_add_nvdimm(struct device *host, struct cxl_memdev *cxlmd)
+int devm_cxl_add_nvdimm(struct cxl_memdev *cxlmd)
 {
+	struct cxl_nvdimm_bridge *cxl_nvb;
 	struct cxl_nvdimm *cxl_nvd;
 	struct device *dev;
 	int rc;
 
-	cxl_nvd = cxl_nvdimm_alloc(cxlmd);
-	if (IS_ERR(cxl_nvd))
-		return PTR_ERR(cxl_nvd);
+	cxl_nvb = cxl_find_nvdimm_bridge(&cxlmd->dev);
+	if (!cxl_nvb)
+		return -ENODEV;
+
+	cxl_nvd = cxl_nvdimm_alloc(cxl_nvb, cxlmd);
+	if (IS_ERR(cxl_nvd)) {
+		rc = PTR_ERR(cxl_nvd);
+		goto err_alloc;
+	}
+	cxlmd->cxl_nvb = cxl_nvb;
 
 	dev = &cxl_nvd->dev;
 	rc = dev_set_name(dev, "pmem%d", cxlmd->id);
@@ -271,13 +306,34 @@ int devm_cxl_add_nvdimm(struct device *host, struct cxl_memdev *cxlmd)
 	if (rc)
 		goto err;
 
-	dev_dbg(host, "%s: register %s\n", dev_name(dev->parent),
-		dev_name(dev));
+	dev_dbg(&cxlmd->dev, "register %s\n", dev_name(dev));
+
+	/*
+	 * The two actions below arrange for @cxl_nvd to be deleted when either
+	 * the top-level PMEM bridge goes down, or the endpoint device goes
+	 * through ->remove().
+	 */
+	device_lock(&cxl_nvb->dev);
+	if (cxl_nvb->dev.driver)
+		rc = devm_add_action_or_reset(&cxl_nvb->dev, cxl_nvd_unregister,
+					      cxl_nvd);
+	else
+		rc = -ENXIO;
+	device_unlock(&cxl_nvb->dev);
+
+	if (rc)
+		goto err_alloc;
 
-	return devm_add_action_or_reset(host, cxl_nvd_unregister, dev);
+	/* @cxlmd carries a reference on @cxl_nvb until cxlmd_release_nvdimm */
+	return devm_add_action_or_reset(&cxlmd->dev, cxlmd_release_nvdimm, cxlmd);
 
 err:
 	put_device(dev);
+err_alloc:
+	cxlmd->cxl_nvb = NULL;
+	cxlmd->cxl_nvd = NULL;
+	put_device(&cxl_nvb->dev);
+
 	return rc;
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_add_nvdimm, CXL);
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index f9ae5ad284ff..1e61d1bafc0c 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1812,6 +1812,7 @@ static struct lock_class_key cxl_pmem_region_key;
 static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
 {
 	struct cxl_region_params *p = &cxlr->params;
+	struct cxl_nvdimm_bridge *cxl_nvb;
 	struct cxl_pmem_region *cxlr_pmem;
 	struct device *dev;
 	int i;
@@ -1839,6 +1840,18 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
 		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 		struct cxl_pmem_region_mapping *m = &cxlr_pmem->mapping[i];
 
+		/*
+		 * Regions never span CXL root devices, so by definition the
+		 * bridge for one device is the same for all.
+		 */
+		if (i == 0) {
+			cxl_nvb = cxl_find_nvdimm_bridge(&cxlmd->dev);
+			if (!cxl_nvb) {
+				cxlr_pmem = ERR_PTR(-ENODEV);
+				goto out;
+			}
+			cxlr->cxl_nvb = cxl_nvb;
+		}
 		m->cxlmd = cxlmd;
 		get_device(&cxlmd->dev);
 		m->start = cxled->dpa_res->start;
@@ -1848,6 +1861,7 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
 
 	dev = &cxlr_pmem->dev;
 	cxlr_pmem->cxlr = cxlr;
+	cxlr->cxlr_pmem = cxlr_pmem;
 	device_initialize(dev);
 	lockdep_set_class(&dev->mutex, &cxl_pmem_region_key);
 	device_set_pm_not_required(dev);
@@ -1860,9 +1874,36 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
 	return cxlr_pmem;
 }
 
-static void cxlr_pmem_unregister(void *dev)
+static void cxlr_pmem_unregister(void *_cxlr_pmem)
 {
-	device_unregister(dev);
+	struct cxl_pmem_region *cxlr_pmem = _cxlr_pmem;
+	struct cxl_region *cxlr = cxlr_pmem->cxlr;
+	struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb;
+
+	/*
+	 * Either the bridge is in ->remove() context under the device_lock(),
+	 * or cxlr_release_nvdimm() is cancelling the bridge's release action
+	 * for @cxlr_pmem and doing it itself (while manually holding the bridge
+	 * lock).
+	 */
+	device_lock_assert(&cxl_nvb->dev);
+	cxlr->cxlr_pmem = NULL;
+	cxlr_pmem->cxlr = NULL;
+	device_unregister(&cxlr_pmem->dev);
+}
+
+static void cxlr_release_nvdimm(void *_cxlr)
+{
+	struct cxl_region *cxlr = _cxlr;
+	struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb;
+
+	device_lock(&cxl_nvb->dev);
+	if (cxlr->cxlr_pmem)
+		devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister,
+				    cxlr->cxlr_pmem);
+	device_unlock(&cxl_nvb->dev);
+	cxlr->cxl_nvb = NULL;
+	put_device(&cxl_nvb->dev);
 }
 
 /**
@@ -1874,12 +1915,14 @@ static void cxlr_pmem_unregister(void *dev)
 static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
 {
 	struct cxl_pmem_region *cxlr_pmem;
+	struct cxl_nvdimm_bridge *cxl_nvb;
 	struct device *dev;
 	int rc;
 
 	cxlr_pmem = cxl_pmem_region_alloc(cxlr);
 	if (IS_ERR(cxlr_pmem))
 		return PTR_ERR(cxlr_pmem);
+	cxl_nvb = cxlr->cxl_nvb;
 
 	dev = &cxlr_pmem->dev;
 	rc = dev_set_name(dev, "pmem_region%d", cxlr->id);
@@ -1893,10 +1936,25 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
 	dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
 		dev_name(dev));
 
-	return devm_add_action_or_reset(&cxlr->dev, cxlr_pmem_unregister, dev);
+	device_lock(&cxl_nvb->dev);
+	if (cxl_nvb->dev.driver)
+		rc = devm_add_action_or_reset(&cxl_nvb->dev,
+					      cxlr_pmem_unregister, cxlr_pmem);
+	else
+		rc = -ENXIO;
+	device_unlock(&cxl_nvb->dev);
+
+	if (rc)
+		goto err_bridge;
+
+	/* @cxlr carries a reference on @cxl_nvb until cxlr_release_nvdimm */
+	return devm_add_action_or_reset(&cxlr->dev, cxlr_release_nvdimm, cxlr);
 
 err:
 	put_device(dev);
+err_bridge:
+	put_device(&cxl_nvb->dev);
+	cxlr->cxl_nvb = NULL;
 	return rc;
 }
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 4ac7938eaf6c..fc6083b0e467 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -386,6 +386,8 @@ struct cxl_region_params {
  * @id: This region's id. Id is globally unique across all regions
  * @mode: Endpoint decoder allocation / access mode
  * @type: Endpoint decoder target type
+ * @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown
+ * @cxlr_pmem: (for pmem regions) cached copy of the nvdimm bridge
  * @params: active + config params for the region
  */
 struct cxl_region {
@@ -393,6 +395,8 @@ struct cxl_region {
 	int id;
 	enum cxl_decoder_mode mode;
 	enum cxl_decoder_type type;
+	struct cxl_nvdimm_bridge *cxl_nvb;
+	struct cxl_pmem_region *cxlr_pmem;
 	struct cxl_region_params params;
 };
 
@@ -438,7 +442,6 @@ struct cxl_pmem_region {
 	struct device dev;
 	struct cxl_region *cxlr;
 	struct nd_region *nd_region;
-	struct cxl_nvdimm_bridge *bridge;
 	struct range hpa_range;
 	int nr_mappings;
 	struct cxl_pmem_region_mapping mapping[];
@@ -637,7 +640,7 @@ struct cxl_nvdimm_bridge *devm_cxl_add_nvdimm_bridge(struct device *host,
 struct cxl_nvdimm *to_cxl_nvdimm(struct device *dev);
 bool is_cxl_nvdimm(struct device *dev);
 bool is_cxl_nvdimm_bridge(struct device *dev);
-int devm_cxl_add_nvdimm(struct device *host, struct cxl_memdev *cxlmd);
+int devm_cxl_add_nvdimm(struct cxl_memdev *cxlmd);
 struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct device *dev);
 
 #ifdef CONFIG_CXL_REGION
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 88e3a8e54b6a..c1c9960ab05f 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -35,6 +35,8 @@
  * @cdev: char dev core object for ioctl operations
  * @cxlds: The device state backing this device
  * @detach_work: active memdev lost a port in its ancestry
+ * @cxl_nvb: coordinate removal of @cxl_nvd if present
+ * @cxl_nvd: optional bridge to an nvdimm if the device supports pmem
  * @id: id number of this memdev instance.
  */
 struct cxl_memdev {
@@ -42,6 +44,8 @@ struct cxl_memdev {
 	struct cdev cdev;
 	struct cxl_dev_state *cxlds;
 	struct work_struct detach_work;
+	struct cxl_nvdimm_bridge *cxl_nvb;
+	struct cxl_nvdimm *cxl_nvd;
 	int id;
 };
 
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 64ccf053d32c..549b6b499bae 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -48,6 +48,7 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
 static int cxl_mem_probe(struct device *dev)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_port *parent_port;
 	struct cxl_dport *dport;
 	struct dentry *dentry;
@@ -95,6 +96,14 @@ static int cxl_mem_probe(struct device *dev)
 	if (rc)
 		return rc;
 
+	if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM)) {
+		rc = devm_cxl_add_nvdimm(cxlmd);
+		if (rc == -ENODEV)
+			dev_info(dev, "PMEM disabled by platform\n");
+		else
+			return rc;
+	}
+
 	/*
 	 * The kernel may be operating out of CXL memory on this device,
 	 * there is no spec defined way to determine whether this device
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 621a0522b554..e15da405b948 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -503,9 +503,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);
 
-	if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM))
-		rc = devm_cxl_add_nvdimm(&pdev->dev, cxlmd);
-
 	return rc;
 }
 
diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index 652f00fc68ca..76cf54eeb310 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -34,26 +34,16 @@ static int cxl_nvdimm_probe(struct device *dev)
 {
 	struct cxl_nvdimm *cxl_nvd = to_cxl_nvdimm(dev);
 	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
+	struct cxl_nvdimm_bridge *cxl_nvb = cxlmd->cxl_nvb;
 	unsigned long flags = 0, cmd_mask = 0;
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	struct cxl_nvdimm_bridge *cxl_nvb;
 	struct nvdimm *nvdimm;
 	int rc;
 
-	cxl_nvb = cxl_find_nvdimm_bridge(dev);
-	if (!cxl_nvb)
-		return -ENXIO;
-
-	device_lock(&cxl_nvb->dev);
-	if (!cxl_nvb->nvdimm_bus) {
-		rc = -ENXIO;
-		goto out;
-	}
-
 	set_exclusive_cxl_commands(cxlds, exclusive_cmds);
 	rc = devm_add_action_or_reset(dev, clear_exclusive, cxlds);
 	if (rc)
-		goto out;
+		return rc;
 
 	set_bit(NDD_LABELING, &flags);
 	set_bit(ND_CMD_GET_CONFIG_SIZE, &cmd_mask);
@@ -61,19 +51,11 @@ static int cxl_nvdimm_probe(struct device *dev)
 	set_bit(ND_CMD_SET_CONFIG_DATA, &cmd_mask);
 	nvdimm = nvdimm_create(cxl_nvb->nvdimm_bus, cxl_nvd, NULL, flags,
 			       cmd_mask, 0, NULL);
-	if (!nvdimm) {
-		rc = -ENOMEM;
-		goto out;
-	}
+	if (!nvdimm)
+		return -ENOMEM;
 
 	dev_set_drvdata(dev, nvdimm);
-	cxl_nvd->bridge = cxl_nvb;
-	rc = devm_add_action_or_reset(dev, unregister_nvdimm, nvdimm);
-out:
-	device_unlock(&cxl_nvb->dev);
-	put_device(&cxl_nvb->dev);
-
-	return rc;
+	return devm_add_action_or_reset(dev, unregister_nvdimm, nvdimm);
 }
 
 static struct cxl_driver cxl_nvdimm_driver = {
@@ -200,6 +182,15 @@ static int cxl_pmem_ctl(struct nvdimm_bus_descriptor *nd_desc,
 	return cxl_pmem_nvdimm_ctl(nvdimm, cmd, buf, buf_len);
 }
 
+static void unregister_nvdimm_bus(void *_cxl_nvb)
+{
+	struct cxl_nvdimm_bridge *cxl_nvb = _cxl_nvb;
+	struct nvdimm_bus *nvdimm_bus = cxl_nvb->nvdimm_bus;
+
+	cxl_nvb->nvdimm_bus = NULL;
+	nvdimm_bus_unregister(nvdimm_bus);
+}
+
 static bool online_nvdimm_bus(struct cxl_nvdimm_bridge *cxl_nvb)
 {
 	if (cxl_nvb->nvdimm_bus)
@@ -303,23 +294,21 @@ static int cxl_nvdimm_bridge_probe(struct device *dev)
 {
 	struct cxl_nvdimm_bridge *cxl_nvb = to_cxl_nvdimm_bridge(dev);
 
-	if (cxl_nvb->state == CXL_NVB_DEAD)
-		return -ENXIO;
+	cxl_nvb->nd_desc = (struct nvdimm_bus_descriptor) {
+		.provider_name = "CXL",
+		.module = THIS_MODULE,
+		.ndctl = cxl_pmem_ctl,
+	};
 
-	if (cxl_nvb->state == CXL_NVB_NEW) {
-		cxl_nvb->nd_desc = (struct nvdimm_bus_descriptor) {
-			.provider_name = "CXL",
-			.module = THIS_MODULE,
-			.ndctl = cxl_pmem_ctl,
-		};
+	cxl_nvb->nvdimm_bus =
+		nvdimm_bus_register(&cxl_nvb->dev, &cxl_nvb->nd_desc);
 
-		INIT_WORK(&cxl_nvb->state_work, cxl_nvb_update_state);
-	}
+	if (!cxl_nvb->nvdimm_bus)
+		return -ENOMEM;
 
-	cxl_nvb->state = CXL_NVB_ONLINE;
-	cxl_nvdimm_bridge_state_work(cxl_nvb);
+	INIT_WORK(&cxl_nvb->state_work, cxl_nvb_update_state);
 
-	return 0;
+	return devm_add_action_or_reset(dev, unregister_nvdimm_bus, cxl_nvb);
 }
 
 static struct cxl_driver cxl_nvdimm_bridge_driver = {
@@ -332,11 +321,6 @@ static struct cxl_driver cxl_nvdimm_bridge_driver = {
 	},
 };
 
-static int match_cxl_nvdimm(struct device *dev, void *data)
-{
-	return is_cxl_nvdimm(dev);
-}
-
 static void unregister_nvdimm_region(void *nd_region)
 {
 	nvdimm_region_delete(nd_region);
@@ -357,8 +341,8 @@ static int cxl_pmem_region_probe(struct device *dev)
 	struct nd_mapping_desc mappings[CXL_DECODER_MAX_INTERLEAVE];
 	struct cxl_pmem_region *cxlr_pmem = to_cxl_pmem_region(dev);
 	struct cxl_region *cxlr = cxlr_pmem->cxlr;
+	struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb;
 	struct cxl_pmem_region_info *info = NULL;
-	struct cxl_nvdimm_bridge *cxl_nvb;
 	struct nd_interleave_set *nd_set;
 	struct nd_region_desc ndr_desc;
 	struct cxl_nvdimm *cxl_nvd;
@@ -366,28 +350,12 @@ static int cxl_pmem_region_probe(struct device *dev)
 	struct resource *res;
 	int rc, i = 0;
 
-	cxl_nvb = cxl_find_nvdimm_bridge(&cxlr_pmem->mapping[0].cxlmd->dev);
-	if (!cxl_nvb) {
-		dev_dbg(dev, "bridge not found\n");
-		return -ENXIO;
-	}
-	cxlr_pmem->bridge = cxl_nvb;
-
-	device_lock(&cxl_nvb->dev);
-	if (!cxl_nvb->nvdimm_bus) {
-		dev_dbg(dev, "nvdimm bus not found\n");
-		rc = -ENXIO;
-		goto out_nvb;
-	}
-
 	memset(&mappings, 0, sizeof(mappings));
 	memset(&ndr_desc, 0, sizeof(ndr_desc));
 
 	res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
-	if (!res) {
-		rc = -ENOMEM;
-		goto out_nvb;
-	}
+	if (!res)
+		return -ENOMEM;
 
 	res->name = "Persistent Memory";
 	res->start = cxlr_pmem->hpa_range.start;
@@ -397,11 +365,11 @@ static int cxl_pmem_region_probe(struct device *dev)
 
 	rc = insert_resource(&iomem_resource, res);
 	if (rc)
-		goto out_nvb;
+		return rc;
 
 	rc = devm_add_action_or_reset(dev, cxlr_pmem_remove_resource, res);
 	if (rc)
-		goto out_nvb;
+		return rc;
 
 	ndr_desc.res = res;
 	ndr_desc.provider_data = cxlr_pmem;
@@ -415,39 +383,23 @@ static int cxl_pmem_region_probe(struct device *dev)
 	}
 
 	nd_set = devm_kzalloc(dev, sizeof(*nd_set), GFP_KERNEL);
-	if (!nd_set) {
-		rc = -ENOMEM;
-		goto out_nvb;
-	}
+	if (!nd_set)
+		return -ENOMEM;
 
 	ndr_desc.memregion = cxlr->id;
 	set_bit(ND_REGION_CXL, &ndr_desc.flags);
 	set_bit(ND_REGION_PERSIST_MEMCTRL, &ndr_desc.flags);
 
 	info = kmalloc_array(cxlr_pmem->nr_mappings, sizeof(*info), GFP_KERNEL);
-	if (!info) {
-		rc = -ENOMEM;
-		goto out_nvb;
-	}
+	if (!info)
+		return -ENOMEM;
 
 	for (i = 0; i < cxlr_pmem->nr_mappings; i++) {
 		struct cxl_pmem_region_mapping *m = &cxlr_pmem->mapping[i];
 		struct cxl_memdev *cxlmd = m->cxlmd;
 		struct cxl_dev_state *cxlds = cxlmd->cxlds;
-		struct device *d;
 
-		d = device_find_child(&cxlmd->dev, NULL, match_cxl_nvdimm);
-		if (!d) {
-			dev_dbg(dev, "[%d]: %s: no cxl_nvdimm found\n", i,
-				dev_name(&cxlmd->dev));
-			rc = -ENODEV;
-			goto out_nvd;
-		}
-
-		/* safe to drop ref now with bridge lock held */
-		put_device(d);
-
-		cxl_nvd = to_cxl_nvdimm(d);
+		cxl_nvd = cxlmd->cxl_nvd;
 		nvdimm = dev_get_drvdata(&cxl_nvd->dev);
 		if (!nvdimm) {
 			dev_dbg(dev, "[%d]: %s: no nvdimm found\n", i,
@@ -488,9 +440,6 @@ static int cxl_pmem_region_probe(struct device *dev)
 				      cxlr_pmem->nd_region);
 out_nvd:
 	kfree(info);
-out_nvb:
-	device_unlock(&cxl_nvb->dev);
-	put_device(&cxl_nvb->dev);
 
 	return rc;
 }
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index aa2df3a15051..a4ee8e61dd60 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -285,9 +285,6 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);
 
-	if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM))
-		rc = devm_cxl_add_nvdimm(dev, cxlmd);
-
 	return 0;
 }
 

Re: [PATCH v6 03/12] cxl/pmem: Refactor nvdimm device registration, delete the workqueue

[Jonathan Cameron]

On Thu, 01 Dec 2022 13:33:37 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> The three objects 'struct cxl_nvdimm_bridge', 'struct cxl_nvdimm', and
> 'struct cxl_pmem_region' manage CXL persistent memory resources. The
> bridge represents base platform resources, the nvdimm represents one or
> more endpoints, and the region is a collection of nvdimms that
> contribute to an assembled address range.
> 
> Their relationship is such that a region is torn down if any component
> endpoints are removed. All regions and endpoints are torn down if the
> foundational bridge device goes down.
> 
> A workqueue was deployed to manage these interdependencies, but it is
> difficult to reason about, and fragile. A recent attempt to take the CXL
> root device lock in the cxl_mem driver was reported by lockdep as
> colliding with the flush_work() in the cxl_pmem flows.
> 
> Instead of the workqueue, arrange for all pmem/nvdimm devices to be torn
> down immediately and hierarchically. A similar change is made to both
> the 'cxl_nvdimm' and 'cxl_pmem_region' objects. For bisect-ability both
> changes are made in the same patch which unfortunately makes the patch
> bigger than desired.
> 
> Arrange for cxl_memdev and cxl_region to register a cxl_nvdimm and
> cxl_pmem_region as a devres release action of the bridge device.
> Additionally, include a devres release action of the cxl_memdev or
> cxl_region device that triggers the bridge's release action if an endpoint
> exits before the bridge. I.e. this allows either unplugging the bridge,
> or unplugging and endpoint to result in the same cleanup actions.
> 
> To keep the patch smaller the cleanup of the now defunct workqueue
> infrastructure is saved for a follow-on patch.
> 
> Tested-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

I wouldn't say it's the most confident review tag I've ever given, but I've
taken another look at it and couldn't identify any remaining issues...

So with that in mind
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH v6 04/12] cxl/pmem: Remove the cxl_pmem_wq and related infrastructure

[Dan Williams]

Now that cxl_nvdimm and cxl_pmem_region objects are torn down
sychronously with the removal of either the bridge, or an endpoint, the
cxl_pmem_wq infrastructure can be jettisoned.

Tested-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pmem.c |   22 -------
 drivers/cxl/cxl.h       |   17 ------
 drivers/cxl/pmem.c      |  143 -----------------------------------------------
 3 files changed, 1 insertion(+), 181 deletions(-)

diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
index 4d36805079ad..16446473d814 100644
--- a/drivers/cxl/core/pmem.c
+++ b/drivers/cxl/core/pmem.c
@@ -99,7 +99,6 @@ static struct cxl_nvdimm_bridge *cxl_nvdimm_bridge_alloc(struct cxl_port *port)
 
 	dev = &cxl_nvb->dev;
 	cxl_nvb->port = port;
-	cxl_nvb->state = CXL_NVB_NEW;
 	device_initialize(dev);
 	lockdep_set_class(&dev->mutex, &cxl_nvdimm_bridge_key);
 	device_set_pm_not_required(dev);
@@ -117,28 +116,7 @@ static struct cxl_nvdimm_bridge *cxl_nvdimm_bridge_alloc(struct cxl_port *port)
 static void unregister_nvb(void *_cxl_nvb)
 {
 	struct cxl_nvdimm_bridge *cxl_nvb = _cxl_nvb;
-	bool flush;
 
-	/*
-	 * If the bridge was ever activated then there might be in-flight state
-	 * work to flush. Once the state has been changed to 'dead' then no new
-	 * work can be queued by user-triggered bind.
-	 */
-	device_lock(&cxl_nvb->dev);
-	flush = cxl_nvb->state != CXL_NVB_NEW;
-	cxl_nvb->state = CXL_NVB_DEAD;
-	device_unlock(&cxl_nvb->dev);
-
-	/*
-	 * Even though the device core will trigger device_release_driver()
-	 * before the unregister, it does not know about the fact that
-	 * cxl_nvdimm_bridge_driver defers ->remove() work. So, do the driver
-	 * release not and flush it before tearing down the nvdimm device
-	 * hierarchy.
-	 */
-	device_release_driver(&cxl_nvb->dev);
-	if (flush)
-		flush_work(&cxl_nvb->state_work);
 	device_unregister(&cxl_nvb->dev);
 }
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index fc6083b0e467..f0ca2d768385 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -400,34 +400,17 @@ struct cxl_region {
 	struct cxl_region_params params;
 };
 
-/**
- * enum cxl_nvdimm_brige_state - state machine for managing bus rescans
- * @CXL_NVB_NEW: Set at bridge create and after cxl_pmem_wq is destroyed
- * @CXL_NVB_DEAD: Set at brige unregistration to preclude async probing
- * @CXL_NVB_ONLINE: Target state after successful ->probe()
- * @CXL_NVB_OFFLINE: Target state after ->remove() or failed ->probe()
- */
-enum cxl_nvdimm_brige_state {
-	CXL_NVB_NEW,
-	CXL_NVB_DEAD,
-	CXL_NVB_ONLINE,
-	CXL_NVB_OFFLINE,
-};
-
 struct cxl_nvdimm_bridge {
 	int id;
 	struct device dev;
 	struct cxl_port *port;
 	struct nvdimm_bus *nvdimm_bus;
 	struct nvdimm_bus_descriptor nd_desc;
-	struct work_struct state_work;
-	enum cxl_nvdimm_brige_state state;
 };
 
 struct cxl_nvdimm {
 	struct device dev;
 	struct cxl_memdev *cxlmd;
-	struct cxl_nvdimm_bridge *bridge;
 };
 
 struct cxl_pmem_region_mapping {
diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index 76cf54eeb310..0910367a3ead 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -11,13 +11,6 @@
 #include "cxlmem.h"
 #include "cxl.h"
 
-/*
- * Ordered workqueue for cxl nvdimm device arrival and departure
- * to coordinate bus rescans when a bridge arrives and trigger remove
- * operations when the bridge is removed.
- */
-static struct workqueue_struct *cxl_pmem_wq;
-
 static __read_mostly DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
 
 static void clear_exclusive(void *cxlds)
@@ -191,105 +184,6 @@ static void unregister_nvdimm_bus(void *_cxl_nvb)
 	nvdimm_bus_unregister(nvdimm_bus);
 }
 
-static bool online_nvdimm_bus(struct cxl_nvdimm_bridge *cxl_nvb)
-{
-	if (cxl_nvb->nvdimm_bus)
-		return true;
-	cxl_nvb->nvdimm_bus =
-		nvdimm_bus_register(&cxl_nvb->dev, &cxl_nvb->nd_desc);
-	return cxl_nvb->nvdimm_bus != NULL;
-}
-
-static int cxl_nvdimm_release_driver(struct device *dev, void *cxl_nvb)
-{
-	struct cxl_nvdimm *cxl_nvd;
-
-	if (!is_cxl_nvdimm(dev))
-		return 0;
-
-	cxl_nvd = to_cxl_nvdimm(dev);
-	if (cxl_nvd->bridge != cxl_nvb)
-		return 0;
-
-	device_release_driver(dev);
-	return 0;
-}
-
-static void offline_nvdimm_bus(struct cxl_nvdimm_bridge *cxl_nvb,
-			       struct nvdimm_bus *nvdimm_bus)
-{
-	if (!nvdimm_bus)
-		return;
-
-	/*
-	 * Set the state of cxl_nvdimm devices to unbound / idle before
-	 * nvdimm_bus_unregister() rips the nvdimm objects out from
-	 * underneath them.
-	 */
-	bus_for_each_dev(&cxl_bus_type, NULL, cxl_nvb,
-			 cxl_nvdimm_release_driver);
-	nvdimm_bus_unregister(nvdimm_bus);
-}
-
-static void cxl_nvb_update_state(struct work_struct *work)
-{
-	struct cxl_nvdimm_bridge *cxl_nvb =
-		container_of(work, typeof(*cxl_nvb), state_work);
-	struct nvdimm_bus *victim_bus = NULL;
-	bool release = false, rescan = false;
-
-	device_lock(&cxl_nvb->dev);
-	switch (cxl_nvb->state) {
-	case CXL_NVB_ONLINE:
-		if (!online_nvdimm_bus(cxl_nvb)) {
-			dev_err(&cxl_nvb->dev,
-				"failed to establish nvdimm bus\n");
-			release = true;
-		} else
-			rescan = true;
-		break;
-	case CXL_NVB_OFFLINE:
-	case CXL_NVB_DEAD:
-		victim_bus = cxl_nvb->nvdimm_bus;
-		cxl_nvb->nvdimm_bus = NULL;
-		break;
-	default:
-		break;
-	}
-	device_unlock(&cxl_nvb->dev);
-
-	if (release)
-		device_release_driver(&cxl_nvb->dev);
-	if (rescan) {
-		int rc = bus_rescan_devices(&cxl_bus_type);
-
-		dev_dbg(&cxl_nvb->dev, "rescan: %d\n", rc);
-	}
-	offline_nvdimm_bus(cxl_nvb, victim_bus);
-
-	put_device(&cxl_nvb->dev);
-}
-
-static void cxl_nvdimm_bridge_state_work(struct cxl_nvdimm_bridge *cxl_nvb)
-{
-	/*
-	 * Take a reference that the workqueue will drop if new work
-	 * gets queued.
-	 */
-	get_device(&cxl_nvb->dev);
-	if (!queue_work(cxl_pmem_wq, &cxl_nvb->state_work))
-		put_device(&cxl_nvb->dev);
-}
-
-static void cxl_nvdimm_bridge_remove(struct device *dev)
-{
-	struct cxl_nvdimm_bridge *cxl_nvb = to_cxl_nvdimm_bridge(dev);
-
-	if (cxl_nvb->state == CXL_NVB_ONLINE)
-		cxl_nvb->state = CXL_NVB_OFFLINE;
-	cxl_nvdimm_bridge_state_work(cxl_nvb);
-}
-
 static int cxl_nvdimm_bridge_probe(struct device *dev)
 {
 	struct cxl_nvdimm_bridge *cxl_nvb = to_cxl_nvdimm_bridge(dev);
@@ -306,15 +200,12 @@ static int cxl_nvdimm_bridge_probe(struct device *dev)
 	if (!cxl_nvb->nvdimm_bus)
 		return -ENOMEM;
 
-	INIT_WORK(&cxl_nvb->state_work, cxl_nvb_update_state);
-
 	return devm_add_action_or_reset(dev, unregister_nvdimm_bus, cxl_nvb);
 }
 
 static struct cxl_driver cxl_nvdimm_bridge_driver = {
 	.name = "cxl_nvdimm_bridge",
 	.probe = cxl_nvdimm_bridge_probe,
-	.remove = cxl_nvdimm_bridge_remove,
 	.id = CXL_DEVICE_NVDIMM_BRIDGE,
 	.drv = {
 		.suppress_bind_attrs = true,
@@ -453,31 +344,6 @@ static struct cxl_driver cxl_pmem_region_driver = {
 	},
 };
 
-/*
- * Return all bridges to the CXL_NVB_NEW state to invalidate any
- * ->state_work referring to the now destroyed cxl_pmem_wq.
- */
-static int cxl_nvdimm_bridge_reset(struct device *dev, void *data)
-{
-	struct cxl_nvdimm_bridge *cxl_nvb;
-
-	if (!is_cxl_nvdimm_bridge(dev))
-		return 0;
-
-	cxl_nvb = to_cxl_nvdimm_bridge(dev);
-	device_lock(dev);
-	cxl_nvb->state = CXL_NVB_NEW;
-	device_unlock(dev);
-
-	return 0;
-}
-
-static void destroy_cxl_pmem_wq(void)
-{
-	destroy_workqueue(cxl_pmem_wq);
-	bus_for_each_dev(&cxl_bus_type, NULL, NULL, cxl_nvdimm_bridge_reset);
-}
-
 static __init int cxl_pmem_init(void)
 {
 	int rc;
@@ -485,13 +351,9 @@ static __init int cxl_pmem_init(void)
 	set_bit(CXL_MEM_COMMAND_ID_SET_SHUTDOWN_STATE, exclusive_cmds);
 	set_bit(CXL_MEM_COMMAND_ID_SET_LSA, exclusive_cmds);
 
-	cxl_pmem_wq = alloc_ordered_workqueue("cxl_pmem", 0);
-	if (!cxl_pmem_wq)
-		return -ENXIO;
-
 	rc = cxl_driver_register(&cxl_nvdimm_bridge_driver);
 	if (rc)
-		goto err_bridge;
+		return rc;
 
 	rc = cxl_driver_register(&cxl_nvdimm_driver);
 	if (rc)
@@ -507,8 +369,6 @@ static __init int cxl_pmem_init(void)
 	cxl_driver_unregister(&cxl_nvdimm_driver);
 err_nvdimm:
 	cxl_driver_unregister(&cxl_nvdimm_bridge_driver);
-err_bridge:
-	destroy_cxl_pmem_wq();
 	return rc;
 }
 
@@ -517,7 +377,6 @@ static __exit void cxl_pmem_exit(void)
 	cxl_driver_unregister(&cxl_pmem_region_driver);
 	cxl_driver_unregister(&cxl_nvdimm_driver);
 	cxl_driver_unregister(&cxl_nvdimm_bridge_driver);
-	destroy_cxl_pmem_wq();
 }
 
 MODULE_LICENSE("GPL v2");

Re: [PATCH v6 04/12] cxl/pmem: Remove the cxl_pmem_wq and related infrastructure

[Jonathan Cameron]

On Thu, 01 Dec 2022 13:33:43 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Now that cxl_nvdimm and cxl_pmem_region objects are torn down
> sychronously with the removal of either the bridge, or an endpoint, the
> cxl_pmem_wq infrastructure can be jettisoned.
> 
> Tested-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Makes sense given prior patches.

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

> ---
>  drivers/cxl/core/pmem.c |   22 -------
>  drivers/cxl/cxl.h       |   17 ------
>  drivers/cxl/pmem.c      |  143 -----------------------------------------------
>  3 files changed, 1 insertion(+), 181 deletions(-)
> 
> diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
> index 4d36805079ad..16446473d814 100644
> --- a/drivers/cxl/core/pmem.c
> +++ b/drivers/cxl/core/pmem.c
> @@ -99,7 +99,6 @@ static struct cxl_nvdimm_bridge *cxl_nvdimm_bridge_alloc(struct cxl_port *port)
>  
>  	dev = &cxl_nvb->dev;
>  	cxl_nvb->port = port;
> -	cxl_nvb->state = CXL_NVB_NEW;
>  	device_initialize(dev);
>  	lockdep_set_class(&dev->mutex, &cxl_nvdimm_bridge_key);
>  	device_set_pm_not_required(dev);
> @@ -117,28 +116,7 @@ static struct cxl_nvdimm_bridge *cxl_nvdimm_bridge_alloc(struct cxl_port *port)
>  static void unregister_nvb(void *_cxl_nvb)
>  {
>  	struct cxl_nvdimm_bridge *cxl_nvb = _cxl_nvb;
> -	bool flush;
>  
> -	/*
> -	 * If the bridge was ever activated then there might be in-flight state
> -	 * work to flush. Once the state has been changed to 'dead' then no new
> -	 * work can be queued by user-triggered bind.
> -	 */
> -	device_lock(&cxl_nvb->dev);
> -	flush = cxl_nvb->state != CXL_NVB_NEW;
> -	cxl_nvb->state = CXL_NVB_DEAD;
> -	device_unlock(&cxl_nvb->dev);
> -
> -	/*
> -	 * Even though the device core will trigger device_release_driver()
> -	 * before the unregister, it does not know about the fact that
> -	 * cxl_nvdimm_bridge_driver defers ->remove() work. So, do the driver
> -	 * release not and flush it before tearing down the nvdimm device
> -	 * hierarchy.
> -	 */
> -	device_release_driver(&cxl_nvb->dev);
> -	if (flush)
> -		flush_work(&cxl_nvb->state_work);
>  	device_unregister(&cxl_nvb->dev);
>  }
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index fc6083b0e467..f0ca2d768385 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -400,34 +400,17 @@ struct cxl_region {
>  	struct cxl_region_params params;
>  };
>  
> -/**
> - * enum cxl_nvdimm_brige_state - state machine for managing bus rescans
> - * @CXL_NVB_NEW: Set at bridge create and after cxl_pmem_wq is destroyed
> - * @CXL_NVB_DEAD: Set at brige unregistration to preclude async probing
> - * @CXL_NVB_ONLINE: Target state after successful ->probe()
> - * @CXL_NVB_OFFLINE: Target state after ->remove() or failed ->probe()
> - */
> -enum cxl_nvdimm_brige_state {
> -	CXL_NVB_NEW,
> -	CXL_NVB_DEAD,
> -	CXL_NVB_ONLINE,
> -	CXL_NVB_OFFLINE,
> -};
> -
>  struct cxl_nvdimm_bridge {
>  	int id;
>  	struct device dev;
>  	struct cxl_port *port;
>  	struct nvdimm_bus *nvdimm_bus;
>  	struct nvdimm_bus_descriptor nd_desc;
> -	struct work_struct state_work;
> -	enum cxl_nvdimm_brige_state state;
>  };
>  
>  struct cxl_nvdimm {
>  	struct device dev;
>  	struct cxl_memdev *cxlmd;
> -	struct cxl_nvdimm_bridge *bridge;
>  };
>  
>  struct cxl_pmem_region_mapping {
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index 76cf54eeb310..0910367a3ead 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -11,13 +11,6 @@
>  #include "cxlmem.h"
>  #include "cxl.h"
>  
> -/*
> - * Ordered workqueue for cxl nvdimm device arrival and departure
> - * to coordinate bus rescans when a bridge arrives and trigger remove
> - * operations when the bridge is removed.
> - */
> -static struct workqueue_struct *cxl_pmem_wq;
> -
>  static __read_mostly DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
>  
>  static void clear_exclusive(void *cxlds)
> @@ -191,105 +184,6 @@ static void unregister_nvdimm_bus(void *_cxl_nvb)
>  	nvdimm_bus_unregister(nvdimm_bus);
>  }
>  
> -static bool online_nvdimm_bus(struct cxl_nvdimm_bridge *cxl_nvb)
> -{
> -	if (cxl_nvb->nvdimm_bus)
> -		return true;
> -	cxl_nvb->nvdimm_bus =
> -		nvdimm_bus_register(&cxl_nvb->dev, &cxl_nvb->nd_desc);
> -	return cxl_nvb->nvdimm_bus != NULL;
> -}
> -
> -static int cxl_nvdimm_release_driver(struct device *dev, void *cxl_nvb)
> -{
> -	struct cxl_nvdimm *cxl_nvd;
> -
> -	if (!is_cxl_nvdimm(dev))
> -		return 0;
> -
> -	cxl_nvd = to_cxl_nvdimm(dev);
> -	if (cxl_nvd->bridge != cxl_nvb)
> -		return 0;
> -
> -	device_release_driver(dev);
> -	return 0;
> -}
> -
> -static void offline_nvdimm_bus(struct cxl_nvdimm_bridge *cxl_nvb,
> -			       struct nvdimm_bus *nvdimm_bus)
> -{
> -	if (!nvdimm_bus)
> -		return;
> -
> -	/*
> -	 * Set the state of cxl_nvdimm devices to unbound / idle before
> -	 * nvdimm_bus_unregister() rips the nvdimm objects out from
> -	 * underneath them.
> -	 */
> -	bus_for_each_dev(&cxl_bus_type, NULL, cxl_nvb,
> -			 cxl_nvdimm_release_driver);
> -	nvdimm_bus_unregister(nvdimm_bus);
> -}
> -
> -static void cxl_nvb_update_state(struct work_struct *work)
> -{
> -	struct cxl_nvdimm_bridge *cxl_nvb =
> -		container_of(work, typeof(*cxl_nvb), state_work);
> -	struct nvdimm_bus *victim_bus = NULL;
> -	bool release = false, rescan = false;
> -
> -	device_lock(&cxl_nvb->dev);
> -	switch (cxl_nvb->state) {
> -	case CXL_NVB_ONLINE:
> -		if (!online_nvdimm_bus(cxl_nvb)) {
> -			dev_err(&cxl_nvb->dev,
> -				"failed to establish nvdimm bus\n");
> -			release = true;
> -		} else
> -			rescan = true;
> -		break;
> -	case CXL_NVB_OFFLINE:
> -	case CXL_NVB_DEAD:
> -		victim_bus = cxl_nvb->nvdimm_bus;
> -		cxl_nvb->nvdimm_bus = NULL;
> -		break;
> -	default:
> -		break;
> -	}
> -	device_unlock(&cxl_nvb->dev);
> -
> -	if (release)
> -		device_release_driver(&cxl_nvb->dev);
> -	if (rescan) {
> -		int rc = bus_rescan_devices(&cxl_bus_type);
> -
> -		dev_dbg(&cxl_nvb->dev, "rescan: %d\n", rc);
> -	}
> -	offline_nvdimm_bus(cxl_nvb, victim_bus);
> -
> -	put_device(&cxl_nvb->dev);
> -}
> -
> -static void cxl_nvdimm_bridge_state_work(struct cxl_nvdimm_bridge *cxl_nvb)
> -{
> -	/*
> -	 * Take a reference that the workqueue will drop if new work
> -	 * gets queued.
> -	 */
> -	get_device(&cxl_nvb->dev);
> -	if (!queue_work(cxl_pmem_wq, &cxl_nvb->state_work))
> -		put_device(&cxl_nvb->dev);
> -}
> -
> -static void cxl_nvdimm_bridge_remove(struct device *dev)
> -{
> -	struct cxl_nvdimm_bridge *cxl_nvb = to_cxl_nvdimm_bridge(dev);
> -
> -	if (cxl_nvb->state == CXL_NVB_ONLINE)
> -		cxl_nvb->state = CXL_NVB_OFFLINE;
> -	cxl_nvdimm_bridge_state_work(cxl_nvb);
> -}
> -
>  static int cxl_nvdimm_bridge_probe(struct device *dev)
>  {
>  	struct cxl_nvdimm_bridge *cxl_nvb = to_cxl_nvdimm_bridge(dev);
> @@ -306,15 +200,12 @@ static int cxl_nvdimm_bridge_probe(struct device *dev)
>  	if (!cxl_nvb->nvdimm_bus)
>  		return -ENOMEM;
>  
> -	INIT_WORK(&cxl_nvb->state_work, cxl_nvb_update_state);
> -
>  	return devm_add_action_or_reset(dev, unregister_nvdimm_bus, cxl_nvb);
>  }
>  
>  static struct cxl_driver cxl_nvdimm_bridge_driver = {
>  	.name = "cxl_nvdimm_bridge",
>  	.probe = cxl_nvdimm_bridge_probe,
> -	.remove = cxl_nvdimm_bridge_remove,
>  	.id = CXL_DEVICE_NVDIMM_BRIDGE,
>  	.drv = {
>  		.suppress_bind_attrs = true,
> @@ -453,31 +344,6 @@ static struct cxl_driver cxl_pmem_region_driver = {
>  	},
>  };
>  
> -/*
> - * Return all bridges to the CXL_NVB_NEW state to invalidate any
> - * ->state_work referring to the now destroyed cxl_pmem_wq.
> - */
> -static int cxl_nvdimm_bridge_reset(struct device *dev, void *data)
> -{
> -	struct cxl_nvdimm_bridge *cxl_nvb;
> -
> -	if (!is_cxl_nvdimm_bridge(dev))
> -		return 0;
> -
> -	cxl_nvb = to_cxl_nvdimm_bridge(dev);
> -	device_lock(dev);
> -	cxl_nvb->state = CXL_NVB_NEW;
> -	device_unlock(dev);
> -
> -	return 0;
> -}
> -
> -static void destroy_cxl_pmem_wq(void)
> -{
> -	destroy_workqueue(cxl_pmem_wq);
> -	bus_for_each_dev(&cxl_bus_type, NULL, NULL, cxl_nvdimm_bridge_reset);
> -}
> -
>  static __init int cxl_pmem_init(void)
>  {
>  	int rc;
> @@ -485,13 +351,9 @@ static __init int cxl_pmem_init(void)
>  	set_bit(CXL_MEM_COMMAND_ID_SET_SHUTDOWN_STATE, exclusive_cmds);
>  	set_bit(CXL_MEM_COMMAND_ID_SET_LSA, exclusive_cmds);
>  
> -	cxl_pmem_wq = alloc_ordered_workqueue("cxl_pmem", 0);
> -	if (!cxl_pmem_wq)
> -		return -ENXIO;
> -
>  	rc = cxl_driver_register(&cxl_nvdimm_bridge_driver);
>  	if (rc)
> -		goto err_bridge;
> +		return rc;
>  
>  	rc = cxl_driver_register(&cxl_nvdimm_driver);
>  	if (rc)
> @@ -507,8 +369,6 @@ static __init int cxl_pmem_init(void)
>  	cxl_driver_unregister(&cxl_nvdimm_driver);
>  err_nvdimm:
>  	cxl_driver_unregister(&cxl_nvdimm_bridge_driver);
> -err_bridge:
> -	destroy_cxl_pmem_wq();
>  	return rc;
>  }
>  
> @@ -517,7 +377,6 @@ static __exit void cxl_pmem_exit(void)
>  	cxl_driver_unregister(&cxl_pmem_region_driver);
>  	cxl_driver_unregister(&cxl_nvdimm_driver);
>  	cxl_driver_unregister(&cxl_nvdimm_bridge_driver);
> -	destroy_cxl_pmem_wq();
>  }
>  
>  MODULE_LICENSE("GPL v2");
> 

[PATCH v6 05/12] cxl/acpi: Move rescan to the workqueue

[Dan Williams]

Now that the cxl_mem driver has a need to take the root device lock, the
cxl_bus_rescan() needs to run outside of the root lock context.

Tested-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/acpi.c      |   17 +++++++++++++++--
 drivers/cxl/core/port.c |   19 +++++++++++++++++--
 drivers/cxl/cxl.h       |    3 ++-
 3 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index c540da0cbf1e..b8407b77aff6 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -511,7 +511,8 @@ static int cxl_acpi_probe(struct platform_device *pdev)
 		return rc;
 
 	/* In case PCI is scanned before ACPI re-trigger memdev attach */
-	return cxl_bus_rescan();
+	cxl_bus_rescan();
+	return 0;
 }
 
 static const struct acpi_device_id cxl_acpi_ids[] = {
@@ -535,7 +536,19 @@ static struct platform_driver cxl_acpi_driver = {
 	.id_table = cxl_test_ids,
 };
 
-module_platform_driver(cxl_acpi_driver);
+static int __init cxl_acpi_init(void)
+{
+	return platform_driver_register(&cxl_acpi_driver);
+}
+
+static void __exit cxl_acpi_exit(void)
+{
+	platform_driver_unregister(&cxl_acpi_driver);
+	cxl_bus_drain();
+}
+
+module_init(cxl_acpi_init);
+module_exit(cxl_acpi_exit);
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(CXL);
 MODULE_IMPORT_NS(ACPI);
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 0d2f5eaaca7d..d225267c69bb 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1844,12 +1844,27 @@ static void cxl_bus_remove(struct device *dev)
 
 static struct workqueue_struct *cxl_bus_wq;
 
-int cxl_bus_rescan(void)
+static void cxl_bus_rescan_queue(struct work_struct *w)
 {
-	return bus_rescan_devices(&cxl_bus_type);
+	int rc = bus_rescan_devices(&cxl_bus_type);
+
+	pr_debug("CXL bus rescan result: %d\n", rc);
+}
+
+void cxl_bus_rescan(void)
+{
+	static DECLARE_WORK(rescan_work, cxl_bus_rescan_queue);
+
+	queue_work(cxl_bus_wq, &rescan_work);
 }
 EXPORT_SYMBOL_NS_GPL(cxl_bus_rescan, CXL);
 
+void cxl_bus_drain(void)
+{
+	drain_workqueue(cxl_bus_wq);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_bus_drain, CXL);
+
 bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd)
 {
 	return queue_work(cxl_bus_wq, &cxlmd->detach_work);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index f0ca2d768385..281b1db5a271 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -552,7 +552,8 @@ int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd,
 			  struct cxl_dport *parent_dport);
 struct cxl_port *find_cxl_root(struct device *dev);
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
-int cxl_bus_rescan(void);
+void cxl_bus_rescan(void);
+void cxl_bus_drain(void);
 struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
 				   struct cxl_dport **dport);
 bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);

Re: [PATCH v6 05/12] cxl/acpi: Move rescan to the workqueue

[Jonathan Cameron]

On Thu, 01 Dec 2022 13:33:48 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Now that the cxl_mem driver has a need to take the root device lock, the
> cxl_bus_rescan() needs to run outside of the root lock context.

If possible add a bit more detail here or a reference to the patch
that needs to take the root device lock.

Change seems fine otherwise, so FWIW

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

> 
> Tested-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/acpi.c      |   17 +++++++++++++++--
>  drivers/cxl/core/port.c |   19 +++++++++++++++++--
>  drivers/cxl/cxl.h       |    3 ++-
>  3 files changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index c540da0cbf1e..b8407b77aff6 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -511,7 +511,8 @@ static int cxl_acpi_probe(struct platform_device *pdev)
>  		return rc;
>  
>  	/* In case PCI is scanned before ACPI re-trigger memdev attach */
> -	return cxl_bus_rescan();
> +	cxl_bus_rescan();
> +	return 0;
>  }
>  
>  static const struct acpi_device_id cxl_acpi_ids[] = {
> @@ -535,7 +536,19 @@ static struct platform_driver cxl_acpi_driver = {
>  	.id_table = cxl_test_ids,
>  };
>  
> -module_platform_driver(cxl_acpi_driver);
> +static int __init cxl_acpi_init(void)
> +{
> +	return platform_driver_register(&cxl_acpi_driver);
> +}
> +
> +static void __exit cxl_acpi_exit(void)
> +{
> +	platform_driver_unregister(&cxl_acpi_driver);
> +	cxl_bus_drain();
> +}
> +
> +module_init(cxl_acpi_init);
> +module_exit(cxl_acpi_exit);
>  MODULE_LICENSE("GPL v2");
>  MODULE_IMPORT_NS(CXL);
>  MODULE_IMPORT_NS(ACPI);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 0d2f5eaaca7d..d225267c69bb 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1844,12 +1844,27 @@ static void cxl_bus_remove(struct device *dev)
>  
>  static struct workqueue_struct *cxl_bus_wq;
>  
> -int cxl_bus_rescan(void)
> +static void cxl_bus_rescan_queue(struct work_struct *w)
>  {
> -	return bus_rescan_devices(&cxl_bus_type);
> +	int rc = bus_rescan_devices(&cxl_bus_type);
> +
> +	pr_debug("CXL bus rescan result: %d\n", rc);
> +}
> +
> +void cxl_bus_rescan(void)
> +{
> +	static DECLARE_WORK(rescan_work, cxl_bus_rescan_queue);
> +
> +	queue_work(cxl_bus_wq, &rescan_work);
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_bus_rescan, CXL);
>  
> +void cxl_bus_drain(void)
> +{
> +	drain_workqueue(cxl_bus_wq);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_bus_drain, CXL);
> +
>  bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd)
>  {
>  	return queue_work(cxl_bus_wq, &cxlmd->detach_work);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index f0ca2d768385..281b1db5a271 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -552,7 +552,8 @@ int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd,
>  			  struct cxl_dport *parent_dport);
>  struct cxl_port *find_cxl_root(struct device *dev);
>  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
> -int cxl_bus_rescan(void);
> +void cxl_bus_rescan(void);
> +void cxl_bus_drain(void);
>  struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
>  				   struct cxl_dport **dport);
>  bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
> 

Re: [PATCH v6 05/12] cxl/acpi: Move rescan to the workqueue

[Dan Williams]

Jonathan Cameron wrote:
> On Thu, 01 Dec 2022 13:33:48 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Now that the cxl_mem driver has a need to take the root device lock, the
> > cxl_bus_rescan() needs to run outside of the root lock context.
> 
> If possible add a bit more detail here or a reference to the patch
> that needs to take the root device lock.

Sure, updated to:

cxl/acpi: Move rescan to the workqueue

Now that the cxl_mem driver has a need to take the root device lock, the
cxl_bus_rescan() needs to run outside of the root lock context. That
need arises from RCH topologies and the locking that the cxl_mem driver
does to attach a descendant to an upstream port. In the RCH case the
lock needed is the CXL root device lock [1].

Link: http://lore.kernel.org/r/166993045621.1882361.1730100141527044744.stgit@dwillia2-xfh.jf.intel.com [1]

[PATCH v6 06/12] tools/testing/cxl: Make mock CEDT parsing more robust

[Dan Williams]

Accept any cxl_test topology device as the first argument in
cxl_chbs_context.

This is in preparation for reworking the detection of the component
registers across VH and RCH topologies. Move
mock_acpi_table_parse_cedt() beneath the definition of is_mock_port()
and use is_mock_port() instead of the explicit mock cxl_acpi device
check.

Acked-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 tools/testing/cxl/test/cxl.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index facfcd11cb67..8acf52b7dab2 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -320,10 +320,12 @@ static int populate_cedt(void)
 	return 0;
 }
 
+static bool is_mock_port(struct device *dev);
+
 /*
- * WARNING, this hack assumes the format of 'struct
- * cxl_cfmws_context' and 'struct cxl_chbs_context' share the property that
- * the first struct member is the device being probed by the cxl_acpi
+ * WARNING, this hack assumes the format of 'struct cxl_cfmws_context'
+ * and 'struct cxl_chbs_context' share the property that the first
+ * struct member is cxl_test device being probed by the cxl_acpi
  * driver.
  */
 struct cxl_cedt_context {
@@ -340,7 +342,7 @@ static int mock_acpi_table_parse_cedt(enum acpi_cedt_type id,
 	unsigned long end;
 	int i;
 
-	if (dev != &cxl_acpi->dev)
+	if (!is_mock_port(dev) && !is_mock_dev(dev))
 		return acpi_table_parse_cedt(id, handler_arg, arg);
 
 	if (id == ACPI_CEDT_TYPE_CHBS)

Re: [PATCH v6 06/12] tools/testing/cxl: Make mock CEDT parsing more robust

[Dave Jiang]

On 12/1/2022 2:33 PM, Dan Williams wrote:
> Accept any cxl_test topology device as the first argument in
> cxl_chbs_context.
> 
> This is in preparation for reworking the detection of the component
> registers across VH and RCH topologies. Move
> mock_acpi_table_parse_cedt() beneath the definition of is_mock_port()
> and use is_mock_port() instead of the explicit mock cxl_acpi device
> check.
> 
> Acked-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   tools/testing/cxl/test/cxl.c |   10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index facfcd11cb67..8acf52b7dab2 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -320,10 +320,12 @@ static int populate_cedt(void)
>   	return 0;
>   }
>   
> +static bool is_mock_port(struct device *dev);
> +
>   /*
> - * WARNING, this hack assumes the format of 'struct
> - * cxl_cfmws_context' and 'struct cxl_chbs_context' share the property that
> - * the first struct member is the device being probed by the cxl_acpi
> + * WARNING, this hack assumes the format of 'struct cxl_cfmws_context'
> + * and 'struct cxl_chbs_context' share the property that the first
> + * struct member is cxl_test device being probed by the cxl_acpi
>    * driver.
>    */
>   struct cxl_cedt_context {
> @@ -340,7 +342,7 @@ static int mock_acpi_table_parse_cedt(enum acpi_cedt_type id,
>   	unsigned long end;
>   	int i;
>   
> -	if (dev != &cxl_acpi->dev)
> +	if (!is_mock_port(dev) && !is_mock_dev(dev))
>   		return acpi_table_parse_cedt(id, handler_arg, arg);
>   
>   	if (id == ACPI_CEDT_TYPE_CHBS)
> 

Re: [PATCH v6 06/12] tools/testing/cxl: Make mock CEDT parsing more robust

[Jonathan Cameron]

On Thu, 01 Dec 2022 13:33:54 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Accept any cxl_test topology device as the first argument in
> cxl_chbs_context.
> 
> This is in preparation for reworking the detection of the component
> registers across VH and RCH topologies. Move
> mock_acpi_table_parse_cedt() beneath the definition of is_mock_port()
> and use is_mock_port() instead of the explicit mock cxl_acpi device
> check.
> 
> Acked-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
 A comment inline on possible improvement elsewhere, but otherwise seems fine.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  tools/testing/cxl/test/cxl.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index facfcd11cb67..8acf52b7dab2 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -320,10 +320,12 @@ static int populate_cedt(void)
>  	return 0;
>  }
>  
> +static bool is_mock_port(struct device *dev);
> +
>  /*
> - * WARNING, this hack assumes the format of 'struct
> - * cxl_cfmws_context' and 'struct cxl_chbs_context' share the property that
> - * the first struct member is the device being probed by the cxl_acpi
> + * WARNING, this hack assumes the format of 'struct cxl_cfmws_context'
> + * and 'struct cxl_chbs_context' share the property that the first
> + * struct member is cxl_test device being probed by the cxl_acpi
>   * driver.
Side note, but that requirement would be useful to add to the two
struct definitions so that we don't change those in future without knowing
we need to rethink this!

Beyond that dark mutterings about reformatting lines above the change made
and hence making this patch noisier than it needs to be...
 
>   */
>  struct cxl_cedt_context {
> @@ -340,7 +342,7 @@ static int mock_acpi_table_parse_cedt(enum acpi_cedt_type id,
>  	unsigned long end;
>  	int i;
>  
> -	if (dev != &cxl_acpi->dev)
> +	if (!is_mock_port(dev) && !is_mock_dev(dev))
>  		return acpi_table_parse_cedt(id, handler_arg, arg);
>  
>  	if (id == ACPI_CEDT_TYPE_CHBS)
> 

Re: [PATCH v6 06/12] tools/testing/cxl: Make mock CEDT parsing more robust

[Dan Williams]

Jonathan Cameron wrote:
> On Thu, 01 Dec 2022 13:33:54 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Accept any cxl_test topology device as the first argument in
> > cxl_chbs_context.
> > 
> > This is in preparation for reworking the detection of the component
> > registers across VH and RCH topologies. Move
> > mock_acpi_table_parse_cedt() beneath the definition of is_mock_port()
> > and use is_mock_port() instead of the explicit mock cxl_acpi device
> > check.
> > 
> > Acked-by: Alison Schofield <alison.schofield@intel.com>
> > Reviewed-by: Robert Richter <rrichter@amd.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>  A comment inline on possible improvement elsewhere, but otherwise seems fine.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 
> > ---
> >  tools/testing/cxl/test/cxl.c |   10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> > index facfcd11cb67..8acf52b7dab2 100644
> > --- a/tools/testing/cxl/test/cxl.c
> > +++ b/tools/testing/cxl/test/cxl.c
> > @@ -320,10 +320,12 @@ static int populate_cedt(void)
> >  	return 0;
> >  }
> >  
> > +static bool is_mock_port(struct device *dev);
> > +
> >  /*
> > - * WARNING, this hack assumes the format of 'struct
> > - * cxl_cfmws_context' and 'struct cxl_chbs_context' share the property that
> > - * the first struct member is the device being probed by the cxl_acpi
> > + * WARNING, this hack assumes the format of 'struct cxl_cfmws_context'
> > + * and 'struct cxl_chbs_context' share the property that the first
> > + * struct member is cxl_test device being probed by the cxl_acpi
> >   * driver.
> Side note, but that requirement would be useful to add to the two
> struct definitions so that we don't change those in future without knowing
> we need to rethink this!

Sure, folded in these hunks:

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index b8407b77aff6..2992bac4c0e4 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -70,6 +70,10 @@ static int cxl_acpi_cfmws_verify(struct device *dev,
        return 0;
 }
 
+/*
+ * Note, @dev must be the first member, see 'struct cxl_chbs_context'
+ * and mock_acpi_table_parse_cedt()
+ */
 struct cxl_cfmws_context {
        struct device *dev;
        struct cxl_port *root_port;
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index 8acf52b7dab2..4f9dc2b3f655 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -325,7 +325,7 @@ static bool is_mock_port(struct device *dev);
 /*
  * WARNING, this hack assumes the format of 'struct cxl_cfmws_context'
  * and 'struct cxl_chbs_context' share the property that the first
- * struct member is cxl_test device being probed by the cxl_acpi
+ * struct member is a cxl_test device being probed by the cxl_acpi
  * driver.
  */
 struct cxl_cedt_context {

> 
> Beyond that dark mutterings about reformatting lines above the change made
> and hence making this patch noisier than it needs to be...

True, I need to watch out for over auto-formatting.

[PATCH v6 07/12] cxl/ACPI: Register CXL host ports by bridge device

[Dan Williams]

From: Robert Richter <rrichter@amd.com>

A port of a CXL host bridge links to the bridge's ACPI device
(&adev->dev) with its corresponding uport/dport device (uport_dev and
dport_dev respectively). The device is not a direct parent device in
the PCI topology as pdev->dev.parent points to a PCI bridge's (struct
pci_host_bridge) device. The following CXL memory device hierarchy
would be valid for an endpoint once an RCD EP would be enabled (note
this will be done in a later patch):

VH mode:

 cxlmd->dev.parent->parent
        ^^^\^^^^^^\ ^^^^^^\
            \      \       pci_dev (Type 1, Downstream Port)
             \      pci_dev (Type 0, PCI Express Endpoint)
              cxl mem device

RCD mode:

 cxlmd->dev.parent->parent
        ^^^\^^^^^^\ ^^^^^^\
            \      \       pci_host_bridge
             \      pci_dev (Type 0, RCiEP)
              cxl mem device

In VH mode a downstream port is created by port enumeration and thus
always exists.

Now, in RCD mode the host bridge also already exists but it references
to an ACPI device. A port lookup by the PCI device's parent device
will fail as a direct link to the registered port is missing. The ACPI
device of the bridge must be determined first.

To prevent this, change port registration of a CXL host to use the
bridge device instead. Do this also for the VH case as port topology
will better reflect the PCI topology then.

Signed-off-by: Robert Richter <rrichter@amd.com>
[djbw: rebase on brige mocking]
Reviewed-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/acpi.c |   35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index b8407b77aff6..50d82376097c 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -193,35 +193,34 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 {
 	struct cxl_port *root_port = arg;
 	struct device *host = root_port->dev.parent;
-	struct acpi_device *bridge = to_cxl_host_bridge(host, match);
+	struct acpi_device *hb = to_cxl_host_bridge(host, match);
 	struct acpi_pci_root *pci_root;
 	struct cxl_dport *dport;
 	struct cxl_port *port;
+	struct device *bridge;
 	int rc;
 
-	if (!bridge)
+	if (!hb)
 		return 0;
 
-	dport = cxl_find_dport_by_dev(root_port, match);
+	pci_root = acpi_pci_find_root(hb->handle);
+	bridge = pci_root->bus->bridge;
+	dport = cxl_find_dport_by_dev(root_port, bridge);
 	if (!dport) {
 		dev_dbg(host, "host bridge expected and not found\n");
 		return 0;
 	}
 
-	/*
-	 * Note that this lookup already succeeded in
-	 * to_cxl_host_bridge(), so no need to check for failure here
-	 */
-	pci_root = acpi_pci_find_root(bridge->handle);
-	rc = devm_cxl_register_pci_bus(host, match, pci_root->bus);
+	rc = devm_cxl_register_pci_bus(host, bridge, pci_root->bus);
 	if (rc)
 		return rc;
 
-	port = devm_cxl_add_port(host, match, dport->component_reg_phys, dport);
+	port = devm_cxl_add_port(host, bridge, dport->component_reg_phys,
+				 dport);
 	if (IS_ERR(port))
 		return PTR_ERR(port);
 
-	dev_info(pci_root->bus->bridge, "host supports CXL\n");
+	dev_info(bridge, "host supports CXL\n");
 
 	return 0;
 }
@@ -253,18 +252,20 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
 static int add_host_bridge_dport(struct device *match, void *arg)
 {
 	acpi_status status;
+	struct device *bridge;
 	unsigned long long uid;
 	struct cxl_dport *dport;
 	struct cxl_chbs_context ctx;
+	struct acpi_pci_root *pci_root;
 	struct cxl_port *root_port = arg;
 	struct device *host = root_port->dev.parent;
-	struct acpi_device *bridge = to_cxl_host_bridge(host, match);
+	struct acpi_device *hb = to_cxl_host_bridge(host, match);
 
-	if (!bridge)
+	if (!hb)
 		return 0;
 
-	status = acpi_evaluate_integer(bridge->handle, METHOD_NAME__UID, NULL,
-				       &uid);
+	status =
+		acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid);
 	if (status != AE_OK) {
 		dev_err(match, "unable to retrieve _UID\n");
 		return -ENODEV;
@@ -285,7 +286,9 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 
 	dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)ctx.chbcr);
 
-	dport = devm_cxl_add_dport(root_port, match, uid, ctx.chbcr);
+	pci_root = acpi_pci_find_root(hb->handle);
+	bridge = pci_root->bus->bridge;
+	dport = devm_cxl_add_dport(root_port, bridge, uid, ctx.chbcr);
 	if (IS_ERR(dport))
 		return PTR_ERR(dport);
 

Re: [PATCH v6 07/12] cxl/ACPI: Register CXL host ports by bridge device

[Dave Jiang]

On 12/1/2022 2:33 PM, Dan Williams wrote:
> From: Robert Richter <rrichter@amd.com>
> 
> A port of a CXL host bridge links to the bridge's ACPI device
> (&adev->dev) with its corresponding uport/dport device (uport_dev and
> dport_dev respectively). The device is not a direct parent device in
> the PCI topology as pdev->dev.parent points to a PCI bridge's (struct
> pci_host_bridge) device. The following CXL memory device hierarchy
> would be valid for an endpoint once an RCD EP would be enabled (note
> this will be done in a later patch):
> 
> VH mode:
> 
>   cxlmd->dev.parent->parent
>          ^^^\^^^^^^\ ^^^^^^\
>              \      \       pci_dev (Type 1, Downstream Port)
>               \      pci_dev (Type 0, PCI Express Endpoint)
>                cxl mem device
> 
> RCD mode:
> 
>   cxlmd->dev.parent->parent
>          ^^^\^^^^^^\ ^^^^^^\
>              \      \       pci_host_bridge
>               \      pci_dev (Type 0, RCiEP)
>                cxl mem device
> 
> In VH mode a downstream port is created by port enumeration and thus
> always exists.
> 
> Now, in RCD mode the host bridge also already exists but it references
> to an ACPI device. A port lookup by the PCI device's parent device
> will fail as a direct link to the registered port is missing. The ACPI
> device of the bridge must be determined first.
> 
> To prevent this, change port registration of a CXL host to use the
> bridge device instead. Do this also for the VH case as port topology
> will better reflect the PCI topology then.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> [djbw: rebase on brige mocking]
> Reviewed-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/acpi.c |   35 +++++++++++++++++++----------------
>   1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index b8407b77aff6..50d82376097c 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -193,35 +193,34 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>   {
>   	struct cxl_port *root_port = arg;
>   	struct device *host = root_port->dev.parent;
> -	struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> +	struct acpi_device *hb = to_cxl_host_bridge(host, match);
>   	struct acpi_pci_root *pci_root;
>   	struct cxl_dport *dport;
>   	struct cxl_port *port;
> +	struct device *bridge;
>   	int rc;
>   
> -	if (!bridge)
> +	if (!hb)
>   		return 0;
>   
> -	dport = cxl_find_dport_by_dev(root_port, match);
> +	pci_root = acpi_pci_find_root(hb->handle);
> +	bridge = pci_root->bus->bridge;
> +	dport = cxl_find_dport_by_dev(root_port, bridge);
>   	if (!dport) {
>   		dev_dbg(host, "host bridge expected and not found\n");
>   		return 0;
>   	}
>   
> -	/*
> -	 * Note that this lookup already succeeded in
> -	 * to_cxl_host_bridge(), so no need to check for failure here
> -	 */
> -	pci_root = acpi_pci_find_root(bridge->handle);
> -	rc = devm_cxl_register_pci_bus(host, match, pci_root->bus);
> +	rc = devm_cxl_register_pci_bus(host, bridge, pci_root->bus);
>   	if (rc)
>   		return rc;
>   
> -	port = devm_cxl_add_port(host, match, dport->component_reg_phys, dport);
> +	port = devm_cxl_add_port(host, bridge, dport->component_reg_phys,
> +				 dport);
>   	if (IS_ERR(port))
>   		return PTR_ERR(port);
>   
> -	dev_info(pci_root->bus->bridge, "host supports CXL\n");
> +	dev_info(bridge, "host supports CXL\n");
>   
>   	return 0;
>   }
> @@ -253,18 +252,20 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
>   static int add_host_bridge_dport(struct device *match, void *arg)
>   {
>   	acpi_status status;
> +	struct device *bridge;
>   	unsigned long long uid;
>   	struct cxl_dport *dport;
>   	struct cxl_chbs_context ctx;
> +	struct acpi_pci_root *pci_root;
>   	struct cxl_port *root_port = arg;
>   	struct device *host = root_port->dev.parent;
> -	struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> +	struct acpi_device *hb = to_cxl_host_bridge(host, match);
>   
> -	if (!bridge)
> +	if (!hb)
>   		return 0;
>   
> -	status = acpi_evaluate_integer(bridge->handle, METHOD_NAME__UID, NULL,
> -				       &uid);
> +	status =
> +		acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid);
>   	if (status != AE_OK) {
>   		dev_err(match, "unable to retrieve _UID\n");
>   		return -ENODEV;
> @@ -285,7 +286,9 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>   
>   	dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)ctx.chbcr);
>   
> -	dport = devm_cxl_add_dport(root_port, match, uid, ctx.chbcr);
> +	pci_root = acpi_pci_find_root(hb->handle);
> +	bridge = pci_root->bus->bridge;
> +	dport = devm_cxl_add_dport(root_port, bridge, uid, ctx.chbcr);
>   	if (IS_ERR(dport))
>   		return PTR_ERR(dport);
>   
> 

Re: [PATCH v6 07/12] cxl/ACPI: Register CXL host ports by bridge device

[Jonathan Cameron]

On Thu, 01 Dec 2022 13:33:59 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> From: Robert Richter <rrichter@amd.com>
> 
> A port of a CXL host bridge links to the bridge's ACPI device
> (&adev->dev) with its corresponding uport/dport device (uport_dev and
> dport_dev respectively). The device is not a direct parent device in
> the PCI topology as pdev->dev.parent points to a PCI bridge's (struct
> pci_host_bridge) device. The following CXL memory device hierarchy
> would be valid for an endpoint once an RCD EP would be enabled (note
> this will be done in a later patch):
> 
> VH mode:
> 
>  cxlmd->dev.parent->parent
>         ^^^\^^^^^^\ ^^^^^^\
>             \      \       pci_dev (Type 1, Downstream Port)
>              \      pci_dev (Type 0, PCI Express Endpoint)
>               cxl mem device
> 
> RCD mode:
> 
>  cxlmd->dev.parent->parent
>         ^^^\^^^^^^\ ^^^^^^\
>             \      \       pci_host_bridge
>              \      pci_dev (Type 0, RCiEP)
>               cxl mem device
> 
> In VH mode a downstream port is created by port enumeration and thus
> always exists.
> 
> Now, in RCD mode the host bridge also already exists but it references
> to an ACPI device. A port lookup by the PCI device's parent device
> will fail as a direct link to the registered port is missing. The ACPI
> device of the bridge must be determined first.
> 
> To prevent this, change port registration of a CXL host to use the
> bridge device instead. Do this also for the VH case as port topology
> will better reflect the PCI topology then.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> [djbw: rebase on brige mocking]
> Reviewed-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Trivial comment inline. Looks fine to me otherwise...

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


> ---
>  drivers/cxl/acpi.c |   35 +++++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index b8407b77aff6..50d82376097c 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -193,35 +193,34 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  {
>  	struct cxl_port *root_port = arg;
>  	struct device *host = root_port->dev.parent;
> -	struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> +	struct acpi_device *hb = to_cxl_host_bridge(host, match);
>  	struct acpi_pci_root *pci_root;
>  	struct cxl_dport *dport;
>  	struct cxl_port *port;
> +	struct device *bridge;
>  	int rc;
>  
> -	if (!bridge)
> +	if (!hb)
>  		return 0;
>  
> -	dport = cxl_find_dport_by_dev(root_port, match);
> +	pci_root = acpi_pci_find_root(hb->handle);
> +	bridge = pci_root->bus->bridge;
> +	dport = cxl_find_dport_by_dev(root_port, bridge);
>  	if (!dport) {
>  		dev_dbg(host, "host bridge expected and not found\n");
>  		return 0;
>  	}
>  
> -	/*
> -	 * Note that this lookup already succeeded in
> -	 * to_cxl_host_bridge(), so no need to check for failure here
> -	 */
> -	pci_root = acpi_pci_find_root(bridge->handle);
> -	rc = devm_cxl_register_pci_bus(host, match, pci_root->bus);
> +	rc = devm_cxl_register_pci_bus(host, bridge, pci_root->bus);
>  	if (rc)
>  		return rc;
>  
> -	port = devm_cxl_add_port(host, match, dport->component_reg_phys, dport);
> +	port = devm_cxl_add_port(host, bridge, dport->component_reg_phys,
> +				 dport);
>  	if (IS_ERR(port))
>  		return PTR_ERR(port);
>  
> -	dev_info(pci_root->bus->bridge, "host supports CXL\n");
> +	dev_info(bridge, "host supports CXL\n");
>  
>  	return 0;
>  }
> @@ -253,18 +252,20 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
>  static int add_host_bridge_dport(struct device *match, void *arg)
>  {
>  	acpi_status status;
> +	struct device *bridge;
>  	unsigned long long uid;
>  	struct cxl_dport *dport;
>  	struct cxl_chbs_context ctx;
> +	struct acpi_pci_root *pci_root;
>  	struct cxl_port *root_port = arg;
>  	struct device *host = root_port->dev.parent;
> -	struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> +	struct acpi_device *hb = to_cxl_host_bridge(host, match);
>  
> -	if (!bridge)
> +	if (!hb)
>  		return 0;
>  
> -	status = acpi_evaluate_integer(bridge->handle, METHOD_NAME__UID, NULL,
> -				       &uid);
> +	status =
> +		acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid);

Bit ugly.  Maybe a good case for a slightly longer line!

>  	if (status != AE_OK) {
>  		dev_err(match, "unable to retrieve _UID\n");
>  		return -ENODEV;
> @@ -285,7 +286,9 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>  
>  	dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)ctx.chbcr);
>  
> -	dport = devm_cxl_add_dport(root_port, match, uid, ctx.chbcr);
> +	pci_root = acpi_pci_find_root(hb->handle);
> +	bridge = pci_root->bus->bridge;
> +	dport = devm_cxl_add_dport(root_port, bridge, uid, ctx.chbcr);
>  	if (IS_ERR(dport))
>  		return PTR_ERR(dport);
>  
> 

Re: [PATCH v6 07/12] cxl/ACPI: Register CXL host ports by bridge device

[Dan Williams]

Jonathan Cameron wrote:
> On Thu, 01 Dec 2022 13:33:59 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > From: Robert Richter <rrichter@amd.com>
> > 
> > A port of a CXL host bridge links to the bridge's ACPI device
> > (&adev->dev) with its corresponding uport/dport device (uport_dev and
> > dport_dev respectively). The device is not a direct parent device in
> > the PCI topology as pdev->dev.parent points to a PCI bridge's (struct
> > pci_host_bridge) device. The following CXL memory device hierarchy
> > would be valid for an endpoint once an RCD EP would be enabled (note
> > this will be done in a later patch):
> > 
> > VH mode:
> > 
> >  cxlmd->dev.parent->parent
> >         ^^^\^^^^^^\ ^^^^^^\
> >             \      \       pci_dev (Type 1, Downstream Port)
> >              \      pci_dev (Type 0, PCI Express Endpoint)
> >               cxl mem device
> > 
> > RCD mode:
> > 
> >  cxlmd->dev.parent->parent
> >         ^^^\^^^^^^\ ^^^^^^\
> >             \      \       pci_host_bridge
> >              \      pci_dev (Type 0, RCiEP)
> >               cxl mem device
> > 
> > In VH mode a downstream port is created by port enumeration and thus
> > always exists.
> > 
> > Now, in RCD mode the host bridge also already exists but it references
> > to an ACPI device. A port lookup by the PCI device's parent device
> > will fail as a direct link to the registered port is missing. The ACPI
> > device of the bridge must be determined first.
> > 
> > To prevent this, change port registration of a CXL host to use the
> > bridge device instead. Do this also for the VH case as port topology
> > will better reflect the PCI topology then.
> > 
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > [djbw: rebase on brige mocking]
> > Reviewed-by: Robert Richter <rrichter@amd.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Trivial comment inline. Looks fine to me otherwise...
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 
> > ---
> >  drivers/cxl/acpi.c |   35 +++++++++++++++++++----------------
> >  1 file changed, 19 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index b8407b77aff6..50d82376097c 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -193,35 +193,34 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> >  {
> >  	struct cxl_port *root_port = arg;
> >  	struct device *host = root_port->dev.parent;
> > -	struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> > +	struct acpi_device *hb = to_cxl_host_bridge(host, match);
> >  	struct acpi_pci_root *pci_root;
> >  	struct cxl_dport *dport;
> >  	struct cxl_port *port;
> > +	struct device *bridge;
> >  	int rc;
> >  
> > -	if (!bridge)
> > +	if (!hb)
> >  		return 0;
> >  
> > -	dport = cxl_find_dport_by_dev(root_port, match);
> > +	pci_root = acpi_pci_find_root(hb->handle);
> > +	bridge = pci_root->bus->bridge;
> > +	dport = cxl_find_dport_by_dev(root_port, bridge);
> >  	if (!dport) {
> >  		dev_dbg(host, "host bridge expected and not found\n");
> >  		return 0;
> >  	}
> >  
> > -	/*
> > -	 * Note that this lookup already succeeded in
> > -	 * to_cxl_host_bridge(), so no need to check for failure here
> > -	 */
> > -	pci_root = acpi_pci_find_root(bridge->handle);
> > -	rc = devm_cxl_register_pci_bus(host, match, pci_root->bus);
> > +	rc = devm_cxl_register_pci_bus(host, bridge, pci_root->bus);
> >  	if (rc)
> >  		return rc;
> >  
> > -	port = devm_cxl_add_port(host, match, dport->component_reg_phys, dport);
> > +	port = devm_cxl_add_port(host, bridge, dport->component_reg_phys,
> > +				 dport);
> >  	if (IS_ERR(port))
> >  		return PTR_ERR(port);
> >  
> > -	dev_info(pci_root->bus->bridge, "host supports CXL\n");
> > +	dev_info(bridge, "host supports CXL\n");
> >  
> >  	return 0;
> >  }
> > @@ -253,18 +252,20 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> >  static int add_host_bridge_dport(struct device *match, void *arg)
> >  {
> >  	acpi_status status;
> > +	struct device *bridge;
> >  	unsigned long long uid;
> >  	struct cxl_dport *dport;
> >  	struct cxl_chbs_context ctx;
> > +	struct acpi_pci_root *pci_root;
> >  	struct cxl_port *root_port = arg;
> >  	struct device *host = root_port->dev.parent;
> > -	struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> > +	struct acpi_device *hb = to_cxl_host_bridge(host, match);
> >  
> > -	if (!bridge)
> > +	if (!hb)
> >  		return 0;
> >  
> > -	status = acpi_evaluate_integer(bridge->handle, METHOD_NAME__UID, NULL,
> > -				       &uid);
> > +	status =
> > +		acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid);
> 
> Bit ugly.  Maybe a good case for a slightly longer line!

I just went ahead and shortened @status to @rc since there's no other
usages in this function.

[PATCH v6 08/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Dan Williams]

From: Robert Richter <rrichter@amd.com>

A downstream port must be connected to a component register block.
For restricted hosts the base address is determined from the RCRB. The
RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
get the RCRB and add code to extract the component register block from
it.

RCRB's BAR[0..1] point to the component block containing CXL subsystem
component registers. MEMBAR extraction follows the PCI base spec here,
esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1). The
RCRB base address is cached in the cxl_dport per-host bridge so that the
upstream port component registers can be retrieved later by an RCD
(RCIEP) associated with the host bridge.

Note: Right now the component register block is used for HDM decoder
capability only which is optional for RCDs. If unsupported by the RCD,
the HDM init will fail. It is future work to bypass it in this case.

Co-developed-by: Terry Bowman <terry.bowman@amd.com>
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
Link: https://lore.kernel.org/r/Y4dsGZ24aJlxSfI1@rric.localdomain
[djbw: introduce devm_cxl_add_rch_dport()]
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/acpi.c            |   51 ++++++++++++++++++++++++++++-----
 drivers/cxl/core/port.c       |   53 ++++++++++++++++++++++++++++++----
 drivers/cxl/core/regs.c       |   64 +++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h             |   16 ++++++++++
 tools/testing/cxl/Kbuild      |    1 +
 tools/testing/cxl/test/cxl.c  |   10 ++++++
 tools/testing/cxl/test/mock.c |   19 ++++++++++++
 tools/testing/cxl/test/mock.h |    3 ++
 8 files changed, 203 insertions(+), 14 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 50d82376097c..db8173f3ee10 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -9,6 +9,8 @@
 #include "cxlpci.h"
 #include "cxl.h"
 
+#define CXL_RCRB_SIZE	SZ_8K
+
 static unsigned long cfmws_to_decoder_flags(int restrictions)
 {
 	unsigned long flags = CXL_DECODER_F_ENABLE;
@@ -211,6 +213,11 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 		return 0;
 	}
 
+	if (dport->rch) {
+		dev_info(bridge, "host supports CXL (restricted)\n");
+		return 0;
+	}
+
 	rc = devm_cxl_register_pci_bus(host, bridge, pci_root->bus);
 	if (rc)
 		return rc;
@@ -226,9 +233,11 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 }
 
 struct cxl_chbs_context {
-	struct device *dev;
-	unsigned long long uid;
-	resource_size_t chbcr;
+	struct device		*dev;
+	unsigned long long	uid;
+	resource_size_t		rcrb;
+	resource_size_t		chbcr;
+	u32			cxl_version;
 };
 
 static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
@@ -244,7 +253,25 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
 
 	if (ctx->uid != chbs->uid)
 		return 0;
-	ctx->chbcr = chbs->base;
+
+	ctx->cxl_version = chbs->cxl_version;
+	ctx->rcrb = CXL_RESOURCE_NONE;
+	ctx->chbcr = CXL_RESOURCE_NONE;
+
+	if (!chbs->base)
+		return 0;
+
+	if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11) {
+		ctx->chbcr = chbs->base;
+		return 0;
+	}
+
+	if (chbs->length != CXL_RCRB_SIZE)
+		return 0;
+
+	ctx->rcrb = chbs->base;
+	ctx->chbcr = cxl_rcrb_to_component(ctx->dev, chbs->base,
+					   CXL_RCRB_DOWNSTREAM);
 
 	return 0;
 }
@@ -274,21 +301,29 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 	dev_dbg(match, "UID found: %lld\n", uid);
 
 	ctx = (struct cxl_chbs_context) {
-		.dev = host,
+		.dev = match,
 		.uid = uid,
 	};
 	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx);
 
-	if (ctx.chbcr == 0) {
+	if (ctx.rcrb != CXL_RESOURCE_NONE)
+		dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.rcrb);
+
+	if (ctx.chbcr == CXL_RESOURCE_NONE) {
 		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
 		return 0;
 	}
 
-	dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)ctx.chbcr);
+	dev_dbg(match, "CHBCR found: %pa\n", &ctx.chbcr);
 
 	pci_root = acpi_pci_find_root(hb->handle);
 	bridge = pci_root->bus->bridge;
-	dport = devm_cxl_add_dport(root_port, bridge, uid, ctx.chbcr);
+	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11)
+		dport = devm_cxl_add_rch_dport(root_port, bridge, uid,
+					       ctx.chbcr, ctx.rcrb);
+	else
+		dport = devm_cxl_add_dport(root_port, bridge, uid,
+					   ctx.chbcr);
 	if (IS_ERR(dport))
 		return PTR_ERR(dport);
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index d225267c69bb..dae2ca31885e 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -628,6 +628,8 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
 			iter = to_cxl_port(iter->dev.parent);
 		if (iter->host_bridge)
 			port->host_bridge = iter->host_bridge;
+		else if (parent_dport->rch)
+			port->host_bridge = parent_dport->dport;
 		else
 			port->host_bridge = iter->uport;
 		dev_dbg(uport, "host-bridge: %s\n", dev_name(port->host_bridge));
@@ -899,10 +901,10 @@ static void cxl_dport_unlink(void *data)
 	sysfs_remove_link(&port->dev.kobj, link_name);
 }
 
-static struct cxl_dport *__devm_cxl_add_dport(struct cxl_port *port,
-					      struct device *dport_dev,
-					      int port_id,
-					      resource_size_t component_reg_phys)
+static struct cxl_dport *
+__devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
+		     int port_id, resource_size_t component_reg_phys,
+		     resource_size_t rcrb)
 {
 	char link_name[CXL_TARGET_STRLEN];
 	struct cxl_dport *dport;
@@ -932,6 +934,9 @@ static struct cxl_dport *__devm_cxl_add_dport(struct cxl_port *port,
 	dport->port_id = port_id;
 	dport->component_reg_phys = component_reg_phys;
 	dport->port = port;
+	if (rcrb != CXL_RESOURCE_NONE)
+		dport->rch = true;
+	dport->rcrb = rcrb;
 
 	cond_cxl_root_lock(port);
 	rc = add_dport(port, dport);
@@ -956,7 +961,7 @@ static struct cxl_dport *__devm_cxl_add_dport(struct cxl_port *port,
 }
 
 /**
- * devm_cxl_add_dport - append downstream port data to a cxl_port
+ * devm_cxl_add_dport - append VH downstream port data to a cxl_port
  * @port: the cxl_port that references this dport
  * @dport_dev: firmware or PCI device representing the dport
  * @port_id: identifier for this dport in a decoder's target list
@@ -973,7 +978,7 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
 	struct cxl_dport *dport;
 
 	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
-				     component_reg_phys);
+				     component_reg_phys, CXL_RESOURCE_NONE);
 	if (IS_ERR(dport)) {
 		dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
 			dev_name(&port->dev), PTR_ERR(dport));
@@ -986,6 +991,42 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, CXL);
 
+/**
+ * devm_cxl_add_rch_dport - append RCH downstream port data to a cxl_port
+ * @port: the cxl_port that references this dport
+ * @dport_dev: firmware or PCI device representing the dport
+ * @port_id: identifier for this dport in a decoder's target list
+ * @component_reg_phys: optional location of CXL component registers
+ * @rcrb: mandatory location of a Root Complex Register Block
+ *
+ * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
+ */
+struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
+					 struct device *dport_dev, int port_id,
+					 resource_size_t component_reg_phys,
+					 resource_size_t rcrb)
+{
+	struct cxl_dport *dport;
+
+	if (rcrb == CXL_RESOURCE_NONE) {
+		dev_dbg(&port->dev, "failed to add RCH dport, missing RCRB\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
+				     component_reg_phys, rcrb);
+	if (IS_ERR(dport)) {
+		dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
+			dev_name(&port->dev), PTR_ERR(dport));
+	} else {
+		dev_dbg(dport_dev, "RCH dport added to %s\n",
+			dev_name(&port->dev));
+	}
+
+	return dport;
+}
+EXPORT_SYMBOL_NS_GPL(devm_cxl_add_rch_dport, CXL);
+
 static int add_ep(struct cxl_ep *new)
 {
 	struct cxl_port *port = new->dport->port;
diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index ec178e69b18f..28ed0ec8ee3e 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -307,3 +307,67 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
 	return -ENODEV;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
+
+resource_size_t cxl_rcrb_to_component(struct device *dev,
+				      resource_size_t rcrb,
+				      enum cxl_rcrb which)
+{
+	resource_size_t component_reg_phys;
+	u32 bar0, bar1;
+	void *addr;
+	u16 cmd;
+	u32 id;
+
+	if (which == CXL_RCRB_UPSTREAM)
+		rcrb += SZ_4K;
+
+	/*
+	 * RCRB's BAR[0..1] point to component block containing CXL
+	 * subsystem component registers. MEMBAR extraction follows
+	 * the PCI Base spec here, esp. 64 bit extraction and memory
+	 * ranges alignment (6.0, 7.5.1.2.1).
+	 */
+	if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
+		return CXL_RESOURCE_NONE;
+	addr = ioremap(rcrb, SZ_4K);
+	if (!addr) {
+		dev_err(dev, "Failed to map region %pr\n", addr);
+		release_mem_region(rcrb, SZ_4K);
+		return CXL_RESOURCE_NONE;
+	}
+
+	id = readl(addr + PCI_VENDOR_ID);
+	cmd = readw(addr + PCI_COMMAND);
+	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
+	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
+	iounmap(addr);
+	release_mem_region(rcrb, SZ_4K);
+
+	/*
+	 * Sanity check, see CXL 3.0 Figure 9-8 CXL Device that Does Not
+	 * Remap Upstream Port and Component Registers
+	 */
+	if (id == U32_MAX) {
+		if (which == CXL_RCRB_DOWNSTREAM)
+			dev_err(dev, "Failed to access Downstream Port RCRB\n");
+		return CXL_RESOURCE_NONE;
+	}
+	if (!(cmd & PCI_COMMAND_MEMORY))
+		return CXL_RESOURCE_NONE;
+	if (bar0 & (PCI_BASE_ADDRESS_MEM_TYPE_1M | PCI_BASE_ADDRESS_SPACE_IO))
+		return CXL_RESOURCE_NONE;
+
+	component_reg_phys = bar0 & PCI_BASE_ADDRESS_MEM_MASK;
+	if (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_64)
+		component_reg_phys |= ((u64)bar1) << 32;
+
+	if (!component_reg_phys)
+		return CXL_RESOURCE_NONE;
+
+	/* MEMBAR is block size (64k) aligned. */
+	if (!IS_ALIGNED(component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE))
+		return CXL_RESOURCE_NONE;
+
+	return component_reg_phys;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 281b1db5a271..1342e4e61537 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -223,6 +223,14 @@ enum cxl_regloc_type;
 int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
 		      struct cxl_register_map *map);
 
+enum cxl_rcrb {
+	CXL_RCRB_DOWNSTREAM,
+	CXL_RCRB_UPSTREAM,
+};
+resource_size_t cxl_rcrb_to_component(struct device *dev,
+				      resource_size_t rcrb,
+				      enum cxl_rcrb which);
+
 #define CXL_RESOURCE_NONE ((resource_size_t) -1)
 #define CXL_TARGET_STRLEN 20
 
@@ -486,12 +494,16 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
  * @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
+ * @rcrb: base address for the Root Complex Register Block
+ * @rch: Indicate whether this dport was enumerated in RCH or VH mode
  * @port: reference to cxl_port that contains this downstream port
  */
 struct cxl_dport {
 	struct device *dport;
 	int port_id;
 	resource_size_t component_reg_phys;
+	resource_size_t rcrb;
+	bool rch;
 	struct cxl_port *port;
 };
 
@@ -561,6 +573,10 @@ bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
 struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
 				     struct device *dport, int port_id,
 				     resource_size_t component_reg_phys);
+struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
+					 struct device *dport_dev, int port_id,
+					 resource_size_t component_reg_phys,
+					 resource_size_t rcrb);
 
 struct cxl_decoder *to_cxl_decoder(struct device *dev);
 struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev);
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 500be85729cc..9e4d94e81723 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -10,6 +10,7 @@ ldflags-y += --wrap=devm_cxl_add_passthrough_decoder
 ldflags-y += --wrap=devm_cxl_enumerate_decoders
 ldflags-y += --wrap=cxl_await_media_ready
 ldflags-y += --wrap=cxl_hdm_decode_init
+ldflags-y += --wrap=cxl_rcrb_to_component
 
 DRIVERS := ../../../drivers
 CXL_SRC := $(DRIVERS)/cxl
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index 8acf52b7dab2..c1e395a5b8f7 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -696,6 +696,15 @@ static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
 	return 0;
 }
 
+resource_size_t mock_cxl_rcrb_to_component(struct device *dev,
+					   resource_size_t rcrb,
+					   enum cxl_rcrb which)
+{
+	dev_dbg(dev, "rcrb: %pa which: %d\n", &rcrb, which);
+
+	return 0;
+}
+
 static struct cxl_mock_ops cxl_mock_ops = {
 	.is_mock_adev = is_mock_adev,
 	.is_mock_bridge = is_mock_bridge,
@@ -704,6 +713,7 @@ static struct cxl_mock_ops cxl_mock_ops = {
 	.is_mock_dev = is_mock_dev,
 	.acpi_table_parse_cedt = mock_acpi_table_parse_cedt,
 	.acpi_evaluate_integer = mock_acpi_evaluate_integer,
+	.cxl_rcrb_to_component = mock_cxl_rcrb_to_component,
 	.acpi_pci_find_root = mock_acpi_pci_find_root,
 	.devm_cxl_port_enumerate_dports = mock_cxl_port_enumerate_dports,
 	.devm_cxl_setup_hdm = mock_cxl_setup_hdm,
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index bce6a21df0d5..5dface08e0de 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -224,6 +224,25 @@ int __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 }
 EXPORT_SYMBOL_NS_GPL(__wrap_cxl_hdm_decode_init, CXL);
 
+resource_size_t __wrap_cxl_rcrb_to_component(struct device *dev,
+					     resource_size_t rcrb,
+					     enum cxl_rcrb which)
+{
+	int index;
+	resource_size_t component_reg_phys;
+	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
+
+	if (ops && ops->is_mock_port(dev))
+		component_reg_phys =
+			ops->cxl_rcrb_to_component(dev, rcrb, which);
+	else
+		component_reg_phys = cxl_rcrb_to_component(dev, rcrb, which);
+	put_cxl_mock_ops(index);
+
+	return component_reg_phys;
+}
+EXPORT_SYMBOL_NS_GPL(__wrap_cxl_rcrb_to_component, CXL);
+
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(ACPI);
 MODULE_IMPORT_NS(CXL);
diff --git a/tools/testing/cxl/test/mock.h b/tools/testing/cxl/test/mock.h
index 738f24e3988a..ef33f159375e 100644
--- a/tools/testing/cxl/test/mock.h
+++ b/tools/testing/cxl/test/mock.h
@@ -15,6 +15,9 @@ struct cxl_mock_ops {
 					     acpi_string pathname,
 					     struct acpi_object_list *arguments,
 					     unsigned long long *data);
+	resource_size_t (*cxl_rcrb_to_component)(struct device *dev,
+						 resource_size_t rcrb,
+						 enum cxl_rcrb which);
 	struct acpi_pci_root *(*acpi_pci_find_root)(acpi_handle handle);
 	bool (*is_mock_bus)(struct pci_bus *bus);
 	bool (*is_mock_port)(struct device *dev);

Re: [PATCH v6 08/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Dave Jiang]

On 12/1/2022 2:34 PM, Dan Williams wrote:
> From: Robert Richter <rrichter@amd.com>
> 
> A downstream port must be connected to a component register block.
> For restricted hosts the base address is determined from the RCRB. The
> RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> get the RCRB and add code to extract the component register block from
> it.
> 
> RCRB's BAR[0..1] point to the component block containing CXL subsystem
> component registers. MEMBAR extraction follows the PCI base spec here,
> esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1). The
> RCRB base address is cached in the cxl_dport per-host bridge so that the
> upstream port component registers can be retrieved later by an RCD
> (RCIEP) associated with the host bridge.
> 
> Note: Right now the component register block is used for HDM decoder
> capability only which is optional for RCDs. If unsupported by the RCD,
> the HDM init will fail. It is future work to bypass it in this case.
> 
> Co-developed-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Link: https://lore.kernel.org/r/Y4dsGZ24aJlxSfI1@rric.localdomain
> [djbw: introduce devm_cxl_add_rch_dport()]
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/acpi.c            |   51 ++++++++++++++++++++++++++++-----
>   drivers/cxl/core/port.c       |   53 ++++++++++++++++++++++++++++++----
>   drivers/cxl/core/regs.c       |   64 +++++++++++++++++++++++++++++++++++++++++
>   drivers/cxl/cxl.h             |   16 ++++++++++
>   tools/testing/cxl/Kbuild      |    1 +
>   tools/testing/cxl/test/cxl.c  |   10 ++++++
>   tools/testing/cxl/test/mock.c |   19 ++++++++++++
>   tools/testing/cxl/test/mock.h |    3 ++
>   8 files changed, 203 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 50d82376097c..db8173f3ee10 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -9,6 +9,8 @@
>   #include "cxlpci.h"
>   #include "cxl.h"
>   
> +#define CXL_RCRB_SIZE	SZ_8K
> +
>   static unsigned long cfmws_to_decoder_flags(int restrictions)
>   {
>   	unsigned long flags = CXL_DECODER_F_ENABLE;
> @@ -211,6 +213,11 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>   		return 0;
>   	}
>   
> +	if (dport->rch) {
> +		dev_info(bridge, "host supports CXL (restricted)\n");
> +		return 0;
> +	}
> +
>   	rc = devm_cxl_register_pci_bus(host, bridge, pci_root->bus);
>   	if (rc)
>   		return rc;
> @@ -226,9 +233,11 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>   }
>   
>   struct cxl_chbs_context {
> -	struct device *dev;
> -	unsigned long long uid;
> -	resource_size_t chbcr;
> +	struct device		*dev;
> +	unsigned long long	uid;
> +	resource_size_t		rcrb;
> +	resource_size_t		chbcr;
> +	u32			cxl_version;
>   };
>   
>   static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> @@ -244,7 +253,25 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
>   
>   	if (ctx->uid != chbs->uid)
>   		return 0;
> -	ctx->chbcr = chbs->base;
> +
> +	ctx->cxl_version = chbs->cxl_version;
> +	ctx->rcrb = CXL_RESOURCE_NONE;
> +	ctx->chbcr = CXL_RESOURCE_NONE;
> +
> +	if (!chbs->base)
> +		return 0;
> +
> +	if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11) {
> +		ctx->chbcr = chbs->base;
> +		return 0;
> +	}
> +
> +	if (chbs->length != CXL_RCRB_SIZE)
> +		return 0;
> +
> +	ctx->rcrb = chbs->base;
> +	ctx->chbcr = cxl_rcrb_to_component(ctx->dev, chbs->base,
> +					   CXL_RCRB_DOWNSTREAM);
>   
>   	return 0;
>   }
> @@ -274,21 +301,29 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>   	dev_dbg(match, "UID found: %lld\n", uid);
>   
>   	ctx = (struct cxl_chbs_context) {
> -		.dev = host,
> +		.dev = match,
>   		.uid = uid,
>   	};
>   	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx);
>   
> -	if (ctx.chbcr == 0) {
> +	if (ctx.rcrb != CXL_RESOURCE_NONE)
> +		dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.rcrb);
> +
> +	if (ctx.chbcr == CXL_RESOURCE_NONE) {
>   		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
>   		return 0;
>   	}
>   
> -	dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)ctx.chbcr);
> +	dev_dbg(match, "CHBCR found: %pa\n", &ctx.chbcr);
>   
>   	pci_root = acpi_pci_find_root(hb->handle);
>   	bridge = pci_root->bus->bridge;
> -	dport = devm_cxl_add_dport(root_port, bridge, uid, ctx.chbcr);
> +	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11)
> +		dport = devm_cxl_add_rch_dport(root_port, bridge, uid,
> +					       ctx.chbcr, ctx.rcrb);
> +	else
> +		dport = devm_cxl_add_dport(root_port, bridge, uid,
> +					   ctx.chbcr);
>   	if (IS_ERR(dport))
>   		return PTR_ERR(dport);
>   
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index d225267c69bb..dae2ca31885e 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -628,6 +628,8 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
>   			iter = to_cxl_port(iter->dev.parent);
>   		if (iter->host_bridge)
>   			port->host_bridge = iter->host_bridge;
> +		else if (parent_dport->rch)
> +			port->host_bridge = parent_dport->dport;
>   		else
>   			port->host_bridge = iter->uport;
>   		dev_dbg(uport, "host-bridge: %s\n", dev_name(port->host_bridge));
> @@ -899,10 +901,10 @@ static void cxl_dport_unlink(void *data)
>   	sysfs_remove_link(&port->dev.kobj, link_name);
>   }
>   
> -static struct cxl_dport *__devm_cxl_add_dport(struct cxl_port *port,
> -					      struct device *dport_dev,
> -					      int port_id,
> -					      resource_size_t component_reg_phys)
> +static struct cxl_dport *
> +__devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> +		     int port_id, resource_size_t component_reg_phys,
> +		     resource_size_t rcrb)
>   {
>   	char link_name[CXL_TARGET_STRLEN];
>   	struct cxl_dport *dport;
> @@ -932,6 +934,9 @@ static struct cxl_dport *__devm_cxl_add_dport(struct cxl_port *port,
>   	dport->port_id = port_id;
>   	dport->component_reg_phys = component_reg_phys;
>   	dport->port = port;
> +	if (rcrb != CXL_RESOURCE_NONE)
> +		dport->rch = true;
> +	dport->rcrb = rcrb;
>   
>   	cond_cxl_root_lock(port);
>   	rc = add_dport(port, dport);
> @@ -956,7 +961,7 @@ static struct cxl_dport *__devm_cxl_add_dport(struct cxl_port *port,
>   }
>   
>   /**
> - * devm_cxl_add_dport - append downstream port data to a cxl_port
> + * devm_cxl_add_dport - append VH downstream port data to a cxl_port
>    * @port: the cxl_port that references this dport
>    * @dport_dev: firmware or PCI device representing the dport
>    * @port_id: identifier for this dport in a decoder's target list
> @@ -973,7 +978,7 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
>   	struct cxl_dport *dport;
>   
>   	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> -				     component_reg_phys);
> +				     component_reg_phys, CXL_RESOURCE_NONE);
>   	if (IS_ERR(dport)) {
>   		dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
>   			dev_name(&port->dev), PTR_ERR(dport));
> @@ -986,6 +991,42 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
>   }
>   EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, CXL);
>   
> +/**
> + * devm_cxl_add_rch_dport - append RCH downstream port data to a cxl_port
> + * @port: the cxl_port that references this dport
> + * @dport_dev: firmware or PCI device representing the dport
> + * @port_id: identifier for this dport in a decoder's target list
> + * @component_reg_phys: optional location of CXL component registers
> + * @rcrb: mandatory location of a Root Complex Register Block
> + *
> + * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
> + */
> +struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> +					 struct device *dport_dev, int port_id,
> +					 resource_size_t component_reg_phys,
> +					 resource_size_t rcrb)
> +{
> +	struct cxl_dport *dport;
> +
> +	if (rcrb == CXL_RESOURCE_NONE) {
> +		dev_dbg(&port->dev, "failed to add RCH dport, missing RCRB\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> +				     component_reg_phys, rcrb);
> +	if (IS_ERR(dport)) {
> +		dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
> +			dev_name(&port->dev), PTR_ERR(dport));
> +	} else {
> +		dev_dbg(dport_dev, "RCH dport added to %s\n",
> +			dev_name(&port->dev));
> +	}
> +
> +	return dport;
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_add_rch_dport, CXL);
> +
>   static int add_ep(struct cxl_ep *new)
>   {
>   	struct cxl_port *port = new->dport->port;
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index ec178e69b18f..28ed0ec8ee3e 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -307,3 +307,67 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
>   	return -ENODEV;
>   }
>   EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
> +
> +resource_size_t cxl_rcrb_to_component(struct device *dev,
> +				      resource_size_t rcrb,
> +				      enum cxl_rcrb which)
> +{
> +	resource_size_t component_reg_phys;
> +	u32 bar0, bar1;
> +	void *addr;
> +	u16 cmd;
> +	u32 id;
> +
> +	if (which == CXL_RCRB_UPSTREAM)
> +		rcrb += SZ_4K;
> +
> +	/*
> +	 * RCRB's BAR[0..1] point to component block containing CXL
> +	 * subsystem component registers. MEMBAR extraction follows
> +	 * the PCI Base spec here, esp. 64 bit extraction and memory
> +	 * ranges alignment (6.0, 7.5.1.2.1).
> +	 */
> +	if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
> +		return CXL_RESOURCE_NONE;
> +	addr = ioremap(rcrb, SZ_4K);
> +	if (!addr) {
> +		dev_err(dev, "Failed to map region %pr\n", addr);
> +		release_mem_region(rcrb, SZ_4K);
> +		return CXL_RESOURCE_NONE;
> +	}
> +
> +	id = readl(addr + PCI_VENDOR_ID);
> +	cmd = readw(addr + PCI_COMMAND);
> +	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> +	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +	iounmap(addr);
> +	release_mem_region(rcrb, SZ_4K);
> +
> +	/*
> +	 * Sanity check, see CXL 3.0 Figure 9-8 CXL Device that Does Not
> +	 * Remap Upstream Port and Component Registers
> +	 */
> +	if (id == U32_MAX) {
> +		if (which == CXL_RCRB_DOWNSTREAM)
> +			dev_err(dev, "Failed to access Downstream Port RCRB\n");
> +		return CXL_RESOURCE_NONE;
> +	}
> +	if (!(cmd & PCI_COMMAND_MEMORY))
> +		return CXL_RESOURCE_NONE;
> +	if (bar0 & (PCI_BASE_ADDRESS_MEM_TYPE_1M | PCI_BASE_ADDRESS_SPACE_IO))
> +		return CXL_RESOURCE_NONE;
> +
> +	component_reg_phys = bar0 & PCI_BASE_ADDRESS_MEM_MASK;
> +	if (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_64)
> +		component_reg_phys |= ((u64)bar1) << 32;
> +
> +	if (!component_reg_phys)
> +		return CXL_RESOURCE_NONE;
> +
> +	/* MEMBAR is block size (64k) aligned. */
> +	if (!IS_ALIGNED(component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE))
> +		return CXL_RESOURCE_NONE;
> +
> +	return component_reg_phys;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 281b1db5a271..1342e4e61537 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -223,6 +223,14 @@ enum cxl_regloc_type;
>   int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
>   		      struct cxl_register_map *map);
>   
> +enum cxl_rcrb {
> +	CXL_RCRB_DOWNSTREAM,
> +	CXL_RCRB_UPSTREAM,
> +};
> +resource_size_t cxl_rcrb_to_component(struct device *dev,
> +				      resource_size_t rcrb,
> +				      enum cxl_rcrb which);
> +
>   #define CXL_RESOURCE_NONE ((resource_size_t) -1)
>   #define CXL_TARGET_STRLEN 20
>   
> @@ -486,12 +494,16 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
>    * @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
> + * @rcrb: base address for the Root Complex Register Block
> + * @rch: Indicate whether this dport was enumerated in RCH or VH mode
>    * @port: reference to cxl_port that contains this downstream port
>    */
>   struct cxl_dport {
>   	struct device *dport;
>   	int port_id;
>   	resource_size_t component_reg_phys;
> +	resource_size_t rcrb;
> +	bool rch;
>   	struct cxl_port *port;
>   };
>   
> @@ -561,6 +573,10 @@ bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
>   struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
>   				     struct device *dport, int port_id,
>   				     resource_size_t component_reg_phys);
> +struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> +					 struct device *dport_dev, int port_id,
> +					 resource_size_t component_reg_phys,
> +					 resource_size_t rcrb);
>   
>   struct cxl_decoder *to_cxl_decoder(struct device *dev);
>   struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev);
> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> index 500be85729cc..9e4d94e81723 100644
> --- a/tools/testing/cxl/Kbuild
> +++ b/tools/testing/cxl/Kbuild
> @@ -10,6 +10,7 @@ ldflags-y += --wrap=devm_cxl_add_passthrough_decoder
>   ldflags-y += --wrap=devm_cxl_enumerate_decoders
>   ldflags-y += --wrap=cxl_await_media_ready
>   ldflags-y += --wrap=cxl_hdm_decode_init
> +ldflags-y += --wrap=cxl_rcrb_to_component
>   
>   DRIVERS := ../../../drivers
>   CXL_SRC := $(DRIVERS)/cxl
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index 8acf52b7dab2..c1e395a5b8f7 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -696,6 +696,15 @@ static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
>   	return 0;
>   }
>   
> +resource_size_t mock_cxl_rcrb_to_component(struct device *dev,
> +					   resource_size_t rcrb,
> +					   enum cxl_rcrb which)
> +{
> +	dev_dbg(dev, "rcrb: %pa which: %d\n", &rcrb, which);
> +
> +	return 0;
> +}
> +
>   static struct cxl_mock_ops cxl_mock_ops = {
>   	.is_mock_adev = is_mock_adev,
>   	.is_mock_bridge = is_mock_bridge,
> @@ -704,6 +713,7 @@ static struct cxl_mock_ops cxl_mock_ops = {
>   	.is_mock_dev = is_mock_dev,
>   	.acpi_table_parse_cedt = mock_acpi_table_parse_cedt,
>   	.acpi_evaluate_integer = mock_acpi_evaluate_integer,
> +	.cxl_rcrb_to_component = mock_cxl_rcrb_to_component,
>   	.acpi_pci_find_root = mock_acpi_pci_find_root,
>   	.devm_cxl_port_enumerate_dports = mock_cxl_port_enumerate_dports,
>   	.devm_cxl_setup_hdm = mock_cxl_setup_hdm,
> diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
> index bce6a21df0d5..5dface08e0de 100644
> --- a/tools/testing/cxl/test/mock.c
> +++ b/tools/testing/cxl/test/mock.c
> @@ -224,6 +224,25 @@ int __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
>   }
>   EXPORT_SYMBOL_NS_GPL(__wrap_cxl_hdm_decode_init, CXL);
>   
> +resource_size_t __wrap_cxl_rcrb_to_component(struct device *dev,
> +					     resource_size_t rcrb,
> +					     enum cxl_rcrb which)
> +{
> +	int index;
> +	resource_size_t component_reg_phys;
> +	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
> +
> +	if (ops && ops->is_mock_port(dev))
> +		component_reg_phys =
> +			ops->cxl_rcrb_to_component(dev, rcrb, which);
> +	else
> +		component_reg_phys = cxl_rcrb_to_component(dev, rcrb, which);
> +	put_cxl_mock_ops(index);
> +
> +	return component_reg_phys;
> +}
> +EXPORT_SYMBOL_NS_GPL(__wrap_cxl_rcrb_to_component, CXL);
> +
>   MODULE_LICENSE("GPL v2");
>   MODULE_IMPORT_NS(ACPI);
>   MODULE_IMPORT_NS(CXL);
> diff --git a/tools/testing/cxl/test/mock.h b/tools/testing/cxl/test/mock.h
> index 738f24e3988a..ef33f159375e 100644
> --- a/tools/testing/cxl/test/mock.h
> +++ b/tools/testing/cxl/test/mock.h
> @@ -15,6 +15,9 @@ struct cxl_mock_ops {
>   					     acpi_string pathname,
>   					     struct acpi_object_list *arguments,
>   					     unsigned long long *data);
> +	resource_size_t (*cxl_rcrb_to_component)(struct device *dev,
> +						 resource_size_t rcrb,
> +						 enum cxl_rcrb which);
>   	struct acpi_pci_root *(*acpi_pci_find_root)(acpi_handle handle);
>   	bool (*is_mock_bus)(struct pci_bus *bus);
>   	bool (*is_mock_port)(struct device *dev);
> 

Re: [PATCH v6 08/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Robert Richter]

On 01.12.22 13:34:05, Dan Williams wrote:
> From: Robert Richter <rrichter@amd.com>
> 
> A downstream port must be connected to a component register block.
> For restricted hosts the base address is determined from the RCRB. The
> RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> get the RCRB and add code to extract the component register block from
> it.
> 
> RCRB's BAR[0..1] point to the component block containing CXL subsystem
> component registers. MEMBAR extraction follows the PCI base spec here,
> esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1). The
> RCRB base address is cached in the cxl_dport per-host bridge so that the
> upstream port component registers can be retrieved later by an RCD
> (RCIEP) associated with the host bridge.
> 
> Note: Right now the component register block is used for HDM decoder
> capability only which is optional for RCDs. If unsupported by the RCD,
> the HDM init will fail. It is future work to bypass it in this case.
> 
> Co-developed-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Link: https://lore.kernel.org/r/Y4dsGZ24aJlxSfI1@rric.localdomain
> [djbw: introduce devm_cxl_add_rch_dport()]
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Found an issue below. Patch looks good to me otherwise.

> ---
>  drivers/cxl/acpi.c            |   51 ++++++++++++++++++++++++++++-----
>  drivers/cxl/core/port.c       |   53 ++++++++++++++++++++++++++++++----
>  drivers/cxl/core/regs.c       |   64 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h             |   16 ++++++++++
>  tools/testing/cxl/Kbuild      |    1 +
>  tools/testing/cxl/test/cxl.c  |   10 ++++++
>  tools/testing/cxl/test/mock.c |   19 ++++++++++++
>  tools/testing/cxl/test/mock.h |    3 ++
>  8 files changed, 203 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c

> @@ -274,21 +301,29 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>  	dev_dbg(match, "UID found: %lld\n", uid);
>  
>  	ctx = (struct cxl_chbs_context) {
> -		.dev = host,
> +		.dev = match,
>  		.uid = uid,
>  	};
>  	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx);
>  
> -	if (ctx.chbcr == 0) {
> +	if (ctx.rcrb != CXL_RESOURCE_NONE)
> +		dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.rcrb);
> +
> +	if (ctx.chbcr == CXL_RESOURCE_NONE) {
>  		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
>  		return 0;
>  	}

The logic must be changed to handle the case where the chbs entry is
missing:

	if (!ctx.chbcr) {
		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
		return 0;
	}

	if (ctx.rcrb != CXL_RESOURCE_NONE)
		dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.rcrb);

	if (ctx.chbcr == CXL_RESOURCE_NONE) {
		dev_warn(match, "CHBCR missing for Host Bridge (UID %lld)\n", uid);
		return 0;
	}

>  
> -	dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)ctx.chbcr);
> +	dev_dbg(match, "CHBCR found: %pa\n", &ctx.chbcr);

Re: [PATCH v6 08/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Dan Williams]

Robert Richter wrote:
> On 01.12.22 13:34:05, Dan Williams wrote:
> > From: Robert Richter <rrichter@amd.com>
> > 
> > A downstream port must be connected to a component register block.
> > For restricted hosts the base address is determined from the RCRB. The
> > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > get the RCRB and add code to extract the component register block from
> > it.
> > 
> > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > component registers. MEMBAR extraction follows the PCI base spec here,
> > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1). The
> > RCRB base address is cached in the cxl_dport per-host bridge so that the
> > upstream port component registers can be retrieved later by an RCD
> > (RCIEP) associated with the host bridge.
> > 
> > Note: Right now the component register block is used for HDM decoder
> > capability only which is optional for RCDs. If unsupported by the RCD,
> > the HDM init will fail. It is future work to bypass it in this case.
> > 
> > Co-developed-by: Terry Bowman <terry.bowman@amd.com>
> > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > Link: https://lore.kernel.org/r/Y4dsGZ24aJlxSfI1@rric.localdomain
> > [djbw: introduce devm_cxl_add_rch_dport()]
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> Found an issue below. Patch looks good to me otherwise.
> 
> > ---
> >  drivers/cxl/acpi.c            |   51 ++++++++++++++++++++++++++++-----
> >  drivers/cxl/core/port.c       |   53 ++++++++++++++++++++++++++++++----
> >  drivers/cxl/core/regs.c       |   64 +++++++++++++++++++++++++++++++++++++++++
> >  drivers/cxl/cxl.h             |   16 ++++++++++
> >  tools/testing/cxl/Kbuild      |    1 +
> >  tools/testing/cxl/test/cxl.c  |   10 ++++++
> >  tools/testing/cxl/test/mock.c |   19 ++++++++++++
> >  tools/testing/cxl/test/mock.h |    3 ++
> >  8 files changed, 203 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> 
> > @@ -274,21 +301,29 @@ static int add_host_bridge_dport(struct device *match, void *arg)
> >  	dev_dbg(match, "UID found: %lld\n", uid);
> >  
> >  	ctx = (struct cxl_chbs_context) {
> > -		.dev = host,
> > +		.dev = match,
> >  		.uid = uid,
> >  	};
> >  	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx);
> >  
> > -	if (ctx.chbcr == 0) {
> > +	if (ctx.rcrb != CXL_RESOURCE_NONE)
> > +		dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.rcrb);
> > +
> > +	if (ctx.chbcr == CXL_RESOURCE_NONE) {
> >  		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
> >  		return 0;
> >  	}
> 
> The logic must be changed to handle the case where the chbs entry is
> missing:
> 
> 	if (!ctx.chbcr) {
> 		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
> 		return 0;
> 	}

Noted, and folded into the patch.

Re: [PATCH v6 08/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Dan Williams]

Dan Williams wrote:
> Robert Richter wrote:
> > On 01.12.22 13:34:05, Dan Williams wrote:
> > > From: Robert Richter <rrichter@amd.com>
> > > 
> > > A downstream port must be connected to a component register block.
> > > For restricted hosts the base address is determined from the RCRB. The
> > > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > > get the RCRB and add code to extract the component register block from
> > > it.
> > > 
> > > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > > component registers. MEMBAR extraction follows the PCI base spec here,
> > > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1). The
> > > RCRB base address is cached in the cxl_dport per-host bridge so that the
> > > upstream port component registers can be retrieved later by an RCD
> > > (RCIEP) associated with the host bridge.
> > > 
> > > Note: Right now the component register block is used for HDM decoder
> > > capability only which is optional for RCDs. If unsupported by the RCD,
> > > the HDM init will fail. It is future work to bypass it in this case.
> > > 
> > > Co-developed-by: Terry Bowman <terry.bowman@amd.com>
> > > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > Link: https://lore.kernel.org/r/Y4dsGZ24aJlxSfI1@rric.localdomain
> > > [djbw: introduce devm_cxl_add_rch_dport()]
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > 
> > Found an issue below. Patch looks good to me otherwise.
> > 
> > > ---
> > >  drivers/cxl/acpi.c            |   51 ++++++++++++++++++++++++++++-----
> > >  drivers/cxl/core/port.c       |   53 ++++++++++++++++++++++++++++++----
> > >  drivers/cxl/core/regs.c       |   64 +++++++++++++++++++++++++++++++++++++++++
> > >  drivers/cxl/cxl.h             |   16 ++++++++++
> > >  tools/testing/cxl/Kbuild      |    1 +
> > >  tools/testing/cxl/test/cxl.c  |   10 ++++++
> > >  tools/testing/cxl/test/mock.c |   19 ++++++++++++
> > >  tools/testing/cxl/test/mock.h |    3 ++
> > >  8 files changed, 203 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > 
> > > @@ -274,21 +301,29 @@ static int add_host_bridge_dport(struct device *match, void *arg)
> > >  	dev_dbg(match, "UID found: %lld\n", uid);
> > >  
> > >  	ctx = (struct cxl_chbs_context) {
> > > -		.dev = host,
> > > +		.dev = match,
> > >  		.uid = uid,
> > >  	};
> > >  	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx);
> > >  
> > > -	if (ctx.chbcr == 0) {
> > > +	if (ctx.rcrb != CXL_RESOURCE_NONE)
> > > +		dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.rcrb);
> > > +
> > > +	if (ctx.chbcr == CXL_RESOURCE_NONE) {
> > >  		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
> > >  		return 0;
> > >  	}
> > 
> > The logic must be changed to handle the case where the chbs entry is
> > missing:
> > 
> > 	if (!ctx.chbcr) {
> > 		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
> > 		return 0;
> > 	}
> 
> Noted, and folded into the patch.

Oh, this also means that the mock version needs to return non-zero.
Folded this change as well:

@@ -702,7 +702,7 @@ resource_size_t mock_cxl_rcrb_to_component(struct device *dev,
 {
        dev_dbg(dev, "rcrb: %pa which: %d\n", &rcrb, which);
 
-       return 0;
+       return (resource_size_t) which + 1;
 }
 
 static struct cxl_mock_ops cxl_mock_ops = {

The component registers are never used when talking to mock devices.

Re: [PATCH v6 08/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Robert Richter]

On 02.12.22 23:04:49, Dan Williams wrote:
> Robert Richter wrote:
> > On 01.12.22 13:34:05, Dan Williams wrote:
> > > From: Robert Richter <rrichter@amd.com>
> > > 
> > > A downstream port must be connected to a component register block.
> > > For restricted hosts the base address is determined from the RCRB. The
> > > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > > get the RCRB and add code to extract the component register block from
> > > it.
> > > 
> > > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > > component registers. MEMBAR extraction follows the PCI base spec here,
> > > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1). The
> > > RCRB base address is cached in the cxl_dport per-host bridge so that the
> > > upstream port component registers can be retrieved later by an RCD
> > > (RCIEP) associated with the host bridge.
> > > 
> > > Note: Right now the component register block is used for HDM decoder
> > > capability only which is optional for RCDs. If unsupported by the RCD,
> > > the HDM init will fail. It is future work to bypass it in this case.
> > > 
> > > Co-developed-by: Terry Bowman <terry.bowman@amd.com>
> > > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > Link: https://lore.kernel.org/r/Y4dsGZ24aJlxSfI1@rric.localdomain
> > > [djbw: introduce devm_cxl_add_rch_dport()]
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > 
> > Found an issue below. Patch looks good to me otherwise.
> > 
> > > ---
> > >  drivers/cxl/acpi.c            |   51 ++++++++++++++++++++++++++++-----
> > >  drivers/cxl/core/port.c       |   53 ++++++++++++++++++++++++++++++----
> > >  drivers/cxl/core/regs.c       |   64 +++++++++++++++++++++++++++++++++++++++++
> > >  drivers/cxl/cxl.h             |   16 ++++++++++
> > >  tools/testing/cxl/Kbuild      |    1 +
> > >  tools/testing/cxl/test/cxl.c  |   10 ++++++
> > >  tools/testing/cxl/test/mock.c |   19 ++++++++++++
> > >  tools/testing/cxl/test/mock.h |    3 ++
> > >  8 files changed, 203 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > 
> > > @@ -274,21 +301,29 @@ static int add_host_bridge_dport(struct device *match, void *arg)
> > >  	dev_dbg(match, "UID found: %lld\n", uid);
> > >  
> > >  	ctx = (struct cxl_chbs_context) {
> > > -		.dev = host,
> > > +		.dev = match,
> > >  		.uid = uid,
> > >  	};
> > >  	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx);
> > >  
> > > -	if (ctx.chbcr == 0) {
> > > +	if (ctx.rcrb != CXL_RESOURCE_NONE)
> > > +		dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.rcrb);
> > > +
> > > +	if (ctx.chbcr == CXL_RESOURCE_NONE) {
> > >  		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
> > >  		return 0;
> > >  	}
> > 
> > The logic must be changed to handle the case where the chbs entry is
> > missing:
> > 
> > 	if (!ctx.chbcr) {
> > 		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
> > 		return 0;
> > 	}
> 
> Noted, and folded into the patch.

In the (ctx.chbcr == CXL_RESOURCE_NONE) case there is a slighly
different error reason. The CHBS was found but the CHBCR was invalid
or something else failed to determine it. That's why a different
message should be reported, e.g.:

	dev_warn(match, "CHBCR invalid for Host Bridge (UID %lld)\n", uid);

(Note I originally used "missing", but "invalid" is more reasonable
as there is something but it's not correct.)

-Robert

Re: [PATCH v6 08/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Dan Williams]

Robert Richter wrote:
> On 02.12.22 23:04:49, Dan Williams wrote:
> > Robert Richter wrote:
> > > On 01.12.22 13:34:05, Dan Williams wrote:
> > > > From: Robert Richter <rrichter@amd.com>
> > > > 
> > > > A downstream port must be connected to a component register block.
> > > > For restricted hosts the base address is determined from the RCRB. The
> > > > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > > > get the RCRB and add code to extract the component register block from
> > > > it.
> > > > 
> > > > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > > > component registers. MEMBAR extraction follows the PCI base spec here,
> > > > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1). The
> > > > RCRB base address is cached in the cxl_dport per-host bridge so that the
> > > > upstream port component registers can be retrieved later by an RCD
> > > > (RCIEP) associated with the host bridge.
> > > > 
> > > > Note: Right now the component register block is used for HDM decoder
> > > > capability only which is optional for RCDs. If unsupported by the RCD,
> > > > the HDM init will fail. It is future work to bypass it in this case.
> > > > 
> > > > Co-developed-by: Terry Bowman <terry.bowman@amd.com>
> > > > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > > Link: https://lore.kernel.org/r/Y4dsGZ24aJlxSfI1@rric.localdomain
> > > > [djbw: introduce devm_cxl_add_rch_dport()]
> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > 
> > > Found an issue below. Patch looks good to me otherwise.
> > > 
> > > > ---
> > > >  drivers/cxl/acpi.c            |   51 ++++++++++++++++++++++++++++-----
> > > >  drivers/cxl/core/port.c       |   53 ++++++++++++++++++++++++++++++----
> > > >  drivers/cxl/core/regs.c       |   64 +++++++++++++++++++++++++++++++++++++++++
> > > >  drivers/cxl/cxl.h             |   16 ++++++++++
> > > >  tools/testing/cxl/Kbuild      |    1 +
> > > >  tools/testing/cxl/test/cxl.c  |   10 ++++++
> > > >  tools/testing/cxl/test/mock.c |   19 ++++++++++++
> > > >  tools/testing/cxl/test/mock.h |    3 ++
> > > >  8 files changed, 203 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > 
> > > > @@ -274,21 +301,29 @@ static int add_host_bridge_dport(struct device *match, void *arg)
> > > >  	dev_dbg(match, "UID found: %lld\n", uid);
> > > >  
> > > >  	ctx = (struct cxl_chbs_context) {
> > > > -		.dev = host,
> > > > +		.dev = match,
> > > >  		.uid = uid,
> > > >  	};
> > > >  	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx);
> > > >  
> > > > -	if (ctx.chbcr == 0) {
> > > > +	if (ctx.rcrb != CXL_RESOURCE_NONE)
> > > > +		dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.rcrb);
> > > > +
> > > > +	if (ctx.chbcr == CXL_RESOURCE_NONE) {
> > > >  		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
> > > >  		return 0;
> > > >  	}
> > > 
> > > The logic must be changed to handle the case where the chbs entry is
> > > missing:
> > > 
> > > 	if (!ctx.chbcr) {
> > > 		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
> > > 		return 0;
> > > 	}
> > 
> > Noted, and folded into the patch.
> 
> In the (ctx.chbcr == CXL_RESOURCE_NONE) case there is a slighly
> different error reason. The CHBS was found but the CHBCR was invalid
> or something else failed to determine it. That's why a different
> message should be reported, e.g.:
> 
> 	dev_warn(match, "CHBCR invalid for Host Bridge (UID %lld)\n", uid);
> 
> (Note I originally used "missing", but "invalid" is more reasonable
> as there is something but it's not correct.)
> 

Care to send a patch on top of cxl/next to fix this up?

Re: [PATCH v6 08/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Jonathan Cameron]

On Thu, 01 Dec 2022 13:34:05 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> From: Robert Richter <rrichter@amd.com>
> 
> A downstream port must be connected to a component register block.
> For restricted hosts the base address is determined from the RCRB. The
> RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> get the RCRB and add code to extract the component register block from
> it.
> 
> RCRB's BAR[0..1] point to the component block containing CXL subsystem
> component registers. MEMBAR extraction follows the PCI base spec here,
> esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1). The
> RCRB base address is cached in the cxl_dport per-host bridge so that the
> upstream port component registers can be retrieved later by an RCD
> (RCIEP) associated with the host bridge.
> 
> Note: Right now the component register block is used for HDM decoder
> capability only which is optional for RCDs. If unsupported by the RCD,
> the HDM init will fail. It is future work to bypass it in this case.
> 
> Co-developed-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Link: https://lore.kernel.org/r/Y4dsGZ24aJlxSfI1@rric.localdomain
> [djbw: introduce devm_cxl_add_rch_dport()]
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Trivial moans that may have something to do with it being near going home time
on a Friday.

Otherwise looks sensible though this was a fairly superficial look.

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



> ---
>  drivers/cxl/acpi.c            |   51 ++++++++++++++++++++++++++++-----
>  drivers/cxl/core/port.c       |   53 ++++++++++++++++++++++++++++++----
>  drivers/cxl/core/regs.c       |   64 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h             |   16 ++++++++++
>  tools/testing/cxl/Kbuild      |    1 +
>  tools/testing/cxl/test/cxl.c  |   10 ++++++
>  tools/testing/cxl/test/mock.c |   19 ++++++++++++
>  tools/testing/cxl/test/mock.h |    3 ++
>  8 files changed, 203 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 50d82376097c..db8173f3ee10 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c

>  struct cxl_chbs_context {
> -	struct device *dev;
> -	unsigned long long uid;
> -	resource_size_t chbcr;
> +	struct device		*dev;
> +	unsigned long long	uid;
> +	resource_size_t		rcrb;
> +	resource_size_t		chbcr;
> +	u32			cxl_version;
>  };

I'm not keen on this style change because it slightly obscures the meaningful
changes in this diff + I suspect it's not consistent with rest of the file.



> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index ec178e69b18f..28ed0ec8ee3e 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -307,3 +307,67 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
>  	return -ENODEV;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
> +
> +resource_size_t cxl_rcrb_to_component(struct device *dev,
> +				      resource_size_t rcrb,
> +				      enum cxl_rcrb which)
> +{
> +	resource_size_t component_reg_phys;
> +	u32 bar0, bar1;
> +	void *addr;
> +	u16 cmd;
> +	u32 id;
> +
> +	if (which == CXL_RCRB_UPSTREAM)
> +		rcrb += SZ_4K;
> +
> +	/*
> +	 * RCRB's BAR[0..1] point to component block containing CXL
> +	 * subsystem component registers. MEMBAR extraction follows
> +	 * the PCI Base spec here, esp. 64 bit extraction and memory
> +	 * ranges alignment (6.0, 7.5.1.2.1).
> +	 */
> +	if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
> +		return CXL_RESOURCE_NONE;
> +	addr = ioremap(rcrb, SZ_4K);
> +	if (!addr) {
> +		dev_err(dev, "Failed to map region %pr\n", addr);
> +		release_mem_region(rcrb, SZ_4K);
> +		return CXL_RESOURCE_NONE;
> +	}
> +
> +	id = readl(addr + PCI_VENDOR_ID);
> +	cmd = readw(addr + PCI_COMMAND);
> +	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> +	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +	iounmap(addr);
> +	release_mem_region(rcrb, SZ_4K);
> +
> +	/*
> +	 * Sanity check, see CXL 3.0 Figure 9-8 CXL Device that Does Not
> +	 * Remap Upstream Port and Component Registers
> +	 */
> +	if (id == U32_MAX) {
> +		if (which == CXL_RCRB_DOWNSTREAM)
> +			dev_err(dev, "Failed to access Downstream Port RCRB\n");
> +		return CXL_RESOURCE_NONE;
> +	}
> +	if (!(cmd & PCI_COMMAND_MEMORY))
> +		return CXL_RESOURCE_NONE;
> +	if (bar0 & (PCI_BASE_ADDRESS_MEM_TYPE_1M | PCI_BASE_ADDRESS_SPACE_IO))

Trivial: A positive match on what we do want might be better...

I had to got look up MEM_TYPE_1M to find out what on earth it was (marked obsolete which
I guess isn't surprising.... )

Up to you though...

> +		return CXL_RESOURCE_NONE;
> +
> +	component_reg_phys = bar0 & PCI_BASE_ADDRESS_MEM_MASK;
> +	if (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_64)
> +		component_reg_phys |= ((u64)bar1) << 32;
> +
> +	if (!component_reg_phys)
> +		return CXL_RESOURCE_NONE;
> +
> +	/* MEMBAR is block size (64k) aligned. */
> +	if (!IS_ALIGNED(component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE))
> +		return CXL_RESOURCE_NONE;
> +
> +	return component_reg_phys;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 281b1db5a271..1342e4e61537 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h



>  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
>  #define CXL_TARGET_STRLEN 20
>  
> @@ -486,12 +494,16 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
>   * @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
> + * @rcrb: base address for the Root Complex Register Block
> + * @rch: Indicate whether this dport was enumerated in RCH or VH mode

Clarify this as
	Indicate this dport was enumerated in RCH rather than VH mode.

a boolean with an or in the comment is confusing!

>   * @port: reference to cxl_port that contains this downstream port
>   */
>  struct cxl_dport {
>  	struct device *dport;
>  	int port_id;
>  	resource_size_t component_reg_phys;
> +	resource_size_t rcrb;
> +	bool rch;
>  	struct cxl_port *port;
>  };

Re: [PATCH v6 08/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Dan Williams]

Jonathan Cameron wrote:
> On Thu, 01 Dec 2022 13:34:05 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > From: Robert Richter <rrichter@amd.com>
> > 
> > A downstream port must be connected to a component register block.
> > For restricted hosts the base address is determined from the RCRB. The
> > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > get the RCRB and add code to extract the component register block from
> > it.
> > 
> > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > component registers. MEMBAR extraction follows the PCI base spec here,
> > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1). The
> > RCRB base address is cached in the cxl_dport per-host bridge so that the
> > upstream port component registers can be retrieved later by an RCD
> > (RCIEP) associated with the host bridge.
> > 
> > Note: Right now the component register block is used for HDM decoder
> > capability only which is optional for RCDs. If unsupported by the RCD,
> > the HDM init will fail. It is future work to bypass it in this case.
> > 
> > Co-developed-by: Terry Bowman <terry.bowman@amd.com>
> > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > Link: https://lore.kernel.org/r/Y4dsGZ24aJlxSfI1@rric.localdomain
> > [djbw: introduce devm_cxl_add_rch_dport()]
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> Trivial moans that may have something to do with it being near going home time
> on a Friday.
> 
> Otherwise looks sensible though this was a fairly superficial look.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 
> 
> > ---
> >  drivers/cxl/acpi.c            |   51 ++++++++++++++++++++++++++++-----
> >  drivers/cxl/core/port.c       |   53 ++++++++++++++++++++++++++++++----
> >  drivers/cxl/core/regs.c       |   64 +++++++++++++++++++++++++++++++++++++++++
> >  drivers/cxl/cxl.h             |   16 ++++++++++
> >  tools/testing/cxl/Kbuild      |    1 +
> >  tools/testing/cxl/test/cxl.c  |   10 ++++++
> >  tools/testing/cxl/test/mock.c |   19 ++++++++++++
> >  tools/testing/cxl/test/mock.h |    3 ++
> >  8 files changed, 203 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 50d82376097c..db8173f3ee10 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> 
> >  struct cxl_chbs_context {
> > -	struct device *dev;
> > -	unsigned long long uid;
> > -	resource_size_t chbcr;
> > +	struct device		*dev;
> > +	unsigned long long	uid;
> > +	resource_size_t		rcrb;
> > +	resource_size_t		chbcr;
> > +	u32			cxl_version;
> >  };
> 
> I'm not keen on this style change because it slightly obscures the meaningful
> changes in this diff + I suspect it's not consistent with rest of the file.

Copy and pasted from Robert's update. Looks much better after hitting it
with clang-format:

@@ -232,7 +239,9 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 struct cxl_chbs_context {
        struct device *dev;
        unsigned long long uid;
+       resource_size_t rcrb;
        resource_size_t chbcr;
+       u32 cxl_version;
 };
 
 static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,

> 
> 
> 
> > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> > index ec178e69b18f..28ed0ec8ee3e 100644
> > --- a/drivers/cxl/core/regs.c
> > +++ b/drivers/cxl/core/regs.c
> > @@ -307,3 +307,67 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> >  	return -ENODEV;
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
> > +
> > +resource_size_t cxl_rcrb_to_component(struct device *dev,
> > +				      resource_size_t rcrb,
> > +				      enum cxl_rcrb which)
> > +{
> > +	resource_size_t component_reg_phys;
> > +	u32 bar0, bar1;
> > +	void *addr;
> > +	u16 cmd;
> > +	u32 id;
> > +
> > +	if (which == CXL_RCRB_UPSTREAM)
> > +		rcrb += SZ_4K;
> > +
> > +	/*
> > +	 * RCRB's BAR[0..1] point to component block containing CXL
> > +	 * subsystem component registers. MEMBAR extraction follows
> > +	 * the PCI Base spec here, esp. 64 bit extraction and memory
> > +	 * ranges alignment (6.0, 7.5.1.2.1).
> > +	 */
> > +	if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
> > +		return CXL_RESOURCE_NONE;
> > +	addr = ioremap(rcrb, SZ_4K);
> > +	if (!addr) {
> > +		dev_err(dev, "Failed to map region %pr\n", addr);
> > +		release_mem_region(rcrb, SZ_4K);
> > +		return CXL_RESOURCE_NONE;
> > +	}
> > +
> > +	id = readl(addr + PCI_VENDOR_ID);
> > +	cmd = readw(addr + PCI_COMMAND);
> > +	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> > +	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> > +	iounmap(addr);
> > +	release_mem_region(rcrb, SZ_4K);
> > +
> > +	/*
> > +	 * Sanity check, see CXL 3.0 Figure 9-8 CXL Device that Does Not
> > +	 * Remap Upstream Port and Component Registers
> > +	 */
> > +	if (id == U32_MAX) {
> > +		if (which == CXL_RCRB_DOWNSTREAM)
> > +			dev_err(dev, "Failed to access Downstream Port RCRB\n");
> > +		return CXL_RESOURCE_NONE;
> > +	}
> > +	if (!(cmd & PCI_COMMAND_MEMORY))
> > +		return CXL_RESOURCE_NONE;
> > +	if (bar0 & (PCI_BASE_ADDRESS_MEM_TYPE_1M | PCI_BASE_ADDRESS_SPACE_IO))
> 
> Trivial: A positive match on what we do want might be better...
> 
> I had to got look up MEM_TYPE_1M to find out what on earth it was (marked obsolete which
> I guess isn't surprising.... )
> 
> Up to you though...

The polarity switch is not any prettier, but a comment would save
someone searching what PCI_BASE_ADDRESS_MEM_TYPE_1M is though. I
actually looked that up myself when I first read it.

> 
> > +		return CXL_RESOURCE_NONE;
> > +
> > +	component_reg_phys = bar0 & PCI_BASE_ADDRESS_MEM_MASK;
> > +	if (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_64)
> > +		component_reg_phys |= ((u64)bar1) << 32;
> > +
> > +	if (!component_reg_phys)
> > +		return CXL_RESOURCE_NONE;
> > +
> > +	/* MEMBAR is block size (64k) aligned. */
> > +	if (!IS_ALIGNED(component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE))
> > +		return CXL_RESOURCE_NONE;
> > +
> > +	return component_reg_phys;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 281b1db5a271..1342e4e61537 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> 
> 
> 
> >  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
> >  #define CXL_TARGET_STRLEN 20
> >  
> > @@ -486,12 +494,16 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
> >   * @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
> > + * @rcrb: base address for the Root Complex Register Block
> > + * @rch: Indicate whether this dport was enumerated in RCH or VH mode
> 
> Clarify this as
> 	Indicate this dport was enumerated in RCH rather than VH mode.
> 
> a boolean with an or in the comment is confusing!
> 
> >   * @port: reference to cxl_port that contains this downstream port
> >   */
> >  struct cxl_dport {
> >  	struct device *dport;
> >  	int port_id;
> >  	resource_size_t component_reg_phys;
> > +	resource_size_t rcrb;
> > +	bool rch;
> >  	struct cxl_port *port;
> >  };
> 

[PATCH v6 09/12] cxl/mem: Move devm_cxl_add_endpoint() from cxl_core to cxl_mem

[Dan Williams]

tl;dr: Clean up an unnecessary export and enable cxl_test.

An RCD (Restricted CXL Device), in contrast to a typical CXL device in
a VH topology, obtains its component registers from the bottom half of
the associated CXL host bridge RCRB (Root Complex Register Block). In
turn this means that cxl_rcrb_to_component() needs to be called from
devm_cxl_add_endpoint().

Presently devm_cxl_add_endpoint() is part of the CXL core, but the only
user is the CXL mem module. Move it from cxl_core to cxl_mem to not only
get rid of an unnecessary export, but to also enable its call out to
cxl_rcrb_to_component(), in a subsequent patch, to be mocked by
cxl_test. Recall that cxl_test can only mock exported symbols, and since
cxl_rcrb_to_component() is itself inside the core, all callers must be
outside of cxl_core to allow cxl_test to mock it.

Reviewed-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/core.h |    8 --------
 drivers/cxl/core/port.c |   39 ---------------------------------------
 drivers/cxl/cxl.h       |    2 --
 drivers/cxl/cxlmem.h    |    9 +++++++++
 drivers/cxl/mem.c       |   38 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 47 insertions(+), 49 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 1d8f87be283f..8c04672dca56 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -58,14 +58,6 @@ extern struct rw_semaphore cxl_dpa_rwsem;
 
 bool is_switch_decoder(struct device *dev);
 struct cxl_switch_decoder *to_cxl_switch_decoder(struct device *dev);
-static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
-					 struct cxl_memdev *cxlmd)
-{
-	if (!port)
-		return NULL;
-
-	return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev);
-}
 
 int cxl_memdev_init(void);
 void cxl_memdev_exit(void);
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index dae2ca31885e..4982b6902ef5 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1212,45 +1212,6 @@ static void reap_dports(struct cxl_port *port)
 	}
 }
 
-int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd,
-			  struct cxl_dport *parent_dport)
-{
-	struct cxl_port *parent_port = parent_dport->port;
-	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	struct cxl_port *endpoint, *iter, *down;
-	int rc;
-
-	/*
-	 * Now that the path to the root is established record all the
-	 * intervening ports in the chain.
-	 */
-	for (iter = parent_port, down = NULL; !is_cxl_root(iter);
-	     down = iter, iter = to_cxl_port(iter->dev.parent)) {
-		struct cxl_ep *ep;
-
-		ep = cxl_ep_load(iter, cxlmd);
-		ep->next = down;
-	}
-
-	endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev,
-				     cxlds->component_reg_phys, parent_dport);
-	if (IS_ERR(endpoint))
-		return PTR_ERR(endpoint);
-
-	rc = cxl_endpoint_autoremove(cxlmd, endpoint);
-	if (rc)
-		return rc;
-
-	if (!endpoint->dev.driver) {
-		dev_err(&cxlmd->dev, "%s failed probe\n",
-			dev_name(&endpoint->dev));
-		return -ENXIO;
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL_NS_GPL(devm_cxl_add_endpoint, CXL);
-
 static void cxl_detach_ep(void *data)
 {
 	struct cxl_memdev *cxlmd = data;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 1342e4e61537..9a212ab3cae4 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -560,8 +560,6 @@ struct pci_bus *cxl_port_to_pci_bus(struct cxl_port *port);
 struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
 				   resource_size_t component_reg_phys,
 				   struct cxl_dport *parent_dport);
-int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd,
-			  struct cxl_dport *parent_dport);
 struct cxl_port *find_cxl_root(struct device *dev);
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
 void cxl_bus_rescan(void);
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index c1c9960ab05f..e082991bc58c 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -80,6 +80,15 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
 
 struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds);
 
+static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
+					 struct cxl_memdev *cxlmd)
+{
+	if (!port)
+		return NULL;
+
+	return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev);
+}
+
 /**
  * struct cxl_mbox_cmd - A command to be submitted to hardware.
  * @opcode: (input) The command set and command submitted to hardware.
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 549b6b499bae..aa63ce8c7ca6 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -45,6 +45,44 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
 	return 0;
 }
 
+static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd,
+				 struct cxl_dport *parent_dport)
+{
+	struct cxl_port *parent_port = parent_dport->port;
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct cxl_port *endpoint, *iter, *down;
+	int rc;
+
+	/*
+	 * Now that the path to the root is established record all the
+	 * intervening ports in the chain.
+	 */
+	for (iter = parent_port, down = NULL; !is_cxl_root(iter);
+	     down = iter, iter = to_cxl_port(iter->dev.parent)) {
+		struct cxl_ep *ep;
+
+		ep = cxl_ep_load(iter, cxlmd);
+		ep->next = down;
+	}
+
+	endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev,
+				     cxlds->component_reg_phys, parent_dport);
+	if (IS_ERR(endpoint))
+		return PTR_ERR(endpoint);
+
+	rc = cxl_endpoint_autoremove(cxlmd, endpoint);
+	if (rc)
+		return rc;
+
+	if (!endpoint->dev.driver) {
+		dev_err(&cxlmd->dev, "%s failed probe\n",
+			dev_name(&endpoint->dev));
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
 static int cxl_mem_probe(struct device *dev)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);

Re: [PATCH v6 09/12] cxl/mem: Move devm_cxl_add_endpoint() from cxl_core to cxl_mem

[Jonathan Cameron]

On Thu, 01 Dec 2022 13:34:10 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> tl;dr: Clean up an unnecessary export and enable cxl_test.
> 
> An RCD (Restricted CXL Device), in contrast to a typical CXL device in
> a VH topology, obtains its component registers from the bottom half of
> the associated CXL host bridge RCRB (Root Complex Register Block). In
> turn this means that cxl_rcrb_to_component() needs to be called from
> devm_cxl_add_endpoint().
> 
> Presently devm_cxl_add_endpoint() is part of the CXL core, but the only
> user is the CXL mem module. Move it from cxl_core to cxl_mem to not only
> get rid of an unnecessary export, but to also enable its call out to
> cxl_rcrb_to_component(), in a subsequent patch, to be mocked by
> cxl_test. Recall that cxl_test can only mock exported symbols, and since
> cxl_rcrb_to_component() is itself inside the core, all callers must be
> outside of cxl_core to allow cxl_test to mock it.
> 
> Reviewed-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reasoning for the move is sound and patch is clearly just a move so
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/core.h |    8 --------
>  drivers/cxl/core/port.c |   39 ---------------------------------------
>  drivers/cxl/cxl.h       |    2 --
>  drivers/cxl/cxlmem.h    |    9 +++++++++
>  drivers/cxl/mem.c       |   38 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 47 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 1d8f87be283f..8c04672dca56 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -58,14 +58,6 @@ extern struct rw_semaphore cxl_dpa_rwsem;
>  
>  bool is_switch_decoder(struct device *dev);
>  struct cxl_switch_decoder *to_cxl_switch_decoder(struct device *dev);
> -static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
> -					 struct cxl_memdev *cxlmd)
> -{
> -	if (!port)
> -		return NULL;
> -
> -	return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev);
> -}
>  
>  int cxl_memdev_init(void);
>  void cxl_memdev_exit(void);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index dae2ca31885e..4982b6902ef5 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1212,45 +1212,6 @@ static void reap_dports(struct cxl_port *port)
>  	}
>  }
>  
> -int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd,
> -			  struct cxl_dport *parent_dport)
> -{
> -	struct cxl_port *parent_port = parent_dport->port;
> -	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> -	struct cxl_port *endpoint, *iter, *down;
> -	int rc;
> -
> -	/*
> -	 * Now that the path to the root is established record all the
> -	 * intervening ports in the chain.
> -	 */
> -	for (iter = parent_port, down = NULL; !is_cxl_root(iter);
> -	     down = iter, iter = to_cxl_port(iter->dev.parent)) {
> -		struct cxl_ep *ep;
> -
> -		ep = cxl_ep_load(iter, cxlmd);
> -		ep->next = down;
> -	}
> -
> -	endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev,
> -				     cxlds->component_reg_phys, parent_dport);
> -	if (IS_ERR(endpoint))
> -		return PTR_ERR(endpoint);
> -
> -	rc = cxl_endpoint_autoremove(cxlmd, endpoint);
> -	if (rc)
> -		return rc;
> -
> -	if (!endpoint->dev.driver) {
> -		dev_err(&cxlmd->dev, "%s failed probe\n",
> -			dev_name(&endpoint->dev));
> -		return -ENXIO;
> -	}
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_NS_GPL(devm_cxl_add_endpoint, CXL);
> -
>  static void cxl_detach_ep(void *data)
>  {
>  	struct cxl_memdev *cxlmd = data;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 1342e4e61537..9a212ab3cae4 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -560,8 +560,6 @@ struct pci_bus *cxl_port_to_pci_bus(struct cxl_port *port);
>  struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
>  				   resource_size_t component_reg_phys,
>  				   struct cxl_dport *parent_dport);
> -int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd,
> -			  struct cxl_dport *parent_dport);
>  struct cxl_port *find_cxl_root(struct device *dev);
>  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
>  void cxl_bus_rescan(void);
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index c1c9960ab05f..e082991bc58c 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -80,6 +80,15 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
>  
>  struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds);
>  
> +static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
> +					 struct cxl_memdev *cxlmd)
> +{
> +	if (!port)
> +		return NULL;
> +
> +	return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev);
> +}
> +
>  /**
>   * struct cxl_mbox_cmd - A command to be submitted to hardware.
>   * @opcode: (input) The command set and command submitted to hardware.
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 549b6b499bae..aa63ce8c7ca6 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -45,6 +45,44 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
>  	return 0;
>  }
>  
> +static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd,
> +				 struct cxl_dport *parent_dport)
> +{
> +	struct cxl_port *parent_port = parent_dport->port;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_port *endpoint, *iter, *down;
> +	int rc;
> +
> +	/*
> +	 * Now that the path to the root is established record all the
> +	 * intervening ports in the chain.
> +	 */
> +	for (iter = parent_port, down = NULL; !is_cxl_root(iter);
> +	     down = iter, iter = to_cxl_port(iter->dev.parent)) {
> +		struct cxl_ep *ep;
> +
> +		ep = cxl_ep_load(iter, cxlmd);
> +		ep->next = down;
> +	}
> +
> +	endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev,
> +				     cxlds->component_reg_phys, parent_dport);
> +	if (IS_ERR(endpoint))
> +		return PTR_ERR(endpoint);
> +
> +	rc = cxl_endpoint_autoremove(cxlmd, endpoint);
> +	if (rc)
> +		return rc;
> +
> +	if (!endpoint->dev.driver) {
> +		dev_err(&cxlmd->dev, "%s failed probe\n",
> +			dev_name(&endpoint->dev));
> +		return -ENXIO;
> +	}
> +
> +	return 0;
> +}
> +
>  static int cxl_mem_probe(struct device *dev)
>  {
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> 

[PATCH v6 10/12] cxl/port: Add RCD endpoint port enumeration

[Dan Williams]

Unlike a CXL memory expander in a VH topology that has at least one
intervening 'struct cxl_port' instance between itself and the CXL root
device, an RCD attaches one-level higher. For example:

               VH
          ┌──────────┐
          │ ACPI0017 │
          │  root0   │
          └─────┬────┘
                │
          ┌─────┴────┐
          │  dport0  │
    ┌─────┤ ACPI0016 ├─────┐
    │     │  port1   │     │
    │     └────┬─────┘     │
    │          │           │
 ┌──┴───┐   ┌──┴───┐   ┌───┴──┐
 │dport0│   │dport1│   │dport2│
 │ RP0  │   │ RP1  │   │ RP2  │
 └──────┘   └──┬───┘   └──────┘
               │
           ┌───┴─────┐
           │endpoint0│
           │  port2  │
           └─────────┘

...vs:

              RCH
          ┌──────────┐
          │ ACPI0017 │
          │  root0   │
          └────┬─────┘
               │
           ┌───┴────┐
           │ dport0 │
           │ACPI0016│
           └───┬────┘
               │
          ┌────┴─────┐
          │endpoint0 │
          │  port1   │
          └──────────┘

So arrange for endpoint port in the RCH/RCD case to appear directly
connected to the host-bridge in its singular role as a dport. Compare
that to the VH case where the host-bridge serves a dual role as a
'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for
the Root Ports in the Root Complex that are modeled as 'cxl_dport'
instances in the CXL topology.

Another deviation from the VH case is that RCDs may need to look up
their component registers from the Root Complex Register Block (RCRB).
That platform firmware specified RCRB area is cached by the cxl_acpi
driver and conveyed via the host-bridge dport to the cxl_mem driver to
perform the cxl_rcrb_to_component() lookup for the endpoint port
(See 9.11.8 CXL Devices Attached to an RCH for the lookup of the
upstream port component registers).

Tested-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/port.c |    7 +++++++
 drivers/cxl/cxlmem.h    |    2 ++
 drivers/cxl/mem.c       |   31 ++++++++++++++++++++++++-------
 drivers/cxl/pci.c       |   10 ++++++++++
 4 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 4982b6902ef5..50bdbd9f8da3 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1369,6 +1369,13 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 	struct device *iter;
 	int rc;
 
+	/*
+	 * Skip intermediate port enumeration in the RCH case, there
+	 * are no ports in between a host bridge and an endpoint.
+	 */
+	if (cxlmd->cxlds->rcd)
+		return 0;
+
 	rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd);
 	if (rc)
 		return rc;
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index e082991bc58c..35d485d041f0 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -201,6 +201,7 @@ struct cxl_endpoint_dvsec_info {
  * @dev: The device associated with this CXL state
  * @regs: Parsed register blocks
  * @cxl_dvsec: Offset to the PCIe device DVSEC
+ * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH)
  * @payload_size: Size of space for payload
  *                (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register)
  * @lsa_size: Size of Label Storage Area
@@ -235,6 +236,7 @@ struct cxl_dev_state {
 	struct cxl_regs regs;
 	int cxl_dvsec;
 
+	bool rcd;
 	size_t payload_size;
 	size_t lsa_size;
 	struct mutex mbox_mutex; /* Protects device mailbox and firmware */
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index aa63ce8c7ca6..4b94e63f78ec 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -45,12 +45,13 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
 	return 0;
 }
 
-static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd,
+static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
 				 struct cxl_dport *parent_dport)
 {
 	struct cxl_port *parent_port = parent_dport->port;
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_port *endpoint, *iter, *down;
+	resource_size_t component_reg_phys;
 	int rc;
 
 	/*
@@ -65,8 +66,18 @@ static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd,
 		ep->next = down;
 	}
 
-	endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev,
-				     cxlds->component_reg_phys, parent_dport);
+	/*
+	 * The component registers for an RCD might come from the
+	 * host-bridge RCRB if they are not already mapped via the
+	 * typical register locator mechanism.
+	 */
+	if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE)
+		component_reg_phys = cxl_rcrb_to_component(
+			&cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
+	else
+		component_reg_phys = cxlds->component_reg_phys;
+	endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys,
+				     parent_dport);
 	if (IS_ERR(endpoint))
 		return PTR_ERR(endpoint);
 
@@ -87,6 +98,7 @@ static int cxl_mem_probe(struct device *dev)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct device *endpoint_parent;
 	struct cxl_port *parent_port;
 	struct cxl_dport *dport;
 	struct dentry *dentry;
@@ -119,17 +131,22 @@ static int cxl_mem_probe(struct device *dev)
 		return -ENXIO;
 	}
 
-	device_lock(&parent_port->dev);
-	if (!parent_port->dev.driver) {
+	if (dport->rch)
+		endpoint_parent = parent_port->uport;
+	else
+		endpoint_parent = &parent_port->dev;
+
+	device_lock(endpoint_parent);
+	if (!endpoint_parent->driver) {
 		dev_err(dev, "CXL port topology %s not enabled\n",
 			dev_name(&parent_port->dev));
 		rc = -ENXIO;
 		goto unlock;
 	}
 
-	rc = devm_cxl_add_endpoint(cxlmd, dport);
+	rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
 unlock:
-	device_unlock(&parent_port->dev);
+	device_unlock(endpoint_parent);
 	put_device(&parent_port->dev);
 	if (rc)
 		return rc;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index e15da405b948..73ff6c33a0c0 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -433,6 +433,15 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
 	}
 }
 
+/*
+ * Assume that any RCIEP that emits the CXL memory expander class code
+ * is an RCD
+ */
+static bool is_cxl_restricted(struct pci_dev *pdev)
+{
+	return pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END;
+}
+
 static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct cxl_register_map map;
@@ -455,6 +464,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (IS_ERR(cxlds))
 		return PTR_ERR(cxlds);
 
+	cxlds->rcd = is_cxl_restricted(pdev);
 	cxlds->serial = pci_get_dsn(pdev);
 	cxlds->cxl_dvsec = pci_find_dvsec_capability(
 		pdev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);

Re: [PATCH v6 10/12] cxl/port: Add RCD endpoint port enumeration

[Robert Richter]

On 01.12.22 13:34:16, Dan Williams wrote:
> Unlike a CXL memory expander in a VH topology that has at least one
> intervening 'struct cxl_port' instance between itself and the CXL root
> device, an RCD attaches one-level higher. For example:
> 
>                VH
>           ┌──────────┐
>           │ ACPI0017 │
>           │  root0   │
>           └─────┬────┘
>                 │
>           ┌─────┴────┐
>           │  dport0  │
>     ┌─────┤ ACPI0016 ├─────┐
>     │     │  port1   │     │
>     │     └────┬─────┘     │
>     │          │           │
>  ┌──┴───┐   ┌──┴───┐   ┌───┴──┐
>  │dport0│   │dport1│   │dport2│
>  │ RP0  │   │ RP1  │   │ RP2  │
>  └──────┘   └──┬───┘   └──────┘
>                │
>            ┌───┴─────┐
>            │endpoint0│
>            │  port2  │
>            └─────────┘
> 
> ...vs:
> 
>               RCH
>           ┌──────────┐
>           │ ACPI0017 │
>           │  root0   │
>           └────┬─────┘
>                │
>            ┌───┴────┐
>            │ dport0 │
>            │ACPI0016│
>            └───┬────┘
>                │
>           ┌────┴─────┐
>           │endpoint0 │
>           │  port1   │
>           └──────────┘
> 
> So arrange for endpoint port in the RCH/RCD case to appear directly
> connected to the host-bridge in its singular role as a dport. Compare
> that to the VH case where the host-bridge serves a dual role as a
> 'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for
> the Root Ports in the Root Complex that are modeled as 'cxl_dport'
> instances in the CXL topology.
> 
> Another deviation from the VH case is that RCDs may need to look up
> their component registers from the Root Complex Register Block (RCRB).
> That platform firmware specified RCRB area is cached by the cxl_acpi
> driver and conveyed via the host-bridge dport to the cxl_mem driver to
> perform the cxl_rcrb_to_component() lookup for the endpoint port
> (See 9.11.8 CXL Devices Attached to an RCH for the lookup of the
> upstream port component registers).
> 
> Tested-by: Robert Richter <rrichter@amd.com>

With the one comment below addressed you can also add my:

Reviewed-by: Robert Richter <rrichter@amd.com>

> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/port.c |    7 +++++++
>  drivers/cxl/cxlmem.h    |    2 ++
>  drivers/cxl/mem.c       |   31 ++++++++++++++++++++++++-------
>  drivers/cxl/pci.c       |   10 ++++++++++
>  4 files changed, 43 insertions(+), 7 deletions(-)

> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c

> @@ -119,17 +131,22 @@ static int cxl_mem_probe(struct device *dev)
>  		return -ENXIO;
>  	}
>  
> -	device_lock(&parent_port->dev);
> -	if (!parent_port->dev.driver) {
> +	if (dport->rch)
> +		endpoint_parent = parent_port->uport;
> +	else
> +		endpoint_parent = &parent_port->dev;
> +
> +	device_lock(endpoint_parent);
> +	if (!endpoint_parent->driver) {
>  		dev_err(dev, "CXL port topology %s not enabled\n",
>  			dev_name(&parent_port->dev));

Already reported: dev_name(endpoint_parent)

>  		rc = -ENXIO;
>  		goto unlock;
>  	}
>  
> -	rc = devm_cxl_add_endpoint(cxlmd, dport);
> +	rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
>  unlock:
> -	device_unlock(&parent_port->dev);
> +	device_unlock(endpoint_parent);
>  	put_device(&parent_port->dev);
>  	if (rc)
>  		return rc;

Re: [PATCH v6 10/12] cxl/port: Add RCD endpoint port enumeration

[Dan Williams]

Robert Richter wrote:
> On 01.12.22 13:34:16, Dan Williams wrote:
> > Unlike a CXL memory expander in a VH topology that has at least one
> > intervening 'struct cxl_port' instance between itself and the CXL root
> > device, an RCD attaches one-level higher. For example:
> > 
> >                VH
> >           ┌──────────┐
> >           │ ACPI0017 │
> >           │  root0   │
> >           └─────┬────┘
> >                 │
> >           ┌─────┴────┐
> >           │  dport0  │
> >     ┌─────┤ ACPI0016 ├─────┐
> >     │     │  port1   │     │
> >     │     └────┬─────┘     │
> >     │          │           │
> >  ┌──┴───┐   ┌──┴───┐   ┌───┴──┐
> >  │dport0│   │dport1│   │dport2│
> >  │ RP0  │   │ RP1  │   │ RP2  │
> >  └──────┘   └──┬───┘   └──────┘
> >                │
> >            ┌───┴─────┐
> >            │endpoint0│
> >            │  port2  │
> >            └─────────┘
> > 
> > ...vs:
> > 
> >               RCH
> >           ┌──────────┐
> >           │ ACPI0017 │
> >           │  root0   │
> >           └────┬─────┘
> >                │
> >            ┌───┴────┐
> >            │ dport0 │
> >            │ACPI0016│
> >            └───┬────┘
> >                │
> >           ┌────┴─────┐
> >           │endpoint0 │
> >           │  port1   │
> >           └──────────┘
> > 
> > So arrange for endpoint port in the RCH/RCD case to appear directly
> > connected to the host-bridge in its singular role as a dport. Compare
> > that to the VH case where the host-bridge serves a dual role as a
> > 'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for
> > the Root Ports in the Root Complex that are modeled as 'cxl_dport'
> > instances in the CXL topology.
> > 
> > Another deviation from the VH case is that RCDs may need to look up
> > their component registers from the Root Complex Register Block (RCRB).
> > That platform firmware specified RCRB area is cached by the cxl_acpi
> > driver and conveyed via the host-bridge dport to the cxl_mem driver to
> > perform the cxl_rcrb_to_component() lookup for the endpoint port
> > (See 9.11.8 CXL Devices Attached to an RCH for the lookup of the
> > upstream port component registers).
> > 
> > Tested-by: Robert Richter <rrichter@amd.com>
> 
> With the one comment below addressed you can also add my:
> 
> Reviewed-by: Robert Richter <rrichter@amd.com>
> 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/port.c |    7 +++++++
> >  drivers/cxl/cxlmem.h    |    2 ++
> >  drivers/cxl/mem.c       |   31 ++++++++++++++++++++++++-------
> >  drivers/cxl/pci.c       |   10 ++++++++++
> >  4 files changed, 43 insertions(+), 7 deletions(-)
> 
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> 
> > @@ -119,17 +131,22 @@ static int cxl_mem_probe(struct device *dev)
> >  		return -ENXIO;
> >  	}
> >  
> > -	device_lock(&parent_port->dev);
> > -	if (!parent_port->dev.driver) {
> > +	if (dport->rch)
> > +		endpoint_parent = parent_port->uport;
> > +	else
> > +		endpoint_parent = &parent_port->dev;
> > +
> > +	device_lock(endpoint_parent);
> > +	if (!endpoint_parent->driver) {
> >  		dev_err(dev, "CXL port topology %s not enabled\n",
> >  			dev_name(&parent_port->dev));
> 
> Already reported: dev_name(endpoint_parent)

Yup, got it, thanks.

Re: [PATCH v6 10/12] cxl/port: Add RCD endpoint port enumeration

[Jonathan Cameron]

On Thu, 01 Dec 2022 13:34:16 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Unlike a CXL memory expander in a VH topology that has at least one
> intervening 'struct cxl_port' instance between itself and the CXL root
> device, an RCD attaches one-level higher. For example:
> 
>                VH
>           ┌──────────┐
>           │ ACPI0017 │
>           │  root0   │
>           └─────┬────┘
>                 │
>           ┌─────┴────┐
>           │  dport0  │
>     ┌─────┤ ACPI0016 ├─────┐
>     │     │  port1   │     │
>     │     └────┬─────┘     │
>     │          │           │
>  ┌──┴───┐   ┌──┴───┐   ┌───┴──┐
>  │dport0│   │dport1│   │dport2│
>  │ RP0  │   │ RP1  │   │ RP2  │
>  └──────┘   └──┬───┘   └──────┘
>                │
>            ┌───┴─────┐
>            │endpoint0│
>            │  port2  │
>            └─────────┘
> 
> ...vs:
> 
>               RCH
>           ┌──────────┐
>           │ ACPI0017 │
>           │  root0   │
>           └────┬─────┘
>                │
>            ┌───┴────┐
>            │ dport0 │
>            │ACPI0016│
>            └───┬────┘
>                │
>           ┌────┴─────┐
>           │endpoint0 │
>           │  port1   │
>           └──────────┘
> 
> So arrange for endpoint port in the RCH/RCD case to appear directly
> connected to the host-bridge in its singular role as a dport. Compare
> that to the VH case where the host-bridge serves a dual role as a
> 'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for
> the Root Ports in the Root Complex that are modeled as 'cxl_dport'
> instances in the CXL topology.
> 
> Another deviation from the VH case is that RCDs may need to look up
> their component registers from the Root Complex Register Block (RCRB).
> That platform firmware specified RCRB area is cached by the cxl_acpi
> driver and conveyed via the host-bridge dport to the cxl_mem driver to
> perform the cxl_rcrb_to_component() lookup for the endpoint port
> (See 9.11.8 CXL Devices Attached to an RCH for the lookup of the
> upstream port component registers).
> 
> Tested-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

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

Bonus points are awarded for the artwork.

J

[PATCH v6 11/12] tools/testing/cxl: Add an RCH topology

[Dan Williams]

In an RCH topology a CXL host-bridge as Root Complex Integrated Endpoint
the represents the memory expander. Unlike a VH topology there is no
CXL/PCIE Root Port that host the endpoint. The CXL subsystem maps this
as the CXL root object (ACPI0017 on ACPI based systems) targeting the
host-bridge as a dport, per usual, but then that dport directly hosts
the endpoint port.

Mock up that configuration with a 4th host-bridge that has a 'cxl_rcd'
device instance as its immediate child.

Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 tools/testing/cxl/test/cxl.c |  151 +++++++++++++++++++++++++++++++++++++++---
 tools/testing/cxl/test/mem.c |   37 ++++++++++
 2 files changed, 176 insertions(+), 12 deletions(-)

diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index c1e395a5b8f7..840ed1a3ba88 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -13,9 +13,11 @@
 
 #define NR_CXL_HOST_BRIDGES 2
 #define NR_CXL_SINGLE_HOST 1
+#define NR_CXL_RCH 1
 #define NR_CXL_ROOT_PORTS 2
 #define NR_CXL_SWITCH_PORTS 2
 #define NR_CXL_PORT_DECODERS 8
+#define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH)
 
 static struct platform_device *cxl_acpi;
 static struct platform_device *cxl_host_bridge[NR_CXL_HOST_BRIDGES];
@@ -35,6 +37,8 @@ static struct platform_device *cxl_swd_single[NR_MEM_SINGLE];
 struct platform_device *cxl_mem[NR_MEM_MULTI];
 struct platform_device *cxl_mem_single[NR_MEM_SINGLE];
 
+static struct platform_device *cxl_rch[NR_CXL_RCH];
+static struct platform_device *cxl_rcd[NR_CXL_RCH];
 
 static inline bool is_multi_bridge(struct device *dev)
 {
@@ -57,7 +61,7 @@ static inline bool is_single_bridge(struct device *dev)
 }
 
 static struct acpi_device acpi0017_mock;
-static struct acpi_device host_bridge[NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST] = {
+static struct acpi_device host_bridge[NR_BRIDGES] = {
 	[0] = {
 		.handle = &host_bridge[0],
 	},
@@ -67,7 +71,9 @@ static struct acpi_device host_bridge[NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST]
 	[2] = {
 		.handle = &host_bridge[2],
 	},
-
+	[3] = {
+		.handle = &host_bridge[3],
+	},
 };
 
 static bool is_mock_dev(struct device *dev)
@@ -80,6 +86,9 @@ static bool is_mock_dev(struct device *dev)
 	for (i = 0; i < ARRAY_SIZE(cxl_mem_single); i++)
 		if (dev == &cxl_mem_single[i]->dev)
 			return true;
+	for (i = 0; i < ARRAY_SIZE(cxl_rcd); i++)
+		if (dev == &cxl_rcd[i]->dev)
+			return true;
 	if (dev == &cxl_acpi->dev)
 		return true;
 	return false;
@@ -101,7 +110,7 @@ static bool is_mock_adev(struct acpi_device *adev)
 
 static struct {
 	struct acpi_table_cedt cedt;
-	struct acpi_cedt_chbs chbs[NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST];
+	struct acpi_cedt_chbs chbs[NR_BRIDGES];
 	struct {
 		struct acpi_cedt_cfmws cfmws;
 		u32 target[1];
@@ -122,6 +131,10 @@ static struct {
 		struct acpi_cedt_cfmws cfmws;
 		u32 target[1];
 	} cfmws4;
+	struct {
+		struct acpi_cedt_cfmws cfmws;
+		u32 target[1];
+	} cfmws5;
 } __packed mock_cedt = {
 	.cedt = {
 		.header = {
@@ -154,6 +167,14 @@ static struct {
 		.uid = 2,
 		.cxl_version = ACPI_CEDT_CHBS_VERSION_CXL20,
 	},
+	.chbs[3] = {
+		.header = {
+			.type = ACPI_CEDT_TYPE_CHBS,
+			.length = sizeof(mock_cedt.chbs[0]),
+		},
+		.uid = 3,
+		.cxl_version = ACPI_CEDT_CHBS_VERSION_CXL11,
+	},
 	.cfmws0 = {
 		.cfmws = {
 			.header = {
@@ -229,6 +250,21 @@ static struct {
 		},
 		.target = { 2 },
 	},
+	.cfmws5 = {
+		.cfmws = {
+			.header = {
+				.type = ACPI_CEDT_TYPE_CFMWS,
+				.length = sizeof(mock_cedt.cfmws5),
+			},
+			.interleave_ways = 0,
+			.granularity = 4,
+			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_TYPE3 |
+					ACPI_CEDT_CFMWS_RESTRICT_VOLATILE,
+			.qtg_id = 5,
+			.window_size = SZ_256M,
+		},
+		.target = { 3 },
+	},
 };
 
 struct acpi_cedt_cfmws *mock_cfmws[] = {
@@ -237,6 +273,7 @@ struct acpi_cedt_cfmws *mock_cfmws[] = {
 	[2] = &mock_cedt.cfmws2.cfmws,
 	[3] = &mock_cedt.cfmws3.cfmws,
 	[4] = &mock_cedt.cfmws4.cfmws,
+	[5] = &mock_cedt.cfmws5.cfmws,
 };
 
 struct cxl_mock_res {
@@ -262,11 +299,11 @@ static void depopulate_all_mock_resources(void)
 	mutex_unlock(&mock_res_lock);
 }
 
-static struct cxl_mock_res *alloc_mock_res(resource_size_t size)
+static struct cxl_mock_res *alloc_mock_res(resource_size_t size, int align)
 {
 	struct cxl_mock_res *res = kzalloc(sizeof(*res), GFP_KERNEL);
 	struct genpool_data_align data = {
-		.align = SZ_256M,
+		.align = align,
 	};
 	unsigned long phys;
 
@@ -301,7 +338,7 @@ static int populate_cedt(void)
 		else
 			size = ACPI_CEDT_CHBS_LENGTH_CXL11;
 
-		res = alloc_mock_res(size);
+		res = alloc_mock_res(size, size);
 		if (!res)
 			return -ENOMEM;
 		chbs->base = res->range.start;
@@ -311,7 +348,7 @@ static int populate_cedt(void)
 	for (i = 0; i < ARRAY_SIZE(mock_cfmws); i++) {
 		struct acpi_cedt_cfmws *window = mock_cfmws[i];
 
-		res = alloc_mock_res(window->window_size);
+		res = alloc_mock_res(window->window_size, SZ_256M);
 		if (!res)
 			return -ENOMEM;
 		window->base_hpa = res->range.start;
@@ -372,6 +409,10 @@ static bool is_mock_bridge(struct device *dev)
 	for (i = 0; i < ARRAY_SIZE(cxl_hb_single); i++)
 		if (dev == &cxl_hb_single[i]->dev)
 			return true;
+	for (i = 0; i < ARRAY_SIZE(cxl_rch); i++)
+		if (dev == &cxl_rch[i]->dev)
+			return true;
+
 	return false;
 }
 
@@ -441,7 +482,7 @@ mock_acpi_evaluate_integer(acpi_handle handle, acpi_string pathname,
 	return AE_OK;
 }
 
-static struct pci_bus mock_pci_bus[NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST];
+static struct pci_bus mock_pci_bus[NR_BRIDGES];
 static struct acpi_pci_root mock_pci_root[ARRAY_SIZE(mock_pci_bus)] = {
 	[0] = {
 		.bus = &mock_pci_bus[0],
@@ -452,7 +493,9 @@ static struct acpi_pci_root mock_pci_root[ARRAY_SIZE(mock_pci_bus)] = {
 	[2] = {
 		.bus = &mock_pci_bus[2],
 	},
-
+	[3] = {
+		.bus = &mock_pci_bus[3],
+	},
 };
 
 static bool is_mock_bus(struct pci_bus *bus)
@@ -738,6 +781,87 @@ static void mock_companion(struct acpi_device *adev, struct device *dev)
 #define SZ_512G (SZ_64G * 8)
 #endif
 
+static __init int cxl_rch_init(void)
+{
+	int rc, i;
+
+	for (i = 0; i < ARRAY_SIZE(cxl_rch); i++) {
+		int idx = NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + i;
+		struct acpi_device *adev = &host_bridge[idx];
+		struct platform_device *pdev;
+
+		pdev = platform_device_alloc("cxl_host_bridge", idx);
+		if (!pdev)
+			goto err_bridge;
+
+		mock_companion(adev, &pdev->dev);
+		rc = platform_device_add(pdev);
+		if (rc) {
+			platform_device_put(pdev);
+			goto err_bridge;
+		}
+
+		cxl_rch[i] = pdev;
+		mock_pci_bus[idx].bridge = &pdev->dev;
+		rc = sysfs_create_link(&pdev->dev.kobj, &pdev->dev.kobj,
+				       "firmware_node");
+		if (rc)
+			goto err_bridge;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(cxl_rcd); i++) {
+		int idx = NR_MEM_MULTI + NR_MEM_SINGLE + i;
+		struct platform_device *rch = cxl_rch[i];
+		struct platform_device *pdev;
+
+		pdev = platform_device_alloc("cxl_rcd", idx);
+		if (!pdev)
+			goto err_mem;
+		pdev->dev.parent = &rch->dev;
+		set_dev_node(&pdev->dev, i % 2);
+
+		rc = platform_device_add(pdev);
+		if (rc) {
+			platform_device_put(pdev);
+			goto err_mem;
+		}
+		cxl_rcd[i] = pdev;
+	}
+
+	return 0;
+
+err_mem:
+	for (i = ARRAY_SIZE(cxl_rcd) - 1; i >= 0; i--)
+		platform_device_unregister(cxl_rcd[i]);
+err_bridge:
+	for (i = ARRAY_SIZE(cxl_rch) - 1; i >= 0; i--) {
+		struct platform_device *pdev = cxl_rch[i];
+
+		if (!pdev)
+			continue;
+		sysfs_remove_link(&pdev->dev.kobj, "firmware_node");
+		platform_device_unregister(cxl_rch[i]);
+	}
+
+	return rc;
+}
+
+static void cxl_rch_exit(void)
+{
+	int i;
+
+	for (i = ARRAY_SIZE(cxl_rcd) - 1; i >= 0; i--)
+		platform_device_unregister(cxl_rcd[i]);
+	for (i = ARRAY_SIZE(cxl_rch) - 1; i >= 0; i--) {
+		struct platform_device *pdev = cxl_rch[i];
+
+		if (!pdev)
+			continue;
+		sysfs_remove_link(&pdev->dev.kobj, "firmware_node");
+		platform_device_unregister(cxl_rch[i]);
+	}
+}
+
 static __init int cxl_single_init(void)
 {
 	int i, rc;
@@ -1010,9 +1134,13 @@ static __init int cxl_test_init(void)
 	if (rc)
 		goto err_mem;
 
+	rc = cxl_rch_init();
+	if (rc)
+		goto err_single;
+
 	cxl_acpi = platform_device_alloc("cxl_acpi", 0);
 	if (!cxl_acpi)
-		goto err_single;
+		goto err_rch;
 
 	mock_companion(&acpi0017_mock, &cxl_acpi->dev);
 	acpi0017_mock.dev.bus = &platform_bus_type;
@@ -1025,6 +1153,8 @@ static __init int cxl_test_init(void)
 
 err_add:
 	platform_device_put(cxl_acpi);
+err_rch:
+	cxl_rch_exit();
 err_single:
 	cxl_single_exit();
 err_mem:
@@ -1062,6 +1192,7 @@ static __exit void cxl_test_exit(void)
 	int i;
 
 	platform_device_unregister(cxl_acpi);
+	cxl_rch_exit();
 	cxl_single_exit();
 	for (i = ARRAY_SIZE(cxl_mem) - 1; i >= 0; i--)
 		platform_device_unregister(cxl_mem[i]);
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index a4ee8e61dd60..b59c5976b2d9 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -100,6 +100,24 @@ static int mock_get_log(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
 	return 0;
 }
 
+static int mock_rcd_id(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
+{
+	struct cxl_mbox_identify id = {
+		.fw_revision = { "mock fw v1 " },
+		.total_capacity =
+			cpu_to_le64(DEV_SIZE / CXL_CAPACITY_MULTIPLIER),
+		.volatile_capacity =
+			cpu_to_le64(DEV_SIZE / CXL_CAPACITY_MULTIPLIER),
+	};
+
+	if (cmd->size_out < sizeof(id))
+		return -EINVAL;
+
+	memcpy(cmd->payload_out, &id, sizeof(id));
+
+	return 0;
+}
+
 static int mock_id(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
 {
 	struct cxl_mbox_identify id = {
@@ -216,7 +234,10 @@ static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *
 		rc = mock_get_log(cxlds, cmd);
 		break;
 	case CXL_MBOX_OP_IDENTIFY:
-		rc = mock_id(cxlds, cmd);
+		if (cxlds->rcd)
+			rc = mock_rcd_id(cxlds, cmd);
+		else
+			rc = mock_id(cxlds, cmd);
 		break;
 	case CXL_MBOX_OP_GET_LSA:
 		rc = mock_get_lsa(cxlds, cmd);
@@ -245,6 +266,13 @@ static void label_area_release(void *lsa)
 	vfree(lsa);
 }
 
+static bool is_rcd(struct platform_device *pdev)
+{
+	const struct platform_device_id *id = platform_get_device_id(pdev);
+
+	return !!id->driver_data;
+}
+
 static int cxl_mock_mem_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -268,6 +296,10 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	cxlds->serial = pdev->id;
 	cxlds->mbox_send = cxl_mock_mbox_send;
 	cxlds->payload_size = SZ_4K;
+	if (is_rcd(pdev)) {
+		cxlds->rcd = true;
+		cxlds->component_reg_phys = CXL_RESOURCE_NONE;
+	}
 
 	rc = cxl_enumerate_cmds(cxlds);
 	if (rc)
@@ -289,7 +321,8 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 }
 
 static const struct platform_device_id cxl_mock_mem_ids[] = {
-	{ .name = "cxl_mem", },
+	{ .name = "cxl_mem", 0 },
+	{ .name = "cxl_rcd", 1 },
 	{ },
 };
 MODULE_DEVICE_TABLE(platform, cxl_mock_mem_ids);

Re: [PATCH v6 11/12] tools/testing/cxl: Add an RCH topology

[Robert Richter]

On 01.12.22 13:34:21, Dan Williams wrote:
> In an RCH topology a CXL host-bridge as Root Complex Integrated Endpoint
> the represents the memory expander. Unlike a VH topology there is no
> CXL/PCIE Root Port that host the endpoint. The CXL subsystem maps this
> as the CXL root object (ACPI0017 on ACPI based systems) targeting the
> host-bridge as a dport, per usual, but then that dport directly hosts
> the endpoint port.
> 
> Mock up that configuration with a 4th host-bridge that has a 'cxl_rcd'
> device instance as its immediate child.
> 
> Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Robert Richter <rrichter@amd.com>

> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  tools/testing/cxl/test/cxl.c |  151 +++++++++++++++++++++++++++++++++++++++---
>  tools/testing/cxl/test/mem.c |   37 ++++++++++
>  2 files changed, 176 insertions(+), 12 deletions(-)

Re: [PATCH v6 11/12] tools/testing/cxl: Add an RCH topology

[Jonathan Cameron]

On Thu, 01 Dec 2022 13:34:21 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> In an RCH topology a CXL host-bridge as Root Complex Integrated Endpoint
> the represents the memory expander. Unlike a VH topology there is no
> CXL/PCIE Root Port that host the endpoint. The CXL subsystem maps this
> as the CXL root object (ACPI0017 on ACPI based systems) targeting the
> host-bridge as a dport, per usual, but then that dport directly hosts
> the endpoint port.
> 
> Mock up that configuration with a 4th host-bridge that has a 'cxl_rcd'
> device instance as its immediate child.
> 
> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

A few trivial things inline.  

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



> -static struct pci_bus mock_pci_bus[NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST];
> +static struct pci_bus mock_pci_bus[NR_BRIDGES];
>  static struct acpi_pci_root mock_pci_root[ARRAY_SIZE(mock_pci_bus)] = {
>  	[0] = {
>  		.bus = &mock_pci_bus[0],
> @@ -452,7 +493,9 @@ static struct acpi_pci_root mock_pci_root[ARRAY_SIZE(mock_pci_bus)] = {
>  	[2] = {
>  		.bus = &mock_pci_bus[2],
>  	},
> -

I guess fixing this stray space here is fine to avoid a rebase to tidy it up
in original patch which you have on your next branch.

> +	[3] = {
> +		.bus = &mock_pci_bus[3],
> +	},
>  };
>  
>  static bool is_mock_bus(struct pci_bus *bus)
> @@ -738,6 +781,87 @@ static void mock_companion(struct acpi_device *adev, struct device *dev)
>  #define SZ_512G (SZ_64G * 8)
>  #endif
>  
> +static __init int cxl_rch_init(void)
> +{
> +	int rc, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(cxl_rch); i++) {
> +		int idx = NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + i;
> +		struct acpi_device *adev = &host_bridge[idx];
> +		struct platform_device *pdev;
> +
> +		pdev = platform_device_alloc("cxl_host_bridge", idx);
> +		if (!pdev)
> +			goto err_bridge;
> +
> +		mock_companion(adev, &pdev->dev);
> +		rc = platform_device_add(pdev);
> +		if (rc) {
> +			platform_device_put(pdev);
> +			goto err_bridge;
> +		}
> +
> +		cxl_rch[i] = pdev;

Reason for this suggestion is below.
Move down cxl_rch[i] = pdev;...

> +		mock_pci_bus[idx].bridge = &pdev->dev;
> +		rc = sysfs_create_link(&pdev->dev.kobj, &pdev->dev.kobj,
> +				       "firmware_node");
> +		if (rc)
> +			goto err_bridge;

to here, and clean up this single loop iteration by having a 
platform_device_unregister in the error path above.

> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(cxl_rcd); i++) {
> +		int idx = NR_MEM_MULTI + NR_MEM_SINGLE + i;
> +		struct platform_device *rch = cxl_rch[i];
> +		struct platform_device *pdev;
> +
> +		pdev = platform_device_alloc("cxl_rcd", idx);
> +		if (!pdev)
> +			goto err_mem;
> +		pdev->dev.parent = &rch->dev;
> +		set_dev_node(&pdev->dev, i % 2);
> +
> +		rc = platform_device_add(pdev);
> +		if (rc) {
> +			platform_device_put(pdev);
> +			goto err_mem;
> +		}
> +		cxl_rcd[i] = pdev;
> +	}
> +
> +	return 0;
> +
> +err_mem:
> +	for (i = ARRAY_SIZE(cxl_rcd) - 1; i >= 0; i--)
> +		platform_device_unregister(cxl_rcd[i]);
> +err_bridge:
> +	for (i = ARRAY_SIZE(cxl_rch) - 1; i >= 0; i--) {
> +		struct platform_device *pdev = cxl_rch[i];
> +
> +		if (!pdev)
> +			continue;
> +		sysfs_remove_link(&pdev->dev.kobj, "firmware_node");

Had to look up that this was safe if the file doesn't exist (it is)
I'd rather not have to check, so maybe make the sysfs path
above clean up the device in the loop iteration and only set
cxl_rch[i] once the loop iteration can't fail?  See above.

To my mind doing it that way is more 'obviously correct'
which is never a bad thing.

> +		platform_device_unregister(cxl_rch[i]);
> +	}
> +
> +	return rc;
> +}
> +
> +static void cxl_rch_exit(void)
> +{
> +	int i;
> +
> +	for (i = ARRAY_SIZE(cxl_rcd) - 1; i >= 0; i--)
> +		platform_device_unregister(cxl_rcd[i]);
> +	for (i = ARRAY_SIZE(cxl_rch) - 1; i >= 0; i--) {
> +		struct platform_device *pdev = cxl_rch[i];
> +
> +		if (!pdev)
> +			continue;
> +		sysfs_remove_link(&pdev->dev.kobj, "firmware_node");
> +		platform_device_unregister(cxl_rch[i]);
> +	}
> +}
> +

Re: [PATCH v6 11/12] tools/testing/cxl: Add an RCH topology

[Dan Williams]

Jonathan Cameron wrote:
> On Thu, 01 Dec 2022 13:34:21 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > In an RCH topology a CXL host-bridge as Root Complex Integrated Endpoint
> > the represents the memory expander. Unlike a VH topology there is no
> > CXL/PCIE Root Port that host the endpoint. The CXL subsystem maps this
> > as the CXL root object (ACPI0017 on ACPI based systems) targeting the
> > host-bridge as a dport, per usual, but then that dport directly hosts
> > the endpoint port.
> > 
> > Mock up that configuration with a 4th host-bridge that has a 'cxl_rcd'
> > device instance as its immediate child.
> > 
> > Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> A few trivial things inline.  
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 
> 
> > -static struct pci_bus mock_pci_bus[NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST];
> > +static struct pci_bus mock_pci_bus[NR_BRIDGES];
> >  static struct acpi_pci_root mock_pci_root[ARRAY_SIZE(mock_pci_bus)] = {
> >  	[0] = {
> >  		.bus = &mock_pci_bus[0],
> > @@ -452,7 +493,9 @@ static struct acpi_pci_root mock_pci_root[ARRAY_SIZE(mock_pci_bus)] = {
> >  	[2] = {
> >  		.bus = &mock_pci_bus[2],
> >  	},
> > -
> 
> I guess fixing this stray space here is fine to avoid a rebase to tidy it up
> in original patch which you have on your next branch.

In fact it's already upstream in v6.1-rc4 unfortunately, but it has no
business being in this patch.

So the hunk is now:

@@ -452,6 +493,9 @@ static struct acpi_pci_root mock_pci_root[ARRAY_SIZE(mock_pci_bus)] = {
        [2] = {
                .bus = &mock_pci_bus[2],
        },
+       [3] = {
+               .bus = &mock_pci_bus[3],
+       },
 
 };
 
> 
> > +	[3] = {
> > +		.bus = &mock_pci_bus[3],
> > +	},
> >  };
> >  
> >  static bool is_mock_bus(struct pci_bus *bus)
> > @@ -738,6 +781,87 @@ static void mock_companion(struct acpi_device *adev, struct device *dev)
> >  #define SZ_512G (SZ_64G * 8)
> >  #endif
> >  
> > +static __init int cxl_rch_init(void)
> > +{
> > +	int rc, i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(cxl_rch); i++) {
> > +		int idx = NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + i;
> > +		struct acpi_device *adev = &host_bridge[idx];
> > +		struct platform_device *pdev;
> > +
> > +		pdev = platform_device_alloc("cxl_host_bridge", idx);
> > +		if (!pdev)
> > +			goto err_bridge;
> > +
> > +		mock_companion(adev, &pdev->dev);
> > +		rc = platform_device_add(pdev);
> > +		if (rc) {
> > +			platform_device_put(pdev);
> > +			goto err_bridge;
> > +		}
> > +
> > +		cxl_rch[i] = pdev;
> 
> Reason for this suggestion is below.
> Move down cxl_rch[i] = pdev;...
> 
> > +		mock_pci_bus[idx].bridge = &pdev->dev;
> > +		rc = sysfs_create_link(&pdev->dev.kobj, &pdev->dev.kobj,
> > +				       "firmware_node");
> > +		if (rc)
> > +			goto err_bridge;
> 
> to here, and clean up this single loop iteration by having a 
> platform_device_unregister in the error path above.

Ok, makes sense.

> 
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(cxl_rcd); i++) {
> > +		int idx = NR_MEM_MULTI + NR_MEM_SINGLE + i;
> > +		struct platform_device *rch = cxl_rch[i];
> > +		struct platform_device *pdev;
> > +
> > +		pdev = platform_device_alloc("cxl_rcd", idx);
> > +		if (!pdev)
> > +			goto err_mem;
> > +		pdev->dev.parent = &rch->dev;
> > +		set_dev_node(&pdev->dev, i % 2);
> > +
> > +		rc = platform_device_add(pdev);
> > +		if (rc) {
> > +			platform_device_put(pdev);
> > +			goto err_mem;
> > +		}
> > +		cxl_rcd[i] = pdev;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_mem:
> > +	for (i = ARRAY_SIZE(cxl_rcd) - 1; i >= 0; i--)
> > +		platform_device_unregister(cxl_rcd[i]);
> > +err_bridge:
> > +	for (i = ARRAY_SIZE(cxl_rch) - 1; i >= 0; i--) {
> > +		struct platform_device *pdev = cxl_rch[i];
> > +
> > +		if (!pdev)
> > +			continue;
> > +		sysfs_remove_link(&pdev->dev.kobj, "firmware_node");
> 
> Had to look up that this was safe if the file doesn't exist (it is)
> I'd rather not have to check, so maybe make the sysfs path
> above clean up the device in the loop iteration and only set
> cxl_rch[i] once the loop iteration can't fail?  See above.
> 
> To my mind doing it that way is more 'obviously correct'
> which is never a bad thing.

So this also dovetails with Robert's feedback to move this error exit
block to just call cxl_rch_exit() [1]. In turn both of these feedbacks
are applicable to the other setup loops in this file. So I am thinking
this calls for a follow-on patch to cleanup all the instances of these 2
patterns in this file.

[1]: https://lore.kernel.org/all/Y4ZwXNpwt83puF4W@rric.localdomain/

[PATCH v6 12/12] cxl/acpi: Set ACPI's CXL _OSC to indicate RCD mode support

[Dan Williams]

From: Terry Bowman <terry.bowman@amd.com>

ACPI uses the CXL _OSC support method to communicate the available CXL
functionality to FW. The CXL _OSC support method includes a field to
indicate the OS is capable of RCD mode. FW can potentially change it's
operation depending on the _OSC support method reported by the OS.

The ACPI driver currently only sets the ACPI _OSC support method to
indicate CXL VH mode. Change the capability reported to also include
CXL RCD mode.

[1] CXL3.0 Table 9-26 'Interpretation of CXL _OSC Support Field'

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
[rrichter@amd.com: Reworded patch description.]
Signed-off-by: Robert Richter <rrichter@amd.com>
Link: http://lore.kernel.org/r/Y4cRV/Sj0epVW7bE@rric.localdomain
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/pci_root.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 4e3db20e9cbb..b3c202d2a433 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -493,6 +493,7 @@ static u32 calculate_cxl_support(void)
 	u32 support;
 
 	support = OSC_CXL_2_0_PORT_DEV_REG_ACCESS_SUPPORT;
+	support |= OSC_CXL_1_1_PORT_REG_ACCESS_SUPPORT;
 	if (pci_aer_available())
 		support |= OSC_CXL_PROTOCOL_ERR_REPORTING_SUPPORT;
 	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))

Re: [PATCH v6 12/12] cxl/acpi: Set ACPI's CXL _OSC to indicate RCD mode support

[Jonathan Cameron]

On Thu, 01 Dec 2022 13:34:27 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> From: Terry Bowman <terry.bowman@amd.com>
> 
> ACPI uses the CXL _OSC support method to communicate the available CXL
> functionality to FW. The CXL _OSC support method includes a field to
> indicate the OS is capable of RCD mode. FW can potentially change it's
> operation depending on the _OSC support method reported by the OS.
> 
> The ACPI driver currently only sets the ACPI _OSC support method to
> indicate CXL VH mode. Change the capability reported to also include
> CXL RCD mode.
> 
> [1] CXL3.0 Table 9-26 'Interpretation of CXL _OSC Support Field'
> 
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> [rrichter@amd.com: Reworded patch description.]
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Link: http://lore.kernel.org/r/Y4cRV/Sj0epVW7bE@rric.localdomain
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Mostly for completeness rather than because a review of this patch
brings any value as it's 'obviously correct'.

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

> ---
>  drivers/acpi/pci_root.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 4e3db20e9cbb..b3c202d2a433 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -493,6 +493,7 @@ static u32 calculate_cxl_support(void)
>  	u32 support;
>  
>  	support = OSC_CXL_2_0_PORT_DEV_REG_ACCESS_SUPPORT;
> +	support |= OSC_CXL_1_1_PORT_REG_ACCESS_SUPPORT;
>  	if (pci_aer_available())
>  		support |= OSC_CXL_PROTOCOL_ERR_REPORTING_SUPPORT;
>  	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
>