On Thu, Jul 20, 2023 at 09:57:58PM +0530, Manivannan Sadhasivam wrote: > On Thu, Jul 20, 2023 at 11:20:27AM -0500, Bjorn Helgaas wrote: > > On Thu, Jul 20, 2023 at 09:37:38PM +0530, Manivannan Sadhasivam wrote: > > ... > > > To be precise, NVMe driver will shutdown the device if there is > > > no ASPM support and keep it in low power mode otherwise (there > > > are other cases as well but we do not need to worry). > > > > > > But here you are not checking for ASPM state in the suspend > > > path, and just forcing the link to be in L2/L3 (thereby D3Cold) > > > even though NVMe driver may expect it to be in low power state > > > like ASPM/APST. > > > > > > So you should only put the link to L2/L3 if there is no ASPM > > > support. Otherwise, you'll ending up with bug reports when users > > > connect NVMe to it. > > > > Can you point me to the NVMe code that shuts down the device if > > there's no ASPM support? That sounds interesting and of interest > > to other drivers that want to do suspend. > > drivers/nvme/host/pci.c #3185 > > Note that, with ACPI based systems and for a few SSDs the behavior > may change (check NVME_QUIRK_SIMPLE_SUSPEND flag). For posterity, since the filename and line number may change: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvme/host/pci.c?id=v6.4#n3185 static int nvme_suspend(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct nvme_dev *ndev = pci_get_drvdata(pdev); struct nvme_ctrl *ctrl = &ndev->ctrl; int ret = -EBUSY; ndev->last_ps = U32_MAX; /* * The platform does not remove power for a kernel managed suspend so * use host managed nvme power settings for lowest idle power if * possible. This should have quicker resume latency than a full device * shutdown. But if the firmware is involved after the suspend or the * device does not support any non-default power states, shut down the * device fully. * * If ASPM is not enabled for the device, shut down the device and allow * the PCI bus layer to put it into D3 in order to take the PCIe link * down, so as to allow the platform to achieve its minimum low-power * state (which may not be possible if the link is up). */ if (pm_suspend_via_firmware() || !ctrl->npss || !pcie_aspm_enabled(pdev) || (ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND)) return nvme_disable_prepare_reset(ndev, true); nvme_start_freeze(ctrl); nvme_wait_freeze(ctrl); nvme_sync_queues(ctrl); ... Added by 4eaefe8c621c ("nvme-pci: Allow PCI bus-level PM to be used if ASPM is disabled"): https://git.kernel.org/linus/4eaefe8c621c
