[PATCH 00/10] PCI: Allow D3cold for PCIe hierarchies

[PATCH 00/10] PCI: Allow D3cold for PCIe hierarchies

[Mika Westerberg]

Hi all,

This patch series aims to allow PCIe hierarchies enter D3cold both during
system sleep and runtime, including PCIe hotplug ports.

The motivation of this series are recent systems such as Lenovo Thinkpad X1
Carbon 6th gen and upcoming Dell laptops where the Thunderbolt controller
is always present in the system (pretty much like what Apple systems have
been doing for years). Because it is always present it consumes energy so
it is important to power it off when idle.

The PCIe root port hosting the Thunderbolt controller and all the connected
external PCIe devices includes a standard ACPI power resource and related
methods _PR3, _PR0, _PRW and _DSW that take care of the actual transition
to D3cold and back to D0.

With current kernels entering system sleep states leaves the root port into
D3hot which means the ACPI power resource is still on. This is bad when
system enters suspend-to-idle (all these systems are s2idle only) because
the BIOS is not involved and the devices are effectively left on, consuming
battery unnecessarily.

Patches 1-4 add D3cold support for system sleep and the subsequent patches
do the same for runtime PM.

In case someone wants to try out runtime PM, the xHCI controller that is
part of the PCIe switch below the root port needs to have runtime PM
"unblocked" manually (this will be automatic in future).

For Thinkpad:

  # echo auto > /sys/bus/pci/devices/0000:3b:00.0/power/control

For Dell:

  # echo auto > /sys/bus/pci/devices/0000:39:00.0/power/control

Note if the root port enters D3cold, running things like 'lspci' brings it
back t0 D0 (because PCI config space is not accessible in D3cold) so if one
wants to check out the power state without accidentally bringing the device
back into D0 needs to do that indirectly. If the root port is 1d.0 power
state can be checked:

  # cat /sys/bus/pci/devices/0000:00:1d.0/power/runtime_status
  suspended
  # cat /sys/bus/pci/devices/0000:00:1d.0/firmware_node/real_power_state
  D3cold

Even if this again revolves around Thunderbolt I think these apply to any
PCIe system supporting D3cold.

These patches apply on top of pci.git pci/hotplug.

Mika Westerberg (10):
  PCI: Do not skip power managed bridges in pci_enable_wake()
  PCI / ACPI: Enable wake automatically for power managed bridges
  PCI: pciehp: Disable hotplug interrupt during suspend
  PCI: pciehp: Do not handle events if interrupts are masked
  PCI: portdrv: Resume upon exit from system suspend if left runtime suspended
  PCI: portdrv: Add runtime PM hooks for port service drivers
  PCI: pciehp: Implement runtime PM callbacks
  PCI/PME: Implement runtime PM callbacks
  ACPI / property: Allow multiple property compatible _DSD entries
  PCI / ACPI: Whitelist D3 for more PCIe hotplug ports

 drivers/acpi/property.c           | 97 ++++++++++++++++++++++---------
 drivers/acpi/x86/apple.c          |  2 +-
 drivers/gpio/gpiolib-acpi.c       |  2 +-
 drivers/pci/hotplug/pciehp.h      |  2 +
 drivers/pci/hotplug/pciehp_core.c | 37 ++++++++++++
 drivers/pci/hotplug/pciehp_hpc.c  | 16 ++++-
 drivers/pci/pci-acpi.c            | 56 +++++++++++++++++-
 drivers/pci/pci.c                 | 15 ++++-
 drivers/pci/pci.h                 |  3 +
 drivers/pci/pcie/pme.c            | 27 +++++++++
 drivers/pci/pcie/portdrv.h        |  4 ++
 drivers/pci/pcie/portdrv_core.c   | 20 +++++++
 drivers/pci/pcie/portdrv_pci.c    | 28 ++++++---
 include/acpi/acpi_bus.h           |  8 ++-
 include/linux/acpi.h              |  9 +++
 15 files changed, 285 insertions(+), 41 deletions(-)

-- 
2.18.0

[PATCH 01/10] PCI: Do not skip power managed bridges in pci_enable_wake()

[Mika Westerberg]

Commit baecc470d5fd ("PCI / PM: Skip bridges in pci_enable_wake()")
changed pci_enable_wake() so that all bridges are skipped when wakeup is
enabled (or disabled) with the reasoning that bridges can only signal
wakeup on behalf of their subordinate devices.

However, there are bridges that can signal wakeup itself. For example
PCIe downstream and root ports supporting hotplug may signal wakeup upon
hotplug event.

For this reason change pci_enable_wake() so that it skips all bridges
except those that we power manage (->bridge_d3 is set). Those are the
ones that can go into low power states and may need to signal wakeup.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/pci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 29ff9619b5fa..074f3f0253f9 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2134,9 +2134,11 @@ static int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable
 
 	/*
 	 * Bridges can only signal wakeup on behalf of subordinate devices,
-	 * but that is set up elsewhere, so skip them.
+	 * but that is set up elsewhere, so skip them. With the exception
+	 * of bridges that we power manage. These can signal wake for
+	 * example on a hotplug event.
 	 */
-	if (pci_has_subordinate(dev))
+	if (!pci_power_manageable(dev))
 		return 0;
 
 	/* Don't do the same thing twice in a row for one device. */
-- 
2.18.0

[PATCH 02/10] PCI / ACPI: Enable wake automatically for power managed bridges

[Mika Westerberg]

We enable power management automatically for bridges where
pci_bridge_d3_possible() returns true. However, these bridges may have
ACPI methods such as _DSW that need to be called before D3 entry. For
example in Lenovo Thinkpad X1 Carbon 6th _DSW method is used to prepare
D3cold for the PCIe root port hosting Thunderbolt chain. Because wake is
not enabled _DSW method is never called and the port does not enter
D3cold properly consuming more power than necessary.

Users can work this around by writing "enabled" to "wakeup" sysfs file
under the device in question but that is not something an ordinary user
is expected to do.

Since we already automatically enable power management for PCIe ports
with ->bridge_d3 set extend that to enable wake for them as well,
assuming the port has any ACPI wakeup related objects implemented in the
namespace (adev->wakeup.flags.valid is true). This ensures the necessary
ACPI methods get called at appropriate times and allows the root port in
Thinkpad X1 Carbon 6th to go into D3cold.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/pci-acpi.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index c2ab57705043..a4a8c02deaa0 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -762,19 +762,32 @@ static void pci_acpi_setup(struct device *dev)
 		return;
 
 	device_set_wakeup_capable(dev, true);
+	/*
+	 * For bridges that can do D3 we enable wake automatically (as
+	 * we do for the power management itself in that case). The
+	 * reason is that the bridge may have additional methods such as
+	 * _DSW that need to be called.
+	 */
+	if (pci_dev->bridge_d3)
+		device_wakeup_enable(dev);
+
 	acpi_pci_wakeup(pci_dev, false);
 }
 
 static void pci_acpi_cleanup(struct device *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct pci_dev *pci_dev = to_pci_dev(dev);
 
 	if (!adev)
 		return;
 
 	pci_acpi_remove_pm_notifier(adev);
-	if (adev->wakeup.flags.valid)
+	if (adev->wakeup.flags.valid) {
+		if (pci_dev->bridge_d3)
+			device_wakeup_disable(dev);
 		device_set_wakeup_capable(dev, false);
+	}
 }
 
 static bool pci_acpi_bus_match(struct device *dev)
-- 
2.18.0

[PATCH 03/10] PCI: pciehp: Disable hotplug interrupt during suspend

[Mika Westerberg]

When PCIe hotplug port is transitioned into D3hot link to the downstream
component will go down. If hotplug interrupt generation is enabled when
that happens it will trigger immediately waking up the system and
bringing the link back up.

To prevent this disable hotplug interrupt generation when system suspend
is entered. This does not prevent wakeup from low power states according
to PCIe 4.0 spec section 6.7.3.4:

  Software enables a hot-plug event to generate a wakeup event by
  enabling software notification of the event as described in Section
  6.7.3.1. Note that in order for software to disable interrupt generation
  while keeping wakeup generation enabled, the Hot-Plug Interrupt Enable
  bit must be cleared.

So as long as we have set the slot event mask accordingly wakeup should
work even if slot interrupt is disabled. The port should trigger wake
and then send PME to the root port when the PCIe hierarchy is brought
back up.

Limit this to systems using native PME mechanism to make sure older
Apple systems depending on commit e3354628c376 ("PCI: pciehp: Support
interrupts sent from D3hot") still continue working.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/pciehp.h      |  2 ++
 drivers/pci/hotplug/pciehp_core.c | 18 ++++++++++++++++++
 drivers/pci/hotplug/pciehp_hpc.c  | 10 ++++++++++
 3 files changed, 30 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 811cf83f956d..58bddeb3c5a9 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -187,6 +187,8 @@ struct controller *pcie_init(struct pcie_device *dev);
 int pcie_init_notification(struct controller *ctrl);
 void pcie_shutdown_notification(struct controller *ctrl);
 void pcie_clear_hotplug_events(struct controller *ctrl);
+void pcie_enable_interrupt(struct controller *ctrl);
+void pcie_disable_interrupt(struct controller *ctrl);
 int pciehp_power_on_slot(struct slot *slot);
 void pciehp_power_off_slot(struct slot *slot);
 void pciehp_get_power_status(struct slot *slot, u8 *status);
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index ec48c9433ae5..c9cf9c3b5f7f 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -302,8 +302,23 @@ static void pciehp_remove(struct pcie_device *dev)
 }
 
 #ifdef CONFIG_PM
+static bool pme_is_native(struct pcie_device *dev)
+{
+	const struct pci_host_bridge *host;
+
+	host = pci_find_host_bridge(dev->port->bus);
+	return pcie_ports_native || host->native_pme;
+}
+
 static int pciehp_suspend(struct pcie_device *dev)
 {
+	/*
+	 * Disable hotplug interrupt so that it does not trigger
+	 * immediately when the downstream link goes down.
+	 */
+	if (pme_is_native(dev))
+		pcie_disable_interrupt(get_service_data(dev));
+
 	return 0;
 }
 
@@ -327,6 +342,9 @@ static int pciehp_resume(struct pcie_device *dev)
 {
 	struct controller *ctrl = get_service_data(dev);
 
+	if (pme_is_native(dev))
+		pcie_enable_interrupt(ctrl);
+
 	pciehp_check_presence(ctrl);
 
 	return 0;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 7136e3430925..2249c4d06efd 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -748,6 +748,16 @@ void pcie_clear_hotplug_events(struct controller *ctrl)
 				   PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC);
 }
 
+void pcie_enable_interrupt(struct controller *ctrl)
+{
+	pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_HPIE, PCI_EXP_SLTCTL_HPIE);
+}
+
+void pcie_disable_interrupt(struct controller *ctrl)
+{
+	pcie_write_cmd(ctrl, 0, PCI_EXP_SLTCTL_HPIE);
+}
+
 /*
  * pciehp has a 1:1 bus:slot relationship so we ultimately want a secondary
  * bus reset of the bridge, but at the same time we want to ensure that it is
-- 
2.18.0

[PATCH 04/10] PCI: pciehp: Do not handle events if interrupts are masked

[Mika Westerberg]

PCIe native hotplug shares MSI vector with native PME so the interrupt
handler might get called even the hotplug interrupt is masked. In that
case we should not handle any events because the interrupt was not meant
for us. Modify the PCIe hotplug interrupt handler to check this
accordingly and bail out if it finds out that the interrupt was not
about hotplug.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/pciehp_hpc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 2249c4d06efd..19ed13d44b8f 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -533,9 +533,11 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 	u16 status, events;
 
 	/*
-	 * Interrupts only occur in D3hot or shallower (PCIe r4.0, sec 6.7.3.4).
+	 * Interrupts only occur in D3hot or shallower and only if enabled
+	 * in the Slot Control register (PCIe r4.0, sec 6.7.3.4).
 	 */
-	if (pdev->current_state == PCI_D3cold)
+	if (pdev->current_state == PCI_D3cold ||
+	    !(ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE))
 		return IRQ_NONE;
 
 	/*
-- 
2.18.0

[PATCH 05/10] PCI: portdrv: Resume upon exit from system suspend if left runtime suspended

[Mika Westerberg]

Currently we try to keep PCIe ports runtime suspended over system
suspend if possible. This mostly happens when entering suspend-to-idle
because there is no need to re-configure wake settings.

This causes problems if the parent port goes into D3cold and it gets
resumed upon exit from system suspend. This may happen for example if
the port is part of PCIe switch and the same switch is connected to a
PCIe endpoint that needs to be resumed. The way exit from D3cold works
according PCIe 4.0 spec 5.3.1.4.2 is that power is restored and cold
reset is signaled. After this the device is in D0unitialized state
keeping PME context if it supports wake from D3cold.

The problem occurs when a PCIe hotplug port is left suspended and the
parent port goes into D3cold and back to D0, the port keeps its PME
context but since everything else is reset back to defaults
(D0unitialized) it is not set to detect hotplug events anymore.

For this reason change the PCIe portdrv power management logic so that
it is fine to keep the port runtime suspended over system suspend but it
needs to be resumed upon exit to make sure it gets properly re-initialized.
The custom ->prepare() hook with DPM_FLAG_SMART_PREPARE is needed
because otherwise pci_pm_prepare() instructs the PM core to go directly
to pci_pm_complete() on resume and this skips resuming the port.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/pcie/portdrv_pci.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index eef22dc29140..74761f660a30 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -43,6 +43,21 @@ __setup("pcie_ports=", pcie_port_setup);
 /* global data */
 
 #ifdef CONFIG_PM
+int pcie_port_prepare(struct device *dev)
+{
+	/*
+	 * Return 0 here to indicate PCI core that:
+	 *   - Direct complete path should be avoided
+	 *   - It is OK to leave the port runtime suspended over system
+	 *     suspend
+	 *
+	 * However, the port needs to be resumed afterwards because it may
+	 * have been in D3cold in which case we need to re-initialize the
+	 * hardware as it is in D0uninitialized in that case.
+	 */
+	return 0;
+}
+
 static int pcie_port_runtime_suspend(struct device *dev)
 {
 	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
@@ -64,6 +79,7 @@ static int pcie_port_runtime_idle(struct device *dev)
 }
 
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
+	.prepare	= pcie_port_prepare,
 	.suspend	= pcie_port_device_suspend,
 	.resume_noirq	= pcie_port_device_resume_noirq,
 	.resume		= pcie_port_device_resume,
@@ -109,8 +125,8 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 
 	pci_save_state(dev);
 
-	dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_SMART_SUSPEND |
-					   DPM_FLAG_LEAVE_SUSPENDED);
+	dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_SMART_PREPARE |
+					   DPM_FLAG_SMART_SUSPEND);
 
 	if (pci_bridge_d3_possible(dev)) {
 		/*
-- 
2.18.0

[PATCH 06/10] PCI: portdrv: Add runtime PM hooks for port service drivers

[Mika Westerberg]

When PCIe port is runtime suspended/resumed some extra steps might be
needed to be executed from the port service driver side. For instance we
may need to disable PCIe hotplug interrupt to prevent it from triggering
immediately when PCIe link to the downstream component goes down.

To make the above possible add optional ->runtime_suspend() and
->runtime_resume() callbacks to struct pcie_port_service_driver and call
them for each port service in runtime suspend/resume callbacks of portdrv.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/pcie/portdrv.h      |  4 ++++
 drivers/pci/pcie/portdrv_core.c | 20 ++++++++++++++++++++
 drivers/pci/pcie/portdrv_pci.c  | 10 ++++------
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index d59afa42fc14..72fa132060e5 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -52,6 +52,8 @@ struct pcie_port_service_driver {
 	int (*suspend) (struct pcie_device *dev);
 	int (*resume_noirq) (struct pcie_device *dev);
 	int (*resume) (struct pcie_device *dev);
+	int (*runtime_suspend) (struct pcie_device *dev);
+	int (*runtime_resume) (struct pcie_device *dev);
 
 	/* Device driver may resume normal operations */
 	void (*error_resume)(struct pci_dev *dev);
@@ -85,6 +87,8 @@ int pcie_port_device_register(struct pci_dev *dev);
 int pcie_port_device_suspend(struct device *dev);
 int pcie_port_device_resume_noirq(struct device *dev);
 int pcie_port_device_resume(struct device *dev);
+int pcie_port_device_runtime_suspend(struct device *dev);
+int pcie_port_device_runtime_resume(struct device *dev);
 #endif
 void pcie_port_device_remove(struct pci_dev *dev);
 int __must_check pcie_port_bus_register(void);
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 7c37d815229e..6542c48c7f59 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -395,6 +395,26 @@ int pcie_port_device_resume(struct device *dev)
 	size_t off = offsetof(struct pcie_port_service_driver, resume);
 	return device_for_each_child(dev, &off, pm_iter);
 }
+
+/**
+ * pcie_port_device_runtime_suspend - runtime suspend port services
+ * @dev: PCI Express port to handle
+ */
+int pcie_port_device_runtime_suspend(struct device *dev)
+{
+	size_t off = offsetof(struct pcie_port_service_driver, runtime_suspend);
+	return device_for_each_child(dev, &off, pm_iter);
+}
+
+/**
+ * pcie_port_device_runtime_resume - runtime resume port services
+ * @dev: PCI Express port to handle
+ */
+int pcie_port_device_runtime_resume(struct device *dev)
+{
+	size_t off = offsetof(struct pcie_port_service_driver, runtime_resume);
+	return device_for_each_child(dev, &off, pm_iter);
+}
 #endif /* PM */
 
 static int remove_iter(struct device *dev, void *data)
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 74761f660a30..d018633694e0 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -60,12 +60,10 @@ int pcie_port_prepare(struct device *dev)
 
 static int pcie_port_runtime_suspend(struct device *dev)
 {
-	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
-}
+	if (!to_pci_dev(dev)->bridge_d3)
+		return -EBUSY;
 
-static int pcie_port_runtime_resume(struct device *dev)
-{
-	return 0;
+	return pcie_port_device_runtime_suspend(dev);
 }
 
 static int pcie_port_runtime_idle(struct device *dev)
@@ -89,7 +87,7 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.restore_noirq	= pcie_port_device_resume_noirq,
 	.restore	= pcie_port_device_resume,
 	.runtime_suspend = pcie_port_runtime_suspend,
-	.runtime_resume	= pcie_port_runtime_resume,
+	.runtime_resume	= pcie_port_device_runtime_resume,
 	.runtime_idle	= pcie_port_runtime_idle,
 };
 
-- 
2.18.0

[PATCH 07/10] PCI: pciehp: Implement runtime PM callbacks

[Mika Westerberg]

Basically we need to do the same thing when runtime suspending than with
system sleep so re-use those operations here. This makes sure hotplug
interrupt does not trigger immediately when the link goes down.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/pciehp_core.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index c9cf9c3b5f7f..25b2ea9f9242 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -349,6 +349,23 @@ static int pciehp_resume(struct pcie_device *dev)
 
 	return 0;
 }
+
+static int pciehp_runtime_resume(struct pcie_device *dev)
+{
+	struct controller *ctrl = get_service_data(dev);
+	struct slot *slot = ctrl->slot;
+
+	/* pci_restore_state() just wrote to the Slot Control register */
+	ctrl->cmd_started = jiffies;
+	ctrl->cmd_busy = true;
+
+	/* clear spurious events from rediscovery of inserted card */
+	if ((slot->state == ON_STATE || slot->state == BLINKINGOFF_STATE) &&
+	     pme_is_native(dev))
+		pcie_clear_hotplug_events(ctrl);
+
+	return pciehp_resume(dev);
+}
 #endif /* PM */
 
 static struct pcie_port_service_driver hpdriver_portdrv = {
@@ -363,6 +380,8 @@ static struct pcie_port_service_driver hpdriver_portdrv = {
 	.suspend	= pciehp_suspend,
 	.resume_noirq	= pciehp_resume_noirq,
 	.resume		= pciehp_resume,
+	.runtime_suspend = pciehp_suspend,
+	.runtime_resume	= pciehp_runtime_resume,
 #endif	/* PM */
 };
 
-- 
2.18.0

[PATCH 08/10] PCI/PME: Implement runtime PM callbacks

[Mika Westerberg]

Basically we need to do the same steps than what we do when system sleep
is entered and disable PME interrupt when the root port is runtime
suspended. This prevents spurious wakeups immediately when the port is
transitioned into D3cold.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/pcie/pme.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index 3ed67676ea2a..62fe122f030a 100644
--- a/drivers/pci/pcie/pme.c
+++ b/drivers/pci/pcie/pme.c
@@ -432,6 +432,31 @@ static void pcie_pme_remove(struct pcie_device *srv)
 	kfree(get_service_data(srv));
 }
 
+static int pcie_pme_runtime_suspend(struct pcie_device *srv)
+{
+	struct pcie_pme_service_data *data = get_service_data(srv);
+
+	spin_lock_irq(&data->lock);
+	pcie_pme_interrupt_enable(srv->port, false);
+	pcie_clear_root_pme_status(srv->port);
+	data->noirq = true;
+	spin_unlock_irq(&data->lock);
+
+	return 0;
+}
+
+static int pcie_pme_runtime_resume(struct pcie_device *srv)
+{
+	struct pcie_pme_service_data *data = get_service_data(srv);
+
+	spin_lock_irq(&data->lock);
+	pcie_pme_interrupt_enable(srv->port, true);
+	data->noirq = false;
+	spin_unlock_irq(&data->lock);
+
+	return 0;
+}
+
 static struct pcie_port_service_driver pcie_pme_driver = {
 	.name		= "pcie_pme",
 	.port_type	= PCI_EXP_TYPE_ROOT_PORT,
@@ -439,6 +464,8 @@ static struct pcie_port_service_driver pcie_pme_driver = {
 
 	.probe		= pcie_pme_probe,
 	.suspend	= pcie_pme_suspend,
+	.runtime_suspend = pcie_pme_runtime_suspend,
+	.runtime_resume	= pcie_pme_runtime_resume,
 	.resume		= pcie_pme_resume,
 	.remove		= pcie_pme_remove,
 };
-- 
2.18.0

[PATCH 09/10] ACPI / property: Allow multiple property compatible _DSD entries

[Mika Westerberg]

It is possible to have _DSD entries where the data is compatible with device
properties format but are using different GUID for various reasons. In addition
to that there can be many such _DSD entries for a single device such as for
PCIe root port used to host a Thunderbolt hierarchy:

    Scope (\_SB.PCI0.RP21)
    {
        Name (_DSD, Package () {
            ToUUID ("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"),
            Package () {
                Package () {"HotPlugSupportInD3", 1}
            },

            ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"),
            Package () {
                Package () {"ExternalFacingPort", 1},
                Package () {"UID", 0 }
            }
        })
    }

For more information about these new _DSD entries can be found in:

  https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports

In order to make these available for drivers via unified device property
APIs modify ACPI property core so that it supports multiple _DSD entries
organized in a linked list. We also store GUID of each _DSD entry in
struct acpi_device_properties in case there is need to differerentiate
between entries. The supported GUIDs are then listed in prp_guids array.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/property.c     | 94 +++++++++++++++++++++++++++----------
 drivers/acpi/x86/apple.c    |  2 +-
 drivers/gpio/gpiolib-acpi.c |  2 +-
 include/acpi/acpi_bus.h     |  8 +++-
 include/linux/acpi.h        |  9 ++++
 5 files changed, 86 insertions(+), 29 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 693cf05b0cc4..f3ddb279d2de 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -24,11 +24,12 @@ static int acpi_data_get_property_array(const struct acpi_device_data *data,
 					acpi_object_type type,
 					const union acpi_object **obj);
 
-/* ACPI _DSD device properties GUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
-static const guid_t prp_guid =
+static const guid_t prp_guids[] = {
+	/* ACPI _DSD device properties GUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
 	GUID_INIT(0xdaffd814, 0x6eba, 0x4d8c,
-		  0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01);
-/* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
+		  0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01),
+};
+
 static const guid_t ads_guid =
 	GUID_INIT(0xdbb8e3e6, 0x5886, 0x4ba6,
 		  0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b);
@@ -56,6 +57,7 @@ static bool acpi_nondev_subnode_extract(const union acpi_object *desc,
 	dn->name = link->package.elements[0].string.pointer;
 	dn->fwnode.ops = &acpi_data_fwnode_ops;
 	dn->parent = parent;
+	INIT_LIST_HEAD(&dn->data.properties);
 	INIT_LIST_HEAD(&dn->data.subnodes);
 
 	result = acpi_extract_properties(desc, &dn->data);
@@ -288,6 +290,35 @@ static void acpi_init_of_compatible(struct acpi_device *adev)
 	adev->flags.of_compatible_ok = 1;
 }
 
+static bool acpi_is_property_guid(const guid_t *guid)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(prp_guids); i++) {
+		if (guid_equal(guid, &prp_guids[i]))
+			return true;
+	}
+
+	return false;
+}
+
+struct acpi_device_properties *
+acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
+		    const union acpi_object *properties)
+{
+	struct acpi_device_properties *props;
+
+	props = kzalloc(sizeof(*props), GFP_KERNEL);
+	if (props) {
+		INIT_LIST_HEAD(&props->list);
+		props->guid = guid;
+		props->properties = properties;
+		list_add_tail(&props->list, &data->properties);
+	}
+
+	return props;
+}
+
 static bool acpi_extract_properties(const union acpi_object *desc,
 				    struct acpi_device_data *data)
 {
@@ -312,7 +343,7 @@ static bool acpi_extract_properties(const union acpi_object *desc,
 		    properties->type != ACPI_TYPE_PACKAGE)
 			break;
 
-		if (!guid_equal((guid_t *)guid->buffer.pointer, &prp_guid))
+		if (!acpi_is_property_guid((guid_t *)guid->buffer.pointer))
 			continue;
 
 		/*
@@ -320,13 +351,13 @@ static bool acpi_extract_properties(const union acpi_object *desc,
 		 * package immediately following it.
 		 */
 		if (!acpi_properties_format_valid(properties))
-			break;
+			continue;
 
-		data->properties = properties;
-		return true;
+		acpi_data_add_props(data, (const guid_t *)guid->buffer.pointer,
+				    properties);
 	}
 
-	return false;
+	return !list_empty(&data->properties);
 }
 
 void acpi_init_properties(struct acpi_device *adev)
@@ -336,6 +367,7 @@ void acpi_init_properties(struct acpi_device *adev)
 	acpi_status status;
 	bool acpi_of = false;
 
+	INIT_LIST_HEAD(&adev->data.properties);
 	INIT_LIST_HEAD(&adev->data.subnodes);
 
 	if (!adev->handle)
