[PATCH] nvme-pci: simple suspend quirk for vmd devices

[PATCH] nvme-pci: simple suspend quirk for vmd devices

[Keith Busch]

Bug reports from users tell us platfroms with vmd enabled regressed
power when using nvme power management on s2idle. We can't get the
StorageD3Enable property on such devices so the driver used the wrong
suspend method. Add a simple suspend quirk for the domain since that is
the safest option.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=215467
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/pci.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6a99ed680915..6fa66d5d924e 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3094,6 +3094,12 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		dev_info(&pdev->dev,
 			 "platform quirk: setting simple suspend\n");
 		quirks |= NVME_QUIRK_SIMPLE_SUSPEND;
+	} else if (pci_domain_nr(pdev->bus) > 0xffff) {
+		/*
+		 * ACPI device properties are not reliable within VMD domains,
+		 * so assume the worst.
+		 */
+		quirks |= NVME_QUIRK_SIMPLE_SUSPEND;
 	}
 
 	/*
-- 
2.25.4

Re: [PATCH] nvme-pci: simple suspend quirk for vmd devices

[Christoph Hellwig]

On Thu, Feb 10, 2022 at 07:22:33AM -0800, Keith Busch wrote:
> +	} else if (pci_domain_nr(pdev->bus) > 0xffff) {
> +		/*
> +		 * ACPI device properties are not reliable within VMD domains,
> +		 * so assume the worst.
> +		 */
> +		quirks |= NVME_QUIRK_SIMPLE_SUSPEND;

We'll need to communicate that from th vmd layer instead of hacking
a check like this into NVMe..

Re: [PATCH] nvme-pci: simple suspend quirk for vmd devices

[Christoph Hellwig]

On Thu, Feb 10, 2022 at 07:22:33AM -0800, Keith Busch wrote:
> Bug reports from users tell us platfroms with vmd enabled regressed
> power when using nvme power management on s2idle. We can't get the
> StorageD3Enable property on such devices so the driver used the wrong
> suspend method. Add a simple suspend quirk for the domain since that is
> the safest option.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215467
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>

Rafael, can you try this on the affected platform?
(not for merge as-is)

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 9e1e6b8d88763..49f4e54da919d 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -723,4 +723,5 @@ struct pci_dev *pci_real_dma_dev(struct pci_dev *dev)
 
 	return dev;
 }
+EXPORT_SYMBOL_GPL(pci_real_dma_dev);
 #endif
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6a99ed6809158..6edc8c514cbc8 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3086,7 +3086,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)) {
+	if (!noacpi && acpi_storage_d3(&pci_real_dma_dev(pdev)->dev)) {
 		/*
 		 * Some systems use a bios work around to ask for D3 on
 		 * platforms that support kernel managed suspend.

Re: [PATCH] nvme-pci: simple suspend quirk for vmd devices

[Rafael J. Wysocki]

On Wed, Feb 16, 2022 at 9:43 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Feb 10, 2022 at 07:22:33AM -0800, Keith Busch wrote:
> > Bug reports from users tell us platfroms with vmd enabled regressed
> > power when using nvme power management on s2idle. We can't get the
> > StorageD3Enable property on such devices so the driver used the wrong
> > suspend method. Add a simple suspend quirk for the domain since that is
> > the safest option.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215467
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>
> Rafael, can you try this on the affected platform?
> (not for merge as-is)

I can't really do that myself (no access to the system in question),
but I've just asked the original reporter (CCed) to do it.

>
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index 9e1e6b8d88763..49f4e54da919d 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -723,4 +723,5 @@ struct pci_dev *pci_real_dma_dev(struct pci_dev *dev)
>
>         return dev;
>  }
> +EXPORT_SYMBOL_GPL(pci_real_dma_dev);
>  #endif
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 6a99ed6809158..6edc8c514cbc8 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -3086,7 +3086,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)) {
> +       if (!noacpi && acpi_storage_d3(&pci_real_dma_dev(pdev)->dev)) {
>                 /*
>                  * Some systems use a bios work around to ask for D3 on
>                  * platforms that support kernel managed suspend.