[PATCH v3 1/2] nvme: Look for StorageD3Enable on companion ACPI device instead

[PATCH v3 1/2] nvme: Look for StorageD3Enable on companion ACPI device instead

[Mario Limonciello]

The documentation around the StorageD3Enable property hints that it
should be made on the PCI device.  This is where newer AMD systems set
the property and it's required for S0i3 support.

So rather than look for nodes of the root port only present on Intel
systems, switch to the companion ACPI device for all systems.
David Box from Intel indicated this should work on Intel as well.

Link: https://lore.kernel.org/linux-nvme/YK6gmAWqaRmvpJXb@google.com/T/#m900552229fa455867ee29c33b854845fce80ba70
Link: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro
Suggested-by: Liang Prike <Prike.Liang@amd.com>
Acked-by: Raul E Rangel <rrangel@chromium.org>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/nvme/host/pci.c | 24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

Changes from v1->v2:
 * Drop the old PXSX/PEGP logic instead of supplement to it
 * Add references to other discussions on this topic
Changes from v2->v3:
 * Initialize variable at beginning of function

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index a29b170701fc..3aa7245a505f 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2831,10 +2831,7 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
 #ifdef CONFIG_ACPI
 static bool nvme_acpi_storage_d3(struct pci_dev *dev)
 {
-	struct acpi_device *adev;
-	struct pci_dev *root;
-	acpi_handle handle;
-	acpi_status status;
+	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
 	u8 val;
 
 	/*
@@ -2842,28 +2839,9 @@ static bool nvme_acpi_storage_d3(struct pci_dev *dev)
 	 * must use D3 to support deep platform power savings during
 	 * suspend-to-idle.
 	 */
-	root = pcie_find_root_port(dev);
-	if (!root)
-		return false;
 
-	adev = ACPI_COMPANION(&root->dev);
 	if (!adev)
 		return false;
-
-	/*
-	 * The property is defined in the PXSX device for South complex ports
-	 * and in the PEGP device for North complex ports.
-	 */
-	status = acpi_get_handle(adev->handle, "PXSX", &handle);
-	if (ACPI_FAILURE(status)) {
-		status = acpi_get_handle(adev->handle, "PEGP", &handle);
-		if (ACPI_FAILURE(status))
-			return false;
-	}
-
-	if (acpi_bus_get_device(handle, &adev))
-		return false;
-
 	if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable",
 			&val))
 		return false;
-- 
2.25.1

Re: [PATCH v3 1/2] nvme: Look for StorageD3Enable on companion ACPI device instead

[Christoph Hellwig]

On Fri, May 28, 2021 at 11:02:34AM -0500, Mario Limonciello wrote:
> The documentation around the StorageD3Enable property hints that it
> should be made on the PCI device.  This is where newer AMD systems set
> the property and it's required for S0i3 support.
> 
> So rather than look for nodes of the root port only present on Intel
> systems, switch to the companion ACPI device for all systems.
> David Box from Intel indicated this should work on Intel as well.

I think we need to wait for the confirmation from David.  This looks good,
but I'd like to see testing.  I also wonder how many of the simple suspend
quirks we can drop with this.

Shyjumon and Jon, can you retests the platforms quirked in
1fae37accfc5 ("nvme/pci: Add sleep quirk for Samsung and Toshiba drives")
with this fix?

RE: [PATCH v3 1/2] nvme: Look for StorageD3Enable on companion ACPI device instead

[N, Shyjumon]

Hi 
Reply in-line. 

Have a good day.

Thank you and Regards,
Shyjumon N.
-----Original Message-----
From: Christoph Hellwig <hch@lst.de> 
Sent: Monday, May 31, 2021 12:17 PM
To: Mario Limonciello <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>; Rafael J . Wysocki <rjw@rjwysocki.net>; open list:NVM EXPRESS DRIVER <linux-nvme@lists.infradead.org>; linux-acpi@vger.kernel.org; rrangel@chromium.org; david.e.box@linux.intel.com; Shyam-sundar.S-k@amd.com; Alexander.Deucher@amd.com; prike.liang@amd.com; N, Shyjumon <shyjumon.n@intel.com>; Derrick, Jonathan <jonathan.derrick@intel.com>
Subject: Re: [PATCH v3 1/2] nvme: Look for StorageD3Enable on companion ACPI device instead

On Fri, May 28, 2021 at 11:02:34AM -0500, Mario Limonciello wrote:
> The documentation around the StorageD3Enable property hints that it 
> should be made on the PCI device.  This is where newer AMD systems set 
> the property and it's required for S0i3 support.
> 
> So rather than look for nodes of the root port only present on Intel 
> systems, switch to the companion ACPI device for all systems.
> David Box from Intel indicated this should work on Intel as well.

