[PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end

[PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end

[Guilherme G. Piccoli]

The struct pci_device_id is used on a PCI driver probe by the PCI core
mechanism to match the devices present on the system with the driver
being probed. For each device, the match procedure reads each entry in
the pci_device_id struct provided by the driver and tries to match the
first entry it is able to do.

For example, if the table contains a generic entry, like a class generic
id, the PCI core can match the driver to this device ignoring the
following entries on pci_device_id. It means that the most generic entries
should go to the end of the table, to allow the PCI probe mechanism trying
to test the maximum number of members of the id table it can. In this way
we are able to avoid a case in which a generic match is done before the
PCI core is able to test a more specific entry (possible denying some
quirk to be enabled).

This patch moves the PCI_DEVICE_CLASS entry to the end of the struct
nvme_id_table[], so the match to the specific devices will be tried
by the PCI core mechanism before the per-class match can happen.

Signed-off-by: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 24ccda3..220e0b5 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2172,8 +2172,8 @@ static const struct pci_device_id nvme_id_table[] = {
 				NVME_QUIRK_DISCARD_ZEROES, },
 	{ PCI_VDEVICE(INTEL, 0x5845),	/* Qemu emulated controller */
 		.driver_data = NVME_QUIRK_IDENTIFY_CNS, },
-	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
+	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, nvme_id_table);
-- 
2.1.0

[PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness

[Guilherme G. Piccoli]

When disabling the controller, the specification says the register
NVME_REG_CC should be written and then driver needs to wait the
adapter to be ready, which is checked by reading another register
bit (NVME_CSTS_RDY). There's a timeout validation in this checking,
so in case this timeout is reached the driver gives up and removes
the adapter from the system.

The PCI_DEVICE(0x1c58, 0x0003) (HGST adapter) end up being removed
after a reset because driver keeps verifying the NVME_REG_CSTS until
the timeout is reached. This patch adds a quirk for this adapter, by
introducing a delay before nvme_wait_ready(), so the reset procedure
is able to be completed. This quirk is necessary because just
increasing the timeout is not enough - the driver must wait _before_
start reading NVME_REG_CSTS register on this specific adapter.

Signed-off-by: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
---
 drivers/nvme/host/core.c |  4 ++++
 drivers/nvme/host/nvme.h | 13 +++++++++++++
 drivers/nvme/host/pci.c  |  2 ++
 3 files changed, 19 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 643f457..37817bb 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -824,6 +824,10 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap)
 	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
 	if (ret)
 		return ret;
+
+	if (ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHECK_RDY)
+		msleep(NVME_QUIRK_DELAY_AMOUNT_MS);
+
 	return nvme_wait_ready(ctrl, cap, false);
 }
 EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index f846da4..b8247c4 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -65,8 +65,21 @@ enum nvme_quirks {
 	 * logical blocks.
 	 */
 	NVME_QUIRK_DISCARD_ZEROES		= (1 << 2),
+
+	/*
+	* The controller needs a delay before starts checking the device
+	* readiness, which is done by reading the NVME_CSTS_RDY bit.
+	*/
+	NVME_QUIRK_DELAY_BEFORE_CHECK_RDY	= (1 << 3),
 };
 
