diff mbox series

[32/32] PCI: Whitelist Thunderbolt ports for runtime D3

Message ID 10206fe7f3967aa73ade5b24dc729de0e94c3b7f.1529173804.git.lukas@wunner.de
State Accepted
Delegated to: Bjorn Helgaas
Headers show
Series Rework pciehp event handling & add runtime PM | expand

Commit Message

Lukas Wunner June 16, 2018, 7:25 p.m. UTC 
Thunderbolt controllers can be runtime suspended to D3cold to save ~1.5W.
This requires that runtime D3 is allowed on its PCIe ports, so whitelist
them.

The 2015 BIOS cutoff that we've instituted for runtime D3 on PCIe ports
is unnecessary on Thunderbolt because we know that even the oldest
controller, Light Ridge (2010), is able to suspend its ports to D3 just
fine -- specifically including its hotplug ports.  And the power saving
should be afforded to machines even if their BIOS predates 2015.

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Andreas Noever <andreas.noever@gmail.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Mika Westerberg June 21, 2018, 11:13 a.m. UTC | #1 
On Sat, Jun 16, 2018 at 09:25:00PM +0200, Lukas Wunner wrote:
> +		/* Even the oldest 2010 Thunderbolt controller supports D3. */
> +		if (bridge->is_thunderbolt)
> +			return true;

I have a small concern here. In PC world native PCIe hotplug used with
Thunderbolt comes in two flavors:

  - Native PCI hotplug without runtime PM
  - Native PCI hotplug with runtime PM

The former works so that even if it uses native PCIe hotplug, the power
management is done so that the Thunderbolt host router is hot-removed
when there is nothing connected (and that brings the power savings).
With the above change we start putting all Thunderbolt PCIe hotplug
ports to D3 runtime. While this probably works (and I tested it on the
same Dell) I don't think Windows does it and it may lead to a path that
has not been tested very thoroughly by OEMs.

So I wonder if it makes sense to restrict this particular check to Apple
hardware at this point?
Lukas Wunner July 18, 2018, 7:30 p.m. UTC | #2 
Hi Mika,

sorry for the delay!

On Thu, Jun 21, 2018 at 02:13:54PM +0300, Mika Westerberg wrote:
> On Sat, Jun 16, 2018 at 09:25:00PM +0200, Lukas Wunner wrote:
> > +		/* Even the oldest 2010 Thunderbolt controller supports D3. */
> > +		if (bridge->is_thunderbolt)
> > +			return true;
> 
> I have a small concern here. In PC world native PCIe hotplug used with
> Thunderbolt comes in two flavors:
> 
>   - Native PCI hotplug without runtime PM
>   - Native PCI hotplug with runtime PM
> 
> The former works so that even if it uses native PCIe hotplug, the power
> management is done so that the Thunderbolt host router is hot-removed
> when there is nothing connected (and that brings the power savings).
> With the above change we start putting all Thunderbolt PCIe hotplug
> ports to D3 runtime. While this probably works (and I tested it on the
> same Dell) I don't think Windows does it and it may lead to a path that
> has not been tested very thoroughly by OEMs.
> 
> So I wonder if it makes sense to restrict this particular check to Apple
> hardware at this point?

I'm not familiar at all with Windows, so this your call.  Normally I'd say,
if there's no known case which forces us to constrain this to Apple, we
probably shouldn't be overzealous.  OTOH I imagine you have to deal with
Windows and broken BIOSes on a daily basis and can anticipate if problems
are to be expected.

The "host router" is the NHI, right?  So IIUC all downstream ports and
the upstream port of the TB PCIe switch may go to D3hot, same for the
root port.  And your concern is that hotplug addition or removal of the
NHI might not work if those ports are in D3hot?

What I don't get is, if we constrain this to Apple, TB ports on non-Apple
systems may not runtime suspend (e.g. if the OEM botched the BIOS date
to be < 2015) and then the "Native PCI hotplug with runtime PM" case
will be broken, won't it?

Thanks,

