Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead

[Keith Busch <kbusch@kernel.org>]

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