@@ -398,11 +430,16 @@ static void acpi_destroy_nondev_subnodes(struct list_head *list)
 
 void acpi_free_properties(struct acpi_device *adev)
 {
+	struct acpi_device_properties *props, *tmp;
+
 	acpi_destroy_nondev_subnodes(&adev->data.subnodes);
 	ACPI_FREE((void *)adev->data.pointer);
 	adev->data.of_compatible = NULL;
 	adev->data.pointer = NULL;
-	adev->data.properties = NULL;
+	list_for_each_entry_safe(props, tmp, &adev->data.properties, list) {
+		list_del(&props->list);
+		kfree(props);
+	}
 }
 
 /**
@@ -427,32 +464,37 @@ static int acpi_data_get_property(const struct acpi_device_data *data,
 				  const char *name, acpi_object_type type,
 				  const union acpi_object **obj)
 {
-	const union acpi_object *properties;
-	int i;
+	struct acpi_device_properties *props;
 
 	if (!data || !name)
 		return -EINVAL;
 
-	if (!data->pointer || !data->properties)
+	if (!data->pointer || list_empty(&data->properties))
 		return -EINVAL;
 
-	properties = data->properties;
-	for (i = 0; i < properties->package.count; i++) {
-		const union acpi_object *propname, *propvalue;
-		const union acpi_object *property;
+	list_for_each_entry(props, &data->properties, list) {
+		const union acpi_object *properties;
+		int i;
 
-		property = &properties->package.elements[i];
+		properties = props->properties;
+		for (i = 0; i < properties->package.count; i++) {
+			const union acpi_object *propname, *propvalue;
+			const union acpi_object *property;
 
-		propname = &property->package.elements[0];
-		propvalue = &property->package.elements[1];
+			property = &properties->package.elements[i];
 
-		if (!strcmp(name, propname->string.pointer)) {
-			if (type != ACPI_TYPE_ANY && propvalue->type != type)
-				return -EPROTO;
-			if (obj)
-				*obj = propvalue;
+			propname = &property->package.elements[0];
+			propvalue = &property->package.elements[1];
 
-			return 0;
+			if (!strcmp(name, propname->string.pointer)) {
+				if (type != ACPI_TYPE_ANY &&
+				    propvalue->type != type)
+					return -EPROTO;
+				if (obj)
+					*obj = propvalue;
+
+				return 0;
+			}
 		}
 	}
 	return -EINVAL;
diff --git a/drivers/acpi/x86/apple.c b/drivers/acpi/x86/apple.c
index 51b4cf9f25da..130df1c8ed7d 100644
--- a/drivers/acpi/x86/apple.c
+++ b/drivers/acpi/x86/apple.c
@@ -132,8 +132,8 @@ void acpi_extract_apple_properties(struct acpi_device *adev)
 	}
 	WARN_ON(free_space != (void *)newprops + newsize);
 
-	adev->data.properties = newprops;
 	adev->data.pointer = newprops;
+	acpi_data_add_props(&adev->data, &apple_prp_guid, newprops);
 
 out_free:
 	ACPI_FREE(props);
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index c48ed9d89ff5..7be705fae4b8 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1192,7 +1192,7 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
 bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
 {
 	/* Never allow fallback if the device has properties */
-	if (adev->data.properties || adev->driver_gpios)
+	if (acpi_dev_has_props(adev) || adev->driver_gpios)
 		return false;
 
 	return con_id == NULL;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index ba4dd54f2c82..cd35e3ce9a8b 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -346,10 +346,16 @@ struct acpi_device_physical_node {
 	bool put_online:1;
 };
 
+struct acpi_device_properties {
+	const guid_t *guid;
+	const union acpi_object *properties;
+	struct list_head list;
+};
+
 /* ACPI Device Specific Data (_DSD) */
 struct acpi_device_data {
 	const union acpi_object *pointer;
-	const union acpi_object *properties;
+	struct list_head properties;
 	const union acpi_object *of_compatible;
 	struct list_head subnodes;
 };
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index de8d3d3fa651..51e3c29663fe 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1074,6 +1074,15 @@ static inline int acpi_node_get_property_reference(
 		NR_FWNODE_REFERENCE_ARGS, args);
 }
 
+static inline bool acpi_dev_has_props(const struct acpi_device *adev)
+{
+	return !list_empty(&adev->data.properties);
+}
+
+struct acpi_device_properties *
+acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
+		    const union acpi_object *properties);
+
 int acpi_node_prop_get(const struct fwnode_handle *fwnode, const char *propname,
 		       void **valptr);
 int acpi_dev_prop_read_single(struct acpi_device *adev,
-- 
2.18.0

[PATCH 10/10] PCI / ACPI: Whitelist D3 for more PCIe hotplug ports

[Mika Westerberg]

In order to have better power management for Thunderbolt PCIe chains,
Windows enables power management for native PCIe hotplug ports if there
is following ACPI _DSD attached to the root port:

  Name (_DSD, Package () {
      ToUUID ("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"),
      Package () {
          Package () {"HotPlugSupportInD3", 1}
      }
  })

This is also documented in:

  https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-pcie-root-ports-supporting-hot-plug-in-d3

Do the same in Linux by introducing new firmware PM callback (->bridge_d3())
and then implement it for ACPI based systems so that the above property is
checked.

There is one catch, though. The initial pci_dev->bridge_d3 is set before
the root port has ACPI companion bound (the device is not added to the
PCI bus either) so we need to look up the ACPI companion manually in
that case in acpi_pci_bridge_d3().

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/property.c |  3 +++
 drivers/pci/pci-acpi.c  | 41 +++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.c       |  9 +++++++++
 drivers/pci/pci.h       |  3 +++
 4 files changed, 56 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index f3ddb279d2de..f9d63ee9f623 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -28,6 +28,9 @@ static const guid_t prp_guids[] = {
 	/* ACPI _DSD device properties GUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
 	GUID_INIT(0xdaffd814, 0x6eba, 0x4d8c,
 		  0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01),
+	/* Hotplug in D3 GUID: 6211e2c0-58a3-4af3-90e1-927a4e0c55a4 */
+	GUID_INIT(0x6211e2c0, 0x58a3, 0x4af3,
+		  0x90, 0xe1, 0x92, 0x7a, 0x4e, 0x0c, 0x55, 0xa4),
 };
 
 static const guid_t ads_guid =
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index a4a8c02deaa0..c0394232fd93 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -519,6 +519,46 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
 	return PCI_POWER_ERROR;
 }
 
+static struct acpi_device *acpi_pci_find_companion(struct device *dev);
+
+static bool acpi_pci_bridge_d3(struct pci_dev *dev)
+{
+	const struct fwnode_handle *fwnode;
+	struct acpi_device *adev;
+	struct pci_dev *root;
+	u8 val;
+
+	if (!dev->is_hotplug_bridge)
+		return false;
+
+	/*
+	 * Look for a special _DSD property for the root port and if it
+	 * is set we know the hierarchy behind it supports D3 just fine.
+	 */
+	root = pci_find_pcie_root_port(dev);
+	if (!root)
+		return false;
+
+	adev = ACPI_COMPANION(&root->dev);
+	if (root == dev) {
+		/*
+		 * It is possible that the ACPI companion is not yet bound
+		 * for the root port so look it up manually here.
+		 */
+		if (!adev && !pci_dev_is_added(root))
+			adev = acpi_pci_find_companion(&root->dev);
+	}
+
+	if (!adev)
+		return false;
+
+	fwnode = acpi_fwnode_handle(adev);
+	if (fwnode_property_read_u8(fwnode, "HotPlugSupportInD3", &val))
+		return false;
+
+	return val == 1;
+}
+
 static bool acpi_pci_power_manageable(struct pci_dev *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
@@ -635,6 +675,7 @@ static bool acpi_pci_need_resume(struct pci_dev *dev)
 }
 
 static const struct pci_platform_pm_ops acpi_pci_platform_pm = {
+	.bridge_d3 = acpi_pci_bridge_d3,
 	.is_manageable = acpi_pci_power_manageable,
 	.set_state = acpi_pci_set_power_state,
 	.get_state = acpi_pci_get_power_state,
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 074f3f0253f9..ce212eb10be9 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -791,6 +791,11 @@ static inline bool platform_pci_need_resume(struct pci_dev *dev)
 	return pci_platform_pm ? pci_platform_pm->need_resume(dev) : false;
 }
 
