linux-kernel - Re: [PATCH v15 2/2] platform/x86/amd: pmc: Don't let PCIe root ports go into D3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <0ebd3eb9-7629-4897-b1c4-01574145d02d@amd.com>
Date:   Mon, 28 Aug 2023 10:15:06 -0500
From:   Mario Limonciello <mario.limonciello@....com>
To:     Hans de Goede <hdegoede@...hat.com>, Shyam-sundar.S-k@....com,
        bhelgaas@...gle.com
Cc:     platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
        iain@...ngesquash.org.uk
Subject: Re: [PATCH v15 2/2] platform/x86/amd: pmc: Don't let PCIe root ports
 go into D3

On 8/28/2023 04:19, Hans de Goede wrote:
> 
> 
> I'm fine with moving this into the amd-pmc code, but I have some questions about the current approach:
> 
> 1. The current approach sets pci_dev->bridge_d3 = 0 for all root ports, I assume this WA is indeed necessary for all root ports and not just for one specific root port ?

For the issue reported, yes it's only needed for two specific root ports 
that the USB4 controllers are connected to.

The reason that I applied it to all root ports is that this is what 
Windows does over Modern Standby on the same hardware.

For stuff like this I generally prefer to be "bug compatible" with 
Windows if we can.

I have another idea here in using the new 'acpi_get_lps0_constraint' 
symbol in the context of the workaround to figure out which root ports 
to apply it to.

This is something that I toyed with in earlier versions of the series 
generally but there was concerns for regressions in other hardware.  It 
might work well in this really narrow context.

Windows uses the constraints to decide which devices *to put into D3*. 
We might be able to use them in reverse for the PCIe root ports.

> 
> 2. The current approach runs from the suspend pm-op for the PCI-device for the PMC. So when it runs we know that the root-port for the PMC will not have been suspended yet. But what is stopping other root ports, which already have had all their children run-time suspended before the system-suspend, from already being in suspended state and thus possibly in D3 state ?
> 

That's a good point.

> And we also cannot just set pci_dev->bridge_d3 = 0 once on probe time since pci_bridge_d3_possible() is called every time pci devices are added/removed so then it may get reset to 1 again.
> 

Yeah; that was my finding too when I was putting this approach together.

> What I think is necessary here and what I hope will be acceptable to Bjorn, is for platform code to be able to register a callback to be called from pci_bridge_d3_possible() which can veto the decision to use d3. This way we don't pollute the PCI core with this, while still allowing platform specific tweaks.
> 
> If we make this a sorted list of callbacks (allowing to specify a priority at register time)
> instead of just 1 callback the the 2015 BIOS date check could be move to arch/x86 and the DMI blacklist can probably also be moved there.

Based on the conversations that have transpired so far on various 
versions of this in PCI core I don't think Bjorn will want to move the 
2015 BIOS date check to somewhere X86 specific in fear of regressions 
for non-X86.

> 
> And the platform_pci_bridge_d3() check can then also be a callback registered by the ACPI code.
> 

Sure, let's see what Bjorn thinks of this idea of yours.

> Regards,
> 
> Hans
> 
>