nvme/pci: Sync controller reset for AER slot_reset

PATCH New Keith Busch 6 participants 9 messages May 10, 2018 – May 12, 2018

Subsystems (2)

Stable branch NVMe

lore

Share & tools

lore

Keith Busch

author

2018-05-10 15:59 UTC #0 lore raw

PATCH 1/1 nvme/pci: Sync controller reset for AER slot_reset

AER handling expects a successful return from slot_reset means the driver made the device functional again. The nvme driver had been using an asynchronous reset to recover the device, so the device may still be initializing after control is returned to the AER handler. This creates problems for subsequent event handling, causing the initializion to fail.

This patch fixes that by syncing the controller reset before returning to the AER driver, and reporting the true state of the reset.

Link https://bugzilla.kernel.org/show_bug.cgi?id=199657

Reported-by Alex Gagniuc <[email protected]>

Cc Sinan Kaya <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---
 drivers/nvme/host/pci.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

drivers/nvme/host/pci.c +9 -2

index b542dce45927..2e221796257a 100644
@@ -2681,8 +2681,15 @@ static pci_ers_result_t nvme_slot_reset(struct pci_dev *pdev)
 
 	dev_info(dev->ctrl.device, "restart after slot reset\n");
 	pci_restore_state(pdev);
-	nvme_reset_ctrl(&dev->ctrl);
-	return PCI_ERS_RESULT_RECOVERED;
+	nvme_reset_ctrl_sync(&dev->ctrl);
+
+	switch (dev->ctrl.state) {
+	case NVME_CTRL_LIVE:
+	case NVME_CTRL_ADMIN_ONLY:
+		return PCI_ERS_RESULT_RECOVERED;
+	default:
+		return PCI_ERS_RESULT_DISCONNECT;
+	}
 }
 
 static void nvme_error_resume(struct pci_dev *pdev)

Signature · 1 line

2.14.3

Alex G.

2018-05-10 18:56 UTC #1 lore raw

PATCH 1/1 nvme/pci: Sync controller reset for AER slot_reset

Quoted 7 lines · Keith Busch

AER handling expects a successful return from slot_reset means the driver made the device functional again. The nvme driver had been using an asynchronous reset to recover the device, so the device may still be initializing after control is returned to the AER handler. This creates problems for subsequent event handling, causing the initializion to fail.

Quoted 5 lines

This patch fixes that by syncing the controller reset before returning to the AER driver, and reporting the true state of the reset.
Link https://bugzilla.kernel.org/show_bug.cgi?id=199657
Reported-by Alex Gagniuc <mr.nuke.me at gmail.com>

Tested-by Alex Gagniuc <mr.nuke.me at gmail.com>

Sponsored-by: DellEMC
You know I had to add that plug somewhere :p

Quoted 13 lines

Cc Sinan Kaya <okaya at codeaurora.org>
Cc Bjorn Helgaas <bhelgaas at google.com>
Cc <stable at vger.kernel.org>
Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/nvme/host/pci.c +0 -0
index b542dce45927..2e221796257a 100644

drivers/nvme/host/pci.c static pci_ers_result_t nvme_slot_reset(struct pci_dev *pdev) · line 2684

Quoted 6 lines

drivers/nvme/host/pci.c +1 -2

@@ -2681,8 +2681,15 @@ static pci_ers_result_t nvme_slot_reset(struct pci_dev *pdev)
 
 	dev_info(dev->ctrl.device, "restart after slot reset\n");
 	pci_restore_state(pdev);
-	nvme_reset_ctrl(&dev->ctrl);
-	return PCI_ERS_RESULT_RECOVERED;
+	nvme_reset_ctrl_sync(&dev->ctrl);

This does wonders when nvme_reset_ctrl_sync() returns in a timely manner. I was also able to get the nvme drive in a state where nvme_reset_ctrl_sync() does not return. Then we end up with the device lock in report_slot_reset, which, as you may imagine, is not a great thing.

I think this step is a move in the better direction, but we still have problems.

Alex

Quoted 11 lines

drivers/nvme/host/pci.c +7 -0

+	switch (dev->ctrl.state) {
+	case NVME_CTRL_LIVE:
+	case NVME_CTRL_ADMIN_ONLY:
+		return PCI_ERS_RESULT_RECOVERED;
+	default:
+		return PCI_ERS_RESULT_DISCONNECT;
+	}
 }
 
 static void nvme_error_resume(struct pci_dev *pdev)

Keith Busch

2018-05-10 19:12 UTC #2 lore raw

PATCH 1/1 nvme/pci: Sync controller reset for AER slot_reset

Quoted 12 lines · Alex G.

drivers/nvme/host/pci.c static pci_ers_result_t nvme_slot_reset(struct pci_dev *pdev) · line 2684
Quoted 7 lines
drivers/nvme/host/pci.c +1 -2
@@ -2681,8 +2681,15 @@ static pci_ers_result_t nvme_slot_reset(struct pci_dev *pdev)
 
 	dev_info(dev->ctrl.device, "restart after slot reset\n");
 	pci_restore_state(pdev);
