Re: [PATCH] PCI: don't power-off apple thunderbolt controller on s2idle

["Rafael J. Wysocki" <rafael@kernel.org>]

On Thu, May 6, 2021 at 11:48 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Rafael, Andreas, linux-pm]
>
> On Thu, May 06, 2021 at 08:38:20PM +0300, Konstantin Kharlamov wrote:
> > On Macbook 2013 resuming from s2idle results 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 turned out that the hw that the quirk
> > powers off does not get powered on back on resume.
>
> quirk_apple_poweroff_thunderbolt() was added in 2014 by 1df5172c5c25
> ("PCI: Suspend/resume quirks for Apple thunderbolt").  It claims
> "power is automatically restored before resume," so there must be
> something special about s2idle that prevents the power-on.
>
> IIUC this change will reduce the s2idle power savings.  I would feel
> better about this if we understood what the difference was.
>
> > Thus, add a check for s2idle to the quirk, and do nothing if the suspend
> > mode is s2idle.
>
> Obviously the *hardware* hasn't changed since 1df5172c5c25.  Is s2idle
> something that wasn't tested back then, or is this problem connected
> to an s2idle change since then?  Can we identify a commit that
> introduced this problem?  That would help with backporting or stable
> tags.
>
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212767
>
> Thanks for this!  Would you mind attaching the output of
> "sudo lspci -vvv"?  If you attach any other dmesg, could you
> use "dmesg --color=never" so the log doesn't include all the
> escape characters?
>
> > Signed-off-by: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> > ---
> >  drivers/pci/quirks.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 653660e3ba9e..86fedcec37e2 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,13 @@ static void quirk_apple_poweroff_thunderbolt(struct pci_dev *dev)
> >               return;
> >       if (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM)
> >               return;
> > +
> > +     /*
> > +      * If suspend mode is s2idle, power won't get restored on resume.
> > +      */

The comment above is incorrect.

The reason to return here is because whatever is done below cannot be
undone unless the platform firmware is invoked to finalize the suspend
transition, which is what the check is about.  And this is not
specific to s2idle.

> > +     if (!pm_suspend_via_firmware())
> > +             return;
> > +
> >       bridge = ACPI_HANDLE(&dev->dev);
> >       if (!bridge)
> >               return;
> > --
> > 2.31.1
> >