[PATCH 0/6] PCI: Add support for suspending (including runtime) of PCIe ports

[PATCH 0/6] PCI: Add support for suspending (including runtime) of PCIe ports

[Mika Westerberg]

Hi,

This series add support for moving PCIe ports to D3hot both runtime and
during system suspend.

When PCIe port is suspended connected devices are effectively moved to
D3cold because their config space is not accessible anymore (and the link
may be powered down as well). So we need to make sure the devices are
allowed to go to D3cold and, if they are wake capable, can do so from
D3cold. Only then we can suspend the port.

Patch [3/6] adds support for system suspend of PCIe ports. Since we do not
know how well this has been supported in older hardware, enable this only
for quite recent hardware (starting from 2015). This can still be
disallowed per port as needed by adding entries to pcie_port_configs[].

Patch [4/6] then extends this to allow runtime PM of PCIe ports whenever
all connected devices are runtime suspended. Patches [5,6/6] enable runtime
PM for recent Intel hardware including Sunrisepoint (Skylake) and Broxton.
These are meant to be alternative for patches by Qipeng Zha here:

  http://www.spinics.net/lists/linux-pci/msg48103.html
  http://www.spinics.net/lists/linux-pci/msg48104.html

The idea how this is supposed to be implemented came from Rafael Wysocki.

Mika Westerberg (6):
  PCI: No need to set d3cold_allowed to PCIe ports
  PCI: Make __pci_bus_set_current_state() available to other files
  PCI: Move PCIe ports to D3hot during suspend
  PCI: Add runtime PM support for PCIe ports
  PCI: Enable runtime PM for Intel Sunrisepoint PCIe root ports
  PCI: Enable runtime PM for Intel Broxton PCIe root ports

 drivers/pci/pci.c              |   2 +-
 drivers/pci/pci.h              |   1 +
 drivers/pci/pcie/portdrv_pci.c | 170 +++++++++++++++++++++++++++++++++++++++--
 3 files changed, 164 insertions(+), 9 deletions(-)

-- 
2.7.0

[PATCH 1/6] PCI: No need to set d3cold_allowed to PCIe ports

[Mika Westerberg]

PCIe ports are already skipped by the PCI core so they are never moved to
D3 (or D3cold) anyway. No need to set the field.

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

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index be35da2e105e..6c6bb03392ea 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -134,11 +134,6 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 		return status;
 
 	pci_save_state(dev);
-	/*
-	 * D3cold may not work properly on some PCIe port, so disable
-	 * it by default.
-	 */
-	dev->d3cold_allowed = false;
 	return 0;
 }
 
-- 
2.7.0

[PATCH 2/6] PCI: Make __pci_bus_set_current_state() available to other files

[Mika Westerberg]

We are going to take advantage of this function in PCIe root port driver so
make it available for other files inside drivers/pci/* directories.

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

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 602eb4223510..e450b20ed4f3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -811,7 +811,7 @@ static int __pci_dev_set_current_state(struct pci_dev *dev, void *data)
  * @bus: Top bus of the subtree to walk.
  * @state: state to be set
  */
