All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] nvme: Add quirk for LiteON CL1 devices running FW 22301111
@ 2019-08-14 16:05 ` Mario Limonciello
  0 siblings, 0 replies; 8+ messages in thread
From: Mario Limonciello @ 2019-08-14 16:05 UTC (permalink / raw)


One of the components in LiteON CL1 device has limitations that
can be encountered based upon boundary race conditions using the
nvme bus specific suspend to idle flow.

When this situation occurs the drive doesn't resume properly from
suspend-to-idle.

LiteON has confirmed this problem and fixed in the next firmware
version.  As this firmware is already in the field, avoid running
nvme specific suspend to idle flow.

Fixes: d916b1be94b6 ("nvme-pci: use host managed power state for suspend")
Link: http://lists.infradead.org/pipermail/linux-nvme/2019-July/thread.html
Signed-off-by: Mario Limonciello <mario.limonciello at dell.com>
Signed-off-by: Charles Hyde <charles.hyde at dellteam.com>
---
 drivers/nvme/host/core.c | 23 +++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  5 +++++
 drivers/nvme/host/pci.c  |  4 +++-
 3 files changed, 31 insertions(+), 1 deletion(-)

This patch is the spiritual successor to the previously submitted
patch "[PATCH] drivers/nvme: save/restore HMB on suspend/resume".

After discussion with LiteON, they agreed to resolve the issue
in their next firwmare release.

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8f3fbe5..47c7754 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2251,6 +2251,29 @@ static const struct nvme_core_quirk_entry core_quirks[] = {
 		.vid = 0x1179,
 		.mn = "THNSF5256GPUK TOSHIBA",
 		.quirks = NVME_QUIRK_NO_APST,
+	},
+	/*
+	 * This LiteON CL1 firmware version has a race condition associated with
+	 * actions related to suspend to idle.  LiteON has resolved the problem
+	 * in future firmware.
+	 */
+	{
+		.vid = 0x14a4,
+		.mn = "CL1-3D128-Q11 NVMe LITEON 128GB",
+		.fr = "22301111",
+		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
+	},
+	{
+		.vid = 0x14a4,
+		.mn = "CL1-3D256-Q11 NVMe LITEON 256GB",
+		.fr = "22301111",
+		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
+	},
+	{
+		.vid = 0x14a4,
+		.mn = "CL1-3D512-Q11 NVMe LITEON 512GB",
+		.fr = "22301111",
+		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
 	}
 };
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 26b563f..fe1ca0d 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -92,6 +92,11 @@ enum nvme_quirks {
 	 * Broken Write Zeroes.
 	 */
 	NVME_QUIRK_DISABLE_WRITE_ZEROES		= (1 << 9),
+
+	/*
+	 * Force simple suspend/resume path.
+	 */
+	NVME_QUIRK_SIMPLE_SUSPEND		= (1 << 10),
 };
 
 /*
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 108e109..55effb5 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2847,6 +2847,7 @@ static int nvme_resume(struct device *dev)
 	struct nvme_ctrl *ctrl = &ndev->ctrl;
 
 	if (ndev->last_ps == U32_MAX ||
+	    (ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND) ||
 	    nvme_set_power_state(ctrl, ndev->last_ps) != 0)
 		nvme_reset_ctrl(ctrl);
 	return 0;
@@ -2875,7 +2876,8 @@ static int nvme_suspend(struct device *dev)
 	 * state (which may not be possible if the link is up).
 	 */
 	if (pm_suspend_via_firmware() || !ctrl->npss ||
-	    !pcie_aspm_enabled(pdev)) {
+	    !pcie_aspm_enabled(pdev) ||
+	    (ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND)) {
 		nvme_dev_disable(ndev, true);
 		return 0;
 	}
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH] nvme: Add quirk for LiteON CL1 devices running FW 22301111
@ 2019-08-14 16:05 ` Mario Limonciello
  0 siblings, 0 replies; 8+ messages in thread
From: Mario Limonciello @ 2019-08-14 16:05 UTC (permalink / raw)
  To: Keith Busch
  Cc: Jens Axboe, Christoph Hellwig, Sagi Grimberg, linux-nvme, LKML,
	Ryan Hong, Crag Wang, sjg, Charles Hyde, Jared Dominguez,
	Mario Limonciello

One of the components in LiteON CL1 device has limitations that
can be encountered based upon boundary race conditions using the
nvme bus specific suspend to idle flow.

When this situation occurs the drive doesn't resume properly from
suspend-to-idle.

LiteON has confirmed this problem and fixed in the next firmware
version.  As this firmware is already in the field, avoid running
nvme specific suspend to idle flow.

