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

[Lukas Wunner <lukas@wunner.de>]

Hi Mika,

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

Sure.


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

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

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

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


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

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

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

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


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

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


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

They've kept mum so far. :-)


Best regards,

Lukas