-static void __pci_bus_set_current_state(struct pci_bus *bus, pci_power_t state)
+void __pci_bus_set_current_state(struct pci_bus *bus, pci_power_t state)
 {
 	if (bus)
 		pci_walk_bus(bus, __pci_dev_set_current_state, &state);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9a1660f592ef..e021da7ad0ff 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -238,6 +238,7 @@ void __pci_bus_assign_resources(const struct pci_bus *bus,
 				struct list_head *realloc_head,
 				struct list_head *fail_head);
 bool pci_bus_clip_resource(struct pci_dev *dev, int idx);
+void __pci_bus_set_current_state(struct pci_bus *bus, pci_power_t state);
 
 void pci_reassigndev_resource_alignment(struct pci_dev *dev);
 void pci_disable_bridge_window(struct pci_dev *dev);
-- 
2.7.0

[PATCH 3/6] PCI: Move PCIe ports to D3hot during suspend

[Mika Westerberg]

Currently the PCI core does not do this automatically as it avoids changing
power state for bridges and PCIe ports. With recent hardware PCIe ports can
be moved to D3hot given that we take into account few restrictions:

  - Devices connected to the ports are effectively in D3cold once the root
    port is moved to D3hot (the config space is not accessible anymore and
    the link may be powered down).

  - The device needs to be able to transition to D3cold.

  - If the device is capable of waking the system it needs to be able to
    do so from D3cold (PME from D3cold).

We assume all recent hardware (starting from 2015) is capable of doing this
but make it possible to add exceptions via entries in pcie_port_configs[].

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

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 6c6bb03392ea..fe3349685141 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -20,6 +20,7 @@
 
 #include "portdrv.h"
 #include "aer/aerdrv.h"
+#include "../pci.h"
 
 /*
  * Version Information
@@ -78,11 +79,105 @@ static int pcie_portdrv_restore_config(struct pci_dev *dev)
 	return 0;
 }
 
+enum pcie_port_type {
+	PCIE_PORT_DEFAULT,
+};
+
+struct pcie_port_config {
+	bool suspend_allowed;
+};
+
+static const struct pcie_port_config pcie_port_configs[] = {
+	[PCIE_PORT_DEFAULT] = {
+		.suspend_allowed = true,
+	},
+};
+
 #ifdef CONFIG_PM
+static const struct pcie_port_config *pcie_port_get_config(struct pci_dev *pdev)
+{
+	const struct pci_device_id *id = pci_match_id(pdev->driver->id_table,
+						      pdev);
+	return &pcie_port_configs[id->driver_data];
+}
+
+static int pcie_port_check_d3cold(struct pci_dev *pdev, void *data)
+{
+	bool *d3cold_ok = data;
+
+	if (pdev->no_d3cold || !pdev->d3cold_allowed)
+		*d3cold_ok = false;
+	if (device_may_wakeup(&pdev->dev) && !pci_pme_capable(pdev, PCI_D3cold))
+		*d3cold_ok = false;
+
+	return !*d3cold_ok;
+}
+
+static bool pcie_port_can_suspend(struct pci_dev *pdev)
+{
+	bool d3cold_ok = true;
+
+	/*
+	 * When the port is put to D3hot the devices behind the port are
+	 * effectively in D3cold as their config space cannot be accessed
+	 * anymore and the link may be powered down.
+	 *
+	 * We only allow the port to go to D3hot the devices:
+	 *  - Are allowed to go to D3cold
+	 *  - Can wake up from D3cold if they are wake capable
+	 */
+	pci_walk_bus(pdev->subordinate, pcie_port_check_d3cold, &d3cold_ok);
+	return d3cold_ok;
+}
+
+static bool pcie_port_suspend_allowed(struct pci_dev *pdev)
+{
+	const struct pcie_port_config *config = pcie_port_get_config(pdev);
+
+	/*
+	 * Older hardware is not capable of moving PCIe ports to D3 so
+	 * anything earlier than 2015 is assumed not to support this.
+	 */
+	if (dmi_available) {
+		unsigned year;
+
+		if (!dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL) ||
+		    year < 2015) {
+			return false;
+		}
+	}
+
+	/* Per port configuration can forbid it as well */
+	if (!config->suspend_allowed)
+		return false;
+
+	return pcie_port_can_suspend(pdev);
+}
+
+static int pcie_port_suspend_noirq(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (pcie_port_suspend_allowed(pdev)) {
+		pci_save_state(pdev);
+		pci_set_power_state(pdev, PCI_D3hot);
+		/*
+		 * All devices behind the port are assumed to be in D3cold
+		 * so update their state now.
+		 */
+		__pci_bus_set_current_state(pdev->subordinate, PCI_D3cold);
+	}
+
+	return 0;
+}
+
 static int pcie_port_resume_noirq(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+
 	/*
 	 * Some BIOSes forget to clear Root PME Status bits after system wakeup
 	 * which breaks ACPI-based runtime wakeup on PCI Express, so clear those
@@ -100,6 +195,7 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.thaw		= pcie_port_device_resume,
 	.poweroff	= pcie_port_device_suspend,
 	.restore	= pcie_port_device_resume,
+	.suspend_noirq	= pcie_port_suspend_noirq,
 	.resume_noirq	= pcie_port_resume_noirq,
 };
 
@@ -285,10 +381,11 @@ static void pcie_portdrv_err_resume(struct pci_dev *dev)
 /*
  * LINUX Device Driver Model
  */
-static const struct pci_device_id port_pci_ids[] = { {
+static const struct pci_device_id port_pci_ids[] = {
 	/* handle any PCI-Express port */
-	PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0),
-	}, { /* end: all zeroes */ }
+	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0),
+	  .driver_data = PCIE_PORT_DEFAULT },
+	{ /* end: all zeroes */ }
 };
 MODULE_DEVICE_TABLE(pci, port_pci_ids);
 
-- 
2.7.0

[PATCH 4/6] PCI: Add runtime PM support for PCIe ports

[Mika Westerberg]

Add back runtime PM support for PCIe ports that was removed in
commit fe9a743a2601 ("PCI/PM: Drop unused runtime PM support code for PCIe
ports").

First of all we cannot enable it automatically for all root ports since
there have been problems previously, so we enable it only based on per port
configuration (runtime_suspend_allowed). Furthermore we need to check that
the device, if wake capable, can do so from D3cold (which is the state it
goes after the root port is powered down). This follows what we did for
PCIe port system suspend already.

Runtime PM is still blocked and needs to be unblocked from userspace like
with other PCI devices.

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

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index fe3349685141..eb0f425f51cc 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -85,6 +85,7 @@ enum pcie_port_type {
 
 struct pcie_port_config {
 	bool suspend_allowed;
+	bool runtime_suspend_allowed;
 };
 
 static const struct pcie_port_config pcie_port_configs[] = {
@@ -154,6 +155,11 @@ static bool pcie_port_suspend_allowed(struct pci_dev *pdev)
 	return pcie_port_can_suspend(pdev);
 }
 
+static bool pcie_port_runtime_suspend_allowed(struct pci_dev *pdev)
+{
+	return pcie_port_get_config(pdev)->runtime_suspend_allowed;
+}
+
 static int pcie_port_suspend_noirq(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -188,6 +194,34 @@ static int pcie_port_resume_noirq(struct device *dev)
 	return 0;
 }
 
+static int pcie_port_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	/*
+	 * All devices behind the port are assumed to be in D3cold so
+	 * update their state now.
+	 */
+	__pci_bus_set_current_state(pdev->subordinate, PCI_D3cold);
+	return 0;
+}
+
+static int pcie_port_runtime_resume(struct device *dev)
+{
+	return 0;
+}
+
+static int pcie_port_runtime_idle(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (pcie_port_can_suspend(pdev)) {
+		pm_schedule_suspend(dev, 10);
+		return 0;
+	}
+	return -EBUSY;
+}
+
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.suspend	= pcie_port_device_suspend,
 	.resume		= pcie_port_device_resume,
@@ -197,12 +231,20 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.restore	= pcie_port_device_resume,
 	.suspend_noirq	= pcie_port_suspend_noirq,
 	.resume_noirq	= pcie_port_resume_noirq,
+	.runtime_suspend = pcie_port_runtime_suspend,
+	.runtime_resume	= pcie_port_runtime_resume,
+	.runtime_idle	= pcie_port_runtime_idle,
 };
 
 #define PCIE_PORTDRV_PM_OPS	(&pcie_portdrv_pm_ops)
 
 #else /* !PM */
 
