[PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001

[PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001

[Jongpil Jung]

LiteON(SSSTC) CL1 device running FW 220TQ,22001 has bugs with simple
suspend.

When NVMe device receive D3hot from host, NVMe firmware will do
garbage collection. While NVMe device do Garbage collection,
firmware has chance to going incorrect address.
In that case, NVMe storage device goes to no device available state.
Finally, host can't access the device any more.

Quirk devices will not use simple suspend even if HMB is enabled.
In case of poweroff scenario, NVMe receive "PME turn off".
So garbage collection will not be happening.

Liteon(SSSTC) will fix the issue, that's why quirk apply on specific
vendor id and firmware version on specific platform.

Signed-off-by: Jongpil Jung <jongpuls@gmail.com>
---
 drivers/nvme/host/core.c | 43 ++++++++++++++++++++++++++++++++++++++--
 drivers/nvme/host/nvme.h |  4 ++++
 drivers/nvme/host/pci.c  |  6 +++++-
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 95ef4943d8bd..414775a77013 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -7,6 +7,7 @@
 #include <linux/blkdev.h>
 #include <linux/blk-mq.h>
 #include <linux/compat.h>
+#include <linux/dmi.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/hdreg.h>
@@ -2637,6 +2638,36 @@ static const struct nvme_core_quirk_entry core_quirks[] = {
 		.vid = 0x14a4,
 		.fr = "22301111",
 		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
+	},
+	{
+		/*
+		 * This LiteON CL1-3D256 CR22001 firmware version has some
+		 * issue in simple suspend.
+		 * Simple Suspend issue will be fixed in future firmware
+		 */
+		.vid = 0x14a4,
+		.fr = "CR22001",
+		.quirks = NVME_QUIRK_NORMAL_SUSPEND_HMB,
+	},
+		{
+		/*
+		 * This LiteON CL1-3D256 CR220TQ firmware version has some
+		 * issue in simple suspend.
+		 * Simple Suspend issue will be fixed in future firmware
+		 */
+		.vid = 0x14a4,
+		.fr = "CR220TQ",
+		.quirks = NVME_QUIRK_NORMAL_SUSPEND_HMB,
+	},
+	{
+		/*
+		 * This SSSTC CL1-3D256 CR22001 firmware version has some
+		 * issue in simple suspend.
+		 * Simple Suspend issue will be fixed in future firmware
+		 */
+		.vid = 0x1e95,
+		.fr = "CR22001",
+		.quirks = NVME_QUIRK_NORMAL_SUSPEND_HMB,
 	}
 };
 
@@ -3007,10 +3038,18 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 		 * could re-scan for quirks every time we reinitialize
 		 * the device, but we'd have to make sure that the driver
 		 * behaves intelligently if the quirks change.
+		 * For NVME_QUIRK_NORMAL_SUSPEND_HMB, need to check board
+		 * as well.
 		 */
 		for (i = 0; i < ARRAY_SIZE(core_quirks); i++) {
-			if (quirk_matches(id, &core_quirks[i]))
-				ctrl->quirks |= core_quirks[i].quirks;
+			if (quirk_matches(id, &core_quirks[i])) {
+				if (core_quirks[i].quirks == NVME_QUIRK_NORMAL_SUSPEND_HMB) {
+					if (dmi_match(DMI_BOARD_VENDOR, "Google") &&
+					    dmi_match(DMI_BOARD_NAME, "Kohaku"))
+						ctrl->quirks |= core_quirks[i].quirks;
+				} else
+					ctrl->quirks |= core_quirks[i].quirks;
+			}
 		}
 	}
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index cc111136a981..2fde019dad8e 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -144,6 +144,10 @@ enum nvme_quirks {
 	 * NVMe 1.3 compliance.
 	 */
 	NVME_QUIRK_NO_NS_DESC_LIST		= (1 << 15),
+	/*
+	 * Force noraml suspend/resume path for HMB enabled devices.
+	 */
+	NVME_QUIRK_NORMAL_SUSPEND_HMB		= (1 << 16),
 };
 
 /*
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index df8f3612107f..1b1221cfb257 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3014,10 +3014,14 @@ static int nvme_suspend(struct device *dev)
 	 * specification allows the device to access the host memory buffer in
 	 * host DRAM from all power states, but hosts will fail access to DRAM
 	 * during S3.
+	 *
+	 * If NVME_QUIRK_NORMAL_SUSPEND_HMB is enabled,
+	 * do not use SIMPLE_SUSPEND for HMB device.
 	 */
 	if (pm_suspend_via_firmware() || !ctrl->npss ||
 	    !pcie_aspm_enabled(pdev) ||
-	    ndev->nr_host_mem_descs ||
+	    (ndev->nr_host_mem_descs &&
+	     !(ndev->ctrl.quirks & NVME_QUIRK_NORMAL_SUSPEND_HMB)) ||
 	    (ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND))
 		return nvme_disable_prepare_reset(ndev, true);
 
