[PATCH v5 0/4] PCI: Allow D3Hot for PCI bridges in Devicetree based platforms

[PATCH v5 0/4] PCI: Allow D3Hot for PCI bridges in Devicetree based platforms

[Manivannan Sadhasivam via B4 Relay]

Hi,

This series allows D3Hot for PCI bridges in Devicetree based platforms.
Even though most of the bridges in Devicetree platforms support D3Hot, PCI
core will allow D3Hot only when one of the following conditions are met:

1. Platform is ACPI based
2. Thunderbolt controller is used
3. pcie_port_pm=force passed in cmdline

While options 1 and 2 do not apply to most of the DT based platforms,
option 3 will make the life harder for distro maintainers.

Initially, I tried to fix this issue by using a Devicetree property [1], but
that was rejected by Bjorn and it was concluded that D3Hot should be allowed by
default for all the Devicetree based platforms.

During the review of v3 series, Bjorn noted several shortcomings of the
pci_bridge_d3_possible() API [2] and I tried to address them in this series as
well.

But please note that the patches 2 and 3 needs closer review from ACPI and x86
folks since I've splitted the D3Hot and D3Cold handling based on my little
understanding of the code.

Testing
=======

This series is tested on SM8450 based development board on top of [3].

- Mani

[1] https://lore.kernel.org/linux-pci/20240214-pcie-qcom-bridge-v3-1-3a713bbc1fd7@linaro.org/
[2] https://lore.kernel.org/linux-pci/20240305175107.GA539676@bhelgaas/
[3] https://lore.kernel.org/linux-arm-msm/20240321-pcie-qcom-bridge-dts-v2-0-1eb790c53e43@linaro.org/

Changes in v5:
- Rebased on top of v6.11-rc1
- Fixed the function name in Kdoc of patch 3/4
- Collected tags
- Link to v4: https://lore.kernel.org/linux-pci/20240326-pci-bridge-d3-v4-0-f1dce1d1f648@linaro.org

Changes in v4:
- Added pci_bridge_d3_possible() rework based on comments from Bjorn
- Got rid of the DT property and allowed D3Hot by default on all DT platforms

Changes in v3:
- Fixed kdoc, used of_property_present() and dev_of_node() (Lukas)
- Link to v2: https://lore.kernel.org/r/20240214-pcie-qcom-bridge-v2-1-9dd6dbb1b817@linaro.org

Changes in v2:
- Switched to DT based approach as suggested by Lukas.
- Link to v1: https://lore.kernel.org/r/20240202-pcie-qcom-bridge-v1-0-46d7789836c0@linaro.org

To: Bjorn Helgaas <bhelgaas@google.com>
To: Rafael J. Wysocki <rafael@kernel.org>
To: Len Brown <lenb@kernel.org>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Cc: lukas@wunner.de
Cc: mika.westerberg@linux.intel.com

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
Manivannan Sadhasivam (4):
      PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility
      PCI: Rename pci_bridge_d3_possible() to pci_bridge_d3_allowed()
      PCI: Decouple D3Hot and D3Cold handling for bridges
      PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

 drivers/pci/bus.c          |  2 +-
 drivers/pci/pci-acpi.c     |  9 ++---
 drivers/pci/pci-sysfs.c    |  2 +-
 drivers/pci/pci.c          | 90 ++++++++++++++++++++++++++++++++--------------
 drivers/pci/pci.h          | 12 ++++---
 drivers/pci/pcie/portdrv.c | 16 ++++-----
 drivers/pci/remove.c       |  2 +-
 include/linux/pci.h        |  3 +-
 8 files changed, 89 insertions(+), 47 deletions(-)
---
base-commit: 705c1da8fa4816fb0159b5602fef1df5946a3ee2
change-id: 20240320-pci-bridge-d3-092e2beac438

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

[PATCH v5 1/4] PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

PCI core is already caching the value of pci_bridge_d3_possible() in
pci_dev::bridge_d3 during pci_pm_init(). Since the value is not going to
change, let's make use of the cached value.

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/pcie/portdrv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 14a4b89a3b83..1f02e5d7b2e9 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -702,7 +702,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE |
 					   DPM_FLAG_SMART_SUSPEND);
 
