Re: [PATCH v16 0/3] Avoid PCIe D3 for AMD PCIe root ports

[Hans de Goede]

Hi Mario,

On 8/29/23 19:12, Mario Limonciello wrote:
> D3 on PCIe root ports isn't used on Windows systems in Modern Standby.
> This series adjusts the amd-pmc driver to choose the same strategy
> for Rembrandt and Phoenix platforms in Linux with s2idle.
> 
> LPS0 constraints are the basis for it; which if they are added for
> Windows would also apply for Linux as well.
> 
> This version doesn't incorporate a callback, as it's pending feedback
> from Bjorn if that approach is amenable.
> 
> NOTE:
> This series relies upon changes that are both in linux-pm.git and
> platform-x86.git. So it won't be able to apply to either maintainer's
> tree until later.
> 
> Mario Limonciello (3):
>   ACPI: x86: s2idle: Export symbol for fetching constraints for module
>     use
>   platform/x86/amd: pmc: Adjust workarounds to be part of a switch/case
>   platform/x86/amd: pmc: Don't let PCIe root ports go into D3

Thank you for the new version.

I understand you wanted to get this new approach "out there" but
this does not address my remarks on v15:

https://lore.kernel.org/platform-driver-x86/53d26a63-64f3-e736-99f5-32bf4b5ba31d@xxxxxxxxxx/

Bjorn, I suggest to allow platform code to register a callback
to influence pci_bridge_d3_possible() results there. Can you
take a look at this and let us know what you think of this
suggestion ?

Looking at this problem again and rereading the commit message
of "platform/x86/amd: pmc: Don't let PCIe root ports go into D3"

I see that the problem is that the PCIe root ports to which
the USB controllers connect should not be allowed to go
into D3 when an USB child of them is configured to wakeup
the system.

It seems to me that given that problem description,
we should not be directly messing with the bridge_d3
setting at all.

Instead the XHCI code should have an AMD specific quirk
where it either unconditionally calls pci_d3cold_disable()
on the XHCI PCIe device; or it could even try to be smart
and call pci_d3cold_enable() / pci_d3cold_disable()
from its (runtime)suspend handler depending on if any
USB child is configured as a system wakeup source.

Note that it is safe to repeatedly call pci_d3cold_enable()
/ _disable() there is no need to balance the calls.

Regards,

Hans