Skip to contentSkip to conversation

nvme/pci: Log PCI_STATUS when the controller dies

PATCH New Andy Lutomirski 4 participants 4 messages Dec 2, 2016 – Dec 3, 2016 NVMe

lore
Share & tools
lore

Andy Lutomirski

author
#0 lore raw

PATCH v3 1/1 nvme/pci: Log PCI_STATUS when the controller dies

When debugging nvme controller crashes, it's nice to know whether the controller died cleanly so that the failure is just reflected in CSTS, it died and put an error in PCI_STATUS, or whether it died so badly that it stopped responding to PCI configuration space reads.

I've seen a failure that gives 0xffff in PCI_STATUS on a Samsung "SM951 NVMe SAMSUNG 256GB" with firmware "BXW75D0Q".

Reviewed-by Christoph Hellwig <hch at lst.de>
Signed-off-by Andy Lutomirski <luto at kernel.org>
---
 drivers/nvme/host/pci.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)
drivers/nvme/host/pci.c +19 -3
index 5e52034ab010..628dc916ae9a 100644
@@ -1281,6 +1281,24 @@ static bool nvme_should_reset(struct nvme_dev *dev, u32 csts)
 	return true;
 }
 
+static void nvme_warn_reset(struct nvme_dev *dev, u32 csts)
+{
+	/* Read a config register to help see what died. */
+	u16 pci_status;
+	int result;
+
+	result = pci_read_config_word(to_pci_dev(dev->dev), PCI_STATUS,
+				      &pci_status);
+	if (result == PCIBIOS_SUCCESSFUL)
+		dev_warn(dev->dev,
+			 "controller is down; will reset: CSTS=0x%x, PCI_STATUS=0x%hx\n",
+			 csts, pci_status);
+	else
+		dev_warn(dev->dev,
+			 "controller is down; will reset: CSTS=0x%x, PCI_STATUS read failed (%d)\n",
+			 csts, result);
+}
+
 static void nvme_watchdog_timer(unsigned long data)
 {
 	struct nvme_dev *dev = (struct nvme_dev *)data;

@@ -1289,9 +1307,7 @@ static void nvme_watchdog_timer(unsigned long data)
 	/* Skip controllers under certain specific conditions. */
 	if (nvme_should_reset(dev, csts)) {
 		if (!nvme_reset(dev))
-			dev_warn(dev->dev,
-				"Failed status: 0x%x, reset controller.\n",
-				csts);
+			 nvme_warn_reset(dev, csts);
 		return;
 	}
Signature · 1 line
2.9.3
#1 lore raw

PATCH v3 1/1 nvme/pci: Log PCI_STATUS when the controller dies

Quoted 10 lines · Andy Lutomirski

When debugging nvme controller crashes, it's nice to know whether the controller died cleanly so that the failure is just reflected in CSTS, it died and put an error in PCI_STATUS, or whether it died so badly that it stopped responding to PCI configuration space reads.

I've seen a failure that gives 0xffff in PCI_STATUS on a Samsung "SM951 NVMe SAMSUNG 256GB" with firmware "BXW75D0Q".

Reviewed-by Christoph Hellwig <hch at lst.de>
Signed-off-by Andy Lutomirski <luto at kernel.org>

Totally fine with this, but just want to mention that even the MMIO read has caused problems when racing a pciehp hot plug event. A config read in this case is another opprotunity for a completion timeout, unless I can get Bjorn to apply the patch series disabling config access on surprise removed devices. Or maybe our nvme health check polling implementation is misguided.

Reviewed-by Keith Busch <keith.busch at intel.com>
#2 lore raw

PATCH v3 1/1 nvme/pci: Log PCI_STATUS when the controller dies

Quoted 12 lines · Keith Busch
Quoted 10 lines · Andy Lutomirski

When debugging nvme controller crashes, it's nice to know whether the controller died cleanly so that the failure is just reflected in CSTS, it died and put an error in PCI_STATUS, or whether it died so badly that it stopped responding to PCI configuration space reads.

I've seen a failure that gives 0xffff in PCI_STATUS on a Samsung "SM951 NVMe SAMSUNG 256GB" with firmware "BXW75D0Q".

Reviewed-by Christoph Hellwig <hch at lst.de>
Signed-off-by Andy Lutomirski <luto at kernel.org>
Quoted 7 lines

Totally fine with this, but just want to mention that even the MMIO read has caused problems when racing a pciehp hot plug event. A config read in this case is another opprotunity for a completion timeout, unless I can get Bjorn to apply the patch series disabling config access on surprise removed devices. Or maybe our nvme health check polling implementation is misguided.

I've been meaning to complain about the keepalive polling kicking my laptop out of idle every now and then :) But yes, I think this particular issue should be solved in the PCI layer. Also, the PCI layer should have a nice way to reset devices that go completely out to lunch, too, IMO.

#3 lore raw

PATCH v3 1/1 nvme/pci: Log PCI_STATUS when the controller dies

In reply to Andy Lutomirski
Quoted 7 lines · Andy Lutomirski

When debugging nvme controller crashes, it's nice to know whether the controller died cleanly so that the failure is just reflected in CSTS, it died and put an error in PCI_STATUS, or whether it died so badly that it stopped responding to PCI configuration space reads.

I've seen a failure that gives 0xffff in PCI_STATUS on a Samsung "SM951 NVMe SAMSUNG 256GB" with firmware "BXW75D0Q".

I have added this for 4.10. Note that I had to hand-apply, since your patch did not apply to the 4.10 branch. Additionally, you had a white space problem before the call to nvme_warn_reset() that I also fixed up.

Signature · 1 line
Jens Axboe