[v3,23/28] cxl/acpi: Do not add DSDT disabled ACPI0016 host bridge ports

Commit Message

Dan Williams Aug. 24, 2021, 4:07 p.m. UTC

From: Alison Schofield <[e-mail]>

During CXL ACPI probe, host bridge ports are discovered by scanning
the ACPI0017 root port for ACPI0016 host bridge devices. The scan
matches on the hardware id of "ACPI0016". An issue occurs when an
ACPI0016 device is defined in the DSDT yet disabled on the platform.
Attempts by the cxl_acpi driver to add host bridge ports using a
disabled device fails, and the entire cxl_acpi probe fails.

The DSDT table includes an _STA method that sets the status and the
ACPI subsystem has checks available to examine it. One such check is
in the acpi_pci_find_root() path. Move the call to acpi_pci_find_root()
to the matching function to prevent this issue when adding either
upstream or downstream ports.

Suggested-by: Dan Williams <[e-mail]>
Signed-off-by: Alison Schofield <[e-mail]>
Fixes: 7d4b5ca2e2cb ("cxl/acpi: Add downstream port data to cxl_port instances")
Cc: <[e-mail]>
Signed-off-by: Dan Williams <[e-mail]>
---
 drivers/cxl/acpi.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Jonathan Cameron Sept. 2, 2021, 6:30 p.m. UTC | #1

On Tue, 24 Aug 2021 09:07:28 -0700
Dan Williams <[e-mail]> wrote:

> From: Alison Schofield <[e-mail]>
> 
> During CXL ACPI probe, host bridge ports are discovered by scanning
> the ACPI0017 root port for ACPI0016 host bridge devices. The scan
> matches on the hardware id of "ACPI0016". An issue occurs when an
> ACPI0016 device is defined in the DSDT yet disabled on the platform.
> Attempts by the cxl_acpi driver to add host bridge ports using a
> disabled device fails, and the entire cxl_acpi probe fails.
> 
> The DSDT table includes an _STA method that sets the status and the
> ACPI subsystem has checks available to examine it. One such check is
> in the acpi_pci_find_root() path. Move the call to acpi_pci_find_root()
> to the matching function to prevent this issue when adding either
> upstream or downstream ports.
You could mention that has the side effect that the existing
call in add_host_bridge_uport() is now guarantee not to fail so
can be moved closer to where it is used.  That chunk had
me briefly confused.

> 
> Suggested-by: Dan Williams <[e-mail]>
> Signed-off-by: Alison Schofield <[e-mail]>
> Fixes: 7d4b5ca2e2cb ("cxl/acpi: Add downstream port data to cxl_port instances")
> Cc: <[e-mail]>
> Signed-off-by: Dan Williams <[e-mail]>
I'd imagine we want to get this in an early rc so perhaps pull out of
this series.

Reviewed-by: Jonathan Cameron <[e-mail]>