-	if (pci_bridge_d3_possible(dev)) {
+	if (dev->bridge_d3) {
 		/*
 		 * Keep the port resumed 100ms to make sure things like
 		 * config space accesses from userspace (lspci) will not
@@ -720,7 +720,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
-	if (pci_bridge_d3_possible(dev)) {
+	if (dev->bridge_d3) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);
@@ -733,7 +733,7 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
 
 static void pcie_portdrv_shutdown(struct pci_dev *dev)
 {
-	if (pci_bridge_d3_possible(dev)) {
+	if (dev->bridge_d3) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);

-- 
2.25.1

[PATCH v5 2/4] PCI: Rename pci_bridge_d3_possible() to pci_bridge_d3_allowed()

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

As per the 'PCI Bus Power Management Interface Specification' v1.2, all
devices should support D3 states (both D3Hot and D3Cold). So the term
'possible' doesn't make sense for the bridge devices, since D3 states
should be possible as per the design. But due to various circumstances,
D3 states might not be allowed for the bridges.

In those cases, the API should be called 'pci_bridge_d3_allowed()' to
reflect the actual behavior. So let's rename it.

This also warrants renaming the variable 'bridge_d3' in 'struct pci_dev'
to 'bridge_d3_allowed' for the reason cited above.

No functional change.

Reported-by: Bjorn Helgaas <helgaas@kernel.org>
Closes: https://lore.kernel.org/linux-pci/20240305175107.GA539676@bhelgaas/
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/pci-acpi.c     |  8 ++++----
 drivers/pci/pci.c          | 18 +++++++++---------
 drivers/pci/pci.h          |  4 ++--
 drivers/pci/pcie/portdrv.c | 14 +++++++-------
 include/linux/pci.h        |  2 +-
 5 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 004575091596..0f260cdc4592 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1429,12 +1429,12 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
 
 	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
+	 * For bridges that are allowed to 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)
+	if (pci_dev->bridge_d3_allowed)
 		device_wakeup_enable(dev);
 
 	acpi_pci_wakeup(pci_dev, false);
@@ -1452,7 +1452,7 @@ void pci_acpi_cleanup(struct device *dev, struct acpi_device *adev)
 	pci_acpi_remove_pm_notifier(adev);
 	if (adev->wakeup.flags.valid) {
 		acpi_device_power_remove_dependent(adev, dev);
-		if (pci_dev->bridge_d3)
+		if (pci_dev->bridge_d3_allowed)
 			device_wakeup_disable(dev);
 
 		device_set_wakeup_capable(dev, false);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e5f243dd4288..0edc4e448c2d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2967,13 +2967,13 @@ static const struct dmi_system_id bridge_d3_blacklist[] = {
 };
 
 /**
- * pci_bridge_d3_possible - Is it possible to put the bridge into D3
+ * pci_bridge_d3_allowed - Is it allowed to put the bridge into D3
  * @bridge: Bridge to check
  *
- * This function checks if it is possible to move the bridge to D3.
+ * This function checks if the bridge is allowed to move to D3.
  * Currently we only allow D3 for recent enough PCIe ports and Thunderbolt.
  */
-bool pci_bridge_d3_possible(struct pci_dev *bridge)
+bool pci_bridge_d3_allowed(struct pci_dev *bridge)
 {
 	if (!pci_is_pcie(bridge))
 		return false;
@@ -3060,14 +3060,14 @@ void pci_bridge_d3_update(struct pci_dev *dev)
 	bool d3cold_ok = true;
 
 	bridge = pci_upstream_bridge(dev);
-	if (!bridge || !pci_bridge_d3_possible(bridge))
+	if (!bridge || !pci_bridge_d3_allowed(bridge))
 		return;
 
 	/*
 	 * If D3 is currently allowed for the bridge, removing one of its
 	 * children won't change that.
 	 */
-	if (remove && bridge->bridge_d3)
+	if (remove && bridge->bridge_d3_allowed)
 		return;
 
 	/*
@@ -3087,12 +3087,12 @@ void pci_bridge_d3_update(struct pci_dev *dev)
 	 * so we need to go through all children to find out if one of them
 	 * continues to block D3.
 	 */
-	if (d3cold_ok && !bridge->bridge_d3)
+	if (d3cold_ok && !bridge->bridge_d3_allowed)
 		pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
 			     &d3cold_ok);
 
-	if (bridge->bridge_d3 != d3cold_ok) {
-		bridge->bridge_d3 = d3cold_ok;
+	if (bridge->bridge_d3_allowed != d3cold_ok) {
+		bridge->bridge_d3_allowed = d3cold_ok;
 		/* Propagate change to upstream bridges */
 		pci_bridge_d3_update(bridge);
 	}
@@ -3167,7 +3167,7 @@ void pci_pm_init(struct pci_dev *dev)
 	dev->pm_cap = pm;
 	dev->d3hot_delay = PCI_PM_D3HOT_WAIT;
 	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
-	dev->bridge_d3 = pci_bridge_d3_possible(dev);
+	dev->bridge_d3_allowed = pci_bridge_d3_allowed(dev);
 	dev->d3cold_allowed = true;
 
 	dev->d1_support = false;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 17fed1846847..53ca75639201 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -92,7 +92,7 @@ void pci_pm_init(struct pci_dev *dev);
 void pci_ea_init(struct pci_dev *dev);
 void pci_msi_init(struct pci_dev *dev);
 void pci_msix_init(struct pci_dev *dev);
-bool pci_bridge_d3_possible(struct pci_dev *dev);
+bool pci_bridge_d3_allowed(struct pci_dev *dev);
 void pci_bridge_d3_update(struct pci_dev *dev);
 int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
 
@@ -113,7 +113,7 @@ static inline bool pci_power_manageable(struct pci_dev *pci_dev)
 	 * Currently we allow normal PCI devices and PCI bridges transition
 	 * into D3 if their bridge_d3 is set.
 	 */
-	return !pci_has_subordinate(pci_dev) || pci_dev->bridge_d3;
+	return !pci_has_subordinate(pci_dev) || pci_dev->bridge_d3_allowed;
 }
 
 static inline bool pcie_downstream_port(const struct pci_dev *dev)
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 1f02e5d7b2e9..8401a0f7b394 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -632,7 +632,7 @@ __setup("pcie_ports=", pcie_port_setup);
 #ifdef CONFIG_PM
 static int pcie_port_runtime_suspend(struct device *dev)
 {
-	if (!to_pci_dev(dev)->bridge_d3)
+	if (!to_pci_dev(dev)->bridge_d3_allowed)
 		return -EBUSY;
 
 	return pcie_port_device_runtime_suspend(dev);
@@ -641,11 +641,11 @@ static int pcie_port_runtime_suspend(struct device *dev)
 static int pcie_port_runtime_idle(struct device *dev)
 {
 	/*
-	 * Assume the PCI core has set bridge_d3 whenever it thinks the port
-	 * should be good to go to D3.  Everything else, including moving
+	 * Assume the PCI core has set bridge_d3_allowed whenever it thinks the
+	 * port should be good to go to D3.  Everything else, including moving
 	 * the port to D3, is handled by the PCI core.
 	 */
-	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
+	return to_pci_dev(dev)->bridge_d3_allowed ? 0 : -EBUSY;
 }
 
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
@@ -702,7 +702,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE |
 					   DPM_FLAG_SMART_SUSPEND);
 
-	if (dev->bridge_d3) {
+	if (dev->bridge_d3_allowed) {
 		/*
 		 * Keep the port resumed 100ms to make sure things like
 		 * config space accesses from userspace (lspci) will not
@@ -720,7 +720,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
-	if (dev->bridge_d3) {
+	if (dev->bridge_d3_allowed) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);
@@ -733,7 +733,7 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
 
 static void pcie_portdrv_shutdown(struct pci_dev *dev)
 {
-	if (dev->bridge_d3) {
+	if (dev->bridge_d3_allowed) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 16493426a04f..2a48c88512e1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -375,7 +375,7 @@ struct pci_dev {
 	unsigned int	d2_support:1;	/* Low power state D2 is supported */
 	unsigned int	no_d1d2:1;	/* D1 and D2 are forbidden */
 	unsigned int	no_d3cold:1;	/* D3cold is forbidden */
-	unsigned int	bridge_d3:1;	/* Allow D3 for bridge */
+	unsigned int	bridge_d3_allowed:1;	/* Allow D3 for bridge */
 	unsigned int	d3cold_allowed:1;	/* D3cold is allowed by user */
 	unsigned int	mmio_always_on:1;	/* Disallow turning off io/mem
 						   decoding during BAR sizing */

-- 
2.25.1

[PATCH v5 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Currently, there is no proper distinction between D3Hot and D3Cold while
handling the power management for PCI bridges. For instance,
pci_bridge_d3_allowed() API decides whether it is allowed to put the
bridge in D3, but it doesn't explicitly specify whether D3Hot or D3Cold
is allowed in a scenario. This often leads to confusion and may be prone
to errors.

So let's split the D3Hot and D3Cold handling where possible. The current
pci_bridge_d3_allowed() API is now split into pci_bridge_d3hot_allowed()
and pci_bridge_d3cold_allowed() APIs and used in relevant places.

Also, pci_bridge_d3_update() API is now renamed to
pci_bridge_d3cold_update() since it was only used to check the possibility
of D3Cold.

Note that it is assumed that only D3Hot needs to be checked while
transitioning the bridge during runtime PM and D3Cold in other places. In
the ACPI case, wakeup is now only enabled if both D3Hot and D3Cold are
allowed for the bridge.

Still, there are places where just 'd3' is used opaquely, but those are
hard to distinguish, hence left for future cleanups.

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/bus.c          |  2 +-
 drivers/pci/pci-acpi.c     |  5 +--
 drivers/pci/pci-sysfs.c    |  2 +-
 drivers/pci/pci.c          | 78 ++++++++++++++++++++++++++++++----------------
 drivers/pci/pci.h          | 12 ++++---
 drivers/pci/pcie/portdrv.c | 16 +++++-----
 drivers/pci/remove.c       |  2 +-
 include/linux/pci.h        |  3 +-
 8 files changed, 75 insertions(+), 45 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 826b5016a101..cb1a1aaefa90 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -346,7 +346,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 		of_pci_make_dev_node(dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);
-	pci_bridge_d3_update(dev);
+	pci_bridge_d3cold_update(dev);
 
 	dev->match_driver = !dn || of_device_is_available(dn);
 	retval = device_attach(&dev->dev);
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 0f260cdc4592..aaf5a68e7984 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1434,7 +1434,7 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
 	 * reason is that the bridge may have additional methods such as
 	 * _DSW that need to be called.
 	 */
-	if (pci_dev->bridge_d3_allowed)
+	if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)
 		device_wakeup_enable(dev);
 
 	acpi_pci_wakeup(pci_dev, false);
@@ -1452,7 +1452,8 @@ void pci_acpi_cleanup(struct device *dev, struct acpi_device *adev)
 	pci_acpi_remove_pm_notifier(adev);
 	if (adev->wakeup.flags.valid) {
 		acpi_device_power_remove_dependent(adev, dev);
-		if (pci_dev->bridge_d3_allowed)
+		if (pci_dev->bridge_d3cold_allowed &&
+		    pci_dev->bridge_d3hot_allowed)
 			device_wakeup_disable(dev);
 
 		device_set_wakeup_capable(dev, false);
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 40cfa716392f..45628b0dd116 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -529,7 +529,7 @@ static ssize_t d3cold_allowed_store(struct device *dev,
 		return -EINVAL;
 
 	pdev->d3cold_allowed = !!val;
-	pci_bridge_d3_update(pdev);
+	pci_bridge_d3cold_update(pdev);
 
 	pm_runtime_resume(dev);
 
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0edc4e448c2d..c7a4f961ec28 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -166,9 +166,9 @@ bool pci_ats_disabled(void)
 }
 EXPORT_SYMBOL_GPL(pci_ats_disabled);
 
-/* Disable bridge_d3 for all PCIe ports */
+/* Disable both D3Hot and D3Cold for all PCIe ports */
 static bool pci_bridge_d3_disable;
-/* Force bridge_d3 for all PCIe ports */
+/* Force both D3Hot and D3Cold for all PCIe ports */
 static bool pci_bridge_d3_force;
 
 static int __init pcie_port_pm_setup(char *str)
@@ -2966,14 +2966,11 @@ static const struct dmi_system_id bridge_d3_blacklist[] = {
 	{ }
 };
 
-/**
- * pci_bridge_d3_allowed - Is it allowed to put the bridge into D3
- * @bridge: Bridge to check
- *
- * This function checks if the bridge is allowed to move to D3.
- * Currently we only allow D3 for recent enough PCIe ports and Thunderbolt.
+/*
+ * Helper function to check whether it is allowed to put the bridge into D3
+ * states (D3Hot and D3Cold).
  */
-bool pci_bridge_d3_allowed(struct pci_dev *bridge)
+static bool pci_bridge_d3_allowed(struct pci_dev *bridge, pci_power_t state)
 {
 	if (!pci_is_pcie(bridge))
 		return false;
@@ -3026,6 +3023,32 @@ bool pci_bridge_d3_allowed(struct pci_dev *bridge)
 	return false;
 }
 
+/**
+ * pci_bridge_d3cold_allowed - Is it allowed to put the bridge into D3Cold
+ * @bridge: Bridge to check
+ *
+ * This function checks if the bridge is allowed to move to D3Cold.
+ * Currently we only allow D3Cold for recent enough PCIe ports on ACPI based
+ * platforms and Thunderbolt.
+ */
+bool pci_bridge_d3cold_allowed(struct pci_dev *bridge)
+{
+	return pci_bridge_d3_allowed(bridge, PCI_D3cold);
+}
+
+/**
+ * pci_bridge_d3hot_allowed - Is it allowed to put the bridge into D3Hot
+ * @bridge: Bridge to check
+ *
+ * This function checks if the bridge is allowed to move to D3Hot.
+ * Currently we only allow D3Hot for recent enough PCIe ports on ACPI based
+ * platforms and Thunderbolt.
+ */
+bool pci_bridge_d3hot_allowed(struct pci_dev *bridge)
+{
+	return pci_bridge_d3_allowed(bridge, PCI_D3hot);
+}
+
 static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
 {
 	bool *d3cold_ok = data;
@@ -3046,55 +3069,55 @@ static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
 }
 
 /*
- * pci_bridge_d3_update - Update bridge D3 capabilities
+ * pci_bridge_d3cold_update - Update bridge D3Cold capabilities
  * @dev: PCI device which is changed
  *
  * Update upstream bridge PM capabilities accordingly depending on if the
  * device PM configuration was changed or the device is being removed.  The
  * change is also propagated upstream.
  */
-void pci_bridge_d3_update(struct pci_dev *dev)
+void pci_bridge_d3cold_update(struct pci_dev *dev)
 {
 	bool remove = !device_is_registered(&dev->dev);
 	struct pci_dev *bridge;
 	bool d3cold_ok = true;
 
 	bridge = pci_upstream_bridge(dev);
-	if (!bridge || !pci_bridge_d3_allowed(bridge))
+	if (!bridge || !pci_bridge_d3cold_allowed(bridge))
 		return;
 
 	/*
-	 * If D3 is currently allowed for the bridge, removing one of its
+	 * If D3Cold is currently allowed for the bridge, removing one of its
 	 * children won't change that.
 	 */
-	if (remove && bridge->bridge_d3_allowed)
+	if (remove && bridge->bridge_d3cold_allowed)
 		return;
 
 	/*
-	 * If D3 is currently allowed for the bridge and a child is added or
-	 * changed, disallowance of D3 can only be caused by that child, so
+	 * If D3Cold is currently allowed for the bridge and a child is added or
+	 * changed, disallowance of D3Cold can only be caused by that child, so
 	 * we only need to check that single device, not any of its siblings.
 	 *
-	 * If D3 is currently not allowed for the bridge, checking the device
-	 * first may allow us to skip checking its siblings.
+	 * If D3Cold is currently not allowed for the bridge, checking the
+	 * device first may allow us to skip checking its siblings.
 	 */
 	if (!remove)
 		pci_dev_check_d3cold(dev, &d3cold_ok);
 
 	/*
-	 * If D3 is currently not allowed for the bridge, this may be caused
+	 * If D3Cold is currently not allowed for the bridge, this may be caused
 	 * either by the device being changed/removed or any of its siblings,
 	 * so we need to go through all children to find out if one of them
-	 * continues to block D3.
+	 * continues to block D3Cold.
 	 */
-	if (d3cold_ok && !bridge->bridge_d3_allowed)
+	if (d3cold_ok && !bridge->bridge_d3cold_allowed)
 		pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
 			     &d3cold_ok);
 
-	if (bridge->bridge_d3_allowed != d3cold_ok) {
-		bridge->bridge_d3_allowed = d3cold_ok;
+	if (bridge->bridge_d3cold_allowed != d3cold_ok) {
+		bridge->bridge_d3cold_allowed = d3cold_ok;
 		/* Propagate change to upstream bridges */
-		pci_bridge_d3_update(bridge);
+		pci_bridge_d3cold_update(bridge);
 	}
 }
 
@@ -3110,7 +3133,7 @@ void pci_d3cold_enable(struct pci_dev *dev)
 {
 	if (dev->no_d3cold) {
 		dev->no_d3cold = false;
-		pci_bridge_d3_update(dev);
+		pci_bridge_d3cold_update(dev);
 	}
 }
 EXPORT_SYMBOL_GPL(pci_d3cold_enable);
@@ -3127,7 +3150,7 @@ void pci_d3cold_disable(struct pci_dev *dev)
 {
 	if (!dev->no_d3cold) {
 		dev->no_d3cold = true;
-		pci_bridge_d3_update(dev);
+		pci_bridge_d3cold_update(dev);
 	}
 }
 EXPORT_SYMBOL_GPL(pci_d3cold_disable);
@@ -3167,7 +3190,8 @@ void pci_pm_init(struct pci_dev *dev)
 	dev->pm_cap = pm;
 	dev->d3hot_delay = PCI_PM_D3HOT_WAIT;
 	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
-	dev->bridge_d3_allowed = pci_bridge_d3_allowed(dev);
+	dev->bridge_d3cold_allowed = pci_bridge_d3cold_allowed(dev);
+	dev->bridge_d3hot_allowed = pci_bridge_d3hot_allowed(dev);
 	dev->d3cold_allowed = true;
 
 	dev->d1_support = false;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 53ca75639201..f819eab793fc 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -92,8 +92,9 @@ void pci_pm_init(struct pci_dev *dev);
 void pci_ea_init(struct pci_dev *dev);
 void pci_msi_init(struct pci_dev *dev);
 void pci_msix_init(struct pci_dev *dev);
-bool pci_bridge_d3_allowed(struct pci_dev *dev);
-void pci_bridge_d3_update(struct pci_dev *dev);
+bool pci_bridge_d3cold_allowed(struct pci_dev *dev);
+bool pci_bridge_d3hot_allowed(struct pci_dev *dev);
+void pci_bridge_d3cold_update(struct pci_dev *dev);
 int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
 
 static inline void pci_wakeup_event(struct pci_dev *dev)
@@ -111,9 +112,12 @@ static inline bool pci_power_manageable(struct pci_dev *pci_dev)
 {
 	/*
 	 * Currently we allow normal PCI devices and PCI bridges transition
-	 * into D3 if their bridge_d3 is set.
+	 * into D3 states if both bridge_d3cold_allowed and bridge_d3hot_allowed
+	 * are set.
 	 */
-	return !pci_has_subordinate(pci_dev) || pci_dev->bridge_d3_allowed;
+	return !pci_has_subordinate(pci_dev) ||
+	       (pci_dev->bridge_d3cold_allowed &&
+		pci_dev->bridge_d3hot_allowed);
 }
 
 static inline bool pcie_downstream_port(const struct pci_dev *dev)
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 8401a0f7b394..655754b9f06a 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -632,7 +632,7 @@ __setup("pcie_ports=", pcie_port_setup);
 #ifdef CONFIG_PM
 static int pcie_port_runtime_suspend(struct device *dev)
 {
-	if (!to_pci_dev(dev)->bridge_d3_allowed)
+	if (!to_pci_dev(dev)->bridge_d3hot_allowed)
 		return -EBUSY;
 
 	return pcie_port_device_runtime_suspend(dev);
@@ -641,11 +641,11 @@ static int pcie_port_runtime_suspend(struct device *dev)
 static int pcie_port_runtime_idle(struct device *dev)
 {
 	/*
-	 * Assume the PCI core has set bridge_d3_allowed whenever it thinks the
-	 * port should be good to go to D3.  Everything else, including moving
-	 * the port to D3, is handled by the PCI core.
+	 * Assume the PCI core has set bridge_d3hot_allowed whenever it thinks
+	 * the port should be good to go to D3Hot.  Everything else, including
+	 * moving the port to D3Hot, is handled by the PCI core.
 	 */
-	return to_pci_dev(dev)->bridge_d3_allowed ? 0 : -EBUSY;
+	return to_pci_dev(dev)->bridge_d3hot_allowed ? 0 : -EBUSY;
 }
 
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
@@ -702,7 +702,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE |
 					   DPM_FLAG_SMART_SUSPEND);
 
-	if (dev->bridge_d3_allowed) {
+	if (dev->bridge_d3hot_allowed) {
 		/*
 		 * Keep the port resumed 100ms to make sure things like
 		 * config space accesses from userspace (lspci) will not
@@ -720,7 +720,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
-	if (dev->bridge_d3_allowed) {
+	if (dev->bridge_d3hot_allowed) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);
@@ -733,7 +733,7 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
 
 static void pcie_portdrv_shutdown(struct pci_dev *dev)
 {
-	if (dev->bridge_d3_allowed) {
+	if (dev->bridge_d3hot_allowed) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index d749ea8250d6..36d8cb50b582 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -41,7 +41,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
 
 	pci_doe_destroy(dev);
 	pcie_aspm_exit_link_state(dev);
-	pci_bridge_d3_update(dev);
+	pci_bridge_d3cold_update(dev);
 	pci_free_resources(dev);
 	put_device(&dev->dev);
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2a48c88512e1..d0947f932b9a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -375,7 +375,8 @@ struct pci_dev {
 	unsigned int	d2_support:1;	/* Low power state D2 is supported */
 	unsigned int	no_d1d2:1;	/* D1 and D2 are forbidden */
 	unsigned int	no_d3cold:1;	/* D3cold is forbidden */
-	unsigned int	bridge_d3_allowed:1;	/* Allow D3 for bridge */
+	unsigned int	bridge_d3cold_allowed:1;	/* Allow D3Cold for bridge */
+	unsigned int	bridge_d3hot_allowed:1;		/* Allow D3Hot for bridge */
 	unsigned int	d3cold_allowed:1;	/* D3cold is allowed by user */
 	unsigned int	mmio_always_on:1;	/* Disallow turning off io/mem
 						   decoding during BAR sizing */

-- 
2.25.1

[PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Unlike ACPI based platforms, there are no known issues with D3Hot for the
PCI bridges in the Devicetree based platforms. So let's allow the PCI
bridges to go to D3Hot during runtime. It should be noted that the bridges
need to be defined in Devicetree for this to work.

Currently, D3Cold is not allowed since Vcc supply which is required for
transitioning the device to D3Cold is not exposed on all Devicetree based
platforms.

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/pci.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c7a4f961ec28..bc1e1ca673f1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2992,6 +2992,18 @@ static bool pci_bridge_d3_allowed(struct pci_dev *bridge, pci_power_t state)
 		if (pci_bridge_d3_force)
 			return true;
 
+		/*
+		 * Allow D3Hot for all Devicetree based platforms having a
+		 * separate node for the bridge. We don't allow D3Cold for now
+		 * since not all platforms are exposing the Vcc supply in
+		 * Devicetree which is required for transitioning the bridge to
+		 * D3Cold.
+		 *
+		 * NOTE: The bridge is expected to be defined in Devicetree.
+		 */
+		if (state == PCI_D3hot && dev_of_node(&bridge->dev))
+			return true;
+
 		/* Even the oldest 2010 Thunderbolt controller supports D3. */
 		if (bridge->is_thunderbolt)
 			return true;
@@ -3042,7 +3054,7 @@ bool pci_bridge_d3cold_allowed(struct pci_dev *bridge)
  *
  * This function checks if the bridge is allowed to move to D3Hot.
  * Currently we only allow D3Hot for recent enough PCIe ports on ACPI based
- * platforms and Thunderbolt.
+ * platforms, Thunderbolt and Devicetree based platforms.
  */
 bool pci_bridge_d3hot_allowed(struct pci_dev *bridge)
 {

-- 
2.25.1

Re: [PATCH v5 1/4] PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility

[Lukas Wunner]

On Fri, Aug 02, 2024 at 11:25:00AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> PCI core is already caching the value of pci_bridge_d3_possible() in
> pci_dev::bridge_d3 during pci_pm_init(). Since the value is not going to
> change, let's make use of the cached value.
[...]
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -702,7 +702,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  	dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE |
>  					   DPM_FLAG_SMART_SUSPEND);
>  
> -	if (pci_bridge_d3_possible(dev)) {
> +	if (dev->bridge_d3) {

I don't know if there was a reason to call pci_bridge_d3_possible()
(instead of using the cached value) on probe, remove and shutdown.

The change is probably safe but it would still be good to get some
positive test results with Thunderbolt laptops etc to raise the
confidence.

Thanks,

Lukas

Re: [PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

[Lukas Wunner]

On Fri, Aug 02, 2024 at 11:25:03AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> Unlike ACPI based platforms, there are no known issues with D3Hot for the
> PCI bridges in the Devicetree based platforms. So let's allow the PCI
> bridges to go to D3Hot during runtime. It should be noted that the bridges
> need to be defined in Devicetree for this to work.
[...]
> +		if (state == PCI_D3hot && dev_of_node(&bridge->dev))
> +			return true;

For such a simple change which several parties are interested in,
I think it would be better to move it to the front of the series.

Patch [1/4] looks like an optimization (using a cached value)
which this patch doesn't depend on.  Patch [2/4] looks like a
change of bikeshed color which isn't strictly necessary for
this fourth patch either.  If you want to propose those changes,
fine, but by making this fourth patch depend on them, you risk
delaying its acceptance.  As an upstreaming strategy it's usually
smarter to move potentially controversial or unnecessary material
to the back of the series, or submit it separately if it can be
applied standalone.


> Currently, D3Cold is not allowed since Vcc supply which is required for
> transitioning the device to D3Cold is not exposed on all Devicetree based
> platforms.

The PCI core cannot put devices into D3cold without help from the
platform.  Checking whether D3cold is possible (or allowed or
whatever) thus requires asking platform support code via
platform_pci_power_manageable(), platform_pci_choose_state() etc.

I think patch [3/4] is a little confusing because it creates
infrastructure to decide whether D3cold is supported (allowed?)
but we already have that in the platform_pci_*() functions.
So I'm not sure if patch [3/4] adds value.  I think generally
speaking if D3hot isn't possible (allowed?), D3cold is assumed
to not be possible either.

Thanks,

Lukas

Re: [PATCH v5 1/4] PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility

[Rafael J. Wysocki]

On Fri, Aug 2, 2024 at 11:49 AM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Fri, Aug 02, 2024 at 11:25:00AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > PCI core is already caching the value of pci_bridge_d3_possible() in
> > pci_dev::bridge_d3 during pci_pm_init(). Since the value is not going to
> > change,

Is that really the case?

Have you seen pci_bridge_d3_update()?

> let's make use of the cached value.
> [...]
> > --- a/drivers/pci/pcie/portdrv.c
> > +++ b/drivers/pci/pcie/portdrv.c
> > @@ -702,7 +702,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> >       dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE |
> >                                          DPM_FLAG_SMART_SUSPEND);
> >
> > -     if (pci_bridge_d3_possible(dev)) {
> > +     if (dev->bridge_d3) {
>
> I don't know if there was a reason to call pci_bridge_d3_possible()
> (instead of using the cached value) on probe, remove and shutdown.
>
> The change is probably safe but it would still be good to get some
> positive test results with Thunderbolt laptops etc to raise the
> confidence.

If I'm not mistaken, the change is not correct.

Re: [PATCH v5 1/4] PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility

[Lukas Wunner]

On Fri, Aug 02, 2024 at 01:19:24PM +0200, Rafael J. Wysocki wrote:
> On Fri, Aug 2, 2024 at 11:49AM Lukas Wunner <lukas@wunner.de> wrote:
> >
> > On Fri, Aug 02, 2024 at 11:25:00AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > PCI core is already caching the value of pci_bridge_d3_possible() in
> > > pci_dev::bridge_d3 during pci_pm_init(). Since the value is not going to
> > > change,
> 
> Is that really the case?
> 
> Have you seen pci_bridge_d3_update()?

Okay the value may change at runtime, e.g. due to user space
manipulating d3cold_allowed in sysfs.

> > I don't know if there was a reason to call pci_bridge_d3_possible()
> > (instead of using the cached value) on probe, remove and shutdown.
> >
> > The change is probably safe but it would still be good to get some
> > positive test results with Thunderbolt laptops etc to raise the
> > confidence.
> 
> If I'm not mistaken, the change is not correct.

You're right.  Because the value may change, different code paths
may be chosen on probe, remove and shutdown.  Sorry for missing that.

Thanks,

Lukas

Re: [PATCH v5 2/4] PCI: Rename pci_bridge_d3_possible() to pci_bridge_d3_allowed()

[kernel test robot]

Hi Manivannan,

kernel test robot noticed the following build errors:

[auto build test ERROR on 705c1da8fa4816fb0159b5602fef1df5946a3ee2]

url:    https://github.com/intel-lab-lkp/linux/commits/Manivannan-Sadhasivam-via-B4-Relay/PCI-portdrv-Make-use-of-pci_dev-bridge_d3-for-checking-the-D3-possibility/20240803-074434
base:   705c1da8fa4816fb0159b5602fef1df5946a3ee2
patch link:    https://lore.kernel.org/r/20240802-pci-bridge-d3-v5-2-2426dd9e8e27%40linaro.org
patch subject: [PATCH v5 2/4] PCI: Rename pci_bridge_d3_possible() to pci_bridge_d3_allowed()
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20240803/202408031855.TEPJlfzl-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240803/202408031855.TEPJlfzl-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408031855.TEPJlfzl-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpu/drm/radeon/radeon_atpx_handler.c: In function 'radeon_atpx_detect':
>> drivers/gpu/drm/radeon/radeon_atpx_handler.c:568:59: error: 'struct pci_dev' has no member named 'bridge_d3'
     568 |                 d3_supported |= parent_pdev && parent_pdev->bridge_d3;
         |                                                           ^~
   drivers/gpu/drm/radeon/radeon_atpx_handler.c:578:59: error: 'struct pci_dev' has no member named 'bridge_d3'
     578 |                 d3_supported |= parent_pdev && parent_pdev->bridge_d3;
         |                                                           ^~
--
   drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c: In function 'amdgpu_atpx_detect':
>> drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c:628:59: error: 'struct pci_dev' has no member named 'bridge_d3'
     628 |                 d3_supported |= parent_pdev && parent_pdev->bridge_d3;
         |                                                           ^~
   drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c:638:59: error: 'struct pci_dev' has no member named 'bridge_d3'
     638 |                 d3_supported |= parent_pdev && parent_pdev->bridge_d3;
         |                                                           ^~
--
   drivers/gpu/drm/nouveau/nouveau_acpi.c: In function 'nouveau_dsm_pci_probe':
>> drivers/gpu/drm/nouveau/nouveau_acpi.c:229:32: error: 'struct pci_dev' has no member named 'bridge_d3'
     229 |                 if (parent_pdev->bridge_d3)
         |                                ^~


vim +568 drivers/gpu/drm/radeon/radeon_atpx_handler.c

6a9ee8af344e3b Dave Airlie  2010-02-01  545  
82e029357d4726 Alex Deucher 2012-08-16  546  /**
82e029357d4726 Alex Deucher 2012-08-16  547   * radeon_atpx_detect - detect whether we have PX
82e029357d4726 Alex Deucher 2012-08-16  548   *
82e029357d4726 Alex Deucher 2012-08-16  549   * Check if we have a PX system (all asics).
82e029357d4726 Alex Deucher 2012-08-16  550   * Returns true if we have a PX system, false if not.
82e029357d4726 Alex Deucher 2012-08-16  551   */
6a9ee8af344e3b Dave Airlie  2010-02-01  552  static bool radeon_atpx_detect(void)
6a9ee8af344e3b Dave Airlie  2010-02-01  553  {
6a9ee8af344e3b Dave Airlie  2010-02-01  554  	char acpi_method_name[255] = { 0 };
6a9ee8af344e3b Dave Airlie  2010-02-01  555  	struct acpi_buffer buffer = {sizeof(acpi_method_name), acpi_method_name};
6a9ee8af344e3b Dave Airlie  2010-02-01  556  	struct pci_dev *pdev = NULL;
6a9ee8af344e3b Dave Airlie  2010-02-01  557  	bool has_atpx = false;
6a9ee8af344e3b Dave Airlie  2010-02-01  558  	int vga_count = 0;
bcfdd5d5105087 Alex Deucher 2016-11-28  559  	bool d3_supported = false;
bcfdd5d5105087 Alex Deucher 2016-11-28  560  	struct pci_dev *parent_pdev;
6a9ee8af344e3b Dave Airlie  2010-02-01  561  
6a9ee8af344e3b Dave Airlie  2010-02-01  562  	while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) != NULL) {
6a9ee8af344e3b Dave Airlie  2010-02-01  563  		vga_count++;
6a9ee8af344e3b Dave Airlie  2010-02-01  564  
6a9ee8af344e3b Dave Airlie  2010-02-01  565  		has_atpx |= (radeon_atpx_pci_probe_handle(pdev) == true);
bcfdd5d5105087 Alex Deucher 2016-11-28  566  
bcfdd5d5105087 Alex Deucher 2016-11-28  567  		parent_pdev = pci_upstream_bridge(pdev);
bcfdd5d5105087 Alex Deucher 2016-11-28 @568  		d3_supported |= parent_pdev && parent_pdev->bridge_d3;
6a9ee8af344e3b Dave Airlie  2010-02-01  569  	}
6a9ee8af344e3b Dave Airlie  2010-02-01  570  
e9a4099a59cc59 Alex Deucher 2014-04-15  571  	/* some newer PX laptops mark the dGPU as a non-VGA display device */
e9a4099a59cc59 Alex Deucher 2014-04-15  572  	while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_OTHER << 8, pdev)) != NULL) {
e9a4099a59cc59 Alex Deucher 2014-04-15  573  		vga_count++;
e9a4099a59cc59 Alex Deucher 2014-04-15  574  
e9a4099a59cc59 Alex Deucher 2014-04-15  575  		has_atpx |= (radeon_atpx_pci_probe_handle(pdev) == true);
bcfdd5d5105087 Alex Deucher 2016-11-28  576  
bcfdd5d5105087 Alex Deucher 2016-11-28  577  		parent_pdev = pci_upstream_bridge(pdev);
bcfdd5d5105087 Alex Deucher 2016-11-28  578  		d3_supported |= parent_pdev && parent_pdev->bridge_d3;
e9a4099a59cc59 Alex Deucher 2014-04-15  579  	}
e9a4099a59cc59 Alex Deucher 2014-04-15  580  
6a9ee8af344e3b Dave Airlie  2010-02-01  581  	if (has_atpx && vga_count == 2) {
492b49a2f21a7c Alex Deucher 2012-08-16  582  		acpi_get_name(radeon_atpx_priv.atpx.handle, ACPI_FULL_PATHNAME, &buffer);
7ca85295d8cc28 Joe Perches  2017-02-28  583  		pr_info("vga_switcheroo: detected switching method %s handle\n",
6a9ee8af344e3b Dave Airlie  2010-02-01  584  			acpi_method_name);
6a9ee8af344e3b Dave Airlie  2010-02-01  585  		radeon_atpx_priv.atpx_detected = true;
bcfdd5d5105087 Alex Deucher 2016-11-28  586  		radeon_atpx_priv.bridge_pm_usable = d3_supported;
69ee9742f945cd Alex Deucher 2016-07-27  587  		radeon_atpx_init();
6a9ee8af344e3b Dave Airlie  2010-02-01  588  		return true;
6a9ee8af344e3b Dave Airlie  2010-02-01  589  	}
6a9ee8af344e3b Dave Airlie  2010-02-01  590  	return false;
6a9ee8af344e3b Dave Airlie  2010-02-01  591  }
6a9ee8af344e3b Dave Airlie  2010-02-01  592  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v5 1/4] PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility

[Manivannan Sadhasivam]

On Fri, Aug 02, 2024 at 10:07:39PM +0200, Lukas Wunner wrote:
> On Fri, Aug 02, 2024 at 01:19:24PM +0200, Rafael J. Wysocki wrote:
> > On Fri, Aug 2, 2024 at 11:49AM Lukas Wunner <lukas@wunner.de> wrote:
> > >
> > > On Fri, Aug 02, 2024 at 11:25:00AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > > PCI core is already caching the value of pci_bridge_d3_possible() in
> > > > pci_dev::bridge_d3 during pci_pm_init(). Since the value is not going to
> > > > change,
> > 
> > Is that really the case?
> > 
> > Have you seen pci_bridge_d3_update()?
> 
> Okay the value may change at runtime, e.g. due to user space
> manipulating d3cold_allowed in sysfs.
> 

The last part of the commit message is wrong, but pci_bridge_d3_update() is
already updating pci_dev::bridge_d3. And pci_bridge_d3_possible() is not making
use of any checks that could change dynamically IIUC. So what is wrong in using
pci_dev::bridge_d3?

Even more, if the client drivers have changed the state of pci_dev::bridge_d3
during runtime, then pci_bridge_d3_possible() won't catch that. Or is there a
reason to not do so purposefully?

> > > I don't know if there was a reason to call pci_bridge_d3_possible()
> > > (instead of using the cached value) on probe, remove and shutdown.
> > >
> > > The change is probably safe but it would still be good to get some
> > > positive test results with Thunderbolt laptops etc to raise the
> > > confidence.
> > 
> > If I'm not mistaken, the change is not correct.
> 
> You're right.  Because the value may change, different code paths
> may be chosen on probe, remove and shutdown.  Sorry for missing that.
> 

Again, pci_bridge_d3_possible() is not making use of values that could change
dynamically.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 2/4] PCI: Rename pci_bridge_d3_possible() to pci_bridge_d3_allowed()

[Manivannan Sadhasivam]

On Sat, Aug 03, 2024 at 07:03:56PM +0800, kernel test robot wrote:
> Hi Manivannan,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on 705c1da8fa4816fb0159b5602fef1df5946a3ee2]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Manivannan-Sadhasivam-via-B4-Relay/PCI-portdrv-Make-use-of-pci_dev-bridge_d3-for-checking-the-D3-possibility/20240803-074434
> base:   705c1da8fa4816fb0159b5602fef1df5946a3ee2
> patch link:    https://lore.kernel.org/r/20240802-pci-bridge-d3-v5-2-2426dd9e8e27%40linaro.org
> patch subject: [PATCH v5 2/4] PCI: Rename pci_bridge_d3_possible() to pci_bridge_d3_allowed()
> config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20240803/202408031855.TEPJlfzl-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240803/202408031855.TEPJlfzl-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202408031855.TEPJlfzl-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/gpu/drm/radeon/radeon_atpx_handler.c: In function 'radeon_atpx_detect':
> >> drivers/gpu/drm/radeon/radeon_atpx_handler.c:568:59: error: 'struct pci_dev' has no member named 'bridge_d3'
>      568 |                 d3_supported |= parent_pdev && parent_pdev->bridge_d3;
>          |                                                           ^~
>    drivers/gpu/drm/radeon/radeon_atpx_handler.c:578:59: error: 'struct pci_dev' has no member named 'bridge_d3'
>      578 |                 d3_supported |= parent_pdev && parent_pdev->bridge_d3;
>          |                                                           ^~
> --
>    drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c: In function 'amdgpu_atpx_detect':
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c:628:59: error: 'struct pci_dev' has no member named 'bridge_d3'
>      628 |                 d3_supported |= parent_pdev && parent_pdev->bridge_d3;
>          |                                                           ^~
>    drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c:638:59: error: 'struct pci_dev' has no member named 'bridge_d3'
>      638 |                 d3_supported |= parent_pdev && parent_pdev->bridge_d3;
>          |                                                           ^~
> --
>    drivers/gpu/drm/nouveau/nouveau_acpi.c: In function 'nouveau_dsm_pci_probe':
> >> drivers/gpu/drm/nouveau/nouveau_acpi.c:229:32: error: 'struct pci_dev' has no member named 'bridge_d3'
>      229 |                 if (parent_pdev->bridge_d3)
>          |                                ^~

Ok, there seems to be a couple of drivers making use of pci_dev::bridge_d3 to
check if D3Cold is supported or not. And this further strengthens the fact that
PCI core should not rely on pci_bridge_d3_possible() as proposed in patch 1.

- Mani

> 
> 
> vim +568 drivers/gpu/drm/radeon/radeon_atpx_handler.c
> 
> 6a9ee8af344e3b Dave Airlie  2010-02-01  545  
> 82e029357d4726 Alex Deucher 2012-08-16  546  /**
> 82e029357d4726 Alex Deucher 2012-08-16  547   * radeon_atpx_detect - detect whether we have PX
> 82e029357d4726 Alex Deucher 2012-08-16  548   *
> 82e029357d4726 Alex Deucher 2012-08-16  549   * Check if we have a PX system (all asics).
> 82e029357d4726 Alex Deucher 2012-08-16  550   * Returns true if we have a PX system, false if not.
> 82e029357d4726 Alex Deucher 2012-08-16  551   */
> 6a9ee8af344e3b Dave Airlie  2010-02-01  552  static bool radeon_atpx_detect(void)
> 6a9ee8af344e3b Dave Airlie  2010-02-01  553  {
> 6a9ee8af344e3b Dave Airlie  2010-02-01  554  	char acpi_method_name[255] = { 0 };
> 6a9ee8af344e3b Dave Airlie  2010-02-01  555  	struct acpi_buffer buffer = {sizeof(acpi_method_name), acpi_method_name};
> 6a9ee8af344e3b Dave Airlie  2010-02-01  556  	struct pci_dev *pdev = NULL;
> 6a9ee8af344e3b Dave Airlie  2010-02-01  557  	bool has_atpx = false;
> 6a9ee8af344e3b Dave Airlie  2010-02-01  558  	int vga_count = 0;
> bcfdd5d5105087 Alex Deucher 2016-11-28  559  	bool d3_supported = false;
> bcfdd5d5105087 Alex Deucher 2016-11-28  560  	struct pci_dev *parent_pdev;
> 6a9ee8af344e3b Dave Airlie  2010-02-01  561  
> 6a9ee8af344e3b Dave Airlie  2010-02-01  562  	while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) != NULL) {
> 6a9ee8af344e3b Dave Airlie  2010-02-01  563  		vga_count++;
> 6a9ee8af344e3b Dave Airlie  2010-02-01  564  
> 6a9ee8af344e3b Dave Airlie  2010-02-01  565  		has_atpx |= (radeon_atpx_pci_probe_handle(pdev) == true);
> bcfdd5d5105087 Alex Deucher 2016-11-28  566  
> bcfdd5d5105087 Alex Deucher 2016-11-28  567  		parent_pdev = pci_upstream_bridge(pdev);
> bcfdd5d5105087 Alex Deucher 2016-11-28 @568  		d3_supported |= parent_pdev && parent_pdev->bridge_d3;
> 6a9ee8af344e3b Dave Airlie  2010-02-01  569  	}
> 6a9ee8af344e3b Dave Airlie  2010-02-01  570  
> e9a4099a59cc59 Alex Deucher 2014-04-15  571  	/* some newer PX laptops mark the dGPU as a non-VGA display device */
> e9a4099a59cc59 Alex Deucher 2014-04-15  572  	while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_OTHER << 8, pdev)) != NULL) {
> e9a4099a59cc59 Alex Deucher 2014-04-15  573  		vga_count++;
> e9a4099a59cc59 Alex Deucher 2014-04-15  574  
> e9a4099a59cc59 Alex Deucher 2014-04-15  575  		has_atpx |= (radeon_atpx_pci_probe_handle(pdev) == true);
> bcfdd5d5105087 Alex Deucher 2016-11-28  576  
> bcfdd5d5105087 Alex Deucher 2016-11-28  577  		parent_pdev = pci_upstream_bridge(pdev);
> bcfdd5d5105087 Alex Deucher 2016-11-28  578  		d3_supported |= parent_pdev && parent_pdev->bridge_d3;
> e9a4099a59cc59 Alex Deucher 2014-04-15  579  	}
> e9a4099a59cc59 Alex Deucher 2014-04-15  580  
> 6a9ee8af344e3b Dave Airlie  2010-02-01  581  	if (has_atpx && vga_count == 2) {
> 492b49a2f21a7c Alex Deucher 2012-08-16  582  		acpi_get_name(radeon_atpx_priv.atpx.handle, ACPI_FULL_PATHNAME, &buffer);
> 7ca85295d8cc28 Joe Perches  2017-02-28  583  		pr_info("vga_switcheroo: detected switching method %s handle\n",
> 6a9ee8af344e3b Dave Airlie  2010-02-01  584  			acpi_method_name);
> 6a9ee8af344e3b Dave Airlie  2010-02-01  585  		radeon_atpx_priv.atpx_detected = true;
> bcfdd5d5105087 Alex Deucher 2016-11-28  586  		radeon_atpx_priv.bridge_pm_usable = d3_supported;
> 69ee9742f945cd Alex Deucher 2016-07-27  587  		radeon_atpx_init();
> 6a9ee8af344e3b Dave Airlie  2010-02-01  588  		return true;
> 6a9ee8af344e3b Dave Airlie  2010-02-01  589  	}
> 6a9ee8af344e3b Dave Airlie  2010-02-01  590  	return false;
> 6a9ee8af344e3b Dave Airlie  2010-02-01  591  }
> 6a9ee8af344e3b Dave Airlie  2010-02-01  592  
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

[Manivannan Sadhasivam]

On Fri, Aug 02, 2024 at 12:13:31PM +0200, Lukas Wunner wrote:
> On Fri, Aug 02, 2024 at 11:25:03AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > Unlike ACPI based platforms, there are no known issues with D3Hot for the
> > PCI bridges in the Devicetree based platforms. So let's allow the PCI
> > bridges to go to D3Hot during runtime. It should be noted that the bridges
> > need to be defined in Devicetree for this to work.
> [...]
> > +		if (state == PCI_D3hot && dev_of_node(&bridge->dev))
> > +			return true;
> 
> For such a simple change which several parties are interested in,
> I think it would be better to move it to the front of the series.
> 
> Patch [1/4] looks like an optimization (using a cached value)
> which this patch doesn't depend on.  Patch [2/4] looks like a
> change of bikeshed color which isn't strictly necessary for
> this fourth patch either.  If you want to propose those changes,
> fine, but by making this fourth patch depend on them, you risk
> delaying its acceptance.  As an upstreaming strategy it's usually
> smarter to move potentially controversial or unnecessary material
> to the back of the series, or submit it separately if it can be
> applied standalone.
> 

Agree with you! Even after doing upstreaming for this much time, I tend to
ignore this...

> 
> > Currently, D3Cold is not allowed since Vcc supply which is required for
> > transitioning the device to D3Cold is not exposed on all Devicetree based
> > platforms.
> 
> The PCI core cannot put devices into D3cold without help from the
> platform.  Checking whether D3cold is possible (or allowed or
> whatever) thus requires asking platform support code via
> platform_pci_power_manageable(), platform_pci_choose_state() etc.
> 
> I think patch [3/4] is a little confusing because it creates
> infrastructure to decide whether D3cold is supported (allowed?)
> but we already have that in the platform_pci_*() functions.
> So I'm not sure if patch [3/4] adds value.  I think generally
> speaking if D3hot isn't possible (allowed?), D3cold is assumed
> to not be possible either.
> 

Why? D3Hot is useful for runtime PM and if the platform doesn't want to do
runtime PM, it can always skip D3Hot (not ideal though). But D3Cold is a power
off state, and the platform may choose to use it for the case of system suspend.

So I still feel that decoupling D3Hot and D3Cold is necessary.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 1/4] PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility

[Lukas Wunner]

On Mon, Aug 05, 2024 at 06:54:42PM +0530, Manivannan Sadhasivam wrote:
> So what is wrong in using pci_dev::bridge_d3?

The bridge_d3 flag may change at runtime, e.g. when writing to the
d3cold_allowed attribute in sysfs.

If e.g. bridge_d3 is set when pcie_portdrv_probe() runs but no longer
set when pcie_portdrv_remove() runs, there would be a runtime PM ref
imbalance.  (Ref would be dropped on probe, but not reacquired on remove.)


> Again, pci_bridge_d3_possible() is not making use of values that could change
> dynamically.

Which is precisely the reason why it (and not the bridge_d3 flag) is
used by pcie_portdrv_{probe,remove,shutdown}().

Thanks,

Lukas

Re: [PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

[Lukas Wunner]

On Mon, Aug 05, 2024 at 07:05:55PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Aug 02, 2024 at 12:13:31PM +0200, Lukas Wunner wrote:
> > The PCI core cannot put devices into D3cold without help from the
> > platform.  Checking whether D3cold is possible (or allowed or
> > whatever) thus requires asking platform support code via
> > platform_pci_power_manageable(), platform_pci_choose_state() etc.
> > 
> > I think patch [3/4] is a little confusing because it creates
> > infrastructure to decide whether D3cold is supported (allowed?)
> > but we already have that in the platform_pci_*() functions.
> > So I'm not sure if patch [3/4] adds value.  I think generally
> > speaking if D3hot isn't possible (allowed?), D3cold is assumed
> > to not be possible either.
> 
> Why? D3Hot is useful for runtime PM and if the platform doesn't want to do
> runtime PM, it can always skip D3Hot (not ideal though).

AFAICS we always program the device to go to D3hot and the platform
then cuts power, thereby putting it into D3cold.  So D3hot is never
skipped.  See __pci_set_power_state():

	if (state == PCI_D3cold) {
		/*
		 * To put the device in D3cold, put it into D3hot in the native
		 * way, then put it into D3cold using platform ops.
		 */
		error = pci_set_low_power_state(dev, PCI_D3hot, locked);

		if (pci_platform_power_transition(dev, PCI_D3cold))
			return error;

Thanks,

Lukas

Re: [PATCH v5 1/4] PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility

[Manivannan Sadhasivam]

On Tue, Aug 06, 2024 at 08:46:48AM +0200, Lukas Wunner wrote:
> On Mon, Aug 05, 2024 at 06:54:42PM +0530, Manivannan Sadhasivam wrote:
> > So what is wrong in using pci_dev::bridge_d3?
> 
> The bridge_d3 flag may change at runtime, e.g. when writing to the
> d3cold_allowed attribute in sysfs.
> 
> If e.g. bridge_d3 is set when pcie_portdrv_probe() runs but no longer
> set when pcie_portdrv_remove() runs, there would be a runtime PM ref
> imbalance.  (Ref would be dropped on probe, but not reacquired on remove.)
> 

Ah, so it is the other way around. Thanks for clearing it up!

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

[Manivannan Sadhasivam]

On Tue, Aug 06, 2024 at 08:53:01AM +0200, Lukas Wunner wrote:
> On Mon, Aug 05, 2024 at 07:05:55PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Aug 02, 2024 at 12:13:31PM +0200, Lukas Wunner wrote:
> > > The PCI core cannot put devices into D3cold without help from the
> > > platform.  Checking whether D3cold is possible (or allowed or
> > > whatever) thus requires asking platform support code via
> > > platform_pci_power_manageable(), platform_pci_choose_state() etc.
> > > 
> > > I think patch [3/4] is a little confusing because it creates
> > > infrastructure to decide whether D3cold is supported (allowed?)
> > > but we already have that in the platform_pci_*() functions.
> > > So I'm not sure if patch [3/4] adds value.  I think generally
> > > speaking if D3hot isn't possible (allowed?), D3cold is assumed
> > > to not be possible either.
> > 
> > Why? D3Hot is useful for runtime PM and if the platform doesn't want to do
> > runtime PM, it can always skip D3Hot (not ideal though).
> 
> AFAICS we always program the device to go to D3hot and the platform
> then cuts power, thereby putting it into D3cold.  So D3hot is never
> skipped.  See __pci_set_power_state():
> 
> 	if (state == PCI_D3cold) {
> 		/*
> 		 * To put the device in D3cold, put it into D3hot in the native
> 		 * way, then put it into D3cold using platform ops.
> 		 */
> 		error = pci_set_low_power_state(dev, PCI_D3hot, locked);
> 
> 		if (pci_platform_power_transition(dev, PCI_D3cold))
> 			return error;
> 

This is applicable only to pci_set_power_state(), but AFAIK PCIe spec doesn't
mandate switching to D3Hot for entering D3Cold. So the PCIe host controller
drivers (especically non-ACPI platforms) may just send PME_Turn_Off followed by
removing the slot power (which again is not controlled by pci_set_power_state())
as there are no non-ACPI related hooks as of now.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

[Lukas Wunner]

On Tue, Aug 06, 2024 at 06:11:07PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Aug 06, 2024 at 08:53:01AM +0200, Lukas Wunner wrote:
> > AFAICS we always program the device to go to D3hot and the platform
> > then cuts power, thereby putting it into D3cold.  So D3hot is never
> > skipped.  See __pci_set_power_state():
> > 
> > 	if (state == PCI_D3cold) {
> > 		/*
> > 		 * To put the device in D3cold, put it into D3hot in the native
> > 		 * way, then put it into D3cold using platform ops.
> > 		 */
> > 		error = pci_set_low_power_state(dev, PCI_D3hot, locked);
> > 
> > 		if (pci_platform_power_transition(dev, PCI_D3cold))
> > 			return error;
> > 
> 
> This is applicable only to pci_set_power_state(), but AFAIK PCIe spec
> doesn't mandate switching to D3Hot for entering D3Cold.

Per PCI Bus Power Management Interface Specification r1.2 sec 5.5 fig 5-1,
the only supported state transition to D3cold is from D3hot.

Per PCIe r6.2 sec 5.2, "PM is compatible with the PCI Bus Power Management
Interface Specification".

Granted, PCI-PM is an ancient spec, so I think anyone can be forgiven
for not knowing its intricacies off-the-cuff. :)


> So the PCIe host controller drivers (especically non-ACPI platforms)
> may just send PME_Turn_Off followed by removing the slot power
> (which again is not controlled by pci_set_power_state())
> as there are no non-ACPI related hooks as of now.

Ideally, devicetree-based platforms should be brought into the
platform_pci_*() fold to align them with ACPI and get common
behavior across all platforms.

Thanks,

Lukas

Re: [PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

[Manivannan Sadhasivam]

On Tue, Aug 06, 2024 at 03:02:39PM +0200, Lukas Wunner wrote:
> On Tue, Aug 06, 2024 at 06:11:07PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Aug 06, 2024 at 08:53:01AM +0200, Lukas Wunner wrote:
> > > AFAICS we always program the device to go to D3hot and the platform
> > > then cuts power, thereby putting it into D3cold.  So D3hot is never
> > > skipped.  See __pci_set_power_state():
> > > 
> > > 	if (state == PCI_D3cold) {
> > > 		/*
> > > 		 * To put the device in D3cold, put it into D3hot in the native
> > > 		 * way, then put it into D3cold using platform ops.
> > > 		 */
> > > 		error = pci_set_low_power_state(dev, PCI_D3hot, locked);
> > > 
> > > 		if (pci_platform_power_transition(dev, PCI_D3cold))
> > > 			return error;
> > > 
> > 
> > This is applicable only to pci_set_power_state(), but AFAIK PCIe spec
> > doesn't mandate switching to D3Hot for entering D3Cold.
> 
> Per PCI Bus Power Management Interface Specification r1.2 sec 5.5 fig 5-1,
> the only supported state transition to D3cold is from D3hot.
> 
> Per PCIe r6.2 sec 5.2, "PM is compatible with the PCI Bus Power Management
> Interface Specification".
> 
> Granted, PCI-PM is an ancient spec, so I think anyone can be forgiven
> for not knowing its intricacies off-the-cuff. :)
> 

Ah, the grand old PCI-PM... I don't remember the last time I looked into it :)

> 
> > So the PCIe host controller drivers (especically non-ACPI platforms)
> > may just send PME_Turn_Off followed by removing the slot power
> > (which again is not controlled by pci_set_power_state())
> > as there are no non-ACPI related hooks as of now.
> 
> Ideally, devicetree-based platforms should be brought into the
> platform_pci_*() fold to align them with ACPI and get common
> behavior across all platforms.
> 

Yeah, that would be the ideal case. Unfortunately, there is no ideal ground for
DT :/ We do not even have the supplies populated properly. But with the advent
of power sequencing framework, I think this can be fixed.

Regarding your comment on patch 3/4, we already have the sysfs attribute to
control whether the device can be put into D3Cold or not and that is directly
coming from userspace. So there is no guarantee to assume that D3Hot support is
considered.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

[Lukas Wunner]

On Tue, Aug 06, 2024 at 08:09:18PM +0530, Manivannan Sadhasivam wrote:
> Regarding your comment on patch 3/4, we already have the sysfs attribute
> to control whether the device can be put into D3Cold or not and that is
> directly coming from userspace. So there is no guarantee to assume that
> D3Hot support is considered.

If a device is not allowed to be suspended to D3cold, it will only be
suspended to D3hot.

If a port is put into anything deeper than D0, its secondary bus is
no longer in B0 (PCI-PM r1.2 table 6-1) and children are inaccessible,
so they're "effectively" in D3cold.  Hence if a device cannot be
suspended to D3cold, it will block all parent bridges from being
suspended.  That's what the logic in pci_bridge_d3_update() and
pci_dev_check_d3cold() is doing.

The d3cold_allowed attribute in sysfs is just one of several reasons
why a device may not go to D3cold (see pci_dev_check_d3cold() for
details).

The d3cold_allowed attribute was originally intended to disable D3cold
on devices where it was known to not work.  Nowadays this should all
be handled automatically, which is why we've discussed moving the
attribute to debugfs:

https://lore.kernel.org/all/20230918132424.GA11357@wunner.de/
https://lore.kernel.org/all/20231002181025.82746-1-mario.limonciello@amd.com/

Thanks,

Lukas

Re: [PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

[Hsin-Yi Wang]

On Tue, Aug 6, 2024 at 7:39 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Aug 06, 2024 at 03:02:39PM +0200, Lukas Wunner wrote:
> > On Tue, Aug 06, 2024 at 06:11:07PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Aug 06, 2024 at 08:53:01AM +0200, Lukas Wunner wrote:
> > > > AFAICS we always program the device to go to D3hot and the platform
> > > > then cuts power, thereby putting it into D3cold.  So D3hot is never
> > > > skipped.  See __pci_set_power_state():
> > > >
> > > >   if (state == PCI_D3cold) {
> > > >           /*
> > > >            * To put the device in D3cold, put it into D3hot in the native
> > > >            * way, then put it into D3cold using platform ops.
> > > >            */
> > > >           error = pci_set_low_power_state(dev, PCI_D3hot, locked);
> > > >
> > > >           if (pci_platform_power_transition(dev, PCI_D3cold))
> > > >                   return error;
> > > >
> > >
> > > This is applicable only to pci_set_power_state(), but AFAIK PCIe spec
> > > doesn't mandate switching to D3Hot for entering D3Cold.
> >
> > Per PCI Bus Power Management Interface Specification r1.2 sec 5.5 fig 5-1,
> > the only supported state transition to D3cold is from D3hot.
> >
> > Per PCIe r6.2 sec 5.2, "PM is compatible with the PCI Bus Power Management
> > Interface Specification".
> >
> > Granted, PCI-PM is an ancient spec, so I think anyone can be forgiven
> > for not knowing its intricacies off-the-cuff. :)
> >
>
> Ah, the grand old PCI-PM... I don't remember the last time I looked into it :)
>
> >
> > > So the PCIe host controller drivers (especically non-ACPI platforms)
> > > may just send PME_Turn_Off followed by removing the slot power
> > > (which again is not controlled by pci_set_power_state())
> > > as there are no non-ACPI related hooks as of now.
> >
> > Ideally, devicetree-based platforms should be brought into the
> > platform_pci_*() fold to align them with ACPI and get common
> > behavior across all platforms.
> >
>
> Yeah, that would be the ideal case. Unfortunately, there is no ideal ground for
> DT :/ We do not even have the supplies populated properly. But with the advent
> of power sequencing framework, I think this can be fixed.
>

Looking in acpi_pci_bridge_d3(), it has several checkings about
whether d3 is supported, including reading power_manageable flag
(acpi_device_power_manageable) and reading the root port property.
For DT, does it make sense to have a chosen property about this?

> Regarding your comment on patch 3/4, we already have the sysfs attribute to
> control whether the device can be put into D3Cold or not and that is directly
> coming from userspace. So there is no guarantee to assume that D3Hot support is
> considered.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Oliver Neukum]

On 02.08.24 07:55, Manivannan Sadhasivam via B4 Relay wrote:

> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1434,7 +1434,7 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
>   	 * reason is that the bridge may have additional methods such as
>   	 * _DSW that need to be called.
>   	 */
> -	if (pci_dev->bridge_d3_allowed)
> +	if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)

Are you sure you want to require both capabilities here?

	Regards
		Oliver

Re: [PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

[Manivannan Sadhasivam]

On Tue, Aug 06, 2024 at 10:20:39PM +0200, Lukas Wunner wrote:
> On Tue, Aug 06, 2024 at 08:09:18PM +0530, Manivannan Sadhasivam wrote:
> > Regarding your comment on patch 3/4, we already have the sysfs attribute
> > to control whether the device can be put into D3Cold or not and that is
> > directly coming from userspace. So there is no guarantee to assume that
> > D3Hot support is considered.
> 
> If a device is not allowed to be suspended to D3cold, it will only be
> suspended to D3hot.
> 
> If a port is put into anything deeper than D0, its secondary bus is
> no longer in B0 (PCI-PM r1.2 table 6-1) and children are inaccessible,
> so they're "effectively" in D3cold.  Hence if a device cannot be
> suspended to D3cold, it will block all parent bridges from being
> suspended.  That's what the logic in pci_bridge_d3_update() and
> pci_dev_check_d3cold() is doing.
> 

Agree.

But patch 3/4 is mostly based on the suggestion from Bjorn [1] for earlier
revision. He specifically mentioned that the platform_pci_bridge_d3() function
doesn't differentiate between D3Hot and D3Cold and that's why I splitted them:

"These are two vastly different scenarios, and I would really like to
untangle them so they aren't conflated.  I see that you're extending
platform_pci_bridge_d3(), which apparently has that conflation baked
into it already, but my personal experience is that this is really
hard to maintain."

I agree with your point that if D3Hot is not possible, then D3Cold is also not
possible as per the PCI PM reference you quoted. But here, D3Hot is possible,
but D3Cold is not. And platform_pci_power_manageable(),
platform_pci_choose_state() are already returning false for DT platforms.

So if 'true' is returned from platform_pci_bridge_d3(), then it implies that
D3Cold is also supported, but it doesn't on DT platforms. So a split seems to be
necessary IMO.

- Mani

[1] https://lore.kernel.org/linux-pci/20240221182000.GA1533634@bhelgaas/

> The d3cold_allowed attribute in sysfs is just one of several reasons
> why a device may not go to D3cold (see pci_dev_check_d3cold() for
> details).
> 
> The d3cold_allowed attribute was originally intended to disable D3cold
> on devices where it was known to not work.  Nowadays this should all
> be handled automatically, which is why we've discussed moving the
> attribute to debugfs:
> 
> https://lore.kernel.org/all/20230918132424.GA11357@wunner.de/
> https://lore.kernel.org/all/20231002181025.82746-1-mario.limonciello@amd.com/
> 
> Thanks,
> 
> Lukas

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Manivannan Sadhasivam]

On Mon, Aug 19, 2024 at 02:44:43PM +0200, Oliver Neukum wrote:
> On 02.08.24 07:55, Manivannan Sadhasivam via B4 Relay wrote:
> 
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -1434,7 +1434,7 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
> >   	 * reason is that the bridge may have additional methods such as
> >   	 * _DSW that need to be called.
> >   	 */
> > -	if (pci_dev->bridge_d3_allowed)
> > +	if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)
> 
> Are you sure you want to require both capabilities here?
> 

Wakeup is common for both D3Hot and D3Cold, isn't it?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Bjorn Helgaas]

On Tue, Aug 20, 2024 at 11:30:08AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Aug 19, 2024 at 02:44:43PM +0200, Oliver Neukum wrote:
> > On 02.08.24 07:55, Manivannan Sadhasivam via B4 Relay wrote:
> > 
> > > --- a/drivers/pci/pci-acpi.c
> > > +++ b/drivers/pci/pci-acpi.c
> > > @@ -1434,7 +1434,7 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
> > >   	 * reason is that the bridge may have additional methods such as
> > >   	 * _DSW that need to be called.
> > >   	 */
> > > -	if (pci_dev->bridge_d3_allowed)
> > > +	if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)
> > 
> > Are you sure you want to require both capabilities here?
> 
> Wakeup is common for both D3Hot and D3Cold, isn't it?

From a spec point of view, moving device from D3hot to D0 is a config
space write that the OS knows how to do, but moving a device from
D3cold to D0 requires some platform-specific magic.  If that's what
you mean by wakeup, they don't look common to me.

Bjorn

Re: [PATCH v5 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Bjorn Helgaas]

On Fri, Aug 02, 2024 at 11:25:02AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Currently, there is no proper distinction between D3Hot and D3Cold while
> handling the power management for PCI bridges. For instance,
> pci_bridge_d3_allowed() API decides whether it is allowed to put the
> bridge in D3, but it doesn't explicitly specify whether D3Hot or D3Cold
> is allowed in a scenario. This often leads to confusion and may be prone
> to errors.
> 
> So let's split the D3Hot and D3Cold handling where possible. The current
> pci_bridge_d3_allowed() API is now split into pci_bridge_d3hot_allowed()
> and pci_bridge_d3cold_allowed() APIs and used in relevant places.

s/So let's split/Split/

> Also, pci_bridge_d3_update() API is now renamed to
> pci_bridge_d3cold_update() since it was only used to check the possibility
> of D3Cold.
> 
> Note that it is assumed that only D3Hot needs to be checked while
> transitioning the bridge during runtime PM and D3Cold in other places. In
> the ACPI case, wakeup is now only enabled if both D3Hot and D3Cold are
> allowed for the bridge.
> 
> Still, there are places where just 'd3' is used opaquely, but those are
> hard to distinguish, hence left for future cleanups.

The spec does use "D3Hot/D3Cold" (with Hot/Cold capitalized and
subscripted), but most Linux doc and comments use "D3hot" and
"D3cold", so I think we should stick with the Linux convention (it's
not 100%, but it's a pretty big majority).

> -	if (pci_dev->bridge_d3_allowed)
> +	if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)

Much of this patch is renames that could be easily reviewed.  But
there are a few things like this that are not simple renames.  Can you
split out these non-rename things to their own patch(es) with their
own explanations?

Bjorn

Re: [PATCH v5 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Manivannan Sadhasivam]

On Tue, Aug 20, 2024 at 08:45:59PM -0500, Bjorn Helgaas wrote:
> On Fri, Aug 02, 2024 at 11:25:02AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > Currently, there is no proper distinction between D3Hot and D3Cold while
> > handling the power management for PCI bridges. For instance,
> > pci_bridge_d3_allowed() API decides whether it is allowed to put the
> > bridge in D3, but it doesn't explicitly specify whether D3Hot or D3Cold
> > is allowed in a scenario. This often leads to confusion and may be prone
> > to errors.
> > 
> > So let's split the D3Hot and D3Cold handling where possible. The current
> > pci_bridge_d3_allowed() API is now split into pci_bridge_d3hot_allowed()
> > and pci_bridge_d3cold_allowed() APIs and used in relevant places.
> 
> s/So let's split/Split/
> 
> > Also, pci_bridge_d3_update() API is now renamed to
> > pci_bridge_d3cold_update() since it was only used to check the possibility
> > of D3Cold.
> > 
> > Note that it is assumed that only D3Hot needs to be checked while
> > transitioning the bridge during runtime PM and D3Cold in other places. In
> > the ACPI case, wakeup is now only enabled if both D3Hot and D3Cold are
> > allowed for the bridge.
> > 
> > Still, there are places where just 'd3' is used opaquely, but those are
> > hard to distinguish, hence left for future cleanups.
> 
> The spec does use "D3Hot/D3Cold" (with Hot/Cold capitalized and
> subscripted), but most Linux doc and comments use "D3hot" and
> "D3cold", so I think we should stick with the Linux convention (it's
> not 100%, but it's a pretty big majority).
> 
> > -	if (pci_dev->bridge_d3_allowed)
> > +	if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)
> 
> Much of this patch is renames that could be easily reviewed.  But
> there are a few things like this that are not simple renames.  Can you
> split out these non-rename things to their own patch(es) with their
> own explanations?
> 

I can, but I do not want these cleanups/refactoring to delay merging the patch
4. Are you OK if I just send it standalone and work on the refactoring as a
separate series?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Bjorn Helgaas]

