RE: [PATCH] PCI/ACPI: Whitelist hotplug ports for D3 if power managed by ACPI

["Deucher, Alexander" <Alexander.Deucher@amd.com>]

[AMD Public Use]

> -----Original Message-----
> From: Lukas Wunner <lukas@wunner.de>
> Sent: Friday, October 2, 2020 1:10 AM
> To: Bjorn Helgaas <helgaas@kernel.org>; Rafael J. Wysocki
> <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>; linux-pci@vger.kernel.org; linux-
> acpi@vger.kernel.org; amd-gfx@lists.freedesktop.org; Arthur Borsboom
> <arthurborsboom@gmail.com>; matoro <matoro@airmail.cc>; Aaron
> Zakhrov <aaron.zakhrov@gmail.com>; Michal Rostecki
> <mrostecki@suse.com>; Shai Coleman <git@shaicoleman.com>
> Subject: [PATCH] PCI/ACPI: Whitelist hotplug ports for D3 if power managed
> by ACPI
> 
> Recent laptops with dual AMD GPUs fail to suspend the discrete GPU, thus
> causing lockups on system sleep and high power consumption at runtime.
> The discrete GPU would normally be suspended to D3cold by turning off
> ACPI _PR3 Power Resources of the Root Port above the GPU.
> 
> However on affected systems, the Root Port is hotplug-capable and
> pci_bridge_d3_possible() only allows hotplug ports to go to D3 if they belong
> to a Thunderbolt device or if the Root Port possesses a
> "HotPlugSupportInD3" ACPI property.  Neither is the case on affected
> laptops.  The reason for whitelisting only specific, known to work hotplug
> ports for D3 is that there have been reports of SkyLake Xeon-SP systems
> raising Hardware Error NMIs upon suspending their hotplug ports:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Flinux-pci%2F20170503180426.GA4058%40otc-nc-
> 03%2F&amp;data=02%7C01%7Calexander.deucher%40amd.com%7C712c82d
> abd82477e540408d8669172c3%7C3dd8961fe4884e608e11a82d994e183d%7C0
> %7C0%7C637372123238938344&amp;sdata=S1%2ByqWpkuZcilrl7kRXQyodrN
> P66MAjcECRDd7tEjpE%3D&amp;reserved=0
> 
> But if a hotplug port is power manageable by ACPI (as can be detected
> through presence of Power Resources and corresponding _PS0 and _PS3
> methods) then it ought to be safe to suspend it to D3.  To this end, amend
> acpi_pci_bridge_d3() to whitelist such ports for D3.
> 
> Link:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitla
> b.freedesktop.org%2Fdrm%2Famd%2F-
> %2Fissues%2F1222&amp;data=02%7C01%7Calexander.deucher%40amd.com
> %7C712c82dabd82477e540408d8669172c3%7C3dd8961fe4884e608e11a82d99
> 4e183d%7C0%7C0%7C637372123238938344&amp;sdata=k%2FRI8kSJclwhzYuc
> WSjAZNzNEaaU9JgIYNjJsV0dAWo%3D&amp;reserved=0
> Link:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitla
> b.freedesktop.org%2Fdrm%2Famd%2F-
> %2Fissues%2F1252&amp;data=02%7C01%7Calexander.deucher%40amd.com
> %7C712c82dabd82477e540408d8669172c3%7C3dd8961fe4884e608e11a82d99
> 4e183d%7C0%7C0%7C637372123238938344&amp;sdata=JvakgHaQNI6XMaM
> eTHqtPJ5ooZP83lN%2F6wcdHyt53yA%3D&amp;reserved=0
> Link:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitla
> b.freedesktop.org%2Fdrm%2Famd%2F-
> %2Fissues%2F1304&amp;data=02%7C01%7Calexander.deucher%40amd.com
> %7C712c82dabd82477e540408d8669172c3%7C3dd8961fe4884e608e11a82d99
> 4e183d%7C0%7C0%7C637372123238938344&amp;sdata=8clLa%2BCnwH4xtK
> Dc7F9%2FqPBSMFygpjP8EiPgVIz9LiE%3D&amp;reserved=0
> Reported-and-tested-by: Arthur Borsboom <arthurborsboom@gmail.com>
> Reported-and-tested-by: matoro <matoro@airmail.cc>
> Reported-by: Aaron Zakhrov <aaron.zakhrov@gmail.com>
> Reported-by: Michal Rostecki <mrostecki@suse.com>
> Reported-by: Shai Coleman <git@shaicoleman.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks for sorting this out.
Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/pci/pci-acpi.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index
> d5869a0..d9aa551 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -944,6 +944,16 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev)
>  	if (!dev->is_hotplug_bridge)
>  		return false;
> 
> +	/* Assume D3 support if the bridge is power-manageable by ACPI. */
> +	adev = ACPI_COMPANION(&dev->dev);
> +	if (!adev && !pci_dev_is_added(dev)) {
> +		adev = acpi_pci_find_companion(&dev->dev);
> +		ACPI_COMPANION_SET(&dev->dev, adev);
> +	}
> +
> +	if (adev && acpi_device_power_manageable(adev))
> +		return true;
> +
>  	/*
>  	 * Look for a special _DSD property for the root port and if it
>  	 * is set we know the hierarchy behind it supports D3 just fine.
> --
> 2.27.0