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

[PATCH v2 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. The previus version of
the series can be viewed here:

  https://www.spinics.net/lists/linux-acpi/msg83841.html

Changes from v1:

  * Updated comment in patch [1/10] according what Rafael suggested
  * Added empty line in patch [2/10]
  * Check for !pciehp_poll_mode in [4/10]
  * Use DPM_FLAG_NEVER_SKIP instead of DPM_FLAG_SMART_PREPARE which
    simplifies patch [5/10] signicantly 
  * Use const in acpi_data_get_property() and change int -> unsigned int
    in patch [9/10]
  * Added tags 

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            | 57 +++++++++++++++++-
 drivers/pci/pci.c                 | 18 +++++-
 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    | 14 ++---
 include/acpi/acpi_bus.h           |  8 ++-
 include/linux/acpi.h              |  9 +++
 15 files changed, 273 insertions(+), 43 deletions(-)

-- 
2.18.0

[PATCH v2 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>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci-acpi.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index c2ab57705043..f8436d1c4d45 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -762,19 +762,33 @@ 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 v2 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>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@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 8131c08b21e5..9a8051a878dc 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -184,6 +184,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 ccaf01e6eced..940c64fdcaae 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -265,8 +265,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;
 }
 
@@ -290,6 +305,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 017161c03a4d..02fb6e367e2a 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 v2 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>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
---
 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 02fb6e367e2a..26598a03e84b 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) && !pciehp_poll_mode))
 		return IRQ_NONE;
 
 	/*
-- 
2.18.0

[PATCH v2 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.

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

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index eef22dc29140..629d5fe4c9b5 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -109,8 +109,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_NEVER_SKIP |
+					   DPM_FLAG_SMART_SUSPEND);
 
 	if (pci_bridge_d3_possible(dev)) {
 		/*
-- 
2.18.0

[PATCH v2 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>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@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 629d5fe4c9b5..42d72cab3b1d 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -45,12 +45,10 @@ __setup("pcie_ports=", pcie_port_setup);
 #ifdef CONFIG_PM
 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)
@@ -73,7 +71,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 v2 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>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@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 940c64fdcaae..203610478d55 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -312,6 +312,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 = {
@@ -326,6 +343,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 v2 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>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@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 v2 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>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Sakari Ailus <sakari.ailus@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..90ba9371bae6 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;
+	const 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;
+		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];
 
-		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 v2 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>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@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 90ba9371bae6..8c7c4583b52d 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 f8436d1c4d45..c8d0549580f4 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 1af6f1887986..b1b3052f15dc 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
@@ -2514,6 +2519,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 v2 01/10] PCI: Do not skip power managed bridges in pci_enable_wake()

[Rafael J. Wysocki]

On Thursday, September 13, 2018 4:33:13 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 | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 29ff9619b5fa..1af6f1887986 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2133,10 +2133,13 @@ static int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable
>  	int ret = 0;
>  
>  	/*
> -	 * Bridges can only signal wakeup on behalf of subordinate devices,
> -	 * but that is set up elsewhere, so skip them.
> +	 * 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. */
> 

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

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

[Rafael J. Wysocki]

