[v4] PCI / ACPI: PM: Take _S0W of the target bridge into account in acpi_pci_bridge_d3(()

Commit Message

Rafael J. Wysocki Jan. 12, 2023, 8:51 p.m. UTC

From: Rafael J. Wysocki <[e-mail]>

It is generally questionable to allow a PCI bridge to go into D3 if
it has _S0W returning D2 or a shallower power state, so modify
acpi_pci_bridge_d3(() to always take the return value of _S0W for the
target bridge into accout.  That is, make it return 'false' if _S0W
returns D2 or a shallower power state for the target bridge regardless
of its ancestor PCIe Root Port properties.  Of course, this also causes
'false' to be returned if the PCIe Root Port itself is the target and
its _S0W returns D2 or a shallower power state.

However, still allow bridges without _S0W that are power-manageable via
ACPI to enter D3 to retain the current code behavior in that case.

Link: https://lore.kernel.org/linux-pci/[e-mail]/
Reported-by: Mario Limonciello <[e-mail]>
Signed-off-by: Rafael J. Wysocki <[e-mail]>
---

v3 -> v4:
   * Use ACPI_STATE_D2 in the checks in acpi_pci_bridge_d3().

v2 -> v3:
   * Use rpadev for the ACPI companion of the Root Port in acpi_pci_bridge_d3(()
     to avoid confusion.
   * Make the function evaluating _S0W return the value produced by it or "unknown
     state" on errors and let its caller deal with that value.

---
 drivers/acpi/device_pm.c |   19 +++++++++++++++++++
 drivers/pci/pci-acpi.c   |   45 +++++++++++++++++++++++++++++++--------------
 include/acpi/acpi_bus.h  |    1 +
 3 files changed, 51 insertions(+), 14 deletions(-)

Comments

Bjorn Helgaas Jan. 12, 2023, 10:01 p.m. UTC | #1

On Thu, Jan 12, 2023 at 09:51:24PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[e-mail]>
> 
> It is generally questionable to allow a PCI bridge to go into D3 if
> it has _S0W returning D2 or a shallower power state, so modify
> acpi_pci_bridge_d3(() to always take the return value of _S0W for the
> target bridge into accout.  That is, make it return 'false' if _S0W
> returns D2 or a shallower power state for the target bridge regardless
> of its ancestor PCIe Root Port properties.  Of course, this also causes
> 'false' to be returned if the PCIe Root Port itself is the target and
> its _S0W returns D2 or a shallower power state.
> 
> However, still allow bridges without _S0W that are power-manageable via
> ACPI to enter D3 to retain the current code behavior in that case.
> 
> Link: https://lore.kernel.org/linux-pci/[e-mail]/
> Reported-by: Mario Limonciello <[e-mail]>
> Signed-off-by: Rafael J. Wysocki <[e-mail]>

Applied to pci/pm for v6.3, thanks!

It'd be great if we could include a short description of the problems
users might see.  I think the original problem was that on some AMD
systems we put a USB4 router in D3 when it should remain in D0.  And I
assume this means something doesn't wake up when it should?  Or maybe
we miss a hotplug event?

If somebody has an example or some text, I'll add it to the commit
log.

> ---
> 
> v3 -> v4:
>    * Use ACPI_STATE_D2 in the checks in acpi_pci_bridge_d3().
> 
> v2 -> v3:
>    * Use rpadev for the ACPI companion of the Root Port in acpi_pci_bridge_d3(()
>      to avoid confusion.
>    * Make the function evaluating _S0W return the value produced by it or "unknown
>      state" on errors and let its caller deal with that value.
> 
> ---
>  drivers/acpi/device_pm.c |   19 +++++++++++++++++++
>  drivers/pci/pci-acpi.c   |   45 +++++++++++++++++++++++++++++++--------------
>  include/acpi/acpi_bus.h  |    1 +
>  3 files changed, 51 insertions(+), 14 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-acpi.c
> +++ linux-pm/drivers/pci/pci-acpi.c
> @@ -976,24 +976,41 @@ bool acpi_pci_power_manageable(struct pc
>  bool acpi_pci_bridge_d3(struct pci_dev *dev)
>  {
>  	struct pci_dev *rpdev;
> -	struct acpi_device *adev;
> -	acpi_status status;
> -	unsigned long long state;
> +	struct acpi_device *adev, *rpadev;
>  	const union acpi_object *obj;
>  
>  	if (acpi_pci_disabled || !dev->is_hotplug_bridge)
>  		return false;
>  
> -	/* Assume D3 support if the bridge is power-manageable by ACPI. */
> -	if (acpi_pci_power_manageable(dev))
> -		return true;
> +	adev = ACPI_COMPANION(&dev->dev);
> +	if (adev) {
> +		/*
> +		 * If the bridge has _S0W, whether or not it can go into D3
> +		 * depends on what is returned by that object.  In particular,
> +		 * if the power state returned by _S0W is D2 or shallower,
> +		 * entering D3 should not be allowed.
> +		 */
> +		if (acpi_dev_power_state_for_wake(adev) <= ACPI_STATE_D2)
> +			return false;
> +
> +		/*
> +		 * Otherwise, assume that the bridge can enter D3 so long as it
> +		 * is power-manageable via ACPI.
> +		 */
> +		if (acpi_device_power_manageable(adev))
> +			return true;
> +	}
>  
>  	rpdev = pcie_find_root_port(dev);
>  	if (!rpdev)
>  		return false;
>  
> -	adev = ACPI_COMPANION(&rpdev->dev);
> -	if (!adev)
> +	if (rpdev == dev)
> +		rpadev = adev;
> +	else
> +		rpadev = ACPI_COMPANION(&rpdev->dev);
> +
> +	if (!rpadev)
>  		return false;
>  
>  	/*
> @@ -1001,15 +1018,15 @@ bool acpi_pci_bridge_d3(struct pci_dev *
>  	 * doesn't supply a wakeup GPE via _PRW, it cannot signal hotplug
>  	 * events from low-power states including D3hot and D3cold.
>  	 */
> -	if (!adev->wakeup.flags.valid)
> +	if (!rpadev->wakeup.flags.valid)
>  		return false;
>  
>  	/*
> -	 * If the Root Port cannot wake itself from D3hot or D3cold, we
> -	 * can't use D3.
> +	 * In the bridge-below-a-Root-Port case, evaluate _S0W for the Root Port
> +	 * to verify whether or not it can signal wakeup from D3.
>  	 */
> -	status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
> -	if (ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT)
> +	if (rpadev != adev &&
> +	    acpi_dev_power_state_for_wake(rpadev) <= ACPI_STATE_D2)
>  		return false;
>  
>  	/*
> @@ -1018,7 +1035,7 @@ bool acpi_pci_bridge_d3(struct pci_dev *
>  	 * bridges *below* that Root Port can also signal hotplug events
>  	 * while in D3.
>  	 */
> -	if (!acpi_dev_get_property(adev, "HotPlugSupportInD3",
> +	if (!acpi_dev_get_property(rpadev, "HotPlugSupportInD3",
>  				   ACPI_TYPE_INTEGER, &obj) &&
>  	    obj->integer.value == 1)
>  		return true;
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -484,6 +484,25 @@ void acpi_dev_power_up_children_with_adr
>  	acpi_dev_for_each_child(adev, acpi_power_up_if_adr_present, NULL);
>  }
>  
> +/**
> + * acpi_dev_power_state_for_wake - Deepest power state for wakeup signaling
> + * @adev: ACPI companion of the target device.
> + *
> + * Evaluate _S0W for @adev and return the value produced by it or return
> + * ACPI_STATE_UNKNOWN on errors (including _S0W not present).
> + */
> +u8 acpi_dev_power_state_for_wake(struct acpi_device *adev)
> +{
> +	unsigned long long state;
> +	acpi_status status;
> +
> +	status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
> +	if (ACPI_FAILURE(status))
> +		return ACPI_STATE_UNKNOWN;
> +
> +	return state;
> +}
> +
>  #ifdef CONFIG_PM
>  static DEFINE_MUTEX(acpi_pm_notifier_lock);
>  static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
> Index: linux-pm/include/acpi/acpi_bus.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_bus.h
> +++ linux-pm/include/acpi/acpi_bus.h
> @@ -533,6 +533,7 @@ int acpi_bus_update_power(acpi_handle ha
>  int acpi_device_update_power(struct acpi_device *device, int *state_p);
>  bool acpi_bus_power_manageable(acpi_handle handle);
>  void acpi_dev_power_up_children_with_adr(struct acpi_device *adev);
> +u8 acpi_dev_power_state_for_wake(struct acpi_device *adev);
>  int acpi_device_power_add_dependent(struct acpi_device *adev,
>  				    struct device *dev);
>  void acpi_device_power_remove_dependent(struct acpi_device *adev,
> 
> 
>

Mario Limonciello Jan. 12, 2023, 10:09 p.m. UTC | #2

[Public]



> -----Original Message-----
> From: Bjorn Helgaas <[e-mail]>
> Sent: Thursday, January 12, 2023 16:02
> To: Rafael J. Wysocki <[e-mail]>
> Cc: [e-mail]; Limonciello, Mario
> <[e-mail]>; Rafael J. Wysocki <[e-mail]>; Len
> Brown <[e-mail]>; Bjorn Helgaas <[e-mail]>; Mika
> Westerberg <[e-mail]>; Mehta, Sanju
> <[e-mail]>; Lukas Wunner <[e-mail]>; Rafael J .
> Wysocki <[e-mail]>; [e-mail]; linux-
> [e-mail]; Linux PM <[e-mail]>
> Subject: Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target bridge into
> account in acpi_pci_bridge_d3(()
> 
> On Thu, Jan 12, 2023 at 09:51:24PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[e-mail]>
> >
> > It is generally questionable to allow a PCI bridge to go into D3 if
> > it has _S0W returning D2 or a shallower power state, so modify
> > acpi_pci_bridge_d3(() to always take the return value of _S0W for the
> > target bridge into accout.  That is, make it return 'false' if _S0W
> > returns D2 or a shallower power state for the target bridge regardless
> > of its ancestor PCIe Root Port properties.  Of course, this also causes
> > 'false' to be returned if the PCIe Root Port itself is the target and
> > its _S0W returns D2 or a shallower power state.
> >
> > However, still allow bridges without _S0W that are power-manageable via
> > ACPI to enter D3 to retain the current code behavior in that case.
> >
> > Link: https://lore.kernel.org/linux-pci/20221031223356.32570-1-
> [e-mail]/
> > Reported-by: Mario Limonciello <[e-mail]>
> > Signed-off-by: Rafael J. Wysocki <[e-mail]>
> 
> Applied to pci/pm for v6.3, thanks!
> 
> It'd be great if we could include a short description of the problems
> users might see.  I think the original problem was that on some AMD
> systems we put a USB4 router in D3 when it should remain in D0.  And I
> assume this means something doesn't wake up when it should?  Or maybe
> we miss a hotplug event?
> 
> If somebody has an example or some text, I'll add it to the commit
> log.

Here's a blurb for what happens on AMD side:

When the platform is configured to not allow the PCIe port used for tunneling
to wakeup from D3 it will runtime suspend into D0 and the USB4 controller
which is a consumer will runtime suspend into D3.

This inconsistency leads to failures to initialize PCIe tunnels for USB4 devices.

> 
> > ---
> >
> > v3 -> v4:
> >    * Use ACPI_STATE_D2 in the checks in acpi_pci_bridge_d3().
> >
> > v2 -> v3:
> >    * Use rpadev for the ACPI companion of the Root Port in
> acpi_pci_bridge_d3(()
> >      to avoid confusion.
> >    * Make the function evaluating _S0W return the value produced by it or
> "unknown
> >      state" on errors and let its caller deal with that value.
> >
> > ---
> >  drivers/acpi/device_pm.c |   19 +++++++++++++++++++
> >  drivers/pci/pci-acpi.c   |   45 +++++++++++++++++++++++++++++++-------
> -------
> >  include/acpi/acpi_bus.h  |    1 +
> >  3 files changed, 51 insertions(+), 14 deletions(-)
> >
> > Index: linux-pm/drivers/pci/pci-acpi.c
> >
> ==========================================================
> =========
> > --- linux-pm.orig/drivers/pci/pci-acpi.c
> > +++ linux-pm/drivers/pci/pci-acpi.c
> > @@ -976,24 +976,41 @@ bool acpi_pci_power_manageable(struct pc
> >  bool acpi_pci_bridge_d3(struct pci_dev *dev)
> >  {
> >  	struct pci_dev *rpdev;
> > -	struct acpi_device *adev;
> > -	acpi_status status;
> > -	unsigned long long state;
> > +	struct acpi_device *adev, *rpadev;
> >  	const union acpi_object *obj;
> >
> >  	if (acpi_pci_disabled || !dev->is_hotplug_bridge)
> >  		return false;
> >
> > -	/* Assume D3 support if the bridge is power-manageable by ACPI. */
> > -	if (acpi_pci_power_manageable(dev))
> > -		return true;
> > +	adev = ACPI_COMPANION(&dev->dev);
> > +	if (adev) {
> > +		/*
> > +		 * If the bridge has _S0W, whether or not it can go into D3
> > +		 * depends on what is returned by that object.  In particular,
> > +		 * if the power state returned by _S0W is D2 or shallower,
> > +		 * entering D3 should not be allowed.
> > +		 */
> > +		if (acpi_dev_power_state_for_wake(adev) <=
> ACPI_STATE_D2)
> > +			return false;
> > +
> > +		/*
> > +		 * Otherwise, assume that the bridge can enter D3 so long as
> it
> > +		 * is power-manageable via ACPI.
> > +		 */
> > +		if (acpi_device_power_manageable(adev))
> > +			return true;
> > +	}
> >
> >  	rpdev = pcie_find_root_port(dev);
> >  	if (!rpdev)
> >  		return false;
> >
> > -	adev = ACPI_COMPANION(&rpdev->dev);
> > -	if (!adev)
> > +	if (rpdev == dev)
> > +		rpadev = adev;
> > +	else
> > +		rpadev = ACPI_COMPANION(&rpdev->dev);
> > +
> > +	if (!rpadev)
> >  		return false;
> >
> >  	/*
> > @@ -1001,15 +1018,15 @@ bool acpi_pci_bridge_d3(struct pci_dev *
> >  	 * doesn't supply a wakeup GPE via _PRW, it cannot signal hotplug
> >  	 * events from low-power states including D3hot and D3cold.
> >  	 */
> > -	if (!adev->wakeup.flags.valid)
> > +	if (!rpadev->wakeup.flags.valid)
> >  		return false;
> >
> >  	/*
> > -	 * If the Root Port cannot wake itself from D3hot or D3cold, we
> > -	 * can't use D3.
> > +	 * In the bridge-below-a-Root-Port case, evaluate _S0W for the Root
> Port
> > +	 * to verify whether or not it can signal wakeup from D3.
> >  	 */
> > -	status = acpi_evaluate_integer(adev->handle, "_S0W", NULL,
> &state);
> > -	if (ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT)
> > +	if (rpadev != adev &&
> > +	    acpi_dev_power_state_for_wake(rpadev) <= ACPI_STATE_D2)
> >  		return false;
> >
> >  	/*
> > @@ -1018,7 +1035,7 @@ bool acpi_pci_bridge_d3(struct pci_dev *
> >  	 * bridges *below* that Root Port can also signal hotplug events
> >  	 * while in D3.
> >  	 */
> > -	if (!acpi_dev_get_property(adev, "HotPlugSupportInD3",
> > +	if (!acpi_dev_get_property(rpadev, "HotPlugSupportInD3",
> >  				   ACPI_TYPE_INTEGER, &obj) &&
> >  	    obj->integer.value == 1)
> >  		return true;
> > Index: linux-pm/drivers/acpi/device_pm.c
> >
> ==========================================================
> =========
> > --- linux-pm.orig/drivers/acpi/device_pm.c
> > +++ linux-pm/drivers/acpi/device_pm.c
> > @@ -484,6 +484,25 @@ void acpi_dev_power_up_children_with_adr
> >  	acpi_dev_for_each_child(adev, acpi_power_up_if_adr_present,
> NULL);
> >  }
> >
> > +/**
> > + * acpi_dev_power_state_for_wake - Deepest power state for wakeup
> signaling
> > + * @adev: ACPI companion of the target device.
> > + *
> > + * Evaluate _S0W for @adev and return the value produced by it or return
> > + * ACPI_STATE_UNKNOWN on errors (including _S0W not present).
> > + */
> > +u8 acpi_dev_power_state_for_wake(struct acpi_device *adev)
> > +{
> > +	unsigned long long state;
> > +	acpi_status status;
> > +
> > +	status = acpi_evaluate_integer(adev->handle, "_S0W", NULL,
> &state);
> > +	if (ACPI_FAILURE(status))
> > +		return ACPI_STATE_UNKNOWN;
> > +
> > +	return state;
> > +}
> > +
> >  #ifdef CONFIG_PM
> >  static DEFINE_MUTEX(acpi_pm_notifier_lock);
> >  static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
> > Index: linux-pm/include/acpi/acpi_bus.h
> >
> ==========================================================
> =========
> > --- linux-pm.orig/include/acpi/acpi_bus.h
> > +++ linux-pm/include/acpi/acpi_bus.h
> > @@ -533,6 +533,7 @@ int acpi_bus_update_power(acpi_handle ha
> >  int acpi_device_update_power(struct acpi_device *device, int *state_p);
> >  bool acpi_bus_power_manageable(acpi_handle handle);
> >  void acpi_dev_power_up_children_with_adr(struct acpi_device *adev);
> > +u8 acpi_dev_power_state_for_wake(struct acpi_device *adev);
> >  int acpi_device_power_add_dependent(struct acpi_device *adev,
> >  				    struct device *dev);
> >  void acpi_device_power_remove_dependent(struct acpi_device *adev,
> >
> >
> >

Bjorn Helgaas Jan. 12, 2023, 10:40 p.m. UTC | #3

On Thu, Jan 12, 2023 at 10:09:21PM +0000, Limonciello, Mario wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <[e-mail]>
> > Sent: Thursday, January 12, 2023 16:02
> > To: Rafael J. Wysocki <[e-mail]>
> > Cc: [e-mail]; Limonciello, Mario
> > <[e-mail]>; Rafael J. Wysocki <[e-mail]>; Len
> > Brown <[e-mail]>; Bjorn Helgaas <[e-mail]>; Mika
> > Westerberg <[e-mail]>; Mehta, Sanju
> > <[e-mail]>; Lukas Wunner <[e-mail]>; Rafael J .
> > Wysocki <[e-mail]>; [e-mail]; linux-
> > [e-mail]; Linux PM <[e-mail]>
> > Subject: Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target bridge into
> > account in acpi_pci_bridge_d3(()
> > 
> > On Thu, Jan 12, 2023 at 09:51:24PM +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[e-mail]>
> > >
> > > It is generally questionable to allow a PCI bridge to go into D3 if
> > > it has _S0W returning D2 or a shallower power state, so modify
> > > acpi_pci_bridge_d3(() to always take the return value of _S0W for the
> > > target bridge into accout.  That is, make it return 'false' if _S0W
> > > returns D2 or a shallower power state for the target bridge regardless
> > > of its ancestor PCIe Root Port properties.  Of course, this also causes
> > > 'false' to be returned if the PCIe Root Port itself is the target and
> > > its _S0W returns D2 or a shallower power state.
> > >
> > > However, still allow bridges without _S0W that are power-manageable via
> > > ACPI to enter D3 to retain the current code behavior in that case.
> > >
> > > Link: https://lore.kernel.org/linux-pci/20221031223356.32570-1-
> > [e-mail]/
> > > Reported-by: Mario Limonciello <[e-mail]>
> > > Signed-off-by: Rafael J. Wysocki <[e-mail]>
> > 
> > Applied to pci/pm for v6.3, thanks!
> > 
> > It'd be great if we could include a short description of the problems
> > users might see.  I think the original problem was that on some AMD
> > systems we put a USB4 router in D3 when it should remain in D0.  And I
> > assume this means something doesn't wake up when it should?  Or maybe
> > we miss a hotplug event?
> > 
> > If somebody has an example or some text, I'll add it to the commit
> > log.
> 
> Here's a blurb for what happens on AMD side:
> 
> When the platform is configured to not allow the PCIe port used for
> tunneling to wakeup from D3 it will runtime suspend into D0 and the
> USB4 controller which is a consumer will runtime suspend into D3.
> 
> This inconsistency leads to failures to initialize PCIe tunnels for
> USB4 devices.

And what is J. Random User going to see?  DisplayPort not working
ever?  It works to begin with, but not after a suspend?  Devices in a
dock not being able to wake the system?

I don't really know what "PCIe tunnels for USB4 devices not being
initialized" means for me.  I want to know what a problem report from
a non-expert user might look like.

Bjorn

Mario Limonciello Jan. 12, 2023, 10:45 p.m. UTC | #4

[AMD Official Use Only - General]



> -----Original Message-----
> From: Bjorn Helgaas <[e-mail]>
> Sent: Thursday, January 12, 2023 16:41
> To: Limonciello, Mario <[e-mail]>
> Cc: Rafael J. Wysocki <[e-mail]>; [e-mail]; Rafael J.
> Wysocki <[e-mail]>; Len Brown <[e-mail]>; Bjorn Helgaas
> <[e-mail]>; Mika Westerberg
> <[e-mail]>; Mehta, Sanju <[e-mail]>;
> Lukas Wunner <[e-mail]>; Rafael J . Wysocki
> <[e-mail]>; [e-mail]; linux-
> [e-mail]; Linux PM <[e-mail]>
> Subject: Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target bridge into
> account in acpi_pci_bridge_d3(()
> 
> On Thu, Jan 12, 2023 at 10:09:21PM +0000, Limonciello, Mario wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <[e-mail]>
> > > Sent: Thursday, January 12, 2023 16:02
> > > To: Rafael J. Wysocki <[e-mail]>
> > > Cc: [e-mail]; Limonciello, Mario
> > > <[e-mail]>; Rafael J. Wysocki <[e-mail]>;
> Len
> > > Brown <[e-mail]>; Bjorn Helgaas <[e-mail]>; Mika
> > > Westerberg <[e-mail]>; Mehta, Sanju
> > > <[e-mail]>; Lukas Wunner <[e-mail]>; Rafael J .
> > > Wysocki <[e-mail]>; [e-mail]; linux-
> > > [e-mail]; Linux PM <[e-mail]>
> > > Subject: Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target bridge into
> > > account in acpi_pci_bridge_d3(()
> > >
> > > On Thu, Jan 12, 2023 at 09:51:24PM +0100, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <[e-mail]>
> > > >
> > > > It is generally questionable to allow a PCI bridge to go into D3 if
> > > > it has _S0W returning D2 or a shallower power state, so modify
> > > > acpi_pci_bridge_d3(() to always take the return value of _S0W for the
> > > > target bridge into accout.  That is, make it return 'false' if _S0W
> > > > returns D2 or a shallower power state for the target bridge regardless
> > > > of its ancestor PCIe Root Port properties.  Of course, this also causes
> > > > 'false' to be returned if the PCIe Root Port itself is the target and
> > > > its _S0W returns D2 or a shallower power state.
> > > >
> > > > However, still allow bridges without _S0W that are power-manageable via
> > > > ACPI to enter D3 to retain the current code behavior in that case.
> > > >
> > > > Link: https://lore.kernel.org/linux-pci/20221031223356.32570-1-
> > > [e-mail]/
> > > > Reported-by: Mario Limonciello <[e-mail]>
> > > > Signed-off-by: Rafael J. Wysocki <[e-mail]>
> > >
> > > Applied to pci/pm for v6.3, thanks!
> > >
> > > It'd be great if we could include a short description of the problems
> > > users might see.  I think the original problem was that on some AMD
> > > systems we put a USB4 router in D3 when it should remain in D0.  And I
> > > assume this means something doesn't wake up when it should?  Or maybe
> > > we miss a hotplug event?
> > >
> > > If somebody has an example or some text, I'll add it to the commit
> > > log.
> >
> > Here's a blurb for what happens on AMD side:
> >
> > When the platform is configured to not allow the PCIe port used for
> > tunneling to wakeup from D3 it will runtime suspend into D0 and the
> > USB4 controller which is a consumer will runtime suspend into D3.
> >
> > This inconsistency leads to failures to initialize PCIe tunnels for
> > USB4 devices.
> 
> And what is J. Random User going to see?  DisplayPort not working
> ever?  It works to begin with, but not after a suspend?  Devices in a
> dock not being able to wake the system?
> 
> I don't really know what "PCIe tunnels for USB4 devices not being
> initialized" means for me.  I want to know what a problem report from
> a non-expert user might look like.
> 

DP tunnels aren't affected, so monitors should still work.

In terms of what doesn't work it depends on the architecture of the
the connected device.  Here's some concrete up-leveled examples:

USB4 docks contain that a PCIe network adapter and that adapter won't
work when the dock is plugged in after the system boot.

USB4 docks that contain a USB network adapter should work properly,
but downstream USB4 or TBT3 devices connected to that USB4 dock will
not work when the device or dock is plugged in after the system boots.

TBT3 storage devices connected after the system has booted will not work.

Thanks,

Bjorn Helgaas Jan. 13, 2023, 5:51 p.m. UTC | #5

On Thu, Jan 12, 2023 at 10:45:03PM +0000, Limonciello, Mario wrote:
> [AMD Official Use Only - General]
> 
> 
> 
> > -----Original Message-----
> > From: Bjorn Helgaas <[e-mail]>
> > Sent: Thursday, January 12, 2023 16:41
> > To: Limonciello, Mario <[e-mail]>
> > Cc: Rafael J. Wysocki <[e-mail]>; [e-mail]; Rafael J.
> > Wysocki <[e-mail]>; Len Brown <[e-mail]>; Bjorn Helgaas
> > <[e-mail]>; Mika Westerberg
> > <[e-mail]>; Mehta, Sanju <[e-mail]>;
> > Lukas Wunner <[e-mail]>; Rafael J . Wysocki
> > <[e-mail]>; [e-mail]; linux-
> > [e-mail]; Linux PM <[e-mail]>
> > Subject: Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target bridge into
> > account in acpi_pci_bridge_d3(()
> > 
> > On Thu, Jan 12, 2023 at 10:09:21PM +0000, Limonciello, Mario wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <[e-mail]>
> > > > Sent: Thursday, January 12, 2023 16:02
> > > > To: Rafael J. Wysocki <[e-mail]>
> > > > Cc: [e-mail]; Limonciello, Mario
> > > > <[e-mail]>; Rafael J. Wysocki <[e-mail]>;
> > Len
> > > > Brown <[e-mail]>; Bjorn Helgaas <[e-mail]>; Mika
> > > > Westerberg <[e-mail]>; Mehta, Sanju
> > > > <[e-mail]>; Lukas Wunner <[e-mail]>; Rafael J .
> > > > Wysocki <[e-mail]>; [e-mail]; linux-
> > > > [e-mail]; Linux PM <[e-mail]>
> > > > Subject: Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target bridge into
> > > > account in acpi_pci_bridge_d3(()
> > > >
> > > > On Thu, Jan 12, 2023 at 09:51:24PM +0100, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <[e-mail]>
> > > > >
> > > > > It is generally questionable to allow a PCI bridge to go into D3 if
> > > > > it has _S0W returning D2 or a shallower power state, so modify
> > > > > acpi_pci_bridge_d3(() to always take the return value of _S0W for the
> > > > > target bridge into accout.  That is, make it return 'false' if _S0W
> > > > > returns D2 or a shallower power state for the target bridge regardless
> > > > > of its ancestor PCIe Root Port properties.  Of course, this also causes
> > > > > 'false' to be returned if the PCIe Root Port itself is the target and
> > > > > its _S0W returns D2 or a shallower power state.
> > > > >
> > > > > However, still allow bridges without _S0W that are power-manageable via
> > > > > ACPI to enter D3 to retain the current code behavior in that case.
> > > > >
> > > > > Link: https://lore.kernel.org/linux-pci/20221031223356.32570-1-
> > > > [e-mail]/
> > > > > Reported-by: Mario Limonciello <[e-mail]>
> > > > > Signed-off-by: Rafael J. Wysocki <[e-mail]>
> > > >
> > > > Applied to pci/pm for v6.3, thanks!
> > > >
> > > > It'd be great if we could include a short description of the problems
> > > > users might see.  I think the original problem was that on some AMD
> > > > systems we put a USB4 router in D3 when it should remain in D0.  And I
> > > > assume this means something doesn't wake up when it should?  Or maybe
> > > > we miss a hotplug event?
> > > >
> > > > If somebody has an example or some text, I'll add it to the commit
> > > > log.
> > >
> > > Here's a blurb for what happens on AMD side:
> > >
> > > When the platform is configured to not allow the PCIe port used for
> > > tunneling to wakeup from D3 it will runtime suspend into D0 and the
> > > USB4 controller which is a consumer will runtime suspend into D3.
> > >
> > > This inconsistency leads to failures to initialize PCIe tunnels for
> > > USB4 devices.
> > 
> > And what is J. Random User going to see?  DisplayPort not working
> > ever?  It works to begin with, but not after a suspend?  Devices in a
> > dock not being able to wake the system?
> > 
> > I don't really know what "PCIe tunnels for USB4 devices not being
> > initialized" means for me.  I want to know what a problem report from
> > a non-expert user might look like.
> 
> DP tunnels aren't affected, so monitors should still work.
> 
> In terms of what doesn't work it depends on the architecture of the
> the connected device.  Here's some concrete up-leveled examples:
> 
> USB4 docks contain that a PCIe network adapter and that adapter won't
> work when the dock is plugged in after the system boot.
> 
> USB4 docks that contain a USB network adapter should work properly,
> but downstream USB4 or TBT3 devices connected to that USB4 dock will
> not work when the device or dock is plugged in after the system boots.
> 
> TBT3 storage devices connected after the system has booted will not work.

Thanks, this is exactly the sort of thing I was looking for.  Since
they all mention "connected after boot," I assume the issue with the
current code is that a hotplug notification is being missed because a
bridge is in D3.

If the devices are present and enumerated at boot, there's no issue
with suspend/resume, right?

What do you think of the following possible text?  I don't want to be
overly specific because I don't think it's practical to list every
scenario.  We just need a hook to make people think "Hmm, I'm seeing a
dock issue; maybe this is the fix."

  This fixes problems where a hotplug notification is missed because a
  bridge is in D3.  That means hot-added devices such as USB4 docks
  (and the devices they contain) and Thunderbolt 3 devices may not
  work.

Mario Limonciello Jan. 13, 2023, 5:53 p.m. UTC | #6

[Public]



> -----Original Message-----
> From: Bjorn Helgaas <[e-mail]>
> Sent: Friday, January 13, 2023 11:52
> To: Limonciello, Mario <[e-mail]>
> Cc: Rafael J. Wysocki <[e-mail]>; [e-mail]; Rafael
> J. Wysocki <[e-mail]>; Len Brown <[e-mail]>; Bjorn
> Helgaas <[e-mail]>; Mika Westerberg
> <[e-mail]>; Mehta, Sanju
> <[e-mail]>; Lukas Wunner <[e-mail]>; Rafael J .
> Wysocki <[e-mail]>; [e-mail]; linux-
> [e-mail]; Linux PM <[e-mail]>
> Subject: Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target bridge into
> account in acpi_pci_bridge_d3(()
> 
> On Thu, Jan 12, 2023 at 10:45:03PM +0000, Limonciello, Mario wrote:
> > [AMD Official Use Only - General]
> >
> >
> >
> > > -----Original Message-----
> > > From: Bjorn Helgaas <[e-mail]>
> > > Sent: Thursday, January 12, 2023 16:41
> > > To: Limonciello, Mario <[e-mail]>
> > > Cc: Rafael J. Wysocki <[e-mail]>; [e-mail];
> Rafael J.
> > > Wysocki <[e-mail]>; Len Brown <[e-mail]>; Bjorn
> Helgaas
> > > <[e-mail]>; Mika Westerberg
> > > <[e-mail]>; Mehta, Sanju
> <[e-mail]>;
> > > Lukas Wunner <[e-mail]>; Rafael J . Wysocki
> > > <[e-mail]>; [e-mail]; linux-
> > > [e-mail]; Linux PM <[e-mail]>
> > > Subject: Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target bridge
> into
> > > account in acpi_pci_bridge_d3(()
> > >
> > > On Thu, Jan 12, 2023 at 10:09:21PM +0000, Limonciello, Mario wrote:
> > > > > -----Original Message-----
> > > > > From: Bjorn Helgaas <[e-mail]>
> > > > > Sent: Thursday, January 12, 2023 16:02
> > > > > To: Rafael J. Wysocki <[e-mail]>
> > > > > Cc: [e-mail]; Limonciello, Mario
> > > > > <[e-mail]>; Rafael J. Wysocki
> <[e-mail]>;
> > > Len
> > > > > Brown <[e-mail]>; Bjorn Helgaas <[e-mail]>;
> Mika
> > > > > Westerberg <[e-mail]>; Mehta, Sanju
> > > > > <[e-mail]>; Lukas Wunner <[e-mail]>;
> Rafael J .
> > > > > Wysocki <[e-mail]>; [e-mail];
> linux-
> > > > > [e-mail]; Linux PM <[e-mail]>
> > > > > Subject: Re: [PATCH v4] PCI / ACPI: PM: Take _S0W of the target
> bridge into
> > > > > account in acpi_pci_bridge_d3(()
> > > > >
> > > > > On Thu, Jan 12, 2023 at 09:51:24PM +0100, Rafael J. Wysocki wrote:
> > > > > > From: Rafael J. Wysocki <[e-mail]>
> > > > > >
> > > > > > It is generally questionable to allow a PCI bridge to go into D3 if
> > > > > > it has _S0W returning D2 or a shallower power state, so modify
> > > > > > acpi_pci_bridge_d3(() to always take the return value of _S0W for
> the
> > > > > > target bridge into accout.  That is, make it return 'false' if _S0W
> > > > > > returns D2 or a shallower power state for the target bridge
> regardless
> > > > > > of its ancestor PCIe Root Port properties.  Of course, this also causes
> > > > > > 'false' to be returned if the PCIe Root Port itself is the target and
> > > > > > its _S0W returns D2 or a shallower power state.
> > > > > >
> > > > > > However, still allow bridges without _S0W that are power-
> manageable via
> > > > > > ACPI to enter D3 to retain the current code behavior in that case.
> > > > > >
> > > > > > Link: https://lore.kernel.org/linux-pci/20221031223356.32570-1-
> > > > > [e-mail]/
> > > > > > Reported-by: Mario Limonciello <[e-mail]>
> > > > > > Signed-off-by: Rafael J. Wysocki <[e-mail]>
> > > > >
> > > > > Applied to pci/pm for v6.3, thanks!
> > > > >
> > > > > It'd be great if we could include a short description of the problems
> > > > > users might see.  I think the original problem was that on some AMD
> > > > > systems we put a USB4 router in D3 when it should remain in D0.  And
> I
> > > > > assume this means something doesn't wake up when it should?  Or
> maybe
> > > > > we miss a hotplug event?
> > > > >
> > > > > If somebody has an example or some text, I'll add it to the commit
> > > > > log.
> > > >
> > > > Here's a blurb for what happens on AMD side:
> > > >
> > > > When the platform is configured to not allow the PCIe port used for
> > > > tunneling to wakeup from D3 it will runtime suspend into D0 and the
> > > > USB4 controller which is a consumer will runtime suspend into D3.
> > > >
> > > > This inconsistency leads to failures to initialize PCIe tunnels for
> > > > USB4 devices.
> > >
> > > And what is J. Random User going to see?  DisplayPort not working
> > > ever?  It works to begin with, but not after a suspend?  Devices in a
> > > dock not being able to wake the system?
> > >
> > > I don't really know what "PCIe tunnels for USB4 devices not being
> > > initialized" means for me.  I want to know what a problem report from
> > > a non-expert user might look like.
> >
> > DP tunnels aren't affected, so monitors should still work.
> >
> > In terms of what doesn't work it depends on the architecture of the
> > the connected device.  Here's some concrete up-leveled examples:
> >
> > USB4 docks contain that a PCIe network adapter and that adapter won't
> > work when the dock is plugged in after the system boot.
> >
> > USB4 docks that contain a USB network adapter should work properly,
> > but downstream USB4 or TBT3 devices connected to that USB4 dock will
> > not work when the device or dock is plugged in after the system boots.
> >
> > TBT3 storage devices connected after the system has booted will not work.
> 
> Thanks, this is exactly the sort of thing I was looking for.  Since
> they all mention "connected after boot," I assume the issue with the
> current code is that a hotplug notification is being missed because a
> bridge is in D3.

Yeah.

> 
> If the devices are present and enumerated at boot, there's no issue
> with suspend/resume, right?
> 

I intentionally left that out because it's going to be conditional based on
a lot of other things.

> What do you think of the following possible text?  I don't want to be
> overly specific because I don't think it's practical to list every
> scenario.  We just need a hook to make people think "Hmm, I'm seeing a
> dock issue; maybe this is the fix."
> 
>   This fixes problems where a hotplug notification is missed because a
>   bridge is in D3.  That means hot-added devices such as USB4 docks
>   (and the devices they contain) and Thunderbolt 3 devices may not
>   work.

Perfect.

Patch

Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -976,24 +976,41 @@  bool acpi_pci_power_manageable(struct pc
 bool acpi_pci_bridge_d3(struct pci_dev *dev)
 {
 	struct pci_dev *rpdev;
-	struct acpi_device *adev;
-	acpi_status status;
-	unsigned long long state;
+	struct acpi_device *adev, *rpadev;
 	const union acpi_object *obj;
 
 	if (acpi_pci_disabled || !dev->is_hotplug_bridge)
 		return false;
 
-	/* Assume D3 support if the bridge is power-manageable by ACPI. */
-	if (acpi_pci_power_manageable(dev))
-		return true;
+	adev = ACPI_COMPANION(&dev->dev);
+	if (adev) {
+		/*
+		 * If the bridge has _S0W, whether or not it can go into D3
+		 * depends on what is returned by that object.  In particular,
+		 * if the power state returned by _S0W is D2 or shallower,
+		 * entering D3 should not be allowed.
+		 */
+		if (acpi_dev_power_state_for_wake(adev) <= ACPI_STATE_D2)
+			return false;
+
+		/*
+		 * Otherwise, assume that the bridge can enter D3 so long as it
+		 * is power-manageable via ACPI.
+		 */
+		if (acpi_device_power_manageable(adev))
+			return true;
+	}
 
 	rpdev = pcie_find_root_port(dev);
 	if (!rpdev)
 		return false;
 
-	adev = ACPI_COMPANION(&rpdev->dev);
-	if (!adev)
+	if (rpdev == dev)
+		rpadev = adev;
+	else
+		rpadev = ACPI_COMPANION(&rpdev->dev);
+
+	if (!rpadev)
 		return false;
 
 	/*
@@ -1001,15 +1018,15 @@  bool acpi_pci_bridge_d3(struct pci_dev *
 	 * doesn't supply a wakeup GPE via _PRW, it cannot signal hotplug
 	 * events from low-power states including D3hot and D3cold.
 	 */
-	if (!adev->wakeup.flags.valid)
+	if (!rpadev->wakeup.flags.valid)
 		return false;
 
 	/*
-	 * If the Root Port cannot wake itself from D3hot or D3cold, we
-	 * can't use D3.
+	 * In the bridge-below-a-Root-Port case, evaluate _S0W for the Root Port
+	 * to verify whether or not it can signal wakeup from D3.
 	 */
-	status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
-	if (ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT)
+	if (rpadev != adev &&
+	    acpi_dev_power_state_for_wake(rpadev) <= ACPI_STATE_D2)
 		return false;
 
 	/*
@@ -1018,7 +1035,7 @@  bool acpi_pci_bridge_d3(struct pci_dev *
 	 * bridges *below* that Root Port can also signal hotplug events
 	 * while in D3.
 	 */
-	if (!acpi_dev_get_property(adev, "HotPlugSupportInD3",
+	if (!acpi_dev_get_property(rpadev, "HotPlugSupportInD3",
 				   ACPI_TYPE_INTEGER, &obj) &&
 	    obj->integer.value == 1)
 		return true;
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -484,6 +484,25 @@  void acpi_dev_power_up_children_with_adr
 	acpi_dev_for_each_child(adev, acpi_power_up_if_adr_present, NULL);
 }
 
+/**
+ * acpi_dev_power_state_for_wake - Deepest power state for wakeup signaling
+ * @adev: ACPI companion of the target device.
+ *
+ * Evaluate _S0W for @adev and return the value produced by it or return
+ * ACPI_STATE_UNKNOWN on errors (including _S0W not present).
+ */
+u8 acpi_dev_power_state_for_wake(struct acpi_device *adev)
+{
+	unsigned long long state;
+	acpi_status status;
+
+	status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
+	if (ACPI_FAILURE(status))
+		return ACPI_STATE_UNKNOWN;
+
+	return state;
+}
+
 #ifdef CONFIG_PM
 static DEFINE_MUTEX(acpi_pm_notifier_lock);
 static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -533,6 +533,7 @@  int acpi_bus_update_power(acpi_handle ha
 int acpi_device_update_power(struct acpi_device *device, int *state_p);
 bool acpi_bus_power_manageable(acpi_handle handle);
 void acpi_dev_power_up_children_with_adr(struct acpi_device *adev);
+u8 acpi_dev_power_state_for_wake(struct acpi_device *adev);
 int acpi_device_power_add_dependent(struct acpi_device *adev,
 				    struct device *dev);
 void acpi_device_power_remove_dependent(struct acpi_device *adev,