[PATCH v1 0/9] PCI: Simplify PCIe port driver

[PATCH v1 0/9] PCI: Simplify PCIe port driver

[Bjorn Helgaas]

This is an attempt to move a few things out of the port driver.

Patches 1-2 move a workaround for a BIOS PME issue from the port driver to
the PCI core, so it doesn't depend on CONFIG_PCIEPORTBUS.

Patch 3 extends that workaround so it works for Root Complex Event
Collectors.  I haven't seen reports of this being a problem, but I think we
should handle Event Collector PMEs the same as Root Port PMEs.

Patch 4 disables the port driver completely for "pcie_ports=compat".  We
used to register the driver, claim port devices, enable them, etc., as part
of supporting the above BIOS workaround.

Patch 5 removes a port driver link order dependency.

Patch 6 removes the unused VC service.

Patch 7 simplifies the _OSC code path by keeping more of the details in the
ACPI pci_root.c driver.

Patch 8 removes an unnecessary #include.

Patch 9 removes the "pcie_hp=nomsi" parameter.  This was added to work
around an issue when shutting down devices, but a later patch fixed the
root cause, and I don't think we need such a specific parameter any more
(we still have "pci=nomsi").

---

Bjorn Helgaas (9):
      PCI/PM: Move pcie_clear_root_pme_status() to core
      PCI/PM: Clear PCIe PME Status bit in core, not PCIe port driver
      PCI/PM: Clear PCIe PME Status bit for Root Complex Event Collectors
      PCI/portdrv: Disable port driver in compat mode
      PCI/portdrv: Remove pcie_port_bus_type link order dependency
      PCI/portdrv: Remove unused PCIE_PORT_SERVICE_VC
      PCI/portdrv: Simplify PCIe feature permission checking
      PCI/portdrv: Remove unnecessary include of <linux/pci-aspm.h>
      PCI/portdrv: Remove "pcie_hp=nomsi" kernel parameter


 Documentation/admin-guide/kernel-parameters.txt |    4 -
 drivers/acpi/pci_root.c                         |   13 +++-
 drivers/pci/pci-driver.c                        |   60 ++++++++++++++++++
 drivers/pci/pci.c                               |    9 +++
 drivers/pci/pci.h                               |    1 
 drivers/pci/pcie/Makefile                       |    3 -
 drivers/pci/pcie/portdrv.h                      |   27 --------
 drivers/pci/pcie/portdrv_acpi.c                 |    2 -
 drivers/pci/pcie/portdrv_bus.c                  |   56 -----------------
 drivers/pci/pcie/portdrv_core.c                 |   77 ++++++++++-------------
 drivers/pci/pcie/portdrv_pci.c                  |   40 +-----------
 drivers/pci/probe.c                             |   10 +++
 include/linux/pci.h                             |    3 +
 include/linux/pcieport_if.h                     |    4 -
 14 files changed, 131 insertions(+), 178 deletions(-)
 delete mode 100644 drivers/pci/pcie/portdrv_bus.c

[PATCH v1 1/9] PCI/PM: Move pcie_clear_root_pme_status() to core

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Move pcie_clear_root_pme_status() from the port driver to the PCI core so
it will be available even when the port driver isn't present.  No
functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c              |    9 +++++++++
 drivers/pci/pci.h              |    1 +
 drivers/pci/pcie/portdrv.h     |    2 --
 drivers/pci/pcie/portdrv_pci.c |    9 ---------
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f6a4dd10d9b0..120e3393fc35 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1683,6 +1683,15 @@ int pci_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state)
 }
 EXPORT_SYMBOL_GPL(pci_set_pcie_reset_state);
 
+/**
+ * pcie_clear_root_pme_status - Clear root port PME interrupt status.
+ * @dev: PCIe root port or event collector.
+ */
+void pcie_clear_root_pme_status(struct pci_dev *dev)
+{
+	pcie_capability_set_dword(dev, PCI_EXP_RTSTA, PCI_EXP_RTSTA_PME);
+}
+
 /**
  * pci_check_pme_status - Check if given device has generated PME.
  * @dev: Device to check.
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fcd81911b127..813ca2c895d8 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -71,6 +71,7 @@ void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 void pci_power_up(struct pci_dev *dev);
 void pci_disable_enabled_device(struct pci_dev *dev);
 int pci_finish_runtime_suspend(struct pci_dev *dev);
+void pcie_clear_root_pme_status(struct pci_dev *dev);
 int __pci_pme_wakeup(struct pci_dev *dev, void *ign);
 void pci_pme_restore(struct pci_dev *dev);
 bool pci_dev_keep_suspended(struct pci_dev *dev);
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index a854bc569117..a4fc44d52206 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -34,8 +34,6 @@ void pcie_port_bus_unregister(void);
 
 struct pci_dev;
 
-void pcie_clear_root_pme_status(struct pci_dev *dev);
-
 #ifdef CONFIG_HOTPLUG_PCI_PCIE
 extern bool pciehp_msi_disabled;
 
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index fb1c1bb87316..4413dd85e923 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -50,15 +50,6 @@ __setup("pcie_ports=", pcie_port_setup);
 
 /* global data */
 
-/**
- * pcie_clear_root_pme_status - Clear root port PME interrupt status.
- * @dev: PCIe root port or event collector.
- */
-void pcie_clear_root_pme_status(struct pci_dev *dev)
-{
-	pcie_capability_set_dword(dev, PCI_EXP_RTSTA, PCI_EXP_RTSTA_PME);
-}
-
 static int pcie_portdrv_restore_config(struct pci_dev *dev)
 {
 	int retval;

[PATCH v1 2/9] PCI/PM: Clear PCIe PME Status bit in core, not PCIe port driver

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

fe31e69740ed ("PCI/PCIe: Clear Root PME Status bits early during system
resume") added a .resume_noirq() callback to the PCIe port driver to clear
the PME Status bit during resume to work around a BIOS issue.

The BIOS evidently enabled PME interrupts for ACPI-based runtime wakeups
but did not clear the PME Status bit during resume, which meant PMEs after
resume did not trigger interrupts because PME Status did not transition
from cleared to set.

The fix was in the PCIe port driver, so it worked when CONFIG_PCIEPORTBUS
was set.  But I think we *always* want the fix because the platform may use
PME interrupts even if Linux is built without the PCIe port driver.

Move the fix from the port driver to the PCI core so we can work around
this "PME doesn't work after waking from a sleep state" issue regardless of
CONFIG_PCIEPORTBUS.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci-driver.c       |   14 ++++++++++++++
 drivers/pci/pcie/portdrv_pci.c |   15 ---------------
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 3bed6beda051..bf0704b75f79 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -525,6 +525,18 @@ static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
 	pci_fixup_device(pci_fixup_resume_early, pci_dev);
 }
 
+static void pcie_resume_early(struct pci_dev *pci_dev)
+{
+	/*
+	 * Some BIOSes forget to clear Root PME Status bits after system wakeup
+	 * which breaks ACPI-based runtime wakeup on PCI Express, so clear those
+	 * bits now just in case (shouldn't hurt).
+	 */
+	if (pci_is_pcie(pci_dev) &&
+	    pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
+		pcie_clear_root_pme_status(pci_dev);
+}
+
 /*
  * Default "suspend" method for devices that have no driver provided suspend,
  * or not even a driver at all (second part).
@@ -873,6 +885,8 @@ static int pci_pm_resume_noirq(struct device *dev)
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
 
+	pcie_resume_early(pci_dev);
+
 	if (drv && drv->pm && drv->pm->resume_noirq)
 		error = drv->pm->resume_noirq(dev);
 
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 4413dd85e923..f91afd09e356 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -62,20 +62,6 @@ static int pcie_portdrv_restore_config(struct pci_dev *dev)
 }
 
 #ifdef CONFIG_PM
-static int pcie_port_resume_noirq(struct device *dev)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	/*
-	 * Some BIOSes forget to clear Root PME Status bits after system wakeup
-	 * which breaks ACPI-based runtime wakeup on PCI Express, so clear those
-	 * bits now just in case (shouldn't hurt).
-	 */
-	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)
-		pcie_clear_root_pme_status(pdev);
-	return 0;
-}
-
 static int pcie_port_runtime_suspend(struct device *dev)
 {
 	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
@@ -103,7 +89,6 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.thaw		= pcie_port_device_resume,
 	.poweroff	= pcie_port_device_suspend,
 	.restore	= pcie_port_device_resume,
-	.resume_noirq	= pcie_port_resume_noirq,
 	.runtime_suspend = pcie_port_runtime_suspend,
 	.runtime_resume	= pcie_port_runtime_resume,
 	.runtime_idle	= pcie_port_runtime_idle,

[PATCH v1 3/9] PCI/PM: Clear PCIe PME Status bit for Root Complex Event Collectors

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Per PCIe r4.0, sec 6.1.6, Root Complex Event Collectors can generate PME
interrupts on behalf of Root Complex Integrated Endpoints.

Linux does not currently enable PME interrupts from RC Event Collectors,
but fe31e69740ed ("PCI/PCIe: Clear Root PME Status bits early during system
resume") suggests PME interrupts may be enabled by the platform for ACPI-
based runtime wakeup.

Clear the PCIe PME Status bit for Root Complex Event Collectors during
resume, just like we already do for Root Ports.

If the BIOS enables PME interrupts for an event collector and neglects to
clear the status bit on resume, this change should fix the same bug as
fe31e69740ed (PMEs not working after waking from a sleep state), but for
Root Complex Integrated Endpoints.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci-driver.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index bf0704b75f79..38ee7c8b4d1a 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -533,7 +533,8 @@ static void pcie_resume_early(struct pci_dev *pci_dev)
 	 * bits now just in case (shouldn't hurt).
 	 */
 	if (pci_is_pcie(pci_dev) &&
-	    pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
+	    (pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT ||
+	     pci_pcie_type(pci_dev) == PCI_EXP_TYPE_RC_EC))
 		pcie_clear_root_pme_status(pci_dev);
 }
 

[PATCH v1 4/9] PCI/portdrv: Disable port driver in compat mode

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

The "pcie_ports=compat" kernel parameter sets pcie_ports_disabled, which is
intended to disable the PCIe port driver.  But even when it was disabled,
we registered pcie_portdriver so we could work around a BIOS PME issue (see
fe31e69740ed ("PCI/PCIe: Clear Root PME Status bits early during system
resume")).

Registering the driver meant that the pcie_portdrv_probe() path called
pci_enable_device(), pci_save_state(), pm_runtime_set_autosuspend_delay(),
pm_runtime_use_autosuspend(), etc., even when the driver was disabled.

We've since moved the BIOS PME workaround from the port driver to the core,
so stop registering the PCIe port driver in compat mode.

This means "pcie_ports=compat" will now be basically the same as turning
off CONFIG_PCIEPORTBUS completely.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/portdrv_core.c |    3 ---
 drivers/pci/pcie/portdrv_pci.c  |    2 +-
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index ef3bad4ad010..9db77c683732 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -212,9 +212,6 @@ static int get_port_device_capability(struct pci_dev *dev)
 	int services = 0;
 	int cap_mask = 0;
 
-	if (pcie_ports_disabled)
-		return 0;
-
 	cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP
 			| PCIE_PORT_SERVICE_VC;
 	if (pci_aer_available())
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index f91afd09e356..c08ebd237242 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -262,7 +262,7 @@ static int __init pcie_portdrv_init(void)
 	int retval;
 
 	if (pcie_ports_disabled)
-		return pci_register_driver(&pcie_portdriver);
+		return -EACCES;
 
 	dmi_check_system(pcie_portdrv_dmi_table);
 

[PATCH v1 5/9] PCI/portdrv: Remove pcie_port_bus_type link order dependency

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

The pcie_port_bus_type must be registered before drivers that depend on it
can be registered.  Those drivers include:

  pcied_init()                # PCIe native hotplug driver
  aer_service_init()          # AER driver
  dpc_service_init()          # DPC driver
  pcie_pme_service_init()     # PME driver

Previously we registered pcie_port_bus_type from pcie_portdrv_init(), a
device_initcall.  The callers of pcie_port_service_register() (above) are
also device_initcalls.  This is fragile because the device_initcall
ordering depends on link order, which is not explicit.

Register pcie_port_bus_type from pci_driver_init() along with pci_bus_type.
This removes the link order dependency between portdrv and the pciehp, AER,
DPC, and PCIe PME drivers.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci-driver.c       |   45 +++++++++++++++++++++++++++++++-
 drivers/pci/pcie/Makefile      |    2 +
 drivers/pci/pcie/portdrv_bus.c |   56 ----------------------------------------
 drivers/pci/pcie/portdrv_pci.c |   13 +--------
 4 files changed, 46 insertions(+), 70 deletions(-)
 delete mode 100644 drivers/pci/pcie/portdrv_bus.c

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 38ee7c8b4d1a..4db85a0faf34 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/pci.h>
+#include <linux/pcieport_if.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/device.h>
@@ -19,6 +20,7 @@
 #include <linux/suspend.h>
 #include <linux/kexec.h>
 #include "pci.h"
+#include "pcie/portdrv.h"
 
 struct pci_dynid {
 	struct list_head node;
@@ -1553,8 +1555,49 @@ struct bus_type pci_bus_type = {
 };
 EXPORT_SYMBOL(pci_bus_type);
 
+#ifdef CONFIG_PCIEPORTBUS
+static int pcie_port_bus_match(struct device *dev, struct device_driver *drv)
+{
+	struct pcie_device *pciedev;
+	struct pcie_port_service_driver *driver;
+
+	if (drv->bus != &pcie_port_bus_type || dev->bus != &pcie_port_bus_type)
+		return 0;
+
+	pciedev = to_pcie_device(dev);
+	driver = to_service_driver(drv);
+
+	if (driver->service != pciedev->service)
+		return 0;
+
+	if ((driver->port_type != PCIE_ANY_PORT) &&
+	    (driver->port_type != pci_pcie_type(pciedev->port)))
+		return 0;
+
+	return 1;
+}
+
+struct bus_type pcie_port_bus_type = {
+	.name		= "pci_express",
+	.match		= pcie_port_bus_match,
+};
+EXPORT_SYMBOL_GPL(pcie_port_bus_type);
+#endif
+
 static int __init pci_driver_init(void)
 {
-	return bus_register(&pci_bus_type);
+	int ret;
+
+	ret = bus_register(&pci_bus_type);
+	if (ret)
+		return ret;
+
+#ifdef CONFIG_PCIEPORTBUS
+	ret = bus_register(&pcie_port_bus_type);
+	if (ret)
+		return ret;
+#endif
+
+	return 0;
 }
 postcore_initcall(pci_driver_init);
diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index 223e4c34c29a..e01c10c97b95 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -6,7 +6,7 @@
 # Build PCI Express ASPM if needed
 obj-$(CONFIG_PCIEASPM)		+= aspm.o
 
-pcieportdrv-y			:= portdrv_core.o portdrv_pci.o portdrv_bus.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_bus.c b/drivers/pci/pcie/portdrv_bus.c
deleted file mode 100644
index f0fba552a0e2..000000000000
--- a/drivers/pci/pcie/portdrv_bus.c
+++ /dev/null
@@ -1,56 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * File:	portdrv_bus.c
- * Purpose:	PCI Express Port Bus Driver's Bus Overloading Functions
- *
- * Copyright (C) 2004 Intel
- * Copyright (C) Tom Long Nguyen (tom.l.nguyen@intel.com)
- */
-
-#include <linux/module.h>
-#include <linux/pci.h>
-#include <linux/kernel.h>
-#include <linux/errno.h>
-#include <linux/pm.h>
-
-#include <linux/pcieport_if.h>
-#include "portdrv.h"
-
-static int pcie_port_bus_match(struct device *dev, struct device_driver *drv);
-
-struct bus_type pcie_port_bus_type = {
-	.name		= "pci_express",
-	.match		= pcie_port_bus_match,
-};
-EXPORT_SYMBOL_GPL(pcie_port_bus_type);
-
-static int pcie_port_bus_match(struct device *dev, struct device_driver *drv)
-{
-	struct pcie_device *pciedev;
-	struct pcie_port_service_driver *driver;
-
-	if (drv->bus != &pcie_port_bus_type || dev->bus != &pcie_port_bus_type)
-		return 0;
-
-	pciedev = to_pcie_device(dev);
-	driver = to_service_driver(drv);
-
-	if (driver->service != pciedev->service)
-		return 0;
-
-	if ((driver->port_type != PCIE_ANY_PORT) &&
-	    (driver->port_type != pci_pcie_type(pciedev->port)))
-		return 0;
-
-	return 1;
-}
-
-int pcie_port_bus_register(void)
-{
-	return bus_register(&pcie_port_bus_type);
-}
-
-void pcie_port_bus_unregister(void)
-{
-	bus_unregister(&pcie_port_bus_type);
-}
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index c08ebd237242..9475886eeb62 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -259,22 +259,11 @@ static const struct dmi_system_id pcie_portdrv_dmi_table[] __initconst = {
 
 static int __init pcie_portdrv_init(void)
 {
-	int retval;
-
 	if (pcie_ports_disabled)
 		return -EACCES;
 
 	dmi_check_system(pcie_portdrv_dmi_table);
 
-	retval = pcie_port_bus_register();
-	if (retval) {
-		printk(KERN_WARNING "PCIE: bus_register error: %d\n", retval);
-		goto out;
-	}
-	retval = pci_register_driver(&pcie_portdriver);
-	if (retval)
-		pcie_port_bus_unregister();
- out:
-	return retval;
+	return pci_register_driver(&pcie_portdriver);
 }
 device_initcall(pcie_portdrv_init);

[PATCH v1 6/9] PCI/portdrv: Remove unused PCIE_PORT_SERVICE_VC

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

No driver registers for PCIE_PORT_SERVICE_VC, so remove it.

This removes the VC "service" files from /sys/bus/pci_express/devices,
e.g., 0000:07:00.0:pcie108, 0000:08:04.0:pcie208 (all the files that
contained "8" as the last digit of the "pcieXXX" part).  The port driver
created these files for PCIe port devices that have a VC Capability.

Since this reduces PCIE_PORT_DEVICE_MAXSERVICES and moves DPC down into the
spot where VC used to be, the DPC sysfs files will now be named "pcieXX8".
I don't think there's anything useful userspace can do with those files, so
I hope nobody cares about these filenames.

There is no VC driver that calls pcie_port_service_register(), so there
never was a /sys/bus/pci_express/drivers/vc directory.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/portdrv.h      |    2 +-
 drivers/pci/pcie/portdrv_acpi.c |    2 +-
 drivers/pci/pcie/portdrv_core.c |   14 ++++----------
 include/linux/pcieport_if.h     |    4 +---
 4 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index a4fc44d52206..749d200936d9 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -12,7 +12,7 @@
 
 #include <linux/compiler.h>
 
-#define PCIE_PORT_DEVICE_MAXSERVICES   5
+#define PCIE_PORT_DEVICE_MAXSERVICES   4
 /*
  * The PCIe Capability Interrupt Message Number (PCIe r3.1, sec 7.8.2) must
  * be one of the first 32 MSI-X entries.  Per PCI r3.0, sec 6.8.3.1, MSI
diff --git a/drivers/pci/pcie/portdrv_acpi.c b/drivers/pci/pcie/portdrv_acpi.c
index 319c94976873..4a1b50867c98 100644
--- a/drivers/pci/pcie/portdrv_acpi.c
+++ b/drivers/pci/pcie/portdrv_acpi.c
@@ -48,7 +48,7 @@ void pcie_port_acpi_setup(struct pci_dev *port, int *srv_mask)
 
 	flags = root->osc_control_set;
 
-	*srv_mask = PCIE_PORT_SERVICE_VC | PCIE_PORT_SERVICE_DPC;
+	*srv_mask = PCIE_PORT_SERVICE_DPC;
 	if (flags & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
 		*srv_mask |= PCIE_PORT_SERVICE_HP;
 	if (flags & OSC_PCI_EXPRESS_PME_CONTROL)
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 9db77c683732..94ce4dc50d1a 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -189,10 +189,8 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
 	if (ret < 0)
 		return -ENODEV;
 
-	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
-		if (i != PCIE_PORT_SERVICE_VC_SHIFT)
-			irqs[i] = pci_irq_vector(dev, 0);
-	}
+	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
+		irqs[i] = pci_irq_vector(dev, 0);
 
 	return 0;
 }
@@ -212,8 +210,7 @@ static int get_port_device_capability(struct pci_dev *dev)
 	int services = 0;
 	int cap_mask = 0;
 
-	cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP
-			| PCIE_PORT_SERVICE_VC;
+	cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP;
 	if (pci_aer_available())
 		cap_mask |= PCIE_PORT_SERVICE_AER | PCIE_PORT_SERVICE_DPC;
 
@@ -240,9 +237,6 @@ static int get_port_device_capability(struct pci_dev *dev)
 		 */
 		pci_disable_pcie_error_reporting(dev);
 	}
-	/* VC support */
-	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_VC))
-		services |= PCIE_PORT_SERVICE_VC;
 	/* Root ports are capable of generating PME too */
 	if ((cap_mask & PCIE_PORT_SERVICE_PME)
 	    && pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
@@ -332,7 +326,7 @@ int pcie_port_device_register(struct pci_dev *dev)
 	 */
 	status = pcie_init_service_irqs(dev, irqs, capabilities);
 	if (status) {
-		capabilities &= PCIE_PORT_SERVICE_VC | PCIE_PORT_SERVICE_HP;
+		capabilities &= PCIE_PORT_SERVICE_HP;
 		if (!capabilities)
 			goto error_disable;
 	}
diff --git a/include/linux/pcieport_if.h b/include/linux/pcieport_if.h
index b69769dbf659..28eb21731db6 100644
--- a/include/linux/pcieport_if.h
+++ b/include/linux/pcieport_if.h
@@ -20,9 +20,7 @@
 #define PCIE_PORT_SERVICE_AER		(1 << PCIE_PORT_SERVICE_AER_SHIFT)
 #define PCIE_PORT_SERVICE_HP_SHIFT	2	/* Native Hotplug */
 #define PCIE_PORT_SERVICE_HP		(1 << PCIE_PORT_SERVICE_HP_SHIFT)
-#define PCIE_PORT_SERVICE_VC_SHIFT	3	/* Virtual Channel */
-#define PCIE_PORT_SERVICE_VC		(1 << PCIE_PORT_SERVICE_VC_SHIFT)
-#define PCIE_PORT_SERVICE_DPC_SHIFT	4	/* Downstream Port Containment */
+#define PCIE_PORT_SERVICE_DPC_SHIFT	3	/* Downstream Port Containment */
 #define PCIE_PORT_SERVICE_DPC		(1 << PCIE_PORT_SERVICE_DPC_SHIFT)
 
 struct pcie_device {

[PATCH v1 7/9] PCI/portdrv: Simplify PCIe feature permission checking

[Bjorn Helgaas]

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>
---
 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(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 6fc204a52493..dce53527cdc1 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 & PCIE_PORT_SERVICE_HP))
+		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 749d200936d9..2c19cf9ffea2 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -66,15 +66,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 94ce4dc50d1a..29210e9bfbd3 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -207,19 +207,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.
@@ -227,20 +228,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
@@ -248,7 +256,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,

[PATCH v1 8/9] PCI/portdrv: Remove unnecessary include of <linux/pci-aspm.h>

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

portdrv_pci.c doesn't use anything from <linux/pci-aspm.h>.  Remove the
include of it.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/portdrv_pci.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 9475886eeb62..d12b58db18a1 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -18,7 +18,6 @@
 #include <linux/pcieport_if.h>
 #include <linux/aer.h>
 #include <linux/dmi.h>
-#include <linux/pci-aspm.h>
 
 #include "../pci.h"
 #include "portdrv.h"

[PATCH v1 9/9] PCI/portdrv: Remove "pcie_hp=nomsi" kernel parameter

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

7570a333d8b0 ("PCI: Add pcie_hp=nomsi to disable MSI/MSI-X for pciehp
driver") added the "pcie_hp=nomsi" kernel parameter to work around this
error on shutdown:

  irq 16: nobody cared (try booting with the "irqpoll" option)
  Pid: 1081, comm: reboot Not tainted 3.2.0 #1
  ...
  Disabling IRQ #16

This happened on an unspecified system (possibly involving the Integrated
Device Technology, Inc. Device 807f bridge) where "an un-wanted interrupt
is generated when PCI driver switches from MSI/MSI-X to INTx while shutting
down the device."

The implication was that the device was buggy, but it is normal for a
device to use INTx after MSI/MSI-X have been disabled.  The only problem
was that the driver was still attached and it wasn't prepared for INTx
interrupts.  Prarit Bhargava fixed this issue with fda78d7a0ead ("PCI/MSI:
Stop disabling MSI/MSI-X in pci_device_shutdown()").

There is no automated way to set this parameter, so it's not very useful
for distributions or end users.  It's really only useful for debugging, and
we have "pci=nomsi" for that purpose.

Revert 7570a333d8b0 to remove the "pcie_hp=nomsi" parameter.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: MUNEDA Takahiro <muneda.takahiro@jp.fujitsu.com>
CC: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
CC: Prarit Bhargava <prarit@redhat.com>
---
 Documentation/admin-guide/kernel-parameters.txt |    4 ----
 drivers/pci/pcie/portdrv.h                      |   12 ------------
 drivers/pci/pcie/portdrv_core.c                 |   20 +++-----------------
 3 files changed, 3 insertions(+), 33 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..761749562165 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3130,10 +3130,6 @@
 		force	Enable ASPM even on devices that claim not to support it.
 			WARNING: Forcing ASPM on may cause system lockups.
 
-	pcie_hp=	[PCIE] PCI Express Hotplug driver options:
-		nomsi	Do not use MSI for PCI Express Native Hotplug (this
-			makes all PCIe ports use INTx for hotplug services).
-
 	pcie_ports=	[PCIE] PCIe ports handling:
 		auto	Ask the BIOS whether or not to use native PCIe services
 			associated with PCIe ports (PME, hot-plug, AER).  Use
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index 2c19cf9ffea2..87a87cb9f42d 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -34,18 +34,6 @@ void pcie_port_bus_unregister(void);
 
 struct pci_dev;
 
-#ifdef CONFIG_HOTPLUG_PCI_PCIE
-extern bool pciehp_msi_disabled;
-
-static inline bool pciehp_no_msi(void)
-{
-	return pciehp_msi_disabled;
-}
-
-#else  /* !CONFIG_HOTPLUG_PCI_PCIE */
-static inline bool pciehp_no_msi(void) { return false; }
-#endif /* !CONFIG_HOTPLUG_PCI_PCIE */
-
 #ifdef CONFIG_PCIE_PME
 extern bool pcie_pme_msi_disabled;
 
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 29210e9bfbd3..bf9c5c885957 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -21,17 +21,6 @@
 #include "../pci.h"
 #include "portdrv.h"
 
-bool pciehp_msi_disabled;
-
-static int __init pciehp_setup(char *str)
-{
-	if (!strncmp(str, "nomsi", 5))
-		pciehp_msi_disabled = true;
-
-	return 1;
-}
-__setup("pcie_hp=", pciehp_setup);
-
 /**
  * release_pcie_device - free PCI Express port service device structure
  * @dev: Port service device to release
@@ -169,16 +158,13 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
 		irqs[i] = -1;
 
 	/*
-	 * If we support PME or hotplug, but we can't use MSI/MSI-X for
-	 * them, we have to fall back to INTx or other interrupts, e.g., a
-	 * system shared interrupt.
+	 * If we support PME but can't use MSI/MSI-X for it, we have to
+	 * fall back to INTx or other interrupts, e.g., a system shared
+	 * interrupt.
 	 */
 	if ((mask & PCIE_PORT_SERVICE_PME) && pcie_pme_no_msi())
 		goto legacy_irq;
 
-	if ((mask & PCIE_PORT_SERVICE_HP) && pciehp_no_msi())
-		goto legacy_irq;
-
 	/* Try to use MSI-X or MSI if supported */
 	if (pcie_port_enable_irq_vec(dev, irqs, mask) == 0)
 		return 0;

Re: [PATCH v1 1/9] PCI/PM: Move pcie_clear_root_pme_status() to core

[Rafael J. Wysocki]

On Wednesday, March 7, 2018 7:13:29 AM CET Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Move pcie_clear_root_pme_status() from the port driver to the PCI core so
> it will be available even when the port driver isn't present.  No
> functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/pci.c              |    9 +++++++++
>  drivers/pci/pci.h              |    1 +
>  drivers/pci/pcie/portdrv.h     |    2 --
>  drivers/pci/pcie/portdrv_pci.c |    9 ---------
>  4 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f6a4dd10d9b0..120e3393fc35 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1683,6 +1683,15 @@ int pci_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state)
>  }
>  EXPORT_SYMBOL_GPL(pci_set_pcie_reset_state);
>  
> +/**
> + * pcie_clear_root_pme_status - Clear root port PME interrupt status.
> + * @dev: PCIe root port or event collector.
> + */
> +void pcie_clear_root_pme_status(struct pci_dev *dev)
> +{
> +	pcie_capability_set_dword(dev, PCI_EXP_RTSTA, PCI_EXP_RTSTA_PME);
> +}
> +
>  /**
>   * pci_check_pme_status - Check if given device has generated PME.
>   * @dev: Device to check.
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index fcd81911b127..813ca2c895d8 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -71,6 +71,7 @@ void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
>  void pci_power_up(struct pci_dev *dev);
>  void pci_disable_enabled_device(struct pci_dev *dev);
>  int pci_finish_runtime_suspend(struct pci_dev *dev);
> +void pcie_clear_root_pme_status(struct pci_dev *dev);
>  int __pci_pme_wakeup(struct pci_dev *dev, void *ign);
>  void pci_pme_restore(struct pci_dev *dev);
>  bool pci_dev_keep_suspended(struct pci_dev *dev);
> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
> index a854bc569117..a4fc44d52206 100644
> --- a/drivers/pci/pcie/portdrv.h
> +++ b/drivers/pci/pcie/portdrv.h
> @@ -34,8 +34,6 @@ void pcie_port_bus_unregister(void);
>  
>  struct pci_dev;
>  
> -void pcie_clear_root_pme_status(struct pci_dev *dev);
> -
>  #ifdef CONFIG_HOTPLUG_PCI_PCIE
>  extern bool pciehp_msi_disabled;
>  
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index fb1c1bb87316..4413dd85e923 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -50,15 +50,6 @@ __setup("pcie_ports=", pcie_port_setup);
>  
>  /* global data */
>  
> -/**
> - * pcie_clear_root_pme_status - Clear root port PME interrupt status.
> - * @dev: PCIe root port or event collector.
> - */
> -void pcie_clear_root_pme_status(struct pci_dev *dev)
> -{
> -	pcie_capability_set_dword(dev, PCI_EXP_RTSTA, PCI_EXP_RTSTA_PME);
> -}
> -
>  static int pcie_portdrv_restore_config(struct pci_dev *dev)
>  {
>  	int retval;
> 
> 

Re: [PATCH v1 2/9] PCI/PM: Clear PCIe PME Status bit in core, not PCIe port driver

[Rafael J. Wysocki]

On Wednesday, March 7, 2018 7:13:34 AM CET Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> fe31e69740ed ("PCI/PCIe: Clear Root PME Status bits early during system
> resume") added a .resume_noirq() callback to the PCIe port driver to clear
> the PME Status bit during resume to work around a BIOS issue.
> 
> The BIOS evidently enabled PME interrupts for ACPI-based runtime wakeups
> but did not clear the PME Status bit during resume, which meant PMEs after
> resume did not trigger interrupts because PME Status did not transition
> from cleared to set.
> 
> The fix was in the PCIe port driver, so it worked when CONFIG_PCIEPORTBUS
> was set.  But I think we *always* want the fix because the platform may use
> PME interrupts even if Linux is built without the PCIe port driver.
> 
> Move the fix from the port driver to the PCI core so we can work around
> this "PME doesn't work after waking from a sleep state" issue regardless of
> CONFIG_PCIEPORTBUS.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pci-driver.c       |   14 ++++++++++++++
>  drivers/pci/pcie/portdrv_pci.c |   15 ---------------
>  2 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3bed6beda051..bf0704b75f79 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -525,6 +525,18 @@ static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
>  	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>  }
>  
> +static void pcie_resume_early(struct pci_dev *pci_dev)

I'd call it pcie_pme_root_status_cleanup() or similar so it is more clear
what this function is for.

LGTM otherwise.

> +{
> +	/*
> +	 * Some BIOSes forget to clear Root PME Status bits after system wakeup
> +	 * which breaks ACPI-based runtime wakeup on PCI Express, so clear those
> +	 * bits now just in case (shouldn't hurt).
> +	 */
> +	if (pci_is_pcie(pci_dev) &&
> +	    pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
> +		pcie_clear_root_pme_status(pci_dev);
> +}
> +
>  /*
>   * Default "suspend" method for devices that have no driver provided suspend,
>   * or not even a driver at all (second part).
> @@ -873,6 +885,8 @@ static int pci_pm_resume_noirq(struct device *dev)
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume_early(dev);
>  
> +	pcie_resume_early(pci_dev);
> +
>  	if (drv && drv->pm && drv->pm->resume_noirq)
>  		error = drv->pm->resume_noirq(dev);
>  
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 4413dd85e923..f91afd09e356 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -62,20 +62,6 @@ static int pcie_portdrv_restore_config(struct pci_dev *dev)
>  }
>  
>  #ifdef CONFIG_PM
> -static int pcie_port_resume_noirq(struct device *dev)
> -{
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -
> -	/*
> -	 * Some BIOSes forget to clear Root PME Status bits after system wakeup
> -	 * which breaks ACPI-based runtime wakeup on PCI Express, so clear those
> -	 * bits now just in case (shouldn't hurt).
> -	 */
> -	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)
> -		pcie_clear_root_pme_status(pdev);
> -	return 0;
> -}
> -
>  static int pcie_port_runtime_suspend(struct device *dev)
>  {
>  	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
> @@ -103,7 +89,6 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
>  	.thaw		= pcie_port_device_resume,
>  	.poweroff	= pcie_port_device_suspend,
>  	.restore	= pcie_port_device_resume,
> -	.resume_noirq	= pcie_port_resume_noirq,
>  	.runtime_suspend = pcie_port_runtime_suspend,
>  	.runtime_resume	= pcie_port_runtime_resume,
>  	.runtime_idle	= pcie_port_runtime_idle,
> 
> 

Re: [PATCH v1 3/9] PCI/PM: Clear PCIe PME Status bit for Root Complex Event Collectors

[Rafael J. Wysocki]

On Wednesday, March 7, 2018 7:13:40 AM CET Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Per PCIe r4.0, sec 6.1.6, Root Complex Event Collectors can generate PME
> interrupts on behalf of Root Complex Integrated Endpoints.
> 
> Linux does not currently enable PME interrupts from RC Event Collectors,
> but fe31e69740ed ("PCI/PCIe: Clear Root PME Status bits early during system
> resume") suggests PME interrupts may be enabled by the platform for ACPI-
> based runtime wakeup.
> 
> Clear the PCIe PME Status bit for Root Complex Event Collectors during
> resume, just like we already do for Root Ports.
> 
> If the BIOS enables PME interrupts for an event collector and neglects to
> clear the status bit on resume, this change should fix the same bug as
> fe31e69740ed (PMEs not working after waking from a sleep state), but for
> Root Complex Integrated Endpoints.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pci-driver.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index bf0704b75f79..38ee7c8b4d1a 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -533,7 +533,8 @@ static void pcie_resume_early(struct pci_dev *pci_dev)
>  	 * bits now just in case (shouldn't hurt).
>  	 */
>  	if (pci_is_pcie(pci_dev) &&
> -	    pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
> +	    (pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +	     pci_pcie_type(pci_dev) == PCI_EXP_TYPE_RC_EC))
>  		pcie_clear_root_pme_status(pci_dev);
>  }
>  
> 
> 

Reveiwed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Re: [PATCH v1 4/9] PCI/portdrv: Disable port driver in compat mode

[Rafael J. Wysocki]

On Wednesday, March 7, 2018 7:13:45 AM CET Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> The "pcie_ports=compat" kernel parameter sets pcie_ports_disabled, which is
> intended to disable the PCIe port driver.  But even when it was disabled,
> we registered pcie_portdriver so we could work around a BIOS PME issue (see
> fe31e69740ed ("PCI/PCIe: Clear Root PME Status bits early during system
> resume")).
> 
> Registering the driver meant that the pcie_portdrv_probe() path called
> pci_enable_device(), pci_save_state(), pm_runtime_set_autosuspend_delay(),
> pm_runtime_use_autosuspend(), etc., even when the driver was disabled.
> 
> We've since moved the BIOS PME workaround from the port driver to the core,
> so stop registering the PCIe port driver in compat mode.
> 
> This means "pcie_ports=compat" will now be basically the same as turning
> off CONFIG_PCIEPORTBUS completely.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pcie/portdrv_core.c |    3 ---
>  drivers/pci/pcie/portdrv_pci.c  |    2 +-
>  2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index ef3bad4ad010..9db77c683732 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -212,9 +212,6 @@ static int get_port_device_capability(struct pci_dev *dev)
>  	int services = 0;
>  	int cap_mask = 0;
>  
> -	if (pcie_ports_disabled)
> -		return 0;
> -
>  	cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP
>  			| PCIE_PORT_SERVICE_VC;
>  	if (pci_aer_available())
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index f91afd09e356..c08ebd237242 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -262,7 +262,7 @@ static int __init pcie_portdrv_init(void)
>  	int retval;
>  
>  	if (pcie_ports_disabled)
> -		return pci_register_driver(&pcie_portdriver);
> +		return -EACCES;
>  
>  	dmi_check_system(pcie_portdrv_dmi_table);
>  
> 
> 

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Re: [PATCH v1 5/9] PCI/portdrv: Remove pcie_port_bus_type link order dependency

[Rafael J. Wysocki]

On Wednesday, March 7, 2018 7:13:51 AM CET Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> The pcie_port_bus_type must be registered before drivers that depend on it
> can be registered.  Those drivers include:
> 
>   pcied_init()                # PCIe native hotplug driver
>   aer_service_init()          # AER driver
>   dpc_service_init()          # DPC driver
>   pcie_pme_service_init()     # PME driver
> 
> Previously we registered pcie_port_bus_type from pcie_portdrv_init(), a
> device_initcall.  The callers of pcie_port_service_register() (above) are
> also device_initcalls.  This is fragile because the device_initcall
> ordering depends on link order, which is not explicit.
> 
> Register pcie_port_bus_type from pci_driver_init() along with pci_bus_type.
> This removes the link order dependency between portdrv and the pciehp, AER,
> DPC, and PCIe PME drivers.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pci-driver.c       |   45 +++++++++++++++++++++++++++++++-
>  drivers/pci/pcie/Makefile      |    2 +
>  drivers/pci/pcie/portdrv_bus.c |   56 ----------------------------------------
>  drivers/pci/pcie/portdrv_pci.c |   13 +--------
>  4 files changed, 46 insertions(+), 70 deletions(-)
>  delete mode 100644 drivers/pci/pcie/portdrv_bus.c
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 38ee7c8b4d1a..4db85a0faf34 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/pci.h>
> +#include <linux/pcieport_if.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/device.h>
> @@ -19,6 +20,7 @@
>  #include <linux/suspend.h>
>  #include <linux/kexec.h>
>  #include "pci.h"
> +#include "pcie/portdrv.h"
>  
>  struct pci_dynid {
>  	struct list_head node;
> @@ -1553,8 +1555,49 @@ struct bus_type pci_bus_type = {
>  };
>  EXPORT_SYMBOL(pci_bus_type);
>  
> +#ifdef CONFIG_PCIEPORTBUS
> +static int pcie_port_bus_match(struct device *dev, struct device_driver *drv)
> +{
> +	struct pcie_device *pciedev;
> +	struct pcie_port_service_driver *driver;
> +
> +	if (drv->bus != &pcie_port_bus_type || dev->bus != &pcie_port_bus_type)
> +		return 0;
> +
> +	pciedev = to_pcie_device(dev);
> +	driver = to_service_driver(drv);
> +
> +	if (driver->service != pciedev->service)
> +		return 0;
> +
> +	if ((driver->port_type != PCIE_ANY_PORT) &&
> +	    (driver->port_type != pci_pcie_type(pciedev->port)))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +struct bus_type pcie_port_bus_type = {
> +	.name		= "pci_express",
> +	.match		= pcie_port_bus_match,
> +};
> +EXPORT_SYMBOL_GPL(pcie_port_bus_type);
> +#endif
> +
>  static int __init pci_driver_init(void)
>  {
> -	return bus_register(&pci_bus_type);
> +	int ret;
> +
> +	ret = bus_register(&pci_bus_type);
> +	if (ret)
> +		return ret;
> +
> +#ifdef CONFIG_PCIEPORTBUS
> +	ret = bus_register(&pcie_port_bus_type);
> +	if (ret)
> +		return ret;
> +#endif
> +
> +	return 0;
>  }
>  postcore_initcall(pci_driver_init);
> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> index 223e4c34c29a..e01c10c97b95 100644
> --- a/drivers/pci/pcie/Makefile
> +++ b/drivers/pci/pcie/Makefile
> @@ -6,7 +6,7 @@
>  # Build PCI Express ASPM if needed
>  obj-$(CONFIG_PCIEASPM)		+= aspm.o
>  
> -pcieportdrv-y			:= portdrv_core.o portdrv_pci.o portdrv_bus.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_bus.c b/drivers/pci/pcie/portdrv_bus.c
> deleted file mode 100644
> index f0fba552a0e2..000000000000
> --- a/drivers/pci/pcie/portdrv_bus.c
> +++ /dev/null
> @@ -1,56 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * File:	portdrv_bus.c
> - * Purpose:	PCI Express Port Bus Driver's Bus Overloading Functions
> - *
> - * Copyright (C) 2004 Intel
> - * Copyright (C) Tom Long Nguyen (tom.l.nguyen@intel.com)
> - */
> -
> -#include <linux/module.h>
> -#include <linux/pci.h>
> -#include <linux/kernel.h>
> -#include <linux/errno.h>
> -#include <linux/pm.h>
> -
> -#include <linux/pcieport_if.h>
> -#include "portdrv.h"
> -
> -static int pcie_port_bus_match(struct device *dev, struct device_driver *drv);
> -
> -struct bus_type pcie_port_bus_type = {
> -	.name		= "pci_express",
> -	.match		= pcie_port_bus_match,
> -};
> -EXPORT_SYMBOL_GPL(pcie_port_bus_type);
> -
> -static int pcie_port_bus_match(struct device *dev, struct device_driver *drv)
> -{
> -	struct pcie_device *pciedev;
> -	struct pcie_port_service_driver *driver;
> -
> -	if (drv->bus != &pcie_port_bus_type || dev->bus != &pcie_port_bus_type)
> -		return 0;
> -
> -	pciedev = to_pcie_device(dev);
> -	driver = to_service_driver(drv);
> -
> -	if (driver->service != pciedev->service)
> -		return 0;
> -
> -	if ((driver->port_type != PCIE_ANY_PORT) &&
> -	    (driver->port_type != pci_pcie_type(pciedev->port)))
> -		return 0;
> -
> -	return 1;
> -}
> -
> -int pcie_port_bus_register(void)
> -{
> -	return bus_register(&pcie_port_bus_type);
> -}
> -
> -void pcie_port_bus_unregister(void)
> -{
> -	bus_unregister(&pcie_port_bus_type);
> -}
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index c08ebd237242..9475886eeb62 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -259,22 +259,11 @@ static const struct dmi_system_id pcie_portdrv_dmi_table[] __initconst = {
>  
>  static int __init pcie_portdrv_init(void)
>  {
> -	int retval;
> -
>  	if (pcie_ports_disabled)
>  		return -EACCES;
>  
>  	dmi_check_system(pcie_portdrv_dmi_table);
>  
> -	retval = pcie_port_bus_register();
> -	if (retval) {
> -		printk(KERN_WARNING "PCIE: bus_register error: %d\n", retval);
> -		goto out;
> -	}
> -	retval = pci_register_driver(&pcie_portdriver);
> -	if (retval)
> -		pcie_port_bus_unregister();
> - out:
> -	return retval;
> +	return pci_register_driver(&pcie_portdriver);
>  }
>  device_initcall(pcie_portdrv_init);
> 
> 

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Re: [PATCH v1 6/9] PCI/portdrv: Remove unused PCIE_PORT_SERVICE_VC

[Rafael J. Wysocki]

On Wednesday, March 7, 2018 7:13:56 AM CET Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> No driver registers for PCIE_PORT_SERVICE_VC, so remove it.
> 
> This removes the VC "service" files from /sys/bus/pci_express/devices,
> e.g., 0000:07:00.0:pcie108, 0000:08:04.0:pcie208 (all the files that
> contained "8" as the last digit of the "pcieXXX" part).  The port driver
> created these files for PCIe port devices that have a VC Capability.
> 
> Since this reduces PCIE_PORT_DEVICE_MAXSERVICES and moves DPC down into the
> spot where VC used to be, the DPC sysfs files will now be named "pcieXX8".
> I don't think there's anything useful userspace can do with those files, so
> I hope nobody cares about these filenames.
> 
> There is no VC driver that calls pcie_port_service_register(), so there
> never was a /sys/bus/pci_express/drivers/vc directory.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pcie/portdrv.h      |    2 +-
>  drivers/pci/pcie/portdrv_acpi.c |    2 +-
>  drivers/pci/pcie/portdrv_core.c |   14 ++++----------
>  include/linux/pcieport_if.h     |    4 +---
>  4 files changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
> index a4fc44d52206..749d200936d9 100644
> --- a/drivers/pci/pcie/portdrv.h
> +++ b/drivers/pci/pcie/portdrv.h
> @@ -12,7 +12,7 @@
>  
>  #include <linux/compiler.h>
>  
> -#define PCIE_PORT_DEVICE_MAXSERVICES   5
> +#define PCIE_PORT_DEVICE_MAXSERVICES   4
>  /*
>   * The PCIe Capability Interrupt Message Number (PCIe r3.1, sec 7.8.2) must
>   * be one of the first 32 MSI-X entries.  Per PCI r3.0, sec 6.8.3.1, MSI
> diff --git a/drivers/pci/pcie/portdrv_acpi.c b/drivers/pci/pcie/portdrv_acpi.c
> index 319c94976873..4a1b50867c98 100644
> --- a/drivers/pci/pcie/portdrv_acpi.c
> +++ b/drivers/pci/pcie/portdrv_acpi.c
> @@ -48,7 +48,7 @@ void pcie_port_acpi_setup(struct pci_dev *port, int *srv_mask)
>  
>  	flags = root->osc_control_set;
>  
> -	*srv_mask = PCIE_PORT_SERVICE_VC | PCIE_PORT_SERVICE_DPC;
> +	*srv_mask = PCIE_PORT_SERVICE_DPC;
>  	if (flags & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
>  		*srv_mask |= PCIE_PORT_SERVICE_HP;
>  	if (flags & OSC_PCI_EXPRESS_PME_CONTROL)
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 9db77c683732..94ce4dc50d1a 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -189,10 +189,8 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  	if (ret < 0)
>  		return -ENODEV;
>  
> -	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
> -		if (i != PCIE_PORT_SERVICE_VC_SHIFT)
> -			irqs[i] = pci_irq_vector(dev, 0);
> -	}
> +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
> +		irqs[i] = pci_irq_vector(dev, 0);
>  
>  	return 0;
>  }
> @@ -212,8 +210,7 @@ static int get_port_device_capability(struct pci_dev *dev)
>  	int services = 0;
>  	int cap_mask = 0;
>  
> -	cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP
> -			| PCIE_PORT_SERVICE_VC;
> +	cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP;
>  	if (pci_aer_available())
>  		cap_mask |= PCIE_PORT_SERVICE_AER | PCIE_PORT_SERVICE_DPC;
>  
> @@ -240,9 +237,6 @@ static int get_port_device_capability(struct pci_dev *dev)
>  		 */
>  		pci_disable_pcie_error_reporting(dev);
>  	}
> -	/* VC support */
> -	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_VC))
> -		services |= PCIE_PORT_SERVICE_VC;
>  	/* Root ports are capable of generating PME too */
>  	if ((cap_mask & PCIE_PORT_SERVICE_PME)
>  	    && pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> @@ -332,7 +326,7 @@ int pcie_port_device_register(struct pci_dev *dev)
>  	 */
>  	status = pcie_init_service_irqs(dev, irqs, capabilities);
>  	if (status) {
> -		capabilities &= PCIE_PORT_SERVICE_VC | PCIE_PORT_SERVICE_HP;
> +		capabilities &= PCIE_PORT_SERVICE_HP;
>  		if (!capabilities)
>  			goto error_disable;
>  	}
> diff --git a/include/linux/pcieport_if.h b/include/linux/pcieport_if.h
> index b69769dbf659..28eb21731db6 100644
> --- a/include/linux/pcieport_if.h
> +++ b/include/linux/pcieport_if.h
> @@ -20,9 +20,7 @@
>  #define PCIE_PORT_SERVICE_AER		(1 << PCIE_PORT_SERVICE_AER_SHIFT)
>  #define PCIE_PORT_SERVICE_HP_SHIFT	2	/* Native Hotplug */
>  #define PCIE_PORT_SERVICE_HP		(1 << PCIE_PORT_SERVICE_HP_SHIFT)
> -#define PCIE_PORT_SERVICE_VC_SHIFT	3	/* Virtual Channel */
> -#define PCIE_PORT_SERVICE_VC		(1 << PCIE_PORT_SERVICE_VC_SHIFT)
> -#define PCIE_PORT_SERVICE_DPC_SHIFT	4	/* Downstream Port Containment */
> +#define PCIE_PORT_SERVICE_DPC_SHIFT	3	/* Downstream Port Containment */
>  #define PCIE_PORT_SERVICE_DPC		(1 << PCIE_PORT_SERVICE_DPC_SHIFT)
>  
>  struct pcie_device {
> 
> 

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Re: [PATCH v1 7/9] PCI/portdrv: Simplify PCIe feature permission checking

[Rafael J. Wysocki]

On Wednesday, March 7, 2018 7:14:02 AM CET Bjorn Helgaas wrote:
> 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>
> ---
>  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(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 6fc204a52493..dce53527cdc1 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 & PCIE_PORT_SERVICE_HP))
> +		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 749d200936d9..2c19cf9ffea2 100644
> --- a/drivers/pci/pcie/portdrv.h
> +++ b/drivers/pci/pcie/portdrv.h
> @@ -66,15 +66,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 94ce4dc50d1a..29210e9bfbd3 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -207,19 +207,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.
> @@ -227,20 +228,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
> @@ -248,7 +256,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,
> 
> 

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Re: [PATCH v1 8/9] PCI/portdrv: Remove unnecessary include of <linux/pci-aspm.h>

[Rafael J. Wysocki]

On Wednesday, March 7, 2018 7:14:07 AM CET Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> portdrv_pci.c doesn't use anything from <linux/pci-aspm.h>.  Remove the
> include of it.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pcie/portdrv_pci.c |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 9475886eeb62..d12b58db18a1 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -18,7 +18,6 @@
>  #include <linux/pcieport_if.h>
>  #include <linux/aer.h>
>  #include <linux/dmi.h>
> -#include <linux/pci-aspm.h>
>  
>  #include "../pci.h"
>  #include "portdrv.h"
> 
> 

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Re: [PATCH v1 9/9] PCI/portdrv: Remove "pcie_hp=nomsi" kernel parameter

[Rafael J. Wysocki]

On Wednesday, March 7, 2018 7:14:13 AM CET Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> 7570a333d8b0 ("PCI: Add pcie_hp=nomsi to disable MSI/MSI-X for pciehp
> driver") added the "pcie_hp=nomsi" kernel parameter to work around this
> error on shutdown:
> 
>   irq 16: nobody cared (try booting with the "irqpoll" option)
>   Pid: 1081, comm: reboot Not tainted 3.2.0 #1
>   ...
>   Disabling IRQ #16
> 
> This happened on an unspecified system (possibly involving the Integrated
> Device Technology, Inc. Device 807f bridge) where "an un-wanted interrupt
> is generated when PCI driver switches from MSI/MSI-X to INTx while shutting
> down the device."
> 
> The implication was that the device was buggy, but it is normal for a
> device to use INTx after MSI/MSI-X have been disabled.  The only problem
> was that the driver was still attached and it wasn't prepared for INTx
> interrupts.  Prarit Bhargava fixed this issue with fda78d7a0ead ("PCI/MSI:
> Stop disabling MSI/MSI-X in pci_device_shutdown()").
> 
> There is no automated way to set this parameter, so it's not very useful
> for distributions or end users.  It's really only useful for debugging, and
> we have "pci=nomsi" for that purpose.
> 
> Revert 7570a333d8b0 to remove the "pcie_hp=nomsi" parameter.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: MUNEDA Takahiro <muneda.takahiro@jp.fujitsu.com>
> CC: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> CC: Prarit Bhargava <prarit@redhat.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |    4 ----
>  drivers/pci/pcie/portdrv.h                      |   12 ------------
>  drivers/pci/pcie/portdrv_core.c                 |   20 +++-----------------
>  3 files changed, 3 insertions(+), 33 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 1d1d53f85ddd..761749562165 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3130,10 +3130,6 @@
>  		force	Enable ASPM even on devices that claim not to support it.
>  			WARNING: Forcing ASPM on may cause system lockups.
>  
> -	pcie_hp=	[PCIE] PCI Express Hotplug driver options:
> -		nomsi	Do not use MSI for PCI Express Native Hotplug (this
> -			makes all PCIe ports use INTx for hotplug services).
> -
>  	pcie_ports=	[PCIE] PCIe ports handling:
>  		auto	Ask the BIOS whether or not to use native PCIe services
>  			associated with PCIe ports (PME, hot-plug, AER).  Use
> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
> index 2c19cf9ffea2..87a87cb9f42d 100644
> --- a/drivers/pci/pcie/portdrv.h
> +++ b/drivers/pci/pcie/portdrv.h
> @@ -34,18 +34,6 @@ void pcie_port_bus_unregister(void);
>  
>  struct pci_dev;
>  
> -#ifdef CONFIG_HOTPLUG_PCI_PCIE
> -extern bool pciehp_msi_disabled;
> -
> -static inline bool pciehp_no_msi(void)
> -{
> -	return pciehp_msi_disabled;
> -}
> -
> -#else  /* !CONFIG_HOTPLUG_PCI_PCIE */
> -static inline bool pciehp_no_msi(void) { return false; }
> -#endif /* !CONFIG_HOTPLUG_PCI_PCIE */
> -
>  #ifdef CONFIG_PCIE_PME
>  extern bool pcie_pme_msi_disabled;
>  
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 29210e9bfbd3..bf9c5c885957 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -21,17 +21,6 @@
>  #include "../pci.h"
>  #include "portdrv.h"
>  
> -bool pciehp_msi_disabled;
> -
> -static int __init pciehp_setup(char *str)
> -{
> -	if (!strncmp(str, "nomsi", 5))
> -		pciehp_msi_disabled = true;
> -
> -	return 1;
> -}
> -__setup("pcie_hp=", pciehp_setup);
> -
>  /**
>   * release_pcie_device - free PCI Express port service device structure
>   * @dev: Port service device to release
> @@ -169,16 +158,13 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  		irqs[i] = -1;
>  
>  	/*
> -	 * If we support PME or hotplug, but we can't use MSI/MSI-X for
> -	 * them, we have to fall back to INTx or other interrupts, e.g., a
> -	 * system shared interrupt.
> +	 * If we support PME but can't use MSI/MSI-X for it, we have to
> +	 * fall back to INTx or other interrupts, e.g., a system shared
> +	 * interrupt.
>  	 */
>  	if ((mask & PCIE_PORT_SERVICE_PME) && pcie_pme_no_msi())
>  		goto legacy_irq;
>  
> -	if ((mask & PCIE_PORT_SERVICE_HP) && pciehp_no_msi())
> -		goto legacy_irq;
> -
>  	/* Try to use MSI-X or MSI if supported */
>  	if (pcie_port_enable_irq_vec(dev, irqs, mask) == 0)
>  		return 0;
> 
> 

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Re: [PATCH v1 2/9] PCI/PM: Clear PCIe PME Status bit in core, not PCIe port driver

[Lukas Wunner]

On Wed, Mar 07, 2018 at 12:13:34AM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> fe31e69740ed ("PCI/PCIe: Clear Root PME Status bits early during system
> resume") added a .resume_noirq() callback to the PCIe port driver to clear
> the PME Status bit during resume to work around a BIOS issue.
> 
> The BIOS evidently enabled PME interrupts for ACPI-based runtime wakeups
> but did not clear the PME Status bit during resume, which meant PMEs after
> resume did not trigger interrupts because PME Status did not transition
> from cleared to set.
> 
> The fix was in the PCIe port driver, so it worked when CONFIG_PCIEPORTBUS
> was set.  But I think we *always* want the fix because the platform may use
> PME interrupts even if Linux is built without the PCIe port driver.
> 
> Move the fix from the port driver to the PCI core so we can work around
> this "PME doesn't work after waking from a sleep state" issue regardless of
> CONFIG_PCIEPORTBUS.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pci-driver.c       |   14 ++++++++++++++
>  drivers/pci/pcie/portdrv_pci.c |   15 ---------------
>  2 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3bed6beda051..bf0704b75f79 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -525,6 +525,18 @@ static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
>  	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>  }
>  
> +static void pcie_resume_early(struct pci_dev *pci_dev)
> +{
> +	/*
> +	 * Some BIOSes forget to clear Root PME Status bits after system wakeup
> +	 * which breaks ACPI-based runtime wakeup on PCI Express, so clear those
> +	 * bits now just in case (shouldn't hurt).
> +	 */
> +	if (pci_is_pcie(pci_dev) &&
> +	    pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
> +		pcie_clear_root_pme_status(pci_dev);
> +}
> +
>  /*
>   * Default "suspend" method for devices that have no driver provided suspend,
>   * or not even a driver at all (second part).
> @@ -873,6 +885,8 @@ static int pci_pm_resume_noirq(struct device *dev)
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume_early(dev);
>  
> +	pcie_resume_early(pci_dev);

I'm wondering if it would be better to implement this as a PCI quirk:

DECLARE_PCI_FIXUP_CLASS_RESUME_EARLY(PCI_ANY_ID, PCI_ANY_ID,
				     PCI_CLASS_BRIDGE_PCI, 8, ...)

Then it would also be executed for the ->restore_noirq phase, not only
->resume_noirq.  And it could be constrained to only the affected PCI
or DMI IDs.

However it would then also be executed on ->runtime_resume, I'm not
sure if that's a problem or not.

Thanks,

Lukas

> +
>  	if (drv && drv->pm && drv->pm->resume_noirq)
>  		error = drv->pm->resume_noirq(dev);
>  
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 4413dd85e923..f91afd09e356 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -62,20 +62,6 @@ static int pcie_portdrv_restore_config(struct pci_dev *dev)
>  }
>  
>  #ifdef CONFIG_PM
> -static int pcie_port_resume_noirq(struct device *dev)
> -{
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -
> -	/*
> -	 * Some BIOSes forget to clear Root PME Status bits after system wakeup
> -	 * which breaks ACPI-based runtime wakeup on PCI Express, so clear those
> -	 * bits now just in case (shouldn't hurt).
> -	 */
> -	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)
> -		pcie_clear_root_pme_status(pdev);
> -	return 0;
> -}
> -
>  static int pcie_port_runtime_suspend(struct device *dev)
>  {
>  	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
> @@ -103,7 +89,6 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
>  	.thaw		= pcie_port_device_resume,
>  	.poweroff	= pcie_port_device_suspend,
>  	.restore	= pcie_port_device_resume,
> -	.resume_noirq	= pcie_port_resume_noirq,
>  	.runtime_suspend = pcie_port_runtime_suspend,
>  	.runtime_resume	= pcie_port_runtime_resume,
>  	.runtime_idle	= pcie_port_runtime_idle,
> 

Re: [PATCH v1 2/9] PCI/PM: Clear PCIe PME Status bit in core, not PCIe port driver

[Rafael J. Wysocki]

On Thursday, March 8, 2018 9:03:31 AM CET Lukas Wunner wrote:
> On Wed, Mar 07, 2018 at 12:13:34AM -0600, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > fe31e69740ed ("PCI/PCIe: Clear Root PME Status bits early during system
> > resume") added a .resume_noirq() callback to the PCIe port driver to clear
> > the PME Status bit during resume to work around a BIOS issue.
> > 
> > The BIOS evidently enabled PME interrupts for ACPI-based runtime wakeups
> > but did not clear the PME Status bit during resume, which meant PMEs after
> > resume did not trigger interrupts because PME Status did not transition
> > from cleared to set.
> > 
> > The fix was in the PCIe port driver, so it worked when CONFIG_PCIEPORTBUS
> > was set.  But I think we *always* want the fix because the platform may use
> > PME interrupts even if Linux is built without the PCIe port driver.
> > 
> > Move the fix from the port driver to the PCI core so we can work around
> > this "PME doesn't work after waking from a sleep state" issue regardless of
> > CONFIG_PCIEPORTBUS.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/pci-driver.c       |   14 ++++++++++++++
> >  drivers/pci/pcie/portdrv_pci.c |   15 ---------------
> >  2 files changed, 14 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 3bed6beda051..bf0704b75f79 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -525,6 +525,18 @@ static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
> >  	pci_fixup_device(pci_fixup_resume_early, pci_dev);
> >  }
> >  
> > +static void pcie_resume_early(struct pci_dev *pci_dev)
> > +{
> > +	/*
> > +	 * Some BIOSes forget to clear Root PME Status bits after system wakeup
> > +	 * which breaks ACPI-based runtime wakeup on PCI Express, so clear those
> > +	 * bits now just in case (shouldn't hurt).
> > +	 */
> > +	if (pci_is_pcie(pci_dev) &&
> > +	    pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
> > +		pcie_clear_root_pme_status(pci_dev);
> > +}
> > +
> >  /*
> >   * Default "suspend" method for devices that have no driver provided suspend,
> >   * or not even a driver at all (second part).
> > @@ -873,6 +885,8 @@ static int pci_pm_resume_noirq(struct device *dev)
> >  	if (pci_has_legacy_pm_support(pci_dev))
> >  		return pci_legacy_resume_early(dev);
> >  
> > +	pcie_resume_early(pci_dev);
> 
> I'm wondering if it would be better to implement this as a PCI quirk:
> 
> DECLARE_PCI_FIXUP_CLASS_RESUME_EARLY(PCI_ANY_ID, PCI_ANY_ID,
> 				     PCI_CLASS_BRIDGE_PCI, 8, ...)
> 
> Then it would also be executed for the ->restore_noirq phase, not only
> ->resume_noirq.  And it could be constrained to only the affected PCI
> or DMI IDs.
> 
> However it would then also be executed on ->runtime_resume, I'm not
> sure if that's a problem or not.

While that may not be a problem strictly speaking (the root port resume
only takes place when the port itself has been suspended, so this should
not interfere with PME handling for the devices under it), but it also is
not necessary in that case, because the platform firmware doesn't mess up
with the root port's PM registers then.

Or if it does, the whole PCIe PM is managed by AML/SMM and we should not
even touch this register at all.