+static inline bool pcie_port_runtime_suspend_allowed(struct pci_dev *pdev)
+{
+	return false;
+}
+
 #define PCIE_PORTDRV_PM_OPS	NULL
 #endif /* !PM */
 
@@ -230,11 +272,18 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 		return status;
 
 	pci_save_state(dev);
+
+	if (pcie_port_runtime_suspend_allowed(dev))
+		pm_runtime_put_noidle(&dev->dev);
+
 	return 0;
 }
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
+	if (pcie_port_runtime_suspend_allowed(dev))
+		pm_runtime_get_noresume(&dev->dev);
+
 	pcie_port_device_remove(dev);
 }
 
-- 
2.7.0

[PATCH 5/6] PCI: Enable runtime PM for Intel Sunrisepoint PCIe root ports

[Mika Westerberg]

Intel Sunrisepoint (Skylake PCH) PCIe root ports are capable of being
suspended runtime so allow runtime PM for these ports.

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

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index eb0f425f51cc..864c1bdd0e91 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -81,6 +81,7 @@ static int pcie_portdrv_restore_config(struct pci_dev *dev)
 
 enum pcie_port_type {
 	PCIE_PORT_DEFAULT,
+	PCIE_PORT_SPT,
 };
 
 struct pcie_port_config {
@@ -92,6 +93,10 @@ static const struct pcie_port_config pcie_port_configs[] = {
 	[PCIE_PORT_DEFAULT] = {
 		.suspend_allowed = true,
 	},
+	[PCIE_PORT_SPT] = {
+		.suspend_allowed = true,
+		.runtime_suspend_allowed = true,
+	},
 };
 
 #ifdef CONFIG_PM
@@ -431,6 +436,9 @@ static void pcie_portdrv_err_resume(struct pci_dev *dev)
  * LINUX Device Driver Model
  */
 static const struct pci_device_id port_pci_ids[] = {
+	/* Intel Sunrisepoint */
+	{ PCI_VDEVICE(INTEL, 0x9d14), .driver_data = PCIE_PORT_SPT },
+	{ PCI_VDEVICE(INTEL, 0x9d15), .driver_data = PCIE_PORT_SPT },
 	/* handle any PCI-Express port */
 	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0),
 	  .driver_data = PCIE_PORT_DEFAULT },
-- 
2.7.0

[PATCH 6/6] PCI: Enable runtime PM for Intel Broxton PCIe root ports

[Mika Westerberg]

Intel Broxton PCIe root ports are capable of being suspended runtime so
allow runtime PM for these ports.

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

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 864c1bdd0e91..43dd23e42c1d 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -436,6 +436,11 @@ static void pcie_portdrv_err_resume(struct pci_dev *dev)
  * LINUX Device Driver Model
  */
 static const struct pci_device_id port_pci_ids[] = {
+	/* Intel Broxton */
+	{ PCI_VDEVICE(INTEL, 0x1ad6), .driver_data = PCIE_PORT_SPT },
+	{ PCI_VDEVICE(INTEL, 0x1ad7), .driver_data = PCIE_PORT_SPT },
+	{ PCI_VDEVICE(INTEL, 0x5ad8), .driver_data = PCIE_PORT_SPT },
+	{ PCI_VDEVICE(INTEL, 0x5ada), .driver_data = PCIE_PORT_SPT },
 	/* Intel Sunrisepoint */
 	{ PCI_VDEVICE(INTEL, 0x9d14), .driver_data = PCIE_PORT_SPT },
 	{ PCI_VDEVICE(INTEL, 0x9d15), .driver_data = PCIE_PORT_SPT },
-- 
2.7.0

Re: [PATCH 1/6] PCI: No need to set d3cold_allowed to PCIe ports

[Bjorn Helgaas]

On Mon, Feb 29, 2016 at 02:56:01PM +0200, Mika Westerberg wrote:
> PCIe ports are already skipped by the PCI core so they are never moved to
> D3 (or D3cold) anyway. No need to set the field.

A pointer to where the PCI core skips these ports would be helpful.
pci_pm_runtime_suspend() is the only place I see that tests d3cold_allowed,
and I don't see that it checks anything about PCIe ports.

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pcie/portdrv_pci.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index be35da2e105e..6c6bb03392ea 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -134,11 +134,6 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  		return status;
>  
>  	pci_save_state(dev);
> -	/*
> -	 * D3cold may not work properly on some PCIe port, so disable
> -	 * it by default.
> -	 */
> -	dev->d3cold_allowed = false;
>  	return 0;
>  }
>  
> -- 
> 2.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] PCI: Move PCIe ports to D3hot during suspend

[Bjorn Helgaas]

