* [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead @ 2021-05-27 13:59 Mario Limonciello 2021-05-27 14:35 ` Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Mario Limonciello @ 2021-05-27 13:59 UTC (permalink / raw) To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg Cc: open list:NVM EXPRESS DRIVER, Mario Limonciello, Prike Liang, rrangel, david.e.box, Shyam-sundar S-k, Alexander Deucher The documentation around the StorageD3Enable property hints that it should be made on the PCI device. This is where AMD systems set the property and it's required for S0i3 support. So rather than look for nodes of the root port only present on Intel systems, switch to the companion ACPI device for all systems. David Box from Intel indicated this should work on Intel as well. Link: https://lore.kernel.org/linux-nvme/YK6gmAWqaRmvpJXb@google.com/T/#m900552229fa455867ee29c33b854845fce80ba70 Link: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro Suggested-by: Prike Liang <Prike.Liang@amd.com> CC: rrangel@chromium.org CC: david.e.box@linux.intel.com CC: Shyam-sundar S-k <Shyam-sundar.S-k@amd.com> CC: Alexander Deucher <Alexander.Deucher@amd.com> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/nvme/host/pci.c | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-) Changes from v1 to v2: * Drop the old PXSX/PEGP logic instead of supplement to it * Add references to other discussions on this topic diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index a29b170701fc..d4eef8caa4cc 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2832,9 +2832,6 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev) static bool nvme_acpi_storage_d3(struct pci_dev *dev) { struct acpi_device *adev; - struct pci_dev *root; - acpi_handle handle; - acpi_status status; u8 val; /* @@ -2842,28 +2839,10 @@ static bool nvme_acpi_storage_d3(struct pci_dev *dev) * 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); + adev = ACPI_COMPANION(&dev->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; - if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable", &val)) return false; -- 2.25.1 _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead 2021-05-27 13:59 [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead Mario Limonciello @ 2021-05-27 14:35 ` Christoph Hellwig 2021-05-27 14:44 ` Limonciello, Mario 0 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2021-05-27 14:35 UTC (permalink / raw) To: Mario Limonciello Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, open list:NVM EXPRESS DRIVER, Prike Liang, rrangel, david.e.box, Shyam-sundar S-k, Alexander Deucher Adding Raul who has been asking for something like this as well. I'd also really like to move nvme_acpi_storage_d3 out of the NVMe driver. The Microsoft document that the original document references makes it very clear that this is not NVMe specific, but also covers at least AHCI. On top of that the platform simply can't know what kind of PCIe device is in any given slot. Last but not least this will also allow us to add quirks for devices that fail to properly mark this misfeature in the ACPI tables. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead 2021-05-27 14:35 ` Christoph Hellwig @ 2021-05-27 14:44 ` Limonciello, Mario 2021-05-27 15:52 ` Bjorn Helgaas 2021-05-27 15:53 ` Raul Rangel 0 siblings, 2 replies; 8+ messages in thread From: Limonciello, Mario @ 2021-05-27 14:44 UTC (permalink / raw) To: Christoph Hellwig Cc: Keith Busch, Jens Axboe, Sagi Grimberg, open list:NVM EXPRESS DRIVER, Prike Liang, rrangel, david.e.box, Shyam-sundar S-k, Alexander Deucher, bhelgaas On 5/27/2021 09:35, Christoph Hellwig wrote: > Adding Raul who has been asking for something like this as well. > I thought I already included him on CC. Sorry if I munged the email or something. > I'd also really like to move nvme_acpi_storage_d3 out of the NVMe > driver. The Microsoft document that the original document references > makes it very clear that this is not NVMe specific, but also covers > at least AHCI. On top of that the platform simply can't know what kind > of PCIe device is in any given slot. Last but not least this will also > allow us to add quirks for devices that fail to properly mark this > misfeature in the ACPI tables. > +Bjorn Back when this feature was first submitted, that was actually the initial way that it was done, but Bjorn had preferred to see it move into the NVME driver directly: https://lore.kernel.org/linux-nvme/20200625173053.GA2694537@bjorn-Precision-5520/ Bjorn, are you OK with this coming "back" to PCI based on Christoph's comments? _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead 2021-05-27 14:44 ` Limonciello, Mario @ 2021-05-27 15:52 ` Bjorn Helgaas 2021-05-27 16:58 ` Keith Busch 2021-05-27 15:53 ` Raul Rangel 1 sibling, 1 reply; 8+ messages in thread From: Bjorn Helgaas @ 2021-05-27 15:52 UTC (permalink / raw) To: Limonciello, Mario Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg, open list:NVM EXPRESS DRIVER, Prike Liang, rrangel, David E. Box, Shyam-sundar S-k, Alexander Deucher On Thu, May 27, 2021 at 9:44 AM Limonciello, Mario <mario.limonciello@amd.com> wrote: > On 5/27/2021 09:35, Christoph Hellwig wrote: > > Adding Raul who has been asking for something like this as well. > > > I'd also really like to move nvme_acpi_storage_d3 out of the NVMe > > driver. The Microsoft document that the original document references > > makes it very clear that this is not NVMe specific, but also covers > > at least AHCI. On top of that the platform simply can't know what kind > > of PCIe device is in any given slot. Last but not least this will also > > allow us to add quirks for devices that fail to properly mark this > > misfeature in the ACPI tables. > > +Bjorn > > Back when this feature was first submitted, that was actually the > initial way that it was done, but Bjorn had preferred to see it move > into the NVME driver directly: > > https://lore.kernel.org/linux-nvme/20200625173053.GA2694537@bjorn-Precision-5520/ > > Bjorn, are you OK with this coming "back" to PCI based on Christoph's > comments? My point was that the PCI core can do nothing with this information, so it doesn't seem like putting it in PCI doesn't really gain anything. The Microsoft document you reference ([1]) doesn't mention PCI or PCIe. As far as I know, there's no PCI spec that mentions "StorageD3Enable". I agree that [1] doesn't seem to be NVMe-specific, since it also mentions SATA, so it might make sense to look for "StorageD3Enable" somewhere other than the NVMe driver. I'm just not convinced the PCI core is the best place. I have the impression that it's possible to have non-PCI SATA or AHCI devices (correct me if I'm wrong), and "StorageD3Enable" could apply to them as well. [1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead 2021-05-27 15:52 ` Bjorn Helgaas @ 2021-05-27 16:58 ` Keith Busch 2021-05-27 17:03 ` Limonciello, Mario 2021-05-27 17:28 ` Christoph Hellwig 0 siblings, 2 replies; 8+ messages in thread From: Keith Busch @ 2021-05-27 16:58 UTC (permalink / raw) To: Bjorn Helgaas Cc: Limonciello, Mario, Christoph Hellwig, Jens Axboe, Sagi Grimberg, open list:NVM EXPRESS DRIVER, Prike Liang, rrangel, David E. Box, Shyam-sundar S-k, Alexander Deucher On Thu, May 27, 2021 at 10:52:43AM -0500, Bjorn Helgaas wrote: > On Thu, May 27, 2021 at 9:44 AM Limonciello, Mario > <mario.limonciello@amd.com> wrote: > > On 5/27/2021 09:35, Christoph Hellwig wrote: > > > Adding Raul who has been asking for something like this as well. > > > > > I'd also really like to move nvme_acpi_storage_d3 out of the NVMe > > > driver. The Microsoft document that the original document references > > > makes it very clear that this is not NVMe specific, but also covers > > > at least AHCI. On top of that the platform simply can't know what kind > > > of PCIe device is in any given slot. Last but not least this will also > > > allow us to add quirks for devices that fail to properly mark this > > > misfeature in the ACPI tables. > > > > +Bjorn > > > > Back when this feature was first submitted, that was actually the > > initial way that it was done, but Bjorn had preferred to see it move > > into the NVME driver directly: > > > > https://lore.kernel.org/linux-nvme/20200625173053.GA2694537@bjorn-Precision-5520/ > > > > Bjorn, are you OK with this coming "back" to PCI based on Christoph's > > comments? > > My point was that the PCI core can do nothing with this information, > so it doesn't seem like putting it in PCI doesn't really gain > anything. The Microsoft document you reference ([1]) doesn't mention > PCI or PCIe. As far as I know, there's no PCI spec that mentions > "StorageD3Enable". > > I agree that [1] doesn't seem to be NVMe-specific, since it also > mentions SATA, so it might make sense to look for "StorageD3Enable" > somewhere other than the NVMe driver. I'm just not convinced the PCI > core is the best place. I have the impression that it's possible to > have non-PCI SATA or AHCI devices (correct me if I'm wrong), and > "StorageD3Enable" could apply to them as well. > > [1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro I agree it doesn't appear to belong in PCI either. This should go in ACPI. Here's my proposal on top of this patch: --- diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index d260bc1f3e6e..ab8a0dfae2df 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -1340,4 +1340,25 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on) return 1; } EXPORT_SYMBOL_GPL(acpi_dev_pm_attach); + +bool acpi_storage_d3(struct device *dev) +{ + struct acpi_device *adev; + 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. + */ + adev = ACPI_COMPANION(dev); + if (!adev) + return false; + if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable", + &val)) + return false; + return val == 1; +} +EXPORT_SYMBOL_GPL(acpi_storage_d3); + #endif /* CONFIG_PM */ diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index d4eef8caa4cc..8fbc4c87a0d8 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2828,33 +2828,6 @@ 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) -{ - struct acpi_device *adev; - 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. - */ - - adev = ACPI_COMPANION(&dev->dev); - if (!adev) - return false; - if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable", - &val)) - return false; - return val == 1; -} -#else -static inline bool nvme_acpi_storage_d3(struct pci_dev *dev) -{ - return false; -} -#endif /* CONFIG_ACPI */ - static void nvme_async_probe(void *data, async_cookie_t cookie) { struct nvme_dev *dev = data; @@ -2904,7 +2877,7 @@ 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)) { + if (!noacpi && acpi_storage_d3(&pdev->dev)) { /* * Some systems use a bios work around to ask for D3 on * platforms that support kernel managed suspend. diff --git a/include/linux/acpi.h b/include/linux/acpi.h index c60745f657e9..dd0dafd21e33 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -1004,6 +1004,7 @@ int acpi_dev_resume(struct device *dev); int acpi_subsys_runtime_suspend(struct device *dev); int acpi_subsys_runtime_resume(struct device *dev); int acpi_dev_pm_attach(struct device *dev, bool power_on); +bool acpi_storage_d3(struct device *dev); #else static inline int acpi_subsys_runtime_suspend(struct device *dev) { return 0; } static inline int acpi_subsys_runtime_resume(struct device *dev) { return 0; } @@ -1011,6 +1012,10 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on) { return 0; } +static inline bool acpi_storage_d3(struct device *dev) +{ + return false; +} #endif #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP) -- _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead 2021-05-27 16:58 ` Keith Busch @ 2021-05-27 17:03 ` Limonciello, Mario 2021-05-27 17:28 ` Christoph Hellwig 1 sibling, 0 replies; 8+ messages in thread From: Limonciello, Mario @ 2021-05-27 17:03 UTC (permalink / raw) To: Keith Busch, Bjorn Helgaas Cc: Christoph Hellwig, Jens Axboe, Sagi Grimberg, open list:NVM EXPRESS DRIVER, Prike Liang, rrangel, David E. Box, Shyam-sundar S-k, Alexander Deucher, Rafael J. Wysocki On 5/27/2021 11:58, Keith Busch wrote: > On Thu, May 27, 2021 at 10:52:43AM -0500, Bjorn Helgaas wrote: >> On Thu, May 27, 2021 at 9:44 AM Limonciello, Mario >> <mario.limonciello@amd.com> wrote: >>> On 5/27/2021 09:35, Christoph Hellwig wrote: >>>> Adding Raul who has been asking for something like this as well. >>> >>>> I'd also really like to move nvme_acpi_storage_d3 out of the NVMe >>>> driver. The Microsoft document that the original document references >>>> makes it very clear that this is not NVMe specific, but also covers >>>> at least AHCI. On top of that the platform simply can't know what kind >>>> of PCIe device is in any given slot. Last but not least this will also >>>> allow us to add quirks for devices that fail to properly mark this >>>> misfeature in the ACPI tables. >>> >>> +Bjorn >>> >>> Back when this feature was first submitted, that was actually the >>> initial way that it was done, but Bjorn had preferred to see it move >>> into the NVME driver directly: >>> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-nvme%2F20200625173053.GA2694537%40bjorn-Precision-5520%2F&data=04%7C01%7Cmario.limonciello%40amd.com%7C5480c9a7da154bb519f908d92130b272%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637577315313175138%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=xDn4vxurgMOokOvwLHdqHWa3tlSRUzh5XPBJdBmBUHo%3D&reserved=0 >>> >>> Bjorn, are you OK with this coming "back" to PCI based on Christoph's >>> comments? >> >> My point was that the PCI core can do nothing with this information, >> so it doesn't seem like putting it in PCI doesn't really gain >> anything. The Microsoft document you reference ([1]) doesn't mention >> PCI or PCIe. As far as I know, there's no PCI spec that mentions >> "StorageD3Enable". >> >> I agree that [1] doesn't seem to be NVMe-specific, since it also >> mentions SATA, so it might make sense to look for "StorageD3Enable" >> somewhere other than the NVMe driver. I'm just not convinced the PCI >> core is the best place. I have the impression that it's possible to >> have non-PCI SATA or AHCI devices (correct me if I'm wrong), and >> "StorageD3Enable" could apply to them as well. >> >> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fwindows-hardware%2Fdesign%2Fcomponent-guidelines%2Fpower-management-for-storage-hardware-devices-intro&data=04%7C01%7Cmario.limonciello%40amd.com%7C5480c9a7da154bb519f908d92130b272%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637577315313175138%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=uEOEgtuxZI3mkSU7G%2BUvZ2d2C18%2BcQu%2B1748kHhSuzU%3D&reserved=0 > > I agree it doesn't appear to belong in PCI either. This should go in > ACPI. Here's my proposal on top of this patch: This seems fine to me. +Rafael for comments on this approach. > > --- > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c > index d260bc1f3e6e..ab8a0dfae2df 100644 > --- a/drivers/acpi/device_pm.c > +++ b/drivers/acpi/device_pm.c > @@ -1340,4 +1340,25 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on) > return 1; > } > EXPORT_SYMBOL_GPL(acpi_dev_pm_attach); > + > +bool acpi_storage_d3(struct device *dev) > +{ > + struct acpi_device *adev; > + 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. > + */ > + adev = ACPI_COMPANION(dev); > + if (!adev) > + return false; > + if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable", > + &val)) > + return false; > + return val == 1; > +} > +EXPORT_SYMBOL_GPL(acpi_storage_d3); > + > #endif /* CONFIG_PM */ > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index d4eef8caa4cc..8fbc4c87a0d8 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -2828,33 +2828,6 @@ 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) > -{ > - struct acpi_device *adev; > - 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. > - */ > - > - adev = ACPI_COMPANION(&dev->dev); > - if (!adev) > - return false; > - if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable", > - &val)) > - return false; > - return val == 1; > -} > -#else > -static inline bool nvme_acpi_storage_d3(struct pci_dev *dev) > -{ > - return false; > -} > -#endif /* CONFIG_ACPI */ > - > static void nvme_async_probe(void *data, async_cookie_t cookie) > { > struct nvme_dev *dev = data; > @@ -2904,7 +2877,7 @@ 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)) { > + if (!noacpi && acpi_storage_d3(&pdev->dev)) { > /* > * Some systems use a bios work around to ask for D3 on > * platforms that support kernel managed suspend. > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index c60745f657e9..dd0dafd21e33 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -1004,6 +1004,7 @@ int acpi_dev_resume(struct device *dev); > int acpi_subsys_runtime_suspend(struct device *dev); > int acpi_subsys_runtime_resume(struct device *dev); > int acpi_dev_pm_attach(struct device *dev, bool power_on); > +bool acpi_storage_d3(struct device *dev); > #else > static inline int acpi_subsys_runtime_suspend(struct device *dev) { return 0; } > static inline int acpi_subsys_runtime_resume(struct device *dev) { return 0; } > @@ -1011,6 +1012,10 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on) > { > return 0; > } > +static inline bool acpi_storage_d3(struct device *dev) > +{ > + return false; > +} > #endif > > #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP) > -- > _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead 2021-05-27 16:58 ` Keith Busch 2021-05-27 17:03 ` Limonciello, Mario @ 2021-05-27 17:28 ` Christoph Hellwig 1 sibling, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2021-05-27 17:28 UTC (permalink / raw) To: Keith Busch Cc: Bjorn Helgaas, Limonciello, Mario, Christoph Hellwig, Jens Axboe, Sagi Grimberg, open list:NVM EXPRESS DRIVER, Prike Liang, rrangel, David E. Box, Shyam-sundar S-k, Alexander Deucher, Rafael J. Wysocki [adding Rafael] On Thu, May 27, 2021 at 09:58:44AM -0700, Keith Busch wrote: > EXPORT_SYMBOL_GPL(acpi_dev_pm_attach); > + > +bool acpi_storage_d3(struct device *dev) This need a really good kerneldoc comment. Not like pci_pr3_present which somehow made it into the PCI core, but has not explanation whatsoever.. > +{ > + struct acpi_device *adev; > + 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. > + */ This is part of what should got into the kerneldoc > + adev = ACPI_COMPANION(dev); > + if (!adev) > + return false; > + if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable", > + &val)) > + return false; > + return val == 1; > +} adev could be initialized at declaration time. Also any chance we could have acpi_fwnode_property_read_* helpers that include the NULL check and the acpi_fwnode_handle conversion which seems to be boilerplated all over? Also I wonder if having this only in ACPI is a good idea. What when we get the same thing in OF? Isn't this something that the PM layer should tell the device when calling ->suspend? _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead 2021-05-27 14:44 ` Limonciello, Mario 2021-05-27 15:52 ` Bjorn Helgaas @ 2021-05-27 15:53 ` Raul Rangel 1 sibling, 0 replies; 8+ messages in thread From: Raul Rangel @ 2021-05-27 15:53 UTC (permalink / raw) To: Limonciello, Mario Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg, open list:NVM EXPRESS DRIVER, Prike Liang, david.e.box, Shyam-sundar S-k, Alexander Deucher, bhelgaas On Thu, May 27, 2021 at 8:44 AM Limonciello, Mario <mario.limonciello@amd.com> wrote: > > On 5/27/2021 09:35, Christoph Hellwig wrote: > > Adding Raul who has been asking for something like this as well. > > > > I thought I already included him on CC. Sorry if I munged the email or > something. I did get V2. Thanks for adding me. I tested this patch out on an AMD platform that doesn't use the PXSX name and it works as expected. Acked-by: Raul E Rangel <rrangel@chromium.org> > > > I'd also really like to move nvme_acpi_storage_d3 out of the NVMe > > driver. The Microsoft document that the original document references > > makes it very clear that this is not NVMe specific, but also covers > > at least AHCI. On top of that the platform simply can't know what kind > > of PCIe device is in any given slot. Last but not least this will also > > allow us to add quirks for devices that fail to properly mark this > > misfeature in the ACPI tables. > > > > +Bjorn > > Back when this feature was first submitted, that was actually the > initial way that it was done, but Bjorn had preferred to see it move > into the NVME driver directly: > > https://lore.kernel.org/linux-nvme/20200625173053.GA2694537@bjorn-Precision-5520/ > > Bjorn, are you OK with this coming "back" to PCI based on Christoph's > comments? Just to add another data point, we currently use StorageD3Enabled on PCI SDHCI controllers: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/coreboot/src/mainboard/google/volteer/variants/baseboard/devicetree.cb;l=504 I would be fine doing that refactor in a follow-up patch. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-05-27 18:48 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-27 13:59 [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead Mario Limonciello 2021-05-27 14:35 ` Christoph Hellwig 2021-05-27 14:44 ` Limonciello, Mario 2021-05-27 15:52 ` Bjorn Helgaas 2021-05-27 16:58 ` Keith Busch 2021-05-27 17:03 ` Limonciello, Mario 2021-05-27 17:28 ` Christoph Hellwig 2021-05-27 15:53 ` Raul Rangel
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.