Re: [PATCH] nvme-pci: Use non-operational power state instead of D3 on Suspend-to-Idle

From: Kai-Heng Feng
Date: Thu May 09 2019 - 05:43:49 EST


at 17:07, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:

On Thu, May 9, 2019 at 8:49 AM Kai-Heng Feng
<kai.heng.feng@xxxxxxxxxxxxx> wrote:
Cc Rafael and linux-pm

I would have been much more useful to CC the patch to linux-pm at
least from the outset.

at 14:12, Christoph Hellwig <hch@xxxxxx> wrote:

On Wed, May 08, 2019 at 08:28:30PM +0000, Mario.Limonciello@xxxxxxxx wrote:
You might think this would be adding runtime_suspend/runtime_resume
callbacks, but those also get called actually at runtime which is not
the goal here. At runtime, these types of disks should rely on APST which
should calculate the appropriate latencies around the different power
states.

This code path is only applicable in the suspend to idle state, which
/does/
call suspend/resume functions associated with dev_pm_ops. There isn't
a dedicated function in there for use only in suspend to idle, which is
why pm_suspend_via_s2idle() needs to get called.

The problem is that it also gets called for others paths:

#ifdef CONFIG_PM_SLEEP
#define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
.suspend = suspend_fn, \
.resume = resume_fn, \
.freeze = suspend_fn, \
.thaw = resume_fn, \
.poweroff = suspend_fn, \
.restore = resume_fn,
#else
else
#define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
#endif

#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
const struct dev_pm_ops name = { \
SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
}

And at least for poweroff this new code seems completely wrong, even
for freeze it looks rather borderline.

Not really, for hibernation pm_suspend_via_s2idle() evaluates to false so
the old code path will be taken.

And more to the points - if these "modern MS standby" systems are
becoming common, which it looks they are, we need support in the PM core
for those instead of working around the decisions in low-level drivers.

Rafael, what do you think about this?

The difference between suspend-to-idle and suspend-to-RAM (S3) boils
down to the fact that at the end of S3 suspend all control of the
system is passed to the platform firmware. Then, the firmware can
take care of some things that may not be taken care of by drivers (it
sometimes assumes that drivers will not take care of those things even
which is generally problematic).

For suspend-to-idle the final physical state of the system should (in
theory) be the same as the deepest possible physical idle state of it
achievable through working-state PM (combination of PM-runtime and
cpuidle, roughly speaking). However, in practice the working-state PM
may not even be able to get there, as it generally requires many
things to happen exactly at the right time in a specific order and the
probability of that in the working-state PM situation is practically
0. Suspend-to-idle helps here by quiescing everything in an ordered
fashion which makes all of the requisite conditions more likely to be
met together.

So yes, from an individual driver perspective, the device handling for
s2idle should be more like for PM-runtime than for S3 (s2R), but this
really shouldn't matter (and it doesn't matter for the vast majority
of drivers).

Unfortunately, the "modern MS standby" concept makes it matter,
because "modern MS standby" causes system-wide transitions to be
"special" and it appears to expect drivers to take care of the "extra
bit" that would have been taken care of by the platform firmware in
the S3 case. [Note that in the Windows world the "modern MS standby"
systems don't support S3 ("modern MS standby" and S3 support are
mutually exclusive in Windows AFAICS) while Linux needs to support S3
and is expected to achieve the minimum power state through s2idle
(generally, even on the same platform) at the same time.]

Including this patch, there are five drivers that use
pm_suspend_via_{firmware,s2idle}() to differentiate between S2I and S3.

Well, that is not a large number relative to the total number of
drivers in Linux.

Thatâs right, but I think we are going to see more of similar cases.


So I think maybe itâs time to introduce a new suspend callback for S2I?

That would be a set of 6 new suspend and resume callbacks, mind you,
and there's quite a few of them already. And the majority of drivers
would not need to use them anyway.

I think suspend_to_idle() and resume_from_idle() should be enough?
What are other 4 callbacks?


Also, please note that, possibly apart from the device power state
setting, the S2I and S2R handling really aren't that different at all.
You basically need to carry out the same preparations during suspend
and reverse them during resume in both cases.

But for this case, itâs quite different to the original suspend and resume callbacks.


That said I admit that there are cases in which device drivers need to
know that the system-wide transition under way is into s2idle and so
they should do extra stuff. If pm_suspend_via_firmware() is not
sufficient for that, then I'm open to other suggestions, but
introducing a new set of callbacks for that alone would be rather
excessive IMO.

From driversâ perspective nothing changed, as PM core can prioritize suspend_to_idle() over suspend() when itâs actually S2I.


SIMPLE_DEV_PM_OPS normally sets the same function for suspend and
freeze (hibernate), so to avoid any changes to the hibernate case it seems
to me that there needs to be a new nvme_freeze() that calls into the
existing
nvme_dev_disable for the freeze pm op and nvme_thaw() that calls into the
existing nvme_reset_ctrl for the thaw pm op.

At least, yes.

Hibernation should remain the same as stated above.

Depending on what check is used in that code path.
pm_suspend_via_s2idle() will return "true" in the hibernation path
too, for one.

You are right, I should use !pm_suspend_via_firmware() instead.


enterprise class NVMe devices
that don't do APST and don't really do different power states at
all in many cases.

Enterprise class NVMe devices that don't do APST - do they typically
have a non-zero value for ndev->ctrl.npss?

If not, they wouldn't enter this new codepath even if the server entered
into S2I.

No, devices that do set NPSS will have at least some power states
per definition, although they might not be too useful. I suspect checking
APSTA might be safer, but if we don't want to rely on APST we should
check for a power state supporting the condition that the MS document
quoted in the original document supports.

If Modern Standby or Connected Standby is not supported by servers, I donât
think the design documents mean much here.
We probably should check if the platform firmware really supports S2I
instead.

S2I is expected to work regardless of the platform firmware and there
is nothing like "platform firmware support for S2I". IOW, that check
would always return "false".

What you really need to know is if the given particular transition is S2I.

Maybe a helper based on FADT flag and _DSM can do this thing?

Kai-Heng