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