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

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

[Mika Westerberg]

Current Linux PCI core does not do any kind of power management to PCIe
ports. This means that we waste energy and consume laptop battery even if
the port has nothing connected to. These patches aim to change that to the
right direction.

The first version of the patches can be found here:

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

This assumes that recent (starting from 2015) PCIe ports are capable of
transition to D3hot/D3cold. We add a new flag to struct pci_dev 'bridge_d3'
that is set whenever the PCI core thinks the port can be put to D3. The
check in pci_pm_suspend_noirq() is then extended to cover devices where
'bridge_d3' is set.

We then add pci_bridge_pm_update() that tries to set/clear 'bridge_d3'
whenever power management status of a device is changed. For example when
userspace writes to 'd3cold_allowed' sysfs file.

For all PCI ports where 'bridge_d3' is set we also enable and unblock
runtime PM automatically. Only exception is when the PCIe port claims to
support hotplug. More information about that is in the changelog of patch
[4/4].

Since this also touches xhci, I'm adding Mathias and Greg to check if the
change looks reasonable.

Changes to the previous version:

  - Dropped patch [2/6] as there is no need to use that function from other
    files anymore.

  - Dropped patches [5-6/6] in favor of using cut-off date.

  - Updated changelog of [1/4] to mention where in the PCI core PCI bridge
    and PCIe ports are skipped from being power managed.

  - Instead of checking at suspend time if it is possible to transition the
    port to D3, do it whenever power management status of a device (below a
    port) is changed or when it is added or removed to the bus.

  - Added patch [3/4] to runtime resume a bridge when ACPI hotplug event is
    received.

Please review.

Mika Westerberg (4):
  PCI: No need to set d3cold_allowed to PCIe ports
  PCI: Move PCIe ports to D3 during suspend
  ACPI / hotplug / PCI: Runtime resume bridge before rescan
  PCI: Add runtime PM support for PCIe ports

 drivers/pci/bus.c                  |   1 +
 drivers/pci/hotplug/acpiphp_glue.c |   8 ++-
 drivers/pci/pci-driver.c           |  15 +++++-
 drivers/pci/pci-sysfs.c            |   1 +
 drivers/pci/pci.c                  | 103 +++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                  |   1 +
 drivers/pci/pcie/portdrv_pci.c     |  90 ++++++++++++++++++++++++++++++--
 drivers/pci/remove.c               |   2 +
 drivers/usb/host/xhci-pci.c        |   2 +-
 include/linux/pci.h                |   2 +
 10 files changed, 216 insertions(+), 9 deletions(-)

-- 
2.8.0.rc3

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

[Mika Westerberg]

The Linux PCI core skips PCI bridges and PCIe ports when system is
suspended. The PCI core checks return value of pci_has_subordinate() in
pci_pm_suspend_noirq() to skip all devices where it is non-zero (which
means PCI bridges and PCIe ports).

Since PCIe ports are never suspended in the first place, there is no need
to set d3cold_allowed for them.

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.8.0.rc3

[PATCH v2 2/4] PCI: Move PCIe ports to D3 during suspend

[Mika Westerberg]

Currently the Linux PCI core does not touch power state of PCI bridges and
PCIe ports when system suspend is entered. Leaving them in D0 consumes
power which is not good thing in portable devices such as laptops. This may
also prevent the CPU from entering deeper C-states.

With recent PCIe hardware we can power down the ports to save power given
that we take into account few restrictions:

  - The PCIe port hardware is recent enough, starting from 2015.

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

  - Devices behind the PCIe port need to be allowed to transition to D3cold
    and back. There is a way both drivers and userspace can forbid this.

  - If the device behind the PCIe port is capable of waking the system it
    needs to be able to do so from D3cold.

This patch adds a new flag to struct pci_device called 'bridge_d3'. This
flag is set and cleared by the PCI core whenever there is a change in power
management state of any of the  devices behind the PCIe port. If the above
restrictions are met we set 'bridge_d3' to true and transition the port to
D3 when system is suspended.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/bus.c           |   1 +
 drivers/pci/pci-driver.c    |  15 ++++++-
 drivers/pci/pci-sysfs.c     |   1 +
 drivers/pci/pci.c           | 103 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h           |   1 +
 drivers/pci/remove.c        |   2 +
 drivers/usb/host/xhci-pci.c |   2 +-
 include/linux/pci.h         |   2 +
 8 files changed, 125 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 6c9f5467bc5f..3ef836b5794b 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -291,6 +291,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_fixup_device(pci_fixup_final, dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);
+	pci_bridge_pm_update(dev, false);
 
 	dev->match_driver = true;
 	retval = device_attach(&dev->dev);
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index d7ffd66814bb..2d52c0b5d9a0 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -744,6 +744,19 @@ static int pci_pm_suspend(struct device *dev)
 	return 0;
 }
 
+/*
+ * Check if given device can go to low power state. Currently we allow
+ * normal PCI devices and PCI bridges if their bridge_d3 is set.
+ */
+static bool pci_can_suspend(struct pci_dev *pdev)
+{
+	if (!pci_has_subordinate(pdev))
+		return true;
+	else if (pdev->bridge_d3)
+		return true;
+	return false;
+}
+
 static int pci_pm_suspend_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
@@ -777,7 +790,7 @@ static int pci_pm_suspend_noirq(struct device *dev)
 
 	if (!pci_dev->state_saved) {
 		pci_save_state(pci_dev);
-		if (!pci_has_subordinate(pci_dev))
+		if (pci_can_suspend(pci_dev))
 			pci_prepare_to_sleep(pci_dev);
 	}
 
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index e982010f0ed1..94ceea990775 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -407,6 +407,7 @@ static ssize_t d3cold_allowed_store(struct device *dev,
 
 	pdev->d3cold_allowed = !!val;
 	pm_runtime_resume(dev);
+	pci_bridge_pm_update(pdev, false);
 
 	return count;
 }
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 25e0327d4429..fe31aad3ab3a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -9,6 +9,7 @@
 
 #include <linux/kernel.h>
 #include <linux/delay.h>
+#include <linux/dmi.h>
 #include <linux/init.h>
 #include <linux/of.h>
 #include <linux/of_pci.h>
@@ -2156,6 +2157,107 @@ void pci_config_pm_runtime_put(struct pci_dev *pdev)
 }
 
 /**
+ * pci_bridge_d3_possible - Is it possible to move the bridge to D3
+ * @bridge: Bridge to check
+ *
+ * This function checks if it is possible to move the bridge to D3.
+ * Currently we only allow D3 for recent enough PCIe ports.
+ */
+static bool pci_bridge_d3_possible(struct pci_dev *bridge)
+{
+	unsigned int year;
+
+	if (!pci_is_pcie(bridge))
+		return false;
+
+	switch (pci_pcie_type(bridge)) {
+	case PCI_EXP_TYPE_ROOT_PORT:
+	case PCI_EXP_TYPE_UPSTREAM:
+	case PCI_EXP_TYPE_DOWNSTREAM:
+		/*
+		 * PCIe ports from 2015 and newer should be capable of
+		 * entering D3.
+		 */
+		if (dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL) &&
+		    year >= 2015) {
+			return true;
+		}
+		break;
+	}
+
+	return false;
+}
+
+static int pci_dev_check_d3cold(struct pci_dev *pdev, void *data)
+{
+	bool *d3cold_ok = data;
+
+	/*
+	 * The device needs to be allowed to go D3cold and if it is wake
+	 * capable to do so from D3cold.
+	 */
+	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;
+}
+
+/**
+ * pci_bridge_pm_update - Update PM capabilities of upstream PCI bridge
+ * @dev: PCI device whose configuration changed
+ * @remove: Is the PCI device about to be removed
+ *
+ * When PM configuration of a PCI device is changed this function updates
+ * PM capabilities of the upstream PCI bridge accordingly.
+ */
+void pci_bridge_pm_update(struct pci_dev *pdev, bool remove)
+{
+	struct pci_dev *bridge;
+	bool d3cold_ok = true;
+
+	bridge = pci_upstream_bridge(pdev);
+	if (!bridge || !pci_bridge_d3_possible(bridge))
+		return;
+
+	pci_dev_get(bridge);
+	if (!remove)
+		pci_dev_check_d3cold(pdev, &d3cold_ok);
+
+	if (d3cold_ok) {
+		/*
+		 * We need to go through all children to find out if all of
+		 * them can still go to D3cold.
+		 */
+		pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
+			     &d3cold_ok);
+	}
+	bridge->bridge_d3 = d3cold_ok;
+	pci_dev_put(bridge);
+}
+
+/**
+ * pci_enable_d3cold - Enables or disables D3cold from the device
+ * @pdev: PCI device to handle
+ * @enable: Enable or disable D3cold
+ *
+ * This function can be used in drivers to prevent D3cold from the device
+ * they handle. It also updates upstream PCI bridge PM capabilities
+ * accordingly.
+ */
+void pci_enable_d3cold(struct pci_dev *pdev, bool enable)
+{
+	bool no_d3cold = !enable;
+
+	if (pdev->no_d3cold != no_d3cold) {
+		pdev->no_d3cold = !enable;
+		pci_bridge_pm_update(pdev, false);
+	}
+}
+EXPORT_SYMBOL_GPL(pci_enable_d3cold);
+
+/**
  * pci_pm_init - Initialize PM functions of given PCI device
  * @dev: PCI device to handle.
  */
@@ -2189,6 +2291,7 @@ void pci_pm_init(struct pci_dev *dev)
 	dev->pm_cap = pm;
 	dev->d3_delay = PCI_PM_D3_WAIT;
 	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
+	dev->bridge_d3 = pci_bridge_d3_possible(dev);
 	dev->d3cold_allowed = true;
 
 	dev->d1_support = false;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index d0fb93481573..de3e95cf26c8 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -78,6 +78,7 @@ bool pci_dev_keep_suspended(struct pci_dev *dev);
 void pci_dev_complete_resume(struct pci_dev *pci_dev);
 void pci_config_pm_runtime_get(struct pci_dev *dev);
 void pci_config_pm_runtime_put(struct pci_dev *dev);
+void pci_bridge_pm_update(struct pci_dev *dev, bool remove);
 void pci_pm_init(struct pci_dev *dev);
 void pci_ea_init(struct pci_dev *dev);
 void pci_allocate_cap_save_buffers(struct pci_dev *dev);
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 8982026637d5..a06bfd3626cc 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -96,6 +96,8 @@ static void pci_remove_bus_device(struct pci_dev *dev)
 		dev->subordinate = NULL;
 	}
 
