Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-15 Thread Rafael J. Wysocki
On Wed, May 15, 2019 at 12:21 AM Keith Busch wrote: > > On Tue, May 14, 2019 at 10:04:22AM +0200, Rafael J. Wysocki wrote: > > On Mon, May 13, 2019 at 5:10 PM Keith Busch wrote: > > > > > > On Mon, May 13, 2019 at 03:05:42PM +, mario.limoncie...@dell.com > > > wrote: > > > > This system powe

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-14 Thread Keith Busch
On Tue, May 14, 2019 at 10:04:22AM +0200, Rafael J. Wysocki wrote: > On Mon, May 13, 2019 at 5:10 PM Keith Busch wrote: > > > > On Mon, May 13, 2019 at 03:05:42PM +, mario.limoncie...@dell.com wrote: > > > This system power state - suspend to idle is going to freeze threads. > > > But we're ta

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-14 Thread Rafael J. Wysocki
On Mon, May 13, 2019 at 5:10 PM Keith Busch wrote: > > On Mon, May 13, 2019 at 03:05:42PM +, mario.limoncie...@dell.com wrote: > > This system power state - suspend to idle is going to freeze threads. > > But we're talking a multi threaded kernel. Can't there be a timing problem > > going >

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Christoph Hellwig
On Mon, May 13, 2019 at 06:01:39PM +, mario.limoncie...@dell.com wrote: > When using HMB the SSD will be writing to some memory mapped region. > Writing to > that region would use DMA to access host memory, no? Memory mapped region? It will use the devices DMA engine to write host memory, whi

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Keith Busch
On Tue, May 14, 2019 at 01:16:22AM +0800, Kai-Heng Feng wrote: > Disabling HMB prior suspend makes my original patch work without memory > barrier. > > However, using the same trick on this patch still freezes the system during > S2I. Could you post your code, please?

RE: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Mario.Limonciello
r.kernel.org; > linux...@vger.kernel.org; kai.heng.f...@canonical.com > Subject: Re: [PATCH] nvme/pci: Use host managed power state for suspend > > > [EXTERNAL EMAIL] > > On Mon, May 13, 2019 at 02:54:49PM +, mario.limoncie...@dell.com wrote: > > The Intel DMA controlle

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Kai-Heng Feng
at 23:16, Keith Busch wrote: On Mon, May 13, 2019 at 04:57:08PM +0200, Christoph Hellwig wrote: On Mon, May 13, 2019 at 02:54:49PM +, mario.limoncie...@dell.com wrote: And NVME spec made it sound to me that while in a low power state it shouldn't be available if the memory isn't availa

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Keith Busch
On Mon, May 13, 2019 at 04:57:08PM +0200, Christoph Hellwig wrote: > On Mon, May 13, 2019 at 02:54:49PM +, mario.limoncie...@dell.com wrote: > > And NVME spec made it sound to me that while in a low power state it > > shouldn't > > be available if the memory isn't available. > > > > NVME spec

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Keith Busch
On Mon, May 13, 2019 at 03:05:42PM +, mario.limoncie...@dell.com wrote: > This system power state - suspend to idle is going to freeze threads. > But we're talking a multi threaded kernel. Can't there be a timing problem > going > on then too? With a disk flush being active in one task and t

RE: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Mario.Limonciello
gt; p...@vger.kernel.org; kai.heng.f...@canonical.com > Subject: Re: [PATCH] nvme/pci: Use host managed power state for suspend > > > [EXTERNAL EMAIL] > > On Mon, May 13, 2019 at 02:43:43PM +, mario.limoncie...@dell.com wrote: > > > Well, it sounds like your partners device d

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Keith Busch
On Mon, May 13, 2019 at 02:43:43PM +, mario.limoncie...@dell.com wrote: > > Well, it sounds like your partners device does not work properly in this > > case. There is nothing in the NVMe spec that says queues should be > > torn down for deep power states, and that whole idea seems rather > >

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Christoph Hellwig
On Mon, May 13, 2019 at 02:54:49PM +, mario.limoncie...@dell.com wrote: > The Intel DMA controller suspend callbacks in drivers/dma/idma64.c look to me > to > turn off the controller. How is that relevant? That thing is neither a NVMe controller, nor even an PCIe device.. > And NVME spec ma

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Christoph Hellwig
On Mon, May 13, 2019 at 02:43:43PM +, mario.limoncie...@dell.com wrote: > Well I've got a thought, quoting the NVME spec: > "After a successful completion of a Set Features command for this feature, > the controller shall be in the > Power State specified. If enabled, autonomous power state tr

RE: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Mario.Limonciello
gt; p...@vger.kernel.org; kai.heng.f...@canonical.com > Subject: Re: [PATCH] nvme/pci: Use host managed power state for suspend > > > [EXTERNAL EMAIL] > > On Mon, May 13, 2019 at 02:24:41PM +, mario.limoncie...@dell.com wrote: > > This was not a disk with HMB, but with