On Thursday, September 13, 2018 4:33:17 PM CEST 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.
> 
> 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.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pcie/portdrv_pci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index eef22dc29140..629d5fe4c9b5 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -109,8 +109,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_NEVER_SKIP |
> +					   DPM_FLAG_SMART_SUSPEND);
>  
>  	if (pci_bridge_d3_possible(dev)) {
>  		/*
> 

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

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

[Mika Westerberg]

On Thu, Sep 13, 2018 at 04:33:39PM +0200, Rafael J. Wysocki wrote:
> On Thursday, September 13, 2018 4:33:13 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 | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 29ff9619b5fa..1af6f1887986 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2133,10 +2133,13 @@ static int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable
> >  	int ret = 0;
> >  
> >  	/*
> > -	 * Bridges can only signal wakeup on behalf of subordinate devices,
> > -	 * but that is set up elsewhere, so skip them.
> > +	 * 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. */
> > 
> 
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks!

For some reason this patch never hit the mailing lists (others did).
Since you kept the whole context people should still be able to review
it. I can resend this patch as well.

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

[Mika Westerberg]

On Thu, Sep 13, 2018 at 04:33:39PM +0200, Rafael J. Wysocki wrote:
> On Thursday, September 13, 2018 4:33:13 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 | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 29ff9619b5fa..1af6f1887986 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2133,10 +2133,13 @@ static int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable
> >  	int ret = 0;
> >  
> >  	/*
> > -	 * Bridges can only signal wakeup on behalf of subordinate devices,
> > -	 * but that is set up elsewhere, so skip them.
> > +	 * 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. */
> > 
> 
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks!

For some reason this patch never hit the mailing lists (others did).
Since you kept the whole context people should still be able to review
it. I can resend this patch as well.

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

[Rafael J. Wysocki]

On Fri, Sep 14, 2018 at 10:07 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Thu, Sep 13, 2018 at 04:33:39PM +0200, Rafael J. Wysocki wrote:
> > On Thursday, September 13, 2018 4:33:13 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 | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 29ff9619b5fa..1af6f1887986 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -2133,10 +2133,13 @@ static int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable
> > >     int ret = 0;
> > >
> > >     /*
> > > -    * Bridges can only signal wakeup on behalf of subordinate devices,
> > > -    * but that is set up elsewhere, so skip them.
> > > +    * 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. */
> > >
> >
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Thanks!
>
> For some reason this patch never hit the mailing lists (others did).
> Since you kept the whole context people should still be able to review
> it. I can resend this patch as well.

You may need to resend for Patchwork to pick it up, though.  Depends
on Bjorn I suppose.

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

[Rafael J. Wysocki]

On Fri, Sep 14, 2018 at 10:07 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Thu, Sep 13, 2018 at 04:33:39PM +0200, Rafael J. Wysocki wrote:
> > On Thursday, September 13, 2018 4:33:13 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 | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 29ff9619b5fa..1af6f1887986 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -2133,10 +2133,13 @@ static int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable
> > >     int ret = 0;
> > >
> > >     /*
> > > -    * Bridges can only signal wakeup on behalf of subordinate devices,
> > > -    * but that is set up elsewhere, so skip them.
> > > +    * 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. */
> > >
> >
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Thanks!
>
> For some reason this patch never hit the mailing lists (others did).
> Since you kept the whole context people should still be able to review
> it. I can resend this patch as well.

You may need to resend for Patchwork to pick it up, though.  Depends
on Bjorn I suppose.

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

[Mika Westerberg]

On Fri, Sep 14, 2018 at 10:11:25AM +0200, Rafael J. Wysocki wrote:
> You may need to resend for Patchwork to pick it up, though.  Depends
> on Bjorn I suppose.

OK, I'll resend it just in case :)

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

[Mika Westerberg]

On Fri, Sep 14, 2018 at 10:11:25AM +0200, Rafael J. Wysocki wrote:
> You may need to resend for Patchwork to pick it up, though.  Depends
> on Bjorn I suppose.

OK, I'll resend it just in case :)

[PATCH RESEND v2 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>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
Resending this because for some reason it did not hit the mailing lists.
Added tag from Rafael as well.

 drivers/pci/pci.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 29ff9619b5fa..1af6f1887986 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2133,10 +2133,13 @@ static int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable
 	int ret = 0;
 
 	/*
-	 * Bridges can only signal wakeup on behalf of subordinate devices,
-	 * but that is set up elsewhere, so skip them.
+	 * 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. */
-- 
2.18.0

[PATCH RESEND v2 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>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
Resending this because for some reason it did not hit the mailing lists.
Added tag from Rafael as well.

 drivers/pci/pci.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 29ff9619b5fa..1af6f1887986 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2133,10 +2133,13 @@ static int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable
 	int ret = 0;
 
 	/*
-	 * Bridges can only signal wakeup on behalf of subordinate devices,
-	 * but that is set up elsewhere, so skip them.
+	 * 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. */
-- 
2.18.0

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

[Bjorn Helgaas]

On Fri, Sep 14, 2018 at 11:31:34AM +0300, 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>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Applied to pci/enumeration for v4.20, thanks!

> ---
> Resending this because for some reason it did not hit the mailing lists.
> Added tag from Rafael as well.
> 
>  drivers/pci/pci.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 29ff9619b5fa..1af6f1887986 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2133,10 +2133,13 @@ static int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable
>  	int ret = 0;
>  
>  	/*
> -	 * Bridges can only signal wakeup on behalf of subordinate devices,
> -	 * but that is set up elsewhere, so skip them.
> +	 * 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. */
> -- 
> 2.18.0
> 

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

[Bjorn Helgaas]

On Fri, Sep 14, 2018 at 11:31:34AM +0300, 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>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Applied to pci/enumeration for v4.20, thanks!

> ---
> Resending this because for some reason it did not hit the mailing lists.
> Added tag from Rafael as well.
> 
>  drivers/pci/pci.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 29ff9619b5fa..1af6f1887986 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2133,10 +2133,13 @@ static int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable
>  	int ret = 0;
>  
>  	/*
> -	 * Bridges can only signal wakeup on behalf of subordinate devices,
> -	 * but that is set up elsewhere, so skip them.
> +	 * 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. */
> -- 
> 2.18.0
> 

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

[Bjorn Helgaas]

On Thu, Sep 13, 2018 at 05:33:12PM +0300, Mika Westerberg wrote:
> 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. The previus version of
> the series can be viewed here:
> 
>   https://www.spinics.net/lists/linux-acpi/msg83841.html
> 
> Changes from v1:
> 
>   * Updated comment in patch [1/10] according what Rafael suggested
>   * Added empty line in patch [2/10]
>   * Check for !pciehp_poll_mode in [4/10]
>   * Use DPM_FLAG_NEVER_SKIP instead of DPM_FLAG_SMART_PREPARE which
>     simplifies patch [5/10] signicantly 
>   * Use const in acpi_data_get_property() and change int -> unsigned int
>     in patch [9/10]
>   * Added tags 
> 
> 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            | 57 +++++++++++++++++-
>  drivers/pci/pci.c                 | 18 +++++-
>  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    | 14 ++---
>  include/acpi/acpi_bus.h           |  8 ++-
>  include/linux/acpi.h              |  9 +++
>  15 files changed, 273 insertions(+), 43 deletions(-)

Applied the whole series (not just 01/10, which I applied by itself for
some reason) to pci/hotplug for v4.20, thanks!

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

[Bjorn Helgaas]

On Thu, Sep 13, 2018 at 05:33:22PM +0300, 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

This doc basically says that if the platform supplies this _DSD, the
root port is "capable of handling hot plug events while in D3 state".

What does that mean?  That statement is not really actionable.  I
*assume* it's telling us about some specific hardware or firmware
functionality, like maybe we'll get a notification for hotplug events
when the device is in D3?  D3hot?  D3cold?  What is the notification?
Is it immediate or when the device comes back to D0?  How do we
control and field the notification?

> 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>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@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 90ba9371bae6..8c7c4583b52d 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 f8436d1c4d45..c8d0549580f4 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 1af6f1887986..b1b3052f15dc 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;

This patch added a .bridge_d3() implementation for ACPI but not for
MID.  What prevents us from calling platform_pci_bridge_d3() on a MID
platform and trying to call through a NULL pointer?

Shouldn't we do something like the patch attached below?

> +}
> +
>  /**
>   * pci_raw_set_power_state - Use PCI PM registers to set the power state of
>   *                           given PCI device
> @@ -2514,6 +2519,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;

*All* devices trivially support D3.  Obviously we're trying to learn
something else here.  What is it?

>  		/*
>  		 * 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);
> -- 


diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c
index aafd58da3a89..0bacd45b30d6 100644
--- a/drivers/pci/pci-mid.c
+++ b/drivers/pci/pci-mid.c
@@ -16,6 +16,11 @@
 
 #include "pci.h"
 
+static bool mid_pci_bridge_d3(struct pci_dev *dev)
+{
+	return false;
+}
+
 static bool mid_pci_power_manageable(struct pci_dev *dev)
 {
 	return true;
@@ -47,6 +52,7 @@ static bool mid_pci_need_resume(struct pci_dev *dev)
 }
 
 static const struct pci_platform_pm_ops mid_pci_platform_pm = {
+	.bridge_d3	= mid_pci_bridge_d3,
 	.is_manageable	= mid_pci_power_manageable,
 	.set_state	= mid_pci_set_power_state,
 	.get_state	= mid_pci_get_power_state,
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 595fcf59843f..fa837e88ea07 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -820,8 +820,9 @@ static const struct pci_platform_pm_ops *pci_platform_pm;
 
 int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
 {
-	if (!ops->is_manageable || !ops->set_state  || !ops->get_state ||
-	    !ops->choose_state  || !ops->set_wakeup || !ops->need_resume)
+	if (!ops->bridge_d3 || !ops->is_manageable || !ops->set_state  ||
+	    !ops->get_state || !ops->choose_state  || !ops->set_wakeup ||
+	    !ops->need_resume)
 		return -EINVAL;
 	pci_platform_pm = ops;
 	return 0;

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

[Mika Westerberg]

On Tue, Apr 07, 2020 at 06:54:23PM -0500, Bjorn Helgaas wrote:
> On Thu, Sep 13, 2018 at 05:33:22PM +0300, 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
> 
> This doc basically says that if the platform supplies this _DSD, the
> root port is "capable of handling hot plug events while in D3 state".
> 
> What does that mean?  That statement is not really actionable.  I
> *assume* it's telling us about some specific hardware or firmware
> functionality, like maybe we'll get a notification for hotplug events
> when the device is in D3?  D3hot?  D3cold?  What is the notification?
> Is it immediate or when the device comes back to D0?  How do we
> control and field the notification?

I think it simply gives the OS a hint that it can put PCIe hotplug
capable port into D3 and expect it to wake up when device is detected.

> > 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>
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@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 90ba9371bae6..8c7c4583b52d 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 f8436d1c4d45..c8d0549580f4 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 1af6f1887986..b1b3052f15dc 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;
> 
> This patch added a .bridge_d3() implementation for ACPI but not for
> MID.  What prevents us from calling platform_pci_bridge_d3() on a MID
> platform and trying to call through a NULL pointer?
> 
> Shouldn't we do something like the patch attached below?

IIRC MID devices in general don't have PCIe ports (so we never enable PM
for them). Is this a real problem that crashes peoples kernels on MID
systems? Then yes I think the patch makes sense to have.

I also remember testing some other stuff on one MID system (Edison)
quite recently and did not see any issues.

> > +}
> > +
> >  /**
> >   * pci_raw_set_power_state - Use PCI PM registers to set the power state of
> >   *                           given PCI device
> > @@ -2514,6 +2519,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;
> 
> *All* devices trivially support D3.  Obviously we're trying to learn
> something else here.  What is it?

D3 has been problematic with hotplug bridges which is the reason we have
not put them in D3 until recently (and still don't do that for ACPI
hotplug bridges).

BTW, this patch was merged over a year ago so I'm not sure why you comment
it now. Or you want me to add incremental changes to it?

> >  		/*
> >  		 * 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);
> > -- 
> 
> 
> diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c
> index aafd58da3a89..0bacd45b30d6 100644
> --- a/drivers/pci/pci-mid.c
> +++ b/drivers/pci/pci-mid.c
> @@ -16,6 +16,11 @@
>  
>  #include "pci.h"
>  
> +static bool mid_pci_bridge_d3(struct pci_dev *dev)
> +{
> +	return false;
> +}
> +
>  static bool mid_pci_power_manageable(struct pci_dev *dev)
>  {
>  	return true;
> @@ -47,6 +52,7 @@ static bool mid_pci_need_resume(struct pci_dev *dev)
>  }
>  
>  static const struct pci_platform_pm_ops mid_pci_platform_pm = {
> +	.bridge_d3	= mid_pci_bridge_d3,
>  	.is_manageable	= mid_pci_power_manageable,
>  	.set_state	= mid_pci_set_power_state,
>  	.get_state	= mid_pci_get_power_state,
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 595fcf59843f..fa837e88ea07 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -820,8 +820,9 @@ static const struct pci_platform_pm_ops *pci_platform_pm;
>  
>  int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
>  {
> -	if (!ops->is_manageable || !ops->set_state  || !ops->get_state ||
> -	    !ops->choose_state  || !ops->set_wakeup || !ops->need_resume)
> +	if (!ops->bridge_d3 || !ops->is_manageable || !ops->set_state  ||
> +	    !ops->get_state || !ops->choose_state  || !ops->set_wakeup ||
> +	    !ops->need_resume)
>  		return -EINVAL;
>  	pci_platform_pm = ops;
>  	return 0;

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

[Bjorn Helgaas]

On Wed, Apr 08, 2020 at 09:04:09AM +0300, Mika Westerberg wrote:
> On Tue, Apr 07, 2020 at 06:54:23PM -0500, Bjorn Helgaas wrote:
> > On Thu, Sep 13, 2018 at 05:33:22PM +0300, 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
> > 
> > This doc basically says that if the platform supplies this _DSD, the
> > root port is "capable of handling hot plug events while in D3 state".
> > 
> > What does that mean?  That statement is not really actionable.  I
> > *assume* it's telling us about some specific hardware or firmware
> > functionality, like maybe we'll get a notification for hotplug events
> > when the device is in D3?  D3hot?  D3cold?  What is the notification?
> > Is it immediate or when the device comes back to D0?  How do we
> > control and field the notification?
> 
> I think it simply gives the OS a hint that it can put PCIe hotplug
> capable port into D3 and expect it to wake up when device is detected.

I'd really like more specific details than this.  PCI power management
is explicitly controlled by software, so I don't know what it means
for a bridge to "wake up when device is detected."

Normally Linux would get some kind of notification like a PME, then
execute an ACPI method and/or write PCI_PM_CTRL to put the device back
in D0.

Are we talking about D3hot or D3cold?

The PCI PM capability already has a way to advertise that it can
generate PME from D3cold.  How is this different from that?

Is this _DSD something that *could* be advertised via PCI config
space, i.e., is it completely determined by the Root Port?  Or is it
something that requires ACPI support, so it cannot be done directly by
the hardware device?

These are all things we need to know in order to make power management
reliable.

> > > 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>
> > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@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 90ba9371bae6..8c7c4583b52d 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 f8436d1c4d45..c8d0549580f4 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 1af6f1887986..b1b3052f15dc 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;
> > 
> > This patch added a .bridge_d3() implementation for ACPI but not for
> > MID.  What prevents us from calling platform_pci_bridge_d3() on a MID
> > platform and trying to call through a NULL pointer?
> > 
> > Shouldn't we do something like the patch attached below?
> 
> IIRC MID devices in general don't have PCIe ports (so we never enable PM
> for them). Is this a real problem that crashes peoples kernels on MID
> systems? Then yes I think the patch makes sense to have.
> 
> I also remember testing some other stuff on one MID system (Edison)
> quite recently and did not see any issues.

I have not seen reports of crashes, but I do think this is a real
problem.  The problem is that the code as-is relies on assumptions
("MID does not have PCI and never will") that are implicit and
impossible to verify, which is a maintenance problem.

> > > +}
> > > +
> > >  /**
> > >   * pci_raw_set_power_state - Use PCI PM registers to set the power state of
> > >   *                           given PCI device
> > > @@ -2514,6 +2519,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;
> > 
> > *All* devices trivially support D3.  Obviously we're trying to learn
> > something else here.  What is it?
> 
> D3 has been problematic with hotplug bridges which is the reason we have
> not put them in D3 until recently (and still don't do that for ACPI
> hotplug bridges).
> 
> BTW, this patch was merged over a year ago so I'm not sure why you comment
> it now. Or you want me to add incremental changes to it?

I was reviewing [1], which updates pci_find_pcie_root_port(), which is
used by acpi_pci_bridge_d3().  I noticed a potential NULL pointer
dereference, which was a distraction.

Unless you object, I'll merge something like the patch below to
prevent that distraction for others.

[1] https://lore.kernel.org/r/1586262717-23566-1-git-send-email-yangyicong@hisilicon.com

> > >  		/*
> > >  		 * 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);
> > > -- 
> > 
> > 
> > diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c
> > index aafd58da3a89..0bacd45b30d6 100644
> > --- a/drivers/pci/pci-mid.c
> > +++ b/drivers/pci/pci-mid.c
> > @@ -16,6 +16,11 @@
> >  
> >  #include "pci.h"
> >  
> > +static bool mid_pci_bridge_d3(struct pci_dev *dev)
> > +{
> > +	return false;
> > +}
> > +
> >  static bool mid_pci_power_manageable(struct pci_dev *dev)
> >  {
> >  	return true;
> > @@ -47,6 +52,7 @@ static bool mid_pci_need_resume(struct pci_dev *dev)
> >  }
> >  
> >  static const struct pci_platform_pm_ops mid_pci_platform_pm = {
> > +	.bridge_d3	= mid_pci_bridge_d3,
> >  	.is_manageable	= mid_pci_power_manageable,
> >  	.set_state	= mid_pci_set_power_state,
> >  	.get_state	= mid_pci_get_power_state,
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 595fcf59843f..fa837e88ea07 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -820,8 +820,9 @@ static const struct pci_platform_pm_ops *pci_platform_pm;
> >  
> >  int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
> >  {
> > -	if (!ops->is_manageable || !ops->set_state  || !ops->get_state ||
> > -	    !ops->choose_state  || !ops->set_wakeup || !ops->need_resume)
> > +	if (!ops->bridge_d3 || !ops->is_manageable || !ops->set_state  ||
> > +	    !ops->get_state || !ops->choose_state  || !ops->set_wakeup ||
> > +	    !ops->need_resume)
> >  		return -EINVAL;
> >  	pci_platform_pm = ops;
> >  	return 0;

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

[Mika Westerberg]

On Wed, Apr 08, 2020 at 03:12:08PM -0500, Bjorn Helgaas wrote:
> On Wed, Apr 08, 2020 at 09:04:09AM +0300, Mika Westerberg wrote:
> > On Tue, Apr 07, 2020 at 06:54:23PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Sep 13, 2018 at 05:33:22PM +0300, 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
> > > 
> > > This doc basically says that if the platform supplies this _DSD, the
> > > root port is "capable of handling hot plug events while in D3 state".
> > > 
> > > What does that mean?  That statement is not really actionable.  I
> > > *assume* it's telling us about some specific hardware or firmware
> > > functionality, like maybe we'll get a notification for hotplug events
> > > when the device is in D3?  D3hot?  D3cold?  What is the notification?
> > > Is it immediate or when the device comes back to D0?  How do we
> > > control and field the notification?
> > 
> > I think it simply gives the OS a hint that it can put PCIe hotplug
> > capable port into D3 and expect it to wake up when device is detected.
> 
> I'd really like more specific details than this.  PCI power management
> is explicitly controlled by software, so I don't know what it means
> for a bridge to "wake up when device is detected."

Note these are for PCIe which is not the same as the paraller PCI.

What I mean here is that there is some sort of wake depending whether
the link is in L1 or L2/L3 and then resulting the link to go to L0 and
PME message being send over to the root complex.

> Normally Linux would get some kind of notification like a PME, then
> execute an ACPI method and/or write PCI_PM_CTRL to put the device back
> in D0.

Right.

> Are we talking about D3hot or D3cold?

My interpretation is that both (D3 implies both D3hot and D3cold) but I
did not write that spec.

Systems where this is used typically go down to D3cold with the PCIe
topology (links are in L2).

> The PCI PM capability already has a way to advertise that it can
> generate PME from D3cold.  How is this different from that?

Well you always need some platform (ACPI) help in order to even to
D3cold. This applies to waking up as well. The PCIe device may advertise
that it supports this but I don't think it can be sure that the system
it is connected to has this plumbing.

For PCIe hotplug ports there have been issues that have prevented doing
power management for those. The _DSD is there to provide a hint to the
OS saying that yeah, this port actually is expected to work even if it
goes into D3 (cold/hot).

> Is this _DSD something that *could* be advertised via PCI config
> space, i.e., is it completely determined by the Root Port?  Or is it
> something that requires ACPI support, so it cannot be done directly by
> the hardware device?

You always need help from platform (ACPI) to get into D3cold.

> These are all things we need to know in order to make power management
> reliable.
> 
> > > > 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>
> > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@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 90ba9371bae6..8c7c4583b52d 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 f8436d1c4d45..c8d0549580f4 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 1af6f1887986..b1b3052f15dc 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;
> > > 
> > > This patch added a .bridge_d3() implementation for ACPI but not for
> > > MID.  What prevents us from calling platform_pci_bridge_d3() on a MID
> > > platform and trying to call through a NULL pointer?
> > > 
> > > Shouldn't we do something like the patch attached below?
> > 
> > IIRC MID devices in general don't have PCIe ports (so we never enable PM
> > for them). Is this a real problem that crashes peoples kernels on MID
> > systems? Then yes I think the patch makes sense to have.
> > 
> > I also remember testing some other stuff on one MID system (Edison)
> > quite recently and did not see any issues.
> 
> I have not seen reports of crashes, but I do think this is a real
> problem.  The problem is that the code as-is relies on assumptions
> ("MID does not have PCI and never will") that are implicit and
> impossible to verify, which is a maintenance problem.

Well, there won't be any new MID devices and the only one we support
partially is Edison and it does not have a single PCIe port that could
even be power managed.

> > > > +}
> > > > +
> > > >  /**
> > > >   * pci_raw_set_power_state - Use PCI PM registers to set the power state of
> > > >   *                           given PCI device
> > > > @@ -2514,6 +2519,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;
> > > 
> > > *All* devices trivially support D3.  Obviously we're trying to learn
> > > something else here.  What is it?
> > 
> > D3 has been problematic with hotplug bridges which is the reason we have
> > not put them in D3 until recently (and still don't do that for ACPI
> > hotplug bridges).
> > 
> > BTW, this patch was merged over a year ago so I'm not sure why you comment
> > it now. Or you want me to add incremental changes to it?
> 
> I was reviewing [1], which updates pci_find_pcie_root_port(), which is
> used by acpi_pci_bridge_d3().  I noticed a potential NULL pointer
> dereference, which was a distraction.
> 
> Unless you object, I'll merge something like the patch below to
> prevent that distraction for others.

No objections :)

However, in order to make this more maintainable, what if we check in
the platform_pci_bridge_d3() itself whether the pointer is NULL and
return false? That would work for any future platforms and avoid adding
the dummy implementation each time.

> [1] https://lore.kernel.org/r/1586262717-23566-1-git-send-email-yangyicong@hisilicon.com
> 
> > > >  		/*
> > > >  		 * 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);
> > > > -- 
> > > 
> > > 
> > > diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c
> > > index aafd58da3a89..0bacd45b30d6 100644
> > > --- a/drivers/pci/pci-mid.c
> > > +++ b/drivers/pci/pci-mid.c
> > > @@ -16,6 +16,11 @@
> > >  
> > >  #include "pci.h"
> > >  
> > > +static bool mid_pci_bridge_d3(struct pci_dev *dev)
> > > +{
> > > +	return false;
> > > +}
> > > +
> > >  static bool mid_pci_power_manageable(struct pci_dev *dev)
> > >  {
> > >  	return true;
> > > @@ -47,6 +52,7 @@ static bool mid_pci_need_resume(struct pci_dev *dev)
> > >  }
> > >  
> > >  static const struct pci_platform_pm_ops mid_pci_platform_pm = {
> > > +	.bridge_d3	= mid_pci_bridge_d3,
> > >  	.is_manageable	= mid_pci_power_manageable,
> > >  	.set_state	= mid_pci_set_power_state,
> > >  	.get_state	= mid_pci_get_power_state,
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 595fcf59843f..fa837e88ea07 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -820,8 +820,9 @@ static const struct pci_platform_pm_ops *pci_platform_pm;
> > >  
> > >  int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
> > >  {
> > > -	if (!ops->is_manageable || !ops->set_state  || !ops->get_state ||
> > > -	    !ops->choose_state  || !ops->set_wakeup || !ops->need_resume)
> > > +	if (!ops->bridge_d3 || !ops->is_manageable || !ops->set_state  ||
> > > +	    !ops->get_state || !ops->choose_state  || !ops->set_wakeup ||
> > > +	    !ops->need_resume)
> > >  		return -EINVAL;
> > >  	pci_platform_pm = ops;
> > >  	return 0;

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

[Bjorn Helgaas]

[breadcrumbs for the archives: this thread is about
26ad34d510a8 ("PCI / ACPI: Whitelist D3 for more PCIe hotplug ports"),
https://git.kernel.org/linus/26ad34d510a8]

On Thu, Apr 09, 2020 at 09:54:11AM +0300, Mika Westerberg wrote:
> On Wed, Apr 08, 2020 at 03:12:08PM -0500, Bjorn Helgaas wrote:
> > On Wed, Apr 08, 2020 at 09:04:09AM +0300, Mika Westerberg wrote:
> > > On Tue, Apr 07, 2020 at 06:54:23PM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Sep 13, 2018 at 05:33:22PM +0300, 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
> > > > 
> > > > This doc basically says that if the platform supplies this _DSD, the
> > > > root port is "capable of handling hot plug events while in D3 state".
> > > > 
> > > > What does that mean?  That statement is not really actionable.  I
> > > > *assume* it's telling us about some specific hardware or firmware
> > > > functionality, like maybe we'll get a notification for hotplug events
> > > > when the device is in D3?  D3hot?  D3cold?  What is the notification?
> > > > Is it immediate or when the device comes back to D0?  How do we
> > > > control and field the notification?
> > > 
> > > I think it simply gives the OS a hint that it can put PCIe hotplug
> > > capable port into D3 and expect it to wake up when device is detected.
> > 
> > I'd really like more specific details than this.  PCI power management
> > is explicitly controlled by software, so I don't know what it means
> > for a bridge to "wake up when device is detected."
> 
> Note these are for PCIe which is not the same as the parallel PCI.

Sure, but the PCI power management framework doesn't need to know
about PCI/PCIe differences.

> What I mean here is that there is some sort of wake depending whether
> the link is in L1 or L2/L3 and then resulting the link to go to L0 and
> PME message being send over to the root complex.

This is compatible with conventional PCI behavior.  PME_Support in the
PM Capability tells us whether the device can generate a PME from D0,
D1, D2, D3hot, or D3cold.  AFAIK, this applies to both conventional
PCI and PCIe, so I think this _DSD must be telling us something more
than just "this PCIe device can assert PME from D3hot and D3cold."

PME_Support tells us whether the device can generate PMEs from D3cold,
but of course it doesn't say anything about whether hotplug events
cause those PMEs.

PCIe r5.0, sec 6.7.3.4, says ports must support PMEs for hotplug
events while the device is in D1, D2, or D3hot.  It clearly leaves out
D3cold.

So maybe this _DSD tells us that this device can generate *hotplug*
event PMEs from D3cold?

> > Normally Linux would get some kind of notification like a PME, then
> > execute an ACPI method and/or write PCI_PM_CTRL to put the device back
> > in D0.
> 
> Right.
> 
> > Are we talking about D3hot or D3cold?
> 
> My interpretation is that both (D3 implies both D3hot and D3cold) but I
> did not write that spec.
> 
> Systems where this is used typically go down to D3cold with the PCIe
> topology (links are in L2).
> 
> > The PCI PM capability already has a way to advertise that it can
> > generate PME from D3cold.  How is this different from that?
> 
> Well you always need some platform (ACPI) help in order to even to
> D3cold. This applies to waking up as well. The PCIe device may advertise
> that it supports this but I don't think it can be sure that the system
> it is connected to has this plumbing.

Yes, transitions to/from D3cold require platform support.  But those
transitions are done by ACPI methods, so any plumbing is ACPI's
problem, and I don't think this _DSD helps the OS with that.

> For PCIe hotplug ports there have been issues that have prevented doing
> power management for those. The _DSD is there to provide a hint to the
> OS saying that yeah, this port actually is expected to work even if it
> goes into D3 (cold/hot).

That doesn't really help fix bugs in this area or help with future
refactoring, because it's not specific about what the issues were or
what the _DSD means.

I think the combination of PCIe r5.0 7.5.2.1 (PME_Support from D0, D1,
D2, D3hot, D3cold) and 6.7.3.4 (hotplug PME events from D1, D2,
D3hot) covers everything except hotplug PME events from D3cold.

So my guess is that this _DSD is a way to tell the OS that this device
generates PMEs for hotplug events even when the device is in D3cold.
Does that make any sense?

> > Is this _DSD something that *could* be advertised via PCI config
> > space, i.e., is it completely determined by the Root Port?  Or is it
> > something that requires ACPI support, so it cannot be done directly by
> > the hardware device?
> 
> You always need help from platform (ACPI) to get into D3cold.
> 
> > These are all things we need to know in order to make power management
> > reliable.

> > > > > +static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
> > > > > +{
> > > > > +	return pci_platform_pm ? pci_platform_pm->bridge_d3(dev) : false;
> > > > 
> > > > This patch added a .bridge_d3() implementation for ACPI but
> > > > not for MID.  What prevents us from calling
> > > > platform_pci_bridge_d3() on a MID platform and trying to call
> > > > through a NULL pointer?
> > > > 
> > > > Shouldn't we do something like the patch attached below?
> > > 
> > > IIRC MID devices in general don't have PCIe ports (so we never
> > > enable PM for them). Is this a real problem that crashes peoples
> > > kernels on MID systems? Then yes I think the patch makes sense
> > > to have.
> > > 
> > > I also remember testing some other stuff on one MID system
> > > (Edison) quite recently and did not see any issues.
> > 
> > I have not seen reports of crashes, but I do think this is a real
> > problem.  The problem is that the code as-is relies on assumptions
> > ("MID does not have PCI and never will") that are implicit and
> > impossible to verify, which is a maintenance problem.
> 
> Well, there won't be any new MID devices and the only one we support
> partially is Edison and it does not have a single PCIe port that
> could even be power managed.

The problem I'm trying to solve is making this code understandable.
It doesn't help to assert that "there are no devices that exercise
this path" because one cannot figure that out by looking at the code.

> > > BTW, this patch was merged over a year ago so I'm not sure why you comment
> > > it now. Or you want me to add incremental changes to it?
> > 
> > I was reviewing [1], which updates pci_find_pcie_root_port(), which is
> > used by acpi_pci_bridge_d3().  I noticed a potential NULL pointer
> > dereference, which was a distraction.
> > 
> > Unless you object, I'll merge something like the patch below to
> > prevent that distraction for others.
> 
> No objections :)
> 
> However, in order to make this more maintainable, what if we check in
> the platform_pci_bridge_d3() itself whether the pointer is NULL and
> return false? That would work for any future platforms and avoid adding
> the dummy implementation each time.

That's a much better idea, thank you!  I'll cc you when I revise the
patch.

Bjorn

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

[Mika Westerberg]

On Fri, Apr 10, 2020 at 05:30:58PM -0500, Bjorn Helgaas wrote:
> [breadcrumbs for the archives: this thread is about
> 26ad34d510a8 ("PCI / ACPI: Whitelist D3 for more PCIe hotplug ports"),
> https://git.kernel.org/linus/26ad34d510a8]
> 
> On Thu, Apr 09, 2020 at 09:54:11AM +0300, Mika Westerberg wrote:
> > On Wed, Apr 08, 2020 at 03:12:08PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Apr 08, 2020 at 09:04:09AM +0300, Mika Westerberg wrote:
> > > > On Tue, Apr 07, 2020 at 06:54:23PM -0500, Bjorn Helgaas wrote:
> > > > > On Thu, Sep 13, 2018 at 05:33:22PM +0300, 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
> > > > > 
> > > > > This doc basically says that if the platform supplies this _DSD, the
> > > > > root port is "capable of handling hot plug events while in D3 state".
> > > > > 
> > > > > What does that mean?  That statement is not really actionable.  I
> > > > > *assume* it's telling us about some specific hardware or firmware
> > > > > functionality, like maybe we'll get a notification for hotplug events
> > > > > when the device is in D3?  D3hot?  D3cold?  What is the notification?
> > > > > Is it immediate or when the device comes back to D0?  How do we
> > > > > control and field the notification?
> > > > 
> > > > I think it simply gives the OS a hint that it can put PCIe hotplug
> > > > capable port into D3 and expect it to wake up when device is detected.
> > > 
> > > I'd really like more specific details than this.  PCI power management
> > > is explicitly controlled by software, so I don't know what it means
> > > for a bridge to "wake up when device is detected."
> > 
> > Note these are for PCIe which is not the same as the parallel PCI.
> 
> Sure, but the PCI power management framework doesn't need to know
> about PCI/PCIe differences.

Well it needs to deal with certain differences anyway like the required
delays etc.

> > What I mean here is that there is some sort of wake depending whether
> > the link is in L1 or L2/L3 and then resulting the link to go to L0 and
> > PME message being send over to the root complex.
> 
> This is compatible with conventional PCI behavior.  PME_Support in the
> PM Capability tells us whether the device can generate a PME from D0,
> D1, D2, D3hot, or D3cold.  AFAIK, this applies to both conventional
> PCI and PCIe, so I think this _DSD must be telling us something more
> than just "this PCIe device can assert PME from D3hot and D3cold."
> 
> PME_Support tells us whether the device can generate PMEs from D3cold,
> but of course it doesn't say anything about whether hotplug events
> cause those PMEs.
> 
> PCIe r5.0, sec 6.7.3.4, says ports must support PMEs for hotplug
> events while the device is in D1, D2, or D3hot.  It clearly leaves out
> D3cold.
> 
> So maybe this _DSD tells us that this device can generate *hotplug*
> event PMEs from D3cold?

Yes, that sounds reasonable.

> > > Normally Linux would get some kind of notification like a PME, then
> > > execute an ACPI method and/or write PCI_PM_CTRL to put the device back
> > > in D0.
> > 
> > Right.
> > 
> > > Are we talking about D3hot or D3cold?
> > 
> > My interpretation is that both (D3 implies both D3hot and D3cold) but I
> > did not write that spec.
> > 
> > Systems where this is used typically go down to D3cold with the PCIe
> > topology (links are in L2).
> > 
> > > The PCI PM capability already has a way to advertise that it can
> > > generate PME from D3cold.  How is this different from that?
> > 
> > Well you always need some platform (ACPI) help in order to even to
> > D3cold. This applies to waking up as well. The PCIe device may advertise
> > that it supports this but I don't think it can be sure that the system
> > it is connected to has this plumbing.
> 
> Yes, transitions to/from D3cold require platform support.  But those
> transitions are done by ACPI methods, so any plumbing is ACPI's
> problem, and I don't think this _DSD helps the OS with that.
>
> > For PCIe hotplug ports there have been issues that have prevented doing
> > power management for those. The _DSD is there to provide a hint to the
> > OS saying that yeah, this port actually is expected to work even if it
> > goes into D3 (cold/hot).
> 
> That doesn't really help fix bugs in this area or help with future
> refactoring, because it's not specific about what the issues were or
> what the _DSD means.
>
> 
> I think the combination of PCIe r5.0 7.5.2.1 (PME_Support from D0, D1,
> D2, D3hot, D3cold) and 6.7.3.4 (hotplug PME events from D1, D2,
> D3hot) covers everything except hotplug PME events from D3cold.
> 
> So my guess is that this _DSD is a way to tell the OS that this device
> generates PMEs for hotplug events even when the device is in D3cold.
> Does that make any sense?

Yes it does.

> > > Is this _DSD something that *could* be advertised via PCI config
> > > space, i.e., is it completely determined by the Root Port?  Or is it
> > > something that requires ACPI support, so it cannot be done directly by
> > > the hardware device?
> > 
> > You always need help from platform (ACPI) to get into D3cold.
> > 
> > > These are all things we need to know in order to make power management
> > > reliable.
> 
> > > > > > +static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
> > > > > > +{
> > > > > > +	return pci_platform_pm ? pci_platform_pm->bridge_d3(dev) : false;
> > > > > 
> > > > > This patch added a .bridge_d3() implementation for ACPI but
> > > > > not for MID.  What prevents us from calling
> > > > > platform_pci_bridge_d3() on a MID platform and trying to call
> > > > > through a NULL pointer?
> > > > > 
> > > > > Shouldn't we do something like the patch attached below?
> > > > 
> > > > IIRC MID devices in general don't have PCIe ports (so we never
> > > > enable PM for them). Is this a real problem that crashes peoples
> > > > kernels on MID systems? Then yes I think the patch makes sense
> > > > to have.
> > > > 
> > > > I also remember testing some other stuff on one MID system
> > > > (Edison) quite recently and did not see any issues.
> > > 
> > > I have not seen reports of crashes, but I do think this is a real
> > > problem.  The problem is that the code as-is relies on assumptions
> > > ("MID does not have PCI and never will") that are implicit and
> > > impossible to verify, which is a maintenance problem.
> > 
> > Well, there won't be any new MID devices and the only one we support
> > partially is Edison and it does not have a single PCIe port that
> > could even be power managed.
> 
> The problem I'm trying to solve is making this code understandable.
> It doesn't help to assert that "there are no devices that exercise
> this path" because one cannot figure that out by looking at the code.

Fair enough.

> > > > BTW, this patch was merged over a year ago so I'm not sure why you comment
> > > > it now. Or you want me to add incremental changes to it?
> > > 
> > > I was reviewing [1], which updates pci_find_pcie_root_port(), which is
> > > used by acpi_pci_bridge_d3().  I noticed a potential NULL pointer
> > > dereference, which was a distraction.
> > > 
> > > Unless you object, I'll merge something like the patch below to
> > > prevent that distraction for others.
> > 
> > No objections :)
> > 
> > However, in order to make this more maintainable, what if we check in
> > the platform_pci_bridge_d3() itself whether the pointer is NULL and
> > return false? That would work for any future platforms and avoid adding
> > the dummy implementation each time.
> 
> That's a much better idea, thank you!  I'll cc you when I revise the
> patch.

OK, thanks!