+	pci_bridge_pm_update(dev, true);
+
 	pci_destroy_dev(dev);
 }
 
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index f0640b7a1c42..aafc1b093bc8 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -379,7 +379,7 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 	 * need to have the registers polled during D3, so avoid D3cold.
 	 */
 	if (xhci->quirks & XHCI_COMP_MODE_QUIRK)
-		pdev->no_d3cold = true;
+		pci_enable_d3cold(pdev, false);
 
 	if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
 		xhci_pme_quirk(hcd);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 004b8133417d..d8801587b4ca 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -296,6 +296,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	d3cold_allowed:1;	/* D3cold is allowed by user */
 	unsigned int	mmio_always_on:1;	/* disallow turning off io/mem
 						   decoding during bar sizing */
@@ -1085,6 +1086,7 @@ int pci_back_from_sleep(struct pci_dev *dev);
 bool pci_dev_run_wake(struct pci_dev *dev);
 bool pci_check_pme_status(struct pci_dev *dev);
 void pci_pme_wakeup_bus(struct pci_bus *bus);
+void pci_enable_d3cold(struct pci_dev *dev, bool enable);
 
 static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 				  bool enable)
-- 
2.8.0.rc3

Re: [PATCH v2 2/4] PCI: Move PCIe ports to D3 during suspend

[Greg Kroah-Hartman]

On Fri, Apr 08, 2016 at 01:36:28PM +0300, Mika Westerberg wrote:
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index f0640b7a1c42..aafc1b093bc8 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -379,7 +379,7 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
>  	 * need to have the registers polled during D3, so avoid D3cold.
>  	 */
>  	if (xhci->quirks & XHCI_COMP_MODE_QUIRK)
> -		pdev->no_d3cold = true;
> +		pci_enable_d3cold(pdev, false);
>  
>  	if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
>  		xhci_pme_quirk(hcd);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 004b8133417d..d8801587b4ca 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -296,6 +296,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	d3cold_allowed:1;	/* D3cold is allowed by user */
>  	unsigned int	mmio_always_on:1;	/* disallow turning off io/mem
>  						   decoding during bar sizing */
> @@ -1085,6 +1086,7 @@ int pci_back_from_sleep(struct pci_dev *dev);
>  bool pci_dev_run_wake(struct pci_dev *dev);
>  bool pci_check_pme_status(struct pci_dev *dev);
>  void pci_pme_wakeup_bus(struct pci_bus *bus);
> +void pci_enable_d3cold(struct pci_dev *dev, bool enable);

That's an ackward api, as is seen in the above use in the xhci driver
(enable false?)

Why not just make 2 functions:
	pci_dc3cold_enable(struct pci_dev *dev)
	pci_dc3cold_disable(struct pci_dev *dev)
which makes it obvious what is going on.

Whenever you add a bool to a function, it requires the developer to go
look up the implementation to see what it means and how to use it, to
check if the usage is correct.  Just make it obvious in the name itself
so everyone knows exactly what is going on.

Same thing goes for pci_bridge_pm_update(), even with the documentation
I'm not quite sure what the bool flag is for.  Please split this into
two calls as well.

thanks,

greg k-h

Re: [PATCH v2 2/4] PCI: Move PCIe ports to D3 during suspend

[Mika Westerberg]

On Fri, Apr 08, 2016 at 08:07:23AM -0700, Greg Kroah-Hartman wrote:
> On Fri, Apr 08, 2016 at 01:36:28PM +0300, Mika Westerberg wrote:
> > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > index f0640b7a1c42..aafc1b093bc8 100644
> > --- a/drivers/usb/host/xhci-pci.c
> > +++ b/drivers/usb/host/xhci-pci.c
> > @@ -379,7 +379,7 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
> >  	 * need to have the registers polled during D3, so avoid D3cold.
> >  	 */
> >  	if (xhci->quirks & XHCI_COMP_MODE_QUIRK)
> > -		pdev->no_d3cold = true;
> > +		pci_enable_d3cold(pdev, false);
> >  
> >  	if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
> >  		xhci_pme_quirk(hcd);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 004b8133417d..d8801587b4ca 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -296,6 +296,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	d3cold_allowed:1;	/* D3cold is allowed by user */
> >  	unsigned int	mmio_always_on:1;	/* disallow turning off io/mem
> >  						   decoding during bar sizing */
> > @@ -1085,6 +1086,7 @@ int pci_back_from_sleep(struct pci_dev *dev);
> >  bool pci_dev_run_wake(struct pci_dev *dev);
> >  bool pci_check_pme_status(struct pci_dev *dev);
> >  void pci_pme_wakeup_bus(struct pci_bus *bus);
> > +void pci_enable_d3cold(struct pci_dev *dev, bool enable);
> 
> That's an ackward api, as is seen in the above use in the xhci driver
> (enable false?)
> 
> Why not just make 2 functions:
> 	pci_dc3cold_enable(struct pci_dev *dev)
> 	pci_dc3cold_disable(struct pci_dev *dev)
> which makes it obvious what is going on.
> 
> Whenever you add a bool to a function, it requires the developer to go
> look up the implementation to see what it means and how to use it, to
> check if the usage is correct.  Just make it obvious in the name itself
> so everyone knows exactly what is going on.
> 
> Same thing goes for pci_bridge_pm_update(), even with the documentation
> I'm not quite sure what the bool flag is for.  Please split this into
> two calls as well.

Will, do. Thanks for the excellent explanation :)

RE: [PATCH v2 2/4] PCI: Move PCIe ports to D3 during suspend

[Zheng, Qi]

> +static int pci_dev_check_d3cold(struct pci_dev *pdev, void *data) {
> +	bool *d3cold_ok = data;
> +
> +	/*
> +	 * The device needs to be allowed to go D3cold and if it is wake
> +	 * capable to do so from D3cold.
> +	 */
> +	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;
>+}

How about the pme_poll?
IMHO, if the pme_poll is set for some device, the PCIe port couldn't go to sleep as well.

> +void pci_bridge_pm_update(struct pci_dev *pdev, bool remove) {
> +	struct pci_dev *bridge;
> +	bool d3cold_ok = true;
> +
> +	bridge = pci_upstream_bridge(pdev);
> +	if (!bridge || !pci_bridge_d3_possible(bridge))
> +		return;
> +
> +	pci_dev_get(bridge);
> +	if (!remove)
> +		pci_dev_check_d3cold(pdev, &d3cold_ok);
> +
> +	if (d3cold_ok) {
> +		/*
> +		 * We need to go through all children to find out if all of
> +		 * them can still go to D3cold.
> +		 */
> +		pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
> +			     &d3cold_ok);
> +	}
> +	bridge->bridge_d3 = d3cold_ok;
> +	pci_dev_put(bridge);
> +}

IMHO, the PCIe port can go to sleep if all the devices behind it are already in D3, not they have the capability to enter D3.
Besides, why the devices should in D3cold? Why D3hot can't?
Thanks.

Re: [PATCH v2 2/4] PCI: Move PCIe ports to D3 during suspend

[Mika Westerberg]

On Mon, Apr 11, 2016 at 03:36:41AM +0000, Zheng, Qi wrote:
> > +static int pci_dev_check_d3cold(struct pci_dev *pdev, void *data) {
> > +	bool *d3cold_ok = data;
> > +
> > +	/*
> > +	 * The device needs to be allowed to go D3cold and if it is wake
> > +	 * capable to do so from D3cold.
> > +	 */
> > +	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;
> >+}
> 
> How about the pme_poll?
> IMHO, if the pme_poll is set for some device, the PCIe port couldn't
> go to sleep as well.

I wasn't sure about that. If the device has pme_poll set, and the bridge
is in D3 (or anything else than D0) the it will not be scanned for PME
(this is done in pci_pme_list_scan()).

My understanding is that pme_poll is a workaround for bridges which do not
forward PME messages upstream properly. Since this whole thing is only
enabled for recent PCIe hardware, I would expect that this works also :)

> > +void pci_bridge_pm_update(struct pci_dev *pdev, bool remove) {
> > +	struct pci_dev *bridge;
> > +	bool d3cold_ok = true;
> > +
> > +	bridge = pci_upstream_bridge(pdev);
> > +	if (!bridge || !pci_bridge_d3_possible(bridge))
> > +		return;
> > +
> > +	pci_dev_get(bridge);
> > +	if (!remove)
> > +		pci_dev_check_d3cold(pdev, &d3cold_ok);
> > +
> > +	if (d3cold_ok) {
> > +		/*
> > +		 * We need to go through all children to find out if all of
> > +		 * them can still go to D3cold.
> > +		 */
> > +		pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
> > +			     &d3cold_ok);
> > +	}
> > +	bridge->bridge_d3 = d3cold_ok;
> > +	pci_dev_put(bridge);
> > +}
> 
> IMHO, the PCIe port can go to sleep if all the devices behind it are
> already in D3, not they have the capability to enter D3.

The capability check in pci_pme_capable() means that we expect the
device to be capable of triggering PME from D3cold.

> Besides, why the devices should in D3cold? Why D3hot can't?

Because when you power down a bridge you cannot access config space of
the devices below that bridge anymore. It may also trigger the PCIe link
to go to L2 or L3.

Re: [PATCH v2 2/4] PCI: Move PCIe ports to D3 during suspend

[Rafael J. Wysocki]

On Mon, Apr 11, 2016 at 10:56 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Apr 11, 2016 at 03:36:41AM +0000, Zheng, Qi wrote:
>> > +static int pci_dev_check_d3cold(struct pci_dev *pdev, void *data) {
>> > +   bool *d3cold_ok = data;
>> > +
>> > +   /*
>> > +    * The device needs to be allowed to go D3cold and if it is wake
>> > +    * capable to do so from D3cold.
>> > +    */
>> > +   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;
>> >+}
>>
>> How about the pme_poll?
>> IMHO, if the pme_poll is set for some device, the PCIe port couldn't
>> go to sleep as well.
>
> I wasn't sure about that. If the device has pme_poll set, and the bridge
> is in D3 (or anything else than D0) the it will not be scanned for PME
> (this is done in pci_pme_list_scan()).
>
> My understanding is that pme_poll is a workaround for bridges which do not
> forward PME messages upstream properly. Since this whole thing is only
> enabled for recent PCIe hardware, I would expect that this works also :)

Fair enough, but we may need to do something about that in the future
if things turn out to not work properly.

Re: [PATCH v2 2/4] PCI: Move PCIe ports to D3 during suspend

[Mika Westerberg]

On Mon, Apr 11, 2016 at 03:38:07PM +0200, Rafael J. Wysocki wrote:
> Fair enough, but we may need to do something about that in the future
> if things turn out to not work properly.

Yeah, I think we can then add an exception table (blacklist) that
includes those devices which do not work properly. Hopefully it is not
needed at all :)

Re: [PATCH v2 2/4] PCI: Move PCIe ports to D3 during suspend

[Lukas Wunner]

Hi Mika,

On Fri, Apr 08, 2016 at 01:36:28PM +0100, Mika Westerberg wrote:
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -744,6 +744,19 @@ static int pci_pm_suspend(struct device *dev)
>  	return 0;
>  }
>  
> +/*
> + * Check if given device can go to low power state. Currently we allow
> + * normal PCI devices and PCI bridges if their bridge_d3 is set.
> + */
> +static bool pci_can_suspend(struct pci_dev *pdev)
> +{
> +	if (!pci_has_subordinate(pdev))
> +		return true;
> +	else if (pdev->bridge_d3)
> +		return true;
> +	return false;
> +}
> +
>  static int pci_pm_suspend_noirq(struct device *dev)
>  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
> @@ -777,7 +790,7 @@ static int pci_pm_suspend_noirq(struct device *dev)
>  
>  	if (!pci_dev->state_saved) {
>  		pci_save_state(pci_dev);
> -		if (!pci_has_subordinate(pci_dev))
> +		if (pci_can_suspend(pci_dev))
>  			pci_prepare_to_sleep(pci_dev);
>  	}

pci_can_suspend() is only used by this single function. It may be
worth to consider folding it into pci_pm_suspend_noirq(), i.e. simply

		if (!pci_has_subordinate(pci_dev) || pdev->bridge_d3)

together with the "Currently we allow..." comment above.

Best regards,

Lukas

Re: [PATCH v2 2/4] PCI: Move PCIe ports to D3 during suspend

[Mika Westerberg]

On Tue, Apr 12, 2016 at 07:45:42PM +0200, Lukas Wunner wrote:
> Hi Mika,
> 
> On Fri, Apr 08, 2016 at 01:36:28PM +0100, Mika Westerberg wrote:
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -744,6 +744,19 @@ static int pci_pm_suspend(struct device *dev)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Check if given device can go to low power state. Currently we allow
> > + * normal PCI devices and PCI bridges if their bridge_d3 is set.
> > + */
> > +static bool pci_can_suspend(struct pci_dev *pdev)
> > +{
> > +	if (!pci_has_subordinate(pdev))
> > +		return true;
> > +	else if (pdev->bridge_d3)
> > +		return true;
> > +	return false;
> > +}
> > +
> >  static int pci_pm_suspend_noirq(struct device *dev)
> >  {
> >  	struct pci_dev *pci_dev = to_pci_dev(dev);
> > @@ -777,7 +790,7 @@ static int pci_pm_suspend_noirq(struct device *dev)
> >  
> >  	if (!pci_dev->state_saved) {
> >  		pci_save_state(pci_dev);
> > -		if (!pci_has_subordinate(pci_dev))
> > +		if (pci_can_suspend(pci_dev))
> >  			pci_prepare_to_sleep(pci_dev);
> >  	}
> 
> pci_can_suspend() is only used by this single function. It may be
> worth to consider folding it into pci_pm_suspend_noirq(), i.e. simply
> 
> 		if (!pci_has_subordinate(pci_dev) || pdev->bridge_d3)
> 
> together with the "Currently we allow..." comment above.

Okay.

[PATCH v2 3/4] ACPI / hotplug / PCI: Runtime resume bridge before rescan

[Mika Westerberg]

If a PCI bridge (or PCIe port) that is runtime suspended gets an ACPI
hotplug event, such as BUS_CHECK we need to make sure it is resumed before
devices below the bridge are re-scanned. Otherwise the devices behind the
port are not accessible and will be treated as hot-unplugged.

To fix this, resume PCI bridges from runtime suspend while rescanning.

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

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index fa49f9143b80..d64ce8aa99b3 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -756,8 +756,10 @@ static void hotplug_event(u32 type, struct acpiphp_context *context)
 
 	acpi_lock_hp_context();
 	bridge = context->bridge;
-	if (bridge)
+	if (bridge) {
 		get_bridge(bridge);
+		pm_runtime_get_sync(&bridge->pci_dev->dev);
+	}
 
 	acpi_unlock_hp_context();
 
@@ -797,8 +799,10 @@ static void hotplug_event(u32 type, struct acpiphp_context *context)
 	}
 
 	pci_unlock_rescan_remove();
-	if (bridge)
+	if (bridge) {
+		pm_runtime_put(&bridge->pci_dev->dev);
 		put_bridge(bridge);
+	}
 }
 
 static int acpiphp_hotplug_notify(struct acpi_device *adev, u32 type)
-- 
2.8.0.rc3

[PATCH v2 4/4] 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 ports since there
has been problems previously as can be seen in [1]. In summary suspended
PCIe ports were not able to deal with hotplug reliably. One reason why this
might happen is the fact that when a PCIe port is powered down, config
space access to the devices behind the port is not possible. If the BIOS
hotplug SMI handler assumes the port is always in D0 it will not be able to
find the hotplugged devices. To be on the safe side only enable runtime PM
if the port does not claim to be hotplug capable.

Furthermore we need to check that the PCI core thinks the port can go to D3
in the first place. The PCI core sets 'bridge_d3' in that case. If both
conditions are met we enable and allow runtime PM for the PCIe port. Since
'bridge_d3' can be changed anytime we check this in driver ->runtime_idle()
and ->runtime_suspend() and only allow runtime suspend if the flag is still
set. The actual power transition to D3 and back is handled in the PCI core.

Idea to automatically unblock (allow) runtime PM for PCIe ports came from
Dave Airlie.

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

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

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 6c6bb03392ea..bbe86527788c 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -58,6 +58,10 @@ __setup("pcie_ports=", pcie_port_setup);
 
 /* global data */
 
+struct pcie_port_data {
+	bool runtime_pm_enabled;
+};
+
 /**
  * pcie_clear_root_pme_status - Clear root port PME interrupt status.
  * @dev: PCIe root port or event collector.
@@ -93,6 +97,58 @@ static int pcie_port_resume_noirq(struct device *dev)
 	return 0;
 }
 
+static int pcie_port_runtime_suspend(struct device *dev)
+{
+	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
+}
+
+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);
+
+	/*
+	 * Rely the PCI core has set bridge_d3 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.
+	 */
+	if (pdev->bridge_d3) {
+		pm_schedule_suspend(dev, 10);
+		return 0;
+	}
+	return -EBUSY;
+}
+
+static bool pcie_port_runtime_pm_possible(struct pci_dev *pdev)
+{
+	/*
+	 * Only enable runtime PM if the PCI core agrees that this port can
+	 * even go to D3.
+	 */
+	if (!pdev->bridge_d3)
+		return false;
+
+	/*
+	 * Prevent runtime PM if the port is hotplug capable. Otherwise the
+	 * hotplug SMI code might not be able to enumerate devices behind
+	 * this port properly (the port is powered down preventing all
+	 * config space accesses to the subordinate devices).
+	 */
+	if (pcie_caps_reg(pdev) & PCI_EXP_FLAGS_SLOT) {
+		u32 sltcap;
+
+		pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &sltcap);
+		if (sltcap & (PCI_EXP_SLTCAP_HPC | PCI_EXP_SLTCAP_HPS))
+			return false;
+	}
+
+	return true;
+}
+
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.suspend	= pcie_port_device_suspend,
 	.resume		= pcie_port_device_resume,
@@ -101,12 +157,20 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.poweroff	= pcie_port_device_suspend,
 	.restore	= pcie_port_device_resume,
 	.resume_noirq	= pcie_port_resume_noirq,
+	.runtime_suspend = pcie_port_runtime_suspend,
+	.runtime_resume	= pcie_port_runtime_resume,
+	.runtime_idle	= pcie_port_runtime_idle,
 };
 
 #define PCIE_PORTDRV_PM_OPS	(&pcie_portdrv_pm_ops)
 
 #else /* !PM */
 
+static inline bool pcie_port_runtime_pm_possible(struct pci_dev *pdev)
+{
+	return false;
+}
+
 #define PCIE_PORTDRV_PM_OPS	NULL
 #endif /* !PM */
 
@@ -121,6 +185,7 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 static int pcie_portdrv_probe(struct pci_dev *dev,
 					const struct pci_device_id *id)
 {
+	struct pcie_port_data *pdata;
 	int status;
 
 	if (!pci_is_pcie(dev) ||
@@ -134,11 +199,31 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 		return status;
 
 	pci_save_state(dev);
+
+	pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pci_set_drvdata(dev, pdata);
+	if (pcie_port_runtime_pm_possible(dev)) {
+		pm_runtime_put_noidle(&dev->dev);
+		pm_runtime_allow(&dev->dev);
+
+		pdata->runtime_pm_enabled = true;
+	}
+
 	return 0;
 }
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
+	const struct pcie_port_data *pdata = pci_get_drvdata(dev);
+
+	if (pdata->runtime_pm_enabled) {
+		pm_runtime_forbid(&dev->dev);
+		pm_runtime_get_noresume(&dev->dev);
+	}
+
 	pcie_port_device_remove(dev);
 }
 
-- 
2.8.0.rc3

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

[Lukas Wunner]

Hi Mika,

I'm working on runtime pm for Thunderbolt on the Mac and your patches
directly impact that. A Thunderbolt controller comprises an upstream
bridge plus multiple downstream bridges below it, the latter are the
actual hotplug ports. I'm using my own runtime pm code for the PCIe
ports at the moment but will rebase on your patches once they're
finalised.

I've just pushed v2 of my patches to GitHub, this is nearly finished,
just needs some more polish before I can post it:
https://github.com/l1k/linux/commits/thunderbolt_runpm_v2

First of all, the root port on my 2012 Ivy Bridge machine suspends to
D3hot just fine, as do the upstream and downstream ports on the 2010
"Light Ridge" Thunderbolt controller. So the 2015 cut-off in patch [2/4]
seems extremely conservative to me, perhaps unnecessarily so. At least
you've made it possible to override that by setting bridge_d3 = true,
however I'd be happier if this could be extended further back, say,
to 2010. That way it would encompass all Macs with Thunderbolt.

Secondly, you're disabling runtime pm on hotplug ports, citing a
bugzilla entry as an example why this is necessary, however there's
a patch attached to that bugzilla entry which fixes the issue:
https://bugzilla.kernel.org/show_bug.cgi?id=53811

It is therefore unclear why you're disabling it. This breaks my
patches and you've not provided a way to selectively re-enable
runtime pm for specific hotplug ports.

FWIW I've had zero issues suspending the hotplug ports on the Light
Ridge Thunderbolt controller. Hotplug detection works fine even in
D3hot, all that is necessary is to resume the port on hotplug and
unplug while we access the hotplugged device's config space.

So it looks like a hotplug port should be *allowed* to suspend,
but its parent ports (by default) should *not* as we wouldn't be
able to access the hotplug port's config space anymore. On the Mac
even the parent ports may suspend because there's a separate GPE
provided which fires on hotplug during D3cold. Just disabling
runtime pm *generally* on all hotplug ports is too strict.

The changes required for pciehp to work with runtime suspended ports
are apparent from the following patch, I can spin them out into a
separate commit if you would like to include them in your series:
https://github.com/l1k/linux/commit/97d1140926670e6498f6671d91201e6d66e78680


> +static int pcie_port_runtime_idle(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	/*
> +	 * Rely the PCI core has set bridge_d3 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.
> +	 */
> +	if (pdev->bridge_d3) {
> +		pm_schedule_suspend(dev, 10);
> +		return 0;
> +	}
> +	return -EBUSY;
> +}

It's unclear why the pm_schedule_suspend() is needed here and what the
10 ms delay is for. I don't have that delay in my runtime pm code and
haven't seen any issues. If the delay is needed then I'm wondering why
this isn't implemented using autosuspend?


> +static bool pcie_port_runtime_pm_possible(struct pci_dev *pdev)
> +{
> +	/*
> +	 * Only enable runtime PM if the PCI core agrees that this port can
> +	 * even go to D3.
> +	 */
> +	if (!pdev->bridge_d3)
> +		return false;
> +
> +	/*
> +	 * Prevent runtime PM if the port is hotplug capable. Otherwise the
> +	 * hotplug SMI code might not be able to enumerate devices behind
> +	 * this port properly (the port is powered down preventing all
> +	 * config space accesses to the subordinate devices).
> +	 */
> +	if (pcie_caps_reg(pdev) & PCI_EXP_FLAGS_SLOT) {
> +		u32 sltcap;
> +
> +		pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &sltcap);
> +		if (sltcap & (PCI_EXP_SLTCAP_HPC | PCI_EXP_SLTCAP_HPS))
> +			return false;
> +	}
> +
> +	return true;
> +}

Why do you re-read the register here, seems like just checking
pdev->hotplug_bridge should be sufficient?


>  static void pcie_portdrv_remove(struct pci_dev *dev)
>  {
> +	const struct pcie_port_data *pdata = pci_get_drvdata(dev);
> +
> +	if (pdata->runtime_pm_enabled) {
> +		pm_runtime_forbid(&dev->dev);
> +		pm_runtime_get_noresume(&dev->dev);
> +	}
> +
>  	pcie_port_device_remove(dev);
>  }

I think you're missing a pci_set_drvdata(dev, NULL) here.


In my runtime pm code I've set pm_runtime_no_callbacks() for the port
service devices to prevent users from mucking around with their sysfs
files. Feel free to copy that from the above-linked patch if you want.

Best regards,

Lukas

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

[Mika Westerberg]

On Tue, Apr 12, 2016 at 07:52:03PM +0200, Lukas Wunner wrote:
> Hi Mika,
> 
> I'm working on runtime pm for Thunderbolt on the Mac and your patches
> directly impact that. A Thunderbolt controller comprises an upstream
> bridge plus multiple downstream bridges below it, the latter are the
> actual hotplug ports. I'm using my own runtime pm code for the PCIe
> ports at the moment but will rebase on your patches once they're
> finalised.
> 
> I've just pushed v2 of my patches to GitHub, this is nearly finished,
> just needs some more polish before I can post it:
> https://github.com/l1k/linux/commits/thunderbolt_runpm_v2

Interesting :)

> First of all, the root port on my 2012 Ivy Bridge machine suspends to
> D3hot just fine, as do the upstream and downstream ports on the 2010
> "Light Ridge" Thunderbolt controller. So the 2015 cut-off in patch [2/4]
> seems extremely conservative to me, perhaps unnecessarily so. At least
> you've made it possible to override that by setting bridge_d3 = true,
> however I'd be happier if this could be extended further back, say,
> to 2010. That way it would encompass all Macs with Thunderbolt.

What about other non-Mac machines? Can we be sure that the same thing
works there?

I have no problem lowering the cut-off date but it should not start
breaking things. I think Windows is doing something similar but I'm not
sure what their cut-off date is.

> Secondly, you're disabling runtime pm on hotplug ports, citing a
> bugzilla entry as an example why this is necessary, however there's
> a patch attached to that bugzilla entry which fixes the issue:
> https://bugzilla.kernel.org/show_bug.cgi?id=53811
> 
> It is therefore unclear why you're disabling it. This breaks my
> patches and you've not provided a way to selectively re-enable
> runtime pm for specific hotplug ports.

If I understand correctly, it actually does not fix anything. By luck it
works intermittently.

I've been testing on Alpine Ridge (which is the latest and greatest TBT3
host controller) and I'm still seeing the issue ->

> FWIW I've had zero issues suspending the hotplug ports on the Light
> Ridge Thunderbolt controller. Hotplug detection works fine even in
> D3hot, all that is necessary is to resume the port on hotplug and
> unplug while we access the hotplugged device's config space.
> 
> So it looks like a hotplug port should be *allowed* to suspend,
> but its parent ports (by default) should *not* as we wouldn't be
> able to access the hotplug port's config space anymore. On the Mac
> even the parent ports may suspend because there's a separate GPE
> provided which fires on hotplug during D3cold. Just disabling
> runtime pm *generally* on all hotplug ports is too strict.

<- Now, my understanding is that Macs do not use ACPI hotplug but
instead this is all native, correct? When you have the controller
exposed all the time, of course you can get hotplug events and handle
them in the driver.

However, problem arises when enumeration and configuration is actually
done in BIOS SMI handler, like in normal non-Mac PCs. If the port is in
D3 the handler is not smart enough to move it back to D0 and then
re-enumerate ports. It just gives up.

That is the reason we do not runtime suspend those ports.

We can allow runtime suspending PCIe ports that use PCIe native hotplug
but I do not have such hardware here so I'm unable to test that but
since you have, maybe we can co-operate on this :)

> The changes required for pciehp to work with runtime suspended ports
> are apparent from the following patch, I can spin them out into a
> separate commit if you would like to include them in your series:
> https://github.com/l1k/linux/commit/97d1140926670e6498f6671d91201e6d66e78680
> 
> 
> > +static int pcie_port_runtime_idle(struct device *dev)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > +	/*
> > +	 * Rely the PCI core has set bridge_d3 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.
> > +	 */
> > +	if (pdev->bridge_d3) {
> > +		pm_schedule_suspend(dev, 10);
> > +		return 0;
> > +	}
> > +	return -EBUSY;
> > +}
> 
> It's unclear why the pm_schedule_suspend() is needed here and what the
> 10 ms delay is for. I don't have that delay in my runtime pm code and
> haven't seen any issues. If the delay is needed then I'm wondering why
> this isn't implemented using autosuspend?

This part is from the original runtime PM patch. I did not want to
change 10ms delay here. Not sure if it is needed.

However, we need to have pcie_port_runtime_idle() callback because here we
actually check if we are allowed to suspend the port. Using autosuspend
does not work because of that. Documentation/power/runtime_pm.txt says
this regarding ->runtime_idle() callback:

  The action performed by the idle callback is totally dependent on the
  subsystem (or driver) in question, but the expected and recommended
  action is to check if the device can be suspended (i.e. if all of the
  conditions necessary for suspending the device are satisfied) and to
  queue up a suspend request for the device in that case.

So delay probably is not necessary but we need the callback to check if
the port can be suspended.

> > +static bool pcie_port_runtime_pm_possible(struct pci_dev *pdev)
> > +{
> > +	/*
> > +	 * Only enable runtime PM if the PCI core agrees that this port can
> > +	 * even go to D3.
> > +	 */
> > +	if (!pdev->bridge_d3)
> > +		return false;
> > +
> > +	/*
> > +	 * Prevent runtime PM if the port is hotplug capable. Otherwise the
> > +	 * hotplug SMI code might not be able to enumerate devices behind
> > +	 * this port properly (the port is powered down preventing all
> > +	 * config space accesses to the subordinate devices).
> > +	 */
> > +	if (pcie_caps_reg(pdev) & PCI_EXP_FLAGS_SLOT) {
> > +		u32 sltcap;
> > +
> > +		pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &sltcap);
> > +		if (sltcap & (PCI_EXP_SLTCAP_HPC | PCI_EXP_SLTCAP_HPS))
> > +			return false;
> > +	}
> > +
> > +	return true;
> > +}
> 
> Why do you re-read the register here, seems like just checking
> pdev->hotplug_bridge should be sufficient?

Indeed. Although it does not check for surprise hotplug I think we can
use that field instead.

> >  static void pcie_portdrv_remove(struct pci_dev *dev)
> >  {
> > +	const struct pcie_port_data *pdata = pci_get_drvdata(dev);
> > +
> > +	if (pdata->runtime_pm_enabled) {
> > +		pm_runtime_forbid(&dev->dev);
> > +		pm_runtime_get_noresume(&dev->dev);
> > +	}
> > +
> >  	pcie_port_device_remove(dev);
> >  }
> 
> I think you're missing a pci_set_drvdata(dev, NULL) here.

Device core does that automatically.

> In my runtime pm code I've set pm_runtime_no_callbacks() for the port
> service devices to prevent users from mucking around with their sysfs
> files. Feel free to copy that from the above-linked patch if you want.

OK, I'll check those.

Thanks!

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

[Andreas Noever]

