* [PATCH 0/2] Add support for StorageD3Enable _DSD property
@ 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
` (5 more replies)
0 siblings, 6 replies; 42+ messages in thread
From: David E. Box @ 2020-04-28 0:32 UTC (permalink / raw)
To: rjw, lenb, bhelgaas, kbusch, axboe, hch, sagi
Cc: David E. Box, linux-acpi, linux-pci, linux-nvme, linux-kernel
NVMe storage power management during suspend-to-idle, particularly on
laptops, has been inconsistent with some devices working with D3 while
others must rely on NVMe APST in order for power savings to be realized.
Currently the default is to use APST unless quirked to do otherwise.
However newer platforms, like Intel Comet Lake systems, may require NVMe
drives to use D3 in order for the PCIe ports to be properly power managed.
To make it easier for drivers to choose, these platforms may supply a
special "StorageD3Enable" _DSD property under the root port that the device
is attached to. If supplied, the driver must use D3 in order for the
platform to realize the deepest power savings in suspend-to-idle.
The first patch adds the new _DSD GUID and fowards the property through the
pci/acpi layer to the pci device.
The second patch adds support for the property to the nvme driver.
David E. Box (2):
pci: Add ACPI StorageD3Enable _DSD support
drivers/nvme: Add support for ACPI StorageD3Enable property
drivers/acpi/property.c | 3 +++
drivers/nvme/host/pci.c | 7 ++++++
drivers/pci/pci-acpi.c | 47 +++++++++++++++++++++++++++++++++++++++++
drivers/pci/pci.c | 6 ++++++
drivers/pci/pci.h | 4 ++++
include/linux/pci.h | 1 +
6 files changed, 68 insertions(+)
--
2.20.1
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH 1/2] pci: Add ACPI StorageD3Enable _DSD support 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-05-18 12:34 ` Rafael J. Wysocki 2020-04-28 0:32 ` [PATCH 2/2] drivers/nvme: Add support for ACPI StorageD3Enable property David E. Box ` (4 subsequent siblings) 5 siblings, 1 reply; 42+ messages in thread From: David E. Box @ 2020-04-28 0:32 UTC (permalink / raw) To: rjw, lenb, bhelgaas, kbusch, axboe, hch, sagi Cc: David E. Box, linux-acpi, linux-pci, linux-nvme, linux-kernel, Dan Williams NVMe storage power management during suspend-to-idle, particularly on laptops, has been inconsistent with some devices working with D3 while others must rely on NVMe APST in order for power savings to be realized. Currently the default is to use APST unless quirked to do otherwise. However newer platforms, like Intel Comet Lake systems, may require NVMe drives to use D3 in order for the PCIe ports to be properly power managed. To make it easier for drivers to choose, these platforms may supply a special "StorageD3Enable" _DSD property under the root port that the device is attached to. If supplied, the driver must use D3 in order for the platform to realize the deepest power savings in suspend-to-idle. Adds support for the _DSD to the pci/acpi layer. Acked-by: Dan Williams <dan.j.williams@intel.com> 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> --- drivers/acpi/property.c | 3 +++ drivers/pci/pci-acpi.c | 47 +++++++++++++++++++++++++++++++++++++++++ drivers/pci/pci.c | 6 ++++++ drivers/pci/pci.h | 4 ++++ include/linux/pci.h | 1 + 5 files changed, 61 insertions(+) diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c index e601c4511a8b..f09375ab40e4 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), + /* D3 Support for storage devivce: 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/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index d21969fba6ab..5df249ebf022 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -972,6 +972,52 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev) return val == 1; } +static bool acpi_pci_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 = pci_find_pcie_root_port(dev); + if (!root) + return false; + + adev = ACPI_COMPANION(&root->dev); + if (root == dev) { + /* + * It is possible that the ACPI companion is not yet bound + * for the root port so look it up manually here. + */ + if (!adev && !pci_dev_is_added(root)) + adev = acpi_pci_find_companion(&root->dev); + } + + if (!adev) + return false; + + status = acpi_get_handle(adev->handle, "PXSX", &handle); + if (ACPI_FAILURE(status)) + return false; + + adev = acpi_bus_get_acpi_device(handle); + if (!adev) + return false; + + fwnode = acpi_fwnode_handle(adev); + if (!fwnode_property_read_u8(fwnode, "StorageD3Enable", &val)) + return val == 1; + + return false; +} + static bool acpi_pci_power_manageable(struct pci_dev *dev) { struct acpi_device *adev = ACPI_COMPANION(&dev->dev); @@ -1100,6 +1146,7 @@ static bool acpi_pci_need_resume(struct pci_dev *dev) static const struct pci_platform_pm_ops acpi_pci_platform_pm = { .bridge_d3 = acpi_pci_bridge_d3, + .storage_d3 = acpi_pci_storage_d3, .is_manageable = acpi_pci_power_manageable, .set_state = acpi_pci_set_power_state, .get_state = acpi_pci_get_power_state, diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 595fcf59843f..217d89f31e8b 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -871,6 +871,11 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev) return pci_platform_pm ? pci_platform_pm->bridge_d3(dev) : false; } +static inline bool platform_pci_storage_d3(struct pci_dev *dev) +{ + return pci_platform_pm->storage_d3(dev); +} + /** * pci_raw_set_power_state - Use PCI PM registers to set the power state of * given PCI device @@ -2903,6 +2908,7 @@ void pci_pm_init(struct pci_dev *dev) dev->d3_delay = PCI_PM_D3_WAIT; dev->d3cold_delay = PCI_PM_D3COLD_WAIT; dev->bridge_d3 = pci_bridge_d3_possible(dev); + dev->storage_d3 = platform_pci_storage_d3(dev); dev->d3cold_allowed = true; dev->d1_support = false; diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 6d3f75867106..4dc4d05da810 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -53,6 +53,9 @@ int pci_bus_error_reset(struct pci_dev *dev); * * @bridge_d3: Does the bridge allow entering into D3 * + * @storage_d3: 'true' if the bridge contains a storage device that must use D3 + * to support platform s2idle + * * @is_manageable: returns 'true' if given device is power manageable by the * platform firmware * @@ -77,6 +80,7 @@ int pci_bus_error_reset(struct pci_dev *dev); */ struct pci_platform_pm_ops { bool (*bridge_d3)(struct pci_dev *dev); + bool (*storage_d3)(struct pci_dev *dev); bool (*is_manageable)(struct pci_dev *dev); int (*set_state)(struct pci_dev *dev, pci_power_t state); pci_power_t (*get_state)(struct pci_dev *dev); diff --git a/include/linux/pci.h b/include/linux/pci.h index 83ce1cdf5676..cbba1555020b 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -346,6 +346,7 @@ struct pci_dev { unsigned int no_d1d2:1; /* D1 and D2 are forbidden */ unsigned int no_d3cold:1; /* D3cold is forbidden */ unsigned int bridge_d3:1; /* Allow D3 for bridge */ + unsigned int storage_d3:1; /* Storage dev must use D3 for s2idle */ unsigned int d3cold_allowed:1; /* D3cold is allowed by user */ unsigned int mmio_always_on:1; /* Disallow turning off io/mem decoding during BAR sizing */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] pci: Add ACPI StorageD3Enable _DSD support 2020-04-28 0:32 ` [PATCH 1/2] pci: Add ACPI StorageD3Enable _DSD support David E. Box @ 2020-05-18 12:34 ` Rafael J. Wysocki 2020-05-19 17:10 ` David E. Box 0 siblings, 1 reply; 42+ messages in thread From: Rafael J. Wysocki @ 2020-05-18 12:34 UTC (permalink / raw) To: David E. Box Cc: lenb, bhelgaas, kbusch, axboe, hch, sagi, linux-acpi, linux-pci, linux-nvme, linux-kernel, Dan Williams On Tuesday, April 28, 2020 2:32:13 AM CEST David E. Box wrote: > NVMe storage power management during suspend-to-idle, particularly on > laptops, has been inconsistent with some devices working with D3 while > others must rely on NVMe APST in order for power savings to be realized. > Currently the default is to use APST unless quirked to do otherwise. > However newer platforms, like Intel Comet Lake systems, may require NVMe > drives to use D3 in order for the PCIe ports to be properly power managed. > To make it easier for drivers to choose, these platforms may supply a > special "StorageD3Enable" _DSD property under the root port that the device > is attached to. If supplied, the driver must use D3 in order for the > platform to realize the deepest power savings in suspend-to-idle. > > Adds support for the _DSD to the pci/acpi layer. > > Acked-by: Dan Williams <dan.j.williams@intel.com> > 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> > --- > drivers/acpi/property.c | 3 +++ > drivers/pci/pci-acpi.c | 47 +++++++++++++++++++++++++++++++++++++++++ > drivers/pci/pci.c | 6 ++++++ > drivers/pci/pci.h | 4 ++++ > include/linux/pci.h | 1 + > 5 files changed, 61 insertions(+) > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c > index e601c4511a8b..f09375ab40e4 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), > + /* D3 Support for storage devivce: 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/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index d21969fba6ab..5df249ebf022 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -972,6 +972,52 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev) > return val == 1; > } > > +static bool acpi_pci_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 = pci_find_pcie_root_port(dev); > + if (!root) > + return false; > + > + adev = ACPI_COMPANION(&root->dev); > + if (root == dev) { > + /* > + * It is possible that the ACPI companion is not yet bound > + * for the root port so look it up manually here. > + */ > + if (!adev && !pci_dev_is_added(root)) > + adev = acpi_pci_find_companion(&root->dev); > + } > + > + if (!adev) > + return false; > + > + status = acpi_get_handle(adev->handle, "PXSX", &handle); > + if (ACPI_FAILURE(status)) > + return false; > + > + adev = acpi_bus_get_acpi_device(handle); > + if (!adev) > + return false; > + > + fwnode = acpi_fwnode_handle(adev); > + if (!fwnode_property_read_u8(fwnode, "StorageD3Enable", &val)) > + return val == 1; > + > + return false; > +} Kind of orthogonal to what happens to the second patch in this series, I don't think that the PCI changes below are all needed. IMO it would be sufficient to export the function above, maybe as pci_acpi_storage_d3(), to drivers, so that they can call it directly as desired. Since _DSD return data are not allowed by the spec to change between subsequent invocations of it, the interested driver may call this function once at the device init time and quirk it accordingly if needed. Cheers! ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] pci: Add ACPI StorageD3Enable _DSD support 2020-05-18 12:34 ` Rafael J. Wysocki @ 2020-05-19 17:10 ` David E. Box 0 siblings, 0 replies; 42+ messages in thread From: David E. Box @ 2020-05-19 17:10 UTC (permalink / raw) To: Rafael J. Wysocki Cc: lenb, bhelgaas, kbusch, axboe, hch, sagi, linux-acpi, linux-pci, linux-nvme, linux-kernel, Dan Williams On Mon, 2020-05-18 at 14:34 +0200, Rafael J. Wysocki wrote: > On Tuesday, April 28, 2020 2:32:13 AM CEST David E. Box wrote: > > NVMe storage power management during suspend-to-idle, particularly > > on > > laptops, has been inconsistent with some devices working with D3 > > while > > others must rely on NVMe APST in order for power savings to be > > realized. > > Currently the default is to use APST unless quirked to do > > otherwise. > > However newer platforms, like Intel Comet Lake systems, may require > > NVMe > > drives to use D3 in order for the PCIe ports to be properly power > > managed. > > To make it easier for drivers to choose, these platforms may supply > > a > > special "StorageD3Enable" _DSD property under the root port that > > the device > > is attached to. If supplied, the driver must use D3 in order for > > the > > platform to realize the deepest power savings in suspend-to-idle. > > > > Adds support for the _DSD to the pci/acpi layer. > > > > Acked-by: Dan Williams <dan.j.williams@intel.com> > > 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> > > --- > > drivers/acpi/property.c | 3 +++ > > drivers/pci/pci-acpi.c | 47 > > +++++++++++++++++++++++++++++++++++++++++ > > drivers/pci/pci.c | 6 ++++++ > > drivers/pci/pci.h | 4 ++++ > > include/linux/pci.h | 1 + > > 5 files changed, 61 insertions(+) > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c > > index e601c4511a8b..f09375ab40e4 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), > > + /* D3 Support for storage devivce: 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/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > > index d21969fba6ab..5df249ebf022 100644 > > --- a/drivers/pci/pci-acpi.c > > +++ b/drivers/pci/pci-acpi.c > > @@ -972,6 +972,52 @@ static bool acpi_pci_bridge_d3(struct pci_dev > > *dev) > > return val == 1; > > } > > > > +static bool acpi_pci_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 = pci_find_pcie_root_port(dev); > > + if (!root) > > + return false; > > + > > + adev = ACPI_COMPANION(&root->dev); > > + if (root == dev) { > > + /* > > + * It is possible that the ACPI companion is not yet > > bound > > + * for the root port so look it up manually here. > > + */ > > + if (!adev && !pci_dev_is_added(root)) > > + adev = acpi_pci_find_companion(&root->dev); > > + } > > + > > + if (!adev) > > + return false; > > + > > + status = acpi_get_handle(adev->handle, "PXSX", &handle); > > + if (ACPI_FAILURE(status)) > > + return false; > > + > > + adev = acpi_bus_get_acpi_device(handle); > > + if (!adev) > > + return false; > > + > > + fwnode = acpi_fwnode_handle(adev); > > + if (!fwnode_property_read_u8(fwnode, "StorageD3Enable", &val)) > > + return val == 1; > > + > > + return false; > > +} > > Kind of orthogonal to what happens to the second patch in this > series, I don't > think that the PCI changes below are all needed. > > IMO it would be sufficient to export the function above, maybe as > pci_acpi_storage_d3(), to drivers, so that they can call it directly > as > desired. > > Since _DSD return data are not allowed by the spec to change between > subsequent invocations of it, the interested driver may call this > function > once at the device init time and quirk it accordingly if needed. Yeah this is better. Will do. Thanks. ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 2/2] drivers/nvme: Add support for ACPI StorageD3Enable property 2020-04-28 0:32 [PATCH 0/2] Add support for StorageD3Enable _DSD property 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-04-28 5:13 ` [PATCH 0/2] Add support for StorageD3Enable _DSD property Christoph Hellwig ` (3 subsequent siblings) 5 siblings, 0 replies; 42+ messages in thread From: David E. Box @ 2020-04-28 0:32 UTC (permalink / raw) To: rjw, lenb, bhelgaas, kbusch, axboe, hch, sagi Cc: David E. Box, linux-acpi, linux-pci, linux-nvme, linux-kernel, Dan Williams NVMe storage power management during suspend-to-idle, particularly on laptops, has been inconsistent with some devices working with D3 while others must rely on NVMe APST in order for power savings to be realized. Currently the default is to use APST unless quirked to do otherwise. However newer platforms, like Intel Comet Lake systems, may require NVMe drives to use D3 in order for the PCIe ports to be properly power managed. To make it easier for drivers to choose, these platforms may supply a special "StorageD3Enable" _DSD property under the root port that the device is attached to. If supplied, the driver must use D3 in order for the platform to realize the deepest power savings in suspend-to-idle. Add check of StorageD3Enable property during probe to use D3 as specified by platform firmware. Acked-by: Dan Williams <dan.j.williams@intel.com> 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> --- drivers/nvme/host/pci.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 4e79e412b276..4d67735975f6 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2777,6 +2777,13 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id) quirks |= check_vendor_combination_bug(pdev); + /* + * Platform requires storage device to use D3 for kernel managed + * suspend. + */ + if (pdev->storage_d3) + quirks |= NVME_QUIRK_SIMPLE_SUSPEND; + /* * Double check that our mempool alloc size will cover the biggest * command we support. -- 2.20.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Add support for StorageD3Enable _DSD property 2020-04-28 0:32 [PATCH 0/2] Add support for StorageD3Enable _DSD property 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 ` [PATCH 2/2] drivers/nvme: Add support for ACPI StorageD3Enable property David E. Box @ 2020-04-28 5:13 ` Christoph Hellwig 2020-04-28 14:09 ` David E. Box 2020-06-12 20:48 ` [PATCH V2 0/2] nvme: Add support for ACPI StorageD3Enable property David E. Box ` (2 subsequent siblings) 5 siblings, 1 reply; 42+ messages in thread From: Christoph Hellwig @ 2020-04-28 5:13 UTC (permalink / raw) To: David E. Box Cc: rjw, lenb, bhelgaas, kbusch, axboe, hch, sagi, linux-acpi, linux-pci, linux-nvme, linux-kernel On Mon, Apr 27, 2020 at 05:32:12PM -0700, David E. Box wrote: > NVMe storage power management during suspend-to-idle, particularly on > laptops, has been inconsistent with some devices working with D3 while > others must rely on NVMe APST in order for power savings to be realized. > Currently the default is to use APST unless quirked to do otherwise. > However newer platforms, like Intel Comet Lake systems, may require NVMe > drives to use D3 in order for the PCIe ports to be properly power managed. > To make it easier for drivers to choose, these platforms may supply a > special "StorageD3Enable" _DSD property under the root port that the device > is attached to. If supplied, the driver must use D3 in order for the > platform to realize the deepest power savings in suspend-to-idle. > > The first patch adds the new _DSD GUID and fowards the property through the > pci/acpi layer to the pci device. > > The second patch adds support for the property to the nvme driver. I'm not sure who came up with the idea to put this into ACPI, but it belongs into NVMe. Please talk to the NVMe technical working group instead of trying to overrules them in an unrelated group that doesn't apply to all of PCIe. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Add support for StorageD3Enable _DSD property 2020-04-28 5:13 ` [PATCH 0/2] Add support for StorageD3Enable _DSD property Christoph Hellwig @ 2020-04-28 14:09 ` David E. Box 2020-04-28 14:22 ` Christoph Hellwig 0 siblings, 1 reply; 42+ messages in thread From: David E. Box @ 2020-04-28 14:09 UTC (permalink / raw) To: Christoph Hellwig Cc: rjw, lenb, bhelgaas, kbusch, axboe, sagi, linux-acpi, linux-pci, linux-nvme, linux-kernel On Tue, 2020-04-28 at 07:13 +0200, Christoph Hellwig wrote: > On Mon, Apr 27, 2020 at 05:32:12PM -0700, David E. Box wrote: > > NVMe storage power management during suspend-to-idle, particularly > > on > > laptops, has been inconsistent with some devices working with D3 > > while > > others must rely on NVMe APST in order for power savings to be > > realized. > > Currently the default is to use APST unless quirked to do > > otherwise. > > However newer platforms, like Intel Comet Lake systems, may require > > NVMe > > drives to use D3 in order for the PCIe ports to be properly power > > managed. > > To make it easier for drivers to choose, these platforms may supply > > a > > special "StorageD3Enable" _DSD property under the root port that > > the device > > is attached to. If supplied, the driver must use D3 in order for > > the > > platform to realize the deepest power savings in suspend-to-idle. > > > > The first patch adds the new _DSD GUID and fowards the property > > through the > > pci/acpi layer to the pci device. > > > > The second patch adds support for the property to the nvme driver. > > I'm not sure who came up with the idea to put this into ACPI, but it > belongs into NVMe. Please talk to the NVMe technical working group > instead of trying to overrules them in an unrelated group that > doesn't > apply to all of PCIe. Agreed that this is not ideal since it does not apply to all of PCIe. But as the property already exists on shipping systems, we need to be able to read it in the NVMe driver and the patch is consitent with the way properties under PCI ports are read. David ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Add support for StorageD3Enable _DSD property 2020-04-28 14:09 ` David E. Box @ 2020-04-28 14:22 ` Christoph Hellwig 2020-04-28 15:27 ` David E. Box 0 siblings, 1 reply; 42+ messages in thread From: Christoph Hellwig @ 2020-04-28 14:22 UTC (permalink / raw) To: David E. Box Cc: Christoph Hellwig, rjw, lenb, bhelgaas, kbusch, axboe, sagi, linux-acpi, linux-pci, linux-nvme, linux-kernel On Tue, Apr 28, 2020 at 07:09:59AM -0700, David E. Box wrote: > > I'm not sure who came up with the idea to put this into ACPI, but it > > belongs into NVMe. Please talk to the NVMe technical working group > > instead of trying to overrules them in an unrelated group that > > doesn't > > apply to all of PCIe. > > Agreed that this is not ideal since it does not apply to all of PCIe. > But as the property already exists on shipping systems, we need to be > able to read it in the NVMe driver and the patch is consitent with the > way properties under PCI ports are read. The point is that it is not the BIOSes job do decide how Linux does power management. For example D3 has really horrible entry and exit latencies in many cases, and will lead to higher power usage. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Add support for StorageD3Enable _DSD property 2020-04-28 14:22 ` Christoph Hellwig @ 2020-04-28 15:27 ` David E. Box 2020-04-29 5:20 ` Williams, Dan J 0 siblings, 1 reply; 42+ messages in thread From: David E. Box @ 2020-04-28 15:27 UTC (permalink / raw) To: Christoph Hellwig Cc: rjw, lenb, bhelgaas, kbusch, axboe, sagi, linux-acpi, linux-pci, linux-nvme, linux-kernel On Tue, 2020-04-28 at 16:22 +0200, Christoph Hellwig wrote: > On Tue, Apr 28, 2020 at 07:09:59AM -0700, David E. Box wrote: > > > I'm not sure who came up with the idea to put this into ACPI, but > > > it > > > belongs into NVMe. Please talk to the NVMe technical working > > > group > > > instead of trying to overrules them in an unrelated group that > > > doesn't > > > apply to all of PCIe. > > > > Agreed that this is not ideal since it does not apply to all of > > PCIe. > > But as the property already exists on shipping systems, we need to > > be > > able to read it in the NVMe driver and the patch is consitent with > > the > > way properties under PCI ports are read. > > The point is that it is not the BIOSes job do decide how Linux does > power management. For example D3 has really horrible entry and exit > latencies in many cases, and will lead to higher power usage. The platform can know which pm policies will save the most power. But since the solution doesn't apply to all PCIe devices (despite BIOS specifying it that way) I'll withdraw this patch. Thanks. David ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Add support for StorageD3Enable _DSD property 2020-04-28 15:27 ` David E. Box @ 2020-04-29 5:20 ` Williams, Dan J 2020-04-29 15:10 ` Keith Busch ` (3 more replies) 0 siblings, 4 replies; 42+ messages in thread From: Williams, Dan J @ 2020-04-29 5:20 UTC (permalink / raw) To: hch, david.e.box Cc: linux-nvme, sagi, lenb, rjw, linux-kernel, linux-acpi, linux-pci, axboe, kbusch, bhelgaas On Tue, 2020-04-28 at 08:27 -0700, David E. Box wrote: > On Tue, 2020-04-28 at 16:22 +0200, Christoph Hellwig wrote: > > On Tue, Apr 28, 2020 at 07:09:59AM -0700, David E. Box wrote: > > > > I'm not sure who came up with the idea to put this into ACPI, > > > > but > > > > it > > > > belongs into NVMe. Please talk to the NVMe technical working > > > > group > > > > instead of trying to overrules them in an unrelated group that > > > > doesn't > > > > apply to all of PCIe. > > > > > > Agreed that this is not ideal since it does not apply to all of > > > PCIe. > > > But as the property already exists on shipping systems, we need > > > to > > > be > > > able to read it in the NVMe driver and the patch is consitent > > > with > > > the > > > way properties under PCI ports are read. > > > > The point is that it is not the BIOSes job do decide how Linux does > > power management. For example D3 has really horrible entry and > > exit > > latencies in many cases, and will lead to higher power usage. > > The platform can know which pm policies will save the most power. But > since the solution doesn't apply to all PCIe devices (despite BIOS > specifying it that way) I'll withdraw this patch. Thanks. Wait, why withdraw? In this case the platform is unfortunately preventing the standard driver from making a proper determination. So while I agree that it's not the BIOSes job, when the platform actively prevents proper operation due to some ill conceived non-standard platform property what is Linux left to do on these systems? The *patch* is not trying to overrule NVME, and the best I can say is that the Intel Linux team was not in the loop when this was being decided between the platform BIOS implemenation and whomever thought they could just publish random ACPI properties that impacted NVME operation [1]. So now David is trying to get these platform unbroken because they are already shipping with this b0rkage. [1]: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Add support for StorageD3Enable _DSD property 2020-04-29 5:20 ` Williams, Dan J @ 2020-04-29 15:10 ` Keith Busch 2020-04-29 16:11 ` David E. Box ` (2 subsequent siblings) 3 siblings, 0 replies; 42+ messages in thread From: Keith Busch @ 2020-04-29 15:10 UTC (permalink / raw) To: Williams, Dan J Cc: hch, david.e.box, linux-nvme, sagi, lenb, rjw, linux-kernel, linux-acpi, linux-pci, axboe, bhelgaas On Wed, Apr 29, 2020 at 05:20:09AM +0000, Williams, Dan J wrote: > On Tue, 2020-04-28 at 08:27 -0700, David E. Box wrote: > > On Tue, 2020-04-28 at 16:22 +0200, Christoph Hellwig wrote: > > > On Tue, Apr 28, 2020 at 07:09:59AM -0700, David E. Box wrote: > > > > > I'm not sure who came up with the idea to put this into ACPI, > > > > > but > > > > > it > > > > > belongs into NVMe. Please talk to the NVMe technical working > > > > > group > > > > > instead of trying to overrules them in an unrelated group that > > > > > doesn't > > > > > apply to all of PCIe. > > > > > > > > Agreed that this is not ideal since it does not apply to all of > > > > PCIe. > > > > But as the property already exists on shipping systems, we need > > > > to > > > > be > > > > able to read it in the NVMe driver and the patch is consitent > > > > with > > > > the > > > > way properties under PCI ports are read. > > > > > > The point is that it is not the BIOSes job do decide how Linux does > > > power management. For example D3 has really horrible entry and > > > exit > > > latencies in many cases, and will lead to higher power usage. > > > > The platform can know which pm policies will save the most power. But > > since the solution doesn't apply to all PCIe devices (despite BIOS > > specifying it that way) I'll withdraw this patch. Thanks. > > Wait, why withdraw? In this case the platform is unfortunately > preventing the standard driver from making a proper determination. So > while I agree that it's not the BIOSes job, when the platform actively > prevents proper operation due to some ill conceived non-standard > platform property what is Linux left to do on these systems? > > The *patch* is not trying to overrule NVME, and the best I can say is > that the Intel Linux team was not in the loop when this was being > decided between the platform BIOS implemenation and whomever thought > they could just publish random ACPI properties that impacted NVME > operation [1]. > > So now David is trying to get these platform unbroken because they are > already shipping with this b0rkage. Rather than quirking all these cases, which I get the feeling there are many more than we've currently got in our quirk list, perhaps it'd be simpler to default to the simple suspend. AFAIK, the simple suspend works for all platforms, though it may not realize the best power savings and/or exit latency. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Add support for StorageD3Enable _DSD property 2020-04-29 5:20 ` Williams, Dan J 2020-04-29 15:10 ` Keith Busch @ 2020-04-29 16:11 ` David E. Box 2020-05-01 13:12 ` hch 2020-05-01 13:10 ` hch 2020-05-18 13:51 ` David Woodhouse 3 siblings, 1 reply; 42+ messages in thread From: David E. Box @ 2020-04-29 16:11 UTC (permalink / raw) To: Williams, Dan J, hch Cc: linux-nvme, sagi, lenb, rjw, linux-kernel, linux-acpi, linux-pci, axboe, kbusch, bhelgaas On Wed, 2020-04-29 at 05:20 +0000, Williams, Dan J wrote: > On Tue, 2020-04-28 at 08:27 -0700, David E. Box wrote: > > On Tue, 2020-04-28 at 16:22 +0200, Christoph Hellwig wrote: > > > On Tue, Apr 28, 2020 at 07:09:59AM -0700, David E. Box wrote: > > > > > I'm not sure who came up with the idea to put this into ACPI, > > > > > but > > > > > it > > > > > belongs into NVMe. Please talk to the NVMe technical working > > > > > group > > > > > instead of trying to overrules them in an unrelated group > > > > > that > > > > > doesn't > > > > > apply to all of PCIe. > > > > > > > > Agreed that this is not ideal since it does not apply to all of > > > > PCIe. > > > > But as the property already exists on shipping systems, we need > > > > to > > > > be > > > > able to read it in the NVMe driver and the patch is consitent > > > > with > > > > the > > > > way properties under PCI ports are read. > > > > > > The point is that it is not the BIOSes job do decide how Linux > > > does > > > power management. For example D3 has really horrible entry and > > > exit > > > latencies in many cases, and will lead to higher power usage. > > > > The platform can know which pm policies will save the most power. > > But > > since the solution doesn't apply to all PCIe devices (despite BIOS > > specifying it that way) I'll withdraw this patch. Thanks. > > Wait, why withdraw? In this case the platform is unfortunately > preventing the standard driver from making a proper determination. So > while I agree that it's not the BIOSes job, when the platform > actively > prevents proper operation due to some ill conceived non-standard > platform property what is Linux left to do on these systems? > > The *patch* is not trying to overrule NVME, and the best I can say is > that the Intel Linux team was not in the loop when this was being > decided between the platform BIOS implemenation > and whomever thought > they could just publish random ACPI properties that impacted NVME > operation [1]. > > So now David is trying to get these platform unbroken because they > are > already shipping with this b0rkage. Not drop completely. This patch copied the code used to read _DSD properties under PCI root ports. But I agree that such properties should apply to all devices on those ports and unfortuntely that's not the case here. BIOS got it wrong. My thought in dropping this patch is to rewrite it to read the property directly from the nvme driver. Not the way it's typically done either but it would avoid a global change in the pci core while allowing us to deal with the firmware we have. David ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Add support for StorageD3Enable _DSD property 2020-04-29 16:11 ` David E. Box @ 2020-05-01 13:12 ` hch 2020-05-01 15:54 ` David E. Box 0 siblings, 1 reply; 42+ messages in thread From: hch @ 2020-05-01 13:12 UTC (permalink / raw) To: David E. Box Cc: Williams, Dan J, hch, linux-nvme, sagi, lenb, rjw, linux-kernel, linux-acpi, linux-pci, axboe, kbusch, bhelgaas On Wed, Apr 29, 2020 at 09:11:13AM -0700, David E. Box wrote: > Not drop completely. This patch copied the code used to read _DSD > properties under PCI root ports. But I agree that such properties > should apply to all devices on those ports and unfortuntely that's not > the case here. BIOS got it wrong. My thought in dropping this patch is > to rewrite it to read the property directly from the nvme driver. Not > the way it's typically done either but it would avoid a global change > in the pci core while allowing us to deal with the firmware we have. I'd be happy to heave less of this crap in nvme actually. But I'm really pissed this shit got out in the wild. It wasn't clear from the mail that this is something already out there because the idiots coming up with it just went ahead with it. Please just update the commit logs and implementation to clearly mark it as a workaround for buggys systems, which just happen to at least be nice enough to tell us that they are buggy as f^$k. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Add support for StorageD3Enable _DSD property 2020-05-01 13:12 ` hch @ 2020-05-01 15:54 ` David E. Box 0 siblings, 0 replies; 42+ messages in thread From: David E. Box @ 2020-05-01 15:54 UTC (permalink / raw) To: hch Cc: Williams, Dan J, linux-nvme, sagi, lenb, rjw, linux-kernel, linux-acpi, linux-pci, axboe, kbusch, bhelgaas On Fri, 2020-05-01 at 15:12 +0200, hch@lst.de wrote: > On Wed, Apr 29, 2020 at 09:11:13AM -0700, David E. Box wrote: > > Not drop completely. This patch copied the code used to read _DSD > > properties under PCI root ports. But I agree that such properties > > should apply to all devices on those ports and unfortuntely that's > > not > > the case here. BIOS got it wrong. My thought in dropping this patch > > is > > to rewrite it to read the property directly from the nvme driver. > > Not > > the way it's typically done either but it would avoid a global > > change > > in the pci core while allowing us to deal with the firmware we > > have. > > I'd be happy to heave less of this crap in nvme actually. But I'm > really > pissed this shit got out in the wild. It wasn't clear from the mail > that this is something already out there because the idiots coming up > with it just went ahead with it. Please just update the commit logs > and implementation to clearly mark it as a workaround for buggys > systems, which just happen to at least be nice enough to tell us that > they are buggy as f^$k. Sure. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Add support for StorageD3Enable _DSD property 2020-04-29 5:20 ` Williams, Dan J 2020-04-29 15:10 ` Keith Busch 2020-04-29 16:11 ` David E. Box @ 2020-05-01 13:10 ` hch 2020-05-18 13:51 ` David Woodhouse 3 siblings, 0 replies; 42+ messages in thread From: hch @ 2020-05-01 13:10 UTC (permalink / raw) To: Williams, Dan J Cc: hch, david.e.box, linux-nvme, sagi, lenb, rjw, linux-kernel, linux-acpi, linux-pci, axboe, kbusch, bhelgaas On Wed, Apr 29, 2020 at 05:20:09AM +0000, Williams, Dan J wrote: > > The platform can know which pm policies will save the most power. But > > since the solution doesn't apply to all PCIe devices (despite BIOS > > specifying it that way) I'll withdraw this patch. Thanks. > > Wait, why withdraw? In this case the platform is unfortunately > preventing the standard driver from making a proper determination. So > while I agree that it's not the BIOSes job, when the platform actively > prevents proper operation due to some ill conceived non-standard > platform property what is Linux left to do on these systems? > > The *patch* is not trying to overrule NVME, and the best I can say is > that the Intel Linux team was not in the loop when this was being > decided between the platform BIOS implemenation and whomever thought > they could just publish random ACPI properties that impacted NVME > operation [1]. > > So now David is trying to get these platform unbroken because they are > already shipping with this b0rkage. So can we please clearly mark this as a quirk and warn in the kernel log about a buggy BIOS? ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Add support for StorageD3Enable _DSD property 2020-04-29 5:20 ` Williams, Dan J ` (2 preceding siblings ...) 2020-05-01 13:10 ` hch @ 2020-05-18 13:51 ` David Woodhouse 2020-05-18 17:20 ` Dan Williams 3 siblings, 1 reply; 42+ messages in thread From: David Woodhouse @ 2020-05-18 13:51 UTC (permalink / raw) To: Williams, Dan J, hch, david.e.box Cc: linux-nvme, sagi, lenb, rjw, linux-kernel, linux-acpi, linux-pci, axboe, kbusch, bhelgaas [-- Attachment #1: Type: text/plain, Size: 615 bytes --] On Wed, 2020-04-29 at 05:20 +0000, Williams, Dan J wrote: > The *patch* is not trying to overrule NVME, and the best I can say is > that the Intel Linux team was not in the loop when this was being > decided between the platform BIOS implemenation and whomever thought > they could just publish random ACPI properties that impacted NVME > operation [1]. > > So now David is trying to get these platform unbroken because they are > already shipping with this b0rkage. This is what we have WARN_TAINT() for though, right? It can suitably warn users when such breakage is detected in the platform. [-- Attachment #2: smime.p7s --] [-- Type: application/x-pkcs7-signature, Size: 5174 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Add support for StorageD3Enable _DSD property 2020-05-18 13:51 ` David Woodhouse @ 2020-05-18 17:20 ` Dan Williams 0 siblings, 0 replies; 42+ messages in thread From: Dan Williams @ 2020-05-18 17:20 UTC (permalink / raw) To: David Woodhouse Cc: hch, david.e.box, linux-nvme, sagi, lenb, rjw, linux-kernel, linux-acpi, linux-pci, axboe, kbusch, bhelgaas On Mon, May 18, 2020 at 6:52 AM David Woodhouse <dwmw2@infradead.org> wrote: > > On Wed, 2020-04-29 at 05:20 +0000, Williams, Dan J wrote: > > The *patch* is not trying to overrule NVME, and the best I can say is > > that the Intel Linux team was not in the loop when this was being > > decided between the platform BIOS implemenation and whomever thought > > they could just publish random ACPI properties that impacted NVME > > operation [1]. > > > > So now David is trying to get these platform unbroken because they are > > already shipping with this b0rkage. > > This is what we have WARN_TAINT() for though, right? It can suitably > warn users when such breakage is detected in the platform. > I see WARN_TAINT() as "BIOS implemented its specification wrong". This case is BIOS "implemented a mechanism in the wrong specification". ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH V2 0/2] nvme: Add support for ACPI StorageD3Enable property 2020-04-28 0:32 [PATCH 0/2] Add support for StorageD3Enable _DSD property David E. Box ` (2 preceding siblings ...) 2020-04-28 5:13 ` [PATCH 0/2] Add support for StorageD3Enable _DSD property Christoph Hellwig @ 2020-06-12 20:48 ` David E. Box 2020-06-24 18:55 ` David E. Box ` (2 more replies) 2020-06-12 20:48 ` [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support David E. Box 2020-06-12 20:48 ` [PATCH V2 2/2] drivers/nvme: Add support for ACPI StorageD3Enable property David E. Box 5 siblings, 3 replies; 42+ messages in thread From: David E. Box @ 2020-06-12 20:48 UTC (permalink / raw) To: shyjumon.n, rjw, lenb, bhelgaas, dan.j.williams, kbusch, axboe, hch, sagi Cc: David E. Box, linux-acpi, linux-kernel, linux-pci, linux-nvme This patch set implements a solution for a BIOS hack used on some currently shipping Intel systems to address issues with power management policy decisions concerning 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 override 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 would lead to higher power consumption for these platforms. Patch 1 provides a symbol in the PCI/ACPI layer to read the property. Patch 2 uses the symbol in the NVMe driver to select D3 as a quirk if set. Changes from V2: - 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. David E. Box (2): PCI: Add ACPI StorageD3Enable _DSD support drivers/nvme: Add support for ACPI StorageD3Enable property drivers/acpi/property.c | 3 +++ drivers/nvme/host/pci.c | 14 ++++++++++ drivers/pci/pci-acpi.c | 59 +++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 2 ++ 4 files changed, 78 insertions(+) -- 2.20.1 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V2 0/2] nvme: Add support for ACPI StorageD3Enable property 2020-06-12 20:48 ` [PATCH V2 0/2] nvme: Add support for ACPI StorageD3Enable property David E. Box @ 2020-06-24 18:55 ` David E. Box 2020-06-24 19:10 ` Dan Williams 2020-07-02 21:04 ` [PATCH v3] drivers/nvme: " David E. Box 2020-07-02 22:50 ` [PATCH v4] " David E. Box 2 siblings, 1 reply; 42+ messages in thread From: David E. Box @ 2020-06-24 18:55 UTC (permalink / raw) To: shyjumon.n, rjw, lenb, bhelgaas, dan.j.williams, kbusch, axboe, hch, sagi Cc: linux-acpi, linux-kernel, linux-pci, linux-nvme Friendly reminder. Thanks. David On Fri, 2020-06-12 at 13:48 -0700, David E. Box wrote: > This patch set implements a solution for a BIOS hack used on some > currently > shipping Intel systems to address issues with power management policy > decisions concerning 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 override 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 would lead to higher power consumption for these > platforms. > > Patch 1 provides a symbol in the PCI/ACPI layer to read the property. > Patch 2 uses the symbol in the NVMe driver to select D3 as a quirk if > set. > > Changes from V2: > - 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. > > David E. Box (2): > PCI: Add ACPI StorageD3Enable _DSD support > drivers/nvme: Add support for ACPI StorageD3Enable property > > drivers/acpi/property.c | 3 +++ > drivers/nvme/host/pci.c | 14 ++++++++++ > drivers/pci/pci-acpi.c | 59 > +++++++++++++++++++++++++++++++++++++++++ > include/linux/pci.h | 2 ++ > 4 files changed, 78 insertions(+) > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V2 0/2] nvme: Add support for ACPI StorageD3Enable property 2020-06-24 18:55 ` David E. Box @ 2020-06-24 19:10 ` Dan Williams 2020-06-24 19:39 ` David E. Box 0 siblings, 1 reply; 42+ messages in thread From: Dan Williams @ 2020-06-24 19:10 UTC (permalink / raw) To: David Box Cc: N, Shyjumon, Rafael J. Wysocki, Len Brown, Bjorn Helgaas, Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, Linux ACPI, Linux Kernel Mailing List, linux-pci, linux-nvme On Wed, Jun 24, 2020 at 11:55 AM David E. Box <david.e.box@linux.intel.com> wrote: > > Friendly reminder. Thanks. Are you looking for this to be merged by ACPI with an NVMe ack, or merged by NVMe with an ACPI ack? It sometimes helps to be explicit to break the log jam. > > David > > On Fri, 2020-06-12 at 13:48 -0700, David E. Box wrote: > > This patch set implements a solution for a BIOS hack used on some > > currently > > shipping Intel systems to address issues with power management policy > > decisions concerning 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 override 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 would lead to higher power consumption for these > > platforms. > > > > Patch 1 provides a symbol in the PCI/ACPI layer to read the property. > > Patch 2 uses the symbol in the NVMe driver to select D3 as a quirk if > > set. > > > > Changes from V2: > > - 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. > > > > David E. Box (2): > > PCI: Add ACPI StorageD3Enable _DSD support > > drivers/nvme: Add support for ACPI StorageD3Enable property > > > > drivers/acpi/property.c | 3 +++ > > drivers/nvme/host/pci.c | 14 ++++++++++ > > drivers/pci/pci-acpi.c | 59 > > +++++++++++++++++++++++++++++++++++++++++ > > include/linux/pci.h | 2 ++ > > 4 files changed, 78 insertions(+) > > > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V2 0/2] nvme: Add support for ACPI StorageD3Enable property 2020-06-24 19:10 ` Dan Williams @ 2020-06-24 19:39 ` David E. Box 0 siblings, 0 replies; 42+ messages in thread From: David E. Box @ 2020-06-24 19:39 UTC (permalink / raw) To: Dan Williams Cc: N, Shyjumon, Rafael J. Wysocki, Len Brown, Bjorn Helgaas, Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, Linux ACPI, Linux Kernel Mailing List, linux-pci, linux-nvme On Wed, 2020-06-24 at 12:10 -0700, Dan Williams wrote: > On Wed, Jun 24, 2020 at 11:55 AM David E. Box > <david.e.box@linux.intel.com> wrote: > > Friendly reminder. Thanks. > > Are you looking for this to be merged by ACPI with an NVMe ack, or > merged by NVMe with an ACPI ack? It sometimes helps to be explicit to > break the log jam. Ah. NVMe merge with ACPI ack. Thanks. ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3] drivers/nvme: Add support for ACPI StorageD3Enable property 2020-06-12 20:48 ` [PATCH V2 0/2] nvme: Add support for ACPI StorageD3Enable property David E. Box 2020-06-24 18:55 ` David E. Box @ 2020-07-02 21:04 ` David E. Box 2020-07-02 22:50 ` [PATCH v4] " David E. Box 2 siblings, 0 replies; 42+ messages in thread From: David E. Box @ 2020-07-02 21:04 UTC (permalink / raw) To: rjw, lenb, kbusch, axboe, hch, sagi, dan.j.williams, shyjumon.n Cc: David E. Box, linux-acpi Cc: linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org 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 V3: - 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 V2: - 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 | 55 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 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 e2bacd369a88..b4160c71c27d 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; @@ -2757,6 +2762,46 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev) return 0; } +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; + bool ret = false; + 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 = pci_find_pcie_root_port(dev); + if (!root) + return false; + + adev = ACPI_COMPANION(&root->dev); + if (!adev) + return false; + + status = acpi_get_handle(adev->handle, "PXSX", &handle); + if (ACPI_FAILURE(status)) + return false; + + adev = acpi_bus_get_acpi_device(handle); + if (!adev) + return false; + + fwnode = acpi_fwnode_handle(adev); + if (!fwnode_property_read_u8(fwnode, "StorageD3Enable", &val)) + ret = (val == 1); + + acpi_bus_put_acpi_device(adev); + + return ret; +} + static void nvme_async_probe(void *data, async_cookie_t cookie) { struct nvme_dev *dev = data; @@ -2806,6 +2851,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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v4] drivers/nvme: Add support for ACPI StorageD3Enable property 2020-06-12 20:48 ` [PATCH V2 0/2] nvme: Add support for ACPI StorageD3Enable property David E. Box 2020-06-24 18:55 ` David E. Box 2020-07-02 21:04 ` [PATCH v3] drivers/nvme: " David E. Box @ 2020-07-02 22:50 ` David E. Box 2020-07-03 7:18 ` kernel test robot ` (3 more replies) 2 siblings, 4 replies; 42+ messages in thread From: David E. Box @ 2020-07-02 22:50 UTC (permalink / raw) To: shyjumon.n, dan.j.williams, rjw, lenb, kbusch, axboe, hch, sagi Cc: David E. Box, linux-acpi, linux-kernel, linux-nvme 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 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 | 55 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 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 e2bacd369a88..a3d3a82b0437 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; @@ -2757,6 +2762,46 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev) return 0; } +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; + bool ret = false; + 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; + + status = acpi_get_handle(adev->handle, "PXSX", &handle); + if (ACPI_FAILURE(status)) + return false; + + adev = acpi_bus_get_acpi_device(handle); + if (!adev) + return false; + + fwnode = acpi_fwnode_handle(adev); + if (!fwnode_property_read_u8(fwnode, "StorageD3Enable", &val)) + ret = (val == 1); + + acpi_bus_put_acpi_device(adev); + + return ret; +} + static void nvme_async_probe(void *data, async_cookie_t cookie) { struct nvme_dev *dev = data; @@ -2806,6 +2851,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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v4] drivers/nvme: Add support for ACPI StorageD3Enable property 2020-07-02 22:50 ` [PATCH v4] " David E. Box @ 2020-07-03 7:18 ` kernel test robot 2020-07-06 14:57 ` Rafael J. Wysocki ` (2 subsequent siblings) 3 siblings, 0 replies; 42+ messages in thread From: kernel test robot @ 2020-07-03 7:18 UTC (permalink / raw) To: David E. Box, shyjumon.n, dan.j.williams, rjw, lenb, kbusch, axboe, hch, sagi Cc: kbuild-all, David E. Box, linux-acpi [-- Attachment #1: Type: text/plain, Size: 1243 bytes --] Hi "David, I love your patch! Yet something to improve: [auto build test ERROR on pm/linux-next] [also build test ERROR on block/for-next linus/master v5.8-rc3 next-20200702] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/David-E-Box/drivers-nvme-Add-support-for-ACPI-StorageD3Enable-property/20200703-065019 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next config: x86_64-rhel-7.6 (attached as .config) compiler: gcc-9 (Debian 9.3.0-14) 9.3.0 reproduce (this is a W=1 build): # save the attached .config to linux build tree make W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>, old ones prefixed by <<): >> ERROR: modpost: "acpi_bus_get_acpi_device" [drivers/nvme/host/nvme.ko] undefined! >> ERROR: modpost: "acpi_bus_put_acpi_device" [drivers/nvme/host/nvme.ko] undefined! --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 49258 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4] drivers/nvme: Add support for ACPI StorageD3Enable property 2020-07-02 22:50 ` [PATCH v4] " David E. Box 2020-07-03 7:18 ` kernel test robot @ 2020-07-06 14:57 ` Rafael J. Wysocki 2020-07-07 21:24 ` David E. Box 2020-07-07 7:09 ` Christoph Hellwig 2020-07-09 18:43 ` [PATCH V5] " David E. Box 3 siblings, 1 reply; 42+ messages in thread From: Rafael J. Wysocki @ 2020-07-06 14:57 UTC (permalink / raw) To: David E. Box Cc: shyjumon.n, Dan Williams, Rafael J. Wysocki, Len Brown, Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, ACPI Devel Maling List, Linux Kernel Mailing List, linux-nvme On Fri, Jul 3, 2020 at 12:49 AM David E. Box <david.e.box@linux.intel.com> wrote: > > 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 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 | 55 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 58 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 e2bacd369a88..a3d3a82b0437 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; > > @@ -2757,6 +2762,46 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev) > return 0; > } > > +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; > + bool ret = false; > + 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; > + > + status = acpi_get_handle(adev->handle, "PXSX", &handle); > + if (ACPI_FAILURE(status)) > + return false; > + > + adev = acpi_bus_get_acpi_device(handle); This function needs to be exported to modules for nvme to be able to use it when modular. > + if (!adev) > + return false; > + > + fwnode = acpi_fwnode_handle(adev); > + if (!fwnode_property_read_u8(fwnode, "StorageD3Enable", &val)) > + ret = (val == 1); > + > + acpi_bus_put_acpi_device(adev); And same here. > + > + return ret; > +} > + > static void nvme_async_probe(void *data, async_cookie_t cookie) > { > struct nvme_dev *dev = data; > @@ -2806,6 +2851,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 > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4] drivers/nvme: Add support for ACPI StorageD3Enable property 2020-07-06 14:57 ` Rafael J. Wysocki @ 2020-07-07 21:24 ` David E. Box 0 siblings, 0 replies; 42+ messages in thread From: David E. Box @ 2020-07-07 21:24 UTC (permalink / raw) To: Rafael J. Wysocki Cc: shyjumon.n, Dan Williams, Rafael J. Wysocki, Len Brown, Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, ACPI Devel Maling List, Linux Kernel Mailing List, linux-nvme On Mon, 2020-07-06 at 16:57 +0200, Rafael J. Wysocki wrote: > On Fri, Jul 3, 2020 at 12:49 AM David E. Box > <david.e.box@linux.intel.com> wrote: > > 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 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 | 55 > > +++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 58 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 e2bacd369a88..a3d3a82b0437 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; > > > > @@ -2757,6 +2762,46 @@ static unsigned long > > check_vendor_combination_bug(struct pci_dev *pdev) > > return 0; > > } > > > > +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; > > + bool ret = false; > > + 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; > > + > > + status = acpi_get_handle(adev->handle, "PXSX", &handle); > > + if (ACPI_FAILURE(status)) > > + return false; > > + > > + adev = acpi_bus_get_acpi_device(handle); > > This function needs to be exported to modules for nvme to be able to > use it when modular. Missed that they weren't exported when I moved from pci-acpi to nvme as it worked with my config. But looking again I see there's already an acpi_bus_get_device() that's exported and should work here. Unfortunately, it would be the only function that's not stubbed out so per Cristoph's later comment, this will need a CONFIG_ACPI wrapper. David ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4] drivers/nvme: Add support for ACPI StorageD3Enable property 2020-07-02 22:50 ` [PATCH v4] " David E. Box 2020-07-03 7:18 ` kernel test robot 2020-07-06 14:57 ` Rafael J. Wysocki @ 2020-07-07 7:09 ` Christoph Hellwig 2020-07-09 18:43 ` [PATCH V5] " David E. Box 3 siblings, 0 replies; 42+ messages in thread From: Christoph Hellwig @ 2020-07-07 7:09 UTC (permalink / raw) To: David E. Box Cc: shyjumon.n, dan.j.williams, rjw, lenb, kbusch, axboe, hch, sagi, linux-acpi, linux-kernel, linux-nvme On Thu, Jul 02, 2020 at 03:50:11PM -0700, David E. Box wrote: > 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 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 | 55 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 58 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 e2bacd369a88..a3d3a82b0437 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; > > @@ -2757,6 +2762,46 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev) > return 0; > } > > +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; > + bool ret = false; > + 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; > + > + status = acpi_get_handle(adev->handle, "PXSX", &handle); > + if (ACPI_FAILURE(status)) > + return false; > + > + adev = acpi_bus_get_acpi_device(handle); > + if (!adev) > + return false; > + > + fwnode = acpi_fwnode_handle(adev); > + if (!fwnode_property_read_u8(fwnode, "StorageD3Enable", &val)) > + ret = (val == 1); > + > + acpi_bus_put_acpi_device(adev); > + > + return ret; > +} Doesn't this need a CONFIG_ACPI of some sort? Or are all these calls properly stubbed out? ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH V5] drivers/nvme: Add support for ACPI StorageD3Enable property 2020-07-02 22:50 ` [PATCH v4] " David E. Box ` (2 preceding siblings ...) 2020-07-07 7:09 ` Christoph Hellwig @ 2020-07-09 18:43 ` David E. Box 2020-07-13 11:12 ` Rafael J. Wysocki 2021-05-26 19:25 ` Raul E Rangel 3 siblings, 2 replies; 42+ messages in thread From: David E. Box @ 2020-07-09 18:43 UTC (permalink / raw) To: rjw, lenb, kbusch, axboe, hch, sagi, dan.j.williams, shyjumon.n Cc: David E. Box, linux-acpi, linux-kernel, linux-nvme 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH V5] drivers/nvme: Add support for ACPI StorageD3Enable property 2020-07-09 18:43 ` [PATCH V5] " David E. Box @ 2020-07-13 11:12 ` Rafael J. Wysocki 2020-07-16 14:39 ` Christoph Hellwig 2021-05-26 19:25 ` Raul E Rangel 1 sibling, 1 reply; 42+ messages in thread From: Rafael J. Wysocki @ 2020-07-13 11:12 UTC (permalink / raw) To: David E. Box Cc: Rafael J. Wysocki, Len Brown, Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, Dan Williams, shyjumon.n, ACPI Devel Maling List, Linux Kernel Mailing List, linux-nvme On Thu, Jul 9, 2020 at 8:43 PM David E. Box <david.e.box@linux.intel.com> wrote: > > 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; I would write this as if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable", &val)) return false; return val == 1; to eliminate the redundant fwnode variable and untangle the last checks somewhat. With that changed please feel free to add Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> to the patch. Thanks! > +} > +#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 > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V5] drivers/nvme: Add support for ACPI StorageD3Enable property 2020-07-13 11:12 ` Rafael J. Wysocki @ 2020-07-16 14:39 ` Christoph Hellwig 0 siblings, 0 replies; 42+ messages in thread From: Christoph Hellwig @ 2020-07-16 14:39 UTC (permalink / raw) To: Rafael J. Wysocki Cc: David E. Box, Rafael J. Wysocki, Len Brown, Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, Dan Williams, shyjumon.n, ACPI Devel Maling List, Linux Kernel Mailing List, linux-nvme On Mon, Jul 13, 2020 at 01:12:26PM +0200, Rafael J. Wysocki wrote: > I would write this as > > if (fwnode_property_read_u8(acpi_fwnode_handle(adev), > "StorageD3Enable", &val)) > return false; > > return val == 1; > > to eliminate the redundant fwnode variable and untangle the last > checks somewhat. > > With that changed please feel free to add > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Applied to nvme-5.9 with the above fixup. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V5] drivers/nvme: Add support for ACPI StorageD3Enable property 2020-07-09 18:43 ` [PATCH V5] " David E. Box 2020-07-13 11:12 ` Rafael J. Wysocki @ 2021-05-26 19:25 ` Raul E Rangel 1 sibling, 0 replies; 42+ messages in thread From: Raul E Rangel @ 2021-05-26 19:25 UTC (permalink / raw) To: David E. Box Cc: rjw, lenb, kbusch, axboe, hch, sagi, dan.j.williams, shyjumon.n, linux-acpi, linux-kernel, linux-nvme On Thu, Jul 09, 2020 at 11:43:33AM -0700, David E. Box wrote: > +#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); So I'm curious why we need to directly look at the PXSX and PEGP devices instead of the ACPI_COMPANION node attached to the pci device? I've looked around and I can't find any documentation that defines the the PXSX and PEGP device names. I've dumped some ACPI from a system that uses the PXSX name and StorageD3Cold attribute: Scope (\_SB.PCI0.GP14) { Device (PXSX) { Name (_ADR, 0x0000000000000000) // _ADR: Address Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) } Name (_DSD, Package (0x02) // _DSD: Device-Specific Data { ToUUID ("5025030f-842f-4ab4-a561-99a5189762d0"), Package (0x01) { Package (0x02) { "StorageD3Enable", One } } }) } } It looks to me like it's just the firmware node for the NVMe device attached to the root port. Is that the correct assumption? I'm wondering if we can simplify the look up logic to look at the ACPI_COMPANION of the pci device? The reason I ask is that I'm working on enabling S0i3 on an AMD device. This device also defines the StorageD3Enable property, but it don't use the PXSX name: Scope (GPP6) { Device (NVME) { Name (_ADR, Zero) // _ADR: Address Name (_DSD, Package (0x02) // _DSD: Device-Specific Data { ToUUID ("5025030f-842f-4ab4-a561-99a5189762d0"), Package (0x01) { Package (0x02) { "StorageD3Enable", One } } }) } } The Windows [documentation](https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro#d3-support) makes it sound like the _DSD should be defined on the PCI device. I can send one of the following patches depending on the feedback: 1) Additionally check the pci device's ACPI_COMPANION for the _DSD. 2) Delete the PXSX and PEGP lookups and only look at the pci device's ACPI_COMPANION. > + 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; > +} Thanks, Raul p.s., Sorry for the second message, I somehow mangled the headers in the first message and dropped the Message-Id. ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support 2020-04-28 0:32 [PATCH 0/2] Add support for StorageD3Enable _DSD property David E. Box ` (3 preceding siblings ...) 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 21:15 ` Bjorn Helgaas 2020-06-24 21:37 ` Bjorn Helgaas 2020-06-12 20:48 ` [PATCH V2 2/2] drivers/nvme: Add support for ACPI StorageD3Enable property David E. Box 5 siblings, 2 replies; 42+ messages in thread From: David E. Box @ 2020-06-12 20:48 UTC (permalink / raw) To: shyjumon.n, rjw, lenb, bhelgaas, dan.j.williams, kbusch, axboe, hch, sagi Cc: David E. Box, linux-acpi, linux-kernel, linux-pci, linux-nvme StorageD3Enable is a boolean property that indicates that the platform wants to use D3 for PCIe storage drives during suspend-to-idle. It is a BIOS work around that is currently in use on shipping systems like some Intel Comet Lake platforms. It is meant to change default driver policy for suspend that may cause higher power consumption. Add the DSD property for recognition by fwnode calls and provide an exported symbol for device drivers to use to read the property as needed. 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> --- drivers/acpi/property.c | 3 +++ drivers/pci/pci-acpi.c | 59 +++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 2 ++ 3 files changed, 64 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/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index d21969fba6ab..732df524e09c 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -972,6 +972,65 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev) return val == 1; } +/** + * pci_acpi_storage_d3 - whether root port requests D3 for idle suspend + * @pdev: PCI device to check + * + * Returns true if the ACPI companion device contains the "StorageD3Enable" + * _DSD property and the value is 1. This indicates that the root port is + * used by a storage device and the platform is requesting D3 for the + * device during suspend to idle in order to support platform pm. + */ +bool pci_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; + bool ret = false; + 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 = pci_find_pcie_root_port(dev); + if (!root) + return false; + + adev = ACPI_COMPANION(&root->dev); + if (!adev) { + /* + * It is possible that the ACPI companion is not yet bound + * for the root port so look it up manually here. + */ + if (!adev && !pci_dev_is_added(root)) + adev = acpi_pci_find_companion(&root->dev); + } + + if (!adev) + return false; + + status = acpi_get_handle(adev->handle, "PXSX", &handle); + if (ACPI_FAILURE(status)) + return false; + + adev = acpi_bus_get_acpi_device(handle); + if (!adev) + return false; + + fwnode = acpi_fwnode_handle(adev); + if (!fwnode_property_read_u8(fwnode, "StorageD3Enable", &val)) + ret = (val == 1); + + acpi_bus_put_acpi_device(adev); + + return ret; +} +EXPORT_SYMBOL_GPL(pci_acpi_storage_d3); + static bool acpi_pci_power_manageable(struct pci_dev *dev) { struct acpi_device *adev = ACPI_COMPANION(&dev->dev); diff --git a/include/linux/pci.h b/include/linux/pci.h index 83ce1cdf5676..396fcb269a60 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -2318,10 +2318,12 @@ struct irq_domain *pci_host_bridge_acpi_msi_domain(struct pci_bus *bus); void pci_msi_register_fwnode_provider(struct fwnode_handle *(*fn)(struct device *)); bool pci_pr3_present(struct pci_dev *pdev); +bool pci_acpi_storage_d3(struct pci_dev *dev); #else static inline struct irq_domain * pci_host_bridge_acpi_msi_domain(struct pci_bus *bus) { return NULL; } static inline bool pci_pr3_present(struct pci_dev *pdev) { return false; } +static inline bool pci_acpi_storage_d3(struct pci_dev *dev) { return false; } #endif #ifdef CONFIG_EEH -- 2.20.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support 2020-06-12 20:48 ` [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support David E. Box @ 2020-06-24 21:15 ` Bjorn Helgaas 2020-06-25 11:30 ` Rafael J. Wysocki 2020-06-24 21:37 ` Bjorn Helgaas 1 sibling, 1 reply; 42+ messages in thread From: Bjorn Helgaas @ 2020-06-24 21:15 UTC (permalink / raw) To: David E. Box Cc: shyjumon.n, rjw, lenb, bhelgaas, dan.j.williams, kbusch, axboe, hch, sagi, linux-acpi, linux-nvme, linux-kernel, linux-pci On Fri, Jun 12, 2020 at 01:48:19PM -0700, David E. Box wrote: > StorageD3Enable is a boolean property that indicates that the platform > wants to use D3 for PCIe storage drives during suspend-to-idle. It is a > BIOS work around that is currently in use on shipping systems like some > Intel Comet Lake platforms. It is meant to change default driver policy for > suspend that may cause higher power consumption. > > Add the DSD property for recognition by fwnode calls and provide an > exported symbol for device drivers to use to read the property as needed. > > 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> > --- > drivers/acpi/property.c | 3 +++ > drivers/pci/pci-acpi.c | 59 +++++++++++++++++++++++++++++++++++++++++ > include/linux/pci.h | 2 ++ > 3 files changed, 64 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/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index d21969fba6ab..732df524e09c 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -972,6 +972,65 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev) > return val == 1; > } > > +/** > + * pci_acpi_storage_d3 - whether root port requests D3 for idle suspend > + * @pdev: PCI device to check > + * > + * Returns true if the ACPI companion device contains the "StorageD3Enable" > + * _DSD property and the value is 1. This indicates that the root port is > + * used by a storage device and the platform is requesting D3 for the > + * device during suspend to idle in order to support platform pm. > + */ > +bool pci_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; > + bool ret = false; > + 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 = pci_find_pcie_root_port(dev); I think this would need to be updated to apply to v5.8-rc1 after 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and pci_find_pcie_root_port()"). https://git.kernel.org/linus/6ae72bfa656e > + if (!root) > + return false; > + > + adev = ACPI_COMPANION(&root->dev); > + if (!adev) { > + /* > + * It is possible that the ACPI companion is not yet bound > + * for the root port so look it up manually here. > + */ > + if (!adev && !pci_dev_is_added(root)) > + adev = acpi_pci_find_companion(&root->dev); I see that you copied this "ACPI companion not yet bound" thing from acpi_pci_bridge_d3(). But it's ugly. Isn't there a way we can bind the ACPI companion during normal PCI enumeration so we don't need this exception case? I really do not like the idea of putting this code in the PCI core because AFAICT the PCI core can do nothing with this information. If we could make sure during enumeration that the root port always has an ACPI companion, this code could go to the nvme driver itself. And we could also clean up the ugliness in acpi_pci_bridge_d3(). Rafael, is that possible? I don't really know how the companion device gets set. Maybe this is could be done somewhere around pci_device_add()? > + } > + > + if (!adev) > + return false; > + > + status = acpi_get_handle(adev->handle, "PXSX", &handle); > + if (ACPI_FAILURE(status)) > + return false; > + > + adev = acpi_bus_get_acpi_device(handle); > + if (!adev) > + return false; > + > + fwnode = acpi_fwnode_handle(adev); > + if (!fwnode_property_read_u8(fwnode, "StorageD3Enable", &val)) > + ret = (val == 1); > + > + acpi_bus_put_acpi_device(adev); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(pci_acpi_storage_d3); > + > static bool acpi_pci_power_manageable(struct pci_dev *dev) > { > struct acpi_device *adev = ACPI_COMPANION(&dev->dev); > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 83ce1cdf5676..396fcb269a60 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -2318,10 +2318,12 @@ struct irq_domain *pci_host_bridge_acpi_msi_domain(struct pci_bus *bus); > void > pci_msi_register_fwnode_provider(struct fwnode_handle *(*fn)(struct device *)); > bool pci_pr3_present(struct pci_dev *pdev); > +bool pci_acpi_storage_d3(struct pci_dev *dev); > #else > static inline struct irq_domain * > pci_host_bridge_acpi_msi_domain(struct pci_bus *bus) { return NULL; } > static inline bool pci_pr3_present(struct pci_dev *pdev) { return false; } > +static inline bool pci_acpi_storage_d3(struct pci_dev *dev) { return false; } > #endif > > #ifdef CONFIG_EEH > -- > 2.20.1 > > > _______________________________________________ > linux-nvme mailing list > linux-nvme@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support 2020-06-24 21:15 ` Bjorn Helgaas @ 2020-06-25 11:30 ` Rafael J. Wysocki 2020-06-25 12:16 ` Mika Westerberg ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: Rafael J. Wysocki @ 2020-06-25 11:30 UTC (permalink / raw) To: Bjorn Helgaas, Mika Westerberg Cc: David E. Box, shyjumon.n, Rafael J. Wysocki, Len Brown, Bjorn Helgaas, Dan Williams, Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, ACPI Devel Maling List, linux-nvme, Linux Kernel Mailing List, Linux PCI On Wed, Jun 24, 2020 at 11:15 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Jun 12, 2020 at 01:48:19PM -0700, David E. Box wrote: > > StorageD3Enable is a boolean property that indicates that the platform > > wants to use D3 for PCIe storage drives during suspend-to-idle. It is a > > BIOS work around that is currently in use on shipping systems like some > > Intel Comet Lake platforms. It is meant to change default driver policy for > > suspend that may cause higher power consumption. > > > > Add the DSD property for recognition by fwnode calls and provide an > > exported symbol for device drivers to use to read the property as needed. > > > > 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> > > --- > > drivers/acpi/property.c | 3 +++ > > drivers/pci/pci-acpi.c | 59 +++++++++++++++++++++++++++++++++++++++++ > > include/linux/pci.h | 2 ++ > > 3 files changed, 64 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/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > > index d21969fba6ab..732df524e09c 100644 > > --- a/drivers/pci/pci-acpi.c > > +++ b/drivers/pci/pci-acpi.c > > @@ -972,6 +972,65 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev) > > return val == 1; > > } > > > > +/** > > + * pci_acpi_storage_d3 - whether root port requests D3 for idle suspend > > + * @pdev: PCI device to check > > + * > > + * Returns true if the ACPI companion device contains the "StorageD3Enable" > > + * _DSD property and the value is 1. This indicates that the root port is > > + * used by a storage device and the platform is requesting D3 for the > > + * device during suspend to idle in order to support platform pm. > > + */ > > +bool pci_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; > > + bool ret = false; > > + 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 = pci_find_pcie_root_port(dev); > > I think this would need to be updated to apply to v5.8-rc1 after > 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and > pci_find_pcie_root_port()"). > > https://git.kernel.org/linus/6ae72bfa656e > > > + if (!root) > > + return false; > > + > > + adev = ACPI_COMPANION(&root->dev); > > + if (!adev) { > > + /* > > + * It is possible that the ACPI companion is not yet bound > > + * for the root port so look it up manually here. > > + */ > > + if (!adev && !pci_dev_is_added(root)) > > + adev = acpi_pci_find_companion(&root->dev); > > I see that you copied this "ACPI companion not yet bound" thing from > acpi_pci_bridge_d3(). But it's ugly. > > Isn't there a way we can bind the ACPI companion during normal PCI > enumeration so we don't need this exception case? > > I really do not like the idea of putting this code in the PCI core > because AFAICT the PCI core can do nothing with this information. > > If we could make sure during enumeration that the root port always has > an ACPI companion, this code could go to the nvme driver itself. And > we could also clean up the ugliness in acpi_pci_bridge_d3(). > > Rafael, is that possible? I don't really know how the companion > device gets set. That's a bit convoluted. device_add() calls device_platform_notify(), before calling bus_add_device(). device_platform_notify() calls acpi_platform_notify() which invokes acpi_device_notify() that looks for the companion via type->find_companion() which for PCI points to acpi_pci_find_companion(). If found, the companion is attached to the dev structure as a physical_node, via acpi_bind_one(). So by the time bus_probe_device() runs, the companion should be there already - if it is there at all. The parent ACPI companion should be present when the child is probing too, as per the above. > Maybe this is could be done somewhere around pci_device_add()? It is done in there. It is not necessary to call acpi_pci_find_companion() from pci_acpi_storage_d3() as long as that function is required to be called by the target device's driver probe or later. Ths acpi_pci_bridge_d3() case is different, though, AFAICS, because it is invoked in the pci_pm_init() path, via pci_bridge_d3_possible(), and that gets called from pci_device_add() *before* calling device_add(). Mika, is that why acpi_pci_find_companion() gets callled from acpi_pci_bridge_d3()? ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support 2020-06-25 11:30 ` Rafael J. Wysocki @ 2020-06-25 12:16 ` Mika Westerberg 2020-06-25 17:07 ` David E. Box 2020-06-25 17:30 ` Bjorn Helgaas 2 siblings, 0 replies; 42+ messages in thread From: Mika Westerberg @ 2020-06-25 12:16 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, David E. Box, shyjumon.n, Rafael J. Wysocki, Len Brown, Bjorn Helgaas, Dan Williams, Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, ACPI Devel Maling List, linux-nvme, Linux Kernel Mailing List, Linux PCI On Thu, Jun 25, 2020 at 01:30:53PM +0200, Rafael J. Wysocki wrote: > It is not necessary to call acpi_pci_find_companion() from > pci_acpi_storage_d3() as long as that function is required to be > called by the target device's driver probe or later. > > Ths acpi_pci_bridge_d3() case is different, though, AFAICS, because it > is invoked in the pci_pm_init() path, via pci_bridge_d3_possible(), > and that gets called from pci_device_add() *before* calling > device_add(). > > Mika, is that why acpi_pci_find_companion() gets callled from > acpi_pci_bridge_d3()? Yes, that's correct. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support 2020-06-25 11:30 ` Rafael J. Wysocki 2020-06-25 12:16 ` Mika Westerberg @ 2020-06-25 17:07 ` David E. Box 2020-06-25 17:30 ` Bjorn Helgaas 2 siblings, 0 replies; 42+ messages in thread From: David E. Box @ 2020-06-25 17:07 UTC (permalink / raw) To: Rafael J. Wysocki, Bjorn Helgaas, Mika Westerberg Cc: shyjumon.n, Rafael J. Wysocki, Len Brown, Bjorn Helgaas, Dan Williams, Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, ACPI Devel Maling List, linux-nvme, Linux Kernel Mailing List, Linux PCI On Thu, 2020-06-25 at 13:30 +0200, Rafael J. Wysocki wrote: > On Wed, Jun 24, 2020 at 11:15 PM Bjorn Helgaas <helgaas@kernel.org> > wrote: > > On Fri, Jun 12, 2020 at 01:48:19PM -0700, David E. Box wrote: > > > StorageD3Enable is a boolean property that indicates that the > > > platform > > > wants to use D3 for PCIe storage drives during suspend-to-idle. > > > It is a > > > BIOS work around that is currently in use on shipping systems > > > like some > > > Intel Comet Lake platforms. It is meant to change default driver > > > policy for > > > suspend that may cause higher power consumption. > > > > > > Add the DSD property for recognition by fwnode calls and provide > > > an > > > exported symbol for device drivers to use to read the property as > > > needed. > > > > > > 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> > > > --- > > > drivers/acpi/property.c | 3 +++ > > > drivers/pci/pci-acpi.c | 59 > > > +++++++++++++++++++++++++++++++++++++++++ > > > include/linux/pci.h | 2 ++ > > > 3 files changed, 64 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/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > > > index d21969fba6ab..732df524e09c 100644 > > > --- a/drivers/pci/pci-acpi.c > > > +++ b/drivers/pci/pci-acpi.c > > > @@ -972,6 +972,65 @@ static bool acpi_pci_bridge_d3(struct > > > pci_dev *dev) > > > return val == 1; > > > } > > > > > > +/** > > > + * pci_acpi_storage_d3 - whether root port requests D3 for idle > > > suspend > > > + * @pdev: PCI device to check > > > + * > > > + * Returns true if the ACPI companion device contains the > > > "StorageD3Enable" > > > + * _DSD property and the value is 1. This indicates that the > > > root port is > > > + * used by a storage device and the platform is requesting D3 > > > for the > > > + * device during suspend to idle in order to support platform > > > pm. > > > + */ > > > +bool pci_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; > > > + bool ret = false; > > > + 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 = pci_find_pcie_root_port(dev); > > > > I think this would need to be updated to apply to v5.8-rc1 after > > 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and > > pci_find_pcie_root_port()"). > > > > https://git.kernel.org/linus/6ae72bfa656e > > > > > + if (!root) > > > + return false; > > > + > > > + adev = ACPI_COMPANION(&root->dev); > > > + if (!adev) { > > > + /* > > > + * It is possible that the ACPI companion is not > > > yet bound > > > + * for the root port so look it up manually here. > > > + */ > > > + if (!adev && !pci_dev_is_added(root)) > > > + adev = acpi_pci_find_companion(&root->dev); > > > > I see that you copied this "ACPI companion not yet bound" thing > > from > > acpi_pci_bridge_d3(). But it's ugly. > > > > Isn't there a way we can bind the ACPI companion during normal PCI > > enumeration so we don't need this exception case? > > > > I really do not like the idea of putting this code in the PCI core > > because AFAICT the PCI core can do nothing with this information. > > > > If we could make sure during enumeration that the root port always > > has > > an ACPI companion, this code could go to the nvme driver itself. This is fine with me if it's acceptable with the nvme maintainers. > > And > > we could also clean up the ugliness in acpi_pci_bridge_d3(). > > > > Rafael, is that possible? I don't really know how the companion > > device gets set. > > That's a bit convoluted. > > device_add() calls device_platform_notify(), before calling > bus_add_device(). > > device_platform_notify() calls acpi_platform_notify() which invokes > acpi_device_notify() that looks for the companion via > type->find_companion() which for PCI points to > acpi_pci_find_companion(). If found, the companion is attached to > the > dev structure as a physical_node, via acpi_bind_one(). > > So by the time bus_probe_device() runs, the companion should be there > already - if it is there at all. > > The parent ACPI companion should be present when the child is probing > too, as per the above. > > > Maybe this is could be done somewhere around pci_device_add()? > > It is done in there. > > It is not necessary to call acpi_pci_find_companion() from > pci_acpi_storage_d3() as long as that function is required to be > called by the target device's driver probe or later. Thanks for the clarification. Since this would get called at driver probe this bit of code is not needed. David ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support 2020-06-25 11:30 ` Rafael J. Wysocki 2020-06-25 12:16 ` Mika Westerberg 2020-06-25 17:07 ` David E. Box @ 2020-06-25 17:30 ` Bjorn Helgaas 2 siblings, 0 replies; 42+ messages in thread From: Bjorn Helgaas @ 2020-06-25 17:30 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Mika Westerberg, David E. Box, shyjumon.n, Rafael J. Wysocki, Len Brown, Bjorn Helgaas, Dan Williams, Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, ACPI Devel Maling List, linux-nvme, Linux Kernel Mailing List, Linux PCI On Thu, Jun 25, 2020 at 01:30:53PM +0200, Rafael J. Wysocki wrote: > On Wed, Jun 24, 2020 at 11:15 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Jun 12, 2020 at 01:48:19PM -0700, David E. Box wrote: > > > StorageD3Enable is a boolean property that indicates that the > > > platform wants to use D3 for PCIe storage drives during > > > suspend-to-idle. It is a BIOS work around that is currently in > > > use on shipping systems like some Intel Comet Lake platforms. It > > > is meant to change default driver policy for suspend that may > > > cause higher power consumption. > > > +/** > > > + * pci_acpi_storage_d3 - whether root port requests D3 for idle suspend > > > + * @pdev: PCI device to check > > > + * > > > + * Returns true if the ACPI companion device contains the "StorageD3Enable" > > > + * _DSD property and the value is 1. This indicates that the root port is > > > + * used by a storage device and the platform is requesting D3 for the > > > + * device during suspend to idle in order to support platform pm. > > > + */ > > > +bool pci_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; > > > + bool ret = false; > > > + 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 = pci_find_pcie_root_port(dev); > > > > I think this would need to be updated to apply to v5.8-rc1 after > > 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and > > pci_find_pcie_root_port()"). > > > > https://git.kernel.org/linus/6ae72bfa656e > > > > > + if (!root) > > > + return false; > > > + > > > + adev = ACPI_COMPANION(&root->dev); > > > + if (!adev) { > > > + /* > > > + * It is possible that the ACPI companion is not yet bound > > > + * for the root port so look it up manually here. > > > + */ > > > + if (!adev && !pci_dev_is_added(root)) > > > + adev = acpi_pci_find_companion(&root->dev); > > > > I see that you copied this "ACPI companion not yet bound" thing from > > acpi_pci_bridge_d3(). But it's ugly. > > > > Isn't there a way we can bind the ACPI companion during normal PCI > > enumeration so we don't need this exception case? > > > > I really do not like the idea of putting this code in the PCI core > > because AFAICT the PCI core can do nothing with this information. > > > > If we could make sure during enumeration that the root port always has > > an ACPI companion, this code could go to the nvme driver itself. And > > we could also clean up the ugliness in acpi_pci_bridge_d3(). > > > > Rafael, is that possible? I don't really know how the companion > > device gets set. > > That's a bit convoluted. > > device_add() calls device_platform_notify(), before calling bus_add_device(). > > device_platform_notify() calls acpi_platform_notify() which invokes > acpi_device_notify() that looks for the companion via > type->find_companion() which for PCI points to > acpi_pci_find_companion(). If found, the companion is attached to the > dev structure as a physical_node, via acpi_bind_one(). > > So by the time bus_probe_device() runs, the companion should be there > already - if it is there at all. > > The parent ACPI companion should be present when the child is probing > too, as per the above. > > > Maybe this is could be done somewhere around pci_device_add()? > > It is done in there. > > It is not necessary to call acpi_pci_find_companion() from > pci_acpi_storage_d3() as long as that function is required to be > called by the target device's driver probe or later. OK, great. IIUC, that means this function doesn't need to be in drivers/pci and it could be moved to the NVMe code. > Ths acpi_pci_bridge_d3() case is different, though, AFAICS, because it > is invoked in the pci_pm_init() path, via pci_bridge_d3_possible(), > and that gets called from pci_device_add() *before* calling > device_add(). > > Mika, is that why acpi_pci_find_companion() gets called from > acpi_pci_bridge_d3()? Is pdev->bridge_d3 really needed before pci_device_add()? It would be really nice if there were a way to get rid of that manual lookup of the companion device in acpi_pci_bridge_d3(). Bjorn ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support 2020-06-12 20:48 ` [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support David E. Box 2020-06-24 21:15 ` Bjorn Helgaas @ 2020-06-24 21:37 ` Bjorn Helgaas 2020-06-24 22:09 ` David E. Box 1 sibling, 1 reply; 42+ messages in thread From: Bjorn Helgaas @ 2020-06-24 21:37 UTC (permalink / raw) To: David E. Box Cc: shyjumon.n, rjw, lenb, bhelgaas, dan.j.williams, kbusch, axboe, hch, sagi, linux-acpi, linux-nvme, linux-kernel, linux-pci On Fri, Jun 12, 2020 at 01:48:19PM -0700, David E. Box wrote: > StorageD3Enable is a boolean property that indicates that the platform > wants to use D3 for PCIe storage drives during suspend-to-idle. Is this something that should apply to plug-in drives, or does this only apply to soldered-in things? > It is a > BIOS work around that is currently in use on shipping systems like some > Intel Comet Lake platforms. What is this BIOS work around? Is there a defect here that's being worked around? What's the defect? > It is meant to change default driver policy for > suspend that may cause higher power consumption. I guess this means that by changing the driver policy from the default, we can save some power? > Add the DSD property for recognition by fwnode calls and provide an > exported symbol for device drivers to use to read the property as needed. > > Link: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro There is surprisingly little information in this intro. The whole paragraph under "Modern Standby Power Management" is duplicated immediately below in "D3 Support". Maybe that's a copyediting error that displaced useful information. It says "drivers should go to the deepest appropriate state" so "function drivers don't have to manage implementation details". No doubt "drivers" and "function drivers" is a meaningful distinction to Windows cognoscenti, but it's not to me. It talks about "enabling D3" without specifying D3hot or D3cold. It talks about "D3 support for storage devices." All PCI devices are required to support both D3hot and D3cold, so this must be talking about some other sort of support; I suppose maybe it's a hint about whether a driver should *use* D3hot (or D3cold, I can't tell). It says nothing about where to look for the _DSD: on a Root Port or on the NVMe endpoint. Bjorn ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support 2020-06-24 21:37 ` Bjorn Helgaas @ 2020-06-24 22:09 ` David E. Box 0 siblings, 0 replies; 42+ messages in thread From: David E. Box @ 2020-06-24 22:09 UTC (permalink / raw) To: Bjorn Helgaas Cc: shyjumon.n, rjw, lenb, bhelgaas, dan.j.williams, kbusch, axboe, hch, sagi, linux-acpi, linux-nvme, linux-kernel, linux-pci On Wed, 2020-06-24 at 16:37 -0500, Bjorn Helgaas wrote: > On Fri, Jun 12, 2020 at 01:48:19PM -0700, David E. Box wrote: > > StorageD3Enable is a boolean property that indicates that the > > platform > > wants to use D3 for PCIe storage drives during suspend-to-idle. > > Is this something that should apply to plug-in drives, or does this > only apply to soldered-in things? > > > It is a > > BIOS work around that is currently in use on shipping systems like > > some > > Intel Comet Lake platforms. > > What is this BIOS work around? Is there a defect here that's being > worked around? What's the defect? > > > It is meant to change default driver policy for > > suspend that may cause higher power consumption. > > I guess this means that by changing the driver policy from the > default, we can save some power? Yes. Maybe 'work around' was a poor choice of words. 'Getting around default driver policy' is the issue. There is no hardware defect. One of the uses of the suspend-to-idle flow is to support compliance with increasingly tighter energy regulations. One of the ways to do this on desktop systems is to power off the ATX power supply during s2idle and use the 5V standby rail for self refresh and other low power needs. But the platforms that support this can't shutdown the PS unless PCI ports are placed in D3. On Linux this won't happen with NVMe drives because the default driver policy is to use ASPM (NVMe APST) during s2idle. Windows has a related concern. So to 'get around' the driver choosing a policy that will result in higher power consumption, they implemented this _DSD to inform the OS of its preference for D3 on the PCI port. David ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH V2 2/2] drivers/nvme: Add support for ACPI StorageD3Enable property 2020-04-28 0:32 [PATCH 0/2] Add support for StorageD3Enable _DSD property David E. Box ` (4 preceding siblings ...) 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 5 siblings, 0 replies; 42+ messages in thread From: David E. Box @ 2020-06-12 20:48 UTC (permalink / raw) To: shyjumon.n, rjw, lenb, bhelgaas, dan.j.williams, kbusch, axboe, hch, sagi Cc: David E. Box, linux-acpi, linux-kernel, linux-pci, linux-nvme This patch implements a solution for a BIOS hack used on some currently shipping Intel systems to address issues with power management policy decisions concerning 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 override 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 would lead to higher power consumption for these platforms. The patch uses the previously added pci_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> Reviewed-by: Dan Williams <dan.j.williams@intel.com> --- drivers/nvme/host/pci.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 4e79e412b276..427505c47e79 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -78,6 +78,10 @@ static unsigned int poll_queues; module_param(poll_queues, uint, 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 all acpi bios quirks"); + struct nvme_dev; struct nvme_queue; @@ -2777,6 +2781,16 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id) quirks |= check_vendor_combination_bug(pdev); + if (!noacpi && pci_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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH V5] drivers/nvme: Add support for ACPI StorageD3Enable property @ 2021-05-26 17:53 Raul Rangel 2021-05-27 2:20 ` David E. Box 0 siblings, 1 reply; 42+ messages in thread From: Raul Rangel @ 2021-05-26 17:53 UTC (permalink / raw) To: David E. Box Cc: Rafael J. Wysocki, Len Brown, kbusch, axboe, hch, sagi, dan.j.williams, shyjumon.n, linux-acpi, linux-kernel, linux-nvme On Thu, Jul 09, 2020 at 11:43:33AM -0700, David E. Box wrote: > +#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); So I'm curious why we need to directly look at the PXSX and PEGP devices instead of the ACPI_COMPANION node attached to the pci device? I've looked around and I can't find any documentation that defines the PXSX and PEGP device names. I've dumped some ACPI from a system that uses the PXSX name and StorageD3Cold attribute: Scope (\_SB.PCI0.GP14) { Device (PXSX) { Name (_ADR, 0x0000000000000000) // _ADR: Address Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) } Name (_DSD, Package (0x02) // _DSD: Device-Specific Data { ToUUID ("5025030f-842f-4ab4-a561-99a5189762d0"), Package (0x01) { Package (0x02) { "StorageD3Enable", One } } }) } } It looks to me like it's just the firmware node for the NVMe device attached to the root port. Is that the correct assumption? I'm wondering if we can simplify the look up logic to look at the ACPI_COMPANION of the pci device? The reason I ask is that I'm working on enabling S0i3 on an AMD device. This device also defines the StorageD3Enable property, but it don't use the PXSX name: Scope (GPP6) { Device (NVME) { Name (_ADR, Zero) // _ADR: Address Name (_DSD, Package (0x02) // _DSD: Device-Specific Data { ToUUID ("5025030f-842f-4ab4-a561-99a5189762d0"), Package (0x01) { Package (0x02) { "StorageD3Enable", One } } }) } } The Windows [documentation](https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro#d3-support) makes it sound like the _DSD should be defined on the PCI device. I can send one of the following patches depending on the feedback: 1) Additionally check the pci device's ACPI_COMPANION for the _DSD. 2) Delete the PXSX and PEGP lookups and only look at the pci device's ACPI_COMPANION. > + 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; > +} Thanks, Raul ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V5] drivers/nvme: Add support for ACPI StorageD3Enable property 2021-05-26 17:53 [PATCH V5] " Raul Rangel @ 2021-05-27 2:20 ` David E. Box 0 siblings, 0 replies; 42+ messages in thread From: David E. Box @ 2021-05-27 2:20 UTC (permalink / raw) To: Raul Rangel, michael.a.bottini Cc: Rafael J. Wysocki, Len Brown, kbusch, axboe, hch, sagi, dan.j.williams, shyjumon.n, linux-acpi, linux-kernel, linux-nvme Hi Raul, On Wed, 2021-05-26 at 11:53 -0600, Raul Rangel wrote: > On Thu, Jul 09, 2020 at 11:43:33AM -0700, David E. Box wrote: > > +#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); > So I'm curious why we need to directly look at the PXSX and PEGP > devices instead of the ACPI_COMPANION node attached to the pci > device? > > I've looked around and I can't find any documentation that defines > the PXSX and PEGP device names. > > I've dumped some ACPI from a system that uses the PXSX name and > StorageD3Cold attribute: > > Scope (\_SB.PCI0.GP14) > { > Device (PXSX) > { > Name (_ADR, 0x0000000000000000) // _ADR: Address > Method (_STA, 0, NotSerialized) // _STA: Status > { > Return (0x0F) > } > > Name (_DSD, Package (0x02) // _DSD: Device-Specific Data > { > ToUUID ("5025030f-842f-4ab4-a561-99a5189762d0"), > Package (0x01) > { > Package (0x02) > { > "StorageD3Enable", > One > } > } > }) > } > } > > It looks to me like it's just the firmware node for the NVMe device > attached to the root port. Is that the correct assumption? > > I'm wondering if we can simplify the look up logic to look at the > ACPI_COMPANION of the pci device? I believe so, but I'd need to confirm on our systems that it will work. I recall trying to use the companion device and not being able to locate the _DSD. But that was on a preproduction platform at the time. > > The reason I ask is that I'm working on enabling S0i3 on an AMD > device. > This device also defines the StorageD3Enable property, but it don't > use > the PXSX name: > > Scope (GPP6) { > Device (NVME) > { > Name (_ADR, Zero) // _ADR: Address > > Name (_DSD, Package (0x02) // _DSD: Device-Specific Data > { > ToUUID ("5025030f-842f-4ab4-a561-99a5189762d0"), > Package (0x01) > { > Package (0x02) > { > "StorageD3Enable", > One > } > } > }) > } > } > > The Windows > [documentation]( > https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro#d3-support > ) > makes it sound like the _DSD should be defined on the PCI device. > > I can send one of the following patches depending on the feedback: > 1) Additionally check the pci device's ACPI_COMPANION for the _DSD. > 2) Delete the PXSX and PEGP lookups and only look at the pci device's > ACPI_COMPANION. > > > + 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; > > +} Go for 2 first. I will check on those systems again with our latest BIOS to ensure it works. David > > Thanks, > Raul ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2021-05-27 2:20 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-28 0:32 [PATCH 0/2] Add support for StorageD3Enable _DSD property David E. Box 2020-04-28 0:32 ` [PATCH 1/2] pci: Add ACPI StorageD3Enable _DSD support David E. Box 2020-05-18 12:34 ` Rafael J. Wysocki 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 5:13 ` [PATCH 0/2] Add support for StorageD3Enable _DSD property Christoph Hellwig 2020-04-28 14:09 ` David E. Box 2020-04-28 14:22 ` Christoph Hellwig 2020-04-28 15:27 ` David E. Box 2020-04-29 5:20 ` Williams, Dan J 2020-04-29 15:10 ` Keith Busch 2020-04-29 16:11 ` David E. Box 2020-05-01 13:12 ` hch 2020-05-01 15:54 ` David E. Box 2020-05-01 13:10 ` hch 2020-05-18 13:51 ` David Woodhouse 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-24 18:55 ` David E. Box 2020-06-24 19:10 ` Dan Williams 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-03 7:18 ` kernel test robot 2020-07-06 14:57 ` Rafael J. Wysocki 2020-07-07 21:24 ` David E. Box 2020-07-07 7:09 ` Christoph Hellwig 2020-07-09 18:43 ` [PATCH V5] " David E. Box 2020-07-13 11:12 ` Rafael J. Wysocki 2020-07-16 14:39 ` Christoph Hellwig 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-24 21:15 ` Bjorn Helgaas 2020-06-25 11:30 ` Rafael J. Wysocki 2020-06-25 12:16 ` Mika Westerberg 2020-06-25 17:07 ` David E. Box 2020-06-25 17:30 ` Bjorn Helgaas 2020-06-24 21:37 ` Bjorn Helgaas 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 2021-05-26 17:53 [PATCH V5] " Raul Rangel 2021-05-27 2:20 ` David E. Box
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).