> ---
>  drivers/cxl/acpi.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 8ae89273f58e..2d8f1ec1abff 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -243,6 +243,9 @@ static struct acpi_device *to_cxl_host_bridge(struct device *dev)
>  {
>  	struct acpi_device *adev = to_acpi_device(dev);
>  
> +	if (!acpi_pci_find_root(adev->handle))
> +		return NULL;
> +
>  	if (strcmp(acpi_device_hid(adev), "ACPI0016") == 0)
>  		return adev;
>  	return NULL;
> @@ -266,10 +269,6 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  	if (!bridge)
>  		return 0;
>  
> -	pci_root = acpi_pci_find_root(bridge->handle);
> -	if (!pci_root)
> -		return -ENXIO;
> -
>  	dport = find_dport_by_dev(root_port, match);
>  	if (!dport) {
>  		dev_dbg(host, "host bridge expected and not found\n");
> @@ -282,6 +281,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  		return PTR_ERR(port);
>  	dev_dbg(host, "%s: add: %s\n", dev_name(match), dev_name(&port->dev));
>  
> +	pci_root = acpi_pci_find_root(bridge->handle);
>  	ctx = (struct cxl_walk_context){
>  		.dev = host,
>  		.root = pci_root->bus,
>

Dan Williams Sept. 3, 2021, 5:51 p.m. UTC | #2

On Thu, Sep 2, 2021 at 11:30 AM Jonathan Cameron
<[e-mail]> wrote:
>
> On Tue, 24 Aug 2021 09:07:28 -0700
> Dan Williams <[e-mail]> wrote:
>
> > From: Alison Schofield <[e-mail]>
> >
> > During CXL ACPI probe, host bridge ports are discovered by scanning
> > the ACPI0017 root port for ACPI0016 host bridge devices. The scan
> > matches on the hardware id of "ACPI0016". An issue occurs when an
> > ACPI0016 device is defined in the DSDT yet disabled on the platform.
> > Attempts by the cxl_acpi driver to add host bridge ports using a
> > disabled device fails, and the entire cxl_acpi probe fails.
> >
> > The DSDT table includes an _STA method that sets the status and the
> > ACPI subsystem has checks available to examine it. One such check is
> > in the acpi_pci_find_root() path. Move the call to acpi_pci_find_root()
> > to the matching function to prevent this issue when adding either
> > upstream or downstream ports.
> You could mention that has the side effect that the existing
> call in add_host_bridge_uport() is now guarantee not to fail so
> can be moved closer to where it is used.  That chunk had
> me briefly confused.
>
> >
> > Suggested-by: Dan Williams <[e-mail]>
> > Signed-off-by: Alison Schofield <[e-mail]>
> > Fixes: 7d4b5ca2e2cb ("cxl/acpi: Add downstream port data to cxl_port instances")
> > Cc: <[e-mail]>
> > Signed-off-by: Dan Williams <[e-mail]>
> I'd imagine we want to get this in an early rc so perhaps pull out of
> this series.

I'm going to keep it in this series because it causes merge conflicts
for what follows, but I'll merge it independently of the follow-on
reworks which may need to wait another cycle.

>
> Reviewed-by: Jonathan Cameron <[e-mail]>
>
>
> > ---
> >  drivers/cxl/acpi.c |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 8ae89273f58e..2d8f1ec1abff 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -243,6 +243,9 @@ static struct acpi_device *to_cxl_host_bridge(struct device *dev)
> >  {
> >       struct acpi_device *adev = to_acpi_device(dev);
> >
> > +     if (!acpi_pci_find_root(adev->handle))
> > +             return NULL;
> > +
> >       if (strcmp(acpi_device_hid(adev), "ACPI0016") == 0)
> >               return adev;
> >       return NULL;
> > @@ -266,10 +269,6 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> >       if (!bridge)
> >               return 0;
> >
> > -     pci_root = acpi_pci_find_root(bridge->handle);
> > -     if (!pci_root)
> > -             return -ENXIO;
> > -
> >       dport = find_dport_by_dev(root_port, match);
> >       if (!dport) {
> >               dev_dbg(host, "host bridge expected and not found\n");
> > @@ -282,6 +281,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> >               return PTR_ERR(port);
> >       dev_dbg(host, "%s: add: %s\n", dev_name(match), dev_name(&port->dev));
> >
> > +     pci_root = acpi_pci_find_root(bridge->handle);
> >       ctx = (struct cxl_walk_context){
> >               .dev = host,
> >               .root = pci_root->bus,
> >
>
>

Patch

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 8ae89273f58e..2d8f1ec1abff 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -243,6 +243,9 @@  static struct acpi_device *to_cxl_host_bridge(struct device *dev)
 {
 	struct acpi_device *adev = to_acpi_device(dev);
 
+	if (!acpi_pci_find_root(adev->handle))
+		return NULL;
+
 	if (strcmp(acpi_device_hid(adev), "ACPI0016") == 0)
 		return adev;
 	return NULL;
@@ -266,10 +269,6 @@  static int add_host_bridge_uport(struct device *match, void *arg)
 	if (!bridge)
 		return 0;
 
-	pci_root = acpi_pci_find_root(bridge->handle);
-	if (!pci_root)
-		return -ENXIO;
-
 	dport = find_dport_by_dev(root_port, match);
 	if (!dport) {
 		dev_dbg(host, "host bridge expected and not found\n");
@@ -282,6 +281,7 @@  static int add_host_bridge_uport(struct device *match, void *arg)
 		return PTR_ERR(port);
 	dev_dbg(host, "%s: add: %s\n", dev_name(match), dev_name(&port->dev));
 
+	pci_root = acpi_pci_find_root(bridge->handle);
 	ctx = (struct cxl_walk_context){
 		.dev = host,
 		.root = pci_root->bus,