[v3,1/9] cxl/acpi: Register CXL host ports by bridge device

  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.

If a mock device is registered by a test driver, the bridge pointer
can be NULL. Keep using the matching ACPI device (&adev->dev) as a
fallback in this case.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/acpi.c | 48 ++++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 17 deletions(-)
  

On Wed, Nov 09, 2022 at 11:40:51AM +0100, Robert Richter wrote:
> A port of a CXL host bridge links to the bridge's acpi device

s/acpi/ACPI/ to match usage below.

> (&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.
> 
> If a mock device is registered by a test driver, the bridge pointer
> can be NULL. Keep using the matching ACPI device (&adev->dev) as a
> fallback in this case.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
  

Robert Richter wrote:
> 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.
> 
> If a mock device is registered by a test driver, the bridge pointer
> can be NULL. Keep using the matching ACPI device (&adev->dev) as a
> fallback in this case.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/acpi.c | 48 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index fb9f72813067..06150c953f58 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -185,6 +185,17 @@ __mock struct acpi_device *to_cxl_host_bridge(struct device *host,
>  	return NULL;
>  }
>  
> +static inline struct acpi_pci_root *to_cxl_pci_root(struct device *host,
> +						    struct device *match)
> +{
> +	struct acpi_device *adev = to_cxl_host_bridge(host, match);
> +
> +	if (!adev)
> +		return NULL;
> +
> +	return acpi_pci_find_root(adev->handle);
> +}
> +
>  /*
>   * A host bridge is a dport to a CFMWS decode and it is a uport to the
>   * dport (PCIe Root Ports) in the host bridge.
> @@ -193,35 +204,35 @@ 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_pci_root *pci_root;
> +	struct acpi_pci_root *pci_root = to_cxl_pci_root(host, match);
>  	struct cxl_dport *dport;
>  	struct cxl_port *port;
> +	struct device *bridge;
>  	int rc;
>  
> -	if (!bridge)
> +	if (!pci_root)
>  		return 0;
>  
> -	dport = cxl_find_dport_by_dev(root_port, match);
> +	/*
> +	 * If it is a mock dev, the bridge can be NULL, use matching
> +	 * device (&adev->dev) as a fallback then then.
> +	 */
> +	bridge = pci_root->bus->bridge ?: match;

While I appreciate that you ran this against the unit tests, production
code should not know or care about the presence of mock devices. So,
this was showing a gap in the mock implementation, now addressed here:

http://lore.kernel.org/r/166845667383.1449826.14492184009399164787.stgit@dwillia2-xfh.jf.intel.com

With that, this approach can be simplified to the following:

-- >8 --
From 3cb7a46e100d016132727ad32904b629061e40d5 Mon Sep 17 00:00:00 2001
From: Robert Richter <rrichter@amd.com>
Date: Mon, 14 Nov 2022 12:20:27 -0800
Subject: [PATCH v4] cxl/acpi: Register CXL host ports by bridge device

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>
Co-developed-by: Dan Williams <dan.j.williams@intel.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 fb9f72813067..71f8bdedd676 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);
  

On 14.11.22 12:22:49, Dan Williams wrote:
> Robert Richter wrote:
> > 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.
> > 
> > If a mock device is registered by a test driver, the bridge pointer
> > can be NULL. Keep using the matching ACPI device (&adev->dev) as a
> > fallback in this case.
> > 
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > ---
> >  drivers/cxl/acpi.c | 48 ++++++++++++++++++++++++++++++----------------
> >  1 file changed, 31 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index fb9f72813067..06150c953f58 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -185,6 +185,17 @@ __mock struct acpi_device *to_cxl_host_bridge(struct device *host,
> >  	return NULL;
> >  }
> >  
> > +static inline struct acpi_pci_root *to_cxl_pci_root(struct device *host,
> > +						    struct device *match)
> > +{
> > +	struct acpi_device *adev = to_cxl_host_bridge(host, match);
> > +
> > +	if (!adev)
> > +		return NULL;
> > +
> > +	return acpi_pci_find_root(adev->handle);
> > +}
> > +
> >  /*
> >   * A host bridge is a dport to a CFMWS decode and it is a uport to the
> >   * dport (PCIe Root Ports) in the host bridge.
> > @@ -193,35 +204,35 @@ 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_pci_root *pci_root;
> > +	struct acpi_pci_root *pci_root = to_cxl_pci_root(host, match);
> >  	struct cxl_dport *dport;
> >  	struct cxl_port *port;
> > +	struct device *bridge;
> >  	int rc;
> >  
> > -	if (!bridge)
> > +	if (!pci_root)
> >  		return 0;
> >  
> > -	dport = cxl_find_dport_by_dev(root_port, match);
> > +	/*
> > +	 * If it is a mock dev, the bridge can be NULL, use matching
> > +	 * device (&adev->dev) as a fallback then then.
> > +	 */
> > +	bridge = pci_root->bus->bridge ?: match;
> 
> While I appreciate that you ran this against the unit tests, production
> code should not know or care about the presence of mock devices. So,
> this was showing a gap in the mock implementation, now addressed here:
> 
> http://lore.kernel.org/r/166845667383.1449826.14492184009399164787.stgit@dwillia2-xfh.jf.intel.com

Yes, with that update the above check can be dropped and the code can
be implemented now without the helper. The patch below looks good to
me. Going to run a test with it.

> 
> With that, this approach can be simplified to the following:
> 
> -- >8 --
> >From 3cb7a46e100d016132727ad32904b629061e40d5 Mon Sep 17 00:00:00 2001
> From: Robert Richter <rrichter@amd.com>
> Date: Mon, 14 Nov 2022 12:20:27 -0800
> Subject: [PATCH v4] cxl/acpi: Register CXL host ports by bridge device

Thanks,

-Robert