All of lore.kernel.org
 help / color / mirror / Atom feed
From: "David E. Box" <david.e.box@linux.intel.com>
To: rjw@rjwysocki.net, lenb@kernel.org, kbusch@kernel.org,
	axboe@fb.com, hch@lst.de, sagi@grimberg.me,
	dan.j.williams@intel.com, shyjumon.n@intel.com
Cc: "David E. Box" <david.e.box@linux.intel.com>,
	linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-nvme@lists.infradead.org
Subject: [PATCH V5] drivers/nvme: Add support for ACPI StorageD3Enable property
Date: Thu,  9 Jul 2020 11:43:33 -0700	[thread overview]
Message-ID: <20200709184333.6241-1-david.e.box@linux.intel.com> (raw)
In-Reply-To: <20200702225011.10932-1-david.e.box@linux.intel.com>

This patch implements a solution for a BIOS hack used on some currently
shipping Intel systems to change driver power management policy for PCIe
NVMe drives. Some newer Intel platforms, like some Comet Lake systems,
require that PCIe devices use D3 when doing suspend-to-idle in order to
allow the platform to realize maximum power savings. This is particularly
needed to support ATX power supply shutdown on desktop systems. In order to
ensure this happens for root ports with storage devices, Microsoft
apparently created this ACPI _DSD property as a way to influence their
driver policy. To my knowledge this property has not been discussed with
the NVME specification body.

Though the solution is not ideal, it addresses a problem that also affects
Linux since the NVMe driver's default policy of using NVMe APST during
suspend-to-idle prevents the PCI root port from going to D3 and leads to
higher power consumption for these platforms. The power consumption
difference may be negligible on laptop systems, but many watts on desktop
systems when the ATX power supply is blocked from powering down.

The patch creates a new nvme_acpi_storage_d3 function to check for the
StorageD3Enable property during probe and enables D3 as a quirk if set.  It
also provides a 'noacpi' module parameter to allow skipping the quirk if
needed.

Tested on:
PM961 NVMe SED Samsung 512GB
INTEL SSDPEKKF512G8

Link: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
Changes from V4:
 	- Add support for North complex PCI root ports.
	- Use acpi_bus_get_device instead of acpi_bus_get_acpi_device.
	  Also fixes compiler error on V4 reported by lkp@intel.com.
	- Place CONFIG_ACPI around function since acpi_bus_get_device
	  is not stubbed out.

Changes from V3:
 	- Use pcie_find_root_port() instead of pci_find_pcie_root_port(),
 	  changed in 5.8.
 	- Remove "Cc:" emails that ended up at top of V3 commit message.
 	- Fix changelog numbering.
 
Changes from V2:
	- Remove check for "not yet bound" ACPI companion device since
	  this will not be a concern at driver probe time per Rafael.
	- Move storage_d3 function out of PCI core and into NVMe driver
	  since there's nothing the PCI core can do with this code as
	  noted by Bjorn.
 
Changes from V1:
	- Export the pci_acpi_storage_d3 function for use by drivers as
	  needed instead of modifying the pci header.
	- Add missing put on acpi device handle.
	- Add 'noacpi' module parameter to allow undoing this change.
	- Add info message that this is a platform quirk.


 drivers/acpi/property.c |  3 ++
 drivers/nvme/host/pci.c | 64 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index e601c4511a8b..c2e2ae774a19 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -45,6 +45,9 @@ static const guid_t prp_guids[] = {
 	/* Thunderbolt GUID for WAKE_SUPPORTED: 6c501103-c189-4296-ba72-9bf5a26ebe5d */
 	GUID_INIT(0x6c501103, 0xc189, 0x4296,
 		  0xba, 0x72, 0x9b, 0xf5, 0xa2, 0x6e, 0xbe, 0x5d),
+	/* Storage device needs D3 GUID: 5025030f-842f-4ab4-a561-99a5189762d0 */
+	GUID_INIT(0x5025030f, 0x842f, 0x4ab4,
+		  0xa5, 0x61, 0x99, 0xa5, 0x18, 0x97, 0x62, 0xd0),
 };
 
 /* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index b1d18f0633c7..7c0be363eb22 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2011-2014, Intel Corporation.
  */
 
+#include <linux/acpi.h>
 #include <linux/aer.h>
 #include <linux/async.h>
 #include <linux/blkdev.h>
@@ -94,6 +95,10 @@ static unsigned int poll_queues;
 module_param_cb(poll_queues, &io_queue_count_ops, &poll_queues, 0644);
 MODULE_PARM_DESC(poll_queues, "Number of queues to use for polled IO.");
 
+static bool noacpi;
+module_param(noacpi, bool, 0444);
+MODULE_PARM_DESC(noacpi, "disable acpi bios quirks");
+
 struct nvme_dev;
 struct nvme_queue;
 
@@ -2759,6 +2764,55 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_ACPI
+static bool nvme_acpi_storage_d3(struct pci_dev *dev)
+{
+	const struct fwnode_handle *fwnode;
+	struct acpi_device *adev;
+	struct pci_dev *root;
+	acpi_handle handle;
+	acpi_status status;
+	u8 val;
+
+	/*
+	 * Look for _DSD property specifying that the storage device on
+	 * the port must use D3 to support deep platform power savings during
+	 * suspend-to-idle
+	 */
+	root = pcie_find_root_port(dev);
+	if (!root)
+		return false;
+
+	adev = ACPI_COMPANION(&root->dev);
+	if (!adev)
+		return false;
+
+	/*
+	 * The property is defined in the PXSX device for South complex ports
+	 * and in the PEGP device for North complex ports.
+	 */
+	status = acpi_get_handle(adev->handle, "PXSX", &handle);
+	if (ACPI_FAILURE(status)) {
+		status = acpi_get_handle(adev->handle, "PEGP", &handle);
+		if (ACPI_FAILURE(status))
+			return false;
+	}
+
+	if (acpi_bus_get_device(handle, &adev))
+		return false;
+
+	fwnode = acpi_fwnode_handle(adev);
+
+	return fwnode_property_read_u8(fwnode, "StorageD3Enable", &val) ?
+		false : val == 1;
+}
+#else
+static inline bool nvme_acpi_storage_d3(struct pci_dev *dev)
+{
+	return false;
+}
+#endif
+
 static void nvme_async_probe(void *data, async_cookie_t cookie)
 {
 	struct nvme_dev *dev = data;
@@ -2808,6 +2862,16 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	quirks |= check_vendor_combination_bug(pdev);
 
+	if (!noacpi && nvme_acpi_storage_d3(pdev)) {
+		/*
+		 * Some systems use a bios work around to ask for D3 on
+		 * platforms that support kernel managed suspend.
+		 */
+		dev_info(&pdev->dev,
+			 "platform quirk: setting simple suspend\n");
+		quirks |= NVME_QUIRK_SIMPLE_SUSPEND;
+	}
+
 	/*
 	 * Double check that our mempool alloc size will cover the biggest
 	 * command we support.
-- 
2.20.1


WARNING: multiple messages have this Message-ID (diff)
From: "David E. Box" <david.e.box@linux.intel.com>
To: rjw@rjwysocki.net, lenb@kernel.org, kbusch@kernel.org,
	axboe@fb.com, hch@lst.de, sagi@grimberg.me,
	dan.j.williams@intel.com, shyjumon.n@intel.com
Cc: linux-acpi@vger.kernel.org, linux-nvme@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	"David E. Box" <david.e.box@linux.intel.com>
Subject: [PATCH V5] drivers/nvme: Add support for ACPI StorageD3Enable property
Date: Thu,  9 Jul 2020 11:43:33 -0700	[thread overview]
Message-ID: <20200709184333.6241-1-david.e.box@linux.intel.com> (raw)
In-Reply-To: <20200702225011.10932-1-david.e.box@linux.intel.com>

This patch implements a solution for a BIOS hack used on some currently
shipping Intel systems to change driver power management policy for PCIe
NVMe drives. Some newer Intel platforms, like some Comet Lake systems,
require that PCIe devices use D3 when doing suspend-to-idle in order to
allow the platform to realize maximum power savings. This is particularly
needed to support ATX power supply shutdown on desktop systems. In order to
ensure this happens for root ports with storage devices, Microsoft
apparently created this ACPI _DSD property as a way to influence their
driver policy. To my knowledge this property has not been discussed with
the NVME specification body.

Though the solution is not ideal, it addresses a problem that also affects
Linux since the NVMe driver's default policy of using NVMe APST during
suspend-to-idle prevents the PCI root port from going to D3 and leads to
higher power consumption for these platforms. The power consumption
difference may be negligible on laptop systems, but many watts on desktop
systems when the ATX power supply is blocked from powering down.

The patch creates a new nvme_acpi_storage_d3 function to check for the
StorageD3Enable property during probe and enables D3 as a quirk if set.  It
also provides a 'noacpi' module parameter to allow skipping the quirk if
needed.

Tested on:
PM961 NVMe SED Samsung 512GB
INTEL SSDPEKKF512G8

Link: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
Changes from V4:
 	- Add support for North complex PCI root ports.
	- Use acpi_bus_get_device instead of acpi_bus_get_acpi_device.
	  Also fixes compiler error on V4 reported by lkp@intel.com.
	- Place CONFIG_ACPI around function since acpi_bus_get_device
	  is not stubbed out.

Changes from V3:
 	- Use pcie_find_root_port() instead of pci_find_pcie_root_port(),
 	  changed in 5.8.
 	- Remove "Cc:" emails that ended up at top of V3 commit message.
 	- Fix changelog numbering.
 
Changes from V2:
	- Remove check for "not yet bound" ACPI companion device since
	  this will not be a concern at driver probe time per Rafael.
	- Move storage_d3 function out of PCI core and into NVMe driver
	  since there's nothing the PCI core can do with this code as
	  noted by Bjorn.
 
Changes from V1:
	- Export the pci_acpi_storage_d3 function for use by drivers as
	  needed instead of modifying the pci header.
	- Add missing put on acpi device handle.
	- Add 'noacpi' module parameter to allow undoing this change.
	- Add info message that this is a platform quirk.


 drivers/acpi/property.c |  3 ++
 drivers/nvme/host/pci.c | 64 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index e601c4511a8b..c2e2ae774a19 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -45,6 +45,9 @@ static const guid_t prp_guids[] = {
 	/* Thunderbolt GUID for WAKE_SUPPORTED: 6c501103-c189-4296-ba72-9bf5a26ebe5d */
 	GUID_INIT(0x6c501103, 0xc189, 0x4296,
 		  0xba, 0x72, 0x9b, 0xf5, 0xa2, 0x6e, 0xbe, 0x5d),
+	/* Storage device needs D3 GUID: 5025030f-842f-4ab4-a561-99a5189762d0 */
+	GUID_INIT(0x5025030f, 0x842f, 0x4ab4,
+		  0xa5, 0x61, 0x99, 0xa5, 0x18, 0x97, 0x62, 0xd0),
 };
 
 /* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index b1d18f0633c7..7c0be363eb22 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2011-2014, Intel Corporation.
  */
 