On Wed, Apr 13, 2016 at 10:33 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Apr 12, 2016 at 07:52:03PM +0200, Lukas Wunner wrote:
>> Hi Mika,
>>
>> I'm working on runtime pm for Thunderbolt on the Mac and your patches
>> directly impact that. A Thunderbolt controller comprises an upstream
>> bridge plus multiple downstream bridges below it, the latter are the
>> actual hotplug ports. I'm using my own runtime pm code for the PCIe
>> ports at the moment but will rebase on your patches once they're
>> finalised.
>>
>> I've just pushed v2 of my patches to GitHub, this is nearly finished,
>> just needs some more polish before I can post it:
>> https://github.com/l1k/linux/commits/thunderbolt_runpm_v2
>
> Interesting :)
>
>> First of all, the root port on my 2012 Ivy Bridge machine suspends to
>> D3hot just fine, as do the upstream and downstream ports on the 2010
>> "Light Ridge" Thunderbolt controller. So the 2015 cut-off in patch [2/4]
>> seems extremely conservative to me, perhaps unnecessarily so. At least
>> you've made it possible to override that by setting bridge_d3 = true,
>> however I'd be happier if this could be extended further back, say,
>> to 2010. That way it would encompass all Macs with Thunderbolt.
>
> What about other non-Mac machines? Can we be sure that the same thing
> works there?
>
> I have no problem lowering the cut-off date but it should not start
> breaking things. I think Windows is doing something similar but I'm not
> sure what their cut-off date is.
>
>> Secondly, you're disabling runtime pm on hotplug ports, citing a
>> bugzilla entry as an example why this is necessary, however there's
>> a patch attached to that bugzilla entry which fixes the issue:
>> https://bugzilla.kernel.org/show_bug.cgi?id=53811
>>
>> It is therefore unclear why you're disabling it. This breaks my
>> patches and you've not provided a way to selectively re-enable
>> runtime pm for specific hotplug ports.
>
> If I understand correctly, it actually does not fix anything. By luck it
> works intermittently.
>
> I've been testing on Alpine Ridge (which is the latest and greatest TBT3
> host controller) and I'm still seeing the issue ->
>
>> FWIW I've had zero issues suspending the hotplug ports on the Light
>> Ridge Thunderbolt controller. Hotplug detection works fine even in
>> D3hot, all that is necessary is to resume the port on hotplug and
>> unplug while we access the hotplugged device's config space.
>>
>> So it looks like a hotplug port should be *allowed* to suspend,
>> but its parent ports (by default) should *not* as we wouldn't be
>> able to access the hotplug port's config space anymore. On the Mac
>> even the parent ports may suspend because there's a separate GPE
>> provided which fires on hotplug during D3cold. Just disabling
>> runtime pm *generally* on all hotplug ports is too strict.
>
> <- Now, my understanding is that Macs do not use ACPI hotplug but
> instead this is all native, correct? When you have the controller
> exposed all the time, of course you can get hotplug events and handle
> them in the driver.
>
> However, problem arises when enumeration and configuration is actually
> done in BIOS SMI handler, like in normal non-Mac PCs. If the port is in
> D3 the handler is not smart enough to move it back to D0 and then
> re-enumerate ports. It just gives up.
Is this issue specific to the "ACPI/SMI Thunderbolt" implementations
used in non-Mac PCs or is PCI hotplug in general done through SMI/ACPI
instead of the native pciehp port driver? Wouldn't it make more sense
to only disable runtime suspend on non-Mac thunderbolt ports (or ports
using ACPI hotplug, if that is detectable).

Andreas

> That is the reason we do not runtime suspend those ports.
>
> We can allow runtime suspending PCIe ports that use PCIe native hotplug
> but I do not have such hardware here so I'm unable to test that but
> since you have, maybe we can co-operate on this :)
>
>> The changes required for pciehp to work with runtime suspended ports
>> are apparent from the following patch, I can spin them out into a
>> separate commit if you would like to include them in your series:
>> https://github.com/l1k/linux/commit/97d1140926670e6498f6671d91201e6d66e78680
>>
>>
>> > +static int pcie_port_runtime_idle(struct device *dev)
>> > +{
>> > +   struct pci_dev *pdev = to_pci_dev(dev);
>> > +
>> > +   /*
>> > +    * Rely the PCI core has set bridge_d3 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.
>> > +    */
>> > +   if (pdev->bridge_d3) {
>> > +           pm_schedule_suspend(dev, 10);
>> > +           return 0;
>> > +   }
>> > +   return -EBUSY;
>> > +}
>>
>> It's unclear why the pm_schedule_suspend() is needed here and what the
>> 10 ms delay is for. I don't have that delay in my runtime pm code and
>> haven't seen any issues. If the delay is needed then I'm wondering why
>> this isn't implemented using autosuspend?
>
> This part is from the original runtime PM patch. I did not want to
> change 10ms delay here. Not sure if it is needed.
>
> However, we need to have pcie_port_runtime_idle() callback because here we
> actually check if we are allowed to suspend the port. Using autosuspend
> does not work because of that. Documentation/power/runtime_pm.txt says
> this regarding ->runtime_idle() callback:
>
>   The action performed by the idle callback is totally dependent on the
>   subsystem (or driver) in question, but the expected and recommended
>   action is to check if the device can be suspended (i.e. if all of the
>   conditions necessary for suspending the device are satisfied) and to
>   queue up a suspend request for the device in that case.
>
> So delay probably is not necessary but we need the callback to check if
> the port can be suspended.
>
>> > +static bool pcie_port_runtime_pm_possible(struct pci_dev *pdev)
>> > +{
>> > +   /*
>> > +    * Only enable runtime PM if the PCI core agrees that this port can
>> > +    * even go to D3.
>> > +    */
>> > +   if (!pdev->bridge_d3)
>> > +           return false;
>> > +
>> > +   /*
>> > +    * Prevent runtime PM if the port is hotplug capable. Otherwise the
>> > +    * hotplug SMI code might not be able to enumerate devices behind
>> > +    * this port properly (the port is powered down preventing all
>> > +    * config space accesses to the subordinate devices).
>> > +    */
>> > +   if (pcie_caps_reg(pdev) & PCI_EXP_FLAGS_SLOT) {
>> > +           u32 sltcap;
>> > +
>> > +           pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &sltcap);
>> > +           if (sltcap & (PCI_EXP_SLTCAP_HPC | PCI_EXP_SLTCAP_HPS))
>> > +                   return false;
>> > +   }
>> > +
>> > +   return true;
>> > +}
>>
>> Why do you re-read the register here, seems like just checking
>> pdev->hotplug_bridge should be sufficient?
>
> Indeed. Although it does not check for surprise hotplug I think we can
> use that field instead.
>
>> >  static void pcie_portdrv_remove(struct pci_dev *dev)
>> >  {
>> > +   const struct pcie_port_data *pdata = pci_get_drvdata(dev);
>> > +
>> > +   if (pdata->runtime_pm_enabled) {
>> > +           pm_runtime_forbid(&dev->dev);
>> > +           pm_runtime_get_noresume(&dev->dev);
>> > +   }
>> > +
>> >     pcie_port_device_remove(dev);
>> >  }
>>
>> I think you're missing a pci_set_drvdata(dev, NULL) here.
>
> Device core does that automatically.
>
>> In my runtime pm code I've set pm_runtime_no_callbacks() for the port
>> service devices to prevent users from mucking around with their sysfs
>> files. Feel free to copy that from the above-linked patch if you want.
>
> OK, I'll check those.
>
> Thanks!

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

[Mika Westerberg]

On Wed, Apr 13, 2016 at 11:08:37AM +0200, Andreas Noever wrote:
> > <- Now, my understanding is that Macs do not use ACPI hotplug but
> > instead this is all native, correct? When you have the controller
> > exposed all the time, of course you can get hotplug events and handle
> > them in the driver.
> >
> > However, problem arises when enumeration and configuration is actually
> > done in BIOS SMI handler, like in normal non-Mac PCs. If the port is in
> > D3 the handler is not smart enough to move it back to D0 and then
> > re-enumerate ports. It just gives up.
> Is this issue specific to the "ACPI/SMI Thunderbolt" implementations
> used in non-Mac PCs or is PCI hotplug in general done through SMI/ACPI
> instead of the native pciehp port driver? Wouldn't it make more sense
> to only disable runtime suspend on non-Mac thunderbolt ports (or ports
> using ACPI hotplug, if that is detectable).

I think it is specific to ACPI hotplug implementations (non-Mac PCs). We
should be able to detect which one is used. ACPI _OSC method is used to
allow/disallow native PCIe hotplug so I think we use that information
along with the slot capabilities to decide whether we enable or disable
runtime PM.

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

[Lukas Wunner]

Hi Mika,

thanks for enabling runtime pm on native pciehp ports in v3 of your
series. I've ditched my own runtime pm code now, rebased on top of
your v3 and got everything working again:
https://github.com/l1k/linux/commits/thunderbolt_runpm_v2


There's one thing I had to change in your patches: Patch [4/4] only
allows runtime pm if pci_dev->bridge_d3 is set when portdrv loads.
However bridge_d3 can change over time. Let's say the bridge is a
Thunderbolt port which has a device attached on boot which is PME
capable but not from D3cold. Then runtime pm will not be allowed.
Once that device is unplugged, bridge_d3 will be set to true,
but the port won't go to D3 because runtime pm wasn't allowed.