+/* The below value is the specific amount of delay needed before checking
+ * readiness in case of the PCI_DEVICE(0x1c58, 0x0003), which needs the
+ * NVME_QUIRK_DELAY_BEFORE_CHECK_RDY quirk enabled. The value was found
+ * empirically. The value is in milliseconds.
+ */
+#define NVME_QUIRK_DELAY_AMOUNT_MS		2000
+
 struct nvme_ctrl {
 	const struct nvme_ctrl_ops *ops;
 	struct request_queue *admin_q;
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 220e0b5..a5e3d0e 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2173,6 +2173,8 @@ static const struct pci_device_id nvme_id_table[] = {
 	{ PCI_VDEVICE(INTEL, 0x5845),	/* Qemu emulated controller */
 		.driver_data = NVME_QUIRK_IDENTIFY_CNS, },
 	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
+	{ PCI_DEVICE(0x1c58, 0x0003),	/*HGST adapter */
+		.driver_data = NVME_QUIRK_DELAY_BEFORE_CHECK_RDY, },
 	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
 	{ 0, }
 };
-- 
2.1.0

[PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end

[Matthew Wilcox]

On Mon, Apr 11, 2016@05:15:50PM -0300, Guilherme G. Piccoli wrote:
> The struct pci_device_id is used on a PCI driver probe by the PCI core
> mechanism to match the devices present on the system with the driver
> being probed. For each device, the match procedure reads each entry in
> the pci_device_id struct provided by the driver and tries to match the
> first entry it is able to do.

What you're missing is that the Apple device doesn't have the NVMe
programming model / class code.  So this patch actually has no effect.

[PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end

[Guilherme G. Piccoli]

On 04/11/2016 05:48 PM, Matthew Wilcox wrote:
> On Mon, Apr 11, 2016@05:15:50PM -0300, Guilherme G. Piccoli wrote:
>> The struct pci_device_id is used on a PCI driver probe by the PCI core
>> mechanism to match the devices present on the system with the driver
>> being probed. For each device, the match procedure reads each entry in
>> the pci_device_id struct provided by the driver and tries to match the
>> first entry it is able to do.
>
> What you're missing is that the Apple device doesn't have the NVMe
> programming model / class code.  So this patch actually has no effect.
>

Oh, okay. Thanks for pointing this. I imagined that it would be possible 
that this Apple device does not have NVMe class code (or it would been a 
mistake have added the entry after the PCI_DEVICE_CLASS() in that table).

Anyway, my choice was to send the patch and take the shot. Do you want 
me to re-send the 2nd part of this patch alone, as v2?

Cheers,



Guilherme

[PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness

[Jeff Lien]

Guilherme, 
What kernel level and system architecture are you running when you hit this problem?   I just verified that the nvme_disable_ctrl call works fine with the HGST device on Centos 6.5 with the 2.6.32-431 kernel on x86_64 arch.   

Also, have you been able to verify that other nvme devices do not require the delay patch?

----------------------------------------------------------
Jeff Lien
Linux Device Driver Development
Device Host Apps and Drivers
Western Digital Corporation
e.? jeff.lien at hgst.com
o.? +1-507-322-2416
m. +1-507-273-9124





________________________________________
From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> on behalf of Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
Sent: Monday, April 11, 2016 3:15 PM
To: linux-nvme at lists.infradead.org; keith.busch at intel.com; axboe at fb.com
Cc: brking at linux.vnet.ibm.com; guenther at tum.de; mniyer at us.ibm.com; gpiccoli at linux.vnet.ibm.com
Subject: [PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness

When disabling the controller, the specification says the register
NVME_REG_CC should be written and then driver needs to wait the
adapter to be ready, which is checked by reading another register
bit (NVME_CSTS_RDY). There's a timeout validation in this checking,
so in case this timeout is reached the driver gives up and removes
the adapter from the system.

The PCI_DEVICE(0x1c58, 0x0003) (HGST adapter) end up being removed
after a reset because driver keeps verifying the NVME_REG_CSTS until
the timeout is reached. This patch adds a quirk for this adapter, by
introducing a delay before nvme_wait_ready(), so the reset procedure
is able to be completed. This quirk is necessary because just
increasing the timeout is not enough - the driver must wait _before_
start reading NVME_REG_CSTS register on this specific adapter.

Signed-off-by: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
---
 drivers/nvme/host/core.c |? 4 ++++
 drivers/nvme/host/nvme.h | 13 +++++++++++++
 drivers/nvme/host/pci.c? |? 2 ++
 3 files changed, 19 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 643f457..37817bb 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -824,6 +824,10 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap)
??????? ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
??????? if (ret)
??????????????? return ret;
+
+?????? if (ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHECK_RDY)
+?????????????? msleep(NVME_QUIRK_DELAY_AMOUNT_MS);
+
??????? return nvme_wait_ready(ctrl, cap, false);
 }
 EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index f846da4..b8247c4 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -65,8 +65,21 @@ enum nvme_quirks {
???????? * logical blocks.
???????? */
??????? NVME_QUIRK_DISCARD_ZEROES?????????????? = (1 << 2),
+
+?????? /*
+?????? * The controller needs a delay before starts checking the device
+?????? * readiness, which is done by reading the NVME_CSTS_RDY bit.
+?????? */
+?????? NVME_QUIRK_DELAY_BEFORE_CHECK_RDY?????? = (1 << 3),
 };

+/* The below value is the specific amount of delay needed before checking
+ * readiness in case of the PCI_DEVICE(0x1c58, 0x0003), which needs the
+ * NVME_QUIRK_DELAY_BEFORE_CHECK_RDY quirk enabled. The value was found
+ * empirically. The value is in milliseconds.
+ */
+#define NVME_QUIRK_DELAY_AMOUNT_MS???????????? 2000
+
 struct nvme_ctrl {
??????? const struct nvme_ctrl_ops *ops;
??????? struct request_queue *admin_q;
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 220e0b5..a5e3d0e 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2173,6 +2173,8 @@ static const struct pci_device_id nvme_id_table[] = {
??????? { PCI_VDEVICE(INTEL, 0x5845),?? /* Qemu emulated controller */
??????????????? .driver_data = NVME_QUIRK_IDENTIFY_CNS, },
??????? { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
+?????? { PCI_DEVICE(0x1c58, 0x0003),?? /*HGST adapter */
+?????????????? .driver_data = NVME_QUIRK_DELAY_BEFORE_CHECK_RDY, },
??????? { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
??????? { 0, }
 };
--
2.1.0


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

[PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness

[David Darrington]

When you say, ?This quirk is necessary because just
increasing the timeout is not enough - the driver must wait _before_
start reading NVME_REG_CSTS register on this specific adapter.,?
This doesn?t make sense to me. I?ve never seen this with the
HGST adapters. Have you reported this to HGST as a problem?




On 4/11/16, 3:15 PM, "Linux-nvme on behalf of Guilherme G. Piccoli" <linux-nvme-bounces@lists.infradead.org on behalf of gpiccoli@linux.vnet.ibm.com> wrote:

>When disabling the controller, the specification says the register
>NVME_REG_CC should be written and then driver needs to wait the
>adapter to be ready, which is checked by reading another register
>bit (NVME_CSTS_RDY). There's a timeout validation in this checking,
>so in case this timeout is reached the driver gives up and removes
>the adapter from the system.
>
>The PCI_DEVICE(0x1c58, 0x0003) (HGST adapter) end up being removed
>after a reset because driver keeps verifying the NVME_REG_CSTS until
>the timeout is reached. This patch adds a quirk for this adapter, by
>introducing a delay before nvme_wait_ready(), so the reset procedure
>is able to be completed. This quirk is necessary because just
>increasing the timeout is not enough - the driver must wait _before_
>start reading NVME_REG_CSTS register on this specific adapter.
>
>Signed-off-by: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
>---
> drivers/nvme/host/core.c |  4 ++++
> drivers/nvme/host/nvme.h | 13 +++++++++++++
> drivers/nvme/host/pci.c  |  2 ++
> 3 files changed, 19 insertions(+)
>
>diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>index 643f457..37817bb 100644
>--- a/drivers/nvme/host/core.c
>+++ b/drivers/nvme/host/core.c
>@@ -824,6 +824,10 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap)
> 	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
> 	if (ret)
> 		return ret;
>+
>+	if (ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHECK_RDY)
>+		msleep(NVME_QUIRK_DELAY_AMOUNT_MS);
>+
> 	return nvme_wait_ready(ctrl, cap, false);
> }
> EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
>diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>index f846da4..b8247c4 100644
>--- a/drivers/nvme/host/nvme.h
>+++ b/drivers/nvme/host/nvme.h
>@@ -65,8 +65,21 @@ enum nvme_quirks {
> 	 * logical blocks.
> 	 */
> 	NVME_QUIRK_DISCARD_ZEROES		= (1 << 2),
>+
>+	/*
>+	* The controller needs a delay before starts checking the device
>+	* readiness, which is done by reading the NVME_CSTS_RDY bit.
>+	*/
>+	NVME_QUIRK_DELAY_BEFORE_CHECK_RDY	= (1 << 3),
> };
> 
>+/* The below value is the specific amount of delay needed before checking
>+ * readiness in case of the PCI_DEVICE(0x1c58, 0x0003), which needs the
>+ * NVME_QUIRK_DELAY_BEFORE_CHECK_RDY quirk enabled. The value was found
>+ * empirically. The value is in milliseconds.
>+ */
>+#define NVME_QUIRK_DELAY_AMOUNT_MS		2000
>+
> struct nvme_ctrl {
> 	const struct nvme_ctrl_ops *ops;
> 	struct request_queue *admin_q;
>diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>index 220e0b5..a5e3d0e 100644
>--- a/drivers/nvme/host/pci.c
>+++ b/drivers/nvme/host/pci.c
>@@ -2173,6 +2173,8 @@ static const struct pci_device_id nvme_id_table[] = {
> 	{ PCI_VDEVICE(INTEL, 0x5845),	/* Qemu emulated controller */
> 		.driver_data = NVME_QUIRK_IDENTIFY_CNS, },
> 	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
>+	{ PCI_DEVICE(0x1c58, 0x0003),	/*HGST adapter */
>+		.driver_data = NVME_QUIRK_DELAY_BEFORE_CHECK_RDY, },
> 	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
> 	{ 0, }
> };
>-- 
>2.1.0
>
>
>_______________________________________________
>Linux-nvme mailing list
>Linux-nvme at lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness

[Keith Busch]

On Tue, Apr 12, 2016@02:48:46PM -0500, Murali N Iyer wrote:
> Jeff, David,
> 
> You might not see the issue 100% of the time if you don't activate the
> different slot. Try with any kernel that provides "reset_controller" interface.
> RHEL 7.2, Ubuntu 16.04 etc. does.
> 
> Example: you are running with FW slot #1
> 
> # nvme fw-log /dev/nvme4
> Firmware Log for device:/dev/nvme4
> afi : 0x11
> frs1 : 0x3430315050494d4b (KMIPP104)
> frs2 : 0x3430315050494d4b (KMIPP104)
> frs3 : 0x3430315050494d4b (KMIPP104)
> frs4 : 0x3430315050494d4b (KMIPP104)
> 
> # echo 1 > /sys/class/nvme/nvme4/reset_controller ==.> This might work most of
> the time
> 
> Now, activate slot #4 and then reset_controller
> 
> # nvme fw-activate /dev/nvme4 -a 2 -s 4
> Success activating firmware action:2 slot:4
> 
> # echo 1 > /sys/class/nvme/nvme4/reset_controller ==> This generates EEH

As older kernels did not export the reset_controller interface, unloading
and reloading the driver with modprobe should reproduce the sequence
you're describing. The method is not as convenient, but should be enough
for a 3rd party with the same controller to confirm results.

[PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness

[Guilherme G. Piccoli]

On 04/12/2016 02:58 PM, David Darrington wrote:
> When you say, ?This quirk is necessary because just
> increasing the timeout is not enough - the driver must wait _before_
> start reading NVME_REG_CSTS register on this specific adapter.,?
> This doesn?t make sense to me. I?ve never seen this with the
> HGST adapters. Have you reported this to HGST as a problem?

Hey David, thanks very much for your comment. We're experiencing this 
exact scenario, as described in commit message. Whenever the adapter 
needs to be disabled, if we don't wait before reading the NVME_CSTS_RDY 
bit, we end up reaching the timeout. The read result from this register 
become 0xFFFF in sme time, and adapter is not able to finish the reset.

If, on the other hand, we add this small timeout before start reading 
the NVME_REG_CSTS register, everything works fine. Notice this patch is 
a quirk, so if you are able to reproduce and have a firmware solution, 
for example, it'd be better.

Problem was reported to HGST (Murali is our contact) and the product 
team is unable to find a root cause yet, while testers are hitting the 
issue.

Thank you,



Guilherme


>
> On 4/11/16, 3:15 PM, "Linux-nvme on behalf of Guilherme G. Piccoli" <linux-nvme-bounces@lists.infradead.org on behalf of gpiccoli@linux.vnet.ibm.com> wrote:
>
>> When disabling the controller, the specification says the register
>> NVME_REG_CC should be written and then driver needs to wait the
>> adapter to be ready, which is checked by reading another register
>> bit (NVME_CSTS_RDY). There's a timeout validation in this checking,
>> so in case this timeout is reached the driver gives up and removes
>> the adapter from the system.
>>
>> The PCI_DEVICE(0x1c58, 0x0003) (HGST adapter) end up being removed
>> after a reset because driver keeps verifying the NVME_REG_CSTS until
>> the timeout is reached. This patch adds a quirk for this adapter, by
>> introducing a delay before nvme_wait_ready(), so the reset procedure
>> is able to be completed. This quirk is necessary because just
>> increasing the timeout is not enough - the driver must wait _before_
>> start reading NVME_REG_CSTS register on this specific adapter.
>>
>> Signed-off-by: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
>> ---
>> drivers/nvme/host/core.c |  4 ++++
>> drivers/nvme/host/nvme.h | 13 +++++++++++++
>> drivers/nvme/host/pci.c  |  2 ++
>> 3 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 643f457..37817bb 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -824,6 +824,10 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap)
>> 	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
>> 	if (ret)
>> 		return ret;
>> +
>> +	if (ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHECK_RDY)
>> +		msleep(NVME_QUIRK_DELAY_AMOUNT_MS);
>> +
>> 	return nvme_wait_ready(ctrl, cap, false);
>> }
>> EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
>> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>> index f846da4..b8247c4 100644
>> --- a/drivers/nvme/host/nvme.h
>> +++ b/drivers/nvme/host/nvme.h
>> @@ -65,8 +65,21 @@ enum nvme_quirks {
>> 	 * logical blocks.
>> 	 */
>> 	NVME_QUIRK_DISCARD_ZEROES		= (1 << 2),
>> +
>> +	/*
>> +	* The controller needs a delay before starts checking the device
>> +	* readiness, which is done by reading the NVME_CSTS_RDY bit.
>> +	*/
>> +	NVME_QUIRK_DELAY_BEFORE_CHECK_RDY	= (1 << 3),
>> };
>>
>> +/* The below value is the specific amount of delay needed before checking
>> + * readiness in case of the PCI_DEVICE(0x1c58, 0x0003), which needs the
>> + * NVME_QUIRK_DELAY_BEFORE_CHECK_RDY quirk enabled. The value was found
>> + * empirically. The value is in milliseconds.
>> + */
>> +#define NVME_QUIRK_DELAY_AMOUNT_MS		2000
>> +
>> struct nvme_ctrl {
>> 	const struct nvme_ctrl_ops *ops;
>> 	struct request_queue *admin_q;
>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>> index 220e0b5..a5e3d0e 100644
>> --- a/drivers/nvme/host/pci.c
>> +++ b/drivers/nvme/host/pci.c
>> @@ -2173,6 +2173,8 @@ static const struct pci_device_id nvme_id_table[] = {
>> 	{ PCI_VDEVICE(INTEL, 0x5845),	/* Qemu emulated controller */
>> 		.driver_data = NVME_QUIRK_IDENTIFY_CNS, },
>> 	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
>> +	{ PCI_DEVICE(0x1c58, 0x0003),	/*HGST adapter */
>> +		.driver_data = NVME_QUIRK_DELAY_BEFORE_CHECK_RDY, },
>> 	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
>> 	{ 0, }
>> };
>> --
>> 2.1.0
>>
>>
>> _______________________________________________
>> Linux-nvme mailing list
>> Linux-nvme at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-nvme
> Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer:
>
> This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system.
>

[PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness

[Guilherme G. Piccoli]

On 04/12/2016 05:02 PM, Keith Busch wrote:
> On Tue, Apr 12, 2016@02:48:46PM -0500, Murali N Iyer wrote:
>> Jeff, David,
>>
>> You might not see the issue 100% of the time if you don't activate the
>> different slot. Try with any kernel that provides "reset_controller" interface.
>> RHEL 7.2, Ubuntu 16.04 etc. does.
>>
>> Example: you are running with FW slot #1
>>
>> # nvme fw-log /dev/nvme4
>> Firmware Log for device:/dev/nvme4
>> afi : 0x11
>> frs1 : 0x3430315050494d4b (KMIPP104)
>> frs2 : 0x3430315050494d4b (KMIPP104)
>> frs3 : 0x3430315050494d4b (KMIPP104)
>> frs4 : 0x3430315050494d4b (KMIPP104)
>>
>> # echo 1 > /sys/class/nvme/nvme4/reset_controller ==.> This might work most of
>> the time
>>
>> Now, activate slot #4 and then reset_controller
>>
>> # nvme fw-activate /dev/nvme4 -a 2 -s 4
>> Success activating firmware action:2 slot:4
>>
>> # echo 1 > /sys/class/nvme/nvme4/reset_controller ==> This generates EEH
>
> As older kernels did not export the reset_controller interface, unloading
> and reloading the driver with modprobe should reproduce the sequence
> you're describing. The method is not as convenient, but should be enough
> for a 3rd party with the same controller to confirm results.
>

Thanks Murali and Keith for the detailed explanation.

Cheers,


Guilherme

[PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness

[David Darrington]

Hi Guilherme,

Thanks for the clarification. I?ll followup with the product support team here and attempt to re-create.


Dave

On 4/12/16, 3:12 PM, "Guilherme G. Piccoli" <gpiccoli@linux.vnet.ibm.com> wrote:

>On 04/12/2016 02:58 PM, David Darrington wrote:
>> When you say, ?This quirk is necessary because just
>> increasing the timeout is not enough - the driver must wait _before_
>> start reading NVME_REG_CSTS register on this specific adapter.,?
>> This doesn?t make sense to me. I?ve never seen this with the
>> HGST adapters. Have you reported this to HGST as a problem?
>
>Hey David, thanks very much for your comment. We're experiencing this 
>exact scenario, as described in commit message. Whenever the adapter 
>needs to be disabled, if we don't wait before reading the NVME_CSTS_RDY 
>bit, we end up reaching the timeout. The read result from this register 
>become 0xFFFF in sme time, and adapter is not able to finish the reset.
>
>If, on the other hand, we add this small timeout before start reading 
>the NVME_REG_CSTS register, everything works fine. Notice this patch is 
>a quirk, so if you are able to reproduce and have a firmware solution, 
>for example, it'd be better.
>
>Problem was reported to HGST (Murali is our contact) and the product 
>team is unable to find a root cause yet, while testers are hitting the 
>issue.
>
>Thank you,
>
>
>
>Guilherme
>
>
>>
>> On 4/11/16, 3:15 PM, "Linux-nvme on behalf of Guilherme G. Piccoli" <linux-nvme-bounces@lists.infradead.org on behalf of gpiccoli@linux.vnet.ibm.com> wrote:
>>
>>> When disabling the controller, the specification says the register
>>> NVME_REG_CC should be written and then driver needs to wait the
>>> adapter to be ready, which is checked by reading another register
>>> bit (NVME_CSTS_RDY). There's a timeout validation in this checking,
>>> so in case this timeout is reached the driver gives up and removes
>>> the adapter from the system.
>>>
>>> The PCI_DEVICE(0x1c58, 0x0003) (HGST adapter) end up being removed
>>> after a reset because driver keeps verifying the NVME_REG_CSTS until
>>> the timeout is reached. This patch adds a quirk for this adapter, by
>>> introducing a delay before nvme_wait_ready(), so the reset procedure
>>> is able to be completed. This quirk is necessary because just
>>> increasing the timeout is not enough - the driver must wait _before_
>>> start reading NVME_REG_CSTS register on this specific adapter.
>>>
>>> Signed-off-by: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
>>> ---
>>> drivers/nvme/host/core.c |  4 ++++
>>> drivers/nvme/host/nvme.h | 13 +++++++++++++
>>> drivers/nvme/host/pci.c  |  2 ++
>>> 3 files changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index 643f457..37817bb 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -824,6 +824,10 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap)
>>> 	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
>>> 	if (ret)
>>> 		return ret;
>>> +
>>> +	if (ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHECK_RDY)
>>> +		msleep(NVME_QUIRK_DELAY_AMOUNT_MS);
>>> +
>>> 	return nvme_wait_ready(ctrl, cap, false);
>>> }
>>> EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
>>> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>>> index f846da4..b8247c4 100644
>>> --- a/drivers/nvme/host/nvme.h
>>> +++ b/drivers/nvme/host/nvme.h
>>> @@ -65,8 +65,21 @@ enum nvme_quirks {
>>> 	 * logical blocks.
>>> 	 */
>>> 	NVME_QUIRK_DISCARD_ZEROES		= (1 << 2),
>>> +
>>> +	/*
>>> +	* The controller needs a delay before starts checking the device
>>> +	* readiness, which is done by reading the NVME_CSTS_RDY bit.
>>> +	*/
>>> +	NVME_QUIRK_DELAY_BEFORE_CHECK_RDY	= (1 << 3),
>>> };
>>>
>>> +/* The below value is the specific amount of delay needed before checking
>>> + * readiness in case of the PCI_DEVICE(0x1c58, 0x0003), which needs the
>>> + * NVME_QUIRK_DELAY_BEFORE_CHECK_RDY quirk enabled. The value was found
>>> + * empirically. The value is in milliseconds.
>>> + */
>>> +#define NVME_QUIRK_DELAY_AMOUNT_MS		2000
>>> +
>>> struct nvme_ctrl {
>>> 	const struct nvme_ctrl_ops *ops;
>>> 	struct request_queue *admin_q;
>>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>>> index 220e0b5..a5e3d0e 100644
>>> --- a/drivers/nvme/host/pci.c
>>> +++ b/drivers/nvme/host/pci.c
>>> @@ -2173,6 +2173,8 @@ static const struct pci_device_id nvme_id_table[] = {
>>> 	{ PCI_VDEVICE(INTEL, 0x5845),	/* Qemu emulated controller */
>>> 		.driver_data = NVME_QUIRK_IDENTIFY_CNS, },
>>> 	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
>>> +	{ PCI_DEVICE(0x1c58, 0x0003),	/*HGST adapter */
>>> +		.driver_data = NVME_QUIRK_DELAY_BEFORE_CHECK_RDY, },
>>> 	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
>>> 	{ 0, }
>>> };
>>> --
>>> 2.1.0
>>>
>>>
>>> _______________________________________________
>>> Linux-nvme mailing list
>>> Linux-nvme at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-nvme
>> Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer:
>>
>> This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system.
>>
>