+#include <linux/acpi.h>
 #include <linux/aer.h>
 #include <linux/async.h>
 #include <linux/blkdev.h>
@@ -94,6 +95,10 @@ static unsigned int poll_queues;
 module_param_cb(poll_queues, &io_queue_count_ops, &poll_queues, 0644);
 MODULE_PARM_DESC(poll_queues, "Number of queues to use for polled IO.");
 
+static bool noacpi;
+module_param(noacpi, bool, 0444);
+MODULE_PARM_DESC(noacpi, "disable acpi bios quirks");
+
 struct nvme_dev;
 struct nvme_queue;
 
@@ -2759,6 +2764,55 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_ACPI
+static bool nvme_acpi_storage_d3(struct pci_dev *dev)
+{
+	const struct fwnode_handle *fwnode;
+	struct acpi_device *adev;
+	struct pci_dev *root;
+	acpi_handle handle;
+	acpi_status status;
+	u8 val;
+
+	/*
+	 * Look for _DSD property specifying that the storage device on
+	 * the port must use D3 to support deep platform power savings during
+	 * suspend-to-idle
+	 */
+	root = pcie_find_root_port(dev);
+	if (!root)
+		return false;
+
+	adev = ACPI_COMPANION(&root->dev);
+	if (!adev)
+		return false;
+
+	/*
+	 * The property is defined in the PXSX device for South complex ports
+	 * and in the PEGP device for North complex ports.
+	 */
+	status = acpi_get_handle(adev->handle, "PXSX", &handle);
+	if (ACPI_FAILURE(status)) {
+		status = acpi_get_handle(adev->handle, "PEGP", &handle);
+		if (ACPI_FAILURE(status))
+			return false;
+	}
+
+	if (acpi_bus_get_device(handle, &adev))
+		return false;
+
+	fwnode = acpi_fwnode_handle(adev);
+
+	return fwnode_property_read_u8(fwnode, "StorageD3Enable", &val) ?
+		false : val == 1;
+}
+#else
+static inline bool nvme_acpi_storage_d3(struct pci_dev *dev)
+{
+	return false;
+}
+#endif
+
 static void nvme_async_probe(void *data, async_cookie_t cookie)
 {
 	struct nvme_dev *dev = data;
@@ -2808,6 +2862,16 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	quirks |= check_vendor_combination_bug(pdev);
 
+	if (!noacpi && nvme_acpi_storage_d3(pdev)) {
+		/*
+		 * Some systems use a bios work around to ask for D3 on
+		 * platforms that support kernel managed suspend.
+		 */
+		dev_info(&pdev->dev,
+			 "platform quirk: setting simple suspend\n");
+		quirks |= NVME_QUIRK_SIMPLE_SUSPEND;
+	}
+
 	/*
 	 * Double check that our mempool alloc size will cover the biggest
 	 * command we support.
-- 
2.20.1


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

  parent reply	other threads:[~2020-07-09 18:43 UTC|newest]

Thread overview: 81+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-28  0:32 [PATCH 0/2] Add support for StorageD3Enable _DSD property David E. Box
2020-04-28  0:32 ` David E. Box
2020-04-28  0:32 ` [PATCH 1/2] pci: Add ACPI StorageD3Enable _DSD support David E. Box
2020-04-28  0:32   ` David E. Box
2020-05-18 12:34   ` Rafael J. Wysocki
2020-05-18 12:34     ` Rafael J. Wysocki
2020-05-19 17:10     ` David E. Box
2020-05-19 17:10       ` David E. Box
2020-04-28  0:32 ` [PATCH 2/2] drivers/nvme: Add support for ACPI StorageD3Enable property David E. Box
2020-04-28  0:32   ` David E. Box
2020-04-28  5:13 ` [PATCH 0/2] Add support for StorageD3Enable _DSD property Christoph Hellwig
2020-04-28  5:13   ` Christoph Hellwig
2020-04-28 14:09   ` David E. Box
2020-04-28 14:09     ` David E. Box
2020-04-28 14:22     ` Christoph Hellwig
2020-04-28 14:22       ` Christoph Hellwig
2020-04-28 15:27       ` David E. Box
2020-04-28 15:27         ` David E. Box
2020-04-29  5:20         ` Williams, Dan J
2020-04-29  5:20           ` Williams, Dan J
2020-04-29 15:10           ` Keith Busch
2020-04-29 15:10             ` Keith Busch
2020-04-29 16:11           ` David E. Box
2020-04-29 16:11             ` David E. Box
2020-05-01 13:12             ` hch
2020-05-01 13:12               ` hch
2020-05-01 15:54               ` David E. Box
2020-05-01 15:54                 ` David E. Box
2020-05-01 13:10           ` hch
2020-05-01 13:10             ` hch
2020-05-18 13:51           ` David Woodhouse
2020-05-18 13:51             ` David Woodhouse
2020-05-18 17:20             ` Dan Williams
2020-05-18 17:20               ` Dan Williams
2020-06-12 20:48 ` [PATCH V2 0/2] nvme: Add support for ACPI StorageD3Enable property David E. Box
2020-06-12 20:48   ` David E. Box
2020-06-24 18:55   ` David E. Box
2020-06-24 18:55     ` David E. Box
2020-06-24 19:10     ` Dan Williams
2020-06-24 19:10       ` Dan Williams
2020-06-24 19:39       ` David E. Box
2020-06-24 19:39         ` David E. Box
2020-07-02 21:04   ` [PATCH v3] drivers/nvme: " David E. Box
2020-07-02 22:50   ` [PATCH v4] " David E. Box
2020-07-02 22:50     ` David E. Box
2020-07-03  7:18     ` kernel test robot
2020-07-03  7:18       ` kernel test robot
2020-07-06 14:57     ` Rafael J. Wysocki
2020-07-06 14:57       ` Rafael J. Wysocki
2020-07-07 21:24       ` David E. Box
2020-07-07 21:24         ` David E. Box
2020-07-07  7:09     ` Christoph Hellwig
2020-07-07  7:09       ` Christoph Hellwig
2020-07-09 18:43     ` David E. Box [this message]
2020-07-09 18:43       ` [PATCH V5] " David E. Box
2020-07-13 11:12       ` Rafael J. Wysocki
2020-07-13 11:12         ` Rafael J. Wysocki
2020-07-16 14:39         ` Christoph Hellwig
2020-07-16 14:39           ` Christoph Hellwig
2021-05-26 19:25       ` Raul E Rangel
2021-05-26 19:25         ` Raul E Rangel
2020-06-12 20:48 ` [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support David E. Box
2020-06-12 20:48   ` David E. Box
2020-06-24 21:15   ` Bjorn Helgaas
2020-06-24 21:15     ` Bjorn Helgaas
2020-06-25 11:30     ` Rafael J. Wysocki
2020-06-25 11:30       ` Rafael J. Wysocki
2020-06-25 12:16       ` Mika Westerberg
2020-06-25 12:16         ` Mika Westerberg
2020-06-25 17:07       ` David E. Box
2020-06-25 17:30       ` Bjorn Helgaas
2020-06-25 17:30         ` Bjorn Helgaas
2020-06-24 21:37   ` Bjorn Helgaas
2020-06-24 21:37     ` Bjorn Helgaas
2020-06-24 22:09     ` David E. Box
2020-06-24 22:09       ` David E. Box
2020-06-12 20:48 ` [PATCH V2 2/2] drivers/nvme: Add support for ACPI StorageD3Enable property David E. Box
2020-06-12 20:48   ` David E. Box
2021-05-26 17:53 [PATCH V5] " Raul Rangel
2021-05-27  2:20 ` David E. Box
2021-05-27  2:20   ` David E. Box

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200709184333.6241-1-david.e.box@linux.intel.com \
    --to=david.e.box@linux.intel.com \
    --cc=axboe@fb.com \
    --cc=dan.j.williams@intel.com \
    --cc=hch@lst.de \
    --cc=kbusch@kernel.org \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=rjw@rjwysocki.net \
    --cc=sagi@grimberg.me \
    --cc=shyjumon.n@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.