I think we need to wait for the confirmation from David.  This looks good, but I'd like to see testing.  I also wonder how many of the simple suspend quirks we can drop with this.

Shyjumon and Jon, can you retests the platforms quirked in
1fae37accfc5 ("nvme/pci: Add sleep quirk for Samsung and Toshiba drives") with this fix?

Shyjumon>> Yes, I do agree we need to test this also. However the boards where which I had these issues are not in my remote access now (as the work frequency on this boards are less and also due to Covid situation),
                        It might  take some time for me to test. I will update as soon as I can.

Re: [PATCH v3 1/2] nvme: Look for StorageD3Enable on companion ACPI device instead

[Limonciello, Mario]

> 
> I think we need to wait for the confirmation from David.  This looks good, but I'd like to see testing.  I also wonder how many of the simple suspend quirks we can drop with this.

David, do you think you can get this soonish or are you similarly 
blocked like Shyjumon by remote work restrictions?  As this is causing 
problems for a variety of already shipped OEM systems I would like to 
see this series land for 5.14.  If you can't get to it in time, I would 
prefer that we switch this patch back to how it was conservatively done 
in v1 for 5.14 (and redo 2/2 on top of that):
https://lore.kernel.org/linux-nvme/20210527113751.GB17266@lst.de/T/#t

and let you get back to the list when you can to optimize it "later".

> 
> Shyjumon and Jon, can you retests the platforms quirked in
> 1fae37accfc5 ("nvme/pci: Add sleep quirk for Samsung and Toshiba drives") with this fix?
> 
> Shyjumon>> Yes, I do agree we need to test this also. However the boards where which I had these issues are not in my remote access now (as the work frequency on this boards are less and also due to Covid situation),
>                          It might  take some time for me to test. I will update as soon as I can.
> 

Re: [PATCH v3 1/2] nvme: Look for StorageD3Enable on companion ACPI device instead

[David E. Box]

Hi Mario,

The patch works for systems that worked on the current implementation.
Thanks.

Reviewed-by: David E. Box <david.e.box@linux.intel.com>

On Tue, 2021-06-01 at 10:15 -0500, Limonciello, Mario wrote:
> > 
> > I think we need to wait for the confirmation from David.  This looks
> > good, but I'd like to see testing.  I also wonder how many of the
> > simple suspend quirks we can drop with this.
> 
> David, do you think you can get this soonish or are you similarly 
> blocked like Shyjumon by remote work restrictions?  As this is causing 
> see this series land for 5.14.  If you can't get to it in time, I would
> prefer that we switch this patch back to how it was conservatively done
> in v1 for 5.14 (and redo 2/2 on top of that):
> https://lore.kernel.org/linux-nvme/20210527113751.GB17266@lst.de/T/#t
> 
> and let you get back to the list when you can to optimize it "later".
> 
> > 
> > Shyjumon and Jon, can you retests the platforms quirked in
> > 1fae37accfc5 ("nvme/pci: Add sleep quirk for Samsung and Toshiba
> > drives") with this fix?
> > 
> > Shyjumon>> Yes, I do agree we need to test this also. However the
> > boards where which I had these issues are not in my remote access now
> > (as the work frequency on this boards are less and also due to Covid
> > situation),
> >                          It might  take some time for me to test. I
> > will update as soon as I can.
> > 

Re: [PATCH v3 1/2] nvme: Look for StorageD3Enable on companion ACPI device instead

[David E. Box]

On Mon, 2021-05-31 at 08:46 +0200, Christoph Hellwig wrote:
> On Fri, May 28, 2021 at 11:02:34AM -0500, Mario Limonciello wrote:
> > The documentation around the StorageD3Enable property hints that it
> > should be made on the PCI device.  This is where newer AMD systems
> > set
> > the property and it's required for S0i3 support.
> > 
> > So rather than look for nodes of the root port only present on
> > Intel
> > systems, switch to the companion ACPI device for all systems.
> > David Box from Intel indicated this should work on Intel as well.
> 
> I think we need to wait for the confirmation from David.

I've tested one configuration remotely already and it works. I've got
one more to test. I'll know by tomorrow.

David

Re: [PATCH v3 1/2] nvme: Look for StorageD3Enable on companion ACPI device instead

[Christoph Hellwig]

Thanks,

applied to nvme-5.14.  I've also added a fixes tag.