diff mbox series

[v2,08/13] PCI/portdrv: Simplify PCIe feature permission checking

Message ID 152062203293.77693.8159909590216160503.stgit@bhelgaas-glaptop.roam.corp.google.com
State Accepted
Delegated to: Bjorn Helgaas
Headers show
Series PCI: Simplify PCIe port driver | expand

Commit Message

Bjorn Helgaas March 9, 2018, 7 p.m. UTC 
From: Bjorn Helgaas <bhelgaas@google.com>

Some PCIe features (AER, DPC, hotplug, PME) can be managed by either the
platform firmware or the OS, so the host bridge driver may have to request
permission from the platform before using them.  On ACPI systems, this is
done by negotiate_os_control() in acpi_pci_root_add().

The PCIe port driver later uses pcie_port_platform_notify() and
pcie_port_acpi_setup() to figure out whether it can use these features.
But all we need is a single bit for each service, so these interfaces are
needlessly complicated.

Simplify this by adding bits in the struct pci_host_bridge to show when the
OS has permission to use each feature:

  + unsigned int use_aer:1;       /* OS may use PCIe AER */
  + unsigned int use_hotplug:1;	  /* OS may use PCIe hotplug */
  + unsigned int use_pme:1;       /* OS may use PCIe PME */

These are set when we create a host bridge, and the host bridge driver can
clear the bits corresponding to any feature the platform doesn't want us to
use.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/pci_root.c         |   13 ++++++++++--
 drivers/pci/pcie/Makefile       |    1 -
 drivers/pci/pcie/portdrv.h      |   11 ----------
 drivers/pci/pcie/portdrv_core.c |   42 ++++++++++++++++++++++++---------------
 drivers/pci/probe.c             |   10 +++++++++
 include/linux/pci.h             |    3 +++
 6 files changed, 50 insertions(+), 30 deletions(-)

Comments

Christoph Hellwig March 12, 2018, 8:04 a.m. UTC | #1 
> +	 * We assume we can manage these PCIe features.  Some systems may
> +	 * reserve these for use by the platform itself, e.g., an ACPI BIOS
> +	 * may implement its own AER handling and use _OSC to prevent the
> +	 * OS from interfering.
> +	 */
> +	bridge->use_aer = 1;
> +	bridge->use_hotplug = 1;
> +	bridge->use_pme = 1;

If we start out with enabled maybe these should be disable_foo flags
instead?

Also please use bool (or a bitfield bool) for true/false values.
Bjorn Helgaas March 12, 2018, 2:03 p.m. UTC | #2 
On Mon, Mar 12, 2018 at 01:04:02AM -0700, Christoph Hellwig wrote:
> > +	 * We assume we can manage these PCIe features.  Some systems may
> > +	 * reserve these for use by the platform itself, e.g., an ACPI BIOS
> > +	 * may implement its own AER handling and use _OSC to prevent the
> > +	 * OS from interfering.
> > +	 */
> > +	bridge->use_aer = 1;
> > +	bridge->use_hotplug = 1;
> > +	bridge->use_pme = 1;
> 
> If we start out with enabled maybe these should be disable_foo flags
> instead?

I went back and forth on that.  "disable_foo" is nice because the
default value is correct (zero means enabled).  But then you end up
with things like:

  if (pcie_ports_native || !host->disable_hotplug)

where the "!host->disable_hotplug" is a double negative, and I have a
really hard time reading that.

> Also please use bool (or a bitfield bool) for true/false values.

I'm a little ambivalent about bool in structs because of things like
this:

  https://lkml.kernel.org/r/CA+55aFxnePDimkVKVtv3gNmRGcwc8KQ5mHYvUxY8sAQg6yvVYg@mail.gmail.com

Bjorn
Lukas Wunner March 12, 2018, 2:20 p.m. UTC | #3 
On Mon, Mar 12, 2018 at 09:03:16AM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 12, 2018 at 01:04:02AM -0700, Christoph Hellwig wrote:
> > > +	 * We assume we can manage these PCIe features.  Some systems may
> > > +	 * reserve these for use by the platform itself, e.g., an ACPI BIOS
> > > +	 * may implement its own AER handling and use _OSC to prevent the
> > > +	 * OS from interfering.
> > > +	 */
> > > +	bridge->use_aer = 1;
> > > +	bridge->use_hotplug = 1;
> > > +	bridge->use_pme = 1;
> > 
> > If we start out with enabled maybe these should be disable_foo flags
> > instead?
> 
> I went back and forth on that.  "disable_foo" is nice because the
> default value is correct (zero means enabled).  But then you end up
> with things like:
> 
>   if (pcie_ports_native || !host->disable_hotplug)
> 
> where the "!host->disable_hotplug" is a double negative, and I have a
> really hard time reading that.