+static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
+{
+	return pci_platform_pm ? pci_platform_pm->bridge_d3(dev) : false;
+}
+
 /**
  * pci_raw_set_power_state - Use PCI PM registers to set the power state of
  *                           given PCI device
@@ -2513,6 +2518,10 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 		if (bridge->is_thunderbolt)
 			return true;
 
+		/* Platform might know better if the bridge supports D3 */
+		if (platform_pci_bridge_d3(bridge))
+			return true;
+
 		/*
 		 * Hotplug ports handled natively by the OS were not validated
 		 * by vendors for runtime D3 at least until 2018 because there
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6e0d1528d471..66fd5c1bf71b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -39,6 +39,8 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
 /**
  * struct pci_platform_pm_ops - Firmware PM callbacks
  *
+ * @bridge_d3: Does the bridge allow entering into D3
+ *
  * @is_manageable: returns 'true' if given device is power manageable by the
  *		   platform firmware
  *
@@ -60,6 +62,7 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
  * these callbacks are mandatory.
  */
 struct pci_platform_pm_ops {
+	bool (*bridge_d3)(struct pci_dev *dev);
 	bool (*is_manageable)(struct pci_dev *dev);
 	int (*set_state)(struct pci_dev *dev, pci_power_t state);
 	pci_power_t (*get_state)(struct pci_dev *dev);
-- 
2.18.0

Re: [PATCH 04/10] PCI: pciehp: Do not handle events if interrupts are masked

[Lukas Wunner]

On Thu, Sep 06, 2018 at 06:50:14PM +0300, Mika Westerberg wrote:
> PCIe native hotplug shares MSI vector with native PME so the interrupt
> handler might get called even the hotplug interrupt is masked. In that
> case we should not handle any events because the interrupt was not meant
> for us. Modify the PCIe hotplug interrupt handler to check this
> accordingly and bail out if it finds out that the interrupt was not
> about hotplug.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Re: [PATCH 04/10] PCI: pciehp: Do not handle events if interrupts are masked

[Keith Busch]

On Thu, Sep 06, 2018 at 06:50:14PM +0300, Mika Westerberg wrote:
> PCIe native hotplug shares MSI vector with native PME so the interrupt
> handler might get called even the hotplug interrupt is masked. In that
> case we should not handle any events because the interrupt was not meant
> for us. Modify the PCIe hotplug interrupt handler to check this
> accordingly and bail out if it finds out that the interrupt was not
> about hotplug.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/hotplug/pciehp_hpc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 2249c4d06efd..19ed13d44b8f 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -533,9 +533,11 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
>  	u16 status, events;
>  
>  	/*
> -	 * Interrupts only occur in D3hot or shallower (PCIe r4.0, sec 6.7.3.4).
> +	 * Interrupts only occur in D3hot or shallower and only if enabled
> +	 * in the Slot Control register (PCIe r4.0, sec 6.7.3.4).
>  	 */
> -	if (pdev->current_state == PCI_D3cold)
> +	if (pdev->current_state == PCI_D3cold ||
> +	    !(ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE))
>  		return IRQ_NONE;
>  
>  	/*

Isn't this going to break pciehp_poll_mode?

Re: [PATCH 04/10] PCI: pciehp: Do not handle events if interrupts are masked

[Lukas Wunner]

On Fri, Sep 07, 2018 at 04:45:46PM -0600, Keith Busch wrote:
> On Thu, Sep 06, 2018 at 06:50:14PM +0300, Mika Westerberg wrote:
> > PCIe native hotplug shares MSI vector with native PME so the interrupt
> > handler might get called even the hotplug interrupt is masked. In that
> > case we should not handle any events because the interrupt was not meant
> > for us. Modify the PCIe hotplug interrupt handler to check this
> > accordingly and bail out if it finds out that the interrupt was not
> > about hotplug.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/pci/hotplug/pciehp_hpc.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> > index 2249c4d06efd..19ed13d44b8f 100644
> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -533,9 +533,11 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
> >  	u16 status, events;
> >  
> >  	/*
> > -	 * Interrupts only occur in D3hot or shallower (PCIe r4.0, sec 6.7.3.4).
> > +	 * Interrupts only occur in D3hot or shallower and only if enabled
> > +	 * in the Slot Control register (PCIe r4.0, sec 6.7.3.4).
> >  	 */
> > -	if (pdev->current_state == PCI_D3cold)
> > +	if (pdev->current_state == PCI_D3cold ||
> > +	    !(ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE))
> >  		return IRQ_NONE;
> >  
> >  	/*
> 
> Isn't this going to break pciehp_poll_mode?

Excellent catch, you're right, it needs to be constrained to
!pciehp_poll_mode.

Re: [PATCH 04/10] PCI: pciehp: Do not handle events if interrupts are masked

[Mika Westerberg]

On Sat, Sep 08, 2018 at 08:16:17AM +0200, Lukas Wunner wrote:
> On Fri, Sep 07, 2018 at 04:45:46PM -0600, Keith Busch wrote:
> > On Thu, Sep 06, 2018 at 06:50:14PM +0300, Mika Westerberg wrote:
> > > PCIe native hotplug shares MSI vector with native PME so the interrupt
> > > handler might get called even the hotplug interrupt is masked. In that
> > > case we should not handle any events because the interrupt was not meant
> > > for us. Modify the PCIe hotplug interrupt handler to check this
> > > accordingly and bail out if it finds out that the interrupt was not
> > > about hotplug.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  drivers/pci/hotplug/pciehp_hpc.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> > > index 2249c4d06efd..19ed13d44b8f 100644
> > > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > > @@ -533,9 +533,11 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
> > >  	u16 status, events;
> > >  
> > >  	/*
> > > -	 * Interrupts only occur in D3hot or shallower (PCIe r4.0, sec 6.7.3.4).
> > > +	 * Interrupts only occur in D3hot or shallower and only if enabled
> > > +	 * in the Slot Control register (PCIe r4.0, sec 6.7.3.4).
> > >  	 */
> > > -	if (pdev->current_state == PCI_D3cold)
> > > +	if (pdev->current_state == PCI_D3cold ||
> > > +	    !(ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE))
> > >  		return IRQ_NONE;
> > >  
> > >  	/*
> > 
> > Isn't this going to break pciehp_poll_mode?
> 
> Excellent catch, you're right, it needs to be constrained to
> !pciehp_poll_mode.

OK, I'll add the check to v2.

Re: [PATCH 05/10] PCI: portdrv: Resume upon exit from system suspend if left runtime suspended

[Rafael J. Wysocki]

On Thu, Sep 6, 2018 at 5:50 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Currently we try to keep PCIe ports runtime suspended over system
> suspend if possible. This mostly happens when entering suspend-to-idle
> because there is no need to re-configure wake settings.
>
> This causes problems if the parent port goes into D3cold and it gets
> resumed upon exit from system suspend. This may happen for example if
> the port is part of PCIe switch and the same switch is connected to a
> PCIe endpoint that needs to be resumed. The way exit from D3cold works
> according PCIe 4.0 spec 5.3.1.4.2 is that power is restored and cold
> reset is signaled. After this the device is in D0unitialized state
> keeping PME context if it supports wake from D3cold.
>
> The problem occurs when a PCIe hotplug port is left suspended and the
> parent port goes into D3cold and back to D0, the port keeps its PME
> context but since everything else is reset back to defaults
> (D0unitialized) it is not set to detect hotplug events anymore.
>
> For this reason change the PCIe portdrv power management logic so that
> it is fine to keep the port runtime suspended over system suspend but it
> needs to be resumed upon exit to make sure it gets properly re-initialized.
> The custom ->prepare() hook with DPM_FLAG_SMART_PREPARE is needed
> because otherwise pci_pm_prepare() instructs the PM core to go directly
> to pci_pm_complete() on resume and this skips resuming the port.

Thanks for the detailed explanation, it helps quite a bit!

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pcie/portdrv_pci.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index eef22dc29140..74761f660a30 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -43,6 +43,21 @@ __setup("pcie_ports=", pcie_port_setup);
>  /* global data */
>
>  #ifdef CONFIG_PM
> +int pcie_port_prepare(struct device *dev)
> +{
> +       /*
> +        * Return 0 here to indicate PCI core that:
> +        *   - Direct complete path should be avoided
> +        *   - It is OK to leave the port runtime suspended over system
> +        *     suspend
> +        *
> +        * However, the port needs to be resumed afterwards because it may
> +        * have been in D3cold in which case we need to re-initialize the
> +        * hardware as it is in D0uninitialized in that case.
> +        */
> +       return 0;
> +}

You wouldn't need this if you passed DPM_FLAG_NEVER_SKIP to
dev_pm_set_driver_flags() (instead of DPM_FLAG_SMART_SUSPEND), would
you?

> +
>  static int pcie_port_runtime_suspend(struct device *dev)
>  {
>         return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
> @@ -64,6 +79,7 @@ static int pcie_port_runtime_idle(struct device *dev)
>  }
>
>  static const struct dev_pm_ops pcie_portdrv_pm_ops = {
> +       .prepare        = pcie_port_prepare,
>         .suspend        = pcie_port_device_suspend,
>         .resume_noirq   = pcie_port_device_resume_noirq,
>         .resume         = pcie_port_device_resume,
> @@ -109,8 +125,8 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>
>         pci_save_state(dev);
>
> -       dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_SMART_SUSPEND |
> -                                          DPM_FLAG_LEAVE_SUSPENDED);
> +       dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_SMART_PREPARE |
> +                                          DPM_FLAG_SMART_SUSPEND);
>
>         if (pci_bridge_d3_possible(dev)) {
>                 /*
> --
> 2.18.0
>

Re: [PATCH 05/10] PCI: portdrv: Resume upon exit from system suspend if left runtime suspended

[Lukas Wunner]

On Thu, Sep 06, 2018 at 06:50:15PM +0300, Mika Westerberg wrote:
> Currently we try to keep PCIe ports runtime suspended over system
> suspend if possible. This mostly happens when entering suspend-to-idle
> because there is no need to re-configure wake settings.
> 
> This causes problems if the parent port goes into D3cold and it gets
> resumed upon exit from system suspend. This may happen for example if
> the port is part of PCIe switch and the same switch is connected to a
> PCIe endpoint that needs to be resumed. The way exit from D3cold works
> according PCIe 4.0 spec 5.3.1.4.2 is that power is restored and cold
> reset is signaled. After this the device is in D0unitialized state
> keeping PME context if it supports wake from D3cold.
> 
> The problem occurs when a PCIe hotplug port is left suspended and the
> parent port goes into D3cold and back to D0, the port keeps its PME
> context but since everything else is reset back to defaults
> (D0unitialized) it is not set to detect hotplug events anymore.

We call pci_wakeup_bus() in __pci_start_power_transition() for this
reason.  Why isn't that sufficient in your use case?


> For this reason change the PCIe portdrv power management logic so that
> it is fine to keep the port runtime suspended over system suspend but it
> needs to be resumed upon exit to make sure it gets properly re-initialized.
> The custom ->prepare() hook with DPM_FLAG_SMART_PREPARE is needed
> because otherwise pci_pm_prepare() instructs the PM core to go directly
> to pci_pm_complete() on resume and this skips resuming the port.

On Macs, if no Thunderbolt device is attached, it is perfectly okay to
use direct complete and it is also perfectly okay to leave the entire
controller (including all its PCIe ports) in D3cold when coming out of
system sleep.  In fact it would be unnecessary and undesirable to
runtime resume the controller, it would just waste energy for no reason.

Can you make sure that ports stay runtime suspended after system sleep
in that case?

Thanks,

Lukas

Re: [PATCH 01/10] PCI: Do not skip power managed bridges in pci_enable_wake()

[Rafael J. Wysocki]

On Thursday, September 6, 2018 5:50:11 PM CEST Mika Westerberg wrote:
> Commit baecc470d5fd ("PCI / PM: Skip bridges in pci_enable_wake()")
> changed pci_enable_wake() so that all bridges are skipped when wakeup is
> enabled (or disabled) with the reasoning that bridges can only signal
> wakeup on behalf of their subordinate devices.
> 
> However, there are bridges that can signal wakeup itself. For example
> PCIe downstream and root ports supporting hotplug may signal wakeup upon
> hotplug event.
> 
> For this reason change pci_enable_wake() so that it skips all bridges
> except those that we power manage (->bridge_d3 is set). Those are the
> ones that can go into low power states and may need to signal wakeup.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pci.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 29ff9619b5fa..074f3f0253f9 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2134,9 +2134,11 @@ static int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable
>  
>  	/*
>  	 * Bridges can only signal wakeup on behalf of subordinate devices,
> -	 * but that is set up elsewhere, so skip them.
> +	 * but that is set up elsewhere, so skip them. With the exception
> +	 * of bridges that we power manage. These can signal wake for
> +	 * example on a hotplug event.
>  	 */

I would change the comment even more, to something like:

"Bridges that are not power-manageable directly only signal wakeup on behalf
of subordinate devices which is set up elsewhere, so skip them.  However,
bridges that are power-manageable may signal wakeup for themselves (for example,
on a hotplug event) and they need to be covered here."

> -	if (pci_has_subordinate(dev))
> +	if (!pci_power_manageable(dev))
>  		return 0;
>  
>  	/* Don't do the same thing twice in a row for one device. */
> 

Re: [PATCH 02/10] PCI / ACPI: Enable wake automatically for power managed bridges

[Rafael J. Wysocki]

On Thursday, September 6, 2018 5:50:12 PM CEST Mika Westerberg wrote:
> We enable power management automatically for bridges where
> pci_bridge_d3_possible() returns true. However, these bridges may have
> ACPI methods such as _DSW that need to be called before D3 entry. For
> example in Lenovo Thinkpad X1 Carbon 6th _DSW method is used to prepare
> D3cold for the PCIe root port hosting Thunderbolt chain. Because wake is
> not enabled _DSW method is never called and the port does not enter
> D3cold properly consuming more power than necessary.
> 
> Users can work this around by writing "enabled" to "wakeup" sysfs file
> under the device in question but that is not something an ordinary user
> is expected to do.
> 
> Since we already automatically enable power management for PCIe ports
> with ->bridge_d3 set extend that to enable wake for them as well,
> assuming the port has any ACPI wakeup related objects implemented in the
> namespace (adev->wakeup.flags.valid is true). This ensures the necessary
> ACPI methods get called at appropriate times and allows the root port in
> Thinkpad X1 Carbon 6th to go into D3cold.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pci-acpi.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index c2ab57705043..a4a8c02deaa0 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -762,19 +762,32 @@ static void pci_acpi_setup(struct device *dev)
>  		return;
>  
>  	device_set_wakeup_capable(dev, true);
> +	/*
> +	 * For bridges that can do D3 we enable wake automatically (as
> +	 * we do for the power management itself in that case). The
> +	 * reason is that the bridge may have additional methods such as
> +	 * _DSW that need to be called.
> +	 */
> +	if (pci_dev->bridge_d3)
> +		device_wakeup_enable(dev);
> +
>  	acpi_pci_wakeup(pci_dev, false);
>  }
>  
>  static void pci_acpi_cleanup(struct device *dev)
>  {
>  	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	struct pci_dev *pci_dev = to_pci_dev(dev);
>  
>  	if (!adev)
>  		return;
>  
>  	pci_acpi_remove_pm_notifier(adev);
> -	if (adev->wakeup.flags.valid)
> +	if (adev->wakeup.flags.valid) {
> +		if (pci_dev->bridge_d3)
> +			device_wakeup_disable(dev);

Add an empty line here?

>  		device_set_wakeup_capable(dev, false);
> +	}
>  }
>  
>  static bool pci_acpi_bus_match(struct device *dev)
> 

Apart from the above nit this is fine by me:

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

Re: [PATCH 03/10] PCI: pciehp: Disable hotplug interrupt during suspend

[Rafael J. Wysocki]

