Skip to contentSkip to conversation

nvme/quirk: Add a delay before checking for adapter readiness

PATCH New Jeff Lien 2 participants 2 messages Jun 13, 2016 1 message not in the archive NVMe

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jeff Lien

author
#1 lore raw

PATCH v2 1/1 nvme/quirk: Add a delay before checking for adapter readiness

I have reviewed this patch and approve it.

----------------------------------------------------------
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: Guilherme G. Piccoli <[email protected]>
Sent: Wednesday, June 01, 2016 12:54:20 PM
To: linux-nvme at lists.infradead.org
Cc: keith.busch at intel.com; axboe at fb.com; hch at infradead.org; brking at linux.vnet.ibm.com; krisman at linux.vnet.ibm.com; halves at linux.vnet.ibm.com; mniyer at us.ibm.com; David Darrington; Jeff Lien; Guilherme G. Piccoli
Subject: [PATCH v2] 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.

After a firmware activation procedure, the PCI_DEVICE(0x1c58, 0x0003) (HGST adapter) end up being removed if we issue a reset_controller, because driver keeps verifying the NVME_REG_CSTS until the timeout is reached. This patch adds a necessary quirk for this adapter, by introducing a delay before nvme_wait_ready(), so the reset procedure is able to be completed. This quirk is needed because just increasing the timeout is not enough in case of this adapter - the driver must wait before start reading NVME_REG_CSTS register on this specific device.

Signed-off-by Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
---
 drivers/nvme/host/core.c |  7 +++++++
 drivers/nvme/host/nvme.h | 13 +++++++++++++
 drivers/nvme/host/pci.c  |  2 ++
 3 files changed, 22 insertions(+)
drivers/nvme/host/core.c +7 -0
index 1a51584..53e344d 100644
@@ -991,6 +991,13 @@ 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;
+
+       /* Checking for ctrl->tagset is a trick to avoid sleeping on module
+        * load, since we only need the quirk on reset_controller.
+        */
+       if ((ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHK_RDY) && ctrl->tagset)
+               msleep(NVME_QUIRK_DELAY_AMOUNT);
+
        return nvme_wait_ready(ctrl, cap, false);
 }
 EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
drivers/nvme/host/nvme.h +13 -0
index 1daa048..97ac7d1 100644
@@ -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_CHK_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_CHK_RDY quirk enabled. The value (in ms) was
+ * found empirically.
+ */
+#define NVME_QUIRK_DELAY_AMOUNT                2000
+
 enum nvme_ctrl_state {
        NVME_CTRL_NEW,
        NVME_CTRL_LIVE,
drivers/nvme/host/pci.c +2 -0
index 78dca31..d02255c 100644
@@ -2117,6 +2117,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(0x1c58, 0x0003),   /* HGST adapter */
+               .driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
        { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
        { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
        { 0, }

--
2.1.0

#2 lore raw

PATCH v2 1/1 nvme/quirk: Add a delay before checking for adapter readiness

Quoted 2 lines · Jeff Lien

I have reviewed this patch and approve it.

Thanks Jeff, I'll sent a v3 implementing Christoph's suggestion. Cheers,

Guilherme

Quoted 110 lines

----------------------------------------------------------
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: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
Sent: Wednesday, June 01, 2016 12:54:20 PM
To: linux-nvme at lists.infradead.org
Cc: keith.busch at intel.com; axboe at fb.com; hch at infradead.org; brking at linux.vnet.ibm.com; krisman at linux.vnet.ibm.com; halves at linux.vnet.ibm.com; mniyer at us.ibm.com; David Darrington; Jeff Lien; Guilherme G. Piccoli
Subject: [PATCH v2] 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.

After a firmware activation procedure, the PCI_DEVICE(0x1c58, 0x0003) (HGST adapter) end up being removed if we issue a reset_controller, because driver keeps verifying the NVME_REG_CSTS until the timeout is reached. This patch adds a necessary quirk for this adapter, by introducing a delay before nvme_wait_ready(), so the reset procedure is able to be completed. This quirk is needed because just increasing the timeout is not enough in case of this adapter - the driver must wait before start reading NVME_REG_CSTS register on this specific device.

Signed-off-by Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
---
  drivers/nvme/host/core.c |  7 +++++++
  drivers/nvme/host/nvme.h | 13 +++++++++++++
  drivers/nvme/host/pci.c  |  2 ++
  3 files changed, 22 insertions(+)
drivers/nvme/host/core.c +7 -0
index 1a51584..53e344d 100644
@@ -991,6 +991,13 @@ 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;
+
+       /* Checking for ctrl->tagset is a trick to avoid sleeping on module
+        * load, since we only need the quirk on reset_controller.
+        */
+       if ((ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHK_RDY) && ctrl->tagset)
+               msleep(NVME_QUIRK_DELAY_AMOUNT);
+
         return nvme_wait_ready(ctrl, cap, false);
  }
  EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
drivers/nvme/host/nvme.h +13 -0
index 1daa048..97ac7d1 100644
@@ -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_CHK_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_CHK_RDY quirk enabled. The value (in ms) was
+ * found empirically.
+ */
+#define NVME_QUIRK_DELAY_AMOUNT                2000
+
  enum nvme_ctrl_state {
         NVME_CTRL_NEW,
         NVME_CTRL_LIVE,
drivers/nvme/host/pci.c +2 -1
index 78dca31..d02255c 100644
@@ -2117,6 +2117,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(0x1c58, 0x0003),   /* HGST adapter */
+               .driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
         { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
         { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
         { 0, }
--

2.1.0

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