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:

quoted

On Mon, May 30, 2016 at 4:44 PM, Mika Westerberg
[off-list ref] wrote:

quoted

On Mon, May 30, 2016 at 12:33:26PM +0300, Mika Westerberg wrote:

quoted

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

quoted

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.

quoted

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 :)