[PATCH] nvme-pci: Disable LTR for simple suspend

[PATCH] nvme-pci: Disable LTR for simple suspend

[Mario Limonciello]

Some drives from SSSTC are showing stability problems after s0i3
entry when the Linux kernel is in s2idle loop if LTR has been
enabled. This leads to failures to resume.

This appears to be a firmware issue specific to SSSTC SSDs, but to
avoid this class of problem, disable LTR when going into s2idle and
simple suspend has been set.

Co-developed-by: Patrick Huang <patrick.huang@amd.com>
Signed-off-by: Patrick Huang <patrick.huang@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/nvme/host/pci.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6a99ed680915..8d193c0842ed 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -139,6 +139,7 @@ struct nvme_dev {
 	struct nvme_ctrl ctrl;
 	u32 last_ps;
 	bool hmb;
+	bool restore_ltr;
 
 	mempool_t *iod_mempool;
 
@@ -3214,11 +3215,30 @@ static int nvme_set_power_state(struct nvme_ctrl *ctrl, u32 ps)
 	return nvme_set_features(ctrl, NVME_FEAT_POWER_MGMT, ps, NULL, 0, NULL);
 }
 
+static void nvme_suspend_ltr(struct device *dev, bool disable)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct nvme_dev *ndev = pci_get_drvdata(pdev);
+
+	if (disable) {
+		u16 word;
+
+		pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &word);
+		ndev->restore_ltr = word & PCI_EXP_DEVCTL2_LTR_EN;
+		pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL2,
+					   PCI_EXP_DEVCTL2_LTR_EN);
+	} else if (ndev->restore_ltr) {
+		pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
+					 PCI_EXP_DEVCTL2_LTR_EN);
+	}
+}
+
 static int nvme_resume(struct device *dev)
 {
 	struct nvme_dev *ndev = pci_get_drvdata(to_pci_dev(dev));
 	struct nvme_ctrl *ctrl = &ndev->ctrl;
 
+	nvme_suspend_ltr(dev, false);
 	if (ndev->last_ps == U32_MAX ||
 	    nvme_set_power_state(ctrl, ndev->last_ps) != 0)
 		goto reset;
@@ -3239,6 +3259,11 @@ static int nvme_suspend(struct device *dev)
 
 	ndev->last_ps = U32_MAX;
 
+	/* If using s2idle with simple suspend, disable LTR to avoid problems. */
+	if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE &&
+	    ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND)
+		nvme_suspend_ltr(dev, true);
+
 	/*
 	 * The platform does not remove power for a kernel managed suspend so
 	 * use host managed nvme power settings for lowest idle power if
-- 
2.34.1

Re: [PATCH] nvme-pci: Disable LTR for simple suspend

[Christoph Hellwig]

On Mon, Mar 14, 2022 at 08:55:37AM -0500, Mario Limonciello wrote:
> Some drives from SSSTC are showing stability problems after s0i3
> entry when the Linux kernel is in s2idle loop if LTR has been
> enabled. This leads to failures to resume.
> 
> This appears to be a firmware issue specific to SSSTC SSDs, but to
> avoid this class of problem, disable LTR when going into s2idle and
> simple suspend has been set.

This seems like a giant hammer to do this for all NVMe devices,
why not quirk the specific ones?

> +static void nvme_suspend_ltr(struct device *dev, bool disable)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct nvme_dev *ndev = pci_get_drvdata(pdev);
> +
> +	if (disable) {
> +		u16 word;
> +
> +		pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &word);
> +		ndev->restore_ltr = word & PCI_EXP_DEVCTL2_LTR_EN;
> +		pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL2,
> +					   PCI_EXP_DEVCTL2_LTR_EN);
> +	} else if (ndev->restore_ltr) {
> +		pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
> +					 PCI_EXP_DEVCTL2_LTR_EN);
> +	}
> +}

The calling conventions of this function are rather strange by
mixing up two very different things.

I think two PCI-level helpers to disable LTR and return the status
it ways in and to enable LTR would be really nice to have here.

>  	if (ndev->last_ps == U32_MAX ||
>  	    nvme_set_power_state(ctrl, ndev->last_ps) != 0)
>  		goto reset;
> @@ -3239,6 +3259,11 @@ static int nvme_suspend(struct device *dev)
>  
>  	ndev->last_ps = U32_MAX;
>  
> +	/* If using s2idle with simple suspend, disable LTR to avoid problems. */

Overly long line here.

RE: [PATCH] nvme-pci: Disable LTR for simple suspend

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Christoph Hellwig <hch@lst.de>
> Sent: Tuesday, March 15, 2022 02:23
> To: Limonciello, Mario <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>; open
> list:NVM EXPRESS DRIVER <linux-nvme@lists.infradead.org>; open list
> <linux-kernel@vger.kernel.org>; Huang, Patrick <Patrick.Huang@amd.com>;
> linux-pci@vger.kernel.org
> Subject: Re: [PATCH] nvme-pci: Disable LTR for simple suspend
> 
> On Mon, Mar 14, 2022 at 08:55:37AM -0500, Mario Limonciello wrote:
> > Some drives from SSSTC are showing stability problems after s0i3
> > entry when the Linux kernel is in s2idle loop if LTR has been
> > enabled. This leads to failures to resume.
> >
> > This appears to be a firmware issue specific to SSSTC SSDs, but to
> > avoid this class of problem, disable LTR when going into s2idle and
> > simple suspend has been set.
> 
> This seems like a giant hammer to do this for all NVMe devices,
> why not quirk the specific ones?
> 

The thought process was to be defensive against any other drives
that have this firmware deficiency as well, but point taken.

Will rework it as a quirk.

> > +static void nvme_suspend_ltr(struct device *dev, bool disable)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	struct nvme_dev *ndev = pci_get_drvdata(pdev);
> > +
> > +	if (disable) {
> > +		u16 word;
> > +
> > +		pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2,
> &word);
> > +		ndev->restore_ltr = word & PCI_EXP_DEVCTL2_LTR_EN;
> > +		pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL2,
> > +					   PCI_EXP_DEVCTL2_LTR_EN);
> > +	} else if (ndev->restore_ltr) {
> > +		pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
> > +					 PCI_EXP_DEVCTL2_LTR_EN);
> > +	}
> > +}
> 
> The calling conventions of this function are rather strange by
> mixing up two very different things.
> 
> I think two PCI-level helpers to disable LTR and return the status
> it ways in and to enable LTR would be really nice to have here.

If adding helper functions to PCI for this, it actually begs the argument
of whether this should just be a PCI quirk.  The LTR behavior is tied
to how it behaves in D3, not based on anything that the NVME driver
has done.

Unless you feel otherwise, I'll rework it as a PCI quirk.

> 
> >  	if (ndev->last_ps == U32_MAX ||
> >  	    nvme_set_power_state(ctrl, ndev->last_ps) != 0)
> >  		goto reset;
> > @@ -3239,6 +3259,11 @@ static int nvme_suspend(struct device *dev)
> >
> >  	ndev->last_ps = U32_MAX;
> >
> > +	/* If using s2idle with simple suspend, disable LTR to avoid problems.
> */
> 
> Overly long line here.