On Thursday, September 6, 2018 5:50:13 PM CEST Mika Westerberg wrote:
> When PCIe hotplug port is transitioned into D3hot link to the downstream
> component will go down. If hotplug interrupt generation is enabled when
> that happens it will trigger immediately waking up the system and
> bringing the link back up.
> 
> To prevent this disable hotplug interrupt generation when system suspend
> is entered. This does not prevent wakeup from low power states according
> to PCIe 4.0 spec section 6.7.3.4:
> 
>   Software enables a hot-plug event to generate a wakeup event by
>   enabling software notification of the event as described in Section
>   6.7.3.1. Note that in order for software to disable interrupt generation
>   while keeping wakeup generation enabled, the Hot-Plug Interrupt Enable
>   bit must be cleared.
> 
> So as long as we have set the slot event mask accordingly wakeup should
> work even if slot interrupt is disabled. The port should trigger wake
> and then send PME to the root port when the PCIe hierarchy is brought
> back up.
> 
> Limit this to systems using native PME mechanism to make sure older
> Apple systems depending on commit e3354628c376 ("PCI: pciehp: Support
> interrupts sent from D3hot") still continue working.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/hotplug/pciehp.h      |  2 ++
>  drivers/pci/hotplug/pciehp_core.c | 18 ++++++++++++++++++
>  drivers/pci/hotplug/pciehp_hpc.c  | 10 ++++++++++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 811cf83f956d..58bddeb3c5a9 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -187,6 +187,8 @@ struct controller *pcie_init(struct pcie_device *dev);
>  int pcie_init_notification(struct controller *ctrl);
>  void pcie_shutdown_notification(struct controller *ctrl);
>  void pcie_clear_hotplug_events(struct controller *ctrl);
> +void pcie_enable_interrupt(struct controller *ctrl);
> +void pcie_disable_interrupt(struct controller *ctrl);
>  int pciehp_power_on_slot(struct slot *slot);
>  void pciehp_power_off_slot(struct slot *slot);
>  void pciehp_get_power_status(struct slot *slot, u8 *status);
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index ec48c9433ae5..c9cf9c3b5f7f 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -302,8 +302,23 @@ static void pciehp_remove(struct pcie_device *dev)
>  }
>  
>  #ifdef CONFIG_PM
> +static bool pme_is_native(struct pcie_device *dev)
> +{
> +	const struct pci_host_bridge *host;
> +
> +	host = pci_find_host_bridge(dev->port->bus);
> +	return pcie_ports_native || host->native_pme;
> +}
> +
>  static int pciehp_suspend(struct pcie_device *dev)
>  {
> +	/*
> +	 * Disable hotplug interrupt so that it does not trigger
> +	 * immediately when the downstream link goes down.
> +	 */
> +	if (pme_is_native(dev))
> +		pcie_disable_interrupt(get_service_data(dev));
> +
>  	return 0;
>  }
>  
> @@ -327,6 +342,9 @@ static int pciehp_resume(struct pcie_device *dev)
>  {
>  	struct controller *ctrl = get_service_data(dev);
>  
> +	if (pme_is_native(dev))
> +		pcie_enable_interrupt(ctrl);
> +
>  	pciehp_check_presence(ctrl);
>  
>  	return 0;
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 7136e3430925..2249c4d06efd 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -748,6 +748,16 @@ void pcie_clear_hotplug_events(struct controller *ctrl)
>  				   PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC);
>  }
>  
> +void pcie_enable_interrupt(struct controller *ctrl)
> +{
> +	pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_HPIE, PCI_EXP_SLTCTL_HPIE);
> +}
> +
> +void pcie_disable_interrupt(struct controller *ctrl)
> +{
> +	pcie_write_cmd(ctrl, 0, PCI_EXP_SLTCTL_HPIE);
> +}
> +
>  /*
>   * pciehp has a 1:1 bus:slot relationship so we ultimately want a secondary
>   * bus reset of the bridge, but at the same time we want to ensure that it is
> 

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

Re: [PATCH 06/10] PCI: portdrv: Add runtime PM hooks for port service drivers

[Rafael J. Wysocki]

On Thursday, September 6, 2018 5:50:16 PM CEST Mika Westerberg wrote:
> When PCIe port is runtime suspended/resumed some extra steps might be
> needed to be executed from the port service driver side. For instance we
> may need to disable PCIe hotplug interrupt to prevent it from triggering
> immediately when PCIe link to the downstream component goes down.
> 
> To make the above possible add optional ->runtime_suspend() and
> ->runtime_resume() callbacks to struct pcie_port_service_driver and call
> them for each port service in runtime suspend/resume callbacks of portdrv.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pcie/portdrv.h      |  4 ++++
>  drivers/pci/pcie/portdrv_core.c | 20 ++++++++++++++++++++
>  drivers/pci/pcie/portdrv_pci.c  | 10 ++++------
>  3 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
> index d59afa42fc14..72fa132060e5 100644
> --- a/drivers/pci/pcie/portdrv.h
> +++ b/drivers/pci/pcie/portdrv.h
> @@ -52,6 +52,8 @@ struct pcie_port_service_driver {
>  	int (*suspend) (struct pcie_device *dev);
>  	int (*resume_noirq) (struct pcie_device *dev);
>  	int (*resume) (struct pcie_device *dev);
> +	int (*runtime_suspend) (struct pcie_device *dev);
> +	int (*runtime_resume) (struct pcie_device *dev);
>  
>  	/* Device driver may resume normal operations */
>  	void (*error_resume)(struct pci_dev *dev);
> @@ -85,6 +87,8 @@ int pcie_port_device_register(struct pci_dev *dev);
>  int pcie_port_device_suspend(struct device *dev);
>  int pcie_port_device_resume_noirq(struct device *dev);
>  int pcie_port_device_resume(struct device *dev);
> +int pcie_port_device_runtime_suspend(struct device *dev);
> +int pcie_port_device_runtime_resume(struct device *dev);
>  #endif
>  void pcie_port_device_remove(struct pci_dev *dev);
>  int __must_check pcie_port_bus_register(void);
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 7c37d815229e..6542c48c7f59 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -395,6 +395,26 @@ int pcie_port_device_resume(struct device *dev)
>  	size_t off = offsetof(struct pcie_port_service_driver, resume);
>  	return device_for_each_child(dev, &off, pm_iter);
>  }
> +
> +/**
> + * pcie_port_device_runtime_suspend - runtime suspend port services
> + * @dev: PCI Express port to handle
> + */
> +int pcie_port_device_runtime_suspend(struct device *dev)
> +{
> +	size_t off = offsetof(struct pcie_port_service_driver, runtime_suspend);
> +	return device_for_each_child(dev, &off, pm_iter);
> +}
> +
> +/**
> + * pcie_port_device_runtime_resume - runtime resume port services
> + * @dev: PCI Express port to handle
> + */
> +int pcie_port_device_runtime_resume(struct device *dev)
> +{
> +	size_t off = offsetof(struct pcie_port_service_driver, runtime_resume);
> +	return device_for_each_child(dev, &off, pm_iter);
> +}
>  #endif /* PM */
>  
>  static int remove_iter(struct device *dev, void *data)
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 74761f660a30..d018633694e0 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -60,12 +60,10 @@ int pcie_port_prepare(struct device *dev)
>  
>  static int pcie_port_runtime_suspend(struct device *dev)
>  {
> -	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
> -}
> +	if (!to_pci_dev(dev)->bridge_d3)
> +		return -EBUSY;
>  
> -static int pcie_port_runtime_resume(struct device *dev)
> -{
> -	return 0;
> +	return pcie_port_device_runtime_suspend(dev);
>  }
>  
>  static int pcie_port_runtime_idle(struct device *dev)
> @@ -89,7 +87,7 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
>  	.restore_noirq	= pcie_port_device_resume_noirq,
>  	.restore	= pcie_port_device_resume,
>  	.runtime_suspend = pcie_port_runtime_suspend,
> -	.runtime_resume	= pcie_port_runtime_resume,
> +	.runtime_resume	= pcie_port_device_runtime_resume,
>  	.runtime_idle	= pcie_port_runtime_idle,
>  };
>  
> 

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

Re: [PATCH 07/10] PCI: pciehp: Implement runtime PM callbacks

[Rafael J. Wysocki]

