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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230301203302.28889-1-mario.limonciello@amd.com>
Date:   Wed, 1 Mar 2023 14:33:01 -0600
From:   Mario Limonciello <mario.limonciello@....com>
To:     Keith Busch <kbusch@...nel.org>, Jens Axboe <axboe@...com>,
        "Christoph Hellwig" <hch@....de>, Sagi Grimberg <sagi@...mberg.me>
CC:     Mario Limonciello <mario.limonciello@....com>,
        <elvis.angelaccio@....org>, <linux-nvme@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: [PATCH v2] nvme: Add a module parameter for users to force simple suspend

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@....org
Signed-off-by: Mario Limonciello <mario.limonciello@....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