Rescanning is broken with runtime PM for PCIe ports

Rescanning is broken with runtime PM for PCIe ports

[Peter Wu]

Hi,

While testing the pci/pm tree from Bjorn with HEAD being 0195d2813547
("PCI: Add runtime PM support for PCIe ports"), I have noticed that
detaching and rescanning is broken.

When a bridgr is in D3 state, it cannot discover children. Reproducer:

    echo > /sys/bus/pci/devices/0000:01:00.0/remove 1
    # wait for the PCIe port to enter D3cold
    echo > /sys/bus/pci/devices/0000:00:01.0/rescan 1
    # Workaround to get the device back
    echo > /sys/bus/pci/devices/0000:00:01.0/power/control on
    echo > /sys/bus/pci/devices/0000:00:01.0/rescan 1

lspci:

    00:01.0 PCI bridge [0604]: Intel Corporation Skylake PCIe Controller (x16) [8086:1901] (rev 07)
    01:00.0 VGA compatible controller [0300]: NVIDIA Corporation GM204M [GeForce GTX 965M] [10de:13d9] (rev a1)

Probably needs a pm_runtime_{get,put}_sync in pci_rescan_bus and
pci_rescan_bus_bridge_resize.
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl

Re: Rescanning is broken with runtime PM for PCIe ports

[Mika Westerberg]

On Wed, May 18, 2016 at 07:14:01PM +0200, Peter Wu wrote:
> Hi,
> 
> While testing the pci/pm tree from Bjorn with HEAD being 0195d2813547
> ("PCI: Add runtime PM support for PCIe ports"), I have noticed that
> detaching and rescanning is broken.
> 
> When a bridgr is in D3 state, it cannot discover children. Reproducer:
> 
>     echo > /sys/bus/pci/devices/0000:01:00.0/remove 1
>     # wait for the PCIe port to enter D3cold
>     echo > /sys/bus/pci/devices/0000:00:01.0/rescan 1
>     # Workaround to get the device back
>     echo > /sys/bus/pci/devices/0000:00:01.0/power/control on
>     echo > /sys/bus/pci/devices/0000:00:01.0/rescan 1
> 
> lspci:
> 
>     00:01.0 PCI bridge [0604]: Intel Corporation Skylake PCIe Controller (x16) [8086:1901] (rev 07)
>     01:00.0 VGA compatible controller [0300]: NVIDIA Corporation GM204M [GeForce GTX 965M] [10de:13d9] (rev a1)
> 
> Probably needs a pm_runtime_{get,put}_sync in pci_rescan_bus and
> pci_rescan_bus_bridge_resize.

Thanks for reporting. Let me investigate this a bit.

Re: Rescanning is broken with runtime PM for PCIe ports

[Mika Westerberg]

On Thu, May 19, 2016 at 10:42:31AM +0300, Mika Westerberg wrote:
> On Wed, May 18, 2016 at 07:14:01PM +0200, Peter Wu wrote:
> > Hi,
> > 
> > While testing the pci/pm tree from Bjorn with HEAD being 0195d2813547
> > ("PCI: Add runtime PM support for PCIe ports"), I have noticed that
> > detaching and rescanning is broken.
> > 
> > When a bridgr is in D3 state, it cannot discover children. Reproducer:
> > 
> >     echo > /sys/bus/pci/devices/0000:01:00.0/remove 1
> >     # wait for the PCIe port to enter D3cold
> >     echo > /sys/bus/pci/devices/0000:00:01.0/rescan 1
> >     # Workaround to get the device back
> >     echo > /sys/bus/pci/devices/0000:00:01.0/power/control on
> >     echo > /sys/bus/pci/devices/0000:00:01.0/rescan 1
> > 
> > lspci:
> > 
> >     00:01.0 PCI bridge [0604]: Intel Corporation Skylake PCIe Controller (x16) [8086:1901] (rev 07)
> >     01:00.0 VGA compatible controller [0300]: NVIDIA Corporation GM204M [GeForce GTX 965M] [10de:13d9] (rev a1)
> > 
> > Probably needs a pm_runtime_{get,put}_sync in pci_rescan_bus and
> > pci_rescan_bus_bridge_resize.
> 
> Thanks for reporting. Let me investigate this a bit.

I think it is enough if we make sure the bridge is powered when it is
being scanned. Can you try if the below patch works for you?

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8004f67c57ec..15e77c92311e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -16,6 +16,7 @@
 #include <linux/aer.h>
 #include <linux/acpi.h>
 #include <linux/irqdomain.h>
+#include <linux/pm_runtime.h>
 #include "pci.h"
 
 #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
@@ -832,6 +833,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 	u8 primary, secondary, subordinate;
 	int broken = 0;
 
+	/*
+	 * Make sure the bridge is powered on to be able to access config
+	 * space of devices below it.
+	 */
+	pm_runtime_get_sync(&dev->dev);
+
 	pci_read_config_dword(dev, PCI_PRIMARY_BUS, &buses);
 	primary = buses & 0xFF;
 	secondary = (buses >> 8) & 0xFF;
@@ -1012,6 +1019,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 out:
 	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl);
 
+	pm_runtime_put(&dev->dev);
+
 	return max;
 }
 EXPORT_SYMBOL(pci_scan_bridge);

Re: Rescanning is broken with runtime PM for PCIe ports

[Peter Wu]

On Thu, May 19, 2016 at 02:36:30PM +0300, Mika Westerberg wrote:
> On Thu, May 19, 2016 at 10:42:31AM +0300, Mika Westerberg wrote:
> > On Wed, May 18, 2016 at 07:14:01PM +0200, Peter Wu wrote:
> > > Hi,
> > > 
> > > While testing the pci/pm tree from Bjorn with HEAD being 0195d2813547
> > > ("PCI: Add runtime PM support for PCIe ports"), I have noticed that
> > > detaching and rescanning is broken.
> > > 
> > > When a bridgr is in D3 state, it cannot discover children. Reproducer:
> > > 
> > >     echo > /sys/bus/pci/devices/0000:01:00.0/remove 1
> > >     # wait for the PCIe port to enter D3cold
> > >     echo > /sys/bus/pci/devices/0000:00:01.0/rescan 1
> > >     # Workaround to get the device back
> > >     echo > /sys/bus/pci/devices/0000:00:01.0/power/control on
> > >     echo > /sys/bus/pci/devices/0000:00:01.0/rescan 1
> > > 
> > > lspci:
> > > 
> > >     00:01.0 PCI bridge [0604]: Intel Corporation Skylake PCIe Controller (x16) [8086:1901] (rev 07)
> > >     01:00.0 VGA compatible controller [0300]: NVIDIA Corporation GM204M [GeForce GTX 965M] [10de:13d9] (rev a1)
> > > 
> > > Probably needs a pm_runtime_{get,put}_sync in pci_rescan_bus and
> > > pci_rescan_bus_bridge_resize.
> > 
> > Thanks for reporting. Let me investigate this a bit.
> 
> I think it is enough if we make sure the bridge is powered when it is
> being scanned. Can you try if the below patch works for you?
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8004f67c57ec..15e77c92311e 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -16,6 +16,7 @@
>  #include <linux/aer.h>
>  #include <linux/acpi.h>
>  #include <linux/irqdomain.h>
> +#include <linux/pm_runtime.h>
>  #include "pci.h"
>  
>  #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
> @@ -832,6 +833,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
>  	u8 primary, secondary, subordinate;
>  	int broken = 0;
>  
> +	/*
> +	 * Make sure the bridge is powered on to be able to access config
> +	 * space of devices below it.
> +	 */
> +	pm_runtime_get_sync(&dev->dev);
> +
>  	pci_read_config_dword(dev, PCI_PRIMARY_BUS, &buses);
>  	primary = buses & 0xFF;
>  	secondary = (buses >> 8) & 0xFF;
> @@ -1012,6 +1019,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
>  out:
>  	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl);
>  
> +	pm_runtime_put(&dev->dev);
> +
>  	return max;
>  }
>  EXPORT_SYMBOL(pci_scan_bridge);

Works for me on top of v4.6-rc7-216-g021351f.

May 20 01:08:27.340318 localhost kernel: pci 0000:01:00.0: PME# disabled
May 20 01:08:27.340589 localhost kernel: vgaarb: device changed decodes: PCI:0000:00:02.0,olddecodes=none,decodes=io+mem:owns=io+mem
May 20 01:08:27.340755 localhost kernel: pcieport 0000:00:01.0: PME# enabled
May 20 01:08:27.388476 localhost kernel: pcieport 0000:00:01.0: power state changed by ACPI to D3cold
May 20 01:08:35.884288 localhost kernel: pci_bus 0000:00: scanning bus
May 20 01:08:36.033274 localhost kernel: pcieport 0000:00:01.0: power state changed by ACPI to D0
May 20 01:08:36.135063 localhost kernel: pcieport 0000:00:01.0: PME# disabled
May 20 01:08:36.136529 localhost kernel: pcieport 0000:00:01.0: scanning [bus 01-01] behind bridge, pass 0
May 20 01:08:36.136641 localhost kernel: pci_bus 0000:01: scanning bus
May 20 01:08:36.138618 localhost kernel: pci 0000:01:00.0: [10de:13d9] type 00 class 0x030000
May 20 01:08:36.138661 localhost kernel: pci 0000:01:00.0: reg 0x10: [mem 0x00000000-0x00ffffff]
May 20 01:08:36.138693 localhost kernel: pci 0000:01:00.0: reg 0x14: [mem 0x00000000-0x0fffffff 64bit pref]
May 20 01:08:36.139226 localhost kernel: pci 0000:01:00.0: reg 0x1c: [mem 0x00000000-0x01ffffff 64bit pref]
May 20 01:08:36.140360 localhost kernel: pci 0000:01:00.0: reg 0x24: [io  0x0000-0x007f]
May 20 01:08:36.140384 localhost kernel: pci 0000:01:00.0: reg 0x30: [mem 0x00000000-0x0007ffff pref]
May 20 01:08:36.141517 localhost kernel: pci 0000:01:00.0: Max Payload Size set to 256 (was 128, max 256)
May 20 01:08:36.152493 localhost kernel: pci 0000:01:00.0: System wakeup disabled by ACPI
May 20 01:08:36.153654 localhost kernel: vgaarb: device added: PCI:0000:01:00.0,decodes=io+mem,owns=none,locks=none
May 20 01:08:36.153681 localhost kernel: vgaarb: device changed decodes: PCI:0000:00:02.0,olddecodes=io+mem,decodes=none:owns=io+mem
May 20 01:08:36.157541 localhost kernel: pci_bus 0000:01: bus scan returning with max=01
May 20 01:08:36.157646 localhost kernel: pcieport 0000:00:1c.0: scanning [bus 02-02] behind bridge, pass 0
May 20 01:08:36.157730 localhost kernel: pci_bus 0000:02: scanning bus
May 20 01:08:36.158865 localhost kernel: pci_bus 0000:02: bus scan returning with max=02
May 20 01:08:36.158964 localhost kernel: pcieport 0000:00:1c.5: scanning [bus 03-03] behind bridge, pass 0
May 20 01:08:36.159567 localhost kernel: pci_bus 0000:03: scanning bus
May 20 01:08:36.160807 localhost kernel: pci_bus 0000:03: bus scan returning with max=03
May 20 01:08:36.160901 localhost kernel: pcieport 0000:00:01.0: scanning [bus 01-01] behind bridge, pass 1
May 20 01:08:36.161527 localhost kernel: pcieport 0000:00:1c.0: scanning [bus 02-02] behind bridge, pass 1
May 20 01:08:36.162753 localhost kernel: pcieport 0000:00:1c.5: scanning [bus 03-03] behind bridge, pass 1
May 20 01:08:36.162832 localhost kernel: pci_bus 0000:00: bus scan returning with max=03
May 20 01:08:36.164654 localhost kernel: pci 0000:01:00.0: BAR 1: assigned [mem 0xc0000000-0xcfffffff 64bit pref]
May 20 01:08:36.164762 localhost kernel: pci 0000:01:00.0: BAR 3: assigned [mem 0xd0000000-0xd1ffffff 64bit pref]
May 20 01:08:36.164847 localhost kernel: pci 0000:01:00.0: BAR 0: assigned [mem 0xde000000-0xdeffffff]
May 20 01:08:36.165284 localhost kernel: pci 0000:01:00.0: BAR 6: assigned [mem 0xdf000000-0xdf07ffff pref]
May 20 01:08:36.166515 localhost kernel: pci 0000:01:00.0: BAR 5: assigned [io  0xe000-0xe07f]
May 20 01:08:36.166612 localhost kernel: pci 0000:01:00.0: calling nv_msi_ht_cap_quirk_leaf+0x0/0x20
May 20 01:08:36.167799 localhost kernel: pci 0000:01:00.0: calling pci_fixup_video+0x0/0x100
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl

[PATCH] PCI: Power on bridges before scanning new devices

[Mika Westerberg]

When a PCI device is removed through sysfs interface the upstream bridge
(PCIe port) can be runtime suspended if it was the last device on that bus.
Now, if the bridge is in D3 we cannot find devices below the bridge
anymore. For example following fails to find the removed device again:

   # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove
   # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan

Where 0000:00:01.0 is the bridge device.

In order to be able to rescan devices below the bridge add
pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This
should keep bridges powered on while their children devices are being
scanned.

Reported-by: Peter Wu <peter@lekensteyn.nl>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/probe.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8004f67c57ec..15e77c92311e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -16,6 +16,7 @@
 #include <linux/aer.h>
 #include <linux/acpi.h>
 #include <linux/irqdomain.h>
+#include <linux/pm_runtime.h>
 #include "pci.h"
 
 #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
@@ -832,6 +833,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 	u8 primary, secondary, subordinate;
 	int broken = 0;
 
+	/*
+	 * Make sure the bridge is powered on to be able to access config
+	 * space of devices below it.
+	 */
+	pm_runtime_get_sync(&dev->dev);
+
 	pci_read_config_dword(dev, PCI_PRIMARY_BUS, &buses);
 	primary = buses & 0xFF;
 	secondary = (buses >> 8) & 0xFF;
@@ -1012,6 +1019,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 out:
 	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl);
 
+	pm_runtime_put(&dev->dev);
+
 	return max;
 }
 EXPORT_SYMBOL(pci_scan_bridge);
-- 
2.8.1

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Bjorn Helgaas]

On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote:
> When a PCI device is removed through sysfs interface the upstream bridge
> (PCIe port) can be runtime suspended if it was the last device on that bus.
> Now, if the bridge is in D3 we cannot find devices below the bridge
> anymore. For example following fails to find the removed device again:
> 
>    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove
>    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan
> 
> Where 0000:00:01.0 is the bridge device.
> 
> In order to be able to rescan devices below the bridge add
> pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This
> should keep bridges powered on while their children devices are being
> scanned.
> 
> Reported-by: Peter Wu <peter@lekensteyn.nl>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

This looks like basically the same idea as "ACPI / hotplug / PCI:
Runtime resume bridge before rescan".

The hotplug_event() path modified by that patch eventually calls
pci_scan_bridge():

  hotplug_event
    enable_slot
      pci_scan_bridge

so this patch looks a little more general.  Does it make "ACPI /
hotplug / PCI: Runtime resume bridge before rescan" unnecessary?  
Can I just replace that patch with this one?

