Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] nvme-pci: Save PCI state before putting drive into deepest state
@ 2019-09-18 18:15 Mario Limonciello
  2019-09-20 17:16 ` Sagi Grimberg
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Mario Limonciello @ 2019-09-18 18:15 UTC (permalink / raw)
  To: Keith Busch
  Cc: Crag Wang, Sagi Grimberg, Mario Limonciello, sjg, LKML,
	linux-nvme, Jens Axboe, Ryan Hong, Jared Dominguez,
	Christoph Hellwig

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 | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Changes from v1:
 * Discard saved state in error scenario
 * Removed unneeded goto statement in error scenario

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 732d5b6..ef69013 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2894,11 +2894,21 @@ 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;
 
 	if (ret) {
+		/* discard the saved state */
+		pci_load_saved_state(pdev, NULL);
+
 		/*
 		 * Clearing npss forces a controller reset on resume. The
 		 * correct value will be resdicovered then.
@@ -2906,14 +2916,7 @@ static int nvme_suspend(struct device *dev)
 		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;
-- 
2.7.4


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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] nvme-pci: Save PCI state before putting drive into deepest state
  2019-09-18 18:15 [PATCH v2] nvme-pci: Save PCI state before putting drive into deepest state Mario Limonciello
@ 2019-09-20 17:16 ` Sagi Grimberg
  2019-09-20 17:46 ` Keith Busch
  2019-09-27 21:45 ` Christoph Hellwig
  2 siblings, 0 replies; 4+ messages in thread
From: Sagi Grimberg @ 2019-09-20 17:16 UTC (permalink / raw)
  To: Mario Limonciello, Keith Busch
  Cc: Crag Wang, sjg, LKML, linux-nvme, Jens Axboe, Ryan Hong,
	Jared Dominguez, Christoph Hellwig

Keith, can we get a review from you for this?

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] nvme-pci: Save PCI state before putting drive into deepest state
  2019-09-18 18:15 [PATCH v2] nvme-pci: Save PCI state before putting drive into deepest state Mario Limonciello
  2019-09-20 17:16 ` Sagi Grimberg
@ 2019-09-20 17:46 ` Keith Busch
  2019-09-27 21:45 ` Christoph Hellwig
  2 siblings, 0 replies; 4+ messages in thread
From: Keith Busch @ 2019-09-20 17:46 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Crag Wang, Sagi Grimberg, sjg, LKML, linux-nvme, Jens Axboe,
	Ryan Hong, Jared Dominguez, Christoph Hellwig

On Wed, Sep 18, 2019 at 01:15:55PM -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>

This looks good. It clashes with something I posted yesterday, but
I'll rebase after this one.

Reviewed-by: Keith Busch <kbusch@kernel.org>

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] nvme-pci: Save PCI state before putting drive into deepest state
  2019-09-18 18:15 [PATCH v2] nvme-pci: Save PCI state before putting drive into deepest state Mario Limonciello
  2019-09-20 17:16 ` Sagi Grimberg
  2019-09-20 17:46 ` Keith Busch
@ 2019-09-27 21:45 ` Christoph Hellwig
  2 siblings, 0 replies; 4+ messages in thread
From: Christoph Hellwig @ 2019-09-27 21:45 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Crag Wang, Sagi Grimberg, sjg, LKML, linux-nvme, Jens Axboe,
	Ryan Hong, Keith Busch, Jared Dominguez, Christoph Hellwig

Looks sensible to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-18 18:15 [PATCH v2] nvme-pci: Save PCI state before putting drive into deepest state Mario Limonciello
2019-09-20 17:16 ` Sagi Grimberg
2019-09-20 17:46 ` Keith Busch
2019-09-27 21:45 ` Christoph Hellwig

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git