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