On Mon, Feb 29, 2016 at 02:56:03PM +0200, Mika Westerberg wrote:
> Currently the PCI core does not do this automatically as it avoids changing
> power state for bridges and PCIe ports. With recent hardware PCIe ports can
> be moved to D3hot given that we take into account few restrictions:
> 
>   - Devices connected to the ports are effectively in D3cold once the root
>     port is moved to D3hot (the config space is not accessible anymore and
>     the link may be powered down).
> 
>   - The device needs to be able to transition to D3cold.

I think this is talking about "devices below the PCIe port", right?

>   - If the device is capable of waking the system it needs to be able to
>     do so from D3cold (PME from D3cold).

Again, "a device below the PCIe port"?

> We assume all recent hardware (starting from 2015) is capable of doing this
> but make it possible to add exceptions via entries in pcie_port_configs[].

Since you have no exceptions yet, I'm not sure it's worth having the
exception infrastructure.  That infrastructure kind of obscures the
meat of the patch, so if we really do need it right away, maybe it
could be added in a new separate patch.

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pcie/portdrv_pci.c | 103 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 100 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 6c6bb03392ea..fe3349685141 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -20,6 +20,7 @@
>  
>  #include "portdrv.h"
>  #include "aer/aerdrv.h"
> +#include "../pci.h"
>  
>  /*
>   * Version Information
> @@ -78,11 +79,105 @@ static int pcie_portdrv_restore_config(struct pci_dev *dev)
>  	return 0;
>  }
>  
> +enum pcie_port_type {
> +	PCIE_PORT_DEFAULT,
> +};
> +
> +struct pcie_port_config {
> +	bool suspend_allowed;
> +};
> +
> +static const struct pcie_port_config pcie_port_configs[] = {
> +	[PCIE_PORT_DEFAULT] = {
> +		.suspend_allowed = true,
> +	},
> +};
> +
>  #ifdef CONFIG_PM
> +static const struct pcie_port_config *pcie_port_get_config(struct pci_dev *pdev)
> +{
> +	const struct pci_device_id *id = pci_match_id(pdev->driver->id_table,
> +						      pdev);
> +	return &pcie_port_configs[id->driver_data];
> +}
> +
> +static int pcie_port_check_d3cold(struct pci_dev *pdev, void *data)
> +{
> +	bool *d3cold_ok = data;
> +
> +	if (pdev->no_d3cold || !pdev->d3cold_allowed)
> +		*d3cold_ok = false;
> +	if (device_may_wakeup(&pdev->dev) && !pci_pme_capable(pdev, PCI_D3cold))
> +		*d3cold_ok = false;
> +
> +	return !*d3cold_ok;
> +}
> +
> +static bool pcie_port_can_suspend(struct pci_dev *pdev)
> +{
> +	bool d3cold_ok = true;
> +
> +	/*
> +	 * When the port is put to D3hot the devices behind the port are
> +	 * effectively in D3cold as their config space cannot be accessed
> +	 * anymore and the link may be powered down.
> +	 *
> +	 * We only allow the port to go to D3hot the devices:

s/the devices/if the subordinate devices/

> +	 *  - Are allowed to go to D3cold
> +	 *  - Can wake up from D3cold if they are wake capable
> +	 */
> +	pci_walk_bus(pdev->subordinate, pcie_port_check_d3cold, &d3cold_ok);
> +	return d3cold_ok;

What happens if the PCIe port initially can be put in D3hot, but we we
later hot-add a device that would disqualify it?

Oh, I think I see -- we walk the subordinate devices every time we
would like to put the port in D3hot:

  pcie_port_suspend_noirq
    pcie_port_suspend_allowed
      dmi_get_date
      pcie_port_can_suspend
        pci_walk_bus

I guess that should work, but it seems clunky to do all that work,
even though it's not really a performance path.  What if we did this:

  - add a d3hot_allowed bit in struct pci_dev

  - when enumerating a port, set d3hot_allowed if BIOS is new enough
    (it makes me a bit nervous that we apparently default to enabling
    D3hot on arches without DMI)

  - when enumerating devices, clear d3hot_allowed in upstream bridge
    if necessary

  - when removing last device on a bus, re-do port config (set
    d3hot_allowed if appropriate)

> +}
> +
> +static bool pcie_port_suspend_allowed(struct pci_dev *pdev)
> +{
> +	const struct pcie_port_config *config = pcie_port_get_config(pdev);
> +
> +	/*
> +	 * Older hardware is not capable of moving PCIe ports to D3 so
> +	 * anything earlier than 2015 is assumed not to support this.
> +	 */
> +	if (dmi_available) {
> +		unsigned year;
> +
> +		if (!dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL) ||
> +		    year < 2015) {
> +			return false;
> +		}
> +	}
> +
> +	/* Per port configuration can forbid it as well */
> +	if (!config->suspend_allowed)
> +		return false;
> +
> +	return pcie_port_can_suspend(pdev);
> +}
> +
> +static int pcie_port_suspend_noirq(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (pcie_port_suspend_allowed(pdev)) {
> +		pci_save_state(pdev);
> +		pci_set_power_state(pdev, PCI_D3hot);
> +		/*
> +		 * All devices behind the port are assumed to be in D3cold
> +		 * so update their state now.
> +		 */
> +		__pci_bus_set_current_state(pdev->subordinate, PCI_D3cold);
> +	}
> +
> +	return 0;
> +}
> +
>  static int pcie_port_resume_noirq(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  
> +	pci_set_power_state(pdev, PCI_D0);
> +	pci_restore_state(pdev);
> +
>  	/*
>  	 * Some BIOSes forget to clear Root PME Status bits after system wakeup
>  	 * which breaks ACPI-based runtime wakeup on PCI Express, so clear those
> @@ -100,6 +195,7 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
>  	.thaw		= pcie_port_device_resume,
>  	.poweroff	= pcie_port_device_suspend,
>  	.restore	= pcie_port_device_resume,
> +	.suspend_noirq	= pcie_port_suspend_noirq,
>  	.resume_noirq	= pcie_port_resume_noirq,
>  };
>  
> @@ -285,10 +381,11 @@ static void pcie_portdrv_err_resume(struct pci_dev *dev)
>  /*
>   * LINUX Device Driver Model
>   */
> -static const struct pci_device_id port_pci_ids[] = { {
> +static const struct pci_device_id port_pci_ids[] = {
>  	/* handle any PCI-Express port */
> -	PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0),
> -	}, { /* end: all zeroes */ }
> +	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0),
> +	  .driver_data = PCIE_PORT_DEFAULT },
> +	{ /* end: all zeroes */ }
>  };
>  MODULE_DEVICE_TABLE(pci, port_pci_ids);
>  
> -- 
> 2.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] PCI: Add runtime PM support for PCIe ports

