nvme/quirk: backport to v4.4 stable for PCI_DEVICE(0x1c58, 0x0003)

Hi Christoph, Jens, and Greg.

Would you ack these 2 backports to be applied to v4.4 stable?

The backports differ from the upstream applied commits in two aspects:
1) use PCI IDs directly rather than nvme quirk infrastructure (which was introduced later, commit 106198edb7 "nvme: add explicit quirk handling", and depends on many further changes, not required for this purpose).

2) use 'struct nvme_dev->tagset' rather than 'struct nvme_ctrl->tagset' (similarly.)

This has been built and tested on top of v4.4.70.

Thanks,

Mauricio Faria de Oliveira
IBM Linux Technology Center

Guilherme G. Piccoli (2):
nvme/quirk: Add a delay before checking for adapter readiness
nvme: apply DELAY_BEFORE_CHK_RDY quirk at probe time too

 drivers/nvme/host/nvme.h | 7 +++++++
 drivers/nvme/host/pci.c  | 5 +++++
 2 files changed, 12 insertions(+)

1.8.3.1

From: "Guilherme G. Piccoli" <[email protected]>

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.

---
 drivers/nvme/host/nvme.h |  7 +++++++
 drivers/nvme/host/pci.c  | 10 ++++++++++
 2 files changed, 17 insertions(+)

index 044253dca30a..b8a5a8e8f57d 100644
@@ -27,6 +27,13 @@ enum {
 	NVME_NS_LIGHTNVM	= 1,
 };
 
+/* 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
+
 /*
  * Represents an NVM Express device.  Each nvme_dev is a PCI function.
  */

index c851bc53831c..e9750a0a6f2c 100644
@@ -1633,10 +1633,20 @@ static int nvme_wait_ready(struct nvme_dev *dev, u64 cap, bool enabled)
  */
 static int nvme_disable_ctrl(struct nvme_dev *dev, u64 cap)
 {
+	struct pci_dev *pdev = to_pci_dev(dev->dev);
+
 	dev->ctrl_config &= ~NVME_CC_SHN_MASK;
 	dev->ctrl_config &= ~NVME_CC_ENABLE;
 	writel(dev->ctrl_config, &dev->bar->cc);
 
+	/* Checking for dev->tagset is a trick to avoid sleeping on module
+	 * load, since we only need the quirk on reset_controller. Notice
+	 * that the HGST device needs this delay only in firmware activation
+	 * procedure; unfortunately we have no (easy) way to verify this.
+	 */
+	if (pdev->vendor == 0x1c58 && pdev->device == 0x0003 && dev->tagset)
+		msleep(NVME_QUIRK_DELAY_AMOUNT);
+
 	return nvme_wait_ready(dev, cap, false);
 }
 

1.8.3.1

From: "Guilherme G. Piccoli" <[email protected]>

Commit 54adc01055b7 ("nvme/quirk: Add a delay before checking for adapter readiness") introduced a quirk to adapters that cannot read the bit NVME_CSTS_RDY right after register NVME_REG_CC is set; these adapters need a delay or else the action of reading the bit NVME_CSTS_RDY could somehow corrupt adapter's registers state and it never recovers.

When this quirk was added, we checked ctrl->tagset in order to avoid quirking in probe time, supposing we would never require such delay during probe. Well, it was too optimistic; we in fact need this quirk at probe time in some cases, like after a kexec.

In some experiments, after abnormal shutdown of machine (aka power cord unplug), we booted into our bootloader in Power, which is a Linux kernel, and kexec'ed into another distro. If this kexec is too quick, we end up reaching the probe of NVMe adapter in that distro when adapter is in bad state (not fully initialized on our bootloader). What happens next is that nvme_wait_ready() is unable to complete, except if the quirk is enabled.

So, this patch removes the original ctrl->tagset verification in order to enable the quirk even on probe time.

---
 drivers/nvme/host/pci.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

index e9750a0a6f2c..4c673d45f1bd 100644
@@ -1639,12 +1639,7 @@ static int nvme_disable_ctrl(struct nvme_dev *dev, u64 cap)
 	dev->ctrl_config &= ~NVME_CC_ENABLE;
 	writel(dev->ctrl_config, &dev->bar->cc);
 
-	/* Checking for dev->tagset is a trick to avoid sleeping on module
-	 * load, since we only need the quirk on reset_controller. Notice
-	 * that the HGST device needs this delay only in firmware activation
-	 * procedure; unfortunately we have no (easy) way to verify this.
-	 */
-	if (pdev->vendor == 0x1c58 && pdev->device == 0x0003 && dev->tagset)
+	if (pdev->vendor == 0x1c58 && pdev->device == 0x0003)
 		msleep(NVME_QUIRK_DELAY_AMOUNT);
 
 	return nvme_wait_ready(dev, cap, false);

1.8.3.1

Applying those two to stable is fine with me.

Jens Axboe

Thanks Maur?cio for the great backport (which I tested and it's working properly, as expected).

And thanks Jens for the quick ack.
Cheers,

Guilherme

Always tell me what the git commit ids are for the patches in Linus's tree, as I need them. I dug it out this time...

thanks,

greg k-h

Thanks a lot Greg! Next time we won't forget.
Cheers,

Guilherme