From: Bjorn Helgaas <helgaas@kernel.org> To: "David E. Box" <david.e.box@linux.intel.com> Cc: shyjumon.n@intel.com, rjw@rjwysocki.net, lenb@kernel.org, bhelgaas@google.com, dan.j.williams@intel.com, kbusch@kernel.org, axboe@fb.com, hch@lst.de, sagi@grimberg.me, linux-acpi@vger.kernel.org, linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org Subject: Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support Date: Wed, 24 Jun 2020 16:15:49 -0500 [thread overview] Message-ID: <20200624211549.GA2586552@bjorn-Precision-5520> (raw) In-Reply-To: <20200612204820.20111-2-david.e.box@linux.intel.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Bjorn Helgaas <helgaas@kernel.org> To: "David E. Box" <david.e.box@linux.intel.com> Cc: axboe@fb.com, sagi@grimberg.me, linux-acpi@vger.kernel.org, linux-pci@vger.kernel.org, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org, shyjumon.n@intel.com, kbusch@kernel.org, bhelgaas@google.com, dan.j.williams@intel.com, hch@lst.de, lenb@kernel.org Subject: Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support Date: Wed, 24 Jun 2020 16:15:49 -0500 [thread overview] Message-ID: <20200624211549.GA2586552@bjorn-Precision-5520> (raw) In-Reply-To: <20200612204820.20111-2-david.e.box@linux.intel.com> 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 _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
next prev parent reply other threads:[~2020-06-24 21:15 UTC|newest] Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-28 0:32 [PATCH 0/2] Add support for StorageD3Enable _DSD property David E. Box 2020-04-28 0:32 ` David E. Box 2020-04-28 0:32 ` [PATCH 1/2] pci: Add ACPI StorageD3Enable _DSD support David E. Box 2020-04-28 0:32 ` David E. Box 2020-05-18 12:34 ` Rafael J. Wysocki 2020-05-18 12:34 ` Rafael J. Wysocki 2020-05-19 17:10 ` David E. Box 2020-05-19 17:10 ` David E. Box 2020-04-28 0:32 ` [PATCH 2/2] drivers/nvme: Add support for ACPI StorageD3Enable property David E. Box 2020-04-28 0:32 ` David E. Box 2020-04-28 5:13 ` [PATCH 0/2] Add support for StorageD3Enable _DSD property Christoph Hellwig 2020-04-28 5:13 ` Christoph Hellwig 2020-04-28 14:09 ` David E. Box 2020-04-28 14:09 ` David E. Box 2020-04-28 14:22 ` Christoph Hellwig 2020-04-28 14:22 ` Christoph Hellwig 2020-04-28 15:27 ` David E. Box 2020-04-28 15:27 ` David E. Box 2020-04-29 5:20 ` Williams, Dan J 2020-04-29 5:20 ` Williams, Dan J 2020-04-29 15:10 ` Keith Busch 2020-04-29 15:10 ` Keith Busch 2020-04-29 16:11 ` David E. Box 2020-04-29 16:11 ` David E. Box 2020-05-01 13:12 ` hch 2020-05-01 13:12 ` hch 2020-05-01 15:54 ` David E. Box 2020-05-01 15:54 ` David E. Box 2020-05-01 13:10 ` hch 2020-05-01 13:10 ` hch 2020-05-18 13:51 ` David Woodhouse 2020-05-18 13:51 ` David Woodhouse 2020-05-18 17:20 ` Dan Williams 2020-05-18 17:20 ` Dan Williams 2020-06-12 20:48 ` [PATCH V2 0/2] nvme: Add support for ACPI StorageD3Enable property David E. Box 2020-06-12 20:48 ` David E. Box 2020-06-24 18:55 ` David E. Box 2020-06-24 18:55 ` David E. Box 2020-06-24 19:10 ` Dan Williams 2020-06-24 19:10 ` Dan Williams 2020-06-24 19:39 ` David E. Box 2020-06-24 19:39 ` David E. Box 2020-07-02 21:04 ` [PATCH v3] drivers/nvme: " David E. Box 2020-07-02 22:50 ` [PATCH v4] " David E. Box 2020-07-02 22:50 ` David E. Box 2020-07-03 7:18 ` kernel test robot 2020-07-03 7:18 ` kernel test robot 2020-07-06 14:57 ` Rafael J. Wysocki 2020-07-06 14:57 ` Rafael J. Wysocki 2020-07-07 21:24 ` David E. Box 2020-07-07 21:24 ` David E. Box 2020-07-07 7:09 ` Christoph Hellwig 2020-07-07 7:09 ` Christoph Hellwig 2020-07-09 18:43 ` [PATCH V5] " David E. Box 2020-07-09 18:43 ` David E. Box 2020-07-13 11:12 ` Rafael J. Wysocki 2020-07-13 11:12 ` Rafael J. Wysocki 2020-07-16 14:39 ` Christoph Hellwig 2020-07-16 14:39 ` Christoph Hellwig 2021-05-26 19:25 ` Raul E Rangel 2021-05-26 19:25 ` Raul E Rangel 2020-06-12 20:48 ` [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support David E. Box 2020-06-12 20:48 ` David E. Box 2020-06-24 21:15 ` Bjorn Helgaas [this message] 2020-06-24 21:15 ` Bjorn Helgaas 2020-06-25 11:30 ` Rafael J. Wysocki 2020-06-25 11:30 ` Rafael J. Wysocki 2020-06-25 12:16 ` Mika Westerberg 2020-06-25 12:16 ` Mika Westerberg 2020-06-25 17:07 ` David E. Box 2020-06-25 17:30 ` Bjorn Helgaas 2020-06-25 17:30 ` Bjorn Helgaas 2020-06-24 21:37 ` Bjorn Helgaas 2020-06-24 21:37 ` Bjorn Helgaas 2020-06-24 22:09 ` David E. Box 2020-06-24 22:09 ` David E. Box 2020-06-12 20:48 ` [PATCH V2 2/2] drivers/nvme: Add support for ACPI StorageD3Enable property David E. Box 2020-06-12 20:48 ` David E. Box
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200624211549.GA2586552@bjorn-Precision-5520 \ --to=helgaas@kernel.org \ --cc=axboe@fb.com \ --cc=bhelgaas@google.com \ --cc=dan.j.williams@intel.com \ --cc=david.e.box@linux.intel.com \ --cc=hch@lst.de \ --cc=kbusch@kernel.org \ --cc=lenb@kernel.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nvme@lists.infradead.org \ --cc=linux-pci@vger.kernel.org \ --cc=rjw@rjwysocki.net \ --cc=sagi@grimberg.me \ --cc=shyjumon.n@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.