On Thursday, September 6, 2018 5:50:17 PM CEST Mika Westerberg wrote:
> Basically we need to do the same thing when runtime suspending than with
> system sleep so re-use those operations here. This makes sure hotplug
> interrupt does not trigger immediately when the link goes down.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/hotplug/pciehp_core.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index c9cf9c3b5f7f..25b2ea9f9242 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -349,6 +349,23 @@ static int pciehp_resume(struct pcie_device *dev)
>  
>  	return 0;
>  }
> +
> +static int pciehp_runtime_resume(struct pcie_device *dev)
> +{
> +	struct controller *ctrl = get_service_data(dev);
> +	struct slot *slot = ctrl->slot;
> +
> +	/* pci_restore_state() just wrote to the Slot Control register */
> +	ctrl->cmd_started = jiffies;
> +	ctrl->cmd_busy = true;
> +
> +	/* clear spurious events from rediscovery of inserted card */
> +	if ((slot->state == ON_STATE || slot->state == BLINKINGOFF_STATE) &&
> +	     pme_is_native(dev))
> +		pcie_clear_hotplug_events(ctrl);
> +
> +	return pciehp_resume(dev);
> +}
>  #endif /* PM */
>  
>  static struct pcie_port_service_driver hpdriver_portdrv = {
> @@ -363,6 +380,8 @@ static struct pcie_port_service_driver hpdriver_portdrv = {
>  	.suspend	= pciehp_suspend,
>  	.resume_noirq	= pciehp_resume_noirq,
>  	.resume		= pciehp_resume,
> +	.runtime_suspend = pciehp_suspend,
> +	.runtime_resume	= pciehp_runtime_resume,
>  #endif	/* PM */
>  };
>  
> 

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

Re: [PATCH 08/10] PCI/PME: Implement runtime PM callbacks

[Rafael J. Wysocki]

On Thursday, September 6, 2018 5:50:18 PM CEST Mika Westerberg wrote:
> Basically we need to do the same steps than what we do when system sleep
> is entered and disable PME interrupt when the root port is runtime
> suspended. This prevents spurious wakeups immediately when the port is
> transitioned into D3cold.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pcie/pme.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
> index 3ed67676ea2a..62fe122f030a 100644
> --- a/drivers/pci/pcie/pme.c
> +++ b/drivers/pci/pcie/pme.c
> @@ -432,6 +432,31 @@ static void pcie_pme_remove(struct pcie_device *srv)
>  	kfree(get_service_data(srv));
>  }
>  
> +static int pcie_pme_runtime_suspend(struct pcie_device *srv)
> +{
> +	struct pcie_pme_service_data *data = get_service_data(srv);
> +
> +	spin_lock_irq(&data->lock);
> +	pcie_pme_interrupt_enable(srv->port, false);
> +	pcie_clear_root_pme_status(srv->port);
> +	data->noirq = true;
> +	spin_unlock_irq(&data->lock);
> +
> +	return 0;
> +}
> +
> +static int pcie_pme_runtime_resume(struct pcie_device *srv)
> +{
> +	struct pcie_pme_service_data *data = get_service_data(srv);
> +
> +	spin_lock_irq(&data->lock);
> +	pcie_pme_interrupt_enable(srv->port, true);
> +	data->noirq = false;
> +	spin_unlock_irq(&data->lock);
> +
> +	return 0;
> +}
> +
>  static struct pcie_port_service_driver pcie_pme_driver = {
>  	.name		= "pcie_pme",
>  	.port_type	= PCI_EXP_TYPE_ROOT_PORT,
> @@ -439,6 +464,8 @@ static struct pcie_port_service_driver pcie_pme_driver = {
>  
>  	.probe		= pcie_pme_probe,
>  	.suspend	= pcie_pme_suspend,
> +	.runtime_suspend = pcie_pme_runtime_suspend,
> +	.runtime_resume	= pcie_pme_runtime_resume,
>  	.resume		= pcie_pme_resume,
>  	.remove		= pcie_pme_remove,
>  };
> 

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

Re: [PATCH 09/10] ACPI / property: Allow multiple property compatible _DSD entries

[Rafael J. Wysocki]

On Thursday, September 6, 2018 5:50:19 PM CEST Mika Westerberg wrote:
> It is possible to have _DSD entries where the data is compatible with device
> properties format but are using different GUID for various reasons. In addition
> to that there can be many such _DSD entries for a single device such as for
> PCIe root port used to host a Thunderbolt hierarchy:
> 
>     Scope (\_SB.PCI0.RP21)
>     {
>         Name (_DSD, Package () {
>             ToUUID ("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"),
>             Package () {
>                 Package () {"HotPlugSupportInD3", 1}
>             },
> 
>             ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"),
>             Package () {
>                 Package () {"ExternalFacingPort", 1},
>                 Package () {"UID", 0 }
>             }
>         })
>     }
> 
> For more information about these new _DSD entries can be found in:
> 
>   https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports
> 
> In order to make these available for drivers via unified device property
> APIs modify ACPI property core so that it supports multiple _DSD entries
> organized in a linked list. We also store GUID of each _DSD entry in
> struct acpi_device_properties in case there is need to differerentiate
> between entries. The supported GUIDs are then listed in prp_guids array.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/acpi/property.c     | 94 +++++++++++++++++++++++++++----------
>  drivers/acpi/x86/apple.c    |  2 +-
>  drivers/gpio/gpiolib-acpi.c |  2 +-
>  include/acpi/acpi_bus.h     |  8 +++-
>  include/linux/acpi.h        |  9 ++++
>  5 files changed, 86 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 693cf05b0cc4..f3ddb279d2de 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -24,11 +24,12 @@ static int acpi_data_get_property_array(const struct acpi_device_data *data,
>  					acpi_object_type type,
>  					const union acpi_object **obj);
>  
> -/* ACPI _DSD device properties GUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
> -static const guid_t prp_guid =
> +static const guid_t prp_guids[] = {
> +	/* ACPI _DSD device properties GUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
>  	GUID_INIT(0xdaffd814, 0x6eba, 0x4d8c,
> -		  0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01);
> -/* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
> +		  0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01),
> +};
> +
>  static const guid_t ads_guid =
>  	GUID_INIT(0xdbb8e3e6, 0x5886, 0x4ba6,
>  		  0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b);
> @@ -56,6 +57,7 @@ static bool acpi_nondev_subnode_extract(const union acpi_object *desc,
>  	dn->name = link->package.elements[0].string.pointer;
>  	dn->fwnode.ops = &acpi_data_fwnode_ops;
>  	dn->parent = parent;
> +	INIT_LIST_HEAD(&dn->data.properties);
>  	INIT_LIST_HEAD(&dn->data.subnodes);
>  
>  	result = acpi_extract_properties(desc, &dn->data);
> @@ -288,6 +290,35 @@ static void acpi_init_of_compatible(struct acpi_device *adev)
>  	adev->flags.of_compatible_ok = 1;
>  }
>  
> +static bool acpi_is_property_guid(const guid_t *guid)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(prp_guids); i++) {
> +		if (guid_equal(guid, &prp_guids[i]))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +struct acpi_device_properties *
> +acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
> +		    const union acpi_object *properties)
> +{
> +	struct acpi_device_properties *props;
> +
> +	props = kzalloc(sizeof(*props), GFP_KERNEL);
> +	if (props) {
> +		INIT_LIST_HEAD(&props->list);
> +		props->guid = guid;
> +		props->properties = properties;
> +		list_add_tail(&props->list, &data->properties);
> +	}
> +
> +	return props;
> +}
> +
>  static bool acpi_extract_properties(const union acpi_object *desc,
>  				    struct acpi_device_data *data)
>  {
> @@ -312,7 +343,7 @@ static bool acpi_extract_properties(const union acpi_object *desc,
>  		    properties->type != ACPI_TYPE_PACKAGE)
>  			break;
>  
> -		if (!guid_equal((guid_t *)guid->buffer.pointer, &prp_guid))
> +		if (!acpi_is_property_guid((guid_t *)guid->buffer.pointer))
>  			continue;
>  
>  		/*
> @@ -320,13 +351,13 @@ static bool acpi_extract_properties(const union acpi_object *desc,
>  		 * package immediately following it.
>  		 */
>  		if (!acpi_properties_format_valid(properties))
> -			break;
> +			continue;
>  
> -		data->properties = properties;
> -		return true;
> +		acpi_data_add_props(data, (const guid_t *)guid->buffer.pointer,
> +				    properties);
>  	}
>  
> -	return false;
> +	return !list_empty(&data->properties);
>  }
>  
>  void acpi_init_properties(struct acpi_device *adev)
> @@ -336,6 +367,7 @@ void acpi_init_properties(struct acpi_device *adev)
>  	acpi_status status;
>  	bool acpi_of = false;
>  
> +	INIT_LIST_HEAD(&adev->data.properties);
>  	INIT_LIST_HEAD(&adev->data.subnodes);
>  
>  	if (!adev->handle)
> @@ -398,11 +430,16 @@ static void acpi_destroy_nondev_subnodes(struct list_head *list)
>  
>  void acpi_free_properties(struct acpi_device *adev)
>  {
> +	struct acpi_device_properties *props, *tmp;
> +
>  	acpi_destroy_nondev_subnodes(&adev->data.subnodes);
>  	ACPI_FREE((void *)adev->data.pointer);
>  	adev->data.of_compatible = NULL;
>  	adev->data.pointer = NULL;
> -	adev->data.properties = NULL;
> +	list_for_each_entry_safe(props, tmp, &adev->data.properties, list) {
> +		list_del(&props->list);
> +		kfree(props);
> +	}
>  }
>  
>  /**
> @@ -427,32 +464,37 @@ static int acpi_data_get_property(const struct acpi_device_data *data,
>  				  const char *name, acpi_object_type type,
>  				  const union acpi_object **obj)
>  {
> -	const union acpi_object *properties;
> -	int i;
> +	struct acpi_device_properties *props;
>  
>  	if (!data || !name)
>  		return -EINVAL;
>  
> -	if (!data->pointer || !data->properties)
> +	if (!data->pointer || list_empty(&data->properties))
>  		return -EINVAL;
>  
> -	properties = data->properties;
> -	for (i = 0; i < properties->package.count; i++) {
> -		const union acpi_object *propname, *propvalue;
> -		const union acpi_object *property;
> +	list_for_each_entry(props, &data->properties, list) {
> +		const union acpi_object *properties;
> +		int i;
>  
> -		property = &properties->package.elements[i];
> +		properties = props->properties;
> +		for (i = 0; i < properties->package.count; i++) {
> +			const union acpi_object *propname, *propvalue;
> +			const union acpi_object *property;
>  
> -		propname = &property->package.elements[0];
> -		propvalue = &property->package.elements[1];
> +			property = &properties->package.elements[i];
>  
> -		if (!strcmp(name, propname->string.pointer)) {
> -			if (type != ACPI_TYPE_ANY && propvalue->type != type)
> -				return -EPROTO;
> -			if (obj)
> -				*obj = propvalue;
> +			propname = &property->package.elements[0];
> +			propvalue = &property->package.elements[1];
>  
> -			return 0;
> +			if (!strcmp(name, propname->string.pointer)) {
> +				if (type != ACPI_TYPE_ANY &&
> +				    propvalue->type != type)
> +					return -EPROTO;
> +				if (obj)
> +					*obj = propvalue;
> +
> +				return 0;
> +			}
>  		}
>  	}
>  	return -EINVAL;
> diff --git a/drivers/acpi/x86/apple.c b/drivers/acpi/x86/apple.c
> index 51b4cf9f25da..130df1c8ed7d 100644
> --- a/drivers/acpi/x86/apple.c
> +++ b/drivers/acpi/x86/apple.c
> @@ -132,8 +132,8 @@ void acpi_extract_apple_properties(struct acpi_device *adev)
>  	}
>  	WARN_ON(free_space != (void *)newprops + newsize);
>  
> -	adev->data.properties = newprops;
>  	adev->data.pointer = newprops;
> +	acpi_data_add_props(&adev->data, &apple_prp_guid, newprops);
>  
>  out_free:
>  	ACPI_FREE(props);
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index c48ed9d89ff5..7be705fae4b8 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -1192,7 +1192,7 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
>  bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
>  {
>  	/* Never allow fallback if the device has properties */
> -	if (adev->data.properties || adev->driver_gpios)
> +	if (acpi_dev_has_props(adev) || adev->driver_gpios)
>  		return false;
>  
>  	return con_id == NULL;
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index ba4dd54f2c82..cd35e3ce9a8b 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -346,10 +346,16 @@ struct acpi_device_physical_node {
>  	bool put_online:1;
>  };
>  
> +struct acpi_device_properties {
> +	const guid_t *guid;
> +	const union acpi_object *properties;
> +	struct list_head list;
> +};
> +
>  /* ACPI Device Specific Data (_DSD) */
>  struct acpi_device_data {
>  	const union acpi_object *pointer;
> -	const union acpi_object *properties;
> +	struct list_head properties;
>  	const union acpi_object *of_compatible;
>  	struct list_head subnodes;
>  };
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index de8d3d3fa651..51e3c29663fe 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1074,6 +1074,15 @@ static inline int acpi_node_get_property_reference(
>  		NR_FWNODE_REFERENCE_ARGS, args);
>  }
>  
> +static inline bool acpi_dev_has_props(const struct acpi_device *adev)
> +{
> +	return !list_empty(&adev->data.properties);
> +}
> +
> +struct acpi_device_properties *
> +acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
> +		    const union acpi_object *properties);
> +
>  int acpi_node_prop_get(const struct fwnode_handle *fwnode, const char *propname,
>  		       void **valptr);
>  int acpi_dev_prop_read_single(struct acpi_device *adev,
> 

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

Re: [PATCH 10/10] PCI / ACPI: Whitelist D3 for more PCIe hotplug ports

[Rafael J. Wysocki]

On Thursday, September 6, 2018 5:50:20 PM CEST Mika Westerberg wrote:
> In order to have better power management for Thunderbolt PCIe chains,
> Windows enables power management for native PCIe hotplug ports if there
> is following ACPI _DSD attached to the root port:
> 
>   Name (_DSD, Package () {
>       ToUUID ("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"),
>       Package () {
>           Package () {"HotPlugSupportInD3", 1}
>       }
>   })
> 
> This is also documented in:
> 
>   https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-pcie-root-ports-supporting-hot-plug-in-d3
> 
> Do the same in Linux by introducing new firmware PM callback (->bridge_d3())
> and then implement it for ACPI based systems so that the above property is
> checked.
> 
> There is one catch, though. The initial pci_dev->bridge_d3 is set before
> the root port has ACPI companion bound (the device is not added to the
> PCI bus either) so we need to look up the ACPI companion manually in
> that case in acpi_pci_bridge_d3().
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/acpi/property.c |  3 +++
>  drivers/pci/pci-acpi.c  | 41 +++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.c       |  9 +++++++++
>  drivers/pci/pci.h       |  3 +++
>  4 files changed, 56 insertions(+)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index f3ddb279d2de..f9d63ee9f623 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -28,6 +28,9 @@ static const guid_t prp_guids[] = {
>  	/* ACPI _DSD device properties GUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
>  	GUID_INIT(0xdaffd814, 0x6eba, 0x4d8c,
>  		  0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01),
> +	/* Hotplug in D3 GUID: 6211e2c0-58a3-4af3-90e1-927a4e0c55a4 */
> +	GUID_INIT(0x6211e2c0, 0x58a3, 0x4af3,
> +		  0x90, 0xe1, 0x92, 0x7a, 0x4e, 0x0c, 0x55, 0xa4),
>  };
>  
>  static const guid_t ads_guid =
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index a4a8c02deaa0..c0394232fd93 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -519,6 +519,46 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
>  	return PCI_POWER_ERROR;
>  }
>  
> +static struct acpi_device *acpi_pci_find_companion(struct device *dev);
> +
> +static bool acpi_pci_bridge_d3(struct pci_dev *dev)
> +{
> +	const struct fwnode_handle *fwnode;
> +	struct acpi_device *adev;
> +	struct pci_dev *root;
> +	u8 val;
> +
> +	if (!dev->is_hotplug_bridge)
> +		return false;
> +
> +	/*
> +	 * Look for a special _DSD property for the root port and if it
> +	 * is set we know the hierarchy behind it supports D3 just fine.
> +	 */
> +	root = pci_find_pcie_root_port(dev);
> +	if (!root)
> +		return false;
> +
> +	adev = ACPI_COMPANION(&root->dev);
> +	if (root == dev) {
> +		/*
> +		 * It is possible that the ACPI companion is not yet bound
> +		 * for the root port so look it up manually here.
> +		 */
> +		if (!adev && !pci_dev_is_added(root))
> +			adev = acpi_pci_find_companion(&root->dev);
> +	}
> +
> +	if (!adev)
> +		return false;
> +
> +	fwnode = acpi_fwnode_handle(adev);
> +	if (fwnode_property_read_u8(fwnode, "HotPlugSupportInD3", &val))
> +		return false;
> +
> +	return val == 1;
> +}
> +
>  static bool acpi_pci_power_manageable(struct pci_dev *dev)
>  {
>  	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
> @@ -635,6 +675,7 @@ static bool acpi_pci_need_resume(struct pci_dev *dev)
>  }
>  
>  static const struct pci_platform_pm_ops acpi_pci_platform_pm = {
> +	.bridge_d3 = acpi_pci_bridge_d3,
>  	.is_manageable = acpi_pci_power_manageable,
>  	.set_state = acpi_pci_set_power_state,
>  	.get_state = acpi_pci_get_power_state,
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 074f3f0253f9..ce212eb10be9 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -791,6 +791,11 @@ static inline bool platform_pci_need_resume(struct pci_dev *dev)
>  	return pci_platform_pm ? pci_platform_pm->need_resume(dev) : false;
>  }
>  
> +static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
> +{
> +	return pci_platform_pm ? pci_platform_pm->bridge_d3(dev) : false;
> +}
> +
>  /**
>   * pci_raw_set_power_state - Use PCI PM registers to set the power state of
>   *                           given PCI device
> @@ -2513,6 +2518,10 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>  		if (bridge->is_thunderbolt)
>  			return true;
>  
> +		/* Platform might know better if the bridge supports D3 */
> +		if (platform_pci_bridge_d3(bridge))
> +			return true;
> +
>  		/*
>  		 * Hotplug ports handled natively by the OS were not validated
>  		 * by vendors for runtime D3 at least until 2018 because there
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6e0d1528d471..66fd5c1bf71b 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -39,6 +39,8 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
>  /**
>   * struct pci_platform_pm_ops - Firmware PM callbacks
>   *
> + * @bridge_d3: Does the bridge allow entering into D3
> + *
>   * @is_manageable: returns 'true' if given device is power manageable by the
>   *		   platform firmware
>   *
> @@ -60,6 +62,7 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
>   * these callbacks are mandatory.
>   */
>  struct pci_platform_pm_ops {
> +	bool (*bridge_d3)(struct pci_dev *dev);
>  	bool (*is_manageable)(struct pci_dev *dev);
>  	int (*set_state)(struct pci_dev *dev, pci_power_t state);
>  	pci_power_t (*get_state)(struct pci_dev *dev);
> 

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

Re: [PATCH 05/10] PCI: portdrv: Resume upon exit from system suspend if left runtime suspended

[Mika Westerberg]

On Tue, Sep 11, 2018 at 10:29:35AM +0200, Lukas Wunner wrote:
> On Thu, Sep 06, 2018 at 06:50:15PM +0300, Mika Westerberg wrote:
> > Currently we try to keep PCIe ports runtime suspended over system
> > suspend if possible. This mostly happens when entering suspend-to-idle
> > because there is no need to re-configure wake settings.
> > 
> > This causes problems if the parent port goes into D3cold and it gets
> > resumed upon exit from system suspend. This may happen for example if
> > the port is part of PCIe switch and the same switch is connected to a
> > PCIe endpoint that needs to be resumed. The way exit from D3cold works
> > according PCIe 4.0 spec 5.3.1.4.2 is that power is restored and cold
> > reset is signaled. After this the device is in D0unitialized state
> > keeping PME context if it supports wake from D3cold.
> > 
> > The problem occurs when a PCIe hotplug port is left suspended and the
> > parent port goes into D3cold and back to D0, the port keeps its PME
> > context but since everything else is reset back to defaults
> > (D0unitialized) it is not set to detect hotplug events anymore.
> 
> We call pci_wakeup_bus() in __pci_start_power_transition() for this
> reason.  Why isn't that sufficient in your use case?

It would otherwise but __pci_start_power_transition() is never called
because the bridge is left suspended.

> > For this reason change the PCIe portdrv power management logic so that
> > it is fine to keep the port runtime suspended over system suspend but it
> > needs to be resumed upon exit to make sure it gets properly re-initialized.
> > The custom ->prepare() hook with DPM_FLAG_SMART_PREPARE is needed
> > because otherwise pci_pm_prepare() instructs the PM core to go directly
> > to pci_pm_complete() on resume and this skips resuming the port.
> 
> On Macs, if no Thunderbolt device is attached, it is perfectly okay to
> use direct complete and it is also perfectly okay to leave the entire
> controller (including all its PCIe ports) in D3cold when coming out of
> system sleep.  In fact it would be unnecessary and undesirable to
> runtime resume the controller, it would just waste energy for no reason.

I'm surprised if it works like that because both PCIe spec and
conventional PCI spec both require reset after D3cold and the only state
for a function after that is D0uninitialized.

Re: [PATCH 01/10] PCI: Do not skip power managed bridges in pci_enable_wake()

[Mika Westerberg]

On Tue, Sep 11, 2018 at 10:47:55AM +0200, Rafael J. Wysocki wrote:
> > @@ -2134,9 +2134,11 @@ static int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable
> >  
> >  	/*
> >  	 * Bridges can only signal wakeup on behalf of subordinate devices,
> > -	 * but that is set up elsewhere, so skip them.
> > +	 * but that is set up elsewhere, so skip them. With the exception
> > +	 * of bridges that we power manage. These can signal wake for
> > +	 * example on a hotplug event.
> >  	 */
> 
> I would change the comment even more, to something like:
> 
> "Bridges that are not power-manageable directly only signal wakeup on behalf
> of subordinate devices which is set up elsewhere, so skip them.  However,
> bridges that are power-manageable may signal wakeup for themselves (for example,
> on a hotplug event) and they need to be covered here."

OK, I'll update the comment accordingly.

Re: [PATCH 05/10] PCI: portdrv: Resume upon exit from system suspend if left runtime suspended

[Mika Westerberg]

On Tue, Sep 11, 2018 at 10:00:07AM +0200, Rafael J. Wysocki wrote:
> On Thu, Sep 6, 2018 at 5:50 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > Currently we try to keep PCIe ports runtime suspended over system
> > suspend if possible. This mostly happens when entering suspend-to-idle
> > because there is no need to re-configure wake settings.
> >
> > This causes problems if the parent port goes into D3cold and it gets
> > resumed upon exit from system suspend. This may happen for example if
> > the port is part of PCIe switch and the same switch is connected to a
> > PCIe endpoint that needs to be resumed. The way exit from D3cold works
> > according PCIe 4.0 spec 5.3.1.4.2 is that power is restored and cold
> > reset is signaled. After this the device is in D0unitialized state
> > keeping PME context if it supports wake from D3cold.
> >
> > The problem occurs when a PCIe hotplug port is left suspended and the
> > parent port goes into D3cold and back to D0, the port keeps its PME
> > context but since everything else is reset back to defaults
> > (D0unitialized) it is not set to detect hotplug events anymore.
> >
> > For this reason change the PCIe portdrv power management logic so that
> > it is fine to keep the port runtime suspended over system suspend but it
> > needs to be resumed upon exit to make sure it gets properly re-initialized.
> > The custom ->prepare() hook with DPM_FLAG_SMART_PREPARE is needed
> > because otherwise pci_pm_prepare() instructs the PM core to go directly
> > to pci_pm_complete() on resume and this skips resuming the port.
> 
> Thanks for the detailed explanation, it helps quite a bit!
> 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/pci/pcie/portdrv_pci.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> > index eef22dc29140..74761f660a30 100644
> > --- a/drivers/pci/pcie/portdrv_pci.c
> > +++ b/drivers/pci/pcie/portdrv_pci.c
> > @@ -43,6 +43,21 @@ __setup("pcie_ports=", pcie_port_setup);
> >  /* global data */
> >
> >  #ifdef CONFIG_PM
> > +int pcie_port_prepare(struct device *dev)
> > +{
> > +       /*
> > +        * Return 0 here to indicate PCI core that:
> > +        *   - Direct complete path should be avoided
> > +        *   - It is OK to leave the port runtime suspended over system
> > +        *     suspend
> > +        *
> > +        * However, the port needs to be resumed afterwards because it may
> > +        * have been in D3cold in which case we need to re-initialize the
> > +        * hardware as it is in D0uninitialized in that case.
> > +        */
> > +       return 0;
> > +}
> 
> You wouldn't need this if you passed DPM_FLAG_NEVER_SKIP to
> dev_pm_set_driver_flags() (instead of DPM_FLAG_SMART_SUSPEND), would
> you?

Yes, I think it would work. I did not use it here because I thought it
would be better to leave the port runtime suspended during system
suspend (to idle) but in both cases we end up resuming them at some
point (before or after system suspend).

DPM_FLAG_NEVER_SKIP would result a cleaner patch, I think.

Re: [PATCH 05/10] PCI: portdrv: Resume upon exit from system suspend if left runtime suspended

[Lukas Wunner]

On Tue, Sep 11, 2018 at 12:08:17PM +0300, Mika Westerberg wrote:
> On Tue, Sep 11, 2018 at 10:29:35AM +0200, Lukas Wunner wrote:
> > On Thu, Sep 06, 2018 at 06:50:15PM +0300, Mika Westerberg wrote:
> > > Currently we try to keep PCIe ports runtime suspended over system
> > > suspend if possible. This mostly happens when entering suspend-to-idle
> > > because there is no need to re-configure wake settings.
> > > 
> > > This causes problems if the parent port goes into D3cold and it gets
> > > resumed upon exit from system suspend. This may happen for example if
> > > the port is part of PCIe switch and the same switch is connected to a
> > > PCIe endpoint that needs to be resumed. The way exit from D3cold works
> > > according PCIe 4.0 spec 5.3.1.4.2 is that power is restored and cold
> > > reset is signaled. After this the device is in D0unitialized state
> > > keeping PME context if it supports wake from D3cold.
> > > 
> > > The problem occurs when a PCIe hotplug port is left suspended and the
> > > parent port goes into D3cold and back to D0, the port keeps its PME
> > > context but since everything else is reset back to defaults
> > > (D0unitialized) it is not set to detect hotplug events anymore.
> > 
> > We call pci_wakeup_bus() in __pci_start_power_transition() for this
> > reason.  Why isn't that sufficient in your use case?
> 
> It would otherwise but __pci_start_power_transition() is never called
> because the bridge is left suspended.

You write above that the parent goes to D0, now you say it is left
suspended.  Which one is it?


> > > For this reason change the PCIe portdrv power management logic so that
> > > it is fine to keep the port runtime suspended over system suspend but it
> > > needs to be resumed upon exit to make sure it gets properly re-initialized.
> > > The custom ->prepare() hook with DPM_FLAG_SMART_PREPARE is needed
> > > because otherwise pci_pm_prepare() instructs the PM core to go directly
> > > to pci_pm_complete() on resume and this skips resuming the port.
> > 
> > On Macs, if no Thunderbolt device is attached, it is perfectly okay to
> > use direct complete and it is also perfectly okay to leave the entire
> > controller (including all its PCIe ports) in D3cold when coming out of
> > system sleep.  In fact it would be unnecessary and undesirable to
> > runtime resume the controller, it would just waste energy for no reason.
> 
> I'm surprised if it works like that because both PCIe spec and
> conventional PCI spec both require reset after D3cold and the only state
> for a function after that is D0uninitialized.

The controller is in D3cold when coming out of system sleep if it was in
D3cold when sytem sleep commenced.

Thanks,

Lukas

Re: [PATCH 05/10] PCI: portdrv: Resume upon exit from system suspend if left runtime suspended

[Mika Westerberg]

On Tue, Sep 11, 2018 at 11:26:30AM +0200, Lukas Wunner wrote:
> On Tue, Sep 11, 2018 at 12:08:17PM +0300, Mika Westerberg wrote:
> > On Tue, Sep 11, 2018 at 10:29:35AM +0200, Lukas Wunner wrote:
> > > On Thu, Sep 06, 2018 at 06:50:15PM +0300, Mika Westerberg wrote:
> > > > Currently we try to keep PCIe ports runtime suspended over system
> > > > suspend if possible. This mostly happens when entering suspend-to-idle
> > > > because there is no need to re-configure wake settings.
> > > > 
> > > > This causes problems if the parent port goes into D3cold and it gets
> > > > resumed upon exit from system suspend. This may happen for example if
> > > > the port is part of PCIe switch and the same switch is connected to a
> > > > PCIe endpoint that needs to be resumed. The way exit from D3cold works
> > > > according PCIe 4.0 spec 5.3.1.4.2 is that power is restored and cold
> > > > reset is signaled. After this the device is in D0unitialized state
> > > > keeping PME context if it supports wake from D3cold.
> > > > 
> > > > The problem occurs when a PCIe hotplug port is left suspended and the
> > > > parent port goes into D3cold and back to D0, the port keeps its PME
> > > > context but since everything else is reset back to defaults
> > > > (D0unitialized) it is not set to detect hotplug events anymore.
> > > 
> > > We call pci_wakeup_bus() in __pci_start_power_transition() for this
> > > reason.  Why isn't that sufficient in your use case?
> > 
> > It would otherwise but __pci_start_power_transition() is never called
> > because the bridge is left suspended.
> 
> You write above that the parent goes to D0, now you say it is left
> suspended.  Which one is it?

The port below the parent is left suspended -- the hotplug port.

Once the upstream port of a PCIe switch is resumed to D0 it will be
reset and that reset is propagated to other ports in that switch so it
makes the hotplug port go to D0uninitialized as well.

> > > > For this reason change the PCIe portdrv power management logic so that
> > > > it is fine to keep the port runtime suspended over system suspend but it
> > > > needs to be resumed upon exit to make sure it gets properly re-initialized.
> > > > The custom ->prepare() hook with DPM_FLAG_SMART_PREPARE is needed
> > > > because otherwise pci_pm_prepare() instructs the PM core to go directly
> > > > to pci_pm_complete() on resume and this skips resuming the port.
> > > 
> > > On Macs, if no Thunderbolt device is attached, it is perfectly okay to
> > > use direct complete and it is also perfectly okay to leave the entire
> > > controller (including all its PCIe ports) in D3cold when coming out of
> > > system sleep.  In fact it would be unnecessary and undesirable to
> > > runtime resume the controller, it would just waste energy for no reason.
> > 
> > I'm surprised if it works like that because both PCIe spec and
> > conventional PCI spec both require reset after D3cold and the only state
> > for a function after that is D0uninitialized.
> 
> The controller is in D3cold when coming out of system sleep if it was in
> D3cold when sytem sleep commenced.

In that case if the whole chain is left in D3cold over system sleep
there is no issue. But at least with the Thunderbolt controllers I've
been dealing with, the xHCI (or the USB stack) that is part of the PCIe
switch wants to resume the controller and that also resumes the upstream
port and the root port which leads to this situation. I did not check
Apple systems, though but I thought they also include xHCI.

I guess the point is if there is any device in the hierarchy that needs
to be resumed, we end up in this situation.

Re: [PATCH 05/10] PCI: portdrv: Resume upon exit from system suspend if left runtime suspended

[Lukas Wunner]

On Tue, Sep 11, 2018 at 12:41:44PM +0300, Mika Westerberg wrote:
> On Tue, Sep 11, 2018 at 11:26:30AM +0200, Lukas Wunner wrote:
> > On Tue, Sep 11, 2018 at 12:08:17PM +0300, Mika Westerberg wrote:
> > > On Tue, Sep 11, 2018 at 10:29:35AM +0200, Lukas Wunner wrote:
> > > > On Thu, Sep 06, 2018 at 06:50:15PM +0300, Mika Westerberg wrote:
> > > > > Currently we try to keep PCIe ports runtime suspended over system
> > > > > suspend if possible. This mostly happens when entering suspend-to-idle
> > > > > because there is no need to re-configure wake settings.
> > > > > 
> > > > > This causes problems if the parent port goes into D3cold and it gets
> > > > > resumed upon exit from system suspend. This may happen for example if
> > > > > the port is part of PCIe switch and the same switch is connected to a
> > > > > PCIe endpoint that needs to be resumed. The way exit from D3cold works
> > > > > according PCIe 4.0 spec 5.3.1.4.2 is that power is restored and cold
> > > > > reset is signaled. After this the device is in D0unitialized state
> > > > > keeping PME context if it supports wake from D3cold.
> > > > > 
> > > > > The problem occurs when a PCIe hotplug port is left suspended and the
> > > > > parent port goes into D3cold and back to D0, the port keeps its PME
> > > > > context but since everything else is reset back to defaults
> > > > > (D0unitialized) it is not set to detect hotplug events anymore.
> > > > 
> > > > We call pci_wakeup_bus() in __pci_start_power_transition() for this
> > > > reason.  Why isn't that sufficient in your use case?
> > > 
> > > It would otherwise but __pci_start_power_transition() is never called
> > > because the bridge is left suspended.
> > 
> > You write above that the parent goes to D0, now you say it is left
> > suspended.  Which one is it?
> 
> The port below the parent is left suspended -- the hotplug port.
> 
> Once the upstream port of a PCIe switch is resumed to D0 it will be
> reset and that reset is propagated to other ports in that switch so it
> makes the hotplug port go to D0uninitialized as well.

Yes, but as said when the PCI core runtime resumes the upstream port to D0,
__pci_start_power_transition() should call pci_wakeup_bus() to wake all
devices on its subordinate bus, i.e. all the Downstream Ports, including
hotplug ports.  So they're runtime resumed as well and thus pass from
D0uninitialized to D0initialized.  Is pci_wakeup_bus() not called in your
case, and if so, why not?


> > > > > For this reason change the PCIe portdrv power management logic so that
> > > > > it is fine to keep the port runtime suspended over system suspend but it
> > > > > needs to be resumed upon exit to make sure it gets properly re-initialized.
> > > > > The custom ->prepare() hook with DPM_FLAG_SMART_PREPARE is needed
> > > > > because otherwise pci_pm_prepare() instructs the PM core to go directly
> > > > > to pci_pm_complete() on resume and this skips resuming the port.
> > > > 
> > > > On Macs, if no Thunderbolt device is attached, it is perfectly okay to
> > > > use direct complete and it is also perfectly okay to leave the entire
> > > > controller (including all its PCIe ports) in D3cold when coming out of
> > > > system sleep.  In fact it would be unnecessary and undesirable to
> > > > runtime resume the controller, it would just waste energy for no reason.
> > > 
> > > I'm surprised if it works like that because both PCIe spec and
> > > conventional PCI spec both require reset after D3cold and the only state
> > > for a function after that is D0uninitialized.
> > 
> > The controller is in D3cold when coming out of system sleep if it was in
> > D3cold when sytem sleep commenced.
> 
> In that case if the whole chain is left in D3cold over system sleep
> there is no issue. But at least with the Thunderbolt controllers I've
> been dealing with, the xHCI (or the USB stack) that is part of the PCIe
> switch wants to resume the controller and that also resumes the upstream
> port and the root port which leads to this situation. I did not check
> Apple systems, though but I thought they also include xHCI.

They do not if they're older than Alpine Ridge of course.

Thanks,

Lukas

Re: [PATCH 05/10] PCI: portdrv: Resume upon exit from system suspend if left runtime suspended

[Mika Westerberg]

On Tue, Sep 11, 2018 at 11:53:40AM +0200, Lukas Wunner wrote:
> On Tue, Sep 11, 2018 at 12:41:44PM +0300, Mika Westerberg wrote:
> > On Tue, Sep 11, 2018 at 11:26:30AM +0200, Lukas Wunner wrote:
> > > On Tue, Sep 11, 2018 at 12:08:17PM +0300, Mika Westerberg wrote:
> > > > On Tue, Sep 11, 2018 at 10:29:35AM +0200, Lukas Wunner wrote:
> > > > > On Thu, Sep 06, 2018 at 06:50:15PM +0300, Mika Westerberg wrote:
> > > > > > Currently we try to keep PCIe ports runtime suspended over system
> > > > > > suspend if possible. This mostly happens when entering suspend-to-idle
> > > > > > because there is no need to re-configure wake settings.
> > > > > > 
> > > > > > This causes problems if the parent port goes into D3cold and it gets
> > > > > > resumed upon exit from system suspend. This may happen for example if
> > > > > > the port is part of PCIe switch and the same switch is connected to a
> > > > > > PCIe endpoint that needs to be resumed. The way exit from D3cold works
> > > > > > according PCIe 4.0 spec 5.3.1.4.2 is that power is restored and cold
> > > > > > reset is signaled. After this the device is in D0unitialized state
> > > > > > keeping PME context if it supports wake from D3cold.
> > > > > > 
> > > > > > The problem occurs when a PCIe hotplug port is left suspended and the
> > > > > > parent port goes into D3cold and back to D0, the port keeps its PME
> > > > > > context but since everything else is reset back to defaults
> > > > > > (D0unitialized) it is not set to detect hotplug events anymore.
> > > > > 
> > > > > We call pci_wakeup_bus() in __pci_start_power_transition() for this
> > > > > reason.  Why isn't that sufficient in your use case?
> > > > 
> > > > It would otherwise but __pci_start_power_transition() is never called
> > > > because the bridge is left suspended.
> > > 
> > > You write above that the parent goes to D0, now you say it is left
> > > suspended.  Which one is it?
> > 
> > The port below the parent is left suspended -- the hotplug port.
> > 
> > Once the upstream port of a PCIe switch is resumed to D0 it will be
> > reset and that reset is propagated to other ports in that switch so it
> > makes the hotplug port go to D0uninitialized as well.
> 
> Yes, but as said when the PCI core runtime resumes the upstream port to D0,
> __pci_start_power_transition() should call pci_wakeup_bus() to wake all
> devices on its subordinate bus, i.e. all the Downstream Ports, including
> hotplug ports.  So they're runtime resumed as well and thus pass from
> D0uninitialized to D0initialized.  Is pci_wakeup_bus() not called in your
> case, and if so, why not?

If I read the PCI PM code right the way back to D0 happens like:

   pci_pm_resume_noirq()
     pci_pm_default_resume_early()
       pci_power_up()
         pci_raw_set_power_state(dev, PCI_D0)

To me it looks like __pci_start_power_transition() is not called in the
resume path.

Re: [PATCH 05/10] PCI: portdrv: Resume upon exit from system suspend if left runtime suspended

[Rafael J. Wysocki]

On Tuesday, September 11, 2018 11:15:20 AM CEST Mika Westerberg wrote:
> On Tue, Sep 11, 2018 at 10:00:07AM +0200, Rafael J. Wysocki wrote:
> > On Thu, Sep 6, 2018 at 5:50 PM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > Currently we try to keep PCIe ports runtime suspended over system
> > > suspend if possible. This mostly happens when entering suspend-to-idle
> > > because there is no need to re-configure wake settings.
> > >
> > > This causes problems if the parent port goes into D3cold and it gets
> > > resumed upon exit from system suspend. This may happen for example if
> > > the port is part of PCIe switch and the same switch is connected to a
> > > PCIe endpoint that needs to be resumed. The way exit from D3cold works
> > > according PCIe 4.0 spec 5.3.1.4.2 is that power is restored and cold
> > > reset is signaled. After this the device is in D0unitialized state
> > > keeping PME context if it supports wake from D3cold.
> > >
> > > The problem occurs when a PCIe hotplug port is left suspended and the
> > > parent port goes into D3cold and back to D0, the port keeps its PME
> > > context but since everything else is reset back to defaults
> > > (D0unitialized) it is not set to detect hotplug events anymore.
> > >
> > > For this reason change the PCIe portdrv power management logic so that
> > > it is fine to keep the port runtime suspended over system suspend but it
> > > needs to be resumed upon exit to make sure it gets properly re-initialized.
> > > The custom ->prepare() hook with DPM_FLAG_SMART_PREPARE is needed
> > > because otherwise pci_pm_prepare() instructs the PM core to go directly
> > > to pci_pm_complete() on resume and this skips resuming the port.
> > 
> > Thanks for the detailed explanation, it helps quite a bit!
> > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  drivers/pci/pcie/portdrv_pci.c | 20 ++++++++++++++++++--
> > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> > > index eef22dc29140..74761f660a30 100644
> > > --- a/drivers/pci/pcie/portdrv_pci.c
> > > +++ b/drivers/pci/pcie/portdrv_pci.c
> > > @@ -43,6 +43,21 @@ __setup("pcie_ports=", pcie_port_setup);
> > >  /* global data */
> > >
> > >  #ifdef CONFIG_PM
> > > +int pcie_port_prepare(struct device *dev)
> > > +{
> > > +       /*
> > > +        * Return 0 here to indicate PCI core that:
> > > +        *   - Direct complete path should be avoided
> > > +        *   - It is OK to leave the port runtime suspended over system
> > > +        *     suspend
> > > +        *
> > > +        * However, the port needs to be resumed afterwards because it may
> > > +        * have been in D3cold in which case we need to re-initialize the
> > > +        * hardware as it is in D0uninitialized in that case.
> > > +        */
> > > +       return 0;
> > > +}
> > 
> > You wouldn't need this if you passed DPM_FLAG_NEVER_SKIP to
> > dev_pm_set_driver_flags() (instead of DPM_FLAG_SMART_SUSPEND), would

I whould have said "instead of DPM_FLAG_SMART_PREPARE" here, sorry.

> > you?
> 
> Yes, I think it would work. I did not use it here because I thought it
> would be better to leave the port runtime suspended during system
> suspend (to idle) but in both cases we end up resuming them at some
> point (before or after system suspend).

DPM_FLAG_NEVER_SKIP only affects the direct-complete optimization.

It has no effect on the other behavior, so it is valid to have both
DPM_FLAG_NEVER_SKIP and DPM_FLAG_SMART_SUSPEND set at the same time
which I think is what you want.

[On the other hand, if DPM_FLAG_NEVER_SKIP is set, DPM_FLAG_SMART_PREPARE
has no effect.]

> DPM_FLAG_NEVER_SKIP would result a cleaner patch, I think.
> 

Well, that's my point. :-)

Re: [PATCH 05/10] PCI: portdrv: Resume upon exit from system suspend if left runtime suspended

[Mika Westerberg]

On Tue, Sep 11, 2018 at 12:33:46PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 11, 2018 11:15:20 AM CEST Mika Westerberg wrote:
> > On Tue, Sep 11, 2018 at 10:00:07AM +0200, Rafael J. Wysocki wrote:
> > > On Thu, Sep 6, 2018 at 5:50 PM Mika Westerberg
> > > <mika.westerberg@linux.intel.com> wrote:
> > > >
> > > > Currently we try to keep PCIe ports runtime suspended over system
> > > > suspend if possible. This mostly happens when entering suspend-to-idle
> > > > because there is no need to re-configure wake settings.
> > > >
> > > > This causes problems if the parent port goes into D3cold and it gets
> > > > resumed upon exit from system suspend. This may happen for example if
> > > > the port is part of PCIe switch and the same switch is connected to a
> > > > PCIe endpoint that needs to be resumed. The way exit from D3cold works
> > > > according PCIe 4.0 spec 5.3.1.4.2 is that power is restored and cold
> > > > reset is signaled. After this the device is in D0unitialized state
> > > > keeping PME context if it supports wake from D3cold.
> > > >
> > > > The problem occurs when a PCIe hotplug port is left suspended and the
> > > > parent port goes into D3cold and back to D0, the port keeps its PME
> > > > context but since everything else is reset back to defaults
> > > > (D0unitialized) it is not set to detect hotplug events anymore.
> > > >
> > > > For this reason change the PCIe portdrv power management logic so that
> > > > it is fine to keep the port runtime suspended over system suspend but it
> > > > needs to be resumed upon exit to make sure it gets properly re-initialized.
> > > > The custom ->prepare() hook with DPM_FLAG_SMART_PREPARE is needed
> > > > because otherwise pci_pm_prepare() instructs the PM core to go directly
> > > > to pci_pm_complete() on resume and this skips resuming the port.
> > > 
> > > Thanks for the detailed explanation, it helps quite a bit!
> > > 
> > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > ---
> > > >  drivers/pci/pcie/portdrv_pci.c | 20 ++++++++++++++++++--
> > > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> > > > index eef22dc29140..74761f660a30 100644
> > > > --- a/drivers/pci/pcie/portdrv_pci.c
> > > > +++ b/drivers/pci/pcie/portdrv_pci.c
> > > > @@ -43,6 +43,21 @@ __setup("pcie_ports=", pcie_port_setup);
> > > >  /* global data */
> > > >
> > > >  #ifdef CONFIG_PM
> > > > +int pcie_port_prepare(struct device *dev)
> > > > +{
> > > > +       /*
> > > > +        * Return 0 here to indicate PCI core that:
> > > > +        *   - Direct complete path should be avoided
> > > > +        *   - It is OK to leave the port runtime suspended over system
> > > > +        *     suspend
> > > > +        *
> > > > +        * However, the port needs to be resumed afterwards because it may
> > > > +        * have been in D3cold in which case we need to re-initialize the
> > > > +        * hardware as it is in D0uninitialized in that case.
> > > > +        */
> > > > +       return 0;
> > > > +}
> > > 
> > > You wouldn't need this if you passed DPM_FLAG_NEVER_SKIP to
> > > dev_pm_set_driver_flags() (instead of DPM_FLAG_SMART_SUSPEND), would
> 
> I whould have said "instead of DPM_FLAG_SMART_PREPARE" here, sorry.
> 
> > > you?
> > 
> > Yes, I think it would work. I did not use it here because I thought it
> > would be better to leave the port runtime suspended during system
> > suspend (to idle) but in both cases we end up resuming them at some
> > point (before or after system suspend).
> 
> DPM_FLAG_NEVER_SKIP only affects the direct-complete optimization.

I did not know that. Thanks for the clarification. :)