> ---
>  drivers/pci/probe.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8004f67c57ec..15e77c92311e 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -16,6 +16,7 @@
>  #include <linux/aer.h>
>  #include <linux/acpi.h>
>  #include <linux/irqdomain.h>
> +#include <linux/pm_runtime.h>
>  #include "pci.h"
>  
>  #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
> @@ -832,6 +833,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
>  	u8 primary, secondary, subordinate;
>  	int broken = 0;
>  
> +	/*
> +	 * Make sure the bridge is powered on to be able to access config
> +	 * space of devices below it.
> +	 */
> +	pm_runtime_get_sync(&dev->dev);
> +
>  	pci_read_config_dword(dev, PCI_PRIMARY_BUS, &buses);
>  	primary = buses & 0xFF;
>  	secondary = (buses >> 8) & 0xFF;
> @@ -1012,6 +1019,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
>  out:
>  	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl);
>  
> +	pm_runtime_put(&dev->dev);
> +
>  	return max;
>  }
>  EXPORT_SYMBOL(pci_scan_bridge);
> -- 
> 2.8.1
> 
> --
> 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] PCI: Power on bridges before scanning new devices

[Bjorn Helgaas]

[+cc Valdis, Dave]

On Mon, May 23, 2016 at 03:00:42PM -0500, Bjorn Helgaas wrote:
> On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote:
> > When a PCI device is removed through sysfs interface the upstream bridge
> > (PCIe port) can be runtime suspended if it was the last device on that bus.
> > Now, if the bridge is in D3 we cannot find devices below the bridge
> > anymore. For example following fails to find the removed device again:
> > 
> >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove
> >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan
> > 
> > Where 0000:00:01.0 is the bridge device.
> > 
> > In order to be able to rescan devices below the bridge add
> > pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This
> > should keep bridges powered on while their children devices are being
> > scanned.
> > 
> > Reported-by: Peter Wu <peter@lekensteyn.nl>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> This looks like basically the same idea as "ACPI / hotplug / PCI:
> Runtime resume bridge before rescan".
> 
> The hotplug_event() path modified by that patch eventually calls
> pci_scan_bridge():
> 
>   hotplug_event
>     enable_slot
>       pci_scan_bridge
> 
> so this patch looks a little more general.  Does it make "ACPI /
> hotplug / PCI: Runtime resume bridge before rescan" unnecessary?  
> Can I just replace that patch with this one?

I speculatively replaced "ACPI / hotplug / PCI: Runtime resume bridge
before rescan" with this one and pushed the result to

  https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/pm

Please take a look, test it, and let me know if I need to add the ACPI
patch back.

This branch also includes the fix for the lockdep splat reported by
Valdis.  This is what I hope to get into v4.7-rc1.

> > ---
> >  drivers/pci/probe.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 8004f67c57ec..15e77c92311e 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/aer.h>
> >  #include <linux/acpi.h>
> >  #include <linux/irqdomain.h>
> > +#include <linux/pm_runtime.h>
> >  #include "pci.h"
> >  
> >  #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
> > @@ -832,6 +833,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
> >  	u8 primary, secondary, subordinate;
> >  	int broken = 0;
> >  
> > +	/*
> > +	 * Make sure the bridge is powered on to be able to access config
> > +	 * space of devices below it.
> > +	 */
> > +	pm_runtime_get_sync(&dev->dev);
> > +
> >  	pci_read_config_dword(dev, PCI_PRIMARY_BUS, &buses);
> >  	primary = buses & 0xFF;
> >  	secondary = (buses >> 8) & 0xFF;
> > @@ -1012,6 +1019,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
> >  out:
> >  	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl);
> >  
> > +	pm_runtime_put(&dev->dev);
> > +
> >  	return max;
> >  }
> >  EXPORT_SYMBOL(pci_scan_bridge);
> > -- 
> > 2.8.1
> > 
> > --
> > 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
> --
> 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] PCI: Power on bridges before scanning new devices

[Bjorn Helgaas]

On Mon, May 23, 2016 at 04:50:15PM -0500, Bjorn Helgaas wrote:
> [+cc Valdis, Dave]
> 
> On Mon, May 23, 2016 at 03:00:42PM -0500, Bjorn Helgaas wrote:
> > On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote:
> > > When a PCI device is removed through sysfs interface the upstream bridge
> > > (PCIe port) can be runtime suspended if it was the last device on that bus.
> > > Now, if the bridge is in D3 we cannot find devices below the bridge
> > > anymore. For example following fails to find the removed device again:
> > > 
> > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove
> > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan
> > > 
> > > Where 0000:00:01.0 is the bridge device.
> > > 
> > > In order to be able to rescan devices below the bridge add
> > > pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This
> > > should keep bridges powered on while their children devices are being
> > > scanned.
> > > 
> > > Reported-by: Peter Wu <peter@lekensteyn.nl>
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > 
> > This looks like basically the same idea as "ACPI / hotplug / PCI:
> > Runtime resume bridge before rescan".
> > 
> > The hotplug_event() path modified by that patch eventually calls
> > pci_scan_bridge():
> > 
> >   hotplug_event
> >     enable_slot
> >       pci_scan_bridge
> > 
> > so this patch looks a little more general.  Does it make "ACPI /
> > hotplug / PCI: Runtime resume bridge before rescan" unnecessary?  
> > Can I just replace that patch with this one?
> 
> I speculatively replaced "ACPI / hotplug / PCI: Runtime resume bridge
> before rescan" with this one and pushed the result to
> 
>   https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/pm
> 
> Please take a look, test it, and let me know if I need to add the ACPI
> patch back.
> 
> This branch also includes the fix for the lockdep splat reported by
> Valdis.  This is what I hope to get into v4.7-rc1.

Ping?  I'd like to ask Linus to pull this pci/pm branch before v4.7-rc1.
It currently has these changes:

  8b71f5652eea PCI: Add runtime PM support for PCIe ports
  af81f0fa638b PCI: Power on bridges before scanning new devices
  9741a01c9f55 PCI: Put PCIe ports into D3 during suspend
  b3a63ff7baf1 PCI: Don't clear d3cold_allowed for PCIe ports

I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan"
on the assumption that "PCI: Power on bridges before scanning new
devices" is sufficient to cover both the ACPI and the generic PCi
rescan cases, but I'd like some reassurance about that.

Lukas, you tested that acpiphp patch.  Would you be able to verify
that your test case still works even with that patch replaced by the
generic one?

FWIW, my pci/pm branch with these updates is included in
next-20160524.

> > > ---
> > >  drivers/pci/probe.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 8004f67c57ec..15e77c92311e 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -16,6 +16,7 @@
> > >  #include <linux/aer.h>
> > >  #include <linux/acpi.h>
> > >  #include <linux/irqdomain.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include "pci.h"
> > >  
> > >  #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
> > > @@ -832,6 +833,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
> > >  	u8 primary, secondary, subordinate;
> > >  	int broken = 0;
> > >  
> > > +	/*
> > > +	 * Make sure the bridge is powered on to be able to access config
> > > +	 * space of devices below it.
> > > +	 */
> > > +	pm_runtime_get_sync(&dev->dev);
> > > +
> > >  	pci_read_config_dword(dev, PCI_PRIMARY_BUS, &buses);
> > >  	primary = buses & 0xFF;
> > >  	secondary = (buses >> 8) & 0xFF;
> > > @@ -1012,6 +1019,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
> > >  out:
> > >  	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl);
> > >  
> > > +	pm_runtime_put(&dev->dev);
> > > +
> > >  	return max;
> > >  }
> > >  EXPORT_SYMBOL(pci_scan_bridge);
> > > -- 
> > > 2.8.1
> > > 
> > > --
> > > 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
> > --
> > 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
> --
> 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] PCI: Power on bridges before scanning new devices

[Lukas Wunner]

Hi Bjorn,

On Tue, May 24, 2016 at 07:23:57AM -0500, Bjorn Helgaas wrote:
> On Mon, May 23, 2016 at 04:50:15PM -0500, Bjorn Helgaas wrote:
> > [+cc Valdis, Dave]
> > 
> > On Mon, May 23, 2016 at 03:00:42PM -0500, Bjorn Helgaas wrote:
> > > On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote:
> > > > When a PCI device is removed through sysfs interface the upstream bridge
> > > > (PCIe port) can be runtime suspended if it was the last device on that bus.
> > > > Now, if the bridge is in D3 we cannot find devices below the bridge
> > > > anymore. For example following fails to find the removed device again:
> > > > 
> > > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove
> > > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan
> > > > 
> > > > Where 0000:00:01.0 is the bridge device.
> > > > 
> > > > In order to be able to rescan devices below the bridge add
> > > > pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This
> > > > should keep bridges powered on while their children devices are being
> > > > scanned.
> > > > 
> > > > Reported-by: Peter Wu <peter@lekensteyn.nl>
> > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > 
> > > This looks like basically the same idea as "ACPI / hotplug / PCI:
> > > Runtime resume bridge before rescan".
> > > 
> > > The hotplug_event() path modified by that patch eventually calls
> > > pci_scan_bridge():
> > > 
> > >   hotplug_event
> > >     enable_slot
> > >       pci_scan_bridge
> > > 
> > > so this patch looks a little more general.  Does it make "ACPI /
> > > hotplug / PCI: Runtime resume bridge before rescan" unnecessary?  
> > > Can I just replace that patch with this one?
> > 
> > I speculatively replaced "ACPI / hotplug / PCI: Runtime resume bridge
> > before rescan" with this one and pushed the result to
> > 
> >   https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/pm
> > 
> > Please take a look, test it, and let me know if I need to add the ACPI
> > patch back.
> > 
> > This branch also includes the fix for the lockdep splat reported by
> > Valdis.  This is what I hope to get into v4.7-rc1.
> 
> Ping?  I'd like to ask Linus to pull this pci/pm branch before v4.7-rc1.
> It currently has these changes:
> 
>   8b71f5652eea PCI: Add runtime PM support for PCIe ports
>   af81f0fa638b PCI: Power on bridges before scanning new devices
>   9741a01c9f55 PCI: Put PCIe ports into D3 during suspend
>   b3a63ff7baf1 PCI: Don't clear d3cold_allowed for PCIe ports
> 
> I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan"
> on the assumption that "PCI: Power on bridges before scanning new
> devices" is sufficient to cover both the ACPI and the generic PCi
> rescan cases, but I'd like some reassurance about that.
> 
> Lukas, you tested that acpiphp patch.  Would you be able to verify
> that your test case still works even with that patch replaced by the
> generic one?

I only have a Mac and Macs don't use ACPI-based hotplug, only native
pciehp hotplug. So unfortunately I can't say anything about whether
the ACPI patch is still needed. For Dave Airlie's use case the patch
seems not to be needed, he wants to suspend root ports to which a GPU
is attached, so I guess he doesn't care much about hotplug. The patch
could therefore be pulled by Linus separately, if it turns that it's
still needed.

Thanks,

Lukas

> 
> FWIW, my pci/pm branch with these updates is included in
> next-20160524.
> 
> > > > ---
> > > >  drivers/pci/probe.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > index 8004f67c57ec..15e77c92311e 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -16,6 +16,7 @@
> > > >  #include <linux/aer.h>
> > > >  #include <linux/acpi.h>
> > > >  #include <linux/irqdomain.h>
> > > > +#include <linux/pm_runtime.h>
> > > >  #include "pci.h"
> > > >  
> > > >  #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
> > > > @@ -832,6 +833,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
> > > >  	u8 primary, secondary, subordinate;
> > > >  	int broken = 0;
> > > >  
> > > > +	/*
> > > > +	 * Make sure the bridge is powered on to be able to access config
> > > > +	 * space of devices below it.
> > > > +	 */
> > > > +	pm_runtime_get_sync(&dev->dev);
> > > > +
> > > >  	pci_read_config_dword(dev, PCI_PRIMARY_BUS, &buses);
> > > >  	primary = buses & 0xFF;
> > > >  	secondary = (buses >> 8) & 0xFF;
> > > > @@ -1012,6 +1019,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
> > > >  out:
> > > >  	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl);
> > > >  
> > > > +	pm_runtime_put(&dev->dev);
> > > > +
> > > >  	return max;
> > > >  }
> > > >  EXPORT_SYMBOL(pci_scan_bridge);
> > > > -- 
> > > > 2.8.1
> > > > 
> > > > --
> > > > 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
> > > --
> > > 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
> > --
> > 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] PCI: Power on bridges before scanning new devices

[Mika Westerberg]

On Tue, May 24, 2016 at 07:23:57AM -0500, Bjorn Helgaas wrote:
> On Mon, May 23, 2016 at 04:50:15PM -0500, Bjorn Helgaas wrote:
> > [+cc Valdis, Dave]
> > 
> > On Mon, May 23, 2016 at 03:00:42PM -0500, Bjorn Helgaas wrote:
> > > On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote:
> > > > When a PCI device is removed through sysfs interface the upstream bridge
> > > > (PCIe port) can be runtime suspended if it was the last device on that bus.
> > > > Now, if the bridge is in D3 we cannot find devices below the bridge
> > > > anymore. For example following fails to find the removed device again:
> > > > 
> > > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove
> > > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan
> > > > 
> > > > Where 0000:00:01.0 is the bridge device.
> > > > 
> > > > In order to be able to rescan devices below the bridge add
> > > > pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This
> > > > should keep bridges powered on while their children devices are being
> > > > scanned.
> > > > 
> > > > Reported-by: Peter Wu <peter@lekensteyn.nl>
> > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > 
> > > This looks like basically the same idea as "ACPI / hotplug / PCI:
> > > Runtime resume bridge before rescan".
> > > 
> > > The hotplug_event() path modified by that patch eventually calls
> > > pci_scan_bridge():
> > > 
> > >   hotplug_event
> > >     enable_slot
> > >       pci_scan_bridge
> > > 
> > > so this patch looks a little more general.  Does it make "ACPI /
> > > hotplug / PCI: Runtime resume bridge before rescan" unnecessary?  
> > > Can I just replace that patch with this one?
> > 
> > I speculatively replaced "ACPI / hotplug / PCI: Runtime resume bridge
> > before rescan" with this one and pushed the result to
> > 
> >   https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/pm
> > 
> > Please take a look, test it, and let me know if I need to add the ACPI
> > patch back.
> > 
> > This branch also includes the fix for the lockdep splat reported by
> > Valdis.  This is what I hope to get into v4.7-rc1.
> 
> Ping?  I'd like to ask Linus to pull this pci/pm branch before v4.7-rc1.
> It currently has these changes:
> 
>   8b71f5652eea PCI: Add runtime PM support for PCIe ports
>   af81f0fa638b PCI: Power on bridges before scanning new devices
>   9741a01c9f55 PCI: Put PCIe ports into D3 during suspend
>   b3a63ff7baf1 PCI: Don't clear d3cold_allowed for PCIe ports

Looks good to me. I've also tested those here and seems to work fine.

> I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan"
> on the assumption that "PCI: Power on bridges before scanning new
> devices" is sufficient to cover both the ACPI and the generic PCi
> rescan cases, but I'd like some reassurance about that.

I agree with your reasoning that the patch should not be needed anymore.
However, I have the machine which needed that patch at home so I'm not
able to test it now. I'll do that later today when I get back home.

One thing I noticed, though. When a bridge is transitioned to D0 we only
wait for 10ms which is requirement for PCI functions. However, PCI PM
specification 1.2 (chapter 4.2) requires that for buses to transition
from B2 to B0 we need to wait minimum of 50ms before accessing a
function on that bus.

We even have PCI_PM_BUS_WAIT defined in include/linux/pci.h but it is
not used anywhere. Maybe it was not needed originally because we never
powered down bridges anyway but now when we do, I think it is good idea
to do what the spec requires.

What do you think? We could add a separate patch doing something like
below to make sure the spec is followed.

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e785dc260e72..b3b794caa380 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2361,7 +2361,12 @@ void pci_pm_init(struct pci_dev *dev)
 	}
 
 	dev->pm_cap = pm;
-	dev->d3_delay = PCI_PM_D3_WAIT;
+	/*
+	 * PCI PM 1.2 specification requires minimum of 50ms before any
+	 * function on the bus is accessed after the bus is transitioned
+	 * from B2 to B0.
+	 */
+	dev->d3_delay = pci_is_bridge(dev) ? PCI_PM_BUS_WAIT : PCI_PM_D3_WAIT;
 	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
 	dev->bridge_d3 = pci_bridge_d3_possible(dev);
 	dev->d3cold_allowed = true;

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Peter Wu]

On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote:
> On Tue, May 24, 2016 at 07:23:57AM -0500, Bjorn Helgaas wrote:
> > On Mon, May 23, 2016 at 04:50:15PM -0500, Bjorn Helgaas wrote:
> > > [+cc Valdis, Dave]
> > > 
> > > On Mon, May 23, 2016 at 03:00:42PM -0500, Bjorn Helgaas wrote:
> > > > On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote:
> > > > > When a PCI device is removed through sysfs interface the upstream bridge
> > > > > (PCIe port) can be runtime suspended if it was the last device on that bus.
> > > > > Now, if the bridge is in D3 we cannot find devices below the bridge
> > > > > anymore. For example following fails to find the removed device again:
> > > > > 
> > > > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove
> > > > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan
> > > > > 
> > > > > Where 0000:00:01.0 is the bridge device.
> > > > > 
> > > > > In order to be able to rescan devices below the bridge add
> > > > > pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This
> > > > > should keep bridges powered on while their children devices are being
> > > > > scanned.
> > > > > 
> > > > > Reported-by: Peter Wu <peter@lekensteyn.nl>
> > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > 
> > > > This looks like basically the same idea as "ACPI / hotplug / PCI:
> > > > Runtime resume bridge before rescan".
> > > > 
> > > > The hotplug_event() path modified by that patch eventually calls
> > > > pci_scan_bridge():
> > > > 
> > > >   hotplug_event
> > > >     enable_slot
> > > >       pci_scan_bridge
> > > > 
> > > > so this patch looks a little more general.  Does it make "ACPI /
> > > > hotplug / PCI: Runtime resume bridge before rescan" unnecessary?  
> > > > Can I just replace that patch with this one?
> > > 
> > > I speculatively replaced "ACPI / hotplug / PCI: Runtime resume bridge
> > > before rescan" with this one and pushed the result to
> > > 
> > >   https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/pm
> > > 
> > > Please take a look, test it, and let me know if I need to add the ACPI
> > > patch back.
> > > 
> > > This branch also includes the fix for the lockdep splat reported by
> > > Valdis.  This is what I hope to get into v4.7-rc1.
> > 
> > Ping?  I'd like to ask Linus to pull this pci/pm branch before v4.7-rc1.
> > It currently has these changes:
> > 
> >   8b71f5652eea PCI: Add runtime PM support for PCIe ports
> >   af81f0fa638b PCI: Power on bridges before scanning new devices
> >   9741a01c9f55 PCI: Put PCIe ports into D3 during suspend
> >   b3a63ff7baf1 PCI: Don't clear d3cold_allowed for PCIe ports
> 
> Looks good to me. I've also tested those here and seems to work fine.

I have tested these patches for some time now on top of v4.6 (just
dropped the ACPI hotplug patch and re-tested just to be sure) and it
works for nouveau, but only if that one is patched to avoid calling the
device-specific Optimus method. Without that patch (WIP below, I plan to
rebase it on a refactoring patch), the nvidia card stays disabled even
after the bridge returns into D0:

    nouveau 0000:01:00.0: power state changed by ACPI to D0
    nouveau 0000:01:00.0: Refused to change power state, currently in D3
    nouveau 0000:01:00.0: restoring config space at offset 0x3c (was 0xffffffff, writing 0x1ff)
    nouveau 0000:01:00.0: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
    nouveau 0000:01:00.0: restoring config space at offset 0x34 (was 0xffffffff, writing 0x60)
    nouveau 0000:01:00.0: restoring config space at offset 0x30 (was 0xffffffff, writing 0xdf000000)
    ...
    INFO: rcu_sched self-detected stall on CPU

A workaround (until nouveau is fixed) is to disable runtime PM on the
bridge (or on nouveau with nouveau.runpm=0):

    # echo on > /sys/bus/pci/devices/0000:00:01.0/power/control

This was tested on a Clevo P651RA laptop (with acpi_osi="!Windows 2013",
there is a weird PCIe PM issue for which I will fill a report later).

Kind regards,
Peter

> > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan"
> > on the assumption that "PCI: Power on bridges before scanning new
> > devices" is sufficient to cover both the ACPI and the generic PCi
> > rescan cases, but I'd like some reassurance about that.
> 
> I agree with your reasoning that the patch should not be needed anymore.
> However, I have the machine which needed that patch at home so I'm not
> able to test it now. I'll do that later today when I get back home.
> 
> One thing I noticed, though. When a bridge is transitioned to D0 we only
> wait for 10ms which is requirement for PCI functions. However, PCI PM
> specification 1.2 (chapter 4.2) requires that for buses to transition
> from B2 to B0 we need to wait minimum of 50ms before accessing a
> function on that bus.
> 
> We even have PCI_PM_BUS_WAIT defined in include/linux/pci.h but it is
> not used anywhere. Maybe it was not needed originally because we never
> powered down bridges anyway but now when we do, I think it is good idea
> to do what the spec requires.
> 
> What do you think? We could add a separate patch doing something like
> below to make sure the spec is followed.
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e785dc260e72..b3b794caa380 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2361,7 +2361,12 @@ void pci_pm_init(struct pci_dev *dev)
>  	}
>  
>  	dev->pm_cap = pm;
> -	dev->d3_delay = PCI_PM_D3_WAIT;
> +	/*
> +	 * PCI PM 1.2 specification requires minimum of 50ms before any
> +	 * function on the bus is accessed after the bus is transitioned
> +	 * from B2 to B0.
> +	 */
> +	dev->d3_delay = pci_is_bridge(dev) ? PCI_PM_BUS_WAIT : PCI_PM_D3_WAIT;
>  	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
>  	dev->bridge_d3 = pci_bridge_d3_possible(dev);
>  	dev->d3cold_allowed = true;

---
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index cdf5227..531d6be 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -45,6 +45,7 @@
 static struct nouveau_dsm_priv {
 	bool dsm_detected;
 	bool optimus_detected;
+	bool use_pr3;
 	acpi_handle dhandle;
 	acpi_handle rom_handle;
 } nouveau_dsm_priv;
@@ -246,6 +247,28 @@ static int nouveau_dsm_pci_probe(struct pci_dev *pdev)
 	return retval;
 }
 
+/* Windows 8/8.1/10 do not use DSM to put the device in D3cold state,
+ * instead it disables power resources on the parent PCIe port device. */
+static bool nouveau_check_pr3(struct pci_dev *dis_dev)
+{
+	acpi_handle parent_handle;
+	struct acpi_device *ad = NULL;
+
+	if (ACPI_FAILURE(acpi_get_parent(nouveau_dsm_priv.dhandle,
+					 &parent_handle))) {
+		pr_warn("Failed to obtain the parent device\n");
+		return false;
+	}
+
+	acpi_bus_get_device(parent_handle, &ad);
+	if (!ad) {
+		pr_warn("Failed to obtain an ACPI device for handle\n");
+		return false;
+	}
+
+	return ad->power.flags.power_resources;
+}
+
 static bool nouveau_dsm_detect(void)
 {
 	char acpi_method_name[255] = { 0 };
@@ -253,6 +276,7 @@ static bool nouveau_dsm_detect(void)
 	struct pci_dev *pdev = NULL;
 	int has_dsm = 0;
 	int has_optimus = 0;
+	int has_pr3 = 0;
 	int vga_count = 0;
 	bool guid_valid;
 	int retval;
@@ -271,8 +295,10 @@ static bool nouveau_dsm_detect(void)
 		retval = nouveau_dsm_pci_probe(pdev);
 		if (retval & NOUVEAU_DSM_HAS_MUX)
 			has_dsm |= 1;
-		if (retval & NOUVEAU_DSM_HAS_OPT)
+		if (retval & NOUVEAU_DSM_HAS_OPT) {
 			has_optimus = 1;
+			has_pr3 |= nouveau_check_pr3(pdev);
+		}
 	}
 
 	while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL) {
@@ -281,8 +307,10 @@ static bool nouveau_dsm_detect(void)
 		retval = nouveau_dsm_pci_probe(pdev);
 		if (retval & NOUVEAU_DSM_HAS_MUX)
 			has_dsm |= 1;
-		if (retval & NOUVEAU_DSM_HAS_OPT)
+		if (retval & NOUVEAU_DSM_HAS_OPT) {
 			has_optimus = 1;
+			has_pr3 |= nouveau_check_pr3(pdev);
+		}
 	}
 
 	/* find the optimus DSM or the old v1 DSM */
@@ -292,6 +320,10 @@ static bool nouveau_dsm_detect(void)
 		printk(KERN_INFO "VGA switcheroo: detected Optimus DSM method %s handle\n",
 			acpi_method_name);
 		nouveau_dsm_priv.optimus_detected = true;