-- 
2.25.1

Re: [PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001

[Christoph Hellwig]

On Wed, Oct 28, 2020 at 06:14:21PM +0900, Jongpil Jung wrote:
> LiteON(SSSTC) CL1 device running FW 220TQ,22001 has bugs with simple
> suspend.
> 
> When NVMe device receive D3hot from host, NVMe firmware will do
> garbage collection. While NVMe device do Garbage collection,
> firmware has chance to going incorrect address.

I'm still lost on what this means.  The device doesn't receive 'D3hot'
from the host, it receives a shutdown request.  And we don't just do
that when hibernating, but also when doing an actual shutdown, a suspend
to disk, unbinding the driver, etc.  So if the device has a problem
with shutdowns we really are in trouble.

RE: [PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001

[Gloria Tsai]

Corrected the description of this bug that SSD will not do GC after receiving shutdown cmd.
Do GC before shutdown -> delete IO Q -> shutdown from host -> breakup GC -> D3hot -> enter PS4 -> have a chance swap block -> use wrong pointer on device SRAM -> over program

SSD gets in low power mode only in suspend, this issue only happens in low power mode. 
As for Hibernate, power off, unbinding the driver or even warm boot, EC boot, SSD won't be in low power state, so there'll be no problem.


Regards,
Gloria Tsai
_____________________________________

Sales PM Division
Solid State Storage Technology Corporation
TEL: +886-3-612-3888 ext. 2201
E-Mail: gloria.tsai@ssstc.com
_____________________________________

-----Original Message-----
From: Christoph Hellwig <hch@lst.de> 
Sent: Thursday, October 29, 2020 1:17 AM
To: Jongpil Jung <jongpuls@gmail.com>
Cc: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>; Christoph Hellwig <hch@lst.de>; Sagi Grimberg <sagi@grimberg.me>; linux-nvme@lists.infradead.org; linux-kernel@vger.kernel.org; Gloria Tsai <Gloria.Tsai@ssstc.com>; jongpil19.jung@samsung.com; jongheony.kim@samsung.com; dj54.sohn@samsung.com
Subject: Re: [PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001



This message was sent from outside of the company. Please do not click links or open attachments unless you recognize the source of this email and know the content is safe.


On Wed, Oct 28, 2020 at 06:14:21PM +0900, Jongpil Jung wrote:
> LiteON(SSSTC) CL1 device running FW 220TQ,22001 has bugs with simple 
> suspend.
>
> When NVMe device receive D3hot from host, NVMe firmware will do 
> garbage collection. While NVMe device do Garbage collection, firmware 
> has chance to going incorrect address.

I'm still lost on what this means.  The device doesn't receive 'D3hot'
from the host, it receives a shutdown request.  And we don't just do that when hibernating, but also when doing an actual shutdown, a suspend to disk, unbinding the driver, etc.  So if the device has a problem with shutdowns we really are in trouble.

Re: [PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001

[Keith Busch]

On Thu, Oct 29, 2020 at 02:20:27AM +0000, Gloria Tsai wrote:
> Corrected the description of this bug that SSD will not do GC after receiving shutdown cmd.
> Do GC before shutdown -> delete IO Q -> shutdown from host -> breakup GC -> D3hot -> enter PS4 -> have a chance swap block -> use wrong pointer on device SRAM -> over program

What do you mean by "wrong pointer"? At the place in the sequence you're
referring to, the PCI BME is disabled: you can't access *any* host RAM,
so there's no "correct" pointer either.

Re: [PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001

[Keith Busch]

On Thu, Oct 29, 2020 at 11:33:06AM +0900, Keith Busch wrote:
> On Thu, Oct 29, 2020 at 02:20:27AM +0000, Gloria Tsai wrote:
> > Corrected the description of this bug that SSD will not do GC after receiving shutdown cmd.
> > Do GC before shutdown -> delete IO Q -> shutdown from host -> breakup GC -> D3hot -> enter PS4 -> have a chance swap block -> use wrong pointer on device SRAM -> over program
> 
> What do you mean by "wrong pointer"? At the place in the sequence you're
> referring to, the PCI BME is disabled: you can't access *any* host RAM,
> so there's no "correct" pointer either.

Re-reading your message, I do see you said "device" rather than "host",
so my response may not be relevant.

RE: [PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001

[Gloria Tsai]

Correct, it's device SRAM.


Regards,
Gloria Tsai
_____________________________________

Sales PM Division
Solid State Storage Technology Corporation
TEL: +886-3-612-3888 ext. 2201
E-Mail: gloria.tsai@ssstc.com
_____________________________________

-----Original Message-----
From: Keith Busch <kbusch@kernel.org> 
Sent: Thursday, October 29, 2020 11:15 AM
To: Gloria Tsai <Gloria.Tsai@ssstc.com>
Cc: Jongpil Jung <jongpuls@gmail.com>; Sagi Grimberg <sagi@grimberg.me>; linux-kernel@vger.kernel.org; linux-nvme@lists.infradead.org; Jens Axboe <axboe@fb.com>; dj54.sohn@samsung.com; jongpil19.jung@samsung.com; jongheony.kim@samsung.com; Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001



This message was sent from outside of the company. Please do not click links or open attachments unless you recognize the source of this email and know the content is safe.


On Thu, Oct 29, 2020 at 11:33:06AM +0900, Keith Busch wrote:
> On Thu, Oct 29, 2020 at 02:20:27AM +0000, Gloria Tsai wrote:
> > Corrected the description of this bug that SSD will not do GC after receiving shutdown cmd.
> > Do GC before shutdown -> delete IO Q -> shutdown from host -> 
> > breakup GC -> D3hot -> enter PS4 -> have a chance swap block -> use 
> > wrong pointer on device SRAM -> over program
>
> What do you mean by "wrong pointer"? At the place in the sequence 
> you're referring to, the PCI BME is disabled: you can't access *any* 
> host RAM, so there's no "correct" pointer either.

Re-reading your message, I do see you said "device" rather than "host", so my response may not be relevant.

Re: [PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001

[Christoph Hellwig]

I'm still worried about this.

If power state based suspend does always work despite a HMB and is
preferred for the specific Google board we should have purely a DMI
based quirk for the board independent of the NVMe controller used with
it.

But if these LiteON devices can't properly handle nvme_dev_disable
calls we have much deeper problems, because it can be called in all
kinds of places, including suspending when not on this specific board.

That being said, I still really do not understand this sentence and thus
the problem at all:

> When NVMe device receive D3hot from host, NVMe firmware will do
> garbage collection. While NVMe device do Garbage collection,
> firmware has chance to going incorrect address.

Re: [PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001

[Christoph Hellwig]

On Thu, Oct 29, 2020 at 03:55:29PM +0100, Christoph Hellwig wrote:
> I'm still worried about this.
> 
> If power state based suspend does always work despite a HMB and is
> preferred for the specific Google board we should have purely a DMI
> based quirk for the board independent of the NVMe controller used with
> it.
> 
> But if these LiteON devices can't properly handle nvme_dev_disable
> calls we have much deeper problems, because it can be called in all
> kinds of places, including suspending when not on this specific board.
> 
> That being said, I still really do not understand this sentence and thus
> the problem at all:
> 
> > When NVMe device receive D3hot from host, NVMe firmware will do
> > garbage collection. While NVMe device do Garbage collection,
> > firmware has chance to going incorrect address.

Any progress in describing the problem a little better?

RE: [PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001

[Gloria Tsai]

Rephrased the problem description here,
When host issue shutdown + D3hot in suspend, NVMe drive might have chance choosing wrong pointer which has already been used by GC then cause over program.
Do GC before shutdown -> delete IO Q -> shutdown from host -> breakup GC -> D3hot -> enter PS4 -> have a chance swap block -> use wrong pointer on device SRAM -> over program
The issue only happens in simple suspend (shutdown+D3hot) with specific FW on Kahoku board.


Regards,
Gloria Tsai
_____________________________________

Sales PM Division
Solid State Storage Technology Corporation
TEL: +886-3-612-3888 ext. 2201
E-Mail: gloria.tsai@ssstc.com
_____________________________________

-----Original Message-----
From: Christoph Hellwig <hch@lst.de> 
Sent: Tuesday, November 3, 2020 2:13 AM
To: Jongpil Jung <jongpuls@gmail.com>
Cc: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>; Christoph Hellwig <hch@lst.de>; Sagi Grimberg <sagi@grimberg.me>; linux-nvme@lists.infradead.org; linux-kernel@vger.kernel.org; Gloria Tsai <Gloria.Tsai@ssstc.com>; jongpil19.jung@samsung.com; jongheony.kim@samsung.com; dj54.sohn@samsung.com
Subject: Re: [PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001



This message was sent from outside of the company. Please do not click links or open attachments unless you recognize the source of this email and know the content is safe.


On Thu, Oct 29, 2020 at 03:55:29PM +0100, Christoph Hellwig wrote:
> I'm still worried about this.
>
> If power state based suspend does always work despite a HMB and is 
> preferred for the specific Google board we should have purely a DMI 
> based quirk for the board independent of the NVMe controller used with 
> it.
>
> But if these LiteON devices can't properly handle nvme_dev_disable 
> calls we have much deeper problems, because it can be called in all 
> kinds of places, including suspending when not on this specific board.
>
> That being said, I still really do not understand this sentence and 
> thus the problem at all:
>
> > When NVMe device receive D3hot from host, NVMe firmware will do 
> > garbage collection. While NVMe device do Garbage collection, 
> > firmware has chance to going incorrect address.

Any progress in describing the problem a little better?

Re: [PATCH V3 1/1] nvme: Add quirk for LiteON CL1 devices running FW 220TQ,22001

[Christoph Hellwig]

On Tue, Nov 03, 2020 at 02:21:16AM +0000, Gloria Tsai wrote:
> When host issue shutdown + D3hot in suspend, NVMe drive might have
> chance choosing wrong pointer which has already been used by GC then
> cause over program.
> Do GC before shutdown -> delete IO Q -> shutdown from host -> breakup GC -> D3hot -> enter PS4 -> have a chance swap block -> use wrong pointer on device SRAM -> over program

Aka there is data corruption?

> The issue only happens in simple suspend (shutdown+D3hot) with specific FW on Kahoku board.

Kahoku is a specific LiteOn controller?  Or it is the host system?

Maybe main issue with this patch is that it mixes up two axis:

 - use power states for suspend despite HMB on specific host systems
   identified by the DMI ids.  This kinda makes sense to me, as
   the power state based suspends has lots of advantages, so having
   a whitelist when to use it seem ok, despite the clutter that this
   causes.
 - then tie this to specific NVMe devices that don't work without this
   quirk, which leaves open the issue what we do when we encounter such
   a device in a different host system.  If shutdown + D3hot causes
   problems there is seems like for the case where the above quirk doesn't
   apply we should just skip the shutdown and let the D3hot do a surprise
   power removal?  That's mean recovery when coming back from the suspend,
   but would cause less corruption?

In other words I think this needs to be two patches:

 1) quirk based on the DMI table and allow power state based
    suspend on given host systems even when a HMB is enabled
 2) quirk based on the nvme device (and if possible use the PCI IDs)
    to disable shutdown before suspend (possibly with a warning printk
    when this happens)