Skip to contentSkip to conversation

nvme-rdma: stop keep_alive before nvme_uninit_ctrl

PATCH New David Milburn 5 participants 11 messages Jun 29, 2017 – Jul 7, 2017

Subsystems (3)
NVMe NVMe target NVMe FC transport
lore
Share & tools
lore

David Milburn

author
#0 lore raw

PATCH 1/1 nvme-rdma: stop keep_alive before nvme_uninit_ctrl

Its possible for nvme_keep_alive_work() to hit an error condition after the nvme_uninit_ctrl() in __nvme_rdma_remove_ctrl(). This can lead to usage of NULL pointer in "dev_err(ctrl->device..." since device has been destroyed by nvme_uninit_ctrl().

This has been seen during continous loop of (discover, connect, IO, disconnect).

Reported-by Yi Zhang <yizhan at redhat.com>
Tested-by Yi Zhang <yizhan at redhat.com>
Signed-off-by David Milburn <dmilburn at redhat.com>
---
 drivers/nvme/host/rdma.c | 1 +
 1 file changed, 1 insertion(+)
drivers/nvme/host/rdma.c +1 -0
index 24397d3..1f73ace 100644
@@ -1683,6 +1683,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl)
 
 static void __nvme_rdma_remove_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
 {
+	nvme_stop_keep_alive(&ctrl->ctrl);
 	nvme_uninit_ctrl(&ctrl->ctrl);
 	if (shutdown)
 		nvme_rdma_shutdown_ctrl(ctrl);
Signature · 1 line
1.8.3.1
#1 lore raw

PATCH 1/1 nvme-rdma: stop keep_alive before nvme_uninit_ctrl

Quoted 7 lines · David Milburn

Its possible for nvme_keep_alive_work() to hit an error condition after the nvme_uninit_ctrl() in __nvme_rdma_remove_ctrl(). This can lead to usage of NULL pointer in "dev_err(ctrl->device..." since device has been destroyed by nvme_uninit_ctrl().

This has been seen during continous loop of (discover, connect, IO, disconnect).

Why can't we stop the keepalive work in nvme core and get rid of the stopping in fc.c as well?

Signature · 6 lines
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
author
#2 lore raw

PATCH 1/1 nvme-rdma: stop keep_alive before nvme_uninit_ctrl

Hi Johannes,

Quoted 6 lines · Johannes Thumshirn
Quoted 5 lines · David Milburn

Its possible for nvme_keep_alive_work() to hit an error condition after the nvme_uninit_ctrl() in __nvme_rdma_remove_ctrl(). This can lead to usage of NULL pointer in "dev_err(ctrl->device..." since device has been destroyed by nvme_uninit_ctrl().

Quoted 6 lines
Quoted 2 lines

This has been seen during continous loop of (discover, connect, IO, disconnect).

Why can't we stop the keepalive work in nvme core and get rid of the stopping in fc.c as well?

Do you mean checking ctrl->device at the beginning of nvme_keep_alive_work()? And if device has been removed, nvme_stop_keep_alive() and return?

Though still looks like nvme_rdma_error_recovery_work() will want to be able to stop keep_alive, and some error handling in fc.c.

Thanks,
David

#3 lore raw

PATCH 1/1 nvme-rdma: stop keep_alive before nvme_uninit_ctrl

Quoted 16 lines · David Milburn

Hi Johannes,

Quoted 6 lines · Johannes Thumshirn
Quoted 5 lines · David Milburn

Its possible for nvme_keep_alive_work() to hit an error condition after the nvme_uninit_ctrl() in __nvme_rdma_remove_ctrl(). This can lead to usage of NULL pointer in "dev_err(ctrl->device..." since device has been destroyed by nvme_uninit_ctrl().

Quoted 7 lines
Quoted 2 lines

This has been seen during continous loop of (discover, connect, IO, disconnect).

Why can't we stop the keepalive work in nvme core and get rid of the stopping
in fc.c as well?

Quoted 7 lines

Do you mean checking ctrl->device at the beginning of nvme_keep_alive_work()? And if device has been removed, nvme_stop_keep_alive() and return?

Though still looks like nvme_rdma_error_recovery_work() will want to be able to stop keep_alive, and some error handling in fc.c.

So calling nvme_stop_keep_alive() prior to nvme_uninit_ctrl() wasn't sufficient to avoid the condition ?

It looks like rdma may have missed this on the nvme_rdma_del_ctrl_work() call, it calls nvme_uninit_ctrl() then calls nvme_stop_keep_alive().

Granted, there's a race between the stop and further teardown, but that should be manageable in the core nvme code around the workaround (e.g. called to stop while its executing, work start but see cancelled, etc).

-- james

maintainer
#4 lore raw

PATCH 1/1 nvme-rdma: stop keep_alive before nvme_uninit_ctrl

In reply to David Milburn
Quoted 7 lines
drivers/nvme/host/rdma.c +0 -0
index 24397d3..1f73ace 100644

drivers/nvme/host/rdma.c static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl) · line 1689

Quoted 7 lines
drivers/nvme/host/rdma.c +1 -0
@@ -1683,6 +1683,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl)
 
  static void __nvme_rdma_remove_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
  {
+	nvme_stop_keep_alive(&ctrl->ctrl);
  	nvme_uninit_ctrl(&ctrl->ctrl);
  	if (shutdown)
  		nvme_rdma_shutdown_ctrl(ctrl);

This is not the only case where we can dereference the ctrl->device after nvme_uninit_ctrl. We cancel all inflight I/O afterwards too.

The problem is that we need the first part of nvme_uninit_ctrl before we start ctrl removal (flush async and scan work and remove namespaces) but the second part should only happen when we are done with the teardown.

I think we want to split nvme_uninit_ctrl into two (and for symmetry split nvme_init_ctrl too), one part to stop ctrl inflight works and one to destroy the device.

How about the patch below?

--
[PATCH] nvme: split nvme_uninit_ctrl into stop and uninit

Usually before we teardown the controller we want to:
1. complete/cancel any ctrl inflight works
2. remove ctrl namespaces

but we do not want to destroy the controller device as we might use it for logging during the teardown stage.

This patch adds nvme_start_ctrl() which queues inflight controller works (aen, ns scan, and keep-alive if kato is set) and nvme_stop_ctrl() which cancels them and remove controller namespaces (also expose __nvme_stop_ctrl for cases where we don't want to remove namespaces i.e. resets and fabrics error recovery.

We replace all the transports code that does:

if (ctrl->ctrl.queue_count > 1) {
nvme_queue_scan(ctrl)
nvme_queue_async_events(ctrl)
}

and:
nvme_start_keep_alive(ctrl)

to:
nvme_start_ctrl()

we move nvme_uninit_ctrl to ->free_ctrl handler which makes better sense as it was initialized in probe (or crete_ctrl). and replace it with nvme_stop_ctrl (or __nvme_stop_ctrl where appropriate).

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
- This is on top of the move of queue_count to struct nvme_ctrl

  drivers/nvme/host/core.c   | 25 ++++++++++++++++++++++++-
  drivers/nvme/host/fc.c     | 15 ++++-----------
  drivers/nvme/host/nvme.h   |  3 +++
  drivers/nvme/host/pci.c    | 15 +++------------
  drivers/nvme/host/rdma.c   | 29 +++++++++--------------------
  drivers/nvme/target/loop.c | 21 ++++++++-------------
  6 files changed, 51 insertions(+), 57 deletions(-)
drivers/nvme/host/core.c +0 -0
index d70df1d0072d..58e15fcb0d01 100644
@@ -2591,12 +2591,35 @@ static void nvme_release_instance(struct

nvme_ctrl *ctrl)

         spin_unlock(&dev_list_lock);

}

-void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
+void __nvme_stop_ctrl(struct nvme_ctrl *ctrl)
{

+       nvme_stop_keep_alive(ctrl);
         flush_work(&ctrl->async_event_work);
         flush_work(&ctrl->scan_work);

+}
+EXPORT_SYMBOL_GPL(__nvme_stop_ctrl);
+
+void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
+{

+       __nvme_stop_ctrl(ctrl);
         nvme_remove_namespaces(ctrl);

+}
+EXPORT_SYMBOL_GPL(nvme_stop_ctrl);
+
+void nvme_start_ctrl(struct nvme_ctrl *ctrl)
+{
+ if (ctrl->kato)
+ nvme_start_keep_alive(ctrl);

+ if (ctrl->queue_count > 1) {
+ nvme_queue_scan(ctrl);
+ nvme_queue_async_events(ctrl);
+ }
+}
+EXPORT_SYMBOL_GPL(nvme_start_ctrl);
+
+void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)