The solution is to allow runtime pm regardless of bridge_d3, and to rely
solely on the bridge_d3 checks in ->runtime_idle and ->runtime_suspend.
Since the only remaining check in pcie_port_runtime_pm_possible()
(whether it's an ACPI hotplug port) doesn't change over time, we can
just call that function again in ->remove and therefore struct
pcie_port_data is no longer necessary.

In case you agree with this, I've prepared a fixup patch which you
can apply with "git rebase --autosquash":
https://github.com/l1k/linux/commit/24fc9d7b34ffc88f562fa67be248f95dd488da1e


Another thing I've spotted but that wasn't needed to get my patches
working: When the bridge_d3 attribute changes to true, you need to
notify the PM core thereof by calling pm_request_idle() for the
bridge device, otherwise the bridge needlessly stays awake. This
needs to be added near the end of pci_bridge_d3_update() I guess.


I've spun out the changes necessary for pciehp to support runtime pm
into a separate commit now. You could either include that in your
series or I could post it as part of my series later on:
https://github.com/l1k/linux/commit/0800e6851264960a3148a5d81076ad079e80caff

One detail I'm not sure about: If you've got a hotplug downstream port
behind an upstream port and the upstream port goes to D3hot, is it
still possible for the downstream port to signal hotplug interrupts?
In other words, can INTx or MSI interrupts generated by the downstream
bridge still be routed via the upstream bridge if the upstream bridge
is in D3hot? I cannot easily test this, I'd have to hack the tg3 driver
for my Thunderbolt Ethernet adapter to use runtime suspend.

My guess is that the interrupts are *not* routed if the upstream bridge
is in D3. If that is true then the algorithm in pci_bridge_d3_update()
and pci_dev_check_d3cold() needs to be amended so that the hotplug bridge
may suspend but anything above it may not.


On Wed, Apr 13, 2016 at 11:33:52AM +0300, Mika Westerberg wrote:
> I did not change the cut-off date from 2015 yet to be on the safe side,
> even if older Macs seem to work just fine. Maybe it can be lowered to 2013
> or so but I would like to hear from Bjorn and Rafael what they think about
> that.

My opinion is that we should strive for maximum power saving, thus try
(at least) 2010 and blacklist individual devices as needed.


> On Tue, Apr 12, 2016 at 07:52:03PM +0200, Lukas Wunner wrote:
> > It's unclear why the pm_schedule_suspend() is needed here and what the
> > 10 ms delay is for. I don't have that delay in my runtime pm code and
> > haven't seen any issues. If the delay is needed then I'm wondering why
> > this isn't implemented using autosuspend?
> 
> This part is from the original runtime PM patch. I did not want to
> change 10ms delay here. Not sure if it is needed.

Found it, the delay is explained in the commit message of 3d8387efe1ad
("PCI/PM: Fix config reg access for D3cold and bridge suspending").
It's supposed to prevent repeatedly suspending and resuming for every
configuration register read/write.

That makes sense but I'd suggest changing this to autosuspend,
thereby allowing users to change it to their heart's content.

The delay should also be re-explained in the commit message of
patch [4/4].


> However, we need to have pcie_port_runtime_idle() callback because here we
> actually check if we are allowed to suspend the port. Using autosuspend
> does not work because of that. Documentation/power/runtime_pm.txt says
> this regarding ->runtime_idle() callback:
> 
>   The action performed by the idle callback is totally dependent on the
>   subsystem (or driver) in question, but the expected and recommended
>   action is to check if the device can be suspended (i.e. if all of the
>   conditions necessary for suspending the device are satisfied) and to
>   queue up a suspend request for the device in that case.
> 
> So delay probably is not necessary but we need the callback to check if
> the port can be suspended.

If ->runtime_idle returns 0, the PM core automatically calls
->runtime_suspend. So there's no need for pm_schedule_suspend().


One more thing, when reviewing patch [2/4], it took me a while to
grasp that you've chosen to determine *in advance* whether a bridge
is allowed to go to D3. The previous attempt at D3 support for PCIe
ports, 448bd857d48e ("PCI/PM: add PCIe runtime D3cold support"),
took the opposite approach and determined this *on demand* in
pcie_port_runtime_suspend(). It may be worth making that explicit
in the commit message so that it's easier for an uninitiated reader
to comprehend what's going on.

I also found it difficult to understand the precise meaning of the
various d3 attributes that we now have in struct pci_dev. That isn't
your fault, it's caused by how things have evolved over time, but it
may be worth to pay back this technical debt while we're at it.
The comment for no_d3cold says "D3cold is forbidden". The actual
meaning is that drivers may set this to true in their ->runtime_suspend
callback to prevent the platform from putting the device into D3cold.
So perhaps a more appropriate comment would be "Runtime d3cold forbidden
by driver".

Likewise the comment for d3cold_allowed is "D3cold is allowed by user".
In reality this attribute is set to true by default, so there's nothing
for the user to allow. Rather, the user may set it to *false* to prevent
the platform from runtime suspending the device to D3cold. A more
appropriate comment would be "Runtime d3cold not forbidden by user".
Alternatively the variable name could be changed to d3cold_forbidden,
or d3cold_usr_forbidden. (Then no_d3cold could be renamed to
d3cold_drv_forbidden.)


Best regards,

Lukas

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

[Mika Westerberg]

On Mon, Apr 18, 2016 at 04:38:23PM +0200, Lukas Wunner wrote:
> Hi Mika,
> 
> thanks for enabling runtime pm on native pciehp ports in v3 of your
> series. I've ditched my own runtime pm code now, rebased on top of
> your v3 and got everything working again:
> https://github.com/l1k/linux/commits/thunderbolt_runpm_v2

Cool, thanks for testing :)

> There's one thing I had to change in your patches: Patch [4/4] only
> allows runtime pm if pci_dev->bridge_d3 is set when portdrv loads.
> However bridge_d3 can change over time. Let's say the bridge is a
> Thunderbolt port which has a device attached on boot which is PME
> capable but not from D3cold. Then runtime pm will not be allowed.
> Once that device is unplugged, bridge_d3 will be set to true,
> but the port won't go to D3 because runtime pm wasn't allowed.
> 
> The solution is to allow runtime pm regardless of bridge_d3, and to rely
> solely on the bridge_d3 checks in ->runtime_idle and ->runtime_suspend.
> Since the only remaining check in pcie_port_runtime_pm_possible()
> (whether it's an ACPI hotplug port) doesn't change over time, we can
> just call that function again in ->remove and therefore struct
> pcie_port_data is no longer necessary.

I thought about that also but then decided to add the check because it
could mislead the user to think their PCIe port has runtime PM enabled
even when it actually is not (because we always return -EBUSY from the
callbacks).

But then again they can enable runtime PM without these patches and the
port is never suspended anyway.

> In case you agree with this, I've prepared a fixup patch which you
> can apply with "git rebase --autosquash":
> https://github.com/l1k/linux/commit/24fc9d7b34ffc88f562fa67be248f95dd488da1e

Based on the above, I'm going to fold your commit to [4/4]. Are you OK
if I add your Signed-off-by to that patch?

> Another thing I've spotted but that wasn't needed to get my patches
> working: When the bridge_d3 attribute changes to true, you need to
> notify the PM core thereof by calling pm_request_idle() for the
> bridge device, otherwise the bridge needlessly stays awake. This
> needs to be added near the end of pci_bridge_d3_update() I guess.

I've been testing this by writing to 'd3cold_allowed' sysfs file and it
suspends just fine when it is 1. How do you reproduce this?

> I've spun out the changes necessary for pciehp to support runtime pm
> into a separate commit now. You could either include that in your
> series or I could post it as part of my series later on:
> https://github.com/l1k/linux/commit/0800e6851264960a3148a5d81076ad079e80caff

I think it may be better if you submit that as part of your series. I
have no means to test it properly here.

> One detail I'm not sure about: If you've got a hotplug downstream port
> behind an upstream port and the upstream port goes to D3hot, is it
> still possible for the downstream port to signal hotplug interrupts?
> In other words, can INTx or MSI interrupts generated by the downstream
> bridge still be routed via the upstream bridge if the upstream bridge
> is in D3hot? I cannot easily test this, I'd have to hack the tg3 driver
> for my Thunderbolt Ethernet adapter to use runtime suspend.

If the downstream port is able to trigger wake (PME) from D3cold, I
think it should work but I'm not 100% sure.

> My guess is that the interrupts are *not* routed if the upstream bridge
> is in D3. If that is true then the algorithm in pci_bridge_d3_update()
> and pci_dev_check_d3cold() needs to be amended so that the hotplug bridge
> may suspend but anything above it may not.

PMEs should be forwarded upstream even if the upstream bridge is in
D3hot.

> On Wed, Apr 13, 2016 at 11:33:52AM +0300, Mika Westerberg wrote:
> > I did not change the cut-off date from 2015 yet to be on the safe side,
> > even if older Macs seem to work just fine. Maybe it can be lowered to 2013
> > or so but I would like to hear from Bjorn and Rafael what they think about
> > that.
> 
> My opinion is that we should strive for maximum power saving, thus try
> (at least) 2010 and blacklist individual devices as needed.

IMHO we should strive for working systems and not maximum power saving
but I'm OK to change that if Bjorn or Rafael are fine with it.

> > On Tue, Apr 12, 2016 at 07:52:03PM +0200, Lukas Wunner wrote:
> > > It's unclear why the pm_schedule_suspend() is needed here and what the
> > > 10 ms delay is for. I don't have that delay in my runtime pm code and
> > > haven't seen any issues. If the delay is needed then I'm wondering why
> > > this isn't implemented using autosuspend?
> > 
> > This part is from the original runtime PM patch. I did not want to
> > change 10ms delay here. Not sure if it is needed.
> 
> Found it, the delay is explained in the commit message of 3d8387efe1ad
> ("PCI/PM: Fix config reg access for D3cold and bridge suspending").
> It's supposed to prevent repeatedly suspending and resuming for every
> configuration register read/write.

Indeed, thanks for looking it up.

> That makes sense but I'd suggest changing this to autosuspend,
> thereby allowing users to change it to their heart's content.

OK, I'll change it to use autosuspend.

> The delay should also be re-explained in the commit message of
> patch [4/4].

Sure, I'll explain it in the commit message.

> > However, we need to have pcie_port_runtime_idle() callback because here we
> > actually check if we are allowed to suspend the port. Using autosuspend
> > does not work because of that. Documentation/power/runtime_pm.txt says
> > this regarding ->runtime_idle() callback:
> > 
> >   The action performed by the idle callback is totally dependent on the
> >   subsystem (or driver) in question, but the expected and recommended
> >   action is to check if the device can be suspended (i.e. if all of the
> >   conditions necessary for suspending the device are satisfied) and to
> >   queue up a suspend request for the device in that case.
> > 
> > So delay probably is not necessary but we need the callback to check if
> > the port can be suspended.
> 
> If ->runtime_idle returns 0, the PM core automatically calls
> ->runtime_suspend. So there's no need for pm_schedule_suspend().

I see, thanks.

> One more thing, when reviewing patch [2/4], it took me a while to
> grasp that you've chosen to determine *in advance* whether a bridge
> is allowed to go to D3. The previous attempt at D3 support for PCIe
> ports, 448bd857d48e ("PCI/PM: add PCIe runtime D3cold support"),
> took the opposite approach and determined this *on demand* in
> pcie_port_runtime_suspend(). It may be worth making that explicit
> in the commit message so that it's easier for an uninitiated reader
> to comprehend what's going on.

OK, I'll mention that in the commit message then.

> I also found it difficult to understand the precise meaning of the
> various d3 attributes that we now have in struct pci_dev. That isn't
> your fault, it's caused by how things have evolved over time, but it
> may be worth to pay back this technical debt while we're at it.
> The comment for no_d3cold says "D3cold is forbidden". The actual
> meaning is that drivers may set this to true in their ->runtime_suspend
> callback to prevent the platform from putting the device into D3cold.
> So perhaps a more appropriate comment would be "Runtime d3cold forbidden
> by driver".

I also got confused by it first so I think adding "by driver" makes it
more understandable. I can cook a separate patch changing this.

> Likewise the comment for d3cold_allowed is "D3cold is allowed by user".
> In reality this attribute is set to true by default, so there's nothing
> for the user to allow. Rather, the user may set it to *false* to prevent
> the platform from runtime suspending the device to D3cold. A more
> appropriate comment would be "Runtime d3cold not forbidden by user".
> Alternatively the variable name could be changed to d3cold_forbidden,
> or d3cold_usr_forbidden. (Then no_d3cold could be renamed to
> d3cold_drv_forbidden.)

Here, I think the comment is fine (explains meaning of that field), at
least I was able to understand what it means :) Also it is not only
about runtime PM but it concerns system sleep as well.

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

[Lukas Wunner]

Hi Mika,

On Tue, Apr 19, 2016 at 03:31:31PM +0300, Mika Westerberg wrote:
> On Mon, Apr 18, 2016 at 04:38:23PM +0200, Lukas Wunner wrote:
> > In case you agree with this, I've prepared a fixup patch which you
> > can apply with "git rebase --autosquash":
> > https://github.com/l1k/linux/commit/24fc9d7b34ff
> 
> Based on the above, I'm going to fold your commit to [4/4]. Are you OK
> if I add your Signed-off-by to that patch?

