From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout1.hostsharing.net ([83.223.95.204]:60370 "EHLO mailout1.hostsharing.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751884AbcDTTTs (ORCPT ); Wed, 20 Apr 2016 15:19:48 -0400 Date: Wed, 20 Apr 2016 21:22:11 +0200 From: Lukas Wunner To: Mika Westerberg Cc: Bjorn Helgaas , "Rafael J. Wysocki" , Qipeng Zha , Qi Zheng , Dave Airlie , Mathias Nyman , Greg Kroah-Hartman , linux-pci@vger.kernel.org, linux-pm@vger.kernel.org, Andreas Noever Subject: Re: [PATCH v2 4/4] PCI: Add runtime PM support for PCIe ports Message-ID: <20160420192211.GA16093@wunner.de> References: <1460111790-92836-1-git-send-email-mika.westerberg@linux.intel.com> <1460111790-92836-5-git-send-email-mika.westerberg@linux.intel.com> <20160412175203.GB13637@wunner.de> <20160413083352.GI1714@lahna.fi.intel.com> <20160418143823.GA15165@wunner.de> <20160419123131.GE1725@lahna.fi.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <20160419123131.GE1725@lahna.fi.intel.com> Sender: linux-pci-owner@vger.kernel.org List-ID: 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