RE: [Regression] Commit "nvme/pci: Use host managed power state for suspend" has problems

[<Mario.Limonciello@dell.com>]

> -----Original Message-----
> From: Keith Busch <kbusch@kernel.org>
> Sent: Tuesday, July 30, 2019 9:42 AM
> To: Rafael J. Wysocki
> Cc: Busch, Keith; Limonciello, Mario; Kai-Heng Feng; Christoph Hellwig; Sagi
> Grimberg; linux-nvme; Linux PM; Linux Kernel Mailing List; Rajat Jain
> Subject: Re: [Regression] Commit "nvme/pci: Use host managed power state for
> suspend" has problems
> 
> 
> [EXTERNAL EMAIL]
> 
> On Tue, Jul 30, 2019 at 03:45:31AM -0700, Rafael J. Wysocki wrote:
> > So I can reproduce this problem with plain 5.3-rc1 and the patch below fixes it.
> >
> > Also Mario reports that the same patch needs to be applied for his 9380 to
> reach
> > SLP_S0 after some additional changes under testing/review now, so here it
> goes.
> >
> > [The changes mentioned above are in the pm-s2idle-testing branch in the
> >  linux-pm.git tree at kernel.org.]
> >
> > ---
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Subject: [PATCH] nvme-pci: Do not prevent PCI bus-level PM from being used
> >
> > One of the modifications made by commit d916b1be94b6 ("nvme-pci: use
> > host managed power state for suspend") was adding a pci_save_state()
> > call to nvme_suspend() in order to prevent the PCI bus-level PM from
> > being applied to the suspended NVMe devices, but that causes the NVMe
> > drive (PC401 NVMe SK hynix 256GB) in my Dell XPS13 9380 to prevent
> > the SoC from reaching package idle states deeper than PC3, which is
> > way insufficient for system suspend.
> >
> > Fix this issue by removing the pci_save_state() call in question.
> 
> I'm okay with the patch if we can get confirmation this doesn't break
> any previously tested devices. I recall we add the pci_save_state() in
> the first place specifically to prevent PCI D3 since that was reported
> to break some devices' low power settings. Kai-Heng or Mario, any input
> here?
> 

It's entirely possible that in fixing the shutdown/flush/send NVME power state command
that D3 will be OK now but it will take some time to double check across the variety of disks that
we tested before.

What's kernel policy in terms of adding a module parameter and removing it later?  My gut
reaction is I'd like to see that behind a module parameter and if we see that all the disks
are actually OK we can potentially rip it out in a future release.  Also gives us a knob for easier
wider testing outside of the 4 of us.

> 
> 
> > Fixes: d916b1be94b6 ("nvme-pci: use host managed power state for suspend")
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/nvme/host/pci.c |    8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> >
> > Index: linux-pm/drivers/nvme/host/pci.c
> >
> ==============================================================
> =====
> > --- linux-pm.orig/drivers/nvme/host/pci.c
> > +++ linux-pm/drivers/nvme/host/pci.c
> > @@ -2897,14 +2897,8 @@ static int nvme_suspend(struct device *d
> >  		nvme_dev_disable(ndev, true);
> >  		ctrl->npss = 0;
> >  		ret = 0;
> > -		goto unfreeze;
> >  	}
> > -	/*
> > -	 * A saved state prevents pci pm from generically controlling the
> > -	 * device's power. If we're using protocol specific settings, we don't
> > -	 * want pci interfering.
> > -	 */
> > -	pci_save_state(pdev);
> > +
> >  unfreeze:
> >  	nvme_unfreeze(ctrl);
> >  	return ret;