[PATCH v2] nvme: Add a module parameter for users to force simple suspend

* [PATCH v2] nvme: Add a module parameter for users to force simple suspend
@ 2023-03-01 20:33 Mario Limonciello
  2023-03-09  9:36 ` Christoph Hellwig
  0 siblings, 1 reply; 3+ messages in thread
From: Mario Limonciello @ 2023-03-01 20:33 UTC (permalink / raw)
  To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg
  Cc: Mario Limonciello, elvis.angelaccio, linux-nvme, linux-kernel

Elvis Angelaccio reports that his HP laptop that has two SSDs takes
a long time to resume from suspend to idle and has low hardware sleep
residency.  In analyzing the logs and acpidump from the BIOS it's clear
that BIOS does set the StorageD3Enable _DSD property but it's only
set on one of the SSDs.

Double checking the behavior in Windows there is no problem with
resume time or hardware sleep residency. It appears that Windows offers
a configuration setting for OEMs to utilize in their factory images
and end users to set to force allowing D3 to be used for sleep.

The preference would be that OEMs fix this in the BIOS by adding the
StorageD3Enable _DSD to both disks, but as this works on Windows by
such a policy we should offer something similar that users can utilize
in Linux too.

Remove the module parameter noacpi which could only be used to ignore
StorageD3Enable and instead add a new module parameter for the NVME
module that will let users force simple suspend to be enabled or
disabled universally.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=216773#c19
Link: https://learn.microsoft.com/en-us/windows/configuration/wcd/wcd-storaged3inmodernstandby
Reported-by: elvis.angelaccio@kde.org
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * Rebase on nvme/nvme-6.3
 * Replace noacpi parameter
 * Clear the quirk if set by driver and user set simple_suspend=0
---
 drivers/nvme/host/pci.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d68e2db00d0d..93dcd9dc8df9 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -102,9 +102,9 @@ 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");
+static int simple_suspend = -1;
+module_param(simple_suspend, int, 0444);
+MODULE_PARM_DESC(simple_suspend, "use simple suspend for disks (0 = never, 1 = always, -1 = auto";)
 
 struct nvme_dev;
 struct nvme_queue;
@@ -2898,7 +2898,30 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
 		 */
 		if ((dmi_match(DMI_BOARD_VENDOR, "LENOVO")) &&
 		     dmi_match(DMI_BOARD_NAME, "LNVNB161216"))
-			return NVME_QUIRK_SIMPLE_SUSPEND;
+			return simple_suspend ? NVME_QUIRK_SIMPLE_SUSPEND : 0;
+	}
+
+	return 0;
+}
+
+/*
+ * Some systems include a BIOS _DSD property to ask for D3
+ * or users may explicitly request it enabled.
+ */
+static unsigned long check_simple_suspend(struct pci_dev *pdev)
+{
+	switch (simple_suspend) {
+	case 0:
+		return 0;
+	case 1:
+		return NVME_QUIRK_SIMPLE_SUSPEND;
+	default:
+		break;
+	}
+	if (acpi_storage_d3(&pdev->dev)) {
+		dev_info(&pdev->dev,
+			 "platform quirk: setting simple suspend\n");
+		return NVME_QUIRK_SIMPLE_SUSPEND;
 	}
 
 	return 0;
@@ -2932,15 +2955,7 @@ static struct nvme_dev *nvme_pci_alloc_dev(struct pci_dev *pdev,
 	dev->dev = get_device(&pdev->dev);
 
 	quirks |= check_vendor_combination_bug(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.
-		 */
-		dev_info(&pdev->dev,
-			 "platform quirk: setting simple suspend\n");
-		quirks |= NVME_QUIRK_SIMPLE_SUSPEND;
-	}
+	quirks |= check_simple_suspend(pdev);
 	ret = nvme_init_ctrl(&dev->ctrl, &pdev->dev, &nvme_pci_ctrl_ops,
 			     quirks);
 	if (ret)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread