PING: Re: [PATCH v2] PCI: don't call firmware hooks on suspend unless it's fw-controlled

[Konstantin Kharlamov <hi-angel@yandex.ru>]

On Fri, 2021-05-21 at 02:55 +0300, Konstantin Kharlamov wrote:
> On Macbook 2013 resuming from s2idle resulted in external monitor no
> longer being detected, and dmesg having errors like:
> 
>     pcieport 0000:06:00.0: can't change power state from D3hot to D0 (config
> space inaccessible)
> 
> and a stacktrace. The reason is that in s2idle (and in S1 as noted by
> Rafael) we do not call firmware code to handle suspend, and as result
> while waking up firmware also does not handle resume.
> 
> This means, for the Thunderbolt controller that gets disabled in the
> quirk by calling the firmware methods, there's no one to wake it back up
> on resume.
> 
> To quote Rafael Wysocki:
> 
> > "Passing control to the platform firmware" means letting
> > some native firmware code (like SMM code) run which happens at the end
> > of S2/S3/S4 suspend transitions and it does not happen during S1
> > (standby) and s2idle suspend transitions.
> > 
> > That's why using SXIO/SXFP/SXLF is only valid during S2/S3/S4 suspend
> > transitions and it is not valid during s2idle and S1 suspend
> > transitions (and yes, S1 is also affected, so s2idle is not special in
> > that respect at all).
> 
> Thus, return early from the quirk when suspend mode isn't one that calls
> firmware.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212767
> Signed-off-by: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/pci/quirks.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 653660e3ba9e..f86b6388a04a 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -27,6 +27,7 @@
>  #include <linux/nvme.h>
>  #include <linux/platform_data/x86/apple.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/suspend.h>
>  #include <linux/switchtec.h>
>  #include <asm/dma.h>   /* isa_dma_bridge_buggy */
>  #include "pci.h"
> @@ -3646,6 +3647,15 @@ static void quirk_apple_poweroff_thunderbolt(struct
> pci_dev *dev)
>                 return;
>         if (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM)
>                 return;
> +
> +       /*
> +        * SXIO/SXFP/SXLF turns off power to the Thunderbolt controller.  We
> don't
> +        * know how to turn it back on again, but firmware does, so we can
> only use
> +        * SXIO/SXFP/SXLF if we're suspending via firmware.
> +        */
> +       if (!pm_suspend_via_firmware())
> +               return;
> +
>         bridge = ACPI_HANDLE(&dev->dev);
>         if (!bridge)
>                 return;