Sure.


> > Another thing I've spotted but that wasn't needed to get my patches
> > working: When the bridge_d3 attribute changes to true, you need to
> > notify the PM core thereof by calling pm_request_idle() for the
> > bridge device, otherwise the bridge needlessly stays awake. This
> > needs to be added near the end of pci_bridge_d3_update() I guess.
> 
> I've been testing this by writing to 'd3cold_allowed' sysfs file and it
> suspends just fine when it is 1. How do you reproduce this?

Testing with d3cold_allowed is misleading in this case because
d3cold_allowed_store() already calls pm_runtime_resume(dev),
and the next time dev runtime suspends, it automatically calls
rpm_idle() on its parent. This masks that a call to pm_request_idle()
is currently missing in pci_bridge_d3_update().

However pci_bridge_d3_update() also gets called e.g. from
pci_remove_bus_device() and there's no call to pm_request_idle()
there, so a bridge would stay awake even if a child that has blocked
d3 has been removed.

I only noticed this because I force bridge_d3 = true when loading
the thunderbolt upstream bridge driver, as a workaround for the BIOS
cut-off date, and noticed that I had to call pm_request_idle() because
otherwise the bridges would stay awake.


I hadn't played with d3cold_allowed before but tested it now that
you've mentioned it. Found a bug there:

If d3cold_allowed is set to false for a device via sysfs, the bridge_d3
attribute of its parent bridges will correctly be updated to false.

If d3cold_allowed is then set to true and the device has runtime
suspended in the meantime, the bridge_d3 attribute of its parent bridges
is *not* reverted back to true as it should. The reason is that when the
device runtime suspended, its no_d3cold attribute was set to false in
pci_pm_runtime_suspend() because d3cold_allowed was false. But no_d3cold
isn't reverted back to true when setting d3cold_allowed to true and
because this attribute is checked in pci_dev_check_d3cold(), the parent
bridges' bridge_d3 attribute remains false.

no_d3cold is just a transient variable used by pci_pm_runtime_suspend()
and acpi_pci_choose_state(), which is indirectly called from it via
pci_finish_runtime_suspend(). One can fix this by reverting no_d3cold
back to false at the end of pci_pm_runtime_suspend(), like this:
https://github.com/l1k/linux/commit/9b9ffb8


> > One detail I'm not sure about: If you've got a hotplug downstream port
> > behind an upstream port and the upstream port goes to D3hot, is it
> > still possible for the downstream port to signal hotplug interrupts?
> > In other words, can INTx or MSI interrupts generated by the downstream
> > bridge still be routed via the upstream bridge if the upstream bridge
> > is in D3hot? I cannot easily test this, I'd have to hack the tg3 driver
> > for my Thunderbolt Ethernet adapter to use runtime suspend.
> 
> If the downstream port is able to trigger wake (PME) from D3cold, I
> think it should work but I'm not 100% sure.

I've been able to test this now with a hacked tg3 driver and it's as
I expected: A hotplug port may go to D3hot and will still generate
interrupts on hotplug, but all its parent ports are *not* allowed
to go to D3hot because otherwise the hotplug interrupts will no longer
come through. The algorithm in pci_bridge_d3_update() and
pci_dev_check_d3cold() needs to be amended to take that into account.
Hm, it's nontrivial I guess, allowing bridge_d3 = true for the lowest
hotplug bridge in a hierarchy but not for anything above?


> > My opinion is that we should strive for maximum power saving, thus try
> > (at least) 2010 and blacklist individual devices as needed.
> 
> IMHO we should strive for working systems and not maximum power saving
> but I'm OK to change that if Bjorn or Rafael are fine with it.

They've kept mum so far. :-)


Best regards,

Lukas

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

[Rafael J. Wysocki]

On Wed, Apr 20, 2016 at 9:22 PM, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Mika,
>
> On Tue, Apr 19, 2016 at 03:31:31PM +0300, Mika Westerberg wrote:
>> On Mon, Apr 18, 2016 at 04:38:23PM +0200, Lukas Wunner wrote:
>> > In case you agree with this, I've prepared a fixup patch which you
>> > can apply with "git rebase --autosquash":
>> > https://github.com/l1k/linux/commit/24fc9d7b34ff
>>
>> Based on the above, I'm going to fold your commit to [4/4]. Are you OK
>> if I add your Signed-off-by to that patch?
>
> Sure.
>
>
>> > Another thing I've spotted but that wasn't needed to get my patches
>> > working: When the bridge_d3 attribute changes to true, you need to
>> > notify the PM core thereof by calling pm_request_idle() for the
>> > bridge device, otherwise the bridge needlessly stays awake. This
>> > needs to be added near the end of pci_bridge_d3_update() I guess.
>>
>> I've been testing this by writing to 'd3cold_allowed' sysfs file and it
>> suspends just fine when it is 1. How do you reproduce this?
>
> Testing with d3cold_allowed is misleading in this case because
> d3cold_allowed_store() already calls pm_runtime_resume(dev),
> and the next time dev runtime suspends, it automatically calls
> rpm_idle() on its parent. This masks that a call to pm_request_idle()
> is currently missing in pci_bridge_d3_update().
>
> However pci_bridge_d3_update() also gets called e.g. from
> pci_remove_bus_device() and there's no call to pm_request_idle()
> there, so a bridge would stay awake even if a child that has blocked
> d3 has been removed.
>
> I only noticed this because I force bridge_d3 = true when loading
> the thunderbolt upstream bridge driver, as a workaround for the BIOS
> cut-off date, and noticed that I had to call pm_request_idle() because
> otherwise the bridges would stay awake.
>
>
> I hadn't played with d3cold_allowed before but tested it now that
> you've mentioned it. Found a bug there:
>
> If d3cold_allowed is set to false for a device via sysfs, the bridge_d3
> attribute of its parent bridges will correctly be updated to false.
>
> If d3cold_allowed is then set to true and the device has runtime
> suspended in the meantime, the bridge_d3 attribute of its parent bridges
> is *not* reverted back to true as it should. The reason is that when the
> device runtime suspended, its no_d3cold attribute was set to false in
> pci_pm_runtime_suspend() because d3cold_allowed was false. But no_d3cold
> isn't reverted back to true when setting d3cold_allowed to true and
> because this attribute is checked in pci_dev_check_d3cold(), the parent
> bridges' bridge_d3 attribute remains false.
>
> no_d3cold is just a transient variable used by pci_pm_runtime_suspend()
> and acpi_pci_choose_state(), which is indirectly called from it via
> pci_finish_runtime_suspend(). One can fix this by reverting no_d3cold
> back to false at the end of pci_pm_runtime_suspend(), like this:
> https://github.com/l1k/linux/commit/9b9ffb8
>
>
>> > One detail I'm not sure about: If you've got a hotplug downstream port
>> > behind an upstream port and the upstream port goes to D3hot, is it
>> > still possible for the downstream port to signal hotplug interrupts?
>> > In other words, can INTx or MSI interrupts generated by the downstream
>> > bridge still be routed via the upstream bridge if the upstream bridge
>> > is in D3hot? I cannot easily test this, I'd have to hack the tg3 driver
>> > for my Thunderbolt Ethernet adapter to use runtime suspend.
>>
>> If the downstream port is able to trigger wake (PME) from D3cold, I
>> think it should work but I'm not 100% sure.
>
> I've been able to test this now with a hacked tg3 driver and it's as
> I expected: A hotplug port may go to D3hot and will still generate
> interrupts on hotplug, but all its parent ports are *not* allowed
> to go to D3hot because otherwise the hotplug interrupts will no longer
> come through. The algorithm in pci_bridge_d3_update() and
> pci_dev_check_d3cold() needs to be amended to take that into account.
> Hm, it's nontrivial I guess, allowing bridge_d3 = true for the lowest
> hotplug bridge in a hierarchy but not for anything above?
>
>
>> > My opinion is that we should strive for maximum power saving, thus try
>> > (at least) 2010 and blacklist individual devices as needed.
>>
>> IMHO we should strive for working systems and not maximum power saving
>> but I'm OK to change that if Bjorn or Rafael are fine with it.
>
> They've kept mum so far. :-)

Breaking any systems that work today is not an option.  if that
happens, the commit that has done that is a clear candidate for
reverting.

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

[Mika Westerberg]

On Wed, Apr 20, 2016 at 10:23:33PM +0200, Rafael J. Wysocki wrote:
> Breaking any systems that work today is not an option.  if that
> happens, the commit that has done that is a clear candidate for
> reverting.

Understood, thanks.

BTW, do you have any preference regarding the cut-off date?

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

[Rafael J. Wysocki]

On Thursday, April 21, 2016 04:12:24 PM Mika Westerberg wrote:
> On Wed, Apr 20, 2016 at 10:23:33PM +0200, Rafael J. Wysocki wrote:
> > Breaking any systems that work today is not an option.  if that
> > happens, the commit that has done that is a clear candidate for
> > reverting.
> 
> Understood, thanks.
> 
> BTW, do you have any preference regarding the cut-off date?

Some time around when machines with Windows 10 started to ship should be
relatively safe.

I guess we can just pick a reasonable date in the initial patch and then
try to move it back to the past subsequently and see if that breaks things
for anyone.

Thanks,
Rafael

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

[Andreas Noever]

On Thu, Apr 21, 2016 at 9:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, April 21, 2016 04:12:24 PM Mika Westerberg wrote:
>> On Wed, Apr 20, 2016 at 10:23:33PM +0200, Rafael J. Wysocki wrote:
>> > Breaking any systems that work today is not an option.  if that
>> > happens, the commit that has done that is a clear candidate for
>> > reverting.
>>
>> Understood, thanks.
>>
>> BTW, do you have any preference regarding the cut-off date?
>
> Some time around when machines with Windows 10 started to ship should be
> relatively safe.
>
> I guess we can just pick a reasonable date in the initial patch and then
> try to move it back to the past subsequently and see if that breaks things
> for anyone.

Maybe add a boot option to overwrite the heuristic (in both
directions) to allow people to test this on older machines and
pinpoint breakage on newer machines?

> Thanks,
> Rafael
>

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

[Rafael J. Wysocki]

On Fri, Apr 22, 2016 at 1:25 AM, Andreas Noever
<andreas.noever@gmail.com> wrote:
> On Thu, Apr 21, 2016 at 9:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Thursday, April 21, 2016 04:12:24 PM Mika Westerberg wrote:
>>> On Wed, Apr 20, 2016 at 10:23:33PM +0200, Rafael J. Wysocki wrote:
>>> > Breaking any systems that work today is not an option.  if that
>>> > happens, the commit that has done that is a clear candidate for
>>> > reverting.
>>>
>>> Understood, thanks.
>>>
>>> BTW, do you have any preference regarding the cut-off date?
>>
>> Some time around when machines with Windows 10 started to ship should be
>> relatively safe.
>>
>> I guess we can just pick a reasonable date in the initial patch and then
>> try to move it back to the past subsequently and see if that breaks things
>> for anyone.
>
> Maybe add a boot option to overwrite the heuristic (in both
> directions) to allow people to test this on older machines and
> pinpoint breakage on newer machines?

