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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <064701C3-2BD4-4D93-891D-B7FBB5040FC4@canonical.com>
Date:   Thu, 9 May 2019 14:48:59 +0800
From:   Kai-Heng Feng <kai.heng.feng@...onical.com>
To:     Christoph Hellwig <hch@....de>, rafael.j.wysocki@...el.com
Cc:     Mario.Limonciello@...l.com, Keith Busch <kbusch@...nel.org>,
        Keith Busch <keith.busch@...el.com>, Jens Axboe <axboe@...com>,
        Sagi Grimberg <sagi@...mberg.me>,
        linux-nvme <linux-nvme@...ts.infradead.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] nvme-pci: Use non-operational power state instead of D3
 on Suspend-to-Idle

Cc Rafael and linux-pm

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

> On Wed, May 08, 2019 at 08:28:30PM +0000, Mario.Limonciello@...l.com 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?
Including this patch, there are five drivers that use  
pm_suspend_via_{firmware,s2idle}() to differentiate between S2I and S3.
So I think maybe it’s time to introduce a new suspend callback for 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.

>
>>> 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.

Kai-Heng