RE: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Mario.Limonciello
gt; p...@vger.kernel.org; kai.heng.f...@canonical.com > Subject: Re: [PATCH] nvme/pci: Use host managed power state for suspend > > > [EXTERNAL EMAIL] > > On Mon, May 13, 2019 at 02:24:41PM +, mario.limoncie...@dell.com wrote: > > I've received the result that from on

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Keith Busch
On Mon, May 13, 2019 at 02:24:41PM +, mario.limoncie...@dell.com wrote: > This was not a disk with HMB, but with regard to the HMB I believe it needs > to be > removed during s0ix so that there isn't any mistake that SSD thinks it can > access HMB > memory in s0ix. Is that really the case, t

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Christoph Hellwig
On Mon, May 13, 2019 at 02:24:41PM +, mario.limoncie...@dell.com wrote: > I've received the result that from one of my partners this patch doesn't > work properly and the platform doesn't go into a lower power state. Well, it sounds like your partners device does not work properly in this case

RE: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Mario.Limonciello
> > Cc: Mario Limonciello > > Cc: Kai Heng Feng > > Signed-off-by: Keith Busch > > --- > > Disclaimer: I've tested only on emulation faking support for the feature. > > Thanks for sharing. I'll arrange some testing with this with storage > partners early > next week. > I've received the res

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Keith Busch
On Sat, May 11, 2019 at 11:06:35PM -0700, Chaitanya Kulkarni wrote: > On 5/10/19 2:35 PM, Keith Busch wrote: > > > > +int nvme_set_power(struct nvme_ctrl *ctrl, unsigned ps) > dev->ctrl.npss is u8 can we use same data type here ? > If this is due to last_ps we use as a result and then call set_

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-13 Thread Keith Busch
On Sat, May 11, 2019 at 12:22:58AM -0700, Christoph Hellwig wrote: > A couple nitpicks, mostly leftover from the previous iteration > (I didn't see replies to those comments from you, despite seeing > a reply to my mail, assuming it didn't get lost): I thought you just meant the freeze/unfreeze se

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-12 Thread Chaitanya Kulkarni
Disregard my comment. On 5/12/19 7:31 AM, Minwoo Im wrote: >>> + union nvme_result res; >>> + int ret; >>> + >>> + if (!result) >>> + return -EINVAL; >>> + >>> + memset(&c, 0, sizeof(c)); >>> + c.features.opcode = nvme_admin_get_features; >>> + c.features.fid = cpu_to_le32(NV

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-12 Thread Sagi Grimberg
+int nvme_set_power(struct nvme_ctrl *ctrl, unsigned ps) +{ + return nvme_set_features(ctrl, NVME_FEAT_POWER_MGMT, ps, NULL, 0, NULL); +} +EXPORT_SYMBOL_GPL(nvme_set_power); + +int nvme_get_power(struct nvme_ctrl *ctrl, u32 *result) +{ + struct nvme_command c; + union nvme_res

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-12 Thread Minwoo Im
> > + union nvme_result res; > > + int ret; > > + > > + if (!result) > > + return -EINVAL; > > + > > + memset(&c, 0, sizeof(c)); > > + c.features.opcode = nvme_admin_get_features; > > + c.features.fid = cpu_to_le32(NVME_FEAT_POWER_MGMT); > > + > > + ret = __nvme_submit_sync_

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-11 Thread Chaitanya Kulkarni
Hi Keith, Thanks for the patch, few comments below mostly nitpicks. On 5/10/19 2:35 PM, Keith Busch wrote: > The nvme pci driver prepares its devices for power loss during suspend > by shutting down the controllers, and the power setting is deferred to > pci driver's power management before the p

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-11 Thread Christoph Hellwig
A couple nitpicks, mostly leftover from the previous iteration (I didn't see replies to those comments from you, despite seeing a reply to my mail, assuming it didn't get lost): > +int nvme_set_power(struct nvme_ctrl *ctrl, unsigned ps) > +{ > + return nvme_set_features(ctrl, NVME_FEAT_POWER_M

Re: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-10 Thread Edmund Nadolski
On 5/10/19 2:29 PM, Keith Busch wrote: The nvme pci driver prepares its devices for power loss during suspend by shutting down the controllers, and the power setting is deferred to pci driver's power management before the platform removes power. The suspend-to-idle mode, however, does not remove

RE: [PATCH] nvme/pci: Use host managed power state for suspend

2019-05-10 Thread Mario.Limonciello
> > Cc: Mario Limonciello > Cc: Kai Heng Feng > Signed-off-by: Keith Busch > --- > Disclaimer: I've tested only on emulation faking support for the feature. Thanks for sharing. I'll arrange some testing with this with storage partners early next week. > > General question: different devic

[PATCH] nvme/pci: Use host managed power state for suspend

2019-05-10 Thread Keith Busch
The nvme pci driver prepares its devices for power loss during suspend by shutting down the controllers, and the power setting is deferred to pci driver's power management before the platform removes power. The suspend-to-idle mode, however, does not remove power. NVMe devices that implement host