Message-ID: <CAJZ5v0j+4xjSOOy8TYE0pBuqd_GdQa683Qq0GEyJ9WAFad9Z=A@mail.gmail.com>
Date: Mon, 16 Dec 2024 18:35:52 +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:11 PM Manivannan Sadhasivam
<manivannan.sadhasivam@...aro.org> wrote:
>
> On Mon, Dec 16, 2024 at 05:24:40PM +0100, Rafael J. Wysocki wrote:
> > On Sat, Dec 14, 2024 at 7:30 AM Manivannan Sadhasivam
> > <manivannan.sadhasivam@...aro.org> wrote:
> > >
> > > On Fri, Dec 13, 2024 at 03:35:15PM +0100, Rafael J. Wysocki wrote:
> > > > On Thu, Dec 12, 2024 at 4:14 PM Christoph Hellwig <hch@....de> wrote:
> > > > >
> > > > > On Thu, Dec 12, 2024 at 01:49:15PM +0100, Ulf Hansson wrote:
> > > > > > Right. This seems to somewhat work for ACPI types of systems, because
> > > > > > ACPI is controlling the low power state for all the devices. Based on
> > > > > > the requested system wide low power state, ACPI can then decide to
> > > > > > call pm_set_suspend_via_firmware() or not.
> > > > > >
> > > > > > Still there is a problem with this for ACPI too.
> > > > > >
> > > > > > How does ACPI know whether it's actually a good idea to keep the NVMe
> > > > > > storage powered in s2idle (ACPI calls pm_set_suspend_via_firmware()
> > > > > > only for S2R and S2disk!?)? Especially when my laptop only supports
> > > > > > s2idle and that's what I will use when I close the lid. In this way,
> > > > > > the NMVe storage will certainly contribute to draining the battery,
> > > > > > especially when I won't be using my laptop for a couple of days.
> > > > > >
> > > > > > In my opinion, we need a better approach that is both flexible and
> > > > > > that dynamically adjusts based upon the use case.
> > > > >
> > > > > Agreed.  I'd be happy to work with the PM maintainers to do this,
> > > > > but I don't really know enough about the PM core to drive it
> > > > > (as the reply from Rafael to my mail makes pretty clear :))
> > > >
> > > > I'm here to help.
> > > >
> > > > Let me know what exactly you want to achieve and we'll see how to make it work.
> > >
> > > I'll try to summarize the requirement here since I started this thread:
> > >
> > > Problem statement
> > > =================
> > >
> > > 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.
> > >
> > > Drivers are relying on couple of options now:
> > >
> > > 1. If pm_suspend_via_firmware() returns true, then drivers will shutdown the
> > > device assuming that the firmware is going to handle the suspend. But this API
> > > is currently used only by ACPI. Even there, ACPI relies on S2R being supported
> > > by the platform and it sets pm_set_suspend_via_firmware() only when the suspend
> > > is S2R. But if the platform doesn't support S2R (current case of most of the
> > > Qcom SoCs), then pm_suspend_via_firmware() will return false and NVMe won't be
> > > powered down draining the battery.
> >
> > So my question here would be why is it not powered down always during
> > system-wide suspend?
> >
> > Why exactly is it necessary to distinguish one case from the other
> > (assuming that we are talking about system-wide suspend only)?
> >
>
> To support Android like usecase with firmware that only supports
> suspend-to-idle (Qcom platforms). This usecase is not applicable right now, but
> one can't just rule out the possibility in the near future.

This doesn't explain anything to me, sorry.

> And the problem is that we need to support both Android and non-Android systems
> with same firmware :/

So what technically is the problem?

> > There are drivers that use pm_suspend_via_firmware() to check whether
> > or not something special needs to be done to the device because if
> > "false" is returned, the platform firmware is not going to remove
> > power from it.
> >
> > However, you seem to be talking about the opposite, so doing something
> > special to the device if "true" is returned.  I'm not sure why this is
> > necessary.
> >
>
> Because, since 'false' is returned, drivers like NVMe are assuming that the
> platform won't remove power on all DT systems and they just keep the devices in
> low power state (not powering them down). But why would I want my NVMe in DT
> based laptop to be always powered in system suspend?

Because it causes the system to use less energy when suspended.

> > > If the platform is using DT, then there is no entity setting
> > > pm_set_suspend_via_firmware().
> >
> > That's true and so the assumption is that in this case the handling of
> > all devices will always be the same regardless of which flavor of
> > system suspend is chosen by user space.
> >
>
> Right and that's why the above concern is raised.

And it is still unclear to me what the problem with it is.

What exactly can go wrong?

> > > So NVMe will be kept in low power state all the
> > > time (still draining the battery).
> >
> > So what would be the problem with powering it down unconditionally?
> >
>
> I'm not sure how would you do that (by checking dev_of_node()?). Even so, it
> will wear out the NVMe devices if used in Android tablets etc...

I understand the wear-out concern.

Is there anything else?

> > > There were attempts to set this flag from
> > > PSCI [1], but there were objections on setting this flag when PSCI_SUSPEND is
> > > not supported by the platform (again, the case with Qcom SoCs). Even if this
> > > approach succeeds, then there are concerns that if the platform is used in an
> > > OS like Android where the S2Idle cycle is far more high, NVMe will wear out
> > > very quickly.
> >
> > I see.
> >
> > > So this is where the forthcoming API need to "dynamically adjusts
> > > based upon the use case" as quoted by Ulf in his previous reply. One way to
> > > achieve would be by giving the flexibility to the userspace to choose the
> > > suspend state (if platform has options to select). UFS does something similar
> > > with 'spm_lvl' [2] sysfs attribute that I believe Android userspace itself makes
> > > use of.
> >
> > Before we're talking about APIs, let's talk about the desired behavior.
> >
> > It looks like there are cases in which you'd want to turn the device
> > off completely (say put it into D3cold in the PCI terminology) and
> > there are cases in which you'd want it to stay in a somewhat-powered
> > low-power state.
> >
> > It is unclear to me what they are at this point.
> >
>
> I hope that my above explanation clarifies.

Sorry, but not really.

> Here is the short version of the suspend requirement across platforms:
>
> 1. D3Cold (power down) - Laptops/Automotive
> 2. D3hot (low power) - Android Tablets

Where do the above requirements come from?

> FWIW, I did receive feedback from people asking to just ignore the Android
> usecase and always power down the devices for DT platforms. But I happen to
> disagree with them. Let me know if I was wrong and I should not worry about
> Android usecase as it is for the future.

I'm not sure what you mean by the "Android usecase" TBH.  Do you mean
the wear-out concern in the Android usage scenario or is there more to
it?

Powered by blists - more mailing lists