> It has no effect on the other behavior, so it is valid to have both
> DPM_FLAG_NEVER_SKIP and DPM_FLAG_SMART_SUSPEND set at the same time
> which I think is what you want.

Indeed, that sounds like the right combination of flags.

Re: [PATCH 09/10] ACPI / property: Allow multiple property compatible _DSD entries

[Sakari Ailus]

Moi Mika,

On Thu, Sep 06, 2018 at 06:50:19PM +0300, Mika Westerberg wrote:
> @@ -427,32 +464,37 @@ static int acpi_data_get_property(const struct acpi_device_data *data,
>  				  const char *name, acpi_object_type type,
>  				  const union acpi_object **obj)
>  {
> -	const union acpi_object *properties;
> -	int i;
> +	struct acpi_device_properties *props;

props could be const.

>  
>  	if (!data || !name)
>  		return -EINVAL;
>  
> -	if (!data->pointer || !data->properties)
> +	if (!data->pointer || list_empty(&data->properties))
>  		return -EINVAL;
>  
> -	properties = data->properties;
> -	for (i = 0; i < properties->package.count; i++) {
> -		const union acpi_object *propname, *propvalue;
> -		const union acpi_object *property;
> +	list_for_each_entry(props, &data->properties, list) {
> +		const union acpi_object *properties;
> +		int i;

unsigned int i?

>  
> -		property = &properties->package.elements[i];
> +		properties = props->properties;
> +		for (i = 0; i < properties->package.count; i++) {
> +			const union acpi_object *propname, *propvalue;
> +			const union acpi_object *property;
>  
> -		propname = &property->package.elements[0];
> -		propvalue = &property->package.elements[1];
> +			property = &properties->package.elements[i];
>  

With the above considered,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Terveisin,

Sakari Ailus
e-mail: sakari.ailus@iki.fi