diff mbox series

nvme: Add a module parameter for users to force simple suspend

Message ID 20230228221148.2672-1-mario.limonciello@amd.com
State New
Headers
Series nvme: Add a module parameter for users to force simple suspend |

Commit Message

Mario Limonciello Feb. 28, 2023, 10:11 p.m. UTC 
  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.

Add a new module parameter for the NVME module that will let users
force simple suspend to be enabled or disabled universally across
disks.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=216773#c19
Link: https://learn.microsoft.com/en-us/windows/configuration/wcd/wcd-storaged3inmodernstandby
Reported-and-tested-by: elvis.angelaccio@kde.org
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/nvme/host/pci.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Comments

Keith Busch Feb. 28, 2023, 10:34 p.m. UTC | #1 
On Tue, Feb 28, 2023 at 04:11:48PM -0600, Mario Limonciello wrote:
> +static bool nvme_use_simple_suspend(struct pci_dev *pdev)
> +{
> +	if (!simple_suspend)
> +		return false;
> +	if (simple_suspend == 1)
> +		return true;
> +	return !noacpi && acpi_storage_d3(&pdev->dev);
> +}
> +
>  static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	int node, result = -ENOMEM;
> @@ -3128,11 +3145,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	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.
> -		 */
> +	if (nvme_use_simple_suspend(pdev)) {
>  		dev_info(&pdev->dev,
>  			 "platform quirk: setting simple suspend\n");
>  		quirks |= NVME_QUIRK_SIMPLE_SUSPEND;

Do you want the user setting "never" to override the driver's default quirks?
Mario Limonciello Feb. 28, 2023, 11:48 p.m. UTC | #2 
On 2/28/23 16:34, Keith Busch wrote:
> On Tue, Feb 28, 2023 at 04:11:48PM -0600, Mario Limonciello wrote:
>> +static bool nvme_use_simple_suspend(struct pci_dev *pdev)
>> +{
>> +	if (!simple_suspend)
>> +		return false;
>> +	if (simple_suspend == 1)
>> +		return true;
>> +	return !noacpi && acpi_storage_d3(&pdev->dev);
>> +}
>> +
>>   static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   {
>>   	int node, result = -ENOMEM;
>> @@ -3128,11 +3145,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   
>>   	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.
>> -		 */
>> +	if (nvme_use_simple_suspend(pdev)) {
>>   		dev_info(&pdev->dev,
>>   			 "platform quirk: setting simple suspend\n");
>>   		quirks |= NVME_QUIRK_SIMPLE_SUSPEND;
> 
> Do you want the user setting "never" to override the driver's default quirks?

That hadn't occurred to me, but if offering a 0/1/-1 it certainly makes 
sense.  I'll add something to explicitly clear it if present for a V2.

Another way I've hypothesized that this problem (at least as reported) 
can be approached is to examine if ANY disks in the system have simple 
suspend set to apply a "global" change to all NVME disks.

If that is preferable I'm fine to spin it that way too.
diff mbox series

Patch

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 488ad7dabeb8..718bec2d793b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -62,6 +62,10 @@  MODULE_PARM_DESC(sgl_threshold,
 		"Use SGLs when average request segment size is larger or equal to "
 		"this size. Use 0 to disable SGLs.");
 
+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";)
+
 #define NVME_PCI_MIN_QUEUE_SIZE 2
 #define NVME_PCI_MAX_QUEUE_SIZE 4095
 static int io_queue_depth_set(const char *val, const struct kernel_param *kp);
@@ -3088,6 +3092,19 @@  static void nvme_async_probe(void *data, async_cookie_t cookie)
 	nvme_put_ctrl(&dev->ctrl);
 }
 
+/*
+ * Some systems include a BIOS _DSD property to ask for D3
+ * or users may explicitly request it enabled.
+ */
+static bool nvme_use_simple_suspend(struct pci_dev *pdev)
+{
+	if (!simple_suspend)
+		return false;
+	if (simple_suspend == 1)
+		return true;
+	return !noacpi && acpi_storage_d3(&pdev->dev);
+}
+
 static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	int node, result = -ENOMEM;
@@ -3128,11 +3145,7 @@  static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	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.
-		 */
+	if (nvme_use_simple_suspend(pdev)) {
 		dev_info(&pdev->dev,
 			 "platform quirk: setting simple suspend\n");
 		quirks |= NVME_QUIRK_SIMPLE_SUSPEND;