+{
         device_destroy(nvme_class, MKDEV(nvme_char_major, ctrl->instance));

         spin_lock(&dev_list_lock);
drivers/nvme/host/fc.c +0 -3
index ffe0589969bd..979cbd456350 100644
@@ -2231,7 +2231,6 @@ nvme_fc_create_io_queues(struct nvme_fc_ctrl *ctrl)
  out_delete_hw_queues:
         nvme_fc_delete_hw_io_queues(ctrl);
  out_cleanup_blk_queue:
-       nvme_stop_keep_alive(&ctrl->ctrl);
         blk_cleanup_queue(ctrl->ctrl.connect_q);
  out_free_tag_set:
         blk_mq_free_tag_set(&ctrl->tag_set);

@@ -2364,8 +2363,6 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
                 goto out_disconnect_admin_queue;
         }
 
-       nvme_start_keep_alive(&ctrl->ctrl);
-
         /* FC-NVME supports normal SGL Data Block Descriptors */
 
         if (opts->queue_size > ctrl->ctrl.maxcmd) {

@@ -2399,17 +2396,14 @@ nvme_fc_create_association(struct nvme_fc_ctrl

*ctrl)

         ctrl->ctrl.nr_reconnects = 0;
-       if (ctrl->ctrl.queue_count > 1) {
+       nvme_start_ctrl(&ctrl->ctrl);
+       if (ctrl->ctrl.queue_count > 1)
                 nvme_start_queues(&ctrl->ctrl);

- nvme_queue_scan(&ctrl->ctrl);
- nvme_queue_async_events(&ctrl->ctrl);
- }

return 0; /* Success */

out_term_aen_ops:

         nvme_fc_term_aen_ops(ctrl);
-       nvme_stop_keep_alive(&ctrl->ctrl);
  out_disconnect_admin_queue:
         /* send a Disconnect(association) LS to fc-nvme target */
         nvme_fc_xmt_disconnect_assoc(ctrl);
@@ -2432,8 +2426,6 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
  {
         unsigned long flags;

- nvme_stop_keep_alive(&ctrl->ctrl);
-

         spin_lock_irqsave(&ctrl->lock, flags);
         ctrl->flags |= FCCTRL_TERMIO;
         ctrl->iocnt = 0;

@@ -2515,7 +2507,7 @@ nvme_fc_delete_ctrl_work(struct work_struct *work)

         cancel_work_sync(&ctrl->ctrl.reset_work);
         cancel_delayed_work_sync(&ctrl->connect_work);

-
+ nvme_stop_ctrl(&ctrl->ctrl);
/*
* kill the association on the link side. this will block
* waiting for io to terminate
@@ -2610,6 +2602,7 @@ nvme_fc_reset_ctrl_work(struct work_struct *work)

                 container_of(work, struct nvme_fc_ctrl, ctrl.reset_work);
         int ret;
+       __nvme_stop_ctrl(&ctrl->ctrl);
         /* will block will waiting for io to terminate */
         nvme_fc_delete_association(ctrl);
drivers/nvme/host/nvme.h +3 -0
index e0b83311d5de..232929d26c7f 100644
@@ -280,6 +280,9 @@ int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl);
  int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
                 const struct nvme_ctrl_ops *ops, unsigned long quirks);
  void nvme_uninit_ctrl(struct nvme_ctrl *ctrl);
+void nvme_start_ctrl(struct nvme_ctrl *ctrl);
+void __nvme_stop_ctrl(struct nvme_ctrl *ctrl);
+void nvme_stop_ctrl(struct nvme_ctrl *ctrl);
  void nvme_put_ctrl(struct nvme_ctrl *ctrl);
  int nvme_init_identify(struct nvme_ctrl *ctrl);
drivers/nvme/host/pci.c +1 -2
index b995bf0e0e75..762cf2d1e953 100644
@@ -2048,6 +2048,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
  {
         struct nvme_dev *dev = to_nvme_dev(ctrl);
 
+       nvme_uninit_ctrl(ctrl);
         nvme_dbbuf_dma_free(dev);
         put_device(dev->dev);
         if (dev->tagset.tags)

@@ -2129,15 +2130,6 @@ static void nvme_reset_work(struct work_struct *work)
                 goto out;
 
         /*
-        * A controller that can not execute IO typically requires user
-        * intervention to correct. For such degraded controllers, the

driver
- * should not submit commands the user did not request, so skip
- * registering for asynchronous event notification on this
condition.
- */
- if (dev->online_queues > 1)
- nvme_queue_async_events(&dev->ctrl);
-
- /*
* Keep the controller around but remove all namespaces if we
don't have
* any working I/O queue.
*/
@@ -2157,8 +2149,7 @@ static void nvme_reset_work(struct work_struct *work)

                 goto out;
         }

- if (dev->online_queues > 1)
- nvme_queue_scan(&dev->ctrl);

+       nvme_start_ctrl(&dev->ctrl);
         return;

out:
@@ -2335,7 +2326,7 @@ static void nvme_remove(struct pci_dev *pdev)

         }

         flush_work(&dev->ctrl.reset_work);
-       nvme_uninit_ctrl(&dev->ctrl);
+       nvme_stop_ctrl(&dev->ctrl);
         nvme_dev_disable(dev, true);
         nvme_free_host_mem(dev);
         nvme_dev_remove_admin(dev);
drivers/nvme/host/rdma.c +2 -0
index cfb22531fc16..6f21b27fbf05 100644
@@ -664,6 +664,8 @@ static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)
         if (list_empty(&ctrl->list))
                 goto free_ctrl;
 
+       nvme_uninit_ctrl(nctrl);
+
         mutex_lock(&nvme_rdma_ctrl_mutex);
         list_del(&ctrl->list);
         mutex_unlock(&nvme_rdma_ctrl_mutex);

@@ -731,8 +733,6 @@ static void nvme_rdma_reconnect_ctrl_work(struct

work_struct *work)

         if (ret)
                 goto requeue;

- nvme_start_keep_alive(&ctrl->ctrl);
-

         if (ctrl->ctrl.queue_count > 1) {
                 ret = nvme_rdma_init_io_queues(ctrl);
                 if (ret)
@@ -750,10 +750,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct 
work_struct *work)
         WARN_ON_ONCE(!changed);
         ctrl->ctrl.nr_reconnects = 0;

- if (ctrl->ctrl.queue_count > 1) {
- nvme_queue_scan(&ctrl->ctrl);
- nvme_queue_async_events(&ctrl->ctrl);
- }
+ nvme_start_ctrl(&ctrl->ctrl);

         dev_info(ctrl->ctrl.device, "Successfully reconnected\n");

@@ -771,7 +768,7 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)

                         struct nvme_rdma_ctrl, err_work);
         int i;

- nvme_stop_keep_alive(&ctrl->ctrl);
+ __nvme_stop_ctrl(&ctrl->ctrl);

         for (i = 0; i < ctrl->ctrl.queue_count; i++)
                 clear_bit(NVME_RDMA_Q_LIVE, &ctrl->queues[i].flags);
