linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Konstantin Kharlamov <hi-angel@yandex.ru>
To: Bjorn Helgaas <helgaas@kernel.org>, Lukas Wunner <lukas@wunner.de>
Cc: linux-pci@vger.kernel.org,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Andreas Noever <andreas.noever@gmail.com>,
	linux-pm@vger.kernel.org
Subject: Re: [PATCH] PCI: don't power-off apple thunderbolt controller on s2idle
Date: Fri, 07 May 2021 17:08:42 +0300	[thread overview]
Message-ID: <8ddea02fc6d37f7c444a1e90c9f03d7656ffe957.camel@yandex.ru> (raw)
In-Reply-To: <20210507133002.GA1499665@bjorn-Precision-5520>

On Fri, 2021-05-07 at 08:30 -0500, Bjorn Helgaas wrote:
> On Fri, May 07, 2021 at 12:07:38AM +0200, Lukas Wunner wrote:
> > On Thu, May 06, 2021 at 04:48:42PM -0500, Bjorn Helgaas wrote:
> > > 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.
> > 
> > With s2idle, the machine isn't suspended via ACPI, so the AML code
> > which powers the controller off isn't executed.  The dance to prepare
> > the controller for power-off consequently isn't necessary but rather
> > harmful.
> > 
> > To get the same power savings as with ACPI suspend, the controller
> > needs to be powered off via runtime suspend.  I posted patches for
> > that back in 2016.  I'm using them on my laptop, they need some
> > polishing and rebasing before I can repost them due to massive
> > changes that have happened in the thunderbolt driver in the meantime.
> > Without these patches, the controller sucks 1.5W of power in 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.
> > 
> > Yes I believe the quirk predates the introduction of s2idle by a couple
> > of years.
> 
> In an ideal world, we would know which commit introduced s2idle and
> hence the possibility of hitting this bug, and we would add a Fixes:
> tag for that commit so we could connect this fix with it.
> 
> Apart from that, what I don't like about this (and about the original
> 1df5172c5c25) is that there's no connection to a spec or to documented
> behavior of the device or of suspend/resume.

I did some research, and found that s2idle was first introduced in 2013 in
commit 7e73c5ae6e799 (except it wasn't called "s2idle", by that name it goes
since around 2016 as Lukas mentioned. In 7e73c5ae6e799 it is called "freeze").
This is before 1df5172c5c25 which was added in 2014, so I guess we can add a:

	Fixes: 1df5172c5c25 ("PCI: Suspend/resume quirks for Apple
thunderbolt")

> For example, "With s2idle, the machine isn't suspended via ACPI, so
> the AML code which powers the controller off isn't executed."  AFAICT
> that isn't actually a required, documented property of s2idle, but
> rather it reaches into the internal implementation.
> 
> The code comment "If suspend mode is s2idle, power won't get restored
> on resume" is similar.  !pm_suspend_via_firmware() tells us that
> platform firmware won't be invoked.  But the connection between *that*
> and "power won't get restored" is unexplained.

Sorry, I can't comment anything regarding AML and power management in general since I am really new to all of this. However, regarding the usage of the `pm_suspend_via_firmware()`: yeah, I also think it is unclear what this does, and I was thinking about adding a wrapper function something like `is_s2idle()` to the suspend.h, which would simply call `pm_suspend_via_firmware` internally. I didn't do that because I thought that usage of pm_suspend_via_firmware() for that task is just something people working with power management are supposed to know, but if someone else questions it too, I can make such wrapper, it's just a 3 lines-of-code change.

FWIW, originally I found out that pm_suspend_via_firmware() can be used for detecting s2idle by simply asking about it on linux-pm: https://marc.info/?l=linux-pm&m=162029296108775&w=2


  reply	other threads:[~2021-05-07 14:08 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-06 17:38 [PATCH] PCI: don't power-off apple thunderbolt controller on s2idle Konstantin Kharlamov
2021-05-06 21:48 ` Bjorn Helgaas
2021-05-06 22:07   ` Lukas Wunner
2021-05-07  9:51     ` Rafael J. Wysocki
2021-05-08  8:48       ` Lukas Wunner
2021-05-07 13:30     ` Bjorn Helgaas
2021-05-07 14:08       ` Konstantin Kharlamov [this message]
2021-05-12 20:36         ` Konstantin Kharlamov
2021-05-17 19:51           ` PING " Konstantin Kharlamov
2021-05-19 17:28         ` Bjorn Helgaas
2021-05-19 19:12           ` Rafael J. Wysocki
2021-05-19 19:48             ` Bjorn Helgaas
2021-05-20 11:27               ` Rafael J. Wysocki
2021-05-20 11:54                 ` Rafael J. Wysocki
2021-05-20 19:49                   ` Bjorn Helgaas
2021-05-20 23:28                     ` Konstantin Kharlamov
2021-05-24  6:59                       ` Konstantin Kharlamov
2021-05-20 23:55                     ` [PATCH v2] PCI: don't call firmware hooks on suspend unless it's fw-controlled Konstantin Kharlamov
2021-05-28  7:39                       ` PING: " Konstantin Kharlamov
2021-06-03  8:36                         ` PING: " Konstantin Kharlamov
2021-06-03 17:46                           ` Bjorn Helgaas
2021-06-04  8:30                             ` Konstantin Kharlamov
2021-05-21  9:47                     ` [PATCH] PCI: don't power-off apple thunderbolt controller on s2idle Rafael J. Wysocki
2021-05-07 15:02       ` Rafael J. Wysocki
2021-05-08  8:20       ` Lukas Wunner
2021-05-07  9:32   ` Konstantin Kharlamov
2021-05-07 13:07     ` Bjorn Helgaas
2021-05-07 13:48       ` Konstantin Kharlamov
2021-05-20 11:58   ` Rafael J. Wysocki
2021-06-07 23:17 ` Bjorn Helgaas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8ddea02fc6d37f7c444a1e90c9f03d7656ffe957.camel@yandex.ru \
    --to=hi-angel@yandex.ru \
    --cc=andreas.noever@gmail.com \
    --cc=helgaas@kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=lukas@wunner.de \
    --cc=rjw@rjwysocki.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).