native_hotplug or, if you want it reversed, platform_hotplug
(or firmware_hotplug?) might improve readability.

Thanks,

Lukas
Bjorn Helgaas March 19, 2018, 6:37 p.m. UTC | #4 
On Mon, Mar 12, 2018 at 03:20:59PM +0100, Lukas Wunner wrote:
> On Mon, Mar 12, 2018 at 09:03:16AM -0500, Bjorn Helgaas wrote:
> > On Mon, Mar 12, 2018 at 01:04:02AM -0700, Christoph Hellwig wrote:
> > > > +	 * We assume we can manage these PCIe features.  Some systems may
> > > > +	 * reserve these for use by the platform itself, e.g., an ACPI BIOS
> > > > +	 * may implement its own AER handling and use _OSC to prevent the
> > > > +	 * OS from interfering.
> > > > +	 */
> > > > +	bridge->use_aer = 1;
> > > > +	bridge->use_hotplug = 1;
> > > > +	bridge->use_pme = 1;
> > > 
> > > If we start out with enabled maybe these should be disable_foo flags
> > > instead?
> > 
> > I went back and forth on that.  "disable_foo" is nice because the
> > default value is correct (zero means enabled).  But then you end up
> > with things like:
> > 
> >   if (pcie_ports_native || !host->disable_hotplug)
> > 
> > where the "!host->disable_hotplug" is a double negative, and I have a
> > really hard time reading that.
> 
> native_hotplug or, if you want it reversed, platform_hotplug
> (or firmware_hotplug?) might improve readability.

Thanks, I like that.  I renamed them to "native_aer", etc.
David Woodhouse May 7, 2019, noon UTC | #5 
On Fri, 2018-03-09 at 13:00 -0600, Bjorn Helgaas wrote:
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -540,6 +540,16 @@ struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
>         INIT_LIST_HEAD(&bridge->windows);
>         bridge->dev.release = pci_release_host_bridge_dev;
>  
> +       /*
> +        * We assume we can manage these PCIe features.  Some systems may
> +        * reserve these for use by the platform itself, e.g., an ACPI BIOS
> +        * may implement its own AER handling and use _OSC to prevent the
> +        * OS from interfering.
> +        */
> +       bridge->use_aer = 1;
> +       bridge->use_hotplug = 1;
> +       bridge->use_pme = 1;
> +
>         return bridge;
>  }
>  EXPORT_SYMBOL(pci_alloc_host_bridge);

Is there a good reason why you've done this only for
pci_alloc_host_bridge() and not also for devm_pci_alloc_host_bridge()? 

In fact, perhaps the devm version should actually call
pci_alloc_host_bridge() and then just override the release method?
Bjorn Helgaas May 7, 2019, 12:49 p.m. UTC | #6 
On Tue, May 07, 2019 at 01:00:03PM +0100, David Woodhouse wrote:
> On Fri, 2018-03-09 at 13:00 -0600, Bjorn Helgaas wrote:
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -540,6 +540,16 @@ struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
> >         INIT_LIST_HEAD(&bridge->windows);
> >         bridge->dev.release = pci_release_host_bridge_dev;
> >  
> > +       /*
> > +        * We assume we can manage these PCIe features.  Some systems may
> > +        * reserve these for use by the platform itself, e.g., an ACPI BIOS
> > +        * may implement its own AER handling and use _OSC to prevent the
> > +        * OS from interfering.
> > +        */
> > +       bridge->use_aer = 1;
> > +       bridge->use_hotplug = 1;
> > +       bridge->use_pme = 1;
> > +
> >         return bridge;
> >  }
> >  EXPORT_SYMBOL(pci_alloc_host_bridge);
> 
> Is there a good reason why you've done this only for
> pci_alloc_host_bridge() and not also for devm_pci_alloc_host_bridge()? 

No good reason; I just screwed up.  Should be fixed in v5.2 (and marked for
stable):

https://lore.kernel.org/linux-pci/20190318160718.10925-1-jean-philippe.brucker@arm.com

Sorry about that.

Bjorn
David Woodhouse May 7, 2019, 1:02 p.m. UTC | #7 
On Tue, 2019-05-07 at 07:49 -0500, Bjorn Helgaas wrote:
> No good reason; I just screwed up.  Should be fixed in v5.2 (and marked for
> stable):
> 
> https://lore.kernel.org/linux-pci/20190318160718.10925-1-jean-philippe.brucker@arm.com

