[PATCH] pci: Added flag to pci_host_bridge to hint not to use acpi

[PATCH] pci: Added flag to pci_host_bridge to hint not to use acpi

[Jon Derrick]

This patch adds a hint, no_acpi, to the pci_host_bridge struct which
informs the hotplug driver to not try and release the host bridge from
acpi.

The VMD driver creates a root port which does not have an acpi entry.
This patch will allow the hotplug driver to bypass acpi release when
enumerating the VMD root port as a hotplug slot.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 arch/x86/pci/vmd.c              | 4 ++++
 drivers/pci/pcie/portdrv_acpi.c | 5 +++++
 include/linux/pci.h             | 1 +
 3 files changed, 10 insertions(+)

diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c
index 7792aba..a196be3 100644
--- a/arch/x86/pci/vmd.c
+++ b/arch/x86/pci/vmd.c
@@ -531,6 +531,7 @@ static int vmd_find_free_domain(void)
 static int vmd_enable_domain(struct vmd_dev *vmd)
 {
 	struct pci_sysdata *sd = &vmd->sysdata;
+	struct pci_host_bridge *host_bridge;
 	struct resource *res;
 	u32 upper_bits;
 	unsigned long flags;
@@ -609,6 +610,9 @@ static int vmd_enable_domain(struct vmd_dev *vmd)
 		return -ENODEV;
 	}
 
+	host_bridge = to_pci_host_bridge(vmd->bus->bridge);
+	host_bridge->no_acpi = true;
+
 	vmd_attach_resources(vmd);
 	vmd_setup_dma_ops(vmd);
 	dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