-	nvme_reset_ctrl(&dev->ctrl);
-	return PCI_ERS_RESULT_RECOVERED;
+	nvme_reset_ctrl_sync(&dev->ctrl);
This does wonders when nvme_reset_ctrl_sync() returns in a timely manner. I was also able to get the nvme drive in a state where nvme_reset_ctrl_sync() does not return. Then we end up with the device lock in report_slot_reset, which, as you may imagine, is not a great thing.

It never returns? That shouldn't happen. There are cases where it may take a very long time, depending on what the controller reports in CAP.TO. The only other case it may stall is if the controller never responds to the initialization admin commands, but that should delay by 60 seconds under default parameters.

Alex G.

2018-05-10 19:20 UTC #3 lore raw

PATCH 1/1 nvme/pci: Sync controller reset for AER slot_reset

Quoted 14 lines · Keith Busch

Quoted 12 lines · Alex G.
drivers/nvme/host/pci.c static pci_ers_result_t nvme_slot_reset(struct pci_dev *pdev) · line 2684
Quoted 7 lines
drivers/nvme/host/pci.c +1 -2
@@ -2681,8 +2681,15 @@ static pci_ers_result_t nvme_slot_reset(struct pci_dev *pdev)
 
 	dev_info(dev->ctrl.device, "restart after slot reset\n");
 	pci_restore_state(pdev);
-	nvme_reset_ctrl(&dev->ctrl);
-	return PCI_ERS_RESULT_RECOVERED;
+	nvme_reset_ctrl_sync(&dev->ctrl);
This does wonders when nvme_reset_ctrl_sync() returns in a timely manner. I was also able to get the nvme drive in a state where nvme_reset_ctrl_sync() does not return. Then we end up with the device lock in report_slot_reset, which, as you may imagine, is not a great thing.

Quoted 5 lines

It never returns? That shouldn't happen. There are cases where it may take a very long time, depending on what the controller reports in CAP.TO. The only other case it may stall is if the controller never responds to the initialization admin commands, but that should delay by 60 seconds under default parameters.

Took 28 minutes before I gave up and rebooted the machine. Maybe I should have waited 30.
Even 60 seconds seems like a terribly long time to wait in AER. Simple stuff like block IO and 'nvme list' hangs in kernel space this entire time. I can raise a separate issue once I find a reliable way to repro.

Alex

Alex G.

2018-05-11 14:18 UTC #4 lore raw

PATCH 1/1 nvme/pci: Sync controller reset for AER slot_reset

Quoted 23 lines · Alex G.

Quoted 14 lines · Keith Busch
Quoted 12 lines · Alex G.
drivers/nvme/host/pci.c static pci_ers_result_t nvme_slot_reset(struct pci_dev *pdev) · line 2684
Quoted 7 lines
drivers/nvme/host/pci.c +1 -2
@@ -2681,8 +2681,15 @@ static pci_ers_result_t nvme_slot_reset(struct pci_dev *pdev)
 
  	dev_info(dev->ctrl.device, "restart after slot reset\n");
  	pci_restore_state(pdev);
-	nvme_reset_ctrl(&dev->ctrl);
-	return PCI_ERS_RESULT_RECOVERED;
+	nvme_reset_ctrl_sync(&dev->ctrl);
This does wonders when nvme_reset_ctrl_sync() returns in a timely manner. I was also able to get the nvme drive in a state where nvme_reset_ctrl_sync() does not return. Then we end up with the device lock in report_slot_reset, which, as you may imagine, is not a great thing.
Quoted 5 lines
It never returns? That shouldn't happen. There are cases where it may take a very long time, depending on what the controller reports in CAP.TO. The only other case it may stall is if the controller never responds to the initialization admin commands, but that should delay by 60 seconds under default parameters.

Quoted 5 lines

Took 28 minutes before I gave up and rebooted the machine. Maybe I should have waited 30.
Even 60 seconds seems like a terribly long time to wait in AER. Simple stuff like block IO and 'nvme list' hangs in kernel space this entire time. I can raise a separate issue once I find a reliable way to repro.

I've been playing some more with this. With recovery from a malformed TLP resulting from a bad MPS value in the switch upstream port we are likely to not return in a timely manner (a few minutes to infinity). This happens with less than 100% consistency. I am in a state of disbelief, since this makes little sense to me.
Log excerpt below.
Do you think it's a separate issue, or related?

Alex