Fixes: d916b1be94b6 ("nvme-pci: use host managed power state for suspend")
Link: http://lists.infradead.org/pipermail/linux-nvme/2019-July/thread.html
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
Signed-off-by: Charles Hyde <charles.hyde@dellteam.com>
---
 drivers/nvme/host/core.c | 23 +++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  5 +++++
 drivers/nvme/host/pci.c  |  4 +++-
 3 files changed, 31 insertions(+), 1 deletion(-)

This patch is the spiritual successor to the previously submitted
patch "[PATCH] drivers/nvme: save/restore HMB on suspend/resume".

After discussion with LiteON, they agreed to resolve the issue
in their next firwmare release.

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8f3fbe5..47c7754 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2251,6 +2251,29 @@ static const struct nvme_core_quirk_entry core_quirks[] = {
 		.vid = 0x1179,
 		.mn = "THNSF5256GPUK TOSHIBA",
 		.quirks = NVME_QUIRK_NO_APST,
+	},
+	/*
+	 * This LiteON CL1 firmware version has a race condition associated with
+	 * actions related to suspend to idle.  LiteON has resolved the problem
+	 * in future firmware.
+	 */
+	{
+		.vid = 0x14a4,
+		.mn = "CL1-3D128-Q11 NVMe LITEON 128GB",
+		.fr = "22301111",
+		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
+	},
+	{
+		.vid = 0x14a4,
+		.mn = "CL1-3D256-Q11 NVMe LITEON 256GB",
+		.fr = "22301111",
+		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
+	},
+	{
+		.vid = 0x14a4,
+		.mn = "CL1-3D512-Q11 NVMe LITEON 512GB",
+		.fr = "22301111",
+		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
 	}
 };
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 26b563f..fe1ca0d 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -92,6 +92,11 @@ enum nvme_quirks {
 	 * Broken Write Zeroes.
 	 */
 	NVME_QUIRK_DISABLE_WRITE_ZEROES		= (1 << 9),
+
+	/*
+	 * Force simple suspend/resume path.
+	 */
+	NVME_QUIRK_SIMPLE_SUSPEND		= (1 << 10),
 };
 
 /*
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 108e109..55effb5 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2847,6 +2847,7 @@ static int nvme_resume(struct device *dev)
 	struct nvme_ctrl *ctrl = &ndev->ctrl;
 
 	if (ndev->last_ps == U32_MAX ||
+	    (ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND) ||
 	    nvme_set_power_state(ctrl, ndev->last_ps) != 0)
 		nvme_reset_ctrl(ctrl);
 	return 0;
@@ -2875,7 +2876,8 @@ static int nvme_suspend(struct device *dev)
 	 * state (which may not be possible if the link is up).
 	 */
 	if (pm_suspend_via_firmware() || !ctrl->npss ||
-	    !pcie_aspm_enabled(pdev)) {
+	    !pcie_aspm_enabled(pdev) ||
+	    (ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND)) {
 		nvme_dev_disable(ndev, true);
 		return 0;
 	}
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH] nvme: Add quirk for LiteON CL1 devices running FW 22301111
  2019-08-14 16:05 ` Mario Limonciello
@ 2019-08-14 19:22   ` Sagi Grimberg
  -1 siblings, 0 replies; 8+ messages in thread
From: Sagi Grimberg @ 2019-08-14 19:22 UTC (permalink / raw)


Looks fine to me, Keith can we get a review from you?

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] nvme: Add quirk for LiteON CL1 devices running FW 22301111
@ 2019-08-14 19:22   ` Sagi Grimberg
  0 siblings, 0 replies; 8+ messages in thread
From: Sagi Grimberg @ 2019-08-14 19:22 UTC (permalink / raw)
  To: Mario Limonciello, Keith Busch
  Cc: Crag Wang, sjg, LKML, linux-nvme, Jens Axboe, Ryan Hong,
	Jared Dominguez, Charles Hyde, Christoph Hellwig

Looks fine to me, Keith can we get a review from you?

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH] nvme: Add quirk for LiteON CL1 devices running FW 22301111
  2019-08-14 16:05 ` Mario Limonciello
