| Message ID | 20210528160234.9402-1-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | Improvements to StorageD3Enable | expand |
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?
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.
> > 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. >
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
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. > >
Thanks, applied to nvme-5.14. I've also added a fixes tag.
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;