[Bjorn Helgaas]

On Mon, Feb 29, 2016 at 02:56:04PM +0200, Mika Westerberg wrote:
> Add back runtime PM support for PCIe ports that was removed in
> commit fe9a743a2601 ("PCI/PM: Drop unused runtime PM support code for PCIe
> ports").
> 
> First of all we cannot enable it automatically for all root ports since
> there have been problems previously, 

Pointers to these problems would be useful.

> so we enable it only based on per port
> configuration (runtime_suspend_allowed).

I see that the next patch basically adds a quirk to set
runtime_suspend_allowed for Sunrisepoint.  How do we prevent this
from being a maintenance nightmare?  I don't want to have to add
a quirk for every new device that supports runtime PM.

> Furthermore we need to check that
> the device, if wake capable, can do so from D3cold (which is the state it
> goes after the root port is powered down).

I don't see this check.  I was expecting something like
device_may_wakeup(&pdev->dev) && !pci_pme_capable(pdev, PCI_D3cold)
like you did in the "Move PCIe ports to D3hot during suspend" patch.

> This follows what we did for
> PCIe port system suspend already.
> 
> Runtime PM is still blocked and needs to be unblocked from userspace like
> with other PCI devices.

Can you expand on this a bit?  I assume you mean that runtime PM is
disabled by default, and userspace has to enable it with a sysfs switch or
something?  Can you include information about how to do that?

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pcie/portdrv_pci.c | 49 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index fe3349685141..eb0f425f51cc 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -85,6 +85,7 @@ enum pcie_port_type {
>  
>  struct pcie_port_config {
>  	bool suspend_allowed;
> +	bool runtime_suspend_allowed;
>  };
>  
>  static const struct pcie_port_config pcie_port_configs[] = {
> @@ -154,6 +155,11 @@ static bool pcie_port_suspend_allowed(struct pci_dev *pdev)
>  	return pcie_port_can_suspend(pdev);
>  }
>  
> +static bool pcie_port_runtime_suspend_allowed(struct pci_dev *pdev)
> +{
> +	return pcie_port_get_config(pdev)->runtime_suspend_allowed;
> +}
> +
>  static int pcie_port_suspend_noirq(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> @@ -188,6 +194,34 @@ static int pcie_port_resume_noirq(struct device *dev)
>  	return 0;
>  }
>  
> +static int pcie_port_runtime_suspend(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	/*
> +	 * All devices behind the port are assumed to be in D3cold so
> +	 * update their state now.
> +	 */
> +	__pci_bus_set_current_state(pdev->subordinate, PCI_D3cold);
> +	return 0;
> +}
> +
> +static int pcie_port_runtime_resume(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int pcie_port_runtime_idle(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (pcie_port_can_suspend(pdev)) {
> +		pm_schedule_suspend(dev, 10);
> +		return 0;
> +	}
> +	return -EBUSY;
> +}
> +
>  static const struct dev_pm_ops pcie_portdrv_pm_ops = {
>  	.suspend	= pcie_port_device_suspend,
>  	.resume		= pcie_port_device_resume,
> @@ -197,12 +231,20 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
>  	.restore	= pcie_port_device_resume,
>  	.suspend_noirq	= pcie_port_suspend_noirq,
>  	.resume_noirq	= pcie_port_resume_noirq,
> +	.runtime_suspend = pcie_port_runtime_suspend,
> +	.runtime_resume	= pcie_port_runtime_resume,
> +	.runtime_idle	= pcie_port_runtime_idle,
>  };
>  
>  #define PCIE_PORTDRV_PM_OPS	(&pcie_portdrv_pm_ops)
>  
>  #else /* !PM */
>  
> +static inline bool pcie_port_runtime_suspend_allowed(struct pci_dev *pdev)
> +{
> +	return false;
> +}
> +
>  #define PCIE_PORTDRV_PM_OPS	NULL
>  #endif /* !PM */
>  
> @@ -230,11 +272,18 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  		return status;
>  
>  	pci_save_state(dev);
> +
> +	if (pcie_port_runtime_suspend_allowed(dev))
> +		pm_runtime_put_noidle(&dev->dev);
> +
>  	return 0;
>  }
>  
>  static void pcie_portdrv_remove(struct pci_dev *dev)
>  {
> +	if (pcie_port_runtime_suspend_allowed(dev))
> +		pm_runtime_get_noresume(&dev->dev);
> +
>  	pcie_port_device_remove(dev);
>  }
>  
> -- 
> 2.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] PCI: No need to set d3cold_allowed to PCIe ports

[Mika Westerberg]

On Fri, Mar 11, 2016 at 06:01:11PM -0600, Bjorn Helgaas wrote:
> On Mon, Feb 29, 2016 at 02:56:01PM +0200, Mika Westerberg wrote:
> > PCIe ports are already skipped by the PCI core so they are never moved to
> > D3 (or D3cold) anyway. No need to set the field.
> 
> A pointer to where the PCI core skips these ports would be helpful.
> pci_pm_runtime_suspend() is the only place I see that tests d3cold_allowed,
> and I don't see that it checks anything about PCIe ports.

Right, sorry about that. The changelog should say that the PCI core
skips PCI bridges and PCIe ports.

The relevant code is in drivers/pci/pci-driver.c:pci_pm_suspend_noirq():

        if (!pci_dev->state_saved) {
                pci_save_state(pci_dev);
                if (!pci_has_subordinate(pci_dev))
                        pci_prepare_to_sleep(pci_dev);
        }

It checks if the device has subordinate devices if not moves the device
to low power state.

Re: [PATCH 4/6] PCI: Add runtime PM support for PCIe ports

[Mika Westerberg]

On Fri, Mar 11, 2016 at 06:30:29PM -0600, Bjorn Helgaas wrote:
> On Mon, Feb 29, 2016 at 02:56:04PM +0200, Mika Westerberg wrote:
> > Add back runtime PM support for PCIe ports that was removed in
> > commit fe9a743a2601 ("PCI/PM: Drop unused runtime PM support code for PCIe
> > ports").
> > 
> > First of all we cannot enable it automatically for all root ports since
> > there have been problems previously, 
> 
> Pointers to these problems would be useful.

Right, sorry about that.

Runtime PM for PCIe ports was disabled by commit de7d5f729c72 ("PCI/PM:
Disable runtime PM of PCIe ports"). Changelog in that commit says that
runtime suspending of ports might prevent things like hotplug from
working. The bug report it refers is here:

https://bugzilla.kernel.org/show_bug.cgi?id=53811

I can add these to the changelog as well.

> > so we enable it only based on per port
> > configuration (runtime_suspend_allowed).
> 
> I see that the next patch basically adds a quirk to set
> runtime_suspend_allowed for Sunrisepoint.  How do we prevent this
> from being a maintenance nightmare?  I don't want to have to add
> a quirk for every new device that supports runtime PM.

One alternative is to do the same that patch [3/6] does for system
suspend and use cut-off date.

As far as I can tell Windows added RTD3 (runtime D3, similart to our
runtime PM) support for PCIe ports quite recently so I would expect
recent hardware to be better validated against that.

> > Furthermore we need to check that
> > the device, if wake capable, can do so from D3cold (which is the state it
> > goes after the root port is powered down).
> 
> I don't see this check.  I was expecting something like
> device_may_wakeup(&pdev->dev) && !pci_pme_capable(pdev, PCI_D3cold)
> like you did in the "Move PCIe ports to D3hot during suspend" patch.

This patch actually uses pcie_port_can_suspend() introduced in patch
[3/6] which does the check.

> > This follows what we did for
> > PCIe port system suspend already.
> > 
> > Runtime PM is still blocked and needs to be unblocked from userspace like
> > with other PCI devices.
> 
> Can you expand on this a bit?  I assume you mean that runtime PM is
> disabled by default, and userspace has to enable it with a sysfs switch or
> something?  Can you include information about how to do that?

That's right. I'm calling it "unblock" to avoid confusion with "enable"
as if runtime PM is merely enabled nothing happens until someone calls
pm_runtime_allow() (or does the same from userspace).

Here is the script I've been using:

-----8<-----8<-----8<-----8<------8<-----8<-----8<------
#!/bin/sh

PORTS=$(lspci -D | grep "PCI bridge" | cut -f 1 -d ' ')

# Enable runtime PM for all ports
for port in $PORTS; do
	# Enable runtime PM for all connected devices
	for device in /sys/bus/pci/devices/$port/????:??:??.?; do
		echo auto > $device/power/control
	done

	# Then for the port itself
	echo auto > /sys/bus/pci/devices/$port/power/control
done
-----8<-----8<-----8<-----8<------8<-----8<-----8<------

I will include that in the changelog as well.

> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/pci/pcie/portdrv_pci.c | 49 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> > 
> > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> > index fe3349685141..eb0f425f51cc 100644
> > --- a/drivers/pci/pcie/portdrv_pci.c
> > +++ b/drivers/pci/pcie/portdrv_pci.c
> > @@ -85,6 +85,7 @@ enum pcie_port_type {
> >  
> >  struct pcie_port_config {
> >  	bool suspend_allowed;
> > +	bool runtime_suspend_allowed;
> >  };
> >  
> >  static const struct pcie_port_config pcie_port_configs[] = {
> > @@ -154,6 +155,11 @@ static bool pcie_port_suspend_allowed(struct pci_dev *pdev)
> >  	return pcie_port_can_suspend(pdev);
> >  }
> >  
> > +static bool pcie_port_runtime_suspend_allowed(struct pci_dev *pdev)
> > +{
> > +	return pcie_port_get_config(pdev)->runtime_suspend_allowed;
> > +}
> > +
> >  static int pcie_port_suspend_noirq(struct device *dev)
> >  {
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> > @@ -188,6 +194,34 @@ static int pcie_port_resume_noirq(struct device *dev)
> >  	return 0;
> >  }
> >  
> > +static int pcie_port_runtime_suspend(struct device *dev)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > +	/*
> > +	 * All devices behind the port are assumed to be in D3cold so
> > +	 * update their state now.
> > +	 */
> > +	__pci_bus_set_current_state(pdev->subordinate, PCI_D3cold);
> > +	return 0;
> > +}
> > +
> > +static int pcie_port_runtime_resume(struct device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int pcie_port_runtime_idle(struct device *dev)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > +	if (pcie_port_can_suspend(pdev)) {
> > +		pm_schedule_suspend(dev, 10);
> > +		return 0;
> > +	}
> > +	return -EBUSY;
> > +}
> > +
> >  static const struct dev_pm_ops pcie_portdrv_pm_ops = {
> >  	.suspend	= pcie_port_device_suspend,
> >  	.resume		= pcie_port_device_resume,
> > @@ -197,12 +231,20 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
> >  	.restore	= pcie_port_device_resume,
> >  	.suspend_noirq	= pcie_port_suspend_noirq,
> >  	.resume_noirq	= pcie_port_resume_noirq,
> > +	.runtime_suspend = pcie_port_runtime_suspend,
> > +	.runtime_resume	= pcie_port_runtime_resume,
> > +	.runtime_idle	= pcie_port_runtime_idle,
> >  };
> >  
> >  #define PCIE_PORTDRV_PM_OPS	(&pcie_portdrv_pm_ops)
> >  
> >  #else /* !PM */
> >  
> > +static inline bool pcie_port_runtime_suspend_allowed(struct pci_dev *pdev)
> > +{
> > +	return false;
> > +}
> > +
> >  #define PCIE_PORTDRV_PM_OPS	NULL
> >  #endif /* !PM */
> >  
> > @@ -230,11 +272,18 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> >  		return status;
> >  
> >  	pci_save_state(dev);
> > +
> > +	if (pcie_port_runtime_suspend_allowed(dev))
> > +		pm_runtime_put_noidle(&dev->dev);
> > +
> >  	return 0;
> >  }
> >  
> >  static void pcie_portdrv_remove(struct pci_dev *dev)
> >  {
> > +	if (pcie_port_runtime_suspend_allowed(dev))
> > +		pm_runtime_get_noresume(&dev->dev);
> > +
> >  	pcie_port_device_remove(dev);
> >  }
> >  
> > -- 
> > 2.7.0
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] PCI: Move PCIe ports to D3hot during suspend

[Mika Westerberg]

On Fri, Mar 11, 2016 at 06:20:39PM -0600, Bjorn Helgaas wrote:
> On Mon, Feb 29, 2016 at 02:56:03PM +0200, Mika Westerberg wrote:
> > Currently the PCI core does not do this automatically as it avoids changing
> > power state for bridges and PCIe ports. With recent hardware PCIe ports can
> > be moved to D3hot given that we take into account few restrictions:
> > 
> >   - Devices connected to the ports are effectively in D3cold once the root
> >     port is moved to D3hot (the config space is not accessible anymore and
> >     the link may be powered down).
> > 
> >   - The device needs to be able to transition to D3cold.
> 
> I think this is talking about "devices below the PCIe port", right?

That's right. I'll fix it in the next version.

> >   - If the device is capable of waking the system it needs to be able to
> >     do so from D3cold (PME from D3cold).
> 
> Again, "a device below the PCIe port"?

Right (will fix in the next version).

> > We assume all recent hardware (starting from 2015) is capable of doing this
> > but make it possible to add exceptions via entries in pcie_port_configs[].
> 
> Since you have no exceptions yet, I'm not sure it's worth having the
> exception infrastructure.  That infrastructure kind of obscures the
> meat of the patch, so if we really do need it right away, maybe it
> could be added in a new separate patch.

I don't think we need it right now. I'll drop it from the patch.

> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/pci/pcie/portdrv_pci.c | 103 +++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 100 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> > index 6c6bb03392ea..fe3349685141 100644
> > --- a/drivers/pci/pcie/portdrv_pci.c
> > +++ b/drivers/pci/pcie/portdrv_pci.c
> > @@ -20,6 +20,7 @@
> >  
> >  #include "portdrv.h"
> >  #include "aer/aerdrv.h"
> > +#include "../pci.h"
> >  
> >  /*
> >   * Version Information
> > @@ -78,11 +79,105 @@ static int pcie_portdrv_restore_config(struct pci_dev *dev)
> >  	return 0;
> >  }
> >  
> > +enum pcie_port_type {
> > +	PCIE_PORT_DEFAULT,
> > +};
> > +
> > +struct pcie_port_config {
> > +	bool suspend_allowed;
> > +};
> > +
> > +static const struct pcie_port_config pcie_port_configs[] = {
> > +	[PCIE_PORT_DEFAULT] = {
> > +		.suspend_allowed = true,
> > +	},
> > +};
> > +
> >  #ifdef CONFIG_PM
> > +static const struct pcie_port_config *pcie_port_get_config(struct pci_dev *pdev)
> > +{
> > +	const struct pci_device_id *id = pci_match_id(pdev->driver->id_table,
> > +						      pdev);
> > +	return &pcie_port_configs[id->driver_data];
> > +}
> > +
> > +static int pcie_port_check_d3cold(struct pci_dev *pdev, void *data)
> > +{
> > +	bool *d3cold_ok = data;
> > +
> > +	if (pdev->no_d3cold || !pdev->d3cold_allowed)
> > +		*d3cold_ok = false;
> > +	if (device_may_wakeup(&pdev->dev) && !pci_pme_capable(pdev, PCI_D3cold))
> > +		*d3cold_ok = false;
> > +
> > +	return !*d3cold_ok;
> > +}
> > +
> > +static bool pcie_port_can_suspend(struct pci_dev *pdev)
> > +{
> > +	bool d3cold_ok = true;
> > +
> > +	/*
> > +	 * When the port is put to D3hot the devices behind the port are
> > +	 * effectively in D3cold as their config space cannot be accessed
> > +	 * anymore and the link may be powered down.
> > +	 *
> > +	 * We only allow the port to go to D3hot the devices:
> 
> s/the devices/if the subordinate devices/

OK

> > +	 *  - Are allowed to go to D3cold
> > +	 *  - Can wake up from D3cold if they are wake capable
> > +	 */
> > +	pci_walk_bus(pdev->subordinate, pcie_port_check_d3cold, &d3cold_ok);
> > +	return d3cold_ok;
> 
> What happens if the PCIe port initially can be put in D3hot, but we we
> later hot-add a device that would disqualify it?
> 
> Oh, I think I see -- we walk the subordinate devices every time we
> would like to put the port in D3hot:
> 
>   pcie_port_suspend_noirq
>     pcie_port_suspend_allowed
>       dmi_get_date
>       pcie_port_can_suspend
>         pci_walk_bus
> 
> I guess that should work, but it seems clunky to do all that work,
> even though it's not really a performance path.  What if we did this:
> 
>   - add a d3hot_allowed bit in struct pci_dev
> 
>   - when enumerating a port, set d3hot_allowed if BIOS is new enough
>     (it makes me a bit nervous that we apparently default to enabling
>     D3hot on arches without DMI)
> 
>   - when enumerating devices, clear d3hot_allowed in upstream bridge
>     if necessary
> 
>   - when removing last device on a bus, re-do port config (set
>     d3hot_allowed if appropriate)

Sounds reasonable. I'll give it a try.

Re: [PATCH 3/6] PCI: Move PCIe ports to D3hot during suspend

[Mika Westerberg]

On Mon, Mar 14, 2016 at 11:32:59AM +0200, Mika Westerberg wrote:
> > What happens if the PCIe port initially can be put in D3hot, but we we
> > later hot-add a device that would disqualify it?
> > 
> > Oh, I think I see -- we walk the subordinate devices every time we
> > would like to put the port in D3hot:
> > 
> >   pcie_port_suspend_noirq
> >     pcie_port_suspend_allowed
> >       dmi_get_date
> >       pcie_port_can_suspend
> >         pci_walk_bus
> > 
> > I guess that should work, but it seems clunky to do all that work,
> > even though it's not really a performance path.  What if we did this:
> > 
> >   - add a d3hot_allowed bit in struct pci_dev
> > 
> >   - when enumerating a port, set d3hot_allowed if BIOS is new enough
> >     (it makes me a bit nervous that we apparently default to enabling
> >     D3hot on arches without DMI)
> > 
> >   - when enumerating devices, clear d3hot_allowed in upstream bridge
> >     if necessary
> > 
> >   - when removing last device on a bus, re-do port config (set
> >     d3hot_allowed if appropriate)
> 
> Sounds reasonable. I'll give it a try.

It looks like we cannot use this approach. Userspace (and drivers) can
prevent devices from entering D3cold by changing dev->d3cold_allowed.
That can happen after the device has been enumerated.

Re: [PATCH 3/6] PCI: Move PCIe ports to D3hot during suspend

[Bjorn Helgaas]

On Thu, Mar 17, 2016 at 12:31:54PM +0200, Mika Westerberg wrote:
> On Mon, Mar 14, 2016 at 11:32:59AM +0200, Mika Westerberg wrote:
> > > What happens if the PCIe port initially can be put in D3hot, but we we
> > > later hot-add a device that would disqualify it?
> > > 
> > > Oh, I think I see -- we walk the subordinate devices every time we
> > > would like to put the port in D3hot:
> > > 
> > >   pcie_port_suspend_noirq
> > >     pcie_port_suspend_allowed
> > >       dmi_get_date
> > >       pcie_port_can_suspend
> > >         pci_walk_bus
> > > 
> > > I guess that should work, but it seems clunky to do all that work,
> > > even though it's not really a performance path.  What if we did this:
> > > 
> > >   - add a d3hot_allowed bit in struct pci_dev
> > > 
> > >   - when enumerating a port, set d3hot_allowed if BIOS is new enough
> > >     (it makes me a bit nervous that we apparently default to enabling
> > >     D3hot on arches without DMI)
> > > 
> > >   - when enumerating devices, clear d3hot_allowed in upstream bridge
> > >     if necessary
> > > 
> > >   - when removing last device on a bus, re-do port config (set
> > >     d3hot_allowed if appropriate)
> > 
> > Sounds reasonable. I'll give it a try.
> 
> It looks like we cannot use this approach. Userspace (and drivers) can
> prevent devices from entering D3cold by changing dev->d3cold_allowed.
> That can happen after the device has been enumerated.

Well, maybe.  If we want to change d3cold_allowed after enumeration,
we could propagate that change through whatever part of the hierarchy
it affects at that time.  I'd rather have the complexity of DMI
checks, walking the bus, etc., there than in the suspend path.

Bjorn