Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state

From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Keith Busch <kbusch@kernel.org>
Cc: Crag Wang <Crag.Wang@dell.com>, Sagi Grimberg <sagi@grimberg.me>,
	Mario Limonciello <mario.limonciello@dell.com>,
	sjg@google.com, LKML <linux-kernel@vger.kernel.org>,
	linux-nvme@lists.infradead.org, Jens Axboe <axboe@fb.com>,
	Ryan Hong <Ryan.Hong@dell.com>,
	Jared Dominguez <jared.dominguez@dell.com>,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH] nvme-pci: Save PCI state before putting drive into deepest state
Date: Tue, 17 Sep 2019 23:35:47 +0200	[thread overview]
Message-ID: <10773060.Xg13aEV830@kreacher> (raw)
In-Reply-To: <20190917212414.GB39848@C02WT3WMHTD6.wdl.wdc.com>

On Tuesday, September 17, 2019 11:24:14 PM CEST Keith Busch wrote:
> On Wed, Sep 11, 2019 at 06:42:33PM -0500, Mario Limonciello wrote:
> > The action of saving the PCI state will cause numerous PCI configuration
> > space reads which depending upon the vendor implementation may cause
> > the drive to exit the deepest NVMe state.
> > 
> > In these cases ASPM will typically resolve the PCIe link state and APST
> > may resolve the NVMe power state.  However it has also been observed
> > that this register access after quiesced will cause PC10 failure
> > on some device combinations.
> > 
> > To resolve this, move the PCI state saving to before SetFeatures has been
> > called.  This has been proven to resolve the issue across a 5000 sample
> > test on previously failing disk/system combinations.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > ---
> >  drivers/nvme/host/pci.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> > index 732d5b6..9b3fed4 100644
> > --- a/drivers/nvme/host/pci.c
> > +++ b/drivers/nvme/host/pci.c
> > @@ -2894,6 +2894,13 @@ static int nvme_suspend(struct device *dev)
> >  	if (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);
> > +
> >  	ret = nvme_set_power_state(ctrl, ctrl->npss);
> >  	if (ret < 0)
> >  		goto unfreeze;
> > @@ -2908,12 +2915,6 @@ static int nvme_suspend(struct device *dev)
> >  		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;
> 
> In the event that something else fails after the point you've saved
> the state, we need to fallback to the behavior for when the driver
> doesn't save the state, right?

Depending on whether or not an error is going to be returned.

When returning an error, it is not necessary to worry about the saved state,
because that will cause the entire system-wide suspend to be aborted.

Otherwise it is sufficient to clear the state_saved flag of the PCI device
before returning 0 to make the PCI layer take over.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme