linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Konstantin Kharlamov <hi-angel@yandex.ru>,
	Lukas Wunner <lukas@wunner.de>,
	Linux PCI <linux-pci@vger.kernel.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Andreas Noever <andreas.noever@gmail.com>,
	Linux PM <linux-pm@vger.kernel.org>
Subject: Re: [PATCH] PCI: don't power-off apple thunderbolt controller on s2idle
Date: Fri, 21 May 2021 11:47:25 +0200	[thread overview]
Message-ID: <CAJZ5v0jr459rYezo6qmU+AnwT2ZWLbR0GGoM=NCTiznvpPSfzg@mail.gmail.com> (raw)
In-Reply-To: <20210520194935.GA348608@bjorn-Precision-5520>

On Thu, May 20, 2021 at 9:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, May 20, 2021 at 01:54:05PM +0200, Rafael J. Wysocki wrote:
> > On Thu, May 20, 2021 at 1:27 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > On Wed, May 19, 2021 at 9:48 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Wed, May 19, 2021 at 09:12:26PM +0200, Rafael J. Wysocki wrote:
>
> > > > > The problem is related to the fact that in s2idle the platform
> > > > > firmware does not finalize the suspend transition and, consequently,
> > > > > it doesn't initiate the resume transition.  Therefore whatever power
> > > > > state the device was left in during suspend must be dealt with during
> > > > > the subsequent resume.  Hence, if whatever is done by SXIO/SXFP/SXLF
> > > > > in the suspend path cannot be reversed in the resume path by the
> > > > > kernel (because there is no known method to do that), they should not
> > > > > be invoked.  And that's exactly because the platform firmware will not
> > > > > finalize the suspend transition which is indicated by
> > > > > PM_SUSPEND_FLAG_FW_SUSPEND being unset.
> > > >
> > > > How can we connect "if (!pm_suspend_via_firmware())" in this patch
> > > > with the fact that firmware doesn't finalize suspend (and consequently
> > > > does not reverse things in resume)?
> > > >
> > > > I don't see any use of pm_suspend_via_firmware() or
> > > > PM_SUSPEND_FLAG_FW_SUSPEND that looks relevant.
> > >
> > > First of all, there is a kerneldoc comment next to
> > > pm_suspend_via_firmware() which is relevant.  Especially the last
> > > paragraph of that comment applies directly to the case at hand IMV.
>
> I do read kerneldoc, but I *rely* on the code, and it's nice when I
> can match up the kerneldoc with what the code is doing :)

Fair enough.

> Part of my confusion is that "passing control to platform firmware"
> isn't particularly useful in itself because it doesn't give a clue
> about what firmware is *doing*.  Without knowing what it does, we
> can't reason about how kernel's actions interact with firmware's
> actions.

While we don't know exactly what happens when the platform firmware is
running, we can reasonably expect that devices will get reset as a
result of that.

It's kind of like heating up things in a microwave oven: you don't
need to know exactly what happens when it is working, but nevertheless
you can predict with quite high confidence what the outcome of that
will be.

> > BTW, the problem at hand is not that s2idle in particular needs to be
> > treated in a special way (this appears to be the source of all
> > confusion here).  The problem is that the kernel cannot undo the
> > SXIO/SXFP/SXLF magic without passing control to the platform firmware.
>
> I assume this is really a case of "the kernel doesn't know *what* to
> do, but platform firmware does," so in principle the kernel *could*
> undo the SXIO/SXFP/SXLF magic if it knew what to do.

In general, that may or may not be the case.

I guess it is the case here, because Lukas seems to know how to make
this work, but the AML in question might be prepared with the
assumption that the firmware code finalizing the transition will run
after it.

> > And "passing control to the platform firmware" doesn't mean "executing
> > some AML" here, because control remains in the kernel when AML is
> > executed.  "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).
> >
> > IMO the changelog of the patch needs to be rewritten, but the code
> > change made by it is reasonable.
>
> So IIUC the comment should say something like:
>
>   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.
>
> Actually, it sounds like the important thing is that we rely on the
> firmware *resume* path to turn on the power again.

Actually both the suspend and resume paths in it together, but the
important piece is that if the firmware runs at the end of suspend, it
will also run at the beginning of resume.

> pm_resume_via_firmware() *sounds* like it would be appropriate, but
> the kerneldoc says that's for use after resume,

That's right.

The rule of thumb is to use pm_suspend_via_firmware() in the
system-wide suspend code paths and pm_resume_via_firmware() in the
resume ones.  They are simply complementary.

And because in this particular case a decision needs to be made
whether or not to do something in a suspend path, the former is the
right one to use.

> and it tells us whether firmware has *already* handled the wakeup event.  And
> PM_SUSPEND_FLAG_FW_RESUME isn't set until after we've run these
> suspend_late fixups, so it wouldn't work here.

Right.

  parent reply	other threads:[~2021-05-21  9:47 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
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                     ` Rafael J. Wysocki [this message]
2021-05-07 15:02       ` [PATCH] PCI: don't power-off apple thunderbolt controller on s2idle 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='CAJZ5v0jr459rYezo6qmU+AnwT2ZWLbR0GGoM=NCTiznvpPSfzg@mail.gmail.com' \
    --to=rafael@kernel.org \
    --cc=andreas.noever@gmail.com \
    --cc=helgaas@kernel.org \
    --cc=hi-angel@yandex.ru \
    --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).