+		if (has_pr3) {
+			pr_info("detected PR3 support\n");
+			nouveau_dsm_priv.use_pr3 = 1;
+		}
 		ret = true;
 	} else if (vga_count == 2 && has_dsm && guid_valid) {
 		acpi_get_name(nouveau_dsm_priv.dhandle, ACPI_FULL_PATHNAME,
@@ -321,7 +353,7 @@ void nouveau_register_dsm_handler(void)
 void nouveau_switcheroo_optimus_dsm(void)
 {
 	u32 result = 0;
-	if (!nouveau_dsm_priv.optimus_detected)
+	if (!nouveau_dsm_priv.optimus_detected || nouveau_dsm_priv.use_pr3)
 		return;
 
 	nouveau_optimus_dsm(nouveau_dsm_priv.dhandle, NOUVEAU_DSM_OPTIMUS_FLAGS,

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Lukas Wunner]

Hi Peter,

On Tue, May 24, 2016 at 04:27:44PM +0200, Peter Wu wrote:
> @@ -246,6 +247,28 @@ static int nouveau_dsm_pci_probe(struct pci_dev *pdev)
>  	return retval;
>  }
>  
> +/* Windows 8/8.1/10 do not use DSM to put the device in D3cold state,
> + * instead it disables power resources on the parent PCIe port device. */
> +static bool nouveau_check_pr3(struct pci_dev *dis_dev)
> +{
> +	acpi_handle parent_handle;
> +	struct acpi_device *ad = NULL;
> +
> +	if (ACPI_FAILURE(acpi_get_parent(nouveau_dsm_priv.dhandle,
> +					 &parent_handle))) {
> +		pr_warn("Failed to obtain the parent device\n");
> +		return false;
> +	}
> +
> +	acpi_bus_get_device(parent_handle, &ad);
> +	if (!ad) {
> +		pr_warn("Failed to obtain an ACPI device for handle\n");
> +		return false;
> +	}
> +
> +	return ad->power.flags.power_resources;
> +}
> +

This can be shortened significantly by using pci_upstream_bridge()
and ACPI_HANDLE().

Best regards,

Lukas

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Bjorn Helgaas]

On Tue, May 24, 2016 at 04:27:44PM +0200, Peter Wu wrote:
> On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote:
> > On Tue, May 24, 2016 at 07:23:57AM -0500, Bjorn Helgaas wrote:
> > > On Mon, May 23, 2016 at 04:50:15PM -0500, Bjorn Helgaas wrote:
> > > > [+cc Valdis, Dave]
> > > > 
> > > > On Mon, May 23, 2016 at 03:00:42PM -0500, Bjorn Helgaas wrote:
> > > > > On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote:
> > > > > > When a PCI device is removed through sysfs interface the upstream bridge
> > > > > > (PCIe port) can be runtime suspended if it was the last device on that bus.
> > > > > > Now, if the bridge is in D3 we cannot find devices below the bridge
> > > > > > anymore. For example following fails to find the removed device again:
> > > > > > 
> > > > > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove
> > > > > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan
> > > > > > 
> > > > > > Where 0000:00:01.0 is the bridge device.
> > > > > > 
> > > > > > In order to be able to rescan devices below the bridge add
> > > > > > pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This
> > > > > > should keep bridges powered on while their children devices are being
> > > > > > scanned.
> > > > > > 
> > > > > > Reported-by: Peter Wu <peter@lekensteyn.nl>
> > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > 
> > > > > This looks like basically the same idea as "ACPI / hotplug / PCI:
> > > > > Runtime resume bridge before rescan".
> > > > > 
> > > > > The hotplug_event() path modified by that patch eventually calls
> > > > > pci_scan_bridge():
> > > > > 
> > > > >   hotplug_event
> > > > >     enable_slot
> > > > >       pci_scan_bridge
> > > > > 
> > > > > so this patch looks a little more general.  Does it make "ACPI /
> > > > > hotplug / PCI: Runtime resume bridge before rescan" unnecessary?  
> > > > > Can I just replace that patch with this one?
> > > > 
> > > > I speculatively replaced "ACPI / hotplug / PCI: Runtime resume bridge
> > > > before rescan" with this one and pushed the result to
> > > > 
> > > >   https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/pm
> > > > 
> > > > Please take a look, test it, and let me know if I need to add the ACPI
> > > > patch back.
> > > > 
> > > > This branch also includes the fix for the lockdep splat reported by
> > > > Valdis.  This is what I hope to get into v4.7-rc1.
> > > 
> > > Ping?  I'd like to ask Linus to pull this pci/pm branch before v4.7-rc1.
> > > It currently has these changes:
> > > 
> > >   8b71f5652eea PCI: Add runtime PM support for PCIe ports
> > >   af81f0fa638b PCI: Power on bridges before scanning new devices
> > >   9741a01c9f55 PCI: Put PCIe ports into D3 during suspend
> > >   b3a63ff7baf1 PCI: Don't clear d3cold_allowed for PCIe ports
> > 
> > Looks good to me. I've also tested those here and seems to work fine.
> 
> I have tested these patches for some time now on top of v4.6 (just
> dropped the ACPI hotplug patch and re-tested just to be sure) and it
> works for nouveau, but only if that one is patched to avoid calling the
> device-specific Optimus method. 

I assume this nouveau issue is a driver problem unrelated to the
pci/pm changes, and it wouldn't make any difference if I included the
ACPI hotplug patch.  Right?

> Without that patch (WIP below, I plan to
> rebase it on a refactoring patch), the nvidia card stays disabled even
> after the bridge returns into D0:
> 
>     nouveau 0000:01:00.0: power state changed by ACPI to D0
>     nouveau 0000:01:00.0: Refused to change power state, currently in D3
>     nouveau 0000:01:00.0: restoring config space at offset 0x3c (was 0xffffffff, writing 0x1ff)
>     nouveau 0000:01:00.0: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
>     nouveau 0000:01:00.0: restoring config space at offset 0x34 (was 0xffffffff, writing 0x60)
>     nouveau 0000:01:00.0: restoring config space at offset 0x30 (was 0xffffffff, writing 0xdf000000)
>     ...
>     INFO: rcu_sched self-detected stall on CPU
> 
> A workaround (until nouveau is fixed) is to disable runtime PM on the
> bridge (or on nouveau with nouveau.runpm=0):
> 
>     # echo on > /sys/bus/pci/devices/0000:00:01.0/power/control
> 
> This was tested on a Clevo P651RA laptop (with acpi_osi="!Windows 2013",
> there is a weird PCIe PM issue for which I will fill a report later).

This seems like a pretty significant problem: if we enable runtime PM
on bridges by default, and Nvidia cards stay disabled when the bridge
returns to D0, that sounds like a bad regression.  I don't think I
could ask Linus to pull these changes with a known issue like that.
Or am I misunderstanding something?

> ---
> diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> index cdf5227..531d6be 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> @@ -45,6 +45,7 @@
>  static struct nouveau_dsm_priv {
>  	bool dsm_detected;
>  	bool optimus_detected;
> +	bool use_pr3;
>  	acpi_handle dhandle;
>  	acpi_handle rom_handle;
>  } nouveau_dsm_priv;
> @@ -246,6 +247,28 @@ static int nouveau_dsm_pci_probe(struct pci_dev *pdev)
>  	return retval;
>  }
>  
> +/* Windows 8/8.1/10 do not use DSM to put the device in D3cold state,
> + * instead it disables power resources on the parent PCIe port device. */
> +static bool nouveau_check_pr3(struct pci_dev *dis_dev)
> +{
> +	acpi_handle parent_handle;
> +	struct acpi_device *ad = NULL;
> +
> +	if (ACPI_FAILURE(acpi_get_parent(nouveau_dsm_priv.dhandle,
> +					 &parent_handle))) {
> +		pr_warn("Failed to obtain the parent device\n");
> +		return false;
> +	}
> +
> +	acpi_bus_get_device(parent_handle, &ad);
> +	if (!ad) {
> +		pr_warn("Failed to obtain an ACPI device for handle\n");
> +		return false;
> +	}
> +
> +	return ad->power.flags.power_resources;
> +}
> +
>  static bool nouveau_dsm_detect(void)
>  {
>  	char acpi_method_name[255] = { 0 };
> @@ -253,6 +276,7 @@ static bool nouveau_dsm_detect(void)
>  	struct pci_dev *pdev = NULL;
>  	int has_dsm = 0;
>  	int has_optimus = 0;
> +	int has_pr3 = 0;
>  	int vga_count = 0;
>  	bool guid_valid;
>  	int retval;
> @@ -271,8 +295,10 @@ static bool nouveau_dsm_detect(void)
>  		retval = nouveau_dsm_pci_probe(pdev);
>  		if (retval & NOUVEAU_DSM_HAS_MUX)
>  			has_dsm |= 1;
> -		if (retval & NOUVEAU_DSM_HAS_OPT)
> +		if (retval & NOUVEAU_DSM_HAS_OPT) {
>  			has_optimus = 1;
> +			has_pr3 |= nouveau_check_pr3(pdev);
> +		}
>  	}
>  
>  	while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL) {
> @@ -281,8 +307,10 @@ static bool nouveau_dsm_detect(void)
>  		retval = nouveau_dsm_pci_probe(pdev);
>  		if (retval & NOUVEAU_DSM_HAS_MUX)
>  			has_dsm |= 1;
> -		if (retval & NOUVEAU_DSM_HAS_OPT)
> +		if (retval & NOUVEAU_DSM_HAS_OPT) {
>  			has_optimus = 1;
> +			has_pr3 |= nouveau_check_pr3(pdev);
> +		}
>  	}
>  
>  	/* find the optimus DSM or the old v1 DSM */
> @@ -292,6 +320,10 @@ static bool nouveau_dsm_detect(void)
>  		printk(KERN_INFO "VGA switcheroo: detected Optimus DSM method %s handle\n",
>  			acpi_method_name);
>  		nouveau_dsm_priv.optimus_detected = true;
> +		if (has_pr3) {
> +			pr_info("detected PR3 support\n");
> +			nouveau_dsm_priv.use_pr3 = 1;
> +		}
>  		ret = true;
>  	} else if (vga_count == 2 && has_dsm && guid_valid) {
>  		acpi_get_name(nouveau_dsm_priv.dhandle, ACPI_FULL_PATHNAME,
> @@ -321,7 +353,7 @@ void nouveau_register_dsm_handler(void)
>  void nouveau_switcheroo_optimus_dsm(void)
>  {
>  	u32 result = 0;
> -	if (!nouveau_dsm_priv.optimus_detected)
> +	if (!nouveau_dsm_priv.optimus_detected || nouveau_dsm_priv.use_pr3)
>  		return;
>  
>  	nouveau_optimus_dsm(nouveau_dsm_priv.dhandle, NOUVEAU_DSM_OPTIMUS_FLAGS,

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Peter Wu]

On Tue, May 24, 2016 at 11:38:48AM -0500, Bjorn Helgaas wrote:
> On Tue, May 24, 2016 at 04:27:44PM +0200, Peter Wu wrote:
> > On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote:
> > > On Tue, May 24, 2016 at 07:23:57AM -0500, Bjorn Helgaas wrote:
> > > > On Mon, May 23, 2016 at 04:50:15PM -0500, Bjorn Helgaas wrote:
> > > > > [+cc Valdis, Dave]
> > > > > 
> > > > > On Mon, May 23, 2016 at 03:00:42PM -0500, Bjorn Helgaas wrote:
> > > > > > On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote:
> > > > > > > When a PCI device is removed through sysfs interface the upstream bridge
> > > > > > > (PCIe port) can be runtime suspended if it was the last device on that bus.
> > > > > > > Now, if the bridge is in D3 we cannot find devices below the bridge
> > > > > > > anymore. For example following fails to find the removed device again:
> > > > > > > 
> > > > > > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove
> > > > > > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan
> > > > > > > 
> > > > > > > Where 0000:00:01.0 is the bridge device.
> > > > > > > 
> > > > > > > In order to be able to rescan devices below the bridge add
> > > > > > > pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This
> > > > > > > should keep bridges powered on while their children devices are being
> > > > > > > scanned.
> > > > > > > 
> > > > > > > Reported-by: Peter Wu <peter@lekensteyn.nl>
> > > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > > 
> > > > > > This looks like basically the same idea as "ACPI / hotplug / PCI:
> > > > > > Runtime resume bridge before rescan".
> > > > > > 
> > > > > > The hotplug_event() path modified by that patch eventually calls
> > > > > > pci_scan_bridge():
> > > > > > 
> > > > > >   hotplug_event
> > > > > >     enable_slot
> > > > > >       pci_scan_bridge
> > > > > > 
> > > > > > so this patch looks a little more general.  Does it make "ACPI /
> > > > > > hotplug / PCI: Runtime resume bridge before rescan" unnecessary?  
> > > > > > Can I just replace that patch with this one?
> > > > > 
> > > > > I speculatively replaced "ACPI / hotplug / PCI: Runtime resume bridge
> > > > > before rescan" with this one and pushed the result to
> > > > > 
> > > > >   https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/pm
> > > > > 
> > > > > Please take a look, test it, and let me know if I need to add the ACPI
> > > > > patch back.
> > > > > 
> > > > > This branch also includes the fix for the lockdep splat reported by
> > > > > Valdis.  This is what I hope to get into v4.7-rc1.
> > > > 
> > > > Ping?  I'd like to ask Linus to pull this pci/pm branch before v4.7-rc1.
> > > > It currently has these changes:
> > > > 
> > > >   8b71f5652eea PCI: Add runtime PM support for PCIe ports
> > > >   af81f0fa638b PCI: Power on bridges before scanning new devices
> > > >   9741a01c9f55 PCI: Put PCIe ports into D3 during suspend
> > > >   b3a63ff7baf1 PCI: Don't clear d3cold_allowed for PCIe ports
> > > 
> > > Looks good to me. I've also tested those here and seems to work fine.
> > 
> > I have tested these patches for some time now on top of v4.6 (just
> > dropped the ACPI hotplug patch and re-tested just to be sure) and it
> > works for nouveau, but only if that one is patched to avoid calling the
> > device-specific Optimus method. 
> 
> I assume this nouveau issue is a driver problem unrelated to the
> pci/pm changes, and it wouldn't make any difference if I included the
> ACPI hotplug patch.  Right?

This is a nouveau-specific driver problem that is triggered by the
adding runtime PM support for PCIe ports.

> > Without that patch (WIP below, I plan to
> > rebase it on a refactoring patch), the nvidia card stays disabled even
> > after the bridge returns into D0:
> > 
> >     nouveau 0000:01:00.0: power state changed by ACPI to D0
> >     nouveau 0000:01:00.0: Refused to change power state, currently in D3
> >     nouveau 0000:01:00.0: restoring config space at offset 0x3c (was 0xffffffff, writing 0x1ff)
> >     nouveau 0000:01:00.0: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
> >     nouveau 0000:01:00.0: restoring config space at offset 0x34 (was 0xffffffff, writing 0x60)
> >     nouveau 0000:01:00.0: restoring config space at offset 0x30 (was 0xffffffff, writing 0xdf000000)
> >     ...
> >     INFO: rcu_sched self-detected stall on CPU
> > 
> > A workaround (until nouveau is fixed) is to disable runtime PM on the
> > bridge (or on nouveau with nouveau.runpm=0):
> > 
> >     # echo on > /sys/bus/pci/devices/0000:00:01.0/power/control
> > 
> > This was tested on a Clevo P651RA laptop (with acpi_osi="!Windows 2013",
> > there is a weird PCIe PM issue for which I will fill a report later).
> 
> This seems like a pretty significant problem: if we enable runtime PM
> on bridges by default, and Nvidia cards stay disabled when the bridge
> returns to D0, that sounds like a bad regression.  I don't think I
> could ask Linus to pull these changes with a known issue like that.
> Or am I misunderstanding something?

That would indeed be a regression, please see this nouveau patch series
for a suggestion:
https://lists.freedesktop.org/archives/nouveau/2016-May/025116.html

By the way, there is also currently a RPM refcounting issue in nouveau
(possibly fixed by
https://lists.freedesktop.org/archives/nouveau/2016-May/025107.html)
which resulted in the following problem:

 1. Initially RPM refcount is 1 when no driver is loaded for a device.
 2. Load nouveau which will RPM suspend the device. refcount becomes 0.
 3. Unload nouveau (bug: refcount stays 0).
 4. As a result, the PCIe ports are runtime suspended (should probably
    not happen as there is no bound driver).
 5. When nouveau is loaded again, the power resources are enabled again,
    *but* it does not put the device into D0 (it expected that this was
    already the case). (with ACPI debugging on I see that _PS0 is not
    called).
 6. As a result, reading registers failed, "unknown chipset (ffffffff)"
    is printed to dmesg and the probe fails.

(Maybe the PCI core should have a WARN when the RPM is "auto" and the
refcount drops below 1 after remove is called with no driver left?)

Kind regards,
Peter Wu

> > ---
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > index cdf5227..531d6be 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > @@ -45,6 +45,7 @@
> >  static struct nouveau_dsm_priv {
> >  	bool dsm_detected;
> >  	bool optimus_detected;
> > +	bool use_pr3;
> >  	acpi_handle dhandle;
> >  	acpi_handle rom_handle;
> >  } nouveau_dsm_priv;
> > @@ -246,6 +247,28 @@ static int nouveau_dsm_pci_probe(struct pci_dev *pdev)
> >  	return retval;
> >  }
> >  
> > +/* Windows 8/8.1/10 do not use DSM to put the device in D3cold state,
> > + * instead it disables power resources on the parent PCIe port device. */
> > +static bool nouveau_check_pr3(struct pci_dev *dis_dev)
> > +{
> > +	acpi_handle parent_handle;
> > +	struct acpi_device *ad = NULL;
> > +
> > +	if (ACPI_FAILURE(acpi_get_parent(nouveau_dsm_priv.dhandle,
> > +					 &parent_handle))) {
> > +		pr_warn("Failed to obtain the parent device\n");
> > +		return false;
> > +	}
> > +
> > +	acpi_bus_get_device(parent_handle, &ad);
> > +	if (!ad) {
> > +		pr_warn("Failed to obtain an ACPI device for handle\n");
> > +		return false;
> > +	}
> > +
> > +	return ad->power.flags.power_resources;
> > +}
> > +
> >  static bool nouveau_dsm_detect(void)
> >  {
> >  	char acpi_method_name[255] = { 0 };
> > @@ -253,6 +276,7 @@ static bool nouveau_dsm_detect(void)
> >  	struct pci_dev *pdev = NULL;
> >  	int has_dsm = 0;
> >  	int has_optimus = 0;
> > +	int has_pr3 = 0;
> >  	int vga_count = 0;
> >  	bool guid_valid;
> >  	int retval;
> > @@ -271,8 +295,10 @@ static bool nouveau_dsm_detect(void)
> >  		retval = nouveau_dsm_pci_probe(pdev);
> >  		if (retval & NOUVEAU_DSM_HAS_MUX)
> >  			has_dsm |= 1;
> > -		if (retval & NOUVEAU_DSM_HAS_OPT)
> > +		if (retval & NOUVEAU_DSM_HAS_OPT) {
> >  			has_optimus = 1;
> > +			has_pr3 |= nouveau_check_pr3(pdev);
> > +		}
> >  	}
> >  
> >  	while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL) {
> > @@ -281,8 +307,10 @@ static bool nouveau_dsm_detect(void)
> >  		retval = nouveau_dsm_pci_probe(pdev);
> >  		if (retval & NOUVEAU_DSM_HAS_MUX)
> >  			has_dsm |= 1;
> > -		if (retval & NOUVEAU_DSM_HAS_OPT)
> > +		if (retval & NOUVEAU_DSM_HAS_OPT) {
> >  			has_optimus = 1;
> > +			has_pr3 |= nouveau_check_pr3(pdev);
> > +		}
> >  	}
> >  
> >  	/* find the optimus DSM or the old v1 DSM */
> > @@ -292,6 +320,10 @@ static bool nouveau_dsm_detect(void)
> >  		printk(KERN_INFO "VGA switcheroo: detected Optimus DSM method %s handle\n",
> >  			acpi_method_name);
> >  		nouveau_dsm_priv.optimus_detected = true;
> > +		if (has_pr3) {
> > +			pr_info("detected PR3 support\n");
> > +			nouveau_dsm_priv.use_pr3 = 1;
> > +		}
> >  		ret = true;
> >  	} else if (vga_count == 2 && has_dsm && guid_valid) {
> >  		acpi_get_name(nouveau_dsm_priv.dhandle, ACPI_FULL_PATHNAME,
> > @@ -321,7 +353,7 @@ void nouveau_register_dsm_handler(void)
> >  void nouveau_switcheroo_optimus_dsm(void)
> >  {
> >  	u32 result = 0;
> > -	if (!nouveau_dsm_priv.optimus_detected)
> > +	if (!nouveau_dsm_priv.optimus_detected || nouveau_dsm_priv.use_pr3)
> >  		return;
> >  
> >  	nouveau_optimus_dsm(nouveau_dsm_priv.dhandle, NOUVEAU_DSM_OPTIMUS_FLAGS,

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Bjorn Helgaas]

On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote:
> On Tue, May 24, 2016 at 07:23:57AM -0500, Bjorn Helgaas wrote:
> > On Mon, May 23, 2016 at 04:50:15PM -0500, Bjorn Helgaas wrote:
> > > [+cc Valdis, Dave]
> > > 
> > > On Mon, May 23, 2016 at 03:00:42PM -0500, Bjorn Helgaas wrote:
> > > > On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote:
> > > > > When a PCI device is removed through sysfs interface the upstream bridge
> > > > > (PCIe port) can be runtime suspended if it was the last device on that bus.
> > > > > Now, if the bridge is in D3 we cannot find devices below the bridge
> > > > > anymore. For example following fails to find the removed device again:
> > > > > 
> > > > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove
> > > > >    # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan
> > > > > 
> > > > > Where 0000:00:01.0 is the bridge device.
> > > > > 
> > > > > In order to be able to rescan devices below the bridge add
> > > > > pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This
> > > > > should keep bridges powered on while their children devices are being
> > > > > scanned.
> > > > > 
> > > > > Reported-by: Peter Wu <peter@lekensteyn.nl>
> > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > 
> > > > This looks like basically the same idea as "ACPI / hotplug / PCI:
> > > > Runtime resume bridge before rescan".
> > > > 
> > > > The hotplug_event() path modified by that patch eventually calls
> > > > pci_scan_bridge():
> > > > 
> > > >   hotplug_event
> > > >     enable_slot
> > > >       pci_scan_bridge
> > > > 
> > > > so this patch looks a little more general.  Does it make "ACPI /
> > > > hotplug / PCI: Runtime resume bridge before rescan" unnecessary?  
> > > > Can I just replace that patch with this one?
> > > 
> > > I speculatively replaced "ACPI / hotplug / PCI: Runtime resume bridge
> > > before rescan" with this one and pushed the result to
> > > 
> > >   https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/pm
> > > 
> > > Please take a look, test it, and let me know if I need to add the ACPI
> > > patch back.
> > > 
> > > This branch also includes the fix for the lockdep splat reported by
> > > Valdis.  This is what I hope to get into v4.7-rc1.
> > 
> > Ping?  I'd like to ask Linus to pull this pci/pm branch before v4.7-rc1.
> > It currently has these changes:
> > 
> >   8b71f5652eea PCI: Add runtime PM support for PCIe ports
> >   af81f0fa638b PCI: Power on bridges before scanning new devices
> >   9741a01c9f55 PCI: Put PCIe ports into D3 during suspend
> >   b3a63ff7baf1 PCI: Don't clear d3cold_allowed for PCIe ports
> 
> Looks good to me. I've also tested those here and seems to work fine.
> 
> > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan"
> > on the assumption that "PCI: Power on bridges before scanning new
> > devices" is sufficient to cover both the ACPI and the generic PCi
> > rescan cases, but I'd like some reassurance about that.
> 
> I agree with your reasoning that the patch should not be needed anymore.
> However, I have the machine which needed that patch at home so I'm not
> able to test it now. I'll do that later today when I get back home.
> 
> One thing I noticed, though. When a bridge is transitioned to D0 we only
> wait for 10ms which is requirement for PCI functions. However, PCI PM
> specification 1.2 (chapter 4.2) requires that for buses to transition
> from B2 to B0 we need to wait minimum of 50ms before accessing a
> function on that bus.
> 
> We even have PCI_PM_BUS_WAIT defined in include/linux/pci.h but it is
> not used anywhere. Maybe it was not needed originally because we never
> powered down bridges anyway but now when we do, I think it is good idea
> to do what the spec requires.
> 
> What do you think? We could add a separate patch doing something like
> below to make sure the spec is followed.
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e785dc260e72..b3b794caa380 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2361,7 +2361,12 @@ void pci_pm_init(struct pci_dev *dev)
>  	}
>  
>  	dev->pm_cap = pm;
> -	dev->d3_delay = PCI_PM_D3_WAIT;
> +	/*
> +	 * PCI PM 1.2 specification requires minimum of 50ms before any
> +	 * function on the bus is accessed after the bus is transitioned
> +	 * from B2 to B0.
> +	 */
> +	dev->d3_delay = pci_is_bridge(dev) ? PCI_PM_BUS_WAIT : PCI_PM_D3_WAIT;

Seems plausible to me, and seems safe enough to include for v4.7 if you
post this with a changelog and signed-off-by.

>  	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
>  	dev->bridge_d3 = pci_bridge_d3_possible(dev);
>  	dev->d3cold_allowed = true;

[PATCH] PCI: Wait for 50ms after bridge is powered up

[Mika Westerberg]

The PCI PM 1.2 specification requires minimum of 50ms before function on a
bus is accessed after the bus is transitioned from B2 to B0. Now that we
actually power down bridges we should make sure the specification is
followed accordingly.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Hi Bjorn,

Since this is only needed when bridges are powered down, I think it makes
sense to put this to your pci/pm branch.

 drivers/pci/pci.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e785dc260e72..e645a3d4f2e0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2361,7 +2361,12 @@ void pci_pm_init(struct pci_dev *dev)
 	}
 
 	dev->pm_cap = pm;
-	dev->d3_delay = PCI_PM_D3_WAIT;
+	/*
+	 * PCI PM 1.2 specification requires minimum of 50ms before any
+	 * function on the bus is accessed after the bus is transitioned
+	 * from B2 to B0.
+	 */
+	dev->d3_delay = pci_is_bridge(dev) ? PCI_PM_BUS_WAIT : PCI_PM_D3_WAIT;
 	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
 	dev->bridge_d3 = pci_bridge_d3_possible(dev);
 	dev->d3cold_allowed = true;
-- 
2.8.1

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Rafael J. Wysocki]

On Wed, May 25, 2016 at 5:04 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> The PCI PM 1.2 specification requires minimum of 50ms before function on a
> bus is accessed after the bus is transitioned from B2 to B0. Now that we
> actually power down bridges we should make sure the specification is
> followed accordingly.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

ACK

> ---
> Hi Bjorn,
>
> Since this is only needed when bridges are powered down, I think it makes
> sense to put this to your pci/pm branch.
>
>  drivers/pci/pci.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e785dc260e72..e645a3d4f2e0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2361,7 +2361,12 @@ void pci_pm_init(struct pci_dev *dev)
>         }
>
>         dev->pm_cap = pm;
> -       dev->d3_delay = PCI_PM_D3_WAIT;
> +       /*
> +        * PCI PM 1.2 specification requires minimum of 50ms before any
> +        * function on the bus is accessed after the bus is transitioned
> +        * from B2 to B0.
> +        */
> +       dev->d3_delay = pci_is_bridge(dev) ? PCI_PM_BUS_WAIT : PCI_PM_D3_WAIT;
>         dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
>         dev->bridge_d3 = pci_bridge_d3_possible(dev);
>         dev->d3cold_allowed = true;
> --
> 2.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Lukas Wunner]

Hi,

On Wed, May 25, 2016 at 06:04:56PM +0300, Mika Westerberg wrote:
> The PCI PM 1.2 specification requires minimum of 50ms before function on a
> bus is accessed after the bus is transitioned from B2 to B0. Now that we
> actually power down bridges we should make sure the specification is
> followed accordingly.

This patch has the unfortunate side effect of increasing boot time on
Macs with Thunderbolt by 320 ms. Granted, it's not much, but still
noticeable:

[    2.358041] pcieport 0000:06:03.0: enabling device (0000 -> 0002)
[    2.358195] pcieport 0000:06:03.0: rpm_idle
[    2.358222] pcieport 0000:06:03.0: rpm_suspend
[    2.411664] pcieport 0000:06:04.0: enabling device (0000 -> 0003)
[    2.411821] pcieport 0000:06:04.0: rpm_idle
[    2.411848] pcieport 0000:06:04.0: rpm_suspend
[    2.467664] pcieport 0000:06:05.0: enabling device (0000 -> 0003)
[    2.467817] pcieport 0000:06:05.0: rpm_idle
[    2.467843] pcieport 0000:06:05.0: rpm_suspend
[    2.523664] pcieport 0000:06:06.0: enabling device (0000 -> 0002)
[    2.523822] pcieport 0000:06:06.0: rpm_idle
[    2.523848] pcieport 0000:06:06.0: rpm_suspend
[    2.579685] pci_hotplug: PCI Hot Plug PCI Core version: 0.5
[    2.579722] pcieport 0000:06:03.0: rpm_resume
[    2.635750] pciehp 0000:06:03.0:pcie204: Slot #3 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.635853] pciehp 0000:06:03.0:pcie204: service driver pciehp loaded
[    2.635858] pcieport 0000:06:03.0: rpm_idle
[    2.635886] pcieport 0000:06:04.0: rpm_resume
[    2.647645] pcieport 0000:06:03.0: rpm_suspend
[    2.691747] pciehp 0000:06:04.0:pcie204: Slot #4 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.691856] pciehp 0000:06:04.0:pcie204: service driver pciehp loaded
[    2.691859] pcieport 0000:06:04.0: rpm_idle
[    2.691888] pcieport 0000:06:05.0: rpm_resume
[    2.703649] pcieport 0000:06:04.0: rpm_suspend
[    2.747748] pciehp 0000:06:05.0:pcie204: Slot #5 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.747842] pciehp 0000:06:05.0:pcie204: service driver pciehp loaded
[    2.747845] pcieport 0000:06:06.0: rpm_resume
[    2.749213] pcieport 0000:06:05.0: rpm_idle
[    2.759650] pcieport 0000:06:05.0: rpm_suspend
[    2.805049] pciehp 0000:06:06.0:pcie204: Slot #6 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.806464] pciehp 0000:06:06.0:pcie204: service driver pciehp loaded
[    2.806471] pciehp: PCI Express Hot Plug Controller Driver version: 0.4
[    2.807876] intel_idle: MWAIT substates: 0x21120
[    2.807878] intel_idle: v0.4.1 model 0x3A
[    2.808170] intel_idle: lapic_timer_reliable_states 0xffffffff
[    2.808201] GHES: HEST is not enabled!
[    2.809613] pcieport 0000:06:06.0: rpm_idle
[    2.809644] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
[    2.810096] Linux agpgart interface v0.103
[    2.810158] AMD IOMMUv2 driver by Joerg Roedel <jroedel@suse.de>
[    2.810158] AMD IOMMUv2 functionality not available on this system
[    2.816468] pcieport 0000:06:06.0: rpm_suspend

I've added debug messages whenever ->runtime_idle / _suspend / _resume
is called for a device.

As can be seen, the PCI core now waits 50 ms after ->runtime_suspend.
Also, the ports are resumed when the pciehp service driver is loaded,
adding another 50 ms delay. For four hotplug ports, that's a total of
400 ms (versus 80 ms without the patch).

I'm wondering if the delay after ->runtime_suspend is really needed. Is
this mandated by the spec? We could perhaps increase the autosuspend_delay
in pcie_portdrv_probe() slightly to prevent the port from going to sleep
between pci_enable_device() and loading the pciehp service driver.

Thanks,

Lukas

> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Hi Bjorn,
> 
> Since this is only needed when bridges are powered down, I think it makes
> sense to put this to your pci/pm branch.
> 
>  drivers/pci/pci.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e785dc260e72..e645a3d4f2e0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2361,7 +2361,12 @@ void pci_pm_init(struct pci_dev *dev)
>  	}
>  
>  	dev->pm_cap = pm;
> -	dev->d3_delay = PCI_PM_D3_WAIT;
> +	/*
> +	 * PCI PM 1.2 specification requires minimum of 50ms before any
> +	 * function on the bus is accessed after the bus is transitioned
> +	 * from B2 to B0.
> +	 */
> +	dev->d3_delay = pci_is_bridge(dev) ? PCI_PM_BUS_WAIT : PCI_PM_D3_WAIT;
>  	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
>  	dev->bridge_d3 = pci_bridge_d3_possible(dev);
>  	dev->d3cold_allowed = true;
> -- 
> 2.8.1
> 

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Mika Westerberg]

On Thu, May 26, 2016 at 12:10:06PM +0200, Lukas Wunner wrote:
> Hi,
> 
> On Wed, May 25, 2016 at 06:04:56PM +0300, Mika Westerberg wrote:
> > The PCI PM 1.2 specification requires minimum of 50ms before function on a
> > bus is accessed after the bus is transitioned from B2 to B0. Now that we
> > actually power down bridges we should make sure the specification is
> > followed accordingly.
> 
> This patch has the unfortunate side effect of increasing boot time on
> Macs with Thunderbolt by 320 ms. Granted, it's not much, but still
> noticeable:

Yes, that's the drawback.

> [    2.358041] pcieport 0000:06:03.0: enabling device (0000 -> 0002)
> [    2.358195] pcieport 0000:06:03.0: rpm_idle
> [    2.358222] pcieport 0000:06:03.0: rpm_suspend
> [    2.411664] pcieport 0000:06:04.0: enabling device (0000 -> 0003)
> [    2.411821] pcieport 0000:06:04.0: rpm_idle
> [    2.411848] pcieport 0000:06:04.0: rpm_suspend
> [    2.467664] pcieport 0000:06:05.0: enabling device (0000 -> 0003)
> [    2.467817] pcieport 0000:06:05.0: rpm_idle
> [    2.467843] pcieport 0000:06:05.0: rpm_suspend
> [    2.523664] pcieport 0000:06:06.0: enabling device (0000 -> 0002)
> [    2.523822] pcieport 0000:06:06.0: rpm_idle
> [    2.523848] pcieport 0000:06:06.0: rpm_suspend
> [    2.579685] pci_hotplug: PCI Hot Plug PCI Core version: 0.5
> [    2.579722] pcieport 0000:06:03.0: rpm_resume
> [    2.635750] pciehp 0000:06:03.0:pcie204: Slot #3 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> [    2.635853] pciehp 0000:06:03.0:pcie204: service driver pciehp loaded
> [    2.635858] pcieport 0000:06:03.0: rpm_idle
> [    2.635886] pcieport 0000:06:04.0: rpm_resume
> [    2.647645] pcieport 0000:06:03.0: rpm_suspend
> [    2.691747] pciehp 0000:06:04.0:pcie204: Slot #4 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> [    2.691856] pciehp 0000:06:04.0:pcie204: service driver pciehp loaded
> [    2.691859] pcieport 0000:06:04.0: rpm_idle
> [    2.691888] pcieport 0000:06:05.0: rpm_resume
> [    2.703649] pcieport 0000:06:04.0: rpm_suspend
> [    2.747748] pciehp 0000:06:05.0:pcie204: Slot #5 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> [    2.747842] pciehp 0000:06:05.0:pcie204: service driver pciehp loaded
> [    2.747845] pcieport 0000:06:06.0: rpm_resume
> [    2.749213] pcieport 0000:06:05.0: rpm_idle
> [    2.759650] pcieport 0000:06:05.0: rpm_suspend
> [    2.805049] pciehp 0000:06:06.0:pcie204: Slot #6 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> [    2.806464] pciehp 0000:06:06.0:pcie204: service driver pciehp loaded
> [    2.806471] pciehp: PCI Express Hot Plug Controller Driver version: 0.4
> [    2.807876] intel_idle: MWAIT substates: 0x21120
> [    2.807878] intel_idle: v0.4.1 model 0x3A
> [    2.808170] intel_idle: lapic_timer_reliable_states 0xffffffff
> [    2.808201] GHES: HEST is not enabled!
> [    2.809613] pcieport 0000:06:06.0: rpm_idle
> [    2.809644] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
> [    2.810096] Linux agpgart interface v0.103
> [    2.810158] AMD IOMMUv2 driver by Joerg Roedel <jroedel@suse.de>
> [    2.810158] AMD IOMMUv2 functionality not available on this system
> [    2.816468] pcieport 0000:06:06.0: rpm_suspend
> 
> I've added debug messages whenever ->runtime_idle / _suspend / _resume
> is called for a device.
> 
> As can be seen, the PCI core now waits 50 ms after ->runtime_suspend.
> Also, the ports are resumed when the pciehp service driver is loaded,
> adding another 50 ms delay. For four hotplug ports, that's a total of
> 400 ms (versus 80 ms without the patch).
> 
> I'm wondering if the delay after ->runtime_suspend is really needed. Is
> this mandated by the spec? We could perhaps increase the autosuspend_delay
> in pcie_portdrv_probe() slightly to prevent the port from going to sleep
> between pci_enable_device() and loading the pciehp service driver.

The PCI PM specification version 1.2 says this in chapter 4.2 (page 37):

  There is a minimum time requirement of 50 ms which must be provided by
  system software between when the bus is switched from B2 to B0 and
  when a function on the bus is accessed to allow time for the clock to
  start up and the bus to settle.

Not sure how much of that still applies to modern hardware.

I don't see why we cannot increase autosuspend delay time, though.

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Lukas Wunner]

On Thu, May 26, 2016 at 01:25:20PM +0300, Mika Westerberg wrote:
> On Thu, May 26, 2016 at 12:10:06PM +0200, Lukas Wunner wrote:
> > On Wed, May 25, 2016 at 06:04:56PM +0300, Mika Westerberg wrote:
> > > The PCI PM 1.2 specification requires minimum of 50ms before function on a
> > > bus is accessed after the bus is transitioned from B2 to B0. Now that we
> > > actually power down bridges we should make sure the specification is
> > > followed accordingly.
> > 
> > This patch has the unfortunate side effect of increasing boot time on
> > Macs with Thunderbolt by 320 ms. Granted, it's not much, but still
> > noticeable:
> 
> Yes, that's the drawback.
> 
> > [    2.358041] pcieport 0000:06:03.0: enabling device (0000 -> 0002)
> > [    2.358195] pcieport 0000:06:03.0: rpm_idle
> > [    2.358222] pcieport 0000:06:03.0: rpm_suspend
> > [    2.411664] pcieport 0000:06:04.0: enabling device (0000 -> 0003)
> > [    2.411821] pcieport 0000:06:04.0: rpm_idle
> > [    2.411848] pcieport 0000:06:04.0: rpm_suspend
> > [    2.467664] pcieport 0000:06:05.0: enabling device (0000 -> 0003)
> > [    2.467817] pcieport 0000:06:05.0: rpm_idle
> > [    2.467843] pcieport 0000:06:05.0: rpm_suspend
> > [    2.523664] pcieport 0000:06:06.0: enabling device (0000 -> 0002)
> > [    2.523822] pcieport 0000:06:06.0: rpm_idle
> > [    2.523848] pcieport 0000:06:06.0: rpm_suspend
> > [    2.579685] pci_hotplug: PCI Hot Plug PCI Core version: 0.5
> > [    2.579722] pcieport 0000:06:03.0: rpm_resume
> > [    2.635750] pciehp 0000:06:03.0:pcie204: Slot #3 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> > [    2.635853] pciehp 0000:06:03.0:pcie204: service driver pciehp loaded
> > [    2.635858] pcieport 0000:06:03.0: rpm_idle
> > [    2.635886] pcieport 0000:06:04.0: rpm_resume
> > [    2.647645] pcieport 0000:06:03.0: rpm_suspend
> > [    2.691747] pciehp 0000:06:04.0:pcie204: Slot #4 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> > [    2.691856] pciehp 0000:06:04.0:pcie204: service driver pciehp loaded
> > [    2.691859] pcieport 0000:06:04.0: rpm_idle
> > [    2.691888] pcieport 0000:06:05.0: rpm_resume
> > [    2.703649] pcieport 0000:06:04.0: rpm_suspend
> > [    2.747748] pciehp 0000:06:05.0:pcie204: Slot #5 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> > [    2.747842] pciehp 0000:06:05.0:pcie204: service driver pciehp loaded
> > [    2.747845] pcieport 0000:06:06.0: rpm_resume
> > [    2.749213] pcieport 0000:06:05.0: rpm_idle
> > [    2.759650] pcieport 0000:06:05.0: rpm_suspend
> > [    2.805049] pciehp 0000:06:06.0:pcie204: Slot #6 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> > [    2.806464] pciehp 0000:06:06.0:pcie204: service driver pciehp loaded
> > [    2.806471] pciehp: PCI Express Hot Plug Controller Driver version: 0.4
> > [    2.807876] intel_idle: MWAIT substates: 0x21120
> > [    2.807878] intel_idle: v0.4.1 model 0x3A
> > [    2.808170] intel_idle: lapic_timer_reliable_states 0xffffffff
> > [    2.808201] GHES: HEST is not enabled!
> > [    2.809613] pcieport 0000:06:06.0: rpm_idle
> > [    2.809644] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
> > [    2.810096] Linux agpgart interface v0.103
> > [    2.810158] AMD IOMMUv2 driver by Joerg Roedel <jroedel@suse.de>
> > [    2.810158] AMD IOMMUv2 functionality not available on this system
> > [    2.816468] pcieport 0000:06:06.0: rpm_suspend
> > 
> > I've added debug messages whenever ->runtime_idle / _suspend / _resume
> > is called for a device.
> > 
> > As can be seen, the PCI core now waits 50 ms after ->runtime_suspend.
> > Also, the ports are resumed when the pciehp service driver is loaded,
> > adding another 50 ms delay. For four hotplug ports, that's a total of
> > 400 ms (versus 80 ms without the patch).
> > 
> > I'm wondering if the delay after ->runtime_suspend is really needed. Is
> > this mandated by the spec? We could perhaps increase the autosuspend_delay
> > in pcie_portdrv_probe() slightly to prevent the port from going to sleep
> > between pci_enable_device() and loading the pciehp service driver.
> 
> The PCI PM specification version 1.2 says this in chapter 4.2 (page 37):
> 
>   There is a minimum time requirement of 50 ms which must be provided by
>   system software between when the bus is switched from B2 to B0 and
>   when a function on the bus is accessed to allow time for the clock to
>   start up and the bus to settle.

But why do we wait 50 ms when *suspending*, i.e. going from B0 to B2?

(Assuming B2 is the state when the bridge goes to D3hot, which I'm not
sure of. The spec says that the bus state may optionally be B3 if the
bridge is in D3hot.)


> Not sure how much of that still applies to modern hardware.

Could you ask hardware engineers at Intel what the bus state is on
modern chipsets (say, ILK or newer) and Thunderbolt ports to clarify
this?

Thanks,

Lukas

>
> I don't see why we cannot increase autosuspend delay time, though.

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Mika Westerberg]

On Thu, May 26, 2016 at 12:45:57PM +0200, Lukas Wunner wrote:
> > The PCI PM specification version 1.2 says this in chapter 4.2 (page 37):
> > 
> >   There is a minimum time requirement of 50 ms which must be provided by
> >   system software between when the bus is switched from B2 to B0 and
> >   when a function on the bus is accessed to allow time for the clock to
> >   start up and the bus to settle.
> 
> But why do we wait 50 ms when *suspending*, i.e. going from B0 to B2?

I guess because PCI requires delays of 10ms for both directions D0 <->
D3hot (see pci_raw_set_power_state()).

> (Assuming B2 is the state when the bridge goes to D3hot, which I'm not
> sure of. The spec says that the bus state may optionally be B3 if the
> bridge is in D3hot.)

B3 is the state where the bus goes when it's power is removed so I would
expect that to require also the 50ms even though the spec does not
explicitly say so.

> > Not sure how much of that still applies to modern hardware.
> 
> Could you ask hardware engineers at Intel what the bus state is on
> modern chipsets (say, ILK or newer) and Thunderbolt ports to clarify
> this?

I can try but it is not always easy to find the right person in company
as big as Intel.

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Rafael J. Wysocki]

On Thursday, May 26, 2016 02:03:08 PM Mika Westerberg wrote:
> On Thu, May 26, 2016 at 12:45:57PM +0200, Lukas Wunner wrote:
> > > The PCI PM specification version 1.2 says this in chapter 4.2 (page 37):
> > > 
> > >   There is a minimum time requirement of 50 ms which must be provided by
> > >   system software between when the bus is switched from B2 to B0 and
> > >   when a function on the bus is accessed to allow time for the clock to
> > >   start up and the bus to settle.
> > 
> > But why do we wait 50 ms when *suspending*, i.e. going from B0 to B2?
> 
> I guess because PCI requires delays of 10ms for both directions D0 <->
> D3hot (see pci_raw_set_power_state()).
> 
> > (Assuming B2 is the state when the bridge goes to D3hot, which I'm not
> > sure of. The spec says that the bus state may optionally be B3 if the
> > bridge is in D3hot.)
> 
> B3 is the state where the bus goes when it's power is removed so I would
> expect that to require also the 50ms even though the spec does not
> explicitly say so.
> 
> > > Not sure how much of that still applies to modern hardware.
> > 
> > Could you ask hardware engineers at Intel what the bus state is on
> > modern chipsets (say, ILK or newer) and Thunderbolt ports to clarify
> > this?
> 
> I can try but it is not always easy to find the right person in company
> as big as Intel.

Well, even if you find someone to tell you that, what about non-Intel?

Are we going to ever know a value that's going to work for everybody unless
that value is clearly stated in the spec?

Thanks,
Rafael

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Mika Westerberg]

On Sat, May 28, 2016 at 02:29:06PM +0200, Rafael J. Wysocki wrote:
> On Thursday, May 26, 2016 02:03:08 PM Mika Westerberg wrote:
> > On Thu, May 26, 2016 at 12:45:57PM +0200, Lukas Wunner wrote:
> > > > The PCI PM specification version 1.2 says this in chapter 4.2 (page 37):
> > > > 
> > > >   There is a minimum time requirement of 50 ms which must be provided by
> > > >   system software between when the bus is switched from B2 to B0 and
> > > >   when a function on the bus is accessed to allow time for the clock to
> > > >   start up and the bus to settle.
> > > 
> > > But why do we wait 50 ms when *suspending*, i.e. going from B0 to B2?
> > 
> > I guess because PCI requires delays of 10ms for both directions D0 <->
> > D3hot (see pci_raw_set_power_state()).
> > 
> > > (Assuming B2 is the state when the bridge goes to D3hot, which I'm not
> > > sure of. The spec says that the bus state may optionally be B3 if the
> > > bridge is in D3hot.)
> > 
> > B3 is the state where the bus goes when it's power is removed so I would
> > expect that to require also the 50ms even though the spec does not
> > explicitly say so.
> > 
> > > > Not sure how much of that still applies to modern hardware.
> > > 
> > > Could you ask hardware engineers at Intel what the bus state is on
> > > modern chipsets (say, ILK or newer) and Thunderbolt ports to clarify
> > > this?
> > 
> > I can try but it is not always easy to find the right person in company
> > as big as Intel.
> 
> Well, even if you find someone to tell you that, what about non-Intel?

Indeed, I was hoping to find someone who knows more about PCIe in
general and it turns out I found one :-)

> Are we going to ever know a value that's going to work for everybody unless
> that value is clearly stated in the spec?

That is a good question and the person I managed to find tells me that
the values are already in the spec.

So there are really no B-states in PCIe. Closest thing is L-states which
are defined in the PCIe spec. The spec has two timing limitations:

 - After bringing the device to D0/L0 from D3hot/L1 there is a
   required 10ms delay after the write to PMCSR before any other config
   space access (5.3.1.4 in PCIe spec 3.1a).

 - After bringing the device out of reset, which is the path to D0/L0
   from D3cold, there is a required 100ms delay before accessing the
   device's config space (6.6.1 in PCIe spec 3.1a).

I also learned that both of these can be shortened with following
mechanisms:

 - PCIe readines notifications (6.23 in PCIe spec 3.1a)
 - ACPI _DSM method returning readines durations from  (4.6.9 in PCI
   firmware spec 3.2).

I think it is actually worth looking into the two mechanisms and try to
get Linux support them.

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Mika Westerberg]

On Mon, May 30, 2016 at 12:33:26PM +0300, Mika Westerberg wrote:
> I also learned that both of these can be shortened with following
> mechanisms:
> 
>  - PCIe readines notifications (6.23 in PCIe spec 3.1a)
>  - ACPI _DSM method returning readines durations from  (4.6.9 in PCI
>    firmware spec 3.2).

BTW, looks like the latter is already implemented in
pci_acpi_optimize_delay().

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Andreas Noever]

On Mon, May 30, 2016 at 4:44 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, May 30, 2016 at 12:33:26PM +0300, Mika Westerberg wrote:
>> I also learned that both of these can be shortened with following
>> mechanisms:
>>
>>  - PCIe readines notifications (6.23 in PCIe spec 3.1a)
>>  - ACPI _DSM method returning readines durations from  (4.6.9 in PCI
>>    firmware spec 3.2).
>
> BTW, looks like the latter is already implemented in
> pci_acpi_optimize_delay().


Hmm, Lukas's trace suggest a few independent problems (or optimisations):
1. Those devices are siblings, we should wake all of them in parallel
and then wait once instead of one by one.
2. Why are we waiting after _suspend at all? It seems we should only
wait before the next access. Sleeping after _suspend looks like a stop
gap measure to guarantee that no accesses take place?
3. The idle timeout is too low, there should be no suspend between
discovery and probing.
4. Even if the spec says 50ms, we might still want to have shorter
sleep times for known good devices. Thunderbolt can produce PCI
hierarchies which are 7 levels deep with 4 hp bridges on each level.
Waking all of that would take 50ms * 7 * 4 = 1400ms (not counting
upstream bridges which have the normal d3 delay).

Does TB support PCIe readines notifications?

Andreas

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Mika Westerberg]

On Mon, May 30, 2016 at 05:19:53PM +0200, Andreas Noever wrote:
> On Mon, May 30, 2016 at 4:44 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Mon, May 30, 2016 at 12:33:26PM +0300, Mika Westerberg wrote:
> >> I also learned that both of these can be shortened with following
> >> mechanisms:
> >>
> >>  - PCIe readines notifications (6.23 in PCIe spec 3.1a)
> >>  - ACPI _DSM method returning readines durations from  (4.6.9 in PCI
> >>    firmware spec 3.2).
> >
> > BTW, looks like the latter is already implemented in
> > pci_acpi_optimize_delay().
> 
> 
> Hmm, Lukas's trace suggest a few independent problems (or optimisations):
> 1. Those devices are siblings, we should wake all of them in parallel
> and then wait once instead of one by one.
> 2. Why are we waiting after _suspend at all? It seems we should only
> wait before the next access. Sleeping after _suspend looks like a stop
> gap measure to guarantee that no accesses take place?