@@ -1603,8 +1600,6 @@ static int nvme_rdma_configure_admin_queue(struct 
nvme_rdma_ctrl *ctrl)
         if (error)
                 goto out_cleanup_queue;

- nvme_start_keep_alive(&ctrl->ctrl);
-

         return 0;

out_cleanup_queue:
@@ -1622,7 +1617,6 @@ static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl)

static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl) {

-       nvme_stop_keep_alive(&ctrl->ctrl);
         cancel_work_sync(&ctrl->err_work);
         cancel_delayed_work_sync(&ctrl->reconnect_work);

@@ -1644,7 +1638,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl)

static void __nvme_rdma_remove_ctrl(struct nvme_rdma_ctrl *ctrl, bool
shutdown)
{
- nvme_uninit_ctrl(&ctrl->ctrl);

+       nvme_stop_ctrl(&ctrl->ctrl);
         if (shutdown)
                 nvme_rdma_shutdown_ctrl(ctrl);

@@ -1709,6 +1703,7 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work)

         int ret;
         bool changed;
+       __nvme_stop_ctrl(&ctrl->ctrl);
         nvme_rdma_shutdown_ctrl(ctrl);

         ret = nvme_rdma_configure_admin_queue(ctrl);
@@ -1738,11 +1733,9 @@ static void nvme_rdma_reset_ctrl_work(struct 
work_struct *work)
         changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
         WARN_ON_ONCE(!changed);
-       if (ctrl->ctrl.queue_count > 1) {
+       nvme_start_ctrl(&ctrl->ctrl);
+       if (ctrl->ctrl.queue_count > 1)
                 nvme_start_queues(&ctrl->ctrl);

- nvme_queue_scan(&ctrl->ctrl);
- nvme_queue_async_events(&ctrl->ctrl);
- }

         return;

@@ -1930,15 +1923,11 @@ static struct nvme_ctrl
*nvme_rdma_create_ctrl(struct device *dev,

         list_add_tail(&ctrl->list, &nvme_rdma_ctrl_list);
         mutex_unlock(&nvme_rdma_ctrl_mutex);

- if (ctrl->ctrl.queue_count > 1) {
- nvme_queue_scan(&ctrl->ctrl);
- nvme_queue_async_events(&ctrl->ctrl);
- }
+ nvme_start_ctrl(&ctrl->ctrl);

         return &ctrl->ctrl;

out_remove_admin_queue:

-       nvme_stop_keep_alive(&ctrl->ctrl);
         nvme_rdma_destroy_admin_queue(ctrl);
  out_kfree_queues:
         kfree(ctrl->queues);
drivers/nvme/target/loop.c +2 -0
index 3d51341e62ee..b28f8504cb2a 100644
@@ -287,6 +287,8 @@ static void nvme_loop_free_ctrl(struct nvme_ctrl *nctrl)
         if (list_empty(&ctrl->list))
                 goto free_ctrl;
 
+       nvme_uninit_ctrl(nctrl);
+
         mutex_lock(&nvme_loop_ctrl_mutex);
         list_del(&ctrl->list);
         mutex_unlock(&nvme_loop_ctrl_mutex);

@@ -407,8 +409,6 @@ static int nvme_loop_configure_admin_queue(struct

nvme_loop_ctrl *ctrl)

         if (error)
                 goto out_cleanup_queue;

- nvme_start_keep_alive(&ctrl->ctrl);
-

         return 0;

out_cleanup_queue:
@@ -422,8 +422,6 @@ static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)

static void nvme_loop_shutdown_ctrl(struct nvme_loop_ctrl *ctrl) {
- nvme_stop_keep_alive(&ctrl->ctrl);
-

         if (ctrl->ctrl.queue_count > 1) {
                 nvme_stop_queues(&ctrl->ctrl);
                 blk_mq_tagset_busy_iter(&ctrl->tag_set,

@@ -445,7 +443,7 @@ static void nvme_loop_del_ctrl_work(struct work_struct *work)
struct nvme_loop_ctrl *ctrl = container_of(work,
struct nvme_loop_ctrl, delete_work);

- nvme_uninit_ctrl(&ctrl->ctrl);

+       nvme_stop_ctrl(&ctrl->ctrl);
         nvme_loop_shutdown_ctrl(ctrl);
         nvme_put_ctrl(&ctrl->ctrl);
  }
@@ -494,6 +492,7 @@ static void nvme_loop_reset_ctrl_work(struct 
work_struct *work)
         bool changed;
         int ret;
+       __nvme_stop_ctrl(&ctrl->ctrl);
         nvme_loop_shutdown_ctrl(ctrl);

         ret = nvme_loop_configure_admin_queue(ctrl);
@@ -514,10 +513,9 @@ static void nvme_loop_reset_ctrl_work(struct 
work_struct *work)
         changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
         WARN_ON_ONCE(!changed);

- nvme_queue_scan(&ctrl->ctrl);
- nvme_queue_async_events(&ctrl->ctrl);
-

-       nvme_start_queues(&ctrl->ctrl);
+       nvme_start_ctrl(&ctrl->ctrl);
+       if (ctrl->ctrl.queue_count > 1)

+ nvme_start_queues(&ctrl->ctrl);

         return;

@@ -652,10 +650,7 @@ static struct nvme_ctrl
*nvme_loop_create_ctrl(struct device *dev,

         list_add_tail(&ctrl->list, &nvme_loop_ctrl_list);
         mutex_unlock(&nvme_loop_ctrl_mutex);

- if (opts->nr_io_queues) {
- nvme_queue_scan(&ctrl->ctrl);
- nvme_queue_async_events(&ctrl->ctrl);
- }
+ nvme_start_ctrl(&ctrl->ctrl);

         return &ctrl->ctrl;

--

maintainer
#5 lore raw

PATCH 1/1 nvme-rdma: stop keep_alive before nvme_uninit_ctrl

Quoted 1 line

How about the patch below?

Ping?

Quoted 425 lines
Signature · 45 lines
[PATCH] nvme: split nvme_uninit_ctrl into stop and uninit

Usually before we teardown the controller we want to:
1. complete/cancel any ctrl inflight works
2. remove ctrl namespaces

but we do not want to destroy the controller device as
we might use it for logging during the teardown stage.

This patch adds nvme_start_ctrl() which queues inflight
controller works (aen, ns scan, and keep-alive if kato is
set) and nvme_stop_ctrl() which cancels them and remove
controller namespaces (also expose __nvme_stop_ctrl for
cases where we don't want to remove namespaces i.e.
resets and fabrics error recovery.

We replace all the transports code that does:

if (ctrl->ctrl.queue_count > 1) {
         nvme_queue_scan(ctrl)
         nvme_queue_async_events(ctrl)
}

and:
         nvme_start_keep_alive(ctrl)

to:
         nvme_start_ctrl()

we move nvme_uninit_ctrl to ->free_ctrl handler
which makes better sense as it was initialized in
probe (or crete_ctrl). and replace it with
nvme_stop_ctrl (or __nvme_stop_ctrl where appropriate).

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
- This is on top of the move of queue_count to struct nvme_ctrl

  drivers/nvme/host/core.c   | 25 ++++++++++++++++++++++++-
  drivers/nvme/host/fc.c     | 15 ++++-----------
  drivers/nvme/host/nvme.h   |  3 +++
  drivers/nvme/host/pci.c    | 15 +++------------
  drivers/nvme/host/rdma.c   | 29 +++++++++--------------------
  drivers/nvme/target/loop.c | 21 ++++++++-------------
  6 files changed, 51 insertions(+), 57 deletions(-)
drivers/nvme/host/core.c +24 -1
index d70df1d0072d..58e15fcb0d01 100644
@@ -2591,12 +2591,35 @@ static void nvme_release_instance(struct 
 nvme_ctrl *ctrl)
         spin_unlock(&dev_list_lock);
  }
 
-void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
+void __nvme_stop_ctrl(struct nvme_ctrl *ctrl)
  {
+       nvme_stop_keep_alive(ctrl);
         flush_work(&ctrl->async_event_work);
         flush_work(&ctrl->scan_work);
+}
+EXPORT_SYMBOL_GPL(__nvme_stop_ctrl);
+
+void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
+{
+       __nvme_stop_ctrl(ctrl);
         nvme_remove_namespaces(ctrl);
+}
+EXPORT_SYMBOL_GPL(nvme_stop_ctrl);
+
+void nvme_start_ctrl(struct nvme_ctrl *ctrl)
+{
+       if (ctrl->kato)
+               nvme_start_keep_alive(ctrl);
 
+       if (ctrl->queue_count > 1) {
+               nvme_queue_scan(ctrl);
+               nvme_queue_async_events(ctrl);
+       }
+}
+EXPORT_SYMBOL_GPL(nvme_start_ctrl);
+
+void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
+{
         device_destroy(nvme_class, MKDEV(nvme_char_major, 
 ctrl->instance));

         spin_lock(&dev_list_lock);
drivers/nvme/host/fc.c +2 -8
index ffe0589969bd..979cbd456350 100644
@@ -2231,7 +2231,6 @@ nvme_fc_create_io_queues(struct nvme_fc_ctrl *ctrl)
  out_delete_hw_queues:
         nvme_fc_delete_hw_io_queues(ctrl);
  out_cleanup_blk_queue:
-       nvme_stop_keep_alive(&ctrl->ctrl);
         blk_cleanup_queue(ctrl->ctrl.connect_q);
  out_free_tag_set:
         blk_mq_free_tag_set(&ctrl->tag_set);

@@ -2364,8 +2363,6 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
                 goto out_disconnect_admin_queue;
         }
 
-       nvme_start_keep_alive(&ctrl->ctrl);
-
         /* FC-NVME supports normal SGL Data Block Descriptors */
 
         if (opts->queue_size > ctrl->ctrl.maxcmd) {

@@ -2399,17 +2396,14 @@ nvme_fc_create_association(struct nvme_fc_ctrl 
 *ctrl)
 
         ctrl->ctrl.nr_reconnects = 0;
 
-       if (ctrl->ctrl.queue_count > 1) {
+       nvme_start_ctrl(&ctrl->ctrl);
+       if (ctrl->ctrl.queue_count > 1)
                 nvme_start_queues(&ctrl->ctrl);
-               nvme_queue_scan(&ctrl->ctrl);
-               nvme_queue_async_events(&ctrl->ctrl);
-       }
 
         return 0;       /* Success */
 
  out_term_aen_ops:
         nvme_fc_term_aen_ops(ctrl);
-       nvme_stop_keep_alive(&ctrl->ctrl);
  out_disconnect_admin_queue:
         /* send a Disconnect(association) LS to fc-nvme target */

         nvme_fc_xmt_disconnect_assoc(ctrl);
drivers/nvme/host/fc.c +2 -3
@@ -2432,8 +2426,6 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
  {
         unsigned long flags;
 
-       nvme_stop_keep_alive(&ctrl->ctrl);
-
         spin_lock_irqsave(&ctrl->lock, flags);
         ctrl->flags |= FCCTRL_TERMIO;
         ctrl->iocnt = 0;

@@ -2515,7 +2507,7 @@ nvme_fc_delete_ctrl_work(struct work_struct *work)
 
         cancel_work_sync(&ctrl->ctrl.reset_work);
         cancel_delayed_work_sync(&ctrl->connect_work);
-
+       nvme_stop_ctrl(&ctrl->ctrl);
         /*
          * kill the association on the link side.  this will block
          * waiting for io to terminate

@@ -2610,6 +2602,7 @@ nvme_fc_reset_ctrl_work(struct work_struct *work)
                 container_of(work, struct nvme_fc_ctrl, ctrl.reset_work);
         int ret;
 
+       __nvme_stop_ctrl(&ctrl->ctrl);
         /* will block will waiting for io to terminate */
         nvme_fc_delete_association(ctrl);
drivers/nvme/host/nvme.h +3 -0
index e0b83311d5de..232929d26c7f 100644
@@ -280,6 +280,9 @@ int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl);
  int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
                 const struct nvme_ctrl_ops *ops, unsigned long quirks);
  void nvme_uninit_ctrl(struct nvme_ctrl *ctrl);
+void nvme_start_ctrl(struct nvme_ctrl *ctrl);
+void __nvme_stop_ctrl(struct nvme_ctrl *ctrl);
+void nvme_stop_ctrl(struct nvme_ctrl *ctrl);
  void nvme_put_ctrl(struct nvme_ctrl *ctrl);
  int nvme_init_identify(struct nvme_ctrl *ctrl);
drivers/nvme/host/pci.c +1 -0
index b995bf0e0e75..762cf2d1e953 100644
@@ -2048,6 +2048,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl 
 *ctrl)
  {
         struct nvme_dev *dev = to_nvme_dev(ctrl);
 
+       nvme_uninit_ctrl(ctrl);
         nvme_dbbuf_dma_free(dev);
         put_device(dev->dev);

         if (dev->tagset.tags)
drivers/nvme/host/pci.c +0 -2
@@ -2129,15 +2130,6 @@ static void nvme_reset_work(struct work_struct 
 *work)
                 goto out;
 
         /*
-        * A controller that can not execute IO typically requires user
-        * intervention to correct. For such degraded controllers, the

driver

drivers/nvme/host/pci.c +0 -2
-        * should not submit commands the user did not request, so skip
-        * registering for asynchronous event notification on this

condition.

drivers/nvme/host/pci.c +0 -5
-        */
-       if (dev->online_queues > 1)
-               nvme_queue_async_events(&dev->ctrl);
-
-       /*
          * Keep the controller around but remove all namespaces if we

don't have
* any working I/O queue.
*/

drivers/nvme/host/pci.c +2 -3
@@ -2157,8 +2149,7 @@ static void nvme_reset_work(struct work_struct *work)
                 goto out;
         }
 
-       if (dev->online_queues > 1)
-               nvme_queue_scan(&dev->ctrl);
+       nvme_start_ctrl(&dev->ctrl);
         return;
 
   out:

@@ -2335,7 +2326,7 @@ static void nvme_remove(struct pci_dev *pdev)
         }
 
         flush_work(&dev->ctrl.reset_work);
-       nvme_uninit_ctrl(&dev->ctrl);
+       nvme_stop_ctrl(&dev->ctrl);
         nvme_dev_disable(dev, true);
         nvme_free_host_mem(dev);
         nvme_dev_remove_admin(dev);
drivers/nvme/host/rdma.c +2 -0
index cfb22531fc16..6f21b27fbf05 100644
@@ -664,6 +664,8 @@ static void nvme_rdma_free_ctrl(struct nvme_ctrl 
 *nctrl)
         if (list_empty(&ctrl->list))
                 goto free_ctrl;
 
+       nvme_uninit_ctrl(nctrl);
+
         mutex_lock(&nvme_rdma_ctrl_mutex);
         list_del(&ctrl->list);

         mutex_unlock(&nvme_rdma_ctrl_mutex);
drivers/nvme/host/rdma.c +0 -2
@@ -731,8 +733,6 @@ static void nvme_rdma_reconnect_ctrl_work(struct 
 work_struct *work)
         if (ret)
                 goto requeue;
 
-       nvme_start_keep_alive(&ctrl->ctrl);
-
         if (ctrl->ctrl.queue_count > 1) {
                 ret = nvme_rdma_init_io_queues(ctrl);

if (ret)

drivers/nvme/host/rdma.c +2 -5
@@ -750,10 +750,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct 
 work_struct *work)
         WARN_ON_ONCE(!changed);
         ctrl->ctrl.nr_reconnects = 0;
 
-       if (ctrl->ctrl.queue_count > 1) {
-               nvme_queue_scan(&ctrl->ctrl);
-               nvme_queue_async_events(&ctrl->ctrl);
-       }
+       nvme_start_ctrl(&ctrl->ctrl);
 
         dev_info(ctrl->ctrl.device, "Successfully reconnected\n");

@@ -771,7 +768,7 @@ static void nvme_rdma_error_recovery_work(struct 
 work_struct *work)
                         struct nvme_rdma_ctrl, err_work);
         int i;
 
