[12/12] nvme-pci: don't unbind the driver on reset failure

Commit Message

Christoph Hellwig Nov. 8, 2022, 3:02 p.m. UTC

Unbind a device driver when a reset fails is very unusual behavior.
Just disable the controller and leave it in dead state if we fail to
reset it.

Signed-off-by: Christoph Hellwig <[e-mail]>
---
 drivers/nvme/host/pci.c | 40 ++++++++++------------------------------
 1 file changed, 10 insertions(+), 30 deletions(-)

Comments

Sagi Grimberg Nov. 9, 2022, 3:15 a.m. UTC | #1

> Unbind a device driver when a reset fails is very unusual behavior.
> Just disable the controller and leave it in dead state if we fail to
> reset it.

OK, that is fine by me.

Reviewed-by: Sagi Grimberg <[e-mail]>

Keith Busch Nov. 9, 2022, 5:10 p.m. UTC | #2

On Tue, Nov 08, 2022 at 04:02:52PM +0100, Christoph Hellwig wrote:
> -static void nvme_remove_dead_ctrl_work(struct work_struct *work)
> -{
> -	struct nvme_dev *dev = container_of(work, struct nvme_dev, remove_work);
> -	struct pci_dev *pdev = to_pci_dev(dev->dev);
> -
> -	if (pci_get_drvdata(pdev))
> -		device_release_driver(&pdev->dev);
> -	nvme_put_ctrl(&dev->ctrl);
> +	/*
> +	 * Set state to deleting now to avoid blocking nvme_wait_reset(), which
> +	 * may be holding this pci_dev's device lock.
> +	 */
> +	dev_warn(dev->ctrl.device, "Disabling device after reset failure: %d\n",
> +		 result);
> +	nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
> +	nvme_dev_disable(dev, false);

I think the shutdown can be set to 'true' now that you're not
immediately unbinding the controller. That will unblock any queued IO
that would have been flushed out during the unbind.

Patch

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 1c8c70767cb8a..b1b659db682fa 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -130,7 +130,6 @@  struct nvme_dev {
 	u32 db_stride;
 	void __iomem *bar;
 	unsigned long bar_mapped_size;
-	struct work_struct remove_work;
 	struct mutex shutdown_lock;
 	bool subsystem;
 	u64 cmb_size;
@@ -2781,20 +2780,6 @@  static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
 	kfree(dev);
 }
 
-static void nvme_remove_dead_ctrl(struct nvme_dev *dev)
-{
-	/*
-	 * Set state to deleting now to avoid blocking nvme_wait_reset(), which
-	 * may be holding this pci_dev's device lock.
-	 */
-	nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
-	nvme_get_ctrl(&dev->ctrl);
-	nvme_dev_disable(dev, false);
-	nvme_mark_namespaces_dead(&dev->ctrl);
-	if (!queue_work(nvme_wq, &dev->remove_work))
-		nvme_put_ctrl(&dev->ctrl);
-}
-
 static void nvme_reset_work(struct work_struct *work)
 {
 	struct nvme_dev *dev =
@@ -2894,20 +2879,16 @@  static void nvme_reset_work(struct work_struct *work)
  out_unlock:
 	mutex_unlock(&dev->shutdown_lock);
  out:
-	if (result)
-		dev_warn(dev->ctrl.device,
-			 "Removing after probe failure status: %d\n", result);
-	nvme_remove_dead_ctrl(dev);
-}
-
-static void nvme_remove_dead_ctrl_work(struct work_struct *work)
-{
-	struct nvme_dev *dev = container_of(work, struct nvme_dev, remove_work);
-	struct pci_dev *pdev = to_pci_dev(dev->dev);
-
-	if (pci_get_drvdata(pdev))
-		device_release_driver(&pdev->dev);
-	nvme_put_ctrl(&dev->ctrl);
+	/*
+	 * Set state to deleting now to avoid blocking nvme_wait_reset(), which
+	 * may be holding this pci_dev's device lock.
+	 */
+	dev_warn(dev->ctrl.device, "Disabling device after reset failure: %d\n",
+		 result);
+	nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
+	nvme_dev_disable(dev, false);
+	nvme_mark_namespaces_dead(&dev->ctrl);
+	nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DEAD);
 }
 
 static int nvme_pci_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
@@ -3045,7 +3026,6 @@  static struct nvme_dev *nvme_pci_alloc_ctrl(struct pci_dev *pdev,
 	if (!dev)
 		return NULL;
 	INIT_WORK(&dev->ctrl.reset_work, nvme_reset_work);
-	INIT_WORK(&dev->remove_work, nvme_remove_dead_ctrl_work);
 	mutex_init(&dev->shutdown_lock);
 
 	dev->nr_write_queues = write_queues;