linux-kernel - Re: [PATCH v3 1/2] PCI: dwc: Implement general suspend/resume functionality for L2/L3 transitions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230720183512.GA539111@bhelgaas>
Date:   Thu, 20 Jul 2023 13:35:12 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Manivannan Sadhasivam <mani@...nel.org>
Cc:     Frank Li <Frank.li@....com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        imx@...ts.linux.dev, bhelgaas@...gle.com,
        devicetree@...r.kernel.org, gustavo.pimentel@...opsys.com,
        kw@...ux.com, leoyang.li@....com,
        linux-arm-kernel@...ts.infradead.org, linux-imx@....com,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        lorenzo.pieralisi@....com, minghuan.lian@....com,
        mingkai.hu@....com, robh+dt@...nel.org, roy.zang@....com,
        shawnguo@...nel.org, zhiqiang.hou@....com
Subject: Re: [PATCH v3 1/2] PCI: dwc: Implement general suspend/resume
 functionality for L2/L3 transitions

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