On Wed, Aug 28, 2024 at 09:22:17PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Aug 20, 2024 at 08:45:59PM -0500, Bjorn Helgaas wrote:
> > On Fri, Aug 02, 2024 at 11:25:02AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > 
> > > Currently, there is no proper distinction between D3Hot and D3Cold while
> > > handling the power management for PCI bridges. For instance,
> > > pci_bridge_d3_allowed() API decides whether it is allowed to put the
> > > bridge in D3, but it doesn't explicitly specify whether D3Hot or D3Cold
> > > is allowed in a scenario. This often leads to confusion and may be prone
> > > to errors.
> > > 
> > > So let's split the D3Hot and D3Cold handling where possible. The current
> > > pci_bridge_d3_allowed() API is now split into pci_bridge_d3hot_allowed()
> > > and pci_bridge_d3cold_allowed() APIs and used in relevant places.
> > 
> > s/So let's split/Split/
> > 
> > > Also, pci_bridge_d3_update() API is now renamed to
> > > pci_bridge_d3cold_update() since it was only used to check the possibility
> > > of D3Cold.
> > > 
> > > Note that it is assumed that only D3Hot needs to be checked while
> > > transitioning the bridge during runtime PM and D3Cold in other places. In
> > > the ACPI case, wakeup is now only enabled if both D3Hot and D3Cold are
> > > allowed for the bridge.
> > > 
> > > Still, there are places where just 'd3' is used opaquely, but those are
> > > hard to distinguish, hence left for future cleanups.
> > 
> > The spec does use "D3Hot/D3Cold" (with Hot/Cold capitalized and
> > subscripted), but most Linux doc and comments use "D3hot" and
> > "D3cold", so I think we should stick with the Linux convention (it's
> > not 100%, but it's a pretty big majority).
> > 
> > > -	if (pci_dev->bridge_d3_allowed)
> > > +	if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)
> > 
> > Much of this patch is renames that could be easily reviewed.  But
> > there are a few things like this that are not simple renames.  Can you
> > split out these non-rename things to their own patch(es) with their
> > own explanations?
> 
> I can, but I do not want these cleanups/refactoring to delay merging
> the patch 4. Are you OK if I just send it standalone and work on the
> refactoring as a separate series?

You mean to send patch 4/4 standalone, and do the rest separately?
That sounds reasonable to me.

Re: [PATCH v5 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Manivannan Sadhasivam]

On Wed, Aug 28, 2024 at 04:07:05PM -0500, Bjorn Helgaas wrote:
> On Wed, Aug 28, 2024 at 09:22:17PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Aug 20, 2024 at 08:45:59PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Aug 02, 2024 at 11:25:02AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > 
> > > > Currently, there is no proper distinction between D3Hot and D3Cold while
> > > > handling the power management for PCI bridges. For instance,
> > > > pci_bridge_d3_allowed() API decides whether it is allowed to put the
> > > > bridge in D3, but it doesn't explicitly specify whether D3Hot or D3Cold
> > > > is allowed in a scenario. This often leads to confusion and may be prone
> > > > to errors.
> > > > 
> > > > So let's split the D3Hot and D3Cold handling where possible. The current
> > > > pci_bridge_d3_allowed() API is now split into pci_bridge_d3hot_allowed()
> > > > and pci_bridge_d3cold_allowed() APIs and used in relevant places.
> > > 
> > > s/So let's split/Split/
> > > 
> > > > Also, pci_bridge_d3_update() API is now renamed to
> > > > pci_bridge_d3cold_update() since it was only used to check the possibility
> > > > of D3Cold.
> > > > 
> > > > Note that it is assumed that only D3Hot needs to be checked while
> > > > transitioning the bridge during runtime PM and D3Cold in other places. In
> > > > the ACPI case, wakeup is now only enabled if both D3Hot and D3Cold are
> > > > allowed for the bridge.
> > > > 
> > > > Still, there are places where just 'd3' is used opaquely, but those are
> > > > hard to distinguish, hence left for future cleanups.
> > > 
> > > The spec does use "D3Hot/D3Cold" (with Hot/Cold capitalized and
> > > subscripted), but most Linux doc and comments use "D3hot" and
> > > "D3cold", so I think we should stick with the Linux convention (it's
> > > not 100%, but it's a pretty big majority).
> > > 
> > > > -	if (pci_dev->bridge_d3_allowed)
> > > > +	if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)
> > > 
> > > Much of this patch is renames that could be easily reviewed.  But
> > > there are a few things like this that are not simple renames.  Can you
> > > split out these non-rename things to their own patch(es) with their
> > > own explanations?
> > 
> > I can, but I do not want these cleanups/refactoring to delay merging
> > the patch 4. Are you OK if I just send it standalone and work on the
> > refactoring as a separate series?
> 
> You mean to send patch 4/4 standalone, and do the rest separately?
> That sounds reasonable to me.

