[PATCH] nvme: Quirk for Samsung PM1733 controllers

[PATCH] nvme: Quirk for Samsung PM1733 controllers

[christopher.walker]

From: Christopher Walker <christopher.walker@gmail.com>

Accommodate the drive-ready times of Samsung 1733 controllers, which
range from 2s for the 2TB model up to 4s for the 16TB model.

Signed-off-by: Christopher Walker <christopher.walker@gmail.com>
---
 drivers/nvme/host/core.c |  3 +++
 drivers/nvme/host/nvme.h | 16 ++++++++++++++++
 drivers/nvme/host/pci.c  |  2 ++
 3 files changed, 21 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c2c5bc4..9a5ebbe 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2234,6 +2234,9 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl)
 	if (ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHK_RDY)
 		msleep(NVME_QUIRK_DELAY_AMOUNT);
 
+	if (ctrl->quirks & NVME_QUIRK_LONG_DELAY_BEFORE_CHK_RDY)
+		msleep(NVME_QUIRK_LONG_DELAY_AMOUNT);
+
 	return nvme_wait_ready(ctrl, ctrl->cap, false);
 }
 EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index c0f4226..8174032 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -129,6 +129,12 @@ enum nvme_quirks {
 	 * Don't change the value of the temperature threshold feature
 	 */
 	NVME_QUIRK_NO_TEMP_THRESH_CHANGE	= (1 << 14),
+
+	/*
+	 * Samsung 1733 controllers need a longer delay before checking device
+	 * readiness
+	 */
+	NVME_QUIRK_LONG_DELAY_BEFORE_CHK_RDY	= (1 << 15),
 };
 
 /*
@@ -173,6 +179,16 @@ static inline u16 nvme_req_qid(struct request *req)
  */
 #define NVME_QUIRK_DELAY_AMOUNT		2300
 
