* [PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end @ 2016-04-11 20:15 Guilherme G. Piccoli 2016-04-11 20:15 ` [PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness Guilherme G. Piccoli 2016-04-11 20:48 ` [PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end Matthew Wilcox 0 siblings, 2 replies; 10+ messages in thread From: Guilherme G. Piccoli @ 2016-04-11 20:15 UTC (permalink / raw) 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness 2016-04-11 20:15 [PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end Guilherme G. Piccoli @ 2016-04-11 20:15 ` Guilherme G. Piccoli 2016-04-12 17:56 ` Jeff Lien 2016-04-12 17:58 ` David Darrington 2016-04-11 20:48 ` [PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end Matthew Wilcox 1 sibling, 2 replies; 10+ messages in thread From: Guilherme G. Piccoli @ 2016-04-11 20:15 UTC (permalink / raw) 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness 2016-04-11 20:15 ` [PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness Guilherme G. Piccoli @ 2016-04-12 17:56 ` Jeff Lien [not found] ` <201604121948.u3CJmrdR002584@d03av01.boulder.ibm.com> 2016-04-12 17:58 ` David Darrington 1 sibling, 1 reply; 10+ messages in thread From: Jeff Lien @ 2016-04-12 17:56 UTC (permalink / raw) 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
[parent not found: <201604121948.u3CJmrdR002584@d03av01.boulder.ibm.com>]
* [PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness [not found] ` <201604121948.u3CJmrdR002584@d03av01.boulder.ibm.com> @ 2016-04-12 20:02 ` Keith Busch 2016-04-12 20:13 ` Guilherme G. Piccoli 0 siblings, 1 reply; 10+ messages in thread From: Keith Busch @ 2016-04-12 20:02 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness 2016-04-12 20:02 ` Keith Busch @ 2016-04-12 20:13 ` Guilherme G. Piccoli 0 siblings, 0 replies; 10+ messages in thread From: Guilherme G. Piccoli @ 2016-04-12 20:13 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness 2016-04-11 20:15 ` [PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness Guilherme G. Piccoli 2016-04-12 17:56 ` Jeff Lien @ 2016-04-12 17:58 ` David Darrington 2016-04-12 20:12 ` Guilherme G. Piccoli 1 sibling, 1 reply; 10+ messages in thread From: David Darrington @ 2016-04-12 17:58 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness 2016-04-12 17:58 ` David Darrington @ 2016-04-12 20:12 ` Guilherme G. Piccoli 2016-04-12 20:31 ` David Darrington 0 siblings, 1 reply; 10+ messages in thread From: Guilherme G. Piccoli @ 2016-04-12 20:12 UTC (permalink / raw) 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. > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness 2016-04-12 20:12 ` Guilherme G. Piccoli @ 2016-04-12 20:31 ` David Darrington 0 siblings, 0 replies; 10+ messages in thread From: David Darrington @ 2016-04-12 20:31 UTC (permalink / raw) 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. >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end 2016-04-11 20:15 [PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end Guilherme G. Piccoli 2016-04-11 20:15 ` [PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness Guilherme G. Piccoli @ 2016-04-11 20:48 ` Matthew Wilcox 2016-04-11 20:57 ` Guilherme G. Piccoli 1 sibling, 1 reply; 10+ messages in thread From: Matthew Wilcox @ 2016-04-11 20:48 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end 2016-04-11 20:48 ` [PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end Matthew Wilcox @ 2016-04-11 20:57 ` Guilherme G. Piccoli 0 siblings, 0 replies; 10+ messages in thread From: Guilherme G. Piccoli @ 2016-04-11 20:57 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-04-12 20:31 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-11 20:15 [PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end Guilherme G. Piccoli
2016-04-11 20:15 ` [PATCH 2/2] nvme/quirk: Add a delay before checking for adapter readiness Guilherme G. Piccoli
2016-04-12 17:56 ` Jeff Lien
[not found] ` <201604121948.u3CJmrdR002584@d03av01.boulder.ibm.com>
2016-04-12 20:02 ` Keith Busch
2016-04-12 20:13 ` Guilherme G. Piccoli
2016-04-12 17:58 ` David Darrington
2016-04-12 20:12 ` Guilherme G. Piccoli
2016-04-12 20:31 ` David Darrington
2016-04-11 20:48 ` [PATCH 1/2] nvme: Fix pci_device_id table by adding the most generic devices in the end Matthew Wilcox
2016-04-11 20:57 ` Guilherme G. Piccoli
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.