Lukas
Mika Westerberg July 20, 2018, 3:23 p.m. UTC | #3 
On Wed, Jul 18, 2018 at 09:30:53PM +0200, Lukas Wunner wrote:
> Hi Mika,
> 
> sorry for the delay!
> 
> On Thu, Jun 21, 2018 at 02:13:54PM +0300, Mika Westerberg wrote:
> > On Sat, Jun 16, 2018 at 09:25:00PM +0200, Lukas Wunner wrote:
> > > +		/* Even the oldest 2010 Thunderbolt controller supports D3. */
> > > +		if (bridge->is_thunderbolt)
> > > +			return true;
> > 
> > I have a small concern here. In PC world native PCIe hotplug used with
> > Thunderbolt comes in two flavors:
> > 
> >   - Native PCI hotplug without runtime PM
> >   - Native PCI hotplug with runtime PM
> > 
> > The former works so that even if it uses native PCIe hotplug, the power
> > management is done so that the Thunderbolt host router is hot-removed
> > when there is nothing connected (and that brings the power savings).
> > With the above change we start putting all Thunderbolt PCIe hotplug
> > ports to D3 runtime. While this probably works (and I tested it on the
> > same Dell) I don't think Windows does it and it may lead to a path that
> > has not been tested very thoroughly by OEMs.
> > 
> > So I wonder if it makes sense to restrict this particular check to Apple
> > hardware at this point?
> 
> I'm not familiar at all with Windows, so this your call.  Normally I'd say,
> if there's no known case which forces us to constrain this to Apple, we
> probably shouldn't be overzealous.  OTOH I imagine you have to deal with
> Windows and broken BIOSes on a daily basis and can anticipate if problems
> are to be expected.
> 
> The "host router" is the NHI, right?  So IIUC all downstream ports and
> the upstream port of the TB PCIe switch may go to D3hot, same for the
> root port.  And your concern is that hotplug addition or removal of the
> NHI might not work if those ports are in D3hot?

Host router is the PCIe switch + NHI + xHCI. My concern is that now we
start putting the downstream hotplug ports of the host router (the ports
where TBT PCIe daisy chain is to be extented) to D3hot even when the
whole power management of the "Native PCI hotplug without runtime PM" is
done pretty much by hot-removing the whole host router if there is
nothing connected.

> What I don't get is, if we constrain this to Apple, TB ports on non-Apple
> systems may not runtime suspend (e.g. if the OEM botched the BIOS date
> to be < 2015) and then the "Native PCI hotplug with runtime PM" case
> will be broken, won't it?

Yes, but it needs additional patches anyway and we are going to use ACPI
_DSD with "HotplugInD3" property to selectively enable this on such
systems.
Mika Westerberg July 20, 2018, 4 p.m. UTC | #4 
On Fri, Jul 20, 2018 at 06:23:49PM +0300, Mika Westerberg wrote:
> On Wed, Jul 18, 2018 at 09:30:53PM +0200, Lukas Wunner wrote:
> > Hi Mika,
> > 
> > sorry for the delay!
> > 
> > On Thu, Jun 21, 2018 at 02:13:54PM +0300, Mika Westerberg wrote:
> > > On Sat, Jun 16, 2018 at 09:25:00PM +0200, Lukas Wunner wrote:
> > > > +		/* Even the oldest 2010 Thunderbolt controller supports D3. */
> > > > +		if (bridge->is_thunderbolt)
> > > > +			return true;
> > > 
> > > I have a small concern here. In PC world native PCIe hotplug used with
> > > Thunderbolt comes in two flavors:
> > > 
> > >   - Native PCI hotplug without runtime PM
> > >   - Native PCI hotplug with runtime PM
> > > 
> > > The former works so that even if it uses native PCIe hotplug, the power
> > > management is done so that the Thunderbolt host router is hot-removed
> > > when there is nothing connected (and that brings the power savings).
> > > With the above change we start putting all Thunderbolt PCIe hotplug
> > > ports to D3 runtime. While this probably works (and I tested it on the
> > > same Dell) I don't think Windows does it and it may lead to a path that
> > > has not been tested very thoroughly by OEMs.
> > > 
> > > So I wonder if it makes sense to restrict this particular check to Apple
> > > hardware at this point?
> > 
> > I'm not familiar at all with Windows, so this your call.  Normally I'd say,
> > if there's no known case which forces us to constrain this to Apple, we
> > probably shouldn't be overzealous.  OTOH I imagine you have to deal with
> > Windows and broken BIOSes on a daily basis and can anticipate if problems
> > are to be expected.
> > 
> > The "host router" is the NHI, right?  So IIUC all downstream ports and
> > the upstream port of the TB PCIe switch may go to D3hot, same for the
> > root port.  And your concern is that hotplug addition or removal of the
> > NHI might not work if those ports are in D3hot?
> 
> Host router is the PCIe switch + NHI + xHCI. My concern is that now we
> start putting the downstream hotplug ports of the host router (the ports
> where TBT PCIe daisy chain is to be extented) to D3hot even when the
> whole power management of the "Native PCI hotplug without runtime PM" is
> done pretty much by hot-removing the whole host router if there is
> nothing connected.

I see Bjorn already merged these so please ignore the above. We can
add the Apple check later if this turns out causing problems.
Bjorn Helgaas July 20, 2018, 8:33 p.m. UTC | #5 
On Fri, Jul 20, 2018 at 07:00:38PM +0300, Mika Westerberg wrote:
> On Fri, Jul 20, 2018 at 06:23:49PM +0300, Mika Westerberg wrote:
> > On Wed, Jul 18, 2018 at 09:30:53PM +0200, Lukas Wunner wrote:
> > > Hi Mika,
> > > 
> > > sorry for the delay!
> > > 
> > > On Thu, Jun 21, 2018 at 02:13:54PM +0300, Mika Westerberg wrote:
> > > > On Sat, Jun 16, 2018 at 09:25:00PM +0200, Lukas Wunner wrote:
> > > > > +		/* Even the oldest 2010 Thunderbolt controller supports D3. */
> > > > > +		if (bridge->is_thunderbolt)
> > > > > +			return true;
> > > > 
> > > > I have a small concern here. In PC world native PCIe hotplug used with
> > > > Thunderbolt comes in two flavors:
> > > > 
> > > >   - Native PCI hotplug without runtime PM
> > > >   - Native PCI hotplug with runtime PM
> > > > 
> > > > The former works so that even if it uses native PCIe hotplug, the power
> > > > management is done so that the Thunderbolt host router is hot-removed
> > > > when there is nothing connected (and that brings the power savings).
> > > > With the above change we start putting all Thunderbolt PCIe hotplug
> > > > ports to D3 runtime. While this probably works (and I tested it on the
> > > > same Dell) I don't think Windows does it and it may lead to a path that
> > > > has not been tested very thoroughly by OEMs.
> > > > 
> > > > So I wonder if it makes sense to restrict this particular check to Apple
> > > > hardware at this point?
> > > 
> > > I'm not familiar at all with Windows, so this your call.  Normally I'd say,
> > > if there's no known case which forces us to constrain this to Apple, we
> > > probably shouldn't be overzealous.  OTOH I imagine you have to deal with
> > > Windows and broken BIOSes on a daily basis and can anticipate if problems
> > > are to be expected.
> > > 
> > > The "host router" is the NHI, right?  So IIUC all downstream ports and
> > > the upstream port of the TB PCIe switch may go to D3hot, same for the
> > > root port.  And your concern is that hotplug addition or removal of the
> > > NHI might not work if those ports are in D3hot?
> > 
> > Host router is the PCIe switch + NHI + xHCI. My concern is that now we
> > start putting the downstream hotplug ports of the host router (the ports
> > where TBT PCIe daisy chain is to be extented) to D3hot even when the
> > whole power management of the "Native PCI hotplug without runtime PM" is
> > done pretty much by hot-removing the whole host router if there is
> > nothing connected.
> 
> I see Bjorn already merged these so please ignore the above. We can
> add the Apple check later if this turns out causing problems.

I merged these just to try to make forward progress and dig out from
my backlog.  If you want to tweak this before the merge window, I'd be
glad to update the branch.

Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 4099a6c14b6d..c4d10726c59a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2290,7 +2290,7 @@  void pci_config_pm_runtime_put(struct pci_dev *pdev)
  * @bridge: Bridge to check
  *
  * This function checks if it is possible to move the bridge to D3.
- * Currently we only allow D3 for recent enough PCIe ports.
+ * Currently we only allow D3 for recent enough PCIe ports and Thunderbolt.
  */
 bool pci_bridge_d3_possible(struct pci_dev *bridge)
 {
@@ -2314,6 +2314,10 @@  bool pci_bridge_d3_possible(struct pci_dev *bridge)
 		if (pci_bridge_d3_force)
 			return true;
 
+		/* Even the oldest 2010 Thunderbolt controller supports D3. */
+		if (bridge->is_thunderbolt)
+			return true;
+
 		/*
 		 * Hotplug ports handled natively by the OS were not validated
 		 * by vendors for runtime D3 at least until 2018 because there