@ 2019-08-14 19:31   ` Keith Busch
  -1 siblings, 0 replies; 8+ messages in thread
From: Keith Busch @ 2019-08-14 19:31 UTC (permalink / raw)


On Wed, Aug 14, 2019@09:05:49AM -0700, Mario Limonciello wrote:
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 8f3fbe5..47c7754 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2251,6 +2251,29 @@ static const struct nvme_core_quirk_entry core_quirks[] = {
>  		.vid = 0x1179,
>  		.mn = "THNSF5256GPUK TOSHIBA",
>  		.quirks = NVME_QUIRK_NO_APST,
> +	},
> +	/*
> +	 * This LiteON CL1 firmware version has a race condition associated with
> +	 * actions related to suspend to idle.  LiteON has resolved the problem
> +	 * in future firmware.
> +	 */
> +	{
> +		.vid = 0x14a4,
> +		.mn = "CL1-3D128-Q11 NVMe LITEON 128GB",
> +		.fr = "22301111",
> +		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
> +	},
> +	{
> +		.vid = 0x14a4,
> +		.mn = "CL1-3D256-Q11 NVMe LITEON 256GB",
> +		.fr = "22301111",
> +		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
> +	},
> +	{
> +		.vid = 0x14a4,
> +		.mn = "CL1-3D512-Q11 NVMe LITEON 512GB",
> +		.fr = "22301111",
> +		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
>  	}
>  };

Are there models from this vendor with this same 'fr' that don't need
this quirk? If not, you can leave .mn blank and just use a single entry.

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] nvme: Add quirk for LiteON CL1 devices running FW 22301111
@ 2019-08-14 19:31   ` Keith Busch
  0 siblings, 0 replies; 8+ messages in thread
From: Keith Busch @ 2019-08-14 19:31 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Jens Axboe, Christoph Hellwig, Sagi Grimberg,
	linux-nvme@lists.infradead.org, LKML, Ryan Hong, Crag Wang,
	sjg@google.com, Charles Hyde, Jared Dominguez

On Wed, Aug 14, 2019 at 09:05:49AM -0700, Mario Limonciello wrote:
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 8f3fbe5..47c7754 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2251,6 +2251,29 @@ static const struct nvme_core_quirk_entry core_quirks[] = {
>  		.vid = 0x1179,
>  		.mn = "THNSF5256GPUK TOSHIBA",
>  		.quirks = NVME_QUIRK_NO_APST,
> +	},
> +	/*
> +	 * This LiteON CL1 firmware version has a race condition associated with
> +	 * actions related to suspend to idle.  LiteON has resolved the problem
> +	 * in future firmware.
> +	 */
> +	{
> +		.vid = 0x14a4,
> +		.mn = "CL1-3D128-Q11 NVMe LITEON 128GB",
> +		.fr = "22301111",
> +		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
> +	},
> +	{
> +		.vid = 0x14a4,
> +		.mn = "CL1-3D256-Q11 NVMe LITEON 256GB",
> +		.fr = "22301111",
> +		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
> +	},
> +	{
> +		.vid = 0x14a4,
> +		.mn = "CL1-3D512-Q11 NVMe LITEON 512GB",
> +		.fr = "22301111",
> +		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
>  	}
>  };

Are there models from this vendor with this same 'fr' that don't need
this quirk? If not, you can leave .mn blank and just use a single entry.

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH] nvme: Add quirk for LiteON CL1 devices running FW 22301111
  2019-08-14 19:31   ` Keith Busch
@ 2019-08-14 19:51     ` Mario.Limonciello
  -1 siblings, 0 replies; 8+ messages in thread
From: Mario.Limonciello @ 2019-08-14 19:51 UTC (permalink / raw)


> -----Original Message-----
> From: Keith Busch <kbusch at kernel.org>
> Sent: Wednesday, August 14, 2019 2:32 PM
> To: Limonciello, Mario
> Cc: Jens Axboe; Christoph Hellwig; Sagi Grimberg; linux-
> nvme at lists.infradead.org; LKML; Hong, Ryan; Wang, Crag; sjg at google.com;
> Hyde, Charles - Dell Team; Dominguez, Jared
> Subject: Re: [PATCH] nvme: Add quirk for LiteON CL1 devices running FW
> 22301111
> 
> 
> [EXTERNAL EMAIL]
> 
> On Wed, Aug 14, 2019@09:05:49AM -0700, Mario Limonciello wrote:
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 8f3fbe5..47c7754 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -2251,6 +2251,29 @@ static const struct nvme_core_quirk_entry
> core_quirks[] = {
> >  		.vid = 0x1179,
> >  		.mn = "THNSF5256GPUK TOSHIBA",
> >  		.quirks = NVME_QUIRK_NO_APST,
> > +	},
> > +	/*
> > +	 * This LiteON CL1 firmware version has a race condition associated with
> > +	 * actions related to suspend to idle.  LiteON has resolved the problem
> > +	 * in future firmware.
> > +	 */
> > +	{
> > +		.vid = 0x14a4,
> > +		.mn = "CL1-3D128-Q11 NVMe LITEON 128GB",
> > +		.fr = "22301111",
> > +		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
> > +	},
> > +	{
> > +		.vid = 0x14a4,
> > +		.mn = "CL1-3D256-Q11 NVMe LITEON 256GB",
> > +		.fr = "22301111",
> > +		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
> > +	},
> > +	{
> > +		.vid = 0x14a4,
> > +		.mn = "CL1-3D512-Q11 NVMe LITEON 512GB",
> > +		.fr = "22301111",
> > +		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
> >  	}
> >  };
> 
> Are there models from this vendor with this same 'fr' that don't need
> this quirk? If not, you can leave .mn blank and just use a single entry.

