[v2] nvme: Add a module parameter for users to force simple suspend
Commit Message
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(-)
Comments
On Wed, Mar 01, 2023 at 02:33:01PM -0600, Mario Limonciello wrote:
> 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.
Please quirk the platform in the PCIe core instead of requiring the
user to override manually in the nvme drivers that is not relevant.
And as a representative for a CPU/platform vendor that is all behind
this stupid crap please work with Microsoft and Intel to sort it out
properly the firmware level. It's a never ending pain caused on us
that you guys caused for absolutely no reason.
[Public]
> -----Original Message-----
> From: Christoph Hellwig <hch@lst.de>
> Sent: Thursday, March 9, 2023 03:37
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>;
> Christoph Hellwig <hch@lst.de>; Sagi Grimberg <sagi@grimberg.me>;
> elvis.angelaccio@kde.org; linux-nvme@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2] nvme: Add a module parameter for users to force
> simple suspend
>
> On Wed, Mar 01, 2023 at 02:33:01PM -0600, Mario Limonciello wrote:
> > 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.
>
> Please quirk the platform in the PCIe core instead of requiring the
> user to override manually in the nvme drivers that is not relevant.
>
OK for this situation reported I'll quirk it another way.
> And as a representative for a CPU/platform vendor that is all behind
> this stupid crap please work with Microsoft and Intel to sort it out
> properly the firmware level. It's a never ending pain caused on us
> that you guys caused for absolutely no reason.
The primary way to do this is via the _DSD, but for whatever reason
Microsoft also offers:
1) An allow list of old parts without the _DSD
2) A knob in the OS users can touch
3) A setting that OEMs can set in the factory
Any OEM that works with us on enabling Linux on their hardware I'll make
sure they have the _DSD for all the ports they use SSDs.
@@ -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)