diff mbox

[v5,3/8] PCI: Don't block runtime PM for Thunderbolt host hotplug ports

Message ID be633737115c0be241fccd75bc136dd1da72c1b5.1484486499.git.lukas@wunner.de (mailing list archive)
State Changes Requested, archived
Headers show

Commit Message

Lukas Wunner Jan. 15, 2017, 8:03 p.m. UTC
Hotplug ports generally block their parents from suspending to D3hot as
otherwise their interrupts couldn't be delivered.

An exception are Thunderbolt host controllers:  They have a separate
GPIO pin to side-band signal plug events even if the controller is
powered down or its parent ports are suspended to D3.  They can be told
apart from Thunderbolt controllers in attached devices by checking if
they're situated below a non-Thunderbolt device (typically a root port,
or the downstream port of a PCIe switch in the case of the MacPro6,1).

To enable runtime PM for Thunderbolt on the Mac, the downstream bridges
of a host controller must not block runtime PM on the upstream bridge as
power to the chip is only cut once the upstream bridge has suspended.
Amend the condition in pci_dev_check_d3cold() accordingly.

This change does not impact non-Macs as their Thunderbolt hotplug ports
are handled by the firmware rather than natively by the OS:  The hotplug
ports are not allowed to suspend in pci_bridge_d3_possible() and keep
their parent ports awake all the time.  Consequently it is meaningless
whether they block runtime PM on their parent ports or not.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Andreas Noever <andreas.noever@gmail.com>
Cc: Tomas Winkler <tomas.winkler@intel.com>
Cc: Amir Levy <amir.jer.levy@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Comments

Mika Westerberg Jan. 16, 2017, 10:29 a.m. UTC | #1
On Sun, Jan 15, 2017 at 09:03:45PM +0100, Lukas Wunner wrote:
> Hotplug ports generally block their parents from suspending to D3hot as
> otherwise their interrupts couldn't be delivered.
> 
> An exception are Thunderbolt host controllers:  They have a separate
> GPIO pin to side-band signal plug events even if the controller is
> powered down or its parent ports are suspended to D3.  They can be told
> apart from Thunderbolt controllers in attached devices by checking if
> they're situated below a non-Thunderbolt device (typically a root port,
> or the downstream port of a PCIe switch in the case of the MacPro6,1).
> 
> To enable runtime PM for Thunderbolt on the Mac, the downstream bridges
> of a host controller must not block runtime PM on the upstream bridge as
> power to the chip is only cut once the upstream bridge has suspended.
> Amend the condition in pci_dev_check_d3cold() accordingly.
> 
> This change does not impact non-Macs as their Thunderbolt hotplug ports
> are handled by the firmware rather than natively by the OS:  The hotplug
> ports are not allowed to suspend in pci_bridge_d3_possible() and keep
> their parent ports awake all the time.  Consequently it is meaningless
> whether they block runtime PM on their parent ports or not.
> 
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Jan. 28, 2017, 11 p.m. UTC | #2
On Sun, Jan 15, 2017 at 09:03:45PM +0100, Lukas Wunner wrote:
> Hotplug ports generally block their parents from suspending to D3hot as
> otherwise their interrupts couldn't be delivered.

Well, the hotplug ports don't actually block anything.  We may decide
not to suspend the parent because normal PCIe hotplug ports deliver
interrupts through their parent, and if we suspend the parent to
D3hot, we won't get those interrupts.  I guess that's being pedantic.

> An exception are Thunderbolt host controllers:  They have a separate
> GPIO pin to side-band signal plug events even if the controller is
> powered down or its parent ports are suspended to D3.  They can be told
> apart from Thunderbolt controllers in attached devices by checking if
> they're situated below a non-Thunderbolt device (typically a root port,
> or the downstream port of a PCIe switch in the case of the MacPro6,1).
> 
> To enable runtime PM for Thunderbolt on the Mac, the downstream bridges
> of a host controller must not block runtime PM on the upstream bridge as
> power to the chip is only cut once the upstream bridge has suspended.
> Amend the condition in pci_dev_check_d3cold() accordingly.
> 
> This change does not impact non-Macs as their Thunderbolt hotplug ports
> are handled by the firmware rather than natively by the OS:  The hotplug
> ports are not allowed to suspend in pci_bridge_d3_possible() and keep
> their parent ports awake all the time.  Consequently it is meaningless
> whether they block runtime PM on their parent ports or not.

I think the references to Macs in the discussion above is confusing.
To cut power to a switch, I assume we have to suspend both upstream
and downstream ports whether it's plain PCIe or Thunderbolt, Mac or
not.

The "non-Mac Thunderbolt ports being handled by firmware" statement is
true today, but there's nothing intrinsic that makes it true; it could
change tomorrow.

> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Andreas Noever <andreas.noever@gmail.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Cc: Amir Levy <amir.jer.levy@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/pci/pci.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8ed098db82e1..e7a354cd13ce 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2269,6 +2269,24 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>  	return false;
>  }
>  
> +static bool pci_hotplug_bridge_may_wakeup(struct pci_dev *dev)
> +{
> +	struct pci_dev *parent, *grandparent;
> +
> +	/*
> +	 * Thunderbolt host controllers have a pin to side-band signal
> +	 * plug events.  Their hotplug ports are recognizable by having
> +	 * a non-Thunderbolt device as grandparent.

This comment should say something about the host controller vs.
attached device distinction you made in the changelog.

> +	 */
> +	if (dev->is_thunderbolt &&
> +	    (parent = pci_upstream_bridge(dev)) &&
> +	    (grandparent = pci_upstream_bridge(parent)) &&
> +	    !grandparent->is_thunderbolt)
> +		return true;
> +
> +	return false;
> +}
> +
>  static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
>  {
>  	bool *d3cold_ok = data;
> @@ -2284,7 +2302,7 @@ static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
>  	    !pci_power_manageable(dev) ||
>  
>  	    /* Hotplug interrupts cannot be delivered if the link is down. */

I think the point of this patch is that there are exceptions to the
statement in the comment, so please update the comment.

> -	    dev->is_hotplug_bridge)
> +	    (dev->is_hotplug_bridge && !pci_hotplug_bridge_may_wakeup(dev)))
>  
>  		*d3cold_ok = false;
>  
> -- 
> 2.11.0
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Feb. 10, 2017, 6:39 p.m. UTC | #3
On Sun, Jan 15, 2017 at 09:03:45PM +0100, Lukas Wunner wrote:
> Hotplug ports generally block their parents from suspending to D3hot as
> otherwise their interrupts couldn't be delivered.

This sounds related to PCIe r3.1, sec 5.3.1.4, which says functions
supporting PME generation from D3 must support it for both D3cold and
D3hot.  I think in PCIe, PMEs mean PME Messages, and the 5.3.1
implementation note says Messages are not affected by the PM state of
virtual bridges.

So that seems to say that hotplug ports *should* be able to deliver
PMEs even while in D3hot.

Maybe you're referring to the hotplug interrupts themselves, not the
PME?  I assume a hotplug event (presence detect, attention button,
etc) would first cause a PME, then the OS would return the path to D0,
then the hotplug interrupt would be delivered.

> An exception are Thunderbolt host controllers:  They have a separate
> GPIO pin to side-band signal plug events even if the controller is
> powered down or its parent ports are suspended to D3.  They can be told
> apart from Thunderbolt controllers in attached devices by checking if
> they're situated below a non-Thunderbolt device (typically a root port,
> or the downstream port of a PCIe switch in the case of the MacPro6,1).

In PCIe terms, does a "Thunderbolt host controller" look like a
downstream port that supports hotplug?

It seems like the PCIe PME mechanism *should* work pretty much like
this sideband GPIO.  But I might be reading the spec wrong.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Wunner Feb. 12, 2017, 5:13 p.m. UTC | #4
On Fri, Feb 10, 2017 at 12:39:12PM -0600, Bjorn Helgaas wrote:
> On Sun, Jan 15, 2017 at 09:03:45PM +0100, Lukas Wunner wrote:
> > Hotplug ports generally block their parents from suspending to D3hot as
> > otherwise their interrupts couldn't be delivered.
> 
> This sounds related to PCIe r3.1, sec 5.3.1.4, which says functions
> supporting PME generation from D3 must support it for both D3cold and
> D3hot.  I think in PCIe, PMEs mean PME Messages, and the 5.3.1
> implementation note says Messages are not affected by the PM state of
> virtual bridges.
> 
> So that seems to say that hotplug ports *should* be able to deliver
> PMEs even while in D3hot.
> 
> Maybe you're referring to the hotplug interrupts themselves, not the
> PME?  I assume a hotplug event (presence detect, attention button,
> etc) would first cause a PME, then the OS would return the path to D0,
> then the hotplug interrupt would be delivered.
> 
> > An exception are Thunderbolt host controllers:  They have a separate
> > GPIO pin to side-band signal plug events even if the controller is
> > powered down or its parent ports are suspended to D3.  They can be told
> > apart from Thunderbolt controllers in attached devices by checking if
> > they're situated below a non-Thunderbolt device (typically a root port,
> > or the downstream port of a PCIe switch in the case of the MacPro6,1).
> 
> In PCIe terms, does a "Thunderbolt host controller" look like a
> downstream port that supports hotplug?
> 
> It seems like the PCIe PME mechanism *should* work pretty much like
> this sideband GPIO.  But I might be reading the spec wrong.

I am dropping this patch in v6 of my Thunderbolt runpm series.

The "Light Ridge" Thunderbolt controller in my machine claims to support
PME, but its WAKE# pin is not connected.  (It's pulled up to 3.3V.)
I also have an external Thunderbolt chassis with the same controller,
and the controller likewise claims to support PME, but its WAKE# pin is
not connected to the PCIe root im my machine in any way.

This led me to the conclusion that PME is not reliable and therefore
in a Thunderbolt daisy chain, which is a cascade of PCIe switches,
only the hotplug port at the farthest end of the chain may suspend to
D3hot, whereas all the ports between it and the root bridge must stay
awake for interrupts to come through.

What I failed to see, due to poor understanding of the (undocumented)
Thunderbolt technology, is that even though ports in the middle of the
chain may be in D3hot and prevent PCIe interrupts from coming through,
the network of Converged I/O switches underlying the PCI tunnels
continues to function and deliver plug events.  It is thus the NHI
driver's job to runtime resume the PCIe switch where a CIO plug event
occurred, as well as its parents, to ensure delivery of PCIe interrupts.
The CIO plug event essentially serves as a sideband signal.  So far the
thunderbolt NHI driver does not support daisy chaining, so this is not
yet an issue.  (Only for tunnels established by the EFI driver.)

Thanks,

Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Feb. 13, 2017, 12:17 p.m. UTC | #5
On Sunday, February 12, 2017 06:13:01 PM Lukas Wunner wrote:
> On Fri, Feb 10, 2017 at 12:39:12PM -0600, Bjorn Helgaas wrote:
> > On Sun, Jan 15, 2017 at 09:03:45PM +0100, Lukas Wunner wrote:
> > > Hotplug ports generally block their parents from suspending to D3hot as
> > > otherwise their interrupts couldn't be delivered.
> > 
> > This sounds related to PCIe r3.1, sec 5.3.1.4, which says functions
> > supporting PME generation from D3 must support it for both D3cold and
> > D3hot.  I think in PCIe, PMEs mean PME Messages, and the 5.3.1
> > implementation note says Messages are not affected by the PM state of
> > virtual bridges.
> > 
> > So that seems to say that hotplug ports *should* be able to deliver
> > PMEs even while in D3hot.
> > 
> > Maybe you're referring to the hotplug interrupts themselves, not the
> > PME?  I assume a hotplug event (presence detect, attention button,
> > etc) would first cause a PME, then the OS would return the path to D0,
> > then the hotplug interrupt would be delivered.
> > 
> > > An exception are Thunderbolt host controllers:  They have a separate
> > > GPIO pin to side-band signal plug events even if the controller is
> > > powered down or its parent ports are suspended to D3.  They can be told
> > > apart from Thunderbolt controllers in attached devices by checking if
> > > they're situated below a non-Thunderbolt device (typically a root port,
> > > or the downstream port of a PCIe switch in the case of the MacPro6,1).
> > 
> > In PCIe terms, does a "Thunderbolt host controller" look like a
> > downstream port that supports hotplug?
> > 
> > It seems like the PCIe PME mechanism *should* work pretty much like
> > this sideband GPIO.  But I might be reading the spec wrong.
> 
> I am dropping this patch in v6 of my Thunderbolt runpm series.
> 
> The "Light Ridge" Thunderbolt controller in my machine claims to support
> PME, but its WAKE# pin is not connected.  (It's pulled up to 3.3V.)
> I also have an external Thunderbolt chassis with the same controller,
> and the controller likewise claims to support PME, but its WAKE# pin is
> not connected to the PCIe root im my machine in any way.

WAKE# should not be necessary if PME messages can be delivered in-band.

Of course, root ports still need to be able to signal PME wakeup via port
interrupts for this to work, and some kind of side-band wakeup signaling
may be necessary for waking up the system from sleep states via PME.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8ed098db82e1..e7a354cd13ce 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2269,6 +2269,24 @@  bool pci_bridge_d3_possible(struct pci_dev *bridge)
 	return false;
 }
 
+static bool pci_hotplug_bridge_may_wakeup(struct pci_dev *dev)
+{
+	struct pci_dev *parent, *grandparent;
+
+	/*
+	 * Thunderbolt host controllers have a pin to side-band signal
+	 * plug events.  Their hotplug ports are recognizable by having
+	 * a non-Thunderbolt device as grandparent.
+	 */
+	if (dev->is_thunderbolt &&
+	    (parent = pci_upstream_bridge(dev)) &&
+	    (grandparent = pci_upstream_bridge(parent)) &&
+	    !grandparent->is_thunderbolt)
+		return true;
+
+	return false;
+}
+
 static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
 {
 	bool *d3cold_ok = data;
@@ -2284,7 +2302,7 @@  static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
 	    !pci_power_manageable(dev) ||
 
 	    /* Hotplug interrupts cannot be delivered if the link is down. */
-	    dev->is_hotplug_bridge)
+	    (dev->is_hotplug_bridge && !pci_hotplug_bridge_may_wakeup(dev)))
 
 		*d3cold_ok = false;