Yes, I confirmed this firmware version string is only used on the CL1 family
of devices.

I will send a v2 modifying this.  I also noticed that the
(ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND)

is unnecessary in nvme_resume because ndev->last_ps is set to U32_MAX
in nvme_suspend, so I will remove that second modification.

^ permalink raw reply	[flat|nested] 8+ messages in thread
* RE: [PATCH] nvme: Add quirk for LiteON CL1 devices running FW 22301111
@ 2019-08-14 19:51     ` Mario.Limonciello
  0 siblings, 0 replies; 8+ messages in thread
From: Mario.Limonciello @ 2019-08-14 19:51 UTC (permalink / raw)
  To: kbusch
  Cc: axboe, hch, sagi, linux-nvme, linux-kernel, Ryan.Hong, Crag.Wang,
	sjg, Charles.Hyde, Jared.Dominguez

> -----Original Message-----
> From: Keith Busch <kbusch@kernel.org>
> Sent: Wednesday, August 14, 2019 2:32 PM
> To: Limonciello, Mario
> Cc: Jens Axboe; Christoph Hellwig; Sagi Grimberg; linux-
> nvme@lists.infradead.org; LKML; Hong, Ryan; Wang, Crag; sjg@google.com;
> Hyde, Charles - Dell Team; Dominguez, Jared
> Subject: Re: [PATCH] nvme: Add quirk for LiteON CL1 devices running FW
> 22301111
> 
> 
> [EXTERNAL EMAIL]
> 
> On Wed, Aug 14, 2019 at 09:05:49AM -0700, Mario Limonciello wrote:
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 8f3fbe5..47c7754 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -2251,6 +2251,29 @@ static const struct nvme_core_quirk_entry
> core_quirks[] = {
> >  		.vid = 0x1179,
> >  		.mn = "THNSF5256GPUK TOSHIBA",
> >  		.quirks = NVME_QUIRK_NO_APST,
> > +	},
> > +	/*
> > +	 * This LiteON CL1 firmware version has a race condition associated with
> > +	 * actions related to suspend to idle.  LiteON has resolved the problem
> > +	 * in future firmware.
> > +	 */
> > +	{
> > +		.vid = 0x14a4,
> > +		.mn = "CL1-3D128-Q11 NVMe LITEON 128GB",
> > +		.fr = "22301111",
> > +		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
> > +	},
> > +	{
> > +		.vid = 0x14a4,
> > +		.mn = "CL1-3D256-Q11 NVMe LITEON 256GB",
> > +		.fr = "22301111",
> > +		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
> > +	},
> > +	{
> > +		.vid = 0x14a4,
> > +		.mn = "CL1-3D512-Q11 NVMe LITEON 512GB",
> > +		.fr = "22301111",
> > +		.quirks = NVME_QUIRK_SIMPLE_SUSPEND,
> >  	}
> >  };
> 
> Are there models from this vendor with this same 'fr' that don't need
> this quirk? If not, you can leave .mn blank and just use a single entry.

Yes, I confirmed this firmware version string is only used on the CL1 family
of devices.

I will send a v2 modifying this.  I also noticed that the
(ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND)

is unnecessary in nvme_resume because ndev->last_ps is set to U32_MAX
in nvme_suspend, so I will remove that second modification.

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-08-14 19:51 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-14 16:05 [PATCH] nvme: Add quirk for LiteON CL1 devices running FW 22301111 Mario Limonciello
2019-08-14 16:05 ` Mario Limonciello
2019-08-14 19:22 ` Sagi Grimberg
2019-08-14 19:22   ` Sagi Grimberg
2019-08-14 19:31 ` Keith Busch
2019-08-14 19:31   ` Keith Busch
2019-08-14 19:51   ` Mario.Limonciello
2019-08-14 19:51     ` Mario.Limonciello

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.