I guess it is because PCI PM spec says that the delay needs to be in
both directions. See table 5-6 in PCI PM spec v1.2.

The code in question is in pci_raw_set_power_state():

        /* Mandatory power management transition delays */
        /* see PCI PM 1.1 5.6.1 table 18 */
        if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
                pci_dev_d3_sleep(dev);
        else if (state == PCI_D2 || dev->current_state == PCI_D2)
                udelay(PCI_PM_D2_DELAY);

> 3. The idle timeout is too low, there should be no suspend between
> discovery and probing.

I agree. We took the 10ms from the original patch but I don't see why we
could not increase it to 100ms or even 500ms.

> 4. Even if the spec says 50ms, we might still want to have shorter
> sleep times for known good devices. Thunderbolt can produce PCI
> hierarchies which are 7 levels deep with 4 hp bridges on each level.
> Waking all of that would take 50ms * 7 * 4 = 1400ms (not counting
> upstream bridges which have the normal d3 delay).

PCIe spec want to have 100ms delay from when transitioning from D3cold
to D0 and we already do that in __pci_start_power_transition().

In other words we should have all necessary delays for PCIe in place
already. This patch should not be needed.

For decreasing delays we already support ACPI _DSM method. What is
missing is PCIe readines notification support which can be added
separately.

> Does TB support PCIe readines notifications?

It seems to be pretty recent feature so it might not be supported by the
existing TBT hardware.

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Mika Westerberg]

On Tue, May 31, 2016 at 11:33:49AM +0300, Mika Westerberg wrote:
> On Mon, May 30, 2016 at 05:19:53PM +0200, Andreas Noever wrote:
> > On Mon, May 30, 2016 at 4:44 PM, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > On Mon, May 30, 2016 at 12:33:26PM +0300, Mika Westerberg wrote:
> > >> I also learned that both of these can be shortened with following
> > >> mechanisms:
> > >>
> > >>  - PCIe readines notifications (6.23 in PCIe spec 3.1a)
> > >>  - ACPI _DSM method returning readines durations from  (4.6.9 in PCI
> > >>    firmware spec 3.2).
> > >
> > > BTW, looks like the latter is already implemented in
> > > pci_acpi_optimize_delay().
> > 
> > 
> > Hmm, Lukas's trace suggest a few independent problems (or optimisations):
> > 1. Those devices are siblings, we should wake all of them in parallel
> > and then wait once instead of one by one.
> > 2. Why are we waiting after _suspend at all? It seems we should only
> > wait before the next access. Sleeping after _suspend looks like a stop
> > gap measure to guarantee that no accesses take place?
> 
> I guess it is because PCI PM spec says that the delay needs to be in
> both directions. See table 5-6 in PCI PM spec v1.2.
> 
> The code in question is in pci_raw_set_power_state():
> 
>         /* Mandatory power management transition delays */
>         /* see PCI PM 1.1 5.6.1 table 18 */
>         if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
>                 pci_dev_d3_sleep(dev);
>         else if (state == PCI_D2 || dev->current_state == PCI_D2)
>                 udelay(PCI_PM_D2_DELAY);
> 
> > 3. The idle timeout is too low, there should be no suspend between
> > discovery and probing.
> 
> I agree. We took the 10ms from the original patch but I don't see why we
> could not increase it to 100ms or even 500ms.
> 
> > 4. Even if the spec says 50ms, we might still want to have shorter
> > sleep times for known good devices. Thunderbolt can produce PCI
> > hierarchies which are 7 levels deep with 4 hp bridges on each level.
> > Waking all of that would take 50ms * 7 * 4 = 1400ms (not counting
> > upstream bridges which have the normal d3 delay).
> 
> PCIe spec want to have 100ms delay from when transitioning from D3cold
> to D0 and we already do that in __pci_start_power_transition().
> 
> In other words we should have all necessary delays for PCIe in place
> already. This patch should not be needed.

To summarize the next steps. I will send new version of the
PCI PM patches with following changes.

  - Drop this 50ms patch, we should have the PCIe 100ms delay already
    covered.

  - Increase runtime PM autosuspend time from 10ms to 500ms (or whatever
    is the prefered default).

  - Add new version of ACPI hotplug patch where pm_runtime_get/put() is
    moved into acpiphp_check_bridge().

Let me know if I'm forgetting something.

Bjorn, is this ok for you? It would be nice to get the updated series to
linux-next for wider testing :)

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Lukas Wunner]

On Tue, May 31, 2016 at 11:58:05AM +0300, Mika Westerberg wrote:
> To summarize the next steps. I will send new version of the
> PCI PM patches with following changes.
> 
>   - Drop this 50ms patch, we should have the PCIe 100ms delay already
>     covered.
> 
>   - Increase runtime PM autosuspend time from 10ms to 500ms (or whatever
>     is the prefered default).

I did some tests, turns out the autosuspend delay need not be increased
to prevent the Thunderbolt hotplug ports from suspending between
"enabling device" and loading the pciehp driver, however the following
is needed:

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 7860ab3..1d1fb1c 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -238,6 +238,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 		pm_runtime_set_autosuspend_delay(&dev->dev, 10);
 		pm_runtime_use_autosuspend(&dev->dev);
 		pm_runtime_put_autosuspend(&dev->dev);
+		pm_runtime_mark_last_busy(&dev->dev);
 		pm_runtime_allow(&dev->dev);
 	}
 

With this small change things look much better (with the 10 ms delay
we have now):

[    2.353643] pcieport 0000:06:03.0: enabling device (0000 -> 0002)
[    2.353789] pcieport 0000:06:03.0: rpm_idle
[    2.353825] pcieport 0000:06:04.0: enabling device (0000 -> 0003)
[    2.353855] pcieport 0000:06:03.0: rpm_idle
[    2.353994] pcieport 0000:06:04.0: rpm_idle
[    2.354017] pcieport 0000:06:04.0: rpm_idle
[    2.354042] pcieport 0000:06:05.0: enabling device (0000 -> 0003)
[    2.354186] pcieport 0000:06:05.0: rpm_idle
[    2.354213] pcieport 0000:06:06.0: enabling device (0000 -> 0002)
[    2.354362] pcieport 0000:06:06.0: rpm_idle
[    2.354407] pci_hotplug: PCI Hot Plug PCI Core version: 0.5
[    2.354441] pciehp 0000:06:03.0:pcie204: Slot #3 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.354524] pciehp 0000:06:03.0:pcie204: service driver pciehp loaded
[    2.354533] pciehp 0000:06:04.0:pcie204: Slot #4 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.354609] pciehp 0000:06:04.0:pcie204: service driver pciehp loaded
[    2.354617] pciehp 0000:06:05.0:pcie204: Slot #5 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.354690] pciehp 0000:06:05.0:pcie204: service driver pciehp loaded
[    2.354698] pciehp 0000:06:06.0:pcie204: Slot #6 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.354772] pciehp 0000:06:06.0:pcie204: service driver pciehp loaded
[    2.354777] pciehp: PCI Express Hot Plug Controller Driver version: 0.4
[    2.354827] intel_idle: MWAIT substates: 0x21120
[    2.354829] intel_idle: v0.4.1 model 0x3A
[    2.355122] intel_idle: lapic_timer_reliable_states 0xffffffff
[    2.355152] GHES: HEST is not enabled!
[    2.355216] pcieport 0000:06:05.0: rpm_idle
[    2.355235] pcieport 0000:06:06.0: rpm_idle
[    2.355238] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
[    2.355277] pcieport 0000:06:03.0: rpm_idle
[    2.355301] pcieport 0000:06:04.0: rpm_idle
[    2.355659] Linux agpgart interface v0.103
[    2.355739] AMD IOMMUv2 driver by Joerg Roedel <jroedel@suse.de>
[    2.355764] AMD IOMMUv2 functionality not available on this system
[    2.356396] i8042: PNP: No PS/2 controller found. Probing ports directly.
[    2.364794] pcieport 0000:06:06.0: rpm_suspend
[    2.366112] pcieport 0000:06:05.0: rpm_suspend
[    2.367402] pcieport 0000:06:04.0: rpm_suspend
[    2.368671] pcieport 0000:06:03.0: rpm_suspend

Thanks,

Lukas

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Mika Westerberg]

On Tue, May 31, 2016 at 12:40:51PM +0200, Lukas Wunner wrote:
> On Tue, May 31, 2016 at 11:58:05AM +0300, Mika Westerberg wrote:
> > To summarize the next steps. I will send new version of the
> > PCI PM patches with following changes.
> > 
> >   - Drop this 50ms patch, we should have the PCIe 100ms delay already
> >     covered.
> > 
> >   - Increase runtime PM autosuspend time from 10ms to 500ms (or whatever
> >     is the prefered default).
> 
> I did some tests, turns out the autosuspend delay need not be increased
> to prevent the Thunderbolt hotplug ports from suspending between
> "enabling device" and loading the pciehp driver, however the following
> is needed:
> 
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 7860ab3..1d1fb1c 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -238,6 +238,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  		pm_runtime_set_autosuspend_delay(&dev->dev, 10);
>  		pm_runtime_use_autosuspend(&dev->dev);
>  		pm_runtime_put_autosuspend(&dev->dev);
> +		pm_runtime_mark_last_busy(&dev->dev);
>  		pm_runtime_allow(&dev->dev);
>  	}

I still prefer increasing the autosuspend delay. The above looks hackish
and does not work if it takes more than 10ms to get to the tbt driver
probe.

Did you try if it also works with 500ms delay?

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Lukas Wunner]

On Tue, May 31, 2016 at 01:47:14PM +0300, Mika Westerberg wrote:
> On Tue, May 31, 2016 at 12:40:51PM +0200, Lukas Wunner wrote:
> > On Tue, May 31, 2016 at 11:58:05AM +0300, Mika Westerberg wrote:
> > > To summarize the next steps. I will send new version of the
> > > PCI PM patches with following changes.
> > > 
> > >   - Drop this 50ms patch, we should have the PCIe 100ms delay already
> > >     covered.
> > > 
> > >   - Increase runtime PM autosuspend time from 10ms to 500ms (or whatever
> > >     is the prefered default).
> > 
> > I did some tests, turns out the autosuspend delay need not be increased
> > to prevent the Thunderbolt hotplug ports from suspending between
> > "enabling device" and loading the pciehp driver, however the following
> > is needed:
> > 
> > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> > index 7860ab3..1d1fb1c 100644
> > --- a/drivers/pci/pcie/portdrv_pci.c
> > +++ b/drivers/pci/pcie/portdrv_pci.c
> > @@ -238,6 +238,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> >  		pm_runtime_set_autosuspend_delay(&dev->dev, 10);
> >  		pm_runtime_use_autosuspend(&dev->dev);
> >  		pm_runtime_put_autosuspend(&dev->dev);
> > +		pm_runtime_mark_last_busy(&dev->dev);
> >  		pm_runtime_allow(&dev->dev);
> >  	}
> 
> I still prefer increasing the autosuspend delay. The above looks hackish
> and does not work if it takes more than 10ms to get to the tbt driver
> probe.
> 
> Did you try if it also works with 500ms delay?

I tried 150 ms and it didn't work. The pm_runtime_mark_last_busy()
is needed no matter how much you increase the autosuspend delay.
This isn't hackish. :-) The issue is that pm_runtime_mark_last_busy()
has never been called so far, so dev->power.last_busy == 0.
The PM core thinks that the device can suspend right away because it's
last been busy more than 2 seconds ago.

One could argue though if pm_runtime_mark_last_busy() should come
before pm_runtime_put_autosuspend(). Usually it should, but in this
case it doesn't matter because pm_runtime_allow() hasn't been called
yet and the PCI core initializes devices to pm_runtime_forbid().

Lukas

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Mika Westerberg]

On Tue, May 31, 2016 at 01:07:57PM +0200, Lukas Wunner wrote:
> On Tue, May 31, 2016 at 01:47:14PM +0300, Mika Westerberg wrote:
> > On Tue, May 31, 2016 at 12:40:51PM +0200, Lukas Wunner wrote:
> > > On Tue, May 31, 2016 at 11:58:05AM +0300, Mika Westerberg wrote:
> > > > To summarize the next steps. I will send new version of the
> > > > PCI PM patches with following changes.
> > > > 
> > > >   - Drop this 50ms patch, we should have the PCIe 100ms delay already
> > > >     covered.
> > > > 
> > > >   - Increase runtime PM autosuspend time from 10ms to 500ms (or whatever
> > > >     is the prefered default).
> > > 
> > > I did some tests, turns out the autosuspend delay need not be increased
> > > to prevent the Thunderbolt hotplug ports from suspending between
> > > "enabling device" and loading the pciehp driver, however the following
> > > is needed:
> > > 
> > > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> > > index 7860ab3..1d1fb1c 100644
> > > --- a/drivers/pci/pcie/portdrv_pci.c
> > > +++ b/drivers/pci/pcie/portdrv_pci.c
> > > @@ -238,6 +238,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> > >  		pm_runtime_set_autosuspend_delay(&dev->dev, 10);
> > >  		pm_runtime_use_autosuspend(&dev->dev);
> > >  		pm_runtime_put_autosuspend(&dev->dev);
> > > +		pm_runtime_mark_last_busy(&dev->dev);
> > >  		pm_runtime_allow(&dev->dev);
> > >  	}
> > 
> > I still prefer increasing the autosuspend delay. The above looks hackish
> > and does not work if it takes more than 10ms to get to the tbt driver
> > probe.
> > 
> > Did you try if it also works with 500ms delay?
> 
> I tried 150 ms and it didn't work. The pm_runtime_mark_last_busy()
> is needed no matter how much you increase the autosuspend delay.
> This isn't hackish. :-) The issue is that pm_runtime_mark_last_busy()
> has never been called so far, so dev->power.last_busy == 0.

Yes, it looks like it is really needed. I somehow remembered that
pm_runtime_set_autosuspend_delay() sets that automatically. So I take
back my hackish comment ;-)

> The PM core thinks that the device can suspend right away because it's
> last been busy more than 2 seconds ago.

Right.

> One could argue though if pm_runtime_mark_last_busy() should come
> before pm_runtime_put_autosuspend(). Usually it should, but in this
> case it doesn't matter because pm_runtime_allow() hasn't been called
> yet and the PCI core initializes devices to pm_runtime_forbid().

I'm going to change the code to look like following (pm_runtime_mark_last_busy()
gets called before pm_runtime_put_autosuspend() even if not strictly needed):

	pm_runtime_set_autosuspend_delay(&dev->dev, 100);
	pm_runtime_use_autosuspend(&dev->dev);
	pm_runtime_mark_last_busy(&dev->dev);
	pm_runtime_put_autosuspend(&dev->dev);
	pm_runtime_allow(&dev->dev);

Note I'm still increasing default autosuspend delay from 10ms to 100ms.

Does the above work for you?

Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Lukas Wunner]

On Wed, Jun 01, 2016 at 12:11:45PM +0300, Mika Westerberg wrote:
> I'm going to change the code to look like following (pm_runtime_mark_last_busy()
> gets called before pm_runtime_put_autosuspend() even if not strictly needed):
> 
> 	pm_runtime_set_autosuspend_delay(&dev->dev, 100);
> 	pm_runtime_use_autosuspend(&dev->dev);
> 	pm_runtime_mark_last_busy(&dev->dev);
> 	pm_runtime_put_autosuspend(&dev->dev);
> 	pm_runtime_allow(&dev->dev);
> 
> Note I'm still increasing default autosuspend delay from 10ms to 100ms.
> 
> Does the above work for you?

Yes, tested it and couldn't spot any issues.

Thanks,

Lukas

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Mika Westerberg]

On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote:
> > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan"
> > on the assumption that "PCI: Power on bridges before scanning new
> > devices" is sufficient to cover both the ACPI and the generic PCi
> > rescan cases, but I'd like some reassurance about that.
> 
> I agree with your reasoning that the patch should not be needed anymore.
> However, I have the machine which needed that patch at home so I'm not
> able to test it now. I'll do that later today when I get back home.

I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power
on bridges before scanning new devices" seems not to be enough. This
machine has SD-card reader connected to one PCIe port and once I unload
the sdhci-pci driver and enable runtime PM for the device, next system
suspend/resume cycle loses the SD-card reader PCI device.

I will investigate more tomorrow -- it is getting late here.

Anyway, maybe it is safer to postpone this series for v4.8 to give it
more testing time in -next.

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Rafael J. Wysocki]

On Wednesday, May 25, 2016 12:13:09 AM Mika Westerberg wrote:
> On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote:
> > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan"
> > > on the assumption that "PCI: Power on bridges before scanning new
> > > devices" is sufficient to cover both the ACPI and the generic PCi
> > > rescan cases, but I'd like some reassurance about that.
> > 
> > I agree with your reasoning that the patch should not be needed anymore.
> > However, I have the machine which needed that patch at home so I'm not
> > able to test it now. I'll do that later today when I get back home.
> 
> I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power
> on bridges before scanning new devices" seems not to be enough. This
> machine has SD-card reader connected to one PCIe port and once I unload
> the sdhci-pci driver and enable runtime PM for the device, next system
> suspend/resume cycle loses the SD-card reader PCI device.
> 
> I will investigate more tomorrow -- it is getting late here.
> 
> Anyway, maybe it is safer to postpone this series for v4.8 to give it
> more testing time in -next.

Agreed.

I don't see a compelling enough reason to rush these changes into 4.7
and it looks like there are gaps in our understanding of the issues
involved.

Thanks,
Rafael

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Mika Westerberg]

On Wed, May 25, 2016 at 12:13:09AM +0300, Mika Westerberg wrote:
> On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote:
> > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan"
> > > on the assumption that "PCI: Power on bridges before scanning new
> > > devices" is sufficient to cover both the ACPI and the generic PCi
> > > rescan cases, but I'd like some reassurance about that.
> > 
> > I agree with your reasoning that the patch should not be needed anymore.
> > However, I have the machine which needed that patch at home so I'm not
> > able to test it now. I'll do that later today when I get back home.
> 
> I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power
> on bridges before scanning new devices" seems not to be enough. This
> machine has SD-card reader connected to one PCIe port and once I unload
> the sdhci-pci driver and enable runtime PM for the device, next system
> suspend/resume cycle loses the SD-card reader PCI device.
> 
> I will investigate more tomorrow -- it is getting late here.

I think I found reason for the issue.

When the laptop resumes it will send ACPI BUS_CHECK event for the two
PCIe root ports. This ends up in acpiphp_check_bridge() where it goes
through all slots in that bridge checking if the devices are still
present. This happens before we call pci_scan_bridge() for the bridge
itself.

Since the bridge is in D3 config space of the device behind it is not
available and we determine that the device is not there anymore.

It looks like we either need that ACPI hotplug patch or alternatively we
could add pm_runtime_get/put() in acpiphp_check_bridge().

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Rafael J. Wysocki]

On Wednesday, May 25, 2016 04:19:48 PM Mika Westerberg wrote:
> On Wed, May 25, 2016 at 12:13:09AM +0300, Mika Westerberg wrote:
> > On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote:
> > > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan"
> > > > on the assumption that "PCI: Power on bridges before scanning new
> > > > devices" is sufficient to cover both the ACPI and the generic PCi
> > > > rescan cases, but I'd like some reassurance about that.
> > > 
> > > I agree with your reasoning that the patch should not be needed anymore.
> > > However, I have the machine which needed that patch at home so I'm not
> > > able to test it now. I'll do that later today when I get back home.
> > 
> > I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power
> > on bridges before scanning new devices" seems not to be enough. This
> > machine has SD-card reader connected to one PCIe port and once I unload
> > the sdhci-pci driver and enable runtime PM for the device, next system
> > suspend/resume cycle loses the SD-card reader PCI device.
> > 
> > I will investigate more tomorrow -- it is getting late here.
> 
> I think I found reason for the issue.
> 
> When the laptop resumes it will send ACPI BUS_CHECK event for the two
> PCIe root ports. This ends up in acpiphp_check_bridge() where it goes
> through all slots in that bridge checking if the devices are still
> present. This happens before we call pci_scan_bridge() for the bridge
> itself.
> 
> Since the bridge is in D3 config space of the device behind it is not
> available and we determine that the device is not there anymore.
> 
> It looks like we either need that ACPI hotplug patch or alternatively we
> could add pm_runtime_get/put() in acpiphp_check_bridge().

Have you tried the latter?

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Mika Westerberg]

On Wed, May 25, 2016 at 10:45:48PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, May 25, 2016 04:19:48 PM Mika Westerberg wrote:
> > On Wed, May 25, 2016 at 12:13:09AM +0300, Mika Westerberg wrote:
> > > On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote:
> > > > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan"
> > > > > on the assumption that "PCI: Power on bridges before scanning new
> > > > > devices" is sufficient to cover both the ACPI and the generic PCi
> > > > > rescan cases, but I'd like some reassurance about that.
> > > > 
> > > > I agree with your reasoning that the patch should not be needed anymore.
> > > > However, I have the machine which needed that patch at home so I'm not
> > > > able to test it now. I'll do that later today when I get back home.
> > > 
> > > I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power
> > > on bridges before scanning new devices" seems not to be enough. This
> > > machine has SD-card reader connected to one PCIe port and once I unload
> > > the sdhci-pci driver and enable runtime PM for the device, next system
> > > suspend/resume cycle loses the SD-card reader PCI device.
> > > 
> > > I will investigate more tomorrow -- it is getting late here.
> > 
> > I think I found reason for the issue.
> > 
> > When the laptop resumes it will send ACPI BUS_CHECK event for the two
> > PCIe root ports. This ends up in acpiphp_check_bridge() where it goes
> > through all slots in that bridge checking if the devices are still
> > present. This happens before we call pci_scan_bridge() for the bridge
> > itself.
> > 
> > Since the bridge is in D3 config space of the device behind it is not
> > available and we determine that the device is not there anymore.
> > 
> > It looks like we either need that ACPI hotplug patch or alternatively we
> > could add pm_runtime_get/put() in acpiphp_check_bridge().
> 
> Have you tried the latter?

Indeed I tried and it worked fine. I can make formal patch doing that
which then replaces the current ACPI hotplug patch, if that is the
preferred way.

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Rafael J. Wysocki]

On Thursday, May 26, 2016 11:16:43 AM Mika Westerberg wrote:
> On Wed, May 25, 2016 at 10:45:48PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, May 25, 2016 04:19:48 PM Mika Westerberg wrote:
> > > On Wed, May 25, 2016 at 12:13:09AM +0300, Mika Westerberg wrote:
> > > > On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote:
> > > > > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan"
> > > > > > on the assumption that "PCI: Power on bridges before scanning new
> > > > > > devices" is sufficient to cover both the ACPI and the generic PCi
> > > > > > rescan cases, but I'd like some reassurance about that.
> > > > > 
> > > > > I agree with your reasoning that the patch should not be needed anymore.
> > > > > However, I have the machine which needed that patch at home so I'm not
> > > > > able to test it now. I'll do that later today when I get back home.
> > > > 
> > > > I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power
> > > > on bridges before scanning new devices" seems not to be enough. This
> > > > machine has SD-card reader connected to one PCIe port and once I unload
> > > > the sdhci-pci driver and enable runtime PM for the device, next system
> > > > suspend/resume cycle loses the SD-card reader PCI device.
> > > > 
> > > > I will investigate more tomorrow -- it is getting late here.
> > > 
> > > I think I found reason for the issue.
> > > 
> > > When the laptop resumes it will send ACPI BUS_CHECK event for the two
> > > PCIe root ports. This ends up in acpiphp_check_bridge() where it goes
> > > through all slots in that bridge checking if the devices are still
> > > present. This happens before we call pci_scan_bridge() for the bridge
> > > itself.
> > > 
> > > Since the bridge is in D3 config space of the device behind it is not
> > > available and we determine that the device is not there anymore.
> > > 
> > > It looks like we either need that ACPI hotplug patch or alternatively we
> > > could add pm_runtime_get/put() in acpiphp_check_bridge().
> > 
> > Have you tried the latter?
> 
> Indeed I tried and it worked fine. I can make formal patch doing that
> which then replaces the current ACPI hotplug patch, if that is the
> preferred way.

It is somewhat cleaner, so I'd prefer it.

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Mika Westerberg]

On Sat, May 28, 2016 at 02:21:11PM +0200, Rafael J. Wysocki wrote:
> On Thursday, May 26, 2016 11:16:43 AM Mika Westerberg wrote:
> > On Wed, May 25, 2016 at 10:45:48PM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, May 25, 2016 04:19:48 PM Mika Westerberg wrote:
> > > > On Wed, May 25, 2016 at 12:13:09AM +0300, Mika Westerberg wrote:
> > > > > On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote:
> > > > > > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan"
> > > > > > > on the assumption that "PCI: Power on bridges before scanning new
> > > > > > > devices" is sufficient to cover both the ACPI and the generic PCi
> > > > > > > rescan cases, but I'd like some reassurance about that.
> > > > > > 
> > > > > > I agree with your reasoning that the patch should not be needed anymore.
> > > > > > However, I have the machine which needed that patch at home so I'm not
> > > > > > able to test it now. I'll do that later today when I get back home.
> > > > > 
> > > > > I tried now on my Lenovo Yoga 900 laptop and unfortunately "PCI: Power
> > > > > on bridges before scanning new devices" seems not to be enough. This
> > > > > machine has SD-card reader connected to one PCIe port and once I unload
> > > > > the sdhci-pci driver and enable runtime PM for the device, next system
> > > > > suspend/resume cycle loses the SD-card reader PCI device.
> > > > > 
> > > > > I will investigate more tomorrow -- it is getting late here.
> > > > 
> > > > I think I found reason for the issue.
> > > > 
> > > > When the laptop resumes it will send ACPI BUS_CHECK event for the two
> > > > PCIe root ports. This ends up in acpiphp_check_bridge() where it goes
> > > > through all slots in that bridge checking if the devices are still
> > > > present. This happens before we call pci_scan_bridge() for the bridge
> > > > itself.
> > > > 
> > > > Since the bridge is in D3 config space of the device behind it is not
> > > > available and we determine that the device is not there anymore.
> > > > 
> > > > It looks like we either need that ACPI hotplug patch or alternatively we
> > > > could add pm_runtime_get/put() in acpiphp_check_bridge().
> > > 
> > > Have you tried the latter?
> > 
> > Indeed I tried and it worked fine. I can make formal patch doing that
> > which then replaces the current ACPI hotplug patch, if that is the
> > preferred way.
> 
> It is somewhat cleaner, so I'd prefer it.

Okay, I'm going to submit an updated version of that patch then.

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Lukas Wunner]

Hi Mika,

On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote:
> One thing I noticed, though. When a bridge is transitioned to D0 we only
> wait for 10ms which is requirement for PCI functions. However, PCI PM
> specification 1.2 (chapter 4.2) requires that for buses to transition
> from B2 to B0 we need to wait minimum of 50ms before accessing a
> function on that bus.
> 
> We even have PCI_PM_BUS_WAIT defined in include/linux/pci.h but it is
> not used anywhere. Maybe it was not needed originally because we never
> powered down bridges anyway but now when we do, I think it is good idea
> to do what the spec requires.

The macro was introduced with

    commit aa8c6c93747f7b55fa11e1624fec8ca33763a805
    Author: Rafael J. Wysocki <rjw@sisk.pl>
    Date:   Fri Jan 16 21:54:43 2009 +0100
    PCI PM: Restore standard config registers of all devices early

but the only usage of it was removed with

    commit 476e7faefc43f106a90b5c96166c59b75de19d30
    Author: Rafael J. Wysocki <rjw@sisk.pl>
    Date:   Thu Jan 22 23:39:57 2009 +0100
    PCI PM: Do not wait for buses in B2 or B3 during resume

Best regards,

Lukas

>
> What do you think? We could add a separate patch doing something like
> below to make sure the spec is followed.
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e785dc260e72..b3b794caa380 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2361,7 +2361,12 @@ void pci_pm_init(struct pci_dev *dev)
>  	}
>  
>  	dev->pm_cap = pm;
> -	dev->d3_delay = PCI_PM_D3_WAIT;
> +	/*
> +	 * PCI PM 1.2 specification requires minimum of 50ms before any
> +	 * function on the bus is accessed after the bus is transitioned
> +	 * from B2 to B0.
> +	 */
> +	dev->d3_delay = pci_is_bridge(dev) ? PCI_PM_BUS_WAIT : PCI_PM_D3_WAIT;
>  	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
>  	dev->bridge_d3 = pci_bridge_d3_possible(dev);
>  	dev->d3cold_allowed = true;

Re: [PATCH] PCI: Power on bridges before scanning new devices

[Mika Westerberg]

On Wed, May 25, 2016 at 02:16:26PM +0200, Lukas Wunner wrote:
> Hi Mika,
> 
> On Tue, May 24, 2016 at 03:53:23PM +0300, Mika Westerberg wrote:
> > One thing I noticed, though. When a bridge is transitioned to D0 we only
> > wait for 10ms which is requirement for PCI functions. However, PCI PM
> > specification 1.2 (chapter 4.2) requires that for buses to transition
> > from B2 to B0 we need to wait minimum of 50ms before accessing a
> > function on that bus.
> > 
> > We even have PCI_PM_BUS_WAIT defined in include/linux/pci.h but it is
> > not used anywhere. Maybe it was not needed originally because we never
> > powered down bridges anyway but now when we do, I think it is good idea
> > to do what the spec requires.
> 
> The macro was introduced with
> 
>     commit aa8c6c93747f7b55fa11e1624fec8ca33763a805
>     Author: Rafael J. Wysocki <rjw@sisk.pl>
>     Date:   Fri Jan 16 21:54:43 2009 +0100
>     PCI PM: Restore standard config registers of all devices early
> 
> but the only usage of it was removed with
> 
>     commit 476e7faefc43f106a90b5c96166c59b75de19d30
>     Author: Rafael J. Wysocki <rjw@sisk.pl>
>     Date:   Thu Jan 22 23:39:57 2009 +0100
>     PCI PM: Do not wait for buses in B2 or B3 during resume
> 

Thanks for looking that up.

If I understand correctly it was removed because we do not expect BIOS
to put buses to B2/B3 so we do not need to wait the extra 50ms before
accessing the device.

However, now that we do that in the OS (and also during runtime), I
think we need to take that into account again.