[   16.828656] IPv6: ADDRCONF(NETDEV_CHANGE): eno1: link becomes ready
[ 1605.101288] megaraid_sas 0000:86:00.0: invalid short VPD tag 00 at 
offset 1
[ 1621.514702] pcieport 0000:ae:00.0: AER: Multiple Uncorrected (Fatal) 
error received: id=b020
[ 1621.696135] pcieport 0000:b0:04.0: PCIe Bus Error: 
severity=Uncorrected (Fatal), type=Transaction Layer, id=b020(Receiver ID)
[ 1621.707429] pcieport 0000:b0:04.0:   device [10b5:9733] error 
status/mask=00440000/01a10000
[ 1621.715780] pcieport 0000:b0:04.0:    [18] Malformed TLP          (First)
[ 1621.722568] pcieport 0000:b0:04.0:    [22] Uncorrectable Internal Error
[ 1621.729192] pcieport 0000:b0:04.0:   TLP Header: 60000040 b10000ff 
00000004 4f4bb000
[ 1621.736942] pcieport 0000:b0:04.0: broadcast error_detected message
[ 1621.736945] nvme 0000:b1:00.0: HACK: report_error_detected: Preparing 
to lock
[ 1621.736946] nvme 0000:b1:00.0: HACK: report_error_detected: locked 
and ready
[ 1621.736948] nvme nvme2: frozen state error detected, reset controller
[ 1625.649049] INFO: NMI handler (ghes_notify_nmi) took too long to run: 
175.406 msecs
[ 1634.244302] nvme 0000:b1:00.0: HACK: report_error_detected: Unlocked 
and DONE
[ 1635.290798] pcieport 0000:b0:04.0: downstream link has been reset
[ 1635.290804] pcieport 0000:b0:04.0: broadcast slot_reset message
[ 1635.290811] nvme 0000:b1:00.0: HACK: report_slot_reset: Preparing to lock
[ 1635.290815] nvme 0000:b1:00.0: HACK: report_slot_reset: locked and ready
[ 1635.290823] nvme nvme2: restart after slot reset

Ming Lei

2018-05-12 09:27 UTC #5 lore raw

PATCH 1/1 nvme/pci: Sync controller reset for AER slot_reset

In reply to Alex G.

Quoted 46 lines · Alex G.

Quoted 7 lines · Keith Busch
AER handling expects a successful return from slot_reset means the driver made the device functional again. The nvme driver had been using an asynchronous reset to recover the device, so the device may still be initializing after control is returned to the AER handler. This creates problems for subsequent event handling, causing the initializion to fail.
Quoted 5 lines
This patch fixes that by syncing the controller reset before returning to the AER driver, and reporting the true state of the reset.
Link https://bugzilla.kernel.org/show_bug.cgi?id=199657
Reported-by Alex Gagniuc <mr.nuke.me at gmail.com>
Tested-by Alex Gagniuc <mr.nuke.me at gmail.com>
Sponsored-by: DellEMC
You know I had to add that plug somewhere :p
Quoted 13 lines
Cc Sinan Kaya <okaya at codeaurora.org>
Cc Bjorn Helgaas <bhelgaas at google.com>
Cc <stable at vger.kernel.org>
Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/nvme/host/pci.c +0 -0
index b542dce45927..2e221796257a 100644
drivers/nvme/host/pci.c static pci_ers_result_t nvme_slot_reset(struct pci_dev *pdev) · line 2684
Quoted 6 lines
drivers/nvme/host/pci.c +1 -2
@@ -2681,8 +2681,15 @@ static pci_ers_result_t nvme_slot_reset(struct pci_dev *pdev)
 
      dev_info(dev->ctrl.device, "restart after slot reset\n");
      pci_restore_state(pdev);
-     nvme_reset_ctrl(&dev->ctrl);
-     return PCI_ERS_RESULT_RECOVERED;
+     nvme_reset_ctrl_sync(&dev->ctrl);
This does wonders when nvme_reset_ctrl_sync() returns in a timely manner. I was also able to get the nvme drive in a state where nvme_reset_ctrl_sync() does not return. Then we end up with the device lock in report_slot_reset, which, as you may imagine, is not a great thing.

Quoted 2 lines

I think this step is a move in the better direction, but we still have problems.

If IOs from nvme_reset_work() times out, nvme_reset_ctrl_sync() may never return, but not sure if that is your case.

You may find where it hangs via 'ps -ax | grep D' and cat /proc/$PID/stack.

Signature · 1 line

Ming Lei

Christoph Hellwig

maintainer

2018-05-11 06:34 UTC #6 lore raw

PATCH 1/1 nvme/pci: Sync controller reset for AER slot_reset

In reply to Keith Busch

Quoted 9 lines · Keith Busch

AER handling expects a successful return from slot_reset means the driver made the device functional again. The nvme driver had been using an asynchronous reset to recover the device, so the device may still be initializing after control is returned to the AER handler. This creates problems for subsequent event handling, causing the initializion to fail.
This patch fixes that by syncing the controller reset before returning to the AER driver, and reporting the true state of the reset.

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

Martin K. Petersen

2018-05-11 20:54 UTC #7 lore raw

PATCH 1/1 nvme/pci: Sync controller reset for AER slot_reset

In reply to Keith Busch

Keith,

Quoted 9 lines

AER handling expects a successful return from slot_reset means the driver made the device functional again. The nvme driver had been using an asynchronous reset to recover the device, so the device may still be initializing after control is returned to the AER handler. This creates problems for subsequent event handling, causing the initializion to fail.
This patch fixes that by syncing the controller reset before returning to the AER driver, and reporting the true state of the reset.

LGTM.

Reviewed-by Martin K. Petersen <martin.petersen at oracle.com>

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Keith Busch

Applied

2018-05-11 21:07 UTC #8 lore raw

PATCH 1/1 nvme/pci: Sync controller reset for AER slot_reset

Thanks, applied to nvme-4.18.