-       nvme_stop_keep_alive(&ctrl->ctrl);
+       __nvme_stop_ctrl(&ctrl->ctrl);
 
         for (i = 0; i < ctrl->ctrl.queue_count; i++)

                 clear_bit(NVME_RDMA_Q_LIVE, &ctrl->queues[i].flags);
drivers/nvme/host/rdma.c +0 -2
@@ -1603,8 +1600,6 @@ static int nvme_rdma_configure_admin_queue(struct 
 nvme_rdma_ctrl *ctrl)
         if (error)
                 goto out_cleanup_queue;
 
-       nvme_start_keep_alive(&ctrl->ctrl);
-
         return 0;

out_cleanup_queue:

drivers/nvme/host/rdma.c +1 -2
@@ -1622,7 +1617,6 @@ static int nvme_rdma_configure_admin_queue(struct 
 nvme_rdma_ctrl *ctrl)
 
  static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl)
  {
-       nvme_stop_keep_alive(&ctrl->ctrl);
         cancel_work_sync(&ctrl->err_work);
         cancel_delayed_work_sync(&ctrl->reconnect_work);

@@ -1644,7 +1638,7 @@ static void nvme_rdma_shutdown_ctrl(struct 
 nvme_rdma_ctrl *ctrl)
 
  static void __nvme_rdma_remove_ctrl(struct nvme_rdma_ctrl *ctrl, bool 
 shutdown)
  {
-       nvme_uninit_ctrl(&ctrl->ctrl);
+       nvme_stop_ctrl(&ctrl->ctrl);
         if (shutdown)

                 nvme_rdma_shutdown_ctrl(ctrl);
drivers/nvme/host/rdma.c +1 -0
@@ -1709,6 +1703,7 @@ static void nvme_rdma_reset_ctrl_work(struct 
 work_struct *work)
         int ret;
         bool changed;
 
+       __nvme_stop_ctrl(&ctrl->ctrl);
         nvme_rdma_shutdown_ctrl(ctrl);
 

         ret = nvme_rdma_configure_admin_queue(ctrl);
drivers/nvme/host/rdma.c +3 -9
@@ -1738,11 +1733,9 @@ static void nvme_rdma_reset_ctrl_work(struct 
 work_struct *work)
         changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
         WARN_ON_ONCE(!changed);
 
-       if (ctrl->ctrl.queue_count > 1) {
+       nvme_start_ctrl(&ctrl->ctrl);
+       if (ctrl->ctrl.queue_count > 1)
                 nvme_start_queues(&ctrl->ctrl);
-               nvme_queue_scan(&ctrl->ctrl);
-               nvme_queue_async_events(&ctrl->ctrl);
-       }
 
         return;

@@ -1930,15 +1923,11 @@ static struct nvme_ctrl 
 *nvme_rdma_create_ctrl(struct device *dev,
         list_add_tail(&ctrl->list, &nvme_rdma_ctrl_list);
         mutex_unlock(&nvme_rdma_ctrl_mutex);
 
-       if (ctrl->ctrl.queue_count > 1) {
-               nvme_queue_scan(&ctrl->ctrl);
-               nvme_queue_async_events(&ctrl->ctrl);
-       }
+       nvme_start_ctrl(&ctrl->ctrl);
 
         return &ctrl->ctrl;
 
  out_remove_admin_queue:
-       nvme_stop_keep_alive(&ctrl->ctrl);
         nvme_rdma_destroy_admin_queue(ctrl);
  out_kfree_queues:

         kfree(ctrl->queues);
drivers/nvme/target/loop.c +2 -0
index 3d51341e62ee..b28f8504cb2a 100644
@@ -287,6 +287,8 @@ static void nvme_loop_free_ctrl(struct nvme_ctrl 
 *nctrl)
         if (list_empty(&ctrl->list))
                 goto free_ctrl;
 
+       nvme_uninit_ctrl(nctrl);
+
         mutex_lock(&nvme_loop_ctrl_mutex);
         list_del(&ctrl->list);

         mutex_unlock(&nvme_loop_ctrl_mutex);
drivers/nvme/target/loop.c +0 -2
@@ -407,8 +409,6 @@ static int nvme_loop_configure_admin_queue(struct 
 nvme_loop_ctrl *ctrl)
         if (error)
                 goto out_cleanup_queue;
 
-       nvme_start_keep_alive(&ctrl->ctrl);
-
         return 0;

out_cleanup_queue:

drivers/nvme/target/loop.c +0 -2
@@ -422,8 +422,6 @@ static int nvme_loop_configure_admin_queue(struct 
 nvme_loop_ctrl *ctrl)
 
  static void nvme_loop_shutdown_ctrl(struct nvme_loop_ctrl *ctrl)
  {
-       nvme_stop_keep_alive(&ctrl->ctrl);
-
         if (ctrl->ctrl.queue_count > 1) {
                 nvme_stop_queues(&ctrl->ctrl);

                 blk_mq_tagset_busy_iter(&ctrl->tag_set,
drivers/nvme/target/loop.c +1 -1
@@ -445,7 +443,7 @@ static void nvme_loop_del_ctrl_work(struct 
 work_struct *work)
         struct nvme_loop_ctrl *ctrl = container_of(work,
                                 struct nvme_loop_ctrl, delete_work);
 
-       nvme_uninit_ctrl(&ctrl->ctrl);
+       nvme_stop_ctrl(&ctrl->ctrl);
         nvme_loop_shutdown_ctrl(ctrl);
         nvme_put_ctrl(&ctrl->ctrl);

  }
drivers/nvme/target/loop.c +1 -0
@@ -494,6 +492,7 @@ static void nvme_loop_reset_ctrl_work(struct 
 work_struct *work)
         bool changed;
         int ret;
 
+       __nvme_stop_ctrl(&ctrl->ctrl);
         nvme_loop_shutdown_ctrl(ctrl);
 

         ret = nvme_loop_configure_admin_queue(ctrl);
drivers/nvme/target/loop.c +4 -8
@@ -514,10 +513,9 @@ static void nvme_loop_reset_ctrl_work(struct 
 work_struct *work)
         changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
         WARN_ON_ONCE(!changed);
 
-       nvme_queue_scan(&ctrl->ctrl);
-       nvme_queue_async_events(&ctrl->ctrl);
-
-       nvme_start_queues(&ctrl->ctrl);
+       nvme_start_ctrl(&ctrl->ctrl);
+       if (ctrl->ctrl.queue_count > 1)
+               nvme_start_queues(&ctrl->ctrl);
 
         return;

@@ -652,10 +650,7 @@ static struct nvme_ctrl 
 *nvme_loop_create_ctrl(struct device *dev,
         list_add_tail(&ctrl->list, &nvme_loop_ctrl_list);
         mutex_unlock(&nvme_loop_ctrl_mutex);
 
-       if (opts->nr_io_queues) {
-               nvme_queue_scan(&ctrl->ctrl);
-               nvme_queue_async_events(&ctrl->ctrl);
-       }
+       nvme_start_ctrl(&ctrl->ctrl);
 
         return &ctrl->ctrl;
Signature · 1 line
--
maintainer
#6 lore raw

PATCH 1/1 nvme-rdma: stop keep_alive before nvme_uninit_ctrl

In reply to Sagi Grimberg

This idea looks fine - please send it out in a separate thread.

A few minor comments below:

Review

Quoted 8 lines
(quoted diff) +4 -1
-void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
+void __nvme_stop_ctrl(struct nvme_ctrl *ctrl)
 {
+       nvme_stop_keep_alive(ctrl);
        flush_work(&ctrl->async_event_work);
        flush_work(&ctrl->scan_work);
+}
+EXPORT_SYMBOL_GPL(__nvme_stop_ctrl);

I don't really like the __nvme_stop_ctrl name for something that is called all the time. I think this should be nvme_stop_ctrl.

Review

Quoted 7 lines
(quoted diff) +6 -0
+
+void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
+{
+       __nvme_stop_ctrl(ctrl);
        nvme_remove_namespaces(ctrl);
+}
+EXPORT_SYMBOL_GPL(nvme_stop_ctrl);

And then this becomes nvme_remove_ctrl?

Review

Quoted 3 lines
(quoted diff) +2 -0
+       nvme_start_ctrl(&ctrl->ctrl);
+       if (ctrl->ctrl.queue_count > 1)
                nvme_start_queues(&ctrl->ctrl);

Maybe add a nvme_restart_ctrl for this always duplicated sequence?

maintainer
#7 lore raw

PATCH 1/1 nvme-rdma: stop keep_alive before nvme_uninit_ctrl

Quoted 1 line

This idea looks fine - please send it out in a separate thread.

Will do

Quoted 13 lines

A few minor comments below:

Review

Quoted 8 lines
(quoted diff) +4 -1
-void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
+void __nvme_stop_ctrl(struct nvme_ctrl *ctrl)
  {
+       nvme_stop_keep_alive(ctrl);
         flush_work(&ctrl->async_event_work);
         flush_work(&ctrl->scan_work);
+}
+EXPORT_SYMBOL_GPL(__nvme_stop_ctrl);

I don't really like the __nvme_stop_ctrl name for something that is called all the time. I think this should be nvme_stop_ctrl.

Fine by me.

Quoted 9 lines

Review

Quoted 7 lines
(quoted diff) +6 -0
+
+void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
+{
+       __nvme_stop_ctrl(ctrl);
         nvme_remove_namespaces(ctrl);
+}
+EXPORT_SYMBOL_GPL(nvme_stop_ctrl);

And then this becomes nvme_remove_ctrl?

Not a good idea as its not actually removing the controller...

Maybe nvme_clear_ctrl?

Quoted 5 lines

Review

Quoted 3 lines
(quoted diff) +2 -0
+       nvme_start_ctrl(&ctrl->ctrl);
+       if (ctrl->ctrl.queue_count > 1)
                 nvme_start_queues(&ctrl->ctrl);

Maybe add a nvme_restart_ctrl for this always duplicated sequence?

sounds like an idea..

maintainer
#8 lore raw

PATCH 1/1 nvme-rdma: stop keep_alive before nvme_uninit_ctrl

Quoted 9 lines · Sagi Grimberg
Quoted 8 lines

Review

Quoted 6 lines
(quoted diff) +5 -0
+void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
+{
+       __nvme_stop_ctrl(ctrl);
         nvme_remove_namespaces(ctrl);
+}
+EXPORT_SYMBOL_GPL(nvme_stop_ctrl);

And then this becomes nvme_remove_ctrl?

Quoted 3 lines

Not a good idea as its not actually removing the controller...

Maybe nvme_clear_ctrl?

Or just call both the new nvme_stop_ctrl and nvme_remove_namespaces from each driver. It's a bit more code, but additional logic..

maintainer
#9 lore raw

PATCH 1/1 nvme-rdma: stop keep_alive before nvme_uninit_ctrl

Quoted 10 lines
Quoted 10 lines
Quoted 9 lines

Review

Quoted 7 lines
(quoted diff) +5 -0
+void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
+{
+       __nvme_stop_ctrl(ctrl);
          nvme_remove_namespaces(ctrl);
+}
+EXPORT_SYMBOL_GPL(nvme_stop_ctrl);

And then this becomes nvme_remove_ctrl?

Quoted 6 lines
Quoted 3 lines

Not a good idea as its not actually removing the controller...

Maybe nvme_clear_ctrl?

Or just call both the new nvme_stop_ctrl and nvme_remove_namespaces from each driver. It's a bit more code, but additional logic..

Yea... I hope we can get it centralize soon so it won't be duplicated.

btw, I think I can unconditionally call nvme_start_queues in nvme_start_ctrl, the extra unquiesce won't hurt. In fact, pci does it today. probe calls reset which unconditionally starts the queues...

author
#10 lore raw

PATCH 1/1 nvme-rdma: stop keep_alive before nvme_uninit_ctrl

In reply to Sagi Grimberg

Hi Sagi,

Quoted 24 lines · Sagi Grimberg
Quoted 13 lines
drivers/nvme/host/rdma.c +1 -0
index 24397d3..1f73ace 100644
@@ -1683,6 +1683,7 @@ static void nvme_rdma_shutdown_ctrl(struct 
 nvme_rdma_ctrl *ctrl)
  static void __nvme_rdma_remove_ctrl(struct nvme_rdma_ctrl *ctrl, 
 bool shutdown)
  {
+    nvme_stop_keep_alive(&ctrl->ctrl);
      nvme_uninit_ctrl(&ctrl->ctrl);
      if (shutdown)
Quoted 2 lines
          nvme_rdma_shutdown_ctrl(ctrl);

This is not the only case where we can dereference the ctrl->device after nvme_uninit_ctrl. We cancel all inflight I/O afterwards too.

The problem is that we need the first part of nvme_uninit_ctrl before we start ctrl removal (flush async and scan work and remove namespaces) but the second part should only happen when we are done with the teardown.

Quoted 5 lines

I think we want to split nvme_uninit_ctrl into two (and for symmetry split nvme_init_ctrl too), one part to stop ctrl inflight works and one to destroy the device.

How about the patch below?

Yi successfully re-tested the rdma changes.

Sorry for the delay,
David

Quoted 432 lines
Signature · 46 lines
-- 
[PATCH] nvme: split nvme_uninit_ctrl into stop and uninit

Usually before we teardown the controller we want to:
1. complete/cancel any ctrl inflight works
2. remove ctrl namespaces

but we do not want to destroy the controller device as
we might use it for logging during the teardown stage.

This patch adds nvme_start_ctrl() which queues inflight
controller works (aen, ns scan, and keep-alive if kato is
set) and nvme_stop_ctrl() which cancels them and remove
controller namespaces (also expose __nvme_stop_ctrl for
cases where we don't want to remove namespaces i.e.
resets and fabrics error recovery.

We replace all the transports code that does:

if (ctrl->ctrl.queue_count > 1) {
         nvme_queue_scan(ctrl)
         nvme_queue_async_events(ctrl)
}

and:
         nvme_start_keep_alive(ctrl)

to:
         nvme_start_ctrl()

we move nvme_uninit_ctrl to ->free_ctrl handler
which makes better sense as it was initialized in
probe (or crete_ctrl). and replace it with
nvme_stop_ctrl (or __nvme_stop_ctrl where appropriate).

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
- This is on top of the move of queue_count to struct nvme_ctrl

  drivers/nvme/host/core.c   | 25 ++++++++++++++++++++++++-
  drivers/nvme/host/fc.c     | 15 ++++-----------
  drivers/nvme/host/nvme.h   |  3 +++
  drivers/nvme/host/pci.c    | 15 +++------------
  drivers/nvme/host/rdma.c   | 29 +++++++++--------------------
  drivers/nvme/target/loop.c | 21 ++++++++-------------
  6 files changed, 51 insertions(+), 57 deletions(-)
drivers/nvme/host/core.c +24 -1
index d70df1d0072d..58e15fcb0d01 100644
@@ -2591,12 +2591,35 @@ static void nvme_release_instance(struct 
 nvme_ctrl *ctrl)
         spin_unlock(&dev_list_lock);
  }
 
-void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
+void __nvme_stop_ctrl(struct nvme_ctrl *ctrl)
  {
+       nvme_stop_keep_alive(ctrl);
         flush_work(&ctrl->async_event_work);
         flush_work(&ctrl->scan_work);
+}
+EXPORT_SYMBOL_GPL(__nvme_stop_ctrl);
+
+void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
+{
+       __nvme_stop_ctrl(ctrl);
         nvme_remove_namespaces(ctrl);
+}
+EXPORT_SYMBOL_GPL(nvme_stop_ctrl);
+
+void nvme_start_ctrl(struct nvme_ctrl *ctrl)
+{
+       if (ctrl->kato)
+               nvme_start_keep_alive(ctrl);
 
+       if (ctrl->queue_count > 1) {
+               nvme_queue_scan(ctrl);
+               nvme_queue_async_events(ctrl);
+       }
+}
+EXPORT_SYMBOL_GPL(nvme_start_ctrl);
+
+void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
+{
         device_destroy(nvme_class, MKDEV(nvme_char_major, 
 ctrl->instance));

         spin_lock(&dev_list_lock);
drivers/nvme/host/fc.c +2 -8
index ffe0589969bd..979cbd456350 100644
@@ -2231,7 +2231,6 @@ nvme_fc_create_io_queues(struct nvme_fc_ctrl *ctrl)
  out_delete_hw_queues:
         nvme_fc_delete_hw_io_queues(ctrl);
  out_cleanup_blk_queue:
-       nvme_stop_keep_alive(&ctrl->ctrl);
         blk_cleanup_queue(ctrl->ctrl.connect_q);
  out_free_tag_set:
         blk_mq_free_tag_set(&ctrl->tag_set);

@@ -2364,8 +2363,6 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
                 goto out_disconnect_admin_queue;
         }
 
-       nvme_start_keep_alive(&ctrl->ctrl);
-
         /* FC-NVME supports normal SGL Data Block Descriptors */
 
         if (opts->queue_size > ctrl->ctrl.maxcmd) {

@@ -2399,17 +2396,14 @@ nvme_fc_create_association(struct nvme_fc_ctrl 
 *ctrl)
 
         ctrl->ctrl.nr_reconnects = 0;
 
-       if (ctrl->ctrl.queue_count > 1) {
+       nvme_start_ctrl(&ctrl->ctrl);
+       if (ctrl->ctrl.queue_count > 1)
                 nvme_start_queues(&ctrl->ctrl);
-               nvme_queue_scan(&ctrl->ctrl);
-               nvme_queue_async_events(&ctrl->ctrl);
-       }
 
         return 0;       /* Success */
 
  out_term_aen_ops:
         nvme_fc_term_aen_ops(ctrl);
-       nvme_stop_keep_alive(&ctrl->ctrl);
  out_disconnect_admin_queue:
         /* send a Disconnect(association) LS to fc-nvme target */

         nvme_fc_xmt_disconnect_assoc(ctrl);
drivers/nvme/host/fc.c +2 -3
@@ -2432,8 +2426,6 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
  {
         unsigned long flags;
 
-       nvme_stop_keep_alive(&ctrl->ctrl);
-
         spin_lock_irqsave(&ctrl->lock, flags);
         ctrl->flags |= FCCTRL_TERMIO;
         ctrl->iocnt = 0;

@@ -2515,7 +2507,7 @@ nvme_fc_delete_ctrl_work(struct work_struct *work)
 
         cancel_work_sync(&ctrl->ctrl.reset_work);
         cancel_delayed_work_sync(&ctrl->connect_work);
-
+       nvme_stop_ctrl(&ctrl->ctrl);
         /*
          * kill the association on the link side.  this will block
          * waiting for io to terminate

@@ -2610,6 +2602,7 @@ nvme_fc_reset_ctrl_work(struct work_struct *work)
                 container_of(work, struct nvme_fc_ctrl, ctrl.reset_work);
         int ret;
 
+       __nvme_stop_ctrl(&ctrl->ctrl);
         /* will block will waiting for io to terminate */
         nvme_fc_delete_association(ctrl);
drivers/nvme/host/nvme.h +3 -0
index e0b83311d5de..232929d26c7f 100644
@@ -280,6 +280,9 @@ int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl);
  int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
                 const struct nvme_ctrl_ops *ops, unsigned long quirks);
  void nvme_uninit_ctrl(struct nvme_ctrl *ctrl);
+void nvme_start_ctrl(struct nvme_ctrl *ctrl);
+void __nvme_stop_ctrl(struct nvme_ctrl *ctrl);
+void nvme_stop_ctrl(struct nvme_ctrl *ctrl);
  void nvme_put_ctrl(struct nvme_ctrl *ctrl);
  int nvme_init_identify(struct nvme_ctrl *ctrl);
drivers/nvme/host/pci.c +1 -0
index b995bf0e0e75..762cf2d1e953 100644
@@ -2048,6 +2048,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl 
 *ctrl)
  {
         struct nvme_dev *dev = to_nvme_dev(ctrl);
 
+       nvme_uninit_ctrl(ctrl);
         nvme_dbbuf_dma_free(dev);
         put_device(dev->dev);

         if (dev->tagset.tags)
drivers/nvme/host/pci.c +0 -2
@@ -2129,15 +2130,6 @@ static void nvme_reset_work(struct work_struct 
 *work)
                 goto out;
 
         /*
-        * A controller that can not execute IO typically requires user
-        * intervention to correct. For such degraded controllers, the

driver

drivers/nvme/host/pci.c +0 -2
-        * should not submit commands the user did not request, so skip
-        * registering for asynchronous event notification on this

condition.

drivers/nvme/host/pci.c +0 -5
-        */
-       if (dev->online_queues > 1)
-               nvme_queue_async_events(&dev->ctrl);
-
-       /*
          * Keep the controller around but remove all namespaces if we

don't have
* any working I/O queue.
*/

drivers/nvme/host/pci.c +2 -3
@@ -2157,8 +2149,7 @@ static void nvme_reset_work(struct work_struct *work)
                 goto out;
         }
 
-       if (dev->online_queues > 1)
-               nvme_queue_scan(&dev->ctrl);
+       nvme_start_ctrl(&dev->ctrl);
         return;
 
   out:

@@ -2335,7 +2326,7 @@ static void nvme_remove(struct pci_dev *pdev)
         }
 
         flush_work(&dev->ctrl.reset_work);
-       nvme_uninit_ctrl(&dev->ctrl);
+       nvme_stop_ctrl(&dev->ctrl);
         nvme_dev_disable(dev, true);
         nvme_free_host_mem(dev);
         nvme_dev_remove_admin(dev);
drivers/nvme/host/rdma.c +2 -0
index cfb22531fc16..6f21b27fbf05 100644
@@ -664,6 +664,8 @@ static void nvme_rdma_free_ctrl(struct nvme_ctrl 
 *nctrl)
         if (list_empty(&ctrl->list))
                 goto free_ctrl;
 
+       nvme_uninit_ctrl(nctrl);
+
         mutex_lock(&nvme_rdma_ctrl_mutex);
         list_del(&ctrl->list);

         mutex_unlock(&nvme_rdma_ctrl_mutex);
drivers/nvme/host/rdma.c +0 -2
@@ -731,8 +733,6 @@ static void nvme_rdma_reconnect_ctrl_work(struct 
 work_struct *work)
         if (ret)
                 goto requeue;
 
-       nvme_start_keep_alive(&ctrl->ctrl);
-
         if (ctrl->ctrl.queue_count > 1) {
                 ret = nvme_rdma_init_io_queues(ctrl);

if (ret)

drivers/nvme/host/rdma.c +2 -5
@@ -750,10 +750,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct 
 work_struct *work)
         WARN_ON_ONCE(!changed);
         ctrl->ctrl.nr_reconnects = 0;
 
-       if (ctrl->ctrl.queue_count > 1) {
-               nvme_queue_scan(&ctrl->ctrl);
-               nvme_queue_async_events(&ctrl->ctrl);
-       }
+       nvme_start_ctrl(&ctrl->ctrl);
 
         dev_info(ctrl->ctrl.device, "Successfully reconnected\n");

@@ -771,7 +768,7 @@ static void nvme_rdma_error_recovery_work(struct 
 work_struct *work)
                         struct nvme_rdma_ctrl, err_work);
         int i;
 
-       nvme_stop_keep_alive(&ctrl->ctrl);
+       __nvme_stop_ctrl(&ctrl->ctrl);
 
         for (i = 0; i < ctrl->ctrl.queue_count; i++)

                 clear_bit(NVME_RDMA_Q_LIVE, &ctrl->queues[i].flags);
drivers/nvme/host/rdma.c +0 -2
@@ -1603,8 +1600,6 @@ static int nvme_rdma_configure_admin_queue(struct 
 nvme_rdma_ctrl *ctrl)
         if (error)
                 goto out_cleanup_queue;
 
-       nvme_start_keep_alive(&ctrl->ctrl);
-
         return 0;

out_cleanup_queue:

drivers/nvme/host/rdma.c +1 -2
@@ -1622,7 +1617,6 @@ static int nvme_rdma_configure_admin_queue(struct 
 nvme_rdma_ctrl *ctrl)
 
  static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl)
  {
-       nvme_stop_keep_alive(&ctrl->ctrl);
         cancel_work_sync(&ctrl->err_work);
         cancel_delayed_work_sync(&ctrl->reconnect_work);

@@ -1644,7 +1638,7 @@ static void nvme_rdma_shutdown_ctrl(struct 
 nvme_rdma_ctrl *ctrl)
 
  static void __nvme_rdma_remove_ctrl(struct nvme_rdma_ctrl *ctrl, bool 
 shutdown)
  {
-       nvme_uninit_ctrl(&ctrl->ctrl);
+       nvme_stop_ctrl(&ctrl->ctrl);
         if (shutdown)

                 nvme_rdma_shutdown_ctrl(ctrl);
drivers/nvme/host/rdma.c +1 -0
@@ -1709,6 +1703,7 @@ static void nvme_rdma_reset_ctrl_work(struct 
 work_struct *work)
         int ret;
         bool changed;
 
+       __nvme_stop_ctrl(&ctrl->ctrl);
         nvme_rdma_shutdown_ctrl(ctrl);
 

         ret = nvme_rdma_configure_admin_queue(ctrl);
drivers/nvme/host/rdma.c +3 -9
@@ -1738,11 +1733,9 @@ static void nvme_rdma_reset_ctrl_work(struct 
 work_struct *work)
         changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
         WARN_ON_ONCE(!changed);
 
-       if (ctrl->ctrl.queue_count > 1) {
+       nvme_start_ctrl(&ctrl->ctrl);
+       if (ctrl->ctrl.queue_count > 1)
                 nvme_start_queues(&ctrl->ctrl);
-               nvme_queue_scan(&ctrl->ctrl);
-               nvme_queue_async_events(&ctrl->ctrl);
-       }
 
         return;

@@ -1930,15 +1923,11 @@ static struct nvme_ctrl 
 *nvme_rdma_create_ctrl(struct device *dev,
         list_add_tail(&ctrl->list, &nvme_rdma_ctrl_list);
         mutex_unlock(&nvme_rdma_ctrl_mutex);
 
-       if (ctrl->ctrl.queue_count > 1) {
-               nvme_queue_scan(&ctrl->ctrl);
-               nvme_queue_async_events(&ctrl->ctrl);
-       }
+       nvme_start_ctrl(&ctrl->ctrl);
 
         return &ctrl->ctrl;
 
  out_remove_admin_queue:
-       nvme_stop_keep_alive(&ctrl->ctrl);
         nvme_rdma_destroy_admin_queue(ctrl);
  out_kfree_queues:

         kfree(ctrl->queues);
drivers/nvme/target/loop.c +2 -0
index 3d51341e62ee..b28f8504cb2a 100644
@@ -287,6 +287,8 @@ static void nvme_loop_free_ctrl(struct nvme_ctrl 
 *nctrl)
         if (list_empty(&ctrl->list))
                 goto free_ctrl;
 
+       nvme_uninit_ctrl(nctrl);
+
         mutex_lock(&nvme_loop_ctrl_mutex);
         list_del(&ctrl->list);

         mutex_unlock(&nvme_loop_ctrl_mutex);
drivers/nvme/target/loop.c +0 -2
@@ -407,8 +409,6 @@ static int nvme_loop_configure_admin_queue(struct 
 nvme_loop_ctrl *ctrl)
         if (error)
                 goto out_cleanup_queue;
 
-       nvme_start_keep_alive(&ctrl->ctrl);
-
         return 0;

out_cleanup_queue:

drivers/nvme/target/loop.c +0 -2
@@ -422,8 +422,6 @@ static int nvme_loop_configure_admin_queue(struct 
 nvme_loop_ctrl *ctrl)
 
  static void nvme_loop_shutdown_ctrl(struct nvme_loop_ctrl *ctrl)
  {
-       nvme_stop_keep_alive(&ctrl->ctrl);
-
         if (ctrl->ctrl.queue_count > 1) {
                 nvme_stop_queues(&ctrl->ctrl);

                 blk_mq_tagset_busy_iter(&ctrl->tag_set,
drivers/nvme/target/loop.c +1 -1
@@ -445,7 +443,7 @@ static void nvme_loop_del_ctrl_work(struct 
 work_struct *work)
         struct nvme_loop_ctrl *ctrl = container_of(work,
                                 struct nvme_loop_ctrl, delete_work);
 
-       nvme_uninit_ctrl(&ctrl->ctrl);
+       nvme_stop_ctrl(&ctrl->ctrl);
         nvme_loop_shutdown_ctrl(ctrl);
         nvme_put_ctrl(&ctrl->ctrl);

  }
drivers/nvme/target/loop.c +1 -0
@@ -494,6 +492,7 @@ static void nvme_loop_reset_ctrl_work(struct 
 work_struct *work)
         bool changed;
         int ret;
 
+       __nvme_stop_ctrl(&ctrl->ctrl);
         nvme_loop_shutdown_ctrl(ctrl);
 

         ret = nvme_loop_configure_admin_queue(ctrl);
drivers/nvme/target/loop.c +4 -8
@@ -514,10 +513,9 @@ static void nvme_loop_reset_ctrl_work(struct 
 work_struct *work)
         changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
         WARN_ON_ONCE(!changed);
 
-       nvme_queue_scan(&ctrl->ctrl);
-       nvme_queue_async_events(&ctrl->ctrl);
-
-       nvme_start_queues(&ctrl->ctrl);
+       nvme_start_ctrl(&ctrl->ctrl);
+       if (ctrl->ctrl.queue_count > 1)
+               nvme_start_queues(&ctrl->ctrl);
 
         return;

@@ -652,10 +650,7 @@ static struct nvme_ctrl 
 *nvme_loop_create_ctrl(struct device *dev,
         list_add_tail(&ctrl->list, &nvme_loop_ctrl_list);
         mutex_unlock(&nvme_loop_ctrl_mutex);
 
-       if (opts->nr_io_queues) {
-               nvme_queue_scan(&ctrl->ctrl);
-               nvme_queue_async_events(&ctrl->ctrl);
-       }
+       nvme_start_ctrl(&ctrl->ctrl);
 
         return &ctrl->ctrl;
Signature · 6 lines
-- 

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme