linux-kernel - Re: [PATCH] nvme-pci: Shutdown the device if D3Cold is allowed by the user

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: <CAJZ5v0hhruAS_b739FTo_63=h=U5zbH5M5jWco8pq+KwVY5UcA@mail.gmail.com>
Date: Mon, 16 Dec 2024 20:10:09 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Christoph Hellwig <hch@....de>, Ulf Hansson <ulf.hansson@...aro.org>, 
	"Rafael J. Wysocki" <rjw@...ysocki.net>, Bjorn Helgaas <helgaas@...nel.org>, kbusch@...nel.org, 
	axboe@...nel.dk, sagi@...mberg.me, linux-nvme@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org, andersson@...nel.org, 
	konradybcio@...nel.org, Len Brown <len.brown@...el.com>, linux-pm@...r.kernel.org
Subject: Re: [PATCH] nvme-pci: Shutdown the device if D3Cold is allowed by the user

On Mon, Dec 16, 2024 at 6:39 PM Manivannan Sadhasivam
<manivannan.sadhasivam@...aro.org> wrote:
>
> On Mon, Dec 16, 2024 at 06:28:55PM +0100, Rafael J. Wysocki wrote:
> > On Mon, Dec 16, 2024 at 5:48 PM Manivannan Sadhasivam
> > <manivannan.sadhasivam@...aro.org> wrote:
> > >
> > > On Mon, Dec 16, 2024 at 05:42:30PM +0100, Rafael J. Wysocki wrote:
> > > > On Mon, Dec 16, 2024 at 5:23 PM Christoph Hellwig <hch@....de> wrote:
> > > > >
> > > > > On Sat, Dec 14, 2024 at 12:00:23PM +0530, Manivannan Sadhasivam wrote:
> > > > > > We need a PM core API that tells the device drivers when it is safe to powerdown
> > > > > > the devices. The usecase here is with PCIe based NVMe devices but the problem is
> > > > > > applicable to other devices as well.
> > > > >
> > > > > Maybe I'm misunderstanding things, but I think the important part is
> > > > > to indicate when a suspend actually MUST put the device into D3.  Because
> > > > > doing that should always be safe, but not always optimal.
> > > >
> > > > I'm not aware of any cases when a device must be put into D3cold
> > > > (which I think is what you mean) during system-wide suspend.
> > > >
> > > > Suspend-to-idle on x86 doesn't require this, at least not for
> > > > correctness.  I don't think any platforms using DT require it either.
> > > >
> > >
> > > On suspend-to-idle, yes D3Cold doesn't make sense,
> >
> > Why?
> >
>
> Because there is no requirement to remove power during S2Idle, isn't it?

There is no requirement, but there is a reason that I've already
mentioned: A device in D3cold causes a system in s2idle use less
energy.

I think this reason is good enough.

> From Documentation/admin-guide/pm/sleep-states.rst:
>
> 'This is a generic, pure software, light-weight variant of system suspend'.

Sure.  Which basically means that platform firmware is not involved
(at least not as much as in the ACPI S3 case) and CPUs are managed in
a more lightweight way.

The power states of devices are what they are.

Moreover, the whole idea of suspend-to-idle is to re-use the
suspend-to-RAM (ACPI S3, basically) control flow, along with the
handling of devices.  The devices handled differently are exceptions,
not a rule.

> > > but on suspend-to-ram it is pretty much required.
> >
> > Well, I know for a fact that on x86 platforms ACPI S3 does not require
> > putting devices into D3cold in general.
> >
> > Why is it required for NVMe?
> >
>
> But ACPI code currently calls pm_set_suspend_via_firmware() for S3 suspend. And
> that causes NVMe to be powered down because of pm_suspend_via_firmware() check.

That's how the driver is designed, but is it actually required to be
designed this way?

> > > That applies to DT as well.
> >
> > Again, why?
>
> On DT systems if firmware supports both S2Idle and S2R, devices can be kept in
> low power state during S2Idle and powered down during S2R.
>
> The problem comes if the firmware only supports the former state.

I don't get it, sorry.

Firmware support for suspend-to-idle is not required, at least in
principle, so I'm not sure what you mean by firmware support for it.
I'm also not sure how S2R is supported by firmware on DT systems, so
care to explain?

In any case, I don't see why in principle devices need to be handled
differently depending on what flavor of system suspend in used at any
given time.