diff --git a/drivers/pci/pcie/portdrv_acpi.c b/drivers/pci/pcie/portdrv_acpi.c
index b4d2894..0e6fa69 100644
--- a/drivers/pci/pcie/portdrv_acpi.c
+++ b/drivers/pci/pcie/portdrv_acpi.c
@@ -35,12 +35,17 @@
 int pcie_port_acpi_setup(struct pci_dev *port, int *srv_mask)
 {
 	struct acpi_pci_root *root;
+	struct pci_host_bridge *host_bridge;
 	acpi_handle handle;
 	u32 flags;
 
 	if (acpi_pci_disabled)
 		return 0;
 
+	host_bridge = pci_find_host_bridge(port->bus);
+	if (host_bridge && host_bridge->no_acpi)
+		return 0;
+
 	handle = acpi_find_root_bridge_handle(port);
 	if (!handle)
 		return -EINVAL;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 15f466e..a6958e9b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -407,6 +407,7 @@ struct pci_host_bridge {
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
 	unsigned int ignore_reset_delay:1;	/* for entire hierarchy */
+	unsigned int no_acpi:1;			/* bypasses acpi release */
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
 			const struct resource *res,
-- 
1.8.3.1

Re: [PATCH] pci: Added flag to pci_host_bridge to hint not to use acpi

[Keith Busch]

On Wed, Mar 23, 2016 at 04:01:58PM -0600, Jon Derrick wrote:
> This patch adds a hint, no_acpi, to the pci_host_bridge struct which
> informs the hotplug driver to not try and release the host bridge from
> acpi.
> 
> The VMD driver creates a root port which does not have an acpi entry.
> This patch will allow the hotplug driver to bypass acpi release when
> enumerating the VMD root port as a hotplug slot.

I want to add a little context to this for the motivation for this
implementation.

Currently we have to set kernel parameter "pcie_ports=native" for pciehp
to work on these domains since they are not known to ACPI. We don't want
to force a system wide PCIe parameter for something specific to a subset
of domains.

If the implementation here sounds reasonable, I suggest having a helper
API function in pci to check for the host bridge requested acpi disabled.

>  arch/x86/pci/vmd.c              | 4 ++++
>  drivers/pci/pcie/portdrv_acpi.c | 5 +++++
>  include/linux/pci.h             | 1 +
>  3 files changed, 10 insertions(+)
> 
> diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c
> index 7792aba..a196be3 100644
> --- a/arch/x86/pci/vmd.c
> +++ b/arch/x86/pci/vmd.c
> @@ -531,6 +531,7 @@ static int vmd_find_free_domain(void)
>  static int vmd_enable_domain(struct vmd_dev *vmd)
>  {
>  	struct pci_sysdata *sd = &vmd->sysdata;
> +	struct pci_host_bridge *host_bridge;
>  	struct resource *res;
>  	u32 upper_bits;
>  	unsigned long flags;
> @@ -609,6 +610,9 @@ static int vmd_enable_domain(struct vmd_dev *vmd)
>  		return -ENODEV;
>  	}
>  
> +	host_bridge = to_pci_host_bridge(vmd->bus->bridge);
> +	host_bridge->no_acpi = true;
> +
>  	vmd_attach_resources(vmd);
>  	vmd_setup_dma_ops(vmd);
>  	dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
> diff --git a/drivers/pci/pcie/portdrv_acpi.c b/drivers/pci/pcie/portdrv_acpi.c
> index b4d2894..0e6fa69 100644
> --- a/drivers/pci/pcie/portdrv_acpi.c
> +++ b/drivers/pci/pcie/portdrv_acpi.c
> @@ -35,12 +35,17 @@
>  int pcie_port_acpi_setup(struct pci_dev *port, int *srv_mask)
>  {
>  	struct acpi_pci_root *root;
> +	struct pci_host_bridge *host_bridge;
>  	acpi_handle handle;
>  	u32 flags;
>  
>  	if (acpi_pci_disabled)
>  		return 0;
>  
> +	host_bridge = pci_find_host_bridge(port->bus);
> +	if (host_bridge && host_bridge->no_acpi)
> +		return 0;
> +
>  	handle = acpi_find_root_bridge_handle(port);
>  	if (!handle)
>  		return -EINVAL;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 15f466e..a6958e9b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -407,6 +407,7 @@ struct pci_host_bridge {
>  	void (*release_fn)(struct pci_host_bridge *);
>  	void *release_data;
>  	unsigned int ignore_reset_delay:1;	/* for entire hierarchy */
> +	unsigned int no_acpi:1;			/* bypasses acpi release */
>  	/* Resource alignment requirements */
>  	resource_size_t (*align_resource)(struct pci_dev *dev,
>  			const struct resource *res,
> -- 
> 1.8.3.1

Re: [PATCH] pci: Added flag to pci_host_bridge to hint not to use acpi

[Bjorn Helgaas]

[+cc Rafael, linux-acpi]

On Thu, Mar 24, 2016 at 04:42:00PM +0000, Keith Busch wrote:
> On Wed, Mar 23, 2016 at 04:01:58PM -0600, Jon Derrick wrote:
> > This patch adds a hint, no_acpi, to the pci_host_bridge struct which
> > informs the hotplug driver to not try and release the host bridge from
> > acpi.
> > 
> > The VMD driver creates a root port which does not have an acpi entry.
> > This patch will allow the hotplug driver to bypass acpi release when
> > enumerating the VMD root port as a hotplug slot.
> 
> I want to add a little context to this for the motivation for this
> implementation.
> 
> Currently we have to set kernel parameter "pcie_ports=native" for pciehp
> to work on these domains since they are not known to ACPI. We don't want
> to force a system wide PCIe parameter for something specific to a subset
> of domains.

I agree, we shouldn't have to ask users to boot with
"pcie_ports=native".

The PCIe port driver figures out what service drivers can be enabled
by a combination of:

  - asking the platform what services it will allow the kernel to
    manage (on ACPI systems, this uses _OSC), and

  - looking at the PCIe capability to see what services the hardware
    supports.

This happens in get_port_device_capability().  When you boot with
"pcie_ports=native", get_port_device_capability() skips the _OSC part
and only looks at what the hardware supports.

Here's the rest of the path:

  pcie_portdrv_probe                     # pci_driver.probe
    pcie_port_device_register
      get_port_device_capability
        if (pcie_ports_auto)             # cleared by pcie_port=native
	  pcie_port_platform_notify
	    pcie_port_acpi_setup
	      if (no_acpi)               # added by your patch
		return 0
	      handle = acpi_find_root_bridge_handle(port)
	      if (!handle)
	        return -EINVAL
	      set mask of what we control based on _OSC
	      return 0

The spec (PCI Firmware spec 3.0, sec 4.5) says _OSC is a child of an
ACPI device and applies to that device.  In your case, I think there
is no ACPI PNP0A0x device for the VMD host bridge, and hence there is
no _OSC method for it.  There's no ACPI handle for the VMD bridge, so
I think pcie_port_acpi_setup() returns -EINVAL, which causes
get_port_device_capability() to give up and decide that the port
can't support *any* PCIe services.

I think that's the real problem: the fact that there's no ACPI device
corresponding to the bridge should not be an error.  It should just
mean the platform doesn't have an opportunity to limit what services
the kernel can manage.  I think pcie_port_acpi_setup() should return 0
in all cases.  Can you try that and see if it fixes the problem?

Bjorn

> >  arch/x86/pci/vmd.c              | 4 ++++
> >  drivers/pci/pcie/portdrv_acpi.c | 5 +++++
> >  include/linux/pci.h             | 1 +
> >  3 files changed, 10 insertions(+)
> > 
> > diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c
> > index 7792aba..a196be3 100644
> > --- a/arch/x86/pci/vmd.c
> > +++ b/arch/x86/pci/vmd.c
> > @@ -531,6 +531,7 @@ static int vmd_find_free_domain(void)
> >  static int vmd_enable_domain(struct vmd_dev *vmd)
> >  {
> >  	struct pci_sysdata *sd = &vmd->sysdata;
> > +	struct pci_host_bridge *host_bridge;
> >  	struct resource *res;
> >  	u32 upper_bits;
> >  	unsigned long flags;
> > @@ -609,6 +610,9 @@ static int vmd_enable_domain(struct vmd_dev *vmd)
> >  		return -ENODEV;
> >  	}
> >  
> > +	host_bridge = to_pci_host_bridge(vmd->bus->bridge);
> > +	host_bridge->no_acpi = true;
> > +
> >  	vmd_attach_resources(vmd);
> >  	vmd_setup_dma_ops(vmd);
> >  	dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
> > diff --git a/drivers/pci/pcie/portdrv_acpi.c b/drivers/pci/pcie/portdrv_acpi.c
> > index b4d2894..0e6fa69 100644
> > --- a/drivers/pci/pcie/portdrv_acpi.c
> > +++ b/drivers/pci/pcie/portdrv_acpi.c
> > @@ -35,12 +35,17 @@
> >  int pcie_port_acpi_setup(struct pci_dev *port, int *srv_mask)
> >  {
> >  	struct acpi_pci_root *root;
> > +	struct pci_host_bridge *host_bridge;
> >  	acpi_handle handle;
> >  	u32 flags;
> >  
> >  	if (acpi_pci_disabled)
> >  		return 0;
> >  
> > +	host_bridge = pci_find_host_bridge(port->bus);
> > +	if (host_bridge && host_bridge->no_acpi)
> > +		return 0;
> > +
> >  	handle = acpi_find_root_bridge_handle(port);
> >  	if (!handle)
> >  		return -EINVAL;
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 15f466e..a6958e9b 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -407,6 +407,7 @@ struct pci_host_bridge {
> >  	void (*release_fn)(struct pci_host_bridge *);
> >  	void *release_data;
> >  	unsigned int ignore_reset_delay:1;	/* for entire hierarchy */
> > +	unsigned int no_acpi:1;			/* bypasses acpi release */
> >  	/* Resource alignment requirements */
> >  	resource_size_t (*align_resource)(struct pci_dev *dev,
> >  			const struct resource *res,
> > -- 
> > 1.8.3.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pci: Added flag to pci_host_bridge to hint not to use acpi

[Jon Derrick]

Hi Bjorn,

First, thanks for the review

> The spec (PCI Firmware spec 3.0, sec 4.5) says _OSC is a child of an
> ACPI device and applies to that device.  In your case, I think there
> is no ACPI PNP0A0x device for the VMD host bridge, and hence there is
> no _OSC method for it.  There's no ACPI handle for the VMD bridge, so
> I think pcie_port_acpi_setup() returns -EINVAL, which causes
> get_port_device_capability() to give up and decide that the port
> can't support *any* PCIe services.
> 
> I think that's the real problem: the fact that there's no ACPI device
> corresponding to the bridge should not be an error.  It should just
> mean the platform doesn't have an opportunity to limit what services
> the kernel can manage.  I think pcie_port_acpi_setup() should return 0
> in all cases.  Can you try that and see if it fixes the problem?
> 

I believe what you are suggesting is:


        handle = acpi_find_root_bridge_handle(port);
        if (!handle)
-               return -EINVAL;
+               return 0;

        root = acpi_pci_find_root(handle);
        if (!root)
-               return -ENODEV;
+               return 0;


This works for me. I will follow up with a proper patch.

Re: [PATCH] pci: Added flag to pci_host_bridge to hint not to use acpi

[Bjorn Helgaas]

On Thu, Apr 07, 2016 at 02:12:16PM -0600, Jon Derrick wrote:
> Hi Bjorn,
> 
> First, thanks for the review
> 
> > The spec (PCI Firmware spec 3.0, sec 4.5) says _OSC is a child of an
> > ACPI device and applies to that device.  In your case, I think there
> > is no ACPI PNP0A0x device for the VMD host bridge, and hence there is
> > no _OSC method for it.  There's no ACPI handle for the VMD bridge, so
> > I think pcie_port_acpi_setup() returns -EINVAL, which causes
> > get_port_device_capability() to give up and decide that the port
> > can't support *any* PCIe services.
> > 
> > I think that's the real problem: the fact that there's no ACPI device
> > corresponding to the bridge should not be an error.  It should just
> > mean the platform doesn't have an opportunity to limit what services
> > the kernel can manage.  I think pcie_port_acpi_setup() should return 0
> > in all cases.  Can you try that and see if it fixes the problem?
> > 
> 
> I believe what you are suggesting is:
> 
> 
>         handle = acpi_find_root_bridge_handle(port);
>         if (!handle)
> -               return -EINVAL;
> +               return 0;
> 
>         root = acpi_pci_find_root(handle);
>         if (!root)
> -               return -ENODEV;
> +               return 0;

Yes, that's what I think we should do.

Bjorn