Aha, thanks. And I see 'dwc: Use devm_pci_alloc_host_bridge()' in there
too, which was the reason we were staring at it in the first place.

That's actually fixing a leak in the error path, not just simplifying
it, and might want tagging for stable too if it's realistic that
devm_of_pci_get_host_bridge_resources() would ever actually fail.
Bjorn Helgaas May 7, 2019, 2:07 p.m. UTC | #8 
On Tue, May 07, 2019 at 02:02:34PM +0100, David Woodhouse wrote:
> On Tue, 2019-05-07 at 07:49 -0500, Bjorn Helgaas wrote:
> > No good reason; I just screwed up.  Should be fixed in v5.2 (and marked for
> > stable):
> > 
> > https://lore.kernel.org/linux-pci/20190318160718.10925-1-jean-philippe.brucker@arm.com
> 
> Aha, thanks. And I see 'dwc: Use devm_pci_alloc_host_bridge()' in there
> too, which was the reason we were staring at it in the first place.
> 
> That's actually fixing a leak in the error path, not just simplifying
> it, and might want tagging for stable too if it's realistic that
> devm_of_pci_get_host_bridge_resources() would ever actually fail.

OK, IIUC, you're proposing this, where I added the stable tag to "dwc:
Use devm_pci_alloc_host_bridge()":

  http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=e6fdd3bf5aec

Lorenzo, just FYI, I cherry-picked remotes/lorenzo/pci/dwc to a pci/dwc
branch to add that tag.

Bjorn
David Woodhouse May 8, 2019, 6:45 a.m. UTC | #9 
On Tue, 2019-05-07 at 09:07 -0500, Bjorn Helgaas wrote:
> On Tue, May 07, 2019 at 02:02:34PM +0100, David Woodhouse wrote:
> > On Tue, 2019-05-07 at 07:49 -0500, Bjorn Helgaas wrote:
> > > No good reason; I just screwed up.  Should be fixed in v5.2 (and marked for
> > > stable):
> > > 
> > > https://lore.kernel.org/linux-pci/20190318160718.10925-1-jean-philippe.brucker@arm.com
> > 
> > Aha, thanks. And I see 'dwc: Use devm_pci_alloc_host_bridge()' in there
> > too, which was the reason we were staring at it in the first place.
> > 
> > That's actually fixing a leak in the error path, not just simplifying
> > it, and might want tagging for stable too if it's realistic that
> > devm_of_pci_get_host_bridge_resources() would ever actually fail.
> 
> OK, IIUC, you're proposing this, where I added the stable tag to "dwc:
> Use devm_pci_alloc_host_bridge()":
> 
>   http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=e6fdd3bf5aec

Indeed. Thanks.
diff mbox series

Patch

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 6fc204a52493..65ebefb99815 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -871,6 +871,7 @@  struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	struct acpi_device *device = root->device;
 	int node = acpi_get_node(device->handle);
 	struct pci_bus *bus;
+	struct pci_host_bridge *host_bridge;
 
 	info->root = root;
 	info->bridge = device;
@@ -895,9 +896,17 @@  struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	if (!bus)
 		goto out_release_info;
 
+	host_bridge = to_pci_host_bridge(bus->bridge);
+	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
+		host_bridge->use_hotplug = 0;
+	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
+		host_bridge->use_aer = 0;
+	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
+		host_bridge->use_pme = 0;
+
 	pci_scan_child_bus(bus);
-	pci_set_host_bridge_release(to_pci_host_bridge(bus->bridge),
-				    acpi_pci_root_release_info, info);
+	pci_set_host_bridge_release(host_bridge, acpi_pci_root_release_info,
+				    info);
 	if (node != NUMA_NO_NODE)
 		dev_printk(KERN_DEBUG, &bus->dev, "on NUMA node %d\n", node);
 	return bus;
diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index e01c10c97b95..11fb633b866c 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -7,7 +7,6 @@ 
 obj-$(CONFIG_PCIEASPM)		+= aspm.o
 
 pcieportdrv-y			:= portdrv_core.o portdrv_pci.o
-pcieportdrv-$(CONFIG_ACPI)	+= portdrv_acpi.o
 
 obj-$(CONFIG_PCIEPORTBUS)	+= pcieportdrv.o
 
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index 7bfd75f9197b..ed84e767085f 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -123,15 +123,4 @@  static inline bool pcie_pme_no_msi(void) { return false; }
 static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {}
 #endif /* !CONFIG_PCIE_PME */
 
-#ifdef CONFIG_ACPI
-void pcie_port_acpi_setup(struct pci_dev *port, int *mask);
-
-static inline void pcie_port_platform_notify(struct pci_dev *port, int *mask)
-{
-	pcie_port_acpi_setup(port, mask);
-}
-#else /* !CONFIG_ACPI */
-static inline void pcie_port_platform_notify(struct pci_dev *port, int *mask){}
-#endif /* !CONFIG_ACPI */
-
 #endif /* _PORTDRV_H_ */
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index bf851da97947..589960fdd8a8 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -206,19 +206,20 @@  static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
  */
 static int get_port_device_capability(struct pci_dev *dev)
 {
+	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
+	bool native;
 	int services = 0;
-	int cap_mask = 0;
 
-	cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP;
-	if (pci_aer_available())
-		cap_mask |= PCIE_PORT_SERVICE_AER | PCIE_PORT_SERVICE_DPC;
-
-	if (pcie_ports_auto)
-		pcie_port_platform_notify(dev, &cap_mask);
+	/*
+	 * If the user specified "pcie_ports=native", use the PCIe services
+	 * regardless of whether the platform has given us permission.  On
+	 * ACPI systems, this means we ignore _OSC.
+	 */
+	native = !pcie_ports_auto;
 
-	/* Hot-Plug Capable */
-	if ((cap_mask & PCIE_PORT_SERVICE_HP) && dev->is_hotplug_bridge) {
+	if (dev->is_hotplug_bridge && (native || host->use_hotplug)) {
 		services |= PCIE_PORT_SERVICE_HP;
+
 		/*
 		 * Disable hot-plug interrupts in case they have been enabled
 		 * by the BIOS and the hot-plug service driver is not loaded.
@@ -226,20 +227,27 @@  static int get_port_device_capability(struct pci_dev *dev)
 		pcie_capability_clear_word(dev, PCI_EXP_SLTCTL,
 			  PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE);
 	}
-	/* AER capable */
-	if ((cap_mask & PCIE_PORT_SERVICE_AER)
-	    && pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR)) {
+
+	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR) &&
+	    pci_aer_available() && (native || host->use_aer)) {
 		services |= PCIE_PORT_SERVICE_AER;
+
 		/*
 		 * Disable AER on this port in case it's been enabled by the
 		 * BIOS (the AER service driver will enable it when necessary).
 		 */
 		pci_disable_pcie_error_reporting(dev);
 	}
-	/* Root ports are capable of generating PME too */
-	if ((cap_mask & PCIE_PORT_SERVICE_PME)
-	    && pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
+
+	/*
+	 * Root ports are capable of generating PME too.  Root Complex
+	 * Event Collectors can also generate PMEs, but we don't handle
+	 * those yet.
+	 */
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
+	    (native || host->use_pme)) {
 		services |= PCIE_PORT_SERVICE_PME;
+
 		/*
 		 * Disable PME interrupt on this port in case it's been enabled
 		 * by the BIOS (the PME service driver will enable it when
@@ -247,7 +255,9 @@  static int get_port_device_capability(struct pci_dev *dev)
 		 */
 		pcie_pme_interrupt_enable(dev, false);
 	}
-	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC))
+
+	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
+	    pci_aer_available())
 		services |= PCIE_PORT_SERVICE_DPC;
 
 	return services;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ef5377438a1e..839fb0059900 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -540,6 +540,16 @@  struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
 	INIT_LIST_HEAD(&bridge->windows);
 	bridge->dev.release = pci_release_host_bridge_dev;
 
+	/*
+	 * We assume we can manage these PCIe features.  Some systems may
+	 * reserve these for use by the platform itself, e.g., an ACPI BIOS
+	 * may implement its own AER handling and use _OSC to prevent the
+	 * OS from interfering.
+	 */
+	bridge->use_aer = 1;
+	bridge->use_hotplug = 1;
+	bridge->use_pme = 1;
+
 	return bridge;
 }
 EXPORT_SYMBOL(pci_alloc_host_bridge);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 024a1beda008..40aec7a6fdd9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -469,6 +469,9 @@  struct pci_host_bridge {
 	struct msi_controller *msi;
 	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
 	unsigned int	no_ext_tags:1;		/* No Extended Tags */
+	unsigned int	use_aer:1;		/* OS may use PCIe AER */
+	unsigned int	use_hotplug:1;		/* OS may use PCIe hotplug */
+	unsigned int	use_pme:1;		/* OS may use PCIe PME */
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
 			const struct resource *res,