+/* Samsung 1733 drives have long drive ready times.  From the spec:
+ * 15.36TB: 4s
+ *  7.68TB: 3s
+ *  3.84TB: 2s
+ *  1.92TB: 2s
+ * The delay below accommodates the 15.36TB drive at the expense of the
+ * smaller drives.
+ */
+#define NVME_QUIRK_LONG_DELAY_AMOUNT	4300
+
 enum nvme_ctrl_state {
 	NVME_CTRL_NEW,
 	NVME_CTRL_LIVE,
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e2bacd3..7ef4867 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3109,6 +3109,8 @@ static void nvme_error_resume(struct pci_dev *pdev)
 		.driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
 	{ PCI_DEVICE(0x144d, 0xa822),   /* Samsung PM1725a */
 		.driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
+	{ PCI_DEVICE(0x144d, 0xa824),   /* Samsung PM1733 */
+		.driver_data = NVME_QUIRK_LONG_DELAY_BEFORE_CHK_RDY, },
 	{ PCI_DEVICE(0x1d1d, 0x1f1f),	/* LighNVM qemu device */
 		.driver_data = NVME_QUIRK_LIGHTNVM, },
 	{ PCI_DEVICE(0x1d1d, 0x2807),	/* CNEX WL */
-- 
1.8.3.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: Quirk for Samsung PM1733 controllers

[Chaitanya Kulkarni]

> +/* Samsung 1733 drives have long drive ready times.  From the spec:
> + * 15.36TB: 4s
> + *  7.68TB: 3s
> + *  3.84TB: 2s
> + *  1.92TB: 2s
> + * The delay below accommodates the 15.36TB drive at the expense of the
> + * smaller drives.
> + */

I'm not sure if this is a right way to comment the code, I'll let others 
decide whether to have spec information duplicated here.


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: Quirk for Samsung PM1733 controllers

[Keith Busch]

On Mon, Jun 15, 2020 at 05:12:22PM -0600, christopher.walker@gmail.com wrote:
> From: Christopher Walker <christopher.walker@gmail.com>
> 
> Accommodate the drive-ready times of Samsung 1733 controllers, which
> range from 2s for the 2TB model up to 4s for the 16TB model.

Wait, why is this necessary? We poll on the drive to tell us it's ready.
What is the hard-coded delay table doing here?

The original "delay" quirk was introduced to work-around a dodgy
controller that would break if MMIO happened too soon after toggling
CC.EN. This patch doesn't sound like that, so what exactly is it working
around?

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: Quirk for Samsung PM1733 controllers

[Chris Walker]

On Mon, Jun 15, 2020 at 7:42 PM Keith Busch <kbusch@kernel.org> wrote:
>
> On Mon, Jun 15, 2020 at 05:12:22PM -0600, christopher.walker@gmail.com wrote:
> > From: Christopher Walker <christopher.walker@gmail.com>
> >
> > Accommodate the drive-ready times of Samsung 1733 controllers, which
> > range from 2s for the 2TB model up to 4s for the 16TB model.
>
> Wait, why is this necessary? We poll on the drive to tell us it's ready.
> What is the hard-coded delay table doing here?
>
> The original "delay" quirk was introduced to work-around a dodgy
> controller that would break if MMIO happened too soon after toggling
> CC.EN. This patch doesn't sound like that, so what exactly is it working
> around?

I was extrapolating from the quirk added for the 1725 series since the
symptom here is the same  ... without this delay, on hot plug we
consistently see:
...
Jun 13 17:51:42 s-lmo-gaz25a kernel: blk_update_request: critical
target error, dev nvme2n1, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1
prio class 0
Jun 13 17:51:42 s-lmo-gaz25a kernel: Buffer I/O error on dev nvme2n1,
logical block 0, async page read
Jun 13 17:51:42 s-lmo-gaz25a kernel: blk_update_request: critical
target error, dev nvme2n1, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1
prio class 0
Jun 13 17:51:42 s-lmo-gaz25a kernel: Buffer I/O error on dev nvme2n1,
logical block 0, async page read
Jun 13 17:51:42 s-lmo-gaz25a kernel: nvme2n1: unable to read partition table

and no partition /dev devices are present.

Thanks,
Chris

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: Quirk for Samsung PM1733 controllers

[Keith Busch]

On Mon, Jun 15, 2020 at 07:53:02PM -0400, Chris Walker wrote:
> On Mon, Jun 15, 2020 at 7:42 PM Keith Busch <kbusch@kernel.org> wrote:
> 
> > On Mon, Jun 15, 2020 at 05:12:22PM -0600, christopher.walker@gmail.com
> > wrote:
> > > From: Christopher Walker <christopher.walker@gmail.com>
> > >
> > > Accommodate the drive-ready times of Samsung 1733 controllers, which
> > > range from 2s for the 2TB model up to 4s for the 16TB model.
> >
> > Wait, why is this necessary? We poll on the drive to tell us it's ready.
> > What is the hard-coded delay table doing here?
> >
> > The original "delay" quirk was introduced to work-around a dodgy
> > controller that would break if MMIO happened too soon after toggling
> > CC.EN. This patch doesn't sound like that, so what exactly is it working
> > around?
> >
> 
> I was extrapolating from the quirk added for the 1725 series since the
> symptom here is the same  ... without this delay, on hot plug we
> consistently see:
> ...
> Jun 13 17:51:42 s-lmo-gaz25a kernel: blk_update_request: critical target
> error, dev nvme2n1, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
> Jun 13 17:51:42 s-lmo-gaz25a kernel: Buffer I/O error on dev nvme2n1,
> logical block 0, async page read
> Jun 13 17:51:42 s-lmo-gaz25a kernel: blk_update_request: critical target
> error, dev nvme2n1, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
> Jun 13 17:51:42 s-lmo-gaz25a kernel: Buffer I/O error on dev nvme2n1,
> logical block 0, async page read
> Jun 13 17:51:42 s-lmo-gaz25a kernel: nvme2n1: unable to read partition
> table
> 
> and no partition /dev devices are present.

Okay, so it's one of those types of nvme controllers that report "ready"
then only return "not ready" error status for an arbitrary amount of
time. The technical committee is considering how to get rid of this
arbitrary wait, either with existing mechanisms or something new. We'll
have to wait to see how they decide.

In the meantime for drives behaving like this, the quirk delay should
have come *after* nvme_wait_ready() rather than before since namespace
readiness happens after controller ready. The currently existing quirks
appear to be wrongly applied to most of the controllers using them.
AFAIK, only one controller ever existed that actually needs the delay
before wait_ready().

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: Quirk for Samsung PM1733 controllers

[Christoph Hellwig]

On Mon, Jun 15, 2020 at 05:23:06PM -0700, Keith Busch wrote:
> Okay, so it's one of those types of nvme controllers that report "ready"
> then only return "not ready" error status for an arbitrary amount of
> time. The technical committee is considering how to get rid of this
> arbitrary wait, either with existing mechanisms or something new. We'll
> have to wait to see how they decide.
> 
> In the meantime for drives behaving like this, the quirk delay should
> have come *after* nvme_wait_ready() rather than before since namespace
> readiness happens after controller ready. The currently existing quirks
> appear to be wrongly applied to most of the controllers using them.
> AFAIK, only one controller ever existed that actually needs the delay
> before wait_ready().

I think we could increase the number of retries to something larger than
nvme_max_retries for Inquiry in general to handle these controllers.
We'll still need some reasonable upper bound, though.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme