[PATCH] nvme-pci: Add sleep quirk for Kingston drives

[PATCH] nvme-pci: Add sleep quirk for Kingston drives

[Werner Sembach]

From: Georg Gottleuber <ggo@tuxedocomputers.com>

Some Kingston NV1 and A2000 are wasting a lot of power on specific TUXEDO
platforms in s2idle sleep if 'Simple Suspend' is used.

This patch applies a new quirk 'Force No Simple Suspend' to achieve a
low power sleep without 'Simple Suspend'.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Georg Gottleuber <ggo@tuxedocomputers.com>
Cc: <stable@vger.kernel.org>
---
 drivers/nvme/host/nvme.h |  5 +++++
 drivers/nvme/host/pci.c  | 16 +++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index f35647c470afa..74f74b459f5fa 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -156,6 +156,11 @@ enum nvme_quirks {
 	 * No temperature thresholds for channels other than 0 (Composite).
 	 */
 	NVME_QUIRK_NO_SECONDARY_TEMP_THRESH	= (1 << 19),
+
+	/*
+	 * Disables simple suspend/resume path.
+	 */
+	NVME_QUIRK_FORCE_NO_SIMPLE_SUSPEND	= (1 << 20),
 };
 
 /*
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 347cb5daebc3c..b4e6f0d21a44c 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2903,6 +2903,18 @@ 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;
+	} else if (pdev->vendor == 0x2646 && (pdev->device == 0x2263 ||
+		   pdev->device == 0x500f)) {
+		/*
+		 * Exclude some Kingston NV1 and A2000 devices from
+		 * NVME_QUIRK_SIMPLE_SUSPEND. Do a full suspend to save a
+		 * lot fo energy with s2idle sleep on some TUXEDO platforms.
+		 */
+		if (dmi_match(DMI_BOARD_NAME, "NS5X_NS7XAU") ||
+		    dmi_match(DMI_BOARD_NAME, "NS5x_7xAU") ||
+		    dmi_match(DMI_BOARD_NAME, "NS5x_7xPU") ||
+		    dmi_match(DMI_BOARD_NAME, "PH4PRX1_PH6PRX1"))
+			return NVME_QUIRK_FORCE_NO_SIMPLE_SUSPEND;
 	}
 
 	return 0;
@@ -2933,7 +2945,9 @@ 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)) {
+	if (!noacpi &&
+	    !(quirks & NVME_QUIRK_FORCE_NO_SIMPLE_SUSPEND) &&
+	    acpi_storage_d3(&pdev->dev)) {
 		/*
 		 * Some systems use a bios work around to ask for D3 on
 		 * platforms that support kernel managed suspend.
-- 
2.34.1

Re: [PATCH] nvme-pci: Add sleep quirk for Kingston drives

[Werner Sembach]

Hi,

Am 20.09.23 um 10:52 schrieb Werner Sembach:
> From: Georg Gottleuber <ggo@tuxedocomputers.com>
>
> Some Kingston NV1 and A2000 are wasting a lot of power on specific TUXEDO
> platforms in s2idle sleep if 'Simple Suspend' is used.
>
> This patch applies a new quirk 'Force No Simple Suspend' to achieve a
> low power sleep without 'Simple Suspend'.
>
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Signed-off-by: Georg Gottleuber <ggo@tuxedocomputers.com>
> Cc: <stable@vger.kernel.org>

Want to leave a gentle bump here. Any issues with this patch?

Kind regards,

Werner Sembach

> ---
>   drivers/nvme/host/nvme.h |  5 +++++
>   drivers/nvme/host/pci.c  | 16 +++++++++++++++-
>   2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index f35647c470afa..74f74b459f5fa 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -156,6 +156,11 @@ enum nvme_quirks {
>   	 * No temperature thresholds for channels other than 0 (Composite).
>   	 */
>   	NVME_QUIRK_NO_SECONDARY_TEMP_THRESH	= (1 << 19),
> +
> +	/*
> +	 * Disables simple suspend/resume path.
> +	 */
> +	NVME_QUIRK_FORCE_NO_SIMPLE_SUSPEND	= (1 << 20),
>   };
>   
>   /*
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 347cb5daebc3c..b4e6f0d21a44c 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2903,6 +2903,18 @@ 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;
> +	} else if (pdev->vendor == 0x2646 && (pdev->device == 0x2263 ||
> +		   pdev->device == 0x500f)) {
> +		/*
> +		 * Exclude some Kingston NV1 and A2000 devices from
> +		 * NVME_QUIRK_SIMPLE_SUSPEND. Do a full suspend to save a
> +		 * lot fo energy with s2idle sleep on some TUXEDO platforms.
> +		 */
> +		if (dmi_match(DMI_BOARD_NAME, "NS5X_NS7XAU") ||
> +		    dmi_match(DMI_BOARD_NAME, "NS5x_7xAU") ||
> +		    dmi_match(DMI_BOARD_NAME, "NS5x_7xPU") ||
> +		    dmi_match(DMI_BOARD_NAME, "PH4PRX1_PH6PRX1"))
> +			return NVME_QUIRK_FORCE_NO_SIMPLE_SUSPEND;
>   	}
>   
>   	return 0;
> @@ -2933,7 +2945,9 @@ 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)) {
> +	if (!noacpi &&
> +	    !(quirks & NVME_QUIRK_FORCE_NO_SIMPLE_SUSPEND) &&
> +	    acpi_storage_d3(&pdev->dev)) {
>   		/*
>   		 * Some systems use a bios work around to ask for D3 on
>   		 * platforms that support kernel managed suspend.

Re: [PATCH] nvme-pci: Add sleep quirk for Kingston drives

[Christoph Hellwig]

What does save a lot of power mean, and why does it only apply to
some platforms?  I suspect the more likely case is that APST is
simply completely broken on this device?

Re: [PATCH] nvme-pci: Add sleep quirk for Kingston drives

[Georg Gottleuber]

Am 06.12.23 um 10:24 schrieb Christoph Hellwig:
> What does save a lot of power mean, and why does it only apply to
> some platforms?  I suspect the more likely case is that APST is
> simply completely broken on this device?

Hello,

power consumption in s0ix varies depending on mainboard and NVMe SSD. 
Some examples:
Board,           nvme(id), mW w/o patch, mW with patch, kernel
NS5X_SN7XAU,     0x500f,   2691mW,       487mW,         6.2.0
NS5X_SN7XAU,     0x2263,   2262mW,       741mW,         6.2.0
PH4PRX1_PH6PRX1, 0x2263,   3042mW,       760mW,         6.2.0
NS5X_SN7XPU,     0x500f,   3135mW,       779mW,         6.5.0

All measurements were taken with the battery disconnected and a
modified adapter plug.

I don't have a deeper insight into APST to judge whether something is 
not working. But all other NVMe SSDs we have tested sleep with a power 
consumption below 2W (Samsung 970 Evo, Samsung 980, Corsair MP600 PRO 
NH, other Kingston NV1 with ID != 0x500f).

We originally noticed that newer Kingston NV1 (with id 500f) led to 
higher standby consumption. With some trial and error I came to the 
conclusion that the problem lies with 'Simple Suspend'.

Kind regards,

Georg Gottleuber

Re: [PATCH] nvme-pci: Add sleep quirk for Kingston drives

[Christoph Hellwig]

Ok.  I guess we have to live with this quirk then :(

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] nvme-pci: Add sleep quirk for Kingston drives

[Keith Busch]

Thanks, applied for nvme-6.7