Ack, thanks.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Manivannan Sadhasivam]

On Tue, Aug 20, 2024 at 06:45:04PM -0500, Bjorn Helgaas wrote:
> On Tue, Aug 20, 2024 at 11:30:08AM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Aug 19, 2024 at 02:44:43PM +0200, Oliver Neukum wrote:
> > > On 02.08.24 07:55, Manivannan Sadhasivam via B4 Relay wrote:
> > > 
> > > > --- a/drivers/pci/pci-acpi.c
> > > > +++ b/drivers/pci/pci-acpi.c
> > > > @@ -1434,7 +1434,7 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
> > > >   	 * reason is that the bridge may have additional methods such as
> > > >   	 * _DSW that need to be called.
> > > >   	 */
> > > > -	if (pci_dev->bridge_d3_allowed)
> > > > +	if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)
> > > 
> > > Are you sure you want to require both capabilities here?
> > 
> > Wakeup is common for both D3Hot and D3Cold, isn't it?
> 
> From a spec point of view, moving device from D3hot to D0 is a config
> space write that the OS knows how to do, but moving a device from
> D3cold to D0 requires some platform-specific magic.  If that's what
> you mean by wakeup, they don't look common to me.
> 

I agree that the wakeup mechanism differs between D3hot and D3cold, but I'm not
sure about enabling the wakeup capability of the bridge if only one (D3hot or
D3cold) is allowed. So I went with the requirement of having both. Otherwise,
how can we differentiate wakeup from D3hot vs wakeup from D3cold?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

[Brian Norris]

Hi Manivannan,

Dredging up an old one, but it seems like there was almost consensus on
this patch, and yet it stalled because the series does too much. I'm
interested in reviving it, but I also have some thoughts on the
usability.

On Fri, Aug 02, 2024 at 11:25:03AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Unlike ACPI based platforms, there are no known issues with D3Hot for the
> PCI bridges in the Devicetree based platforms. So let's allow the PCI
> bridges to go to D3Hot during runtime. It should be noted that the bridges
> need to be defined in Devicetree for this to work.

IMO, it's not an amazing idea to key off the presence of a bridge DT
node for this. AFAIK, that's not really required for most other things
(especially if we're not mapping legacy INTx support), and many
platforms I work with do not define a bridge node. But they do use DT,
and I'd like to be able to suspend their bridges.

Personally, I'd choose to match the same requirements as used by
devm_pci_alloc_host_bridge() -> devm_of_pci_bridge_init() -- that the
parent device under which the host bridge is created has an of_node.
Code sample below.

> Currently, D3Cold is not allowed since Vcc supply which is required for
> transitioning the device to D3Cold is not exposed on all Devicetree based
> platforms.
> 
> Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/pci.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index c7a4f961ec28..bc1e1ca673f1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2992,6 +2992,18 @@ static bool pci_bridge_d3_allowed(struct pci_dev *bridge, pci_power_t state)
>  		if (pci_bridge_d3_force)
>  			return true;
>  
> +		/*
> +		 * Allow D3Hot for all Devicetree based platforms having a
> +		 * separate node for the bridge. We don't allow D3Cold for now
> +		 * since not all platforms are exposing the Vcc supply in
> +		 * Devicetree which is required for transitioning the bridge to
> +		 * D3Cold.
> +		 *
> +		 * NOTE: The bridge is expected to be defined in Devicetree.
> +		 */
> +		if (state == PCI_D3hot && dev_of_node(&bridge->dev))
> +			return true;
> +

For me, a way to lighten the bridge-node restriction is:

	struct pci_host_bridge *host_bridge;

	...
		/*
		 * Allow D3 for all Device Tree based systems. We check
		 * if our host bridge's parent has a Device Tree node.
		 * None of the D3 restrictions that applied to old BIOS
		 * systems are known to apply to DT systems.
		 */
		host_bridge = pci_find_host_bridge(bridge->bus);
		if (dev_of_node(host_bridge->dev.parent))
			return true;

Brian

>  		/* Even the oldest 2010 Thunderbolt controller supports D3. */
>  		if (bridge->is_thunderbolt)
>  			return true;
> @@ -3042,7 +3054,7 @@ bool pci_bridge_d3cold_allowed(struct pci_dev *bridge)
>   *
>   * This function checks if the bridge is allowed to move to D3Hot.
>   * Currently we only allow D3Hot for recent enough PCIe ports on ACPI based
> - * platforms and Thunderbolt.
> + * platforms, Thunderbolt and Devicetree based platforms.
>   */
>  bool pci_bridge_d3hot_allowed(struct pci_dev *bridge)
>  {
> 
> -- 
> 2.25.1
> 
> 

Re: [PATCH v5 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Brian Norris]

Hi Manivannan,

On Thu, Aug 29, 2024 at 10:52:44AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Aug 28, 2024 at 04:07:05PM -0500, Bjorn Helgaas wrote:
> > On Wed, Aug 28, 2024 at 09:22:17PM +0530, Manivannan Sadhasivam wrote:
> > > I can, but I do not want these cleanups/refactoring to delay merging
> > > the patch 4. Are you OK if I just send it standalone and work on the
> > > refactoring as a separate series?
> > 
> > You mean to send patch 4/4 standalone, and do the rest separately?
> > That sounds reasonable to me.
> 
> Ack, thanks.

Did this ever happen? I ask, because I'm also interested in supporting
D3hot for bridges on device tree systems, and it seems like there's
pretty clear agreement that pci_bridge_d3_possible() should not prevent
it.

If there isn't an updated posting and plan to merge yet, would you mind
if I submitted one myself, carrying links to your work for
back-reference?

Also, I had some questions on patch 4, as I think it places too much
restriction on how the DT should look. So I might tweak it a bit if I
send a new revision.

Brian