[v5] PCI/ACPI: PCI/ACPI: Validate devices with power resources support D3

  Firmware typically advertises that ACPI devices that represent PCIe
devices can support D3 by a combination of the value returned by
_S0W as well as the HotPlugSupportInD3 _DSD [1].

`acpi_pci_bridge_d3` looks for this combination but also contains
an assumption that if an ACPI device contains power resources the PCIe
device it's associated with can support D3.  This was introduced
from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
D3 if power managed by ACPI").

Some firmware configurations for "AMD Pink Sardine" do not support
wake from D3 in _S0W for the ACPI device representing the PCIe root
port used for tunneling. The PCIe device will still be opted into
runtime PM in the kernel [2] because of the logic within
`acpi_pci_bridge_d3`. This currently happens because the ACPI
device contains power resources.

When the thunderbolt driver is loaded two device links are created:
* USB4 router <- PCIe root port for tunneling
* USB4 router <- XHCI PCIe device

These device links are created because the ACPI devices declare the
`usb4-host-interface` _DSD [3]. For both links the USB4 router is the
supplier and these other devices are the consumers.
Here is a demonstration of this topology that occurs:

|
├─ 00:03.1
|       | "PCIe root port used for tunneling"
|       | ACPI Path: \_SB_.PCI0.GP11
|       | ACPI Power Resources: Yes
|       | ACPI _S0W return value: 0
|       | Device Links: supplier:pci:0000:c4:00.5
|       └─ PCIe Power state: D0
└─ 00:08.3
        | ACPI Path: \_SB_.PCI0.GP19
        ├─ PCIe Power state: D0
        ├─ c4:00.3
        |       | "XHCI PCIe device used for tunneling"
        |       | ACPI Path: \_SB_.PCI0.GP19.XHC3
        |       | ACPI Power Resources: Yes
        |       | ACPI _S0W return value: 4
        |       | Device Links: supplier:pci:0000:c4:00.5
        |       └─ PCIe Power state: D3cold
        └─ c4:00.5
                | "USB4 Router"
                | ACPI Path: \_SB_.PCI0.GP19.NHI0
                | ACPI Power Resources: Yes
                | ACPI _S0W return value: 4
                | Device Links: consumer:pci:0000:00:03.1 consumer:pci:0000:c4:00.3
                └─ PCIe Power state: D3cold

Currently runtime PM is allowed for all of these devices.  This means that
when all consumers are idle long enough, they will runtime suspend to
enter their deepest allowed sleep state. Once all consumers are in their
deepest allowed sleep state the suppliers will enter the deepest sleep
state as well.

* The PCIe root port for tunneling doesn't support waking from D3hot or
  D3cold so although runtime suspended it stays in D0.
* The XHCI PCIe device supports wakeup from D3cold so it runtime suspends
  to D3cold.
* Both consumers are in their deepest state and the USB4 router supports
  wakeup from D3cold, so it runtime suspends into this state.

At least for AMD's case the hardware designer's expectation is the USB4
router should have also remained in D0 since the PCIe root port for
tunneling remained in D0 and a device link exists between the two devices.
The current Linux behavior is undefined.

Instead of making the assertion that the device can support D3 (and thus
runtime PM) solely from the presence of ACPI power resources, move the check
to later on in the function, which will have validated that the ACPI device
supports wake from D3hot or D3cold.

This fix prevents the USB4 router being put into D3 when the firmware
says that ACPI device representing the PCIe root port for tunneling can't
handle it while still allowing ACPI devices that don't have the
HotplugSupportInD3 _DSD to also enter D3 if they have power resources that
can wake from D3.

Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-pcie-root-ports-supporting-hot-plug-in-d3 [1]
Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pcie/portdrv_pci.c?id=v6.0#n126 [2]
Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thunderbolt/acpi.c?id=v6.0#n29 [3]
Fixes: dff6139015dc6 ("PCI/ACPI: Allow D3 only if Root Port can signal and wake from D3")
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v4->v5:
 * Update github->git.kernel.org
 * Correct arrow direction
 * Clarify some commit message comments from Lukas' feedback.
v3->v4:
 * Pick up tags
 * Add more details to the commit message
v2->v3:
 * Reword commit message
v1->v2:
 * Just return value of acpi_pci_power_manageable (Rafael)
 * Remove extra word in commit message
---
 drivers/pci/pci-acpi.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
  

On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> Firmware typically advertises that ACPI devices that represent PCIe
> devices can support D3 by a combination of the value returned by
> _S0W as well as the HotPlugSupportInD3 _DSD [1].
> 
> `acpi_pci_bridge_d3` looks for this combination but also contains
> an assumption that if an ACPI device contains power resources the PCIe
> device it's associated with can support D3.  This was introduced
> from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> D3 if power managed by ACPI").
> 
> Some firmware configurations for "AMD Pink Sardine" do not support
> wake from D3 in _S0W for the ACPI device representing the PCIe root
> port used for tunneling. The PCIe device will still be opted into
> runtime PM in the kernel [2] because of the logic within
> `acpi_pci_bridge_d3`. This currently happens because the ACPI
> device contains power resources.
> 
> When the thunderbolt driver is loaded two device links are created:
> * USB4 router <- PCIe root port for tunneling
> * USB4 router <- XHCI PCIe device
> 
> These device links are created because the ACPI devices declare the
> `usb4-host-interface` _DSD [3]. For both links the USB4 router is the
> supplier and these other devices are the consumers.
> Here is a demonstration of this topology that occurs:
> 
> |
> ├─ 00:03.1
> |       | "PCIe root port used for tunneling"
> |       | ACPI Path: \_SB_.PCI0.GP11
> |       | ACPI Power Resources: Yes

I guess this means it has _PR0 and/or _PS0?  (same for devices below)

> |       | ACPI _S0W return value: 0
> |       | Device Links: supplier:pci:0000:c4:00.5
> |       └─ PCIe Power state: D0

What bus does 00:03.1 lead to?  I guess it doesn't lead to any of the
devices below?

> └─ 00:08.3
>         | ACPI Path: \_SB_.PCI0.GP19
>         ├─ PCIe Power state: D0

I guess 00:08.3 is a Root Port leading to bus c4?

>         ├─ c4:00.3
>         |       | "XHCI PCIe device used for tunneling"
>         |       | ACPI Path: \_SB_.PCI0.GP19.XHC3
>         |       | ACPI Power Resources: Yes
>         |       | ACPI _S0W return value: 4
>         |       | Device Links: supplier:pci:0000:c4:00.5
>         |       └─ PCIe Power state: D3cold
>         └─ c4:00.5
>                 | "USB4 Router"
>                 | ACPI Path: \_SB_.PCI0.GP19.NHI0
>                 | ACPI Power Resources: Yes
>                 | ACPI _S0W return value: 4
>                 | Device Links: consumer:pci:0000:00:03.1 consumer:pci:0000:c4:00.3
>                 └─ PCIe Power state: D3cold

I'm reading the excellent Documentation/driver-api/device_link.rst and
trying to match up with this topology.  This is a case where 00:03.1
is a consumer of c4:00.5.  The driver core automatically enforces that
children are suspended before parents, but since 00:03.1 is an aunt
(not a child) of c4:00.5, the device link enforces the requirement
that 00:03.1 be suspended before c4:00.5.  Right?

Is c4:00.5 an NHI?

The PCI power states shown above are the failure case?  c4:00.5
*should* remain in D0 as long as 00:03.1 is in D0, but was instead put
in D3cold?

> Currently runtime PM is allowed for all of these devices.

This is because they all have _PR0 and/or _PS0?  (Diagram doesn't show
that for 00:08.3, but I assume it must?)

And I guess they all must have dev->is_hotplug_bridge set?

> This means that
> when all consumers are idle long enough, they will runtime suspend to
> enter their deepest allowed sleep state. Once all consumers are in their
> deepest allowed sleep state the suppliers will enter the deepest sleep
> state as well.
> 
> * The PCIe root port for tunneling doesn't support waking from D3hot or
>   D3cold so although runtime suspended it stays in D0.
> * The XHCI PCIe device supports wakeup from D3cold so it runtime suspends
>   to D3cold.
> * Both consumers are in their deepest state and the USB4 router supports
>   wakeup from D3cold, so it runtime suspends into this state.
> 
> At least for AMD's case the hardware designer's expectation is the USB4
> router should have also remained in D0 since the PCIe root port for
> tunneling remained in D0 and a device link exists between the two devices.
> The current Linux behavior is undefined.

Is the requirement that the supplier can never be in a lower-power
state than the consumer?

I guess that's *not* actually a requirement even though that's the
effect of this patch in this situation.  If it *were* that simple, we
would just check the supplier and consumer power states instead of
looking at all these ACPI properties.

> Instead of making the assertion that the device can support D3 (and thus
> runtime PM) solely from the presence of ACPI power resources, move the check
> to later on in the function, which will have validated that the ACPI device
> supports wake from D3hot or D3cold.
> 
> This fix prevents the USB4 router being put into D3 when the firmware
> says that ACPI device representing the PCIe root port for tunneling can't
> handle it while still allowing ACPI devices that don't have the
> HotplugSupportInD3 _DSD to also enter D3 if they have power resources that
> can wake from D3.

So I guess this changes what acpi_pci_bridge_d3() returns for 00:03.1?
Previously it returned "true" because it has _PR0/_PS0 (so
acpi_pci_power_manageable() is true), but now it will return "false"
because _S0W says it can only wake from D0?

> Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-pcie-root-ports-supporting-hot-plug-in-d3 [1]
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pcie/portdrv_pci.c?id=v6.0#n126 [2]
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thunderbolt/acpi.c?id=v6.0#n29 [3]
> Fixes: dff6139015dc6 ("PCI/ACPI: Allow D3 only if Root Port can signal and wake from D3")
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v4->v5:
>  * Update github->git.kernel.org
>  * Correct arrow direction
>  * Clarify some commit message comments from Lukas' feedback.
> v3->v4:
>  * Pick up tags
>  * Add more details to the commit message
> v2->v3:
>  * Reword commit message
> v1->v2:
>  * Just return value of acpi_pci_power_manageable (Rafael)
>  * Remove extra word in commit message
> ---
>  drivers/pci/pci-acpi.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index a46fec776ad77..8c6aec50dd471 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -984,10 +984,6 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>  	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;
> -
>  	rpdev = pcie_find_root_port(dev);
>  	if (!rpdev)
>  		return false;
> @@ -1023,7 +1019,8 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>  	    obj->integer.value == 1)
>  		return true;
>  
> -	return false;
> +	/* Assume D3 support if the bridge is power-manageable by ACPI. */
> +	return acpi_pci_power_manageable(dev);
>  }
>  
>  int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> -- 
> 2.34.1
>
  

On 11/11/2022 11:41, Bjorn Helgaas wrote:
> On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
>> Firmware typically advertises that ACPI devices that represent PCIe
>> devices can support D3 by a combination of the value returned by
>> _S0W as well as the HotPlugSupportInD3 _DSD [1].
>>
>> `acpi_pci_bridge_d3` looks for this combination but also contains
>> an assumption that if an ACPI device contains power resources the PCIe
>> device it's associated with can support D3.  This was introduced
>> from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
>> D3 if power managed by ACPI").
>>
>> Some firmware configurations for "AMD Pink Sardine" do not support
>> wake from D3 in _S0W for the ACPI device representing the PCIe root
>> port used for tunneling. The PCIe device will still be opted into
>> runtime PM in the kernel [2] because of the logic within
>> `acpi_pci_bridge_d3`. This currently happens because the ACPI
>> device contains power resources.
>>
>> When the thunderbolt driver is loaded two device links are created:
>> * USB4 router <- PCIe root port for tunneling
>> * USB4 router <- XHCI PCIe device
>>
>> These device links are created because the ACPI devices declare the
>> `usb4-host-interface` _DSD [3]. For both links the USB4 router is the
>> supplier and these other devices are the consumers.
>> Here is a demonstration of this topology that occurs:
>>
>> |
>> ├─ 00:03.1
>> |       | "PCIe root port used for tunneling"
>> |       | ACPI Path: \_SB_.PCI0.GP11
>> |       | ACPI Power Resources: Yes
> 
> I guess this means it has _PR0 and/or _PS0?  (same for devices below)

Yeah.

> 
>> |       | ACPI _S0W return value: 0
>> |       | Device Links: supplier:pci:0000:c4:00.5
>> |       └─ PCIe Power state: D0
> 
> What bus does 00:03.1 lead to?  I guess it doesn't lead to any of the
> devices below?

By default - nothing.  The topology was drawn correctly.

When you plug in a USB4 device with PCIe the newly enumerated devices 
hang off this bus.

> 
>> └─ 00:08.3
>>          | ACPI Path: \_SB_.PCI0.GP19
>>          ├─ PCIe Power state: D0
> 
> I guess 00:08.3 is a Root Port leading to bus c4?
> 

Right.

>>          ├─ c4:00.3
>>          |       | "XHCI PCIe device used for tunneling"
>>          |       | ACPI Path: \_SB_.PCI0.GP19.XHC3
>>          |       | ACPI Power Resources: Yes
>>          |       | ACPI _S0W return value: 4
>>          |       | Device Links: supplier:pci:0000:c4:00.5
>>          |       └─ PCIe Power state: D3cold
>>          └─ c4:00.5
>>                  | "USB4 Router"
>>                  | ACPI Path: \_SB_.PCI0.GP19.NHI0
>>                  | ACPI Power Resources: Yes
>>                  | ACPI _S0W return value: 4
>>                  | Device Links: consumer:pci:0000:00:03.1 consumer:pci:0000:c4:00.3
>>                  └─ PCIe Power state: D3cold
> 
> I'm reading the excellent Documentation/driver-api/device_link.rst and
> trying to match up with this topology.  This is a case where 00:03.1
> is a consumer of c4:00.5.  The driver core automatically enforces that
> children are suspended before parents, but since 00:03.1 is an aunt
> (not a child) of c4:00.5, the device link enforces the requirement
> that 00:03.1 be suspended before c4:00.5.  Right?

Yup!

> 
> Is c4:00.5 an NHI?

Yes.

> 
> The PCI power states shown above are the failure case?  c4:00.5
> *should* remain in D0 as long as 00:03.1 is in D0, but was instead put
> in D3cold?

Yes.

> 
>> Currently runtime PM is allowed for all of these devices.
> 
> This is because they all have _PR0 and/or _PS0?  (Diagram doesn't show
> that for 00:08.3, but I assume it must?)

For the root port used for tunneling it's because 
pci_bridge_d3_possible() returns TRUE.
This returns true because platform_pci_bridge(d3) return TRUE.

For the NHI it's because thunderbolt.ko sets this for all NHIs (see 
drivers/thunderbolt/nhi.c).

> 
> And I guess they all must have dev->is_hotplug_bridge set?

The PCIe root port for tunneling does; yes.

> 
>> This means that
>> when all consumers are idle long enough, they will runtime suspend to
>> enter their deepest allowed sleep state. Once all consumers are in their
>> deepest allowed sleep state the suppliers will enter the deepest sleep
>> state as well.
>>
>> * The PCIe root port for tunneling doesn't support waking from D3hot or
>>    D3cold so although runtime suspended it stays in D0.
>> * The XHCI PCIe device supports wakeup from D3cold so it runtime suspends
>>    to D3cold.
>> * Both consumers are in their deepest state and the USB4 router supports
>>    wakeup from D3cold, so it runtime suspends into this state.
>>
>> At least for AMD's case the hardware designer's expectation is the USB4
>> router should have also remained in D0 since the PCIe root port for
>> tunneling remained in D0 and a device link exists between the two devices.
>> The current Linux behavior is undefined.
> 
> Is the requirement that the supplier can never be in a lower-power
> state than the consumer?
> 
> I guess that's *not* actually a requirement even though that's the
> effect of this patch in this situation.  If it *were* that simple, we
> would just check the supplier and consumer power states instead of
> looking at all these ACPI properties.

Yeah I think that's too broad of a generalization.

I don't know how realistic this is but for example what if the supplier 
supported D3 but the consumer supported D2?

> 
>> Instead of making the assertion that the device can support D3 (and thus
>> runtime PM) solely from the presence of ACPI power resources, move the check
>> to later on in the function, which will have validated that the ACPI device
>> supports wake from D3hot or D3cold.
>>
>> This fix prevents the USB4 router being put into D3 when the firmware
>> says that ACPI device representing the PCIe root port for tunneling can't
>> handle it while still allowing ACPI devices that don't have the
>> HotplugSupportInD3 _DSD to also enter D3 if they have power resources that
>> can wake from D3.
> 
> So I guess this changes what acpi_pci_bridge_d3() returns for 00:03.1?
> Previously it returned "true" because it has _PR0/_PS0 (so
> acpi_pci_power_manageable() is true), but now it will return "false"
> because _S0W says it can only wake from D0?
> 

Exactly.
  

On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > Firmware typically advertises that ACPI devices that represent PCIe
> > > devices can support D3 by a combination of the value returned by
> > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > 
> > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > an assumption that if an ACPI device contains power resources the PCIe
> > > device it's associated with can support D3.  This was introduced
> > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > D3 if power managed by ACPI").
> > > 
> > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > port used for tunneling. The PCIe device will still be opted into
> > > runtime PM in the kernel [2] because of the logic within
> > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > device contains power resources.

Wait.  Is this as simple as just recognizing that:

  _PS0 means the OS has a knob to put the device in D0, but it doesn't
  mean the device can wake itself from a low-power state.  The OS has
  to use _S0W to learn the device's ability to wake itself.

If that's enough, maybe we don't need to complicate this with all the
Thunderbolt and device link stuff.  Which would be great, because the
code change itself has nothing to do with those things.

Bjorn
  

On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > devices can support D3 by a combination of the value returned by
> > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > >
> > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > device it's associated with can support D3.  This was introduced
> > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > D3 if power managed by ACPI").
> > > >
> > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > port used for tunneling. The PCIe device will still be opted into
> > > > runtime PM in the kernel [2] because of the logic within
> > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > device contains power resources.
>
> Wait.  Is this as simple as just recognizing that:
>
>   _PS0 means the OS has a knob to put the device in D0, but it doesn't
>   mean the device can wake itself from a low-power state.  The OS has
>   to use _S0W to learn the device's ability to wake itself.

It is.

> If that's enough, maybe we don't need to complicate this with all the
> Thunderbolt and device link stuff.  Which would be great, because the
> code change itself has nothing to do with those things.

Indeed.
  

On 11/14/2022 09:33, Rafael J. Wysocki wrote:
> On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>
>> On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
>>> On 11/11/2022 11:41, Bjorn Helgaas wrote:
>>>> On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
>>>>> Firmware typically advertises that ACPI devices that represent PCIe
>>>>> devices can support D3 by a combination of the value returned by
>>>>> _S0W as well as the HotPlugSupportInD3 _DSD [1].
>>>>>
>>>>> `acpi_pci_bridge_d3` looks for this combination but also contains
>>>>> an assumption that if an ACPI device contains power resources the PCIe
>>>>> device it's associated with can support D3.  This was introduced
>>>>> from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
>>>>> D3 if power managed by ACPI").
>>>>>
>>>>> Some firmware configurations for "AMD Pink Sardine" do not support
>>>>> wake from D3 in _S0W for the ACPI device representing the PCIe root
>>>>> port used for tunneling. The PCIe device will still be opted into
>>>>> runtime PM in the kernel [2] because of the logic within
>>>>> `acpi_pci_bridge_d3`. This currently happens because the ACPI
>>>>> device contains power resources.
>>
>> Wait.  Is this as simple as just recognizing that:
>>
>>    _PS0 means the OS has a knob to put the device in D0, but it doesn't
>>    mean the device can wake itself from a low-power state.  The OS has
>>    to use _S0W to learn the device's ability to wake itself.
> 
> It is.
> 
>> If that's enough, maybe we don't need to complicate this with all the
>> Thunderbolt and device link stuff.  Which would be great, because the
>> code change itself has nothing to do with those things.
> 
> Indeed.

I'd think it's still useful to leave "something" in the commit message 
about how we got to that conclusion though.

Bjorn, do you want me to to attempt to rewrite the commit message and 
send a v6, or would you like to?

Thanks,
  

On Mon, Nov 14, 2022 at 09:37:58AM -0600, Limonciello, Mario wrote:
> On 11/14/2022 09:33, Rafael J. Wysocki wrote:
> > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > devices can support D3 by a combination of the value returned by
> > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > > 
> > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > device it's associated with can support D3.  This was introduced
> > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > D3 if power managed by ACPI").
> > > > > > 
> > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > device contains power resources.
> > > 
> > > Wait.  Is this as simple as just recognizing that:
> > > 
> > >    _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > >    mean the device can wake itself from a low-power state.  The OS has
> > >    to use _S0W to learn the device's ability to wake itself.
> > 
> > It is.
> > 
> > > If that's enough, maybe we don't need to complicate this with all the
> > > Thunderbolt and device link stuff.  Which would be great, because the
> > > code change itself has nothing to do with those things.
> > 
> > Indeed.
> 
> I'd think it's still useful to leave "something" in the commit message about
> how we got to that conclusion though.
> 
> Bjorn, do you want me to to attempt to rewrite the commit message and send a
> v6, or would you like to?

Let me give it a try and post it for your reaction.

Bjorn
  

On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > devices can support D3 by a combination of the value returned by
> > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > >
> > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > device it's associated with can support D3.  This was introduced
> > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > D3 if power managed by ACPI").
> > > > >
> > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > runtime PM in the kernel [2] because of the logic within
> > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > device contains power resources.
> >
> > Wait.  Is this as simple as just recognizing that:
> >
> >   _PS0 means the OS has a knob to put the device in D0, but it doesn't
> >   mean the device can wake itself from a low-power state.  The OS has
> >   to use _S0W to learn the device's ability to wake itself.
> 
> It is.

Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
web page [1] says it identifies Root Ports capable of handling hot
plug events while in D3.  That sounds kind of related to _S0W: If _S0W
says "I can wake myself from D3hot and D3cold", how is that different
from "I can handle hotplug events in D3"?

This patch says that if dev's Root Port has "HotPlugSupportInD3", we
don't need _PS0 or _PR0 for dev.  I guess that must be true, because
previously the fact that we checked for "HotPlugSupportInD3" meant the
device did NOT have _PS0 or _PR0.

[1] https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-pcie-root-ports-supporting-hot-plug-in-d3
  

On 11/15/2022 18:37, Bjorn Helgaas wrote:
> On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
>> On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>
>>> On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
>>>> On 11/11/2022 11:41, Bjorn Helgaas wrote:
>>>>> On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
>>>>>> Firmware typically advertises that ACPI devices that represent PCIe
>>>>>> devices can support D3 by a combination of the value returned by
>>>>>> _S0W as well as the HotPlugSupportInD3 _DSD [1].
>>>>>>
>>>>>> `acpi_pci_bridge_d3` looks for this combination but also contains
>>>>>> an assumption that if an ACPI device contains power resources the PCIe
>>>>>> device it's associated with can support D3.  This was introduced
>>>>>> from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
>>>>>> D3 if power managed by ACPI").
>>>>>>
>>>>>> Some firmware configurations for "AMD Pink Sardine" do not support
>>>>>> wake from D3 in _S0W for the ACPI device representing the PCIe root
>>>>>> port used for tunneling. The PCIe device will still be opted into
>>>>>> runtime PM in the kernel [2] because of the logic within
>>>>>> `acpi_pci_bridge_d3`. This currently happens because the ACPI
>>>>>> device contains power resources.
>>>
>>> Wait.  Is this as simple as just recognizing that:
>>>
>>>    _PS0 means the OS has a knob to put the device in D0, but it doesn't
>>>    mean the device can wake itself from a low-power state.  The OS has
>>>    to use _S0W to learn the device's ability to wake itself.
>>
>> It is.
> 
> Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
> web page [1] says it identifies Root Ports capable of handling hot
> plug events while in D3.  That sounds kind of related to _S0W: If _S0W
> says "I can wake myself from D3hot and D3cold", how is that different
> from "I can handle hotplug events in D3"?


It's impossible to know for sure the logic in Windows, but from all the 
discussion and patches that have flowed related to it my inference is 
the logic for Windows must only examine and use the "HotPlugSupportInD3" 
property if the device also has _S0W.

> 
> This patch says that if dev's Root Port has "HotPlugSupportInD3", we
> don't need _PS0 or _PR0 for dev.  I guess that must be true, because
> previously the fact that we checked for "HotPlugSupportInD3" meant the
> device did NOT have _PS0 or _PR0.
> 

A lot of this confusion and this patch of mine stem from c6e331312ebfb 
being too broad to start out with IMO.  Wouldn't it have made more sense 
to only match and allowlist that specific topology combination (dGPU 
connected to hotplug port and missing those properties)?

> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flearn.microsoft.com%2Fen-us%2Fwindows-hardware%2Fdrivers%2Fpci%2Fdsd-for-pcie-root-ports%23identifying-pcie-root-ports-supporting-hot-plug-in-d3&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7Cc883ba6351534df445f408dac76ac5e5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638041558659543898%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=5Qv3wUYB%2FXJhbeu%2Fh3A0swvgRaB8afjyEYzu9SpHK%2Bo%3D&amp;reserved=0
  

On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > devices can support D3 by a combination of the value returned by
> > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > >
> > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > device it's associated with can support D3.  This was introduced
> > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > D3 if power managed by ACPI").
> > > > > >
> > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > device contains power resources.
> > >
> > > Wait.  Is this as simple as just recognizing that:
> > >
> > >   _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > >   mean the device can wake itself from a low-power state.  The OS has
> > >   to use _S0W to learn the device's ability to wake itself.
> >
> > It is.
>
> Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
> web page [1] says it identifies Root Ports capable of handling hot
> plug events while in D3.  That sounds kind of related to _S0W: If _S0W
> says "I can wake myself from D3hot and D3cold", how is that different
> from "I can handle hotplug events in D3"?

For native PME/hot-plug signaling there is no difference.  This is the
same interrupt by the spec after all IIRC.

For GPE-based signaling, though, there is a difference, because GPEs
can only be used directly for wake signaling (this is related to
_PRW).  In particular, the only provision in the ACPI spec for device
hot-add are the Bus Check and Device Check notification values (0 and
1) which require AML to run and evaluate Notify() on specific AML
objects.

Hence, there is no spec-defined way to tell the OS that "something can
be hot-added under this device while in D3 and you will get notified
about that".

> This patch says that if dev's Root Port has "HotPlugSupportInD3", we
> don't need _PS0 or _PR0 for dev.  I guess that must be true, because
> previously the fact that we checked for "HotPlugSupportInD3" meant the
> device did NOT have _PS0 or _PR0.
>
> [1] https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-pcie-root-ports-supporting-hot-plug-in-d3
  

On Wed, Nov 16, 2022 at 01:00:36PM +0100, Rafael J. Wysocki wrote:
> On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> > > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > >
> > > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > > devices can support D3 by a combination of the value returned by
> > > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > > >
> > > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > > device it's associated with can support D3.  This was introduced
> > > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > > D3 if power managed by ACPI").
> > > > > > >
> > > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > > device contains power resources.
> > > >
> > > > Wait.  Is this as simple as just recognizing that:
> > > >
> > > >   _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > > >   mean the device can wake itself from a low-power state.  The OS has
> > > >   to use _S0W to learn the device's ability to wake itself.
> > >
> > > It is.
> >
> > Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
> > web page [1] says it identifies Root Ports capable of handling hot
> > plug events while in D3.  That sounds kind of related to _S0W: If _S0W
> > says "I can wake myself from D3hot and D3cold", how is that different
> > from "I can handle hotplug events in D3"?
> 
> For native PME/hot-plug signaling there is no difference.  This is the
> same interrupt by the spec after all IIRC.
> 
> For GPE-based signaling, though, there is a difference, because GPEs
> can only be used directly for wake signaling (this is related to
> _PRW).  In particular, the only provision in the ACPI spec for device
> hot-add are the Bus Check and Device Check notification values (0 and
> 1) which require AML to run and evaluate Notify() on specific AML
> objects.
> 
> Hence, there is no spec-defined way to tell the OS that "something can
> be hot-added under this device while in D3 and you will get notified
> about that".

So I guess acpi_pci_bridge_d3() looks for:

  - "wake signaling while in D3" (_S0W) and
  - "notification of hotplug while in D3" ("HotPlugSupportInD3")

For Root Ports with both those abilities (or bridges below such Root
Ports), we allow D3, and this patch doesn't change that.

What this patch *does* change is that all bridges with _PS0 or _PR0
previously could use D3, but now will only be able to use D3 if they
are also (or are below) a Root Port that can signal wakeup
(wakeup.flags.valid) and can wakeup from D3hot or D3cold (_S0W).

And this fixes the Pink Sardine because it has Root Ports that do
Thunderbolt tunneling, and they have _PS0 or _PR0 but their _S0W says
they cannot wake from D3.  Previously we put those in D3, but they
couldn't wake up.  Now we won't put them in D3.

I guess there's a possibility that this could break or cause higher
power consumption on systems that were fixed by c6e331312ebf
("PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI").
I don't know enough about that scenario.  Maybe Lukas will chime in.

> > This patch says that if dev's Root Port has "HotPlugSupportInD3", we
> > don't need _PS0 or _PR0 for dev.  I guess that must be true, because
> > previously the fact that we checked for "HotPlugSupportInD3" meant the
> > device did NOT have _PS0 or _PR0.
> >
> > [1] https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-pcie-root-ports-supporting-hot-plug-in-d3
  

On Thu, Nov 17, 2022 at 12:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Nov 16, 2022 at 01:00:36PM +0100, Rafael J. Wysocki wrote:
> > On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> > > > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > >
> > > > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > > > devices can support D3 by a combination of the value returned by
> > > > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > > > >
> > > > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > > > device it's associated with can support D3.  This was introduced
> > > > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > > > D3 if power managed by ACPI").
> > > > > > > >
> > > > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > > > device contains power resources.
> > > > >
> > > > > Wait.  Is this as simple as just recognizing that:
> > > > >
> > > > >   _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > > > >   mean the device can wake itself from a low-power state.  The OS has
> > > > >   to use _S0W to learn the device's ability to wake itself.
> > > >
> > > > It is.
> > >
> > > Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
> > > web page [1] says it identifies Root Ports capable of handling hot
> > > plug events while in D3.  That sounds kind of related to _S0W: If _S0W
> > > says "I can wake myself from D3hot and D3cold", how is that different
> > > from "I can handle hotplug events in D3"?
> >
> > For native PME/hot-plug signaling there is no difference.  This is the
> > same interrupt by the spec after all IIRC.
> >
> > For GPE-based signaling, though, there is a difference, because GPEs
> > can only be used directly for wake signaling (this is related to
> > _PRW).  In particular, the only provision in the ACPI spec for device
> > hot-add are the Bus Check and Device Check notification values (0 and
> > 1) which require AML to run and evaluate Notify() on specific AML
> > objects.
> >
> > Hence, there is no spec-defined way to tell the OS that "something can
> > be hot-added under this device while in D3 and you will get notified
> > about that".
>
> So I guess acpi_pci_bridge_d3() looks for:
>
>   - "wake signaling while in D3" (_S0W) and
>   - "notification of hotplug while in D3" ("HotPlugSupportInD3")
>
> For Root Ports with both those abilities (or bridges below such Root
> Ports), we allow D3, and this patch doesn't change that.
>
> What this patch *does* change is that all bridges with _PS0 or _PR0
> previously could use D3, but now will only be able to use D3 if they
> are also (or are below) a Root Port that can signal wakeup
> (wakeup.flags.valid) and can wakeup from D3hot or D3cold (_S0W).
>
> And this fixes the Pink Sardine because it has Root Ports that do
> Thunderbolt tunneling, and they have _PS0 or _PR0 but their _S0W says
> they cannot wake from D3.  Previously we put those in D3, but they
> couldn't wake up.  Now we won't put them in D3.
>
> I guess there's a possibility that this could break or cause higher
> power consumption on systems that were fixed by c6e331312ebf
> ("PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI").
> I don't know enough about that scenario.  Maybe Lukas will chime in.

Well, it is possible that some of these systems will be affected.

One of such cases is when the port in question has _S0W which says
that wakeup from D3 is not supported.  In that case I think the kernel
should honor the _S0W input, because there may be a good reason known
to the platform integrator for it.

The other case is when wakeup.flags.valid is unset for the port's ACPI
companion which means that the port cannot signal wakeup through
ACPI-related means at all and this may be problematic, especially in
the system-wide suspend case in which the wakeup capability is not too
relevant unless there is a system wakeup device under the port.

I don't think that the adev->wakeup.flags.valid check has any bearing
on the _S0W check - if there is _S0W and it says "no wakeup from D3",
it should still be taken into account - so that check can be moved
past the _S0W check.

Now, for compatibility with systems where ports have neither _S0W nor
the HotPlugSupportInD3 property, the acpi_pci_power_manageable()
return value should determine the outcome regardless of the
adev->wakeup.flags.valid value, so the latter should only determine
whether or not the HotPlugSupportInD3 property will be inspected
(which may cause true to be returned before the "power manageable"
check).

IOW, something like this (after checking _S0W):

if (adev->wakeup.flags.valid &&
    !acpi_dev_get_property(adev, "HotPlugSupportInD3",
ACPI_TYPE_INTEGER, &obj) &&
    obj->integer.value == 1)
        return true;

return acpi_pci_power_manageable(dev);

Where the if () condition basically means that wakeup signaling is
supported (and there is no indication that it cannot be done from D3
as per the previous _S0W check) and hotplug signaling from D3 is
supported.

> > > This patch says that if dev's Root Port has "HotPlugSupportInD3", we
> > > don't need _PS0 or _PR0 for dev.  I guess that must be true, because
> > > previously the fact that we checked for "HotPlugSupportInD3" meant the
> > > device did NOT have _PS0 or _PR0.
> > >
> > > [1] https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-pcie-root-ports-supporting-hot-plug-in-d3
  

On Thu, Nov 17, 2022 at 06:01:26PM +0100, Rafael J. Wysocki wrote:
> On Thu, Nov 17, 2022 at 12:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Nov 16, 2022 at 01:00:36PM +0100, Rafael J. Wysocki wrote:
> > > On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> > > > > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > >
> > > > > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > > > > devices can support D3 by a combination of the value returned by
> > > > > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > > > > >
> > > > > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > > > > device it's associated with can support D3.  This was introduced
> > > > > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > > > > D3 if power managed by ACPI").
> > > > > > > > >
> > > > > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > > > > device contains power resources.
> > > > > >
> > > > > > Wait.  Is this as simple as just recognizing that:
> > > > > >
> > > > > >   _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > > > > >   mean the device can wake itself from a low-power state.  The OS has
> > > > > >   to use _S0W to learn the device's ability to wake itself.
> > > > >
> > > > > It is.
> > > >
> > > > Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
> > > > web page [1] says it identifies Root Ports capable of handling hot
> > > > plug events while in D3.  That sounds kind of related to _S0W: If _S0W
> > > > says "I can wake myself from D3hot and D3cold", how is that different
> > > > from "I can handle hotplug events in D3"?
> > >
> > > For native PME/hot-plug signaling there is no difference.  This is the
> > > same interrupt by the spec after all IIRC.
> > >
> > > For GPE-based signaling, though, there is a difference, because GPEs
> > > can only be used directly for wake signaling (this is related to
> > > _PRW).  In particular, the only provision in the ACPI spec for device
> > > hot-add are the Bus Check and Device Check notification values (0 and
> > > 1) which require AML to run and evaluate Notify() on specific AML
> > > objects.
> > >
> > > Hence, there is no spec-defined way to tell the OS that "something can
> > > be hot-added under this device while in D3 and you will get notified
> > > about that".
> >
> > So I guess acpi_pci_bridge_d3() looks for:
> >
> >   - "wake signaling while in D3" (_S0W) and
> >   - "notification of hotplug while in D3" ("HotPlugSupportInD3")
> >
> > For Root Ports with both those abilities (or bridges below such Root
> > Ports), we allow D3, and this patch doesn't change that.
> >
> > What this patch *does* change is that all bridges with _PS0 or _PR0
> > previously could use D3, but now will only be able to use D3 if they
> > are also (or are below) a Root Port that can signal wakeup
> > (wakeup.flags.valid) and can wakeup from D3hot or D3cold (_S0W).
> >
> > And this fixes the Pink Sardine because it has Root Ports that do
> > Thunderbolt tunneling, and they have _PS0 or _PR0 but their _S0W says
> > they cannot wake from D3.  Previously we put those in D3, but they
> > couldn't wake up.  Now we won't put them in D3.
> >
> > I guess there's a possibility that this could break or cause higher
> > power consumption on systems that were fixed by c6e331312ebf
> > ("PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI").
> > I don't know enough about that scenario.  Maybe Lukas will chime in.
> 
> Well, it is possible that some of these systems will be affected.
> 
> One of such cases is when the port in question has _S0W which says
> that wakeup from D3 is not supported.  In that case I think the kernel
> should honor the _S0W input, because there may be a good reason known
> to the platform integrator for it.
> 
> The other case is when wakeup.flags.valid is unset for the port's ACPI
> companion which means that the port cannot signal wakeup through
> ACPI-related means at all and this may be problematic, especially in
> the system-wide suspend case in which the wakeup capability is not too
> relevant unless there is a system wakeup device under the port.
> 
> I don't think that the adev->wakeup.flags.valid check has any bearing
> on the _S0W check - if there is _S0W and it says "no wakeup from D3",
> it should still be taken into account - so that check can be moved
> past the _S0W check.

So if _S0W says it can wake from D3, but wakeup.flags is not valid,
it's still OK to use D3?  I guess in this case we assume wakeup would
be via native PME/hotplug signaling?

> Now, for compatibility with systems where ports have neither _S0W nor
> the HotPlugSupportInD3 property, the acpi_pci_power_manageable()
> return value should determine the outcome regardless of the
> adev->wakeup.flags.valid value, so the latter should only determine
> whether or not the HotPlugSupportInD3 property will be inspected
> (which may cause true to be returned before the "power manageable"
> check).
> 
> IOW, something like this (after checking _S0W):
> 
> if (adev->wakeup.flags.valid &&
>     !acpi_dev_get_property(adev, "HotPlugSupportInD3",
> ACPI_TYPE_INTEGER, &obj) &&
>     obj->integer.value == 1)
>         return true;
> 
> return acpi_pci_power_manageable(dev);
> 
> Where the if () condition basically means that wakeup signaling is
> supported (and there is no indication that it cannot be done from D3
> as per the previous _S0W check) and hotplug signaling from D3 is
> supported.
> 
> > > > This patch says that if dev's Root Port has "HotPlugSupportInD3", we
> > > > don't need _PS0 or _PR0 for dev.  I guess that must be true, because
> > > > previously the fact that we checked for "HotPlugSupportInD3" meant the
> > > > device did NOT have _PS0 or _PR0.
> > > >
> > > > [1] https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-pcie-root-ports-supporting-hot-plug-in-d3

I think you're suggesting the patch below, which will make
acpi_pci_bridge_d3(dev) return "true" if:

  - Root Port can wake from D3hot or D3cold, has "HotPlugSupportInD3",
    and has wakeup.flags.valid, OR

  - Root Port can wake from D3hot or D3cold, and "dev" has _PR0 or
    _PS0

Previously, all bridges with _PR0 or _PS0 could use D3; now we also
require that the Root Port's _S0W says it can wake from at least
D3hot.

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index a46fec776ad7..66c9ae1dc5da 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -984,10 +984,6 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
 	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;
-
 	rpdev = pcie_find_root_port(dev);
 	if (!rpdev)
 		return false;
@@ -996,14 +992,6 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
 	if (!adev)
 		return false;
 
-	/*
-	 * If the Root Port cannot signal wakeup signals at all, i.e., it
-	 * 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)
-		return false;
-
 	/*
 	 * If the Root Port cannot wake itself from D3hot or D3cold, we
 	 * can't use D3.
@@ -1014,16 +1002,21 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
 
 	/*
 	 * The "HotPlugSupportInD3" property in a Root Port _DSD indicates
-	 * the Port can signal hotplug events while in D3.  We assume any
-	 * bridges *below* that Root Port can also signal hotplug events
-	 * while in D3.
+	 * the Port can signal hotplug events while in D3.  This differs
+	 * from _S0W because _S0W may rely on GPEs, which can only be used
+	 * directly for wake signaling, not hotplug events.
+	 *
+	 * We assume any bridges *below* that Root Port can also signal
+	 * hotplug events while in D3.
 	 */
-	if (!acpi_dev_get_property(adev, "HotPlugSupportInD3",
+	if (adev->wakeup.flags.valid &&
+	    !acpi_dev_get_property(adev, "HotPlugSupportInD3",
 				   ACPI_TYPE_INTEGER, &obj) &&
 	    obj->integer.value == 1)
 		return true;
 
-	return false;
+	/* Assume D3 support if the bridge is power-manageable by ACPI. */
+	return acpi_pci_power_manageable(dev);
 }
 
 int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
  

On Thu, Nov 17, 2022 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Nov 17, 2022 at 06:01:26PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Nov 17, 2022 at 12:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Wed, Nov 16, 2022 at 01:00:36PM +0100, Rafael J. Wysocki wrote:
> > > > On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> > > > > > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > >
> > > > > > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > > > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > > > > > devices can support D3 by a combination of the value returned by
> > > > > > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > > > > > >
> > > > > > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > > > > > device it's associated with can support D3.  This was introduced
> > > > > > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > > > > > D3 if power managed by ACPI").
> > > > > > > > > >
> > > > > > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > > > > > device contains power resources.
> > > > > > >
> > > > > > > Wait.  Is this as simple as just recognizing that:
> > > > > > >
> > > > > > >   _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > > > > > >   mean the device can wake itself from a low-power state.  The OS has
> > > > > > >   to use _S0W to learn the device's ability to wake itself.
> > > > > >
> > > > > > It is.
> > > > >
> > > > > Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
> > > > > web page [1] says it identifies Root Ports capable of handling hot
> > > > > plug events while in D3.  That sounds kind of related to _S0W: If _S0W
> > > > > says "I can wake myself from D3hot and D3cold", how is that different
> > > > > from "I can handle hotplug events in D3"?
> > > >
> > > > For native PME/hot-plug signaling there is no difference.  This is the
> > > > same interrupt by the spec after all IIRC.
> > > >
> > > > For GPE-based signaling, though, there is a difference, because GPEs
> > > > can only be used directly for wake signaling (this is related to
> > > > _PRW).  In particular, the only provision in the ACPI spec for device
> > > > hot-add are the Bus Check and Device Check notification values (0 and
> > > > 1) which require AML to run and evaluate Notify() on specific AML
> > > > objects.
> > > >
> > > > Hence, there is no spec-defined way to tell the OS that "something can
> > > > be hot-added under this device while in D3 and you will get notified
> > > > about that".
> > >
> > > So I guess acpi_pci_bridge_d3() looks for:
> > >
> > >   - "wake signaling while in D3" (_S0W) and
> > >   - "notification of hotplug while in D3" ("HotPlugSupportInD3")
> > >
> > > For Root Ports with both those abilities (or bridges below such Root
> > > Ports), we allow D3, and this patch doesn't change that.
> > >
> > > What this patch *does* change is that all bridges with _PS0 or _PR0
> > > previously could use D3, but now will only be able to use D3 if they
> > > are also (or are below) a Root Port that can signal wakeup
> > > (wakeup.flags.valid) and can wakeup from D3hot or D3cold (_S0W).
> > >
> > > And this fixes the Pink Sardine because it has Root Ports that do
> > > Thunderbolt tunneling, and they have _PS0 or _PR0 but their _S0W says
> > > they cannot wake from D3.  Previously we put those in D3, but they
> > > couldn't wake up.  Now we won't put them in D3.
> > >
> > > I guess there's a possibility that this could break or cause higher
> > > power consumption on systems that were fixed by c6e331312ebf
> > > ("PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI").
> > > I don't know enough about that scenario.  Maybe Lukas will chime in.
> >
> > Well, it is possible that some of these systems will be affected.
> >
> > One of such cases is when the port in question has _S0W which says
> > that wakeup from D3 is not supported.  In that case I think the kernel
> > should honor the _S0W input, because there may be a good reason known
> > to the platform integrator for it.
> >
> > The other case is when wakeup.flags.valid is unset for the port's ACPI
> > companion which means that the port cannot signal wakeup through
> > ACPI-related means at all and this may be problematic, especially in
> > the system-wide suspend case in which the wakeup capability is not too
> > relevant unless there is a system wakeup device under the port.
> >
> > I don't think that the adev->wakeup.flags.valid check has any bearing
> > on the _S0W check - if there is _S0W and it says "no wakeup from D3",
> > it should still be taken into account - so that check can be moved
> > past the _S0W check.
>
> So if _S0W says it can wake from D3, but wakeup.flags is not valid,
> it's still OK to use D3?

No, it isn't, as per the code today and I don't think that this
particular part should be changed now.

_S0W may only cause acpi_pci_bridge_d3() to return false, it is not
sufficient for true to be returned.

> I guess in this case we assume wakeup would
> be via native PME/hotplug signaling?

This may be taken into consideration at one point, if need be, but not
in this particular patch IMO.

> > Now, for compatibility with systems where ports have neither _S0W nor
> > the HotPlugSupportInD3 property, the acpi_pci_power_manageable()
> > return value should determine the outcome regardless of the
> > adev->wakeup.flags.valid value, so the latter should only determine
> > whether or not the HotPlugSupportInD3 property will be inspected
> > (which may cause true to be returned before the "power manageable"
> > check).
> >
> > IOW, something like this (after checking _S0W):
> >
> > if (adev->wakeup.flags.valid &&
> >     !acpi_dev_get_property(adev, "HotPlugSupportInD3",
> > ACPI_TYPE_INTEGER, &obj) &&
> >     obj->integer.value == 1)
> >         return true;
> >
> > return acpi_pci_power_manageable(dev);
> >
> > Where the if () condition basically means that wakeup signaling is
> > supported (and there is no indication that it cannot be done from D3
> > as per the previous _S0W check) and hotplug signaling from D3 is
> > supported.
> >
> > > > > This patch says that if dev's Root Port has "HotPlugSupportInD3", we
> > > > > don't need _PS0 or _PR0 for dev.  I guess that must be true, because
> > > > > previously the fact that we checked for "HotPlugSupportInD3" meant the
> > > > > device did NOT have _PS0 or _PR0.
> > > > >
> > > > > [1] https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-pcie-root-ports-supporting-hot-plug-in-d3
>
> I think you're suggesting the patch below, which will make
> acpi_pci_bridge_d3(dev) return "true" if:
>
>   - Root Port can wake from D3hot or D3cold, has "HotPlugSupportInD3",
>     and has wakeup.flags.valid, OR
>
>   - Root Port can wake from D3hot or D3cold, and "dev" has _PR0 or
>     _PS0

Well, not exactly.  The second point should be

 - Root Port's ACPI companion ('dev') has _PR0 or _PS0.

> Previously, all bridges with _PR0 or _PS0 could use D3; now we also
> require that the Root Port's _S0W says it can wake from at least
> D3hot.

Yes, if _S0W is present and it evaluates successfully, then it is
required to confirm that wakeup signaling from at least D3hot is
supported for 'true' to be returned (but it is not sufficient for that
by itself).

That's the only functional change made by that patch and yes, the
patch below is what I mean.

> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index a46fec776ad7..66c9ae1dc5da 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -984,10 +984,6 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>         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;
> -
>         rpdev = pcie_find_root_port(dev);
>         if (!rpdev)
>                 return false;
> @@ -996,14 +992,6 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>         if (!adev)
>                 return false;
>
> -       /*
> -        * If the Root Port cannot signal wakeup signals at all, i.e., it
> -        * 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)
> -               return false;
> -
>         /*
>          * If the Root Port cannot wake itself from D3hot or D3cold, we
>          * can't use D3.
> @@ -1014,16 +1002,21 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>
>         /*
>          * The "HotPlugSupportInD3" property in a Root Port _DSD indicates
> -        * the Port can signal hotplug events while in D3.  We assume any
> -        * bridges *below* that Root Port can also signal hotplug events
> -        * while in D3.
> +        * the Port can signal hotplug events while in D3.  This differs
> +        * from _S0W because _S0W may rely on GPEs, which can only be used
> +        * directly for wake signaling, not hotplug events.
> +        *
> +        * We assume any bridges *below* that Root Port can also signal
> +        * hotplug events while in D3.
>          */
> -       if (!acpi_dev_get_property(adev, "HotPlugSupportInD3",
> +       if (adev->wakeup.flags.valid &&
> +           !acpi_dev_get_property(adev, "HotPlugSupportInD3",
>                                    ACPI_TYPE_INTEGER, &obj) &&
>             obj->integer.value == 1)
>                 return true;
>
> -       return false;
> +       /* Assume D3 support if the bridge is power-manageable by ACPI. */
> +       return acpi_pci_power_manageable(dev);
>  }
>
>  int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)

Note that the adev->wakeup.flags.valid is still necessary in the cases
when _S0W is not present, because in those cases wakeup support
implies that it is supported in all D-states.  It is sort of redundant
when _S0W is present, but the current code has it and this particular
patch doesn't (or even shouldn't) change that.
  

Hi Rafael,

Sorry, I'm still confused (my perpetual state :)).

On Fri, Nov 18, 2022 at 02:16:17PM +0100, Rafael J. Wysocki wrote:
> On Thu, Nov 17, 2022 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Nov 17, 2022 at 06:01:26PM +0100, Rafael J. Wysocki wrote:
> > > On Thu, Nov 17, 2022 at 12:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Wed, Nov 16, 2022 at 01:00:36PM +0100, Rafael J. Wysocki wrote:
> > > > > On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> > > > > > > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > > > > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > > > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > > > > > > devices can support D3 by a combination of the value returned by
> > > > > > > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > > > > > > >
> > > > > > > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > > > > > > device it's associated with can support D3.  This was introduced
> > > > > > > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > > > > > > D3 if power managed by ACPI").
> > > > > > > > > > >
> > > > > > > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > > > > > > device contains power resources.
> > > > > > > >
> > > > > > > > Wait.  Is this as simple as just recognizing that:
> > > > > > > >
> > > > > > > >   _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > > > > > > >   mean the device can wake itself from a low-power state.  The OS has
> > > > > > > >   to use _S0W to learn the device's ability to wake itself.
> > > > > > >
> > > > > > > It is.
> > > > > >
> > > > > > Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
> > > > > > web page [1] says it identifies Root Ports capable of handling hot
> > > > > > plug events while in D3.  That sounds kind of related to _S0W: If _S0W
> > > > > > says "I can wake myself from D3hot and D3cold", how is that different
> > > > > > from "I can handle hotplug events in D3"?
> > > > >
> > > > > For native PME/hot-plug signaling there is no difference.  This is the
> > > > > same interrupt by the spec after all IIRC.
> > > > >
> > > > > For GPE-based signaling, though, there is a difference, because GPEs
> > > > > can only be used directly for wake signaling (this is related to
> > > > > _PRW).  In particular, the only provision in the ACPI spec for device
> > > > > hot-add are the Bus Check and Device Check notification values (0 and
> > > > > 1) which require AML to run and evaluate Notify() on specific AML
> > > > > objects.
> > > > >
> > > > > Hence, there is no spec-defined way to tell the OS that "something can
> > > > > be hot-added under this device while in D3 and you will get notified
> > > > > about that".
> > > >
> > > > So I guess acpi_pci_bridge_d3() looks for:
> > > >
> > > >   - "wake signaling while in D3" (_S0W) and
> > > >   - "notification of hotplug while in D3" ("HotPlugSupportInD3")
> > > >
> > > > For Root Ports with both those abilities (or bridges below such Root
> > > > Ports), we allow D3, and this patch doesn't change that.
> > > >
> > > > What this patch *does* change is that all bridges with _PS0 or _PR0
> > > > previously could use D3, but now will only be able to use D3 if they
> > > > are also (or are below) a Root Port that can signal wakeup
> > > > (wakeup.flags.valid) and can wakeup from D3hot or D3cold (_S0W).
> > > >
> > > > And this fixes the Pink Sardine because it has Root Ports that do
> > > > Thunderbolt tunneling, and they have _PS0 or _PR0 but their _S0W says
> > > > they cannot wake from D3.  Previously we put those in D3, but they
> > > > couldn't wake up.  Now we won't put them in D3.
> > > >
> > > > I guess there's a possibility that this could break or cause higher
> > > > power consumption on systems that were fixed by c6e331312ebf
> > > > ("PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI").
> > > > I don't know enough about that scenario.  Maybe Lukas will chime in.
> > >
> > > Well, it is possible that some of these systems will be affected.
> > >
> > > One of such cases is when the port in question has _S0W which says
> > > that wakeup from D3 is not supported.  In that case I think the kernel
> > > should honor the _S0W input, because there may be a good reason known
> > > to the platform integrator for it.
> > >
> > > The other case is when wakeup.flags.valid is unset for the port's ACPI
> > > companion which means that the port cannot signal wakeup through
> > > ACPI-related means at all and this may be problematic, especially in
> > > the system-wide suspend case in which the wakeup capability is not too
> > > relevant unless there is a system wakeup device under the port.
> > >
> > > I don't think that the adev->wakeup.flags.valid check has any bearing
> > > on the _S0W check - if there is _S0W and it says "no wakeup from D3",
> > > it should still be taken into account - so that check can be moved
> > > past the _S0W check.
> >
> > So if _S0W says it can wake from D3, but wakeup.flags is not valid,
> > it's still OK to use D3?
> 
> No, it isn't, as per the code today and I don't think that this
> particular part should be changed now.

But the current upstream code checks acpi_pci_power_manageable(dev)
first, so if "dev" has _PR0 or _PS0, we'll use D3 even if _S0W says it
can wake from D3 and wakeup.flags is not valid.

> _S0W may only cause acpi_pci_bridge_d3() to return false, it is not
> sufficient for true to be returned.
> 
> > I guess in this case we assume wakeup would
> > be via native PME/hotplug signaling?
> 
> This may be taken into consideration at one point, if need be, but not
> in this particular patch IMO.
> 
> > > Now, for compatibility with systems where ports have neither _S0W nor
> > > the HotPlugSupportInD3 property, the acpi_pci_power_manageable()
> > > return value should determine the outcome regardless of the
> > > adev->wakeup.flags.valid value, so the latter should only determine
> > > whether or not the HotPlugSupportInD3 property will be inspected
> > > (which may cause true to be returned before the "power manageable"
> > > check).
> > >
> > > IOW, something like this (after checking _S0W):
> > >
> > > if (adev->wakeup.flags.valid &&
> > >     !acpi_dev_get_property(adev, "HotPlugSupportInD3",
> > > ACPI_TYPE_INTEGER, &obj) &&
> > >     obj->integer.value == 1)
> > >         return true;
> > >
> > > return acpi_pci_power_manageable(dev);
> > >
> > > Where the if () condition basically means that wakeup signaling is
> > > supported (and there is no indication that it cannot be done from D3
> > > as per the previous _S0W check) and hotplug signaling from D3 is
> > > supported.
> > >
> > > > > > This patch says that if dev's Root Port has "HotPlugSupportInD3", we
> > > > > > don't need _PS0 or _PR0 for dev.  I guess that must be true, because
> > > > > > previously the fact that we checked for "HotPlugSupportInD3" meant the
> > > > > > device did NOT have _PS0 or _PR0.
> > > > > >
> > > > > > [1] https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-pcie-root-ports-supporting-hot-plug-in-d3
> >
> > I think you're suggesting the patch below, which will make
> > acpi_pci_bridge_d3(dev) return "true" if:
> >
> >   - Root Port can wake from D3hot or D3cold, has "HotPlugSupportInD3",
> >     and has wakeup.flags.valid, OR
> >
> >   - Root Port can wake from D3hot or D3cold, and "dev" has _PR0 or
> >     _PS0
> 
> Well, not exactly.  The second point should be
> 
>  - Root Port's ACPI companion ('dev') has _PR0 or _PS0.

With the patch below, the RP _S0W must either fail or return D3hot or
D3cold (this is what I meant by "RP can wake from D3hot or D3cold")
before we even look for _PR0/_PS0.

Maybe we're talking about two different things -- you suggest we
should check whether the *Root Port* has _PR0 or _PS0, but the current
code checks the bridge "dev", which might be *below* the Root Port
IIUC.

> > Previously, all bridges with _PR0 or _PS0 could use D3; now we also
> > require that the Root Port's _S0W says it can wake from at least
> > D3hot.
> 
> Yes, if _S0W is present and it evaluates successfully, then it is
> required to confirm that wakeup signaling from at least D3hot is
> supported for 'true' to be returned (but it is not sufficient for that
> by itself).

Hmm.  In the case where _S0W is present and says at least D3hot but
wakeup.flags is not valid, the patch below returns 'true' if "dev"
has _PR0 or _PS0.

> That's the only functional change made by that patch and yes, the
> patch below is what I mean.

> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index a46fec776ad7..66c9ae1dc5da 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -984,10 +984,6 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
> >         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;
> > -
> >         rpdev = pcie_find_root_port(dev);
> >         if (!rpdev)
> >                 return false;
> > @@ -996,14 +992,6 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
> >         if (!adev)
> >                 return false;
> >
> > -       /*
> > -        * If the Root Port cannot signal wakeup signals at all, i.e., it
> > -        * 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)
> > -               return false;
> > -
> >         /*
> >          * If the Root Port cannot wake itself from D3hot or D3cold, we
> >          * can't use D3.
> > @@ -1014,16 +1002,21 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
> >
> >         /*
> >          * The "HotPlugSupportInD3" property in a Root Port _DSD indicates
> > -        * the Port can signal hotplug events while in D3.  We assume any
> > -        * bridges *below* that Root Port can also signal hotplug events
> > -        * while in D3.
> > +        * the Port can signal hotplug events while in D3.  This differs
> > +        * from _S0W because _S0W may rely on GPEs, which can only be used
> > +        * directly for wake signaling, not hotplug events.
> > +        *
> > +        * We assume any bridges *below* that Root Port can also signal
> > +        * hotplug events while in D3.
> >          */
> > -       if (!acpi_dev_get_property(adev, "HotPlugSupportInD3",
> > +       if (adev->wakeup.flags.valid &&
> > +           !acpi_dev_get_property(adev, "HotPlugSupportInD3",
> >                                    ACPI_TYPE_INTEGER, &obj) &&
> >             obj->integer.value == 1)
> >                 return true;
> >
> > -       return false;
> > +       /* Assume D3 support if the bridge is power-manageable by ACPI. */
> > +       return acpi_pci_power_manageable(dev);
> >  }
> >
> >  int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> 
> Note that the adev->wakeup.flags.valid is still necessary in the cases
> when _S0W is not present, because in those cases wakeup support
> implies that it is supported in all D-states.  It is sort of redundant
> when _S0W is present, but the current code has it and this particular
> patch doesn't (or even shouldn't) change that.

In the current patch, wakeup.flags is only relevant if the RP has
"HotPlugSupportInD3".  If "dev" has _PR0 or _PS0, we'll return 'true'
even if wakeup.flags is not valid.  Maybe that's wrong?

Bjorn
  

On Fri, Nov 18, 2022 at 9:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Hi Rafael,
>
> Sorry, I'm still confused (my perpetual state :)).

No worries, doing my best to address that.

> On Fri, Nov 18, 2022 at 02:16:17PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Nov 17, 2022 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Thu, Nov 17, 2022 at 06:01:26PM +0100, Rafael J. Wysocki wrote:
> > > > On Thu, Nov 17, 2022 at 12:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Wed, Nov 16, 2022 at 01:00:36PM +0100, Rafael J. Wysocki wrote:
> > > > > > On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> > > > > > > > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > > > > > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > > > > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > > > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > > > > > > > devices can support D3 by a combination of the value returned by
> > > > > > > > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > > > > > > > >
> > > > > > > > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > > > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > > > > > > > device it's associated with can support D3.  This was introduced
> > > > > > > > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > > > > > > > D3 if power managed by ACPI").
> > > > > > > > > > > >
> > > > > > > > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > > > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > > > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > > > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > > > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > > > > > > > device contains power resources.
> > > > > > > > >
> > > > > > > > > Wait.  Is this as simple as just recognizing that:
> > > > > > > > >
> > > > > > > > >   _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > > > > > > > >   mean the device can wake itself from a low-power state.  The OS has
> > > > > > > > >   to use _S0W to learn the device's ability to wake itself.
> > > > > > > >
> > > > > > > > It is.
> > > > > > >
> > > > > > > Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
> > > > > > > web page [1] says it identifies Root Ports capable of handling hot
> > > > > > > plug events while in D3.  That sounds kind of related to _S0W: If _S0W
> > > > > > > says "I can wake myself from D3hot and D3cold", how is that different
> > > > > > > from "I can handle hotplug events in D3"?
> > > > > >
> > > > > > For native PME/hot-plug signaling there is no difference.  This is the
> > > > > > same interrupt by the spec after all IIRC.
> > > > > >
> > > > > > For GPE-based signaling, though, there is a difference, because GPEs
> > > > > > can only be used directly for wake signaling (this is related to
> > > > > > _PRW).  In particular, the only provision in the ACPI spec for device
> > > > > > hot-add are the Bus Check and Device Check notification values (0 and
> > > > > > 1) which require AML to run and evaluate Notify() on specific AML
> > > > > > objects.
> > > > > >
> > > > > > Hence, there is no spec-defined way to tell the OS that "something can
> > > > > > be hot-added under this device while in D3 and you will get notified
> > > > > > about that".
> > > > >
> > > > > So I guess acpi_pci_bridge_d3() looks for:
> > > > >
> > > > >   - "wake signaling while in D3" (_S0W) and
> > > > >   - "notification of hotplug while in D3" ("HotPlugSupportInD3")
> > > > >
> > > > > For Root Ports with both those abilities (or bridges below such Root
> > > > > Ports), we allow D3, and this patch doesn't change that.
> > > > >
> > > > > What this patch *does* change is that all bridges with _PS0 or _PR0
> > > > > previously could use D3, but now will only be able to use D3 if they
> > > > > are also (or are below) a Root Port that can signal wakeup
> > > > > (wakeup.flags.valid) and can wakeup from D3hot or D3cold (_S0W).
> > > > >
> > > > > And this fixes the Pink Sardine because it has Root Ports that do
> > > > > Thunderbolt tunneling, and they have _PS0 or _PR0 but their _S0W says
> > > > > they cannot wake from D3.  Previously we put those in D3, but they
> > > > > couldn't wake up.  Now we won't put them in D3.
> > > > >
> > > > > I guess there's a possibility that this could break or cause higher
> > > > > power consumption on systems that were fixed by c6e331312ebf
> > > > > ("PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI").
> > > > > I don't know enough about that scenario.  Maybe Lukas will chime in.
> > > >
> > > > Well, it is possible that some of these systems will be affected.
> > > >
> > > > One of such cases is when the port in question has _S0W which says
> > > > that wakeup from D3 is not supported.  In that case I think the kernel
> > > > should honor the _S0W input, because there may be a good reason known
> > > > to the platform integrator for it.
> > > >
> > > > The other case is when wakeup.flags.valid is unset for the port's ACPI
> > > > companion which means that the port cannot signal wakeup through
> > > > ACPI-related means at all and this may be problematic, especially in
> > > > the system-wide suspend case in which the wakeup capability is not too
> > > > relevant unless there is a system wakeup device under the port.
> > > >
> > > > I don't think that the adev->wakeup.flags.valid check has any bearing
> > > > on the _S0W check - if there is _S0W and it says "no wakeup from D3",
> > > > it should still be taken into account - so that check can be moved
> > > > past the _S0W check.
> > >
> > > So if _S0W says it can wake from D3, but wakeup.flags is not valid,
> > > it's still OK to use D3?
> >
> > No, it isn't, as per the code today and I don't think that this
> > particular part should be changed now.
>
> But the current upstream code checks acpi_pci_power_manageable(dev)
> first, so if "dev" has _PR0 or _PS0, we'll use D3 even if _S0W says it
> can wake from D3 and wakeup.flags is not valid.

Yes, the current code will return 'true' if _PR0 or _PS0 is present
for dev regardless of anything else.

The proposed change is to make that conditional on whether or not _S0W
for the root port says that wakeup from D3 is supported (or it is not
present or unusable).

I see that I've missed one point now which is when the root port
doesn't have an ACPI companion, in which case we should go straight
for the "dev is power manageable" check.  Let me redo the patch to
address this.
  

On Friday, November 18, 2022 10:13:39 PM CET Rafael J. Wysocki wrote:
> On Fri, Nov 18, 2022 at 9:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > Hi Rafael,
> >
> > Sorry, I'm still confused (my perpetual state :)).
> 
> No worries, doing my best to address that.
> 
> > On Fri, Nov 18, 2022 at 02:16:17PM +0100, Rafael J. Wysocki wrote:
> > > On Thu, Nov 17, 2022 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Thu, Nov 17, 2022 at 06:01:26PM +0100, Rafael J. Wysocki wrote:
> > > > > On Thu, Nov 17, 2022 at 12:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > On Wed, Nov 16, 2022 at 01:00:36PM +0100, Rafael J. Wysocki wrote:
> > > > > > > On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> > > > > > > > > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > > >
> > > > > > > > > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > > > > > > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > > > > > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > > > > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > > > > > > > > devices can support D3 by a combination of the value returned by
> > > > > > > > > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > > > > > > > > >
> > > > > > > > > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > > > > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > > > > > > > > device it's associated with can support D3.  This was introduced
> > > > > > > > > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > > > > > > > > D3 if power managed by ACPI").
> > > > > > > > > > > > >
> > > > > > > > > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > > > > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > > > > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > > > > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > > > > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > > > > > > > > device contains power resources.
> > > > > > > > > >
> > > > > > > > > > Wait.  Is this as simple as just recognizing that:
> > > > > > > > > >
> > > > > > > > > >   _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > > > > > > > > >   mean the device can wake itself from a low-power state.  The OS has
> > > > > > > > > >   to use _S0W to learn the device's ability to wake itself.
> > > > > > > > >
> > > > > > > > > It is.
> > > > > > > >
> > > > > > > > Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
> > > > > > > > web page [1] says it identifies Root Ports capable of handling hot
> > > > > > > > plug events while in D3.  That sounds kind of related to _S0W: If _S0W
> > > > > > > > says "I can wake myself from D3hot and D3cold", how is that different
> > > > > > > > from "I can handle hotplug events in D3"?
> > > > > > >
> > > > > > > For native PME/hot-plug signaling there is no difference.  This is the
> > > > > > > same interrupt by the spec after all IIRC.
> > > > > > >
> > > > > > > For GPE-based signaling, though, there is a difference, because GPEs
> > > > > > > can only be used directly for wake signaling (this is related to
> > > > > > > _PRW).  In particular, the only provision in the ACPI spec for device
> > > > > > > hot-add are the Bus Check and Device Check notification values (0 and
> > > > > > > 1) which require AML to run and evaluate Notify() on specific AML
> > > > > > > objects.
> > > > > > >
> > > > > > > Hence, there is no spec-defined way to tell the OS that "something can
> > > > > > > be hot-added under this device while in D3 and you will get notified
> > > > > > > about that".
> > > > > >
> > > > > > So I guess acpi_pci_bridge_d3() looks for:
> > > > > >
> > > > > >   - "wake signaling while in D3" (_S0W) and
> > > > > >   - "notification of hotplug while in D3" ("HotPlugSupportInD3")
> > > > > >
> > > > > > For Root Ports with both those abilities (or bridges below such Root
> > > > > > Ports), we allow D3, and this patch doesn't change that.
> > > > > >
> > > > > > What this patch *does* change is that all bridges with _PS0 or _PR0
> > > > > > previously could use D3, but now will only be able to use D3 if they
> > > > > > are also (or are below) a Root Port that can signal wakeup
> > > > > > (wakeup.flags.valid) and can wakeup from D3hot or D3cold (_S0W).
> > > > > >
> > > > > > And this fixes the Pink Sardine because it has Root Ports that do
> > > > > > Thunderbolt tunneling, and they have _PS0 or _PR0 but their _S0W says
> > > > > > they cannot wake from D3.  Previously we put those in D3, but they
> > > > > > couldn't wake up.  Now we won't put them in D3.
> > > > > >
> > > > > > I guess there's a possibility that this could break or cause higher
> > > > > > power consumption on systems that were fixed by c6e331312ebf
> > > > > > ("PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI").
> > > > > > I don't know enough about that scenario.  Maybe Lukas will chime in.
> > > > >
> > > > > Well, it is possible that some of these systems will be affected.
> > > > >
> > > > > One of such cases is when the port in question has _S0W which says
> > > > > that wakeup from D3 is not supported.  In that case I think the kernel
> > > > > should honor the _S0W input, because there may be a good reason known
> > > > > to the platform integrator for it.
> > > > >
> > > > > The other case is when wakeup.flags.valid is unset for the port's ACPI
> > > > > companion which means that the port cannot signal wakeup through
> > > > > ACPI-related means at all and this may be problematic, especially in
> > > > > the system-wide suspend case in which the wakeup capability is not too
> > > > > relevant unless there is a system wakeup device under the port.
> > > > >
> > > > > I don't think that the adev->wakeup.flags.valid check has any bearing
> > > > > on the _S0W check - if there is _S0W and it says "no wakeup from D3",
> > > > > it should still be taken into account - so that check can be moved
> > > > > past the _S0W check.
> > > >
> > > > So if _S0W says it can wake from D3, but wakeup.flags is not valid,
> > > > it's still OK to use D3?
> > >
> > > No, it isn't, as per the code today and I don't think that this
> > > particular part should be changed now.
> >
> > But the current upstream code checks acpi_pci_power_manageable(dev)
> > first, so if "dev" has _PR0 or _PS0, we'll use D3 even if _S0W says it
> > can wake from D3 and wakeup.flags is not valid.
> 
> Yes, the current code will return 'true' if _PR0 or _PS0 is present
> for dev regardless of anything else.
> 
> The proposed change is to make that conditional on whether or not _S0W
> for the root port says that wakeup from D3 is supported (or it is not
> present or unusable).
> 
> I see that I've missed one point now which is when the root port
> doesn't have an ACPI companion, in which case we should go straight
> for the "dev is power manageable" check.

Moreover, it is possible that the bridge passed to acpi_pci_bridge_d3() has its
own _S0W or a wakeup GPE if it is power-manageable via ACPI.  In those cases
it is not necessary to ask the Root Port's _S0W about wakeup from D3, so overall
I would go for the patch like the below (not really tested).

This works in the same way as the current code (unless I have missed anything)
except for the case when the "target" bridge is power-manageable via ACPI, but
it cannot signal wakeup via ACPI and has no _S0W.  In that case it will consult
the upstream Root Port's _S0W to check whether or not wakeup from D3 is
supported.

[Note that if dev_has_acpi_pm is 'true', it is kind of pointless to look for the
"HotPlugSupportInD3" property of the Root Port, because the function is going to
return 'true' regardless, but I'm not sure if adding an extra if () for handling
this particular case is worth the hassle.]

---
 drivers/pci/pci-acpi.c |   32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -975,6 +975,7 @@ bool acpi_pci_power_manageable(struct pc
 
 bool acpi_pci_bridge_d3(struct pci_dev *dev)
 {
+	bool dev_has_acpi_pm = false;
 	struct pci_dev *rpdev;
 	struct acpi_device *adev;
 	acpi_status status;
@@ -984,17 +985,34 @@ bool acpi_pci_bridge_d3(struct pci_dev *
 	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 && acpi_device_power_manageable(adev)) {
+		/*
+		 * Let the bridge go into D3 if it can signal wakeup from these
+		 * states (i.e. it has _S0W which says so or it can signal
+		 * wakeup via ACPI).
+		 */
+		status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
+		if (ACPI_SUCCESS(status)) {
+			if (state >= ACPI_STATE_D3_HOT)
+				return true;
+		} else if (adev->wakeup.flags.valid) {
+			return true;
+		}
+		/*
+		 * If the bridge is power-manageable by ACPI, let it go into D3
+		 * by default.
+		 */
+		dev_has_acpi_pm = true;
+	}
 
 	rpdev = pcie_find_root_port(dev);
 	if (!rpdev)
-		return false;
+		return dev_has_acpi_pm;
 
 	adev = ACPI_COMPANION(&rpdev->dev);
 	if (!adev)
-		return false;
+		return dev_has_acpi_pm;
 
 	/*
 	 * If the Root Port cannot signal wakeup signals at all, i.e., it
@@ -1002,7 +1020,7 @@ bool acpi_pci_bridge_d3(struct pci_dev *
 	 * events from low-power states including D3hot and D3cold.
 	 */
 	if (!adev->wakeup.flags.valid)
-		return false;
+		return dev_has_acpi_pm;
 
 	/*
 	 * If the Root Port cannot wake itself from D3hot or D3cold, we
@@ -1023,7 +1041,7 @@ bool acpi_pci_bridge_d3(struct pci_dev *
 	    obj->integer.value == 1)
 		return true;
 
-	return false;
+	return dev_has_acpi_pm;
 }
 
 int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
  

On Mon, Nov 21, 2022 at 03:33:00PM +0100, Rafael J. Wysocki wrote:
> On Friday, November 18, 2022 10:13:39 PM CET Rafael J. Wysocki wrote:
> > On Fri, Nov 18, 2022 at 9:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > Hi Rafael,
> > >
> > > Sorry, I'm still confused (my perpetual state :)).
> > 
> > No worries, doing my best to address that.
> > 
> > > On Fri, Nov 18, 2022 at 02:16:17PM +0100, Rafael J. Wysocki wrote:
> > > > On Thu, Nov 17, 2022 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Thu, Nov 17, 2022 at 06:01:26PM +0100, Rafael J. Wysocki wrote:
> > > > > > On Thu, Nov 17, 2022 at 12:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > On Wed, Nov 16, 2022 at 01:00:36PM +0100, Rafael J. Wysocki wrote:
> > > > > > > > On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > > On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> > > > > > > > > > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > > > > > > > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > > > > > > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > > > > > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > > > > > > > > > devices can support D3 by a combination of the value returned by
> > > > > > > > > > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > > > > > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > > > > > > > > > device it's associated with can support D3.  This was introduced
> > > > > > > > > > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > > > > > > > > > D3 if power managed by ACPI").
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > > > > > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > > > > > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > > > > > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > > > > > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > > > > > > > > > device contains power resources.
> > > > > > > > > > >
> > > > > > > > > > > Wait.  Is this as simple as just recognizing that:
> > > > > > > > > > >
> > > > > > > > > > >   _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > > > > > > > > > >   mean the device can wake itself from a low-power state.  The OS has
> > > > > > > > > > >   to use _S0W to learn the device's ability to wake itself.
> > > > > > > > > >
> > > > > > > > > > It is.
> > > > > > > > >
> > > > > > > > > Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
> > > > > > > > > web page [1] says it identifies Root Ports capable of handling hot
> > > > > > > > > plug events while in D3.  That sounds kind of related to _S0W: If _S0W
> > > > > > > > > says "I can wake myself from D3hot and D3cold", how is that different
> > > > > > > > > from "I can handle hotplug events in D3"?
> > > > > > > >
> > > > > > > > For native PME/hot-plug signaling there is no difference.  This is the
> > > > > > > > same interrupt by the spec after all IIRC.
> > > > > > > >
> > > > > > > > For GPE-based signaling, though, there is a difference, because GPEs
> > > > > > > > can only be used directly for wake signaling (this is related to
> > > > > > > > _PRW).  In particular, the only provision in the ACPI spec for device
> > > > > > > > hot-add are the Bus Check and Device Check notification values (0 and
> > > > > > > > 1) which require AML to run and evaluate Notify() on specific AML
> > > > > > > > objects.
> > > > > > > >
> > > > > > > > Hence, there is no spec-defined way to tell the OS that "something can
> > > > > > > > be hot-added under this device while in D3 and you will get notified
> > > > > > > > about that".
> > > > > > >
> > > > > > > So I guess acpi_pci_bridge_d3() looks for:
> > > > > > >
> > > > > > >   - "wake signaling while in D3" (_S0W) and
> > > > > > >   - "notification of hotplug while in D3" ("HotPlugSupportInD3")
> > > > > > >
> > > > > > > For Root Ports with both those abilities (or bridges below such Root
> > > > > > > Ports), we allow D3, and this patch doesn't change that.
> > > > > > >
> > > > > > > What this patch *does* change is that all bridges with _PS0 or _PR0
> > > > > > > previously could use D3, but now will only be able to use D3 if they
> > > > > > > are also (or are below) a Root Port that can signal wakeup
> > > > > > > (wakeup.flags.valid) and can wakeup from D3hot or D3cold (_S0W).
> > > > > > >
> > > > > > > And this fixes the Pink Sardine because it has Root Ports that do
> > > > > > > Thunderbolt tunneling, and they have _PS0 or _PR0 but their _S0W says
> > > > > > > they cannot wake from D3.  Previously we put those in D3, but they
> > > > > > > couldn't wake up.  Now we won't put them in D3.
> > > > > > >
> > > > > > > I guess there's a possibility that this could break or cause higher
> > > > > > > power consumption on systems that were fixed by c6e331312ebf
> > > > > > > ("PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI").
> > > > > > > I don't know enough about that scenario.  Maybe Lukas will chime in.
> > > > > >
> > > > > > Well, it is possible that some of these systems will be affected.
> > > > > >
> > > > > > One of such cases is when the port in question has _S0W which says
> > > > > > that wakeup from D3 is not supported.  In that case I think the kernel
> > > > > > should honor the _S0W input, because there may be a good reason known
> > > > > > to the platform integrator for it.
> > > > > >
> > > > > > The other case is when wakeup.flags.valid is unset for the port's ACPI
> > > > > > companion which means that the port cannot signal wakeup through
> > > > > > ACPI-related means at all and this may be problematic, especially in
> > > > > > the system-wide suspend case in which the wakeup capability is not too
> > > > > > relevant unless there is a system wakeup device under the port.
> > > > > >
> > > > > > I don't think that the adev->wakeup.flags.valid check has any bearing
> > > > > > on the _S0W check - if there is _S0W and it says "no wakeup from D3",
> > > > > > it should still be taken into account - so that check can be moved
> > > > > > past the _S0W check.
> > > > >
> > > > > So if _S0W says it can wake from D3, but wakeup.flags is not valid,
> > > > > it's still OK to use D3?
> > > >
> > > > No, it isn't, as per the code today and I don't think that this
> > > > particular part should be changed now.
> > >
> > > But the current upstream code checks acpi_pci_power_manageable(dev)
> > > first, so if "dev" has _PR0 or _PS0, we'll use D3 even if _S0W says it
> > > can wake from D3 and wakeup.flags is not valid.
> > 
> > Yes, the current code will return 'true' if _PR0 or _PS0 is present
> > for dev regardless of anything else.
> > 
> > The proposed change is to make that conditional on whether or not _S0W
> > for the root port says that wakeup from D3 is supported (or it is not
> > present or unusable).
> > 
> > I see that I've missed one point now which is when the root port
> > doesn't have an ACPI companion, in which case we should go straight
> > for the "dev is power manageable" check.
> 
> Moreover, it is possible that the bridge passed to acpi_pci_bridge_d3() has its
> own _S0W or a wakeup GPE if it is power-manageable via ACPI.  In those cases
> it is not necessary to ask the Root Port's _S0W about wakeup from D3, so overall
> I would go for the patch like the below (not really tested).
> 
> This works in the same way as the current code (unless I have missed anything)
> except for the case when the "target" bridge is power-manageable via ACPI, but
> it cannot signal wakeup via ACPI and has no _S0W.  In that case it will consult
> the upstream Root Port's _S0W to check whether or not wakeup from D3 is
> supported.
> 
> [Note that if dev_has_acpi_pm is 'true', it is kind of pointless to look for the
> "HotPlugSupportInD3" property of the Root Port, because the function is going to
> return 'true' regardless, but I'm not sure if adding an extra if () for handling
> this particular case is worth the hassle.]

I think this has a lot of potential.  I haven't tried it, but I wonder
if splitting out the Root Port-specific parts to a separate function
would be helpful, if only to make it more obvious that there may be
two different devices involved.

If there are two devices ("dev" is a bridge below a Root Port), I
guess support in the Root Port is not necessarily required?  E.g.,
could "dev" assert a wakeup GPE that's not routed through the Root
Port?  If Root Port support *is* required, maybe it would read more
clearly to test that first, before looking at the downstream device.

> ---
>  drivers/pci/pci-acpi.c |   32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-acpi.c
> +++ linux-pm/drivers/pci/pci-acpi.c
> @@ -975,6 +975,7 @@ bool acpi_pci_power_manageable(struct pc
>  
>  bool acpi_pci_bridge_d3(struct pci_dev *dev)
>  {
> +	bool dev_has_acpi_pm = false;
>  	struct pci_dev *rpdev;
>  	struct acpi_device *adev;
>  	acpi_status status;
> @@ -984,17 +985,34 @@ bool acpi_pci_bridge_d3(struct pci_dev *
>  	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 && acpi_device_power_manageable(adev)) {
> +		/*
> +		 * Let the bridge go into D3 if it can signal wakeup from these
> +		 * states (i.e. it has _S0W which says so or it can signal
> +		 * wakeup via ACPI).
> +		 */
> +		status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
> +		if (ACPI_SUCCESS(status)) {
> +			if (state >= ACPI_STATE_D3_HOT)
> +				return true;
> +		} else if (adev->wakeup.flags.valid) {
> +			return true;
> +		}
> +		/*
> +		 * If the bridge is power-manageable by ACPI, let it go into D3
> +		 * by default.
> +		 */
> +		dev_has_acpi_pm = true;
> +	}
>  
>  	rpdev = pcie_find_root_port(dev);
>  	if (!rpdev)
> -		return false;
> +		return dev_has_acpi_pm;
>  
>  	adev = ACPI_COMPANION(&rpdev->dev);
>  	if (!adev)
> -		return false;
> +		return dev_has_acpi_pm;
>  
>  	/*
>  	 * If the Root Port cannot signal wakeup signals at all, i.e., it
> @@ -1002,7 +1020,7 @@ bool acpi_pci_bridge_d3(struct pci_dev *
>  	 * events from low-power states including D3hot and D3cold.
>  	 */
>  	if (!adev->wakeup.flags.valid)
> -		return false;
> +		return dev_has_acpi_pm;
>  
>  	/*
>  	 * If the Root Port cannot wake itself from D3hot or D3cold, we
> @@ -1023,7 +1041,7 @@ bool acpi_pci_bridge_d3(struct pci_dev *
>  	    obj->integer.value == 1)
>  		return true;
>  
> -	return false;
> +	return dev_has_acpi_pm;
>  }
>  
>  int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> 
> 
>
  

On Monday, November 21, 2022 11:17:42 PM CET Bjorn Helgaas wrote:
> On Mon, Nov 21, 2022 at 03:33:00PM +0100, Rafael J. Wysocki wrote:
> > On Friday, November 18, 2022 10:13:39 PM CET Rafael J. Wysocki wrote:
> > > On Fri, Nov 18, 2022 at 9:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > >
> > > > Hi Rafael,
> > > >
> > > > Sorry, I'm still confused (my perpetual state :)).
> > > 
> > > No worries, doing my best to address that.
> > > 
> > > > On Fri, Nov 18, 2022 at 02:16:17PM +0100, Rafael J. Wysocki wrote:
> > > > > On Thu, Nov 17, 2022 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > On Thu, Nov 17, 2022 at 06:01:26PM +0100, Rafael J. Wysocki wrote:
> > > > > > > On Thu, Nov 17, 2022 at 12:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > On Wed, Nov 16, 2022 at 01:00:36PM +0100, Rafael J. Wysocki wrote:
> > > > > > > > > On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > > > On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> > > > > > > > > > > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > > > > > > > > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > > > > > > > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > > > > > > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > > > > > > > > > > devices can support D3 by a combination of the value returned by
> > > > > > > > > > > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > > > > > > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > > > > > > > > > > device it's associated with can support D3.  This was introduced
> > > > > > > > > > > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > > > > > > > > > > D3 if power managed by ACPI").
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > > > > > > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > > > > > > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > > > > > > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > > > > > > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > > > > > > > > > > device contains power resources.
> > > > > > > > > > > >
> > > > > > > > > > > > Wait.  Is this as simple as just recognizing that:
> > > > > > > > > > > >
> > > > > > > > > > > >   _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > > > > > > > > > > >   mean the device can wake itself from a low-power state.  The OS has
> > > > > > > > > > > >   to use _S0W to learn the device's ability to wake itself.
> > > > > > > > > > >
> > > > > > > > > > > It is.
> > > > > > > > > >
> > > > > > > > > > Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
> > > > > > > > > > web page [1] says it identifies Root Ports capable of handling hot
> > > > > > > > > > plug events while in D3.  That sounds kind of related to _S0W: If _S0W
> > > > > > > > > > says "I can wake myself from D3hot and D3cold", how is that different
> > > > > > > > > > from "I can handle hotplug events in D3"?
> > > > > > > > >
> > > > > > > > > For native PME/hot-plug signaling there is no difference.  This is the
> > > > > > > > > same interrupt by the spec after all IIRC.
> > > > > > > > >
> > > > > > > > > For GPE-based signaling, though, there is a difference, because GPEs
> > > > > > > > > can only be used directly for wake signaling (this is related to
> > > > > > > > > _PRW).  In particular, the only provision in the ACPI spec for device
> > > > > > > > > hot-add are the Bus Check and Device Check notification values (0 and
> > > > > > > > > 1) which require AML to run and evaluate Notify() on specific AML
> > > > > > > > > objects.
> > > > > > > > >
> > > > > > > > > Hence, there is no spec-defined way to tell the OS that "something can
> > > > > > > > > be hot-added under this device while in D3 and you will get notified
> > > > > > > > > about that".
> > > > > > > >
> > > > > > > > So I guess acpi_pci_bridge_d3() looks for:
> > > > > > > >
> > > > > > > >   - "wake signaling while in D3" (_S0W) and
> > > > > > > >   - "notification of hotplug while in D3" ("HotPlugSupportInD3")
> > > > > > > >
> > > > > > > > For Root Ports with both those abilities (or bridges below such Root
> > > > > > > > Ports), we allow D3, and this patch doesn't change that.
> > > > > > > >
> > > > > > > > What this patch *does* change is that all bridges with _PS0 or _PR0
> > > > > > > > previously could use D3, but now will only be able to use D3 if they
> > > > > > > > are also (or are below) a Root Port that can signal wakeup
> > > > > > > > (wakeup.flags.valid) and can wakeup from D3hot or D3cold (_S0W).
> > > > > > > >
> > > > > > > > And this fixes the Pink Sardine because it has Root Ports that do
> > > > > > > > Thunderbolt tunneling, and they have _PS0 or _PR0 but their _S0W says
> > > > > > > > they cannot wake from D3.  Previously we put those in D3, but they
> > > > > > > > couldn't wake up.  Now we won't put them in D3.
> > > > > > > >
> > > > > > > > I guess there's a possibility that this could break or cause higher
> > > > > > > > power consumption on systems that were fixed by c6e331312ebf
> > > > > > > > ("PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI").
> > > > > > > > I don't know enough about that scenario.  Maybe Lukas will chime in.
> > > > > > >
> > > > > > > Well, it is possible that some of these systems will be affected.
> > > > > > >
> > > > > > > One of such cases is when the port in question has _S0W which says
> > > > > > > that wakeup from D3 is not supported.  In that case I think the kernel
> > > > > > > should honor the _S0W input, because there may be a good reason known
> > > > > > > to the platform integrator for it.
> > > > > > >
> > > > > > > The other case is when wakeup.flags.valid is unset for the port's ACPI
> > > > > > > companion which means that the port cannot signal wakeup through
> > > > > > > ACPI-related means at all and this may be problematic, especially in
> > > > > > > the system-wide suspend case in which the wakeup capability is not too
> > > > > > > relevant unless there is a system wakeup device under the port.
> > > > > > >
> > > > > > > I don't think that the adev->wakeup.flags.valid check has any bearing
> > > > > > > on the _S0W check - if there is _S0W and it says "no wakeup from D3",
> > > > > > > it should still be taken into account - so that check can be moved
> > > > > > > past the _S0W check.
> > > > > >
> > > > > > So if _S0W says it can wake from D3, but wakeup.flags is not valid,
> > > > > > it's still OK to use D3?
> > > > >
> > > > > No, it isn't, as per the code today and I don't think that this
> > > > > particular part should be changed now.
> > > >
> > > > But the current upstream code checks acpi_pci_power_manageable(dev)
> > > > first, so if "dev" has _PR0 or _PS0, we'll use D3 even if _S0W says it
> > > > can wake from D3 and wakeup.flags is not valid.
> > > 
> > > Yes, the current code will return 'true' if _PR0 or _PS0 is present
> > > for dev regardless of anything else.
> > > 
> > > The proposed change is to make that conditional on whether or not _S0W
> > > for the root port says that wakeup from D3 is supported (or it is not
> > > present or unusable).
> > > 
> > > I see that I've missed one point now which is when the root port
> > > doesn't have an ACPI companion, in which case we should go straight
> > > for the "dev is power manageable" check.
> > 
> > Moreover, it is possible that the bridge passed to acpi_pci_bridge_d3() has its
> > own _S0W or a wakeup GPE if it is power-manageable via ACPI.  In those cases
> > it is not necessary to ask the Root Port's _S0W about wakeup from D3, so overall
> > I would go for the patch like the below (not really tested).
> > 
> > This works in the same way as the current code (unless I have missed anything)
> > except for the case when the "target" bridge is power-manageable via ACPI, but
> > it cannot signal wakeup via ACPI and has no _S0W.  In that case it will consult
> > the upstream Root Port's _S0W to check whether or not wakeup from D3 is
> > supported.
> > 
> > [Note that if dev_has_acpi_pm is 'true', it is kind of pointless to look for the
> > "HotPlugSupportInD3" property of the Root Port, because the function is going to
> > return 'true' regardless, but I'm not sure if adding an extra if () for handling
> > this particular case is worth the hassle.]
> 
> I think this has a lot of potential.  I haven't tried it, but I wonder
> if splitting out the Root Port-specific parts to a separate function
> would be helpful, if only to make it more obvious that there may be
> two different devices involved.
> 
> If there are two devices ("dev" is a bridge below a Root Port), I
> guess support in the Root Port is not necessarily required?  E.g.,
> could "dev" assert a wakeup GPE that's not routed through the Root
> Port?  If Root Port support *is* required, maybe it would read more
> clearly to test that first, before looking at the downstream device.

Sorry for the delay.

I don't really think that Root Port support is required for a bridge below
a Root Port if that bridge itself is power-manageable via ACPI.  Moreover,
I don't think that the _S0W of a Root Port has any bearing on devices below
it that have their own _S0W.

So what we really want appears to be to evaluate _S0W for the target bridge,
regardless of whether or not it is a Root Port, and return 'false' if that
produces D2 or a shallower power state.  Otherwise, we can do what we've
done so far.

The patch below implements, this - please let me know what you think.

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] PCI / ACPI: PM: Take _S0W of the target bridge into account in acpi_pci_bridge_d3(()

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.

Reported-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci-acpi.c |   39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -984,15 +984,33 @@ bool acpi_pci_bridge_d3(struct pci_dev *
 	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.
+		 */
+		status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
+		if (ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT)
+			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 (rpdev != dev)
+		adev = ACPI_COMPANION(&rpdev->dev);
+
 	if (!adev)
 		return false;
 
@@ -1005,13 +1023,14 @@ bool acpi_pci_bridge_d3(struct pci_dev *
 		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)
-		return false;
-
+	if (rpdev != dev) {
+		status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
+		if (ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT)
+			return false;
+	}
 	/*
 	 * The "HotPlugSupportInD3" property in a Root Port _DSD indicates
 	 * the Port can signal hotplug events while in D3.  We assume any
  

On Monday, January 2, 2023 5:34:19 PM CET Rafael J. Wysocki wrote:
> On Monday, November 21, 2022 11:17:42 PM CET Bjorn Helgaas wrote:
> > On Mon, Nov 21, 2022 at 03:33:00PM +0100, Rafael J. Wysocki wrote:
> > > On Friday, November 18, 2022 10:13:39 PM CET Rafael J. Wysocki wrote:
> > > > On Fri, Nov 18, 2022 at 9:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > >
> > > > > Hi Rafael,
> > > > >
> > > > > Sorry, I'm still confused (my perpetual state :)).
> > > > 
> > > > No worries, doing my best to address that.
> > > > 
> > > > > On Fri, Nov 18, 2022 at 02:16:17PM +0100, Rafael J. Wysocki wrote:
> > > > > > On Thu, Nov 17, 2022 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > On Thu, Nov 17, 2022 at 06:01:26PM +0100, Rafael J. Wysocki wrote:
> > > > > > > > On Thu, Nov 17, 2022 at 12:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > > On Wed, Nov 16, 2022 at 01:00:36PM +0100, Rafael J. Wysocki wrote:
> > > > > > > > > > On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > > > > On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> > > > > > > > > > > > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > > > > > > > > > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > > > > > > > > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > > > > > > > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > > > > > > > > > > > devices can support D3 by a combination of the value returned by
> > > > > > > > > > > > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > > > > > > > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > > > > > > > > > > > device it's associated with can support D3.  This was introduced
> > > > > > > > > > > > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > > > > > > > > > > > D3 if power managed by ACPI").
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > > > > > > > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > > > > > > > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > > > > > > > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > > > > > > > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > > > > > > > > > > > device contains power resources.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Wait.  Is this as simple as just recognizing that:
> > > > > > > > > > > > >
> > > > > > > > > > > > >   _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > > > > > > > > > > > >   mean the device can wake itself from a low-power state.  The OS has
> > > > > > > > > > > > >   to use _S0W to learn the device's ability to wake itself.
> > > > > > > > > > > >
> > > > > > > > > > > > It is.
> > > > > > > > > > >
> > > > > > > > > > > Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
> > > > > > > > > > > web page [1] says it identifies Root Ports capable of handling hot
> > > > > > > > > > > plug events while in D3.  That sounds kind of related to _S0W: If _S0W
> > > > > > > > > > > says "I can wake myself from D3hot and D3cold", how is that different
> > > > > > > > > > > from "I can handle hotplug events in D3"?
> > > > > > > > > >
> > > > > > > > > > For native PME/hot-plug signaling there is no difference.  This is the
> > > > > > > > > > same interrupt by the spec after all IIRC.
> > > > > > > > > >
> > > > > > > > > > For GPE-based signaling, though, there is a difference, because GPEs
> > > > > > > > > > can only be used directly for wake signaling (this is related to
> > > > > > > > > > _PRW).  In particular, the only provision in the ACPI spec for device
> > > > > > > > > > hot-add are the Bus Check and Device Check notification values (0 and
> > > > > > > > > > 1) which require AML to run and evaluate Notify() on specific AML
> > > > > > > > > > objects.
> > > > > > > > > >
> > > > > > > > > > Hence, there is no spec-defined way to tell the OS that "something can
> > > > > > > > > > be hot-added under this device while in D3 and you will get notified
> > > > > > > > > > about that".
> > > > > > > > >
> > > > > > > > > So I guess acpi_pci_bridge_d3() looks for:
> > > > > > > > >
> > > > > > > > >   - "wake signaling while in D3" (_S0W) and
> > > > > > > > >   - "notification of hotplug while in D3" ("HotPlugSupportInD3")
> > > > > > > > >
> > > > > > > > > For Root Ports with both those abilities (or bridges below such Root
> > > > > > > > > Ports), we allow D3, and this patch doesn't change that.
> > > > > > > > >
> > > > > > > > > What this patch *does* change is that all bridges with _PS0 or _PR0
> > > > > > > > > previously could use D3, but now will only be able to use D3 if they
> > > > > > > > > are also (or are below) a Root Port that can signal wakeup
> > > > > > > > > (wakeup.flags.valid) and can wakeup from D3hot or D3cold (_S0W).
> > > > > > > > >
> > > > > > > > > And this fixes the Pink Sardine because it has Root Ports that do
> > > > > > > > > Thunderbolt tunneling, and they have _PS0 or _PR0 but their _S0W says
> > > > > > > > > they cannot wake from D3.  Previously we put those in D3, but they
> > > > > > > > > couldn't wake up.  Now we won't put them in D3.
> > > > > > > > >
> > > > > > > > > I guess there's a possibility that this could break or cause higher
> > > > > > > > > power consumption on systems that were fixed by c6e331312ebf
> > > > > > > > > ("PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI").
> > > > > > > > > I don't know enough about that scenario.  Maybe Lukas will chime in.
> > > > > > > >
> > > > > > > > Well, it is possible that some of these systems will be affected.
> > > > > > > >
> > > > > > > > One of such cases is when the port in question has _S0W which says
> > > > > > > > that wakeup from D3 is not supported.  In that case I think the kernel
> > > > > > > > should honor the _S0W input, because there may be a good reason known
> > > > > > > > to the platform integrator for it.
> > > > > > > >
> > > > > > > > The other case is when wakeup.flags.valid is unset for the port's ACPI
> > > > > > > > companion which means that the port cannot signal wakeup through
> > > > > > > > ACPI-related means at all and this may be problematic, especially in
> > > > > > > > the system-wide suspend case in which the wakeup capability is not too
> > > > > > > > relevant unless there is a system wakeup device under the port.
> > > > > > > >
> > > > > > > > I don't think that the adev->wakeup.flags.valid check has any bearing
> > > > > > > > on the _S0W check - if there is _S0W and it says "no wakeup from D3",
> > > > > > > > it should still be taken into account - so that check can be moved
> > > > > > > > past the _S0W check.
> > > > > > >
> > > > > > > So if _S0W says it can wake from D3, but wakeup.flags is not valid,
> > > > > > > it's still OK to use D3?
> > > > > >
> > > > > > No, it isn't, as per the code today and I don't think that this
> > > > > > particular part should be changed now.
> > > > >
> > > > > But the current upstream code checks acpi_pci_power_manageable(dev)
> > > > > first, so if "dev" has _PR0 or _PS0, we'll use D3 even if _S0W says it
> > > > > can wake from D3 and wakeup.flags is not valid.
> > > > 
> > > > Yes, the current code will return 'true' if _PR0 or _PS0 is present
> > > > for dev regardless of anything else.
> > > > 
> > > > The proposed change is to make that conditional on whether or not _S0W
> > > > for the root port says that wakeup from D3 is supported (or it is not
> > > > present or unusable).
> > > > 
> > > > I see that I've missed one point now which is when the root port
> > > > doesn't have an ACPI companion, in which case we should go straight
> > > > for the "dev is power manageable" check.
> > > 
> > > Moreover, it is possible that the bridge passed to acpi_pci_bridge_d3() has its
> > > own _S0W or a wakeup GPE if it is power-manageable via ACPI.  In those cases
> > > it is not necessary to ask the Root Port's _S0W about wakeup from D3, so overall
> > > I would go for the patch like the below (not really tested).
> > > 
> > > This works in the same way as the current code (unless I have missed anything)
> > > except for the case when the "target" bridge is power-manageable via ACPI, but
> > > it cannot signal wakeup via ACPI and has no _S0W.  In that case it will consult
> > > the upstream Root Port's _S0W to check whether or not wakeup from D3 is
> > > supported.
> > > 
> > > [Note that if dev_has_acpi_pm is 'true', it is kind of pointless to look for the
> > > "HotPlugSupportInD3" property of the Root Port, because the function is going to
> > > return 'true' regardless, but I'm not sure if adding an extra if () for handling
> > > this particular case is worth the hassle.]
> > 
> > I think this has a lot of potential.  I haven't tried it, but I wonder
> > if splitting out the Root Port-specific parts to a separate function
> > would be helpful, if only to make it more obvious that there may be
> > two different devices involved.
> > 
> > If there are two devices ("dev" is a bridge below a Root Port), I
> > guess support in the Root Port is not necessarily required?  E.g.,
> > could "dev" assert a wakeup GPE that's not routed through the Root
> > Port?  If Root Port support *is* required, maybe it would read more
> > clearly to test that first, before looking at the downstream device.
> 
> Sorry for the delay.
> 
> I don't really think that Root Port support is required for a bridge below
> a Root Port if that bridge itself is power-manageable via ACPI.  Moreover,
> I don't think that the _S0W of a Root Port has any bearing on devices below
> it that have their own _S0W.
> 
> So what we really want appears to be to evaluate _S0W for the target bridge,
> regardless of whether or not it is a Root Port, and return 'false' if that
> produces D2 or a shallower power state.  Otherwise, we can do what we've
> done so far.
> 
> The patch below implements, this - please let me know what you think.
> 

And here's a v2 with somewhat less code duplication.

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] PCI / ACPI: PM: Take _S0W of the target bridge into account in acpi_pci_bridge_d3(()

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.

Reported-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   16 ++++++++++++++++
 drivers/pci/pci-acpi.c   |   34 ++++++++++++++++++++++++----------
 include/acpi/acpi_bus.h  |    1 +
 3 files changed, 41 insertions(+), 10 deletions(-)

Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -977,22 +977,37 @@ bool acpi_pci_bridge_d3(struct pci_dev *
 {
 	struct pci_dev *rpdev;
 	struct acpi_device *adev;
-	acpi_status status;
-	unsigned long long state;
 	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_no_wakeup_from_d3(adev))
+			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 (rpdev != dev)
+		adev = ACPI_COMPANION(&rpdev->dev);
+
 	if (!adev)
 		return false;
 
@@ -1005,11 +1020,10 @@ bool acpi_pci_bridge_d3(struct pci_dev *
 		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 (rpdev != dev && acpi_dev_no_wakeup_from_d3(adev))
 		return false;
 
 	/*
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,22 @@ void acpi_dev_power_up_children_with_adr
 	acpi_dev_for_each_child(adev, acpi_power_up_if_adr_present, NULL);
 }
 
+/**
+ * acpi_dev_no_wakeup_from_d3 - Check if wakeup signaling from D3 is supported
+ * @adev: ACPI companion of the target device.
+ *
+ * Evaluate _S0W for @adev and return 'true' if it is successful and the power
+ * state returned by it is D2 or shallower.
+ */
+bool acpi_dev_no_wakeup_from_d3(struct acpi_device *adev)
+{
+	unsigned long long state;
+	acpi_status status;
+
+	status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
+	return ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT;
+}
+
 #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);
+bool acpi_dev_no_wakeup_from_d3(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,
  

On 1/2/2023 10:59, Rafael J. Wysocki wrote:
> On Monday, January 2, 2023 5:34:19 PM CET Rafael J. Wysocki wrote:
>> On Monday, November 21, 2022 11:17:42 PM CET Bjorn Helgaas wrote:
>>> On Mon, Nov 21, 2022 at 03:33:00PM +0100, Rafael J. Wysocki wrote:
>>>> On Friday, November 18, 2022 10:13:39 PM CET Rafael J. Wysocki wrote:
>>>>> On Fri, Nov 18, 2022 at 9:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>>>
>>>>>> Hi Rafael,
>>>>>>
>>>>>> Sorry, I'm still confused (my perpetual state :)).
>>>>>
>>>>> No worries, doing my best to address that.
>>>>>
>>>>>> On Fri, Nov 18, 2022 at 02:16:17PM +0100, Rafael J. Wysocki wrote:
>>>>>>> On Thu, Nov 17, 2022 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>>>>> On Thu, Nov 17, 2022 at 06:01:26PM +0100, Rafael J. Wysocki wrote:
>>>>>>>>> On Thu, Nov 17, 2022 at 12:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>>>>>>> On Wed, Nov 16, 2022 at 01:00:36PM +0100, Rafael J. Wysocki wrote:
>>>>>>>>>>> On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>>>>>>>>> On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
>>>>>>>>>>>>> On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
>>>>>>>>>>>>>>> On 11/11/2022 11:41, Bjorn Helgaas wrote:
>>>>>>>>>>>>>>>> On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
>>>>>>>>>>>>>>>>> Firmware typically advertises that ACPI devices that represent PCIe
>>>>>>>>>>>>>>>>> devices can support D3 by a combination of the value returned by
>>>>>>>>>>>>>>>>> _S0W as well as the HotPlugSupportInD3 _DSD [1].
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> `acpi_pci_bridge_d3` looks for this combination but also contains
>>>>>>>>>>>>>>>>> an assumption that if an ACPI device contains power resources the PCIe
>>>>>>>>>>>>>>>>> device it's associated with can support D3.  This was introduced
>>>>>>>>>>>>>>>>> from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
>>>>>>>>>>>>>>>>> D3 if power managed by ACPI").
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Some firmware configurations for "AMD Pink Sardine" do not support
>>>>>>>>>>>>>>>>> wake from D3 in _S0W for the ACPI device representing the PCIe root
>>>>>>>>>>>>>>>>> port used for tunneling. The PCIe device will still be opted into
>>>>>>>>>>>>>>>>> runtime PM in the kernel [2] because of the logic within
>>>>>>>>>>>>>>>>> `acpi_pci_bridge_d3`. This currently happens because the ACPI
>>>>>>>>>>>>>>>>> device contains power resources.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Wait.  Is this as simple as just recognizing that:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>    _PS0 means the OS has a knob to put the device in D0, but it doesn't
>>>>>>>>>>>>>>    mean the device can wake itself from a low-power state.  The OS has
>>>>>>>>>>>>>>    to use _S0W to learn the device's ability to wake itself.
>>>>>>>>>>>>>
>>>>>>>>>>>>> It is.
>>>>>>>>>>>>
>>>>>>>>>>>> Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
>>>>>>>>>>>> web page [1] says it identifies Root Ports capable of handling hot
>>>>>>>>>>>> plug events while in D3.  That sounds kind of related to _S0W: If _S0W
>>>>>>>>>>>> says "I can wake myself from D3hot and D3cold", how is that different
>>>>>>>>>>>> from "I can handle hotplug events in D3"?
>>>>>>>>>>>
>>>>>>>>>>> For native PME/hot-plug signaling there is no difference.  This is the
>>>>>>>>>>> same interrupt by the spec after all IIRC.
>>>>>>>>>>>
>>>>>>>>>>> For GPE-based signaling, though, there is a difference, because GPEs
>>>>>>>>>>> can only be used directly for wake signaling (this is related to
>>>>>>>>>>> _PRW).  In particular, the only provision in the ACPI spec for device
>>>>>>>>>>> hot-add are the Bus Check and Device Check notification values (0 and
>>>>>>>>>>> 1) which require AML to run and evaluate Notify() on specific AML
>>>>>>>>>>> objects.
>>>>>>>>>>>
>>>>>>>>>>> Hence, there is no spec-defined way to tell the OS that "something can
>>>>>>>>>>> be hot-added under this device while in D3 and you will get notified
>>>>>>>>>>> about that".
>>>>>>>>>>
>>>>>>>>>> So I guess acpi_pci_bridge_d3() looks for:
>>>>>>>>>>
>>>>>>>>>>    - "wake signaling while in D3" (_S0W) and
>>>>>>>>>>    - "notification of hotplug while in D3" ("HotPlugSupportInD3")
>>>>>>>>>>
>>>>>>>>>> For Root Ports with both those abilities (or bridges below such Root
>>>>>>>>>> Ports), we allow D3, and this patch doesn't change that.
>>>>>>>>>>
>>>>>>>>>> What this patch *does* change is that all bridges with _PS0 or _PR0
>>>>>>>>>> previously could use D3, but now will only be able to use D3 if they
>>>>>>>>>> are also (or are below) a Root Port that can signal wakeup
>>>>>>>>>> (wakeup.flags.valid) and can wakeup from D3hot or D3cold (_S0W).
>>>>>>>>>>
>>>>>>>>>> And this fixes the Pink Sardine because it has Root Ports that do
>>>>>>>>>> Thunderbolt tunneling, and they have _PS0 or _PR0 but their _S0W says
>>>>>>>>>> they cannot wake from D3.  Previously we put those in D3, but they
>>>>>>>>>> couldn't wake up.  Now we won't put them in D3.
>>>>>>>>>>
>>>>>>>>>> I guess there's a possibility that this could break or cause higher
>>>>>>>>>> power consumption on systems that were fixed by c6e331312ebf
>>>>>>>>>> ("PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI").
>>>>>>>>>> I don't know enough about that scenario.  Maybe Lukas will chime in.
>>>>>>>>>
>>>>>>>>> Well, it is possible that some of these systems will be affected.
>>>>>>>>>
>>>>>>>>> One of such cases is when the port in question has _S0W which says
>>>>>>>>> that wakeup from D3 is not supported.  In that case I think the kernel
>>>>>>>>> should honor the _S0W input, because there may be a good reason known
>>>>>>>>> to the platform integrator for it.
>>>>>>>>>
>>>>>>>>> The other case is when wakeup.flags.valid is unset for the port's ACPI
>>>>>>>>> companion which means that the port cannot signal wakeup through
>>>>>>>>> ACPI-related means at all and this may be problematic, especially in
>>>>>>>>> the system-wide suspend case in which the wakeup capability is not too
>>>>>>>>> relevant unless there is a system wakeup device under the port.
>>>>>>>>>
>>>>>>>>> I don't think that the adev->wakeup.flags.valid check has any bearing
>>>>>>>>> on the _S0W check - if there is _S0W and it says "no wakeup from D3",
>>>>>>>>> it should still be taken into account - so that check can be moved
>>>>>>>>> past the _S0W check.
>>>>>>>>
>>>>>>>> So if _S0W says it can wake from D3, but wakeup.flags is not valid,
>>>>>>>> it's still OK to use D3?
>>>>>>>
>>>>>>> No, it isn't, as per the code today and I don't think that this
>>>>>>> particular part should be changed now.
>>>>>>
>>>>>> But the current upstream code checks acpi_pci_power_manageable(dev)
>>>>>> first, so if "dev" has _PR0 or _PS0, we'll use D3 even if _S0W says it
>>>>>> can wake from D3 and wakeup.flags is not valid.
>>>>>
>>>>> Yes, the current code will return 'true' if _PR0 or _PS0 is present
>>>>> for dev regardless of anything else.
>>>>>
>>>>> The proposed change is to make that conditional on whether or not _S0W
>>>>> for the root port says that wakeup from D3 is supported (or it is not
>>>>> present or unusable).
>>>>>
>>>>> I see that I've missed one point now which is when the root port
>>>>> doesn't have an ACPI companion, in which case we should go straight
>>>>> for the "dev is power manageable" check.
>>>>
>>>> Moreover, it is possible that the bridge passed to acpi_pci_bridge_d3() has its
>>>> own _S0W or a wakeup GPE if it is power-manageable via ACPI.  In those cases
>>>> it is not necessary to ask the Root Port's _S0W about wakeup from D3, so overall
>>>> I would go for the patch like the below (not really tested).
>>>>
>>>> This works in the same way as the current code (unless I have missed anything)
>>>> except for the case when the "target" bridge is power-manageable via ACPI, but
>>>> it cannot signal wakeup via ACPI and has no _S0W.  In that case it will consult
>>>> the upstream Root Port's _S0W to check whether or not wakeup from D3 is
>>>> supported.
>>>>
>>>> [Note that if dev_has_acpi_pm is 'true', it is kind of pointless to look for the
>>>> "HotPlugSupportInD3" property of the Root Port, because the function is going to
>>>> return 'true' regardless, but I'm not sure if adding an extra if () for handling
>>>> this particular case is worth the hassle.]
>>>
>>> I think this has a lot of potential.  I haven't tried it, but I wonder
>>> if splitting out the Root Port-specific parts to a separate function
>>> would be helpful, if only to make it more obvious that there may be
>>> two different devices involved.
>>>
>>> If there are two devices ("dev" is a bridge below a Root Port), I
>>> guess support in the Root Port is not necessarily required?  E.g.,
>>> could "dev" assert a wakeup GPE that's not routed through the Root
>>> Port?  If Root Port support *is* required, maybe it would read more
>>> clearly to test that first, before looking at the downstream device.
>>
>> Sorry for the delay.
>>
>> I don't really think that Root Port support is required for a bridge below
>> a Root Port if that bridge itself is power-manageable via ACPI.  Moreover,
>> I don't think that the _S0W of a Root Port has any bearing on devices below
>> it that have their own _S0W.
>>
>> So what we really want appears to be to evaluate _S0W for the target bridge,
>> regardless of whether or not it is a Root Port, and return 'false' if that
>> produces D2 or a shallower power state.  Otherwise, we can do what we've
>> done so far.
>>
>> The patch below implements, this - please let me know what you think.

This looks good to me, thanks.

>>
> 
> And here's a v2 with somewhat less code duplication.
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] PCI / ACPI: PM: Take _S0W of the target bridge into account in acpi_pci_bridge_d3(()
> 
> 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.
> 
> Reported-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/acpi/device_pm.c |   16 ++++++++++++++++
>   drivers/pci/pci-acpi.c   |   34 ++++++++++++++++++++++++----------
>   include/acpi/acpi_bus.h  |    1 +
>   3 files changed, 41 insertions(+), 10 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-acpi.c
> +++ linux-pm/drivers/pci/pci-acpi.c
> @@ -977,22 +977,37 @@ bool acpi_pci_bridge_d3(struct pci_dev *
>   {
>   	struct pci_dev *rpdev;
>   	struct acpi_device *adev;
> -	acpi_status status;
> -	unsigned long long state;
>   	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_no_wakeup_from_d3(adev))
> +			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 (rpdev != dev)
> +		adev = ACPI_COMPANION(&rpdev->dev);
> +
>   	if (!adev)
>   		return false;
>   
> @@ -1005,11 +1020,10 @@ bool acpi_pci_bridge_d3(struct pci_dev *
>   		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 (rpdev != dev && acpi_dev_no_wakeup_from_d3(adev))
>   		return false;
>   
>   	/*
> 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,22 @@ void acpi_dev_power_up_children_with_adr
>   	acpi_dev_for_each_child(adev, acpi_power_up_if_adr_present, NULL);
>   }
>   
> +/**
> + * acpi_dev_no_wakeup_from_d3 - Check if wakeup signaling from D3 is supported
> + * @adev: ACPI companion of the target device.
> + *
> + * Evaluate _S0W for @adev and return 'true' if it is successful and the power
> + * state returned by it is D2 or shallower.
> + */
> +bool acpi_dev_no_wakeup_from_d3(struct acpi_device *adev)
> +{
> +	unsigned long long state;
> +	acpi_status status;
> +
> +	status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
> +	return ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT;
> +}
> +
>   #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);
> +bool acpi_dev_no_wakeup_from_d3(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,
> 
> 
>
  

On Monday, January 2, 2023 5:59:36 PM CET Rafael J. Wysocki wrote:
> On Monday, January 2, 2023 5:34:19 PM CET Rafael J. Wysocki wrote:
> > On Monday, November 21, 2022 11:17:42 PM CET Bjorn Helgaas wrote:
> > > On Mon, Nov 21, 2022 at 03:33:00PM +0100, Rafael J. Wysocki wrote:
> > > > On Friday, November 18, 2022 10:13:39 PM CET Rafael J. Wysocki wrote:
> > > > > On Fri, Nov 18, 2022 at 9:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > >
> > > > > > Hi Rafael,
> > > > > >
> > > > > > Sorry, I'm still confused (my perpetual state :)).
> > > > > 
> > > > > No worries, doing my best to address that.
> > > > > 
> > > > > > On Fri, Nov 18, 2022 at 02:16:17PM +0100, Rafael J. Wysocki wrote:
> > > > > > > On Thu, Nov 17, 2022 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > On Thu, Nov 17, 2022 at 06:01:26PM +0100, Rafael J. Wysocki wrote:
> > > > > > > > > On Thu, Nov 17, 2022 at 12:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > > > On Wed, Nov 16, 2022 at 01:00:36PM +0100, Rafael J. Wysocki wrote:
> > > > > > > > > > > On Wed, Nov 16, 2022 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > > > > > On Mon, Nov 14, 2022 at 04:33:52PM +0100, Rafael J. Wysocki wrote:
> > > > > > > > > > > > > On Fri, Nov 11, 2022 at 10:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Fri, Nov 11, 2022 at 12:58:28PM -0600, Limonciello, Mario wrote:
> > > > > > > > > > > > > > > On 11/11/2022 11:41, Bjorn Helgaas wrote:
> > > > > > > > > > > > > > > > On Mon, Oct 31, 2022 at 05:33:55PM -0500, Mario Limonciello wrote:
> > > > > > > > > > > > > > > > > Firmware typically advertises that ACPI devices that represent PCIe
> > > > > > > > > > > > > > > > > devices can support D3 by a combination of the value returned by
> > > > > > > > > > > > > > > > > _S0W as well as the HotPlugSupportInD3 _DSD [1].
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > `acpi_pci_bridge_d3` looks for this combination but also contains
> > > > > > > > > > > > > > > > > an assumption that if an ACPI device contains power resources the PCIe
> > > > > > > > > > > > > > > > > device it's associated with can support D3.  This was introduced
> > > > > > > > > > > > > > > > > from commit c6e331312ebf ("PCI/ACPI: Whitelist hotplug ports for
> > > > > > > > > > > > > > > > > D3 if power managed by ACPI").
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Some firmware configurations for "AMD Pink Sardine" do not support
> > > > > > > > > > > > > > > > > wake from D3 in _S0W for the ACPI device representing the PCIe root
> > > > > > > > > > > > > > > > > port used for tunneling. The PCIe device will still be opted into
> > > > > > > > > > > > > > > > > runtime PM in the kernel [2] because of the logic within
> > > > > > > > > > > > > > > > > `acpi_pci_bridge_d3`. This currently happens because the ACPI
> > > > > > > > > > > > > > > > > device contains power resources.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Wait.  Is this as simple as just recognizing that:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >   _PS0 means the OS has a knob to put the device in D0, but it doesn't
> > > > > > > > > > > > > >   mean the device can wake itself from a low-power state.  The OS has
> > > > > > > > > > > > > >   to use _S0W to learn the device's ability to wake itself.
> > > > > > > > > > > > >
> > > > > > > > > > > > > It is.
> > > > > > > > > > > >
> > > > > > > > > > > > Now I'm confused again about what "HotPlugSupportInD3" means.  The MS
> > > > > > > > > > > > web page [1] says it identifies Root Ports capable of handling hot
> > > > > > > > > > > > plug events while in D3.  That sounds kind of related to _S0W: If _S0W
> > > > > > > > > > > > says "I can wake myself from D3hot and D3cold", how is that different
> > > > > > > > > > > > from "I can handle hotplug events in D3"?
> > > > > > > > > > >
> > > > > > > > > > > For native PME/hot-plug signaling there is no difference.  This is the
> > > > > > > > > > > same interrupt by the spec after all IIRC.
> > > > > > > > > > >
> > > > > > > > > > > For GPE-based signaling, though, there is a difference, because GPEs
> > > > > > > > > > > can only be used directly for wake signaling (this is related to
> > > > > > > > > > > _PRW).  In particular, the only provision in the ACPI spec for device
> > > > > > > > > > > hot-add are the Bus Check and Device Check notification values (0 and
> > > > > > > > > > > 1) which require AML to run and evaluate Notify() on specific AML
> > > > > > > > > > > objects.
> > > > > > > > > > >
> > > > > > > > > > > Hence, there is no spec-defined way to tell the OS that "something can
> > > > > > > > > > > be hot-added under this device while in D3 and you will get notified
> > > > > > > > > > > about that".
> > > > > > > > > >
> > > > > > > > > > So I guess acpi_pci_bridge_d3() looks for:
> > > > > > > > > >
> > > > > > > > > >   - "wake signaling while in D3" (_S0W) and
> > > > > > > > > >   - "notification of hotplug while in D3" ("HotPlugSupportInD3")
> > > > > > > > > >
> > > > > > > > > > For Root Ports with both those abilities (or bridges below such Root
> > > > > > > > > > Ports), we allow D3, and this patch doesn't change that.
> > > > > > > > > >
> > > > > > > > > > What this patch *does* change is that all bridges with _PS0 or _PR0
> > > > > > > > > > previously could use D3, but now will only be able to use D3 if they
> > > > > > > > > > are also (or are below) a Root Port that can signal wakeup
> > > > > > > > > > (wakeup.flags.valid) and can wakeup from D3hot or D3cold (_S0W).
> > > > > > > > > >
> > > > > > > > > > And this fixes the Pink Sardine because it has Root Ports that do
> > > > > > > > > > Thunderbolt tunneling, and they have _PS0 or _PR0 but their _S0W says
> > > > > > > > > > they cannot wake from D3.  Previously we put those in D3, but they
> > > > > > > > > > couldn't wake up.  Now we won't put them in D3.
> > > > > > > > > >
> > > > > > > > > > I guess there's a possibility that this could break or cause higher
> > > > > > > > > > power consumption on systems that were fixed by c6e331312ebf
> > > > > > > > > > ("PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI").
> > > > > > > > > > I don't know enough about that scenario.  Maybe Lukas will chime in.
> > > > > > > > >
> > > > > > > > > Well, it is possible that some of these systems will be affected.
> > > > > > > > >
> > > > > > > > > One of such cases is when the port in question has _S0W which says
> > > > > > > > > that wakeup from D3 is not supported.  In that case I think the kernel
> > > > > > > > > should honor the _S0W input, because there may be a good reason known
> > > > > > > > > to the platform integrator for it.
> > > > > > > > >
> > > > > > > > > The other case is when wakeup.flags.valid is unset for the port's ACPI
> > > > > > > > > companion which means that the port cannot signal wakeup through
> > > > > > > > > ACPI-related means at all and this may be problematic, especially in
> > > > > > > > > the system-wide suspend case in which the wakeup capability is not too
> > > > > > > > > relevant unless there is a system wakeup device under the port.
> > > > > > > > >
> > > > > > > > > I don't think that the adev->wakeup.flags.valid check has any bearing
> > > > > > > > > on the _S0W check - if there is _S0W and it says "no wakeup from D3",
> > > > > > > > > it should still be taken into account - so that check can be moved
> > > > > > > > > past the _S0W check.
> > > > > > > >
> > > > > > > > So if _S0W says it can wake from D3, but wakeup.flags is not valid,
> > > > > > > > it's still OK to use D3?
> > > > > > >
> > > > > > > No, it isn't, as per the code today and I don't think that this
> > > > > > > particular part should be changed now.
> > > > > >
> > > > > > But the current upstream code checks acpi_pci_power_manageable(dev)
> > > > > > first, so if "dev" has _PR0 or _PS0, we'll use D3 even if _S0W says it
> > > > > > can wake from D3 and wakeup.flags is not valid.
> > > > > 
> > > > > Yes, the current code will return 'true' if _PR0 or _PS0 is present
> > > > > for dev regardless of anything else.
> > > > > 
> > > > > The proposed change is to make that conditional on whether or not _S0W
> > > > > for the root port says that wakeup from D3 is supported (or it is not
> > > > > present or unusable).
> > > > > 
> > > > > I see that I've missed one point now which is when the root port
> > > > > doesn't have an ACPI companion, in which case we should go straight
> > > > > for the "dev is power manageable" check.
> > > > 
> > > > Moreover, it is possible that the bridge passed to acpi_pci_bridge_d3() has its
> > > > own _S0W or a wakeup GPE if it is power-manageable via ACPI.  In those cases
> > > > it is not necessary to ask the Root Port's _S0W about wakeup from D3, so overall
> > > > I would go for the patch like the below (not really tested).
> > > > 
> > > > This works in the same way as the current code (unless I have missed anything)
> > > > except for the case when the "target" bridge is power-manageable via ACPI, but
> > > > it cannot signal wakeup via ACPI and has no _S0W.  In that case it will consult
> > > > the upstream Root Port's _S0W to check whether or not wakeup from D3 is
> > > > supported.
> > > > 
> > > > [Note that if dev_has_acpi_pm is 'true', it is kind of pointless to look for the
> > > > "HotPlugSupportInD3" property of the Root Port, because the function is going to
> > > > return 'true' regardless, but I'm not sure if adding an extra if () for handling
> > > > this particular case is worth the hassle.]
> > > 
> > > I think this has a lot of potential.  I haven't tried it, but I wonder
> > > if splitting out the Root Port-specific parts to a separate function
> > > would be helpful, if only to make it more obvious that there may be
> > > two different devices involved.
> > > 
> > > If there are two devices ("dev" is a bridge below a Root Port), I
> > > guess support in the Root Port is not necessarily required?  E.g.,
> > > could "dev" assert a wakeup GPE that's not routed through the Root
> > > Port?  If Root Port support *is* required, maybe it would read more
> > > clearly to test that first, before looking at the downstream device.
> > 
> > Sorry for the delay.
> > 
> > I don't really think that Root Port support is required for a bridge below
> > a Root Port if that bridge itself is power-manageable via ACPI.  Moreover,
> > I don't think that the _S0W of a Root Port has any bearing on devices below
> > it that have their own _S0W.
> > 
> > So what we really want appears to be to evaluate _S0W for the target bridge,
> > regardless of whether or not it is a Root Port, and return 'false' if that
> > produces D2 or a shallower power state.  Otherwise, we can do what we've
> > done so far.
> > 
> > The patch below implements, this - please let me know what you think.
> > 
> 
> And here's a v2 with somewhat less code duplication.

I'm wondering if you have any comments on this one?

> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] PCI / ACPI: PM: Take _S0W of the target bridge into account in acpi_pci_bridge_d3(()
> 
> 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.
> 
> Reported-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/device_pm.c |   16 ++++++++++++++++
>  drivers/pci/pci-acpi.c   |   34 ++++++++++++++++++++++++----------
>  include/acpi/acpi_bus.h  |    1 +
>  3 files changed, 41 insertions(+), 10 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-acpi.c
> +++ linux-pm/drivers/pci/pci-acpi.c
> @@ -977,22 +977,37 @@ bool acpi_pci_bridge_d3(struct pci_dev *
>  {
>  	struct pci_dev *rpdev;
>  	struct acpi_device *adev;
> -	acpi_status status;
> -	unsigned long long state;
>  	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_no_wakeup_from_d3(adev))
> +			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 (rpdev != dev)
> +		adev = ACPI_COMPANION(&rpdev->dev);
> +
>  	if (!adev)
>  		return false;
>  
> @@ -1005,11 +1020,10 @@ bool acpi_pci_bridge_d3(struct pci_dev *
>  		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 (rpdev != dev && acpi_dev_no_wakeup_from_d3(adev))
>  		return false;
>  
>  	/*
> 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,22 @@ void acpi_dev_power_up_children_with_adr
>  	acpi_dev_for_each_child(adev, acpi_power_up_if_adr_present, NULL);
>  }
>  
> +/**
> + * acpi_dev_no_wakeup_from_d3 - Check if wakeup signaling from D3 is supported
> + * @adev: ACPI companion of the target device.
> + *
> + * Evaluate _S0W for @adev and return 'true' if it is successful and the power
> + * state returned by it is D2 or shallower.
> + */
> +bool acpi_dev_no_wakeup_from_d3(struct acpi_device *adev)
> +{
> +	unsigned long long state;
> +	acpi_status status;
> +
> +	status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
> +	return ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT;
> +}
> +
>  #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);
> +bool acpi_dev_no_wakeup_from_d3(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,
> 
> 
> 
>
  

On Mon, Jan 02, 2023 at 05:59:36PM +0100, Rafael J. Wysocki wrote:
> On Monday, January 2, 2023 5:34:19 PM CET Rafael J. Wysocki wrote:
> ...

> > I don't really think that Root Port support is required for a bridge below
> > a Root Port if that bridge itself is power-manageable via ACPI.  Moreover,
> > I don't think that the _S0W of a Root Port has any bearing on devices below
> > it that have their own _S0W.
> > 
> > So what we really want appears to be to evaluate _S0W for the target bridge,
> > regardless of whether or not it is a Root Port, and return 'false' if that
> > produces D2 or a shallower power state.  Otherwise, we can do what we've
> > done so far.

> +bool acpi_dev_no_wakeup_from_d3(struct acpi_device *adev)
> +{
> +	unsigned long long state;
> +	acpi_status status;
> +
> +	status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
> +	return ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT;

This returns "false" (i.e., "yes, device can signal wakeup from D3")
if _S0W doesn't exist.  Is that right?

I think this might be less confusing as:

  bool acpi_dev_can_wake_from_d3(struct acpi_device *adev)
  {
    status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
    return ACPI_SUCCESS(status) && state >= ACPI_STATE_D3_HOT;
  }

That would look like this (including minor change to add "rp_adev" to
make it more obviously that it's different from "adev"):


diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 97450f4003cc..789a717d4819 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -484,6 +484,22 @@ void acpi_dev_power_up_children_with_adr(struct acpi_device *adev)
 	acpi_dev_for_each_child(adev, acpi_power_up_if_adr_present, NULL);
 }
 
+/**
+ * acpi_dev_can_wake_from_d3 - Check if wakeup signaling from D3 is supported
+ * @adev: ACPI companion of the target device.
+ *
+ * Evaluate _S0W for @adev and return 'true' if it is successful and the power
+ * state returned by it is D3hot or deeper.
+ */
+bool acpi_dev_can_wake_from_d3(struct acpi_device *adev)
+{
+	unsigned long long state;
+	acpi_status status;
+
+	status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
+	return ACPI_SUCCESS(status) && state >= ACPI_STATE_D3_HOT;
+}
+
 #ifdef CONFIG_PM
 static DEFINE_MUTEX(acpi_pm_notifier_lock);
 static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 068d6745bf98..03dbb57047be 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -976,24 +976,37 @@ bool acpi_pci_power_manageable(struct pci_dev *dev)
 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, *rp_adev;
 	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_can_wake_from_d3(adev))
+			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)
+	rp_adev = (rpdev == dev) ? adev : ACPI_COMPANION(&rpdev->dev);
+	if (!rp_adev)
 		return false;
 
 	/*
@@ -1001,15 +1014,14 @@ bool acpi_pci_bridge_d3(struct pci_dev *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 (!rp_adev->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 (rp_adev != adev && !acpi_dev_can_wake_from_d3(rp_adev))
 		return false;
 
 	/*
@@ -1018,7 +1030,7 @@ bool acpi_pci_bridge_d3(struct pci_dev *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(rp_adev, "HotPlugSupportInD3",
 				   ACPI_TYPE_INTEGER, &obj) &&
 	    obj->integer.value == 1)
 		return true;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index cd3b75e08ec3..0d0c6aa367e0 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -533,6 +533,7 @@ int acpi_bus_update_power(acpi_handle handle, int *state_p);
 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);
+bool acpi_dev_can_wake_from_d3(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,
  

On Tue, Jan 10, 2023 at 9:55 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Mon, Jan 02, 2023 at 05:59:36PM +0100, Rafael J. Wysocki wrote:
> > On Monday, January 2, 2023 5:34:19 PM CET Rafael J. Wysocki wrote:
> > ...
>
> > > I don't really think that Root Port support is required for a bridge below
> > > a Root Port if that bridge itself is power-manageable via ACPI.  Moreover,
> > > I don't think that the _S0W of a Root Port has any bearing on devices below
> > > it that have their own _S0W.
> > >
> > > So what we really want appears to be to evaluate _S0W for the target bridge,
> > > regardless of whether or not it is a Root Port, and return 'false' if that
> > > produces D2 or a shallower power state.  Otherwise, we can do what we've
> > > done so far.
>
> > +bool acpi_dev_no_wakeup_from_d3(struct acpi_device *adev)
> > +{
> > +     unsigned long long state;
> > +     acpi_status status;
> > +
> > +     status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
> > +     return ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT;
>
> This returns "false" (i.e., "yes, device can signal wakeup from D3")
> if _S0W doesn't exist.  Is that right?

Yes, it is.

The reason why I did it that way was because the bridge cannot signal
wakeup from D3 if both the following conditions take place:

1. There is _S0W and it can be evaluated successfully.
2. _S0W produces D2 or a shallower power state.

In particular, if 1 is not the case, it is still not clear whether or
not the bridge can signal wakeup from D3 and additional checks are
needed.

> I think this might be less confusing as:
>
>   bool acpi_dev_can_wake_from_d3(struct acpi_device *adev)
>   {
>     status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
>     return ACPI_SUCCESS(status) && state >= ACPI_STATE_D3_HOT;
>   }

So I don't think the above will work, because
!acpi_dev_can_wake_from_d3(adev) is also true if _S0W is not present,
for example, in which case acpi_pci_bridge_d3() should not return
'false' right away.

However, the additional function can simply return the value produced
by _S0W or ACPI_STATE_UNKNOWN on all errors and its caller can do the
checks as needed which is done here:

https://patchwork.kernel.org/project/linux-acpi/patch/5659681.DvuYhMxLoT@kreacher/

(posted as a proper patch, because I wanted patchwork to pick it up).

I've also picked up the idea of using rpadev for representing the ACPI
companion of the Root Port in acpi_pci_bridge_d3().

Cheers!
  

On Wed, Jan 11, 2023 at 11:56:05AM +0100, Rafael J. Wysocki wrote:
> On Tue, Jan 10, 2023 at 9:55 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Jan 02, 2023 at 05:59:36PM +0100, Rafael J. Wysocki wrote:
> > > On Monday, January 2, 2023 5:34:19 PM CET Rafael J. Wysocki wrote:
> > > ...
> >
> > > > I don't really think that Root Port support is required for a bridge below
> > > > a Root Port if that bridge itself is power-manageable via ACPI.  Moreover,
> > > > I don't think that the _S0W of a Root Port has any bearing on devices below
> > > > it that have their own _S0W.
> > > >
> > > > So what we really want appears to be to evaluate _S0W for the target bridge,
> > > > regardless of whether or not it is a Root Port, and return 'false' if that
> > > > produces D2 or a shallower power state.  Otherwise, we can do what we've
> > > > done so far.
> >
> > > +bool acpi_dev_no_wakeup_from_d3(struct acpi_device *adev)
> > > +{
> > > +     unsigned long long state;
> > > +     acpi_status status;
> > > +
> > > +     status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
> > > +     return ACPI_SUCCESS(status) && state < ACPI_STATE_D3_HOT;
> >
> > This returns "false" (i.e., "yes, device can signal wakeup from D3")
> > if _S0W doesn't exist.  Is that right?
> 
> Yes, it is.
> 
> The reason why I did it that way was because the bridge cannot signal
> wakeup from D3 if both the following conditions take place:
> 
> 1. There is _S0W and it can be evaluated successfully.
> 2. _S0W produces D2 or a shallower power state.
> 
> In particular, if 1 is not the case, it is still not clear whether or
> not the bridge can signal wakeup from D3 and additional checks are
> needed.
> 
> > I think this might be less confusing as:
> >
> >   bool acpi_dev_can_wake_from_d3(struct acpi_device *adev)
> >   {
> >     status = acpi_evaluate_integer(adev->handle, "_S0W", NULL, &state);
> >     return ACPI_SUCCESS(status) && state >= ACPI_STATE_D3_HOT;
> >   }
> 
> So I don't think the above will work, because
> !acpi_dev_can_wake_from_d3(adev) is also true if _S0W is not present,
> for example, in which case acpi_pci_bridge_d3() should not return
> 'false' right away.

OK, that makes sense, thanks!

> However, the additional function can simply return the value produced
> by _S0W or ACPI_STATE_UNKNOWN on all errors and its caller can do the
> checks as needed which is done here:
> 
> https://patchwork.kernel.org/project/linux-acpi/patch/5659681.DvuYhMxLoT@kreacher/
> 
> (posted as a proper patch, because I wanted patchwork to pick it up).
> 
> I've also picked up the idea of using rpadev for representing the ACPI
> companion of the Root Port in acpi_pci_bridge_d3().
> 
> Cheers!