Yes, a kernel command line override would be fine by me.

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

[Mika Westerberg]

On Fri, Apr 22, 2016 at 02:26:22AM +0200, Rafael J. Wysocki wrote:
> On Fri, Apr 22, 2016 at 1:25 AM, Andreas Noever
> <andreas.noever@gmail.com> wrote:
> > On Thu, Apr 21, 2016 at 9:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> On Thursday, April 21, 2016 04:12:24 PM Mika Westerberg wrote:
> >>> On Wed, Apr 20, 2016 at 10:23:33PM +0200, Rafael J. Wysocki wrote:
> >>> > Breaking any systems that work today is not an option.  if that
> >>> > happens, the commit that has done that is a clear candidate for
> >>> > reverting.
> >>>
> >>> Understood, thanks.
> >>>
> >>> BTW, do you have any preference regarding the cut-off date?
> >>
> >> Some time around when machines with Windows 10 started to ship should be
> >> relatively safe.
> >>
> >> I guess we can just pick a reasonable date in the initial patch and then
> >> try to move it back to the past subsequently and see if that breaks things
> >> for anyone.
> >
> > Maybe add a boot option to overwrite the heuristic (in both
> > directions) to allow people to test this on older machines and
> > pinpoint breakage on newer machines?
> 
> Yes, a kernel command line override would be fine by me.

OK, so to summarize:

 - Use cut-off date 2015 which is the year Windows 10 was released

 - Add command line parameter that allows this to be overridden.
   Something like:

	pcie_port_pm=   [PCIE] PCIe port power management handling:
		off     Disable power management off all PCIe ports
		force   Forcibly enable power management of all PCIe ports

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

[Rafael J. Wysocki]

On Friday, April 22, 2016 12:10:26 PM Mika Westerberg wrote:
> On Fri, Apr 22, 2016 at 02:26:22AM +0200, Rafael J. Wysocki wrote:
> > On Fri, Apr 22, 2016 at 1:25 AM, Andreas Noever
> > <andreas.noever@gmail.com> wrote:
> > > On Thu, Apr 21, 2016 at 9:19 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >> On Thursday, April 21, 2016 04:12:24 PM Mika Westerberg wrote:
> > >>> On Wed, Apr 20, 2016 at 10:23:33PM +0200, Rafael J. Wysocki wrote:
> > >>> > Breaking any systems that work today is not an option.  if that
> > >>> > happens, the commit that has done that is a clear candidate for
> > >>> > reverting.
> > >>>
> > >>> Understood, thanks.
> > >>>
> > >>> BTW, do you have any preference regarding the cut-off date?
> > >>
> > >> Some time around when machines with Windows 10 started to ship should be
> > >> relatively safe.
> > >>
> > >> I guess we can just pick a reasonable date in the initial patch and then
> > >> try to move it back to the past subsequently and see if that breaks things
> > >> for anyone.
> > >
> > > Maybe add a boot option to overwrite the heuristic (in both
> > > directions) to allow people to test this on older machines and
> > > pinpoint breakage on newer machines?
> > 
> > Yes, a kernel command line override would be fine by me.
> 
> OK, so to summarize:
> 
>  - Use cut-off date 2015 which is the year Windows 10 was released
> 
>  - Add command line parameter that allows this to be overridden.
>    Something like:
> 
> 	pcie_port_pm=   [PCIE] PCIe port power management handling:
> 		off     Disable power management off all PCIe ports
> 		force   Forcibly enable power management of all PCIe ports

Correct, unless Bjorn has a differing opinion.

Bjorn?

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

[Mika Westerberg]

On Wed, Apr 20, 2016 at 09:22:11PM +0200, Lukas Wunner wrote:
> > I've been testing this by writing to 'd3cold_allowed' sysfs file and it
> > suspends just fine when it is 1. How do you reproduce this?
> 
> Testing with d3cold_allowed is misleading in this case because
> d3cold_allowed_store() already calls pm_runtime_resume(dev),
> and the next time dev runtime suspends, it automatically calls
> rpm_idle() on its parent. This masks that a call to pm_request_idle()
> is currently missing in pci_bridge_d3_update().

I don't think pci_bridge_d3_* functions should care about device runtime
PM at all. They are meant to update whether the bridge can go to D3 or
not nothing else.

> However pci_bridge_d3_update() also gets called e.g. from
> pci_remove_bus_device() and there's no call to pm_request_idle()
> there, so a bridge would stay awake even if a child that has blocked
> d3 has been removed.

As far as I can tell removing a device ends up calling
__device_release_driver() where runtime PM is updated accordingly. That
should trigger the parent device (upstream bridge) to runtime suspend if
there is no more active children.

> I only noticed this because I force bridge_d3 = true when loading
> the thunderbolt upstream bridge driver, as a workaround for the BIOS
> cut-off date, and noticed that I had to call pm_request_idle() because
> otherwise the bridges would stay awake.

Well, that's probably the right thing to do. bridge_d3 is about whether
the bridge can go to D3, not if it should runtime suspend right now.

> I hadn't played with d3cold_allowed before but tested it now that
> you've mentioned it. Found a bug there:
> 
> If d3cold_allowed is set to false for a device via sysfs, the bridge_d3
> attribute of its parent bridges will correctly be updated to false.
> 
> If d3cold_allowed is then set to true and the device has runtime
> suspended in the meantime, the bridge_d3 attribute of its parent bridges
> is *not* reverted back to true as it should. The reason is that when the
> device runtime suspended, its no_d3cold attribute was set to false in
> pci_pm_runtime_suspend() because d3cold_allowed was false. But no_d3cold
> isn't reverted back to true when setting d3cold_allowed to true and
> because this attribute is checked in pci_dev_check_d3cold(), the parent
> bridges' bridge_d3 attribute remains false.

Yes, you are right.

> no_d3cold is just a transient variable used by pci_pm_runtime_suspend()
> and acpi_pci_choose_state(), which is indirectly called from it via
> pci_finish_runtime_suspend(). One can fix this by reverting no_d3cold
> back to false at the end of pci_pm_runtime_suspend(), like this:
> https://github.com/l1k/linux/commit/9b9ffb8

That works or alternatively we could make no_d3cold sticky by making all
changes go through pci_d3cold_enable()/disable(). That way upstream
bridges ->bridge_d3 should be in always in sync.

> > > One detail I'm not sure about: If you've got a hotplug downstream port
> > > behind an upstream port and the upstream port goes to D3hot, is it
> > > still possible for the downstream port to signal hotplug interrupts?
> > > In other words, can INTx or MSI interrupts generated by the downstream
> > > bridge still be routed via the upstream bridge if the upstream bridge
> > > is in D3hot? I cannot easily test this, I'd have to hack the tg3 driver
> > > for my Thunderbolt Ethernet adapter to use runtime suspend.
> > 
> > If the downstream port is able to trigger wake (PME) from D3cold, I
> > think it should work but I'm not 100% sure.
> 
> I've been able to test this now with a hacked tg3 driver and it's as
> I expected: A hotplug port may go to D3hot and will still generate
> interrupts on hotplug, but all its parent ports are *not* allowed
> to go to D3hot because otherwise the hotplug interrupts will no longer
> come through.

Interrupts are not possible from any other state than D0 so it is always
PME that gets sent upstream.

Once you move parent port of that downstream port to D3hot it means that
the downstream port is, in fact in D3cold and the link may be in L2 or
L3 (depending on the platform). So a hotplug capable port must be able
to trigger PME from D3cold and you need to enable that as well.

> The algorithm in pci_bridge_d3_update() and pci_dev_check_d3cold()
> needs to be amended to take that into account. Hm, it's nontrivial I
> guess, allowing bridge_d3 = true for the lowest hotplug bridge in a
> hierarchy but not for anything above?

If we need to do things like that, it will get pretty complex and we
still cannot be sure whether hotplug works. I think it is safer to go
back to what I already had and disable runtime PM from such ports.

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

[Lukas Wunner]

Hi Mika,

On Thu, Apr 21, 2016 at 04:10:02PM +0300, Mika Westerberg wrote:
> On Wed, Apr 20, 2016 at 09:22:11PM +0200, Lukas Wunner wrote:
> > However pci_bridge_d3_update() also gets called e.g. from
> > pci_remove_bus_device() and there's no call to pm_request_idle()
> > there, so a bridge would stay awake even if a child that has blocked
> > d3 has been removed.
> 
> As far as I can tell removing a device ends up calling
> __device_release_driver() where runtime PM is updated accordingly. That
> should trigger the parent device (upstream bridge) to runtime suspend if
> there is no more active children.

Right, makes sense.


> > I've been able to test this now with a hacked tg3 driver and it's as
> > I expected: A hotplug port may go to D3hot and will still generate
> > interrupts on hotplug, but all its parent ports are *not* allowed
> > to go to D3hot because otherwise the hotplug interrupts will no longer
> > come through.
> 
> Interrupts are not possible from any other state than D0 so it is always
> PME that gets sent upstream.
> 
> Once you move parent port of that downstream port to D3hot it means that
> the downstream port is, in fact in D3cold and the link may be in L2 or
> L3 (depending on the platform). So a hotplug capable port must be able
> to trigger PME from D3cold and you need to enable that as well.

The PME WAKE# pin of Thunderbolt controllers built into Macs is wired
to the southbridge so that it wakes the entire system from sleep.
IIUC for the downstream port to deliver a side-band interrupt to
the root port so that the root port resumes from D3, WAKE# would have
to be wired to the root complex.

In the case of Thunderbolt (as compared to CardBus or whatever),
there's a separate wake pin on the controller which signals such
a side-band interrupt on hotplug, on Macs it's delivered as a GPE.


> > The algorithm in pci_bridge_d3_update() and pci_dev_check_d3cold()
> > needs to be amended to take that into account. Hm, it's nontrivial I
> > guess, allowing bridge_d3 = true for the lowest hotplug bridge in a
> > hierarchy but not for anything above?
> 
> If we need to do things like that, it will get pretty complex and we
> still cannot be sure whether hotplug works. I think it is safer to go
> back to what I already had and disable runtime PM from such ports.

Okay, maybe I'll just solve this by allowing D3 for all PCIe ports
that belong to a Thunderbolt device if DMI says we're on a Mac.

Best regards,

Lukas