nvme/pci: Remove watchdog timer

The controller status polling was added to preemptively reset a failed controller. This early detection would allow commands that would normally timeout a chance for a retry, or find broken links when the platform didn't support hotplug.

This once-per-second MMIO read, however, created more problems than it solves. This often races with PCIe Hotplug events that required complicated syncing between work queues, frequently triggered PCIe Completion Timeout errors that also lead to fatal machine checks, and unnecessarily disrupts low power modes by running on idle controllers.

This patch removes the watchdog timer, and instead checks controller health only on an IO timeout when we have a reason to believe something is wrong. If the controller is failed, the driver will disable immediately and request scheduling a reset.

Suggested-by Andy Lutomirski <luto at amacapital.net>

Signed-off-by Keith Busch <keith.busch at intel.com>

Cc Christoph Hellwig <hch at lst.de>

---
 drivers/nvme/host/pci.c | 123 ++++++++++++++++++++++--------------------------
 1 file changed, 56 insertions(+), 67 deletions(-)

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

index d52701df..eedb9b4 100644
@@ -94,7 +94,6 @@ struct nvme_dev {
 	void __iomem *bar;
 	struct work_struct reset_work;
 	struct work_struct remove_work;
-	struct timer_list watchdog_timer;
 	struct mutex shutdown_lock;
 	bool subsystem;
 	void __iomem *cmb;

@@ -950,6 +949,51 @@ static void abort_endio(struct request *req, int error)
 	blk_mq_free_request(req);
 }
 
+static bool nvme_should_reset(struct nvme_dev *dev, u32 csts)
+{
+
+	/* If true, indicates loss of adapter communication, possibly by a
+	 * NVMe Subsystem reset.
+	 */
+	bool nssro = dev->subsystem && (csts & NVME_CSTS_NSSRO);
+
+	/* If there is a reset ongoing, we shouldn't reset again. */
+	if (work_busy(&dev->reset_work))
+		return false;
+
+	/* We shouldn't reset unless the controller is on fatal error state
+	 * _or_ if we lost the communication with it.
+	 */
+	if (!(csts & NVME_CSTS_CFS) && !nssro)
+		return false;
+
+	/* If PCI error recovery process is happening, we cannot reset or
+	 * the recovery mechanism will surely fail.
+	 */
+	if (pci_channel_offline(to_pci_dev(dev->dev)))
+		return false;
+
+	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->ctrl.device,
+			 "controller is down; will reset: CSTS=0x%x, PCI_STATUS=0x%hx\n",
+			 csts, pci_status);
+	else
+		dev_warn(dev->ctrl.device,
+			 "controller is down; will reset: CSTS=0x%x, PCI_STATUS read failed (%d)\n",
+			 csts, result);
+}
+
 static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 {
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);

Otherwise this looks good to me, but I'd like to defer it until we have the multiple reset issue sorted out, to avoid conflicts.

I am hoping to get this in this round since it would quiet a lot of errors I'm frequently pinged on: a non-posted request during a hotremove downstream a switch creates a lot of problems for hardware if you don't have DPC enabled.

One thing I'm just realizing, though, this will break subsystem reset. We were depending on the watchdog to detect that event, but now we require an IO timeout. It's easy enough to fix that from the host initiating the NSSR, but there's no way to notify other hosts connected to the same subsystem.

Quoted 4 lines · Keith Busch

I am hoping to get this in this round since it would quiet a lot of errors I'm frequently pinged on: a non-posted request during a hotremove downstream a switch creates a lot of problems for hardware if you don't have DPC enabled.

With this round do you mean 4.12-rc or 4.13?

Quoted 5 lines

One thing I'm just realizing, though, this will break subsystem reset. We were depending on the watchdog to detect that event, but now we require an IO timeout. It's easy enough to fix that from the host initiating the NSSR, but there's no way to notify other hosts connected to the same subsystem.

I don't think I've actually seen it trigger for that case due to the PCIe link going down earlier. But I'll need to double check as I actually have access to a dual ported drive at the moment, the machine setup is a bit quirky, though.

Quoted 7 lines · Christoph Hellwig

Quoted 4 lines · Keith Busch

I am hoping to get this in this round since it would quiet a lot of errors I'm frequently pinged on: a non-posted request during a hotremove downstream a switch creates a lot of problems for hardware if you don't have DPC enabled.

With this round do you mean 4.12-rc or 4.13?

Was hoping 4.12 simply only the problems from doing this are recently becomming more clear, but I guess no rush at this point.

static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved) {

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

 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);

@@ -957,6 +1001,17 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 	struct nvme_dev *dev = nvmeq->dev;

Quoted 10 lines

static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved) {

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

 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);

@@ -957,6 +1001,17 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 	struct nvme_dev *dev = nvmeq->dev;
 	struct request *abort_req;
 	struct nvme_command cmd;
+	u32 csts = readl(dev->bar + NVME_REG_CSTS);
+

drivers/nvme/host/pci.c static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved) · line 1011

Quoted 6 lines

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

+	/*
+	 * Reset immediately if the controller is failed
+	 */
+	if (nvme_should_reset(dev, readl(dev->bar + NVME_REG_CSTS))) {
+		nvme_warn_reset(dev, csts);
+		nvme_dev_disable(dev, false);

Why the nvme_dev_disable here? its going to happen in reset_work..